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 |
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 41.84% 41.85% +0.01%
==========================================
Files 759 759
Lines 94073 94099 +26
==========================================
+ Hits 39361 39387 +26
+ Misses 51956 51951 -5
- Partials 2756 2761 +5
... and 2 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:
|
| // 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.
There was a problem hiding this comment.
I'm still waiting for a response to this.
| // +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: #8249 (comment) Thanks for the walkthrough and pre-merge checks. Regarding the two warnings:
AI-assisted response via Claude Code |
…Config - Add V4InternalSubnet field for customizing OVN-Kubernetes internal IPv4 subnet (default 100.64.0.0/16), validated via CEL for IPv4 CIDR format, prefix /0-/30, and non-zero first octet - Add V6InternalSubnet field for customizing OVN-Kubernetes internal IPv6 subnet (default fd98::/64), validated via CEL for IPv6 CIDR format, prefix /0-/125 - Both fields are immutable once set, enforced at struct and field level - 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)
Signed-off-by: OpenShift CI Bot <ci-bot@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
…nalSubnet - Extract appendCIDREntry helper for compile-time safe CIDR entry construction - Add v4InternalSubnet and v6InternalSubnet to CIDR overlap validation in validateSliceNetworkCIDRs - Add unit tests for overlap detection against machine, cluster, and service networks, as well as cross-field overlap with internalJoinSubnet Signed-off-by: OpenShift CI Bot <ci-bot@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
…k operator - Extract applyOVNConfig to reduce cyclomatic complexity in ReconcileNetworkOperator - Propagate V4InternalSubnet and V6InternalSubnet from HostedCluster OVN config to the guest cluster network operator CR - User-specified values override platform defaults (e.g. KubeVirt's 100.66.0.0/16) - Add tests for AWS propagation, KubeVirt override, combined fields, and non-OVN network type no-ops Signed-off-by: OpenShift CI Bot <ci-bot@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
Signed-off-by: OpenShift CI Bot <ci-bot@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
|
Re: #8249 (comment) Done. Rebased branch onto latest AI-assisted response via Claude Code |
|
Re: #8249 (comment) Done. Verified commits are already structured by component in the correct order:
No restructuring was needed — file boundaries and commit order match the component mapping. AI-assisted response via Claude Code |
The pre-commit framework passes changed filenames as extra arguments to hook commands by default. For make-based hooks, this causes make to interpret those filenames as additional targets, triggering parallel invocations of the generate dependency. This creates a race condition in git clean -fx -- '*_mock.go' where one invocation deletes the mock files while others simultaneously try to lstat them, causing fatal errors. Add pass_filenames: false to both make-verify and make-test pre-push hooks since these make targets operate on the entire codebase and should not receive individual filenames. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
hypershift-jira-solve-ci
Bot
force-pushed
the
fix-CNTRLPLANE-647
branch
from
eb90ac0 to
7bd6c96
Compare
|
@everettraven's comment #10 ("How is Both fields currently share the same default ( Suggestion: add a clarifying sentence to
Same applies to the v6 counterparts. |
|
/test address-review-comments |
|
Re: #8249 (comment) Done. Updated the GoDoc for both The GoDoc now states:
(Analogous clarification added for CRDs and API docs regenerated. AI-assisted response via Claude Code |
Update GoDoc for v4InternalSubnet and v6InternalSubnet to clearly explain their purpose (gateway router LRP addresses and masquerade/SNAT traffic) and explicitly distinguish them from ipv4/ipv6.internalJoinSubnet (join switch interconnecting gateway routers with the cluster router). Both share the same default but control different OVN-Kubernetes internal networks. Regenerated CRDs and API docs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/approve |
openshift-ci
Bot
added
the
approved
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox, csrwng, hypershift-jira-solve-ci[bot] 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 |
|
/retest |
|
None of the failures seem related to this pr |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
|
/retest AWS throttling |
|
/retest ci infra issues |
|
@hypershift-jira-solve-ci[bot]: The following tests failed, say
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. |
|
I now have all the evidence needed. The test log output at lines 6528-6537 is not from a second run — it's the same test output being echoed (the build log is echoed multiple times due to the test output being printed at the end). The key failure is clear. Let me assemble the final report. The complete timeline is:
Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe Root CauseThe root cause is a Karpenter node replacement race condition / timing flake in the
Recommendations
Evidence
|
6 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