Merge pull request #40 from tumblr/gabe/remove-default-token-mounts

make sure we remove any volumeMounts that were precreated when inject…

internal/pkg/config/config_test.go

@@ -160,6 +160,22 @@ var (
 			InitContainerCount: 0,
 			ServiceAccount:     "someaccount",
 		},
+		// also, if we inject a serviceAccount and any container has a VolumeMount
+		// with a mountPath of /var/run/secrets/kubernetes.io/serviceaccount, we
+		// must remove it, to allow the ServiceAccountController to inject the
+		// appropriate token volume
+		"service-account-default-token": testhelper.ConfigExpectation{
+			Name:               "service-account-default-token",
+			Version:            "latest",
+			Path:               fixtureSidecarsDir + "/service-account-default-token.yaml",
+			EnvCount:           0,
+			ContainerCount:     0,
+			VolumeCount:        0,
+			VolumeMountCount:   0,
+			HostAliasCount:     0,
+			InitContainerCount: 0,
+			ServiceAccount:     "someaccount",
+		},
 	}
 )
 

pkg/server/webhook.go

@@ -26,9 +26,10 @@ const (
 )
 
 var (
-	runtimeScheme = runtime.NewScheme()
-	codecs        = serializer.NewCodecFactory(runtimeScheme)
-	deserializer  = codecs.UniversalDeserializer()
+	serviceAccountTokenMountPath = "/var/run/secrets/kubernetes.io/serviceaccount"
+	runtimeScheme                = runtime.NewScheme()
+	codecs                       = serializer.NewCodecFactory(runtimeScheme)
+	deserializer                 = codecs.UniversalDeserializer()
 
 	// (https://github.com/kubernetes/kubernetes/issues/57982)
 	defaulter = runtime.ObjectDefaulter(runtimeScheme)
@@ -230,27 +231,6 @@ func addContainers(target, added []corev1.Container, basePath string) (patch []p
 	return patch
 }
 
-func addInitContainers(target, added []corev1.Container, basePath string) (patch []patchOperation) {
-	first := len(target) == 0
-	var value interface{}
-	for _, add := range added {
-		value = add
-		path := basePath
-		if first {
-			first = false
-			value = []corev1.Container{add}
-		} else {
-			path = path + "/-"
-		}
-		patch = append(patch, patchOperation{
-			Op:    "add",
-			Path:  path,
-			Value: value,
-		})
-	}
-	return patch
-}
-
 func addVolumes(target, added []corev1.Volume, basePath string) (patch []patchOperation) {
 	first := len(target) == 0
 	var value interface{}
@@ -328,12 +308,39 @@ func addHostAliases(target, added []corev1.HostAlias, basePath string) (patch []
 	return patch
 }
 
-func setServiceAccount(sa string, basePath string) (patch []patchOperation) {
+func setServiceAccount(initContainers []corev1.Container, containers []corev1.Container, sa string, basePath string) (patch []patchOperation) {
 	patch = append(patch, patchOperation{
 		Op:    "replace",
 		Path:  path.Join(basePath, "serviceAccountName"),
 		Value: sa,
 	})
+
+	// if we find any pre-existing VolumeMounts that provide the default serviceaccount token, we need to snip
+	// them out, so the ServiceAccountController will create the correct VolumeMount once we patch this pod
+	// volumeMounts:
+	//  - name: default-token-wlfz2
+	//    readOnly: true
+	//    mountPath: /var/run/secrets/kubernetes.io/serviceaccount
+	for icIndex, ic := range initContainers {
+		for vmIndex, vm := range ic.VolumeMounts {
+			if vm.MountPath == serviceAccountTokenMountPath {
+				patch = append(patch, patchOperation{
+					Op:   "remove",
+					Path: fmt.Sprintf("%s/initContainers/%d/volumeMounts/%d", basePath, icIndex, vmIndex),
+				})
+			}
+		}
+	}
+	for cIndex, c := range containers {
+		for vmIndex, vm := range c.VolumeMounts {
+			if vm.MountPath == serviceAccountTokenMountPath {
+				patch = append(patch, patchOperation{
+					Op:   "remove",
+					Path: fmt.Sprintf("%s/containers/%d/volumeMounts/%d", basePath, cIndex, vmIndex),
+				})
+			}
+		}
+	}
 	return patch
 }
 
@@ -410,6 +417,14 @@ func updateAnnotations(target map[string]string, added map[string]string) (patch
 func createPatch(pod *corev1.Pod, inj *config.InjectionConfig, annotations map[string]string) ([]byte, error) {
 	var patch []patchOperation
 
+	// be sure to inject the serviceAccountName before adding any volumeMounts, because we must prune out any existing
+	// volumeMounts that were added to support the default service account. Because this removal is by index, we splice
+	// them out before appending new volumes at the end.
+	if inj.ServiceAccountName != "" && (pod.Spec.ServiceAccountName == "" || pod.Spec.ServiceAccountName == "default") {
+		// only override the serviceaccount name if not set in the pod spec
+		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)
@@ -432,11 +447,6 @@ func createPatch(pod *corev1.Pod, inj *config.InjectionConfig, annotations map[s
 	patch = append(patch, addHostAliases(pod.Spec.HostAliases, inj.HostAliases, "/spec/hostAliases")...)
 	patch = append(patch, addVolumes(pod.Spec.Volumes, inj.Volumes, "/spec/volumes")...)
 
-	if inj.ServiceAccountName != "" && (pod.Spec.ServiceAccountName == "" || pod.Spec.ServiceAccountName == "default") {
-		// only override the serviceaccount name if not set in the pod spec
-		patch = append(patch, setServiceAccount(inj.ServiceAccountName, "/spec")...)
-	}
-
 	// last but not least, set annotations
 	patch = append(patch, updateAnnotations(pod.Annotations, annotations)...)
 	return json.Marshal(patch)

pkg/server/webhook_test.go

@@ -63,6 +63,7 @@ var (
 		{name: "service-account", allowed: true, patchExpected: true},
 		{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},
 	}
 	sidecarConfigs, _           = filepath.Glob(path.Join(sidecars, "*.yaml"))
 	expectedNumInjectionConfigs = len(sidecarConfigs)

test/fixtures/k8s/admissioncontrol/patch/service-account-default-token.json

@@ -0,0 +1,20 @@
+[
+   {
+      "op": "replace",
+      "path": "/spec/serviceAccountName",
+      "value": "someaccount"
+   },
+   {
+      "op": "remove",
+      "path": "/spec/initContainers/0/volumeMounts/0"
+   },
+   {
+      "op": "remove",
+      "path": "/spec/containers/1/volumeMounts/1"
+   },
+   {
+      "op" : "add",
+      "path" : "/metadata/annotations/injector.unittest.com~1status",
+      "value" : "injected"
+   }
+]

test/fixtures/k8s/admissioncontrol/request/service-account-default-token.yaml

@@ -0,0 +1,48 @@
+---
+# this is an AdmissionRequest object
+# https://godoc.org/k8s.io/api/admission/v1beta1#AdmissionRequest
+object:
+  metadata:
+    annotations:
+      injector.unittest.com/request: "service-account-default-token"
+  spec:
+    serviceAccountName: "default" # this should get replaced
+    volumes:
+      - name: bogusvolume
+        configMap:
+          name: config-production
+          defaultMode: 420
+      - name: default-token-wlfz2
+        secret:
+          secretName: default-token-wlfz2
+          defaultMode: 420
+    initContainers:
+      - name: init-ctr1-with-token
+        volumeMounts:
+        # this volume mount must be removed, because
+        # by default, a serviceAccount will mount its token,
+        # preventing the injected serviceAccount from settings up its mount
+        - name: default-token-wlfz2
+          readOnly: true
+          mountPath: /var/run/secrets/kubernetes.io/serviceaccount
+      - name: init-ctr2
+        volumeMounts: []
+    containers:
+      - name: ctr1
+        volumeMounts:
+        - name: bogusvolume
+          readOnly: true
+          mountPath: /app/config
+      - name: ctr2-with-token
+        volumeMounts:
+        - name: bogusvolume
+          readOnly: true
+          mountPath: /app/config
+        # this volume mount must be removed, because
+        # by default, a serviceAccount will mount its token,
+        # preventing the injected serviceAccount from settings up its mount
+        - name: default-token-wlfz2
+          readOnly: true
+          mountPath: /var/run/secrets/kubernetes.io/serviceaccount
+      - name: ctr3
+        volumeMounts: []

test/fixtures/sidecars/service-account-default-token.yaml

@@ -0,0 +1,3 @@
+---
+name: service-account-default-token
+serviceAccountName: someaccount