KEP-3488: Promote ValidatingAdmissionPolicy to Beta #118644

alexzielenski · 2023-06-13T22:15:08Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Prerequisites:

Graduation Criteria from KEP:

Add tests which registers a validation policy for everything and iterates through all groups/versions/resources/subresources and ensures they get intercepted and work properly with a CEL validation policy
- Context: Implement CEL for Admission Control #113314 (comment)
- Implementation: Add test cases for ValidatingAdmissionPolicy #119409
Set paramKind in a ValidatingAdmissionPolicy results in starting a new informer that watches all instances of that object using a new unstructured informer which is inefficient
- Context: Implement CEL for Admission Control #113314 (comment)
- Implementation: ValidatingAdmissionPolicy: use natively-typed informer for params #115145
Switch to a lock-free implementation to address lock having to wait for all existing admission evaluations to complete and blocking all new admission evaluations until this completes.
- Context: Implement CEL for Admission Control #113314 (comment)
- Implementation: refactor policy admission Validator to be lock free #114527
Perform minimal possible number of conversions when evaluating multiple admission policies for a request resource. If multiple admission policies require the same conversion, convert only once.
- Context: @liggitt: "webhook code loops up one level, first accumulates all the validation webhooks we'll run, then converts to the versions needed by those webhooks then evaluates in parallel"
- Partial Implementation: make CEL admission controller code consumable #115412
  added caching of the conversion on per-policy basis, but multiple policies sharing paramKind may still reconvert
Authz check to the specific resource referenced in the policy's paramKind.
- Context: Implement CEL for Admission Control #113314 (comment)
- Implementation: d3f4813
Complete feature of access to namespace metadata
- Implementation: ValidatingAdmissionPolicy: support namespace access #118267
Complete type check for CRD: Purely advisory and should not block
- Implementation: ValidatingAdmissionPolicy - Type Checking for API Expensions types #119109
add controlled rollout strategy to support future CEL library/function/variable changes
- Implementation: Controlled rollout of CEL libraries and language feautres #116779
Quantity support from CEL expression and tested properly
- CEL Library Implementation & tests: Add quantity library to CEL #118803
Support phase 2 features:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Promoted API groups `ValidatingAdmissionPolicy` and `ValidatingAdmissionPolicyBinding` to `v1beta1`.

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

- [KEP]: https://github.com/kubernetes/enhancements/issues/3488

/sig api-machinery
/cc @cici37 @jpbetz

jpbetz

@alexzielenski This looks ready for API review to me. Would you set that up?

pkg/apis/admissionregistration/types.go

alexzielenski · 2023-06-26T17:23:53Z

/label api-review

/cc @deads2k
for API review

Still adding tests and tuning implementation. Expecting no further changes to types/global API code (excepting documentation and other feedback).

alexzielenski · 2023-06-27T20:04:39Z

/triage accepted

deads2k

I did not finish reviewing all the controller changes. Can we separate the "api, defaulting, conversion, validation" from the controller and admission changes? I know there are behavioral changes required, but freeing up the API first broadens reviewers.

staging/src/k8s.io/api/admissionregistration/v1beta1/types.go

pkg/apis/admissionregistration/validation/validation.go

pkg/apis/admissionregistration/v1beta1/defaults.go

staging/src/k8s.io/api/admissionregistration/v1alpha1/types.go

staging/src/k8s.io/api/admissionregistration/v1beta1/types.go

pkg/printers/internalversion/printers.go

pkg/registry/admissionregistration/validatingadmissionpolicybinding/authz.go

...rc/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/matching/matching_test.go

staging/src/k8s.io/apiserver/pkg/features/kube_features.go

deads2k · 2023-07-21T20:05:38Z

API and impl changes look good. The tests need fixing before merge and the tests need to be added for the beta.

/approve

/assign @jpbetz @cici37
for lgtm

cici37 · 2023-07-21T20:07:43Z

Thank you! @alexzielenski Please update following when you got time.

Revert the feature gate change in this PR
Fix the accidentally touch for version in match_test

Thank you! ^^

v1alpha -> v1beta fill in DenyAction where there is no ParameterNotFoundAction

