Add more suggestions for avoiding deadlocks to webhook docs #46798
Conversation
|
Welcome @fasaxc! |
✅ Pull request preview available for checkingBuilt without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
I recommend clearly calling out ValidatingAdmissionPolicies as a way to limit the impact.
If you run an admission webhook in a namespace scrutineer, you can exempt the scrutineer namespace from the webhook admission checks, and you can still restrict changes to scrutineer using a ValidatingAdmissionPolicy.
You should consider Pod security admission for that scrutineer namespace, too (at least baseline, Restricted if you can make it work).
For example, have a ValidatingAdmissionPolicy that restricts who can remove the scrutineer namespace and also limits writes to the ConfigMap that configures the webhook. Have another ValidatingAdmissionPolicy that prevents making the scrutineer namespace privileged, and a further one that prevents creating new ClusterRoleBindings inside the scrutineer namespace.
That's just an example but I hope you get the idea.
| It is recommended to exclude cluster infratructure namespaces from webhooks, including kube-system, | ||
| any namespaces used by CNI plugins, etc. |
There was a problem hiding this comment.
- typo (“infratructure”)
- let's mention ValidatingAdmissionPolicies as a way to provide (at least) some protection without having the admission webhook protect itself
content/en/docs/reference/access-authn-authz/extensible-admission-controllers.md
Outdated
Show resolved
Hide resolved
content/en/docs/reference/access-authn-authz/extensible-admission-controllers.md
Outdated
Show resolved
Hide resolved
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
|
/remove-lifecycle stale |
content/en/docs/reference/access-authn-authz/extensible-admission-controllers.md
Outdated
Show resolved
Hide resolved
content/en/docs/reference/access-authn-authz/extensible-admission-controllers.md
Outdated
Show resolved
Hide resolved
content/en/docs/reference/access-authn-authz/extensible-admission-controllers.md
Outdated
Show resolved
Hide resolved
content/en/docs/reference/access-authn-authz/extensible-admission-controllers.md
Outdated
Show resolved
Hide resolved
| example, due to a cluster-wide outage or a node failure where both pods run on the same node) | ||
| deadlock occurs because neither webhook pod can be recreated without the other already running. | ||
| It is recommended to establish a desired order for webhooks to start, then to exclude "earlier" |
There was a problem hiding this comment.
I don't think this is a recommendation that the Kubernetes project makes.
There was a problem hiding this comment.
OK, I've toned it down from recommendation to an example
content/en/docs/reference/access-authn-authz/extensible-admission-controllers.md
Outdated
Show resolved
Hide resolved
content/en/docs/reference/access-authn-authz/extensible-admission-controllers.md
Outdated
Show resolved
Hide resolved
| It is recommended to exclude cluster infrastructure namespaces from webhooks, including kube-system, | ||
| any namespaces used by CNI plugins, etc. |
There was a problem hiding this comment.
This isn't a Kubernetes project recommendation, although in many (not all) cases it is a good idea to do.
content/en/docs/reference/access-authn-authz/extensible-admission-controllers.md
Outdated
Show resolved
Hide resolved
<
8000
/summary>
content/en/docs/reference/access-authn-authz/extensible-admission-controllers.md
Outdated
Show resolved
Hide resolved
|
@sftim do you want to take over this PR? Seems like you want to rewrite this section of the docs. |
I'd much rather not, honestly. If I can help you follow our style guide, great - I'll provide advice. If you've found that you don't have capacity to move this forward, that's OK; please let us know. If you would like to get this merged, the feedback so far will help you make the right kind of changes. |
|
LGTM label has been added. Git tree hash: f01bff874888c599957b47bf82ef0db5adc994c2
|
|
Applied your markups and tweaked the language to avoid making "official recommendations" PTAL. |
| One way to prevent this is to establish a desired order for webhooks to start, then to exclude | ||
| "earlier" webhooks' resources from being inspected by "later" webhooks. This ensures that the | ||
| "earliest" webhook can start, which in turn allows the next. |
There was a problem hiding this comment.
I don't understand how this could work and don't think it's an effective control. See https://stackoverflow.com/a/69166466 for informal confirmation that admission webhooks are not ordered.
There was a problem hiding this comment.
I think you've taken the wrong idea from that paragraph, which suggests I could explain it more clearly! I'm talking about the order in which the webhook pods start rather than the order that webhooks apply to a particular resource.
The deadlock that can happen is if
- webhook A is unavailable
- webhook B is unavailable
- webhook A is needed to approve webhook B's pod
- webhook B is needed to approve webhook A's pod.
So the suggestion is:
- Pick the webhook that needs to apply most broadly. Let's say that's A
- Make sure that no other webhooks can interfere with starting A.
- Therefore A can always start, even if no other webhooks are available
- Therefore B can start, because it depends on A but we know that A can start.
If you have3 webhooks, A, B, C, then
- Make sure B and C exclude A's pods
- Therefore A can start
- Make sure C excludes B's pods
- Therefore B can start, since we know that A can start
- Therefore C can start, since A and B can start.
There was a problem hiding this comment.
I'll try to reword it, but suggestions also welcome
There was a problem hiding this comment.
This kind of workload sequencing doesn't really hold because nodes are not considered durable and resilient. Also, do try to reword.
There was a problem hiding this comment.
I get the point we want to make. Maybe that's worth shifting to its own point (eg a blog article)?
There was a problem hiding this comment.
I think as long as you've got an ordering, node failure is fine:
- webhook A comes up
- webhook A's node dies
- webhook A is rescheduled somewhere else
- now webhook B can start... and so on.
There was a problem hiding this comment.
It should get there eventually, whereas without some sort of ordering like this, node failures will cause deadlocks and you'll be forced to intervene by temporarily removing your webhook
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
| container with name "foo-sidecar" with the expected configuration exists in the to-be-created | ||
| object. | ||
|
|
||
| ### Avoiding deadlocks in self-hosted webhooks |
There was a problem hiding this comment.
Maybe this scenario can also be incorporated: kubernetes/enhancements#5068 (comment)
There was a problem hiding this comment.
This PR feels like it's at danger of not converging, I'd rather not add anything more to it.
content/en/docs/reference/access-authn-authz/extensible-admission-controllers.md
Outdated
Show resolved
Hide resolved
|
/lgtm We should merge this, it will make the docs better. |
|
LGTM label has been added. Git tree hash: 9647147c1f415d781b8078b9ce710d5f81f366f4
|
|
@fasaxc would you squash this to 1 commit and force-push? (if you keep the same merge base, Prow won't drop the LGTM label). |
Webhooks can cause deadlocks in several ways, expand the list to cover more subtle cases. Co-authored-by: Kat Cosgrove <kat.cosgrove@gmail.com> Co-authored-by: Tim Bannister <tim@scalefactory.com>
Source PR: kubernetes#46798 Co-authored-by: Shaun Crampton <shaun@tigera.io> Co-authored-by: Kat Cosgrove <kat.cosgrove@gmail.com> Co-authored-by: Tim Bannister <tim@scalefactory.com>
Source PR: kubernetes#46798 Co-authored-by: Shaun Crampton <shaun@tigera.io> Co-authored-by: Kat Cosgrove <kat.cosgrove@gmail.com> Co-authored-by: Tim Bannister <tim@scalefactory.com>
I agree, I don't see any reason not to merge this in as is. Any unaddressed feedback can be left for a followup PR. Thanks for this @fasaxc, and thanks to all the reviewers! /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nate-double-u 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 |
Source PR: kubernetes#46798 Co-authored-by: Shaun Crampton <shaun@tigera.io> Co-authored-by: Kat Cosgrove <kat.cosgrove@gmail.com> Co-authored-by: Tim Bannister <tim@scalefactory.com>
Source PR: kubernetes#46798 Co-authored-by: Shaun Crampton <shaun@tigera.io> Co-authored-by: Kat Cosgrove <kat.cosgrove@gmail.com> Co-authored-by: Tim Bannister <tim@scalefactory.com>
Source PR: kubernetes#46798 Co-authored-by: Shaun Crampton <shaun@tigera.io> Co-authored-by: Kat Cosgrove <kat.cosgrove@gmail.com> Co-authored-by: Tim Bannister <tim@scalefactory.com>
Webhooks can cause deadlocks in several ways, expand the list to cover more subtle cases.
Was recently debugging an issue where this came up and I thought, perhaps I can improve the docs to call out more cases. Not sure we've got the root cause of the issue correct yet but I can't see why it wouldn't be possible to cause deadlocks in these ways (and I think I was seeing these problems in a customer cluster).