Erroring out instead of silent override, and bubbling up the error to the caller

Signed-off-by: Manish Ranjan Mahanta <mmahanta@microsoft.com>

Author: Manish Ranjan Mahanta

internal/gcs-sidecar/handlers.go

@@ -526,7 +526,10 @@ func (b *Bridge) modifyServiceSettings(req *request) (err error) {
 					log.G(req.ctx).Tracef("Allowed log sources after policy enforcement: %v", allowedLogSources)
 
 					// Update the allowed log sources in the settings. This will be forwarded to inbox GCS which expects the log sources in a JSON string format with GUIDs for providers included.
-					allowedLogSources = etw.UpdateLogSources(req.ctx, allowedLogSources, false, true)
+					allowedLogSources, err := etw.UpdateLogSources(allowedLogSources, false, true)
+					if err != nil {
+						return fmt.Errorf("failed to update log sources: %w", err)
+					}
 					settings.Settings = allowedLogSources
 				}
 			default:

internal/uvm/log_wcow.go

@@ -4,6 +4,7 @@ package uvm
 
 import (
 	"context"
+	"fmt"
 
 	"github.com/Microsoft/hcsshim/internal/gcs"
 	"github.com/Microsoft/hcsshim/internal/gcs/prot"
@@ -69,15 +70,17 @@ 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.defaultLogSourcesEnabled, !uvm.HasConfidentialPolicy())
-
+		settings, err := etw.UpdateLogSources(uvm.logSources, uvm.defaultLogSourcesEnabled, !uvm.HasConfidentialPolicy())
+		if err != nil {
+			return fmt.Errorf("failed to parse log sources: %w", err)
+		}
 		req := guestrequest.LogForwardServiceRPCRequest{
 			RPCType:  guestrequest.RPCModifyServiceSettings,
 			Settings: settings,
 		}
-		err := uvm.gc.ModifyServiceSettings(ctx, prot.LogForwardService, req)
+		err = uvm.gc.ModifyServiceSettings(ctx, prot.LogForwardService, req)
 		if err != nil {
-			return err
+			return fmt.Errorf("failed to modify service settings: %w", err)
 		}
 	}
 	return nil

internal/vm/vmutils/etw/provider_map.go

@@ -1,13 +1,12 @@
 package etw
 
 import (
-	"context"
 	"encoding/base64"
 	"encoding/json"
+	"fmt"
 	"strings"
 
 	"github.com/Microsoft/go-winio/pkg/guid"
-	"github.com/Microsoft/hcsshim/internal/log"
 )
 
 // Log Sources JSON structure
@@ -94,17 +93,15 @@ func mergeLogSources(resultSources []Source, userSources []Source) []Source {
 }
 
 // decodeAndUnmarshalLogSources decodes a base64-encoded JSON string and unmarshals it into a LogSourcesInfo.
