OCPSTRAT-2371: MutatingAdmissionPolicy e2es depend on both v1alpha1 and v1beta1 of admissionregistration.k8s.io being served in k8 1.34 #1927
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@bertinatto: This pull request references OCPSTRAT-2371 which is a valid jira issue. 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 openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughUpdated MutatingAdmissionPolicy KubeVersionRange entries in the runtime config observer: v1alpha1 range set to ">= 1.33.0 and < 1.35.0", v1beta1 set to ">= 1.34.0 and < 1.35.0". Added explanatory comments about pre-GA API handling and removed a stray blank line. No public API or type signature changes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/payload-job-with-prs periodic-ci-openshift-release-master-nightly-4.21-e2e-vsphere-ovn-techpreview openshift/kubernetes#2148 |
|
@bertinatto: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/d2dcf890-97bf-11f0-8395-2a98cc7e2228-0 |
|
/payload-job-with-prs periodic-ci-openshift-release-master-ci-4.21-e2e-aws-ovn-techpreview openshift/kubernetes#2148 |
|
@bertinatto: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/f022de10-97bf-11f0-9461-3197e061a774-0 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/operator/configobservation/apienablement/observe_runtime_config.go (1)
20-22: Correct lower bound; consider bounding v1alpha1 to avoid double-enabling post‑1.34
- Setting v1alpha1 to start at 1.32 is correct (introduced in 1.32), and v1beta1 is available in 1.34. This change aligns with upstream. (pkg.go.dev)
- If we don’t want to enable both v1alpha1 and v1beta1 on ≥1.34 clusters, tighten the v1alpha1 range to be mutually exclusive:
- {KubeVersionRange: semver.MustParseRange(">= 1.32.0"), GroupVersion: schema.GroupVersion{Group: "admissionregistration.k8s.io", Version: "v1alpha1"}}, + {KubeVersionRange: semver.MustParseRange(">= 1.32.0 < 1.34.0"), GroupVersion: schema.GroupVersion{Group: "admissionregistration.k8s.io", Version: "v1alpha1"}},If serving both versions simultaneously is desired (it’s generally fine), then the current edit is good to go. Please confirm the intent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
pkg/operator/configobservation/apienablement/observe_runtime_config.go(1 hunks)
|
/lgtm |
|
/retest-required |
bertinatto
force-pushed
the
mutatingadmissionpolicy-v1alpha1
branch
from
7018157 to
a9091c7
Compare
|
/retitle OCPSTRAT-2371: MutatingAdmissionPolicy e2es depend on both v1alpha1 and v1beta1 of admissionregistration.k8s.io being served in k8 1.34 |
openshift-ci
bot
changed the title
bertinatto
force-pushed
the
mutatingadmissionpolicy-v1alpha1
branch
from
a9091c7 to
1942ea5
Compare
|
/approve |
openshift-ci
bot
added
the
approved
bertinatto
force-pushed
the
mutatingadmissionpolicy-v1alpha1
branch
from
1942ea5 to
cd3079a
Compare
| {KubeVersionRange: semver.MustParseRange("< 1.34.0"), GroupVersion: schema.GroupVersion{Group: "admissionregistration.k8s.io", Version: "v1alpha1"}}, | ||
| {KubeVersionRange: semver.MustParseRange(">= 1.34.0"), GroupVersion: schema.GroupVersion{Group: "admissionregistration.k8s.io", Version: "v1beta1"}}, | ||
| // MutatingAdmissionPolicy was introduced as TechPreviewNoUpgrade in OCP 4.20 (k8s v1.33) with v1alpha1 API. | ||
| // OCP 4.21 (k8s v1.34) added v1beta1 API support alongside v1alpha1. |
There was a problem hiding this comment.
Please mention that the upstream E2E feature tests exercise both versions, so pre-GA we must serve both.
| {KubeVersionRange: semver.MustParseRange(">= 1.34.0"), GroupVersion: schema.GroupVersion{Group: "admissionregistration.k8s.io", Version: "v1beta1"}}, | ||
| // MutatingAdmissionPolicy was introduced as TechPreviewNoUpgrade in OCP 4.20 (k8s v1.33) with v1alpha1 API. | ||
| // OCP 4.21 (k8s v1.34) added v1beta1 API support alongside v1alpha1. | ||
| // Both APIs are expected to be removed when the feature goes GA. |
There was a problem hiding this comment.
They won't be removed right away. What we must prevent is a GA release of OpenShift that serves either of these versions. That could happen if MutatingAdmissionPolicy is added to the default featureset in openshift/api in the future as part of a pivot from (feature default-off, v1beta1 default-off) to (feature default-on, v1 default-on).
| // OCP 4.21 (k8s v1.34) added v1beta1 API support alongside v1alpha1. | ||
| // Both APIs are expected to be removed when the feature goes GA. | ||
| // GA is tentatively assumed for k8s v1.35, but the KEP does not specify a version yet: https://github.com/kubernetes/enhancements/issues/3962 | ||
| // TODO: Update these ranges when rebasing to k8s v1.35+ |
There was a problem hiding this comment.
To be specific: if we have a v1 version of the resources required for MutatingAdmissionPolicy in 1.35, then we can remove all references to MutatingAdmissionPolicy here. If we don't get v1 resources, then these ranges should be bumped to "<1.36.0" and we'll make another decision during the following rebase.
MutatingAdmissionPolicy e2es depend on both v1alpha1 and v1beta1 of admissionregistration.k8s.io being served in k8 1.34.
bertinatto
force-pushed
the
mutatingadmissionpolicy-v1alpha1
branch
from
cd3079a to
03b528d
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: benluddy, bertinatto, jacobsee 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 |
|
/retest-requird |
|
/retest-required |
25 similar comments
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
@bertinatto: The following tests failed, say
Full PR test history. Your PR dashboard. 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-sigs/prow repository. I understand the commands that are listed here. |
|
/retest-required |
openshift-merge-bot
bot
merged commit aaf8f8b
into
openshift:main
4 participants
The API started being served in v1.32.0: https://github.com/kubernetes/kubernetes/blob/release-1.34/pkg/features/kube_features.go#L1964
/assign @benluddy
CC @jacobsee