PR: Update tests, add annotation expansion

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

Combined common OCI and gMSA tests together with subtests

Functional tests do not currently work, waiting on updated test
utilities and support, moved to another branch/PR.

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

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

@@ -91,7 +91,10 @@ func (s *service) createInternal(ctx context.Context, req *task.CreateTaskReques
  }
  f.Close()
 
- spec = oci.UpdateSpecFromOptions(spec, shimOpts)
+ spec, err = oci.UpdateSpecFromOptions(spec, shimOpts)
+ if err != nil {
+ return nil, errors.Wrap(err, "unable to update OCI Spec from runtime shim options")
+ }
 
  if len(req.Rootfs) == 0 {
  // If no mounts are passed via the snapshotter its the callers full

internal/oci/uvm.go

@@ -220,7 +220,7 @@ func parseAnnotationsString(a map[string]string, key string, def string) string
 // if providing a gMSA credential should be disallowed
 // is found the returns the actual value, returns false otherwise.
 func ParseAnnotationsDisableGmsa(ctx context.Context, s *specs.Spec) bool {
- return parseAnnotationsBool(ctx, s.Annotations, annotations.WcowDisableGmsa, false)
+ return parseAnnotationsBool(ctx, s.Annotations, annotations.WCOWDisableGmsa, false)
 }
 
 // ParseAnnotationsSaveAsTemplate searches for the boolean value which specifies
@@ -408,9 +408,9 @@ func SpecToUVMCreateOpts(ctx context.Context, s *specs.Spec, id, owner string) (
 
 // UpdateSpecFromOptions sets extra annotations on the OCI spec based on the
 // `opts` struct.
-func UpdateSpecFromOptions(s specs.Spec, opts *runhcsopts.Options) specs.Spec {
+func UpdateSpecFromOptions(s specs.Spec, opts *runhcsopts.Options) (specs.Spec, error) {
  if opts == nil {
- return s
+ return s, nil
  }
 
  if _, ok := s.Annotations[annotations.BootFilesRootPath]; !ok && opts.BootFilesRootPath != "" {
@@ -439,5 +439,21 @@ func UpdateSpecFromOptions(s specs.Spec, opts *runhcsopts.Options) specs.Spec {
  s.Annotations[key] = value
  }
  }
- return s
+
+ // 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 {
+ return s, fmt.Errorf("could not expand %q into %q: values %q and %q are mismatched", key, k, val, v)
+ }
+ s.Annotations[k] = val
+ }
+ }
+ }
+
+ return s, nil
 }

internal/oci/uvm_test.go

