webhook integration tests for non persistent resources #76959

sbezverk · 2019-04-23T18:55:15Z

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Adds integration tests for webhook admission for:
non persistent resources

Which issue(s) this PR fixes:
xref #76907

Does this PR introduce a user-facing change?:

Validating admission webhooks are now properly called for CREATE operations on the following resources: tokenreviews, subjectaccessreviews, localsubjectaccessreviews, selfsubjectaccessreviews, selfsubjectrulesreviews

sbezverk · 2019-04-23T18:56:16Z

/cc @liggitt

sbezverk · 2019-04-23T18:57:12Z

/release-note-none

liggitt · 2019-04-23T19:18:11Z

pkg/registry/authentication/tokenreview/storage.go

@@ -108,5 +108,11 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation
 		tokenReview.Status.Audiences = resp.Audiences
 	}

+	if createValidation != nil {


do this on line 70, after we've validated but before we take action based on the review

liggitt · 2019-04-23T19:18:50Z

pkg/registry/authorization/localsubjectaccessreview/rest.go

@@ -75,5 +75,11 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation
 		localSubjectAccessReview.Status.EvaluationError = evaluationErr.Error()
 	}

+	if createValidation != nil {


move to line 65, after validation, before action

liggitt · 2019-04-23T19:19:13Z

pkg/registry/authorization/selfsubjectaccessreview/rest.go

@@ -78,5 +78,11 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation
 		selfSAR.Status.EvaluationError = evaluationErr.Error()
 	}

+	if createValidation != nil {


move to line 62

liggitt · 2019-04-23T19:19:34Z

pkg/registry/authorization/selfsubjectrulesreview/rest.go

@@ -79,6 +79,12 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation
 		ret.Status.EvaluationError = err.Error()
 	}

+	if createValidation != nil {


move to line 68

liggitt · 2019-04-23T19:20:09Z

pkg/registry/authorization/subjectaccessreview/rest.go

@@ -67,5 +67,11 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation
 		subjectAccessReview.Status.EvaluationError = evaluationErr.Error()
 	}

+	if createValidation != nil {


move to line 57

liggitt · 2019-04-23T19:22:10Z

test/integration/apiserver/admissionwebhook/admission_test.go

@@ -852,6 +926,25 @@ func createOrGetResource(client dynamic.Interface, gvr schema.GroupVersionResour
 	return client.Resource(gvr).Namespace(ns).Create(stubObj, metav1.CreateOptions{})
 }

+func createOrGetNonPersistentResource(client dynamic.Interface, gvr schema.GroupVersionResource, resource metav1.APIResource) (*unstructured.Unstructured, error) {


this function isn't used, remove?

liggitt · 2019-04-23T19:24:31Z

test/integration/apiserver/admissionwebhook/admission_test.go

+		gvr("", "v1", "namespaces"):                                         {"delete": testNamespaceDelete},
+		gvr("apps", "v1beta1", "deployments/rollback"):                      {"create": testDeploymentRollback},
+		gvr("extensions", "v1beta1", "deployments/rollback"):                {"create": testDeploymentRollback},
+		gvr("authentication.k8s.io", "v1", "tokenreviews"):                  {"create": testNonPersistentResourceCreate},


nit, add a blank line here to group all the types that use testNonPersistentResourceCreate (will need to be rebased anyway)

liggitt · 2019-04-23T19:26:09Z

test/integration/apiserver/admissionwebhook/admission_test.go

@@ -128,18 +129,52 @@ var (
 	parentResources = map[schema.GroupVersionResource]schema.GroupVersionResource{
 		gvr("extensions", "v1beta1", "replicationcontrollers/scale"): gvr("", "v1", "replicationcontrollers"),
 	}
+
+	nonPersistentObjects = map[schema.GroupVersionResource]etcd.StorageData{


nit: nonPersistentReviewObjects (to distinguish between this and other non-persistent subresources)

simplify this to map[schema.GroupVersionResource]string, no need to nest the Stub field

I would prefer to keep the same name nonPersistentObjects in case GetStubObject can be used as a generic func to get stubs either from persistent or a non persistent data sources. If Reviews specific name is used then adding other non persistent type of object would require change of func..

liggitt · 2019-04-23T19:32:16Z

test/integration/apiserver/admissionwebhook/admission_test.go

@@ -833,6 +894,19 @@ func getStubObj(gvr schema.GroupVersionResource, resource metav1.APIResource) (*
 	return stubObj, nil
 }

+func getNonPersistentStubObj(gvr schema.GroupVersionResource, resource metav1.APIResource) (*unstructured.Unstructured, error) {


inline this into testNonPersistentResourceCreate, we have no other callers

liggitt · 2019-04-23T19:34:21Z

looks good, thanks for helping knock these out

a few comments, and will need a rebase once #76911 merges

Signed-off-by: Serguei Bezverkhi <sbezverk@cisco.com>

liggitt · 2019-04-23T21:38:06Z

test/integration/apiserver/admissionwebhook/admission_test.go

 	}
+	// excludedResources lists resources / verb combinations that are not yet tested. this set should trend to zero.
+	excludedResources = map[schema.GroupVersionResource]sets.String{}


go ahead and delete excludedResources and drop the code that checks it. 100% coverage 🎉

ok, will do.. but we do not want to keep it as a place holder in case it is needed?

I don't want a convenient way for people to skip this test :)

if we really need it in the future for some unforeseen reason, we can add it back

test/integration/apiserver/admissionwebhook/admission_test.go

Signed-off-by: Serguei Bezverkhi <sbezverk@cisco.com>

liggitt · 2019-04-23T22:10:11Z

/lgtm

sbezverk · 2019-04-23T23:21:26Z

/test pull-kubernetes-kubemark-e2e-gce-big

sbezverk · 2019-04-24T00:58:56Z

@liggitt Could you approve it please..

liggitt · 2019-04-24T01:08:03Z

/approve

k8s-ci-robot · 2019-04-24T01:08:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, sbezverk

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/registry/OWNERS~~ [liggitt]
~~test/OWNERS~~ [liggitt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

liggitt · 2019-04-24T01:08:16Z

/priority important-soon

k8s-ci-robot requested review from janetkuo and saad-ali April 23, 2019 18:55

k8s-ci-robot added area/test sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Apr 23, 2019

k8s-ci-robot requested a review from liggitt April 23, 2019 18:56

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Apr 23, 2019

liggitt mentioned this pull request Apr 23, 2019

Complete integration testing for admission webhooks #76907

Closed

5 tasks

liggitt reviewed Apr 23, 2019

View reviewed changes

no admission logic

cc7700a

Signed-off-by: Serguei Bezverkhi <sbezverk@cisco.com>

liggitt reviewed Apr 23, 2019

View reviewed changes

test/integration/apiserver/admissionwebhook/admission_test.go Show resolved Hide resolved

Adding non persistent review test

6fe28ee

Signed-off-by: Serguei Bezverkhi <sbezverk@cisco.com>

k8s-ci-robot assigned liggitt Apr 23, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 23, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 24, 2019

k8s-ci-robot merged commit 279d4e1 into kubernetes:master Apr 24, 2019

liggitt removed the sig/auth Categorizes an issue or PR as relevant to SIG Auth. label Feb 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

webhook integration tests for non persistent resources #76959

webhook integration tests for non persistent resources #76959

sbezverk commented Apr 23, 2019 •

edited by liggitt

sbezverk commented Apr 23, 2019

sbezverk commented Apr 23, 2019

liggitt Apr 23, 2019

sbezverk Apr 23, 2019

liggitt Apr 23, 2019

sbezverk Apr 23, 2019

liggitt Apr 23, 2019

sbezverk Apr 23, 2019

liggitt Apr 23, 2019

sbezverk Apr 23, 2019

liggitt Apr 23, 2019

sbezverk Apr 23, 2019

liggitt Apr 23, 2019

sbezverk Apr 23, 2019

liggitt Apr 23, 2019

liggitt Apr 23, 2019

liggitt Apr 23, 2019

sbezverk Apr 23, 2019

liggitt Apr 23, 2019

liggitt Apr 23, 2019

liggitt commented Apr 23, 2019

liggitt Apr 23, 2019

sbezverk Apr 23, 2019

liggitt Apr 23, 2019

liggitt commented Apr 23, 2019

sbezverk commented Apr 23, 2019

sbezverk commented Apr 24, 2019

liggitt commented Apr 24, 2019

k8s-ci-robot commented Apr 24, 2019

liggitt commented Apr 24, 2019

webhook integration tests for non persistent resources #76959

webhook integration tests for non persistent resources #76959

Conversation

sbezverk commented Apr 23, 2019 • edited by liggitt

sbezverk commented Apr 23, 2019

sbezverk commented Apr 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Apr 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Apr 23, 2019

sbezverk commented Apr 23, 2019

sbezverk commented Apr 24, 2019

liggitt commented Apr 24, 2019

k8s-ci-robot commented Apr 24, 2019

liggitt commented Apr 24, 2019

sbezverk commented Apr 23, 2019 •

edited by liggitt