Addressing review comments

Author: Manish Ranjan Mahanta

internal/gcs-sidecar/handlers.go

@@ -500,7 +500,7 @@ func (b *Bridge) modifyServiceSettings(req *request) (err error) {
 	// Todo: Add policy enforcement for modifying service settings
 	modifyRequest, err := unmarshalModifyServiceSettings(req)
 	if err != nil {
-		return err
+		return fmt.Errorf("failed to unmarshal modifyServiceSettings request: %w", err)
 	}
 
 	switch modifyRequest.PropertyType {

internal/oci/uvm.go

@@ -419,13 +419,13 @@ func SpecToUVMCreateOpts(ctx context.Context, s *specs.Spec, id, owner string) (
 		if err := handleWCOWSecurityPolicy(ctx, s.Annotations, wopts); err != nil {
 			return nil, err
 		}
-		// If security policy is enable, wopts.DisableLogForwarding default value should be true (CWCOW should not allow log forwarding by default)
+		// If security policy is enable, wopts.LogForwardingEnabled default value should be false (CWCOW should not allow log forwarding by default)
 		if wopts.SecurityPolicyEnabled {
-			wopts.DisableLogForwarding = true
+			wopts.LogForwardingEnabled = false
 		}
 		wopts.LogSources = ParseAnnotationsString(s.Annotations, annotations.LogSources, wopts.LogSources)
-		wopts.DisableLogForwarding = ParseAnnotationsBool(ctx, s.Annotations, annotations.DisableForwardLogs, wopts.DisableLogForwarding)
-		wopts.DisableDefaultLogSources = ParseAnnotationsBool(ctx, s.Annotations, annotations.DisableDefaultLogSources, wopts.DisableDefaultLogSources)
+		wopts.LogForwardingEnabled = ParseAnnotationsBool(ctx, s.Annotations, annotations.LogForwardingEnabled, wopts.LogForwardingEnabled)
+		wopts.DefaultLogSourcesEnabled = ParseAnnotationsBool(ctx, s.Annotations, annotations.DefaultLogSourcesEnabled, wopts.DefaultLogSourcesEnabled)
 
 		return wopts, nil
 	}

internal/uvm/create_wcow.go

@@ -72,8 +72,8 @@ type OptionsWCOW struct {
 
 	OutputHandlerCreator     vmutils.OutputHandlerCreator // Creates an [OutputHandler] that controls how output received over HVSocket from the UVM is handled. Defaults to parsing output as ETW Log events
 	LogSources               string                       // ETW providers to be set for the logging service
-	DisableLogForwarding     bool                         // Whether to disable forwarding of logs to the host or not
-	DisableDefaultLogSources bool                         // Whether to disable using default log sources
+	LogForwardingEnabled     bool                         // Whether to enable forwarding of logs to the host or not
+	DefaultLogSourcesEnabled bool                         // Whether to enable using default log sources
 }
 
 func defaultConfidentialWCOWOSBootFilesPath() string {
@@ -113,8 +113,8 @@ func NewDefaultOptionsWCOW(id, owner string) *OptionsWCOW {
 			},
 		},
 		OutputHandlerCreator:     vmutils.ParseGCSLogrus,
-		DisableLogForwarding:     false, // Default to true for WCOW, and set to false for CWCOW in internal/oci/uvm.go SpecToUVMCreateOpts
-		DisableDefaultLogSources: false,
+		LogForwardingEnabled:     true, // Default to true for WCOW, and set to false for CWCOW in internal/oci/uvm.go SpecToUVMCreateOpts
+		DefaultLogSourcesEnabled: true,
 		LogSources:               "",
 	}
 }
@@ -293,7 +293,7 @@ func prepareCommonConfigDoc(ctx context.Context, uvm *UtilityVM, opts *OptionsWC
 	}
 
 	maps.Copy(doc.VirtualMachine.Devices.HvSocket.HvSocketConfig.ServiceTable, opts.AdditionalHyperVConfig)
