unlock unlimited code runs for $79, limited time →
NOSSLETTER.techLIVE
back to OSSpedia
cilium/cilium
mid8/10
refactoringdata-structureserror-handling

wireguard: Reconcile peers from node table

Project: cilium/cilium | Language: Go | Concepts: refactoring, data-structures, error-handling | Difficulty: mid
Link: https://github.com/cilium/cilium/pull/48078

The problem

Previously, WireGuard peer management in Cilium was handled by a bespoke NodeHandler interface, which responded to events from a node manager. This approach made peer state management more error-prone, especially during startup and shutdown, and was not aligned with the newer StateDB-based reconciliation model used elsewhere in the project. There was also a risk that a transient failure during reconciliation could cause desired peers to be pruned prematurely.

Why it's non-trivial

A naive fix might simply replace the event handler with a loop over the node table, but this would miss important synchronization and error-handling details. Specifically:

  • Peer state must not be pruned until all initial node reconciliation is complete, or else valid peers could be removed due to transient errors.
  • The agent must gracefully handle shutdown, ensuring that no further actions are taken after the WireGuard client is closed.
  • The new design must integrate with StateDB's reconciliation and revision-tracking mechanisms, and avoid race conditions or deadlocks.

The approach

1. Remove the old NodeHandler event-based interface:
The file node_handler.go is deleted, removing all the event callback logic:

-diff --git a/pkg/wireguard/agent/node_handler.go b/pkg/wireguard/agent/node_handler.go
-deleted file mode 100644
...
-// NodeAdd is called when a node is discovered for the first time.
-func (a *Agent) NodeAdd(newNode nodeTypes.Node) error {
-	return a.nodeUpsert(newNode)
-}
-...
-// NodeDelete is called after a node has been deleted
-func (a *Agent) NodeDelete(node nodeTypes.Node) error {
-	if node.IsLocal() {
-		return nil
-	}
-	return a.deletePeer(node.Fullname())
-}

This removes the event-driven peer management, paving the way for a reconciler-based approach.

2. Implement a StateDB reconciler for nodes:
A new file, node_reconciler.go, defines how the agent reacts to node table changes:

+func (a *Agent) Update(
+	_ context.Context,
+	_ statedb.ReadTxn,
+	_ statedb.Revision,
+	n *node.Node,
+) error {
+	if n.Local != nil {
+		return nil
+	}
+	if n.WireguardPubKey == "" {
+		return a.deletePeer(n.Fullname())
+	}
+	return a.updatePeer(n.Fullname(), n.WireguardPubKey, n.GetNodeIP(false), n.GetNodeIP(true))
+}
+
+func (a *Agent) Delete(
+	_ context.Context,
+	_ statedb.ReadTxn,
+	_ statedb.Revision,
+	n *node.Node,
+) error {
+	if n.Local != nil {
+		return nil
+	}
+	return a.deletePeer(n.Fullname())
+}

This code defines how the agent adds, updates, or deletes WireGuard peers based on the node table's state.

3. Register the reconciler and wait for reconciliation before pruning peers:
In agent.go, the agent registers the reconciler and ensures that peer pruning only happens after all initial reconciliation is complete:

+a.nodesReconciler, err = reconciler.Register(
+	a.reconcilerParams,
+	a.nodes.(statedb.RWTable[*node.Node]),
+	(*node.Node).DeepCopy,
+	func(n *node.Node, status reconciler.Status) *node.Node {
+		n.Statuses = n.Statuses.Set(wireGuardNodeReconcilerName, status)
+		return n
+	},
+	func(n *node.Node) reconciler.Status {
+		return n.Statuses.Get(wireGuardNodeReconcilerName)
+	},
+	a,
+	nil,
+	reconciler.WithoutPruning(),
+)

This sets up the reconciler for the node table, with custom status handling and disables automatic pruning (so the agent can control when to prune).

In the peer garbage collector, it waits for all nodes to be reconciled before removing any peers:

+	_, initWatch := a.nodes.Initialized(a.db.ReadTxn())
+	select {
+	case <-initWatch:
+	case <-ctx.Done():
+		return nil
+	}
+
+	// Wait until all nodes up to this point have reconciled to ensure
+	// existence of peers derived from them.
+	_, _, err := a.nodesReconciler.WaitUntilReconciled(ctx, a.nodes.Revision(a.db.ReadTxn()))
+	if err != nil {
+		return err
+	}

This ensures that no desired peer is pruned before its corresponding node has had a chance to reconcile.

4. Make peer operations safe after shutdown:
All peer update/delete operations now check if the WireGuard client is closed (a.wgClient == nil) before proceeding:

+	if a.wgClient == nil {
+		return nil
+	}

This prevents panics or errors if the agent is stopped while operations are still in flight.

5. Add targeted tests for the reconciler logic:
A new test file verifies the new reconciler behavior:

+func TestNodeReconciler(t *testing.T) {
+	...
+	require.NoError(t, a.Update(t.Context(), nil, 1, local))
+	require.Empty(t, a.peerByNodeName)
+
+	require.NoError(t, a.Update(t.Context(), nil, 1, n))
+	require.Contains(t, a.peerByNodeName, k8s1NodeName)
+	require.NoError(t, a.Delete(t.Context(), nil, 2, local))
+	require.Contains(t, a.peerByNodeName, k8s1NodeName)
+	...
+	n.WireguardPubKey = ""
+	require.NoError(t, a.Update(t.Context(), nil, 3, n))
+	require.NotContains(t, a.peerByNodeName, k8s1NodeName)
+	...
+	require.NoError(t, a.Delete(t.Context(), nil, 4, n))
+}

This ensures the reconciler adds, updates, and deletes peers as expected, and handles edge cases safely.

From the review thread

gandro: Do we still need to wait on a.clustermesh.NodesSynced(ctx)?

joamaki: ClusterMesh has its own table initializer so just waiting on Initialized() covers it. Removed the NodesSynced call.

gandro: Why is this error removed?

joamaki: Bubbling this up to the reconciler would cause deletions of non-existing peers to be retried indefinitely (could maybe conceivably happen if the initial Update had failed or had never been invoked due to e.g. that node object never been marked Pending). We could log this though, but I'm not sure that's useful considering there's valid reasons to end up here.

gandro: I guess ultimately we should probably move more of the WireGuard architecture into a reconciler, i.e. have the device itself also be configured based on a table.

I slightly prefer logging this (since again, this should never happen), but yeah makes sense not to return an error here.

joamaki: Added the logging.

Takeaways

  • Favor declarative reconciliation over event-driven mutation for managing state derived from shared data sources. This approach is less error-prone and easier to reason about, especially as systems grow more complex.
  • Ensure all initial reconciliation is complete before performing destructive operations like pruning, to avoid data loss from transient failures.
  • Guard all resource access after shutdown to prevent use-after-close bugs, especially when operations may still be in flight or triggered by background processes.
  • When integrating with a reconciliation framework, make error semantics explicit: avoid retrying non-existent resource deletions indefinitely, and prefer idempotence and logging for unexpected-but-benign conditions.