Fixes delete admission nil object attributes by yue9944882 · Pull Request #66535 · kubernetes/kubernetes

yue9944882 · 2018-07-24T04:56:47Z

What this PR does / why we need it:

Fixes nil old object in admission attributes for storage delete calls.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #66536

Special notes for your reviewer:

This PR doesn't change the current behavior of kube-apiserver.

Release note:

NONE

yue9944882 · 2018-07-24T04:57:03Z

/sig api-machinery
/kind bug

yue9944882 · 2018-07-24T05:09:32Z

staging/src/k8s.io/apiserver/pkg/registry/rest/delete.go

For reviewers, forcing the name via accessor here is because that DeleteCollections will set an empty name in the attributes.

CaoShuFeng · 2018-07-24T07:01:49Z

staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go

We call admission for each element in the collection?

Hi @CaoShuFeng, thanks for reviewing! I am sort of confused here when I found that we are actually not doing loose admission checks in delete collection. Shouldn't we force the check for every item of it?

attrs := admission.NewAttributesRecord(nil, nil, scope.Kind, namespace, "", scope.Resource, scope.Subresource, admission.Delete, userInfo)

The changes here are not quite related to the PR tho.. Or we can put the changes into another PR so that this PR doesn't change any behavior of the server.

Shouldn't we force the check for every item of it?

If we have remote admission controllers set, this will not be that good.

e.g. I delete a collection of 1000 elements, than the api-server will call the remote admission controller 1000 times in a short time, this will be a flood I think.

@CaoShuFeng Thanks, these changes reverted.

yue9944882 · 2018-07-24T09:49:14Z

/cc @deads2k @liggitt @sttts

yue9944882 · 2018-07-24T10:13:56Z

/test pull-kubernetes-integration

liggitt · 2018-07-24T12:07:57Z

See questions raised in
#27193 (comment) guaranteeing the delete admission is called

yue9944882 · 2018-07-24T16:24:13Z

(Update guarantees this by requiring the call in order to get the updated data)

Thanks @liggitt . I'm afraid I got a little confused about the line above. In the Update calls we only guarantee the mutating validation call to ensure that the object has to be mutated as expected but other validation funcs like createValidation and updateValidation are not really guaranteed for those storage impl. OTOH, is it really neccesary to force mutating on a delete call? i.e. what is the purpose to mutate an object before deletion?

Do you mean that guaranteeing both mutating and validating admission check for object deletion via some approach like Update's guaranteeing mutating checks?