k8s-ci-robot · 2023-07-21T20:42:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexzielenski, deads2k

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:

~~api/OWNERS~~ [deads2k]
~~cmd/kube-apiserver/OWNERS~~ [deads2k]
~~cmd/kube-controller-manager/OWNERS~~ [deads2k]
~~pkg/api/OWNERS~~ [deads2k]
~~pkg/apis/OWNERS~~ [deads2k]
~~pkg/controller/OWNERS~~ [deads2k]
~~pkg/controlplane/OWNERS~~ [deads2k]
~~pkg/generated/openapi/OWNERS~~ [deads2k]
~~pkg/registry/admissionregistration/OWNERS~~ [deads2k]
~~staging/src/k8s.io/api/OWNERS~~ [deads2k]
~~staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/OWNERS~~ [alexzielenski,deads2k]
~~staging/src/k8s.io/client-go/OWNERS~~ [deads2k]
~~test/e2e/apimachinery/OWNERS~~ [deads2k]
~~test/integration/apiserver/OWNERS~~ [alexzielenski,deads2k]
~~test/integration/etcd/OWNERS~~ [deads2k]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller.go

cici37 · 2023-07-21T21:08:31Z

/lgtm
Thank you!

k8s-ci-robot · 2023-07-21T21:08:37Z

LGTM label has been added.

Git tree hash: 6f3cbba547c06b8dfe7f1f0c2fcc03a6f3ea0726

cici37 · 2023-07-21T21:53:25Z

/retest

k8s-ci-robot · 2023-07-22T00:10:32Z

@alexzielenski: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-e2e-gce-cos-alpha-features	`e73fbb2`	link	false	`/test pull-kubernetes-e2e-gce-cos-alpha-features`
pull-kubernetes-e2e-gce	`d647958`	link	unknown	`/test pull-kubernetes-e2e-gce`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

alexzielenski · 2023-07-22T00:42:40Z

/retest

sftim · 2023-07-25T09:12:57Z

The KEP link for the changelog should be to k/enhancements, not - https://github.com/cici37/enhancements/

sftim · 2023-07-25T09:13:39Z

Changelog suggestion:

- Promoted validating admission policies to beta
- Promoted API groups `ValidatingAdmissionPolicy` and `ValidatingAdmissionPolicyBinding` to `v1beta1`.

cici37 · 2023-07-25T17:00:55Z

