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
Add feature gate CelValidatingAdmission #112792
Conversation
/priority soon |
@cici37: The label(s) 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. |
/priority important-soon |
/triage accepted |
/triage accepted |
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.
/approve
/hold
(approving changes based on KEP, deferring lgtm to @jpbetz)
// alpha: v1.26 | ||
// | ||
// Enables expression validation in Admission Control | ||
CelValidatingAdmissionExtensibility featuregate.Feature = "CelValidatingAdmissionExtensibility" |
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 on naming: I feel like the Extensibility
is maybe redundant in the feature gate name, maybe just CELValidatingAdmission
or CELAdmission
might be more concise.
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.
Also, should Cel
be capitlized to CEL
, seems like we capitalize abbreviations in other features gates (e.g. APIPriorityAndFairness)
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 for reviewing! CELValidatingAdmission
looks good to me(we might have CELDefaultingAdmission or converting in the future :) ). Will update if no objection.
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.
Updated
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.
CELValidatingAdmission
sounds good to me, the pros of just CELAdmission
is that you leave flexibility to reuse that same feature gate for defaulting/converting admission if it happens to fall under the same KEP (but may be we want them distinct anyways)
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'm okay with either. Slight preference for CELValidatingAdmission
just because I'm 99% sure mutation support is quite a ways off.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrewsykim, cici37 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 |
/unhold |
/test pull-kubernetes-e2e-gce-ubuntu-containerd |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Add feature gate CelValidatingAdmissionExtensibility based on kep: http://kep.k8s.io/3488
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
/sig api-machinery