Merge pull request #1096 from Random-Liu/finish-runtime-annotations

Cleanup pod annotation test and only support wildcard

docs/config.md

@@ -138,10 +138,14 @@ The explanation and default value of each configuration item are as follows:
       # runtime_type is the runtime type to use in containerd e.g. io.containerd.runtime.v1.linux
       runtime_type = "io.containerd.runc.v1"
 
-      # pod_annotations is list of pod annotations passed to both pod sandbox as well as
-      # container OCI annotations. Pod_annotations also support golang supported
-      # regular expression - https://github.com/google/re2/wiki/Syntax.
-      # e.g. ["runc.com.github.containers.runc.*"]
+      # pod_annotations is a list of pod annotations passed to both pod
+      # sandbox as well as container OCI annotations. Pod_annotations also
+      # supports golang path match pattern - https://golang.org/pkg/path/#Match.
+      # e.g. ["runc.com.*"], ["*.runc.com"], ["runc.com/*"].
+      #
+      # For the naming convention of annotation keys, please reference:
+      # * Kubernetes: https://kubernetes.io/docs/concepts/overview/working-with-objects/annotations/#syntax-and-character-set
+      # * OCI: https://github.com/opencontainers/image-spec/blob/master/annotations.md
       pod_annotations = []
 
       # "plugins.cri.containerd.runtimes.runc.options" is options specific to

pkg/config/config.go

