Skip to content

Conversation

@benluddy
Copy link

We missed the 4.20 removal of this GV on Hypershift in the course of https://issues.redhat.com/browse/OCPBUGS-55465:

$ kubectl get --raw /apis/admissionregistration.k8s.io/
{
  "kind": "APIGroup",
  "apiVersion": "v1",
  "name": "admissionregistration.k8s.io",
  "versions": [
    {
      "groupVersion": "admissionregistration.k8s.io/v1",
      "version": "v1"
    },
    {
      "groupVersion": "admissionregistration.k8s.io/v1beta1",
      "version": "v1beta1"
    }
  ],
  "preferredVersion": {
    "groupVersion": "admissionregistration.k8s.io/v1",
    "version": "v1"
  }
}

@benluddy benluddy requested a review from sjenning November 20, 2025 21:32
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 20, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 20, 2025

Walkthrough

Removed enforcement and runtime-config wiring for the ValidatingAdmissionPolicy feature gate from KAS config generation; tests and fixtures updated to remove admissionregistration.k8s.io/v1beta1=true from default runtime-config. StructuredAuthenticationConfiguration enforcement remains.

Changes

Cohort / File(s) Summary
KAS config logic
control-plane-operator/controllers/hostedcontrolplane/v2/kas/config.go
Removed enforcement of the ValidatingAdmissionPolicy feature gate and the conditional insertion of admissionregistration.k8s.io/v1beta1=true into apiServerArguments.runtime-config.
Unit tests
control-plane-operator/controllers/hostedcontrolplane/v2/kas/config_test.go
Removed/updated tests asserting wiring for ValidatingAdmissionPolicy; expectations updated so default runtime-config no longer contains admissionregistration.k8s.io/v1beta1.
Fixtures — kas config ConfigMaps
control-plane-operator/controllers/hostedcontrolplane/testdata/kube-apiserver/.../zz_fixture_TestControlPlaneComponents_kas_config_configmap.yaml
Updated embedded config.json to change apiServerArguments.runtime-config from ["admissionregistration.k8s.io/v1beta1=true"] to [].
Fixtures — kube-apiserver Deployments (annotation updates)
control-plane-operator/controllers/hostedcontrolplane/testdata/kube-apiserver/.../zz_fixture_TestControlPlaneComponents_kube_apiserver_deployment.yaml
Updated component.hypershift.openshift.io/config-hash annotation values in multiple test fixtures; changes are metadata-only (hash values).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Check config.go for any callers or integrations that relied on the removed runtime-config entry.
  • Verify config_test.go still fully exercises StructuredAuthenticationConfiguration behavior.
  • Confirm fixture JSON and YAML edits are syntactically correct and that deployment annotation hash changes are intentional.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch stop-serving-admissionregistration-v1beta1

📜 Recent 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

📥 Commits

Reviewing files that changed from the base of the PR and between e0b5c8e and c616f5a.

📒 Files selected for processing (8)
  • control-plane-operator/controllers/hostedcontrolplane/testdata/kube-apiserver/IBMCloud/zz_fixture_TestControlPlaneComponents_kas_config_configmap.yaml (1 hunks)
  • control-plane-operator/controllers/hostedcontrolplane/testdata/kube-apiserver/IBMCloud/zz_fixture_TestControlPlaneComponents_kube_apiserver_deployment.yaml (1 hunks)
  • control-plane-operator/controllers/hostedcontrolplane/testdata/kube-apiserver/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_kas_config_configmap.yaml (1 hunks)
  • control-plane-operator/controllers/hostedcontrolplane/testdata/kube-apiserver/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_kube_apiserver_deployment.yaml (1 hunks)
  • control-plane-operator/controllers/hostedcontrolplane/testdata/kube-apiserver/zz_fixture_TestControlPlaneComponents_kas_config_configmap.yaml (1 hunks)
  • control-plane-operator/controllers/hostedcontrolplane/testdata/kube-apiserver/zz_fixture_TestControlPlaneComponents_kube_apiserver_deployment.yaml (1 hunks)
  • control-plane-operator/controllers/hostedcontrolplane/v2/kas/config.go (1 hunks)
  • control-plane-operator/controllers/hostedcontrolplane/v2/kas/config_test.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
  • control-plane-operator/controllers/hostedcontrolplane/v2/kas/config.go
  • control-plane-operator/controllers/hostedcontrolplane/testdata/kube-apiserver/zz_fixture_TestControlPlaneComponents_kube_apiserver_deployment.yaml
  • control-plane-operator/controllers/hostedcontrolplane/testdata/kube-apiserver/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_kube_apiserver_deployment.yaml
  • control-plane-operator/controllers/hostedcontrolplane/testdata/kube-apiserver/zz_fixture_TestControlPlaneComponents_kas_config_configmap.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**

⚙️ CodeRabbit configuration file

-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.

