From 204da0b2dbd700f9f6674b54dc4659282429f3cc Mon Sep 17 00:00:00 2001 From: "Dr. Stefan Schimanski" Date: Tue, 7 Nov 2017 10:40:43 +0100 Subject: [PATCH] apiserver: add validating admission tests - in endpoint tests - in generic registry - in patch handler - in admission chain Kubernetes-commit: c558d2a3517fafdb704edd2c00b6df6738786959 --- pkg/admission/chain_test.go | 45 +++- pkg/endpoints/apiserver_test.go | 215 +++++++++++--------- pkg/endpoints/handlers/rest_test.go | 103 +++++++--- pkg/registry/generic/registry/store_test.go | 50 ++++- 4 files changed, 277 insertions(+), 136 deletions(-) diff --git a/pkg/admission/chain_test.go b/pkg/admission/chain_test.go index 8bdd74ca4..faab4c875 100644 --- a/pkg/admission/chain_test.go +++ b/pkg/admission/chain_test.go @@ -39,22 +39,23 @@ func (h *FakeHandler) Admit(a Attributes) (err error) { } func (h *FakeHandler) Validate(a Attributes) (err error) { - h.admitCalled = true - if h.admit { + h.validateCalled = true + if h.validate { return nil } - return fmt.Errorf("Don't admit") + return fmt.Errorf("Don't validate") } -func makeHandler(name string, admit bool, ops ...Operation) Interface { +func makeHandler(name string, accept bool, ops ...Operation) Interface { return &FakeHandler{ - name: name, - admit: admit, - Handler: NewHandler(ops...), + name: name, + admit: accept, + validate: accept, + Handler: NewHandler(ops...), } } -func TestAdmit(t *testing.T) { +func TestAdmitAndValidate(t *testing.T) { tests := []struct { name string operation Operation @@ -108,6 +109,7 @@ func TestAdmit(t *testing.T) { }, } for _, test := range tests { + // call admit and check that validate was not called at all err := test.chain.Admit(NewAttributesRecord(nil, nil, schema.GroupVersionKind{}, "", "", schema.GroupVersionResource{}, "", test.operation, nil)) accepted := (err == nil) if accepted != test.accept { @@ -117,9 +119,34 @@ func TestAdmit(t *testing.T) { fake := h.(*FakeHandler) _, shouldBeCalled := test.calls[fake.name] if shouldBeCalled != fake.admitCalled { - t.Errorf("%s: handler %s not called as expected: %v", test.name, fake.name, fake.admitCalled) + t.Errorf("%s: admit handler %s not called as expected: %v", test.name, fake.name, fake.admitCalled) continue } + if fake.validateCalled { + t.Errorf("%s: validate handler %s called during admit", test.name, fake.name) + } + + // reset value for validation test + fake.admitCalled = false + } + + // call validate and check that admit was not called at all + err = test.chain.Validate(NewAttributesRecord(nil, nil, schema.GroupVersionKind{}, "", "", schema.GroupVersionResource{}, "", test.operation, nil)) + accepted = (err == nil) + if accepted != test.accept { + t.Errorf("%s: unexpected result of validate call: %v\n", test.name, accepted) + } + for _, h := range test.chain { + fake := h.(*FakeHandler) + _, shouldBeCalled := test.calls[fake.name] + if shouldBeCalled != fake.validateCalled { + t.Errorf("%s: validate handler %s not called as expected: %v", test.name, fake.name, fake.validateCalled) + continue + } + + if fake.admitCalled { + t.Errorf("%s: admit handler %s called during admit", test.name, fake.name) + } } } } diff --git a/pkg/endpoints/apiserver_test.go b/pkg/endpoints/apiserver_test.go index 458c4d2ac..4b8ee2988 100644 --- a/pkg/endpoints/apiserver_test.go +++ b/pkg/endpoints/apiserver_test.go @@ -82,13 +82,23 @@ func (alwaysAdmit) Handles(operation admission.Operation) bool { return true } -type alwaysDeny struct{} +type alwaysMutatingDeny struct{} -func (alwaysDeny) Admit(a admission.Attributes) (err error) { - return admission.NewForbidden(a, errors.New("Admission control is denying all modifications")) +func (alwaysMutatingDeny) Admit(a admission.Attributes) (err error) { + return admission.NewForbidden(a, errors.New("Mutating admission control is denying all modifications")) } -func (alwaysDeny) Handles(operation admission.Operation) bool { +func (alwaysMutatingDeny) Handles(operation admission.Operation) bool { + return true +} + +type alwaysValidatingDeny struct{} + +func (alwaysValidatingDeny) Validate(a admission.Attributes) (err error) { + return admission.NewForbidden(a, errors.New("Validating admission control is denying all modifications")) +} + +func (alwaysValidatingDeny) Handles(operation admission.Operation) bool { return true } @@ -258,11 +268,6 @@ func handle(storage map[string]rest.Storage) http.Handler { return handleInternal(storage, admissionControl, selfLinker, nil) } -// tests with a deny admission controller -func handleDeny(storage map[string]rest.Storage) http.Handler { - return handleInternal(storage, alwaysDeny{}, selfLinker, nil) -} - // tests using the new namespace scope mechanism func handleNamespaced(storage map[string]rest.Storage) http.Handler { return handleInternal(storage, admissionControl, selfLinker, nil) @@ -529,6 +534,9 @@ func (storage *SimpleRESTStorage) Create(ctx request.Context, obj runtime.Object if storage.injectedFunction != nil { obj, err = storage.injectedFunction(obj) } + if err := createValidation(obj); err != nil { + return nil, err + } return obj, err } @@ -545,6 +553,9 @@ func (storage *SimpleRESTStorage) Update(ctx request.Context, name string, objIn if storage.injectedFunction != nil { obj, err = storage.injectedFunction(obj) } + if err := updateValidation(&storage.item, obj); err != nil { + return nil, false, err + } return obj, false, err } @@ -725,6 +736,9 @@ func (storage *NamedCreaterRESTStorage) Create(ctx request.Context, name string, if storage.injectedFunction != nil { obj, err = storage.injectedFunction(obj) } + if err := createValidation(obj); err != nil { + return nil, err + } return obj, err } @@ -2813,22 +2827,27 @@ func TestLegacyDeleteIgnoresOptions(t *testing.T) { } func TestDeleteInvokesAdmissionControl(t *testing.T) { - storage := map[string]rest.Storage{} - simpleStorage := SimpleRESTStorage{} - ID := "id" - storage["simple"] = &simpleStorage - handler := handleDeny(storage) - server := httptest.NewServer(handler) - defer server.Close() + // TODO: remove mutating deny when we removed it from the endpoint implementation and ported all plugins + for _, admit := range []admission.Interface{alwaysMutatingDeny{}, alwaysValidatingDeny{}} { + t.Logf("Testing %T", admit) - client := http.Client{} - request, err := http.NewRequest("DELETE", server.URL+"/"+prefix+"/"+testGroupVersion.Group+"/"+testGroupVersion.Version+"/namespaces/default/simple/"+ID, nil) - response, err := client.Do(request) - if err != nil { - t.Errorf("unexpected error: %v", err) - } - if response.StatusCode != http.StatusForbidden { - t.Errorf("Unexpected response %#v", response) + storage := map[string]rest.Storage{} + simpleStorage := SimpleRESTStorage{} + ID := "id" + storage["simple"] = &simpleStorage + handler := handleInternal(storage, admit, selfLinker, nil) + server := httptest.NewServer(handler) + defer server.Close() + + client := http.Client{} + request, err := http.NewRequest("DELETE", server.URL+"/"+prefix+"/"+testGroupVersion.Group+"/"+testGroupVersion.Version+"/namespaces/default/simple/"+ID, nil) + response, err := client.Do(request) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + if response.StatusCode != http.StatusForbidden { + t.Errorf("Unexpected response %#v", response) + } } } @@ -2971,38 +2990,42 @@ func TestUpdate(t *testing.T) { } func TestUpdateInvokesAdmissionControl(t *testing.T) { - storage := map[string]rest.Storage{} - simpleStorage := SimpleRESTStorage{} - ID := "id" - storage["simple"] = &simpleStorage - handler := handleDeny(storage) - server := httptest.NewServer(handler) - defer server.Close() + for _, admit := range []admission.Interface{alwaysMutatingDeny{}, alwaysValidatingDeny{}} { + t.Logf("Testing %T", admit) - item := &genericapitesting.Simple{ - ObjectMeta: metav1.ObjectMeta{ - Name: ID, - Namespace: metav1.NamespaceDefault, - }, - Other: "bar", - } - body, err := runtime.Encode(testCodec, item) - if err != nil { - // The following cases will fail, so die now - t.Fatalf("unexpected error: %v", err) - } + storage := map[string]rest.Storage{} + simpleStorage := SimpleRESTStorage{} + ID := "id" + storage["simple"] = &simpleStorage + handler := handleInternal(storage, admit, selfLinker, nil) + server := httptest.NewServer(handler) + defer server.Close() - client := http.Client{} - request, err := http.NewRequest("PUT", server.URL+"/"+prefix+"/"+testGroupVersion.Group+"/"+testGroupVersion.Version+"/namespaces/default/simple/"+ID, bytes.NewReader(body)) - response, err := client.Do(request) - if err != nil { - t.Errorf("unexpected error: %v", err) - } - dump, _ := httputil.DumpResponse(response, true) - t.Log(string(dump)) + item := &genericapitesting.Simple{ + ObjectMeta: metav1.ObjectMeta{ + Name: ID, + Namespace: metav1.NamespaceDefault, + }, + Other: "bar", + } + body, err := runtime.Encode(testCodec, item) + if err != nil { + // The following cases will fail, so die now + t.Fatalf("unexpected error: %v", err) + } - if response.StatusCode != http.StatusForbidden { - t.Errorf("Unexpected response %#v", response) + client := http.Client{} + request, err := http.NewRequest("PUT", server.URL+"/"+prefix+"/"+testGroupVersion.Group+"/"+testGroupVersion.Version+"/namespaces/default/simple/"+ID, bytes.NewReader(body)) + response, err := client.Do(request) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + dump, _ := httputil.DumpResponse(response, true) + t.Log(string(dump)) + + if response.StatusCode != http.StatusForbidden { + t.Errorf("Unexpected response %#v", response) + } } } @@ -3602,49 +3625,53 @@ func TestCreateInNamespace(t *testing.T) { } } -func TestCreateInvokesAdmissionControl(t *testing.T) { - storage := SimpleRESTStorage{ - injectedFunction: func(obj runtime.Object) (runtime.Object, error) { - time.Sleep(5 * time.Millisecond) - return obj, nil - }, - } - selfLinker := &setTestSelfLinker{ - t: t, - name: "bar", - namespace: "other", - expectedSet: "/" + prefix + "/" + testGroupVersion.Group + "/" + testGroupVersion.Version + "/namespaces/other/foo/bar", - } - handler := handleInternal(map[string]rest.Storage{"foo": &storage}, alwaysDeny{}, selfLinker, nil) - server := httptest.NewServer(handler) - defer server.Close() - client := http.Client{} +func TestCreateInvokeAdmissionControl(t *testing.T) { + for _, admit := range []admission.Interface{alwaysMutatingDeny{}, alwaysValidatingDeny{}} { + t.Logf("Testing %T", admit) - simple := &genericapitesting.Simple{ - Other: "bar", - } - data, err := runtime.Encode(testCodec, simple) - if err != nil { - t.Errorf("unexpected error: %v", err) - } - request, err := http.NewRequest("POST", server.URL+"/"+prefix+"/"+testGroupVersion.Group+"/"+testGroupVersion.Version+"/namespaces/other/foo", bytes.NewBuffer(data)) - if err != nil { - t.Errorf("unexpected error: %v", err) - } + storage := SimpleRESTStorage{ + injectedFunction: func(obj runtime.Object) (runtime.Object, error) { + time.Sleep(5 * time.Millisecond) + return obj, nil + }, + } + selfLinker := &setTestSelfLinker{ + t: t, + name: "bar", + namespace: "other", + expectedSet: "/" + prefix + "/" + testGroupVersion.Group + "/" + testGroupVersion.Version + "/namespaces/other/foo/bar", + } + handler := handleInternal(map[string]rest.Storage{"foo": &storage}, admit, selfLinker, nil) + server := httptest.NewServer(handler) + defer server.Close() + client := http.Client{} - wg := sync.WaitGroup{} - wg.Add(1) - var response *http.Response - go func() { - response, err = client.Do(request) - wg.Done() - }() - wg.Wait() - if err != nil { - t.Errorf("unexpected error: %v", err) - } - if response.StatusCode != http.StatusForbidden { - t.Errorf("Unexpected status: %d, Expected: %d, %#v", response.StatusCode, http.StatusForbidden, response) + simple := &genericapitesting.Simple{ + Other: "bar", + } + data, err := runtime.Encode(testCodec, simple) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + request, err := http.NewRequest("POST", server.URL+"/"+prefix+"/"+testGroupVersion.Group+"/"+testGroupVersion.Version+"/namespaces/other/foo", bytes.NewBuffer(data)) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + + wg := sync.WaitGroup{} + wg.Add(1) + var response *http.Response + go func() { + response, err = client.Do(request) + wg.Done() + }() + wg.Wait() + if err != nil { + t.Errorf("unexpected error: %v", err) + } + if response.StatusCode != http.StatusForbidden { + t.Errorf("Unexpected status: %d, Expected: %d, %#v", response.StatusCode, http.StatusForbidden, response) + } } } diff --git a/pkg/endpoints/handlers/rest_test.go b/pkg/endpoints/handlers/rest_test.go index 7a3c1e4e3..e6dc7b2cd 100644 --- a/pkg/endpoints/handlers/rest_test.go +++ b/pkg/endpoints/handlers/rest_test.go @@ -186,6 +186,16 @@ func (p *testPatcher) Update(ctx request.Context, name string, objInfo rest.Upda return nil, false, apierrors.NewConflict(example.Resource("pods"), inPod.Name, fmt.Errorf("existing %v, new %v", p.updatePod.ResourceVersion, inPod.ResourceVersion)) } + if currentPod == nil { + if err := createValidation(currentPod); err != nil { + return nil, false, err + } + } else { + if err := updateValidation(currentPod, inPod); err != nil { + return nil, false, err + } + } + return inPod, false, nil } @@ -236,7 +246,7 @@ type patchTestCase struct { // admission chain to use, nil is fine admissionMutation mutateObjectUpdateFunc - admissionValidation rest.ValidateObjectUpdateFunc // TODO: add test for this + admissionValidation rest.ValidateObjectUpdateFunc // startingPod is used as the starting point for the first Update startingPod *example.Pod @@ -557,35 +567,76 @@ func TestPatchWithAdmissionRejection(t *testing.T) { fifteen := int64(15) thirty := int64(30) - tc := &patchTestCase{ - name: "TestPatchWithAdmissionRejection", - - admissionMutation: func(updatedObject runtime.Object, currentObject runtime.Object) error { - return errors.New("admission failure") - }, - - startingPod: &example.Pod{}, - changedPod: &example.Pod{}, - updatePod: &example.Pod{}, - - expectedError: "admission failure", + type Test struct { + name string + admissionMutation mutateObjectUpdateFunc + admissionValidation rest.ValidateObjectUpdateFunc + expectedError string } + for _, test := range []Test{ + { + name: "TestPatchWithMutatingAdmissionRejection", + admissionMutation: func(updatedObject runtime.Object, currentObject runtime.Object) error { + return errors.New("mutating admission failure") + }, + admissionValidation: rest.ValidateAllObjectUpdateFunc, + expectedError: "mutating admission failure", + }, + { + name: "TestPatchWithValidatingAdmissionRejection", + admissionMutation: rest.ValidateAllObjectUpdateFunc, + admissionValidation: func(updatedObject runtime.Object, currentObject runtime.Object) error { + return errors.New("validating admission failure") + }, + expectedError: "validating admission failure", + }, + { + name: "TestPatchWithBothAdmissionRejections", + admissionMutation: func(updatedObject runtime.Object, currentObject runtime.Object) error { + return errors.New("mutating admission failure") + }, + admissionValidation: func(updatedObject runtime.Object, currentObject runtime.Object) error { + return errors.New("validating admission failure") + }, + expectedError: "mutating admission failure", + }, + } { + tc := &patchTestCase{ + name: test.name, - tc.startingPod.Name = name - tc.startingPod.Namespace = namespace - tc.startingPod.UID = uid - tc.startingPod.ResourceVersion = "1" - tc.startingPod.APIVersion = examplev1.SchemeGroupVersion.String() - tc.startingPod.Spec.ActiveDeadlineSeconds = &fifteen + admissionMutation: test.admissionMutation, + admissionValidation: test.admissionValidation, - tc.changedPod.Name = name - tc.changedPod.Namespace = namespace - tc.changedPod.UID = uid - tc.changedPod.ResourceVersion = "1" - tc.changedPod.APIVersion = examplev1.SchemeGroupVersion.String() - tc.changedPod.Spec.ActiveDeadlineSeconds = &thirty + startingPod: &example.Pod{}, + changedPod: &example.Pod{}, + updatePod: &example.Pod{}, - tc.Run(t) + expectedError: test.expectedError, + } + + tc.startingPod.Name = name + tc.startingPod.Namespace = namespace + tc.startingPod.UID = uid + tc.startingPod.ResourceVersion = "1" + tc.startingPod.APIVersion = examplev1.SchemeGroupVersion.String() + tc.startingPod.Spec.ActiveDeadlineSeconds = &fifteen + + tc.changedPod.Name = name + tc.changedPod.Namespace = namespace + tc.changedPod.UID = uid + tc.changedPod.ResourceVersion = "1" + tc.changedPod.APIVersion = examplev1.SchemeGroupVersion.String() + tc.changedPod.Spec.ActiveDeadlineSeconds = &thirty + + tc.updatePod.Name = name + tc.updatePod.Namespace = namespace + tc.updatePod.UID = uid + tc.updatePod.ResourceVersion = "1" + tc.updatePod.APIVersion = examplev1.SchemeGroupVersion.String() + tc.updatePod.Spec.ActiveDeadlineSeconds = &fifteen + + tc.Run(t) + } } func TestPatchWithVersionConflictThenAdmissionFailure(t *testing.T) { diff --git a/pkg/registry/generic/registry/store_test.go b/pkg/registry/generic/registry/store_test.go index b49b382c3..9e632113c 100644 --- a/pkg/registry/generic/registry/store_test.go +++ b/pkg/registry/generic/registry/store_test.go @@ -311,8 +311,14 @@ func TestStoreCreate(t *testing.T) { defaultDeleteStrategy := testRESTStrategy{scheme, names.SimpleNameGenerator, true, false, true} registry.DeleteStrategy = testGracefulStrategy{defaultDeleteStrategy} + // create the object with denying admission + objA, err := registry.Create(testContext, podA, denyCreateValidation, false) + if err == nil { + t.Errorf("Expected admission error: %v", err) + } + // create the object - objA, err := registry.Create(testContext, podA, rest.ValidateAllObjectFunc, false) + objA, err = registry.Create(testContext, podA, rest.ValidateAllObjectFunc, false) if err != nil { t.Errorf("Unexpected error: %v", err) } @@ -609,31 +615,53 @@ func TestStoreUpdate(t *testing.T) { destroyFunc, registry := NewTestGenericStoreRegistry(t) defer destroyFunc() - // Test1 try to update a non-existing node - _, _, err := registry.Update(testContext, podA.Name, rest.DefaultUpdatedObjectInfo(podA), rest.ValidateAllObjectFunc, rest.ValidateAllObjectUpdateFunc) + // try to update a non-existing node with denying admission, should still return NotFound + _, _, err := registry.Update(testContext, podA.Name, rest.DefaultUpdatedObjectInfo(podA), denyCreateValidation, denyUpdateValidation) if !errors.IsNotFound(err) { t.Errorf("Unexpected error: %v", err) } - // Test2 createIfNotFound and verify + // try to update a non-existing node + _, _, err = registry.Update(testContext, podA.Name, rest.DefaultUpdatedObjectInfo(podA), rest.ValidateAllObjectFunc, rest.ValidateAllObjectUpdateFunc) + if !errors.IsNotFound(err) { + t.Errorf("Unexpected error: %v", err) + } + + // allow creation registry.UpdateStrategy.(*testRESTStrategy).allowCreateOnUpdate = true + + // createIfNotFound with denying create admission + _, _, err = registry.Update(testContext, podA.Name, rest.DefaultUpdatedObjectInfo(podA), denyCreateValidation, rest.ValidateAllObjectUpdateFunc) + if err == nil { + t.Errorf("expected admission error on create") + } + + // createIfNotFound and verify if !updateAndVerify(t, testContext, registry, podA) { t.Errorf("Unexpected error updating podA") } + + // forbid creation again registry.UpdateStrategy.(*testRESTStrategy).allowCreateOnUpdate = false - // Test3 outofDate + // outofDate _, _, err = registry.Update(testContext, podAWithResourceVersion.Name, rest.DefaultUpdatedObjectInfo(podAWithResourceVersion), rest.ValidateAllObjectFunc, rest.ValidateAllObjectUpdateFunc) if !errors.IsConflict(err) { t.Errorf("Unexpected error updating podAWithResourceVersion: %v", err) } - // Test4 normal update and verify + // try to update with denying admission + _, _, err = registry.Update(testContext, podA.Name, rest.DefaultUpdatedObjectInfo(podA), rest.ValidateAllObjectFunc, denyUpdateValidation) + if err == nil { + t.Errorf("expected admission error on update") + } + + // normal update and verify if !updateAndVerify(t, testContext, registry, podB) { t.Errorf("Unexpected error updating podB") } - // Test5 unconditional update + // unconditional update // NOTE: The logic for unconditional updates doesn't make sense to me, and imho should be removed. // doUnconditionalUpdate := resourceVersion == 0 && e.UpdateStrategy.AllowUnconditionalUpdate() // ^^ That condition can *never be true due to the creation of root objects. @@ -1969,3 +1997,11 @@ func TestQualifiedResource(t *testing.T) { t.Fatalf("Unexpected error: %#v", err) } } + +func denyCreateValidation(obj runtime.Object) error { + return fmt.Errorf("admission denied") +} + +func denyUpdateValidation(obj, old runtime.Object) error { + return fmt.Errorf("admission denied") +}