Repository navigation
Refactor SA4006/ineffassign-flagged production-code sites - #1731
Open
randomizedcoder wants to merge 5 commits into
Open
randomizedcoder wants to merge 5 commits into
randomizedcoder wants to merge 5 commits into
Conversation
randomizedcoder
added a commit
to randomizedcoder/nebula
that referenced
this pull request
May 19, 2026
Local record of the upstream PR opened against slackhq/nebula for
the three SA4006 / ineffassign production-code findings in
connection_manager.go, pki.go, and overlay/tun_linux.go.
Captures the 3-commit shape, the okG/okV/okV2 design that the
operator chose over the initial "drop the ok tracking entirely"
proposal, and the decideTrafficAction helper extraction that
trades the inline switch{} doc proposal for a much smaller test
fixture cost.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The in-traffic decision tree in makeTrafficDecision had three
shapes:
if mainHostInfo {
decision = tryRehandshake
} else {
if cm.shouldSwapPrimary(hostinfo) {
decision = swapPrimary
} else {
decision = migrateRelays
}
}
prefixed by `decision := doNothing` which staticcheck and ineffassign
both flag as dead - every branch overwrites it. Only the
tryRehandshake branch had test coverage; swapPrimary and migrateRelays
were exercised end-to-end by integration paths but not pinned by any
unit assertion.
Extract decideTrafficAction(mainHostInfo, shouldSwap bool)
trafficDecision as a pure helper next to shouldSwapPrimary. The
caller computes `shouldSwap := !mainHostInfo &&
cm.shouldSwapPrimary(hostinfo)`, which preserves the original
short-circuit so shouldSwapPrimary is not called in the mainHostInfo
path. The helper is a switch{} expression that maps the (mainHostInfo,
shouldSwap) pair onto the three trafficDecision variants.
Add TestDecideTrafficAction (4 rows) pinning each branch plus the
shouldSwap-is-don't-care-when-primary corner. A mutation test
confirms the table is load-bearing: inverting the swap/migrate
branches makes the migrate row fail.
The original code's `decision := doNothing` initializer was a
defensive fallback. The refactor preserves that defense via the
helper's default arm returning migrateRelays - but a future branch
addition that needs doNothing as the fallback would have to be
explicit, which is the safer evolution path.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
newCertStateFromConfig contained an assignment that staticcheck
SA4006 / ineffassign both flag:
if strings.Contains(pubPathOrPEM, "-----BEGIN") {
rawCert = []byte(pubPathOrPEM)
pubPathOrPEM = "<inline>" <-- never read after this point
} else { ... }
The variable is not referenced past line 317 (the only later read
is in the else-branch above the deletion). Git blame shows the
assignment dates to commit 5a131b2 (slackhq#952, "Combine ca, cert, and
key handling"); the most likely original intent was a log statement
like s.l.Info("Loaded certificate", "source", pubPathOrPEM) that
would emit either the file path or "<inline>". That log was never
written or was removed; the conditional rename was left behind.
Deleting the line is behaviour-preserving by inspection (no other
reads of pubPathOrPEM exist after this point).
Add pki_test.go with TestNewCertStateFromConfig_RejectsBadInput
(3 rows). The function had zero direct test coverage before this
commit; it was only exercised via the integration paths that call
pki.NewPKIFromConfig from higher up the stack. The new rows pin
the three early-return error guards:
- missing pki.key -> error names "no pki.key path or PEM data
provided"
- missing pki.cert with a nonexistent pki.key path -> error
surfaces the pki.key path (loadPrivateKey fails first)
- pki.cert pointing to a nonexistent file -> error names the
path so the operator can grep their config
Happy-path coverage (valid v1 / v2 inline / file-path certs) is
intentionally not added here: it would require ~150 lines of
PEM-encoded cert generation fixture for negligible additional
value over the existing integration paths. A follow-up PR could
add that fixture if the maintainer wants it.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
getGatewayAddr at overlay/tun_linux.go:819 had a shadowed-ok
pattern that staticcheck SA4006 and golangci-lint ineffassign
both flagged:
gwAddr, ok := netip.AddrFromSlice(gw) // outer ok
if !ok {
rVia, ok := via.(*netlink.Via) // SHADOWED ok
if ok {
gwAddr, ok = netip.AddrFromSlice(rVia.Addr) // assigns shadowed ok (dies at scope exit)
}
}
if gwAddr.IsValid() { return gwAddr.Unmap(), true }
return netip.Addr{}, false
The function works today because gwAddr.IsValid() is independently
equivalent to a (non-shadowed) ok - both reflect whether the most
recent AddrFromSlice call succeeded. But the dead-assignment-to-
shadowed-ok pattern is a real-bug HAZARD: a future refactor that
swaps gwAddr.IsValid() for an `if ok` check would silently break
in the RTA_VIA-fallback path because the outer ok is still false
from the original failed parse.
Restructure to early returns with three distinct ok names:
- okG for the RTA_GATEWAY slice parse
- okV for the *netlink.Via type assertion
- okV2 for the RTA_VIA slice parse
Each ok is checked alongside the corresponding IsValid() so the
function still reads "validity comes from two places that agree".
Separate gwAddr / viaAddr locals so the two sources of truth never
share a variable; a future reader can grep for either and see the
provenance directly.
Add overlay/tun_linux_test.go TestGetGatewayAddr (11 rows) for
direct coverage where there was none:
- 3 rows for valid RTA_GATEWAY (IPv4, IPv6, IPv4-mapped-IPv6
asserts the Unmap on return)
- 2 rows for the via-type-assertion rejection (nil via,
MPLSDestination via)
- 2 rows for RTA_VIA fallback happy (IPv4, IPv6)
- 3 rows for RTA_VIA fallback rejected (nil / empty / 5-byte
malformed Addr)
- 1 boundary row: malformed 5-byte gw falls through to valid
RTA_VIA
The table passes against both the pre- and post-refactor forms of
getGatewayAddr, proving behaviour preservation.
Mutation tests confirm the table is load-bearing:
- Dropping the okV check makes the wrong-type-via row panic on
nil deref of rVia.Addr (was previously silent UB).
- Dropping the Unmap on the gateway-happy return makes the
IPv4-mapped-IPv6 row fail (returns ::ffff:10.0.0.1 instead of
10.0.0.1).
The redundant okG && IsValid() and !okV2 || !IsValid() AND-pairs
are defensive-in-depth (netip.AddrFromSlice returns invalid Addr
only when the bool is false, so the two flags always agree today).
Keeping both makes the intent obvious and hedges against future
stdlib semantics changes.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
randomizedcoder
force-pushed
the
sa4006-production-fixes
branch
from
November 2, 2026 05:59
e3ab4da to
fc705de
Compare
Contributor
Author
|
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
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.
Remove three dead-assignment/shadowing sites in traffic decisions, certificate configuration and Linux gateway parsing. Preserve short-circuit behavior and gateway fallback order.
Updated for current PKI and netlink APIs. Certificate tests now use valid keys to reach certificate-loading failures and cover valid inline/file certificates. Full unit suite, vet, repository lint and 100 affected-path race repetitions across CPU 1/2/4 pass.
Broader analyzer follow-ups are tracked separately; the standalone branch still shares upstream's independent shutdown/handshake races.