From 1d764ed9b7b087beb61f67a48b490ec58c05bd45 Mon Sep 17 00:00:00 2001 From: Gabe Conradi Date: Tue, 13 Aug 2019 14:27:19 -0400 Subject: [PATCH] migrating to a better request/response test model trying to improve tests and fix a panic in prometheus counter labels trying to get admissionresponse tests in shape working up a better test for request/response got patch diffing working clarify patch diffing fix up request/response model to be more expressive reformat where patch tests are on the FS add unit test docs remove unneeded bit --- Dockerfile | 3 +- cmd/main.go | 2 +- docs/unit-tests.md | 9 ++ go.mod | 1 + go.sum | 2 + internal/pkg/config/config.go | 2 - internal/pkg/config/watcher/watcher.go | 6 +- internal/pkg/config/watcher/watcher_test.go | 4 +- pkg/server/webhook.go | 21 ++-- pkg/server/webhook_test.go | 115 +++++++++++++++--- .../admissioncontrol/patch/env-override.json | 23 ++++ .../patch/sidecar-test-1.json | 76 ++++++++++++ .../request/env-override.yaml | 15 +++ .../request/missing-sidecar-config.yaml | 18 +++ .../request/sidecar-test-1.yaml | 9 ++ 15 files changed, 274 insertions(+), 32 deletions(-) create mode 100644 docs/unit-tests.md create mode 100644 test/fixtures/k8s/admissioncontrol/patch/env-override.json create mode 100644 test/fixtures/k8s/admissioncontrol/patch/sidecar-test-1.json create mode 100644 test/fixtures/k8s/admissioncontrol/request/env-override.yaml create mode 100644 test/fixtures/k8s/admissioncontrol/request/missing-sidecar-config.yaml create mode 100644 test/fixtures/k8s/admissioncontrol/request/sidecar-test-1.yaml diff --git a/Dockerfile b/Dockerfile index f779de3..b8144c5 100644 --- a/Dockerfile +++ b/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 diff --git a/cmd/main.go b/cmd/main.go index 64822cd..8c466ab 100644 --- a/cmd/main.go +++ b/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()) diff --git a/docs/unit-tests.md b/docs/unit-tests.md new file mode 100644 index 0000000..c32425f --- /dev/null +++ b/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. diff --git a/go.mod b/go.mod index 3357c07..d3ef821 100644 --- a/go.mod +++ b/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 diff --git a/go.sum b/go.sum index 05827cd..c0c3276 100644 --- a/go.sum +++ b/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= diff --git a/internal/pkg/config/config.go b/internal/pkg/config/config.go index 0778397..85ae115 100644 --- a/internal/pkg/config/config.go +++ b/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 .. diff --git a/internal/pkg/config/watcher/watcher.go b/internal/pkg/config/watcher/watcher.go index 60e5f59..f38c04b 100644 --- a/internal/pkg/config/watcher/watcher.go +++ b/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) diff --git a/internal/pkg/config/watcher/watcher_test.go b/internal/pkg/config/watcher/watcher_test.go index 0a6fb68..da4850c 100644 --- a/internal/pkg/config/watcher/watcher_test.go +++ b/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) } } diff --git a/pkg/server/webhook.go b/pkg/server/webhook.go index c009abf..4e088d9 100644 --- a/pkg/server/webhook.go +++ b/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{} diff --git a/pkg/server/webhook_test.go b/pkg/server/webhook_test.go index 4331c3d..6dcd9b4 100644 --- a/pkg/server/webhook_test.go +++ b/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) + } + + } + } } diff --git a/test/fixtures/k8s/admissioncontrol/patch/env-override.json b/test/fixtures/k8s/admissioncontrol/patch/env-override.json new file mode 100644 index 0000000..228c489 --- /dev/null +++ b/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" + } +] diff --git a/test/fixtures/k8s/admissioncontrol/patch/sidecar-test-1.json b/test/fixtures/k8s/admissioncontrol/patch/sidecar-test-1.json new file mode 100644 index 0000000..4077135 --- /dev/null +++ b/test/fixtures/k8s/admissioncontrol/patch/sidecar-test-1.json @@ -0,0 +1,76 @@ +[ + { + "op" : "add", + "path" : "/spec/containers", + "value" : [ + { + "env" : [ + { + "name" : "DATACENTER", + "value" : "bf2" + }, + { + "name" : "FROM_INJECTOR", + "value" : "bar" + } + ], + "image" : "nginx:1.12.2", + "imagePullPolicy" : "IfNotPresent", + "name" : "sidecar-nginx", + "ports" : [ + { + "containerPort" : 80 + } + ], + "resources" : {}, + "volumeMounts" : [ + { + "mountPath" : "/etc/nginx", + "name" : "nginx-conf" + } + ] + } + ] + }, + { + "op" : "add", + "path" : "/spec/containers/-", + "value" : { + "env" : [ + { + "name" : "DATACENTER", + "value" : "foo" + }, + { + "name" : "FROM_INJECTOR", + "value" : "bar" + } + ], + "image" : "foo:69", + "name" : "another-sidecar", + "ports" : [ + { + "containerPort" : 420 + } + ], + "resources" : {} + } + }, + { + "op" : "add", + "path" : "/spec/volumes", + "value" : [ + { + "configMap" : { + "name" : "nginx-configmap" + }, + "name" : "nginx-conf" + } + ] + }, + { + "op" : "add", + "path" : "/metadata/annotations/injector.unittest.com~1status", + "value" : "injected" + } +] diff --git a/test/fixtures/k8s/admissioncontrol/request/env-override.yaml b/test/fixtures/k8s/admissioncontrol/request/env-override.yaml new file mode 100644 index 0000000..86a8361 --- /dev/null +++ b/test/fixtures/k8s/admissioncontrol/request/env-override.yaml @@ -0,0 +1,15 @@ +--- +# this is an AdmissionRequest object +# https://godoc.org/k8s.io/api/admission/v1beta1#AdmissionRequest +object: + metadata: + annotations: + injector.unittest.com/request: "env1" + spec: + containers: + - name: something + env: + - name: SOME_VARIABLE + value: dope + - name: DATACENTER + value: definedbypod diff --git a/test/fixtures/k8s/admissioncontrol/request/missing-sidecar-config.yaml b/test/fixtures/k8s/admissioncontrol/request/missing-sidecar-config.yaml new file mode 100644 index 0000000..a68eade --- /dev/null +++ b/test/fixtures/k8s/admissioncontrol/request/missing-sidecar-config.yaml @@ -0,0 +1,18 @@ +--- +# this is an AdmissionRequest object +# https://godoc.org/k8s.io/api/admission/v1beta1#AdmissionRequest +object: + metadata: + annotations: + injector.unittest.com/request: this-doesnt-exist + spec: + containers: + - name: memory-demo-2-ctr + image: polinux/stress + resources: + requests: + memory: "50Mi" + limits: + memory: "100Mi" + command: ["stress"] + args: ["--vm", "1", "--vm-bytes", "250M", "--vm-hang", "1"] diff --git a/test/fixtures/k8s/admissioncontrol/request/sidecar-test-1.yaml b/test/fixtures/k8s/admissioncontrol/request/sidecar-test-1.yaml new file mode 100644 index 0000000..90c0094 --- /dev/null +++ b/test/fixtures/k8s/admissioncontrol/request/sidecar-test-1.yaml @@ -0,0 +1,9 @@ +--- +# this is an AdmissionRequest object +# https://godoc.org/k8s.io/api/admission/v1beta1#AdmissionRequest +object: + metadata: + annotations: + injector.unittest.com/request: "sidecar-test" + spec: + containers: []