PR: tests, annotation expansion, naming, creation options

Can now expand an annotation into several sub annotations to toggle
groups of features on and off. Added tests for this as well.

Moved general annotation handling and parsing from `internal/oci/uvm.go`
to `internal/oci/annotations`, and updated tests.

Combined common OCI and gMSA tests together with subtests
Made capitalization for GMSA consistent throughout.

Removed changes to test/functional/utilities

Removed unnecessary error return.

Removed creation option for disabling gMSA and instead parse the
annotation directly from the spec.

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

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

@@ -94,6 +94,13 @@ func (s *service) createInternal(ctx context.Context, req *task.CreateTaskReques
  f.Close()
 
  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 process OCI Spec annotations")
+ }
 
  if len(req.Rootfs) == 0 {
  // If no mounts are passed via the snapshotter its the callers full

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

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

internal/hcsoci/create.go

@@ -55,9 +55,6 @@ type CreateOptions struct {
  // ScaleCPULimitsToSandbox indicates that the container CPU limits should be adjusted to account
  // for the difference in CPU count between the host and the UVM.
  ScaleCPULimitsToSandbox bool
-
- // disable creating containers with GMSA credentials (Spec.Windows.CredentialSpec)
- NoGmsa bool
 }
 
 // createOptionsInternal is the set of user-supplied create options, but includes internal
@@ -176,10 +173,10 @@ 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 {
+ disableGMSA := oci.ParseAnnotationsDisableGMSA(ctx, coi.Spec)
+ if _, ok := coi.Spec.Windows.CredentialSpec.(string); ok && disableGMSA {
  return fmt.Errorf("gMSA credentials are disabled: %w", hcs.ErrOperationDenied)
  }
-
  }
 
  return nil

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.DisableWritableFileShares: "false",
+ }
+
+ errExp := fmt.Sprintf("could not expand %q into %q",
+ annotations.DisableUnsafeOperations,
+ annotations.DisableWritableFileShares)
+
+ err := ProcessAnnotations(ctx, &tt.spec)
+ if !errors.Is(err, ErrAnnotationExpansionConflict) {
+ t.Fatalf("UpdateSpecFromOptions should have failed with %q, actual was %v", errExp, err)
+ }
+ })
+ }
+}