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: CEL admission: Add graceful rollout, warning and audit support #3732

Merged
merged 8 commits into from
Feb 9, 2023
Merged

KEP-3488: CEL admission: Add graceful rollout, warning and audit support #3732

merged 8 commits into from
Feb 9, 2023

Conversation

jpbetz
Copy link
Contributor

@jpbetz jpbetz commented Jan 12, 2023

  • One-line PR description: Update the CEL admission KEP to support graceful policy rollout by adding ability to report validation failures only as warnings or audit annotations and for enhancing metrics to container the information needed to monitor a rollout.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 12, 2023
@k8s-ci-robot k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory approved Indicates a PR has been approved by an approver from all required OWNERS files. 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. labels Jan 12, 2023
@jpbetz
Copy link
Contributor Author

jpbetz commented Jan 12, 2023

cc @maxsmythe @cici37 @liggitt @tallclair @andrewsykim

@jpbetz jpbetz force-pushed the cel-admission-enforcement-actions branch from 73776a7 to 21cade5 Compare January 24, 2023 18:55
@jpbetz
Copy link
Contributor Author

jpbetz commented Jan 24, 2023 •
edited
Loading

cc @liggitt

@jpbetz
Copy link
Contributor Author

jpbetz commented Jan 24, 2023

/assign @deads2k @lavalamp
Here is another small KEP update to ValidatingAdmissionPolicy is ready for an approver pass.

@lavalamp
Copy link
Member

LGTM but two comments for you to look at first

@jpbetz
Copy link
Contributor Author

jpbetz commented Jan 31, 2023

Feedback applied. I've left some of the comment threads open where further discussion might be needed.

@jpbetz jpbetz force-pushed the cel-admission-enforcement-actions branch from da481d5 to 2edd5fc Compare January 31, 2023 23:26
This was referenced Feb 3, 2023
Merged
Open
@jpbetz jpbetz force-pushed the cel-admission-enforcement-actions branch from db6abf8 to 87615fc Compare February 3, 2023 23:31
@liggitt
Copy link
Member

liggitt commented Feb 4, 2023

thanks for the update, I did a quick sweep and my open questions were resolved

@jpbetz
Copy link
Contributor Author

jpbetz commented Feb 6, 2023

@deads2k @lavalamp All open issues on this one are resolved. It's ready for another approver pass.

tallclair
tallclair reviewed Feb 7, 2023
View reviewed changes
keps/sig-api-machinery/3488-cel-admission-control/README.md Outdated Show resolved Hide resolved
keps/sig-api-machinery/3488-cel-admission-control/README.md Outdated Show resolved Hide resolved
keps/sig-api-machinery/3488-cel-admission-control/README.md Outdated Show resolved Hide resolved
keps/sig-api-machinery/3488-cel-admission-control/README.md Show resolved Hide resolved
keps/sig-api-machinery/3488-cel-admission-control/README.md Show resolved Hide resolved
keps/sig-api-machinery/3488-cel-admission-control/README.md Outdated Show resolved Hide resolved
keps/sig-api-machinery/3488-cel-admission-control/README.md Outdated
"kind": "Event",
"apiVersion": "audit.k8s.io/v1",
"annotations": {
"mypolicy.mygroup.example.com/validation_failure": "{\"expression\": 1, \"message\": \"x must be greater than y\", \"enforcement\": \"Deny\", \"binding\": \"mybinding.mygroup.example.com\"}"
Copy link
Member

Choose a reason for hiding this comment

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

not something you need to address here, but I'm starting to worry that we're overusing audit annotations, and should start moving more metadata like this to dedicated fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I'm not satisfied with how the value payload works here. I'll admit to following precedence of other audit annotations rather than trying to re-think it. That said, I'd be okay with restructuring this. Any recommendations?

keps/sig-api-machinery/3488-cel-admission-control/README.md Show resolved Hide resolved
keps/sig-api-machinery/3488-cel-admission-control/README.md Show resolved Hide resolved
@jpbetz
Copy link
Contributor Author

jpbetz commented Feb 7, 2023

Thanks @tallclair, I've incorporated the feedback into the KEP. I've left some of the comments open until you've gotten a chance to respond.

@jpbetz
Copy link
Contributor Author

jpbetz commented Feb 8, 2023

@lavalamp this is ready for another approver pass.

@jpbetz jpbetz closed this Feb 8, 2023
@jpbetz jpbetz reopened this Feb 8, 2023
@jpbetz
Copy link
Contributor Author

jpbetz commented Feb 9, 2023

Thanks @lavalamp, feedback applied.

@lavalamp
Copy link
Member

lavalamp commented Feb 9, 2023

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jpbetz, lavalamp

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 merged commit 43a9cd2 into kubernetes:master Feb 9, 2023
@k8s-ci-robot k8s-ci-robot added this to the v1.27 milestone Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tallclair tallclair tallclair left review comments

@maxsmythe maxsmythe maxsmythe left review comments

@liggitt liggitt liggitt left review comments

@lavalamp lavalamp lavalamp left review comments

@apelisse apelisse Awaiting requested review from apelisse

@fedebongio fedebongio Awaiting requested review from fedebongio

Assignees

@lavalamp lavalamp

@deads2k deads2k

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.27
Development

Successfully merging this pull request may close these issues.
7 participants
@jpbetz @lavalamp @liggitt @k8s-ci-robot @maxsmythe @tallclair @deads2k