Sending existing object to the webhook for the DELETE verb by caesarxuchao · Pull Request #76346 · kubernetes/kubernetes

caesarxuchao · 2019-04-09T20:50:35Z

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.

caesarxuchao · 2019-04-10T22:11:12Z

test/e2e/apimachinery/webhook.go

This is a duplicate of line 183.

caesarxuchao · 2019-04-11T00:14:48Z

staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go

This might be controversial.

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.

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

kubernetes/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go

Line 610 in 0889c3e

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

So this special handling enables us to resolve the issue.

caesarxuchao · 2019-04-11T18:00:37Z

/retest

caesarxuchao · 2019-04-11T19:16:03Z

/unassign
/assign @liggitt @mbohlool @roycaihw @yue9944882

caesarxuchao · 2019-05-08T18:00:02Z

CHAO: finalizer for roles.rbac.authorization.k8s.io/role1 is [test/k8s.io], uid is 01f1dcc1-65e4-4836-b85f-ccf0b3be4f56, rv is 1312
CHAO: again, finalizer for roles.rbac.authorization.k8s.io/role1 is [test/k8s.io], uid is 01f1dcc1-65e4-4836-b85f-ccf0b3be4f56, rv is 1312
CHAO: in updateForGracefulDeletionAndFinalizers 1, finalizer for role1 is [], uid is 01f1dcc1-65e4-4836-b85f-ccf0b3be4f56, rv is 1311
CHAO: in updateForGracefulDeletionAndFinalizers 2, finalizer for role1 is [], uid is 01f1dcc1-65e4-4836-b85f-ccf0b3be4f56, rv is 1311
CHAO: in updateForGracefulDeletionAndFinalizers 3, finalizer for role1 is [], uid is 01f1dcc1-65e4-4836-b85f-ccf0b3be4f56, rv is 1311

Somehow the rv goes backwards in this code block: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go#L873-L909.

sttts · 2019-05-10T08:23:05Z

Any chance to squash this into logical pieces for easier review?

caesarxuchao · 2019-05-14T22:42:23Z

test/integration/apiserver/admissionwebhook/admission_test.go

I had to add a specific delete test for CRD, because like namespaces, apiserver does not delete CRD on an update that removes the last pending finalizer.

should it? that seems like the same bug as the one you fixed in storage