@@ -3,6 +3,7 @@ package oci
 import (
  "context"
  "fmt"
+ "strings"
  "testing"
 
  runhcsopts "github.com/Microsoft/hcsshim/cmd/containerd-shim-runhcs-v1/options"
@@ -13,7 +14,6 @@ import (
 )
 
 func Test_SpecUpdate_MemorySize_WithAnnotation_WithOpts(t *testing.T) {
-
  opts := &runhcsopts.Options{
  VmMemorySizeInMb: 3072,
  }
@@ -23,23 +23,28 @@ func Test_SpecUpdate_MemorySize_WithAnnotation_WithOpts(t *testing.T) {
  annotations.MemorySizeInMB: "2048",
  },
  }
- updatedSpec := UpdateSpecFromOptions(*s, opts)
+ updatedSpec, err := UpdateSpecFromOptions(*s, opts)
+ if err != nil {
+ t.Fatalf("could not update spec from options: %v", err)
+ }
 
  if updatedSpec.Annotations[annotations.MemorySizeInMB] != "2048" {
  t.Fatal("should not have updated annotation to default when annotation is provided in the spec")
  }
 }
 
 func Test_SpecUpdate_MemorySize_NoAnnotation_WithOpts(t *testing.T) {
-
  opts := &runhcsopts.Options{
  VmMemorySizeInMb: 3072,
  }
  s := &specs.Spec{
  Linux: &specs.Linux{},
  Annotations: map[string]string{},
  }
- updatedSpec := UpdateSpecFromOptions(*s, opts)
+ updatedSpec, err := UpdateSpecFromOptions(*s, opts)
+ if err != nil {
+ t.Fatalf("could not update spec from options: %v", err)
+ }
 
  if updatedSpec.Annotations[annotations.MemorySizeInMB] != "3072" {
  t.Fatal("should have updated annotation to default when annotation is not provided in the spec")
@@ -57,7 +62,10 @@ func Test_SpecUpdate_ProcessorCount_WithAnnotation_WithOpts(t *testing.T) {
  annotations.ProcessorCount: "8",
  },
  }
- updatedSpec := UpdateSpecFromOptions(*s, opts)
+ updatedSpec, err := UpdateSpecFromOptions(*s, opts)
+ if err != nil {
+ t.Fatalf("could not update spec from options: %v", err)
+ }
 
  if updatedSpec.Annotations[annotations.ProcessorCount] != "8" {
  t.Fatal("should not have updated annotation to default when annotation is provided in the spec")
@@ -73,7 +81,10 @@ func Test_SpecUpdate_ProcessorCount_NoAnnotation_WithOpts(t *testing.T) {
  Linux: &specs.Linux{},
  Annotations: map[string]string{},
  }
- updatedSpec := UpdateSpecFromOptions(*s, opts)
+ updatedSpec, err := UpdateSpecFromOptions(*s, opts)
+ if err != nil {
+ t.Fatalf("could not update spec from options: %v", err)
+ }
 
  if updatedSpec.Annotations[annotations.ProcessorCount] != "4" {
  t.Fatal("should have updated annotation to default when annotation is not provided in the spec")
@@ -83,7 +94,7 @@ func Test_SpecUpdate_ProcessorCount_NoAnnotation_WithOpts(t *testing.T) {
 func Test_SpecToUVMCreateOptions_Default_LCOW(t *testing.T) {
  s := &specs.Spec{
  Linux: &specs.Linux{},
- Annotations: map[string]string{},
+ Annotations: make(map[string]string),
  }
 
  opts, err := SpecToUVMCreateOpts(context.Background(), s, t.Name(), "")
@@ -108,7 +119,7 @@ func Test_SpecToUVMCreateOptions_Default_WCOW(t *testing.T) {
  Windows: &specs.Windows{
  HyperV: &specs.WindowsHyperV{},
  },
- Annotations: map[string]string{},
+ Annotations: make(map[string]string),
  }
 
  opts, err := SpecToUVMCreateOpts(context.Background(), s, t.Name(), "")
@@ -124,72 +135,143 @@ func Test_SpecToUVMCreateOptions_Default_WCOW(t *testing.T) {
  }
 }
 
-func Test_SpecToUVMCreateOptions_Common_LCOW(t *testing.T) {
+func Test_SpecToUVMCreateOptions_Common(t *testing.T) {
  cpugroupid := "1"
  lowmmiogap := 1024
- s := &specs.Spec{
- Linux: &specs.Linux{},
- Annotations: map[string]string{
- annotations.ProcessorCount: "8",
- annotations.CPUGroupID: cpugroupid,
- annotations.DisableWriteableFileShares: "true",
- annotations.MemoryLowMMIOGapInMB: fmt.Sprint(lowmmiogap),
- },
+ as := map[string]string{
+ annotations.ProcessorCount: "8",
+ annotations.CPUGroupID: cpugroupid,
+ annotations.DisableWriteableFileShares: "true",
+ annotations.MemoryLowMMIOGapInMB: fmt.Sprint(lowmmiogap),
  }
 
- opts, err := SpecToUVMCreateOpts(context.Background(), s, t.Name(), "")
- if err != nil {
- t.Fatalf("could not generate creation options from spec: %v", err)
+ tests := []struct {
+ name string
+ spec specs.Spec
+ extract func(interface{}) *uvm.Options
+ }{
+ {
+ name: "lcow",
+ spec: specs.Spec{
+ Linux: &specs.Linux{},
+ },
+ // generics would be nice ...
+ extract: func(i interface{}) *uvm.Options {
+ o := (i).(*uvm.OptionsLCOW)
+ return o.Options
+ },
+ },
+ {
+ name: "wcow-hypervisor",
+ spec: specs.Spec{
+ Windows: &specs.Windows{
+ HyperV: &specs.WindowsHyperV{},
+ },
+ },
+ extract: func(i interface{}) *uvm.Options {
+ o := (i).(*uvm.OptionsWCOW)
+ return o.Options
+ },
+ },
  }
 
- lopts := (opts).(*uvm.OptionsLCOW)
- // todo: move these all into subtests with t.Run?
- if lopts.LowMMIOGapInMB != uint64(lowmmiogap) {
- t.Fatalf("should have updated creation options low MMIO Gap when annotation is provided: %v != %v", lopts.LowMMIOGapInMB, lowmmiogap)
- }
- if lopts.ProcessorCount != 8 {
- t.Fatalf("should have updated creation options processor count when annotation is provided: %v != 8", lopts.ProcessorCount)
- }
- if lopts.CPUGroupID != cpugroupid {
- t.Fatalf("should have updated creation options CPU group Id when annotation is provided: %v != %v", lopts.CPUGroupID, cpugroupid)
- }
- if !lopts.NoWritableFileShares {
- t.Fatal("should have disabled writable in shares creation when annotation is provided")
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ tt.spec.Annotations = as
+ opts, err := SpecToUVMCreateOpts(context.Background(), &tt.spec, t.Name(), "")
+ if err != nil {
+ t.Fatalf("could not generate creation options from spec: %v", err)
+ }
+
+ // get the underlying uvm.Options from uvm.Options[LW]COW
+ copts := tt.extract(opts)
+ if copts.LowMMIOGapInMB != uint64(lowmmiogap) {
+ t.Fatalf("should have updated creation options low MMIO Gap when annotation is provided: %v != %v", copts.LowMMIOGapInMB, lowmmiogap)
+ }
+ if copts.ProcessorCount != 8 {
+ t.Fatalf("should have updated creation options processor count when annotation is provided: %v != 8", copts.ProcessorCount)
+ }
+ if copts.CPUGroupID != cpugroupid {
+ t.Fatalf("should have updated creation options CPU group Id when annotation is provided: %v != %v", copts.CPUGroupID, cpugroupid)
+ }
+ if !copts.NoWritableFileShares {
+ t.Fatal("should have disabled writable in shares creation when annotation is provided")
+ }
+ })
  }
+
 }
 
-func Test_SpecToUVMCreateOptions_Common_WCOW(t *testing.T) {
- cpugroupid := "1"
- lowmmiogap := 1024
- s := &specs.Spec{
- Windows: &specs.Windows{
- HyperV: &specs.WindowsHyperV{},
+func Test_SpecUpdate_Annotation_Expansion(t *testing.T) {
+ opts := &runhcsopts.Options{}
+
+ tests := []struct {
+ name string
+ spec specs.Spec
+ }{
+ {
+ name: "lcow",
+ spec: specs.Spec{
+ Linux: &specs.Linux{},
+ },
  },
- Annotations: map[string]string{
- annotations.ProcessorCount: "8",
- annotations.CPUGroupID: cpugroupid,
- annotations.DisableWriteableFileShares: "true",
- annotations.MemoryLowMMIOGapInMB: fmt.Sprint(lowmmiogap),
+ {
+ name: "wcow-hypervisor",
+ spec: specs.Spec{
+ Windows: &specs.Windows{
+ HyperV: &specs.WindowsHyperV{},
+ },
+ },
+ },
+ {
+ name: "wcow-process",
+ spec: specs.Spec{
+ Windows: &specs.Windows{},
+ },
  },
  }
 
- opts, err := SpecToUVMCreateOpts(context.Background(), s, t.Name(), "")
- if err != nil {
- t.Fatalf("could not generate creation options from spec: %v", err)
- }
+ 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,
+ }
 
- wopts := (opts).(*uvm.OptionsWCOW)
- // todo: move these all into subtests with t.Run?
- if wopts.LowMMIOGapInMB != uint64(lowmmiogap) {
- t.Fatalf("should have updated creation options low MMIO Gap when annotation is provided: %v != %v", wopts.LowMMIOGapInMB, lowmmiogap)
- }
- if wopts.ProcessorCount != 8 {
- t.Fatalf("should have updated creation options processor count when annotation is provided: %v != 8", wopts.ProcessorCount)
- }
- if wopts.CPUGroupID != cpugroupid {
- t.Fatalf("should have updated creation options CPU group Id when annotation is provided: %v != %v", wopts.CPUGroupID, cpugroupid)
- }
- if !wopts.NoWritableFileShares {
- t.Fatal("should have disabled writable in shares creation when annotation is provided")
+ updatedSpec, err := UpdateSpecFromOptions(tt.spec, opts)
+ if err != nil {
+ subtest.Fatalf("could not update spec from options: %v", err)
+ }
+
+ // the original should be kept
+ if vv := updatedSpec.Annotations[annotations.DisableUnsafeOperations]; vv != v {
+ subtest.Fatalf("original annotation was modified: found %q, expected %q", vv, v)
+ }
+
+ for _, k := range annotations.AnnotationExpansions[annotations.DisableUnsafeOperations] {
+ if vv := updatedSpec.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 := UpdateSpecFromOptions(tt.spec, opts)
+ if !strings.HasPrefix(err.Error(), errExp) {
+ t.Fatalf("UpdateSpecFromOptions should have failed with %q, actual was %v", errExp, err)
+ }
+ })
  }
 }

pkg/annotations/annotations.go

@@ -271,5 +271,22 @@ const (
 
  // WCOWDisableGMSA disables providing gMSA (Group Managed Service Accounts) to
  // a WCOW container
- WcowDisableGmsa = "io.microsoft.wcow.gmsa.disable"
+ WCOWDisableGmsa = "io.microsoft.container.wcow.gmsa.disable"
+
+ // DisableUnsafeOperations disables several unsafe operations, such as writable
+ // file share mounts, for hostile multi-tenant environments. See `AnnotationExpansions`
+ // for more information
+ DisableUnsafeOperations = "io.microsoft.disable-unsafe-operations"
 )
+
+// AnnotationExpansions maps annotations that will be expanded into an array of
+// other annotations. The expanded annotations will have the same value as the
+// original. It is an error for the expansions to already exist and have a value
+// that differs from the original.
+var AnnotationExpansions map[string][]string = map[string][]string{
+ DisableUnsafeOperations: {
+ WCOWDisableGmsa,
+ DisableWriteableFileShares,
+ VSMBNoDirectMap,
+ },
+}

test/cri-containerd/container_fileshare_test.go

@@ -73,12 +73,12 @@ func Test_Container_File_Share_Writable_WCOW(t *testing.T) {
 
  // container should fail because of writable mount
  _, err := client.StartContainer(ctx, &runtime.StartContainerRequest{ContainerId: cID})
+ if err == nil {
+ stopContainer(t, client, ctx, cID)
+ }
  // error is serialized over gRPC then embedded into "rpc error: code = %s desc = %s"
  // so error.Is() wont work
  if !strings.Contains(err.Error(), fmt.Errorf("adding writable shares is denied: %w", hcs.ErrOperationDenied).Error()) {
- if err == nil {
- stopContainer(t, client, ctx, cID)
- }
  t.Fatalf("StartContainer did not fail with writable fileshare: error is %q", err)
  }
 
@@ -164,12 +164,12 @@ func Test_Container_File_Share_Writable_LCOW(t *testing.T) {
 
  // container should fail because of writable mount
  _, err := client.StartContainer(ctx, &runtime.StartContainerRequest{ContainerId: cID})
+ if err == nil {
+ stopContainer(t, client, ctx, cID)
+ }
  // error is serialized over gRPC then embedded into "rpc error: code = %s desc = %s"
  // so error.Is() wont work
  if !strings.Contains(err.Error(), fmt.Errorf("adding writable shares is denied: %w", hcs.ErrOperationDenied).Error()) {
- if err == nil {
- stopContainer(t, client, ctx, cID)
- }
  t.Fatalf("StartContainer did not fail with writable fileshare: error is %q", err)
  }