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

Improve the ValidationAdmissionPolicy docs #38504

Open
samos123 opened this issue Dec 16, 2022 · 26 comments
Open

Improve the ValidationAdmissionPolicy docs #38504

samos123 opened this issue Dec 16, 2022 · 26 comments
Labels
kind/feature Categorizes issue or PR as related to a new feature. language/en Issues or PRs related to English language priority/backlog Higher priority than priority/awaiting-more-evidence. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@samos123
Copy link
Contributor

samos123 commented Dec 16, 2022

This is a Feature Request

What would you like to be added

  • End-to-end example: Add example of adding a label environment=test to one of the namespace
  • Add explanation to the examples
  • Add a note about how to enable the admissionregistration.k8s.io/v1alpha1 API or link to another doc that talks about runtimeConfig
  • remove all the example.com parts since it was mostly confusing
  • Link to API docs for the various objects mentioned in the doc
  • Move all examples to separate files and ensure they are tested, this would prevent issues like ValidationAdmissionPolicy binding example is incorrect #38495 from occurring again, see comment from @tengqm below

comment from @tengqm:
Issues like this would be easy to detect if we make the manifest an example YAML.
For example:

  1. move the YAML snippet into a separate file content/en/examples/policy/validating-admission-ex.yaml and,
  2. reference it as {{< codenew file="policy/validating-admission-ex.yaml" >}} in the markdown page,
  3. (optionally) amend content/en/examples/examples_test.go so that it can be validated against current and future k8s releases.

Why is this needed
Current docs was hard to follow for a new user like myself

@samos123 samos123 added the kind/feature Categorizes issue or PR as related to a new feature. label Dec 16, 2022
@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Dec 16, 2022
@jimmyraywv
Copy link

So, more documentation about what the admission controller receives from the API would also be helpful. Is it the customary AdmissionReview object?

@sftim
Copy link
Contributor

sftim commented Dec 17, 2022

more documentation about what the admission controller receives from the API would also be helpful. Is it the customary AdmissionReview object?

We typically don't document internal details like that. Maybe that's something we could document within eg https://k8s.dev/docs/

Kubernetes is free to change its internal implementation and end users shouldn't need to know how that's done.

@sftim
Copy link
Contributor

sftim commented Dec 17, 2022

/sig api-machinery

Improvements are welcome

This is an alpha feature; we should aim to make the docs better before this graduates to beta.
/priority backlog

@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. priority/backlog Higher priority than priority/awaiting-more-evidence. labels Dec 17, 2022
@sftim
Copy link
Contributor

sftim commented Dec 17, 2022

/language en

If we wanted to, we could add a new tutorial that shows how to do admission-time validation. First with a webhook and then with CEL.

@k8s-ci-robot k8s-ci-robot added the language/en Issues or PRs related to English language label Dec 17, 2022
@jimmyraywv
Copy link

jimmyraywv commented Dec 17, 2022

@sftim The issue is that we need to know what data is sent in the api server request to the ValidatingAdmissionPolicy, so that we can understand what capabilities this solution has.

If you look at the docs for Webhook Mode, you can see that the API server request is documented there: https://kubernetes.io/docs/reference/access-authn-authz/webhook/

The AdmissionReview object is also documented in the Dynamic Admission Control docs: https://kubernetes.io/docs/reference/access-authn-authz/extensible-admission-controllers/#webhook-request-and-response

@tengqm
Copy link
Contributor

tengqm commented Dec 18, 2022

PR #38522 is adding a reference to the admission config API where AdmissionReview, AdmissionRequest are documented. Is that what you want, @jimmyraywv ?

@jimmyraywv
Copy link

@tengqm I think that will work. However for UserInfo v1 authentication.k8s.io, for the groups []string, are those k8s RBAC groups or are those AuthN IDP groups, like group claims from OIDC?

What I am driving towards is that will this feature allow us to write policies with CEL that blur the lines between traditional validating admission webhooks, and the AuthZ Webhook Mode?

@tengqm
Copy link
Contributor

tengqm commented Dec 18, 2022

@tengqm I think that will work. However for UserInfo v1 authentication.k8s.io, for the groups []string, are those k8s RBAC groups or are those AuthN IDP groups, like group claims from OIDC?

AFAICT, k8s doesn't have an abstraction for users or an abstraction for groups. All those information are extracted from the authentication input.

What I am driving towards is that will this feature allow us to write policies with CEL that blur the lines between traditional validating admission webhooks, and the AuthZ Webhook Mode?

I believe CEL was not designed for authn or authz's purpose. It is a mechanism for you to leverage, to impose constraints that are difficult or impossible to express using OpenAPI schemas. You can mix these admission plugins in any way to meet your requirements.

@sftim
Copy link
Contributor

sftim commented Dec 18, 2022

AFAICT, k8s doesn't have an abstraction for users or an abstraction for groups. All those information are extracted from the authentication input.

You can extend Kubernetes to add those things - this isn't just theoretical, there's a User API in OpenShift 4.8 for example.

Core Kubernetes doesn't have User or Group APIs. If your container platform does have extension APIs for users and groups, this alpha feature lets you use CEL to validate changes at admission time.

