ctmap: Iterate SNAT map once when doing GC

Previously, after receiving the signal from the datapath, we iterated
NAT map twice: first to compare against CT TCP map, second - against CT
any map.

Obviously, doing the iterations two times was inefficient. This commit
fixes that by passing both CT {TCP,any} maps to the NAT GC routine. This
allows the NAT GC to iterate once.

Suggested-by: Joe Stringer <joe@cilium.io>
Signed-off-by: Martynas Pumputis <m@lambda.lt>

pkg/maps/ctmap/ctmap.go

@@ -542,33 +542,27 @@ func GC(m *Map, filter *GCFilter) int {
 // CT_INGRESS CT entry. See the unit test TestOrphanNatGC for more examples.
 //
 // The function only handles 1-3 cases, the 4. case is TODO(brb).
-func PurgeOrphanNATEntries(ctMap *Map) *NatGCStats {
+func PurgeOrphanNATEntries(ctMapTCP, ctMapAny *Map) *NatGCStats {
 	if option.Config.NodePortMode == option.NodePortModeDSR ||
 		option.Config.NodePortMode == option.NodePortModeHybrid {
 		return nil
 	}
 
-	natMap := mapInfo[ctMap.mapType].natMap
+	// Both CT maps should point to the same natMap, so use the first one
+	// to determine natMap
+	natMap := mapInfo[ctMapTCP.mapType].natMap
 	if natMap == nil {
 		return nil
 	}
 
-	isCTMapTCP := ctMap.mapType == mapTypeIPv4TCPLocal ||
-		ctMap.mapType == mapTypeIPv6TCPLocal ||
-		ctMap.mapType == mapTypeIPv4TCPGlobal ||
-		ctMap.mapType == mapTypeIPv6TCPGlobal
 	stats := newNatGCStats(natMap)
-
 	cb := func(key bpf.MapKey, value bpf.MapValue) {
 		natKey := key.(nat.NatKey)
 		natVal := value.(nat.NatEntry)
 
-		// In opposite to the CT maps, TCP and UDP entries are stored in the same
-		// SNAT map. Therefore, to avoid a case when the given ctMap does not
-		// store entries of the given natKey.NextHeader proto, we should return
-		// early. Otherwise, the natKey entries will be removed, which is wrong.
-		if (natKey.GetNextHeader() == u8proto.TCP) != isCTMapTCP {
-			return
+		ctMap := ctMapAny
+		if natKey.GetNextHeader() == u8proto.TCP {
+			ctMap = ctMapTCP
 		}
 
 		if natKey.GetFlags()&tuple.TUPLE_F_IN == 1 { // natKey is r(everse)tuple

pkg/maps/ctmap/ctmap_privileged_test.go

@@ -222,23 +222,23 @@ func (k *CTMapTestSuite) TestOrphanNatGC(c *C) {
 	c.Assert(err, IsNil)
 	defer natMap.Map.Unpin()
 
-	ctMapName := MapNameAny4Global + "_test"
-	setupMapInfo(mapTypeIPv4AnyGlobal, ctMapName,
+	ctMapAnyName := MapNameAny4Global + "_test"
+	setupMapInfo(mapTypeIPv4AnyGlobal, ctMapAnyName,
 		&CtKey4Global{}, int(unsafe.Sizeof(CtKey4Global{})),
 		100, natMap)
-	ctMap := newMap(ctMapName, mapTypeIPv4AnyGlobal)
-	_, err = ctMap.OpenOrCreate()
+	ctMapAny := newMap(ctMapAnyName, mapTypeIPv4AnyGlobal)
+	_, err = ctMapAny.OpenOrCreate()
 	c.Assert(err, IsNil)
-	defer ctMap.Map.Unpin()
+	defer ctMapAny.Map.Unpin()
 
-	ctTCPMapName := MapNameTCP4Global + "_test"
-	setupMapInfo(mapTypeIPv4TCPGlobal, ctTCPMapName,
+	ctMapTCPName := MapNameTCP4Global + "_test"
+	setupMapInfo(mapTypeIPv4TCPGlobal, ctMapTCPName,
 		&CtKey4Global{}, int(unsafe.Sizeof(CtKey4Global{})),
 		100, natMap)
-	ctTCPMap := newMap(ctTCPMapName, mapTypeIPv4TCPGlobal)
-	_, err = ctTCPMap.OpenOrCreate()
+	ctMapTCP := newMap(ctMapTCPName, mapTypeIPv4TCPGlobal)
+	_, err = ctMapTCP.OpenOrCreate()
 	c.Assert(err, IsNil)
-	defer ctTCPMap.Map.Unpin()
+	defer ctMapTCP.Map.Unpin()
 
 	// Create the following entries and check that SNAT entries are NOT GC-ed
 	// (as we have the CT entry which they belong to):
@@ -278,7 +278,7 @@ func (k *CTMapTestSuite) TestOrphanNatGC(c *C) {
 		TxBytes:   216,
 		Lifetime:  37459,
 	}
-	err = bpf.UpdateElement(ctMap.Map.GetFd(), unsafe.Pointer(ctKey),
+	err = bpf.UpdateElement(ctMapAny.Map.GetFd(), unsafe.Pointer(ctKey),
 		unsafe.Pointer(ctVal), 0)
 	c.Assert(err, IsNil)
 
@@ -325,28 +325,7 @@ func (k *CTMapTestSuite) TestOrphanNatGC(c *C) {
 		unsafe.Pointer(natVal), 0)
 	c.Assert(err, IsNil)
 
-	ctKeyTCP := &CtKey4Global{
-		tuple.TupleKey4Global{
-			tuple.TupleKey4{
-				DestAddr:   types.IPv4{10, 23, 32, 45},
-				SourceAddr: types.IPv4{10, 23, 53, 48},
-				SourcePort: 0x50d6,
-				DestPort:   0x1821,
-				NextHeader: u8proto.TCP,
-				Flags:      tuple.TUPLE_F_OUT,
-			},
-		},
-	}
-	err = bpf.UpdateElement(ctTCPMap.Map.GetFd(), unsafe.Pointer(ctKeyTCP),
-		unsafe.Pointer(ctVal), 0)
-	c.Assert(err, IsNil)
-
-	// UDP SNAT entries should not be removed when the TCP CT map is given
-	stats := PurgeOrphanNATEntries(ctTCPMap)
-	c.Assert(stats.IngressDeleted, Equals, uint32(0))
-	c.Assert(stats.EgressDeleted, Equals, uint32(0))
-
-	stats = PurgeOrphanNATEntries(ctMap)
+	stats := PurgeOrphanNATEntries(ctMapTCP, ctMapAny)
 	c.Assert(stats.IngressAlive, Equals, uint32(1))
 	c.Assert(stats.IngressDeleted, Equals, uint32(0))
 	c.Assert(stats.EgressDeleted, Equals, uint32(0))
@@ -357,9 +336,9 @@ func (k *CTMapTestSuite) TestOrphanNatGC(c *C) {
 	c.Assert(len(buf), Equals, 2)
 
 	// Now remove the CT entry which should remove both NAT entries
-	err = bpf.DeleteElement(ctMap.Map.GetFd(), unsafe.Pointer(ctKey))
+	err = bpf.DeleteElement(ctMapAny.Map.GetFd(), unsafe.Pointer(ctKey))
 	c.Assert(err, IsNil)
-	stats = PurgeOrphanNATEntries(ctMap)
+	stats = PurgeOrphanNATEntries(ctMapTCP, ctMapAny)
 	c.Assert(stats.IngressDeleted, Equals, uint32(1))
 	c.Assert(stats.IngressAlive, Equals, uint32(0))
 	c.Assert(stats.EgressDeleted, Equals, uint32(1))
@@ -374,7 +353,7 @@ func (k *CTMapTestSuite) TestOrphanNatGC(c *C) {
 		unsafe.Pointer(natVal), 0)
 	c.Assert(err, IsNil)
 
-	stats = PurgeOrphanNATEntries(ctMap)
+	stats = PurgeOrphanNATEntries(ctMapTCP, ctMapAny)
 	c.Assert(stats.IngressDeleted, Equals, uint32(1))
 	c.Assert(stats.EgressDeleted, Equals, uint32(0))
 	buf = make(map[string][]string)

pkg/maps/ctmap/ctmap_test.go

@@ -86,3 +86,25 @@ func (t *CTMapTestSuite) TestCalculateInterval(c *C) {
 	c.Assert(calculateInterval(bpf.MapTypeLRUHash, 24*time.Hour, 0.01), Equals, defaults.ConntrackGCMaxLRUInterval)
 	c.Assert(calculateInterval(bpf.MapTypeHash, 24*time.Hour, 0.01), Equals, defaults.ConntrackGCMaxInterval)
 }
+
+func (t *CTMapTestSuite) TestFilterMapsByProto(c *C) {
+	maps := []*Map{
+		newMap("tcp4", mapTypeIPv4TCPGlobal),
+		newMap("any4", mapTypeIPv4AnyGlobal),
+		newMap("tcp6", mapTypeIPv6TCPGlobal),
+		newMap("any6", mapTypeIPv6AnyGlobal),
+	}
+
+	ctMapTCP, ctMapAny := FilterMapsByProto(maps, CTMapIPv4)
+	c.Assert(ctMapTCP.mapType, Equals, mapTypeIPv4TCPGlobal)
+	c.Assert(ctMapAny.mapType, Equals, mapTypeIPv4AnyGlobal)
+
+	ctMapTCP, ctMapAny = FilterMapsByProto(maps, CTMapIPv6)
+	c.Assert(ctMapTCP.mapType, Equals, mapTypeIPv6TCPGlobal)
+	c.Assert(ctMapAny.mapType, Equals, mapTypeIPv6AnyGlobal)
+
+	maps = maps[0:2] // remove ipv6 maps
+	ctMapTCP, ctMapAny = FilterMapsByProto(maps, CTMapIPv6)
+	c.Assert(ctMapTCP, IsNil)
+	c.Assert(ctMapAny, IsNil)
+}

pkg/maps/ctmap/gc/gc.go

@@ -199,14 +199,26 @@ func runGC(e *endpoint.Endpoint, ipv4, ipv6, triggeredBySignal bool, filter *ctm
 				"count":        deleted,
 			}).Debug("Deleted filtered entries from map")
 		}
+	}
+
+	if e == nil && triggeredBySignal {
+		vsns := []ctmap.CTMapIPVersion{}
+		if ipv4 {
+			vsns = append(vsns, ctmap.CTMapIPv4)
+		}
+		if ipv6 {
+			vsns = append(vsns, ctmap.CTMapIPv6)
+		}
 
-		if triggeredBySignal {
-			stats := ctmap.PurgeOrphanNATEntries(m)
+		for _, vsn := range vsns {
+			ctMapTCP, ctMapAny := ctmap.FilterMapsByProto(maps, vsn)
+			stats := ctmap.PurgeOrphanNATEntries(ctMapTCP, ctMapAny)
 			if stats != nil && (stats.EgressDeleted != 0 || stats.IngressDeleted != 0) {
 				log.WithFields(logrus.Fields{
 					"ingressDeleted": stats.IngressDeleted,
 					"egressDeleted":  stats.EgressDeleted,
 					"ingressAlive":   stats.IngressAlive,
+					"ctMapIPVersion": vsn,
 				}).Info("Deleted orphan SNAT entries from map")
 			}
 		}

pkg/maps/ctmap/types.go

@@ -107,6 +107,37 @@ func (m mapType) isTCP() bool {
 	return false
 }
 
+type CTMapIPVersion int
+
+const (
+	CTMapIPv4 CTMapIPVersion = iota
+	CTMapIPv6
+)
+
+// FilterMapsByProto filters the given CT maps by the given IP version, and
+// returns two maps - one for TCP and one for any protocol.
+func FilterMapsByProto(maps []*Map, ipVsn CTMapIPVersion) (ctMapTCP *Map, ctMapAny *Map) {
+	for _, m := range maps {
+		switch ipVsn {
+		case CTMapIPv4:
+			switch m.mapType {
+			case mapTypeIPv4TCPLocal, mapTypeIPv4TCPGlobal:
+				ctMapTCP = m
+			case mapTypeIPv4AnyLocal, mapTypeIPv4AnyGlobal:
+				ctMapAny = m
+			}
+		case CTMapIPv6:
+			switch m.mapType {
+			case mapTypeIPv6TCPLocal, mapTypeIPv6TCPGlobal:
+				ctMapTCP = m
+			case mapTypeIPv6AnyLocal, mapTypeIPv6AnyGlobal:
+				ctMapAny = m
+			}
+		}
+	}
+	return
+}
+
 type CtKey interface {
 	bpf.MapKey