-	if !opts.DisableLogForwarding {
+	if opts.LogForwardingEnabled {
 		key := prot.WindowsLoggingHvsockServiceID.String()
 		doc.VirtualMachine.Devices.HvSocket.HvSocketConfig.ServiceTable[key] = hcsschema.HvSocketServiceConfig{
 			AllowWildcardBinds:        true,
@@ -579,8 +579,8 @@ func CreateWCOW(ctx context.Context, opts *OptionsWCOW) (_ *UtilityVM, err error
 		createOpts:               opts,
 		blockCIMMounts:           make(map[string]*UVMMountedBlockCIMs),
 		logSources:               opts.LogSources,
-		forwardLogs:              !opts.DisableLogForwarding,
-		disableDefaultLogSources: opts.DisableDefaultLogSources,
+		logForwardingEnabled:     opts.LogForwardingEnabled,
+		defaultLogSourcesEnabled: opts.DefaultLogSourcesEnabled,
 	}
 
 	defer func() {
@@ -620,7 +620,7 @@ func CreateWCOW(ctx context.Context, opts *OptionsWCOW) (_ *UtilityVM, err error
 		return nil, fmt.Errorf("error while creating the compute system: %w", err)
 	}
 
-	if !opts.DisableLogForwarding {
+	if opts.LogForwardingEnabled {
 		// Create a socket that the executed program can send to. This is usually
 		// used by Log Forward Service to send log data.
 		uvm.outputHandler = opts.OutputHandlerCreator(opts.ID)

internal/uvm/log_wcow.go

@@ -69,7 +69,7 @@ func (uvm *UtilityVM) SetLogSources(ctx context.Context) error {
 		// For confidential WCOw, we skip the adding guids to the log sources as the sidecar-GCS will verify the
 		// allowed log sources against policy and append the necessary GUIDs to the ones allowed. Rest are dropped.
 		// For non-confidential WCOW, we include the GUIDs in the log sources as the hcsshim communicates directly with the inboxGCS.
-		settings := etw.UpdateLogSources(ctx, uvm.logSources, !uvm.disableDefaultLogSources, !uvm.HasConfidentialPolicy())
+		settings := etw.UpdateLogSources(ctx, uvm.logSources, uvm.defaultLogSourcesEnabled, !uvm.HasConfidentialPolicy())
 
 		req := guestrequest.LogForwardServiceRPCRequest{
 			RPCType:  guestrequest.RPCModifyServiceSettings,

internal/uvm/start.go

@@ -285,7 +285,7 @@ func (uvm *UtilityVM) Start(ctx context.Context) (err error) {
 		}
 	}
 
-	if uvm.OS() == "windows" && uvm.forwardLogs {
+	if uvm.OS() == "windows" && uvm.logForwardingEnabled {
 		// If the UVM is Windows and log forwarding is enabled, set the log sources
 		// and start the log forwarding service.
 		if err := uvm.SetLogSources(ctx); err != nil {

internal/uvm/types.go

@@ -144,8 +144,8 @@ type UtilityVM struct {
 	blockCIMMounts    map[string]*UVMMountedBlockCIMs
 	blockCIMMountLock sync.Mutex
 
-	forwardLogs              bool   // Indicates whether to forward logs from the UVM to the host
-	disableDefaultLogSources bool   // Specifies whether addition of default list of ETW providers should be disabled
+	logForwardingEnabled     bool   // Indicates whether to forward logs from the UVM to the host
+	defaultLogSourcesEnabled bool   // Specifies whether addition of default list of ETW providers should be disabled
 	logSources               string // ETW providers to enable for log forwarding
 }
 

internal/vm/vmutils/etw/default-sources.go

@@ -1,6 +1,6 @@
 package etw
 
-// defaultLogSourcesInfo is the native Go representation of the default-logsources.json file.
+// defaultLogSourcesInfo defines the list of trusted ETW providers
 var defaultLogSourcesInfo = LogSourcesInfo{
 	LogConfig: LogConfig{
 		Sources: []Source{

internal/vm/vmutils/etw/default-sources_test.go

@@ -6,12 +6,12 @@ import (
 )
 
 func TestDefaultSources_ETWProvidersExistInETWMap(t *testing.T) {
-	if len(etwNameToGuidMap) == 0 {
-		t.Fatal("etwNameToGuidMap is empty")
+	if len(etwNameToGUIDMap) == 0 {
+		t.Fatal("etwNameToGUIDMap is empty")
 	}
 
 	for si, src := range defaultLogSourcesInfo.LogConfig.Sources {
-		// Only ETW sources should be validated against etwNameToGuidMap.
+		// Only ETW sources should be validated against etwNameToGUIDMap.
 		if !strings.EqualFold(src.Type, "ETW") {
 			continue
 		}
@@ -22,9 +22,9 @@ func TestDefaultSources_ETWProvidersExistInETWMap(t *testing.T) {
 			}
 
 			key := strings.ToLower(p.ProviderName)
-			if _, ok := etwNameToGuidMap[key]; !ok {
+			if _, ok := etwNameToGUIDMap[key]; !ok {
 				t.Fatalf(
-					"provider not found in etwNameToGuidMap: source index=%d provider index=%d provider=%q lookup key=%q",
+					"provider not found in etwNameToGUIDMap: source index=%d provider index=%d provider=%q lookup key=%q",
 					si, pi, p.ProviderName, key,
 				)
 			}

internal/vm/vmutils/etw/etw_map.go

@@ -1,7 +1,7 @@
 package etw
 
 // LOWERCASE ONLY keys for easier lookups and case-insensitive matching.
-var etwNameToGuidMap = map[string]string{
+var etwNameToGUIDMap = map[string]string{
 	"microsoft.windows.containers.setup":                                         "22267b1c-b979-5c81-9e24-0db386a62dd1",
 	"microsoft.windows.containers.storage":                                       "2551390d-5927-5c84-6f0a-027a7e78d38d",
 	"microsoft.windows.containers.library":                                       "67eb0417-9297-42ae-a1d9-98bfeb359059",

internal/vm/vmutils/etw/etw_map_test.go

@@ -5,12 +5,12 @@ import (
 	"testing"
 )
 
-func TestETWNameToGuidMap_AllKeysAndValuesAreLowercase(t *testing.T) {
-	if len(etwNameToGuidMap) == 0 {
-		t.Fatal("etwNameToGuidMap is empty")
+func TestETWNameToGUIDMap_AllKeysAndValuesAreLowercase(t *testing.T) {
+	if len(etwNameToGUIDMap) == 0 {
+		t.Fatal("etwNameToGUIDMap is empty")
 	}
 
-	for key, value := range etwNameToGuidMap {
+	for key, value := range etwNameToGUIDMap {
 		if key != strings.ToLower(key) {
 			t.Fatalf("map key is not lowercase: key=%q value=%q", key, value)
 		}
@@ -20,7 +20,7 @@ func TestETWNameToGuidMap_AllKeysAndValuesAreLowercase(t *testing.T) {
 	}
 }
 
-func isValidGuid(guid string) bool {
+func isValidGUID(guid string) bool {
 	// GUID format: xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx (8-4-4-4-12 hex digits)
 	if len(guid) != 36 {
 		return false
@@ -40,24 +40,24 @@ func isValidGuid(guid string) bool {
 	return true
 }
 
-func TestETWNameToGuidMap_AllGuidsAreValid(t *testing.T) {
-	for key, guid := range etwNameToGuidMap {
-		if !isValidGuid(guid) {
+func TestETWNameToGUIDMap_AllGUIDsAreValid(t *testing.T) {
+	for key, guid := range etwNameToGUIDMap {
+		if !isValidGUID(guid) {
 			t.Fatalf("invalid GUID format: key=%q guid=%q", key, guid)
 		}
 	}
 }
 
-func TestETWNameToGuidMap_KeysAreNonEmpty(t *testing.T) {
-	for key := range etwNameToGuidMap {
+func TestETWNameToGUIDMap_KeysAreNonEmpty(t *testing.T) {
+	for key := range etwNameToGUIDMap {
 		if strings.TrimSpace(key) == "" {
-			t.Fatal("found empty key in etwNameToGuidMap")
+			t.Fatal("found empty key in etwNameToGUIDMap")
 		}
 	}
 }
 
-func TestETWNameToGuidMap_ValuesAreNonEmpty(t *testing.T) {
-	for key, value := range etwNameToGuidMap {
+func TestETWNameToGUIDMap_ValuesAreNonEmpty(t *testing.T) {
+	for key, value := range etwNameToGUIDMap {
 		if strings.TrimSpace(value) == "" {
 			t.Fatalf("found empty value for key=%q", key)
 		}