OCPBUGS-84303: fix(api): add IPv6 OVN join subnet config to prevent dual-stack routing collision#8421
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/severity-important
|
@orenc1: This pull request references Jira Issue OCPBUGS-84303, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds IPv6 support for OVN-Kubernetes: a new constant Sequence Diagram(s)sequenceDiagram
participant HCP as HostedControlPlane
participant Detector as hasIPv6Network
participant Reconciler as ReconcileNetworkOperator
participant OVNConfig as OVN Config
participant DefaultNet as DefaultNetwork.OVNKubernetesConfig
participant Validator as validateSliceNetworkCIDRs
HCP->>Detector: provide HCP.Spec.Networking
Detector-->>Reconciler: hasIPv6Network (true/false)
HCP->>Reconciler: submit HostedCluster + platform + ovnConfig
Reconciler->>Reconciler: check platform == KubeVirt && network == OVNKubernetes
alt hasIPv6Network = true and IPv6 not set
Reconciler->>DefaultNet: set IPv6.InternalJoinSubnet = KubevirtDefaultV6InternalJoinSubnet
end
OVNConfig->>Reconciler: provide ovnConfig.IPv6.* (if present)
Reconciler->>DefaultNet: copy OVN IPv4/IPv6 fields into operator spec
DefaultNet->>Validator: include OVN IPv6 internal subnets in overlap checks
Validator-->>DefaultNet: validation result
DefaultNet-->>Reconciler: apply or reject network configuration
🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/api
openshift-ci-robot
added
jira/valid-bug
|
@orenc1: This pull request references Jira Issue OCPBUGS-84303, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
DetailsIn 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. |
orenc1
force-pushed
the
fix_dual_stack__ovn_joinsubnet_collision
branch
from
43beb31 to
b261379
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go`:
- Around line 4338-4357: The code only appends user-provided OVNKubernetes IPv6
CIDRs to cidrEntries; change it to also include the effective KubeVirt default
join subnet (fd99::/64) when the OVNKubernetesConfig IPv6 is nil or when
IPv6.InternalJoinSubnet is empty: inside the block that checks
hc.Spec.Networking.NetworkType == hyperv1.OVNKubernetes (and related
OperatorConfiguration presence), detect when ovnIPv6Config is nil or
ovnIPv6Config.InternalJoinSubnet == "" and create a cidrEntry for fd99::/64
(using the same cidrEntry type and field.NewPath pointing at
"spec","operatorConfiguration","clusterNetworkOperator","ovnKubernetesConfig","ipv6","internalJoinSubnet")
and append it to cidrEntries so validateNetworks sees the effective default;
keep existing user-specified parsing logic for non-empty InternalJoinSubnet and
InternalTransitSwitchSubnet as-is.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 5b9e4b77-5cab-4687-becb-c627b18e3dad
⛔ Files ignored due to path filters (40)
api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*.go,!**/zz_generated*api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HCPEtcdBackup.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ImageStreamImportMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/TLSAdherence.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDC.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/HCPEtcdBackup.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ImageStreamImportMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/TLSAdherence.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**client/applyconfiguration/hypershift/v1beta1/ovnipv6config.gois excluded by!client/**client/applyconfiguration/hypershift/v1beta1/ovnkubernetesconfig.gois excluded by!client/**client/applyconfiguration/utils.gois excluded by!client/**cmd/install/assets/crds/hypershift-operator/tests/hostedclusters.hypershift.openshift.io/stable.hostedclusters.networking.testsuite.yamlis excluded by!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamldocs/content/reference/api.mdis excluded by!docs/content/reference/api.mdvendor/github.com/openshift/hypershift/api/hypershift/v1beta1/operator.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*.go,!**/zz_generated*
📒 Files selected for processing (5)
api/hypershift/v1beta1/operator.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/network/reconcile.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/network/reconcile_test.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go
|
@orenc1: This pull request references Jira Issue OCPBUGS-84303, which is valid. 3 validation(s) were run on this bug
DetailsIn 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. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8421 +/- ##
==========================================
+ Coverage 40.00% 40.12% +0.12%
==========================================
Files 751 753 +2
Lines 92838 93050 +212
==========================================
+ Hits 37137 37338 +201
+ Misses 53014 53012 -2
- Partials 2687 2700 +13
... and 6 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
orenc1
force-pushed
the
fix_dual_stack__ovn_joinsubnet_collision
branch
from
b261379 to
8d5be28
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
control-plane-operator/hostedclusterconfigoperator/controllers/resources/network/reconcile.go (1)
93-122:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReconcile deleted IPv6 overrides, not just additions.
This path only copies non-empty source fields. If a user later removes
ipv6.internalJoinSubnet,ipv6.internalTransitSwitchSubnet, or the wholeovnKubernetesConfig, the previous values stay on theoperatorv1.Networkobject, so the cluster cannot roll back to the KubeVirt default or to the platform default behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@control-plane-operator/hostedclusterconfigoperator/controllers/resources/network/reconcile.go` around lines 93 - 122, When reconciling OVNKubernetes config you must also remove overrides when the source is deleted: update the block handling networkType == hyperv1.OVNKubernetes so that if ovnConfig is nil you clear network.Spec.DefaultNetwork.OVNKubernetesConfig (set to nil); if ovnConfig != nil but ovnConfig.IPv6 is nil then set ovnCfg.IPv6 = nil; and if ovnConfig.IPv6 exists then copy IPv6.InternalJoinSubnet and IPv6.InternalTransitSwitchSubnet when non-empty but explicitly clear those ovnCfg.IPv6 fields (or set ovnCfg.IPv6 = nil if both are empty) when the source strings are empty—this ensures previous values are removed; use the existing symbols ovnConfig, ovnCfg, network.Spec.DefaultNetwork.OVNKubernetesConfig, and the IPv6.InternalJoinSubnet / IPv6.InternalTransitSwitchSubnet fields to locate and implement the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@control-plane-operator/hostedclusterconfigoperator/controllers/resources/network/reconcile.go`:
- Around line 93-122: When reconciling OVNKubernetes config you must also remove
overrides when the source is deleted: update the block handling networkType ==
hyperv1.OVNKubernetes so that if ovnConfig is nil you clear
network.Spec.DefaultNetwork.OVNKubernetesConfig (set to nil); if ovnConfig !=
nil but ovnConfig.IPv6 is nil then set ovnCfg.IPv6 = nil; and if ovnConfig.IPv6
exists then copy IPv6.InternalJoinSubnet and IPv6.InternalTransitSwitchSubnet
when non-empty but explicitly clear those ovnCfg.IPv6 fields (or set ovnCfg.IPv6
= nil if both are empty) when the source strings are empty—this ensures previous
values are removed; use the existing symbols ovnConfig, ovnCfg,
network.Spec.DefaultNetwork.OVNKubernetesConfig, and the IPv6.InternalJoinSubnet
/ IPv6.InternalTransitSwitchSubnet fields to locate and implement the changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: a71e1352-933e-4833-aab8-03e61a0455f6
⛔ Files ignored due to path filters (41)
api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*.go,!**/zz_generated*api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HCPEtcdBackup.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ImageStreamImportMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/TLSAdherence.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDC.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/HCPEtcdBackup.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ImageStreamImportMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/TLSAdherence.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**client/applyconfiguration/hypershift/v1beta1/ovnipv6config.gois excluded by!client/**client/applyconfiguration/hypershift/v1beta1/ovnkubernetesconfig.gois excluded by!client/**client/applyconfiguration/utils.gois excluded by!client/**cmd/install/assets/crds/hypershift-operator/tests/hostedclusters.hypershift.openshift.io/stable.hostedclusters.networking.testsuite.yamlis excluded by!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamldocs/content/reference/aggregated-docs.mdis excluded by!docs/content/reference/aggregated-docs.mddocs/content/reference/api.mdis excluded by!docs/content/reference/api.mdvendor/github.com/openshift/hypershift/api/hypershift/v1beta1/operator.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*.go,!**/zz_generated*
📒 Files selected for processing (5)
api/hypershift/v1beta1/operator.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/network/reconcile.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/network/reconcile_test.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go
✅ Files skipped from review due to trivial changes (1)
- control-plane-operator/hostedclusterconfigoperator/controllers/resources/network/reconcile_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go
This is a pre-existing pattern — the IPv4 block uses the exact same additive-only approach (only copies non-empty values, never clears). The MTU field follows the same convention as well. Changing this behavior would alter semantics for all OVN config fields (IPv4, IPv6, MTU), which is out of scope for this bug fix. Additionally, OVN internal join/transit subnets are effectively immutable after cluster creation — changing them at runtime would break OVN networking. The additive-only reconciliation pattern is appropriate here. |
…ual-stack routing collision Cherry-pick of openshift#8421 to release-4.21. On KubeVirt dual-stack hosted clusters, the guest OVN-Kubernetes cluster shares the same default IPv6 join subnet (fd98::/64) as the management cluster. When external IPv6 LoadBalancer traffic is SNAT'd to a join switch IP, the guest cluster intercepts the response because both clusters own the same fd98::/64 range, causing a routing black hole. This fix: - Defaults the guest cluster's IPv6 OVN join subnet to fd99::/64 for KubeVirt hosted clusters, avoiding the collision automatically - Adds OVNIPv6Config API type allowing users to explicitly configure IPv6 internalJoinSubnet and internalTransitSwitchSubnet - Extends CIDR overlap validation to cover IPv6 OVN subnets including the implicit KubeVirt default (fd99::/64) - Adds unit tests for all new IPv6 validation and reconciliation logic Signed-off-by: Oren Cohen <ocohen@redhat.com> Assisted-by: Claude Opus 4 (via Cursor) Co-authored-by: Cursor <cursoragent@cursor.com>
|
The fix is verified. with a different, non-conflicting OVN join IPv6 subnet for the hosted cluster, LB service is accessible: Verification: IPv6 LoadBalancer Fix (OVN Join Subnet Collision)Environment
Setup
Results
Key ConfirmationThe guest cluster's OVN network operator config shows the fix is active: {
"ipv6": {
"internalJoinSubnet": "fd99::/64"
}
}/verified by @orenc1 |
openshift-ci-robot
added
the
verified
|
@orenc1: This PR has been marked as verified by DetailsIn 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. |
orenc1
force-pushed
the
fix_dual_stack__ovn_joinsubnet_collision
branch
from
b9610c9 to
71baf0a
Compare
orenc1
force-pushed
the
fix_dual_stack__ovn_joinsubnet_collision
branch
from
71baf0a to
4545f1e
Compare
…ual-stack routing collision When a KubeVirt hosted cluster and its management cluster both use OVN-Kubernetes with dual-stack networking, they each default to fd98::/64 for the IPv6 join switch subnet. External IPv6 LoadBalancer traffic targeting VM pods is SNAT'd to the management cluster's join IP (e.g. fd98::2). Inside the VM, the guest cluster's OVN intercepts the response because it also owns fd98::/64, black-holing the packet. This commit fixes the issue in two ways: 1. Automatic KubeVirt default: for KubeVirt hosted clusters with OVNKubernetes and IPv6 networks, the reconciler now sets IPv6.InternalJoinSubnet to fd99::/64 by default, avoiding the collision with the management cluster's fd98::/64. This mirrors the existing V4InternalSubnet override (100.66.0.0/16) already in place for IPv4. 2. User-facing API: adds OVNIPv6Config type to OVNKubernetesConfig, allowing explicit configuration of IPv6 internalJoinSubnet and internalTransitSwitchSubnet for any platform. This maps to the upstream operatorv1.IPv6OVNKubernetesConfig and includes IPv6 CIDR format validation via CEL isCIDR() rules. Also extends CIDR overlap validation in the HostedCluster webhook to cover IPv6 OVN subnets and the KubeVirt IPv4 default (100.66.0.0/16). Backport of openshift#8421 Signed-off-by: Oren Cohen <ocohen@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
API structure looks good to me overall. Just a handful of comments.
| // +kubebuilder:validation:XValidation:rule="isCIDR(self) && cidr(self).ip().family() == 6", message="Subnet must be in valid IPv6 CIDR format (e.g., fd97::/64)" | ||
| // +kubebuilder:validation:XValidation:rule="isCIDR(self) && cidr(self).prefixLength() <= 125", message="subnet must be in the range /0 to /125 inclusive" |
There was a problem hiding this comment.
General note for any other reviewers - the CIDR CEL library was introduced in Kubernetes 1.30. From what I recall being told, we are look for N-4 compatibility from the kube version skew, so with 4.22 being based on 1.35 that means the oldest kube version we would end up running on would be 1.31. This should be OK.
| // +kubebuilder:validation:XValidation:rule="self.matches('^\\\\s*((([0-9A-Fa-f]{1,4}:){7}([0-9A-Fa-f]{1,4}|:))|(([0-9A-Fa-f]{1,4}:){6}(:[0-9A-Fa-f]{1,4}|:))|(([0-9A-Fa-f]{1,4}:){5}((:[0-9A-Fa-f]{1,4}){1,2}|:))|(([0-9A-Fa-f]{1,4}:){4}((:[0-9A-Fa-f]{1,4}){1,3}|:))|(([0-9A-Fa-f]{1,4}:){3}((:[0-9A-Fa-f]{1,4}){1,4}|:))|(([0-9A-Fa-f]{1,4}:){2}((:[0-9A-Fa-f]{1,4}){1,5}|:))|(([0-9A-Fa-f]{1,4}:){1}((:[0-9A-Fa-f]{1,4}){1,6}|:))|(::((:[0-9A-Fa-f]{1,4}){1,7}|:)))\\\\s*/([0-9]|[1-9][0-9]|1[0-1][0-9]|12[0-8])$')", message="Subnet must be in valid IPv6 CIDR format (e.g., fd98::/64)" | ||
| // +kubebuilder:validation:XValidation:rule="self.matches('^.*/[0-9]+$') && int(self.split('/')[1]) <= 125", message="subnet must be in the range /0 to /125 inclusive" | ||
| // +optional | ||
| InternalJoinSubnet string `json:"internalJoinSubnet,omitempty"` |
There was a problem hiding this comment.
If we know ahead of time that making a change to the networking configuration on a running cluster would be bad, let's go ahead and prevent it where we can now.
Just a note that immutability rules must be done on the closest parent field that is required, otherwise the field isn't actually immutable because it can be removed by removing an optional parent (or the field itself) in the chain and re-applying with a different value.
| route: {} | ||
| expectedError: "ipv6 internalJoinSubnet and internalTransitSwitchSubnet must not be the same" | ||
|
|
||
| - name: When ovnKubernetesConfig ipv6 internalJoinSubnet has invalid CIDR it should fail |
There was a problem hiding this comment.
Replicate this test for internalTrasitSwitchSubnet?
There was a problem hiding this comment.
envtest case has been added, expecting valid IPv6 CIDR format
| route: {} | ||
| expectedError: "Subnet must be in valid IPv6 CIDR format" | ||
|
|
||
| - name: When ovnKubernetesConfig ipv6 internalJoinSubnet has prefix length greater than 125 it should fail |
There was a problem hiding this comment.
Replicate this test for internalTrasitSwitchSubnet?
There was a problem hiding this comment.
envtest case has been added, expecting range of /0 to /125 inclusive
orenc1
force-pushed
the
fix_dual_stack__ovn_joinsubnet_collision
branch
from
4545f1e to
befec34
Compare
orenc1
force-pushed
the
fix_dual_stack__ovn_joinsubnet_collision
branch
from
befec34 to
df51feb
Compare
| // +kubebuilder:validation:XValidation:rule="!has(self.ipv6) || !has(self.ipv6.internalJoinSubnet) || !has(self.ipv6.internalTransitSwitchSubnet) || self.ipv6.internalJoinSubnet != self.ipv6.internalTransitSwitchSubnet", message="ipv6 internalJoinSubnet and internalTransitSwitchSubnet must not be the same" | ||
| // +kubebuilder:validation:XValidation:rule="!has(oldSelf.mtu) || has(self.mtu)",message="mtu is immutable once set and cannot be removed" | ||
| // +kubebuilder:validation:XValidation:rule="!has(oldSelf.ipv6) || has(self.ipv6)", message="ipv6 is immutable once set and cannot be removed" | ||
| // +kubebuilder:validation:XValidation:rule="!has(oldSelf.ipv6) || !has(oldSelf.ipv6.internalJoinSubnet) || (has(self.ipv6) && has(self.ipv6.internalJoinSubnet))", message="ipv6.internalJoinSubnet cannot be removed once set" | ||
| // +kubebuilder:validation:XValidation:rule="!has(oldSelf.ipv6) || !has(oldSelf.ipv6.internalTransitSwitchSubnet) || (has(self.ipv6) && has(self.ipv6.internalTransitSwitchSubnet))", message="ipv6.internalTransitSwitchSubnet cannot be removed once set" |
There was a problem hiding this comment.
For these immutability rules, what happens if the parent ovnKubernetesConfig field is set, removed, and re-applied with new values?
There was a problem hiding this comment.
you're right, good catch. if we remove the entire ovnKubernetesConfig field, then re-apply with different values, the immutability rules could be bypassed.
to mitigate that, i'm adding a validation rule of "cannot be removed once set" on ClusterNetworkOperatorSpec for ovnKubernetesConfig.
There was a problem hiding this comment.
It still looks like you could just remove the ClusterNetworkOperatorSpec field based on:
hypershift/api/hypershift/v1beta1/hostedcluster_types.go
Lines 2372 to 2375 in f76be88
If you want true immutability you need to set the rule all the way up to the first required field, which looks like it means on the root HostedCluster type because even spec is optional:
hypershift/api/hypershift/v1beta1/hostedcluster_types.go
Lines 2411 to 2413 in f76be88
I'd make sure you pay particular attention to how immutability applies as you go up the chain of fields to make sure you don't make something that is OK to mutate immutable.
There was a problem hiding this comment.
I see. how do you suggest to proceed? should I modify the validation rules of the higher fields in the chain, up to the root? I feel it's beyond the scope of this PR.
There was a problem hiding this comment.
I don't have a strong preference. If this significantly increases the scope of this work to achieve this, I'm OK with best effort immutability for now with a more concrete immutability story for the near-ish future.
orenc1
force-pushed
the
fix_dual_stack__ovn_joinsubnet_collision
branch
from
df51feb to
a824440
Compare
…ual-stack routing collision When a KubeVirt hosted cluster and its management cluster both use OVN-Kubernetes with dual-stack networking, they each default to fd98::/64 for the IPv6 join switch subnet. External IPv6 LoadBalancer traffic targeting VM pods is SNAT'd to the management cluster's join IP (e.g. fd98::2). Inside the VM, the guest cluster's OVN intercepts the response because it also owns fd98::/64, black-holing the packet. This commit fixes the issue in two ways: 1. Automatic KubeVirt default: for KubeVirt hosted clusters with OVNKubernetes, the reconciler now sets IPv6.InternalJoinSubnet to fd99::/64 by default, avoiding the collision with the management cluster's fd98::/64. This mirrors the existing V4InternalSubnet override (100.66.0.0/16) already in place for IPv4. 2. User-facing API: adds OVNIPv6Config type to OVNKubernetesConfig, allowing explicit configuration of IPv6 internalJoinSubnet and internalTransitSwitchSubnet for any platform. This maps to the upstream operatorv1.IPv6OVNKubernetesConfig and includes IPv6 CIDR format validation via CEL rules. Also extends CIDR overlap validation in the HostedCluster webhook to cover IPv6 OVN subnets, and adds envtest CRD validation cases. Fixes: https://redhat.atlassian.net/browse/OCPBUGS-84303 Signed-off-by: Oren Cohen <ocohen@redhat.com> Assisted-by: Claude <noreply@anthropic.com>
orenc1
force-pushed
the
fix_dual_stack__ovn_joinsubnet_collision
branch
from
a824440 to
5c1ee1f
Compare
|
/approve |
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
Aside from the immutability rules being able to be circumvented, this looks good to me.
Not blocking on concrete immutability rules to prevent unnecessary scope increase for this PR. It has best-effort immutability rules in place which is good enough for me for now.
/approve
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: everettraven, jparrill, orenc1 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
|
/retest |
|
/retest |
|
The background task completed — it was the earlier |
|
@orenc1: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
5 participants
When a KubeVirt hosted cluster and its management cluster both use OVN-Kubernetes with dual-stack networking, they each default to fd98::/64 for the IPv6 join switch subnet. External IPv6 LoadBalancer traffic targeting VM pods is SNAT'd to the management cluster's join IP (e.g. fd98::2). Inside the VM, the guest cluster's OVN intercepts the response because it also owns fd98::/64, black-holing the packet.
This commit fixes the issue in two ways:
Automatic KubeVirt default: for KubeVirt hosted clusters with OVNKubernetes, the reconciler now sets IPv6.InternalJoinSubnet to fd99::/64 by default, avoiding the collision with the management cluster's fd98::/64. This mirrors the existing V4InternalSubnet override (100.66.0.0/16) already in place for IPv4.
User-facing API: adds OVNIPv6Config type to OVNKubernetesConfig, allowing explicit configuration of IPv6 internalJoinSubnet and internalTransitSwitchSubnet for any platform. This maps to the upstream operatorv1.IPv6OVNKubernetesConfig and includes IPv6 CIDR format validation via CEL rules.
Also extends CIDR overlap validation in the HostedCluster webhook to cover IPv6 OVN subnets, and adds envtest CRD validation cases.
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes https://redhat.atlassian.net/browse/OCPBUGS-84303
Special notes for your reviewer:
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes / Validation
Tests