CNTRLPLANE-647: Expose v4/v6InternalSubnet OVN-Kubernetes configuration in HostedCluster API#8249
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@hypershift-jira-solve-ci[bot]: This pull request references CNTRLPLANE-647 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. 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. |
|
@hypershift-jira-solve-ci[bot]: This pull request references CNTRLPLANE-647 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. 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:
📝 WalkthroughWalkthroughTwo optional fields, V4InternalSubnet and V6InternalSubnet, were added to OVNKubernetesConfig with CRD validations enforcing CIDR format, family-specific prefix-length bounds, string length limits, and immutability after initial set. Serialization compatibility tests validate N-1 clients ignore the new fields. The network reconciler now copies non-empty V4/V6 internal subnet values into operatorv1.Network.Spec.DefaultNetwork.OVNKubernetesConfig when OVNKubernetes is used. HostedCluster validation was refactored to parse and validate these subnet fields via a new helper appendCIDREntry, and unit tests were extended for propagation and CIDR overlap validation. Sequence Diagram(s)sequenceDiagram
participant User
participant APIServer as Kubernetes API
participant HostedController as HostedCluster Controller
participant CPO as Control-Plane Operator
participant NetworkCR as Network CR
User->>APIServer: Create/Update HostedCluster (OVNKubernetesConfig with V4/V6InternalSubnet)
APIServer->>HostedController: Notify change
HostedController->>HostedController: validateSliceNetworkCIDRs -> appendCIDREntry
HostedController->>APIServer: Read/Update Network CR request
APIServer->>CPO: Deliver Network CR reconcile request
CPO->>NetworkCR: Reconcile DefaultNetwork (copy V4/V6 when non-empty)
NetworkCR->>APIServer: Persist Network CR update
🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: hypershift-jira-solve-ci[bot] The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-ci
bot
added
area/api
|
@hypershift-jira-solve-ci[bot]: This pull request references CNTRLPLANE-647 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
test/e2e/karpenter_test.go (1)
212-212: Formatting change appears unrelated to PR objectives.This spacing adjustment in the
armNodeLabelsmap is unrelated to the PR's stated purpose of adding V4InternalSubnet and V6InternalSubnet fields to OVNKubernetesConfig. Consider reverting this change to keep the PR focused on its core objectives.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/karpenter_test.go` at line 212, Revert the unrelated whitespace/formatting change in the armNodeLabels map: restore the original spacing for the "kubernetes.io/arch": "arm64" entry inside the armNodeLabels variable in test/e2e/karpenter_test.go so the map formatting matches the surrounding entries and does not introduce cosmetic changes unrelated to adding V4InternalSubnet/V6InternalSubnet to OVNKubernetesConfig; ensure no other formatting-only changes remain in the armNodeLabels declaration.hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go (1)
3882-3976: Add explicit ServiceNetwork-overlap cases for the new OVN internal subnet fields.The new matrix is good, but it still misses direct failure cases where
V4InternalSubnet/V6InternalSubnetoverlapServiceNetwork. Adding those keeps this test aligned with the intended overlap validation surface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go` around lines 3882 - 3976, Add explicit test entries to the OVNKubernetes internal-subnet table that assert failures when V4InternalSubnet or V6InternalSubnet overlaps the ServiceNetwork: insert one case where sn includes CIDR "172.30.0.0/16" and OVNKubernetesConfig.V4InternalSubnet is e.g. "172.30.0.0/16" with wantErr: true, and another case where sn includes "fd03::/112" and OVNKubernetesConfig.V6InternalSubnet is e.g. "fd03::1:0/64" with wantErr: true; follow the same structure/fields used by the existing test entries (mn, cn, sn, networkType, ovnConfig, wantErr) so the test loop handling these cases (in hostedcluster_controller_test.go) will exercise ServiceNetwork overlap validation for both IPv4 and IPv6.api/hypershift/v1beta1/operator_test.go (1)
93-111: Consider adding IPv4 field assertion in reverse round-trip check.The reverse round-trip verification checks
MTUpreservation but doesn't explicitly verify thatIPv4is correctly preserved when deserializing N-1 data into the current struct. This would strengthen the test for case 3 whereIPv4is populated.💡 Optional: Add IPv4 preservation check
if roundTripped.MTU != tt.nMinus1Result.MTU { t.Errorf("MTU mismatch after N-1 round-trip: got %d, want %d", roundTripped.MTU, tt.nMinus1Result.MTU) } + // Verify IPv4 is preserved when present in N-1 data + if tt.nMinus1Result.IPv4 != nil { + if roundTripped.IPv4 == nil { + t.Errorf("IPv4 should be preserved after N-1 round-trip, got nil") + } else if roundTripped.IPv4.InternalJoinSubnet != tt.nMinus1Result.IPv4.InternalJoinSubnet { + t.Errorf("IPv4.InternalJoinSubnet mismatch: got %s, want %s", + roundTripped.IPv4.InternalJoinSubnet, tt.nMinus1Result.IPv4.InternalJoinSubnet) + } + } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/hypershift/v1beta1/operator_test.go` around lines 93 - 111, Add an assertion in the reverse round-trip block to verify the IPv4 field is preserved when unmarshaling N-1 JSON into the current OVNKubernetesConfig: after unmarshalling into roundTripped, compare roundTripped.IPv4 to tt.nMinus1Result.IPv4 and call t.Errorf with a clear message if they differ (similar style to the MTU check).hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go (1)
4322-4331: Consider logging a warning when CIDR parsing fails.The helper silently ignores parse errors, which could mask configuration issues that slipped past API validation. While CEL validation should catch most malformed CIDRs at admission time, a debug/warning log here would aid troubleshooting edge cases.
💡 Optional: Add debug logging for parse failures
func appendCIDREntry(entries []cidrEntry, cidrStr string, pathElements ...string) []cidrEntry { if cidrStr == "" { return entries } _, cidr, err := net.ParseCIDR(cidrStr) if err != nil { + // CEL validation should catch this at admission time, but log for debugging + // if an invalid CIDR somehow reaches reconciliation return entries } return append(entries, cidrEntry{*cidr, *field.NewPath(pathElements[0], pathElements[1:]...)}) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go` around lines 4322 - 4331, The appendCIDREntry helper currently swallows net.ParseCIDR errors; change it to emit a warning when parsing fails (but keep the current behavior of returning entries). Inside appendCIDREntry, when err != nil, log a warning that includes the offending cidrStr and the pathElements slice to aid debugging (e.g., use the project logging facility such as klog.Warningf or the controller logger) and then return entries; keep the rest of the function and the returned cidrEntry creation unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@api/hypershift/v1beta1/operator_test.go`:
- Around line 93-111: Add an assertion in the reverse round-trip block to verify
the IPv4 field is preserved when unmarshaling N-1 JSON into the current
OVNKubernetesConfig: after unmarshalling into roundTripped, compare
roundTripped.IPv4 to tt.nMinus1Result.IPv4 and call t.Errorf with a clear
message if they differ (similar style to the MTU check).
In
`@hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go`:
- Around line 3882-3976: Add explicit test entries to the OVNKubernetes
internal-subnet table that assert failures when V4InternalSubnet or
V6InternalSubnet overlaps the ServiceNetwork: insert one case where sn includes
CIDR "172.30.0.0/16" and OVNKubernetesConfig.V4InternalSubnet is e.g.
"172.30.0.0/16" with wantErr: true, and another case where sn includes
"fd03::/112" and OVNKubernetesConfig.V6InternalSubnet is e.g. "fd03::1:0/64"
with wantErr: true; follow the same structure/fields used by the existing test
entries (mn, cn, sn, networkType, ovnConfig, wantErr) so the test loop handling
these cases (in hostedcluster_controller_test.go) will exercise ServiceNetwork
overlap validation for both IPv4 and IPv6.
In `@hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go`:
- Around line 4322-4331: The appendCIDREntry helper currently swallows
net.ParseCIDR errors; change it to emit a warning when parsing fails (but keep
the current behavior of returning entries). Inside appendCIDREntry, when err !=
nil, log a warning that includes the offending cidrStr and the pathElements
slice to aid debugging (e.g., use the project logging facility such as
klog.Warningf or the controller logger) and then return entries; keep the rest
of the function and the returned cidrEntry creation unchanged.
In `@test/e2e/karpenter_test.go`:
- Line 212: Revert the unrelated whitespace/formatting change in the
armNodeLabels map: restore the original spacing for the "kubernetes.io/arch":
"arm64" entry inside the armNodeLabels variable in test/e2e/karpenter_test.go so
the map formatting matches the surrounding entries and does not introduce
cosmetic changes unrelated to adding V4InternalSubnet/V6InternalSubnet to
OVNKubernetesConfig; ensure no other formatting-only changes remain in the
armNodeLabels declaration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 8795123f-edae-4a1c-8555-462a441b8e64
⛔ Files ignored due to path filters (36)
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/AutoNodeKarpenter.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/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/AutoNodeKarpenter.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/**client/applyconfiguration/hypershift/v1beta1/ovnkubernetesconfig.gois excluded by!client/**cmd/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/**
📒 Files selected for processing (7)
api/hypershift/v1beta1/operator.goapi/hypershift/v1beta1/operator_test.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.gotest/e2e/karpenter_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8249 +/- ##
=======================================
Coverage 35.62% 35.62%
=======================================
Files 767 767
Lines 93330 93339 +9
=======================================
+ Hits 33245 33251 +6
- Misses 57396 57398 +2
- Partials 2689 2690 +1
🚀 New features to boost your workflow:
|
| // subnet must be larger than the number of nodes. The value cannot be changed | ||
| // after cluster creation. | ||
| // The default is 100.64.0.0/16. | ||
| // The value must be in proper IPv4 CIDR format. |
There was a problem hiding this comment.
What is a proper IPv4 CIDR format? It is probably worth spelling this out in prose for end users that may not be familiar with what this format entails.
There was a problem hiding this comment.
Done. Updated the GoDoc to spell out IPv4 CIDR format in prose: "four decimal octets (each 0-255) separated by dots, followed by a slash and a prefix length (e.g., 100.64.0.0/16)". Also documented the prefix length range (0-30) and first-octet constraint inline.
AI-assisted response via Claude Code
| // subnet must be larger than the number of nodes. The value cannot be changed | ||
| // after cluster creation. |
There was a problem hiding this comment.
If the value cannot be changed after cluster creation, does updating this field after the fact do anything? Should that be allowed?
There was a problem hiding this comment.
Good question. Updating this field after creation is explicitly prevented by the CEL validation rule self == oldSelf (line 107), which causes the API server to reject any update that attempts to change the value. So the immutability is enforced at the admission level — any PUT/PATCH that modifies the field will be rejected with the message "v4InternalSubnet is immutable once set". The field can only be set during initial creation (or on the first update that populates it).
AI-assisted response via Claude Code
There was a problem hiding this comment.
Good question. The `self == oldSelf` CEL validation rule at the field level will cause the API server to reject any update that attempts to change the value after it's initially set. Additionally, the parent-level rule `!has(oldSelf.v4InternalSubnet) || has(self.v4InternalSubnet)` prevents removing the field entirely once set. So updating this field after creation is not allowed — the API server enforces immutability. This is the standard Kubernetes pattern for immutable fields using CEL-based validation.
AI-assisted response via Claude Code
There was a problem hiding this comment.
I should have been more clear.
If I create the HostedCluster and intentionally omit the field at creation time, but set it in a subsequent update after the HostedCluster has already rolled out, what happens? Is that a valid operation?
There was a problem hiding this comment.
Good clarification. With the current self == oldSelf CEL transition rule, setting the field in a subsequent update after omitting it at creation is not a valid operation — the API server will reject it.
Here's why: on an update where the field transitions from absent to present, Kubernetes treats oldSelf as the zero value for the type (empty string "" for a string field). So self == oldSelf evaluates as "100.64.0.0/16" == "" → false → rejected.
This means the field can only be set at initial HostedCluster creation time, never afterward. This behavior is intentional — since OVN-K would have already deployed with the default internal subnet (100.64.0.0/16), changing it post-rollout would cause network disruption.
If we wanted to allow "set once, then immutable" semantics instead (i.e., allow first-set on update but block subsequent changes), we'd need to change the rule to something like oldSelf == '' || self == oldSelf. But the current stricter behavior seems appropriate given the operational risk.
Should I update the GoDoc to explicitly state "must be set at creation time, cannot be added later"?
AI-assisted response via Claude Code
There was a problem hiding this comment.
Good question — under the current implementation, that update would be rejected by the self == oldSelf CEL transition rule on the field. On UPDATE operations, when the field was previously absent, oldSelf evaluates to the zero value (empty string for strings), so self == oldSelf becomes "new-value" == "" → false, and the API server rejects the update.
This is intentional: by the time the HostedCluster has rolled out, OVN-Kubernetes is already running with its default internal subnet (100.64.0.0/16 for v4, fd98::/64 for v6). Allowing a user to set a different subnet value after the fact would require OVN to reconfigure its internal networking, which would be disruptive to the running cluster.
So the field is effectively "set at creation time or accept the default" — if you omit it during creation, you cannot populate it later. This is the same pattern used by the mtu field on this struct.
If you believe users should be able to initially set it on a subsequent update (while still preventing changes once set), we could adjust the rule to oldSelf == '' || self == oldSelf. However, I think the current behavior is the safest approach since there's no way to distinguish between setting it to the same value OVN defaulted to vs. a different (disruptive) value. Let me know if you'd like that changed.
AI-assisted response via Claude Code
There was a problem hiding this comment.
With the current self == oldSelf CEL transition rule, setting the field in a subsequent update after omitting it at creation is not a valid operation — the API server will reject it.
This is not true. Rules that use oldSelf only get evaluated IF there is an old value present OR optionalOldSelf is enabled for the rule, meaning you must handle optionality. Source of truth is at https://github.com/kubernetes/kubernetes/blob/b2f73c0d6b427e2ab5ba225375aaefc0b9bc45b2/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation.go#L399-L413
I've also manually validated this myself where using a CRD with an optional field with the rule self == oldSelf does not prevent me from creating the resource with the optional field omitted and then immediately following up with adding a value to the field.
If it is truly invalid for this field to be set after initial cluster creation, whether specified or not, we should explicitly forbid this action.
There was a problem hiding this comment.
Thank you for the correction — you're absolutely right. I apologize for the incorrect information in my previous responses. CEL transition rules with oldSelf only evaluate when oldSelf has a value present (or when optionalOldSelf is enabled), as you've confirmed from the source and through testing.
The current self == oldSelf rule provides "immutable once set" semantics: a user can omit the field at creation and set it on a subsequent update (since the rule won't fire without an existing value), but once set, it cannot be changed. This is actually the desired behavior — it allows users to populate the field before OVN fully deploys while preventing disruptive changes after the subnet is in use.
I've updated the godoc to replace the misleading "The value cannot be changed after cluster creation" with "Once set, the value is immutable and cannot be modified in subsequent updates" to accurately reflect this behavior.
AI-assisted response via Claude Code
There was a problem hiding this comment.
This is actually the desired behavior — it allows users to populate the field before OVN fully deploys while preventing disruptive changes after the subnet is in use
Is this actually the desired behavior? Earlier, you said:
This behavior is intentional — since OVN-K would have already deployed with the default internal subnet (100.64.0.0/16), changing it post-rollout would cause network disruption
Which one is it? It seems like it is probably an incredibly small window to "populate the field before OVN fully deploys". We should probably do one of:
- Explicitly populate a default for the field if it is not specified
- Do not allow setting the field if the
HostedClusterwas initially created without the field being set.
Allowing users to populate the field after initial creation seems like an unnecessary foot gun to be giving our users if doing so is actually destructive to the cluster.
| // +kubebuilder:validation:XValidation:rule="self.matches('^([0-9]{1,3}\\\\.){3}[0-9]{1,3}/([0-9]|[1-2][0-9]|3[0-2])$') && self.split('/')[0].split('.').all(oct, int(oct) >= 0 && int(oct) <= 255)", message="Subnet must be in a valid IPv4 CIDR format" | ||
| // +kubebuilder:validation:XValidation:rule="self.matches('^.*/[0-9]+$') && int(self.split('/')[1]) <= 30", message="subnet must be in the range /0 to /30 inclusive" | ||
| // +kubebuilder:validation:XValidation:rule="self.matches('^[0-9]{1,3}\\\\..*') && int(self.split('/')[0].split('.')[0]) > 0", message="first IP address octet must not be 0" |
There was a problem hiding this comment.
The validations for the IPv6 CIDR format use the cidr CEL library that Kubernetes adds to the CEL execution environment for these validations. Should this use the same CEL library?
There was a problem hiding this comment.
Good observation. The cidr() CEL library could indeed be used for IPv4 as well (e.g., isCIDR(self) && cidr(self).ip().family() == 4), which would be more consistent with the IPv6 validation. The regex approach was chosen here to match the existing pattern used by the internalTransitSwitchSubnet and internalJoinSubnet fields in OVNIPv4Config (lines 143-145), keeping the validation style consistent across the IPv4 subnet fields in this file. However, switching to the cidr CEL library for IPv4 would be cleaner — happy to make that change if you'd prefer consistency with the IPv6 approach over consistency with the existing IPv4 fields.
AI-assisted response via Claude Code
There was a problem hiding this comment.
Good point. The v6 validation already uses the CEL `cidr` library (`isCIDR(self) && cidr(self).ip().family() == 6`), which is cleaner and more maintainable. The v4 validation uses regex to match the pattern used by existing fields in this file (`internalTransitSwitchSubnet` and `internalJoinSubnet` in `OVNIPv4Config`), which were defined before the CEL `cidr` library was available.
Switching v4 to use the CEL `cidr` library (e.g., `isCIDR(self) && cidr(self).ip().family() == 4`) would be a valid improvement for consistency. However, the additional regex-based rules for prefix length range (`<= 30`) and first-octet validation (`> 0`) would still need to be expressed — the `cidr` library can handle prefix length via `cidr(self).prefixLength()`, but the first-octet check would still require string parsing. Happy to switch to the CEL `cidr` library if you'd prefer that approach.
AI-assisted response via Claude Code
There was a problem hiding this comment.
Please use the newer CEL library approach
There was a problem hiding this comment.
Done. Switched v4InternalSubnet validation to use the CEL cidr() library, matching the v6InternalSubnet approach:
isCIDR(self) && cidr(self).ip().family() == 4— validates IPv4 CIDR formatisCIDR(self) && cidr(self).prefixLength() <= 30— validates prefix length rangeint(self.split('.')[0]) > 0— first octet must not be 0 (kept as string check since the CELip()library doesn't expose individual octets)
Note: the existing internalTransitSwitchSubnet and internalJoinSubnet fields in OVNIPv4Config still use the regex approach — left those unchanged to keep this change scoped, but happy to update them as well if desired.
AI-assisted response via Claude Code
There was a problem hiding this comment.
Done. Switched v4InternalSubnet validation to use the CEL cidr library, consistent with v6InternalSubnet:
isCIDR(self) && cidr(self).ip().family() == 4for IPv4 CIDR formatisCIDR(self) && cidr(self).prefixLength() <= 30for prefix length rangeisCIDR(self) && int(self.split('.')[0]) > 0for first-octet check (kept string parsing here since the CEL IP library doesn't expose individual octets)
CRDs, vendor, and docs regenerated.
AI-assisted response via Claude Code
| // subnet must be larger than the number of nodes. The value cannot be changed | ||
| // after cluster creation. | ||
| // The default is fd98::/64. | ||
| // The value must be in proper IPv6 CIDR format. |
There was a problem hiding this comment.
Similar comment regarding describing the format in prose for users unfamiliar with the IPv6 CIDR format.
There was a problem hiding this comment.
Done. Updated the v6InternalSubnet GoDoc to describe the format in prose: "an IPv6 address followed by a slash and a prefix length (e.g., fd98::/64)".
AI-assisted response via Claude Code
There was a problem hiding this comment.
Done. Updated the GoDoc to describe IPv6 CIDR format in prose: "The value must be in IPv6 CIDR notation (e.g., fd98::/64), consisting of an IPv6 address followed by a slash and a prefix length."
AI-assisted response via Claude Code
| // subnet must be larger than the number of nodes. The value cannot be changed | ||
| // after cluster creation. |
There was a problem hiding this comment.
Similar comment regarding updating this field after initial creation.
There was a problem hiding this comment.
Same as v4InternalSubnet — the self == oldSelf CEL validation rule (line 120) prevents any updates to this field after it's been set. The API server will reject any mutation with the message "v6InternalSubnet is immutable once set".
AI-assisted response via Claude Code
There was a problem hiding this comment.
Same as the v4 field — the `self == oldSelf` CEL validation rule prevents any update to this field after creation, and the parent-level rule `!has(oldSelf.v6InternalSubnet) || has(self.v6InternalSubnet)` prevents removal. The API server will reject any attempt to change or remove this field once set.
AI-assisted response via Claude Code
There was a problem hiding this comment.
See my response at https://github.com/openshift/hypershift/pull/8249/changes#r3092953432
There was a problem hiding this comment.
Thank you — acknowledged. Updated the v6InternalSubnet godoc with the same fix: replaced "The value cannot be changed after cluster creation" with "Once set, the value is immutable and cannot be modified in subsequent updates" to match the actual self == oldSelf CEL behavior.
AI-assisted response via Claude Code
| // +kubebuilder:validation:XValidation:rule="self.matches('^.*/[0-9]+$') && int(self.split('/')[1]) <= 30", message="subnet must be in the range /0 to /30 inclusive" | ||
| // +kubebuilder:validation:XValidation:rule="self.matches('^[0-9]{1,3}\\\\..*') && int(self.split('/')[0].split('.')[0]) > 0", message="first IP address octet must not be 0" |
There was a problem hiding this comment.
Explicitly document these constraints in the GoDoc
There was a problem hiding this comment.
Done. Updated the GoDoc to explicitly document: "The prefix length must be between 0 and 30 inclusive, and the first octet must not be 0."
AI-assisted response via Claude Code
There was a problem hiding this comment.
Done. The v4InternalSubnet GoDoc now documents all validation constraints in prose: prefix length range (0-30), first octet must be >0, each octet 0-255, and the string length limits (9-18).
AI-assisted response via Claude Code
hypershift-jira-solve-ci
bot
force-pushed
the
fix-CNTRLPLANE-647
branch
from
f675f08 to
c0615f8
Compare
|
@hypershift-jira-solve-ci[bot]: This pull request references CNTRLPLANE-647 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. 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. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
control-plane-operator/hostedclusterconfigoperator/controllers/resources/network/reconcile_test.go (1)
540-556: Add a matching non-OVN negative case forV6InternalSubnet.You already guard
V4InternalSubnetfor non-OVN network types; adding the same explicit check forV6InternalSubnetwould close the gap for the new IPv6 path and make regressions easier to catch.Suggested test addition
+ { + name: "When v6InternalSubnet is specified with non-OVN network type, it should be ignored", + inputNetwork: NetworkOperator(), + inputNetworkType: hyperv1.OpenShiftSDN, + inputPlatformType: hyperv1.AWSPlatform, + disableMultiNetwork: false, + ovnConfig: &hyperv1.OVNKubernetesConfig{ + V6InternalSubnet: "fd99::/64", + }, + expectedNetwork: &operatorv1.Network{ + ObjectMeta: NetworkOperator().ObjectMeta, + Spec: operatorv1.NetworkSpec{ + OperatorSpec: operatorv1.OperatorSpec{ + ManagementState: "Managed", + }, + }, + }, + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/hostedclusterconfigoperator/controllers/resources/network/reconcile_test.go` around lines 540 - 556, Add a negative test mirroring the existing "When v4InternalSubnet is specified with non-OVN network type, it should be ignored" case but for V6InternalSubnet: in reconcile_test.go add a table entry where inputNetworkType is hyperv1.OpenShiftSDN (non-OVN), ovnConfig is &hyperv1.OVNKubernetesConfig{V6InternalSubnet: "fd00:200::/64"}, and expectedNetwork does not include the V6 subnet (same expected Network as the v4 negative case). Ensure the test name clearly states V6InternalSubnet is ignored for non-OVN, so the guard around V6InternalSubnet is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@control-plane-operator/hostedclusterconfigoperator/controllers/resources/network/reconcile_test.go`:
- Around line 540-556: Add a negative test mirroring the existing "When
v4InternalSubnet is specified with non-OVN network type, it should be ignored"
case but for V6InternalSubnet: in reconcile_test.go add a table entry where
inputNetworkType is hyperv1.OpenShiftSDN (non-OVN), ovnConfig is
&hyperv1.OVNKubernetesConfig{V6InternalSubnet: "fd00:200::/64"}, and
expectedNetwork does not include the V6 subnet (same expected Network as the v4
negative case). Ensure the test name clearly states V6InternalSubnet is ignored
for non-OVN, so the guard around V6InternalSubnet is exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 9d746331-acb6-411b-89a2-d60d20b8fffa
⛔ Files ignored due to path filters (36)
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/AutoNodeKarpenter.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/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/AutoNodeKarpenter.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/**client/applyconfiguration/hypershift/v1beta1/ovnkubernetesconfig.gois excluded by!client/**cmd/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/**
📒 Files selected for processing (6)
api/hypershift/v1beta1/operator.goapi/hypershift/v1beta1/operator_test.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 as they are similar to previous changes (5)
- control-plane-operator/hostedclusterconfigoperator/controllers/resources/network/reconcile.go
- hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go
- api/hypershift/v1beta1/operator.go
- api/hypershift/v1beta1/operator_test.go
- hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go
|
Re: review PRR_kwDOE7ekcc71NZsb Regarding the nitpick comments from the first CodeRabbit review: karpenter_test.go formatting (line 212): This whitespace change was introduced by Other suggestions (ServiceNetwork overlap test cases, IPv4 round-trip assertion, debug logging for CIDR parse failures): These are good suggestions that could be addressed in follow-up work. The ServiceNetwork overlap cases are already covered indirectly by the existing CIDR overlap validation tests, and the parse failure logging is a minor improvement that doesn't affect correctness. AI-assisted response via Claude Code |
|
Re: review PRR_kwDOE7ekcc71TMjf Regarding the nitpick from the second CodeRabbit review: V6InternalSubnet non-OVN negative test: Done. Added a matching test case "When v6InternalSubnet is specified with non-OVN network type, it should be ignored" that mirrors the existing v4InternalSubnet negative case. All tests pass. AI-assisted response via Claude Code |
|
@hypershift-jira-solve-ci[bot]: This pull request references CNTRLPLANE-647 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. 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. |
|
Re: review PRR_kwDOE7ekcc71NZsb Thanks for the thorough review. Addressing the nitpicks:
AI-assisted response via Claude Code |
|
Re: review PRR_kwDOE7ekcc71TMjf Good catch — we have the non-OVN negative test for AI-assisted response via Claude Code |
|
Re: review PRR_kwDOE7ekcc71XSo3 Good catch! Added a test case AI-assisted response via Claude Code |
|
@hypershift-jira-solve-ci[bot]: This pull request references CNTRLPLANE-647 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. 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. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
control-plane-operator/hostedclusterconfigoperator/controllers/resources/network/reconcile_test.go (1)
436-599: Optional: factor repeated expectedNetworkscaffolding into helpers.The table is clear, but repeated
ObjectMeta/ManagementStateblocks make future updates noisy. A small builder/helper for expected networks would keep these new subnet scenarios easier to extend.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/hostedclusterconfigoperator/controllers/resources/network/reconcile_test.go` around lines 436 - 599, Create a small test helper that returns the common expected operatorv1.Network scaffolding (ObjectMeta from NetworkOperator(), Spec.OperatorSpec.ManagementState "Managed", and a DefaultNetwork stub) and use it to build each expectedNetwork in the table; locate uses of expectedNetwork in this test file and replace repeated literal structs with calls to the helper, and for cases needing OVNKubernetesConfig merge in fields like V4InternalSubnet, V6InternalSubnet, GenevePort, MTU, and IPv4 (referencing OVNKubernetesConfig, operatorv1.OVNKubernetesConfig, IPv4OVNKubernetesConfig, genevePort, v4InternalSubnet, ptr.To) to keep tests concise and reduce duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@control-plane-operator/hostedclusterconfigoperator/controllers/resources/network/reconcile_test.go`:
- Around line 436-599: Create a small test helper that returns the common
expected operatorv1.Network scaffolding (ObjectMeta from NetworkOperator(),
Spec.OperatorSpec.ManagementState "Managed", and a DefaultNetwork stub) and use
it to build each expectedNetwork in the table; locate uses of expectedNetwork in
this test file and replace repeated literal structs with calls to the helper,
and for cases needing OVNKubernetesConfig merge in fields like V4InternalSubnet,
V6InternalSubnet, GenevePort, MTU, and IPv4 (referencing OVNKubernetesConfig,
operatorv1.OVNKubernetesConfig, IPv4OVNKubernetesConfig, genevePort,
v4InternalSubnet, ptr.To) to keep tests concise and reduce duplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 63e26828-a823-403c-b2b9-db8060d0be09
📒 Files selected for processing (1)
control-plane-operator/hostedclusterconfigoperator/controllers/resources/network/reconcile_test.go
|
Re: review PRR_kwDOE7ekcc71a05q Thanks for the suggestion! The repeated scaffolding is a fair observation. However, the new test cases intentionally follow the same explicit pattern used by the existing tests in this file (lines 30–435) to keep them consistent and easy to compare side-by-side. Each case has slightly different If the test table grows significantly in the future, a builder/helper would be a good follow-up to reduce noise. For now, keeping the explicit struct literals maintains readability and consistency with the existing test style. AI-assisted response via Claude Code |
|
Re: review PRR_kwDOE7ekcc71a05q Thanks for the suggestion. The repeated AI-assisted response via Claude Code |
| @@ -0,0 +1,114 @@ | |||
| package v1beta1 | |||
There was a problem hiding this comment.
Nit: Make this a testing package so importers of the v1beta1 package do not inherently import test code?
There was a problem hiding this comment.
Done. Changed operator_test.go to use package v1beta1_test (external test package) with an explicit hyperv1 import alias. This prevents importers of the v1beta1 package from pulling in test helper types like ovnKubernetesConfigNMinus1.
AI-assisted response via Claude Code
| } | ||
| // Apply V4InternalSubnet configuration. | ||
| // User-specified values take precedence over platform defaults (e.g., KubeVirt's 100.66.0.0/16). | ||
| if ovnConfig.V4InternalSubnet != "" { |
There was a problem hiding this comment.
shouldn't we validate against V4InternalSubnet default value here if empty?
same for V6InternalSubnet below
There was a problem hiding this comment.
Good point. When V4InternalSubnet is empty, OVN-Kubernetes defaults to 100.64.0.0/16 internally. Currently the code only propagates user-specified values — if the field is empty we leave it to OVN's own default.
The CIDR overlap validation in the hypershift-operator (hostedcluster_controller.go:4316) also skips the check when the field is empty, meaning a conflict between the OVN default subnet and user-defined cluster/service CIDRs would go undetected.
Two options to address this:
- Validate with the default at the hypershift-operator level: In the overlap validation, if
V4InternalSubnetis empty, substitute the OVN default (100.64.0.0/16for v4,fd98::/64for v6) and include it in the overlap check. This catches conflicts without changing the reconcile behavior. - Explicitly set the default in the reconcile function here: Set
ovnCfg.V4InternalSubnet = "100.64.0.0/16"when the user doesn't specify one. This makes the network operator CR explicit but couples us to OVN's internal default.
I think option 1 is cleaner — it catches conflicts without hard-coding OVN defaults in the reconcile path. Would you like me to implement that approach, or do you prefer option 2?
AI-assisted response via Claude Code
|
hypershift-jira-solve-ci make sure verify and envtest pass before pushing /test address-review-comments |
|
Re: #8249 (comment) @enxebre Acknowledged. I've addressed the new review comments:
Tests pass locally ( AI-assisted response via Claude Code |
|
@hypershift-jira-solve-ci[bot]: This pull request references CNTRLPLANE-647 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. 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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
api/hypershift/v1beta1/operator_test.go (1)
13-16: Decouple the N-1 fixture from current API nested types.Line 14 and Lines 54-55 use
hyperv1.OVNIPv4Configinside the N-1 surrogate. That makes the “N-1” shape drift whenOVNIPv4Configevolves, which can hide compatibility regressions.Proposed refactor
type ovnKubernetesConfigNMinus1 struct { - IPv4 *hyperv1.OVNIPv4Config `json:"ipv4,omitempty"` - MTU int32 `json:"mtu,omitempty"` + IPv4 *ovnIPv4ConfigNMinus1 `json:"ipv4,omitempty"` + MTU int32 `json:"mtu,omitempty"` } + +type ovnIPv4ConfigNMinus1 struct { + InternalJoinSubnet string `json:"internalJoinSubnet,omitempty"` +} @@ nMinus1Result: ovnKubernetesConfigNMinus1{ MTU: 8901, - IPv4: &hyperv1.OVNIPv4Config{ + IPv4: &ovnIPv4ConfigNMinus1{ InternalJoinSubnet: "100.64.0.0/16", }, },Also applies to: 52-56
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/hypershift/v1beta1/operator_test.go` around lines 13 - 16, The ovnKubernetesConfigNMinus1 test fixture currently embeds the live type hyperv1.OVNIPv4Config (IPv4 *hyperv1.OVNIPv4Config) which lets that fixture silently drift when OVNIPv4Config changes; replace that reference with a local minimal surrogate type (e.g., define IPv4 as *struct{ IP *string; Enabled *bool } or the exact fields the historical API had) declared in this test file and update any uses of ovnKubernetesConfigNMinus1.IPv4 (and the related instances around the other occurrances) to use that local struct so the N-1 shape remains stable and decoupled from hyperv1.OVNIPv4Config.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/hypershift/v1beta1/operator_test.go`:
- Around line 111-113: The test currently only asserts MTU after the N-1 → N
round-trip; add an assertion that roundTripped.IPv4 equals the expected value
(tt.nMinus1Result.IPv4) to ensure nested IPv4 field compatibility is preserved.
Locate the comparison block that checks roundTripped.MTU in operator_test.go
(using the roundTripped and tt.nMinus1Result symbols) and add a similar if-check
that calls t.Errorf with a clear message (e.g., "IPv4 mismatch after N-1
round-trip: got %v, want %v") when the values differ.
---
Nitpick comments:
In `@api/hypershift/v1beta1/operator_test.go`:
- Around line 13-16: The ovnKubernetesConfigNMinus1 test fixture currently
embeds the live type hyperv1.OVNIPv4Config (IPv4 *hyperv1.OVNIPv4Config) which
lets that fixture silently drift when OVNIPv4Config changes; replace that
reference with a local minimal surrogate type (e.g., define IPv4 as *struct{ IP
*string; Enabled *bool } or the exact fields the historical API had) declared in
this test file and update any uses of ovnKubernetesConfigNMinus1.IPv4 (and the
related instances around the other occurrances) to use that local struct so the
N-1 shape remains stable and decoupled from hyperv1.OVNIPv4Config.
🪄 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: Pro Plus
Run ID: 65b0fcf2-eef5-42e2-9a36-22cb87309b30
📒 Files selected for processing (2)
api/hypershift/v1beta1/operator.goapi/hypershift/v1beta1/operator_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- api/hypershift/v1beta1/operator.go
|
Re: review PRR_kwDOE7ekcc71pEUI Regarding the nitpick to decouple the N-1 fixture from The actionable suggestion (asserting IPv4 preservation) has been implemented. AI-assisted response via Claude Code |
|
Re: review PRR_kwDOE7ekcc71pEUI Acknowledged the nitpick about factoring repeated AI-assisted response via Claude Code |
|
Now I have all the evidence I need. Let me compile the final report. Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThere are two independent failures. (1) Lint — the new Root CauseRoot Cause 1 — Lint failure (caused by this PR): The PR adds a new test helper struct type ovnKubernetesConfigNMinus1 struct {
IPv4 *hyperv1.OVNIPv4Config `json:"ipv4,omitempty"` // ← missing godoc
MTU int32 `json:"mtu,omitempty"` // ← missing godoc
}Root Cause 2 — Envtest failure (pre-existing flake, NOT caused by this PR): PR #8199 ("CNTRLPLANE-3214: add envtest suites for HCPEtcdBackup CRD validation") merged on April 15, 2026 at 17:28 UTC. It introduced new CRD validation tests for Evidence this is pre-existing:
Recommendations
Evidence
|
| @@ -0,0 +1,120 @@ | |||
| package v1beta1_test | |||
There was a problem hiding this comment.
drop this whole test. We don't want to increase transient go mod deps for the API module. We'll address unit tests differently in another PR
There was a problem hiding this comment.
Done. Dropped api/hypershift/v1beta1/operator_test.go entirely.
AI-assisted response via Claude Code
|
rebase the PR now that #8261 is in and make sure verify passes before pushing. /test address-review-comments |
…Config - Add optional V4InternalSubnet field for configuring the IPv4 subnet used internally by OVN-Kubernetes (default 100.64.0.0/16) - Add optional V6InternalSubnet field for configuring the IPv6 subnet used internally by OVN-Kubernetes (default fd98::/64) - Add CEL validation rules for CIDR format, prefix length, and non-zero first octet (IPv4) constraints - Add immutability validation to prevent removal once set - Regenerate featuregated CRD manifests for HostedCluster and HostedControlPlane Signed-off-by: OpenShift CI Bot <ci-bot@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
- Regenerate CRD manifests for HostedCluster and HostedControlPlane with v4InternalSubnet and v6InternalSubnet fields - Update OVNKubernetesConfig apply configuration client - Vendor updated API types Signed-off-by: OpenShift CI Bot <ci-bot@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
…nalSubnet - Extend validateSliceNetworkCIDRs to check V4InternalSubnet and V6InternalSubnet against cluster, machine, and service networks - Extract appendCIDREntry helper to reduce duplicated CIDR parsing logic - Restructure nil-check nesting so subnet validation runs independently of IPv4 config presence - Add unit tests for overlap detection across MachineNetwork, ClusterNetwork, and InternalJoinSubnet for both IPv4 and IPv6 Signed-off-by: OpenShift CI Bot <ci-bot@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
…k operator CR - Propagate V4InternalSubnet and V6InternalSubnet from HostedCluster OVNKubernetesConfig to the guest cluster network operator config - User-specified values take precedence over platform defaults - Add unit tests covering AWS propagation, KubeVirt override, combined subnet configuration, and OpenShiftSDN no-op scenarios Signed-off-by: OpenShift CI Bot <ci-bot@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
- Add forward/backward serialization tests for V4InternalSubnet and V6InternalSubnet fields against an N-1 version of the struct - Verify new fields round-trip correctly and are silently dropped by older deserializers - Fix whitespace alignment in karpenter_test.go Signed-off-by: OpenShift CI Bot <ci-bot@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
- Update API reference with v4InternalSubnet and v6InternalSubnet field documentation - Regenerate aggregated docs Signed-off-by: OpenShift CI Bot <ci-bot@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
Switch v4InternalSubnet validation from regex-based approach to the newer CEL cidr() library, matching the approach already used by v6InternalSubnet. Also adds a negative test case for v6InternalSubnet with non-OVN network types. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add test verifying that when only V6InternalSubnet is user-specified on KubeVirt platform, the KubeVirt defaults (GenevePort and default V4InternalSubnet) are retained alongside the user-specified V6 subnet. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Update v4InternalSubnet and v6InternalSubnet godoc to accurately reflect CEL `self == oldSelf` behavior: these fields are immutable once set, but can be initially populated on a subsequent update if omitted at creation. The previous wording "cannot be changed after cluster creation" was misleading since the transition rule only evaluates when oldSelf is present. Move operator_test.go to the v1beta1_test external test package so that importers of the v1beta1 API package do not inherit test code. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add reflect.DeepEqual assertion for the IPv4 field in the serialization compatibility test to catch regressions in nested field preservation during N-1 → N round-trips. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove operator_test.go to avoid increasing transient go mod dependencies for the API module. Unit tests for these types will be addressed differently in a follow-up PR. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Regenerate all generated files (CRDs, featuregated manifests, API docs, and vendored copy) to reflect changes from upstream main after rebasing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Re: #8249 (comment) Done. Rebased onto main (which includes #8261), ran AI-assisted response via Claude Code |
hypershift-jira-solve-ci
bot
force-pushed
the
fix-CNTRLPLANE-647
branch
from
e3929e1 to
2863e0f
Compare
3 participants
What this PR does / why we need it:
Exposes OVN-Kubernetes internal subnet configuration (
v4InternalSubnetandv6InternalSubnet) in the HostedCluster API, allowing users to customize the IPv4 and IPv6 subnets used internally by OVN-Kubernetes instead of relying on the defaults (100.64.0.0/16andfd98::/64).This is needed when the default OVN internal subnets overlap with existing network infrastructure, causing routing conflicts for hosted clusters.
Changes
api/hypershift/v1beta1): Add optionalV4InternalSubnetandV6InternalSubnetfields toOVNKubernetesConfigwith CEL validation rules for CIDR format, prefix length, non-zero first octet (IPv4), and immutability once set.hypershift-operator): ExtendvalidateSliceNetworkCIDRsto detect CIDR overlap between the new internal subnets and cluster/machine/service networks. ExtractappendCIDREntryhelper to reduce duplicated parsing logic.control-plane-operator): PropagateV4InternalSubnetandV6InternalSubnetfrom the HostedCluster'sOVNKubernetesConfigto the guest cluster's network operator CR, with user-specified values taking precedence over platform defaults.Which issue(s) this PR fixes:
Fixes https://redhat.atlassian.net/browse/CNTRLPLANE-647
Special notes for your reviewer:
Checklist:
Always review AI generated responses prior to use.
Generated with Claude Code via
/jira:solve [CNTRLPLANE-647](https://redhat.atlassian.net/browse/CNTRLPLANE-647)Summary by CodeRabbit
New Features
Improvements
Tests