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

KEP-3962: Add MutatingAdmissionPolicy as provisional #3963

Closed
wants to merge 2 commits into from

Conversation

jpbetz
Copy link
Contributor

@jpbetz jpbetz commented Apr 24, 2023

  • One-line PR description: Propose using CEL object instantiation + server side apply merges to implement mutating admission for CEL.

  • Issue link: Mutating Admission Policies #3962

  • Other comments:

    • See https://github.com/jpbetz/celpatch for working examples of CEL to perform mutations. The prototype work done on that repo is being used to inform this KEP.
    • We do not plan to implement this until after ValidatingAdmissionPolicy reaches beta (1.28 hopefully). So I hope to merge this as provisional for now and then transition it to implementable for 1.29.

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 24, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jpbetz

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 kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory label Apr 24, 2023
@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 24, 2023
@jpbetz
Copy link
Contributor Author

jpbetz commented Apr 24, 2023

/sig api-machinery
cc @lavalamp @deads2k @ritazh @maxsmythe @cici37

CI/CD pipelines, and auditing can run the same CEL validation checks
that the API server does.

### Non-Goals
Copy link

Choose a reason for hiding this comment

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

Question: will accessing user info (what is currently admission webhook's user.Info field) ever be possible?
For context, we use mutating webhook to set requester info on an object being created for a later authorization type of check https://github.com/cert-manager/cert-manager/blob/v1.12.0-beta.0/internal/plugin/admission/certificaterequest/identity/certificaterequest_identity.go#L65-L71

Copy link
Contributor Author

@jpbetz jpbetz Apr 26, 2023

Choose a reason for hiding this comment

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

Yes, it will. For ValidatingAdmissionPolicy we provided access to all the request info available to admission webhooks (including user info) via a "request" variable. We will do the same here. I'll add a note in this KEP.

We also provide an "authorizer" variable that provides access to functions that can be used to perform authz checks.


```

The `mutation` field contains a CEL expression that evalutes to a partially
Copy link
Member

Choose a reason for hiding this comment

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

One question here, does it support like injecting a node label to pod and validate the pod's node labels not conflict. E.g.
pod has a label foo:bar, we want to inject a new label like foo:fuz, and we found they're conflicted, then fail into pod creation failure. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I fully understand the question, but a mutation can add a label, and it can do this conditionally. A validation can be paired with the mutation to reject requests where there was a conflict.

Copy link
Member

Choose a reason for hiding this comment

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

My use case is when a pod is located at a specific namespace, I would like to inject some node selectors and node taints, but before injecting, I would like verify whether they're conflict with pod's current node selectors and taints, if conflict, then reject the pod creation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I understand. What would be considered a conflict?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(I'm going to add a use case for this)

Copy link
Member

Choose a reason for hiding this comment

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

Take the nodeSelector for example, we want to inject a foo:bar to pod.nodeSelector but the current pod has a foo:fuz, then they're conflict.

All these ideas come from the exist podNodeSelector and podTolerationRestriction admission plugins.

manager. This has important implications in how the merge is performed that will
be discussed in more detail in the below "Unsetting values" section.

The `mutationValidation` field declares how the validate the the mutation has
Copy link
Member

Choose a reason for hiding this comment

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

Given that the controller knows exactly which fields it's changing and how it should be changed, can this controller do the validation automatically to ensure only the requested fields are updated while others are untouched?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I suggested a possible way to specify this via a ExactAfterReplay enum value for mutationValidation (https://github.com/kubernetes/enhancements/blob/59ea27c348c7a27c7b233730f6851c4b82605917/keps/sig-api-machinery/3962-mutating-admission-policies/README.md#safety).

I will admit I'm not happy with the enum value name ExactAfterReplay, so ideas welcome both on the concept of this validation and of the enum name.

@jpbetz
Copy link
Contributor Author

jpbetz commented Jan 9, 2024

A replacement KEP for this is in the works by @cici37. Closing this one.

@jpbetz jpbetz closed this Jan 9, 2024
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants