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

Sending existing object to the webhook for the DELETE verb #76346

Merged
merged 4 commits into from May 18, 2019

Conversation

@caesarxuchao
Copy link
Member

commented Apr 9, 2019

Based on #66535, with these major changes:

  • The deleteValidation is plumbed to the GuaranteedUpdate and etcd3#Delete, so that it's always called when the update and delete retry on conflicts.

/assign
/sig api-machinery
/kind bug

For admission webhooks registered for DELETE operations on k8s built APIs or CRDs, the apiserver now sends the existing object as admissionRequest.Request.OldObject to the webhook. 
For custom apiservers they uses the generic registry in the apiserver library, they get this behavior automatically.
@@ -198,13 +206,6 @@ var _ = SIGDescribe("AdmissionWebhook", func() {
testMultiVersionCustomResourceWebhook(f, testcrd)
})

It("Should deny crd creation", func() {

This comment has been minimized.

Copy link
@caesarxuchao

caesarxuchao Apr 10, 2019

Author Member

This is a duplicate of line 183.

@caesarxuchao caesarxuchao force-pushed the caesarxuchao:delete-admission-objects branch 3 times, most recently from 8a3bd0f to 35c38f7 Apr 10, 2019

if err != nil && storage.IsTransformerError(err) && preconditions != nil {
return err
}
if err != nil && storage.IsTransformerError(err) && preconditions == nil {

This comment has been minimized.

Copy link
@caesarxuchao

caesarxuchao Apr 11, 2019

Author Member

This might be controversial.

This comment has been minimized.

Copy link
@liggitt

liggitt Apr 16, 2019

Member

this is interesting. wouldn't malformed data in etcd also prevent us from listing and finding out what the problematic key was? I'm trying to understand if this special handling actually enables us to resolve the issue, or if it's just a step in that direction.

This comment has been minimized.

Copy link
@caesarxuchao

caesarxuchao Apr 17, 2019

Author Member

Good point. The error message send by the LIST handler does include the key:

return storage.NewInternalErrorf("unable to transform key %q: %v", kv.Key, err)

So this special handling enables us to resolve the issue.

@caesarxuchao caesarxuchao force-pushed the caesarxuchao:delete-admission-objects branch 4 times, most recently from 6bafd02 to f6b94b4 Apr 11, 2019

@caesarxuchao

This comment has been minimized.

Copy link
Member Author

commented Apr 11, 2019

/retest

@caesarxuchao

This comment has been minimized.

Copy link
Member Author

commented Apr 11, 2019

@caesarxuchao caesarxuchao force-pushed the caesarxuchao:delete-admission-objects branch from fe57b61 to 4696a30 May 16, 2019

@k8s-ci-robot k8s-ci-robot added size/XL and removed size/XXL labels May 16, 2019

@caesarxuchao caesarxuchao force-pushed the caesarxuchao:delete-admission-objects branch from 4696a30 to 9e05a47 May 16, 2019

@caesarxuchao

This comment has been minimized.

Copy link
Member Author

commented May 16, 2019

@liggitt I cherry-picked your #77952 to get the integration tests working, I would need a rebase after #77952 merged. The PR is otherwise ready for another round of review. Thank you for the meticulous reviews.

@liggitt

This comment has been minimized.

Copy link
Member

commented May 17, 2019

/lgtm
/approve

needs a rebase since #77952 merged

yue9944882 and others added 4 commits Jul 24, 2018
Cherrypicking #66535
validate deletion admission object

backward compatibility: add validation for direct storage delete calls

apply nil validation to existing tests

revert behavior changes in deleteCollection call

fixes validation on wiring graceful deletion

remove nil validation check

continue admission check on not found error
Run deleteValidation at the storage layer so that it will be retried on
conflict.

Adding unit test verify that deleteValidation is retried.

adding e2e test verifying the webhook can intercept configmap and custom
resource deletion, and the existing object is sent via the
admissionreview.OldObject.

update the admission integration test to verify that the existing object
is passed to the deletion admission webhook as oldObject, in case of an
immediate deletion and in case of an update-on-delete.

@caesarxuchao caesarxuchao force-pushed the caesarxuchao:delete-admission-objects branch from 9e05a47 to 6d40e3c May 17, 2019

@k8s-ci-robot k8s-ci-robot removed the lgtm label May 17, 2019

@caesarxuchao

This comment has been minimized.

Copy link
Member Author

commented May 17, 2019

/retest

1 similar comment
@caesarxuchao

This comment has been minimized.

Copy link
Member Author

commented May 17, 2019

/retest

@liggitt

This comment has been minimized.

Copy link
Member

commented May 17, 2019

/lgtm
/approve
/priority important-soon

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@k8s-ci-robot k8s-ci-robot merged commit df8e241 into kubernetes:master May 18, 2019

20 checks passed

cla/linuxfoundation caesarxuchao 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
@yue9944882

This comment has been minimized.

Copy link
Member

commented May 18, 2019

awesome! 🎉

@liggitt

This comment has been minimized.

Copy link
Member

commented May 31, 2019

included in docs PR for 1.15 at kubernetes/website#14671

@roycaihw

This comment has been minimized.

Copy link
Member

commented Sep 17, 2019

/area admission-control

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.