Repository: cilium/cilium
Pull request #14386 (2020-12-12 18:09:34)

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


jrajahalme: test-me-please
jrajahalme: test-me-please
jrajahalme: retest-net-next

Patch:
diff --git a/pkg/policy/mapstate.go b/pkg/policy/mapstate.go
index 88f6550b2a..063272607b 100644
--- a/pkg/policy/mapstate.go
+++ b/pkg/policy/mapstate.go
@@ -174,10 +174,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?
@@ -188,8 +184,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.DatapathEqual(&entry)) {
-		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)
@@ -210,11 +205,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)
@@ -333,8 +330,7 @@ func (keys MapState) RedirectPreferredInsert(key Key, entry MapStateEntry, adds,
 		// insertion to the bpf map.
 		// TODO: Remove this hack when not needed any more.
 		if adds != nil && entry.IsRedirectEntry() && oldEntry.IsRedirectEntry() {
-			entry.selectors = nil
-			adds[key] = entry // copy w/o selectors
+			adds[key] = entry
 		}
 		return
 	}


Test patch:
diff --git a/pkg/policy/distillery_test.go b/pkg/policy/distillery_test.go
index ade82d7ec3..a364b0f774 100644
--- a/pkg/policy/distillery_test.go
+++ b/pkg/policy/distillery_test.go
@@ -425,7 +425,7 @@ func (d *policyDistillery) distillPolicy(owner PolicyOwner, epLabels labels.Labe
 // cache at the time when the expectations are created.
 func (m MapState) clearCachedSelectors() {
 	for k, v := range m {
-		v.selectors = nil
+		v.selectors = make(map[CachedSelector]struct{})
 		m[k] = v
 	}
 }
diff --git a/pkg/policy/mapstate_test.go b/pkg/policy/mapstate_test.go
index 50691b69f4..515c14ea6a 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
 }
@@ -776,11 +772,9 @@ func (ds *PolicyTestSuite) TestMapState_AccumulateMapChanges(c *check.C) {
 			ProxyPort: proxyPort,
 			IsDeny:    deny,
 		}
-		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
 	}
@@ -811,7 +805,7 @@ func (ds *PolicyTestSuite) TestMapState_AccumulateMapChanges(c *check.C) {
 			HttpIngressKey(42): TestEntry(0, false, csFoo),
 		},
 		adds: MapState{
-			HttpIngressKey(42): TestEntry(0, false),
+			HttpIngressKey(42): TestEntry(0, false, csFoo),
 		},
 		deletes: MapState{},
 	}, {
@@ -835,8 +829,8 @@ func (ds *PolicyTestSuite) TestMapState_AccumulateMapChanges(c *check.C) {
 			HttpIngressKey(43): TestEntry(0, false, csFoo),
 		},
 		adds: MapState{
-			HttpIngressKey(42): TestEntry(0, false),
-			HttpIngressKey(43): TestEntry(0, false),
+			HttpIngressKey(42): TestEntry(0, false, csFoo),
+			HttpIngressKey(43): TestEntry(0, false, csFoo),
 		},
 		deletes: MapState{},
 	}, {
@@ -851,7 +845,7 @@ func (ds *PolicyTestSuite) TestMapState_AccumulateMapChanges(c *check.C) {
 			HttpIngressKey(44): TestEntry(0, false, csBar),
 		},
 		adds: MapState{
-			HttpIngressKey(44): TestEntry(0, false),
+			HttpIngressKey(44): TestEntry(0, false, csBar),
 		},
 		deletes: MapState{},
 	}, {
@@ -869,10 +863,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, deny: false},
 		},
+		state: MapState{
+			HttpIngressKey(42): TestEntry(0, false, csBar),
+			HttpIngressKey(43): TestEntry(0, false, csFoo),
+			HttpIngressKey(44): TestEntry(0, false, 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, deny: false},
+		},
 		state: MapState{
 			HttpIngressKey(43): TestEntry(0, false, csFoo),
 			HttpIngressKey(44): TestEntry(0, false, csBar),
@@ -909,10 +916,10 @@ func (ds *PolicyTestSuite) TestMapState_AccumulateMapChanges(c *check.C) {
 			DNSTCPEgressKey(42): TestEntry(0, false, csBar),
 		},
 		adds: MapState{
-			AnyIngressKey():     TestEntry(0, false),
-			HostIngressKey():    TestEntry(0, false),
-			DNSUDPEgressKey(42): TestEntry(0, false),
-			DNSTCPEgressKey(42): TestEntry(0, false),
+			AnyIngressKey():     TestEntry(0, false, nil),
+			HostIngressKey():    TestEntry(0, false, nil),
+			DNSUDPEgressKey(42): TestEntry(0, false, csBar),
+			DNSTCPEgressKey(42): TestEntry(0, false, csBar),
 		},
 		deletes: MapState{},
 	}, {
@@ -929,7 +936,7 @@ func (ds *PolicyTestSuite) TestMapState_AccumulateMapChanges(c *check.C) {
 			HttpEgressKey(43):   TestEntry(1, false, csFoo),
 		},
 		adds: MapState{
-			HttpEgressKey(43): TestEntry(1, false),
+			HttpEgressKey(43): TestEntry(1, false, csFoo),
 		},
 		deletes: MapState{},
 	}, {
diff --git a/pkg/policy/resolve_deny_test.go b/pkg/policy/resolve_deny_test.go
index db8c6dde7f..1db359158a 100644
--- a/pkg/policy/resolve_deny_test.go
+++ b/pkg/policy/resolve_deny_test.go
@@ -490,8 +490,8 @@ func (ds *PolicyTestSuite) TestMapStateWithIngressDeny(c *C) {
 	// maps on the policy got cleared
 
 	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(),
diff --git a/pkg/policy/resolve_test.go b/pkg/policy/resolve_test.go
index 27b1590f40..6d2c944365 100644
--- a/pkg/policy/resolve_test.go
+++ b/pkg/policy/resolve_test.go
@@ -664,8 +664,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(),