Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

webhook integration tests for non persistent resources #76959

Merged
merged 2 commits into from Apr 24, 2019

Conversation

@sbezverk
Copy link
Contributor

commented Apr 23, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

/cc @liggitt

@k8s-ci-robot k8s-ci-robot requested a review from liggitt Apr 23, 2019

@sbezverk

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

/release-note-none

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

if createValidation != nil {

This comment has been minimized.

Copy link
@liggitt

liggitt Apr 23, 2019

Member

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

This comment has been minimized.

Copy link
@sbezverk

sbezverk Apr 23, 2019

Author Contributor

done

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

if createValidation != nil {

This comment has been minimized.

Copy link
@liggitt

liggitt Apr 23, 2019

Member

move to line 65, after validation, before action

This comment has been minimized.

Copy link
@sbezverk

sbezverk Apr 23, 2019

Author Contributor

done

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

if createValidation != nil {

This comment has been minimized.

Copy link
@liggitt

liggitt Apr 23, 2019

Member

move to line 62

This comment has been minimized.

Copy link
@sbezverk

sbezverk Apr 23, 2019

Author Contributor

done

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

if createValidation != nil {

This comment has been minimized.

Copy link
@liggitt

liggitt Apr 23, 2019

Member

move to line 68

This comment has been minimized.

Copy link
@sbezverk

sbezverk Apr 23, 2019

Author Contributor

done

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

if createValidation != nil {

This comment has been minimized.

Copy link
@liggitt

liggitt Apr 23, 2019

Member

move to line 57

This comment has been minimized.

Copy link
@sbezverk

sbezverk Apr 23, 2019

Author Contributor

done

@@ -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 comment has been minimized.

Copy link
@liggitt

liggitt Apr 23, 2019

Member

this function isn't used, remove?

This comment has been minimized.

Copy link
@sbezverk

sbezverk Apr 23, 2019

Author Contributor

done

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},

This comment has been minimized.

Copy link
@liggitt

liggitt Apr 23, 2019

Member

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

@@ -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{

This comment has been minimized.

Copy link
@liggitt

liggitt Apr 23, 2019

Member

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

This comment has been minimized.

Copy link
@liggitt

liggitt Apr 23, 2019

Member

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

This comment has been minimized.

Copy link
@sbezverk

sbezverk Apr 23, 2019

Author Contributor

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..

This comment has been minimized.

Copy link
@liggitt
@@ -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) {

This comment has been minimized.

Copy link
@liggitt

liggitt Apr 23, 2019

Member

inline this into testNonPersistentResourceCreate, we have no other callers

@liggitt

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

looks good, thanks for helping knock these out

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

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

@sbezverk sbezverk force-pushed the sbezverk:non_persistent_objects branch from 56fcb77 to cad90b4 Apr 23, 2019

@sbezverk sbezverk force-pushed the sbezverk:non_persistent_objects branch from cad90b4 to ae6061f Apr 23, 2019

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

This comment has been minimized.

Copy link
@liggitt

liggitt Apr 23, 2019

Member

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

This comment has been minimized.

Copy link
@sbezverk

sbezverk Apr 23, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@liggitt

liggitt Apr 23, 2019

Member

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

Adding non persistent review test
Signed-off-by: Serguei Bezverkhi <sbezverk@cisco.com>

@sbezverk sbezverk force-pushed the sbezverk:non_persistent_objects branch from ae6061f to 6fe28ee Apr 23, 2019

@liggitt

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

/lgtm

@sbezverk

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

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

@sbezverk

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

@liggitt Could you approve it please..

@liggitt

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

[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:

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

@liggitt

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

/priority important-soon

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

20 checks passed

cla/linuxfoundation sbezverk authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.