-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
AdmissionWebhookMatchConditions feature documentation #40058
AdmissionWebhookMatchConditions feature documentation #40058
Conversation
@tallclair: GitHub didn't allow me to request PR reviews from the following users: maxsmythe. Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
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. |
c0fd4cb
to
b1e6989
Compare
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
Show resolved
Hide resolved
content/en/docs/reference/access-authn-authz/extensible-admission-controllers.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Some thoughts / feedback.
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
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
content/en/docs/reference/access-authn-authz/extensible-admission-controllers.md
Outdated
Show resolved
Hide resolved
9f8fb44
to
78211e8
Compare
Thanks for the feedback, comments addressed. |
content/en/docs/reference/access-authn-authz/extensible-admission-controllers.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM for docs; ready for tech review.
content/en/examples/access/admission-webhook-match-conditions.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I left a few small comments but I'm happy with how this looks.
content/en/docs/reference/access-authn-authz/extensible-admission-controllers.md
Outdated
Show resolved
Hide resolved
- name: 'breakglass' | ||
# Skip requests made by users authorized to 'breakglass' on this webhook. | ||
# The 'breakglass' API verb does not need to exist outside this check. | ||
expression: 'authorizer.group('admissionregistration.k8s.io').resource('validatingwebhookconfigurations').name('my-webhook.example.com').check('breakglass').denied()' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love everything about this example.
content/en/docs/reference/access-authn-authz/extensible-admission-controllers.md
Outdated
Show resolved
Hide resolved
/lgtm |
LGTM label has been added. Git tree hash: 3edc1fa69c704cff8764d7e211237a07f0bf9830
|
expression: '!(request.resource.group == "coordination.k8s.io" && request.resource.resource == "leases")' # Match non-lease resources. | ||
- name: 'exclude-kubelet-requests' | ||
expression: '!("system:nodes" in request.userInfo.groups)' # Match requests made by non-node users. | ||
- name: 'breakglass' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is just an example, but I would hate to see people mimic putting an authorizer condition on an admission policy that matches *
/*
/*
... doesn't this mean we run another authorization check on every API request?
edit: I guess the part I consider an anti-pattern is matching *
/*
/*
, not necessarily the authz check, but I worry that people who previously wouldn't dream of matching *
/*
/*
will think it's ok to do so if they guard it with an authz check, which can be somewhat expensive depending on webhook authz config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To narrow it, how about changing it to be a restriction on RBAC changes? That's a believable-enough story and something often worth protecting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Narrowing the rules weakens the "everything except leases" example. How about this: use short-circuiting to scope just the authorization check to RBAC? Is that getting too convoluted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Narrowing the rules weakens the "everything except leases" example.
I agree
How about this: use short-circuiting to scope just the authorization check to RBAC? Is that getting too convoluted?
Maybe a little? Would two different examples be easier to follow?
- A
*
/*
/*
policy that excludes leases, and node requests using conditions - A policy on something important like RBAC or networkpolicies that has an authorization-based breakglass
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How's this? I put the RBAC check on a separate webhook (and also added a couple missing fields)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that better, thanks
content/en/examples/access/admission-webhook-match-conditions.yaml
Outdated
Show resolved
Hide resolved
lgtm overall |
/label tide/merge-method-squash |
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/command-line-tools-reference/feature-gates.md
Outdated
Show resolved
Hide resolved
content/en/examples/access/admission-webhook-match-conditions.yaml
Outdated
Show resolved
Hide resolved
sideEffects: None | ||
clientConfig: {} # Omitted for this example | ||
matchConditions: | ||
- name: 'breakglass' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- name: 'breakglass' | |
- name: 'breakglass' # again, each match condition must have a unique name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They don't need to be globally unique. This feels superfluous here?
content/en/docs/reference/command-line-tools-reference/feature-gates.md
Outdated
Show resolved
Hide resolved
content/en/docs/reference/access-authn-authz/extensible-admission-controllers.md
Show resolved
Hide resolved
👷 Deploy Preview for kubernetes-io-vnext-staging processing.
|
{{< feature-state state="alpha" for_k8s_version="v1.27" >}} | ||
|
||
{{< note >}} | ||
Use of `matchConditions` requires the [featuregate](/docs/reference/command-line-tools-reference/feature-gates/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use of `matchConditions` requires the [featuregate](/docs/reference/command-line-tools-reference/feature-gates/) | |
Use of `matchConditions` requires the [feature gate](/docs/reference/command-line-tools-reference/feature-gates/) |
`AdmissionWebhookMatchConditions` to be explicitly enabled on the kube-apiserver before this feature can be used. | ||
{{< /note >}} | ||
|
||
You can define _match conditions_for webhooks if you need fine-grained request filtering. These |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can define _match conditions_for webhooks if you need fine-grained request filtering. These | |
You can define _match conditions_ for webhooks if you need fine-grained request filtering. These |
{{< /note >}} | ||
|
||
You can define _match conditions_for webhooks if you need fine-grained request filtering. These | ||
conditions are useful if you find that match rules, `objectSelectors` and `namespaceSelectors` still |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
conditions are useful if you find that match rules, `objectSelectors` and `namespaceSelectors` still | |
conditions are useful if you find that match rules, `objectSelectors` and `namespaceSelectors`, still |
- for [`failurePolicy: Fail`](#failure-policy), reject the request (without calling the webhook). | ||
- for [`failurePolicy: Ignore`](#failure-policy), proceed with the request but skip the webhook. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Avoid 4-spaces indentation when possible. Sometimes the YAML parser may treat it as a code snippet.
- for [`failurePolicy: Fail`](#failure-policy), reject the request (without calling the webhook). | |
- for [`failurePolicy: Ignore`](#failure-policy), proceed with the request but skip the webhook. | |
- for [`failurePolicy: Fail`](#failure-policy), reject the request (without calling the webhook). | |
- for [`failurePolicy: Ignore`](#failure-policy), proceed with the request but skip the webhook. |
@@ -56,6 +56,7 @@ For a reference to old feature gates that are removed, please refer to | |||
|
|||
| Feature | Default | Stage | Since | Until | | |||
|---------|---------|-------|-------|-------| | |||
| `AdmissionWebhookMatchConditions` | Alpha | `false` | 1.27 | | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this before line 70?
@@ -368,6 +369,7 @@ A *General Availability* (GA) feature is also referred to as a *stable* feature. | |||
|
|||
Each feature gate is designed for enabling/disabling a specific feature: | |||
|
|||
- `AdmissionWebhookMatchConditions`: Enable [match conditions](/docs/reference/access-authn-authz/extensible-admission-controllers/#matching-requests-matchconditions) on mutating & validating admission webhooks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- `AdmissionWebhookMatchConditions`: Enable [match conditions](/docs/reference/access-authn-authz/extensible-admission-controllers/#matching-requests-matchconditions) on mutating & validating admission webhooks. | |
- `AdmissionWebhookMatchConditions`: Enable [match conditions](/docs/reference/access-authn-authz/extensible-admission-controllers/#matching-requests-matchconditions) | |
on mutating & validating admission webhooks. |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tengqm 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 |
/lgtm |
LGTM label has been added. Git tree hash: 2628295bcd8762d7e6eb0e1f43ef79cd409b38fe
|
* AdmissionWebhookMatchConditions feature documentation * #squash ivelichkovich feedback * #squash sftim feedback * Correct statement about request.object * #squash: sftim feedback * #squash jpbetz feedback * #squash: denied function removed * #squash fix match conditions example * #squash fix expression quoting * #squash scope authorizatoin check example * #squash separate RBAC webhook example * #squash sftim feedback * #squash add shared client config for example * Don't use yaml anchors in example
Documentation for the
AdmissionWebhookMatchConditions
(alpha) feature.KEP: https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/3716-admission-webhook-match-conditions
/cc @ivelichkovich @maxsmythe @liggitt