Merge pull request #55228 from sttts/sttts-validation-admission-tests

Automatic merge from submit-queue (batch tested with PRs 55331, 55272, 55228, 49763, 55242). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

apiserver: add validating admission tests

Follow-up of https://github.com/kubernetes/kubernetes/pull/54484

This includes tests
- in endpoint tests,
- in generic registry,
- in patch handler,
- in admission chain.

staging/src/k8s.io/apiserver/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
+	h.validateCalled = true
-	if h.admit {
+	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,
+		name:     name,
-		admit:   admit,
+		admit:    accept,
-		Handler: NewHandler(ops...),
+		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)
+			}
 		}
 	}
 }

staging/src/k8s.io/apiserver/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) {
+func (alwaysMutatingDeny) Admit(a admission.Attributes) (err error) {
-	return admission.NewForbidden(a, errors.New("Admission control is denying all modifications"))
+	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{}
+	// TODO: remove mutating deny when we removed it from the endpoint implementation and ported all plugins
-	simpleStorage := SimpleRESTStorage{}
+	for _, admit := range []admission.Interface{alwaysMutatingDeny{}, alwaysValidatingDeny{}} {
-	ID := "id"
+		t.Logf("Testing %T", admit)
-	storage["simple"] = &simpleStorage
-	handler := handleDeny(storage)
-	server := httptest.NewServer(handler)
-	defer server.Close()
 
-	client := http.Client{}
+		storage := map[string]rest.Storage{}
-	request, err := http.NewRequest("DELETE", server.URL+"/"+prefix+"/"+testGroupVersion.Group+"/"+testGroupVersion.Version+"/namespaces/default/simple/"+ID, nil)
+		simpleStorage := SimpleRESTStorage{}
-	response, err := client.Do(request)
+		ID := "id"
-	if err != nil {
+		storage["simple"] = &simpleStorage
-		t.Errorf("unexpected error: %v", err)
+		handler := handleInternal(storage, admit, selfLinker, nil)
-	}
+		server := httptest.NewServer(handler)
-	if response.StatusCode != http.StatusForbidden {
+		defer server.Close()
-		t.Errorf("Unexpected response %#v", response)
+
+		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{}
+	for _, admit := range []admission.Interface{alwaysMutatingDeny{}, alwaysValidatingDeny{}} {
-	simpleStorage := SimpleRESTStorage{}
+		t.Logf("Testing %T", admit)
-	ID := "id"
-	storage["simple"] = &simpleStorage
-	handler := handleDeny(storage)
-	server := httptest.NewServer(handler)
-	defer server.Close()
 
-	item := &genericapitesting.Simple{
+		storage := map[string]rest.Storage{}
-		ObjectMeta: metav1.ObjectMeta{
+		simpleStorage := SimpleRESTStorage{}
-			Name:      ID,
+		ID := "id"
-			Namespace: metav1.NamespaceDefault,
+		storage["simple"] = &simpleStorage
-		},
+		handler := handleInternal(storage, admit, selfLinker, nil)
-		Other: "bar",
+		server := httptest.NewServer(handler)
-	}
+		defer server.Close()
-	body, err := runtime.Encode(testCodec, item)
-	if err != nil {
-		// The following cases will fail, so die now
-		t.Fatalf("unexpected error: %v", err)
-	}
 
-	client := http.Client{}
+		item := &genericapitesting.Simple{
-	request, err := http.NewRequest("PUT", server.URL+"/"+prefix+"/"+testGroupVersion.Group+"/"+testGroupVersion.Version+"/namespaces/default/simple/"+ID, bytes.NewReader(body))
+			ObjectMeta: metav1.ObjectMeta{
-	response, err := client.Do(request)
+				Name:      ID,
-	if err != nil {
+				Namespace: metav1.NamespaceDefault,
-		t.Errorf("unexpected error: %v", err)
+			},
-	}
+			Other: "bar",
-	dump, _ := httputil.DumpResponse(response, true)
+		}
-	t.Log(string(dump))
+		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 {
+		client := http.Client{}
-		t.Errorf("Unexpected response %#v", response)
+		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) {
+func TestCreateInvokeAdmissionControl(t *testing.T) {
-	storage := SimpleRESTStorage{
+	for _, admit := range []admission.Interface{alwaysMutatingDeny{}, alwaysValidatingDeny{}} {
-		injectedFunction: func(obj runtime.Object) (runtime.Object, error) {
+		t.Logf("Testing %T", admit)
-			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{}
 
-	simple := &genericapitesting.Simple{
+		storage := SimpleRESTStorage{
-		Other: "bar",
+			injectedFunction: func(obj runtime.Object) (runtime.Object, error) {
-	}
+				time.Sleep(5 * time.Millisecond)
-	data, err := runtime.Encode(testCodec, simple)
+				return obj, nil
-	if err != nil {
+			},
-		t.Errorf("unexpected error: %v", err)
+		}
-	}
+		selfLinker := &setTestSelfLinker{
-	request, err := http.NewRequest("POST", server.URL+"/"+prefix+"/"+testGroupVersion.Group+"/"+testGroupVersion.Version+"/namespaces/other/foo", bytes.NewBuffer(data))
+			t:           t,
-	if err != nil {
+			name:        "bar",
-		t.Errorf("unexpected error: %v", err)
+			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{}
+		simple := &genericapitesting.Simple{
-	wg.Add(1)
+			Other: "bar",
-	var response *http.Response
+		}
-	go func() {
+		data, err := runtime.Encode(testCodec, simple)
-		response, err = client.Do(request)
+		if err != nil {
-		wg.Done()
+			t.Errorf("unexpected error: %v", err)
-	}()
+		}
-	wg.Wait()
+		request, err := http.NewRequest("POST", server.URL+"/"+prefix+"/"+testGroupVersion.Group+"/"+testGroupVersion.Version+"/namespaces/other/foo", bytes.NewBuffer(data))
-	if err != nil {
+		if err != nil {
-		t.Errorf("unexpected error: %v", err)
+			t.Errorf("unexpected error: %v", err)
-	}
+		}
-	if response.StatusCode != http.StatusForbidden {
+
-		t.Errorf("Unexpected status: %d, Expected: %d, %#v", response.StatusCode, http.StatusForbidden, response)
+		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)
+		}
 	}
 }
 

staging/src/k8s.io/apiserver/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{
+	type Test struct {
-		name: "TestPatchWithAdmissionRejection",
+		name                string
-
+		admissionMutation   mutateObjectUpdateFunc
-		admissionMutation: func(updatedObject runtime.Object, currentObject runtime.Object) error {
+		admissionValidation rest.ValidateObjectUpdateFunc
-			return errors.New("admission failure")
+		expectedError       string
-		},
-
-		startingPod: &example.Pod{},
-		changedPod:  &example.Pod{},
-		updatePod:   &example.Pod{},
-
-		expectedError: "admission failure",
 	}
+	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
+			admissionMutation:   test.admissionMutation,
-	tc.startingPod.Namespace = namespace
+			admissionValidation: test.admissionValidation,
-	tc.startingPod.UID = uid
-	tc.startingPod.ResourceVersion = "1"
-	tc.startingPod.APIVersion = examplev1.SchemeGroupVersion.String()
-	tc.startingPod.Spec.ActiveDeadlineSeconds = &fifteen
 
-	tc.changedPod.Name = name
+			startingPod: &example.Pod{},
-	tc.changedPod.Namespace = namespace
+			changedPod:  &example.Pod{},
-	tc.changedPod.UID = uid
+			updatePod:   &example.Pod{},
-	tc.changedPod.ResourceVersion = "1"
-	tc.changedPod.APIVersion = examplev1.SchemeGroupVersion.String()
-	tc.changedPod.Spec.ActiveDeadlineSeconds = &thirty
 
-	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) {

staging/src/k8s.io/apiserver/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
+	// try to update a non-existing node with denying admission, should still return NotFound
-	_, _, err := registry.Update(testContext, podA.Name, rest.DefaultUpdatedObjectInfo(podA), rest.ValidateAllObjectFunc, rest.ValidateAllObjectUpdateFunc)
+	_, _, 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")
+}