Fix nodeport egress tuple reuse for closed connections

Project: cilium/cilium | Language: Go (BPF C) | Concepts: error-handling, testing-strategy | Difficulty: mid
Link: https://github.com/cilium/cilium/pull/48306

The problem

When a NodePort connection is closed, its connection tracking (CT) egress tuple could be reused by a new, unrelated flow. The old, stale CT entry could incorrectly drive reverse NAT for the new flow, causing replies to be rewritten to the wrong destination and breaking the connection.

Why it's non-trivial

A naive fix might only clear the CT entry, but in high-performance networking, tuples can be rapidly reused. The system must distinguish between truly closed connections and those still in use, and only skip NodePort logic for the former—without breaking legitimate flows or introducing regressions.

The approach

1. Add a liveness check to NodePort egress CT entry logic

The function __ct_has_nodeport_egress_entry is updated to skip entries for closed connections:

+	if (!ct_entry_alive(entry))
+		return false;

What this does:
Before, the function would treat any CT entry with node_port set as valid for NodePort reverse NAT. Now, it first checks if the entry is "alive" (i.e., the connection is not fully closed). If not, it returns false, skipping NodePort processing for that tuple. This prevents stale entries from interfering with new, unrelated connections.

2. Add targeted tests for the new logic

A new test block is added to verify the correct handling of closed and open CT entries:

+	TEST("ct_has_nodeport_egress_entry", {
+		struct ct_entry entry = {};
+		__u16 rev_nat_index = 0;
+
+		entry.node_port = 1;
+		entry.rev_nat_index = 42;
+
+		/* An alive NodePort entry drives reverse-NAT. */
+		assert(__ct_has_nodeport_egress_entry(&entry, &rev_nat_index, false));
+		assert(rev_nat_index == 42);
+
+		/* Once both directions have seen a closing packet, the connection
+		 * is fully closed. Its egress tuple can now be reused by an
+		 * unrelated flow, so it must no longer be treated as a NodePort
+		 * egress entry.
+		 */
+		rev_nat_index = 0;
+		entry.rx_closing = 1;
+		entry.tx_closing = 1;
+		assert(!__ct_has_nodeport_egress_entry(&entry, &rev_nat_index, false));
+		assert(rev_nat_index == 0);
+
+		/* Closing in only one direction doesn't terminate the connection. */
+		entry.tx_closing = 0;
+		assert(__ct_has_nodeport_egress_entry(&entry, &rev_nat_index, false));
+		assert(rev_nat_index == 42);
+	});

What this does:

• Checks that a living NodePort entry correctly drives reverse NAT.
• Checks that a fully closed entry (both directions closed) is skipped.
• Checks that only one direction closed does not terminate the connection.

From the review thread

fristonio: There's a bunch more raw lookup helpers in conntrack.h, I could see them wanting a similar change.

I made a point fix for the bug that we were observing as CI flake. I am not super familiar with the conntrack bpf handling and did not attempt to find/fix other lookup helpers. Happy to make fixes for other call sites if you can help me figure out scenarios that might be similarly affected.

fristonio: Are we no longer doing backports for fixes?

Yes, we need backport for the fix. I have added the relevant labels.

Takeaways

• Always check for connection liveness before applying connection tracking logic to avoid acting on stale entries.
• When fixing a bug, add targeted tests that cover both the broken and fixed scenarios, including edge cases.
• Point fixes are sometimes necessary, but similar logic may need to be audited elsewhere in the codebase to prevent related bugs.