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.

Sign up for GitHub

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

Jump to bottom

Never let cluster-scoped resources skip webhooks #58185

Merged
merged 2 commits into from
Jan 25, 2018
Merged

Never let cluster-scoped resources skip webhooks #58185

merged 2 commits into from
Jan 25, 2018

Conversation

caesarxuchao
Copy link
Member

@caesarxuchao caesarxuchao commented Jan 12, 2018 •
edited
Loading

Fix #57964

This allows user write webhooks for cluster-scoped custom resources.

We still need to figure out how to selectively exempt cluster-scoped resources from webhooks to avoid bootstrapping deadlocks. For now, if a deadlock occurs, users can work around by first deleting the webhook configuration, then rebooting the webhook, then re-enabling the webhook configuration.

Bug fix: webhooks now do not skip cluster-scoped resources

Action required: Before upgrading your Kubernetes clusters, double check if you had configured webhooks for cluster-scoped objects (e.g., nodes, persistentVolume), these webhooks will start to take effect. Delete/modify the configs if that's not desirable.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 12, 2018
@caesarxuchao caesarxuchao added the kind/bug Categorizes issue or PR as related to a bug. label Jan 12, 2018
@caesarxuchao caesarxuchao added this to the v1.9 milestone Jan 12, 2018
@caesarxuchao caesarxuchao added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Jan 12, 2018
@caesarxuchao caesarxuchao mentioned this pull request Jan 12, 2018
Closed
@sttts
Copy link
Contributor

sttts commented Jan 12, 2018

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 12, 2018
@sttts sttts removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 12, 2018
@sttts
Copy link
Contributor

sttts commented Jan 12, 2018

Please update also the type docs https://github.com/sttts/kubernetes/blob/951962512b9cfe15b25e9c715a5f33f088854f97/staging/src/k8s.io/api/admissionregistration/v1beta1/types.go#L150. Then lgtm.

@k8s-github-robot
Copy link

[MILESTONENOTIFIER] Milestone Removed From Pull Request

@caesarxuchao @sttts

Important: This pull request was missing labels required for the v1.9 milestone for more than 3 days:

priority: Must specify exactly one of priority/critical-urgent, priority/important-longterm or priority/important-soon.

Help

@k8s-github-robot k8s-github-robot removed this from the v1.9 milestone Jan 15, 2018
@caesarxuchao caesarxuchao force-pushed the webhook-cluster-scoped-resources branch from 9c8f402 to b3d2dfa Compare January 19, 2018 01:33
@k8s-github-robot k8s-github-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Jan 19, 2018
@k8s-ci-robot k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 19, 2018
caesarxuchao
caesarxuchao commented Jan 19, 2018
View reviewed changes
pkg/apis/admissionregistration/types.go Outdated
@@ -196,7 +196,7 @@ type Webhook struct {
// on whether the namespace for that object matches the selector. If the
// object itself is a namespace, the matching is performed on
// object.metadata.labels. If the object is other cluster scoped resource,
// it is not subjected to the webhook.
// it never skips the webhook.
Copy link
Member Author

Choose a reason for hiding this comment

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

@sttts PTAL. Sorry it has been off my radar for a while.

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: another cluster scoped resource.

But I am no native speaker either @deads2k you are. Can you comment?

Copy link
Member Author

Choose a reason for hiding this comment

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

Confirmed with @cheftako that we should use "another" :)

Copy link
Contributor

Choose a reason for hiding this comment

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

:)

@sttts
Copy link
Contributor

sttts commented Jan 19, 2018

lgtm modulo the nit.

/approve

@caesarxuchao caesarxuchao force-pushed the webhook-cluster-scoped-resources branch from b3d2dfa to c80a7ee Compare January 19, 2018 22:05
@caesarxuchao
Copy link
Member Author

Nit fixed. I'll self apply the lgtm.

@caesarxuchao caesarxuchao added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 19, 2018
@caesarxuchao
Copy link
Member Author

/retest

@sttts
Copy link
Contributor

sttts commented Jan 22, 2018

/assign @lavalamp

For the api Godoc changes.

@lavalamp
Copy link
Member

This is a breaking API change. I think at a minimum it needs an "action required" notice in the release notes. Anyone who has added a webhook for a cluster scoped resource and forgotten about it is going to be in for a surprise when they get this patch.

I think I agree that this is a bug that should be fixed, but is it so urgent that we want to do it before adding some exception mechanism?

@deads2k
Copy link
Contributor

deads2k commented Jan 23, 2018

This is a breaking API change. I think at a minimum it needs an "action required" notice in the release notes. Anyone who has added a webhook for a cluster scoped resource and forgotten about it is going to be in for a surprise when they get this patch.

No issue with adding "action required", but worrying about a case where someone created the config, tested it, realized it didn't work, didn't remove it, upgrades, and then gets upset that it starts working seems like an edge of an edge of an edge for a config that is already broken.

I think I agree that this is a bug that should be fixed, but is it so urgent that we want to do it before adding some exception mechanism?

Adding an exception mechanism would be a backward compatible change. Having exceptions on cluster-scoped things is less obvious of a use-case since they're already privileged so there are likely to be fewer webhooks for them.

I had thought about applying the same label selection scheme since the data is already present (labels on the object you're touching or its previous value), but I wouldn't have blocked this on it.

@lavalamp
Copy link
Member

I'm the last person to apply rules for their own sake, and I agree this should be very rare, but I still worry about whether people will trust our API stability guarantees when we're willing to break things like this. I won't block it if we add the action-required notice.

I should write a blog post called "it's not safe to upgrade your cluster".

@caesarxuchao caesarxuchao added the release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. label Jan 23, 2018
@k8s-ci-robot k8s-ci-robot removed the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jan 23, 2018
@caesarxuchao
Copy link
Member Author

Added "action required". Feel free to reword it.

@lavalamp
Copy link
Member

/retest
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: caesarxuchao, lavalamp, sttts

Associated issue: #57964

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your 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 Jan 25, 2018
@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 0b8f3a2 into kubernetes:master Jan 25, 2018
@yguo0905 yguo0905 mentioned this pull request Jan 26, 2018
Merged
k8s-github-robot pushed a commit that referenced this pull request Jan 27, 2018
Merge pull request #58884 from yguo0905/automated-cherry-pick-of-#581…
05c9cfa 
…85-upstream-release-1.9

Automatic merge from submit-queue.

Automated cherry pick of #58185: Never let cluster-scoped resources skip webhooks

Cherry pick of #58185 on release-1.9.

#58185: Never let cluster-scoped resources skip webhooks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sttts sttts sttts left review comments

Assignees

@lavalamp lavalamp

@sttts sttts

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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 kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. milestone/removed release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

admission webhooks: broken for cluster-scoped resources
6 participants
@caesarxuchao @sttts @k8s-github-robot @lavalamp @deads2k @k8s-ci-robot