@sftim Thank you for the suggestion! I have updated the PR description for the KEP link and updated the release note section as well. Note that we promote the feature gate in another PR(#119409) hence I will only add the last API promotion info into the changelog of this PR.

k8s-ci-robot requested a review from cici37 June 13, 2023 22:15

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Jun 13, 2023

k8s-ci-robot requested a review from jpbetz June 13, 2023 22:15

alexzielenski changed the title ~~[WIP] ValidatingAdmissionPolicy v1beta1 types~~ [WIP] ValidatingAdmissionPolicy v1beta1 types and controller update Jun 13, 2023

k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Jun 23, 2023

jpbetz suggested changes Jun 24, 2023

View reviewed changes

pkg/apis/admissionregistration/types.go Outdated Show resolved Hide resolved

pkg/apis/admissionregistration/types.go Outdated Show resolved Hide resolved

pkg/apis/admissionregistration/types.go Outdated Show resolved Hide resolved

k8s-ci-robot requested a review from deads2k June 26, 2023 17:23

k8s-ci-robot added the api-review Categorizes an issue or PR as actively needing an API review. label Jun 26, 2023

alexzielenski changed the title ~~[WIP] ValidatingAdmissionPolicy v1beta1 types and controller update~~ ValidatingAdmissionPolicy v1beta1 types and controller update Jun 26, 2023

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 26, 2023

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 Jun 27, 2023

deads2k requested changes Jun 29, 2023

View reviewed changes

alexzielenski force-pushed the apiserver/policy/namespaceParamRef branch 2 times, most recently from 8d0206a to bd581b1 Compare June 29, 2023 22:33

benluddy reviewed Jul 3, 2023

View reviewed changes

pkg/printers/internalversion/printers.go Outdated Show resolved Hide resolved

pkg/registry/admissionregistration/validatingadmissionpolicybinding/authz.go Outdated Show resolved Hide resolved

deads2k reviewed Jul 21, 2023

View reviewed changes

...rc/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/matching/matching_test.go Outdated Show resolved Hide resolved

deads2k reviewed Jul 21, 2023

View reviewed changes

...rc/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/matching/matching_test.go Outdated Show resolved Hide resolved

deads2k reviewed Jul 21, 2023

View reviewed changes

staging/src/k8s.io/apiserver/pkg/features/kube_features.go Outdated Show resolved Hide resolved

k8s-ci-robot assigned cici37 and jpbetz Jul 21, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 21, 2023

refactor: replace usage of v1alpha1 with v1beta1

ef8670c

v1alpha -> v1beta fill in DenyAction where there is no ParameterNotFoundAction

alexzielenski force-pushed the apiserver/policy/namespaceParamRef branch from e73fbb2 to c4cd92a Compare July 21, 2023 20:41

cici37 reviewed Jul 21, 2023

View reviewed changes

staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller.go Outdated Show resolved Hide resolved

cici37 reviewed Jul 21, 2023

View reviewed changes

staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller.go Outdated Show resolved Hide resolved

update codegen

d647958

alexzielenski force-pushed the apiserver/policy/namespaceParamRef branch from c4cd92a to d647958 Compare July 21, 2023 20:56

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 21, 2023

k8s-ci-robot merged commit 18f8cb8 into kubernetes:master Jul 22, 2023
12 of 13 checks passed

cici37 mentioned this pull request Jul 25, 2023

[WIP]KEP-3488: promote ValidatingAdmissionPolicy to beta #118258

Closed

liggitt moved this from Assigned to API review completed, 1.28 in API Reviews Aug 17, 2023

alexzielenski mentioned this pull request Aug 17, 2023

Store validating admission as beta #120018

Merged

alexzielenski mentioned this pull request Feb 7, 2024

KEP-4330: Compatibility versions kubernetes/enhancements#4395

Merged

jaskaransarkaria mentioned this pull request Jun 5, 2024

Planning upgrade to EKS 1.28 ministryofjustice/cloud-platform#5570

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KEP-3488: Promote ValidatingAdmissionPolicy to Beta #118644

KEP-3488: Promote ValidatingAdmissionPolicy to Beta #118644

alexzielenski commented Jun 13, 2023 •

edited by cici37

jpbetz left a comment

alexzielenski commented Jun 26, 2023 •

edited

alexzielenski commented Jun 27, 2023

deads2k left a comment

deads2k commented Jul 21, 2023

cici37 commented Jul 21, 2023

k8s-ci-robot commented Jul 21, 2023

cici37 commented Jul 21, 2023

k8s-ci-robot commented Jul 21, 2023

cici37 commented Jul 21, 2023

k8s-ci-robot commented Jul 22, 2023

alexzielenski commented Jul 22, 2023

sftim commented Jul 25, 2023

sftim commented Jul 25, 2023

cici37 commented Jul 25, 2023

KEP-3488: Promote ValidatingAdmissionPolicy to Beta #118644

KEP-3488: Promote ValidatingAdmissionPolicy to Beta #118644

Conversation

alexzielenski commented Jun 13, 2023 • edited by cici37

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR 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.:

jpbetz left a comment

Choose a reason for hiding this comment

alexzielenski commented Jun 26, 2023 • edited

alexzielenski commented Jun 27, 2023

deads2k left a comment

Choose a reason for hiding this comment

deads2k commented Jul 21, 2023

cici37 commented Jul 21, 2023

k8s-ci-robot commented Jul 21, 2023

cici37 commented Jul 21, 2023

k8s-ci-robot commented Jul 21, 2023

cici37 commented Jul 21, 2023

k8s-ci-robot commented Jul 22, 2023

alexzielenski commented Jul 22, 2023

sftim commented Jul 25, 2023

sftim commented Jul 25, 2023

cici37 commented Jul 25, 2023

alexzielenski commented Jun 13, 2023 •

edited by cici37

alexzielenski commented Jun 26, 2023 •

edited