CORS-3897: Dual stack support for cluster manifest & VNet#10319
CORS-3897: Dual stack support for cluster manifest & VNet#10319jhixson74 wants to merge 4 commits intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@jhixson74: This pull request references CORS-3901 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
jhixson74
force-pushed
the
main-ipv6-azure
branch
3 times, most recently
from
bc4a130 to
53acac8
Compare
jhixson74
changed the title
jhixson74
force-pushed
the
main-ipv6-azure
branch
3 times, most recently
from
135e207 to
fa580ef
Compare
jhixson74
changed the title
|
@jhixson74: This pull request references CORS-3897 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.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. |
jhixson74
force-pushed
the
main-ipv6-azure
branch
2 times, most recently
from
37cc589 to
44ded41
Compare
|
/retest-required |
jhixson74
force-pushed
the
main-ipv6-azure
branch
from
44ded41 to
6e5e796
Compare
pkg/asset/manifests/azure/cluster.go
Outdated
| PrivateIPAddress: lbip, | ||
| PrivateIPAddress: lbip4, | ||
| }, | ||
| }} |
There was a problem hiding this comment.
Can we move virtualNetworkID from ln 229 to here? It is updated in the following block of code.
There was a problem hiding this comment.
I'm not following. Can you be more specific?
jhixson74
force-pushed
the
main-ipv6-azure
branch
5 times, most recently
from
a880127 to
e6b588b
Compare
jhixson74
force-pushed
the
main-ipv6-azure
branch
from
e6b588b to
308eb09
Compare
patrickdillon
left a comment
patrickdillon
left a comment
There was a problem hiding this comment.
A few things I noticed when reviewing:
- aso crds are included, but can be removed
- this doesn't make use of the
IPFamiliesfield that was introduced, which isn't strictly necessary in this pr, but perhaps there are some places where it would be handy?
| } | ||
|
|
||
| // DeepCopyInto deep copies an AddressFamilySubnet struct. | ||
| func (in *AddressFamilySubnet) DeepCopyInto(out *AddressFamilySubnet) { |
There was a problem hiding this comment.
This doesn't look right. I think the DeepCopyInto function should be in a zz generated file?
There was a problem hiding this comment.
The generated file does not generate the correct code. This is why I've manually done this here.
pkg/asset/manifests/azure/cluster.go
Outdated
| // If we are using Internal publishing, we need a security rule for each CIDR | ||
| var securityRules []capz.SecurityRule | ||
| var securityRulePriority int32 = 100 | ||
| if addressFamilySubnets.IPv4Count() > 0 && installConfig.Config.Publish == types.InternalPublishingStrategy { |
There was a problem hiding this comment.
you want to use the install config's PublicAPI() function here to account for the MixedPublishing strategy use case
There was a problem hiding this comment.
So, looking at the existing code, we aren't accounting for the MixedPublishing strategy, which is what this code is based on.
source := "*"
if installConfig.Config.Publish == types.InternalPublishingStrategy {
source = mainCIDR
}
Since we aren't currently handling it, does that mean this is a bug?
pkg/asset/manifests/azure/cluster.go
Outdated
| return nil, err | ||
| // IPv6 outbound load balancer for worker subnet(s) | ||
| var nodeOutboundLB *capz.LoadBalancerSpec | ||
| if addressFamilySubnets.IsDualStack() { |
There was a problem hiding this comment.
Can you provide more context for why we need this outbound lb for ipv6? Can we not use the same pattern we've been using for single stack ipv4?
There was a problem hiding this comment.
We are currently using a NAT gateway for the default outbound networking for the worker subnet. v1 NAT gatways do not support IPv6 addresses. So, we need a way for IPv6 to get outbound, that is what this is for. This isn't necessary with v2 NAT gateways. However, CAPZ does not currently support v2 NAT gateways.
I don't understand what this means. You want this PR to remove ASO CRD's somewhere?
Where would you like it to be used? |
jhixson74
force-pushed
the
main-ipv6-azure
branch
from
f038ee4 to
c7da64a
Compare
WalkthroughReplaces flat CIDR handling with address‑family (IPv4/IPv6) aware subnets via new AddressFamilySubnets, updates cluster manifest generation to emit mixed-stack subnet specs and per‑CIDR security rules, revises NAT/LB provisioning for dual‑stack, and adds helpers to collect MachineNetwork CIDRs and VNet prefixes. Changes
Sequence Diagram(s)sequenceDiagram
participant InstallConfig
participant AFParser as AddressFamilySubnets
participant ManifestGen as ClusterManifestGenerator
participant AzureVNet as AzureVNet
participant AzureNSG as AzureNSG
participant AzureNAT as AzureNATGateway
participant AzureLB as AzureLoadBalancer
InstallConfig->>AFParser: Provide MachineNetwork CIDRs (CIDRsFromInstallConfig)
AFParser-->>InstallConfig: Return AddressFamilySubnets (paired IPv4/IPv6, roles)
InstallConfig->>ManifestGen: Provide install config + AF subnets
ManifestGen->>AzureVNet: getVirtualNetworkAddressPrefixes -> Create VNet with IPv4/IPv6 prefixes
ManifestGen->>AzureVNet: Create subnets (subnet_01, subnet_02...) from AF subnets
ManifestGen->>AzureNSG: Generate per‑CIDR security rules (apiserver_in/ssh_in) with incremental priorities
ManifestGen->>AzureNAT: Decide/attach NAT gateway(s) per-address-family and per-subnet as needed
ManifestGen->>AzureLB: Create API server LB frontends (IPv4 and optional IPv6) and outbound LBs for IPv6 outbound when required
AzureLB-->>ManifestGen: Return frontend IP assignments
AzureNAT-->>ManifestGen: Confirm NAT gateway assignments
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
@jhixson74: This pull request references CORS-3897 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
hack/build.sh (1)
30-36: Consider separating build parallelization changes.Per the PR discussion, the reviewer (patrickdillon) suggested that build-related changes should be handled separately from the dual-stack functional changes. The author acknowledged these changes "slipped into the PR."
The changes themselves are functionally reasonable for parallelization, but may benefit from being in a dedicated PR for cleaner review and change tracking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/build.sh` around lines 30 - 36, The build parallelization changes (export GOBUILDFLAGS, export MAKEFLAGS and the make ${MAKEFLAGS} -C cluster-api all invocation) were introduced alongside dual-stack functional changes and should be moved into a separate, focused commit/PR; revert or remove the parallelization exports and the modified make invocation from this branch (restore prior behavior), then create a new branch/PR that adds GOBUILDFLAGS/MAKEFLAGS exports and updates the make call (make ${MAKEFLAGS} -C cluster-api all) with a clear description of the parallelization intent so reviewers can evaluate that change independently of the dual-stack code.hack/build-cluster-api.sh (2)
12-12: Clarify intent of-j1flag.The flag
-j1combines-j(junk directory names) with-1(fastest compression). If the intent is just to junk paths without specifying compression level, use-jalone. If fastest compression is desired, this is fine but could be clearer as separate flags-j -1for readability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/build-cluster-api.sh` at line 12, The ZIPFLAGS value currently set as ZIPFLAGS="-j1" conflates the -j (junk paths) and -1 (fastest compression) flags; update ZIPFLAGS to make intent explicit by either using ZIPFLAGS="-j" if you only want to junk paths, or ZIPFLAGS="-j -1" (or ZIPFLAGS="-j" and ZIPLEVEL="-1" if you prefer separate variables) if you intend both junking paths and fastest compression, and update any code that consumes ZIPFLAGS accordingly.
33-36: Fix inconsistent indentation.Line 35 uses 8-space indentation while the rest of the file uses 2-space indentation.
🧹 Proposed fix
# Nothing to do if [ "${RET}" = "12" ]; then - return 0 + return 0 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/build-cluster-api.sh` around lines 33 - 36, Fix the inconsistent indentation in the conditional block that checks the RET variable: align the indentation for the "return 0" line to use 2 spaces like the rest of the script (the block starting with if [ "${RET}" = "12" ]; then and containing return 0) so all lines in that conditional use consistent 2-space indentation.pkg/types/azure/network.go (1)
169-347: LGTM with optional refactoring note.The accessor methods are consistent and correct. There's noticeable duplication between IPv4 and IPv6 variants (e.g.,
GetIPv4ComputeSubnetsvsGetIPv6ComputeSubnets), which could potentially be consolidated using generics or a helper function, but this is acceptable for clarity and type safety.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/types/azure/network.go` around lines 169 - 347, The subnet accessor methods duplicate IPv4/IPv6 logic; refactor by extracting a small helper used by GetIPv4ComputeSubnets/GetIPv6ComputeSubnets, GetIPv4AdditionalSubnets/GetIPv6AdditionalSubnets, and the CIDR getters to reduce repetition: create a helper (e.g., collectIPNets or collectCIDRs) that accepts a slice of AddressFamilySubnet and a selector/flag to return either []*net.IPNet or []string, then call that helper from the named methods (GetComputeSubnets, GetAdditionalSubnets, GetIPv4ComputeCIDRBlocks, GetIPv6ComputeCIDRBlocks, etc.) to centralize the filtering and nil checks.pkg/asset/manifests/azure/cluster.go (1)
46-135: LGTM with minor inconsistency note.The security rule generation correctly handles both default (public) and per-CIDR (internal) publishing strategies. The priority management is sound.
Minor: The code mixes
ptr.To(k8s.io/utils/ptr, line 52) andto.Ptr(Azure SDK, line 82) for creating pointers. Consider standardizing on one for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/manifests/azure/cluster.go` around lines 46 - 135, The code mixes two pointer helpers (ptr.To and to.Ptr) causing inconsistency; standardize to the k8s helper used in defaultSecurityRules by replacing to.Ptr(...) with ptr.To(...) in the per-CIDR security rule construction (the blocks that append to securityRules for IPv4/IPv6 where Name is formatted like "apiserver_in_ipv4_%02d" / "ssh_in_ipv6_%02d"), and then update imports accordingly (remove or adjust the azure/sdk to alias if it becomes unused) so all SecurityRule Source/Destination/Ports use ptr.To.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hack/build.sh`:
- Around line 23-28: The regex used to parse MAKEFLAGS into MAKEJOBS in
hack/build.sh is incorrect: change the sed pattern to accept any digits and
optional space after -j, e.g., update the capture from [1-9]+ to [0-9]+ and from
\s+? to \s*; also remove the extra quotes around ${MAKEFLAGS} in the echo and
quote ${MAKEJOBS} in the numeric test to safely check its value before appending
to GOBUILDFLAGS (refer to MAKEFLAGS, MAKEJOBS, and GOBUILDFLAGS in the script).
In `@pkg/asset/manifests/azure/cluster.go`:
- Around line 680-692: The getVirtualNetworkAddressPrefixes function currently
appends hard-coded "10.1.0.0/16" and "10.2.0.0/16" which is undocumented and can
overlap user CIDRs; remove these magic CIDRs from
getVirtualNetworkAddressPrefixes and instead (a) if they are required by CAPZ
behavior, surface them as configurable constants or fields (e.g.,
DefaultNodeSubnet/AdditionalVNetPrefixes) with clear comments and/or config
flags, (b) add validation in the same function to detect and deduplicate
overlaps against the provided cidrs slice (return an error or log and skip
conflicting prefixes), and (c) update documentation/comments to explain the
source/purpose if kept. Ensure references to getVirtualNetworkAddressPrefixes
and any new constant/field names are used so reviewers can find the change.
In `@pkg/types/azure/network.go`:
- Around line 156-167: The counters are incremented by the full number of new
split subnets but the original compute subnet is removed, so adjust the
increments to add (N - 1) instead of N: in the method that performs the
replacement (the block that updates a.addressFamilySubnets and then updates
a.length, a.ipv4Count, a.ipv6Count), change the increments to subtract 1 from
the added counts (e.g., use len(ipv4ComputeSubnets) - 1 and ipv4Count - 1 /
ipv6Count - 1 as appropriate) so the removed original subnet is not
double-counted.
- Around line 356-363: The loop that classifies cidrs into ipv4Cidrs and
ipv6Cidrs uses len(cidr.IP) which is unreliable; change the classification to
use cidr.IP.To4() != nil for IPv4 detection and append to ipv4Cidrs, otherwise
append to ipv6Cidrs (replace the switch over net.IPv4len/net.IPv6len); reference
the variables ipv4Cidrs, ipv6Cidrs and the cidr.IP.To4() method when making the
change.
---
Nitpick comments:
In `@hack/build-cluster-api.sh`:
- Line 12: The ZIPFLAGS value currently set as ZIPFLAGS="-j1" conflates the -j
(junk paths) and -1 (fastest compression) flags; update ZIPFLAGS to make intent
explicit by either using ZIPFLAGS="-j" if you only want to junk paths, or
ZIPFLAGS="-j -1" (or ZIPFLAGS="-j" and ZIPLEVEL="-1" if you prefer separate
variables) if you intend both junking paths and fastest compression, and update
any code that consumes ZIPFLAGS accordingly.
- Around line 33-36: Fix the inconsistent indentation in the conditional block
that checks the RET variable: align the indentation for the "return 0" line to
use 2 spaces like the rest of the script (the block starting with if [ "${RET}"
= "12" ]; then and containing return 0) so all lines in that conditional use
consistent 2-space indentation.
In `@hack/build.sh`:
- Around line 30-36: The build parallelization changes (export GOBUILDFLAGS,
export MAKEFLAGS and the make ${MAKEFLAGS} -C cluster-api all invocation) were
introduced alongside dual-stack functional changes and should be moved into a
separate, focused commit/PR; revert or remove the parallelization exports and
the modified make invocation from this branch (restore prior behavior), then
create a new branch/PR that adds GOBUILDFLAGS/MAKEFLAGS exports and updates the
make call (make ${MAKEFLAGS} -C cluster-api all) with a clear description of the
parallelization intent so reviewers can evaluate that change independently of
the dual-stack code.
In `@pkg/asset/manifests/azure/cluster.go`:
- Around line 46-135: The code mixes two pointer helpers (ptr.To and to.Ptr)
causing inconsistency; standardize to the k8s helper used in
defaultSecurityRules by replacing to.Ptr(...) with ptr.To(...) in the per-CIDR
security rule construction (the blocks that append to securityRules for
IPv4/IPv6 where Name is formatted like "apiserver_in_ipv4_%02d" /
"ssh_in_ipv6_%02d"), and then update imports accordingly (remove or adjust the
azure/sdk to alias if it becomes unused) so all SecurityRule
Source/Destination/Ports use ptr.To.
In `@pkg/types/azure/network.go`:
- Around line 169-347: The subnet accessor methods duplicate IPv4/IPv6 logic;
refactor by extracting a small helper used by
GetIPv4ComputeSubnets/GetIPv6ComputeSubnets,
GetIPv4AdditionalSubnets/GetIPv6AdditionalSubnets, and the CIDR getters to
reduce repetition: create a helper (e.g., collectIPNets or collectCIDRs) that
accepts a slice of AddressFamilySubnet and a selector/flag to return either
[]*net.IPNet or []string, then call that helper from the named methods
(GetComputeSubnets, GetAdditionalSubnets, GetIPv4ComputeCIDRBlocks,
GetIPv6ComputeCIDRBlocks, etc.) to centralize the filtering and nil checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c4fe704f-85ec-4c68-943d-30deca68aa7c
⛔ Files ignored due to path filters (1)
pkg/types/azure/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (7)
cluster-api/Makefiledata/data/cluster-api/azureaso-infrastructure-components.yamlhack/build-cluster-api.shhack/build.shpkg/asset/manifests/azure/cluster.gopkg/asset/manifests/capiutils/helpers.gopkg/types/azure/network.go
jhixson74
force-pushed
the
main-ipv6-azure
branch
from
c7da64a to
63bd624
Compare
|
@jhixson74: This pull request references CORS-3897 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/types/azure/network.go (1)
39-43: Variable shadowing reduces readability.Lines 40-42 shadow the receiver parameter names
inandoutwith local variables. While this is a common Go deep-copy pattern, it can be confusing. Consider using distinct names likesrcRoleanddstRole.♻️ Suggested refactor
if in.SubnetRole != nil { - in, out := &in.SubnetRole, &out.SubnetRole - *out = new(capz.SubnetRole) - **out = **in + out.SubnetRole = new(capz.SubnetRole) + *out.SubnetRole = *in.SubnetRole }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/types/azure/network.go` around lines 39 - 43, The local variables in the deep-copy block inside the DeepCopyInto (or similar) method shadow the receiver params `in` and `out`; rename the locals to avoid shadowing (e.g., use `srcRole` and `dstRole`) and update the assignments accordingly so the logic still allocates a new capz.SubnetRole and copies from the source to destination (replace the `in, out := &in.SubnetRole, &out.SubnetRole` pattern and the subsequent `*out = *in` with the new names to improve readability).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/asset/manifests/azure/cluster.go`:
- Around line 408-413: The code initializes controlPlaneCIDRBlocks with
ipv4ControlPlaneCIDRBlocks even when it's an empty string, producing
[]string{""}; change the logic in the block that builds controlPlaneCIDRBlocks
(using addressFamilySubnets.GetIPv4ControlPlaneCIDRBlocks() and
GetIPv6ControlPlaneCIDRBlocks()) to start with an empty slice and append
ipv4ControlPlaneCIDRBlocks and ipv6ControlPlaneCIDRBlocks only if each string is
non-empty (i.e., check != ""), ensuring no empty-string elements are added to
controlPlaneCIDRBlocks.
---
Nitpick comments:
In `@pkg/types/azure/network.go`:
- Around line 39-43: The local variables in the deep-copy block inside the
DeepCopyInto (or similar) method shadow the receiver params `in` and `out`;
rename the locals to avoid shadowing (e.g., use `srcRole` and `dstRole`) and
update the assignments accordingly so the logic still allocates a new
capz.SubnetRole and copies from the source to destination (replace the `in, out
:= &in.SubnetRole, &out.SubnetRole` pattern and the subsequent `*out = *in` with
the new names to improve readability).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8c264781-8575-4946-a276-bbd93be09873
⛔ Files ignored due to path filters (1)
pkg/types/azure/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (4)
data/data/cluster-api/azureaso-infrastructure-components.yamlpkg/asset/manifests/azure/cluster.gopkg/asset/manifests/capiutils/helpers.gopkg/types/azure/network.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/asset/manifests/capiutils/helpers.go
jhixson74
force-pushed
the
main-ipv6-azure
branch
from
11e4b6f to
91dd9be
Compare
|
@jhixson74: This pull request references CORS-3897 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.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. |
jhixson74
force-pushed
the
main-ipv6-azure
branch
from
91dd9be to
465a5df
Compare
|
@jhixson74: This pull request references CORS-3897 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/asset/manifests/azure/cluster.go (1)
71-133:⚠️ Potential issue | 🟠 MajorUse API visibility, not
Config.Publish, to decide whether to restrict ingress.Line 74 and Line 103 still key off
installConfig.Config.Publish == types.InternalPublishingStrategy. A MixedPublishing install with an internal API will miss these per-CIDR rules and fall through to theSource: "*"defaults on Lines 132-133, which is broader than intended.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/manifests/azure/cluster.go` around lines 71 - 133, The code currently gates per-CIDR security rules on installConfig.Config.Publish == types.InternalPublishingStrategy (seen in the IPv4 and IPv6 blocks); change those conditions to check the API publishing/visibility setting instead (i.e., whether the API is configured as internal) so MixedPublishing with an internal API still gets per-CIDR rules. Update the two checks that reference installConfig.Config.Publish to use the API visibility/publishing config (or the installer helper that determines "is API internal") and leave the rest of the logic that builds securityRules, securityRulePriority, addressFamilySubnets, and defaultSecurityRules unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/asset/manifests/azure/cluster.go`:
- Around line 219-229: The IPv6 node outbound LB is being created
unconditionally for dual-stack clusters; update the logic so nodeOutboundLB is
not created when installConfig.Config.Platform.Azure.OutboundType equals
azure.UserDefinedRoutingOutboundType (same check used to set
controlPlaneOutboundLB to nil). In practice, wrap the
addressFamilySubnets.IsDualStack() nodeOutboundLB creation with the
outbound-type check (or add a guard returning nil when OutboundType ==
azure.UserDefinedRoutingOutboundType) so that nodeOutboundLB remains nil for
user-defined routing; reference
installConfig.Config.Platform.Azure.OutboundType,
azure.UserDefinedRoutingOutboundType, nodeOutboundLB, and
addressFamilySubnets.IsDualStack() when making the change.
---
Duplicate comments:
In `@pkg/asset/manifests/azure/cluster.go`:
- Around line 71-133: The code currently gates per-CIDR security rules on
installConfig.Config.Publish == types.InternalPublishingStrategy (seen in the
IPv4 and IPv6 blocks); change those conditions to check the API
publishing/visibility setting instead (i.e., whether the API is configured as
internal) so MixedPublishing with an internal API still gets per-CIDR rules.
Update the two checks that reference installConfig.Config.Publish to use the API
visibility/publishing config (or the installer helper that determines "is API
internal") and leave the rest of the logic that builds securityRules,
securityRulePriority, addressFamilySubnets, and defaultSecurityRules unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4b05b7a1-4eb8-4a0a-b784-1f4a710278ee
⛔ Files ignored due to path filters (1)
pkg/types/azure/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (3)
data/data/cluster-api/azureaso-infrastructure-components.yamlpkg/asset/manifests/azure/cluster.gopkg/asset/manifests/capiutils/helpers.go
jhixson74
force-pushed
the
main-ipv6-azure
branch
from
465a5df to
d63f487
Compare
|
@jhixson74: This pull request references CORS-3897 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.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. |
jhixson74
force-pushed
the
main-ipv6-azure
branch
from
d63f487 to
6ba1cf4
Compare
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
- Support IPv4 and IPv6 CIDRs - Set VNet CIDR blocks - Set subnet CIDR blocks https://issues.redhat.com/browse/CORS-3897
jhixson74
force-pushed
the
main-ipv6-azure
branch
from
6ba1cf4 to
6377401
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@jhixson74: This pull request references CORS-3897 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/asset/manifests/azure/cluster.go`:
- Around line 448-465: The loop that builds extra node subnets (building
capz.SubnetSpec entries in subnetSpec) omits assigning the SecurityGroup, so the
generated subnet_%02d entries lack the NSG; update the code that constructs spec
(the capz.SubnetSpec with SubnetClassSpec{Name: fmt.Sprintf("subnet_%02d", i),
Role: capz.SubnetNode}) to set spec.SecurityGroup = securityGroup (or the
appropriate securityGroup variable used elsewhere) before appending to
subnetSpec so these autogenerated extra node subnets inherit the same NSG as the
other installer-generated subnets.
- Around line 70-134: The per-CIDR NSG rule blocks are currently gated by
installConfig.Config.PublicAPI(), which is inverted: change both guards (the
IPv4 block using addressFamilySubnets.IPv4Count() &&
installConfig.Config.PublicAPI() and the IPv6 block using
addressFamilySubnets.IPv6Count() && installConfig.Config.PublicAPI()) to run
when the API is NOT public (use !installConfig.Config.PublicAPI() or
installConfig.Config.PublicAPI() == false) so that the restrictive per-CIDR
capz.SecurityRule entries (apiserver_in_ipv4_*, ssh_in_ipv4_*,
apiserver_in_ipv6_*, ssh_in_ipv6_*) are created for internal publishing and
public/mixed clusters fall back to defaultSecurityRules.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e217051a-4701-449d-8182-818c9d9d6b09
⛔ Files ignored due to path filters (1)
pkg/types/azure/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (3)
data/data/cluster-api/azureaso-infrastructure-components.yamlpkg/asset/manifests/azure/cluster.gopkg/asset/manifests/capiutils/helpers.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/asset/manifests/capiutils/helpers.go
| // If we are using Internal publishing, we need a security rule for each CIDR | ||
| var securityRules []capz.SecurityRule | ||
| var securityRulePriority int32 = 100 | ||
| //if addressFamilySubnets.IPv4Count() > 0 && installConfig.Config.Publish == types.InternalPublishingStrategy { | ||
| if addressFamilySubnets.IPv4Count() > 0 && installConfig.Config.PublicAPI() { | ||
| for i := 0; i < addressFamilySubnets.IPv4Count(); i++ { | ||
| securityRules = append(securityRules, capz.SecurityRule{ | ||
| Name: fmt.Sprintf("apiserver_in_ipv4_%02d", i), | ||
| Protocol: capz.SecurityGroupProtocolTCP, | ||
| Direction: capz.SecurityRuleDirectionInbound, | ||
| SourcePorts: ptr.To("*"), | ||
| DestinationPorts: ptr.To("6443"), | ||
| Source: ptr.To(addressFamilySubnets.GetIPv4Subnets()[i].String()), | ||
| Destination: ptr.To("*"), | ||
| Priority: securityRulePriority, | ||
| Action: capz.SecurityRuleActionAllow, | ||
| }) | ||
| securityRulePriority += 10 | ||
|
|
||
| securityRules = append(securityRules, capz.SecurityRule{ | ||
| Name: fmt.Sprintf("ssh_in_ipv4_%02d", i), | ||
| Protocol: capz.SecurityGroupProtocolTCP, | ||
| Direction: capz.SecurityRuleDirectionInbound, | ||
| SourcePorts: ptr.To("*"), | ||
| DestinationPorts: ptr.To("22"), | ||
| Source: ptr.To(addressFamilySubnets.GetIPv4Subnets()[i].String()), | ||
| Destination: ptr.To("*"), | ||
| Priority: securityRulePriority, | ||
| Action: capz.SecurityRuleActionAllow, | ||
| }) | ||
| securityRulePriority += 10 | ||
| } | ||
| } | ||
| if addressFamilySubnets.IPv6Count() > 0 && installConfig.Config.PublicAPI() { | ||
| for i := 0; i < addressFamilySubnets.IPv6Count(); i++ { | ||
| securityRules = append(securityRules, capz.SecurityRule{ | ||
| Name: fmt.Sprintf("apiserver_in_ipv6_%02d", i), | ||
| Protocol: capz.SecurityGroupProtocolTCP, | ||
| Direction: capz.SecurityRuleDirectionInbound, | ||
| SourcePorts: ptr.To("*"), | ||
| DestinationPorts: ptr.To("6443"), | ||
| Source: ptr.To(addressFamilySubnets.GetIPv6Subnets()[i].String()), | ||
| Destination: ptr.To("*"), | ||
| Priority: securityRulePriority, | ||
| Action: capz.SecurityRuleActionAllow, | ||
| }) | ||
| securityRulePriority += 10 | ||
|
|
||
| securityRules = append(securityRules, capz.SecurityRule{ | ||
| Name: fmt.Sprintf("ssh_in_ipv6_%02d", i), | ||
| Protocol: capz.SecurityGroupProtocolTCP, | ||
| Direction: capz.SecurityRuleDirectionInbound, | ||
| SourcePorts: ptr.To("*"), | ||
| DestinationPorts: ptr.To("22"), | ||
| Source: ptr.To(addressFamilySubnets.GetIPv6Subnets()[i].String()), | ||
| Destination: ptr.To("*"), | ||
| Priority: securityRulePriority, | ||
| Action: capz.SecurityRuleActionAllow, | ||
| }) | ||
| securityRulePriority += 10 | ||
| } | ||
| } | ||
| if len(securityRules) == 0 { | ||
| securityRules = append(securityRules, defaultSecurityRules...) | ||
| } |
There was a problem hiding this comment.
Invert the PublicAPI() guard on the CIDR-scoped NSG rules.
Lines 74 and 103 currently build the restrictive per-CIDR ingress rules when PublicAPI() is true, which flips the intended behavior: public/mixed clusters lose the * ingress rules, while internal clusters fall back to the open defaults below. That can break public API reachability and weakens the internal-publishing path.
🐛 Proposed fix
- if addressFamilySubnets.IPv4Count() > 0 && installConfig.Config.PublicAPI() {
+ if addressFamilySubnets.IPv4Count() > 0 && !installConfig.Config.PublicAPI() {
for i := 0; i < addressFamilySubnets.IPv4Count(); i++ {
securityRules = append(securityRules, capz.SecurityRule{
Name: fmt.Sprintf("apiserver_in_ipv4_%02d", i),
@@
- if addressFamilySubnets.IPv6Count() > 0 && installConfig.Config.PublicAPI() {
+ if addressFamilySubnets.IPv6Count() > 0 && !installConfig.Config.PublicAPI() {
for i := 0; i < addressFamilySubnets.IPv6Count(); i++ {
securityRules = append(securityRules, capz.SecurityRule{
Name: fmt.Sprintf("apiserver_in_ipv6_%02d", i),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/asset/manifests/azure/cluster.go` around lines 70 - 134, The per-CIDR NSG
rule blocks are currently gated by installConfig.Config.PublicAPI(), which is
inverted: change both guards (the IPv4 block using
addressFamilySubnets.IPv4Count() && installConfig.Config.PublicAPI() and the
IPv6 block using addressFamilySubnets.IPv6Count() &&
installConfig.Config.PublicAPI()) to run when the API is NOT public (use
!installConfig.Config.PublicAPI() or installConfig.Config.PublicAPI() == false)
so that the restrictive per-CIDR capz.SecurityRule entries (apiserver_in_ipv4_*,
ssh_in_ipv4_*, apiserver_in_ipv6_*, ssh_in_ipv6_*) are created for internal
publishing and public/mixed clusters fall back to defaultSecurityRules.
| subnetSpec := []capz.SubnetSpec{} | ||
| subnetsCount := max(len(ipv4Subnets), len(ipv6Subnets)) | ||
| for i := 0; i < subnetsCount; i++ { | ||
| spec := capz.SubnetSpec{ | ||
| SubnetClassSpec: capz.SubnetClassSpec{ | ||
| Name: fmt.Sprintf("subnet_%02d", i), | ||
| Role: capz.SubnetNode, | ||
| }, | ||
| } | ||
| var cidrBlocks []string | ||
| if i < len(ipv4Subnets) { | ||
| cidrBlocks = append(cidrBlocks, ipv4Subnets[i].String()) | ||
| } | ||
| if i < len(ipv6Subnets) { | ||
| cidrBlocks = append(cidrBlocks, ipv6Subnets[i].String()) | ||
| } | ||
| spec.CIDRBlocks = cidrBlocks | ||
| subnetSpec = append(subnetSpec, spec) |
There was a problem hiding this comment.
Attach the generated NSG to the autogenerated extra node subnets.
Line 451 creates subnet_%02d without SecurityGroup: securityGroup. Every other installer-generated subnet in this function carries the NSG, so this path emits manifests where the extra generated node subnets do not inherit the security rules assembled above.
🐛 Proposed fix
spec := capz.SubnetSpec{
SubnetClassSpec: capz.SubnetClassSpec{
Name: fmt.Sprintf("subnet_%02d", i),
Role: capz.SubnetNode,
},
+ SecurityGroup: securityGroup,
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/asset/manifests/azure/cluster.go` around lines 448 - 465, The loop that
builds extra node subnets (building capz.SubnetSpec entries in subnetSpec) omits
assigning the SecurityGroup, so the generated subnet_%02d entries lack the NSG;
update the code that constructs spec (the capz.SubnetSpec with
SubnetClassSpec{Name: fmt.Sprintf("subnet_%02d", i), Role: capz.SubnetNode}) to
set spec.SecurityGroup = securityGroup (or the appropriate securityGroup
variable used elsewhere) before appending to subnetSpec so these autogenerated
extra node subnets inherit the same NSG as the other installer-generated
subnets.
5 participants
Summary by CodeRabbit