ctmap: consider CT entry's .dsr flag in PurgeOrphanNATEntries()

[ upstream commit dfbae95 ]

[ backporter's notes: also bring back isDsrEntry() ]

The BPF datapath potentially re-creates a CT entry, in particular when a
DSR connection gets re-opened as local connection. Such a re-purposed CT
entry then leaves a DSR NAT entry behind.

Currently we wouldn't clean up such NAT entries (as the matching CT entry
still exists). But once we look at the CT entry's .dsr flag, we understand
that the CT entry is actually no longer a match for the NAT entry.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>

pkg/maps/ctmap/ctmap.go

@@ -609,7 +609,7 @@ func PurgeOrphanNATEntries(ctMapTCP, ctMapAny *Map) *NatGCStats {
 		if natKey.GetFlags()&tuple.TUPLE_F_IN == tuple.TUPLE_F_IN { // natKey is r(everse)tuple
 			ctKey := egressCTKeyFromIngressNatKeyAndVal(natKey, natVal)
 
-			if !ctEntryExist(ctMap, ctKey) {
+			if !ctEntryExist(ctMap, ctKey, nil) {
 				// No egress CT entry is found, delete the orphan ingress SNAT entry
 				if deleted, _ := natMap.Delete(natKey); deleted {
 					stats.IngressDeleted += 1
@@ -618,11 +618,16 @@ func PurgeOrphanNATEntries(ctMapTCP, ctMapAny *Map) *NatGCStats {
 				stats.IngressAlive += 1
 			}
 		} else if natKey.GetFlags()&tuple.TUPLE_F_OUT == tuple.TUPLE_F_OUT {
+			checkDsr := func(entry *CtEntry) bool {
+				return entry.isDsrEntry()
+			}
+
 			ingressCTKey := ingressCTKeyFromEgressNatKey(natKey)
 			egressCTKey := egressCTKeyFromEgressNatKey(natKey)
 
-			if !ctEntryExist(ctMap, ingressCTKey) && !ctEntryExist(ctMap, egressCTKey) {
-				// No ingress and egress CT entries were found, delete the orphan egress NAT entry
+			if !ctEntryExist(ctMap, ingressCTKey, checkDsr) &&
+				!ctEntryExist(ctMap, egressCTKey, nil) {
+				// No relevant CT entries were found, delete the orphan egress NAT entry
 				if deleted, _ := natMap.Delete(natKey); deleted {
 					stats.EgressDeleted += 1
 				}

pkg/maps/ctmap/ctmap_privileged_test.go

@@ -435,6 +435,31 @@ func (k *CTMapTestSuite) TestOrphanNatGC(c *C) {
 	c.Assert(err, IsNil)
 	c.Assert(len(buf), Equals, 0)
 
+	// When a connection is re-opened and switches from DSR to local-backend,
+	// its CT entry gets re-created but uses the same CT tuple as key.
+	//
+	// Validate that we clean up the stale DSR NAT entry in such a case.
+	ctVal.Flags = 0
+
+	err = bpf.UpdateElement(ctMapTCP.Map.GetFd(), ctMapTCP.Map.Name(), unsafe.Pointer(ctKey),
+		unsafe.Pointer(ctVal), 0)
+	c.Assert(err, IsNil)
+
+	err = bpf.UpdateElement(natMap.Map.GetFd(), natMap.Map.Name(), unsafe.Pointer(natKey),
+		unsafe.Pointer(natVal), 0)
+	c.Assert(err, IsNil)
+
+	stats = PurgeOrphanNATEntries(ctMapTCP, ctMapTCP)
+	c.Assert(stats.IngressAlive, Equals, uint32(0))
+	c.Assert(stats.IngressDeleted, Equals, uint32(0))
+	c.Assert(stats.EgressAlive, Equals, uint32(0))
+	c.Assert(stats.EgressDeleted, Equals, uint32(1))
+	// Check that the orphan NAT entry has been removed
+	buf = make(map[string][]string)
+	err = natMap.Map.Dump(buf)
+	c.Assert(err, IsNil)
+	c.Assert(len(buf), Equals, 0)
+
 	// Let's check IPv6
 
 	natMapV6 := nat.NewMap("cilium_nat_any6_test", false, 1000)

pkg/maps/ctmap/types.go

@@ -522,6 +522,10 @@ const (
 	MaxFlags
 )
 
+func (c *CtEntry) isDsrEntry() bool {
+	return c.Flags&DSR != 0
+}
+
 func (c *CtEntry) flagsString() string {
 	var sb strings.Builder
 

pkg/maps/ctmap/utils.go

@@ -5,6 +5,7 @@ package ctmap
 
 import (
 	"errors"
+	"unsafe"
 
 	"golang.org/x/sys/unix"
 
@@ -133,7 +134,18 @@ func egressCTKeyFromEgressNatKey(k nat.NatKey) bpf.MapKey {
 	}
 }
 
-func ctEntryExist(ctMap *Map, ctKey bpf.MapKey) bool {
-	_, err := ctMap.Lookup(ctKey)
-	return !errors.Is(err, unix.ENOENT)
+func ctEntryExist(ctMap *Map, ctKey bpf.MapKey, f func(*CtEntry) bool) bool {
+	v, err := ctMap.Lookup(ctKey)
+
+	if err != nil {
+		return !errors.Is(err, unix.ENOENT)
+	}
+
+	if f == nil {
+		return true
+	}
+
+	val := (*CtEntry)(unsafe.Pointer(v.GetValuePtr()))
+
+	return f(val)
 }