fix: fall back to an IP-derived name for backend addresses with no cached node name - #10814
fix: fall back to an IP-derived name for backend addresses with no cached node name#10814pujitha24 wants to merge 1 commit into
Conversation
…ched node name Motivation: addNodeIPAddressesToBackendPool looks up the node name for a private IP in az.nodePrivateIPToNodeNameMap and uses it as the Name of the armnetwork.LoadBalancerBackendAddress sent to Azure. When that lookup misses, Name is left as a pointer to an empty string. This is reachable in the local-service backend-pool-updater path (pkg/provider/azure_local_services.go, exercised by TestLoadBalancerBackendPoolUpdater and friends): its tests seed nodePrivateIPs directly without going through updateNodeCaches, so nodePrivateIPToNodeNameMap stays empty there and every backend address built on that path previously got Name: "". In production both maps are populated together, in the same loop, by updateNodeCaches (pkg/provider/azure.go), so a persistent empty name there would only happen if a private-IP cache entry existed ahead of its name-map entry (e.g. a timing gap during cache population) -- consistent with the "for unknown reason" note in the linked report. This change does not claim to have reproduced that race live; it protects the empty-name case itself, which is directly reproduced by the added unit test. Approach: When the cached node name is empty, fall back to a name derived from the IP address (dots/colons replaced with dashes, prefixed with "ip-" to avoid colliding with a real node name that happens to look IP-derived), so the backend address is never created with an empty name. Also updated the azure_local_services_test.go helper that synthesizes backend pools, which previously hardcoded an empty name, to generate the same fallback name. Validation: - go build ./... - go test ./pkg/provider/... (full package, all green, no regressions) - Added TestAddNodeIPAddressesToBackendPoolGeneratesNameWhenNodeNameMissing; confirmed it fails with "expected ip-10-0-0-1, actual \"\"" when the fix is reverted, and passes with the fix applied. - golangci-lint run ./pkg/provider/... shows no new findings on the changed lines (pre-existing goconst/gosec/govet findings elsewhere in the package are unrelated to this diff). Report: kubernetes-sigs#9803 Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: pujitha24 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 |
|
Hi @pujitha24. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
/assign @bridgetkromhout Still waiting on ok-to-test and a first look whenever you have a chance. |
What type of PR is this?
/kind bug
What this PR does / why we need it:
When
addNodeIPAddressesToBackendPoolbuilds a newarmnetwork.LoadBalancerBackendAddressfor a node IP, it looks up the node name from
az.nodePrivateIPToNodeNameMapand uses itas the address
Name. If that lookup misses,Nameis set to a pointer to an emptystring.
This is reachable today: the local-service backend-pool-updater test suite
(
pkg/provider/azure_local_services_test.go, exercisingpkg/provider/azure_local_services.go) seedsnodePrivateIPsdirectly without goingthrough
updateNodeCaches, sonodePrivateIPToNodeNameMapstays empty and every backendaddress built on that path previously got
Name: "". In production, both maps arepopulated together by the same node-informer callback (
pkg/provider/azure.go,updateNodeCaches), so a persistently empty name there would only happen for a node whoseprivate-IP cache entry exists ahead of/without its name-map entry (e.g. a timing/race gap
during cache population), matching the "for unknown reason" note in the linked issue. This
PR does not claim to have reproduced that race live; the fix protects the code path (used by
both production and this test suite) that hits the empty-name case, and is validated by a
unit test that reproduces "cache miss -> empty name" directly.
An empty-named backend address risks being rejected, or colliding with other empty-named
addresses, when the pool is sent to the Azure API. This PR falls back to a name derived
from the IP address (e.g.
10.0.0.1->ip-10-0-0-1) whenever the cached node name isempty, so the address always gets a non-empty, distinguishable name. The
ip-prefixavoids colliding with a real node whose name happens to already look IP-derived.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
be missing in production (that root cause is not confirmed — see discussion above).
pkg/provider/azure_local_services_test.go'sgetTestBackendAddressPoolWithIPshelper, which previously hardcodedName: ptr.To("")for every synthesized backend address, to generate the samefallback name production code now generates, since that test path never populates
nodePrivateIPToNodeNameMap.go build ./...passes.go test ./pkg/provider/...passes (fullpackage, no regressions). Added
TestAddNodeIPAddressesToBackendPoolGeneratesNameWhenNodeNameMissing, confirmed itfails with
expected "ip-10-0-0-1", actual ""when the fix is reverted and passeswith the fix applied.
golangci-lint run ./pkg/provider/...shows no new findings onthe changed lines (pre-existing goconst/gosec/govet findings elsewhere in the package
are unrelated to this diff).
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
Fixes #9803