CRD storage doesn't override Update, and only changes Delete behavior on first delete... I don't think it should require a custom test method (I'd expect removing finalizers on update of a CRD with a deletiontimestamp set to actually delete)

I removed the commit that modified shouldDeleteDuringUpdate. The function returns false unless the DeletionGracePeriodSeconds was set on the first Delete. CRD storage doesn't set the DeletionGracePeriodSeconds.

I removed my previous fix to shouldDeleteDuringUpdate because that would make Namespaces misbehave.

I tracked the issue in #77944. We can solve it in a followup.

caesarxuchao · 2019-05-14T22:56:37Z

staging/src/k8s.io/apiserver/pkg/storage/etcd3/store_test.go

This matches the existing behavior:

kubernetes/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store_test.go

Lines 743 to 750 in 08afeb8

// Delete succeeds but reports an error because we cannot access the body

if err := store.Delete(ctx, preset[1].key, &example.Pod{}, nil); !storage.IsInternalError(err) {

t.Errorf("Unexpected error: %v", err)

}

if err := store.Get(ctx, preset[1].key, "", &example.Pod{}, false); !storage.IsNotFound(err) {

t.Errorf("Unexpected error: %v", err)

}

liggitt · 2019-05-15T12:00:31Z

/retest

liggitt · 2019-05-15T14:16:00Z

staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go

Is it better to skip validateDeletion or call it with a nil object? Currently, delete admission is always called, regardless of transform error, with a nil oldObject. would it be preferable to continue calling delete admission with a nil oldObject in the case of a transform error over skipping it entirely?

I can't decide which bothers me more, giving an inconsistent API to admission consumers (sometimes getting a nil oldObject in a delete admission call), skipping delete admission entirely, or requiring talking to etcd to delete a malformed value.

Will CRD webhook conversion errors and KMS transform errors exercise this path? I'm not sure I'd want to abandon calling admission if an external system like that flakes.

Indeed the KMS transform is part of the transformer stack:

kubernetes/staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig/config.go

Line 190 in 4d3d153

transformer, err = getEnvelopePrefixTransformer(provider.KMS, envelopeService, kmsTransformerPrefixV1)

.

I'll trace the CRD webhook path later.

[update] CRD webhook conversion is plumbed to the storage codec, not as a transformer. See code.

Among all the options, "giving an inconsistent API to admission consumers" seems to be the lesser of the evils.

I added the last commit to send nil objects to validateDelete.

Discussed offline. since we already have to look up the existing object in the REST handler to verify there are no remaining finalizers, a persistent transform error is already blocking, and we'll never make it here. Continuing to simply return if we encounter a transform error (rather than skipping admission or passing a nil object) is no worse than the current state, and lets us give a consistent API to admission authors.

Let's drop the transformation error changes from this PR to keep it focused on passing existing objects to delete admission

liggitt · 2019-05-15T16:17:51Z

staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go

also return if origState is nil, since we don't have the rev info to use later

Added a return after the error checks.

test/e2e/apimachinery/webhook.go

test/integration/apiserver/admissionwebhook/admission_test.go

liggitt · 2019-05-15T16:26:35Z

test/integration/apiserver/admissionwebhook/admission_test.go

CRD storage doesn't override Update, and only changes Delete behavior on first delete... I don't think it should require a custom test method (I'd expect removing finalizers on update of a CRD with a deletiontimestamp set to actually delete)

caesarxuchao · 2019-05-15T21:43:56Z

/retest

caesarxuchao · 2019-05-16T21:58:09Z

@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 · 2019-05-17T13:20:54Z

/lgtm
/approve

needs a rebase since #77952 merged

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

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 · 2019-05-17T17:33:51Z

/retest

caesarxuchao · 2019-05-17T18:13:51Z

/retest

liggitt · 2019-05-17T18:55:48Z

/lgtm
/approve
/priority important-soon

k8s-ci-robot · 2019-05-17T18:56:04Z

[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

Details

Needs approval from an approver in each of these files:

~~cmd/kube-apiserver/OWNERS~~ [caesarxuchao,liggitt]
~~pkg/master/OWNERS~~ [liggitt]
~~pkg/registry/OWNERS~~ [liggitt]
~~staging/src/k8s.io/apiextensions-apiserver/OWNERS~~ [liggitt]
~~staging/src/k8s.io/apiserver/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

yue9944882 · 2019-05-18T10:04:30Z

awesome! 🎉

liggitt · 2019-05-31T16:02:58Z

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

roycaihw · 2019-09-17T22:00:05Z

/area admission-control

k8s-ci-robot assigned caesarxuchao Apr 9, 2019

k8s-ci-robot requested review from CaoShuFeng and brendandburns April 9, 2019 20:51

k8s-ci-robot added the area/apiserver label Apr 9, 2019

caesarxuchao force-pushed the delete-admission-objects branch from 390f260 to 744bf2a Compare April 10, 2019 21:06

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 10, 2019

caesarxuchao force-pushed the delete-admission-objects branch from 744bf2a to 8e93456 Compare April 10, 2019 21:09

caesarxuchao commented Apr 10, 2019

View reviewed changes

caesarxuchao force-pushed the delete-admission-objects branch 3 times, most recently from 8a3bd0f to 35c38f7 Compare April 11, 2019 00:11

caesarxuchao commented Apr 11, 2019

View reviewed changes

caesarxuchao force-pushed the delete-admission-objects branch 4 times, most recently from 6bafd02 to f6b94b4 Compare April 11, 2019 17:46

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. 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 11, 2019

k8s-ci-robot unassigned caesarxuchao Apr 11, 2019

caesarxuchao mentioned this pull request May 8, 2019

In GuaranteedUpdate, retry on any error if we are working with cached data #77619

Merged

caesarxuchao commented May 14, 2019

View reviewed changes

liggitt reviewed May 15, 2019

View reviewed changes

caesarxuchao mentioned this pull request May 15, 2019

Namespaces and CRD have inconsistent delete-on-update behavior than other built-in resources. #77944

Closed

yue9944882 and others added 4 commits May 17, 2019 09:50

Switch admission webhook test to work with shared etcd

0b88095

generated

6d40e3c

liggitt mentioned this pull request May 31, 2019

Webhook admission 1.15 docs kubernetes/website#14671

Merged

7 tasks

ritazh mentioned this pull request Jun 19, 2019

Support DELETE operations open-policy-agent/gatekeeper#144

Closed

awesomenix mentioned this pull request Jul 19, 2019

⚠️ support delete validation in validator interface kubernetes-sigs/controller-runtime#525

Merged

mrparkers mentioned this pull request Mar 9, 2020

Delete validation for Validator interface is broken on k8s versions < 1.15 kubernetes-sigs/controller-runtime#845

Closed

YangKeao mentioned this pull request Mar 16, 2020

Refactoring validator chaos-mesh/chaos-mesh#338

Merged

aylei mentioned this pull request Apr 29, 2020

Gardenlet v1.3.* cannot bootstrap on Kubernetes v1.14.9 gardener/gardener#2257

Closed

	// Delete succeeds but reports an error because we cannot access the body
	if err := store.Delete(ctx, preset[1].key, &example.Pod{}, nil); !storage.IsInternalError(err) {
	t.Errorf("Unexpected error: %v", err)
	}

	if err := store.Get(ctx, preset[1].key, "", &example.Pod{}, false); !storage.IsNotFound(err) {
	t.Errorf("Unexpected error: %v", err)
	}

Conversation

caesarxuchao commented Apr 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caesarxuchao commented Apr 11, 2019

Uh oh!

caesarxuchao commented Apr 11, 2019

Uh oh!

caesarxuchao commented May 8, 2019

Uh oh!

sttts commented May 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liggitt commented May 15, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caesarxuchao May 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caesarxuchao commented May 15, 2019

Uh oh!

caesarxuchao commented May 16, 2019

Uh oh!

liggitt commented May 17, 2019

Uh oh!

caesarxuchao commented May 17, 2019

Uh oh!

caesarxuchao commented May 17, 2019

Uh oh!

liggitt commented May 17, 2019

Uh oh!

k8s-ci-robot commented May 17, 2019

Uh oh!

yue9944882 commented May 18, 2019

Uh oh!

liggitt commented May 31, 2019

Uh oh!

roycaihw commented Sep 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

caesarxuchao commented Apr 9, 2019 •

edited

Loading

caesarxuchao May 15, 2019 •

edited

Loading