CORS-3893: Create nat rule and associate to NIC#10361
CORS-3893: Create nat rule and associate to NIC#10361rna-afk wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@rna-afk: This pull request references CORS-3893 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 |
|
/retest-required |
|
Looks right to me. Tests need to pass. |
Adding IPv6 NAT rule for bootstrap SSH access and updating NAT rules to the correct IP version on the bootstrap NIC.
rna-afk
force-pushed
the
azure_dualstack_add_nat_rule
branch
from
776e3e0 to
bf7dea8
Compare
WalkthroughIn Azure infrastructure provisioning, when dual-stack is enabled with public API configured, the code now creates and associates an IPv6 inbound NAT rule for SSH on the public load balancer during the PostProvision operation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 10✅ Passed checks (10 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.11.4)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 |
|
@rna-afk: This pull request references CORS-3893 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: 1
🤖 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/infrastructure/azure/azure.go`:
- Around line 656-687: PostDestroy currently only removes the IPv4 SSH NAT rule;
add symmetric cleanup for the IPv6 rule named fmt.Sprintf("%s_ssh_in_v6") inside
the PostDestroy function: locate the IPv4 cleanup block that deletes
`${InfraID}_ssh_in` and mirror it to first disassociate the IPv6 inbound NAT
from the bootstrap NIC (use the same pattern/clients as
associateInboundNatRuleToInterface/inboundNatRuleInput but calling your
disassociation helper) and then delete the IPv6 inbound NAT rule from the load
balancer (use the same deletion helper used for IPv4 cleanup), passing the same
resourceGroupName, loadBalancerName, frontendIPv6ConfigID and
p.NetworkClientFactory so the `${InfraID}_ssh_in_v6` rule and its NIC
association are fully removed.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: edf77614-420a-462f-9cbd-d55c647ec901
📒 Files selected for processing (1)
pkg/infrastructure/azure/azure.go
| sshRuleNameV6 := fmt.Sprintf("%s_ssh_in_v6", in.InfraID) | ||
| frontendIPv6ConfigID := fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Network/loadBalancers/%s/frontendIPConfigurations/%s", | ||
| subscriptionID, | ||
| p.ResourceGroupName, | ||
| loadBalancerName, | ||
| frontendIPv6ConfigName, | ||
| ) | ||
|
|
||
| inboundNatRuleV6, err := addInboundNatRuleToLoadBalancer(ctx, &inboundNatRuleInput{ | ||
| resourceGroupName: p.ResourceGroupName, | ||
| loadBalancerName: loadBalancerName, | ||
| frontendIPConfigID: frontendIPv6ConfigID, | ||
| inboundNatRuleName: sshRuleNameV6, | ||
| inboundNatRulePort: 22, | ||
| networkClientFactory: p.NetworkClientFactory, | ||
| }) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create IPv6 SSH inbound nat rule: %w", err) | ||
| } | ||
| _, err = associateInboundNatRuleToInterface(ctx, &inboundNatRuleInput{ | ||
| resourceGroupName: p.ResourceGroupName, | ||
| loadBalancerName: loadBalancerName, | ||
| bootstrapNicName: fmt.Sprintf("%s-bootstrap-nic", in.InfraID), | ||
| frontendIPConfigID: frontendIPv6ConfigID, | ||
| inboundNatRuleID: *inboundNatRuleV6.ID, | ||
| inboundNatRuleName: sshRuleNameV6, | ||
| inboundNatRulePort: 22, | ||
| networkClientFactory: p.NetworkClientFactory, | ||
| }) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to associate IPv6 SSH inbound nat rule to interface: %w", err) | ||
| } |
There was a problem hiding this comment.
Add cleanup for the new IPv6 SSH NAT rule in PostDestroy.
Line 656 introduces ${InfraID}_ssh_in_v6, but PostDestroy only deletes ${InfraID}_ssh_in (IPv4). This leaves a stale IPv6 inbound NAT rule and unnecessary SSH exposure on the IPv6 frontend after bootstrap teardown.
💡 Proposed fix
--- a/pkg/infrastructure/azure/azure.go
+++ b/pkg/infrastructure/azure/azure.go
@@
sshRuleName := fmt.Sprintf("%s_ssh_in", in.Metadata.InfraID)
+ sshRuleNameV6 := fmt.Sprintf("%s_ssh_in_v6", in.Metadata.InfraID)
@@
_, err = networkClientFactory.NewInboundNatRulesClient().Get(
ctx,
resourceGroupName,
in.Metadata.InfraID,
sshRuleName,
nil,
)
if err == nil {
err = deleteInboundNatRule(ctx, &inboundNatRuleInput{
resourceGroupName: resourceGroupName,
loadBalancerName: in.Metadata.InfraID,
inboundNatRuleName: sshRuleName,
networkClientFactory: networkClientFactory,
})
if err != nil {
return fmt.Errorf("failed to delete inbound nat rule: %w", err)
}
}
+
+ _, err = networkClientFactory.NewInboundNatRulesClient().Get(
+ ctx,
+ resourceGroupName,
+ in.Metadata.InfraID,
+ sshRuleNameV6,
+ nil,
+ )
+ if err == nil {
+ err = deleteInboundNatRule(ctx, &inboundNatRuleInput{
+ resourceGroupName: resourceGroupName,
+ loadBalancerName: in.Metadata.InfraID,
+ inboundNatRuleName: sshRuleNameV6,
+ networkClientFactory: networkClientFactory,
+ })
+ if err != nil {
+ return fmt.Errorf("failed to delete IPv6 inbound nat rule: %w", err)
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/infrastructure/azure/azure.go` around lines 656 - 687, PostDestroy
currently only removes the IPv4 SSH NAT rule; add symmetric cleanup for the IPv6
rule named fmt.Sprintf("%s_ssh_in_v6") inside the PostDestroy function: locate
the IPv4 cleanup block that deletes `${InfraID}_ssh_in` and mirror it to first
disassociate the IPv6 inbound NAT from the bootstrap NIC (use the same
pattern/clients as associateInboundNatRuleToInterface/inboundNatRuleInput but
calling your disassociation helper) and then delete the IPv6 inbound NAT rule
from the load balancer (use the same deletion helper used for IPv4 cleanup),
passing the same resourceGroupName, loadBalancerName, frontendIPv6ConfigID and
p.NetworkClientFactory so the `${InfraID}_ssh_in_v6` rule and its NIC
association are fully removed.
|
@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
Adding IPv6 NAT rule for bootstrap SSH access and updating NAT rules to the correct IP version on the bootstrap NIC.
Summary by CodeRabbit