Repository: cilium/cilium
Pull request #14375 (2020-12-11 12:46:08)

pkg/k8s: fix nil pointer for invalid CCNP
If a CCNP does not contain a spec nor specs, which means the embedded
CNP is nil, it will cause a nil pointer exception if we try to access
those fields. To prevent this we should be checking for this embedded
CNP struct before access its fields.

Fixes: 9d7082e9a57e ("policy: add k8s controller to daemon for clusterwide policies")
Signed-off-by: André Martins <andre@cilium.io>

```release-note
Fix potential nil pointer exception for an invalid CCNP in the Cilium Operator
```

Fixes https://github.com/cilium/cilium/issues/14373


aanm: test-me-please
aanm: retest-runtime
aanm: retest-gke
aanm: test-me-please

Patch:
diff --git a/pkg/k8s/factory_functions.go b/pkg/k8s/factory_functions.go
index cec9af9d38..c997c40924 100644
--- a/pkg/k8s/factory_functions.go
+++ b/pkg/k8s/factory_functions.go
@@ -483,10 +483,12 @@ func ConvertToCCNP(obj interface{}) interface{} {
 			CiliumNetworkPolicy: &cilium_v2.CiliumNetworkPolicy{
 				TypeMeta:   concreteObj.TypeMeta,
 				ObjectMeta: concreteObj.ObjectMeta,
-				Spec:       concreteObj.Spec,
-				Specs:      concreteObj.Specs,
 			},
 		}
+		if concreteObj.CiliumNetworkPolicy != nil {
+			cnp.Spec = concreteObj.Spec
+			cnp.Specs = concreteObj.Specs
+		}
 		*concreteObj = cilium_v2.CiliumClusterwideNetworkPolicy{}
 		return cnp
 
@@ -495,16 +497,19 @@ func ConvertToCCNP(obj interface{}) interface{} {
 		if !ok {
 			return obj
 		}
+		slimCNP := &types.SlimCNP{
+			CiliumNetworkPolicy: &cilium_v2.CiliumNetworkPolicy{
+				TypeMeta:   cnp.TypeMeta,
+				ObjectMeta: cnp.ObjectMeta,
+			},
+		}
+		if cnp.CiliumNetworkPolicy != nil {
+			slimCNP.Spec = cnp.Spec
+			slimCNP.Specs = cnp.Specs
+		}
 		dfsu := cache.DeletedFinalStateUnknown{
 			Key: concreteObj.Key,
-			Obj: &types.SlimCNP{
-				CiliumNetworkPolicy: &cilium_v2.CiliumNetworkPolicy{
-					TypeMeta:   cnp.TypeMeta,
-					ObjectMeta: cnp.ObjectMeta,
-					Spec:       cnp.Spec,
-					Specs:      cnp.Specs,
-				},
-			},
+			Obj: slimCNP,
 		}
 		*cnp = cilium_v2.CiliumClusterwideNetworkPolicy{}
 		return dfsu
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/k8s/factory_functions_test.go b/pkg/k8s/factory_functions_test.go
index ccb13a8b2a..8b3ba99582 100644
--- a/pkg/k8s/factory_functions_test.go
+++ b/pkg/k8s/factory_functions_test.go
@@ -1173,6 +1173,15 @@ func (s *K8sSuite) Test_ConvertToCCNP(c *C) {
 				CiliumNetworkPolicy: &v2.CiliumNetworkPolicy{},
 			},
 		},
+		{
+			name: "A CCNP where it doesn't contain neither a spec nor specs",
+			args: args{
+				obj: &v2.CiliumClusterwideNetworkPolicy{},
+			},
+			want: &types.SlimCNP{
+				CiliumNetworkPolicy: &v2.CiliumNetworkPolicy{},
+			},
+		},
 		{
 			name: "delete final state unknown conversion",
 			args: args{
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(),