NVIDIA-314: Support Node CIDR Allocation for networkType=Other Using AllocateNodesCIDRs - #7278
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@linoyaslan: This pull request references NVIDIA-314 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 epic to target the "4.21.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. |
WalkthroughAdded a new, immutable Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas requiring extra attention:
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
do-not-merge/needs-area
do-not-merge/work-in-progress
openshift-ci
Bot
added
area/api
f5ca5a0 to
1333030
Compare
openshift-ci
Bot
added
the
area/testing
|
@linoyaslan: This pull request references NVIDIA-314 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 epic to target the "4.21.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. |
|
@linoyaslan: This pull request references NVIDIA-314 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 epic to target the "4.21.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. |
1333030 to
de56786
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
de56786 to
0b1b468
Compare
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (24)
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ImageStreamImportMode.yaml (2)
2725-2733: Define day-2 behavior; likely makeisFlannelimmutable.Toggling
spec.networking.isFlannelpost‑install can change KCM node CIDR behavior. Either:
- document allowed day‑2 changes clearly, or
- add an immutability rule (preferred; see suggested rule in the spec-level validations comment below).
Please confirm KCM derives node-cidr-mask-size safely without
hostPrefix, or makehostPrefixmandatory whenisFlannelis true (see follow‑up suggestion below).
5329-5331: Current isFlannel validation is correct; recommended immutability and hostPrefix guardrails are not implemented.The existing rule correctly restricts
isFlannel: truetonetworkType: 'Other'and is consistently applied across all CRD variants. The KCM integration properly sets--allocate-node-cidrs=truewhen enabled. However, the suggested immutability rule and hostPrefix requirement are not present in the current implementation:- message: isFlannel can only be set to true when networkType is 'Other' rule: '!has(self.networking) || !has(self.networking.isFlannel) || !self.networking.isFlannel || self.networking.networkType == ''Other''' + - message: isFlannel is immutable after creation + rule: '!has(oldSelf.networking) || self.networking.isFlannel == oldSelf.networking.isFlannel' + - message: clusterNetwork.hostPrefix must be set when isFlannel is true + rule: '!has(self.networking) || !has(self.networking.isFlannel) || !self.networking.isFlannel + || (has(self.networking.clusterNetwork) && self.networking.clusterNetwork.all(c, has(c.hostPrefix)))'Add these validations to
api/hypershift/v1beta1/hostedcluster_types.goandapi/hypershift/v1beta1/hosted_controlplane.goas decorators on the HostedClusterSpec and HostedControlPlaneSpec types respectively. Regenerate the CRD manifests afterward.api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/NetworkDiagnosticsConfig.yaml (1)
5463-5466: LGTM on the cross‑field gate (isFlannel ⇒ networkType=='Other').Validation is correct and narrowly scoped. Minor: adjust the message to reference “spec.networking.networkType” for consistency.
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yaml (1)
3105-3113: MakeisFlannelimmutable and validate at the networking scope
- Prevent risky day-2 flips: add an immutability check for
networking.isFlannelalongside other immutable networking fields.- Optionally co-locate the constraint “isFlannel ⇒ networkType==Other” under
spec.networking.x-kubernetes-validationsfor cohesion (you can keep the root rule or move it).Suggested additions under
spec.properties.networking.x-kubernetes-validations:x-kubernetes-validations: - message: CIDR ranges in machineNetwork, clusterNetwork, and serviceNetwork must be unique and non-overlapping rule: (!has(self.machineNetwork) && self.clusterNetwork.all(c, self.serviceNetwork.all(s, c.cidr != s.cidr)) || (has(self.machineNetwork) && (self.machineNetwork.all(m, self.clusterNetwork.all(c, m.cidr != c.cidr)) && self.machineNetwork.all(m, self.serviceNetwork.all(s, m.cidr != s.cidr)) && self.clusterNetwork.all(c, self.serviceNetwork.all(s, c.cidr != s.cidr))))) + - message: isFlannel is immutable + rule: !has(oldSelf.isFlannel) || self.isFlannel == oldSelf.isFlannel + - message: isFlannel can only be set to true when networkType is 'Other' + rule: '!has(self.isFlannel) || !self.isFlannel || self.networkType == ''Other'''api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AutoNodeKarpenter.yaml (2)
5184-5186: Validation looks good; also require hostPrefix when isFlannel=true.Current rule correctly ties isFlannel to networkType=Other. Add a CEL to ensure at least one clusterNetwork entry has hostPrefix when KCM allocates Node CIDRs.
Apply this diff alongside the existing validation:
- message: isFlannel can only be set to true when networkType is 'Other' rule: '!has(self.networking) || !has(self.networking.isFlannel) || !self.networking.isFlannel || self.networking.networkType == ''Other''' + - message: When isFlannel is true, at least one clusterNetwork entry must set hostPrefix + rule: '!has(self.networking) || !has(self.networking.isFlannel) || !self.networking.isFlannel + || (has(self.networking.clusterNetwork) && self.networking.clusterNetwork.exists(c, has(c.hostPrefix)))'If Multus must be disabled for Flannel in your design, consider a follow‑up rule linking isFlannel=true with operatorConfiguration.clusterNetworkOperator.disableMultiNetwork=true.
2662-2670: Field path is consistent; consider immutability for hardening.Path verification complete:
spec.networking.isFlannelis uniformly used across all HostedCluster and HostedControlPlane CRD variants, Go types, and tests. No inconsistencies found.The immutability suggestion remains valid: while networkType restriction prevents invalid combinations, adding
x-kubernetes-validations: [rule: self == oldSelf]would prevent day-2 flips once set—a safety measure used on similar fields. This is optional hardening.api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/KMSEncryptionProvider.yaml (1)
5387-5390: Rule looks good; consider enforcing required inputs when isFlannel=true.The cross‑field guard for networkType=='Other' is correct. Recommend adding a follow‑up validation to ensure inputs needed for KCM CIDR allocation are present (e.g., hostPrefix), if that’s a requirement for your Flannel path. Example:
- message: isFlannel can only be set to true when networkType is 'Other' rule: '!has(self.networking) || !has(self.networking.isFlannel) || !self.networking.isFlannel || self.networking.networkType == ''Other''' + - message: When isFlannel is true, clusterNetwork[0].hostPrefix must be set + rule: '!has(self.networking) || !has(self.networking.isFlannel) || !self.networking.isFlannel || (has(self.networking.clusterNetwork) && self.networking.clusterNetwork.size() > 0 && has(self.networking.clusterNetwork[0].hostPrefix))'If hostPrefix isn’t required because you default KCM node‑CIDR mask elsewhere, ignore this and keep the current rule.
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yaml (1)
2707-2715: Harden isFlannel with immutability + require hostPrefix when enabled
- Make isFlannel immutable to avoid risky day‑2 flips of networking behavior.
- When using KCM for node CIDR allocation (isFlannel=true), require hostPrefix on each clusterNetwork entry to ensure per‑node CIDR sizing is defined.
- Minor: in the description, refer to “networkType” (lowercase) to match the field name.
Apply these schema additions under the existing networking-level validations (near the current x-kubernetes-validations for CIDR overlap):
type: object x-kubernetes-validations: - message: CIDR ranges in machineNetwork, clusterNetwork, and serviceNetwork must be unique and non-overlapping rule: (!has(self.machineNetwork) && self.clusterNetwork.all(c, self.serviceNetwork.all(s, c.cidr != s.cidr)) || (has(self.machineNetwork) && (self.machineNetwork.all(m, self.clusterNetwork.all(c, m.cidr != c.cidr)) && self.machineNetwork.all(m, self.serviceNetwork.all(s, m.cidr != s.cidr)) && self.clusterNetwork.all(c, self.serviceNetwork.all(s, c.cidr != s.cidr))))) + - message: isFlannel is immutable + rule: '!has(oldSelf.isFlannel) || self.isFlannel == oldSelf.isFlannel' + - message: When isFlannel is true, each clusterNetwork entry must define hostPrefix + rule: '!self.isFlannel || self.clusterNetwork.all(c, has(c.hostPrefix))'And update the field help text (typo-level tweak):
- This field can only be set to true when NetworkType is "Other". Setting it to true + This field can only be set to true when networkType is "Other". Setting it to trueapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yaml (1)
2716-2724: Clarify wording; avoid acronym; keep field doc crisp.Prefer “kube-controller-manager” over “KCM” and tighten phrasing.
isFlannel: default: false description: |- - isFlannel – Set this variable to "true" when using networkType=Other with Flannel as the CNI. - This enables the KCM to handle node CIDR allocation. + isFlannel – Set to "true" when using networkType=Other with Flannel as the CNI. + When true, the kube-controller-manager handles node CIDR allocation. Default is false. This field can only be set to true when NetworkType is "Other". Setting it to true with any other NetworkType will result in a validation error during cluster creation. type: booleancmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Default.crd.yaml (1)
3378-3386: Add immutability for isFlannel and consider requiring hostPrefix when enabled
- Recommend making isFlannel immutable to avoid unsafe day‑2 flips that switch CIDR allocation mode.
- If KCM allocates node CIDRs for Flannel here, require hostPrefix on each clusterNetwork entry when isFlannel=true.
Proposed diffs (update types and regenerate CRDs; shown here against the generated YAML for clarity):
Immutability at the field node:
isFlannel: default: false description: |- isFlannel – Set this variable to "true" when using networkType=Other with Flannel as the CNI. This enables the KCM to handle node CIDR allocation. Default is false. This field can only be set to true when NetworkType is "Other". Setting it to true with any other NetworkType will result in a validation error during cluster creation. type: boolean + x-kubernetes-validations: + - message: isFlannel is immutable + rule: self == oldSelfHostPrefix requirement at the spec.x-kubernetes-validations level:
- message: isFlannel can only be set to true when networkType is 'Other' rule: '!has(self.networking) || !has(self.networking.isFlannel) || !self.networking.isFlannel || self.networking.networkType == ''Other''' + - message: when isFlannel is true, clusterNetwork.hostPrefix must be set for all entries + rule: '!has(self.networking) || !has(self.networking.isFlannel) || !self.networking.isFlannel + || (has(self.networking.clusterNetwork) && self.networking.clusterNetwork.all(c, has(c.hostPrefix)))'If controllers already support safe day‑2 flips or derive mask size elsewhere, please confirm and we can skip the above. As per coding guidelines.
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/GCPPlatform.yaml (1)
2615-2623: Make isFlannel immutable (avoid risky day‑2 flips).Given this toggles KCM node CIDR allocation behavior, it should not change post‑creation.
Apply this diff under the isFlannel property:
isFlannel: default: false description: |- isFlannel – Set this variable to "true" when using networkType=Other with Flannel as the CNI. This enables the KCM to handle node CIDR allocation. Default is false. This field can only be set to true when NetworkType is "Other". Setting it to true with any other NetworkType will result in a validation error during cluster creation. type: boolean + x-kubernetes-validations: + - message: isFlannel is immutable + rule: self == oldSelfapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AAA_ungated.yaml (1)
5154-5156: Cross-field validation LGTM; consider guardrails only if required.Rule correctly gates
isFlannel: truetonetworkType: "Other". If product requires additional constraints when Flannel is enabled (e.g., specific clusterNetwork/hostPrefix expectations or dual-stack handling), we can add a follow-up CEL rule; otherwise this is good to ship.If additional constraints are desired, please confirm and I can propose a CEL rule accordingly.
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yaml (1)
5358-5361: Validation LGTM; optional simplificationRule is correct. Since spec.networking is required, the leading check
!has(self.networking)is redundant. You can simplify without behavior change:- rule: '!has(self.networking) || !has(self.networking.isFlannel) || !self.networking.isFlannel - || self.networking.networkType == ''Other''' + rule: '!has(self.networking.isFlannel) || !self.networking.isFlannel || self.networking.networkType == ''Other'''api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/OpenStack.yaml (1)
5688-5691: Validation is good; tighten wording and ensure naming consistency.Rule correctly gates truthiness to networkType=='Other'. Minor nits:
- Message/description use “networkType” in code but “NetworkType” in prose. Align to “networkType”.
- If dual‑stack with Flannel is unsupported, add a companion rule to forbid isFlannel=true when multiple service/cluster networks are set. I can draft that once support matrix is confirmed.
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/DynamicResourceAllocation.yaml (1)
5332-5335: Tighten validation when isFlannel=true: require hostPrefix on clusterNetwork.KCM node CIDR allocation typically needs a per‑node mask; enforce hostPrefix presence when isFlannel is true.
Apply this additional spec-level validation alongside the existing rule:
- message: isFlannel can only be set to true when networkType is 'Other' rule: '!has(self.networking) || !has(self.networking.isFlannel) || !self.networking.isFlannel || self.networking.networkType == ''Other''' + - message: when isFlannel is true, each clusterNetwork entry must set hostPrefix + rule: '!has(self.networking) || !has(self.networking.isFlannel) || !self.networking.isFlannel + || (has(self.networking.clusterNetwork) && self.networking.clusterNetwork.all(c, has(c.hostPrefix)))'Please confirm this assumption matches your Flannel/KCM wiring and tests cover it. If hostPrefix is derived elsewhere, ignore this suggestion.
docs/content/reference/api.md (1)
4640-4655: Tighten wording and align with field paths; explicitly name kube-controller-managerRecommend clarifying the semantics and constraints and using the full field path for consistency. Proposed replacement keeps the same meaning and avoids “variable” wording:
-<p>isFlannel – Set this variable to “true” when using networkType=Other with Flannel as the CNI. -This enables the KCM to handle node CIDR allocation. -Default is false. -This field can only be set to true when NetworkType is “Other”. Setting it to true -with any other NetworkType will result in a validation error during cluster creation.</p> +<p>isFlannel — Set to true when using Flannel as the CNI with <code>networking.networkType</code> set to "Other". +When true, the kube-controller-manager enables node CIDR allocation (<code>allocate-node-cidrs</code>). +Default is false. +Valid only when <code>networking.networkType</code> is "Other"; otherwise validation fails at cluster creation.</p>Also, please confirm and document if:
- Dual-stack/IPv6 is supported with this setting.
- The field is mutable after creation or effectively immutable due to control-plane behavior.
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yaml (1)
2707-2715: Make isFlannel immutable; fix doc casing of networkType
- Flipping networking knobs day‑2 is generally unsafe. Align with nearby networking immutables by making isFlannel immutable.
- In the description, use “networkType” (lowercase “n”) to match the field name used elsewhere.
Apply:
isFlannel: default: false description: |- - isFlannel – Set this variable to "true" when using networkType=Other with Flannel as the CNI. + isFlannel – Set this variable to "true" when using networkType=Other with Flannel as the CNI. This enables the KCM to handle node CIDR allocation. Default is false. - This field can only be set to true when NetworkType is "Other". Setting it to true + This field can only be set to true when networkType is "Other". Setting it to true with any other NetworkType will result in a validation error during cluster creation. type: boolean + x-kubernetes-validations: + - message: isFlannel is immutable + rule: self == oldSelfIf day‑2 toggling is intended, please confirm that controllers tolerate switching NodeCIDR allocation without disruption; otherwise the immutability gate should be kept. Would you like a follow‑up PR to add/update unit tests around spec immutability?
cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-TechPreviewNoUpgrade.crd.yaml (2)
3333-3341: Add immutability + hostPrefix guard for Flannel path
- Make isFlannel immutable to avoid day‑2 flips that can break networking.
- When isFlannel=true, require clusterNetwork[].hostPrefix to be set (all entries, incl. dual‑stack), so KCM can allocate node CIDRs deterministically.
Place under spec.networking.x-kubernetes-validations (not on the boolean field), e.g.:
# under spec.networking: x-kubernetes-validations: - message: isFlannel is immutable rule: '!has(oldSelf.isFlannel) || self.isFlannel == oldSelf.isFlannel' - message: When isFlannel is true, hostPrefix must be set on all clusterNetwork entries rule: '!has(self.isFlannel) || !self.isFlannel || (has(self.clusterNetwork) && self.clusterNetwork.all(cn, has(cn.hostPrefix)))'Also, in the description, use “networkType” (field name casing) instead of “NetworkType”.
6509-6511: Scope the CEL to spec.networking for consistency; keep rule identicalCurrent rule is correct but lives at spec root while similar checks sit under spec.networking. Prefer:
# move under spec.networking.x-kubernetes-validations - message: isFlannel can only be set to true when networkType is 'Other' rule: '!has(self.isFlannel) || !self.isFlannel || self.networkType == ''Other'''Then drop the spec‑level duplicate to avoid drift. If you keep it at spec root, no functional change, but local scoping is clearer and aligns with existing networking validations in this CRD.
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yaml (1)
3043-3051: Make isFlannel immutable to prevent day‑2 drift.Flipping this flag after install can desync networking and KCM behavior. Guard it with an immutability CEL on the boolean node.
isFlannel: default: false description: |- isFlannel – Set this variable to "true" when using networkType=Other with Flannel as the CNI. This enables the KCM to handle node CIDR allocation. Default is false. This field can only be set to true when NetworkType is "Other". Setting it to true with any other NetworkType will result in a validation error during cluster creation. type: boolean + x-kubernetes-validations: + - message: isFlannel is immutable + rule: self == oldSelfcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-TechPreviewNoUpgrade.crd.yaml (1)
6683-6686: Gating rule looks correct; add complementary hostPrefix check nearbyThe CEL rule properly restricts isFlannel=true to networkType='Other'. Recommend adding a sibling rule to require hostPrefix when isFlannel=true so KCM NodeCIDR can function deterministically (covers single/dual‑stack by checking all entries).
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yaml (1)
3197-3205: Make isFlannel immutable to prevent risky day‑2 flips.Flipping this after creation can disrupt KCM/node CIDR behavior. Add a per‑field immutability rule and tighten the description to reference the exact field name.
Apply:
isFlannel: default: false description: |- - isFlannel – Set this variable to "true" when using networkType=Other with Flannel as the CNI. + isFlannel – Set this to true only when using networking.networkType="Other" with Flannel as the CNI. This enables the KCM to handle node CIDR allocation. Default is false. - This field can only be set to true when NetworkType is "Other". Setting it to true - with any other NetworkType will result in a validation error during cluster creation. + This field can only be set to true when networking.networkType is "Other". + Setting it to true with any other value will fail validation. type: boolean + x-kubernetes-validations: + - message: isFlannel is immutable + rule: self == oldSelfOptionally (only if required by KCM behavior), enforce hostPrefix when isFlannel=true:
+ - message: When isFlannel is true, each clusterNetwork entry must set hostPrefix + rule: '!has(self.networking) || !has(self.networking.isFlannel) || !self.networking.isFlannel + || (has(self.networking.clusterNetwork) && self.networking.clusterNetwork.all(n, has(n.hostPrefix)))'api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDC.yaml (1)
5473-5476: Gating looks good; add CEL to require hostPrefix when isFlannel=trueYour rule correctly restricts isFlannel to networkType="Other". For KCM-based node CIDR allocation, ensure clusterNetwork entries define hostPrefix when isFlannel=true.
Append this validation alongside the existing ones:
- message: isFlannel can only be set to true when networkType is 'Other' rule: '!has(self.networking) || !has(self.networking.isFlannel) || !self.networking.isFlannel || self.networking.networkType == ''Other''' + - message: When isFlannel is true, each clusterNetwork entry must set hostPrefix + rule: '!has(self.networking) || !has(self.networking.isFlannel) || !self.networking.isFlannel + || (has(self.networking.clusterNetwork) && self.networking.clusterNetwork.all(c, has(c.hostPrefix)))'cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Default.crd.yaml (1)
3286-3294: Add immutability and tighten the description.
- Recommend making isFlannel immutable to avoid risky day‑2 flips of KCM node CIDR allocation.
- The description says “validation error during cluster creation”; CEL validation applies on updates too. Suggest rewording.
- Optional: consider aligning the name with existing boolean style (e.g., flannelEnabled or useFlannel).
Apply this under the isFlannel property:
isFlannel: default: false description: |- - isFlannel – Set this variable to "true" when using networkType=Other with Flannel as the CNI. + isFlannel - Set to "true" only when using networkType=Other with Flannel as the CNI. This enables the KCM to handle node CIDR allocation. Default is false. - This field can only be set to true when NetworkType is "Other". Setting it to true - with any other NetworkType will result in a validation error during cluster creation. + This field can only be set to true when networkType is "Other". Setting it to true + with any other networkType will be rejected by validation. type: boolean + x-kubernetes-validations: + - message: isFlannel is immutable + rule: self == oldSelf
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (3)
vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hosted_controlplane.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (36)
api/hypershift/v1beta1/hosted_controlplane.go(1 hunks)api/hypershift/v1beta1/hostedcluster_types.go(2 hunks)api/hypershift/v1beta1/zz_generated.deepcopy.go(1 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/DynamicResourceAllocation.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ImageStreamImportMode.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/KMSEncryptionProvider.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/NetworkDiagnosticsConfig.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AAA_ungated.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AutoNodeKarpenter.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/DynamicResourceAllocation.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDC.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/GCPPlatform.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ImageStreamImportMode.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/KMSEncryptionProvider.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/NetworkDiagnosticsConfig.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/OpenStack.yaml(2 hunks)cmd/cluster/core/create.go(4 hunks)cmd/cluster/core/create_test.go(2 hunks)cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-CustomNoUpgrade.crd.yaml(2 hunks)cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Default.crd.yaml(2 hunks)cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-TechPreviewNoUpgrade.crd.yaml(2 hunks)cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-CustomNoUpgrade.crd.yaml(2 hunks)cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Default.crd.yaml(2 hunks)cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-TechPreviewNoUpgrade.crd.yaml(2 hunks)control-plane-operator/controllers/hostedcontrolplane/v2/kcm/deployment.go(1 hunks)docs/content/reference/api.md(1 hunks)test/e2e/create_cluster_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
cmd/cluster/core/create_test.gotest/e2e/create_cluster_test.goapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/NetworkDiagnosticsConfig.yamlcontrol-plane-operator/controllers/hostedcontrolplane/v2/kcm/deployment.goapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ImageStreamImportMode.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AutoNodeKarpenter.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ImageStreamImportMode.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/KMSEncryptionProvider.yamlapi/hypershift/v1beta1/hosted_controlplane.goapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/DynamicResourceAllocation.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/DynamicResourceAllocation.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yamlcmd/cluster/core/create.gocmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-CustomNoUpgrade.crd.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Default.crd.yamldocs/content/reference/api.mdapi/hypershift/v1beta1/hostedcluster_types.gocmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-TechPreviewNoUpgrade.crd.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Default.crd.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDC.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/GCPPlatform.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/KMSEncryptionProvider.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yamlapi/hypershift/v1beta1/zz_generated.deepcopy.goapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-TechPreviewNoUpgrade.crd.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-CustomNoUpgrade.crd.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AAA_ungated.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/NetworkDiagnosticsConfig.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/OpenStack.yaml
🧬 Code graph analysis (3)
cmd/cluster/core/create_test.go (2)
cmd/cluster/core/create.go (3)
RawCreateOptions(128-192)CreateOptions(797-800)ValidatedCreateOptions(658-661)api/hypershift/v1beta1/hostedcluster_types.go (1)
NetworkType(1162-1162)
test/e2e/create_cluster_test.go (1)
api/hypershift/v1beta1/hostedcluster_types.go (3)
NetworkType(1162-1162)Other(1175-1175)OVNKubernetes(1172-1172)
cmd/cluster/core/create.go (1)
api/hypershift/v1beta1/hostedcluster_types.go (1)
NetworkType(1162-1162)
🔇 Additional comments (27)
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/NetworkDiagnosticsConfig.yaml (1)
2859-2867: Target the Go source file, not the generated YAML; apply immutability and documentation improvements toapi/hypershift/v1beta1/hostedcluster_types.go.The YAML file is auto-generated; any manual changes will be lost on regeneration. The IsFlannel field definition must be updated in the Go struct to include immutability markers and improved wording.
Update the IsFlannel field in
ClusterNetworkingstruct:- // isFlannel – Set this variable to "true" when using networkType=Other with Flannel as the CNI. - // This enables the KCM to handle node CIDR allocation. - // Default is false. - // This field can only be set to true when NetworkType is "Other". Setting it to true - // with any other NetworkType will result in a validation error during cluster creation. - // - // +optional - // +kubebuilder:default:=false - IsFlannel *bool `json:"isFlannel,omitempty"` // nolint:kubeapilinter + // isFlannel - Set to true when networking.networkType is "Other" and using Flannel as the CNI. + // Enables the kube-controller-manager (KCM) to allocate node CIDRs. Default is false. + // This field is immutable once set. + // + // +optional + // +kubebuilder:default:=false + // +immutable + // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="isFlannel is immutable and cannot be modified once set." + IsFlannel *bool `json:"isFlannel,omitempty"` // nolint:kubeapilinterThe existing XValidation rule already enforces that isFlannel can only be true when networkType is 'Other', so no additional validation is needed for that constraint.
Likely an incorrect or invalid review comment.
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yaml (1)
5627-5630: Verification confirms proper propagation across all CRD variants—no action neededThe
isFlannelfield and its CEL validation rule are correctly present and consistent across all generated CRD manifests:
- Feature-gated variants: 11 HostedControlPlanes + 11 HostedClusters manifest files ✓
- Standard CRD variants: 3 HostedControlPlanes + 3 HostedClusters manifest files ✓
- Validation rule identical everywhere:
!has(self.networking) || !has(self.networking.isFlannel) || !self.networking.isFlannel || self.networking.networkType == ''Other'''✓No drift detected between standard and feature-gated manifests.
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yaml (1)
5862-5864: Validation rule is consistently present across all platform variant CRDsVerification confirms the
isFlannelvalidation rule is correctly applied across all platform variant CRDs in both thehostedclusters.hypershift.openshift.ioandhostedcontrolplanes.hypershift.openshift.iodirectories, sourced from the kubebuilder annotations inhostedcluster_types.goandhosted_controlplane.go. The rule is properly propagated to all 31 generated CRD manifests. No issues found.api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yaml (1)
5331-5334: Cross-field gating looks correctThe CEL rule cleanly enforces isFlannel=true only with networkType='Other'. LGTM. Please ensure unit tests cover both create and update paths (including toggling networkType or isFlannel).
cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-CustomNoUpgrade.crd.yaml (1)
6772-6774: Validation coverage confirmed across all CRD variants and Go types.The verification confirms the isFlannel validation is properly implemented:
- ✓ Field defined consistently in Go types (
api/hypershift/v1beta1/hostedcluster_types.go,hosted_controlplane.go)- ✓ Identical CEL rule across all HostedCluster CRD variants (Default, CustomNoUpgrade, TechPreviewNoUpgrade)
- ✓ Identical CEL rule across all HostedControlPlane CRD variants (Default, CustomNoUpgrade, TechPreviewNoUpgrade)
- ✓ Featuregated CRD manifests also include the validation
- ✓ CLI-level validation in
cmd/cluster/core/create.go- ✓ E2E test coverage for the validation scenarios
The guard correctly ties
isFlanneltonetworkType='Other'consistently throughout the codebase.api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yaml (1)
5157-5160: Field definitions and markers are in sync.The Go type at
api/hypershift/v1beta1/hostedcluster_types.go:1089correctly declaresIsFlannel *boolwith JSON tagjson:"isFlannel,omitempty"and markers// +optionaland// +kubebuilder:default:=false. The field appears in all generated CRD manifests with the CEL validation rule, and the deepcopy generator includes it. The CEL constraint (isFlannel only true when networkType == 'Other') matches the CLI validation atcmd/cluster/core/create.go:781–782. No risk of codegen dropping the field.api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/NetworkDiagnosticsConfig.yaml (1)
5289-5291: Cross-field gate looks correct; please ensure parity across CRDsThe rule correctly restricts isFlannel=true to networkType='Other'. Confirm the same constraint exists on the HostedCluster CRD and any mirrored schemas to avoid drift.
cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Default.crd.yaml (1)
5990-5993: Validation rule LGTMThe cross‑field guard restricting isFlannel=true to networkType='Other' is correct and clear.
Please confirm whether additional guards are desired (e.g., requiring hostPrefix as suggested above) for the intended KCM allocation flow.
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/KMSEncryptionProvider.yaml (1)
5213-5215: Verification complete: isFlannel validation rule consistently appliedThe cross-field validation is correctly present and consistent across the codebase. The CEL rule gates
isFlannel=truetonetworkType='Other'in:
- Source API types:
hosted_controlplane.go(line 44),hostedcluster_types.go(line 490)- All generated CRD manifests for both hostedcontrolplanes and hostedclusters across all feature gate variants
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/GCPPlatform.yaml (1)
5230-5233: Cross‑field validation confirmed across all CRD variants; no API drift detected.Verification complete: The isFlannel gating rule is present in all 22 feature-gated CRD manifests (11 HostedControlPlane + 11 HostedCluster variants) and all 6 cmd/install asset CRDs. The rule correctly enforces the constraint in both source annotations and generated manifests. Test coverage validates the validation logic. No additional action required.
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yaml (1)
2754-2762: Apply immutability to isFlannel in source Go code; clarify hostPrefix requirementThe suggestions target a generated YAML file that will be overwritten during code generation. Changes must go in the source:
api/hypershift/v1beta1/hostedcluster_types.go, specifically theIsFlannelfield in theClusterNetworkingstruct.Immutability suggestion is valid: IsFlannel lacks the
+immutabletag and CEL validation rule (self == oldSelf) that protect other network fields (machineNetwork, clusterNetwork, serviceNetwork), creating inconsistency and allowing potentially destabilizing day-2 changes.HostPrefix requirement needs clarification: The hostPrefix field in
ClusterNetworkEntryis marked+optionalwith a comment stating it "can be left unset." Adding a validation to require hostPrefix when isFlannel=true may be overly restrictive. Confirm whether KCM's built-in defaults (node-cidr-mask-size flags) make this validation necessary, or if this is defensive programming without corresponding operational need.Apply these changes to
api/hypershift/v1beta1/hostedcluster_types.go:
- Add immutability markers to IsFlannel field to match machineNetwork/clusterNetwork pattern
- If hostPrefix requirement is confirmed necessary, add CEL validation at the
ClusterNetworkinglevelThen regenerate the CRD manifests using the project's codegen tooling.
control-plane-operator/controllers/hostedcontrolplane/v2/kcm/deployment.go (1)
43-62: Flannel node CIDR allocation flag handling looks correct and scopedThe logic cleanly enforces
--allocate-node-cidrs=truewhenIsFlannelis set, without perturbing non-Flannel cases and without duplicating conflicting flags. This aligns well with the PR’s intent.api/hypershift/v1beta1/zz_generated.deepcopy.go (1)
1095-1099: DeepCopy forIsFlannelmatches existing generated patternsThe new
IsFlanneldeepcopy block correctly handles the optional bool pointer and is consistent with other pointer fields in this file.api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/DynamicResourceAllocation.yaml (1)
5158-5161: Verification complete; CEL gate wiring confirmed across all layers.The review verification confirms:
✅ KCM config consumption:
control-plane-operator/controllers/hostedcontrolplane/v2/kcm/deployment.gocorrectly sets--allocate-node-cidrs=truewhenisFlannel=true.✅ CRD consistency: The CEL validation rule is identical across both HostedCluster and HostedControlPlane types and propagates to all variants (Default, CustomNoUpgrade, TechPreviewNoUpgrade, all feature-gated manifests).
✅ CLI/docs alignment:
cmd/cluster/core/create.goenforces the same restriction, comprehensive test coverage incmd/cluster/core/create_test.goandtest/e2e/create_cluster_test.go, and documentation indocs/content/reference/api.md.test/e2e/create_cluster_test.go (1)
782-810: LGTM! Comprehensive test coverage for isFlannel validation.The three test cases appropriately validate the isFlannel field behavior:
- False value (lines 782-790): Confirms that setting isFlannel to false always passes, regardless of networkType.
- Valid combination (lines 791-800): Verifies that isFlannel=true with networkType=Other passes validation.
- Invalid combination (lines 801-810): Ensures that isFlannel=true with a non-Other networkType (OVNKubernetes) correctly fails with the expected error message.
The tests follow existing patterns, use proper pointer semantics with
ptr.To(), and are well-placed within the networking validation section.cmd/cluster/core/create_test.go (2)
421-480: IsFlannel validation cases align with CLI validation logicThe added
TestValidatecases foris-flannelvsnetwork-typecorrectly mirror the newValidateguard (allowing onlyOtherwhen true and ensuring non-Othertypes fail with the expected message), so this coverage looks solid.
561-612: Flag-to-spec wiring test for IsFlannel is accurate
TestIsFlannelFlagnicely verifies the intended tri-state behavior for the CRD field (pointer set only when the flag is true, nil otherwise), matching the implementation inprototypeResources.cmd/cluster/core/create.go (2)
116-116: CLI flag and RawCreateOptions plumbing for IsFlannel are consistentThe new
--is-flannelflag andRawCreateOptions.IsFlannelfield integrate cleanly with the existing options pattern and defaulting; no functional or readability issues here.Also applies to: 182-182
477-479: IsFlannel wiring and validation correctly gate Flannel to networkType=OtherSetting
Spec.Networking.IsFlannelonly whenopts.IsFlannelis true (leaving it nil otherwise) matches the CRD defaulting intent, and theValidatecheck ensures this flag is only usable withnetwork-type=Other, which is exactly what the tests exercise.Also applies to: 781-783
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yaml (1)
5404-5407: Validation rule is correct and consistently applied across all provider CRDsThe verification confirms the isFlannel validation rule is correctly enforced in all 11 feature-gated CRD manifests with identical message and logic. Since these are generated files (zz_generated.featuregated-crd-manifests), the optional refactoring suggestion to colocate under networking is not applicable to manual PR changes.
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ImageStreamImportMode.yaml (1)
5155-5157: Cross-field validation verified across CRDs.The isFlannel constraint is correctly implemented and synchronized between HostedControlPlane and HostedCluster APIs. Both CRDs enforce the identical validation rule:
rule: '!has(self.networking) || !has(self.networking.isFlannel) || !self.networking.isFlannel || self.networking.networkType == ''Other'''This constraint appears consistently in:
- Source annotations (hosted_controlplane.go line 44, hostedcluster_types.go line 490)
- Generated CRD manifests for both resource types
- Test cases confirming expected behavior (e2e and unit tests)
No API drift detected.
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yaml (1)
5647-5649: I need to verify several technical claims in this review comment. Let me search for information about KCM's node-cidr-mask-size derivation and Flannel's hostPrefix requirements.Let me search for more specific information about HyperShift, the ExternalOIDC resource, and whether this is truly a generated file that should be edited.
Based on my verification through web research, I found critical issues with the review comment's technical rationale.
Critical technical claim is incorrect: KCM does not derive node-cidr-mask-size from hostPrefix
Kubernetes KCM uses cluster_cidr block and node-cidr-mask-size as separate control plane configuration settings, not derived from clusterNetwork hostPrefix. The node-cidr-mask-size is a KCM controller flag (default 24 for IPv4, 64 for IPv6), with separate flags for dual-stack IPv4/IPv6.
Additionally, hostPrefix validation is not required for non-OVN/SDN networkTypes and is explicitly skipped in those cases, which would include Flannel when
isFlannel=trueand networkType is 'Other'.The review comment's proposed validation rule is based on a misunderstanding of how Kubernetes allocates node CIDRs. While hostPrefix matters for the CNI plugin's subnet allocation, KCM allocates node CIDRs independently via its own configuration flags.
Regarding the generated file: HyperShift maintainers intentionally edit zz_generated.featuregated-crd-manifests files as part of their release process, so editing this file is appropriate for this codebase.
The proposed validation rule appears to add requirements that are neither technically necessary nor architecturally correct for Flannel networking.
Likely an incorrect or invalid review comment.
api/hypershift/v1beta1/hostedcluster_types.go (1)
490-490: LGTM!The validation rule correctly ensures
isFlannelcan only be set to true whennetworkTypeis 'Other'. The CEL logic properly handles all cases: allowing unset values, false values with any network type, and enforcing the constraint only when the field is explicitly set to true.api/hypershift/v1beta1/hosted_controlplane.go (1)
44-44: LGTM!The validation rule is consistent with the one added to
HostedClusterSpecand correctly enforces the same constraint onHostedControlPlaneSpec. This ensures that theisFlannelfield can only be set to true whennetworkTypeis 'Other' across both API resources.cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-TechPreviewNoUpgrade.crd.yaml (1)
3425-3433: Do not modify the generated CRD YAML directly; add immutability to the Go source instead.The file
cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-TechPreviewNoUpgrade.crd.yamlis auto-generated from the Go API type. The review suggests modifying the generated YAML to add immutability onisFlannel, but this approach has a critical flaw: manual changes will be overwritten on the next controller-gen run.The correct fix is in the Go source (
./api/hypershift/v1beta1/hostedcluster_types.go, line 1089). Similar immutable fields (clusterNetwork,serviceNetwork) define their validations as kubebuilder annotations at the source level (e.g.,+kubebuilder:validation:XValidation:rule="self == oldSelf"), which then generate the corresponding x-kubernetes-validations in the CRD YAML.Add the immutability validation to the
IsFlannelfield in hostedcluster_types.go alongside its existing+kubebuilder:default:=falsemarker, matching the pattern used forClusterNetworkandServiceNetwork. The hostPrefix validation requirement should similarly be added at the source level if intended to be permanent.Likely an incorrect or invalid review comment.
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yaml (1)
5801-5804: Validation rule for isFlannel consistently applied across all CRD variants.Verification confirms the constraint is present and identical in both generated CRD manifests:
- HostedClusters variant (lines 5801–5803)
- HostedControlPlanes variant (lines 5627–5629)
The rule correctly enforces that
isFlannelcan only be true whennetworkType == 'Other'. No inconsistencies detected.cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Default.crd.yaml (1)
5816-5818: CEL guard verified correct; KCM wiring and API/CRD symmetry confirmed.✓ Rule correctly gates
isFlannel=truetonetworkType=='Other'.
✓ KCM deployment (control-plane-operator/controllers/hostedcontrolplane/v2/kcm/deployment.go:44-62) properly enables--allocate-node-cidrs=truewhen isFlannel is set, and sources cluster and service CIDRs fromhcp.Spec.Networking.
✓ HostedCluster CRD (api/hypershift/v1beta1/hostedcluster_types.go:490) enforces identical constraint, matching HostedControlPlane (api/hypershift/v1beta1/hosted_controlplane.go:44). Both validation rules and messages are symmetric.Comprehensive test coverage validates the constraint across e2e, API, and CLI layers (test/e2e/create_cluster_test.go, cmd/cluster/core/create_test.go, test/e2e/v2/tests/api_ux_validation_test.go).
There was a problem hiding this comment.
Thanks for the contribution!. The implementation looks good overall, I would ask for the immutability of that field via CEL validation as I commented.
I have one concern with the backward compatibility. Let me elaborate the scenario:
- The CRD is updated to this version
- ROSA creates a HC with this field set to true
- For w/e reason ROSA needs to rollback the HO version to an older one
- The HO short circuit.
Another problematic scenario that comes to my mind:
- If the MGMT cluster contains the updated CRD but the CPO has vendored an old version and the field is not there
|
/retest-required |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/OpenStack.yaml (1)
2709-2719: Require hostPrefix when allocateNodeCIDRs is Enabled.Without per‑node hostPrefix, KCM node CIDR allocation is ill‑defined; enforce this at admission.
x-kubernetes-validations: - message: CIDR ranges in machineNetwork, clusterNetwork, and serviceNetwork must be unique and non-overlapping rule: (!has(self.machineNetwork) && self.clusterNetwork.all(c, self.serviceNetwork.all(s, c.cidr != s.cidr)) || (has(self.machineNetwork) && (self.machineNetwork.all(m, self.clusterNetwork.all(c, m.cidr != c.cidr)) && self.machineNetwork.all(m, self.serviceNetwork.all(s, m.cidr != s.cidr)) && self.clusterNetwork.all(c, self.serviceNetwork.all(s, c.cidr != s.cidr))))) - message: allocateNodeCIDRs can only be set to Enabled when networkType is 'Other' rule: 'has(self.allocateNodeCIDRs) && self.allocateNodeCIDRs == ''Enabled'' ? self.networkType == ''Other'' : true' + - message: hostPrefix must be set on all clusterNetwork entries when allocateNodeCIDRs is Enabled + rule: '!has(self.allocateNodeCIDRs) || self.allocateNodeCIDRs != ''Enabled'' + ? true + : (has(self.clusterNetwork) && self.clusterNetwork.all(c, has(c.hostPrefix) && c.hostPrefix > 0))'Please add the corresponding kubebuilder XValidation in the source types and regenerate CRDs.
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yaml (1)
2709-2720: Add guard: require hostPrefix when allocateNodeCIDRs is Enabled.Prevents runtime failures in KCM node CIDR allocation when per‑node mask is missing.
x-kubernetes-validations: - message: CIDR ranges in machineNetwork, clusterNetwork, and serviceNetwork must be unique and non-overlapping rule: (!has(self.machineNetwork) && self.clusterNetwork.all(c, self.serviceNetwork.all(s, c.cidr != s.cidr)) || (has(self.machineNetwork) && (self.machineNetwork.all(m, self.clusterNetwork.all(c, m.cidr != c.cidr)) && self.machineNetwork.all(m, self.serviceNetwork.all(s, m.cidr != s.cidr)) && self.clusterNetwork.all(c, self.serviceNetwork.all(s, c.cidr != s.cidr))))) - message: allocateNodeCIDRs can only be set to Enabled when networkType is 'Other' rule: 'has(self.allocateNodeCIDRs) && self.allocateNodeCIDRs == ''Enabled'' ? self.networkType == ''Other'' : true' + - message: allocateNodeCIDRs=Enabled requires hostPrefix on all clusterNetwork entries + rule: '!has(self.allocateNodeCIDRs) || self.allocateNodeCIDRs != ''Enabled'' + || (!has(self.clusterNetwork) ? false : self.clusterNetwork.all(cn, has(cn.hostPrefix)))'
♻️ Duplicate comments (17)
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/IngressControllerLBSubnetsAWS.yaml (1)
2526-2541: Vendor‑neutral wording + default/doc fix for allocateNodeCIDRs.
- Drop Flannel from the public API description.
- Either add a CRD default or remove “Default is 'Disabled'” from docs to avoid mismatch.
- Clarify immutability in text (set on create only), since the field-level rule forbids day‑2 changes.
Apply this schema diff (regenerate from Go/kubebuilder types):
- allocateNodeCIDRs: + allocateNodeCIDRs: + default: Disabled description: |- - allocateNodeCIDRs controls whether the kube-controller-manager manages node CIDR allocation. - When using networkType=Other, it is recommended to set this field to "Enabled" - if Flannel is used as the CNI, as it relies on this behavior. - Default is "Disabled". + allocateNodeCIDRs controls whether the kube-controller-manager allocates per-node PodCIDRs + from the cluster CIDR. When using networkType=Other, set this to "Enabled" for CNIs that + require controller-managed node CIDR allocation. + This field is immutable after creation. This field can only be set to "Enabled" when NetworkType is "Other". Setting it to "Enabled" with any other NetworkType will result in a validation error during cluster creation. enum: - Enabled - Disabled type: string x-kubernetes-validations: - - message: allocateNodeCIDRs is immutable and cannot be modified - once set. + - message: allocateNodeCIDRs is immutable. rule: self == oldSelfIf you prefer “set-once after create” semantics, switch immutability to a type‑scoped rule under spec.networking.x‑kubernetes‑validations instead:
- message: allocateNodeCIDRs is immutable after first set. rule: '!has(oldSelf.allocateNodeCIDRs) || self.allocateNodeCIDRs == oldSelf.allocateNodeCIDRs'api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yaml (2)
2665-2673: De-flannelize and fix casing in description; keep guidance genericAvoid implying Flannel support and use “networkType” casing consistently. Proposed update:
allocateNodeCIDRs: description: |- - allocateNodeCIDRs controls whether the kube-controller-manager manages node CIDR allocation. - When using networkType=Other, it is recommended to set this field to "Enabled" - if Flannel is used as the CNI, as it relies on this behavior. - Default is "Disabled". - This field can only be set to "Enabled" when NetworkType is "Other". Setting it to "Enabled" - with any other NetworkType will result in a validation error during cluster creation. + allocateNodeCIDRs controls whether the kube-controller-manager allocates per-node CIDRs. + When using networkType="Other" with CNIs that rely on controller‑managed node CIDRs, + set this field to "Enabled". Default is "Disabled". + This field can only be set to "Enabled" when networkType is "Other". Setting it to "Enabled" + with any other networkType will result in a validation error during cluster creation.
2855-2858: Add CEL: require hostPrefix when controller allocates node CIDRsIf allocateNodeCIDRs is Enabled but clusterNetwork[].hostPrefix is unset, KCM lacks a per-node mask and will fail at runtime. Add a guard alongside existing networking-level validations:
x-kubernetes-validations: - message: CIDR ranges in machineNetwork, clusterNetwork, and serviceNetwork must be unique and non-overlapping rule: (!has(self.machineNetwork) && self.clusterNetwork.all(c, self.serviceNetwork.all(s, c.cidr != s.cidr)) || (has(self.machineNetwork) && (self.machineNetwork.all(m, self.clusterNetwork.all(c, m.cidr != c.cidr)) && self.machineNetwork.all(m, self.serviceNetwork.all(s, m.cidr != s.cidr)) && self.clusterNetwork.all(c, self.serviceNetwork.all(s, c.cidr != s.cidr))))) + - message: hostPrefix must be set on all clusterNetwork entries when allocateNodeCIDRs is "Enabled" + rule: '!has(self.allocateNodeCIDRs) || self.allocateNodeCIDRs != ''Enabled'' + ? true + : (has(self.clusterNetwork) && self.clusterNetwork.all(c, has(c.hostPrefix)))' - message: allocateNodeCIDRs can only be set to Enabled when networkType is 'Other' rule: 'has(self.allocateNodeCIDRs) && self.allocateNodeCIDRs == ''Enabled'' ? self.networkType == ''Other'' : true'(Optional, confirm before enforcing)
+ - message: When allocateNodeCIDRs is "Enabled", only a single clusterNetwork entry is supported + rule: '!has(self.allocateNodeCIDRs) || self.allocateNodeCIDRs != ''Enabled'' || size(self.clusterNetwork) == 1'api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yaml (1)
2618-2633: Immutability rule blocks first-time set; allow unset → set once.With
rule: self == oldSelf, users cannot setallocateNodeCIDRson day‑2 if it was omitted at creation. If the intent is “immutable after first set,” permit the initial transition.Apply:
- x-kubernetes-validations: - - message: allocateNodeCIDRs is immutable and cannot be modified - once set. - rule: self == oldSelf + x-kubernetes-validations: + - message: allocateNodeCIDRs is immutable after first set. + rule: oldSelf == null || self == oldSelfNote: The description states “Default is ‘Disabled’,” but no schema default is set. That’s fine if the controller treats nil as Disabled. If you plan to add a schema default later, be aware it will pre‑set the field and make it forever immutable under this rule. Consider keeping it nil-by-default to preserve day‑2 enablement.
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/KMSEncryptionProvider.yaml (1)
2602-2617: Make text generic (no Flannel) and set an explicit defaultRemove Flannel-specific guidance to avoid implying product support and add a schema default to match docs (“Default is Disabled”).
Apply:
allocateNodeCIDRs: - description: |- - allocateNodeCIDRs controls whether the kube-controller-manager manages node CIDR allocation. - When using networkType=Other, it is recommended to set this field to "Enabled" - if Flannel is used as the CNI, as it relies on this behavior. - Default is "Disabled". - This field can only be set to "Enabled" when NetworkType is "Other". Setting it to "Enabled" - with any other NetworkType will result in a validation error during cluster creation. + description: |- + allocateNodeCIDRs controls whether the kube-controller-manager allocates node CIDRs. + When using networkType=Other and the chosen CNI requires controller-managed node CIDRs, + set this field to "Enabled". Default is "Disabled". + This field can only be set to "Enabled" when NetworkType is "Other". Setting it to "Enabled" + with any other NetworkType will result in a validation error during cluster creation. + default: Disabled enum: - Enabled - Disabled type: string x-kubernetes-validations: - message: allocateNodeCIDRs is immutable and cannot be modified once set. rule: self == oldSelfapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yaml (1)
3108-3123: Make API CNI‑agnostic and add explicit default: Disabled.
- Remove the Flannel reference to avoid implying CNI support policy in a public API.
- Add
default: Disabledto align schema and controller defaults.Apply:
allocateNodeCIDRs: description: |- - allocateNodeCIDRs controls whether the kube-controller-manager manages node CIDR allocation. - When using networkType=Other, it is recommended to set this field to "Enabled" - if Flannel is used as the CNI, as it relies on this behavior. - Default is "Disabled". + Controls whether the kube-controller-manager manages per-node PodCIDR allocation. + Recommended only when networkType=Other and your CNI expects the control plane to allocate PodCIDRs. + Default is "Disabled". This field can only be set to "Enabled" when NetworkType is "Other". Setting it to "Enabled" with any other NetworkType will result in a validation error during cluster creation. enum: - Enabled - Disabled type: string + default: Disabled x-kubernetes-validations: - message: allocateNodeCIDRs is immutable and cannot be modified once set. rule: self == oldSelf#!/bin/bash # Verify consistency, defaults, and wording across the repo rg -nC3 --glob '!**/vendor/**' 'allocateNodeCIDRs' api/ | sed -n '1,200p' # Ensure no Flannel mentions remain in field descriptions rg -nC2 --glob '!**/vendor/**' 'allocateNodeCIDRs|Flannel' api/cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-TechPreviewNoUpgrade.crd.yaml (2)
3336-3351: Keep description provider‑agnostic; remove Flannel reference.Referencing Flannel implies support; make it generic to “CNIs that require KCM to allocate node CIDRs.” No functional change.
- description: |- - allocateNodeCIDRs controls whether the kube-controller-manager manages node CIDR allocation. - When using networkType=Other, it is recommended to set this field to "Enabled" - if Flannel is used as the CNI, as it relies on this behavior. - Default is "Disabled". - This field can only be set to "Enabled" when NetworkType is "Other". Setting it to "Enabled" - with any other NetworkType will result in a validation error during cluster creation. + description: |- + allocateNodeCIDRs controls whether the kube-controller-manager allocates node CIDRs. + For networkType=Other, set this to "Enabled" when the chosen CNI requires controller‑managed + node CIDR allocation. Default is "Disabled". + This field can only be set to "Enabled" when networkType is "Other"; attempting to enable it + with any other networkType will be rejected at validation time.
3526-3529: Add CEL to require hostPrefix when allocateNodeCIDRs == Enabled.Without hostPrefix on each clusterNetwork entry, KCM node CIDR allocation can misconfigure nodes. Enforce it alongside the existing gating.
x-kubernetes-validations: - message: CIDR ranges in machineNetwork, clusterNetwork, and serviceNetwork must be unique and non-overlapping rule: (!has(self.machineNetwork) && self.clusterNetwork.all(c, self.serviceNetwork.all(s, c.cidr != s.cidr)) || (has(self.machineNetwork) && (self.machineNetwork.all(m, self.clusterNetwork.all(c, m.cidr != c.cidr)) && self.machineNetwork.all(m, self.serviceNetwork.all(s, m.cidr != s.cidr)) && self.clusterNetwork.all(c, self.serviceNetwork.all(s, c.cidr != s.cidr))))) + - message: hostPrefix must be set on each clusterNetwork entry when allocateNodeCIDRs is Enabled + rule: '!has(self.allocateNodeCIDRs) || self.allocateNodeCIDRs != ''Enabled'' || (has(self.clusterNetwork) && self.clusterNetwork.all(c, has(c.hostPrefix)))' - message: allocateNodeCIDRs can only be set to Enabled when networkType is 'Other' rule: 'has(self.allocateNodeCIDRs) && self.allocateNodeCIDRs == ''Enabled'' ? self.networkType == ''Other'' : true'cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-CustomNoUpgrade.crd.yaml (1)
3516-3526: Require hostPrefix when allocateNodeCIDRs is Enabled.Without per‑node hostPrefix, KCM node CIDR allocation can fail at runtime. Add a cross‑field CEL to enforce hostPrefix on every clusterNetwork entry when Enabled.
x-kubernetes-validations: - message: CIDR ranges in machineNetwork, clusterNetwork, and serviceNetwork must be unique and non-overlapping rule: (!has(self.machineNetwork) && self.clusterNetwork.all(c, self.serviceNetwork.all(s, c.cidr != s.cidr)) || (has(self.machineNetwork) && (self.machineNetwork.all(m, self.clusterNetwork.all(c, m.cidr != c.cidr)) && self.machineNetwork.all(m, self.serviceNetwork.all(s, m.cidr != s.cidr)) && self.clusterNetwork.all(c, self.serviceNetwork.all(s, c.cidr != s.cidr))))) + - message: hostPrefix must be set (>0) on each clusterNetwork entry when allocateNodeCIDRs is Enabled + rule: '!has(self.allocateNodeCIDRs) || self.allocateNodeCIDRs != ''Enabled'' || (has(self.clusterNetwork) && self.clusterNetwork.all(c, has(c.hostPrefix) && c.hostPrefix > 0))' - message: allocateNodeCIDRs can only be set to Enabled when networkType is 'Other' rule: 'has(self.allocateNodeCIDRs) && self.allocateNodeCIDRs == ''Enabled'' ? self.networkType == ''Other'' : true'api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/OpenStack.yaml (1)
2526-2541: Make the field generic; add a default.Remove Flannel-specific guidance and set an explicit default to avoid day‑2 toggles via implicit nils.
allocateNodeCIDRs: description: |- - allocateNodeCIDRs controls whether the kube-controller-manager manages node CIDR allocation. - When using networkType=Other, it is recommended to set this field to "Enabled" - if Flannel is used as the CNI, as it relies on this behavior. - Default is "Disabled". - This field can only be set to "Enabled" when NetworkType is "Other". Setting it to "Enabled" - with any other NetworkType will result in a validation error during cluster creation. + allocateNodeCIDRs controls whether the kube-controller-manager manages node CIDR allocation. + Intended for third‑party CNIs when networking.networkType is "Other". + Default is "Disabled". This field is immutable after creation. + Enabling is only valid when networking.networkType is "Other". + default: Disabled enum: - Enabled - Disabled type: string x-kubernetes-validations: - message: allocateNodeCIDRs is immutable and cannot be modified once set. rule: self == oldSelfNote: update the Go types/kubebuilder markers and regenerate this CRD so the change persists.
cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-CustomNoUpgrade.crd.yaml (2)
3425-3441: Debrand field description and set explicit default to match docs.
- Remove Flannel from the description to avoid implying CNI support.
- The text says “Default is Disabled” but no default is specified; add default: "Disabled" for consistency.
allocateNodeCIDRs: description: |- - allocateNodeCIDRs controls whether the kube-controller-manager manages node CIDR allocation. - When using networkType=Other, it is recommended to set this field to "Enabled" - if Flannel is used as the CNI, as it relies on this behavior. - Default is "Disabled". + allocateNodeCIDRs controls whether the kube-controller-manager allocates node CIDRs. + When using networkType=Other, set this to "Enabled" for CNIs that rely on KCM‑managed + node CIDR assignment. + Default is "Disabled". This field can only be set to "Enabled" when NetworkType is "Other". Setting it to "Enabled" with any other NetworkType will result in a validation error during cluster creation. enum: - Enabled - Disabled type: string + default: Disabled x-kubernetes-validations: - message: allocateNodeCIDRs is immutable and cannot be modified once set. rule: self == oldSelf
3615-3619: Require hostPrefix when KCM allocates node CIDRs.Without hostPrefix, node block sizing is undefined and KCM allocation can misconfigure pods. Enforce hostPrefix on all clusterNetwork entries when allocateNodeCIDRs=="Enabled".
x-kubernetes-validations: - message: CIDR ranges in machineNetwork, clusterNetwork, and serviceNetwork must be unique and non-overlapping rule: (!has(self.machineNetwork) && self.clusterNetwork.all(c, self.serviceNetwork.all(s, c.cidr != s.cidr)) || (has(self.machineNetwork) && (self.machineNetwork.all(m, self.clusterNetwork.all(c, m.cidr != c.cidr)) && self.machineNetwork.all(m, self.serviceNetwork.all(s, m.cidr != s.cidr)) && self.clusterNetwork.all(c, self.serviceNetwork.all(s, c.cidr != s.cidr))))) + - message: hostPrefix must be set for all clusterNetwork entries when allocateNodeCIDRs is Enabled + rule: '!has(self.allocateNodeCIDRs) || self.allocateNodeCIDRs != ''Enabled'' || (has(self.clusterNetwork) && self.clusterNetwork.all(c, has(c.hostPrefix)))' - message: allocateNodeCIDRs can only be set to Enabled when networkType is 'Other' rule: 'has(self.allocateNodeCIDRs) && self.allocateNodeCIDRs == ''Enabled'' ? self.networkType == ''Other'' : true'api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/DynamicResourceAllocation.yaml (1)
2547-2562: Guard against KCM misconfig: require hostPrefix when allocation is enabled.If kube-controller-manager allocates node CIDRs, it needs a per-node mask. Enforce that every clusterNetwork entry defines hostPrefix when allocateNodeCIDRs == "Enabled".
Apply under spec.networking.x-kubernetes-validations:
x-kubernetes-validations: + - message: When allocateNodeCIDRs is Enabled, every clusterNetwork entry must define hostPrefix + rule: '!has(self.allocateNodeCIDRs) || self.allocateNodeCIDRs != "Enabled" ? true : (has(self.clusterNetwork) && self.clusterNetwork.all(c, has(c.hostPrefix)))'api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/NetworkDiagnosticsConfig.yaml (2)
2678-2686: Make description CNI‑agnostic and fix casing; avoid implying Flannel support.Replace Flannel‑specific guidance and "NetworkType" with generic wording and correct "networkType".
allocateNodeCIDRs: description: |- - allocateNodeCIDRs controls whether the kube-controller-manager manages node CIDR allocation. - When using networkType=Other, it is recommended to set this field to "Enabled" - if Flannel is used as the CNI, as it relies on this behavior. - Default is "Disabled". - This field can only be set to "Enabled" when NetworkType is "Other". Setting it to "Enabled" - with any other NetworkType will result in a validation error during cluster creation. + allocateNodeCIDRs controls whether the kube-controller-manager allocates node CIDRs. + When using networkType=Other, set this to "Enabled" for CNIs that require KCM-driven + node CIDR allocation. Default is "Disabled". + This field can only be set to "Enabled" when networkType is "Other". Setting it to "Enabled" + with any other networkType will result in a validation error during cluster creation. enum: - Enabled - Disabled type: string x-kubernetes-validations: - message: allocateNodeCIDRs is immutable and cannot be modified once set. rule: self == oldSelfAlso applies to: 2690-2694
2868-2871: Add CEL guard: require hostPrefix when allocateNodeCIDRs == "Enabled".Without clusterNetwork[0].hostPrefix, KCM allocation can misconfigure node CIDRs at runtime. Enforce via CEL under spec.networking validations.
x-kubernetes-validations: - message: CIDR ranges in machineNetwork, clusterNetwork, and serviceNetwork must be unique and non-overlapping rule: (!has(self.machineNetwork) && self.clusterNetwork.all(c, self.serviceNetwork.all(s, c.cidr != s.cidr)) || (has(self.machineNetwork) && (self.machineNetwork.all(m, self.clusterNetwork.all(c, m.cidr != c.cidr)) && self.machineNetwork.all(m, self.serviceNetwork.all(s, m.cidr != s.cidr)) && self.clusterNetwork.all(c, self.serviceNetwork.all(s, c.cidr != s.cidr))))) + - message: When allocateNodeCIDRs is Enabled, clusterNetwork[0].hostPrefix must be set for KCM node CIDR allocation + rule: '!has(self.allocateNodeCIDRs) || self.allocateNodeCIDRs != ''Enabled'' || (has(self.clusterNetwork) && self.clusterNetwork.size() >= 1 && has(self.clusterNetwork[0].hostPrefix))' - message: allocateNodeCIDRs can only be set to Enabled when networkType is 'Other' - rule: 'has(self.allocateNodeCIDRs) && self.allocateNodeCIDRs == ''Enabled'' ? self.networkType == ''Other'' : true' + rule: 'has(self.allocateNodeCIDRs) && self.allocateNodeCIDRs == ''Enabled'' ? self.networkType == ''Other'' : true'api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AutoNodeKarpenter.yaml (1)
2573-2589: Immutability is too strict; allow Disabled→Enabled day‑2 (keep Enabled→Disabled blocked)As written (self == oldSelf), a cluster created with the implicit/Default “Disabled” can never enable it later without recreation. If that’s not intentional, loosen the rule to forbid only turning it off once enabled. Also consider making the default explicit in the schema for clarity.
enum: - Enabled - Disabled type: string x-kubernetes-validations: - - message: allocateNodeCIDRs is immutable and cannot be modified - once set. - rule: self == oldSelf + - message: allocateNodeCIDRs cannot be disabled once enabled + rule: oldSelf == 'Enabled' ? self == 'Enabled' : true + # Optional but recommended to match docs/defaulting webhook: + # default: DisabledTo confirm defaulting exists in code paths beyond this CRD, search for webhook/defaulting that sets AllocateNodeCIDRs to Disabled:
#!/bin/bash rg -nP --hidden --glob '!**/vendor/**' -C2 '\bAllocateNodeCIDRs\b|allocateNodeCIDRs'cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Default.crd.yaml (1)
3197-3212: Enable-only semantics, default, and vendor-neutral wording for allocateNodeCIDRs
- Current immutability (
self == oldSelf) prevents day‑2 enablement. Prefer enable‑only: allow nil/Disabled → Enabled, forbid Enabled → Disabled.- The description mentions Flannel and “during cluster creation”; make it vendor‑neutral and applicable on create/update.
- The doc says “Default is Disabled” but no
default:is set.Proposed diff:
allocateNodeCIDRs: - description: |- - allocateNodeCIDRs controls whether the kube-controller-manager manages node CIDR allocation. - When using networkType=Other, it is recommended to set this field to "Enabled" - if Flannel is used as the CNI, as it relies on this behavior. - Default is "Disabled". - This field can only be set to "Enabled" when NetworkType is "Other". Setting it to "Enabled" - with any other NetworkType will result in a validation error during cluster creation. + description: |- + allocateNodeCIDRs controls whether the kube-controller-manager allocates per-node PodCIDRs. + When using networkType=Other, some CNIs require this to be Enabled. + Default is "Disabled". + May only be set to "Enabled" when networkType is "Other". + default: Disabled enum: - Enabled - Disabled type: string x-kubernetes-validations: - - message: allocateNodeCIDRs is immutable and cannot be modified once set. - rule: self == oldSelf + - message: allocateNodeCIDRs cannot be disabled once enabled. + rule: 'oldSelf == "Enabled" ? self == "Enabled" : true'Run to verify defaults, webhooks, and controller wiring:
#!/bin/bash # Locate API types, defaults, and controller wiring for AllocateNodeCIDRs rg -nP 'AllocateNodeCIDRs|allocateNodeCIDRs' -C3 rg -nP --type go 'Default.*AllocateNodeCIDRs|AllocateNodeCIDRs.*=' -C2 rg -nP --type go 'allocate-node-cidrs' -C3 # Check other CRDs for consistency (HostedCluster, etc.) fd -a 'hostedcluster.*crd.*yaml' | xargs -I{} rg -n 'allocateNodeCIDRs|networkType' {}
🧹 Nitpick comments (13)
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yaml (1)
3016-3031: Add schema default; make description generic; clarify immutability text.
- The text says “Default is Disabled” but the schema lacks a default. Add a JSONSchema default to avoid drift and to make immutability behavior deterministic.
- Per prior feedback, remove Flannel-specific mention; keep it generic as a KCM toggle.
- The rule enforces immutability after creation (unset→set on day-2 is blocked). If that’s intentional, adjust the message to avoid “once set” ambiguity.
Apply:
allocateNodeCIDRs: description: |- - allocateNodeCIDRs controls whether the kube-controller-manager manages node CIDR allocation. - When using networkType=Other, it is recommended to set this field to "Enabled" - if Flannel is used as the CNI, as it relies on this behavior. - Default is "Disabled". - This field can only be set to "Enabled" when NetworkType is "Other". Setting it to "Enabled" - with any other NetworkType will result in a validation error during cluster creation. + allocateNodeCIDRs controls whether the kube-controller-manager (-allocate-node-cidrs) + manages node CIDR allocation. Valid only when networkType is "Other". + Default is "Disabled". + This field is immutable after creation. enum: - Enabled - Disabled + default: Disabled type: string x-kubernetes-validations: - - message: allocateNodeCIDRs is immutable and cannot be modified once set. + - message: allocateNodeCIDRs is immutable after creation. rule: self == oldSelfBased on reviewer guidance to avoid implying Flannel support.
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AAA_ungated.yaml (1)
2535-2550: Neutralize vendor reference and set an explicit default for clarity.
- Make the description vendor‑neutral (avoid implying Flannel support).
- Either add
default: Disabledhere to match the stated default, or drop the sentence about the default if webhook defaulting is authoritative.Proposed manifest edits (update Go type doc/comments accordingly and re‑generate):
allocateNodeCIDRs: - description: |- - allocateNodeCIDRs controls whether the kube-controller-manager manages node CIDR allocation. - When using networkType=Other, it is recommended to set this field to "Enabled" - if Flannel is used as the CNI, as it relies on this behavior. - Default is "Disabled". + description: |- + allocateNodeCIDRs controls whether the kube-controller-manager (KCM) manages node CIDR allocation. + Some CNIs require controller-managed node CIDR allocation. When using networkType=Other, + set this to "Enabled" if your CNI requires it. + Default is "Disabled". + default: Disabled enum: - Enabled - Disabled type: string x-kubernetes-validations: - message: allocateNodeCIDRs is immutable and cannot be modified once set. rule: self == oldSelfIf webhook defaulting is already setting Disabled, you can omit the explicit default and keep the neutralized description; please confirm.
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDC.yaml (1)
2862-2877: Set schema default and keep docs generic; reference exact path
- The text says “Default is 'Disabled'” but no CRD default is set. Add a schema default for consistency.
- Avoid naming Flannel to not imply vendor support; make wording generic and reference networking.networkType.
Apply:
allocateNodeCIDRs: - description: |- - allocateNodeCIDRs controls whether the kube-controller-manager manages node CIDR allocation. - When using networkType=Other, it is recommended to set this field to "Enabled" - if Flannel is used as the CNI, as it relies on this behavior. - Default is "Disabled". - This field can only be set to "Enabled" when NetworkType is "Other". Setting it to "Enabled" - with any other NetworkType will result in a validation error during cluster creation. + description: |- + allocateNodeCIDRs controls whether the kube-controller-manager manages node CIDR allocation. + When using networking.networkType="Other", set this to "Enabled" for CNIs that rely on + Node CIDR allocation by the controller-manager. Default is "Disabled". + This field can only be "Enabled" when networking.networkType is "Other"; otherwise validation fails. enum: - Enabled - Disabled type: string + default: Disabled x-kubernetes-validations: - message: allocateNodeCIDRs is immutable and cannot be modified once set. rule: self == oldSelfdocs/content/reference/api.md (2)
2778-2800: Clarify KCM acronym on first mention.Expand “KCM” on first use for clarity.
-<p>AllocateNodeCIDRsMode specifies whether the KCM manages node CIDR allocation.</p> +<p>AllocateNodeCIDRsMode specifies whether the kube-controller-manager (KCM) allocates node CIDRs.</p>
4678-4696: Neutralize Flannel-specific phrasing and expand KCM; confirm immutability/hostPrefix note.Keep the guidance generic (avoid implying Flannel support), expand KCM, and use consistent casing for the field name.
-<p>allocateNodeCIDRs controls whether the kube-controller-manager manages node CIDR allocation. -When using networkType=Other, it is recommended to set this field to “Enabled” -if Flannel is used as the CNI, as it relies on this behavior. -Default is “Disabled”. -This field can only be set to “Enabled” when NetworkType is “Other”. Setting it to “Enabled” -with any other NetworkType will result in a validation error during cluster creation.</p> +<p>allocateNodeCIDRs controls whether the kube-controller-manager (KCM) allocates node CIDRs. +When using networkType="Other", enable this if your CNI requires KCM-based node CIDR allocation (for example, Flannel). +Default is "Disabled". +This field can be set to "Enabled" only when networkType is "Other"; using "Enabled" with any other networkType results in a validation error during cluster creation.</p>Please also confirm:
- Is allocateNodeCIDRs immutable after creation? If yes, add “This field is immutable.”
- Does enabling this require clusterNetwork.hostPrefix to be set (so KCM can derive the per‑node mask size)? If yes, add a brief note here.
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/GCPPlatform.yaml (1)
2526-2541: Add default and confirm immutability semantics
- The description says “Default is ‘Disabled’,” but the schema lacks a default. Add it for consistency and predictability.
- Current immutability rule
self == oldSelfforbids setting the field after creation if it was initially omitted. If that’s intended (creation-time only), fine; otherwise switch to “immutable once set” semantics.Suggested diff:
allocateNodeCIDRs: description: |- allocateNodeCIDRs controls whether the kube-controller-manager manages node CIDR allocation. When using networkType=Other, it is recommended to set this field to "Enabled" if Flannel is used as the CNI, as it relies on this behavior. - Default is "Disabled". + Default is "Disabled". This field can only be set to "Enabled" when NetworkType is "Other". Setting it to "Enabled" with any other NetworkType will result in a validation error during cluster creation. enum: - Enabled - Disabled type: string + default: Disabled x-kubernetes-validations: - - message: allocateNodeCIDRs is immutable and cannot be modified - once set. - rule: self == oldSelf + - message: allocateNodeCIDRs is immutable once set. + rule: oldSelf == "" || self == oldSelftest/e2e/create_cluster_test.go (1)
783-814: AllocateNodeCIDRs networking validations look correct; consider usingptr.Tofor consistencyThe three new cases correctly exercise the API contract for
AllocateNodeCIDRsvsNetworkTypeand follow the existing pattern of networking validations in this table-driven test. You might optionally switch the localenabled/disabledvariables toptr.To(hyperv1.AllocateNodeCIDRsEnabled/Disabled)to align with how pointers are created elsewhere in this file, but the current code is functionally fine.api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ImageStreamImportMode.yaml (2)
2544-2559: Set an explicit default and confirm day‑2 behavior for allocateNodeCIDRs.
- The description says “Default is 'Disabled'” but no default is defined. Add an explicit default to avoid nil vs. Disabled ambiguity.
Apply:
allocateNodeCIDRs: description: |- allocateNodeCIDRs controls whether the kube-controller-manager manages node CIDR allocation. When using networkType=Other, it is recommended to set this field to "Enabled" if Flannel is used as the CNI, as it relies on this behavior. - Default is "Disabled". + Default is "Disabled". This field can only be set to "Enabled" when NetworkType is "Other". Setting it to "Enabled" with any other NetworkType will result in a validation error during cluster creation. + default: Disabled enum: - Enabled - Disabled type: string x-kubernetes-validations: - message: allocateNodeCIDRs is immutable and cannot be modified once set. rule: self == oldSelfPlease confirm intended day‑2 semantics: with
rule: self == oldSelf, switching from Disabled→Enabled after creation will be rejected. Is that desired for this feature? If not, we can relax to “immutable after first non-empty set.”
2734-2737: Gate Enabled on Other: LGTM. Consider also requiring hostPrefix when Enabled.The gating rule is correct. To prevent misconfiguration of KCM node CIDR allocation, consider requiring a hostPrefix on each clusterNetwork entry when allocateNodeCIDRs is Enabled.
Apply near the existing validations:
- message: allocateNodeCIDRs can only be set to Enabled when networkType is 'Other' rule: 'has(self.allocateNodeCIDRs) && self.allocateNodeCIDRs == ''Enabled'' ? self.networkType == ''Other'' : true' + - message: when allocateNodeCIDRs is Enabled, each clusterNetwork entry must set hostPrefix + rule: 'has(self.allocateNodeCIDRs) && self.allocateNodeCIDRs == ''Enabled'' ? has(self.clusterNetwork) && self.clusterNetwork.all(c, has(c.hostPrefix)) : true'Please verify controller behavior: does the CPO/KCM derive
--node-cidr-mask-size(or equivalents) withouthostPrefix? If not, the above guard prevents clusters from landing in an invalid state. If it is auto‑derived safely, feel free to skip this rule.cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-CustomNoUpgrade.crd.yaml (1)
3333-3348: AllocateNodeCIDRs: add default and make description CNI‑agnostic.
- Schema says “Default is Disabled” but no default set.
- Description mentions Flannel; per earlier review, keep API wording generic to KCM’s allocate-node-cidrs.
Apply:
allocateNodeCIDRs: - description: |- - allocateNodeCIDRs controls whether the kube-controller-manager manages node CIDR allocation. - When using networkType=Other, it is recommended to set this field to "Enabled" - if Flannel is used as the CNI, as it relies on this behavior. - Default is "Disabled". + description: |- + allocateNodeCIDRs controls whether the kube-controller-manager manages node CIDR allocation + (kube-controller-manager flag: -allocate-node-cidrs). + When using networkType=Other and the chosen CNI requires per-node PodCIDR assignment by KCM, + set this field to "Enabled". + Default is "Disabled". This field can only be set to "Enabled" when NetworkType is "Other". Setting it to "Enabled" with any other NetworkType will result in a validation error during cluster creation. enum: - Enabled - Disabled type: string + default: Disabled x-kubernetes-validations: - message: allocateNodeCIDRs is immutable and cannot be modified once set. rule: self == oldSelfapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/DynamicResourceAllocation.yaml (1)
2547-2562: Optional: align docs and defaults.Description says “Default is Disabled” but no schema default is set here. Either add a schema default or confirm webhook defaulting covers this.
Proposed schema tweak:
allocateNodeCIDRs: description: | allocateNodeCIDRs controls whether the kube-controller-manager manages node CIDR allocation. + default: Disabled enum: - Enabled - DisabledIf a webhook already sets this default, ignore this and keep one source of truth.
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yaml (1)
2526-2541: Set a real default and use vendor‑neutral docs for a public API knob.
- Schema lacks a default while docs claim “Default is Disabled.”
- Description singles out Flannel; keep wording neutral.
Apply:
allocateNodeCIDRs: description: |- - allocateNodeCIDRs controls whether the kube-controller-manager manages node CIDR allocation. - When using networkType=Other, it is recommended to set this field to "Enabled" - if Flannel is used as the CNI, as it relies on this behavior. - Default is "Disabled". + allocateNodeCIDRs controls whether the kube-controller-manager allocates node CIDRs. + When using networkType=Other, set this to "Enabled" for CNIs that rely on controller-managed + node CIDR allocation. + Default is "Disabled". + default: Disabled This field can only be set to "Enabled" when NetworkType is "Other". Setting it to "Enabled" with any other NetworkType will result in a validation error during cluster creation. enum: - Enabled - Disabled type: stringapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AutoNodeKarpenter.yaml (1)
2763-2767: Gating rule LGTM; optional tighten-up when networkType != OtherThe rule correctly blocks Enabled unless networkType=='Other'. Optionally, to reduce confusing no‑op config, forbid specifying this field (or require it be 'Disabled') when networkType != 'Other'.
- rule: 'has(self.allocateNodeCIDRs) && self.allocateNodeCIDRs == ''Enabled'' ? self.networkType == ''Other'' : true' + rule: 'self.networkType == ''Other'' ? true : + (!has(self.allocateNodeCIDRs) || self.allocateNodeCIDRs == ''Disabled'')'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (2)
vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (46)
api/hypershift/v1beta1/hostedcluster_types.go(3 hunks)api/hypershift/v1beta1/zz_generated.deepcopy.go(1 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/DynamicResourceAllocation.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ImageStreamImportMode.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/IngressControllerLBSubnetsAWS.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/KMSEncryptionProvider.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/NetworkDiagnosticsConfig.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/SetEIPForNLBIngressController.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AAA_ungated.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AutoNodeKarpenter.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/DynamicResourceAllocation.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDC.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/GCPPlatform.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ImageStreamImportMode.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/IngressControllerLBSubnetsAWS.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/KMSEncryptionProvider.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/NetworkDiagnosticsConfig.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/OpenStack.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/SetEIPForNLBIngressController.yaml(2 hunks)client/applyconfiguration/hypershift/v1beta1/clusternetworking.go(2 hunks)cmd/cluster/core/create.go(4 hunks)cmd/cluster/core/create_test.go(2 hunks)cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-CustomNoUpgrade.crd.yaml(2 hunks)cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Default.crd.yaml(2 hunks)cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-TechPreviewNoUpgrade.crd.yaml(2 hunks)cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-CustomNoUpgrade.crd.yaml(2 hunks)cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Default.crd.yaml(2 hunks)cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-TechPreviewNoUpgrade.crd.yaml(2 hunks)control-plane-operator/controllers/hostedcontrolplane/testdata/kube-controller-manager/GCP/zz_fixture_TestControlPlaneComponents_kube_controller_manager_deployment.yaml(1 hunks)control-plane-operator/controllers/hostedcontrolplane/testdata/kube-controller-manager/IBMCloud/zz_fixture_TestControlPlaneComponents_kube_controller_manager_deployment.yaml(1 hunks)control-plane-operator/controllers/hostedcontrolplane/testdata/kube-controller-manager/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_kube_controller_manager_deployment.yaml(1 hunks)control-plane-operator/controllers/hostedcontrolplane/testdata/kube-controller-manager/zz_fixture_TestControlPlaneComponents_kube_controller_manager_deployment.yaml(1 hunks)control-plane-operator/controllers/hostedcontrolplane/v2/assets/kube-controller-manager/deployment.yaml(0 hunks)control-plane-operator/controllers/hostedcontrolplane/v2/kcm/deployment.go(1 hunks)docs/content/reference/api.md(2 hunks)hypershift-operator/controllers/hostedcluster/hostedcluster_webhook.go(1 hunks)test/e2e/create_cluster_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- control-plane-operator/controllers/hostedcontrolplane/v2/assets/kube-controller-manager/deployment.yaml
🚧 Files skipped from review as they are similar to previous changes (17)
- control-plane-operator/controllers/hostedcontrolplane/testdata/kube-controller-manager/GCP/zz_fixture_TestControlPlaneComponents_kube_controller_manager_deployment.yaml
- api/hypershift/v1beta1/zz_generated.deepcopy.go
- cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-TechPreviewNoUpgrade.crd.yaml
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/SetEIPForNLBIngressController.yaml
- hypershift-operator/controllers/hostedcluster/hostedcluster_webhook.go
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/IngressControllerLBSubnetsAWS.yaml
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yaml
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yaml
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/DynamicResourceAllocation.yaml
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ImageStreamImportMode.yaml
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/SetEIPForNLBIngressController.yaml
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yaml
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/NetworkDiagnosticsConfig.yaml
- control-plane-operator/controllers/hostedcontrolplane/testdata/kube-controller-manager/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_kube_controller_manager_deployment.yaml
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yaml
- cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Default.crd.yaml
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/KMSEncryptionProvider.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
client/applyconfiguration/hypershift/v1beta1/clusternetworking.gocmd/cluster/core/create.goapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/KMSEncryptionProvider.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlcontrol-plane-operator/controllers/hostedcontrolplane/v2/kcm/deployment.goapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDC.yamlcontrol-plane-operator/controllers/hostedcontrolplane/testdata/kube-controller-manager/zz_fixture_TestControlPlaneComponents_kube_controller_manager_deployment.yamltest/e2e/create_cluster_test.goapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/GCPPlatform.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-TechPreviewNoUpgrade.crd.yamlcontrol-plane-operator/controllers/hostedcontrolplane/testdata/kube-controller-manager/IBMCloud/zz_fixture_TestControlPlaneComponents_kube_controller_manager_deployment.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/IngressControllerLBSubnetsAWS.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ImageStreamImportMode.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamldocs/content/reference/api.mdapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AAA_ungated.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/NetworkDiagnosticsConfig.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-CustomNoUpgrade.crd.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/DynamicResourceAllocation.yamlapi/hypershift/v1beta1/hostedcluster_types.goapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/OpenStack.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AutoNodeKarpenter.yamlcmd/cluster/core/create_test.gocmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-CustomNoUpgrade.crd.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Default.crd.yaml
🧬 Code graph analysis (4)
cmd/cluster/core/create.go (1)
api/hypershift/v1beta1/hostedcluster_types.go (2)
AllocateNodeCIDRsEnabled(1200-1200)NetworkType(1178-1178)
control-plane-operator/controllers/hostedcontrolplane/v2/kcm/deployment.go (1)
api/hypershift/v1beta1/hostedcluster_types.go (1)
AllocateNodeCIDRsEnabled(1200-1200)
test/e2e/create_cluster_test.go (1)
api/hypershift/v1beta1/hostedcluster_types.go (6)
AllocateNodeCIDRsDisabled(1203-1203)ClusterNetworking(1045-1106)AllocateNodeCIDRsEnabled(1200-1200)NetworkType(1178-1178)Other(1191-1191)OVNKubernetes(1188-1188)
cmd/cluster/core/create_test.go (2)
cmd/cluster/core/create.go (2)
RawCreateOptions(128-192)CreateOptions(798-801)api/hypershift/v1beta1/hostedcluster_types.go (3)
NetworkType(1178-1178)AllocateNodeCIDRsMode(1196-1196)AllocateNodeCIDRsEnabled(1200-1200)
| - message: allocateNodeCIDRs can only be set to Enabled when networkType | ||
| is 'Other' | ||
| rule: 'has(self.allocateNodeCIDRs) && self.allocateNodeCIDRs == | ||
| ''Enabled'' ? self.networkType == ''Other'' : true' | ||
| nodeSelector: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for test mentions of allocateNodeCIDRs and networkType in test files
rg -n --type=go --type=yaml 'allocateNodeCIDRs|networkType.*Other' test/ controllers/ pkg/ 2>/dev/null | head -100Repository: openshift/hypershift
Length of output: 1695
🏁 Script executed:
#!/bin/bash
# Read the test file to check allocateNodeCIDRs test cases and see if update paths are covered
sed -n '784,820p' test/e2e/create_cluster_test.goRepository: openshift/hypershift
Length of output: 1407
🏁 Script executed:
#!/bin/bash
# Search for update tests related to allocateNodeCIDRs or validation rules
rg -n -B3 -A3 'update.*allocateNodeCIDRs|PATCH.*allocateNodeCIDRs|allocateNodeCIDRs.*update' test/ --type=go 2>/dev/null | head -50Repository: openshift/hypershift
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Check the test file structure to see if it tests updates at all
rg -n 'func.*Update|Update.*test|test.*Update' test/e2e/create_cluster_test.go --type=go | head -20Repository: openshift/hypershift
Length of output: 46
Validation rule is correct, but update path testing is missing.
Create path tests comprehensively cover the allocateNodeCIDRs validation: Disabled passes, Enabled+networkType=Other passes, and Enabled with other networkTypes properly fails with the expected error message. However, explicit test coverage for the update path is not present. Consider adding update tests to ensure the validation rule also applies when patching existing clusters.
🤖 Prompt for AI Agents
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yaml
lines 3298-3302: the validation rule for allocateNodeCIDRs is present but lacks
update-path tests; add unit/integration tests that exercise the update
(patch/put) path covering: (1) create/update where allocateNodeCIDRs is Disabled
— should pass, (2) create/update where allocateNodeCIDRs is Enabled and
networkType == "Other" — should pass, and (3) create/update where
allocateNodeCIDRs is Enabled and networkType != "Other" — should fail with the
existing error message "allocateNodeCIDRs can only be set to Enabled when
networkType is 'Other'"; implement assertions that the API returns the same
validation error on PATCH/PUT as on create and include both full-object replace
and strategic merge/JSON patch update variations where applicable.
| allocateNodeCIDRs: | ||
| description: |- | ||
| allocateNodeCIDRs controls whether the kube-controller-manager manages node CIDR allocation. | ||
| When using networkType=Other, it is recommended to set this field to "Enabled" | ||
| if Flannel is used as the CNI, as it relies on this behavior. | ||
| Default is "Disabled". | ||
| This field can only be set to "Enabled" when NetworkType is "Other". Setting it to "Enabled" | ||
| with any other NetworkType will result in a validation error during cluster creation. | ||
| enum: | ||
| - Enabled | ||
| - Disabled | ||
| type: string | ||
| x-kubernetes-validations: | ||
| - message: allocateNodeCIDRs is immutable and cannot be modified | ||
| once set. | ||
| rule: self == oldSelf |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# From repo root
echo "=== References to allocate-node-cidrs flag in controllers/manifests ==="
rg -n -C3 --type go --type yaml --type yml --type json 'allocate-node-cidrs|NodeCIDRMaskSize|node-cidr-mask-size'
echo
echo "=== Places that read HostedControlPlane.spec.networking.allocateNodeCIDRs ==="
rg -n -C3 --type go 'AllocateNodeCIDRs|allocateNodeCIDRs'
echo
echo "=== Ensure hostPrefix is consumed where KCM config is rendered ==="
rg -n -C3 --type go --type yaml 'hostPrefix|nodeCIDRMaskSize|NodeCIDRMaskSize'Repository: openshift/hypershift
Length of output: 50422
🏁 Script executed:
#!/bin/bash
echo "=== Check full KCM deployment.go file for node CIDR handling ==="
cat -n control-plane-operator/controllers/hostedcontrolplane/v2/kcm/deployment.go
echo
echo "=== Search for NodeCIDRMaskSize or node-cidr-mask ==="
rg -n "node-cidr-mask|NodeCIDRMaskSize" --type go
echo
echo "=== Search for service network configuration ==="
rg -n "ServiceNetwork|serviceNetwork" --type go control-plane-operator/ | head -30Repository: openshift/hypershift
Length of output: 7661
🏁 Script executed:
#!/bin/bash
echo "=== Search for any hostPrefix usage in KCM or related components ==="
rg -n "hostPrefix" --type go control-plane-operator/ | head -20
echo
echo "=== Search for node-cidr or NodeCIDR across the codebase ==="
rg -n "node.?cidr|NodeCIDR" --type go -i | head -30
echo
echo "=== Check if there's TODO or FIXME related to this ==="
rg -n "TODO|FIXME" --type go control-plane-operator/controllers/hostedcontrolplane/v2/kcm/ | head -20Repository: openshift/hypershift
Length of output: 3558
Verify node CIDR mask configuration in kube-controller-manager.
The --allocate-node-cidrs=true flag is correctly wired when allocateNodeCIDRs=Enabled (confirmed in control-plane-operator/controllers/hostedcontrolplane/v2/kcm/deployment.go:44-47), but the node CIDR mask size is not being configured. The --node-cidr-mask-size flag is missing from the KCM deployment arguments, meaning hostPrefix from networking.clusterNetwork[].hostPrefix is not being passed to KCM even when node CIDR allocation is enabled. This should be added to ensure proper subnet allocation when using custom CNI with Flannel.
|
/retest |
1 similar comment
|
/retest |
…Using AllocateNodesCIDRs introduces a new field, AllocateNodesCIDRs, which allows the kube-controller-manager to allocate node CIDRs when networkType=Other. It’s recommended to enable this field when using the Flannel CNI.
b68fa85 to
66121bb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (23)
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yaml (1)
2618-2634: Fix allocateNodeCIDRs immutability for legacy objects and remove Flannel‑specific wording
- The strict immutability rule
self == oldSelfwill reject updates to legacy HostedCluster objects that had noallocateNodeCIDRsfield when they are first backfilled/defaulted toDisabled(oldSelf is empty, self isDisabled). That’s a backwards‑compatibility break on day‑2 updates.- The description still calls out Flannel explicitly, which was already flagged as misleading because it implies Flannel is a supported CNI rather than documenting a generic KCM behavior.
Recommend:
- Allow a one‑time transition from empty to
Disabledwhile still blocking day‑2 flips toEnabled/Disabled.- Make the schema default explicit (
Disabled), in line with the doc text.- Reword the description to talk generically about CNIs that depend on controller‑managed node CIDRs, without naming Flannel.
Suggested diff:
allocateNodeCIDRs: description: |- - allocateNodeCIDRs controls whether the kube-controller-manager manages node CIDR allocation. - When using networkType=Other, it is recommended to set this field to "Enabled" - if Flannel is used as the CNI, as it relies on this behavior. - Default is "Disabled". - This field can only be set to "Enabled" when NetworkType is "Other". Setting it to "Enabled" - with any other NetworkType will result in a validation error during cluster creation. + allocateNodeCIDRs controls whether the kube-controller-manager manages node CIDR allocation + (allocates per-node pod CIDRs via Node.spec.podCIDR). + When using networkType=Other, set this field to "Enabled" only if your CNI requires + controller-managed node CIDR allocation. + Default is "Disabled". + This field can only be set to "Enabled" when networkType is "Other". Setting it to "Enabled" + with any other networkType will result in a validation error during cluster creation. enum: - Enabled - Disabled type: string + default: Disabled x-kubernetes-validations: - - message: allocateNodeCIDRs is immutable and cannot be modified once set. - rule: self == oldSelf + - message: allocateNodeCIDRs is immutable after creation; legacy objects may default to Disabled. + rule: (oldSelf == "" && self == 'Disabled') || self == oldSelf @@ - message: allocateNodeCIDRs can only be set to Enabled when networkType is 'Other' rule: 'has(self.allocateNodeCIDRs) && self.allocateNodeCIDRs == ''Enabled'' ? self.networkType == ''Other'' : true'This keeps the field effectively immutable for users while allowing existing objects created before this field existed to be safely backfilled to
Disabledand avoids implying Flannel support in user‑facing schema text.Also applies to: 2808-2811
cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-TechPreviewNoUpgrade.crd.yaml (1)
3336-3351: MakeallocateNodeCIDRsdescription CNI-agnostic and enforcehostPrefixwhen Enabled.The field still explicitly calls out Flannel and there is no safeguard ensuring
clusterNetwork[*].hostPrefixis set whenallocateNodeCIDRs == "Enabled". That both conflicts with prior guidance to avoid implying Flannel support and risks misconfiguring KCM’s node CIDR allocator for CNIs that depend on per-node CIDRs.Consider updating the description to be provider-agnostic and adding a CEL validation to require
hostPrefixwhenallocateNodeCIDRsisEnabled:@@ - allocateNodeCIDRs: - description: |- - allocateNodeCIDRs controls whether the kube-controller-manager manages node CIDR allocation. - When using networkType=Other, it is recommended to set this field to "Enabled" - if Flannel is used as the CNI, as it relies on this behavior. - Default is "Disabled". - This field can only be set to "Enabled" when NetworkType is "Other". Setting it to "Enabled" - with any other NetworkType will result in a validation error during cluster creation. + allocateNodeCIDRs: + description: |- + allocateNodeCIDRs controls whether the kube-controller-manager allocates node CIDRs. + For networkType=Other, set this to "Enabled" when the selected CNI requires controller-managed + node CIDR allocation. + Default is "Disabled". + This field can only be set to "Enabled" when networkType is "Other". Setting it to "Enabled" + with any other networkType will result in a validation error during cluster creation. @@ x-kubernetes-validations: - message: CIDR ranges in machineNetwork, clusterNetwork, and serviceNetwork must be unique and non-overlapping rule: (!has(self.machineNetwork) && self.clusterNetwork.all(c, self.serviceNetwork.all(s, c.cidr != s.cidr)) || (has(self.machineNetwork) && (self.machineNetwork.all(m, self.clusterNetwork.all(c, m.cidr != c.cidr)) && self.machineNetwork.all(m, self.serviceNetwork.all(s, m.cidr != s.cidr)) && self.clusterNetwork.all(c, self.serviceNetwork.all(s, c.cidr != s.cidr))))) - - message: allocateNodeCIDRs can only be set to Enabled when networkType - is 'Other' - rule: 'has(self.allocateNodeCIDRs) && self.allocateNodeCIDRs == - ''Enabled'' ? self.networkType == ''Other'' : true' + - message: allocateNodeCIDRs can only be set to Enabled when networkType + is 'Other' + rule: 'has(self.allocateNodeCIDRs) && self.allocateNodeCIDRs == + ''Enabled'' ? self.networkType == ''Other'' : true' + - message: hostPrefix must be set on each clusterNetwork entry when allocateNodeCIDRs + is Enabled + rule: '!has(self.allocateNodeCIDRs) || self.allocateNodeCIDRs != ''Enabled'' || (has(self.clusterNetwork) && self.clusterNetwork.all(c, has(c.hostPrefix)))'Also applies to: 3519-3529
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yaml (1)
2627-2642: MakeallocateNodeCIDRsdescription vendor‑neutral (drop Flannel‑specific wording).The field description still calls out Flannel explicitly and recommends
Enabledwhen Flannel is used. Per review discussion, this API should not imply first‑class support for a specific CNI. Rephrase along the lines of “enable when your CNI requires kube‑controller‑manager–managed node CIDR allocation” and avoid naming Flannel directly. The validation and enum are fine as‑is.cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-CustomNoUpgrade.crd.yaml (2)
3425-3436: MakeallocateNodeCIDRsdescription generic (avoid Flannel-specific wording).The description still calls out Flannel explicitly, which was called out in review as something to avoid because it implicitly suggests CNI support for a specific vendor. You already gate this on
networkType=Other, so the text can be generic about “CNIs that rely on KCM‑managed node CIDRs”.Consider something along these lines:
- allocateNodeCIDRs controls whether the kube-controller-manager manages node CIDR allocation. - When using networkType=Other, it is recommended to set this field to "Enabled" - if Flannel is used as the CNI, as it relies on this behavior. + allocateNodeCIDRs controls whether the kube-controller-manager allocates node CIDRs. + When using networkType=Other, set this to "Enabled" for CNIs that rely on + kube-controller-manager-managed node CIDR assignment. Default is "Disabled". - This field can only be set to "Enabled" when NetworkType is "Other". Setting it to "Enabled" - with any other NetworkType will result in a validation error during cluster creation. + This field can only be set to "Enabled" when networkType is "Other". Setting it to "Enabled" + with any other networkType will result in a validation error during cluster creation.
3615-3618: RequirehostPrefixwhenallocateNodeCIDRsis Enabled to avoid misconfigured KCM node CIDRs.Right now you only gate
allocateNodeCIDRs == "Enabled"onnetworkType == "Other". For KCM to allocate node CIDRs deterministically, allclusterNetworkentries should havehostPrefixset; otherwise users can enable this and end up with broken or surprising networking.Add a second CEL rule under
spec.networking.x-kubernetes-validationsto enforcehostPrefixwhenallocateNodeCIDRsis Enabled:x-kubernetes-validations: - message: CIDR ranges in machineNetwork, clusterNetwork, and serviceNetwork must be unique and non-overlapping rule: (!has(self.machineNetwork) && self.clusterNetwork.all(c, self.serviceNetwork.all(s, c.cidr != s.cidr)) || (has(self.machineNetwork) && (self.machineNetwork.all(m, self.clusterNetwork.all(c, m.cidr != c.cidr)) && self.machineNetwork.all(m, self.serviceNetwork.all(s, m.cidr != s.cidr)) && self.clusterNetwork.all(c, self.serviceNetwork.all(s, c.cidr != s.cidr))))) - message: allocateNodeCIDRs can only be set to Enabled when networkType is 'Other' rule: 'has(self.allocateNodeCIDRs) && self.allocateNodeCIDRs == ''Enabled'' ? self.networkType == ''Other'' : true' + - message: hostPrefix must be set for all clusterNetwork entries when allocateNodeCIDRs is Enabled + rule: '!has(self.allocateNodeCIDRs) || self.allocateNodeCIDRs != ''Enabled'' || (has(self.clusterNetwork) && self.clusterNetwork.all(c, has(c.hostPrefix)))'This keeps the API self-validating and avoids day‑0 misconfigurations that would otherwise only surface at runtime.
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/NetworkDiagnosticsConfig.yaml (1)
2678-2693: Make allocateNodeCIDRs CNI‑agnostic and enforce hostPrefix when EnabledTwo issues here:
- The description is Flannel‑specific and refers to
NetworkType(not the actualnetworkTypefield). This implies product support for Flannel and is inconsistent with the field name.- There is no CEL guard requiring
clusterNetwork[0].hostPrefixwhenallocateNodeCIDRs == "Enabled". That can lead to kube‑controller‑manager node‑CIDR allocation being misconfigured at runtime.Update the description and add a cross‑field validation alongside the existing networking validations:
- allocateNodeCIDRs: - description: |- - allocateNodeCIDRs controls whether the kube-controller-manager manages node CIDR allocation. - When using networkType=Other, it is recommended to set this field to "Enabled" - if Flannel is used as the CNI, as it relies on this behavior. - Default is "Disabled". - This field can only be set to "Enabled" when NetworkType is "Other". Setting it to "Enabled" - with any other NetworkType will result in a validation error during cluster creation. + allocateNodeCIDRs: + description: |- + allocateNodeCIDRs controls whether the kube-controller-manager allocates node + CIDRs. When using networkType=Other, set this to "Enabled" for CNIs that rely on + KCM-driven node CIDR allocation. + Default is "Disabled". + This field can only be set to "Enabled" when networkType is "Other". Setting it to "Enabled" + with any other networkType will result in a validation error during cluster creation. enum: - Enabled - Disabled type: string x-kubernetes-validations: - message: allocateNodeCIDRs is immutable and cannot be modified once set. rule: self == oldSelf @@ x-kubernetes-validations: - message: CIDR ranges in machineNetwork, clusterNetwork, and serviceNetwork must be unique and non-overlapping rule: (!has(self.machineNetwork) && self.clusterNetwork.all(c, self.serviceNetwork.all(s, c.cidr != s.cidr)) || (has(self.machineNetwork) && (self.machineNetwork.all(m, self.clusterNetwork.all(c, m.cidr != c.cidr)) && self.machineNetwork.all(m, self.serviceNetwork.all(s, m.cidr != s.cidr)) && self.clusterNetwork.all(c, self.serviceNetwork.all(s, c.cidr != s.cidr))))) + - message: When allocateNodeCIDRs is Enabled, clusterNetwork[0].hostPrefix must be set for KCM node CIDR allocation + rule: '!has(self.allocateNodeCIDRs) || self.allocateNodeCIDRs != ''Enabled'' || (has(self.clusterNetwork) && self.clusterNetwork.size() >= 1 && has(self.clusterNetwork[0].hostPrefix))' - message: allocateNodeCIDRs can only be set to Enabled when networkType is 'Other' rule: 'has(self.allocateNodeCIDRs) && self.allocateNodeCIDRs == ''Enabled'' ? self.networkType == ''Other'' : true'Also applies to: 2868-2871
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ImageStreamImportMode.yaml (1)
2636-2651: EnforcehostPrefixwhenallocateNodeCIDRsis Enabled and make description CNI‑agnosticTwo concerns here:
- There is still no CEL guard to ensure
spec.networking.clusterNetwork[*].hostPrefixis set whenallocateNodeCIDRs == "Enabled". WithouthostPrefix, kube-controller-manager node CIDR allocation is invalid/ambiguous and can lead to broken clusters. Add a validation underspec.networking.x-kubernetes-validationsto enforce this, e.g.:x-kubernetes-validations: - message: CIDR ranges in machineNetwork, clusterNetwork, and serviceNetwork must be unique and non-overlapping rule: (!has(self.machineNetwork) && self.clusterNetwork.all(c, self.serviceNetwork.all(s, c.cidr != s.cidr)) || (has(self.machineNetwork) && (self.machineNetwork.all(m, self.clusterNetwork.all(c, m.cidr != c.cidr)) && self.machineNetwork.all(m, self.serviceNetwork.all(s, m.cidr != s.cidr)) && self.clusterNetwork.all(c, self.serviceNetwork.all(s, c.cidr != s.cidr))))) + - message: hostPrefix is required for all clusterNetwork entries when allocateNodeCIDRs is Enabled + rule: '!has(self.allocateNodeCIDRs) || self.allocateNodeCIDRs != ''Enabled'' || (has(self.clusterNetwork) && self.clusterNetwork.all(c, has(c.hostPrefix)))'
- The field description explicitly calls out Flannel, which was previously called out as undesirable because it implies vendor/CNI support from the API. Please rephrase this as a generic KCM toggle (e.g., “set to Enabled only when the chosen CNI requires controller-managed node PodCIDRs”) and avoid naming Flannel directly.
cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Default.crd.yaml (2)
3289-3296: Make allocateNodeCIDRs description vendor‑neutral and expand kube-controller-managerThe description currently calls out Flannel explicitly and uses the unexplained “KCM” abbreviation. That implies Flannel support and is inconsistent with prior review guidance. Suggest updating to generic CNI wording and spelling out kube-controller-manager:
- description: |- - allocateNodeCIDRs controls whether the kube-controller-manager manages node CIDR allocation. - When using networkType=Other, it is recommended to set this field to "Enabled" - if Flannel is used as the CNI, as it relies on this behavior. - Default is "Disabled". + description: |- + allocateNodeCIDRs controls whether the kube-controller-manager manages node CIDR allocation. + When using networkType=Other, set this to "Enabled" if your CNI relies on controller-managed node CIDR allocation. + Default is "Disabled" (set in code, not at API level). This field can only be set to "Enabled" when NetworkType is "Other". Setting it to "Enabled" with any other NetworkType will result in a validation error during cluster creation.
3301-3304: Fix immutability validation to allow first assignment when previously unset
self == oldSelfmakesallocateNodeCIDRseffectively unchangeable, including from “unset” to a value, which contradicts the “immutable once set” intent and differs from other string fields in this CRD that use the standard pattern.Recommend:
- x-kubernetes-validations: - - message: allocateNodeCIDRs is immutable and cannot be modified - once set. - rule: self == oldSelf + x-kubernetes-validations: + - message: allocateNodeCIDRs is immutable once set. + rule: oldSelf == "" || self == oldSelfcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-TechPreviewNoUpgrade.crd.yaml (2)
3244-3255: MakeallocateNodeCIDRsdescription CNI‑agnostic and aligned with enum semanticsThe description still calls out Flannel explicitly and talks about
"true"while the field is an"Enabled"/"Disabled"enum. This both suggests Flannel is a special/supported CNI and is slightly inconsistent with the actual API.You can keep the behavior description but make it provider‑neutral and enum‑accurate, e.g.:
- description: |- - allocateNodeCIDRs controls whether the kube-controller-manager manages node CIDR allocation. - When using networkType=Other, it is recommended to set this field to "Enabled" - if Flannel is used as the CNI, as it relies on this behavior. - Default is "Disabled". - This field can only be set to "Enabled" when NetworkType is "Other". Setting it to "Enabled" - with any other NetworkType will result in a validation error during cluster creation. + description: |- + allocateNodeCIDRs controls whether the kube-controller-manager manages per-node + CIDR allocation. + When using networkType="Other", set this field to "Enabled" only if the chosen + CNI requires kube-controller-manager-managed node CIDRs. + Default is "Disabled". + This field may only be "Enabled" when networkType is "Other".
3434-3437: EnforcehostPrefixwhenallocateNodeCIDRsis Enabled (avoid broken KCM CIDR allocation)Right now users can set
networking.allocateNodeCIDRs: Enabledwithout specifyinghostPrefixonclusterNetworkentries. KCM’s CIDR allocation depends onhostPrefix, so this allows misconfigurations that can break node CIDR assignment.Add a CEL guard alongside the existing
networkType == 'Other'rule:- message: allocateNodeCIDRs can only be set to Enabled when networkType is 'Other' rule: 'has(self.allocateNodeCIDRs) && self.allocateNodeCIDRs == ''Enabled'' ? self.networkType == ''Other'' : true' + - message: When allocateNodeCIDRs is Enabled, hostPrefix must be set on all clusterNetwork entries + rule: '!has(self.allocateNodeCIDRs) || self.allocateNodeCIDRs != ''Enabled'' + || (has(self.clusterNetwork) && self.clusterNetwork.all(cn, has(cn.hostPrefix)))'This keeps the field optional in all other cases but forces a well‑formed configuration when KCM CIDR allocation is enabled.
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AAA_ungated.yaml (1)
2535-2550: MakeallocateNodeCIDRsdescription vendor‑neutral and align wording with other CRDs.The enum + immutability + cross‑field validation look good, but the description still hard‑codes Flannel as the consumer of this knob. Earlier review guidance was to keep this field generic (“some CNIs require controller‑managed node CIDR allocation”) and not imply Flannel is specially supported.
Consider updating the description (and keeping the existing behavior text) along these lines:
- allocateNodeCIDRs: - description: |- - allocateNodeCIDRs controls whether the kube-controller-manager manages node CIDR allocation. - When using networkType=Other, it is recommended to set this field to "Enabled" - if Flannel is used as the CNI, as it relies on this behavior. - Default is "Disabled". - This field can only be set to "Enabled" when NetworkType is "Other". Setting it to "Enabled" - with any other NetworkType will result in a validation error during cluster creation. + allocateNodeCIDRs: + description: |- + allocateNodeCIDRs controls whether the kube-controller-manager (KCM) manages node CIDR allocation. + Some CNIs rely on controller-managed node CIDR allocation; when using networkType=Other, + set this field to "Enabled" if your CNI requires it. + Default is "Disabled". + This field can only be set to "Enabled" when NetworkType is "Other". Setting it to "Enabled" + with any other NetworkType will result in a validation error during cluster creation. enum: - Enabled - Disabled type: string x-kubernetes-validations: - - message: allocateNodeCIDRs is immutable and cannot be modified - once set. + - message: allocateNodeCIDRs is immutable and cannot be modified once set. rule: self == oldSelfThis keeps behavior and validation as‑is while making the API contract CNI‑agnostic and consistent with the HostedCluster CRD/docs.
Also applies to: 2718-2728
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yaml (1)
3108-3123: ClarifyallocateNodeCIDRsdocs (remove Flannel mention, clarify validation scope, consider explicit default).The schema is now generic but the description still calls out Flannel and says the validation error happens “during cluster creation,” while the CEL rule applies on both create and update. Also, the description claims the default is
Disabledbut there is nodefault:here, so the effective default is only in code/webhook.I’d suggest:
- Make the description CNI-agnostic (just say “when your CNI expects the control plane to allocate per-node PodCIDRs”).
- Replace “during cluster creation” with wording that covers create and update.
- Either add
default: Disabledto the schema or explicitly say “Default controller behavior is Disabled; the API does not set a default.”This keeps the public API surface neutral and avoids user confusion about when validation applies and what the real default is.
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yaml (1)
2618-2633: Make allocateNodeCIDRs description CNI‑agnostic and fix networkType casingThe field is meant as a generic KCM toggle, but the description still calls out Flannel explicitly and uses
NetworkType(capital N) instead of the actualnetworkTypefield name. This can be misleading from an API/contract perspective.Consider updating the description to be generic and accurate, for example:
- description: |- - allocateNodeCIDRs controls whether the kube-controller-manager manages node CIDR allocation. - When using networkType=Other, it is recommended to set this field to "Enabled" - if Flannel is used as the CNI, as it relies on this behavior. - Default is "Disabled". - This field can only be set to "Enabled" when NetworkType is "Other". Setting it to "Enabled" - with any other NetworkType will result in a validation error during cluster creation. + description: |- + allocateNodeCIDRs controls whether the kube-controller-manager manages node CIDR allocation. + When using networkType=Other, it is recommended to set this field to "Enabled" + if your CNI requires kube-controller-manager-based node CIDR allocation. + Default is "Disabled". + This field can only be set to "Enabled" when networkType is "Other". Setting it to "Enabled" + with any other networkType will result in a validation error during cluster creation.api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yaml (2)
2665-2672: De-flannelize and rephraseallocateNodeCIDRsdescription; fixnetworkTypecasingThe description still calls out Flannel specifically and uses
NetworkTypeinstead of the actual field namenetworkType. This can be misleading about supported CNIs and is inconsistent with the API surface. The behavior is generic to kube-controller-manager.Consider updating this block like:
- allocateNodeCIDRs: - description: |- - allocateNodeCIDRs controls whether the kube-controller-manager manages node CIDR allocation. - When using networkType=Other, it is recommended to set this field to "Enabled" - if Flannel is used as the CNI, as it relies on this behavior. - Default is "Disabled". - This field can only be set to "Enabled" when NetworkType is "Other". Setting it to "Enabled" - with any other NetworkType will result in a validation error during cluster creation. + allocateNodeCIDRs: + description: |- + allocateNodeCIDRs controls whether the kube-controller-manager allocates per-node CIDRs. + When using networkType="Other" with CNIs that rely on controller‑managed node CIDRs, + set this field to "Enabled". Default is "Disabled". + This field can only be set to "Enabled" when networkType is "Other". Setting it to "Enabled" + with any other networkType will result in a validation error during cluster creation.
2847-2858: Add CEL validation to requirehostPrefixwhenallocateNodeCIDRsis EnabledRight now you gate
allocateNodeCIDRs=EnabledonnetworkType=="Other", but you don’t enforce thatclusterNetwork[*].hostPrefixis set when controller-managed node CIDRs are used. KCM needs a per-node mask; without hostPrefix this misconfiguration will only fail at runtime.Extend the networking-level CEL validations to imply hostPrefix from
allocateNodeCIDRs == "Enabled":x-kubernetes-validations: - message: CIDR ranges in machineNetwork, clusterNetwork, and serviceNetwork must be unique and non-overlapping rule: (!has(self.machineNetwork) && self.clusterNetwork.all(c, self.serviceNetwork.all(s, c.cidr != s.cidr)) || (has(self.machineNetwork) && (self.machineNetwork.all(m, self.clusterNetwork.all(c, m.cidr != c.cidr)) && self.machineNetwork.all(m, self.serviceNetwork.all(s, m.cidr != s.cidr)) && self.clusterNetwork.all(c, self.serviceNetwork.all(s, c.cidr != s.cidr))))) + - message: hostPrefix must be set on all clusterNetwork entries when allocateNodeCIDRs is Enabled + rule: '!has(self.allocateNodeCIDRs) || self.allocateNodeCIDRs != ''Enabled'' ? true : + (has(self.clusterNetwork) && self.clusterNetwork.all(c, has(c.hostPrefix)))' - - message: allocateNodeCIDRs can only be set to Enabled when networkType - is 'Other' - rule: 'has(self.allocateNodeCIDRs) && self.allocateNodeCIDRs == - ''Enabled'' ? self.networkType == ''Other'' : true' + - message: allocateNodeCIDRs can only be set to Enabled when networkType + is 'Other' + rule: 'has(self.allocateNodeCIDRs) && self.allocateNodeCIDRs == + ''Enabled'' ? self.networkType == ''Other'' : true'(Keep the existing gating rule; only insert the new
hostPrefiximplication above it.)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/DynamicResourceAllocation.yaml (1)
2737-2740: Enforce hostPrefix when allocateNodeCIDRs is Enabled (to prevent KCM misconfig)When kube-controller-manager allocates node CIDRs, it needs a per-node mask; here that comes from
networking.clusterNetwork[*].hostPrefix. WithallocateNodeCIDRs == "Enabled"but missinghostPrefix, you risk invalid or unexpected KCM behavior. Consider adding a networking-scoped CEL validation alongside the existing ones:x-kubernetes-validations: - message: CIDR ranges in machineNetwork, clusterNetwork, and serviceNetwork must be unique and non-overlapping rule: (!has(self.machineNetwork) && self.clusterNetwork.all(c, self.serviceNetwork.all(s, c.cidr != s.cidr)) || (has(self.machineNetwork) && (self.machineNetwork.all(m, self.clusterNetwork.all(c, m.cidr != c.cidr)) && self.machineNetwork.all(m, self.serviceNetwork.all(s, m.cidr != s.cidr)) && self.clusterNetwork.all(c, self.serviceNetwork.all(s, c.cidr != s.cidr))))) + - message: When allocateNodeCIDRs is Enabled, every clusterNetwork entry must define hostPrefix + rule: '!has(self.allocateNodeCIDRs) || self.allocateNodeCIDRs != ''Enabled'' ? true : (has(self.clusterNetwork) && self.clusterNetwork.all(c, has(c.hostPrefix)))' - message: allocateNodeCIDRs can only be set to Enabled when networkType is 'Other' rule: 'has(self.allocateNodeCIDRs) && self.allocateNodeCIDRs == ''Enabled'' ? self.networkType == ''Other'' : true'This keeps the invariant local to
networkingand avoids invalid combinations at admission time.api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/KMSEncryptionProvider.yaml (1)
2602-2617: MakeallocateNodeCIDRsdescription CNI‑generic (remove Flannel mention)The field is now a generic KCM knob, but the description still calls out Flannel explicitly, which conflicts with earlier guidance to avoid implying Flannel is a supported CNI.
Recommend rewording to something like:
- description: |- - allocateNodeCIDRs controls whether the kube-controller-manager manages node CIDR allocation. - When using networkType=Other, it is recommended to set this field to "Enabled" - if Flannel is used as the CNI, as it relies on this behavior. - Default is "Disabled". - This field can only be set to "Enabled" when NetworkType is "Other". Setting it to "Enabled" - with any other NetworkType will result in a validation error during cluster creation. + description: |- + allocateNodeCIDRs controls whether the kube-controller-manager allocates node CIDRs. + When using networkType=Other and the chosen CNI requires controller-managed node CIDRs, + set this field to "Enabled". Default is "Disabled". + This field can only be set to "Enabled" when NetworkType is "Other". Setting it to "Enabled" + with any other NetworkType will result in a validation error during cluster creation.api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/OpenStack.yaml (1)
2526-2541: MakeallocateNodeCIDRsdescription CNI‑agnostic and add hostPrefix validation when EnabledThe field description still hard‑codes Flannel and implies explicit Flannel support, which contradicts earlier review guidance to keep this knob generic and KCM‑centric. Also, when
allocateNodeCIDRs == "Enabled", nothing enforces thatclusterNetwork[*].hostPrefixis set, which is typically required for KCM‑driven node CIDR allocation.Consider updating the description and adding a cross‑field CEL like below (applied in Go types via kubebuilder markers, then regenerate this CRD):
- allocateNodeCIDRs: - description: |- - allocateNodeCIDRs controls whether the kube-controller-manager manages node CIDR allocation. - When using networkType=Other, it is recommended to set this field to "Enabled" - if Flannel is used as the CNI, as it relies on this behavior. - Default is "Disabled". - This field can only be set to "Enabled" when NetworkType is "Other". Setting it to "Enabled" - with any other NetworkType will result in a validation error during cluster creation. + allocateNodeCIDRs: + description: |- + allocateNodeCIDRs controls whether the kube-controller-manager manages node CIDR allocation. + This is intended for third-party CNIs that rely on kube-controller-manager–driven + node CIDR management when networking.networkType is "Other". + Default is "Disabled". This field is immutable once set. enum: - Enabled - Disabled type: string x-kubernetes-validations: - message: allocateNodeCIDRs is immutable and cannot be modified once set. rule: self == oldSelf ... x-kubernetes-validations: - message: CIDR ranges in machineNetwork, clusterNetwork, and serviceNetwork must be unique and non-overlapping rule: (!has(self.machineNetwork) && self.clusterNetwork.all(c, self.serviceNetwork.all(s, c.cidr != s.cidr)) || (has(self.machineNetwork) && (self.machineNetwork.all(m, self.clusterNetwork.all(c, m.cidr != c.cidr)) && self.machineNetwork.all(m, self.serviceNetwork.all(s, m.cidr != s.cidr)) && self.clusterNetwork.all(c, self.serviceNetwork.all(s, c.cidr != s.cidr))))) + - message: hostPrefix must be set and greater than 0 on all clusterNetwork entries when allocateNodeCIDRs is Enabled + rule: '!has(self.allocateNodeCIDRs) || self.allocateNodeCIDRs != ''Enabled'' ? true : + (has(self.clusterNetwork) && self.clusterNetwork.all(c, has(c.hostPrefix) && c.hostPrefix > 0))' - message: allocateNodeCIDRs can only be set to Enabled when networkType is 'Other' rule: 'has(self.allocateNodeCIDRs) && self.allocateNodeCIDRs == ''Enabled'' ? self.networkType == ''Other'' : true'Please also verify that the same generic wording and CEL set (networkType gating + hostPrefix requirement) are consistently present in the other generated HostedControlPlane/HostedCluster CRDs for all platforms.
If you want, I can provide a
rg/ast-grepscript to scan all CRDs and Go types for inconsistentallocateNodeCIDRsdescriptions and validations.Also applies to: 2716-2719
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/IngressControllerLBSubnetsAWS.yaml (1)
2526-2541: MakeallocateNodeCIDRsdescription vendor‑neutral and align docs with schema.The field behavior/validation look fine, but the CRD description still singles out Flannel and claims a default
"Disabled"that is not expressed in the schema. This leaks a specific CNI choice into a generic KCM knob and can confuse consumers about where the actual default is enforced (CRD vs webhook).I suggest keeping the generic enum + networkType gating, but updating only the description text:
- Drop the Flannel‑specific wording; describe it generically as “for CNIs that require controller‑managed node PodCIDRs”.
- Remove or reword the “Default is 'Disabled'” sentence so it matches reality (e.g. point to webhook/platform defaults).
Example schema tweak in this hunk:
- allocateNodeCIDRs: - description: |- - allocateNodeCIDRs controls whether the kube-controller-manager manages node CIDR allocation. - When using networkType=Other, it is recommended to set this field to "Enabled" - if Flannel is used as the CNI, as it relies on this behavior. - Default is "Disabled". - This field can only be set to "Enabled" when NetworkType is "Other". Setting it to "Enabled" - with any other NetworkType will result in a validation error during cluster creation. + allocateNodeCIDRs: + description: |- + allocateNodeCIDRs controls whether the kube-controller-manager allocates per-node PodCIDRs + from the cluster network CIDR. + When using networkType=Other, set this to "Enabled" for CNIs that require + controller-managed node CIDR allocation. + If not specified, platform defaults apply (see admission/webhook defaults). + This field can only be set to "Enabled" when networkType is "Other". Setting it to "Enabled" + with any other networkType will result in a validation error during cluster creation.(Then update the Go type comment / kubebuilder markers that generate this CRD and regenerate.)
Also applies to: 2716-2719
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yaml (1)
2618-2633: Immutability rule still blocks first-time unset→set; align CEL with “once set” wording
x-kubernetes-validationsforallocateNodeCIDRsusesrule: self == oldSelf, which prevents changing the field from unset/null to any value on day‑2. That means if the field is omitted at creation, it can never be set later, despite the description saying “immutable … once set”, which normally implies “immutable after first set”, not “must be decided at initial create only”.Suggest updating the rule to allow the initial transition from null/unset and then freeze the value:
x-kubernetes-validations: - message: allocateNodeCIDRs is immutable after first set. rule: oldSelf == null || self == oldSelfThe cross-field validation tying
"Enabled"tonetworkType == "Other"looks correct as is.Also applies to: 2808-2811
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yaml (1)
2526-2533: KeepallocateNodeCIDRsAPI docs CNI‑agnostic; drop Flannel‑specific wording.The field is now a generic KCM toggle, but the description still calls out Flannel, which again implies vendor‑specific support and conflicts with earlier guidance to keep this knob neutral. Suggest rephrasing without naming a particular CNI.
- allocateNodeCIDRs: - description: |- - allocateNodeCIDRs controls whether the kube-controller-manager manages node CIDR allocation. - When using networkType=Other, it is recommended to set this field to "Enabled" - if Flannel is used as the CNI, as it relies on this behavior. - Default is "Disabled". - This field can only be set to "Enabled" when NetworkType is "Other". Setting it to "Enabled" - with any other NetworkType will result in a validation error during cluster creation. + allocateNodeCIDRs: + description: |- + allocateNodeCIDRs controls whether the kube-controller-manager allocates node CIDRs for nodes. + When using networkType=Other, set this field to "Enabled" only for CNIs that rely on + kube-controller-manager–managed node CIDR allocation. + Default is "Disabled". + This field can only be set to "Enabled" when networkType is "Other". Setting it to "Enabled" + with any other networkType will result in a validation error during cluster creation.cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Default.crd.yaml (1)
3197-3213: Reconsider full immutability ofallocateNodeCIDRs; allow one‑way enablement
x-kubernetes-validations: rule: self == oldSelfmakesallocateNodeCIDRsfully immutable. Combined with the default/initial value ofDisabled, this prevents day‑2 enabling of node CIDR allocation, even though disabling after enabling is the risky transition. That’s stricter than necessary and blocks existing clusters from adopting this behavior without a recreate.If the intent is to prevent disabling once enabled but still allow
Disabled → Enabled, consider a one‑way rule instead:- x-kubernetes-validations: - - message: allocateNodeCIDRs is immutable and cannot be modified - once set. - rule: self == oldSelf + x-kubernetes-validations: + - message: allocateNodeCIDRs cannot be set back to Disabled once Enabled. + rule: 'oldSelf == ''Enabled'' ? self == ''Enabled'' : true'This keeps safety guarantees while permitting upgrades of existing clusters to start using KCM‑managed PodCIDRs without recreating the HostedControlPlane. The cross‑field gating at Lines 3387‑3390 (
Enabledonly whennetworkType == 'Other') already looks correct and can stay as is.Also applies to: 3387-3390
🧹 Nitpick comments (2)
api/hypershift/v1beta1/hostedcluster_types.go (1)
1096-1105: Consider more generic documentation and optionally add immutability marker.The field implementation correctly follows past feedback (enum type, code-based defaulting, XValidation for immutability). However, consider these optional improvements:
The documentation mentions "Flannel" specifically. Per past review feedback, this could imply Flannel is a supported CNI. Consider more generic phrasing: "When using networkType=Other, it is recommended to set this field to 'Enabled' if your CNI requires the kube-controller-manager to allocate node CIDRs."
While XValidation provides runtime immutability enforcement, consider adding the
// +immutablemarker as a Kubernetes API convention for schema-level documentation and clarity.Apply this diff for more generic documentation:
- // allocateNodeCIDRs controls whether the kube-controller-manager manages node CIDR allocation. - // When using networkType=Other, it is recommended to set this field to "Enabled" - // if Flannel is used as the CNI, as it relies on this behavior. + // allocateNodeCIDRs controls whether the kube-controller-manager manages node CIDR allocation. + // When using networkType=Other, it is recommended to set this field to "Enabled" + // if your CNI requires the kube-controller-manager to allocate node CIDRs. // Default is "Disabled". // This field can only be set to "Enabled" when NetworkType is "Other". Setting it to "Enabled" // with any other NetworkType will result in a validation error during cluster creation. // + // +immutable // +optional // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="allocateNodeCIDRs is immutable and cannot be modified once set." AllocateNodeCIDRs *AllocateNodeCIDRsMode `json:"allocateNodeCIDRs,omitempty"`api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/DynamicResourceAllocation.yaml (1)
2547-2562: Avoid Flannel-specific wording in allocateNodeCIDRs descriptionThe behavior here is generic kube-controller-manager behavior; explicitly recommending this only when “Flannel is used as the CNI” makes the API look Flannel-specific, which earlier review feedback aimed to avoid. Consider rephrasing to something like “CNIs that rely on controller-managed node CIDR allocation (for example, those that consume node CIDRs from the Kubernetes apiserver) may require this to be Enabled” and move Flannel-specific guidance into external docs or operator-level documentation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (2)
vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (46)
api/hypershift/v1beta1/hostedcluster_types.go(3 hunks)api/hypershift/v1beta1/zz_generated.deepcopy.go(1 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/DynamicResourceAllocation.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ImageStreamImportMode.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/IngressControllerLBSubnetsAWS.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/KMSEncryptionProvider.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/NetworkDiagnosticsConfig.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/SetEIPForNLBIngressController.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AAA_ungated.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AutoNodeKarpenter.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/DynamicResourceAllocation.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDC.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/GCPPlatform.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ImageStreamImportMode.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/IngressControllerLBSubnetsAWS.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/KMSEncryptionProvider.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/NetworkDiagnosticsConfig.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/OpenStack.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/SetEIPForNLBIngressController.yaml(2 hunks)client/applyconfiguration/hypershift/v1beta1/clusternetworking.go(2 hunks)cmd/cluster/core/create.go(4 hunks)cmd/cluster/core/create_test.go(2 hunks)cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-CustomNoUpgrade.crd.yaml(2 hunks)cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Default.crd.yaml(2 hunks)cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-TechPreviewNoUpgrade.crd.yaml(2 hunks)cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-CustomNoUpgrade.crd.yaml(2 hunks)cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Default.crd.yaml(2 hunks)cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-TechPreviewNoUpgrade.crd.yaml(2 hunks)control-plane-operator/controllers/hostedcontrolplane/testdata/kube-controller-manager/GCP/zz_fixture_TestControlPlaneComponents_kube_controller_manager_deployment.yaml(1 hunks)control-plane-operator/controllers/hostedcontrolplane/testdata/kube-controller-manager/IBMCloud/zz_fixture_TestControlPlaneComponents_kube_controller_manager_deployment.yaml(1 hunks)control-plane-operator/controllers/hostedcontrolplane/testdata/kube-controller-manager/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_kube_controller_manager_deployment.yaml(1 hunks)control-plane-operator/controllers/hostedcontrolplane/testdata/kube-controller-manager/zz_fixture_TestControlPlaneComponents_kube_controller_manager_deployment.yaml(1 hunks)control-plane-operator/controllers/hostedcontrolplane/v2/assets/kube-controller-manager/deployment.yaml(0 hunks)control-plane-operator/controllers/hostedcontrolplane/v2/kcm/deployment.go(1 hunks)docs/content/reference/api.md(2 hunks)hypershift-operator/controllers/hostedcluster/hostedcluster_webhook.go(1 hunks)test/e2e/create_cluster_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- control-plane-operator/controllers/hostedcontrolplane/v2/assets/kube-controller-manager/deployment.yaml
✅ Files skipped from review due to trivial changes (1)
- control-plane-operator/controllers/hostedcontrolplane/testdata/kube-controller-manager/IBMCloud/zz_fixture_TestControlPlaneComponents_kube_controller_manager_deployment.yaml
🚧 Files skipped from review as they are similar to previous changes (17)
- test/e2e/create_cluster_test.go
- control-plane-operator/controllers/hostedcontrolplane/testdata/kube-controller-manager/GCP/zz_fixture_TestControlPlaneComponents_kube_controller_manager_deployment.yaml
- docs/content/reference/api.md
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/IngressControllerLBSubnetsAWS.yaml
- control-plane-operator/controllers/hostedcontrolplane/testdata/kube-controller-manager/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_kube_controller_manager_deployment.yaml
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/KMSEncryptionProvider.yaml
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDC.yaml
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/SetEIPForNLBIngressController.yaml
- api/hypershift/v1beta1/zz_generated.deepcopy.go
- cmd/cluster/core/create.go
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yaml
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AutoNodeKarpenter.yaml
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/NetworkDiagnosticsConfig.yaml
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/SetEIPForNLBIngressController.yaml
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yaml
- cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-CustomNoUpgrade.crd.yaml
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/DynamicResourceAllocation.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
hypershift-operator/controllers/hostedcluster/hostedcluster_webhook.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kcm/deployment.goapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ImageStreamImportMode.yamlclient/applyconfiguration/hypershift/v1beta1/clusternetworking.gocmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-TechPreviewNoUpgrade.crd.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ImageStreamImportMode.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-CustomNoUpgrade.crd.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/DynamicResourceAllocation.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Default.crd.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/OpenStack.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AAA_ungated.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/KMSEncryptionProvider.yamlcontrol-plane-operator/controllers/hostedcontrolplane/testdata/kube-controller-manager/zz_fixture_TestControlPlaneComponents_kube_controller_manager_deployment.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/GCPPlatform.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-TechPreviewNoUpgrade.crd.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlapi/hypershift/v1beta1/hostedcluster_types.gocmd/cluster/core/create_test.goapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/IngressControllerLBSubnetsAWS.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Default.crd.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/NetworkDiagnosticsConfig.yaml
🧬 Code graph analysis (3)
hypershift-operator/controllers/hostedcluster/hostedcluster_webhook.go (1)
api/hypershift/v1beta1/hostedcluster_types.go (1)
AllocateNodeCIDRsDisabled(1203-1203)
control-plane-operator/controllers/hostedcontrolplane/v2/kcm/deployment.go (1)
api/hypershift/v1beta1/hostedcluster_types.go (1)
AllocateNodeCIDRsEnabled(1200-1200)
cmd/cluster/core/create_test.go (2)
cmd/cluster/core/create.go (3)
RawCreateOptions(128-192)CreateOptions(798-801)ValidatedCreateOptions(659-662)api/hypershift/v1beta1/hostedcluster_types.go (3)
NetworkType(1178-1178)AllocateNodeCIDRsMode(1196-1196)AllocateNodeCIDRsEnabled(1200-1200)
|
/lgtm |
|
/retest-required |
2 similar comments
|
/retest-required |
|
/retest-required |
|
/verified |
|
@linoyaslan: The 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. |
|
/verified by @linoyaslan |
openshift-ci-robot
added
the
verified
|
@linoyaslan: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/ackknowledge-critical-fixes-only |
|
/label acknowledge-critical-fixes-only |
openshift-ci
Bot
added
the
acknowledge-critical-fixes-only
|
/retest-required |
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
@linoyaslan: The following test 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. |
2b93119
into
openshift:main
4 participants
What this PR does / why we need it:
This PR introduces a new field,
AllocateNodesCIDRs, which allows the kube-controller-manager to allocate node CIDRs when networkType=Other. It’s recommended to enable this field when using the Flannel CNI.Checklist: