Repository: cilium/cilium
Pull request #14390 (2020-12-13 02:01:41)

policy: Don't nil an empty selectors map.
[ upstream commit 03bfb2bece5108549b3d613e119059758035d448 ]

Turns out unit testing did not need this any more and this actually caused a runtime panic.

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>


jrajahalme: test-backport-1.8

Patch:
diff --git a/pkg/policy/mapstate.go b/pkg/policy/mapstate.go
index f6625a6fcd..c0d96cb270 100644
--- a/pkg/policy/mapstate.go
+++ b/pkg/policy/mapstate.go
@@ -164,10 +164,6 @@ func (keys MapState) addKeyWithChanges(key Key, entry MapStateEntry, adds, delet
 	} else if len(entry.selectors) > 0 {
 		// create a new selectors map
 		updatedEntry.selectors = make(map[CachedSelector]struct{}, len(entry.selectors))
-	} else {
-		// Keep the map as nil when empty. This makes a difference for unit testing only.
-		// MergeSelectors below becomes a no-op as 'entry' is empty.
-		updatedEntry.selectors = nil
 	}
 
 	// TODO: Do we need to merge labels as well?
@@ -178,8 +174,7 @@ func (keys MapState) addKeyWithChanges(key Key, entry MapStateEntry, adds, delet
 
 	// Record an incremental Add if desired and entry is new or changed
 	if adds != nil && (!exists || oldEntry.ProxyPort != entry.ProxyPort) {
-		updatedEntry.selectors = nil
-		adds[key] = updatedEntry // copy w/o selectors
+		adds[key] = updatedEntry
 		// Key add overrides any previous delete of the same key
 		if deletes != nil {
 			delete(deletes, key)
@@ -200,11 +195,13 @@ func (keys MapState) deleteKeyWithChanges(key Key, cs CachedSelector, adds, dele
 				if len(entry.selectors) > 0 {
 					return
 				}
+			} else {
+				// 'cs' was not found, do not change anything
+				return
 			}
 		}
 		if deletes != nil {
-			entry.selectors = nil
-			deletes[key] = entry // copy w/o selectors
+			deletes[key] = entry
 			// Remove a potential previously added key
 			if adds != nil {
 				delete(adds, key)
@@ -227,8 +224,7 @@ func (keys MapState) redirectPreferredInsertWithChanges(key Key, entry MapStateE
 		// We store the new entry here, the proxy port of it will be fixed up before
 		// insertion to the bpf map.
 		if adds != nil && entry.IsRedirectEntry() {
-			entry.selectors = nil
-			adds[key] = entry // copy w/o selectors
+			adds[key] = entry
 		}
 		return
 	}


Test patch:
diff --git a/pkg/policy/mapstate_test.go b/pkg/policy/mapstate_test.go
index 39609441d3..2ee0ad1b56 100644
--- a/pkg/policy/mapstate_test.go
+++ b/pkg/policy/mapstate_test.go
@@ -27,13 +27,9 @@ import (
 // No selectors is represented with a 'nil' map.
 func (e MapStateEntry) WithSelectors(selectors ...CachedSelector) MapStateEntry {
 	mse := e
-	if len(selectors) > 0 {
-		mse.selectors = make(map[CachedSelector]struct{}, len(selectors))
-		for _, cs := range selectors {
-			mse.selectors[cs] = struct{}{}
-		}
-	} else {
-		mse.selectors = nil
+	mse.selectors = make(map[CachedSelector]struct{}, len(selectors))
+	for _, cs := range selectors {
+		mse.selectors[cs] = struct{}{}
 	}
 	return mse
 }
@@ -98,11 +94,9 @@ func (ds *PolicyTestSuite) TestMapState_AccumulateMapChanges(c *check.C) {
 		entry := MapStateEntry{
 			ProxyPort: proxyPort,
 		}
-		if len(selectors) > 0 {
-			entry.selectors = make(map[CachedSelector]struct{}, len(selectors))
-			for _, cs := range selectors {
-				entry.selectors[cs] = struct{}{}
-			}
+		entry.selectors = make(map[CachedSelector]struct{}, len(selectors))
+		for _, cs := range selectors {
+			entry.selectors[cs] = struct{}{}
 		}
 		return entry
 	}
@@ -132,7 +126,7 @@ func (ds *PolicyTestSuite) TestMapState_AccumulateMapChanges(c *check.C) {
 			HttpIngressKey(42): TestEntry(0, csFoo),
 		},
 		adds: MapState{
-			HttpIngressKey(42): TestEntry(0),
+			HttpIngressKey(42): TestEntry(0, csFoo),
 		},
 		deletes: MapState{},
 	}, {
@@ -156,8 +150,8 @@ func (ds *PolicyTestSuite) TestMapState_AccumulateMapChanges(c *check.C) {
 			HttpIngressKey(43): TestEntry(0, csFoo),
 		},
 		adds: MapState{
-			HttpIngressKey(42): TestEntry(0),
-			HttpIngressKey(43): TestEntry(0),
+			HttpIngressKey(42): TestEntry(0, csFoo),
+			HttpIngressKey(43): TestEntry(0, csFoo),
 		},
 		deletes: MapState{},
 	}, {
@@ -172,7 +166,7 @@ func (ds *PolicyTestSuite) TestMapState_AccumulateMapChanges(c *check.C) {
 			HttpIngressKey(44): TestEntry(0, csBar),
 		},
 		adds: MapState{
-			HttpIngressKey(44): TestEntry(0),
+			HttpIngressKey(44): TestEntry(0, csBar),
 		},
 		deletes: MapState{},
 	}, {
@@ -190,10 +184,23 @@ func (ds *PolicyTestSuite) TestMapState_AccumulateMapChanges(c *check.C) {
 		deletes: MapState{},
 	}, {
 		continued: true,
-		name:      "test-6 - Deleting 42 from Bar, deleted",
+		name:      "test-5b - Deleting 42 from Foo again, not deleted",
 		args: []args{
 			{cs: csFoo, adds: []int{}, deletes: []int{42}, port: 80, proto: 6, ingress: true, redirect: false},
 		},
+		state: MapState{
+			HttpIngressKey(42): TestEntry(0, csBar),
+			HttpIngressKey(43): TestEntry(0, csFoo),
+			HttpIngressKey(44): TestEntry(0, csBar),
+		},
+		adds:    MapState{},
+		deletes: MapState{},
+	}, {
+		continued: true,
+		name:      "test-6 - Deleting 42 from Bar, deleted",
+		args: []args{
+			{cs: csBar, adds: []int{}, deletes: []int{42}, port: 80, proto: 6, ingress: true, redirect: false},
+		},
 		state: MapState{
 			HttpIngressKey(43): TestEntry(0, csFoo),
 			HttpIngressKey(44): TestEntry(0, csBar),
@@ -230,10 +237,10 @@ func (ds *PolicyTestSuite) TestMapState_AccumulateMapChanges(c *check.C) {
 			DNSTCPEgressKey(42): TestEntry(0, csBar),
 		},
 		adds: MapState{
-			AnyIngressKey():     TestEntry(0),
-			HostIngressKey():    TestEntry(0),
-			DNSUDPEgressKey(42): TestEntry(0),
-			DNSTCPEgressKey(42): TestEntry(0),
+			AnyIngressKey():     TestEntry(0, nil),
+			HostIngressKey():    TestEntry(0, nil),
+			DNSUDPEgressKey(42): TestEntry(0, csBar),
+			DNSTCPEgressKey(42): TestEntry(0, csBar),
 		},
 		deletes: MapState{},
 	}, {
@@ -250,7 +257,7 @@ func (ds *PolicyTestSuite) TestMapState_AccumulateMapChanges(c *check.C) {
 			HttpEgressKey(43):   TestEntry(1, csFoo),
 		},
 		adds: MapState{
-			HttpEgressKey(43): TestEntry(1),
+			HttpEgressKey(43): TestEntry(1, csFoo),
 		},
 		deletes: MapState{},
 	}, {
diff --git a/pkg/policy/resolve_test.go b/pkg/policy/resolve_test.go
index 2a18220dff..72bbfdf76f 100644
--- a/pkg/policy/resolve_test.go
+++ b/pkg/policy/resolve_test.go
@@ -642,8 +642,8 @@ func (ds *PolicyTestSuite) TestMapStateWithIngress(c *C) {
 	c.Assert(policy.policyMapChanges.changes, IsNil)
 
 	c.Assert(adds, checker.Equals, MapState{
-		{Identity: 192, DestPort: 80, Nexthdr: 6}: rule1MapStateEntry.WithoutSelectors(),
-		{Identity: 194, DestPort: 80, Nexthdr: 6}: rule1MapStateEntry.WithoutSelectors(),
+		{Identity: 192, DestPort: 80, Nexthdr: 6}: rule1MapStateEntry,
+		{Identity: 194, DestPort: 80, Nexthdr: 6}: rule1MapStateEntry,
 	})
 	c.Assert(deletes, checker.Equals, MapState{
 		{Identity: 193, DestPort: 80, Nexthdr: 6}: rule1MapStateEntry.WithoutSelectors(),