Skip to content

Adding ObjectSelector to admission webhooks#78505

Merged
k8s-ci-robot merged 8 commits intokubernetes:masterfrom
caesarxuchao:dynamic-object-selector
Jun 1, 2019
Merged

Adding ObjectSelector to admission webhooks#78505
k8s-ci-robot merged 8 commits intokubernetes:masterfrom
caesarxuchao:dynamic-object-selector

Conversation

@caesarxuchao
Copy link
Copy Markdown
Contributor

@caesarxuchao caesarxuchao commented May 30, 2019 •
edited by liggitt
Loading

Supersedes #78226.

Based on #78491 and #78080, which are both reviewed and approved already

Implementing KEP#objectSelector and #78226 (comment).

/sig api-machinery
/assign @liggitt @jpbetz @sttts @roycaihw

Added objectSelector to admission webhook configurations. objectSelector is evaluated the oldObject and newObject that would be sent to the webhook, and is considered to match if either object matches the selector. A null object (oldObject in the case of create, or newObject in the case of delete) or an object that cannot have labels (like a DeploymentRollback or a PodProxyOptions object) is not considered to match. Use the object selector only if the webhook is opt-in, because end users may skip the admission webhook by setting the labels.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. 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. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels May 30, 2019
@caesarxuchao caesarxuchao mentioned this pull request May 30, 2019
Closed
@k8s-ci-robot k8s-ci-robot added area/apiserver kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels May 30, 2019
@fejta-bot
Copy link
Copy Markdown

fejta-bot commented May 30, 2019

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

@liggitt liggitt added this to the v1.15 milestone May 30, 2019
liggitt
liggitt reviewed May 30, 2019
View reviewed changes
This was referenced May 30, 2019
Closed
Merged
jpbetz
jpbetz reviewed May 30, 2019
View reviewed changes
@caesarxuchao caesarxuchao force-pushed the dynamic-object-selector branch from b859b1f to 814a4a3 Compare May 30, 2019 21:28
@caesarxuchao caesarxuchao force-pushed the dynamic-object-selector branch from c95c02d to 3b5c1ea Compare May 30, 2019 23:46
liggitt
liggitt reviewed May 31, 2019
View reviewed changes
staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/generic/interfaces.go Outdated
Copy link
Copy Markdown
Member

@liggitt liggitt May 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems likely to cause confusion... admission attributes GetObject() functions don't usually contain external objects

Copy link
Copy Markdown
Member

@liggitt liggitt May 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is fine for now, and we have complete test coverage, but a follow-up issue to rework this a little in 1.16 would be good

liggitt
liggitt reviewed May 31, 2019
View reviewed changes
staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/validating/dispatcher.go Outdated
Copy link
Copy Markdown
Member

@liggitt liggitt May 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this works for now, but is pretty awkward, can you open a follow-up issue to avoid needing to inject the full plugin into the dispatchers?

@liggitt
Copy link
Copy Markdown
Member

liggitt commented May 31, 2019 •
edited
Loading

looks like a compile error in k8s.io/apiserver/pkg/admission/plugin/webhook/object, and openapi spec needs updating (I think that CI job was stale)

@caesarxuchao caesarxuchao mentioned this pull request May 31, 2019
Closed
@caesarxuchao caesarxuchao force-pushed the dynamic-object-selector branch from 3b5c1ea to 7738c7e Compare May 31, 2019 00:23
@liggitt
Copy link
Copy Markdown
Member

liggitt commented May 31, 2019

ran against #78309, v1.14.0 compatibility looked good, new fields showed up as expected in the data fixtures for HEAD

@liggitt
Copy link
Copy Markdown
Member

liggitt commented May 31, 2019

/lgtm
/approve

API change looks good

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 31, 2019
@liggitt
Copy link
Copy Markdown
Member

liggitt commented May 31, 2019

/priority important-soon

@k8s-ci-robot k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels May 31, 2019
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented May 31, 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

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 k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 31, 2019
This was referenced May 31, 2019
Closed
Closed
@liggitt
Copy link
Copy Markdown
Member

liggitt commented May 31, 2019

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

@liggitt liggitt mentioned this pull request May 31, 2019
Merged
7 tasks
@alpeb alpeb mentioned this pull request May 31, 2019
Closed
@caesarxuchao caesarxuchao mentioned this pull request May 31, 2019
Merged
@k8s-ci-robot k8s-ci-robot merged commit bada1c6 into kubernetes:master Jun 1, 2019
@masa213f masa213f mentioned this pull request Aug 20, 2019
Merged
@timuthy timuthy mentioned this pull request Dec 31, 2019
Merged
@Ark-kun
Copy link
Copy Markdown

Ark-kun commented Sep 9, 2020

/cc @caesarxuchao

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Sep 9, 2020

@Ark-kun: GitHub didn't allow me to request PR reviews from the following users: caesarxuchao.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

/cc @caesarxuchao

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.

@liggitt
Copy link
Copy Markdown
Member

liggitt commented Sep 9, 2020

This looks like a breaking change.

The type name changed, but the wire serialization of the two types is identical.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@liggitt liggitt liggitt left review comments

@lavalamp lavalamp Awaiting requested review from lavalamp

@pwittrock pwittrock Awaiting requested review from pwittrock

+2 more reviewers

@jpbetz jpbetz jpbetz left review comments

@roycaihw roycaihw roycaihw left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@jpbetz jpbetz

@sttts sttts

@liggitt liggitt

@roycaihw roycaihw

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/admission-control area/apiserver area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

API Reviews
Status: API review completed, 1.15

Milestone

v1.15

Development

Successfully merging this pull request may close these issues.

8 participants

@caesarxuchao @fejta-bot @liggitt @k8s-ci-robot @Ark-kun @jpbetz @roycaihw @sttts