From fb04303bbe2ccc273c45f5f67e3c709cd1b3ae6d Mon Sep 17 00:00:00 2001 From: Gabe Conradi Date: Fri, 31 Jan 2020 12:51:40 -0500 Subject: [PATCH 1/8] rejigger how initContainers are mutated to not forget VolumeMounts --- internal/pkg/config/config_test.go | 11 ++ pkg/server/webhook.go | 52 ++++---- pkg/server/webhook_test.go | 2 + .../admissioncontrol/patch/volumetest.json | 122 ++++++++++++++++++ .../admissioncontrol/request/volumetest.yaml | 25 ++++ test/fixtures/sidecars/maxmind.yaml | 21 +++ 6 files changed, 210 insertions(+), 23 deletions(-) create mode 100644 test/fixtures/k8s/admissioncontrol/patch/volumetest.json create mode 100644 test/fixtures/k8s/admissioncontrol/request/volumetest.yaml create mode 100644 test/fixtures/sidecars/maxmind.yaml diff --git a/internal/pkg/config/config_test.go b/internal/pkg/config/config_test.go index 21ea7af..b7cab8f 100644 --- a/internal/pkg/config/config_test.go +++ b/internal/pkg/config/config_test.go @@ -176,6 +176,17 @@ var ( InitContainerCount: 0, ServiceAccount: "someaccount", }, + "maxmind": testhelper.ConfigExpectation{ + Name: "maxminddb", + Version: "latest", + Path: fixtureSidecarsDir + "/maxmind.yaml", + EnvCount: 0, + ContainerCount: 1, + VolumeCount: 1, + VolumeMountCount: 1, + HostAliasCount: 0, + InitContainerCount: 1, + }, } ) diff --git a/pkg/server/webhook.go b/pkg/server/webhook.go index a728821..95bf721 100644 --- a/pkg/server/webhook.go +++ b/pkg/server/webhook.go @@ -175,13 +175,13 @@ func (whsvr *WebhookServer) getSidecarConfigurationRequested(ignoredList []strin return ic.FullName(), nil } -func setEnvironment(target []corev1.Container, addedEnv []corev1.EnvVar) (patch []patchOperation) { +func setEnvironment(target []corev1.Container, addedEnv []corev1.EnvVar, basePath string) (patch []patchOperation) { var value interface{} for containerIndex, container := range target { // for each container in the spec, determine if we want to patch with any env vars first := len(container.Env) == 0 for _, add := range addedEnv { - path := fmt.Sprintf("/spec/containers/%d/env", containerIndex) + path := fmt.Sprintf("%s/%d/env", basePath, containerIndex) hasKey := false // make sure we dont override any existing env vars; we only add, dont replace for _, origEnv := range container.Env { @@ -252,13 +252,13 @@ func addVolumes(target, added []corev1.Volume, basePath string) (patch []patchOp return patch } -func addVolumeMounts(target []corev1.Container, addedVolumeMounts []corev1.VolumeMount) (patch []patchOperation) { +func addVolumeMounts(target []corev1.Container, addedVolumeMounts []corev1.VolumeMount, basePath string) (patch []patchOperation) { var value interface{} for containerIndex, container := range target { // for each container in the spec, determine if we want to patch with any volume mounts first := len(container.VolumeMounts) == 0 for _, add := range addedVolumeMounts { - path := fmt.Sprintf("/spec/containers/%d/volumeMounts", containerIndex) + path := fmt.Sprintf("%s/%d/volumeMounts", basePath, containerIndex) hasKey := false // make sure we dont override any existing volume mounts; we only add, dont replace for _, origVolumeMount := range container.VolumeMounts { @@ -425,27 +425,33 @@ func createPatch(pod *corev1.Pod, inj *config.InjectionConfig, annotations map[s patch = append(patch, setServiceAccount(pod.Spec.InitContainers, pod.Spec.Containers, inj.ServiceAccountName, "/spec")...) } - // first, make sure any injected containers in our config get the EnvVars and VolumeMounts injected - // this mutates inj.Containers with our environment vars - mutatedInjectedContainers := mergeEnvVars(inj.Environment, inj.Containers) - mutatedInjectedContainers = mergeVolumeMounts(inj.VolumeMounts, mutatedInjectedContainers) - - // next, make sure any injected init containers in our config get the EnvVars and VolumeMounts injected - // this mutates inj.InitContainers with our environment vars - mutatedInjectedInitContainers := mergeEnvVars(inj.Environment, inj.InitContainers) - mutatedInjectedInitContainers = mergeVolumeMounts(inj.VolumeMounts, mutatedInjectedInitContainers) - - // next, patch containers with our injected containers - patch = append(patch, addContainers(pod.Spec.Containers, mutatedInjectedContainers, "/spec/containers")...) + { // initcontainer injections + // patch all existing InitContainers with the VolumeMounts+EnvVars, and add injected initcontainers + patch = append(patch, setEnvironment(pod.Spec.InitContainers, inj.Environment, "/spec/initContainers")...) + patch = append(patch, addVolumeMounts(pod.Spec.InitContainers, inj.VolumeMounts, "/spec/initContainers")...) + // next, make sure any injected init containers in our config get the EnvVars and VolumeMounts injected + // this mutates inj.InitContainers with our environment vars + mutatedInjectedInitContainers := mergeEnvVars(inj.Environment, inj.InitContainers) + mutatedInjectedInitContainers = mergeVolumeMounts(inj.VolumeMounts, mutatedInjectedInitContainers) + patch = append(patch, addContainers(pod.Spec.InitContainers, mutatedInjectedInitContainers, "/spec/initContainers")...) + } - // now, patch all existing containers with the env vars and volume mounts - patch = append(patch, setEnvironment(pod.Spec.Containers, inj.Environment)...) - patch = append(patch, addVolumeMounts(pod.Spec.Containers, inj.VolumeMounts)...) + { // container injections + // now, patch all existing containers with the env vars and volume mounts, and add injected containers + patch = append(patch, setEnvironment(pod.Spec.Containers, inj.Environment, "/spec/containers")...) + patch = append(patch, addVolumeMounts(pod.Spec.Containers, inj.VolumeMounts, "/spec/containers")...) + // first, make sure any injected containers in our config get the EnvVars and VolumeMounts injected + // this mutates inj.Containers with our environment vars + mutatedInjectedContainers := mergeEnvVars(inj.Environment, inj.Containers) + mutatedInjectedContainers = mergeVolumeMounts(inj.VolumeMounts, mutatedInjectedContainers) + patch = append(patch, addContainers(pod.Spec.Containers, mutatedInjectedContainers, "/spec/containers")...) + } - // now, add initContainers, hostAliases and volumes - patch = append(patch, addContainers(pod.Spec.InitContainers, mutatedInjectedInitContainers, "/spec/initContainers")...) - patch = append(patch, addHostAliases(pod.Spec.HostAliases, inj.HostAliases, "/spec/hostAliases")...) - patch = append(patch, addVolumes(pod.Spec.Volumes, inj.Volumes, "/spec/volumes")...) + { // pod level mutations + // now, add hostAliases and volumes + patch = append(patch, addHostAliases(pod.Spec.HostAliases, inj.HostAliases, "/spec/hostAliases")...) + patch = append(patch, addVolumes(pod.Spec.Volumes, inj.Volumes, "/spec/volumes")...) + } // last but not least, set annotations patch = append(patch, updateAnnotations(pod.Annotations, annotations)...) diff --git a/pkg/server/webhook_test.go b/pkg/server/webhook_test.go index 083c883..b5ddca5 100644 --- a/pkg/server/webhook_test.go +++ b/pkg/server/webhook_test.go @@ -64,6 +64,7 @@ var ( {name: "service-account-already-set", allowed: true, patchExpected: true}, {name: "service-account-set-default", allowed: true, patchExpected: true}, {name: "service-account-default-token", allowed: true, patchExpected: true}, + {name: "volumetest", allowed: true, patchExpected: true}, } sidecarConfigs, _ = filepath.Glob(path.Join(sidecars, "*.yaml")) expectedNumInjectionConfigs = len(sidecarConfigs) @@ -179,6 +180,7 @@ func TestMutation(t *testing.T) { } difference, diffString := jsondiff.Compare(expectedPatchData, resPatch, &jsondiffopts) if difference != jsondiff.FullMatch { + t.Errorf("Actual patch JSON: %s", string(resPatch)) t.Fatalf("received AdmissionResponse.patch field differed from expected with %s (%s) (actual on left, expected on right):\n%s", resPatchFile, difference.String(), diffString) } } diff --git a/test/fixtures/k8s/admissioncontrol/patch/volumetest.json b/test/fixtures/k8s/admissioncontrol/patch/volumetest.json new file mode 100644 index 0000000..363531d --- /dev/null +++ b/test/fixtures/k8s/admissioncontrol/patch/volumetest.json @@ -0,0 +1,122 @@ +[ + { + "op" : "add", + "path" : "/spec/initContainers/0/env", + "value" : [ + { + "name" : "EVERYWHERE", + "value" : "injected in all containers" + } + ] + }, + { + "op" : "add", + "path" : "/spec/initContainers/1/env", + "value" : [ + { + "name" : "EVERYWHERE", + "value" : "injected in all containers" + } + ] + }, + { + "op" : "add", + "path" : "/spec/initContainers/0/volumeMounts", + "value" : [ + { + "mountPath" : "/usr/share/GeoIP", + "name" : "maxminddb" + } + ] + }, + { + "op" : "add", + "path" : "/spec/initContainers/-", + "value" : { + "command" : [ + "/bin/init.sh" + ], + "env" : [ + { + "name" : "EVERYWHERE", + "value" : "already present" + } + ], + "image" : "maxmindimage", + "name" : "maxminddb-init", + "resources" : {}, + "volumeMounts" : [ + { + "mountPath" : "/usr/share/GeoIP", + "name" : "maxminddb" + } + ] + } + }, + { + "op" : "add", + "path" : "/spec/containers/0/env", + "value" : [ + { + "name" : "EVERYWHERE", + "value" : "injected in all containers" + } + ] + }, + { + "op" : "add", + "path" : "/spec/containers/1/env", + "value" : [ + { + "name" : "EVERYWHERE", + "value" : "injected in all containers" + } + ] + }, + { + "op" : "add", + "path" : "/spec/containers/1/volumeMounts", + "value" : [ + { + "mountPath" : "/usr/share/GeoIP", + "name" : "maxminddb" + } + ] + }, + { + "op" : "add", + "path" : "/spec/containers/-", + "value" : { + "env" : [ + { + "name" : "EVERYWHERE", + "value" : "injected in all containers" + } + ], + "image" : "maxmindimage", + "name" : "maxminddb", + "resources" : {}, + "volumeMounts" : [ + { + "mountPath" : "/usr/share/GeoIP", + "name" : "maxminddb" + } + ] + } + }, + { + "op" : "add", + "path" : "/spec/volumes", + "value" : [ + { + "emptyDir" : {}, + "name" : "maxminddb" + } + ] + }, + { + "op" : "add", + "path" : "/metadata/annotations/injector.unittest.com~1status", + "value" : "injected" + } +] diff --git a/test/fixtures/k8s/admissioncontrol/request/volumetest.yaml b/test/fixtures/k8s/admissioncontrol/request/volumetest.yaml new file mode 100644 index 0000000..767f269 --- /dev/null +++ b/test/fixtures/k8s/admissioncontrol/request/volumetest.yaml @@ -0,0 +1,25 @@ +--- +# this is an AdmissionRequest object +# https://godoc.org/k8s.io/api/admission/v1beta1#AdmissionRequest +object: + metadata: + annotations: + injector.unittest.com/request: "maxmind" + spec: + initContainers: + - image: maxmindimage + # init2 should get a volumeMount injected + name: init2 + command: ["/bin/init.sh"] + - image: maxmindimage + name: already-has-volumemount + command: ["/bin/init.sh"] + volumeMounts: + - name: maxminddb # this should NOT get duplicated + mountPath: /usr/share/GeoIP + containers: + - name: ctr-with-volumemount + volumeMounts: + - name: maxminddb # this should NOT get duplicated + mountPath: /usr/share/GeoIP + - name: ctr-without-volumemount diff --git a/test/fixtures/sidecars/maxmind.yaml b/test/fixtures/sidecars/maxmind.yaml new file mode 100644 index 0000000..1696fbe --- /dev/null +++ b/test/fixtures/sidecars/maxmind.yaml @@ -0,0 +1,21 @@ +--- +name: maxmind +env: + - name: EVERYWHERE + value: "injected in all containers" +initContainers: + - image: maxmindimage + name: maxminddb-init + command: ["/bin/init.sh"] + env: + - name: EVERYWHERE + value: already present +containers: + - image: maxmindimage + name: maxminddb +volumes: + - name: maxminddb + emptyDir: {} +volumeMounts: + - name: maxminddb + mountPath: /usr/share/GeoIP From 1f2de614a1c5763ca497ae959b9f6a6fa45412f5 Mon Sep 17 00:00:00 2001 From: Gabe Conradi Date: Fri, 31 Jan 2020 13:54:05 -0500 Subject: [PATCH 2/8] run on more go versions --- .travis.yml | 5 ++++- go.mod | 2 +- go.sum | 11 +++++++++++ internal/pkg/config/config_test.go | 2 +- 4 files changed, 17 insertions(+), 3 deletions(-) diff --git a/.travis.yml b/.travis.yml index 299b20f..ef79f9b 100644 --- a/.travis.yml +++ b/.travis.yml @@ -2,7 +2,10 @@ language: go go: - "1.x" - - "1.11.5" + - "1.11.x" + - "1.12.x" + - "1.13.x" + - "1.14.x" env: - GO111MODULE=on diff --git a/go.mod b/go.mod index f56b51b..644c1f5 100644 --- a/go.mod +++ b/go.mod @@ -27,7 +27,7 @@ require ( github.com/prometheus/procfs v0.0.0-20181005140218-185b4288413d // indirect github.com/spf13/pflag v1.0.3 // indirect github.com/stretchr/testify v1.4.0 // indirect - golang.org/x/lint v0.0.0-20191125180803-fdd1cda4f05f // indirect + golang.org/x/lint v0.0.0-20200130185559-910be7a94367 // indirect golang.org/x/oauth2 v0.0.0-20181106182150-f42d05182288 // indirect golang.org/x/time v0.0.0-20181108054448-85acf8d2951c // indirect google.golang.org/appengine v1.3.0 // indirect diff --git a/go.sum b/go.sum index 06a8255..338c83e 100644 --- a/go.sum +++ b/go.sum @@ -60,15 +60,21 @@ golang.org/x/crypto v0.0.0-20181106171534-e4dc69e5b2fd h1:VtIkGDhk0ph3t+THbvXHfM golang.org/x/crypto v0.0.0-20181106171534-e4dc69e5b2fd/go.mod h1:6SG95UA2DQfeDnfUPMdvaQW0Q7yPrPDi9nlGo2tz2b4= golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2 h1:VklqNMn3ovrHsnt90PveolxSbWFaJdECFbxSq0Mqo2M= golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w= +golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550 h1:ObdrDkeb4kJdCP557AjRjq69pTHfNouLtWZG7j9rPN8= +golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI= golang.org/x/lint v0.0.0-20190930215403-16217165b5de h1:5hukYrvBGR8/eNkX5mdUezrA6JiaEZDtJb9Ei+1LlBs= golang.org/x/lint v0.0.0-20190930215403-16217165b5de/go.mod h1:6SW0HCj/g11FgYtHlgUYUwCkIfeOF89ocIRzGO/8vkc= golang.org/x/lint v0.0.0-20191125180803-fdd1cda4f05f h1:J5lckAjkw6qYlOZNj90mLYNTEKDvWeuc1yieZ8qUzUE= golang.org/x/lint v0.0.0-20191125180803-fdd1cda4f05f/go.mod h1:5qLYkcX4OjUUV8bRuDixDT3tpyyb+LUpUlRWLxfhWrs= +golang.org/x/lint v0.0.0-20200130185559-910be7a94367 h1:0IiAsCRByjO2QjX7ZPkw5oU9x+n1YqRL802rjC0c3Aw= +golang.org/x/lint v0.0.0-20200130185559-910be7a94367/go.mod h1:3xt1FjdF8hUf6vQPIChWIBhFzV8gjjsPE/fR3IyQdNY= +golang.org/x/mod v0.1.1-0.20191105210325-c90efee705ee/go.mod h1:QqPTAvyqsEbceGzBzNggFXnrqF1CaUcvgkdR5Ot7KZg= golang.org/x/net v0.0.0-20180724234803-3673e40ba225/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= golang.org/x/net v0.0.0-20181108082009-03003ca0c849 h1:FSqE2GGG7wzsYUsWiQ8MZrvEd1EOyU3NCF0AW3Wtltg= golang.org/x/net v0.0.0-20181108082009-03003ca0c849/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= golang.org/x/net v0.0.0-20190311183353-d8887717615a h1:oWX7TPOiFAMXLq8o0ikBYfCJVlRHBcsciT5bXOrH628= golang.org/x/net v0.0.0-20190311183353-d8887717615a/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg= +golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg= golang.org/x/net v0.0.0-20190620200207-3b0461eec859 h1:R/3boaszxrf1GEUWTVDzSKVwLmSJpwZ1yqXm8j0v2QI= golang.org/x/net v0.0.0-20190620200207-3b0461eec859/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= golang.org/x/oauth2 v0.0.0-20181106182150-f42d05182288 h1:JIqe8uIcRBHXDQVvZtHwp80ai3Lw3IJAeJEs55Dc1W0= @@ -78,6 +84,8 @@ golang.org/x/sys v0.0.0-20181107165924-66b7b1311ac8 h1:YoY1wS6JYVRpIfFngRf2HHo9R golang.org/x/sys v0.0.0-20181107165924-66b7b1311ac8/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a h1:1BGLXjeY4akVXGgbC9HugT3Jv3hCI0z56oJR5vAMgBU= golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= +golang.org/x/sys v0.0.0-20190412213103-97732733099d h1:+R4KGOnez64A81RvjARKc4UT5/tI9ujCIVX+P5KiHuI= +golang.org/x/sys v0.0.0-20190412213103-97732733099d/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/text v0.3.0 h1:g61tztE5qeGQ89tm6NTjjM9VPIm088od1l6aSorWRWg= golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/time v0.0.0-20181108054448-85acf8d2951c h1:fqgJT0MGcGpPgpWU7VRdRjuArfcOvC4AoJmILihzhDg= @@ -86,7 +94,10 @@ golang.org/x/tools v0.0.0-20190311212946-11955173bddd h1:/e+gpKk9r3dJobndpTytxS2 golang.org/x/tools v0.0.0-20190311212946-11955173bddd/go.mod h1:LCzVGOaR6xXOjkQ3onu1FJEFr0SW1gC7cKk1uF8kGRs= golang.org/x/tools v0.0.0-20191125144606-a911d9008d1f h1:kDxGY2VmgABOe55qheT/TFqUMtcTHnomIPS1iv3G4Ms= golang.org/x/tools v0.0.0-20191125144606-a911d9008d1f/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo= +golang.org/x/tools v0.0.0-20200130002326-2f3ba24bd6e7 h1:EBZoQjiKKPaLbPrbpssUfuHtwM6KV/vb4U85g/cigFY= +golang.org/x/tools v0.0.0-20200130002326-2f3ba24bd6e7/go.mod h1:TB2adYChydJhpapKDTa4BR/hXlZSLoq2Wpct/0txZ28= golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= +golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= google.golang.org/appengine v1.3.0 h1:FBSsiFRMz3LBeXIomRnVzrQwSDj4ibvcRexLG0LZGQk= google.golang.org/appengine v1.3.0/go.mod h1:xpcJRLb0r/rnEns0DIKYYv+WjYCduHsrkT7/EB5XEv4= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= diff --git a/internal/pkg/config/config_test.go b/internal/pkg/config/config_test.go index b7cab8f..3fbed33 100644 --- a/internal/pkg/config/config_test.go +++ b/internal/pkg/config/config_test.go @@ -177,7 +177,7 @@ var ( ServiceAccount: "someaccount", }, "maxmind": testhelper.ConfigExpectation{ - Name: "maxminddb", + Name: "maxmind", Version: "latest", Path: fixtureSidecarsDir + "/maxmind.yaml", EnvCount: 0, From 5d2f4c0b1ec5a36776e052d92fb9aa83766c3c66 Mon Sep 17 00:00:00 2001 From: Gabe Conradi Date: Mon, 3 Feb 2020 11:04:01 -0500 Subject: [PATCH 3/8] fixing test --- internal/pkg/config/config_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/pkg/config/config_test.go b/internal/pkg/config/config_test.go index 3fbed33..c408b3b 100644 --- a/internal/pkg/config/config_test.go +++ b/internal/pkg/config/config_test.go @@ -180,7 +180,7 @@ var ( Name: "maxmind", Version: "latest", Path: fixtureSidecarsDir + "/maxmind.yaml", - EnvCount: 0, + EnvCount: 1, ContainerCount: 1, VolumeCount: 1, VolumeMountCount: 1, From c169402e84d550e3dc4328cdbc89b12aa7728a85 Mon Sep 17 00:00:00 2001 From: Gabe Conradi Date: Mon, 3 Feb 2020 11:28:25 -0500 Subject: [PATCH 4/8] whoopsies, 1.14.x isnt a go release. got a bit too spicy for my britches --- .travis.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index ef79f9b..7017051 100644 --- a/.travis.yml +++ b/.travis.yml @@ -5,7 +5,6 @@ go: - "1.11.x" - "1.12.x" - "1.13.x" - - "1.14.x" env: - GO111MODULE=on From baf478bb2ac69795779bd7236f237f32d4269575 Mon Sep 17 00:00:00 2001 From: Gabe Conradi Date: Mon, 3 Feb 2020 14:14:56 -0500 Subject: [PATCH 5/8] adding test for duplicated volume --- pkg/server/webhook_test.go | 1 + .../patch/volumetest-existingvolume.json | 112 ++++++++++++++++++ .../request/volumetest-existingvolume.yaml | 30 +++++ 3 files changed, 143 insertions(+) create mode 100644 test/fixtures/k8s/admissioncontrol/patch/volumetest-existingvolume.json create mode 100644 test/fixtures/k8s/admissioncontrol/request/volumetest-existingvolume.yaml diff --git a/pkg/server/webhook_test.go b/pkg/server/webhook_test.go index b5ddca5..4f0d58c 100644 --- a/pkg/server/webhook_test.go +++ b/pkg/server/webhook_test.go @@ -65,6 +65,7 @@ var ( {name: "service-account-set-default", allowed: true, patchExpected: true}, {name: "service-account-default-token", allowed: true, patchExpected: true}, {name: "volumetest", allowed: true, patchExpected: true}, + {name: "volumetest-existingvolume", allowed: true, patchExpected: true}, } sidecarConfigs, _ = filepath.Glob(path.Join(sidecars, "*.yaml")) expectedNumInjectionConfigs = len(sidecarConfigs) diff --git a/test/fixtures/k8s/admissioncontrol/patch/volumetest-existingvolume.json b/test/fixtures/k8s/admissioncontrol/patch/volumetest-existingvolume.json new file mode 100644 index 0000000..6ded0ed --- /dev/null +++ b/test/fixtures/k8s/admissioncontrol/patch/volumetest-existingvolume.json @@ -0,0 +1,112 @@ +[ + { + "op" : "add", + "path" : "/spec/initContainers/0/env", + "value" : [ + { + "name" : "EVERYWHERE", + "value" : "injected in all containers" + } + ] + }, + { + "op" : "add", + "path" : "/spec/initContainers/1/env", + "value" : [ + { + "name" : "EVERYWHERE", + "value" : "injected in all containers" + } + ] + }, + { + "op" : "add", + "path" : "/spec/initContainers/0/volumeMounts", + "value" : [ + { + "mountPath" : "/usr/share/GeoIP", + "name" : "maxminddb" + } + ] + }, + { + "op" : "add", + "path" : "/spec/initContainers/-", + "value" : { + "command" : [ + "/bin/init.sh" + ], + "env" : [ + { + "name" : "EVERYWHERE", + "value" : "already present" + } + ], + "image" : "maxmindimage", + "name" : "maxminddb-init", + "resources" : {}, + "volumeMounts" : [ + { + "mountPath" : "/usr/share/GeoIP", + "name" : "maxminddb" + } + ] + } + }, + { + "op" : "add", + "path" : "/spec/containers/0/env", + "value" : [ + { + "name" : "EVERYWHERE", + "value" : "injected in all containers" + } + ] + }, + { + "op" : "add", + "path" : "/spec/containers/1/env", + "value" : [ + { + "name" : "EVERYWHERE", + "value" : "injected in all containers" + } + ] + }, + { + "op" : "add", + "path" : "/spec/containers/1/volumeMounts", + "value" : [ + { + "mountPath" : "/usr/share/GeoIP", + "name" : "maxminddb" + } + ] + }, + { + "op" : "add", + "path" : "/spec/containers/-", + "value" : { + "env" : [ + { + "name" : "EVERYWHERE", + "value" : "injected in all containers" + } + ], + "image" : "maxmindimage", + "name" : "maxminddb", + "resources" : {}, + "volumeMounts" : [ + { + "mountPath" : "/usr/share/GeoIP", + "name" : "maxminddb" + } + ] + } + }, + { + "op" : "add", + "path" : "/metadata/annotations/injector.unittest.com~1status", + "value" : "injected" + } +] diff --git a/test/fixtures/k8s/admissioncontrol/request/volumetest-existingvolume.yaml b/test/fixtures/k8s/admissioncontrol/request/volumetest-existingvolume.yaml new file mode 100644 index 0000000..2ab2c9f --- /dev/null +++ b/test/fixtures/k8s/admissioncontrol/request/volumetest-existingvolume.yaml @@ -0,0 +1,30 @@ +--- +# this is an AdmissionRequest object +# https://godoc.org/k8s.io/api/admission/v1beta1#AdmissionRequest +object: + metadata: + annotations: + injector.unittest.com/request: "maxmind" + spec: + initContainers: + - image: maxmindimage + # init2 should get a volumeMount injected + name: init2 + command: ["/bin/init.sh"] + - image: maxmindimage + name: already-has-volumemount + command: ["/bin/init.sh"] + volumeMounts: + - name: maxminddb # this should NOT get duplicated + mountPath: /usr/share/GeoIP + containers: + - name: ctr-with-volumemount + volumeMounts: + - name: maxminddb # this should NOT get duplicated + mountPath: /usr/share/GeoIP + - name: ctr-without-volumemount + volumes: + # do not redefine the volume here + - name: maxminddb + emptyDir: {} + From cbd34937aafb038952f2aa764e6b2221afd3c190 Mon Sep 17 00:00:00 2001 From: Gabe Conradi Date: Mon, 3 Feb 2020 14:54:17 -0500 Subject: [PATCH 6/8] identifying trap for volume duplication --- pkg/server/webhook.go | 49 +++++++++++-------- .../patch/sidecar-test-1.json | 16 +++--- .../patch/volumetest-existingvolume.json | 8 +++ .../admissioncontrol/patch/volumetest.json | 22 ++++++--- test/fixtures/sidecars/maxmind.yaml | 5 +- 5 files changed, 60 insertions(+), 40 deletions(-) diff --git a/pkg/server/webhook.go b/pkg/server/webhook.go index 95bf721..995715e 100644 --- a/pkg/server/webhook.go +++ b/pkg/server/webhook.go @@ -231,21 +231,26 @@ func addContainers(target, added []corev1.Container, basePath string) (patch []p return patch } -func addVolumes(target, added []corev1.Volume, basePath string) (patch []patchOperation) { - first := len(target) == 0 - var value interface{} +func addVolumes(existing, added []corev1.Volume, basePath string) (patch []patchOperation) { + hasVolume := func(existing []corev1.Volume, add corev1.Volume) bool { + for _, v := range existing { + // if any of the existing volumes have the same name as test.Name, skip + // injecting it + if v.Name == add.Name { + return true + } + } + return false + } for _, add := range added { - value = add - path := basePath - if first { - first = false - value = []corev1.Volume{add} - } else { - path = path + "/-" + value := add + + if hasVolume(existing, add) { + continue } patch = append(patch, patchOperation{ Op: "add", - Path: path, + Path: basePath + "/-", Value: value, }) } @@ -348,21 +353,25 @@ func setServiceAccount(initContainers []corev1.Container, containers []corev1.Co // this does _not_ return patches; this is intended to be used only on containers defined // in the injection config, so the resources do not exist yet in the k8s api (thus no patch needed) func mergeEnvVars(envs []corev1.EnvVar, containers []corev1.Container) []corev1.Container { + hasEnvVar := func(existing []corev1.EnvVar, add corev1.EnvVar) bool { + for _, v := range existing { + // if any of the existing volumes have the same name as test.Name, skip + // injecting it + if v.Name == add.Name { + return true + } + } + return false + } mutatedContainers := []corev1.Container{} for _, c := range containers { for _, newEnv := range envs { // check each container for each env var by name. // if the container has a matching name, dont override! - skip := false - for _, origEnv := range c.Env { - if origEnv.Name == newEnv.Name { - skip = true - break - } - } - if !skip { - c.Env = append(c.Env, newEnv) + if hasEnvVar(c.Env, newEnv) { + continue } + c.Env = append(c.Env, newEnv) } mutatedContainers = append(mutatedContainers, c) } diff --git a/test/fixtures/k8s/admissioncontrol/patch/sidecar-test-1.json b/test/fixtures/k8s/admissioncontrol/patch/sidecar-test-1.json index 4077135..df45e8c 100644 --- a/test/fixtures/k8s/admissioncontrol/patch/sidecar-test-1.json +++ b/test/fixtures/k8s/admissioncontrol/patch/sidecar-test-1.json @@ -58,15 +58,13 @@ }, { "op" : "add", - "path" : "/spec/volumes", - "value" : [ - { - "configMap" : { - "name" : "nginx-configmap" - }, - "name" : "nginx-conf" - } - ] + "path" : "/spec/volumes/-", + "value" : { + "configMap" : { + "name" : "nginx-configmap" + }, + "name" : "nginx-conf" + } }, { "op" : "add", diff --git a/test/fixtures/k8s/admissioncontrol/patch/volumetest-existingvolume.json b/test/fixtures/k8s/admissioncontrol/patch/volumetest-existingvolume.json index 6ded0ed..eeddade 100644 --- a/test/fixtures/k8s/admissioncontrol/patch/volumetest-existingvolume.json +++ b/test/fixtures/k8s/admissioncontrol/patch/volumetest-existingvolume.json @@ -104,6 +104,14 @@ ] } }, + { + "op" : "add", + "path" : "/spec/volumes/-", + "value" : { + "emptyDir" : {}, + "name" : "hell" + } + }, { "op" : "add", "path" : "/metadata/annotations/injector.unittest.com~1status", diff --git a/test/fixtures/k8s/admissioncontrol/patch/volumetest.json b/test/fixtures/k8s/admissioncontrol/patch/volumetest.json index 363531d..1e258bb 100644 --- a/test/fixtures/k8s/admissioncontrol/patch/volumetest.json +++ b/test/fixtures/k8s/admissioncontrol/patch/volumetest.json @@ -39,7 +39,7 @@ "env" : [ { "name" : "EVERYWHERE", - "value" : "already present" + "value" : "injected in all containers" } ], "image" : "maxmindimage", @@ -106,13 +106,19 @@ }, { "op" : "add", - "path" : "/spec/volumes", - "value" : [ - { - "emptyDir" : {}, - "name" : "maxminddb" - } - ] + "path" : "/spec/volumes/-", + "value" : { + "emptyDir" : {}, + "name" : "maxminddb" + } + }, + { + "op" : "add", + "path" : "/spec/volumes/-", + "value" : { + "emptyDir" : {}, + "name" : "anothervolume" + } }, { "op" : "add", diff --git a/test/fixtures/sidecars/maxmind.yaml b/test/fixtures/sidecars/maxmind.yaml index 1696fbe..090479f 100644 --- a/test/fixtures/sidecars/maxmind.yaml +++ b/test/fixtures/sidecars/maxmind.yaml @@ -7,15 +7,14 @@ initContainers: - image: maxmindimage name: maxminddb-init command: ["/bin/init.sh"] - env: - - name: EVERYWHERE - value: already present containers: - image: maxmindimage name: maxminddb volumes: - name: maxminddb emptyDir: {} + - name: anothervolume + emptyDir: {} volumeMounts: - name: maxminddb mountPath: /usr/share/GeoIP From a596e298818bb4fb973528c70a46595e18c7919f Mon Sep 17 00:00:00 2001 From: Gabe Conradi Date: Mon, 3 Feb 2020 14:55:59 -0500 Subject: [PATCH 7/8] fixing env vars --- .../k8s/admissioncontrol/patch/volumetest-existingvolume.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/fixtures/k8s/admissioncontrol/patch/volumetest-existingvolume.json b/test/fixtures/k8s/admissioncontrol/patch/volumetest-existingvolume.json index eeddade..b9015da 100644 --- a/test/fixtures/k8s/admissioncontrol/patch/volumetest-existingvolume.json +++ b/test/fixtures/k8s/admissioncontrol/patch/volumetest-existingvolume.json @@ -39,7 +39,7 @@ "env" : [ { "name" : "EVERYWHERE", - "value" : "already present" + "value" : "injected in all containers" } ], "image" : "maxmindimage", @@ -109,7 +109,7 @@ "path" : "/spec/volumes/-", "value" : { "emptyDir" : {}, - "name" : "hell" + "name" : "anothervolume" } }, { From 29441fbd43ea022ef53e0e751adf7b0e78a88f17 Mon Sep 17 00:00:00 2001 From: Gabe Conradi Date: Mon, 3 Feb 2020 15:38:52 -0500 Subject: [PATCH 8/8] no more caching test runs --- Makefile | 4 ++-- internal/pkg/config/config_test.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Makefile b/Makefile index 4d2cdac..364ae00 100644 --- a/Makefile +++ b/Makefile @@ -63,10 +63,10 @@ test-race: ARGS=-race ## Run tests with race detector $(TEST_TARGETS): NAME=$(MAKECMDGOALS:test-%=%) $(TEST_TARGETS): test check test tests: fmt lint vendor | ; $(info $(M) running $(NAME:%=% )tests…) @ ## Run tests - $Q $(GO) test -timeout $(TIMEOUT)s $(ARGS) $(TESTPKGS) + $Q $(GO) test -count=1 -timeout=$(TIMEOUT)s $(ARGS) $(TESTPKGS) test-xml: fmt lint vendor | $(GO2XUNIT) ; $(info $(M) running $(NAME:%=% )tests…) @ ## Run tests with xUnit output - $Q 2>&1 $(GO) test -timeout 20s -v $(TESTPKGS) | tee test/tests.output + $Q 2>&1 $(GO) test -count=1 -timeout=$(TIMEOUT)s -v $(TESTPKGS) | tee test/tests.output $(GO2XUNIT) -fail -input test/tests.output -output test/tests.xml COVERAGE_MODE = atomic diff --git a/internal/pkg/config/config_test.go b/internal/pkg/config/config_test.go index c408b3b..93e9779 100644 --- a/internal/pkg/config/config_test.go +++ b/internal/pkg/config/config_test.go @@ -182,7 +182,7 @@ var ( Path: fixtureSidecarsDir + "/maxmind.yaml", EnvCount: 1, ContainerCount: 1, - VolumeCount: 1, + VolumeCount: 2, VolumeMountCount: 1, HostAliasCount: 0, InitContainerCount: 1,