CORS-4466: Correctly calculate subnet CIDR length#10542
CORS-4466: Correctly calculate subnet CIDR length#10542bear-redhat wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@bear-redhat: This pull request references CORS-4466 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 bug to target the "5.0.0" version, but no target version was set. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughThis PR modifies AWS subnet allocation logic in ChangesPublic-Only AWS Subnet Mode
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 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.12.1)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/asset/manifests/aws/zones.go (1)
359-364:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEdge-CIDR validation does not account for
isPublicOnlyand will spuriously fail with multiple edge zones.When
isPublicOnly && isPublishingExternal, onlylen(allEdgeZones)edge CIDRs are needed (one public per edge zone), andnumEdgeSubnetsis computed aslen(allEdgeZones) + 1(no doubling). However, the check on Line 362 still requireslen(edgeCIDRs) >= len(allEdgeZones) * 2, mirroring the non‑public‑only path. With ≥2 edge zones this becomes reachable:
- 3 edge zones, public‑only:
numEdgeSubnets = 4→SplitIntoSubnetsIPv4returns 4 → check4 < 3*2 = 6is true → returns"unable to define CIDR blocks to all edge zones for public subnets", even though there are sufficient CIDRs for the public subnets actually being created.The existing test only exercises a single edge zone (
numEdgeSubnets = 2, check2 < 2is false), so this path is not covered.🛠️ Proposed fix: parameterize the expected count by
isPublicOnly- if isPublishingExternal && (len(edgeCIDRs) < (len(allEdgeZones) * 2)) { - return fmt.Errorf("unable to define CIDR blocks to all edge zones for public subnets") + if isPublishingExternal { + expectedPublicEdgeCIDRs := len(allEdgeZones) + if !isPublicOnly { + expectedPublicEdgeCIDRs += len(allEdgeZones) + } + if len(edgeCIDRs) < expectedPublicEdgeCIDRs { + return fmt.Errorf("unable to define CIDR blocks to all edge zones for public subnets") + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/asset/manifests/aws/zones.go` around lines 359 - 364, The public-subnet check incorrectly assumes two CIDRs per edge zone regardless of isPublicOnly; update the second validation to compute the required edge CIDR count based on isPublicOnly (e.g. required := len(allEdgeZones) if isPublicOnly && isPublishingExternal else len(allEdgeZones)*2) and replace the hardcoded (len(allEdgeZones) * 2) comparison with this computed required value so the check on edgeCIDRs uses the correct expected count; adjust references in the same block that use isPublishingExternal, edgeCIDRs, and allEdgeZones (and any related numEdgeSubnets/SplitIntoSubnetsIPv4 logic) to be consistent.
🧹 Nitpick comments (3)
pkg/asset/manifests/aws/zones.go (1)
285-306: 💤 Low valueConsider consolidating the
publicCIDRs/edgeCIDRderivation to reduce branching.The new
if isPublicOnly && isPublishingExternal { ... } else { ... }block, combined withnumSubnetsalready accounting for whether public reservation is needed, makes the index arithmetic harder to follow. A single offset-based computation can express both cases without duplicating the edge-CIDR slot logic, and avoids the deadpublicCIDRcomputation in theisPublicOnly && !isPublishingExternalcorner case.For example:
azCount := len(allAvailabilityZones) if isPublishingExternal { if isPublicOnly { publicCIDRs = privateCIDRs[:azCount] } else { publicCIDR := privateCIDRs[azCount].String() publicCIDRs, err = utilscidr.SplitIntoSubnetsIPv4(publicCIDR, azCount) if err != nil { return fmt.Errorf("unable to generate CIDR blocks for all public subnets: %w", err) } } } if len(allEdgeZones) > 0 { edgeIdx := azCount if isPublishingExternal && !isPublicOnly { edgeIdx++ } edgeCIDR = privateCIDRs[edgeIdx].String() }Not blocking — purely a readability suggestion.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/asset/manifests/aws/zones.go` around lines 285 - 306, Consolidate the branching that computes publicCIDRs and edgeCIDR by using an azCount := len(allAvailabilityZones) and a single offset-based index for the edge slot instead of separate if/else blocks; when isPublishingExternal is true, set publicCIDRs to privateCIDRs[:azCount] if isPublicOnly, otherwise call utilscidr.SplitIntoSubnetsIPv4(privateCIDRs[azCount].String(), azCount) and handle its error, and compute edgeIdx := azCount (increment edgeIdx by 1 if isPublishingExternal && !isPublicOnly) to set edgeCIDR = privateCIDRs[edgeIdx].String() only if len(allEdgeZones) > 0, removing the dead publicCIDR computation path and duplicated logic.pkg/asset/manifests/aws/zones_test.go (2)
754-781: 💤 Low valueTest scaffolding largely duplicates
Test_setSubnetsManagedVPC— consider a shared helper.The post-
setSubnetsManagedVPCplumbing here (nil checks ontt.args.in/Cluster, empty-subnet handling,tSortCapaSubnetsByID, and the finalcmp.Equalcomparison) is identical to the existing test runner inTest_setSubnetsManagedVPC(Lines 588-612). Extracting anassertManagedVPCNetworkSpec(t, in, want, wantErr)helper would deduplicate ~20 lines and ensure both tests stay in sync if the validation logic evolves. Optional cleanup — not blocking.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/asset/manifests/aws/zones_test.go` around lines 754 - 781, Extract the duplicated post-call assertions in Test_setSubnetsManagedVPC and the other test into a single helper, e.g. assertManagedVPCNetworkSpec(t, in, want, wantErr), that calls setSubnetsManagedVPC(in) (or accepts pre-called results), performs the nil checks on tt.args.in and tt.args.in.Cluster, handles the empty-subnet early-return and wantErr logic, runs tSortCapaSubnetsByID on Cluster.Spec.NetworkSpec.Subnets, and does the final cmp.Equal comparison against want; then replace the duplicated block in both tests with a single call to assertManagedVPCNetworkSpec(t, tt.args.in, tt.want, tt.wantErr) to keep behavior identical to the existing checks around setSubnetsManagedVPC, tSortCapaSubnetsByID, and cmp.Diff.
681-752: ⚡ Quick winAdd a public-only test case with multiple edge zones.
Both new cases use at most one edge zone, so
numEdgeSubnetsends at2and the validationlen(edgeCIDRs) < len(allEdgeZones)*2happens to pass (2 < 2is false). With ≥2 edge zones in public-only mode the validation insetSubnetsManagedVPC(Line 362 inzones.go) is reachable and would currently return an error even though enough public-edge CIDRs are available. A test like “public-only /22 with 3 AZs and 2 edge zones” would catch that path and pin down the expected/26CIDR allocations for public edge subnets.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/asset/manifests/aws/zones_test.go` around lines 681 - 752, Add a new table test case in zones_test.go mirroring the "public-only /22 with 3 AZs and edge zone" case but with two edge zones to exercise the validation in setSubnetsManagedVPC: update the test's networkInput.InstallConfig Compute platform to include an AWS MachinePool with Zones: []string{"edge-a","edge-b"}, add "edge-b" to ZonesInRegion, and extend the expected want.Subnets to include both "infra-id-subnet-public-edge-a" and "infra-id-subnet-public-edge-b" with the expected /26 CIDR allocations; this will trigger the numEdgeSubnets/edgeCIDRs vs allEdgeZones path in setSubnetsManagedVPC and pin the correct public-edge CIDR sizes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pkg/asset/manifests/aws/zones.go`:
- Around line 359-364: The public-subnet check incorrectly assumes two CIDRs per
edge zone regardless of isPublicOnly; update the second validation to compute
the required edge CIDR count based on isPublicOnly (e.g. required :=
len(allEdgeZones) if isPublicOnly && isPublishingExternal else
len(allEdgeZones)*2) and replace the hardcoded (len(allEdgeZones) * 2)
comparison with this computed required value so the check on edgeCIDRs uses the
correct expected count; adjust references in the same block that use
isPublishingExternal, edgeCIDRs, and allEdgeZones (and any related
numEdgeSubnets/SplitIntoSubnetsIPv4 logic) to be consistent.
---
Nitpick comments:
In `@pkg/asset/manifests/aws/zones_test.go`:
- Around line 754-781: Extract the duplicated post-call assertions in
Test_setSubnetsManagedVPC and the other test into a single helper, e.g.
assertManagedVPCNetworkSpec(t, in, want, wantErr), that calls
setSubnetsManagedVPC(in) (or accepts pre-called results), performs the nil
checks on tt.args.in and tt.args.in.Cluster, handles the empty-subnet
early-return and wantErr logic, runs tSortCapaSubnetsByID on
Cluster.Spec.NetworkSpec.Subnets, and does the final cmp.Equal comparison
against want; then replace the duplicated block in both tests with a single call
to assertManagedVPCNetworkSpec(t, tt.args.in, tt.want, tt.wantErr) to keep
behavior identical to the existing checks around setSubnetsManagedVPC,
tSortCapaSubnetsByID, and cmp.Diff.
- Around line 681-752: Add a new table test case in zones_test.go mirroring the
"public-only /22 with 3 AZs and edge zone" case but with two edge zones to
exercise the validation in setSubnetsManagedVPC: update the test's
networkInput.InstallConfig Compute platform to include an AWS MachinePool with
Zones: []string{"edge-a","edge-b"}, add "edge-b" to ZonesInRegion, and extend
the expected want.Subnets to include both "infra-id-subnet-public-edge-a" and
"infra-id-subnet-public-edge-b" with the expected /26 CIDR allocations; this
will trigger the numEdgeSubnets/edgeCIDRs vs allEdgeZones path in
setSubnetsManagedVPC and pin the correct public-edge CIDR sizes.
In `@pkg/asset/manifests/aws/zones.go`:
- Around line 285-306: Consolidate the branching that computes publicCIDRs and
edgeCIDR by using an azCount := len(allAvailabilityZones) and a single
offset-based index for the edge slot instead of separate if/else blocks; when
isPublishingExternal is true, set publicCIDRs to privateCIDRs[:azCount] if
isPublicOnly, otherwise call
utilscidr.SplitIntoSubnetsIPv4(privateCIDRs[azCount].String(), azCount) and
handle its error, and compute edgeIdx := azCount (increment edgeIdx by 1 if
isPublishingExternal && !isPublicOnly) to set edgeCIDR =
privateCIDRs[edgeIdx].String() only if len(allEdgeZones) > 0, removing the dead
publicCIDR computation path and duplicated logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 54ff18dc-5a34-42f1-ac7b-3c499df70dd4
📒 Files selected for processing (2)
pkg/asset/manifests/aws/zones.gopkg/asset/manifests/aws/zones_test.go
|
/test e2e-aws-ovn-public-subnets |
|
/approve |
|
@bear-redhat: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
4 participants
Summary by CodeRabbit
Bug Fixes
Tests