C-WCOW: Address review comments

Signed-off-by: Mahati Chamarthy <mahati.chamarthy@gmail.com>

Author: MahatiC

internal/gcs-sidecar/handlers.go

@@ -84,7 +84,6 @@ func (b *Bridge) createContainer(req *request) (err error) {
 		user := securitypolicy.IDName{
 			Name: spec.Process.User.Username,
 		}
-		log.G(ctx).Tracef("user test: %v", user)
 		_, _, _, err := b.hostState.securityPolicyEnforcer.EnforceCreateContainerPolicyV2(req.ctx, containerID, spec.Process.Args, spec.Process.Env, spec.Process.Cwd, spec.Mounts, user, nil)
 
 		if err != nil {
@@ -97,11 +96,12 @@ func (b *Bridge) createContainer(req *request) (err error) {
 		}
 		log.G(ctx).Tracef("Adding ContainerID: %v", containerID)
 		if err := b.hostState.AddContainer(req.ctx, containerID, c); err != nil {
-			log.G(ctx).Tracef("Container exists in the map!")
+			log.G(ctx).Tracef("Container exists in the map.")
+			return err
 		}
 		defer func(err error) {
 			if err != nil {
-				b.hostState.RemoveContainer(containerID)
+				b.hostState.RemoveContainer(ctx, containerID)
 			}
 		}(err)
 		// Write security policy, signed UVM reference and host AMD certificate to
@@ -242,9 +242,6 @@ func (b *Bridge) shutdownGraceful(req *request) (err error) {
 		return fmt.Errorf("failed to unmarshal shutdownGraceful: %w", err)
 	}
 
-	// TODO (kiashok/Mahati): Since gcs-sidecar can be used for all types of windows
-	// containers, it is important to check if we want to
-	// enforce policy or not.
 	err = b.hostState.securityPolicyEnforcer.EnforceShutdownContainerPolicy(req.ctx, r.ContainerID)
 	if err != nil {
 		return fmt.Errorf("rpcShudownGraceful operation not allowed: %w", err)
@@ -313,7 +310,7 @@ func (b *Bridge) executeProcess(req *request) (err error) {
 		c, err := b.hostState.GetCreatedContainer(req.ctx, containerID)
 		if err != nil {
 			log.G(req.ctx).Tracef("Container not found during exec: %v", containerID)
-			return errors.Wrapf(err, "containerID doesn't exist")
+			return fmt.Errorf("failed to get created container: %w", err)
 		}
 
 		// if this is an exec of Container command line, then it's already enforced
@@ -420,7 +417,7 @@ func (b *Bridge) signalProcess(req *request) (err error) {
 		containerID := r.ContainerID
 		c, err := b.hostState.GetCreatedContainer(req.ctx, containerID)
 		if err != nil {
-			return err
+			return fmt.Errorf("failed to get created container: %w", err)
 		}
 
 		p, err := c.GetProcess(r.ProcessID)
@@ -515,15 +512,13 @@ func (b *Bridge) deleteContainerState(req *request) (err error) {
 	if err := commonutils.UnmarshalJSONWithHresult(req.message, &r); err != nil {
 		return fmt.Errorf("failed to unmarshal deleteContainerState: %w", err)
 	}
-
-	//TODO (Mahati): Remove container state locally before passing it to inbox-gcs
-	/*
-		c, err := b.hostState.GetCreatedContainer(request.ContainerID)
-		if err != nil {
-			return nil, err
-		}
-		// remove container state regardless of delete's success
-		defer b.hostState.RemoveContainer(request.ContainerID)*/
+	_, err = b.hostState.GetCreatedContainer(req.ctx, r.ContainerID)
+	if err != nil {
+		log.G(req.ctx).Tracef("Container not found during deleteContainerState: %v", r.ContainerID)
+		return fmt.Errorf("container not found: %w", err)
+	}
+	// remove container state regardless of delete's success
+	defer b.hostState.RemoveContainer(req.ctx, r.ContainerID)
 
 	b.forwardRequestToGcs(req)
 	return nil

internal/gcs-sidecar/host.go

@@ -169,7 +169,6 @@ func (h *Host) SetWCOWConfidentialUVMOptions(ctx context.Context, securityPolicy
 		DefaultCRIMounts(),
 		DefaultCRIPrivilegedMounts(),
 		maxErrorMessageLength,
-		"windows",
 	)
 	if err != nil {
 		return fmt.Errorf("error creating security policy enforcer: %w", err)
@@ -193,18 +192,20 @@ func (h *Host) AddContainer(ctx context.Context, id string, c *Container) error
 
 	if _, ok := h.containers[id]; ok {
 		log.G(ctx).Tracef("Container exists in the map: %v", ok)
+		return gcserr.NewHresultError(gcserr.HrVmcomputeSystemAlreadyExists)
 	}
 	log.G(ctx).Tracef("AddContainer: ID: %v", id)
 	h.containers[id] = c
 	return nil
 }
 
-func (h *Host) RemoveContainer(id string) {
+func (h *Host) RemoveContainer(ctx context.Context, id string) {
 	h.containersMutex.Lock()
 	defer h.containersMutex.Unlock()
 
 	_, ok := h.containers[id]
 	if !ok {
+		log.G(ctx).Tracef("RemoveContainer: Container not found: ID: %v", id)
 		return
 	}
 
@@ -217,6 +218,7 @@ func (h *Host) GetCreatedContainer(ctx context.Context, id string) (*Container,
 
 	c, ok := h.containers[id]
 	if !ok {
+		log.G(ctx).Tracef("GetCreatedContainer: Container not found: ID: %v", id)
 		return nil, gcserr.NewHresultError(gcserr.HrVmcomputeSystemNotFound)
 	}
 	return c, nil

internal/guest/runtime/hcsv2/uvm.go

@@ -121,7 +121,6 @@ func (h *Host) SetConfidentialUVMOptions(ctx context.Context, r *guestresource.L
 		policy.DefaultCRIMounts(),
 		policy.DefaultCRIPrivilegedMounts(),
 		maxErrorMessageLength,
-		"linux",
 	)
 	if err != nil {
 		return err

pkg/securitypolicy/securitypolicy_linux.go

@@ -17,6 +17,9 @@ import (
 	"github.com/pkg/errors"
 )
 
+//nolint:unused
+const osType = "linux"
+
 // This is being used by StandEnforcer.
 // substituteUVMPath substitutes mount prefix to an appropriate path inside
 // UVM. At policy generation time, it's impossible to tell what the sandboxID

pkg/securitypolicy/securitypolicy_windows.go

@@ -5,6 +5,9 @@ package securitypolicy
 
 import oci "github.com/opencontainers/runtime-spec/specs-go"
 
+//nolint:unused
+const osType = "windows"
+
 // This is being used by StandEnforcer and is a no-op for windows.
 // substituteUVMPath substitutes mount prefix to an appropriate path inside
 // UVM. At policy generation time, it's impossible to tell what the sandboxID

pkg/securitypolicy/securitypolicyenforcer.go

@@ -7,7 +7,6 @@ import (
 	"fmt"
 	"regexp"
 	"strconv"
-	"strings"
 	"sync"
 	"syscall"
 
@@ -57,11 +56,6 @@ var (
 	defaultEnforcer     = standardEnforcer
 )
 
-// Package-level variable for OS type, set during enforcer creation
-//
-//nolint:unused
-var osType string = "unknown"
-
 func init() {
 	registeredEnforcers[openDoorEnforcer] = createOpenDoorEnforcer
 	registeredEnforcers[standardEnforcer] = createStandardEnforcer
@@ -260,15 +254,13 @@ func CreateSecurityPolicyEnforcer(
 	criMounts,
 	criPrivilegedMounts []oci.Mount,
 	maxErrorMessageLength int,
-	operatingSystem string,
 ) (SecurityPolicyEnforcer, error) {
 	if enforcer == "" {
 		enforcer = defaultEnforcer
 		if base64EncodedPolicy == "" {
 			enforcer = openDoorEnforcer
 		}
 	}
-	osType = strings.ToLower(operatingSystem)
 
 	if createEnforcer, ok := registeredEnforcers[enforcer]; !ok {
 		return nil, fmt.Errorf("unknown enforcer: %q", enforcer)

pkg/securitypolicy/securitypolicyenforcer_rego.go

@@ -211,6 +211,10 @@ func (policy *regoEnforcer) enableLogging(path string, logLevel rpi.LogLevel) {
 func newRegoPolicy(code string, defaultMounts []oci.Mount, privilegedMounts []oci.Mount, osType string) (policy *regoEnforcer, err error) {
 	policy = new(regoEnforcer)
 
+	if osType != "windows" && osType != "linux" {
+		return nil, fmt.Errorf("unsupported operating system: %q", osType)
+	}
+
 	policy.osType = osType
 	policy.defaultMounts = make([]oci.Mount, len(defaultMounts))
 	copy(policy.defaultMounts, defaultMounts)