-func decodeAndUnmarshalLogSources(ctx context.Context, base64EncodedJSONLogConfig string) (LogSourcesInfo, error) {
+func decodeAndUnmarshalLogSources(base64EncodedJSONLogConfig string) (LogSourcesInfo, error) {
 	jsonBytes, err := base64.StdEncoding.DecodeString(base64EncodedJSONLogConfig)
 	if err != nil {
-		log.G(ctx).Errorf("Error decoding base64 log config: %v", err)
-		return LogSourcesInfo{}, err
+		return LogSourcesInfo{}, fmt.Errorf("error decoding base64 log config: %w", err)
 	}
 
 	var userLogSources LogSourcesInfo
 	if err := json.Unmarshal(jsonBytes, &userLogSources); err != nil {
-		log.G(ctx).Errorf("Error unmarshalling user log config: %v", err)
-		return LogSourcesInfo{}, err
+		return LogSourcesInfo{}, fmt.Errorf("error unmarshalling user log config: %w", err)
 	}
 	return userLogSources, nil
 }
@@ -119,14 +116,13 @@ func trimGUID(in string) string {
 
 // resolveGUIDsWithLookup normalizes and fills in provider GUIDs from the well-known ETW map
 // for all providers across all sources. Providers with an invalid GUID are warned and skipped.
-func resolveGUIDsWithLookup(ctx context.Context, sources []Source) []Source {
+func resolveGUIDsWithLookup(sources []Source) ([]Source, error) {
 	for i, src := range sources {
 		for j, provider := range src.Providers {
 			if provider.ProviderGUID != "" {
 				guid, err := guid.FromString(trimGUID(provider.ProviderGUID))
 				if err != nil {
-					log.G(ctx).Warningf("Skipping invalid GUID %q for provider %q: %v", provider.ProviderGUID, provider.ProviderName, err)
-					continue
+					return nil, fmt.Errorf("invalid GUID %q for provider %q: %w", provider.ProviderGUID, provider.ProviderName, err)
 				}
 				sources[i].Providers[j].ProviderGUID = strings.ToLower(guid.String())
 			}
@@ -135,77 +131,86 @@ func resolveGUIDsWithLookup(ctx context.Context, sources []Source) []Source {
 			}
 		}
 	}
-	return sources
+	return sources, nil
 }
 
 // stripRedundantGUIDs removes the GUID from providers where both Name and GUID are present and
 // the GUID matches the well-known lookup by name. This ensures sidecar-GCS prefers name-based
-// policy verification. Invalid GUIDs are warned and left as-is after normalization.
-func stripRedundantGUIDs(ctx context.Context, sources []Source) []Source {
+// policy verification. Invalid GUIDs are errored out.
+func stripRedundantGUIDs(sources []Source) ([]Source, error) {
 	for i, src := range sources {
 		for j, provider := range src.Providers {
 			if provider.ProviderName == "" || provider.ProviderGUID == "" {
 				continue
 			}
 			guid, err := guid.FromString(trimGUID(provider.ProviderGUID))
 			if err != nil {
-				log.G(ctx).Warningf("Skipping invalid GUID %q for provider %q: %v", provider.ProviderGUID, provider.ProviderName, err)
-				continue
+				return nil, fmt.Errorf("invalid GUID %q for provider %q: %w", provider.ProviderGUID, provider.ProviderName, err)
 			}
 			if strings.EqualFold(guid.String(), getProviderGUIDFromName(provider.ProviderName)) {
 				sources[i].Providers[j].ProviderGUID = ""
 			} else {
+				// If the GUID doesn't match the well-known GUID for the provider name,
+				// we keep it but ensure it's normalized to lowercase without braces.
+				// However, we remove the provider name to avoid incorrect policy matches
+				// in sidecar-GCS, since the GUID is the source of truth in this case.
+				sources[i].Providers[j].ProviderName = ""
 				sources[i].Providers[j].ProviderGUID = strings.ToLower(guid.String())
 			}
 		}
 	}
-	return sources
+	return sources, nil
 }
 
 // applyGUIDPolicy applies GUID resolution or stripping to all sources depending on the includeGUIDs flag.
 // See resolveGUIDsWithLookup and stripRedundantGUIDs for the respective behaviors.
-func applyGUIDPolicy(ctx context.Context, sources []Source, includeGUIDs bool) []Source {
+func applyGUIDPolicy(sources []Source, includeGUIDs bool) ([]Source, error) {
 	if len(sources) == 0 {
-		return sources
+		return sources, nil
 	}
 	if includeGUIDs {
-		return resolveGUIDsWithLookup(ctx, sources)
+		return resolveGUIDsWithLookup(sources)
 	}
-	return stripRedundantGUIDs(ctx, sources)
+	return stripRedundantGUIDs(sources)
 }
 
 // marshalAndEncodeLogSources marshals the given LogSourcesInfo to JSON and encodes it as a base64 string.
 // On error, it logs and returns the original fallback string.
-func marshalAndEncodeLogSources(ctx context.Context, logCfg LogSourcesInfo, fallback string) (string, error) {
+func marshalAndEncodeLogSources(logCfg LogSourcesInfo) (string, error) {
 	jsonBytes, err := json.Marshal(logCfg)
 	if err != nil {
-		log.G(ctx).Errorf("Error marshalling log config: %v", err)
-		return fallback, err
+		return "", fmt.Errorf("error marshalling log config: %w", err)
 	}
 	return base64.StdEncoding.EncodeToString(jsonBytes), nil
 }
 
 // UpdateLogSources updates the user provided log sources with the default log sources based on the
 // configuration and returns the updated log sources as a base64 encoded JSON string.
 // If there is an error in the process, it returns the original user provided log sources string.
-func UpdateLogSources(ctx context.Context, base64EncodedJSONLogConfig string, useDefaultLogSources bool, includeGUIDs bool) string {
+func UpdateLogSources(base64EncodedJSONLogConfig string, useDefaultLogSources bool, includeGUIDs bool) (string, error) {
 	var resultLogCfg LogSourcesInfo
 	if useDefaultLogSources {
 		resultLogCfg = defaultLogSourcesInfo
 	}
 
 	if base64EncodedJSONLogConfig != "" {
-		userLogSources, err := decodeAndUnmarshalLogSources(ctx, base64EncodedJSONLogConfig)
-		if err == nil {
-			resultLogCfg.LogConfig.Sources = mergeLogSources(resultLogCfg.LogConfig.Sources, userLogSources.LogConfig.Sources)
+		userLogSources, err := decodeAndUnmarshalLogSources(base64EncodedJSONLogConfig)
+		if err != nil {
+			return "", fmt.Errorf("failed to decode and unmarshal user log sources: %w", err)
 		}
+		resultLogCfg.LogConfig.Sources = mergeLogSources(resultLogCfg.LogConfig.Sources, userLogSources.LogConfig.Sources)
+
 	}
 
-	resultLogCfg.LogConfig.Sources = applyGUIDPolicy(ctx, resultLogCfg.LogConfig.Sources, includeGUIDs)
+	var err error
+	resultLogCfg.LogConfig.Sources, err = applyGUIDPolicy(resultLogCfg.LogConfig.Sources, includeGUIDs)
+	if err != nil {
+		return "", fmt.Errorf("failed to apply GUID policy: %w", err)
+	}
 
-	result, err := marshalAndEncodeLogSources(ctx, resultLogCfg, base64EncodedJSONLogConfig)
+	result, err := marshalAndEncodeLogSources(resultLogCfg)
 	if err != nil {
-		return base64EncodedJSONLogConfig
+		return "", fmt.Errorf("failed to marshal and encode log sources: %w", err)
 	}
-	return result
+	return result, nil
 }