Skip to content

Conversation

@jpbetz
Copy link
Contributor

@jpbetz jpbetz commented Sep 5, 2024

What type of PR is this?

/kind feature

What this PR does / why we need it:

Implements alpha of kubernetes/enhancements#3962

This supersedes #126497 to include:

  • API Updated to support JSON Patch and Apply Configuration mutations
  • Remove message and messageExpression since they have no obvious relevance for mutation
  • JSON Patch implementation + "jsonpatch.escapeKey()" CEL function
  • Variables
  • Runtime cost
  • request variable
  • authorizer variable
  • type check: CEL Object types must match field name paths
  • CRD version conversion
  • matchConditions
  • failurePolicy handling
  • relocation of reinvocationPolicy field to spec level
  • Disallow mutation of atomics with apply configurations
  • integration testing of: CRDs, CRD version conversion, matchConditions, failurePolicy, subresources, authorizers, request variables, json patch

Which issue(s) this PR fixes:

Special notes for your reviewer:

  • CEL expressions for Validating and Mutating admission policy are not type checked at compile time because there is no guarantee that the type to be validated exists yet (for CRDs), and the expressions are allowed to match multiple types. This has lots of ripple effects in this PR. At runtime we do some additional type checking.. I've added some type mismatch checking to avoid obvious errors, but there are still significant gaps, even at runtime. I have additional work not yet in this PR that takes the type checking further, but I figured it would be good to get a baseline implementation reviewed and then iterate.

Does this PR introduce a user-facing change?

Introduce v1alpha1 API for mutating admission policies, enabling extensible admission control via CEL expressions (KEP  3962: Mutating Admission Policies). To use, enable the `MutatingAdmissionPolicy` feature gate and the `admissionregistration.k8s.io/v1alpha1` API via `--runtime-config`.

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

Please use the following format for linking documentation:


@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 Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. 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. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Sep 5, 2024
@k8s-ci-robot k8s-ci-robot added 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/etcd Categorizes an issue or PR as relevant to SIG Etcd. 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 Sep 5, 2024
@jpbetz
Copy link
Contributor Author

jpbetz commented Sep 5, 2024

/label api-review

This needs additional tests but is otherwise feature complete and ready for API review.

@k8s-ci-robot k8s-ci-robot added the api-review Categorizes an issue or PR as actively needing an API review. label Sep 5, 2024
@jpbetz jpbetz force-pushed the mutating-admission branch from e898cdc to 0104d90 Compare September 5, 2024 04:19
@jpbetz
Copy link
Contributor Author

jpbetz commented Sep 5, 2024

/triage accepted
/priority important-longterm

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Sep 5, 2024
@jpbetz jpbetz marked this pull request as draft September 5, 2024 04:35
@jpbetz jpbetz marked this pull request as ready for review September 5, 2024 04:35
jpbetz and others added 12 commits November 4, 2024 21:40
This expands the generic plugin support to both validating and mutating policies.  It also adds the
mutating policy admission plugin using the generics plugin support.

This also implements both ApplyConfiguration and JSONPatch support.

Co-authored-by: Alexander Zielensk <alexzielenski@gmail.com>
Co-authored-by: cici37 <cicih@google.com>
Co-authored-by: Alexander Zielensk <alexzielenski@gmail.com>
Co-authored-by: Alexander Zielensk <alexzielenski@gmail.com>
Also apply reviewer feedback
@sttts
Copy link
Contributor

sttts commented Nov 5, 2024

/lgtm
/approve

/assign @deads2k
for final API review and approval.

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

LGTM label has been added.

Git tree hash: 59aabefafd6d980290490755f44ea9e39d30ff16

@jpbetz jpbetz force-pushed the mutating-admission branch from d02b47b to 0dc08ed Compare November 5, 2024 13:03
@jpbetz
Copy link
Contributor Author

jpbetz commented Nov 5, 2024

/retest

Flake was:

Kubernetes e2e suite: [It] [sig-node] Lifecycle Sleep Hook [NodeConformance] when create a pod with lifecycle hook using sleep action reduce GracePeriodSeconds during runtime expand_less	39s
{ failed [FAILED] unexpected delay duration before killing the pod, cost = 20.523691046s
In [It] at: k8s.io/kubernetes/test/e2e/common/node/lifecycle_hook.go:610 @ 11/05/24 13:29:02.553
}


genericfeatures.MutatingAdmissionPolicy: {
{Version: version.MustParse("1.30"), Default: false, PreRelease: featuregate.Alpha},
{Version: version.MustParse("1.32"), Default: false, PreRelease: featuregate.Alpha},
Copy link
Member

Choose a reason for hiding this comment

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

@aaron-prindle fyi something like this could cause our feature gate test to fail with 1.32 emulating 1.31. We should special case scenarios where gates were introduced and then deferred to a following release.

Copy link
Contributor

Choose a reason for hiding this comment

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

Discussed offline, the Compatility versions feature gate integration test that verifies gates are correct for n-1..3 versions (eg: binary @ 1.32 using --emulated-version=1.31 is in compliance with the feature gates from 1.31) already will work for this change as it is an alpha API and alpha APIs are allowed to be removed w.r.t to Compatibility versions

@deads2k
Copy link
Contributor

deads2k commented Nov 5, 2024

/approve

@jpbetz
Copy link
Contributor Author

jpbetz commented Nov 5, 2024

/assign @seans3 or @soltysh would either of you be willing to approve the printer changes here?

@ardaguclu
Copy link
Member

/lgtm

@ardaguclu
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ardaguclu, deads2k, jpbetz, sttts

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 5, 2024
@k8s-ci-robot k8s-ci-robot merged commit 2d6c8a1 into kubernetes:master Nov 5, 2024
17 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.32 milestone Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api-review Categorizes an issue or PR as actively needing an API review. approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/code-generation area/dependency Issues or PRs related to dependency changes 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. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. release-note Denotes a PR that will be considered when it comes time to generate release notes. 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/etcd Categorizes an issue or PR as relevant to SIG Etcd. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Projects

Status: API review completed, 1.32
Archived in project

Development

Successfully merging this pull request may close these issues.