Does admission get called for the delete operation twice if something is gracefully deleted, then actually deleted (I'd expect it to)

Current state of this PR is right doing admission check twice for graceful deletion IMHO.😃

liggitt · 2018-07-24T17:04:50Z

pkg/registry/core/namespace/storage/storage.go

this change means that admission is not actually called for namespaces on the first delete request (since this function does a r.store.Storage.GuaranteedUpdate call internally to set the deletion timestamp and namespace finalizer

@liggitt Added the validation check for these manual update-on-delete

liggitt · 2018-07-24T17:06:05Z

staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresourcedefinition/etcd.go

same here, admission is not called on first delete

caesarxuchao · 2019-03-27T00:54:36Z

@yue9944882 do you have time reviving this PR? We need this fix for webhook GA. I can complete this PR if you are busy with the rate limiting project.

yue9944882 · 2019-03-27T03:00:50Z

@caesarxuchao heh sure, take it and go 😋

caesarxuchao · 2019-03-27T22:19:37Z

@yue9944882 thanks, do you mind squashing the commits to one? That way I can keep your authorship by cherrypicking just one commit.

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

k8s-ci-robot · 2019-03-28T03:00:46Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: yue9944882
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: deads2k

If they are not already assigned, you can assign the PR to them by writing /assign @deads2k in a comment when ready.

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:

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 · 2019-03-28T03:03:06Z

@yue9944882: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-gce-100-performance	`ab07482`	link	`/test pull-kubernetes-e2e-gce-100-performance`
pull-kubernetes-kubemark-e2e-gce-big	`ab07482`	link	`/test pull-kubernetes-kubemark-e2e-gce-big`
pull-kubernetes-bazel-test	`ab07482`	link	`/test pull-kubernetes-bazel-test`
pull-kubernetes-bazel-build	`ab07482`	link	`/test pull-kubernetes-bazel-build`
pull-kubernetes-integration	`ab07482`	link	`/test pull-kubernetes-integration`
pull-kubernetes-godeps	`ab07482`	link	`/test pull-kubernetes-godeps`
pull-kubernetes-verify	`ab07482`	link	`/test pull-kubernetes-verify`
pull-kubernetes-typecheck	`ab07482`	link	`/test pull-kubernetes-typecheck`
pull-kubernetes-node-e2e	`ab07482`	link	`/test pull-kubernetes-node-e2e`
pull-kubernetes-e2e-gce-device-plugin-gpu	`ab07482`	link	`/test pull-kubernetes-e2e-gce-device-plugin-gpu`
pull-kubernetes-e2e-gce	`ab07482`	link	`/test pull-kubernetes-e2e-gce`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

yue9944882 · 2019-03-28T03:13:47Z

thanks for asking. squashed.

liggitt · 2019-04-29T20:22:04Z

closing in favor of #76346

/close

k8s-ci-robot · 2019-04-29T20:22:05Z

@liggitt: Closed this PR.

Details

In response to this:

closing in favor of #76346

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

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

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 24, 2018

k8s-ci-robot requested review from CaoShuFeng and deads2k July 24, 2018 04:56

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Jul 24, 2018

yue9944882 commented Jul 24, 2018

View reviewed changes

yue9944882 force-pushed the fixes-delete-admission-nil-object-attributes branch 5 times, most recently from 5593744 to c386a21 Compare July 24, 2018 06:59

CaoShuFeng reviewed Jul 24, 2018

View reviewed changes

yue9944882 force-pushed the fixes-delete-admission-nil-object-attributes branch 3 times, most recently from 53f82d1 to 2128cf0 Compare July 24, 2018 09:18

k8s-ci-robot requested review from liggitt and sttts July 24, 2018 09:49

liggitt added kind/feature Categorizes issue or PR as related to a new feature. and removed kind/bug Categorizes issue or PR as related to a bug. labels Jul 24, 2018

liggitt reviewed Jul 24, 2018

View reviewed changes

yue9944882 force-pushed the fixes-delete-admission-nil-object-attributes branch 2 times, most recently from ef059d0 to 4a16762 Compare July 25, 2018 15:48

yue9944882 force-pushed the fixes-delete-admission-nil-object-attributes branch from 1c33139 to ab07482 Compare March 28, 2019 02:59

k8s-ci-robot added the area/apiserver label Mar 28, 2019

caesarxuchao mentioned this pull request Apr 10, 2019

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

Merged

k8s-ci-robot closed this Apr 29, 2019

Conversation

yue9944882 commented Jul 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yue9944882 commented Jul 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yue9944882 Jul 24, 2018 • 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!

CaoShuFeng Jul 24, 2018

Choose a reason for hiding this comment

Uh oh!

yue9944882 Jul 24, 2018

Choose a reason for hiding this comment

Uh oh!

yue9944882 Jul 24, 2018

Choose a reason for hiding this comment

Uh oh!

CaoShuFeng Jul 24, 2018 • 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!

yue9944882 Jul 24, 2018

Choose a reason for hiding this comment

Uh oh!

yue9944882 commented Jul 24, 2018

Uh oh!

yue9944882 commented Jul 24, 2018

Uh oh!

liggitt commented Jul 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yue9944882 commented Jul 24, 2018

Uh oh!

liggitt Jul 24, 2018

Choose a reason for hiding this comment

Uh oh!

yue9944882 Jul 25, 2018

Choose a reason for hiding this comment

Uh oh!

liggitt Jul 24, 2018

Choose a reason for hiding this comment

Uh oh!

caesarxuchao commented Mar 27, 2019

Uh oh!

yue9944882 commented Mar 27, 2019

Uh oh!

caesarxuchao commented Mar 27, 2019

Uh oh!

k8s-ci-robot commented Mar 28, 2019

Uh oh!

k8s-ci-robot commented Mar 28, 2019

Uh oh!

yue9944882 commented Mar 28, 2019

Uh oh!

liggitt commented Apr 29, 2019

Uh oh!

k8s-ci-robot commented Apr 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

yue9944882 commented Jul 24, 2018 •

edited

Loading

yue9944882 commented Jul 24, 2018 •

edited

Loading

yue9944882 Jul 24, 2018 •

edited

Loading

CaoShuFeng Jul 24, 2018 •

edited

Loading

liggitt commented Jul 24, 2018 •

edited

Loading