You asked about the (also alpha) SelfSubjectReview API.

https://kubernetes.io/docs/reference/kubernetes-api/authorization-resources/self-subject-review-v1alpha1/#SelfSubjectReviewStatus is filled in after you create a SelfSubjectReview. Here's what you might submit, as YAML, if you wanted to find out who the Kubernetes cluster thinks you are:

apiVersion: authentication.k8s.io/v1alpha1
kind: SelfSubjectReview

At HTTP level, the body of response might look like:

{
  "apiVersion": "authentication.k8s.io/v1alpha1",
  "kind": "SelfSubjectReview",
  "status": {
    "userInfo": {
      "username": "sftim",
      "uid": "71d99f8d-5b1b-4625-9149-037dc46fdc83",
      "groups": [
        "en-reviewers",
        "en-approvers",
        "system:authenticated"
      ],
      "extra": {
        "emoji": [
          "true"
        ]
      }
    }
  }
}

but a ValidatingAdmissionPolicy constrains what you send there, not what you get back.


In a couple of days, we'll have a blog article about this. I'm afraid it's not yet live. Please check back on the 20th of December.

We don't have anything to document here that I can see. Is there a page that needs to be more clear?

@sftim
Copy link
Contributor

sftim commented Dec 19, 2022

The issue is that we need to know what data is sent in the API server request to the ValidatingAdmissionPolicy, so that we can understand what capabilities this solution has.

As a model of its operation, the API server doesn't send any data to itself to implement and enforce a ValidatingAdmissionPolicy. Instead, the ValidatingAdmissionPolicy has access to:

  • the entire future object, were admission to succeed
  • its parameter data, it any

A ValidatingAdmissionPolicy doesn't have access to any context about the client, the current time, the phase of the moon, etc; if you want to take those into account, you'd need to use a validating admission webhook or some even more custom way to extend Kubernetes.

The docs don't explicitly say this; they explain what is possible and leave you to work the rest out by example. In a tutorial page, if we had one, we could make that point much more explicitly.

@sftim
Copy link
Contributor

sftim commented Dec 19, 2022

A new reference page would be good too: we could specify what version of CEL you can use, and the (short) list of available variables.

If we add one, that reference should cover CEL expressions for CRD validation and for ValidatingAdmissionPolicies, in one place. Would that help you @samos123?

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle stale
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 19, 2023
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle rotten
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Apr 18, 2023
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue with /reopen
  • Mark this issue as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close not-planned

@k8s-ci-robot
Copy link
Contributor

@k8s-triage-robot: Closing this issue, marking it as "Not Planned".

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue with /reopen
  • Mark this issue as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close not-planned

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.

@k8s-ci-robot k8s-ci-robot closed this as not planned Won't fix, can't repro, duplicate, stale May 18, 2023
@sftim
Copy link
Contributor

sftim commented May 18, 2023

/reopen

I think this is worth doing.

/sig api-machinery
/remove-lifecycle rotten
/priority backlog

@k8s-ci-robot
Copy link
Contributor

@sftim: Reopened this issue.

In response to this:

/reopen

I think this is worth doing.

/sig api-machinery
/remove-lifecycle rotten
/priority backlog

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.

@k8s-ci-robot k8s-ci-robot reopened this May 18, 2023
@k8s-ci-robot k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label May 18, 2023
@jpbetz
Copy link
Contributor

jpbetz commented May 18, 2023

@cici37 Would you be willing to find someone to propose an outline?

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle stale
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 20, 2024
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle rotten
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Feb 19, 2024
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue with /reopen
  • Mark this issue as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close not-planned

@k8s-ci-robot
Copy link
Contributor

@k8s-triage-robot: Closing this issue, marking it as "Not Planned".

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue with /reopen
  • Mark this issue as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close not-planned

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.

@k8s-ci-robot k8s-ci-robot closed this as not planned Won't fix, can't repro, duplicate, stale Mar 20, 2024
@cici37
Copy link
Contributor

cici37 commented Mar 20, 2024

/assign @mjlshen
/reopen
Hi @mjlshen , here is the old issue of suggestions on existing doc style. It would be great if it would squeeze into your doc PR :)

/triage accepted

@k8s-ci-robot
Copy link
Contributor

@cici37: GitHub didn't allow me to assign the following users: mjlshen.

Note that only kubernetes members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @mjlshen
/reopen
Hi @mjlshen , here is the old issue of suggestions on existing doc style. It would be great if it would squeeze into your doc PR :)

/triage accepted

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.

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Mar 20, 2024
@k8s-ci-robot
Copy link
Contributor

@cici37: Reopened this issue.

In response to this:

/assign @mjlshen
/reopen
Hi @mjlshen , here is the old issue of suggestions on existing doc style. It would be great if it would squeeze into your doc PR :)

/triage accepted

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.

@k8s-ci-robot k8s-ci-robot reopened this Mar 20, 2024
@a-mccarthy
Copy link
Contributor

/remove-lifecycle rotten

@k8s-ci-robot k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. language/en Issues or PRs related to English language priority/backlog Higher priority than priority/awaiting-more-evidence. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

No branches or pull requests

9 participants