Merge pull request #41 from tumblr/gabe/dupe-volumes

rejigger how initContainers are mutated to not forget VolumeMounts

.travis.yml

@@ -2,7 +2,9 @@ language: go
 
 go:
   - "1.x"
-  - "1.11.5"
+  - "1.11.x"
+  - "1.12.x"
+  - "1.13.x"
 
 env:
   - GO111MODULE=on

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

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

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=

internal/pkg/config/config_test.go

@@ -176,6 +176,17 @@ var (
 			InitContainerCount: 0,
 			ServiceAccount:     "someaccount",
 		},
+		"maxmind": testhelper.ConfigExpectation{
+			Name:               "maxmind",
+			Version:            "latest",
+			Path:               fixtureSidecarsDir + "/maxmind.yaml",
+			EnvCount:           1,
+			ContainerCount:     1,
+			VolumeCount:        2,
+			VolumeMountCount:   1,
+			HostAliasCount:     0,
+			InitContainerCount: 1,
+		},
 	}
 )
 

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 {
@@ -231,34 +231,39 @@ 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,
 		})
 	}
 	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 {
@@ -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)
 	}
@@ -425,27 +434,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)...)

pkg/server/webhook_test.go

@@ -64,6 +64,8 @@ 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},
+		{name: "volumetest-existingvolume", allowed: true, patchExpected: true},
 	}
 	sidecarConfigs, _           = filepath.Glob(path.Join(sidecars, "*.yaml"))
 	expectedNumInjectionConfigs = len(sidecarConfigs)
@@ -179,6 +181,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)
 			}
 		}

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",