net/netlink: a failed link lookup is not an absent interface - #668
Open
Philip Lombardi (plombardi89) wants to merge 4 commits into
Open
net/netlink: a failed link lookup is not an absent interface#668Philip Lombardi (plombardi89) wants to merge 4 commits into
Philip Lombardi (plombardi89) wants to merge 4 commits into
Conversation
link_manager.go decides, in twelve places, whether a netlink error means "the interface is not there" or something else. Nothing can test those decisions while the lookup is a direct call to netlink.LinkByName, because a unit test has no way to make a real netlink syscall fail on demand and no privileges to make one succeed. Add netlinkLinkByName, netlinkLinkAdd and netlinkLinkDel and route all 36 call sites in this file through them. This follows the convention already used for netlinkRuleList, netlinkRuleAdd and netlinkRuleDel in route_table_setup.go, which gateway_policy_manager_mock_test.go swaps and restores under defer. This commit changes no behaviour and is mechanical: reversing the rename and removing the var block reproduces the previous file byte for byte. The error classification it exists to enable is the next commit, kept separate so it can be read on its own. LinkAdd and LinkDel are included because the interesting assertion is usually negative: given a transient lookup error, the manager must not go on to create or delete an interface. LinkSetUp, LinkSetMTU, LinkSetARPOff, LinkSetHardwareAddr and LinkList are left as direct calls, as no test reaches them.
netlink.LinkByName can fail without the interface being missing: a busy
socket, a permissions problem, a truncated response. link_manager.go read
every one of those as "not there" in twelve places, and drew one of two
wrong conclusions from it.
DeleteLink drew the worse one. Any lookup error returned nil, so a caller
that asked for an interface to be removed was told it had been, when
nothing had been deleted and the interface was still carrying traffic.
The ten Ensure* paths drew the other. On a lookup failure they fell
through to creation, so a transient error turned into an attempt to
create an interface that already exists. That one is self-correcting,
because the create then fails with EEXIST, but it fails noisily on a
healthy system and it covers every tunnel type: IPIP, GENEVE, VXLAN,
WireGuard, the CNI bridge and the dummy interface.
Both now go through lookupError, which defers to isLinkGoneError. That
helper already existed in this file and is already used by the bridge MTU
paths for exactly this distinction; it recognises LinkNotFoundError,
ENODEV and ENOENT.
lookupError treats a nil error as nothing to report, which is load
bearing rather than defensive. isLinkGoneError(nil) is false:
errors.As returns false for a nil error and errors.Is(nil, ENODEV)
reduces to nil == ENODEV. Without that first condition lookupError would
wrap nil and return a non-nil error rendering as "%!w(<nil>)".
EnsureIPIPExternalInterface and EnsureGeneveInterface reach it with
err == nil, having just deleted an interface they are about to recreate,
so they would delete the interface and then abort instead of recreating
it. On the GENEVE path that aborts the whole eBPF tunnel reconcile
(tunnel_config.go:253) with geneve0 already gone. Those two are also
written as if err != nil { ... } else { ... } so the control flow does
not depend on lookupError being total.
Exists keeps its bool signature. Its six callers, in tunnel_config.go,
wireguard_config.go and mtu.go, are all reconcile predicates that retry,
so a transient false is harmless there and returning an error would only
give six call sites something to log. It logs the condition itself at V(2)
instead, so a false caused by a broken lookup can be told apart from a
false caused by an absent interface.
Lookup failures increment InterfaceOperationErrors with a new "lookup"
label, alongside the existing "create" and "set_mtu", so the class of
failure this commit makes visible is visible to Prometheus too.
The tests fail without the change. Reverting the guards and rerunning
gives ten failing Ensure* subtests and a failing DeleteLink; reverting
only the nil case gives the three recreate subtests, failing with the
"%!w(<nil>)" message itself. The no-op cases are covered in both
directions, so neither fix can pass by refusing to act at all.
Not addressed here: EnsureGeneveInterfaceWithCache and
EnsureIPIPInterfaceWithCache decide via cache.HasLink, a plain map
lookup, and go straight to creation on a miss with no fallback lookup, so
those two keep the same shape sourced from a stale cache rather than a
failed syscall. The risk is low, because NetlinkCache.Refresh only swaps
its maps after both LinkList and RouteList succeed, but it is not fixed
here.
Philip Lombardi (plombardi89)
force-pushed
the
fix/net-netlink-classify-link-errors
branch
from
August 26, 2026 16:28
ea1f5fd to
0a1658f
Compare
pruneStaleInterfacesLocked removes the policy routing rules for any
interface it decides has gone away, and it decided that from
`if _, err := netlink.LinkByName(iface); err == nil { continue }`. A
lookup that failed for any other reason therefore removed the mangle
rules and the routing table entry for an interface that was still up,
black-holing traffic on it until something else put the rules back.
This is the same mistake as the previous commit, in the same package, and
worse in kind: the Ensure* paths fail noisily on a healthy system, this
one silently tears down working configuration.
Gate the prune on isLinkGoneError, and log at V(2) when a lookup failure
leaves the rules in place.
On the cost of not pruning, which is smaller than it first looks.
ReconcileExpectedInterfaces already clears both managed mangle chains and
rebuilds them from the expected set, and then replaces configuredIfaces
wholesale, so orphaned rules and stale map entries are removed on that
path whatever the link lookups do; the delete() skipped here is
redundant with it. cleanupStaleIPRulesLocked likewise finds stale ip
rules from netlinkRuleList and flushes their route tables and rt_tables
entries without consulting a link at all. So this is not the only
cleanup, and declining to prune does not strand rules indefinitely.
Exposure is smaller still. EnablePolicyRouting is false in the flag
default, in the runtime config default, in the shipped ConfigMap
template and in the rendered output, and the field is documented as
deprecated and replaced by the UNBOUNDED-FORWARD chain. Both this
prune's caller and ReconcileExpectedInterfaces are gated on it, so in
any default deployment none of this code runs.
The change is still worth making: the behaviour was wrong, and the
feature is retained for backward compatibility rather than removed.
This commit is deliberately separable from the rest of the branch and can
be dropped on its own.
No direct test. GatewayPolicyManager holds ipt4 and ipt6 as concrete
*iptables.IPTables, so the loop cannot be reached without real iptables
handles, and adding an interface seam for them is a larger change than
this fix. The classification it relies on, isLinkGoneError, is covered by
link_manager_test.go.
The previous commits added an InterfaceOperationErrors{operation="lookup"}
counter, but only on the paths that return an error. The two paths that
swallow the condition were left uncounted, which is backwards: those are
the ones with nothing else to show for it.
Exists() returns a plain bool by design, because its six callers are all
reconcile predicates that retry. That makes a node whose netlink lookups
are failing indistinguishable, from the outside, from one whose
interfaces are legitimately absent. It now counts the failure as well as
logging it at V(2).
The gateway policy prune does the same. It declines to remove rules when
a lookup fails for a reason other than absence, which is the right call,
but the declined cleanup was visible only at V(2).
Both count only lookups that are not absence. An interface that is gone
is not an error and must not inflate the counter; a test pins that in
both directions, and fails if the increment in Exists() is removed.
DeleteLink loses a redundant isLinkGoneError check. lookupError already
returns nil for an absent interface, so the explicit check before calling
it tested the same condition twice.
Philip Lombardi (plombardi89)
force-pushed
the
fix/net-netlink-classify-link-errors
branch
from
August 26, 2026 17:41
0a1658f to
78422bf
Compare
Patrick W. Healy (phealy)
approved these changes
Aug 27, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Aug 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First of three PRs splitting out the net fixes found while working through #657. This one is a bug fix and changes no lint configuration;
nilerrandpredeclaredget enabled in the last of the three.The bug
netlink.LinkByNamecan fail without the interface being missing: a busy socket, a permissions problem, a truncated response.link_manager.goread every such error as "not there" in twelve places — tenEnsure*paths,DeleteLinkandExists()— and drew one of two wrong conclusions.DeleteLinkdrew the worse one. Any lookup error returnednil, so a caller that asked for an interface to be removed was told it had been, when nothing was deleted.The ten
Ensure*paths drew the other. On a lookup failure they fell through to creation, turning a transient error into an attempt to create an interface that already exists. Self-correcting viaEEXIST, but noisy on a healthy system. Five of those ten turn out to have no production caller at all (see below); the live ones span IPIP, GENEVE, VXLAN, WireGuard and the dummy interface.The fix defers to
isLinkGoneError, which already existed in this file (link_manager.go:844) and was already used for exactly this distinction byEnsureBridgePortMTUsandEnsureBridgePodMTUs.A reviewer independently confirmed the helper is complete for
netlink@v1.3.1—ENODEV(link_linux.go:2030), zero-message (:2039) and the EINVAL dump fallback (:1904) all yieldLinkNotFoundError. So there is no gap that would makeEnsure*permanently refuse to create.Four commits
route link calls through seams, no behaviour change— 3 seam vars following thenetlinkRuleListconvention inroute_table_setup.go:20-22; 36 call sites rerouted (23LinkByName, 10LinkAdd, 3LinkDel). Provably mechanical: reversing the rename and removing the var block reproduces the previous file byte for byte.a failed link lookup is not an absent interface— the fix, pluslink_manager_test.go.do not prune gateway rules on a failed link lookup— separable; drop this commit alone if you disagree.count the lookup failures that are only logged— metric coverage for the two paths that swallow the condition, and removal of a redundantisLinkGoneErrorcheck inDeleteLink.lookupErrortreats nil as nothing to report, and that is load bearingisLinkGoneError(nil)is false —errors.Asreturns false for a nil error (errors/wrap.go:103), anderrors.Is(nil, ENODEV)reduces tonil == ENODEV(:46-48). Without the nil case it would wrap nil and return an error rendering as%!w(<nil>).EnsureIPIPExternalInterfaceandEnsureGeneveInterfacereach it witherr == nilafter deleting an interface they are about to recreate. They would delete and then abort. Both are additionally written asif err != nil { … } else { … }so control flow does not depend on the helper being total.Scope of the
DeleteLinkfix: latent, not manifestingWorth being straight about. All 8
DeleteLinkproduction callers only log the error —V(2),WarningforErrorf— and none propagate or retry. Five sit behindif lm.Exists(), which this PR deliberately keeps returningfalseon a broken lookup, soDeleteLinkis not even reached from them. The onecontinueon the error path (site_watch_reconcile.go:631-634) skips only a success log, and aLinkDelsweep follows at:640.The fix is correct and worth keeping as prophylaxis. It does not fix an outage anyone is currently having.
After commit 4 it is simply
return lm.lookupError(err); the earlier explicitisLinkGoneErrorcheck tested the same condition the helper already tests.Exists()keeps itsboolAll six callers —
tunnel_config.go:307,363,432,886,wireguard_config.go:96,mtu.go:39— are reconcile predicates that retry, so a transientfalseis harmless. Commit 4 makes it count the condition as well as log it, so a node whose lookups are failing is distinguishable from one whose interfaces are legitimately absent, which a plainboolotherwise hides.Commit 3, on honest grounds
The gateway prune tore down policy routing for interfaces that were still up whenever a lookup failed for a non-absence reason. That was wrong and is worth fixing.
But my original justification was false, and is corrected in the commit message.
ReconcileExpectedInterfacesalready clears both managed mangle chains and rebuilds fromexpected(gateway_policy_manager.go:1016-1047), then replacesconfiguredIfaceswholesale (:584-589);cleanupStaleIPRulesLockedflushes stale route tables offnetlinkRuleListwith no link lookup. So the prune is not the only cleanup, and declining to prune does not strand rules indefinitely.Exposure is close to zero anyway:
EnablePolicyRoutingisfalsein the flag default (main.go:278), the runtime config default (:200), the ConfigMap template (01-configmap.yaml.tmpl:44) and the rendered output, and is documented as deprecated in favour of theUNBOUNDED-FORWARDchain (runtime_config.go:68-70). Both this prune's caller andReconcileExpectedInterfacesare gated on it.The honest cost of the trade — #671
Deferring a reconcile pass is not benign on the tunnel path.
pendingBPFEntriesis nil'd beforeconfigureTunnelPeers(site_watch_reconcile.go:2047); an abort leaves it nil; the caller only warns (:2066);reconcilePendingBPFEntriesstill runs (:2147);entries == nilbecomes an empty map (tunnel_config.go:760-762); andTunnelMap.Reconciledeletes everything not in desired (tunnel_map.go:288-296). So an aborted pass withdraws every tunnel peer from the LPM trie.This is pre-existing and unchanged in the common case, because the old code fell through to
LinkAdd, hitEEXISTand aborted at the same line. The one genuinely new way to reach it is a transient lookup failure where the interface really is absent: the old code created it, the new code defers. Right trade, real cost. Filed as #671.Known: five tested methods are unreachable — #670
EnsureIPIPInterfaceWithRemote,EnsureGeneveInterfaceWithRemote,EnsureBridge,EnsureGeneveInterfaceWithCacheandEnsureIPIPInterfaceWithCachehave no production caller; the tests added here are their only callers. Two further exportedLinkManagermethods,SetLinkNoARPandGetAddresses, have no references anywhere — 7 of 22 exported methods on the type. None of the seven is required to satisfy an interface, so none is load-bearing.unusedis enabled and cannot see any of it: rule (1.1) makes the package use exported named typeLinkManager, and (2.1) makes that type use all its exported methods, so they are transitively live because the type is exported.Two consequences for this PR's claims:
Ensure*fix does not cover the CNI bridge.EnsureBridgeis dead; the live bridge path is thecniBridgeLinkManagerinterface inmtu.go:19-24—Exists/EnsureMTUWithCache/EnsureBridgePortMTUs/EnsureBridgePodMTUs— and is untouched here.*WithCachemethods, which decide viacache.HasLink()and go straight to creation on a miss, retain the same bug shape sourced from a stale cache rather than a failed syscall. That is not live residual risk, because nothing calls them.Deliberately not deleted here, to keep this PR to one idea. Tracked in #670, along with a proposal to evaluate
x/tools/cmd/deadcode, which does the whole-program analysisunuseddeclines to.The tests fail without the fix
Measured against this branch's HEAD, in both directions:
DeleteLink: 10 failingEnsure*subtests and a failingDeleteLink.%!w(<nil>)in the output.Exists()counter: the metric test fails.The seams are package-level vars, so no test calls
t.Parallel(): parallel subtests overwrite each other's stubs and the failures look like the code misbehaving. There is a comment saying so. This is enforced by convention, not by the compiler.Verification
Against the pinned golangci-lint v2.13.1 (
Makefile:435), not a locally-installed version.go build ./...clean,make lint0 issues,make fmtidempotentgo test -race ./internal/net/...all passnilerrmeasured alone: 14 → 13, withlink_manager.gogone from the listRisk worth naming: this touches every live tunnel-creation path on the node. The tests are unit-level with seams and cannot prove real netlink behaviour.
gantry e2e,agent e2eandpxe-smokeare the actual safety net.The first CI run after the round-2 push showed two failures,
LintandStorage Integration. Both were transient module-proxy errors during dependency download —sum.golang.organdproxy.golang.orgreturningINTERNAL_ERRORthree minutes apart — and both died before reaching any code from this branch. Rerun is green.Review responses
Round 2
Ensure*have no production callerDeleteLink's error return is unobservableDeleteLinkdouble-checksisLinkGoneErrorkloginExists()would trip the nil-fixture panicroute_manager.go:161same shapeOn m3. The
klog.V(2)inExists()is gated on!isLinkGoneError(err), which excludes precisely the gone errors, soLinkNotFoundError{}never reaches that format string at any verbosity.errors.Astype-matches by reflection and never callsError(). The trap is real for any future log placed on the gone path — which the fixture comment already warns about — but it is not reachable today.Round 1
lookupError(nil)non-nil regresses 2 pathslookupError(nil)misspellshould flagrecognisesmisspellblock exists in.golangci.yaml, so nolocale: US;"recognises"appears only inwords_us.go:960, gated behind it. Verified twice at 0 issuesLinkNotFoundError{}would panic if formattedWithCache