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: Mutating admission policy documentation #48646

Merged
merged 5 commits into from
Nov 26, 2024

Conversation

Jefftree
Copy link
Member

@Jefftree Jefftree commented Nov 5, 2024

Description

This documents KEP-3962 for the alpha release of the enhancement in 1.32

Supersedes #48467

Comments still outstanding from that PR are addressed.

@k8s-ci-robot k8s-ci-robot added this to the 1.32 milestone Nov 5, 2024
@k8s-ci-robot k8s-ci-robot added the language/en Issues or PRs related to English language label Nov 5, 2024
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 5, 2024
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 5, 2024
Copy link

netlify bot commented Nov 5, 2024

👷 Deploy Preview for kubernetes-io-vnext-staging processing.

Name Link
🔨 Latest commit d58a4e5
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/674607b0ca6f640008a5f084

Copy link

netlify bot commented Nov 5, 2024

Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit d58a4e5
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/674607b088ce7f0008587d95
😎 Deploy Preview https://deploy-preview-48646--kubernetes-io-main-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@sftim
Copy link
Contributor

sftim commented Nov 7, 2024

Comments still outstanding from that PR that need to be addressed here.

/retitle [WIP] KEP-3962: Mutating admission policy documentation

Sounds like this is not yet ready for review, so I'll mark it as a draft. @Jefftree if you can get it ready, that's helpful.

@k8s-ci-robot k8s-ci-robot changed the title KEP-3962: Mutating admission policy documentation [WIP] KEP-3962: Mutating admission policy documentation Nov 7, 2024
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 7, 2024
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 19, 2024
@Jefftree Jefftree changed the title [WIP] KEP-3962: Mutating admission policy documentation KEP-3962: Mutating admission policy documentation Nov 21, 2024
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 21, 2024
@Jefftree
Copy link
Member Author

/assign @jpbetz @cici37

Copy link
Contributor

@jpbetz jpbetz left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm
For technical content for alpha

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 21, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: fa93e23869f2228fea584e83482f736326d923c0

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 22, 2024
Copy link
Member Author

@Jefftree Jefftree left a comment

Choose a reason for hiding this comment

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

Comments addressed @tengqm

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

Thanks. It's good to have this doc ready for review on schedule.

We try to avoid putting tutorials and conceptual explanations into the docs reference section, so I'd love to see changes to this PR.

Please consider rewording https://kubernetes.io/docs/concepts/policy/#apply-policies-using-validatingadmissionpolicy to mention mutating admission policies too. This change is important.

For beta (and ideally even for alpha), we should (as in could, not must) turn this guide-style explanation into:

  • a tutorial that helps people try out VAP and MAP in their toy cluster
    • yes, I mean one combined tutorial that teaches both kinds of admission policies
    • yes, I know that ValidatingAdmissionPolicy is GA and MutatingAdmissionPolicy is alpha; that's OK
  • a shorter reference explanation of MutatingAdmissionPolicy, to live within the reference pages

Please consider doing or supporting that work eventually, @Jefftree. "Supporting" need not mean you do the work; it could be more around putting out a call for help.
As an opinion, but also the opinion of a former tech lead, that refactoring to a tutorial would really help people learn about K8s admission.

I've added some inline feedback too. See what you think.


Here's the part of the page I'd keep for the reference section (perhaps slightly expanded):

CEL expressions have access to the types needed to create JSON patches and objects:

  • JSONPatch - CEL type of JSON Patch operations. JSONPatch has the fields op, from, path and value.
    See JSON patch for more details. The value field may be set to any of: string,
    integer, array, map or object. If set, the path and from fields must be set to a
    JSON pointer string, where the jsonpatch.escapeKey() CEL
    function may be used to escape path keys containing / and ~.
  • Object - CEL type of the resource object.
  • Object.<fieldName> - CEL type of object field (such as Object.spec)
  • Object.<fieldName1>.<fieldName2>...<fieldNameN> - CEL type of nested field (such as Object.spec.containers)

CEL expressions have access to the contents of the API request, organized into CEL variables as well as some other useful variables:

  • object - The object from the incoming request. The value is null for DELETE requests.
  • oldObject - The existing object. The value is null for CREATE requests.
  • request - Attributes of the API request.
  • params - Parameter resource referred to by the policy binding being evaluated. Only populated if the policy has a ParamKind.
  • namespaceObject - The namespace object that the incoming object belongs to. The value is null for cluster-scoped resources.
  • variables - Map of composited variables, from its name to its lazily evaluated value.
    For example, a variable named foo can be accessed as variables.foo.
  • authorizer - A CEL Authorizer. May be used to perform authorization checks for the principal (user or service account) of the request.
    See https://pkg.go.dev/k8s.io/apiserver/pkg/cel/library#Authz
  • authorizer.requestResource - A CEL ResourceCheck constructed from the authorizer and configured with the
    request resource.

@sftim
Copy link
Contributor

sftim commented Nov 23, 2024

Overall: I'm wary to approve this as-is, but @Jefftree once you have read the feedback, please feel free to make the minimum set of changes you feel are right for alpha stage. Then request a new review (you could consider the auto-assigned reviewers, @natalisucks and @reylejano).

@AnshumanTripathi
Copy link
Contributor

Hi @jpbetz 👋 !
I'm reaching out from the Docs team. Just checking in as we approach Docs Freeze on Tuesday November 26th 18:00 PDT. This documentation appears to still be under review. To meet the Docs Freeze, this PR must have a technical review as well as lgtm and approve labels applied, without any unaddressed comments or concerns from SIG Docs. Thank you!

@Jefftree
Copy link
Member Author

@sftim comments addressed. Tried to put together a minimal set of references with a single example for applyconfiguration and json patch without too many examples making this into a tutorial. As a follow up (after docs freeze), will create a new page as a tutorial for VAP and MAP.

request resource.

The `apiVersion`, `kind`, `metadata.name` and `metadata.generateName` are always accessible from the root of the
object. No other metadata properties are accessible.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it able to check the labels of an object?
I don't mean I want to change it.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, thanks for catching!

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, also: annotations?

@sftim
Copy link
Contributor

sftim commented Nov 26, 2024

/label tide/merge-method-squash

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Nov 26, 2024
Co-authored-by: Tim Bannister <tim@scalefactory.com>
@Jefftree
Copy link
Member Author

@sftim comments addressed, thanks!

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

/lgtm

with a quibble

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 26, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: ff4ae603d81471439400e990fb183b4543a463ed

@chanieljdan
Copy link
Contributor

Docs and Tech LGTM provided above

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chanieljdan, 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 approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 26, 2024
@k8s-ci-robot k8s-ci-robot merged commit cb8e5a7 into kubernetes:dev-1.32 Nov 26, 2024
6 checks passed
Andygol pushed a commit to Andygol/k8s-website that referenced this pull request Dec 31, 2024
* Introduce concept page for mutating admission policy

* add example and documentation for MAP

* fix MAP feature gate documentation

* address comments

* Apply suggestions from code review

Co-authored-by: Tim Bannister <tim@scalefactory.com>

---------

Co-authored-by: Joe Betz <jpbetz@google.com>
Co-authored-by: Tim Bannister <tim@scalefactory.com>
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. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants