PR: name, test cleanup, move annotation expansion

Made capitalization for GMSA consistent throughout.

Removed changes to test/functional/utilities

Moved general annotation handling and parsing from `internal/oci/uvm.go`
to `internal/oci/annotations`, moved annotation expansion there as
well, and updated tests.

Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>

cmd/containerd-shim-runhcs-v1/service_internal.go

@@ -91,9 +91,13 @@ func (s *service) createInternal(ctx context.Context, req *task.CreateTaskReques
 	}
 	f.Close()
 
-	spec, err = oci.UpdateSpecFromOptions(spec, shimOpts)
+	spec = oci.UpdateSpecFromOptions(spec, shimOpts)
+	//expand annotations after defaults have been loaded in from options
+	err = oci.ProcessAnnotations(ctx, &spec)
+	// since annotation expansion is used to toggle security features
+	// raise it rather than suppress and move on
 	if err != nil {
-		return nil, errors.Wrap(err, "unable to update OCI Spec from runtime shim options")
+		return nil, errors.Wrap(err, "unable to process OCI Spec annotations")
 	}
 
 	if len(req.Rootfs) == 0 {

cmd/containerd-shim-runhcs-v1/task_hcs.go

@@ -129,14 +129,14 @@ func createContainer(ctx context.Context, id, owner, netNS string, s *specs.Spec
 			return nil, nil, err
 		}
 	} else {
-		disableGmsa := oci.ParseAnnotationsDisableGmsa(ctx, s)
+		disableGMSA := oci.ParseAnnotationsDisableGMSA(ctx, s)
 		opts := &hcsoci.CreateOptions{
 			ID:               id,
 			Owner:            owner,
 			Spec:             s,
 			HostingSystem:    parent,
 			NetworkNamespace: netNS,
-			NoGmsa:           disableGmsa,
+			NoGMSA:           disableGMSA,
 		}
 		if shimOpts != nil {
 			opts.ScaleCPULimitsToSandbox = shimOpts.ScaleCpuLimitsToSandbox

internal/hcsoci/create.go

@@ -57,7 +57,7 @@ type CreateOptions struct {
 	ScaleCPULimitsToSandbox bool
 
 	// disable creating containers with GMSA credentials (Spec.Windows.CredentialSpec)
-	NoGmsa bool
+	NoGMSA bool
 }
 
 // createOptionsInternal is the set of user-supplied create options, but includes internal
@@ -176,7 +176,7 @@ func validateContainerConfig(ctx context.Context, coi *createOptionsInternal) er
 
 	// check if gMSA is disabled
 	if coi.Spec.Windows != nil {
-		if _, ok := coi.Spec.Windows.CredentialSpec.(string); ok && coi.NoGmsa {
+		if _, ok := coi.Spec.Windows.CredentialSpec.(string); ok && coi.NoGMSA {
 			return fmt.Errorf("gMSA credentials are disabled: %w", hcs.ErrOperationDenied)
 		}
 

internal/oci/annotations.go

@@ -0,0 +1,150 @@
+package oci
+
+import (
+	"context"
+	"errors"
+	"strconv"
+	"strings"
+
+	"github.com/Microsoft/hcsshim/internal/log"
+	"github.com/Microsoft/hcsshim/internal/logfields"
+	"github.com/Microsoft/hcsshim/pkg/annotations"
+	"github.com/opencontainers/runtime-spec/specs-go"
+	"github.com/sirupsen/logrus"
+)
+
+var ErrAnnotationExpansionConflict = errors.New("annotation expansion conflict")
+
+// ProcessAnnotations expands annotations into their corresponding annotation groups
+func ProcessAnnotations(ctx context.Context, s *specs.Spec) (err error) {
+	// Named `Process` and not `Expand` since this function may be expanded (pun intended) to
+	// deal with other annotation issues and validation.
+
+	// Rathen than give up part of the way through on error, this just emits a warning (similar
+	// to the `parseAnnotation*` functions) and continues through, so the spec is not left in a
+	// (partially) unusable form.
+	// If multiple different errors are to be raised, they should be combined or, if they
+	// are logged, only the last kept, depending on their severity.
+
+	// expand annotations
+	for key, exps := range annotations.AnnotationExpansions {
+		// check if annotation is present
+		if val, ok := s.Annotations[key]; ok {
+			// ideally, some normalization would occur here (ie, "True" -> "true")
+			// but strings may be case-sensitive
+			for _, k := range exps {
+				if v, ok := s.Annotations[k]; ok && val != v {
+					err = ErrAnnotationExpansionConflict
+					log.G(ctx).WithFields(logrus.Fields{
+						logfields.OCIAnnotation:               key,
+						logfields.Value:                       val,
+						logfields.OCIAnnotation + "-conflict": k,
+						logfields.Value + "-conflict":         v,
+					}).WithError(err).Warning("annotation expansion would overwrite conflicting value")
+					continue
+				}
+				s.Annotations[k] = val
+			}
+		}
+	}
+
+	return err
+}
+
+// handle specific annotations
+
+// ParseAnnotationsDisableGMSA searches for the boolean value which specifies
+// if providing a gMSA credential should be disallowed. Returns the value found,
+// if parsable, otherwise returns false otherwise.
+func ParseAnnotationsDisableGMSA(ctx context.Context, s *specs.Spec) bool {
+	return parseAnnotationsBool(ctx, s.Annotations, annotations.WCOWDisableGMSA, false)
+}
+
+// ParseAnnotationsSaveAsTemplate searches for the boolean value which specifies
+// if this create request should be considered as a template creation request. If value
+// is found the returns the actual value, returns false otherwise.
+func ParseAnnotationsSaveAsTemplate(ctx context.Context, s *specs.Spec) bool {
+	return parseAnnotationsBool(ctx, s.Annotations, annotations.SaveAsTemplate, false)
+}
+
+// ParseAnnotationsTemplateID searches for the templateID in the create request. If the
+// value is found then returns the value otherwise returns the empty string.
+func ParseAnnotationsTemplateID(ctx context.Context, s *specs.Spec) string {
+	return parseAnnotationsString(s.Annotations, annotations.TemplateID, "")
+}
+
+// general annotation parsing
+
+// parseAnnotationsBool searches `a` for `key` and if found verifies that the
+// value is `true` or `false` in any case. If `key` is not found returns `def`.
+func parseAnnotationsBool(ctx context.Context, a map[string]string, key string, def bool) bool {
+	if v, ok := a[key]; ok {
+		switch strings.ToLower(v) {
+		case "true":
+			return true
+		case "false":
+			return false
+		default:
+			logAnnotationParseError(ctx, key, v, logfields.Bool, nil)
+		}
+	}
+	return def
+}
+
+// parseAnnotationsUint32 searches `a` for `key` and if found verifies that the
+// value is a 32 bit unsigned integer. If `key` is not found returns `def`.
+func parseAnnotationsUint32(ctx context.Context, a map[string]string, key string, def uint32) uint32 {
+	if v, ok := a[key]; ok {
+		countu, err := strconv.ParseUint(v, 10, 32)
+		if err == nil {
+			v := uint32(countu)
+			return v
+		}
+		logAnnotationParseError(ctx, key, v, logfields.Uint32, err)
+	}
+	return def
+}
+
+// parseAnnotationsUint64 searches `a` for `key` and if found verifies that the
+// value is a 64 bit unsigned integer. If `key` is not found returns `def`.
+func parseAnnotationsUint64(ctx context.Context, a map[string]string, key string, def uint64) uint64 {
+	if v, ok := a[key]; ok {
+		countu, err := strconv.ParseUint(v, 10, 64)
+		if err == nil {
+			return countu
+		}
+		logAnnotationParseError(ctx, key, v, logfields.Uint64, err)
+	}
+	return def
+}
+
+// parseAnnotationsString searches `a` for `key`. If `key` is not found returns `def`.
+func parseAnnotationsString(a map[string]string, key string, def string) string {
+	if v, ok := a[key]; ok {
+		return v
+	}
+	return def
+}
+
+// ParseAnnotationCommaSeparated searches `annotations` for `annotation` corresponding to a
+// list of comma separated strings
+func ParseAnnotationCommaSeparated(annotation string, annotations map[string]string) []string {
+	cs, ok := annotations[annotation]
+	if !ok || cs == "" {
+		return nil
+	}
+	results := strings.Split(cs, ",")
+	return results
+}
+
+func logAnnotationParseError(ctx context.Context, k, v, et string, err error) {
+	entry := log.G(ctx).WithFields(logrus.Fields{
+		logfields.OCIAnnotation: k,
+		logfields.Value:         v,
+		logfields.ExpectedType:  et,
+	})
+	if err != nil {
+		entry = entry.WithError(err)
+	}
+	entry.Warning("annotation could not be parsed")
+}

internal/oci/annotations_test.go

@@ -0,0 +1,86 @@
+package oci
+
+import (
+	"context"
+	"errors"
+	"fmt"
+	"testing"
+
+	"github.com/Microsoft/hcsshim/pkg/annotations"
+	"github.com/opencontainers/runtime-spec/specs-go"
+	"github.com/sirupsen/logrus"
+)
+
+func Test_ProccessAnnotations_Expansion(t *testing.T) {
+	// suppress warnings raised by process annotation
+	defer func(l logrus.Level) {
+		logrus.SetLevel(l)
+	}(logrus.GetLevel())
+	logrus.SetLevel(logrus.ErrorLevel)
+	ctx := context.Background()
+
+	tests := []struct {
+		name string
+		spec specs.Spec
+	}{
+		{
+			name: "lcow",
+			spec: specs.Spec{
+				Linux: &specs.Linux{},
+			},
+		},
+		{
+			name: "wcow-hypervisor",
+			spec: specs.Spec{
+				Windows: &specs.Windows{
+					HyperV: &specs.WindowsHyperV{},
+				},
+			},
+		},
+		{
+			name: "wcow-process",
+			spec: specs.Spec{
+				Windows: &specs.Windows{},
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		// test correct expansion
+		for _, v := range []string{"true", "false"} {
+			t.Run(tt.name+"_disable_unsafe_"+v, func(subtest *testing.T) {
+				tt.spec.Annotations = map[string]string{
+					annotations.DisableUnsafeOperations: v,
+				}
+
+				err := ProcessAnnotations(ctx, &tt.spec)
+				if err != nil {
+					subtest.Fatalf("could not update spec from options: %v", err)
+				}
+
+				for _, k := range annotations.AnnotationExpansions[annotations.DisableUnsafeOperations] {
+					if vv := tt.spec.Annotations[k]; vv != v {
+						subtest.Fatalf("annotation %q was incorrectly expanded to %q, expected %q", k, vv, v)
+					}
+				}
+			})
+		}
+
+		// test errors raised on conflict
+		t.Run(tt.name+"_disable_unsafe_error", func(subtest *testing.T) {
+			tt.spec.Annotations = map[string]string{
+				annotations.DisableUnsafeOperations:    "true",
+				annotations.DisableWriteableFileShares: "false",
+			}
+
+			errExp := fmt.Sprintf("could not expand %q into %q",
+				annotations.DisableUnsafeOperations,
+				annotations.DisableWriteableFileShares)
+
+			err := ProcessAnnotations(ctx, &tt.spec)
+			if !errors.Is(err, ErrAnnotationExpansionConflict) {
+				t.Fatalf("UpdateSpecFromOptions should have failed with %q, actual was %v", errExp, err)
+			}
+		})
+	}
+}