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

Validating admission metrics integration #113475

Merged
merged 1 commit into from Nov 10, 2022
Merged

Validating admission metrics integration #113475

merged 1 commit into from Nov 10, 2022

Conversation

DangerOnTheRanger
Copy link
Contributor

@DangerOnTheRanger DangerOnTheRanger commented Oct 31, 2022 •
edited

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR is the additional PR mentioned in #112994; this PR integrates the merged metrics implementation with the rest of the work done for validation admission.

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[KEP]: https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/3488-cel-admission-control

@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 do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. kind/feature Categorizes issue or PR as related to a new feature. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/apiserver area/code-generation area/test kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 31, 2022
@DangerOnTheRanger DangerOnTheRanger changed the title [WIP] Validation admission metrics integration Validation admission metrics integration Oct 31, 2022
@DangerOnTheRanger DangerOnTheRanger changed the title Validation admission metrics integration Validating admission metrics integration Oct 31, 2022
@fedebongio
Copy link
Contributor

/triage accepted

@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 Nov 1, 2022
@jpbetz jpbetz mentioned this pull request Nov 2, 2022
Open
16 tasks
jpbetz
jpbetz reviewed Nov 5, 2022
View reviewed changes
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.

This needs a rebase.

staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/implementation.go Outdated Show resolved Hide resolved
@jpbetz
Copy link
Contributor

jpbetz commented Nov 6, 2022

@cici37, @DangerOnTheRanger, Is this still targeting 1.26?

@DangerOnTheRanger DangerOnTheRanger force-pushed the validation-admission-metrics-integration branch from 72c6786 to a82dc77 Compare November 6, 2022 22:59
@k8s-ci-robot k8s-ci-robot added area/cloudprovider sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. labels Nov 6, 2022
@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 8, 2022
@cici37
Copy link
Contributor

cici37 commented Nov 8, 2022

The changes look good. Could we have a follow-up task to rename the package staging/src/k8s.io/apiserver/pkg/admission/cel? It only includes metrics inside :)

@cici37
Copy link
Contributor

cici37 commented Nov 8, 2022 •
edited

Note: currently we use ObserveRejection for all deny cases. Later we plan to further differentiate the cases between deny because of the validation failed and deny because of error. And possibly add one for all admit cases. Thanks

@DangerOnTheRanger DangerOnTheRanger force-pushed the validation-admission-metrics-integration branch from 82338ed to c2c886d Compare November 8, 2022 20:25
lavalamp
lavalamp reviewed Nov 8, 2022
View reviewed changes
staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/policy_decision.go Outdated
// this is important for metrics, since we want to differentiate between
// a regular "admit" (no evaluation errors/expressions failing to validate)
// versus an "admit" due to failure policy (some error/failure occurred, but
// was ignored)
Copy link
Member

Choose a reason for hiding this comment

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

This is extremely confusing.
Consider having two axes:

  • evaluation: error, admit, block
  • action: admit, block

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense - I'll rework policyDecisionKind into policyDecisionAction and then add a policyDecisionEvaluation type and add that into policyDecision and the other appropriate places.

Copy link
Member

Choose a reason for hiding this comment

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

Much clearer, thanks. Can you squash?

@DangerOnTheRanger DangerOnTheRanger force-pushed the validation-admission-metrics-integration branch from de0be04 to 588f03a Compare November 8, 2022 21:11
@lavalamp
Copy link
Member

lavalamp commented Nov 8, 2022

/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 Nov 8, 2022
BenTheElder
BenTheElder reviewed Nov 8, 2022
View reviewed changes
Copy link
Member

@BenTheElder BenTheElder left a comment

Choose a reason for hiding this comment

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

/approve
for vendor/modules.txt

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, DangerOnTheRanger, 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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 8, 2022
@andrewsykim
Copy link
Member

Adding the v1.26 milestone since this was ready to merge before code freeze yesterday and was just waiting for CI to run

/milestone v1.26

@k8s-ci-robot k8s-ci-robot added this to the v1.26 milestone Nov 9, 2022
@DangerOnTheRanger
Copy link
Contributor Author

/retest

@DangerOnTheRanger DangerOnTheRanger force-pushed the validation-admission-metrics-integration branch from 588f03a to cc3cf20 Compare November 9, 2022 19:03
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 9, 2022
@DangerOnTheRanger
Copy link
Contributor Author

(force push to fix CI, deny/admit were refactored out in this PR and it was causing issues with

kubernetes/staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/validator_test.go

Line 521 in c84e920

generatedDecision(deny, "params as required", metav1.StatusReasonForbidden),
)

@DangerOnTheRanger DangerOnTheRanger force-pushed the validation-admission-metrics-integration branch from cc3cf20 to 99494e6 Compare November 9, 2022 19:11
@DangerOnTheRanger
Copy link
Contributor Author

(force push again, fix some formatting issues)

@jpbetz
Copy link
Contributor

jpbetz commented Nov 9, 2022

/lgtm

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

/retest

@k8s-ci-robot k8s-ci-robot merged commit f872310 into kubernetes:master Nov 10, 2022
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

@lavalamp lavalamp lavalamp left review comments

@BenTheElder BenTheElder BenTheElder left review comments

@liggitt liggitt liggitt left review comments

@cici37 cici37 cici37 left review comments

@apelisse apelisse Awaiting requested review from apelisse

@ardaguclu ardaguclu Awaiting requested review from ardaguclu

@alexzielenski alexzielenski Awaiting requested review from alexzielenski

@andrewsykim andrewsykim Awaiting requested review from andrewsykim

Assignees

@jpbetz jpbetz

@lavalamp lavalamp

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/cloudprovider area/code-generation area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
v1.26
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@DangerOnTheRanger @k8s-ci-robot @fedebongio @jpbetz @k8s-triage-robot @cici37 @lavalamp @andrewsykim @BenTheElder @liggitt