Merge pull request #21 from tumblr/gabe/issue-20

Fix #20, improve unit tests

Dockerfile

@@ -8,7 +8,8 @@ RUN apk --no-cache add \
 
 WORKDIR /src
 COPY go.mod go.sum Makefile ./
-RUN make vendor
+# run vendor install and lint, so we have all deps installed
+RUN make vendor lint
 COPY . .
 RUN make test all
 

cmd/main.go

@@ -114,7 +114,7 @@ func main() {
 				err := configWatcher.Watch(ctx, sigChan)
 				if err != nil {
 					switch err {
-					case watcher.WatchChannelClosedError:
+					case watcher.ErrWatchChannelClosed:
 						glog.Errorf("watcher got error, try to restart watcher: %s", err.Error())
 					default:
 						glog.Fatalf("error watching for new ConfigMaps (terminating): %s", err.Error())

docs/unit-tests.md

@@ -0,0 +1,9 @@
+# Unit Tests
+
+To add a new unit test for some behavior, please do the following:
+
+1. create a AdmissionRequest in YAML format at `test/fixtures/k8s/admissioncontrol/request/foo.yaml`. This should include the pod spec k8s will send with the request, and the annotation with the desired injected sidecar
+2. create a Patch JSON at `test/fixtures/k8s/admissioncontrol/patch/foo.json`.
+3. register your test in the `pkg/server/webhook_test.go` list `mutationTests`
+
+Please use the `injector.unittest.com/request` annotation on your `AdmissionRequest` YAML to signal which sidecar you want to be injected.

go.mod

@@ -18,6 +18,7 @@ require (
 	github.com/matttproud/golang_protobuf_extensions v1.0.1 // indirect
 	github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect
 	github.com/modern-go/reflect2 v0.0.0-20180701023420-4b7aa43c6742 // indirect
+	github.com/nsf/jsondiff v0.0.0-20190712045011-8443391ee9b6
 	github.com/petar/GoLLRB v0.0.0-20130427215148-53be0d36a84c // indirect
 	github.com/peterbourgon/diskv v2.0.1+incompatible // indirect
 	github.com/prometheus/client_golang v0.9.1

go.sum

@@ -4,6 +4,7 @@ github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c
 github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/ghodss/yaml v1.0.0 h1:wQHKEahhL6wmXdzwWG11gIVCkOv05bNOh+Rxn0yngAk=
 github.com/ghodss/yaml v1.0.0/go.mod h1:4dBDuWmgqj2HViK6kFavaiC9ZROes6MMH2rRYeMEF04=
+github.com/go-test/deep v1.0.2/go.mod h1:wGDj63lr65AM2AQyKZd/NYHGb0R+1RLqB8NKt3aSFNA=
 github.com/gogo/protobuf v1.1.1 h1:72R+M5VuhED/KujmZVcIquuo8mBgX4oVda//DQb3PXo=
 github.com/gogo/protobuf v1.1.1/go.mod h1:r8qH/GZQm5c6nD/R0oafs1akxWv10x8SbQlK7atdtwQ=
 github.com/golang/glog v0.0.0-20160126235308-23def4e6c14b h1:VKtxabqXZkF25pY9ekfRL6a582T4P37/31XEstQ5p58=
@@ -34,6 +35,7 @@ github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd h1:TRLaZ9cD/w
 github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd/go.mod h1:6dJC0mAP4ikYIbvyc7fijjWJddQyLn8Ig3JB5CqoB9Q=
 github.com/modern-go/reflect2 v0.0.0-20180701023420-4b7aa43c6742 h1:Esafd1046DLDQ0W1YjYsBW+p8U2u7vzgW2SQVmlNazg=
 github.com/modern-go/reflect2 v0.0.0-20180701023420-4b7aa43c6742/go.mod h1:bx2lNnkwVCuqBIxFjflWJWanXIb3RllmbCylyMrvgv0=
+github.com/nsf/jsondiff v0.0.0-20190712045011-8443391ee9b6/go.mod h1:uFMI8w+ref4v2r9jz+c9i1IfIttS/OkmLfrk1jne5hs=
 github.com/petar/GoLLRB v0.0.0-20130427215148-53be0d36a84c/go.mod h1:HUpKUBZnpzkdx0kD/+Yfuft+uD3zHGtXF/XJB14TUr4=
 github.com/peterbourgon/diskv v2.0.1+incompatible h1:UBdAOUP5p4RWqPBg048CAvpKN+vxiaj6gdUUzhl4XmI=
 github.com/peterbourgon/diskv v2.0.1+incompatible/go.mod h1:uqqh8zWWbv1HBMNONnaR/tNboyR3/BZd58JJSHlUSCU=

internal/pkg/config/config.go

@@ -20,8 +20,6 @@ const (
 )
 
 var (
-	// InjectionStatusAnnotation is the annotation set on resources to reflect the status of injection
-	InjectionStatusAnnotation = fmt.Sprintf("%s/status", annotationNamespaceDefault)
 	// ErrMissingName ..
 	ErrMissingName = fmt.Errorf(`name field is required for an injection config`)
 	// ErrNoConfigurationLoaded ..

internal/pkg/config/watcher/watcher.go

@@ -26,8 +26,8 @@ const (
 	serviceAccountNamespaceFilePath = "/var/run/secrets/kubernetes.io/serviceaccount/namespace"
 )
 
-// WatchChannelClosedError: should restart watcher
-var WatchChannelClosedError = errors.New("watcher channel has closed")
+// ErrWatchChannelClosed should restart watcher
+var ErrWatchChannelClosed = errors.New("watcher channel has closed")
 
 // K8sConfigMapWatcher is a struct that connects to the API and collects, parses, and emits sidecar configurations
 type K8sConfigMapWatcher struct {
@@ -114,7 +114,7 @@ func (c *K8sConfigMapWatcher) Watch(ctx context.Context, notifyMe chan<- interfa
 			// detail at https://github.com/kubernetes/client-go/issues/334
 			if !ok {
 				glog.Errorf("channel has closed, should restart watcher")
-				return WatchChannelClosedError
+				return ErrWatchChannelClosed
 			}
 			if e.Type == watch.Error {
 				return apierrs.FromObject(e.Object)

internal/pkg/config/watcher/watcher_test.go

@@ -50,8 +50,8 @@ func TestWatcherChannelClose(t *testing.T) {
 	ctx := context.Background()
 
 	err := w.Watch(ctx, sigChan)
-	if err != nil && err != WatchChannelClosedError {
-		t.Errorf("expect catch WatchChannelClosedError, but got %s", err)
+	if err != nil && err != ErrWatchChannelClosed {
+		t.Errorf("expect catch ErrWatchChannelClosed, but got %s", err)
 	}
 }
 

pkg/server/webhook.go

@@ -124,6 +124,14 @@ func applyDefaultsWorkaround(containers []corev1.Container, volumes []corev1.Vol
 	})
 }
 
+func (whsvr *WebhookServer) statusAnnotationKey() string {
+	return whsvr.Config.AnnotationNamespace + "/status"
+}
+
+func (whsvr *WebhookServer) requestAnnotationKey() string {
+	return whsvr.Config.AnnotationNamespace + "/request"
+}
+
 // Check whether the target resoured need to be mutated
 func (whsvr *WebhookServer) getSidecarConfigurationRequested(ignoredList []string, metadata *metav1.ObjectMeta) (string, error) {
 	// skip special kubernetes system namespaces
@@ -139,8 +147,8 @@ func (whsvr *WebhookServer) getSidecarConfigurationRequested(ignoredList []strin
 		annotations = map[string]string{}
 	}
 
-	statusAnnotationKey := whsvr.Config.AnnotationNamespace + "/status"
-	requestAnnotationKey := whsvr.Config.AnnotationNamespace + "/request"
+	statusAnnotationKey := whsvr.statusAnnotationKey()
+	requestAnnotationKey := whsvr.requestAnnotationKey()
 
 	status, ok := annotations[statusAnnotationKey]
 	if ok && strings.ToLower(status) == StatusInjected {
@@ -419,12 +427,11 @@ func createPatch(pod *corev1.Pod, inj *config.InjectionConfig, annotations map[s
 }
 
 // main mutation process
-func (whsvr *WebhookServer) mutate(ar *v1beta1.AdmissionReview) *v1beta1.AdmissionResponse {
-	req := ar.Request
+func (whsvr *WebhookServer) mutate(req *v1beta1.AdmissionRequest) *v1beta1.AdmissionResponse {
 	var pod corev1.Pod
 	if err := json.Unmarshal(req.Object.Raw, &pod); err != nil {
 		glog.Errorf("Could not unmarshal raw object: %v", err)
-		injectionCounter.With(prometheus.Labels{"status": "error", "reason": "unmarshal_error"}).Inc()
+		injectionCounter.With(prometheus.Labels{"status": "error", "reason": "unmarshal_error", "requested": ""}).Inc()
 		return &v1beta1.AdmissionResponse{
 			Result: &metav1.Status{
 				Message: err.Error(),
@@ -459,7 +466,7 @@ func (whsvr *WebhookServer) mutate(ar *v1beta1.AdmissionReview) *v1beta1.Admissi
 	// Workaround: https://github.com/kubernetes/kubernetes/issues/57982
 	applyDefaultsWorkaround(injectionConfig.Containers, injectionConfig.Volumes)
 	annotations := map[string]string{}
-	annotations[config.InjectionStatusAnnotation] = StatusInjected
+	annotations[whsvr.statusAnnotationKey()] = StatusInjected
 	patchBytes, err := createPatch(&pod, injectionConfig, annotations)
 	if err != nil {
 		injectionCounter.With(prometheus.Labels{"status": "error", "reason": "patching_error", "requested": injectionKey}).Inc()
@@ -532,7 +539,7 @@ func (whsvr *WebhookServer) mutateHandler(w http.ResponseWriter, r *http.Request
 			},
 		}
 	} else {
-		admissionResponse = whsvr.mutate(&ar)
+		admissionResponse = whsvr.mutate(ar.Request)
 	}
 
 	admissionReview := v1beta1.AdmissionReview{}

pkg/server/webhook_test.go

@@ -1,18 +1,23 @@
 package server
 
 import (
+	"fmt"
 	"io/ioutil"
 	"net/http"
+	"os"
 	"testing"
 
 	"github.com/ghodss/yaml"
+	"github.com/nsf/jsondiff" // for json diffing patches
 	"github.com/tumblr/k8s-sidecar-injector/internal/pkg/config"
 	_ "github.com/tumblr/k8s-sidecar-injector/internal/pkg/testing"
+	"k8s.io/api/admission/v1beta1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 var (
-	sidecars = "test/fixtures/sidecars"
+	sidecars     = "test/fixtures/sidecars"
+	jsondiffopts = jsondiff.DefaultConsoleOptions()
 
 	// all these configs are deserialized into metav1.ObjectMeta structs
 	obj1             = "test/fixtures/k8s/object1.yaml"
@@ -27,6 +32,27 @@ var (
 	badSidecar       = "test/fixtures/k8s/bad-sidecar.yaml"
 
 	testIgnoredNamespaces = []string{"ignore-me"}
+
+	// tests to check config loading of sidecars
+	configTests = []expectedSidecarConfiguration{
+		{configuration: obj1, expectedSidecar: "sidecar-test"},
+		{configuration: obj2, expectedSidecar: "complex-sidecar"},
+		{configuration: env1, expectedSidecar: "env1"},
+		{configuration: obj3Missing, expectedSidecar: "", expectedError: ErrMissingRequestAnnotation}, // this one is missing any annotations :)
+		{configuration: obj4, expectedSidecar: "", expectedError: ErrSkipAlreadyInjected},             // this one is already injected, so it should not get injected again
+		{configuration: obj5, expectedSidecar: "volume-mounts"},
+		{configuration: obj6, expectedSidecar: "host-aliases"},
+		{configuration: obj7, expectedSidecar: "init-containers"},
+		{configuration: ignoredNamespace, expectedSidecar: "", expectedError: ErrSkipIgnoredNamespace},
+		{configuration: badSidecar, expectedSidecar: "this-doesnt-exist", expectedError: ErrRequestedSidecarNotFound},
+	}
+
+	// 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},
+	}
 )
 
 type expectedSidecarConfiguration struct {
@@ -35,6 +61,13 @@ type expectedSidecarConfiguration struct {
 	expectedError   error
 }
 
+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
+}
+
 func TestLoadConfig(t *testing.T) {
 	expectedNumInjectionConfigs := 6
 	c, err := config.LoadConfigDirectory(sidecars)
@@ -59,21 +92,7 @@ func TestLoadConfig(t *testing.T) {
 		},
 	}
 
-	// load some objects that are k8s metadata objects
-	tests := []expectedSidecarConfiguration{
-		{configuration: obj1, expectedSidecar: "sidecar-test"},
-		{configuration: obj2, expectedSidecar: "complex-sidecar"},
-		{configuration: env1, expectedSidecar: "env1"},
-		{configuration: obj3Missing, expectedSidecar: "", expectedError: ErrMissingRequestAnnotation}, // this one is missing any annotations :)
-		{configuration: obj4, expectedSidecar: "", expectedError: ErrSkipAlreadyInjected},             // this one is already injected, so it should not get injected again
-		{configuration: obj5, expectedSidecar: "volume-mounts"},
-		{configuration: obj6, expectedSidecar: "host-aliases"},
-		{configuration: obj7, expectedSidecar: "init-containers"},
-		{configuration: ignoredNamespace, expectedSidecar: "", expectedError: ErrSkipIgnoredNamespace},
-		{configuration: badSidecar, expectedSidecar: "this-doesnt-exist", expectedError: ErrRequestedSidecarNotFound},
-	}
-
-	for _, test := range tests {
+	for _, test := range configTests {
 		data, err := ioutil.ReadFile(test.configuration)
 		if err != nil {
 			t.Errorf("unable to load object metadata yaml: %v", err)
@@ -94,5 +113,69 @@ func TestLoadConfig(t *testing.T) {
 			t.Errorf("%s: expected sidecar to be %v but was %v instead", test.configuration, test.expectedSidecar, key)
 			t.Fail()
 		}
+
+	}
+}
+
+func TestMutation(t *testing.T) {
+	c, err := config.LoadConfigDirectory(sidecars)
+	if err != nil {
+		t.Error(err)
+		t.Fail()
+	}
+	c.AnnotationNamespace = "injector.unittest.com"
+
+	s := &WebhookServer{
+		Config: c,
+		Server: &http.Server{
+			Addr: ":6969",
+		},
+	}
+
+	for _, test := range mutationTests {
+		// now, try to perform the mutation on the k8s object
+		var req v1beta1.AdmissionRequest
+		reqFile := fmt.Sprintf("test/fixtures/k8s/admissioncontrol/request/%s.yaml", test.name)
+		resPatchFile := fmt.Sprintf("test/fixtures/k8s/admissioncontrol/patch/%s.json", test.name)
+		// load the AdmissionRequest object
+		reqData, err := ioutil.ReadFile(reqFile)
+		if err != nil {
+			t.Errorf("%s: unable to load AdmissionRequest object: %v", reqFile, err)
+			t.Fail()
+		}
+		if err := yaml.Unmarshal(reqData, &req); err != nil {
+			t.Errorf("%s: unable to unmarshal AdmissionRequest yaml: %v", reqFile, err)
+			t.Fail()
+		}
+
+		// stuff the request into mutate, and catch the response
+		res := s.mutate(&req)
+
+		// extract this field, so we can diff json separate from the AdmissionResponse object
+		resPatch := res.Patch
+		res.Patch = nil // zero this field out
+
+		if test.allowed != res.Allowed {
+			t.Errorf("expected AdmissionResponse.Allowed=%v differed from received AdmissionResponse.Allowed=%v", test.allowed, res.Allowed)
+			t.Fail()
+		}
+
+		// 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 {
+			t.Logf("Loading patch data from %s...", resPatchFile)
+			expectedPatchData, err := ioutil.ReadFile(resPatchFile)
+			if err != nil {
+				t.Error(err)
+				t.Fail()
+			}
+			difference, diffString := jsondiff.Compare(expectedPatchData, resPatch, &jsondiffopts)
+			if difference != jsondiff.FullMatch {
+				t.Errorf("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/env-override.json

@@ -0,0 +1,23 @@
+[
+   {
+      "op": "add",
+      "path": "/spec/containers/0/env/-",
+      "value": {
+          "name": "FOO_BAR",
+          "value": "something interesting"
+      }
+  },
+  {
+      "path": "/spec/containers/0/env/-",
+      "value": {
+          "name": "ENVIRONMENT",
+          "value": "production"
+      },
+      "op": "add"
+  },
+  {
+      "op": "add",
+      "path": "/metadata/annotations/injector.unittest.com~1status",
+      "value": "injected"
+  }
+]