Files:

  • control-plane-operator/controllers/hostedcontrolplane/testdata/kube-apiserver/IBMCloud/zz_fixture_TestControlPlaneComponents_kube_apiserver_deployment.yaml
  • control-plane-operator/controllers/hostedcontrolplane/testdata/kube-apiserver/IBMCloud/zz_fixture_TestControlPlaneComponents_kas_config_configmap.yaml
  • control-plane-operator/controllers/hostedcontrolplane/testdata/kube-apiserver/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_kas_config_configmap.yaml
  • control-plane-operator/controllers/hostedcontrolplane/v2/kas/config_test.go
🔇 Additional comments (4)
control-plane-operator/controllers/hostedcontrolplane/testdata/kube-apiserver/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_kas_config_configmap.yaml (1)

3-3: TechPreview kas-config fixture now correctly drops v1beta1 runtime-config

The embedded config.json shows apiServerArguments.runtime-config: [], which is consistent with no longer serving admissionregistration.k8s.io/v1beta1 while leaving the rest of the KAS configuration unchanged. Looks good.

control-plane-operator/controllers/hostedcontrolplane/testdata/kube-apiserver/IBMCloud/zz_fixture_TestControlPlaneComponents_kube_apiserver_deployment.yaml (1)

32-32: IBMCloud kube-apiserver Deployment hash update is consistent with config change

The component.hypershift.openshift.io/config-hash annotation change reflects the updated kas-config contents; no other Deployment fields are modified, so this fixture update is appropriate.

control-plane-operator/controllers/hostedcontrolplane/v2/kas/config_test.go (1)

765-765: Default KAS config runtime-config baseline matches v1beta1 removal

Setting APIServerArguments["runtime-config"] to an empty kcpv1.Arguments{} establishes a clean baseline with no admissionregistration.k8s.io/v1beta1 entry; feature-specific tests (e.g., DynamicResourceAllocation, MutatingAdmissionPolicy) still verify adding their own runtime-config entries. This aligns with the PR’s goal and keeps tests accurate.

control-plane-operator/controllers/hostedcontrolplane/testdata/kube-apiserver/IBMCloud/zz_fixture_TestControlPlaneComponents_kas_config_configmap.yaml (1)

3-3: IBMCloud kas-config fixture correctly reflects empty runtime-config

The embedded config.json now has apiServerArguments.runtime-config: [], matching the new default behavior and ensuring v1beta1 for admissionregistration is no longer served in this profile. No other fields are altered.

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.5.0)

Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions
The command is terminated due to an error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions


Comment @coderabbitai help to get the list of available commands and usage tips.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 20, 2025

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

@openshift-ci openshift-ci bot added do-not-merge/needs-area area/control-plane-operator Indicates the PR includes changes for the control plane operator - in an OCP release and removed do-not-merge/needs-area labels Nov 20, 2025
@csrwng
Copy link
Contributor

csrwng commented Nov 21, 2025

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 21, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: benluddy, csrwng

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 21, 2025
@benluddy benluddy marked this pull request as ready for review November 21, 2025 16:58
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 21, 2025
@openshift-ci openshift-ci bot requested review from csrwng and devguyio November 21, 2025 16:59
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 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

📥 Commits

Reviewing files that changed from the base of the PR and between dea6d2f and 605745b.

📒 Files selected for processing (1)
  • control-plane-operator/controllers/hostedcontrolplane/v2/kas/config.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**

⚙️ CodeRabbit configuration file

-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.

Files:

  • control-plane-operator/controllers/hostedcontrolplane/v2/kas/config.go
🔇 Additional comments (2)
control-plane-operator/controllers/hostedcontrolplane/v2/kas/config.go (2)

237-248: LGTM - Runtime config correctly updated.

The removal of the conditional block that added admissionregistration.k8s.io/v1beta1=true to runtime-config (based on the ValidatingAdmissionPolicy feature gate) correctly achieves the PR objective of stopping serving the v1beta1 API. The remaining conditional blocks follow the same pattern for other features.


334-334: Confirm that keeping ValidatingAdmissionPolicy admission plugin enabled is intentional.

The ValidatingAdmissionPolicy admission plugin remains in the enabled list even though the feature gate is no longer enforced and the v1beta1 API is not being served. This appears correct since the plugin can work with the v1 API, but please confirm this is the intended behavior.

@benluddy benluddy force-pushed the stop-serving-admissionregistration-v1beta1 branch 2 times, most recently from cded0f9 to e0b5c8e Compare November 21, 2025 17:26
4.17 went GA with this group/version inadvertently served due to a mistake in the pivot of
ValidatingAdmissionPolicy from tech preview to default. We retroactively marked it as deprecated and
phased it out with removal occurring in standalone for 4.20.
@benluddy benluddy force-pushed the stop-serving-admissionregistration-v1beta1 branch from e0b5c8e to c616f5a Compare November 21, 2025 18:18
@bertinatto
Copy link
Member

/retest

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 24, 2025

@benluddy: 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
ci/prow/unit c616f5a link true /test unit
ci/prow/e2e-aks c616f5a link true /test e2e-aks
ci/prow/e2e-aws c616f5a link true /test e2e-aws

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.

@bryan-cox
Copy link
Member

@benluddy looks like UPDATE=true go test ./... needs run and the changes from that committed to pass unit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/control-plane-operator Indicates the PR includes changes for the control plane operator - in an OCP release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants