Skip to content

Conversation

@danwinship
Copy link
Contributor

@danwinship danwinship commented Dec 13, 2024

The ServiceCIDR API is beta (and thus available by default) in k8s 1.31, but it will not work correctly in OCP, because (among other things) we do not support dynamically updating ovn-kubernetes with changes to the service CIDRs. Thus, we need to block admins from using this API (while not interfering with kube-apiserver's own use of the API).

EDIT: the API was beta in 1.31, but not available by default. It doesn't become available by default until 1.33.

The ValidatingAdmissionPolicy here is identical to the sample one from the documentation beyond having a custom error message. (Note that, confusingly, validations.expression gives the condition describing when a change is allowed, while validations.messages gives the message printed when a change is denied.)

(The reason that the policy allows changes to the default ("kubernetes") ServiceCIDR is to allow single-stack to dual-stack migration; the apiserver itself will modify the default ServiceCIDR in that case when it is reconfigured.)

@openshift-ci-robot openshift-ci-robot added jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Dec 13, 2024
@openshift-ci-robot
Copy link
Contributor

@danwinship: This pull request references Jira Issue OCPBUGS-46422, which is invalid:

  • expected the bug to target either version "4.19." or "openshift-4.19.", but it targets "4.18.0" instead

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

The ServiceCIDR API is beta (and thus available by default) in k8s 1.31, but it will not work correctly in OCP, because (among other things) we do not support dynamically updating ovn-kubernetes with changes to the service CIDRs. Thus, we need to block admins from using this API (while not interfering with kube-apiserver's own use of the API).

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.

@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 Dec 13, 2024
@danwinship
Copy link
Contributor Author

/jira refresh

@openshift-ci-robot openshift-ci-robot added jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. and removed jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Dec 13, 2024
@openshift-ci-robot
Copy link
Contributor

@danwinship: This pull request references Jira Issue OCPBUGS-46422, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target version (4.19.0) matches configured target version for branch (4.19.0)
  • bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @anuragthehatter

Details

In response to this:

/jira refresh

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.

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 13, 2024
@danwinship danwinship force-pushed the servicecidr-admission branch from b62995a to 7e3972b Compare December 14, 2024 13:18
@danwinship danwinship marked this pull request as draft December 15, 2024 02:04
@danwinship danwinship changed the title WIP: OCPBUGS-46422: Add a ValidatingAdmissionPolicy blocking ServiceCIDR changes OCPBUGS-46422: Add a ValidatingAdmissionPolicy blocking ServiceCIDR changes Dec 15, 2024
@openshift-ci-robot
Copy link
Contributor

@danwinship: This pull request references Jira Issue OCPBUGS-46422, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target version (4.19.0) matches configured target version for branch (4.19.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @anuragthehatter

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

The ServiceCIDR API is beta (and thus available by default) in k8s 1.31, but it will not work correctly in OCP, because (among other things) we do not support dynamically updating ovn-kubernetes with changes to the service CIDRs. Thus, we need to block admins from using this API (while not interfering with kube-apiserver's own use of the API).

EDIT: the API was beta in 1.31, but not available by default. It doesn't become available by default until 1.33.

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.

@danwinship danwinship marked this pull request as ready for review July 16, 2025 20:32
@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 Jul 16, 2025
@openshift-ci openshift-ci bot requested review from jcaamano and kyrtapz July 16, 2025 20:33
@danwinship danwinship marked this pull request as draft July 17, 2025 14:06
@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 Jul 17, 2025
@danwinship danwinship force-pushed the servicecidr-admission branch from 7e3972b to d0c8955 Compare July 17, 2025 14:06
@danwinship danwinship marked this pull request as ready for review July 28, 2025 14:41
@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 Jul 28, 2025
@openshift-ci-robot openshift-ci-robot added jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. and removed jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. labels Jul 28, 2025
@openshift-ci-robot
Copy link
Contributor

@danwinship: This pull request references Jira Issue OCPBUGS-46422, which is invalid:

  • expected the bug to target the "4.20.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

The ServiceCIDR API is beta (and thus available by default) in k8s 1.31, but it will not work correctly in OCP, because (among other things) we do not support dynamically updating ovn-kubernetes with changes to the service CIDRs. Thus, we need to block admins from using this API (while not interfering with kube-apiserver's own use of the API).

EDIT: the API was beta in 1.31, but not available by default. It doesn't become available by default until 1.33.

The ValidatingAdmissionPolicy here is identical to the sample one from the documentation beyond having a custom error message. (Note that, confusingly, validations.expression gives the condition describing when a change is allowed, while validations.messages gives the message printed when a change is denied.)

(The reason that the policy allows changes to the default ("kubernetes") ServiceCIDR is to allow single-stack to dual-stack migration; the apiserver itself will modify the default ServiceCIDR in that case when it is reconfigured.)

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.

@danwinship
Copy link
Contributor Author

/retest-required

@kyrtapz
Copy link
Contributor

kyrtapz commented Jul 29, 2025

(The reason that the policy allows changes to the default ("kubernetes") ServiceCIDR is to allow single-stack to dual-stack migration; the apiserver itself will modify the default ServiceCIDR in that case when it is reconfigured.)

Do you think we could also check the user? To ensure only the apiserver can change the default ServiceCIDR.

OCP does not yet support changing the service CIDRs at runtime.
@danwinship danwinship force-pushed the servicecidr-admission branch from f793f60 to a795c90 Compare September 5, 2025 13:20
@danwinship
Copy link
Contributor Author

/hold cancel

it might take a bit to propagate through, but openshift/kubernetes#2444 fixes the conformance test failure, so once e2e passes, this can merge

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 5, 2025
@danwinship
Copy link
Contributor Author

/test e2e-gcp-ovn

@danwinship
Copy link
Contributor Author

/retest-required

2 similar comments
@danwinship
Copy link
Contributor Author

/retest-required

@danwinship
Copy link
Contributor Author

/retest-required

@danwinship
Copy link
Contributor Author

/override ci/prow/e2e-aws-ovn-upgrade
which is nearly permafailing and not affected by this PR anyway

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 10, 2025

@danwinship: Overrode contexts on behalf of danwinship: ci/prow/e2e-aws-ovn-upgrade

Details

In response to this:

/override ci/prow/e2e-aws-ovn-upgrade
which is nearly permafailing and not affected by this PR anyway

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.

@danwinship
Copy link
Contributor Author

/verified by "ServiceCIDR should be blocked"

(new test from openshift/origin#30234, which can merge after this does. Test run at https://prow.ci.openshift.org/view/gs/test-platform-results/logs/release-openshift-origin-installer-launch-aws-modern/1965905253861691392.)

@openshift-ci-robot openshift-ci-robot added the verified Signifies that the PR passed pre-merge verification criteria label Sep 11, 2025
@openshift-ci-robot
Copy link
Contributor

@danwinship: This PR has been marked as verified by "ServiceCIDR should be blocked".

Details

In response to this:

/verified by "ServiceCIDR should be blocked"

(new test from openshift/origin#30234, which can merge after this does. Test run at https://prow.ci.openshift.org/view/gs/test-platform-results/logs/release-openshift-origin-installer-launch-aws-modern/1965905253861691392.)

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.

@kyrtapz
Copy link
Contributor

kyrtapz commented Sep 11, 2025

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 11, 2025
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 11, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danwinship, kyrtapz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 11ee8f9 and 2 for PR HEAD a795c90 in total

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 11, 2025

@danwinship: 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/4.18-upgrade-from-stable-4.17-e2e-aws-ovn-upgrade 7e3972b link false /test 4.18-upgrade-from-stable-4.17-e2e-aws-ovn-upgrade
ci/prow/4.18-upgrade-from-stable-4.17-e2e-gcp-ovn-upgrade 7e3972b link false /test 4.18-upgrade-from-stable-4.17-e2e-gcp-ovn-upgrade
ci/prow/4.18-upgrade-from-stable-4.17-e2e-azure-ovn-upgrade 7e3972b link false /test 4.18-upgrade-from-stable-4.17-e2e-azure-ovn-upgrade
ci/prow/e2e-gcp-ovn-techpreview 7e3972b link true /test e2e-gcp-ovn-techpreview
ci/prow/e2e-vsphere-ovn-dualstack-primaryv6 d0c8955 link false /test e2e-vsphere-ovn-dualstack-primaryv6
ci/prow/e2e-vsphere-ovn-dualstack a795c90 link false /test e2e-vsphere-ovn-dualstack
ci/prow/e2e-aws-ovn-local-to-shared-gateway-mode-migration a795c90 link false /test e2e-aws-ovn-local-to-shared-gateway-mode-migration
ci/prow/4.20-upgrade-from-stable-4.19-e2e-gcp-ovn-upgrade a795c90 link false /test 4.20-upgrade-from-stable-4.19-e2e-gcp-ovn-upgrade
ci/prow/e2e-openstack-ovn a795c90 link false /test e2e-openstack-ovn
ci/prow/security a795c90 link false /test security
ci/prow/okd-scos-e2e-aws-ovn a795c90 link false /test okd-scos-e2e-aws-ovn
ci/prow/e2e-ovn-step-registry a795c90 link false /test e2e-ovn-step-registry
ci/prow/4.20-upgrade-from-stable-4.19-e2e-aws-ovn-upgrade a795c90 link false /test 4.20-upgrade-from-stable-4.19-e2e-aws-ovn-upgrade
ci/prow/e2e-network-mtu-migration-ovn-ipv4 a795c90 link false /test e2e-network-mtu-migration-ovn-ipv4
ci/prow/e2e-aws-ovn-single-node a795c90 link false /test e2e-aws-ovn-single-node
ci/prow/4.20-upgrade-from-stable-4.19-e2e-azure-ovn-upgrade a795c90 link false /test 4.20-upgrade-from-stable-4.19-e2e-azure-ovn-upgrade
ci/prow/e2e-azure-ovn a795c90 link false /test e2e-azure-ovn
ci/prow/e2e-aws-ovn-shared-to-local-gateway-mode-migration a795c90 link false /test e2e-aws-ovn-shared-to-local-gateway-mode-migration
ci/prow/e2e-vsphere-ovn a795c90 link false /test e2e-vsphere-ovn
ci/prow/e2e-ovn-hybrid-step-registry a795c90 link false /test e2e-ovn-hybrid-step-registry
ci/prow/e2e-aws-hypershift-ovn-kubevirt a795c90 link false /test e2e-aws-hypershift-ovn-kubevirt
ci/prow/e2e-aws-ovn-serial a795c90 link false /test e2e-aws-ovn-serial
ci/prow/e2e-network-mtu-migration-ovn-ipv6 a795c90 link false /test e2e-network-mtu-migration-ovn-ipv6

Full PR test history. Your PR dashboard.

Details

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.

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 11ee8f9 and 2 for PR HEAD a795c90 in total

@openshift-merge-bot openshift-merge-bot bot merged commit 3dcc3fe into openshift:master Sep 11, 2025
22 of 40 checks passed
@openshift-ci-robot
Copy link
Contributor

@danwinship: Jira Issue OCPBUGS-61196: Some pull requests linked via external trackers have merged:

The following pull request, linked via external tracker, has not merged:

All associated pull requests must be merged or unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with /jira refresh.

Jira Issue OCPBUGS-61196 has not been moved to the MODIFIED state.

This PR is marked as verified. If the remaining PRs listed above are marked as verified before merging, the issue will automatically be moved to VERIFIED after all of the changes from the PRs are available in an accepted nightly payload.

Details

In response to this:

The ServiceCIDR API is beta (and thus available by default) in k8s 1.31, but it will not work correctly in OCP, because (among other things) we do not support dynamically updating ovn-kubernetes with changes to the service CIDRs. Thus, we need to block admins from using this API (while not interfering with kube-apiserver's own use of the API).

EDIT: the API was beta in 1.31, but not available by default. It doesn't become available by default until 1.33.

The ValidatingAdmissionPolicy here is identical to the sample one from the documentation beyond having a custom error message. (Note that, confusingly, validations.expression gives the condition describing when a change is allowed, while validations.messages gives the message printed when a change is denied.)

(The reason that the policy allows changes to the default ("kubernetes") ServiceCIDR is to allow single-stack to dual-stack migration; the apiserver itself will modify the default ServiceCIDR in that case when it is reconfigured.)

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.

@openshift-merge-robot
Copy link
Contributor

Fix included in accepted release 4.21.0-0.nightly-2025-10-02-215712

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. jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. verified Signifies that the PR passed pre-merge verification criteria

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants