Azure / Azure/unbounded

An aborted configureTunnelPeers pass withdraws every tunnel peer from the eBPF LPM trie

Open
#671 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
28
Forks
11
Avg merge
1d 8h
Merged PRs (30d)
55

Description

Found during review of #668. Pre-existing, and orthogonal to that PR: it is why "defer a reconcile pass" is not a benign outcome on the tunnel path.

## The chain

1. `site_watch_reconcile.go:2046-2048` sets `state.pendingBPFEntries = nil` **before** calling `configureTunnelPeers`.
2. `configureTunnelPeers` returns early on any error, so `pendingBPFEntries` is never repopulated. Two of the abort points are `EnsureGeneveInterface` (`tunnel_config.go:253`) and `EnsureVXLANInterface` (`:385`).
3. The caller only warns and continues: `klog.Warningf("Tunnel configuration failed (WireGuard will still be configured): %v", ...)` (`site_watch_reconcile.go:2066-2067`).
4. `reconcilePendingBPFEntries(state)` still runs at `site_watch_reconcile.go:2147`.
5. It reads the nil map and substitutes an **empty** one (`tunnel_config.go:760-762`):

```go
entries := state.pendingBPFEntries
state.pendingBPFEntries = nil
...
if entries == nil {
entries = make(map[string]ebpfpkg.TunnelEndpoint)
}

if err := tm.Reconcile(entries); err != nil {
```

6. `TunnelMap.Reconcile` is documented as *"sets the LPM trie to exactly match the desired state. Stale entries (present in the kernel but not in desired) are removed"* and deletes accordingly (`internal/net/ebpf/tunnel_map.go:288-296`).

An empty desired set therefore means **every** GENEVE/VXLAN/IPIP peer is deleted from the trie, on a pass that only logged a warning.

## Why it matters

The failure is presented as recoverable — the log says WireGuard will still be configured, and the next pass will retry. But the dataplane for every shared-tunnel peer is torn down in the meantime, so the cost of one aborted pass is a tunnel outage rather than a deferred update.

Same shape at `wireguard_config.go:137`, where an `EnsureWireGuardInterface` failure aborts before `SyncRoutes`.

## Suggested direction

Either do not clear `pendingBPFEntries` until `configureTunnelPeers` has produced a replacement, or skip `reconcilePendingBPFEntries` when the pass aborted. The distinction that matters is "desired set is genuinely empty" versus "desired set was never computed" — the current code cannot tell them apart, and treats the second as the first.

## Relationship to #668

#668 changes `Ensure*` to return an error when a link lookup fails for a reason other than the interface being absent, instead of falling through to creation. In the common case the outcome is unchanged, because the old code fell through to `LinkAdd`, got `EEXIST`, and aborted at the same line.

The one genuinely new way to reach this: the lookup fails transiently **and** the interface really is absent. The old code created it and carried on; the new code defers the pass. That is the right trade for correctness, but its cost is this wipe, which is worth fixing on its own terms.

Contributor guide

Open the contributing guide

Research direction

Start at site_watch_reconcile.go:2046-2147 and trace configureTunnelPeers through tunnel_config.go:253, 385, and 760-762, then read internal/net/ebpf/tunnel_map.go:288-296. Ensure an aborted configuration does not reconcile a nil desired set as empty, while a genuinely empty desired set still reconciles normally; consider the analogous failure path in wireguard_config.go:137.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
networking
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.