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.

Sign up for GitHub

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

Jump to bottom

KEP-3488: promote validatingadmissionpolicy to beta #3949

Merged
merged 2 commits into from
Jun 9, 2023
Merged

KEP-3488: promote validatingadmissionpolicy to beta #3949

merged 2 commits into from
Jun 9, 2023

Conversation

cici37
Copy link
Contributor

@cici37 cici37 commented Apr 10, 2023

  • One-line PR description: Clear beta graduation criteria for CEL in Admission Control
  • Other comments:

@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 the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 10, 2023
@cici37 cici37 marked this pull request as ready for review April 10, 2023 08:46
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Apr 10, 2023
@k8s-ci-robot k8s-ci-robot added 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/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 10, 2023
@cici37 cici37 mentioned this pull request Apr 10, 2023
Open
16 tasks
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 9, 2023
@cici37
Copy link
Contributor Author

cici37 commented May 16, 2023

/milestone v1.28

@k8s-ci-robot k8s-ci-robot added this to the v1.28 milestone May 16, 2023
@cici37 cici37 changed the title KEP-3488: Clear beta graduation criteria KEP-3488: promote validatingadmissionpolicy to beta May 23, 2023
@cici37 cici37 mentioned this pull request May 25, 2023
Closed
@cici37 cici37 force-pushed the vapBeta branch 2 times, most recently from 28d5008 to 241156a Compare May 26, 2023 18:30
@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 May 26, 2023
@cici37
Copy link
Contributor Author

cici37 commented May 26, 2023

/assign @jpbetz @deads2k
Thank you!

@cici37
Copy link
Contributor Author

cici37 commented May 31, 2023

/assign @johnbelamaric
for PRR review. Thank you

@deads2k
Copy link
Contributor

deads2k commented Jun 1, 2023

/hold

I'd like to work through what needs to happen to achieve original goals of per-namespace parameters with cluster defined policy.

will require a new API which could not be easily aligned with existing implementation

Sounds like we may have an API problem to address before we pin an API by moving to beta.

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jun 1, 2023
@cici37
Copy link
Contributor Author

cici37 commented Jun 2, 2023

Comments have been addressed and KEP updated with the new proposed design for namespaced policy binding. @deads2k Please take a look when have time. Thank you!

jpbetz
jpbetz reviewed Jun 2, 2023
View reviewed changes
keps/sig-api-machinery/3488-cel-admission-control/README.md
also have use cases that need to be able to inspects the fields in CEL
expressions.
`namespaceObject` will provide access to all existing fields under namespace metadata, namespace spec and namespace status except for metadata.managedFields and metadata.ownerReferences.
The fields could be directly accessed through `namespaceObject` variable. e.g. `namespaceObject.metadata.name` or `namespaceObject.status.phase`.
Copy link
Contributor

Choose a reason for hiding this comment

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

super nit: Can we call this variable objectNamespace? It might be just me, but I'm looking for a name that hints at the fact that this is "the namespace of the object", and when I see namespaceObject I interpret it as "a namespace that is an object"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is the implementation PR: kubernetes/kubernetes#118267
Maybe we could discuss there and update the KEP later?

@jpbetz
Copy link
Contributor

jpbetz commented Jun 2, 2023 •
edited

Added a few minor comments then lgtm.

cc @deads2k for any thoughts.

EDIT: Oops, wrong PR.

deads2k
deads2k reviewed Jun 7, 2023
View reviewed changes
keps/sig-api-machinery/3488-cel-admission-control/README.md
spec:
policyName: "demo-policy.example.com"
namespaceParamRef:
name: "param-resource.example.com"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want an option of name or label selector here to mean, "this particular one" or "honor all the parameter resources that match this label selector in that namespace" to allow slightly looser coupling. That allows evolution of namespace names using a kustomize style hash in each namespace with a single ValidatingAdmissionPolicyBinding working against many namespaces.

Copy link
Contributor

Choose a reason for hiding this comment

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

Added. I haven't showed a kustomize example, but I have shown one with a label selector along with a summary of the behavior if the selector matches multiple resources.

Copy link
Contributor

Choose a reason for hiding this comment

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

Added. I haven't showed a kustomize example, but I have shown one with a label selector along with a summary of the behavior if the selector matches multiple resources.

Commit 057a9c3 appears to be missing from the diff. Conflicting push somewhere?

It's exactly what I'm thinking though.

@deads2k
Copy link
Contributor

deads2k commented Jun 7, 2023

after https://github.com/kubernetes/enhancements/pull/3949/files#r1222291507, lgtm

@deads2k
Copy link
Contributor

deads2k commented Jun 9, 2023

/lgtm
/approve
/hold

holding for @johnbelamaric to approve PRR and a squash. I'm approving for the sig.

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 9, 2023
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 9, 2023
@jpbetz
Copy link
Contributor

jpbetz commented Jun 9, 2023

Rebased to minimal set of commits.

@johnbelamaric
Copy link
Member

/approve
/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cici37, deads2k, johnbelamaric

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

@johnbelamaric
Copy link
Member

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 9, 2023
@k8s-ci-robot k8s-ci-robot merged commit 3821d25 into kubernetes:master Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpbetz jpbetz jpbetz left review comments

@deads2k deads2k deads2k left review comments

@johnbelamaric johnbelamaric johnbelamaric left review comments

@lavalamp lavalamp Awaiting requested review from lavalamp

@fedebongio fedebongio Awaiting requested review from fedebongio

Assignees

@jpbetz jpbetz

@deads2k deads2k

@johnbelamaric johnbelamaric

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. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.28
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@cici37 @k8s-ci-robot @jpbetz @deads2k @johnbelamaric