CORS-3898: azure: Fix the dualstack errors#10656
Conversation
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
@rna-afk: This pull request explicitly references no jira issue. 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. |
📝 WalkthroughWalkthroughAzure dual-stack provisioning now uses separate IPv4 and IPv6 infra IDs, updates the bootstrap NIC for IPv6 backend pools, removes outbound IPv6 load balancer handling, and adds Azure-specific dual-stack install-config ordering validation with new tests. ChangesAzure dual-stack provisioning
Dual-stack validation
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ 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.2)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 |
|
/test e2e-azure-ovn-dualstack-ipv6-primary-techpreview |
|
/assign |
rna-afk
force-pushed
the
azure_dualstack_missing_parts
branch
from
7271f50 to
2836f66
Compare
rna-afk
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
@rna-afk: This pull request references CORS-3898 which is a valid jira issue. 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. |
rna-afk
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
rna-afk
force-pushed
the
azure_dualstack_missing_parts
branch
from
2836f66 to
3fa6c43
Compare
rna-afk
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhixson74 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
Will wait for #10654 to test this and verify. Works locally. |
|
/test e2e-azure-ovn-dualstack-ipv6-primary-techpreview |
openshift-ci
Bot
added
the
needs-rebase
rna-afk
force-pushed
the
azure_dualstack_missing_parts
branch
from
3fa6c43 to
96877c1
Compare
|
New changes are detected. LGTM label has been removed. |
openshift-ci
Bot
removed
the
needs-rebase
|
/test e2e-azure-ovn-dualstack-ipv6-primary-techpreview |
Fixing a few errors like DNS conflict with ipv4 and ipv6 entry, setting the right load balancer backend pools and adding a validation check since serviceNetwork ips need to be ipv4 first irrespective of ipfamily primary type.
rna-afk
force-pushed
the
azure_dualstack_missing_parts
branch
from
96877c1 to
4a196af
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/types/validation/installconfig_test.go (1)
1980-2057: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing test coverage for machineNetwork/clusterNetwork ordering under Azure.
These new cases thoroughly cover the serviceNetwork IPv4-first requirement, but none assert what happens to machineNetwork/clusterNetwork ordering when it conflicts with the declared
ipFamily(e.g.,DualStackIPv4Primarywith IPv6-first machineNetwork). Given the behavior gap flagged inpkg/types/validation/installconfig.go(Lines 513-520), an explicit test locking in the current (possibly unintended) behavior would help avoid future regressions either way.Want me to draft a test case exercising this scenario?
🤖 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/types/validation/installconfig_test.go` around lines 1980 - 2057, Add an explicit Azure validation test that covers machineNetwork/clusterNetwork ordering when it conflicts with the configured IP family, using the existing installConfig validation cases in installconfig_test.go. Reuse the Azure dual-stack setup from the nearby serviceNetwork tests, but set up a conflicting IPv6-first machineNetwork or clusterNetwork with DualStackIPv4Primary (or the reverse) and assert the current expected result. Keep the test alongside the existing Azure dual-stack cases so the behavior tied to installConfig validation is locked in by name and scenario.
🤖 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.
Inline comments:
In `@pkg/infrastructure/azure/azure.go`:
- Around line 624-653: The bootstrap IPv6 backend pool association is
incorrectly tied to the PublicAPI-only path, so dual-stack private clusters
never attach the bootstrap NIC’s IPv6 IP configuration to the internal -v6
backend pool. Move the IPv6 NIC update logic around the
bootstrapNicName/nicClient/BeginCreateOrUpdate flow out of the PublicAPI() guard
in the Azure install path so it runs for all dual-stack installs, and keep the
existing IPv6 pool selection by suffix. Add a unit test for the private
dual-stack case to verify the bootstrap NIC is updated with the IPv6 backend
pool attachment.
---
Nitpick comments:
In `@pkg/types/validation/installconfig_test.go`:
- Around line 1980-2057: Add an explicit Azure validation test that covers
machineNetwork/clusterNetwork ordering when it conflicts with the configured IP
family, using the existing installConfig validation cases in
installconfig_test.go. Reuse the Azure dual-stack setup from the nearby
serviceNetwork tests, but set up a conflicting IPv6-first machineNetwork or
clusterNetwork with DualStackIPv4Primary (or the reverse) and assert the current
expected result. Keep the test alongside the existing Azure dual-stack cases so
the behavior tied to installConfig validation is locked in by name and scenario.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 67d62899-17eb-4346-923c-9f252a220f69
📒 Files selected for processing (4)
pkg/infrastructure/azure/azure.gopkg/infrastructure/azure/network.gopkg/types/validation/installconfig.gopkg/types/validation/installconfig_test.go
| if in.InstallConfig.Config.Azure.IPFamily.DualStackEnabled() { | ||
| bootstrapNicName := fmt.Sprintf("%s-bootstrap-nic", in.InfraID) | ||
| nicClient := p.NetworkClientFactory.NewInterfacesClient() | ||
| bootstrapNic, err := nicClient.Get(ctx, p.ResourceGroupName, bootstrapNicName, nil) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get bootstrap nic: %w", err) | ||
| } | ||
| for _, ipconfig := range bootstrapNic.Properties.IPConfigurations { | ||
| if ipconfig.Properties.PrivateIPAddressVersion != nil && *ipconfig.Properties.PrivateIPAddressVersion == armnetwork.IPVersionIPv6 { | ||
| for _, pool := range p.lbBackendAddressPools { | ||
| if pool.Name != nil && strings.HasSuffix(*pool.Name, "-v6") { | ||
| ipconfig.Properties.LoadBalancerBackendAddressPools = append( | ||
| ipconfig.Properties.LoadBalancerBackendAddressPools, | ||
| pool, | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| pollerResp, err := nicClient.BeginCreateOrUpdate(ctx, p.ResourceGroupName, bootstrapNicName, bootstrapNic.Interface, nil) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to update bootstrap nic with IPv6 backend pools: %w", err) | ||
| } | ||
| _, err = pollerResp.PollUntilDone(ctx, nil) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to update bootstrap nic with IPv6 backend pools: %w", err) | ||
| } | ||
| logrus.Debugf("associated bootstrap NIC with IPv6 backend pools") | ||
| } | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Don’t gate bootstrap IPv6 pool association on PublicAPI().
Line 624 is inside the Line 600 public-API branch, but the internal IPv6 load balancer frontend/rule is created for every Azure dual-stack install at Lines 570-579. Private dual-stack clusters will skip attaching the bootstrap IPv6 IP config to the internal -v6 backend pool. Move this block outside the PublicAPI() guard and cover the private dual-stack path with a unit test. As per coding guidelines, “All code under ./cmd/..., ./data/..., and ./pkg/... must have unit tests”.
🤖 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/infrastructure/azure/azure.go` around lines 624 - 653, The bootstrap IPv6
backend pool association is incorrectly tied to the PublicAPI-only path, so
dual-stack private clusters never attach the bootstrap NIC’s IPv6 IP
configuration to the internal -v6 backend pool. Move the IPv6 NIC update logic
around the bootstrapNicName/nicClient/BeginCreateOrUpdate flow out of the
PublicAPI() guard in the Azure install path so it runs for all dual-stack
installs, and keep the existing IPv6 pool selection by suffix. Add a unit test
for the private dual-stack case to verify the bootstrap NIC is updated with the
IPv6 backend pool attachment.
Source: Coding guidelines
|
/test e2e-azure-ovn-dualstack-ipv6-primary-techpreview |
|
/test e2e-azure-ovn-dualstack-ipv4-primary-techpreview |
|
@rna-afk: 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. |
3 participants
Fixing a few errors like DNS conflict with ipv4 and ipv6 entry,
setting the right load balancer backend pools and adding a validation
check since serviceNetwork ips need to be ipv4 first irrespective
of ipfamily primary type.
Summary by CodeRabbit
New Features
Bug Fixes
serviceNetworkon Azure.Tests