no-jira: IPv6 networking functions#10432
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@jhixson74: 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRenamed Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ 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.3)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
pkg/infrastructure/azure/network.go (4)
93-104: Same inconsistency inmcsProbe().This function uses
NumberOfProbes(line 100), but the existing probe inupdateInternalLoadBalancer(line 525) usesProbeThreshold. Consider usingProbeThresholdfor consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/infrastructure/azure/network.go` around lines 93 - 104, The mcsProbe() function uses NumberOfProbes but should use ProbeThreshold to match the probe used in updateInternalLoadBalancer; update mcsProbe() by removing NumberOfProbes and adding ProbeThreshold: set it to to.Ptr[int32](2) (same value as before) on the armnetwork.ProbePropertiesFormat so the probe property names are consistent with updateInternalLoadBalancer.
129-141: Remove commented-out code and verify probe property consistency.Line 131 contains commented-out code that should be removed. Additionally, this
apiProbe()function usesNumberOfProbes(line 137), while the existing inline probe definitions increateAPILoadBalancer(line 378) useProbeThreshold. These are different Azure properties with different semantics:
NumberOfProbes: Number of probes before marking unhealthyProbeThreshold: Threshold for declaring endpoint unhealthyEnsure consistency across probe definitions.
♻️ Proposed cleanup
func apiProbe() *armnetwork.Probe { return &armnetwork.Probe{ - //Name: to.Ptr("api-probe"), Name: to.Ptr("HTTPSProbe"), Properties: &armnetwork.ProbePropertiesFormat{ Protocol: to.Ptr(armnetwork.ProbeProtocolHTTPS), Port: to.Ptr[int32](6443), IntervalInSeconds: to.Ptr[int32](5), - NumberOfProbes: to.Ptr[int32](2), + ProbeThreshold: to.Ptr[int32](2), RequestPath: to.Ptr("/readyz"), }, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/infrastructure/azure/network.go` around lines 129 - 141, Remove the commented-out Name line in apiProbe() and make probe semantics consistent with createAPILoadBalancer by replacing NumberOfProbes in apiProbe() with the same property used there (ProbeThreshold) and ensuring the numeric value matches the intended threshold; update apiProbe()'s Properties to use ProbeThreshold instead of NumberOfProbes and keep the Name set to "HTTPSProbe", and verify createAPILoadBalancer's inline probe uses the same Protocol, Port, RequestPath and numeric threshold so both probe constructors (apiProbe() and createAPILoadBalancer) behave identically.
267-287: Remove stale comment on line 274.The comment
// We need a vnet & subnetappears to be incomplete or leftover from development. Either complete the implementation or remove the comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/infrastructure/azure/network.go` around lines 267 - 287, The comment "// We need a vnet & subnet" in addFrontendIPConfigurationToLoadBalancer is stale; remove it or replace it with a clear, accurate comment explaining why FrontendIPConfigurations is being appended (e.g., noting that the frontend config is added to the existing LoadBalancer before BeginCreateOrUpdate). Update the comment adjacent to lb.Properties.FrontendIPConfigurations (or delete it entirely) so the intent is not misleading.
167-184: Address TODO comment and simplify composite literal.Line 172 has a comment
// configurable ??suggesting this value may need to be parameterized. Additionally, line 175 has a redundant type in the composite literal that can be simplified.♻️ Minor cleanup for composite literal
FrontendIPConfigurations: []*armnetwork.SubResource{ - &armnetwork.SubResource{ + { ID: to.Ptr(fmt.Sprintf("/%s/%s/frontendIPConfigurations/%s", in.idPrefix, in.loadBalancerName, in.frontendIPConfigName)), }, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/infrastructure/azure/network.go` around lines 167 - 184, The AllocatedOutboundPorts value is marked TODO and the composite literal uses a redundant generic type; update lbRuleInput to include an AllocatedOutboundPorts int32 (or pointer) field and make outboundRule use that value (falling back to 1024 when zero/nil), and simplify the pointer call by replacing to.Ptr[int32](1024) with to.Ptr(1024) so the composite literal has no redundant explicit type; keep the rest of outboundRule (frontend/backend SubResource IDs and names) 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/infrastructure/azure/network.go`:
- Around line 207-247: Both addLoadBalancingRuleToLoadBalancer and
addOutboundRuleToLoadBalancer can nil‑dereference lbResp or lb.Properties; first
validate lbResp.LoadBalancer != nil and lb.Properties != nil after the Get call,
return a clear error if nil, and ensure the specific slice fields
(lb.Properties.LoadBalancingRules and lb.Properties.OutboundRules) are non‑nil
before appending (initialize them to empty slices if needed) so the subsequent
BeginCreateOrUpdate call receives a valid LoadBalancer.
- Around line 186-206: In addProbeToLoadBalancer, avoid a potential nil
dereference of lb.Properties before using lb.Properties.Probes: check if
lb.Properties is nil and if so initialize it to a new
armnetwork.LoadBalancerPropertiesFormat (and ensure Probes is an initialized
slice) before appending the incoming probe; then append probe to
lb.Properties.Probes and proceed with BeginCreateOrUpdate and polling as before.
---
Nitpick comments:
In `@pkg/infrastructure/azure/network.go`:
- Around line 93-104: The mcsProbe() function uses NumberOfProbes but should use
ProbeThreshold to match the probe used in updateInternalLoadBalancer; update
mcsProbe() by removing NumberOfProbes and adding ProbeThreshold: set it to
to.Ptr[int32](2) (same value as before) on the armnetwork.ProbePropertiesFormat
so the probe property names are consistent with updateInternalLoadBalancer.
- Around line 129-141: Remove the commented-out Name line in apiProbe() and make
probe semantics consistent with createAPILoadBalancer by replacing
NumberOfProbes in apiProbe() with the same property used there (ProbeThreshold)
and ensuring the numeric value matches the intended threshold; update
apiProbe()'s Properties to use ProbeThreshold instead of NumberOfProbes and keep
the Name set to "HTTPSProbe", and verify createAPILoadBalancer's inline probe
uses the same Protocol, Port, RequestPath and numeric threshold so both probe
constructors (apiProbe() and createAPILoadBalancer) behave identically.
- Around line 267-287: The comment "// We need a vnet & subnet" in
addFrontendIPConfigurationToLoadBalancer is stale; remove it or replace it with
a clear, accurate comment explaining why FrontendIPConfigurations is being
appended (e.g., noting that the frontend config is added to the existing
LoadBalancer before BeginCreateOrUpdate). Update the comment adjacent to
lb.Properties.FrontendIPConfigurations (or delete it entirely) so the intent is
not misleading.
- Around line 167-184: The AllocatedOutboundPorts value is marked TODO and the
composite literal uses a redundant generic type; update lbRuleInput to include
an AllocatedOutboundPorts int32 (or pointer) field and make outboundRule use
that value (falling back to 1024 when zero/nil), and simplify the pointer call
by replacing to.Ptr[int32](1024) with to.Ptr(1024) so the composite literal has
no redundant explicit type; keep the rest of outboundRule (frontend/backend
SubResource IDs and names) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 106038f6-e47c-4599-98d8-a4e77dc491a9
📒 Files selected for processing (2)
pkg/infrastructure/azure/azure.gopkg/infrastructure/azure/network.go
jhixson74
force-pushed
the
main-ipv6-networking-functions
branch
from
812cf0a to
511a976
Compare
|
/assign @rna-afk |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
jhixson74
force-pushed
the
main-ipv6-networking-functions
branch
from
511a976 to
af678af
Compare
|
@coderabbitai review |
|
[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 |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/infrastructure/azure/network.go (1)
93-164: Unify the API/MCS probe and rule definitions.These new builders already drift from the existing LB definitions in this file:
apiProbe()usesHTTPSProbewhile the current API LB path usesapi-probe, the helper probes useNumberOfProbeswhile the inline paths useProbeThreshold, andapiRule()setsDisableOutboundSnatonly here. If IPv6 is built through these helpers while IPv4 stays on the inline path, the two families won't be configured the same. Route both paths through one shared builder/constant set.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/infrastructure/azure/network.go` around lines 93 - 164, The API and MCS probe/rule builders (apiProbe, mcsProbe, apiRule, mcsRule) diverge in names and properties (probe Name: "HTTPSProbe" vs "api-probe", NumberOfProbes vs ProbeThreshold, and DisableOutboundSnat only set in apiRule), causing inconsistent IPv4/IPv6 configuration; refactor by introducing a single shared probe builder and a single shared load-balancing-rule builder (or shared constants) that both apiRule and mcsRule call, consolidate probe name and use the same health-check field (use ProbeThreshold or NumberOfProbes consistently across the builders), ensure DisableOutboundSnat and other flags are applied uniformly, and reuse lbRuleInput for any variant-specific values so both IPv4 and IPv6 paths route through the same builder logic.
🤖 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/network.go`:
- Around line 186-208: The helper addProbeToLoadBalancer currently does a blind
Get → append → BeginCreateOrUpdate which can create duplicates or clobber
concurrent changes; change it to be idempotent by reading lb once, checking
lb.Properties.Probes for an existing probe with the same Name (or matching
Port/Protocol), and upserting (replace existing entry) instead of always
appending; perform the mutation in-memory and then call
lbClient.BeginCreateOrUpdate exactly once; apply the same upsert-by-name pattern
and single-update flow to the sibling helpers that mutate lb.Properties (the
functions handling loadBalancers’ Probes, LoadBalancingRules,
BackendAddressPools, and NAT rules) and consider retrying the update on
transient/ETag conflicts returned by BeginCreateOrUpdate.
---
Nitpick comments:
In `@pkg/infrastructure/azure/network.go`:
- Around line 93-164: The API and MCS probe/rule builders (apiProbe, mcsProbe,
apiRule, mcsRule) diverge in names and properties (probe Name: "HTTPSProbe" vs
"api-probe", NumberOfProbes vs ProbeThreshold, and DisableOutboundSnat only set
in apiRule), causing inconsistent IPv4/IPv6 configuration; refactor by
introducing a single shared probe builder and a single shared
load-balancing-rule builder (or shared constants) that both apiRule and mcsRule
call, consolidate probe name and use the same health-check field (use
ProbeThreshold or NumberOfProbes consistently across the builders), ensure
DisableOutboundSnat and other flags are applied uniformly, and reuse lbRuleInput
for any variant-specific values so both IPv4 and IPv6 paths route through the
same builder logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dd71a357-082a-4f58-9b62-69cf0cd40a78
📒 Files selected for processing (2)
pkg/infrastructure/azure/azure.gopkg/infrastructure/azure/network.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/infrastructure/azure/azure.go
| func addProbeToLoadBalancer(ctx context.Context, probe *armnetwork.Probe, in *lbInput) (*armnetwork.LoadBalancer, error) { | ||
| lbResp, err := in.lbClient.Get(ctx, in.resourceGroupName, in.loadBalancerName, nil) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get load balancer: %w", err) | ||
| } | ||
| lb := lbResp.LoadBalancer | ||
| if lb.Properties == nil { | ||
| return nil, fmt.Errorf("no load balancer properties found") | ||
| } | ||
|
|
||
| lb.Properties.Probes = append(lb.Properties.Probes, probe) | ||
|
|
||
| pollerResp, err := in.lbClient.BeginCreateOrUpdate(ctx, in.resourceGroupName, in.loadBalancerName, lb, nil) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("cannot update load balancer: %w", err) | ||
| } | ||
|
|
||
| resp, err := pollerResp.PollUntilDone(ctx, nil) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return &resp.LoadBalancer, nil | ||
| } |
There was a problem hiding this comment.
Make the LB mutation helpers idempotent.
These helpers each do Get → blind append → BeginCreateOrUpdate on the full load balancer. A retry after an ARM write succeeds, or two helpers touching the same LB close together, can duplicate child resources or overwrite the other change with a stale snapshot. A get once, mutate many, update once flow—or at minimum upsert-by-name checks—would avoid that.
Also applies to: 210-232, 234-256, 276-296, 298-317
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/infrastructure/azure/network.go` around lines 186 - 208, The helper
addProbeToLoadBalancer currently does a blind Get → append → BeginCreateOrUpdate
which can create duplicates or clobber concurrent changes; change it to be
idempotent by reading lb once, checking lb.Properties.Probes for an existing
probe with the same Name (or matching Port/Protocol), and upserting (replace
existing entry) instead of always appending; perform the mutation in-memory and
then call lbClient.BeginCreateOrUpdate exactly once; apply the same
upsert-by-name pattern and single-update flow to the sibling helpers that mutate
lb.Properties (the functions handling loadBalancers’ Probes, LoadBalancingRules,
BackendAddressPools, and NAT rules) and consider retrying the update on
transient/ETag conflicts returned by BeginCreateOrUpdate.
af678af to
7aff14f
Compare
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/network.go`:
- Around line 289-332: Both addFrontendIPConfigurationToLoadBalancer and
addBackendAddressPoolToLoadBalancer assume lb.Properties is non-nil; add a
defensive nil check after retrieving lb (lb := lbResp.LoadBalancer) and if
lb.Properties == nil set it to a new armnetwork.LoadBalancerPropertiesFormat{}
before appending to lb.Properties.FrontendIPConfigurations or
lb.Properties.BackendAddressPools, then continue with the existing
BeginCreateOrUpdate / PollUntilDone flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 52fc23a2-5fab-416d-9321-6ecedaf04d2a
📒 Files selected for processing (2)
pkg/infrastructure/azure/azure.gopkg/infrastructure/azure/network.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/infrastructure/azure/azure.go
jhixson74
force-pushed
the
main-ipv6-networking-functions
branch
from
7aff14f to
9b64124
Compare
|
@coderabbitai summary |
✅ Actions performedSummary regeneration triggered. |
|
@coderabbitai summary |
✅ Actions performedSummary regeneration triggered. |
|
@coderabbitai generate sequence diagram |
Sequence Diagram for PRsequenceDiagram
participant Az as Azure.go<br/>(Entrypoint)
participant Net as network.go<br/>(Helpers)
participant Probe as Probe/Rule<br/>Builders
participant Fetch as Fetch<br/>Helpers
participant ARM as ARM Client
participant Azure as Azure Cloud<br/>Service
Az->>Net: Create Load Balancer
activate Net
Net->>Net: Set resourceGroupName & ipVersion
Net->>ARM: Create Public IP (IPv4)
ARM->>Azure: Deploy Public IP
Azure-->>ARM: IP Resource
ARM-->>Net: Public IP Created
Note over Net: Build LB Rules & Probes
Net->>Probe: mcsProbe()
Probe-->>Net: MCS Probe Config
Net->>Probe: mcsRule()
Probe-->>Net: MCS Rule Config
Net->>Probe: apiProbe()
Probe-->>Net: API Probe Config
Net->>Probe: apiRule()
Probe-->>Net: API Rule Config
Net->>Probe: outboundRule()
Probe-->>Net: Outbound Rule Config
Note over Net: Fetch Virtual Network<br/>& Subnet
Net->>Fetch: getVirtualNetwork()
Fetch->>ARM: Query VNet
ARM->>Azure: GET VNet
Azure-->>ARM: VNet Resource
ARM-->>Fetch: VNet Data
Fetch-->>Net: VNet Retrieved
Net->>Fetch: getSubnet()
Fetch->>ARM: Query Subnet
ARM->>Azure: GET Subnet
Azure-->>ARM: Subnet Resource
ARM-->>Fetch: Subnet Data
Fetch-->>Net: Subnet Retrieved
Note over Net: Update LB Incrementally
Net->>ARM: Update LB with Rules
ARM->>Azure: PATCH Load Balancer
Azure-->>ARM: LB Updated
ARM-->>Net: LB Rules Applied
Net->>ARM: Create NICs
ARM->>Azure: Deploy NICs
Azure-->>ARM: NIC Resources
ARM-->>Net: NICs Created
Net->>ARM: Associate NICs to LB Pools
ARM->>Azure: Update Backend Pools
Azure-->>ARM: Associations Complete
ARM-->>Net: Association Success
deactivate Net
Net-->>Az: Load Balancer Ready
|
✅ Actions performedSequence diagram generated. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/infrastructure/azure/network.go (1)
464-526:⚠️ Potential issue | 🔴 CriticalMake the load balancer update paths idempotent.
Lines 464-526 and Lines 587-593 still do full-resource read/append/write mutations. A retry after ARM accepted the write, or a second caller touching the same load balancer, can duplicate child resources or clobber a sibling change with a stale snapshot. Upsert by child
Nameand do one read/update per load balancer change.Also applies to: 587-593
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/infrastructure/azure/network.go` around lines 464 - 526, The current code constructs a new armnetwork.LoadBalancer from extLB.Properties by append-style mutations (see extLB.Properties.FrontendIPConfigurations, BackendAddressPools, OutboundRules) and then calls in.lbClient.BeginCreateOrUpdate which can duplicate child resources or clobber concurrent updates; change to an idempotent upsert: read the existing LB once (Get), merge or replace child collections by matching child Name (e.g., frontendIPConfigName, backendAddressPoolName, probeName, the "api-v4" LoadBalancingRule, OutboundRules) and update only those entries (create if missing, update if exists) instead of blindly appending, then call BeginCreateOrUpdate with the merged LoadBalancer; ensure merging logic lives next to the BeginCreateOrUpdate call (same function) and uses Name-based equality to avoid duplicates and lost updates.
🧹 Nitpick comments (1)
pkg/infrastructure/azure/network.go (1)
96-172: Avoid landing a second, drifting source of truth for the LB probes/rules.
mcsProbe(),apiProbe(),mcsRule(), andapiRule()duplicate the live ARM payloads later in this file, but they already disagree on probe names and health-check fields. Since they're also still//nolint:unused, future callers can silently diverge from the current LB behavior. Either route the existing create/update paths through these helpers now or drop them until they're the real source of truth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/infrastructure/azure/network.go` around lines 96 - 172, The file contains duplicate helper constructors mcsProbe, apiProbe, mcsRule, and apiRule that disagree with the real ARM payloads later and are marked //nolint:unused; either wire the create/update code to use these helpers or delete them to avoid a second drifting source of truth. Fix by choosing one approach: (a) replace the inline probe/load-balancer payloads in the create/update functions to call mcsProbe() and apiProbe() and mcsRule(in)/apiRule(in) (remove //nolint:unused and ensure the probe Name, RequestPath, Port and other properties in the helpers match the live ARM values), or (b) remove these helper functions entirely so there’s only the single authoritative payload; ensure any references to probe names/IDs use the canonical values (compare Name fields like "sint-probe" vs "HTTPSProbe" and RequestPath "/healthz" vs "/readyz") when making the change.
🤖 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/network.go`:
- Around line 621-624: The loop that sets LoadBalancerBackendAddressPools on
each IP configuration (in the nic.Properties.IPConfigurations loop) currently
appends in.backendAddressPools unconditionally; change it to deduplicate by
comparing pool.ID and only append pools whose ID is not already present in
ipconfig.Properties.LoadBalancerBackendAddressPools (handle nil slices safely).
Update the code where you mutate
ipconfig.Properties.LoadBalancerBackendAddressPools so it merges
in.backendAddressPools by ID (skip duplicates) before calling
nicClient.BeginCreateOrUpdate, ensuring no duplicate pool IDs are written on
retries.
---
Duplicate comments:
In `@pkg/infrastructure/azure/network.go`:
- Around line 464-526: The current code constructs a new armnetwork.LoadBalancer
from extLB.Properties by append-style mutations (see
extLB.Properties.FrontendIPConfigurations, BackendAddressPools, OutboundRules)
and then calls in.lbClient.BeginCreateOrUpdate which can duplicate child
resources or clobber concurrent updates; change to an idempotent upsert: read
the existing LB once (Get), merge or replace child collections by matching child
Name (e.g., frontendIPConfigName, backendAddressPoolName, probeName, the
"api-v4" LoadBalancingRule, OutboundRules) and update only those entries (create
if missing, update if exists) instead of blindly appending, then call
BeginCreateOrUpdate with the merged LoadBalancer; ensure merging logic lives
next to the BeginCreateOrUpdate call (same function) and uses Name-based
equality to avoid duplicates and lost updates.
---
Nitpick comments:
In `@pkg/infrastructure/azure/network.go`:
- Around line 96-172: The file contains duplicate helper constructors mcsProbe,
apiProbe, mcsRule, and apiRule that disagree with the real ARM payloads later
and are marked //nolint:unused; either wire the create/update code to use these
helpers or delete them to avoid a second drifting source of truth. Fix by
choosing one approach: (a) replace the inline probe/load-balancer payloads in
the create/update functions to call mcsProbe() and apiProbe() and
mcsRule(in)/apiRule(in) (remove //nolint:unused and ensure the probe Name,
RequestPath, Port and other properties in the helpers match the live ARM
values), or (b) remove these helper functions entirely so there’s only the
single authoritative payload; ensure any references to probe names/IDs use the
canonical values (compare Name fields like "sint-probe" vs "HTTPSProbe" and
RequestPath "/healthz" vs "/readyz") when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 23468ccf-8a4f-407a-8423-b4d39b026bfc
📒 Files selected for processing (2)
pkg/infrastructure/azure/azure.gopkg/infrastructure/azure/network.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/infrastructure/azure/azure.go
| for _, ipconfig := range nic.Properties.IPConfigurations { | ||
| ipconfig.Properties.LoadBalancerBackendAddressPools = append(ipconfig.Properties.LoadBalancerBackendAddressPools, in.backendAddressPools...) | ||
| } | ||
| pollerResp, err := in.nicClient.BeginCreateOrUpdate(ctx, in.resourceGroup, nicName, nic.Interface, nil) | ||
| pollerResp, err := in.nicClient.BeginCreateOrUpdate(ctx, in.resourceGroupName, nicName, nic.Interface, nil) |
There was a problem hiding this comment.
Deduplicate backend-pool attachments before updating the NIC.
Line 622 blindly appends in.backendAddressPools on every pass. If this path is retried after a successful ARM write, the next update sends the same pool IDs again and can fail. Compare by pool.ID and only append missing backend pools.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/infrastructure/azure/network.go` around lines 621 - 624, The loop that
sets LoadBalancerBackendAddressPools on each IP configuration (in the
nic.Properties.IPConfigurations loop) currently appends in.backendAddressPools
unconditionally; change it to deduplicate by comparing pool.ID and only append
pools whose ID is not already present in
ipconfig.Properties.LoadBalancerBackendAddressPools (handle nil slices safely).
Update the code where you mutate
ipconfig.Properties.LoadBalancerBackendAddressPools so it merges
in.backendAddressPools by ID (skip duplicates) before calling
nicClient.BeginCreateOrUpdate, ensuring no duplicate pool IDs are written on
retries.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
pkg/infrastructure/azure/network.go (2)
195-217: 🛠️ Refactor suggestion | 🟠 MajorMake the new LB mutation helpers idempotent before they become the IPv6 write path.
addProbeToLoadBalancer,addLoadBalancingRuleToLoadBalancer,addOutboundRuleToLoadBalancer,addFrontendIPConfigurationToLoadBalancer, andaddBackendAddressPoolToLoadBalancerall doGet→ blindappend→BeginCreateOrUpdateon the full load balancer. A retry after ARM accepts the write, or two helpers mutating the same LB close together, will duplicate children or overwrite a sibling change with a stale snapshot. Upsert by childName(or batch the mutations and send one update) instead of unconditional append.Also applies to: 220-242, 245-267, 290-313, 316-338
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/infrastructure/azure/network.go` around lines 195 - 217, The helper functions (addProbeToLoadBalancer, addLoadBalancingRuleToLoadBalancer, addOutboundRuleToLoadBalancer, addFrontendIPConfigurationToLoadBalancer, addBackendAddressPoolToLoadBalancer) currently do Get → blind append → BeginCreateOrUpdate which can create duplicate children or overwrite concurrent changes; change each helper to upsert by the child resource Name: after retrieving lb (and ensuring Properties and the specific slice are non-nil), search the relevant slice (Probes, LoadBalancingRules, OutboundRules, FrontendIPConfigurations, BackendAddressPools) for an entry with the same *Name* and replace it if found or append only if not found, then call BeginCreateOrUpdate with the updated lb; alternatively consider batching multiple child mutations into a single update to avoid race windows.
621-624:⚠️ Potential issue | 🔴 CriticalDeduplicate backend-pool attachments before updating the NIC.
This still appends
in.backendAddressPoolson every pass.PostProvisioninpkg/infrastructure/azure/azure.gocalls this on the live control-plane NIC update path, so a retry after a successful ARM write can send the same backend-pool IDs again and fail the next NIC update.Proposed fix
for _, ipconfig := range nic.Properties.IPConfigurations { - ipconfig.Properties.LoadBalancerBackendAddressPools = append(ipconfig.Properties.LoadBalancerBackendAddressPools, in.backendAddressPools...) + existingPoolIDs := map[string]struct{}{} + for _, pool := range ipconfig.Properties.LoadBalancerBackendAddressPools { + if pool != nil && pool.ID != nil { + existingPoolIDs[*pool.ID] = struct{}{} + } + } + for _, pool := range in.backendAddressPools { + if pool == nil || pool.ID == nil { + continue + } + if _, found := existingPoolIDs[*pool.ID]; found { + continue + } + ipconfig.Properties.LoadBalancerBackendAddressPools = append(ipconfig.Properties.LoadBalancerBackendAddressPools, pool) + existingPoolIDs[*pool.ID] = struct{}{} + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/infrastructure/azure/network.go` around lines 621 - 624, The NIC update loop appends in.backendAddressPools to each nic.Properties.IPConfigurations[].Properties.LoadBalancerBackendAddressPools unconditionally, causing duplicate backend-pool IDs on retries; before calling in.nicClient.BeginCreateOrUpdate (or within the loop that touches nic.Properties.IPConfigurations), deduplicate by checking existing LoadBalancerBackendAddressPools entries and only append those IDs from in.backendAddressPools that are not already present (handle nil slices and compare by the ID field), so BeginCreateOrUpdate is only sent with unique backend-pool references.
🧹 Nitpick comments (1)
pkg/infrastructure/azure/network.go (1)
97-191: Don't keep a second, drifting source of truth for LB objects.These builders are all
//nolint:unused, but they already disagree with the active provisioning path below:apiProbe()hard-codesHTTPSProbe/NumberOfProbes, while the live code buildsapi-probe/ProbeThreshold, andapiRule()setsDisableOutboundSnatwhere the current v4 rule builder does not. Either route the live path through these helpers now, or defer them until there is a caller/test so the semantics cannot drift.As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/infrastructure/azure/network.go` around lines 97 - 191, The helper builders mcsProbe, mcsRule, apiProbe, apiRule, and outboundRule are unused and have diverged from the active provisioning code (mismatched probe names/fields like "HTTPSProbe"/NumberOfProbes vs "api-probe"/ProbeThreshold and differing DisableOutboundSnat semantics), so either wire the live provisioning path to use these helpers or remove/defer them to avoid a second source of truth; to fix, choose one: (A) update the live v4 rule/health probe construction to call apiProbe/mcsProbe/apiRule/mcsRule/outboundRule (adjust lbRuleInput and any field mappings so ProbeThreshold vs NumberOfProbes and probe Name match and preserve DisableOutboundSnat semantics), or (B) delete or comment out these nolint:unused builders (or add a clear TODO) so they cannot drift, ensuring only the canonical builder remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/infrastructure/azure/network.go`:
- Around line 195-217: The helper functions (addProbeToLoadBalancer,
addLoadBalancingRuleToLoadBalancer, addOutboundRuleToLoadBalancer,
addFrontendIPConfigurationToLoadBalancer, addBackendAddressPoolToLoadBalancer)
currently do Get → blind append → BeginCreateOrUpdate which can create duplicate
children or overwrite concurrent changes; change each helper to upsert by the
child resource Name: after retrieving lb (and ensuring Properties and the
specific slice are non-nil), search the relevant slice (Probes,
LoadBalancingRules, OutboundRules, FrontendIPConfigurations,
BackendAddressPools) for an entry with the same *Name* and replace it if found
or append only if not found, then call BeginCreateOrUpdate with the updated lb;
alternatively consider batching multiple child mutations into a single update to
avoid race windows.
- Around line 621-624: The NIC update loop appends in.backendAddressPools to
each
nic.Properties.IPConfigurations[].Properties.LoadBalancerBackendAddressPools
unconditionally, causing duplicate backend-pool IDs on retries; before calling
in.nicClient.BeginCreateOrUpdate (or within the loop that touches
nic.Properties.IPConfigurations), deduplicate by checking existing
LoadBalancerBackendAddressPools entries and only append those IDs from
in.backendAddressPools that are not already present (handle nil slices and
compare by the ID field), so BeginCreateOrUpdate is only sent with unique
backend-pool references.
---
Nitpick comments:
In `@pkg/infrastructure/azure/network.go`:
- Around line 97-191: The helper builders mcsProbe, mcsRule, apiProbe, apiRule,
and outboundRule are unused and have diverged from the active provisioning code
(mismatched probe names/fields like "HTTPSProbe"/NumberOfProbes vs
"api-probe"/ProbeThreshold and differing DisableOutboundSnat semantics), so
either wire the live provisioning path to use these helpers or remove/defer them
to avoid a second source of truth; to fix, choose one: (A) update the live v4
rule/health probe construction to call
apiProbe/mcsProbe/apiRule/mcsRule/outboundRule (adjust lbRuleInput and any field
mappings so ProbeThreshold vs NumberOfProbes and probe Name match and preserve
DisableOutboundSnat semantics), or (B) delete or comment out these nolint:unused
builders (or add a clear TODO) so they cannot drift, ensuring only the canonical
builder remains.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 67a68d13-70f2-422d-8150-1ae337081949
📒 Files selected for processing (2)
pkg/infrastructure/azure/azure.gopkg/infrastructure/azure/network.go
|
@jhixson74: 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
Azure networking functions to simplify adding IPv6 rules, load balancers, nics, etc.