Correctly handle inheritance with ServiceAccounts

internal/pkg/config/config.go

@@ -59,15 +59,20 @@ func (c *InjectionConfig) String() string {
 		inheritsString = fmt.Sprintf(" (inherits %s)", c.Inherits)
 	}
 
-	return fmt.Sprintf("%s%s: %d containers, %d init containers, %d volumes, %d environment vars, %d volume mounts, %d host aliases",
+	saString := ""
+	if c.ServiceAccountName != "" {
+		saString = fmt.Sprintf(", serviceAccountName %s", c.ServiceAccountName)
+	}
+	return fmt.Sprintf("%s%s: %d containers, %d init containers, %d volumes, %d environment vars, %d volume mounts, %d host aliases%s",
 		c.FullName(),
 		inheritsString,
 		len(c.Containers),
 		len(c.InitContainers),
 		len(c.Volumes),
 		len(c.Environment),
 		len(c.VolumeMounts),
-		len(c.HostAliases))
+		len(c.HostAliases),
+		saString)
 }
 
 // Version returns the parsed version of this injection config. If no version is specified,
@@ -265,6 +270,11 @@ func (base *InjectionConfig) Merge(child *InjectionConfig) error {
 		}
 	}
 
+	// merge serviceAccount settings to the left
+	if child.ServiceAccountName != "" {
+		base.ServiceAccountName = child.ServiceAccountName
+	}
+
 	return nil
 }
 

internal/pkg/config/config_test.go

@@ -144,7 +144,21 @@ var (
 			VolumeMountCount:   0,
 			HostAliasCount:     0,
 			InitContainerCount: 0,
-			ServiceAccount:     "fuck",
+			ServiceAccount:     "someaccount",
+		},
+		// we found that inheritance could cause the loading of the ServiceAccount
+		// to fail, so we test explicitly for this case.
+		"service-account-with-inheritance": testhelper.ConfigExpectation{
+			Name:               "service-account-inherits-env1",
+			Version:            "latest",
+			Path:               fixtureSidecarsDir + "/service-account-with-inheritance.yaml",
+			EnvCount:           3,
+			ContainerCount:     0,
+			VolumeCount:        0,
+			VolumeMountCount:   0,
+			HostAliasCount:     0,
+			InitContainerCount: 0,
+			ServiceAccount:     "someaccount",
 		},
 	}
 )
@@ -200,6 +214,9 @@ func TestGoodConfigs(t *testing.T) {
 		if len(c.InitContainers) != testConfig.InitContainerCount {
 			t.Fatalf("expected %d InitContainers loaded from %s but got %d", testConfig.InitContainerCount, testConfig.Path, len(c.InitContainers))
 		}
+		if c.ServiceAccountName != testConfig.ServiceAccount {
+			t.Fatalf("expected ServiceAccountName %s, but got %s", testConfig.ServiceAccount, c.ServiceAccountName)
+		}
 	}
 }
 
@@ -258,4 +275,7 @@ func TestGetInjectionConfig(t *testing.T) {
 	if len(i.InitContainers) != cfg.InitContainerCount {
 		t.Fatalf("expected %d InitContainers, but got %d", cfg.InitContainerCount, len(i.InitContainers))
 	}
+	if i.ServiceAccountName != cfg.ServiceAccount {
+		t.Fatalf("expected ServiceAccountName %s, but got %s", cfg.ServiceAccount, i.ServiceAccountName)
+	}
 }

pkg/server/webhook.go

@@ -431,7 +431,8 @@ func createPatch(pod *corev1.Pod, inj *config.InjectionConfig, annotations map[s
 	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")...)
-	if inj.ServiceAccountName != "" && pod.Spec.ServiceAccountName == "" {
+
+	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")...)
 	}

pkg/server/webhook_test.go

@@ -57,11 +57,12 @@ var (
 
 	// tests to check the mutate() function for correct operation
 	mutationTests = []mutationTest{
-		{name: "missing-sidecar-config", allowed: true},
-		{name: "sidecar-test-1", allowed: true},
-		{name: "env-override", allowed: true},
-		{name: "service-account", allowed: true},
-		{name: "service-account-already-set", allowed: true},
+		{name: "missing-sidecar-config", allowed: true, patchExpected: false},
+		{name: "sidecar-test-1", allowed: true, patchExpected: true},
+		{name: "env-override", allowed: true, patchExpected: true},
+		{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},
 	}
 	sidecarConfigs, _           = filepath.Glob(path.Join(sidecars, "*.yaml"))
 	expectedNumInjectionConfigs = len(sidecarConfigs)
@@ -76,8 +77,9 @@ type expectedSidecarConfiguration struct {
 type mutationTest struct {
 	// name is a file relative to test/fixtures/k8s/admissioncontrol/request/ ending in .yaml
 	//  which is the v1beta1.AdmissionRequest object passed to mutate
-	name    string
-	allowed bool
+	name          string
+	allowed       bool
+	patchExpected bool
 }
 
 func TestLoadConfig(t *testing.T) {
@@ -164,7 +166,10 @@ func TestMutation(t *testing.T) {
 		// diff the JSON patch object with expected JSON loaded from disk
 		// we do this because this is way easier on the eyes than diffing
 		// a yaml base64 encoded string
-		if _, err := os.Stat(resPatchFile); err == nil {
+		if test.patchExpected {
+			if _, err := os.Stat(resPatchFile); err != nil {
+				t.Fatalf("%s: unable to load expected patch JSON response: %v", resPatchFile, err)
+			}
 			t.Logf("Loading patch data from %s...", resPatchFile)
 			expectedPatchData, err := ioutil.ReadFile(resPatchFile)
 			if err != nil {
@@ -175,7 +180,6 @@ func TestMutation(t *testing.T) {
 			if difference != jsondiff.FullMatch {
 				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/service-account-set-default.json

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

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

@@ -0,0 +1,10 @@
+---
+# this is an AdmissionRequest object
+# https://godoc.org/k8s.io/api/admission/v1beta1#AdmissionRequest
+object:
+  metadata:
+    annotations:
+      injector.unittest.com/request: "service-account"
+  spec:
+    serviceAccountName: "default" # this should get replaced
+    containers: []

test/fixtures/sidecars/service-account-with-inheritance.yaml

@@ -0,0 +1,4 @@
+---
+name: service-account-inherits-env1
+inherits: env1.yaml
+serviceAccountName: someaccount