@@ -31,7 +31,7 @@ type Runtime struct {
 	// This only works for runtime type "io.containerd.runtime.v1.linux".
 	// DEPRECATED: use Options instead. Remove when shim v1 is deprecated.
 	Engine string `toml:"runtime_engine" json:"runtimeEngine"`
-	// PodAnnotations is list of pod annotations passed to both pod sandbox as well as
+	// PodAnnotations is a list of pod annotations passed to both pod sandbox as well as
 	// container OCI annotations.
 	PodAnnotations []string `toml:"pod_annotations" json:"PodAnnotations"`
 	// Root is the directory used by containerd for runtime state.

pkg/server/container_create.go

@@ -169,7 +169,7 @@ func (c *criService) CreateContainer(ctx context.Context, r *runtime.CreateConta
 	if err != nil {
 		return nil, errors.Wrap(err, "failed to get sandbox runtime")
 	}
-	logrus.Debugf("Use OCI %+v for sandbox %q and container %q", ociRuntime, sandboxID, id)
+	logrus.Debugf("Use OCI runtime %+v for sandbox %q and container %q", ociRuntime, sandboxID, id)
 
 	spec, err := c.generateContainerSpec(id, sandboxID, sandboxPid, config, sandboxConfig,
 		&image.ImageSpec.Config, append(mounts, volumeMounts...), ociRuntime.PodAnnotations)

pkg/server/container_create_test.go

@@ -36,7 +36,6 @@ import (
 	runtime "k8s.io/kubernetes/pkg/kubelet/apis/cri/runtime/v1alpha2"
 
 	"github.com/containerd/cri/pkg/annotations"
-	criconfig "github.com/containerd/cri/pkg/config"
 	ostesting "github.com/containerd/cri/pkg/os/testing"
 	"github.com/containerd/cri/pkg/util"
 )
@@ -295,82 +294,64 @@ func TestPodAnnotationPassthroughContainerSpec(t *testing.T) {
 	testPid := uint32(1234)
 
 	for desc, test := range map[string]struct {
-		configCriService func(*criService)
-		configChange     func(*runtime.PodSandboxConfig)
-		specCheck        func(*testing.T, *runtimespec.Spec)
+		podAnnotations []string
+		configChange   func(*runtime.PodSandboxConfig)
+		specCheck      func(*testing.T, *runtimespec.Spec)
 	}{
-		// Scenario 1 - containerd config allows "c" and podSpec defines "c" to be passed to OCI
-		"passthroughAnnotations scenario 1": {
-			configCriService: func(c *criService) {
-				c.config.Runtimes = make(map[string]criconfig.Runtime)
-				c.config.Runtimes["runc"] = criconfig.Runtime{
-					PodAnnotations: []string{"c"},
-				}
-			},
+		"a passthrough annotation should be passed as an OCI annotation": {
+			podAnnotations: []string{"c"},
 			specCheck: func(t *testing.T, spec *runtimespec.Spec) {
 				assert.Equal(t, spec.Annotations["c"], "d")
 			},
 		},
-		// Scenario 2 - containerd config allows only "c" but podSpec defines "c" and "d"
-		// Only annotation "c" will be injected in OCI annotations and "d" should be dropped.
-		"passthroughAnnotations scenario 2": {
+		"a non-passthrough annotation should not be passed as an OCI annotation": {
 			configChange: func(c *runtime.PodSandboxConfig) {
 				c.Annotations["d"] = "e"
 			},
-			configCriService: func(c *criService) {
-				c.config.Runtimes = make(map[string]criconfig.Runtime)
-				c.config.Runtimes["runc"] = criconfig.Runtime{
-					PodAnnotations: []string{"c"},
-				}
-			},
+			podAnnotations: []string{"c"},
 			specCheck: func(t *testing.T, spec *runtimespec.Spec) {
 				assert.Equal(t, spec.Annotations["c"], "d")
 				_, ok := spec.Annotations["d"]
-				assert.Equal(t, ok, false)
+				assert.False(t, ok)
 			},
 		},
-		// Scenario 3 - Let's test some wildcard support
-		// podSpec has following annotations
-		"passthroughAnnotations scenario 3": {
+		"passthrough annotations should support wildcard match": {
 			configChange: func(c *runtime.PodSandboxConfig) {
 				c.Annotations["t.f"] = "j"
 				c.Annotations["z.g"] = "o"
+				c.Annotations["z"] = "o"
 				c.Annotations["y.ca"] = "b"
+				c.Annotations["y"] = "b"
 			},
-			configCriService: func(c *criService) {
-				c.config.Runtimes = make(map[string]criconfig.Runtime)
-				c.config.Runtimes["runc"] = criconfig.Runtime{
-					PodAnnotations: []string{"t*", "z.*", "y.c*"},
-				}
-			},
+			podAnnotations: []string{"t*", "z.*", "y.c*"},
 			specCheck: func(t *testing.T, spec *runtimespec.Spec) {
+				t.Logf("%+v", spec.Annotations)
 				assert.Equal(t, spec.Annotations["t.f"], "j")
 				assert.Equal(t, spec.Annotations["z.g"], "o")
 				assert.Equal(t, spec.Annotations["y.ca"], "b")
+				_, ok := spec.Annotations["y"]
+				assert.False(t, ok)
+				_, ok = spec.Annotations["z"]
+				assert.False(t, ok)
 			},
 		},
 	} {
+		t.Run(desc, func(t *testing.T) {
+			c := newTestCRIService()
+			config, sandboxConfig, imageConfig, specCheck := getCreateContainerTestData()
+			if test.configChange != nil {
+				test.configChange(sandboxConfig)
+			}
 
-		t.Logf("TestCase %q", desc)
-		c := newTestCRIService()
-		config, sandboxConfig, imageConfig, specCheck := getCreateContainerTestData()
-		if test.configChange != nil {
-			test.configChange(sandboxConfig)
-		}
-
-		if test.configCriService != nil {
-			test.configCriService(c)
-		}
-
-		spec, err := c.generateContainerSpec(testID, testSandboxID, testPid,
-			config, sandboxConfig, imageConfig, nil, c.config.Runtimes["runc"].PodAnnotations)
-
-		assert.NoError(t, err)
-		assert.NotNil(t, spec)
-		specCheck(t, testID, testSandboxID, testPid, spec)
-		if test.specCheck != nil {
-			test.specCheck(t, spec)
-		}
+			spec, err := c.generateContainerSpec(testID, testSandboxID, testPid,
+				config, sandboxConfig, imageConfig, nil, test.podAnnotations)
+			assert.NoError(t, err)
+			assert.NotNil(t, spec)
+			specCheck(t, testID, testSandboxID, testPid, spec)
+			if test.specCheck != nil {
+				test.specCheck(t, spec)
+			}
+		})
 	}
 
 }

pkg/server/helpers.go

@@ -634,9 +634,11 @@ func getPassthroughAnnotations(podAnnotations map[string]string,
 	passthroughAnnotations = make(map[string]string)
 
 	for podAnnotationKey, podAnnotationValue := range podAnnotations {
-		for _, r := range runtimePodAnnotations {
-			match, _ := regexp.MatchString(r, podAnnotationKey)
-			if match {
+		for _, pattern := range runtimePodAnnotations {
+			// Use path.Match instead of filepath.Match here.
+			// filepath.Match treated `\\` as path separator
+			// on windows, which is not what we want.
+			if ok, _ := path.Match(pattern, podAnnotationKey); ok {
 				passthroughAnnotations[podAnnotationKey] = podAnnotationValue
 			}
 		}

pkg/server/helpers_test.go

@@ -413,33 +413,105 @@ func TestPassThroughAnnotationsFilter(t *testing.T) {
 		runtimePodAnnotations  []string
 		passthroughAnnotations map[string]string
 	}{
-		// Scenario 1 - containerd config allows "c" and podSpec defines "c" to be passed to OCI
-		"scenario1": {
-			podAnnotations:         map[string]string{"c": "d"},
-			runtimePodAnnotations:  []string{"c"},
-			passthroughAnnotations: map[string]string{"c": "d"},
-		},
-		// Scenario 2 - containerd config allows only "c" but podSpec defines "c" and "d"
-		// Only annotation "c" will be injected in OCI annotations and "d" should be dropped.
-		"scenario2": {
+		"should support direct match": {
 			podAnnotations:         map[string]string{"c": "d", "d": "e"},
 			runtimePodAnnotations:  []string{"c"},
 			passthroughAnnotations: map[string]string{"c": "d"},
 		},
-		// Scenario 3 - Let's test some wildcard support
-		// podSpec has following annotations
-		"scenario3": {
-			podAnnotations: map[string]string{"t.f": "j",
+		"should support wildcard match": {
+			podAnnotations: map[string]string{
+				"t.f":  "j",
 				"z.g":  "o",
-				"y.ca": "b"},
-			runtimePodAnnotations: []string{"t*", "z.*", "y.c*"},
-			passthroughAnnotations: map[string]string{"t.f": "j",
+				"z":    "o",
+				"y.ca": "b",
+				"y":    "b",
+			},
+			runtimePodAnnotations: []string{"*.f", "z*g", "y.c*"},
+			passthroughAnnotations: map[string]string{
+				"t.f":  "j",
 				"z.g":  "o",
-				"y.ca": "b"},
+				"y.ca": "b",
+			},
+		},
+		"should support wildcard match all": {
+			podAnnotations: map[string]string{
+				"t.f":  "j",
+				"z.g":  "o",
+				"z":    "o",
+				"y.ca": "b",
+				"y":    "b",
+			},
+			runtimePodAnnotations: []string{"*"},
+			passthroughAnnotations: map[string]string{
+				"t.f":  "j",
+				"z.g":  "o",
+				"z":    "o",
+				"y.ca": "b",
+				"y":    "b",
+			},
+		},
+		"should support match including path separator": {
+			podAnnotations: map[string]string{
+				"matchend.com/end":    "1",
+				"matchend.com/end1":   "2",
+				"matchend.com/1end":   "3",
+				"matchmid.com/mid":    "4",
+				"matchmid.com/mi1d":   "5",
+				"matchmid.com/mid1":   "6",
+				"matchhead.com/head":  "7",
+				"matchhead.com/1head": "8",
+				"matchhead.com/head1": "9",
+				"matchall.com/abc":    "10",
+				"matchall.com/def":    "11",
+				"end/matchend":        "12",
+				"end1/matchend":       "13",
+				"1end/matchend":       "14",
+				"mid/matchmid":        "15",
+				"mi1d/matchmid":       "16",
+				"mid1/matchmid":       "17",
+				"head/matchhead":      "18",
+				"1head/matchhead":     "19",
+				"head1/matchhead":     "20",
+				"abc/matchall":        "21",
+				"def/matchall":        "22",
+				"match1/match2":       "23",
+				"nomatch/nomatch":     "24",
+			},
+			runtimePodAnnotations: []string{
+				"matchend.com/end*",
+				"matchmid.com/mi*d",
+				"matchhead.com/*head",
+				"matchall.com/*",
+				"end*/matchend",
+				"mi*d/matchmid",
+				"*head/matchhead",
+				"*/matchall",
+				"match*/match*",
+			},
+			passthroughAnnotations: map[string]string{
+				"matchend.com/end":    "1",
+				"matchend.com/end1":   "2",
+				"matchmid.com/mid":    "4",
+				"matchmid.com/mi1d":   "5",
+				"matchhead.com/head":  "7",
+				"matchhead.com/1head": "8",
+				"matchall.com/abc":    "10",
+				"matchall.com/def":    "11",
+				"end/matchend":        "12",
+				"end1/matchend":       "13",
+				"mid/matchmid":        "15",
+				"mi1d/matchmid":       "16",
+				"head/matchhead":      "18",
+				"1head/matchhead":     "19",
+				"abc/matchall":        "21",
+				"def/matchall":        "22",
+				"match1/match2":       "23",
+			},
 		},
 	} {
-		t.Logf("TestCase %q", desc)
-		passthroughAnnotations := getPassthroughAnnotations(test.podAnnotations, test.runtimePodAnnotations)
-		assert.Equal(t, test.passthroughAnnotations, passthroughAnnotations)
+		t.Run(desc, func(t *testing.T) {
+			passthroughAnnotations := getPassthroughAnnotations(test.podAnnotations, test.runtimePodAnnotations)
+			assert.Equal(t, test.passthroughAnnotations, passthroughAnnotations)
+		})
 	}
 }

pkg/server/sandbox_run_test.go

@@ -86,8 +86,8 @@ func TestGenerateSandboxContainerSpec(t *testing.T) {
 	testID := "test-id"
 	nsPath := "test-cni"
 	for desc, test := range map[string]struct {
-		configCriService  func(*criService)
 		configChange      func(*runtime.PodSandboxConfig)
+		podAnnotations    []string
 		imageConfigChange func(*imagespec.ImageConfig)
 		specCheck         func(*testing.T, *runtimespec.Spec)
 		expectErr         bool
@@ -158,54 +158,40 @@ func TestGenerateSandboxContainerSpec(t *testing.T) {
 				assert.Contains(t, spec.Process.User.AdditionalGids, uint32(2222))
 			},
 		},
-		// Scenario 1 - containerd config allows "c" and podSpec defines "c" to be passed to OCI
-		"passthroughAnnotations scenario 1": {
-			configCriService: func(c *criService) {
-				c.config.Runtimes = make(map[string]criconfig.Runtime)
-				c.config.Runtimes["runc"] = criconfig.Runtime{
-					PodAnnotations: []string{"c"},
-				}
-			}, //assert.Equal(t, passthroughAnnotations["c"], "d")
+		"a passthrough annotation should be passed as an OCI annotation": {
+			podAnnotations: []string{"c"},
 			specCheck: func(t *testing.T, spec *runtimespec.Spec) {
 				assert.Equal(t, spec.Annotations["c"], "d")
 			},
 		},
-		// Scenario 2 - containerd config allows only "c" but podSpec defines "c" and "d"
-		// Only annotation "c" will be injected in OCI annotations and "d" should be dropped.
-		"passthroughAnnotations scenario 2": {
+		"a non-passthrough annotation should not be passed as an OCI annotation": {
 			configChange: func(c *runtime.PodSandboxConfig) {
 				c.Annotations["d"] = "e"
 			},
-			configCriService: func(c *criService) {
-				c.config.Runtimes = make(map[string]criconfig.Runtime)
-				c.config.Runtimes["runc"] = criconfig.Runtime{
-					PodAnnotations: []string{"c"},
-				}
-			},
+			podAnnotations: []string{"c"},
 			specCheck: func(t *testing.T, spec *runtimespec.Spec) {
 				assert.Equal(t, spec.Annotations["c"], "d")
 				_, ok := spec.Annotations["d"]
-				assert.Equal(t, ok, false)
+				assert.False(t, ok)
 			},
 		},
-		// Scenario 3 - Let's test some wildcard support
-		// podSpec has following annotations
-		"passthroughAnnotations scenario 3": {
+		"passthrough annotations should support wildcard match": {
 			configChange: func(c *runtime.PodSandboxConfig) {
 				c.Annotations["t.f"] = "j"
 				c.Annotations["z.g"] = "o"
+				c.Annotations["z"] = "o"
 				c.Annotations["y.ca"] = "b"
+				c.Annotations["y"] = "b"
 			},
-			configCriService: func(c *criService) {
-				c.config.Runtimes = make(map[string]criconfig.Runtime)
-				c.config.Runtimes["runc"] = criconfig.Runtime{
-					PodAnnotations: []string{"t*", "z.*", "y.c*"},
-				}
-			},
+			podAnnotations: []string{"t*", "z.*", "y.c*"},
 			specCheck: func(t *testing.T, spec *runtimespec.Spec) {
 				assert.Equal(t, spec.Annotations["t.f"], "j")
 				assert.Equal(t, spec.Annotations["z.g"], "o")
 				assert.Equal(t, spec.Annotations["y.ca"], "b")
+				_, ok := spec.Annotations["y"]
+				assert.False(t, ok)
+				_, ok = spec.Annotations["z"]
+				assert.False(t, ok)
 			},
 		},
 	} {
@@ -216,15 +202,11 @@ func TestGenerateSandboxContainerSpec(t *testing.T) {
 			test.configChange(config)
 		}
 
-		if test.configCriService != nil {
-			test.configCriService(c)
-		}
-
 		if test.imageConfigChange != nil {
 			test.imageConfigChange(imageConfig)
 		}
 		spec, err := c.generateSandboxContainerSpec(testID, config, imageConfig, nsPath,
-			c.config.Runtimes["runc"].PodAnnotations)
+			test.podAnnotations)
 		if test.expectErr {
 			assert.Error(t, err)
 			assert.Nil(t, spec)
@@ -412,7 +394,6 @@ options timeout:1
 		c.os.(*ostesting.FakeOS).HostnameFn = func() (string, error) {
 			return realhostname, nil
 		}
-
 		cfg := &runtime.PodSandboxConfig{
 			Hostname:  test.hostname,
 			DnsConfig: test.dnsConfig,