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

Adding ObjectSelector to admission webhooks #78505

Merged

Conversation

caesarxuchao
Copy link
Member

@caesarxuchao caesarxuchao commented May 30, 2019

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

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 added this to In progress in API Reviews May 30, 2019
@@ -48,6 +48,14 @@ type VersionedAttributes struct {
Dirty bool
}

// GetObject overrides the Attributes.GetObject()
func (v *VersionedAttributes) GetObject() runtime.Object {
Copy link
Member

@liggitt liggitt May 30, 2019

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
Member

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

}

func newValidatingDispatcher(cm *webhookutil.ClientManager) generic.Dispatcher {
return &validatingDispatcher{cm}
func newValidatingDispatcher(p *Plugin) func(cm *webhookutil.ClientManager) generic.Dispatcher {
Copy link
Member

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
Member

liggitt commented May 31, 2019

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)

@liggitt
Copy link
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
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
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
@liggitt liggitt moved this from In progress to Completed, 1.15 in API Reviews May 31, 2019
@k8s-ci-robot
Copy link
Contributor

[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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 31, 2019
@liggitt
Copy link
Member

liggitt commented May 31, 2019

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

@Ark-kun
Copy link

Ark-kun commented Sep 9, 2020

/cc @caesarxuchao

@k8s-ci-robot
Copy link
Contributor

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

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
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
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
Status: API review completed, 1.15
Development

Successfully merging this pull request may close these issues.

None yet

8 participants