From 08858c2a106e2ba6d52d77a940a8665c12ced047 Mon Sep 17 00:00:00 2001
From: Martynas Pumputis <martynasp@gmail.com>
Date: Sun, 4 Feb 2018 12:33:18 +0100
Subject: [PATCH] npc: Fix bug when dstSelector is overwritten by srcSelector

There was a bug in the npc analyser when adding a netpol with
dstSelector == srcSelector. The analyser was overwriting
dstSelector with srcSelector which resulted in Pod selected
by dstSelector not being removed from a corresponding
"default-allow" ipset.

This applies only to non-legacy netpols.
---
 npc/analyser.go        | 15 +++++++--
 npc/controller_test.go | 73 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 86 insertions(+), 2 deletions(-)

diff --git a/npc/analyser.go b/npc/analyser.go
index 2ad440fc..e73d37b8 100644
--- a/npc/analyser.go
+++ b/npc/analyser.go
@@ -108,7 +108,12 @@ func (ns *ns) analysePolicy(policy *networkingv1.NetworkPolicy) (
 	if err != nil {
 		return nil, nil, nil, err
 	}
-	podSelectors[dstSelector.key] = dstSelector
+
+	// To prevent dstSelector being overwritten by a subsequent selector with
+	// the same key, addIfNotExist MUST be used when adding to podSelectors.
+	// Otherwise, dstSelector' "dst" property might be lost resulting in
+	// an invalid content of the "default-allow" ipset.
+	addIfNotExist(dstSelector, podSelectors)
 
 	// If ingress is empty then this NetworkPolicy does not allow any traffic
 	if policy.Spec.Ingress == nil || len(policy.Spec.Ingress) == 0 {
@@ -142,7 +147,7 @@ func (ns *ns) analysePolicy(policy *networkingv1.NetworkPolicy) (
 					if err != nil {
 						return nil, nil, nil, err
 					}
-					podSelectors[srcSelector.key] = srcSelector
+					addIfNotExist(srcSelector, podSelectors)
 
 				} else if peer.NamespaceSelector != nil {
 					srcSelector, err = newSelectorSpec(peer.NamespaceSelector, false, "", ipset.ListSet)
@@ -168,6 +173,12 @@ func (ns *ns) analysePolicy(policy *networkingv1.NetworkPolicy) (
 	return rules, nsSelectors, podSelectors, nil
 }
 
+func addIfNotExist(s *selectorSpec, ss map[string]*selectorSpec) {
+	if _, ok := ss[s.key]; !ok {
+		ss[s.key] = s
+	}
+}
+
 func withNormalisedProtoAndPortLegacy(npps []extnapi.NetworkPolicyPort, f func(proto, port string)) {
 	for _, npp := range npps {
 		f(proto(npp.Protocol), port(npp.Port))
diff --git a/npc/controller_test.go b/npc/controller_test.go
index 4219bae2..c8a2c393 100644
--- a/npc/controller_test.go
+++ b/npc/controller_test.go
@@ -376,3 +376,76 @@ func TestOutOfOrderPodEvents(t *testing.T) {
 	require.Equal(t, 0, len(m.sets[defaultAllowIPSetName].subSets))
 	require.False(t, m.entryExists(runBarIPSetName, podIP))
 }
+
+// Test case for https://github.com/weaveworks/weave/issues/3222
+func TestNewDstSelector(t *testing.T) {
+	const (
+		defaultAllowIPSetName = "weave-E.1.0W^NGSp]0_t5WwH/]gX@L"
+		podIP                 = "10.32.0.10"
+	)
+
+	m := newMockIPSet()
+	controller := New("baz", false, &mockIPTables{}, &m)
+
+	defaultNamespace := &coreapi.Namespace{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "default",
+		},
+	}
+	controller.AddNamespace(defaultNamespace)
+
+	podFoo := &coreapi.Pod{
+		ObjectMeta: metav1.ObjectMeta{
+			UID:       "foo",
+			Namespace: "default",
+			Name:      "foo",
+			Labels:    map[string]string{"run": "foo"}},
+		Status: coreapi.PodStatus{PodIP: podIP}}
+	controller.AddPod(podFoo)
+
+	require.True(t, m.entryExists(defaultAllowIPSetName, podIP))
+
+	netpolBar := &networkingv1.NetworkPolicy{
+		ObjectMeta: metav1.ObjectMeta{
+			UID:       "netpol-bar",
+			Name:      "allow-from-default-1",
+			Namespace: "default",
+		},
+		Spec: networkingv1.NetworkPolicySpec{
+			PodSelector: metav1.LabelSelector{},
+			Ingress: []networkingv1.NetworkPolicyIngressRule{{
+				From: []networkingv1.NetworkPolicyPeer{{
+					PodSelector: &metav1.LabelSelector{},
+				}},
+			}},
+		},
+	}
+	controller.AddNetworkPolicy(netpolBar)
+
+	// netpolBar dst selector selects podFoo
+	require.False(t, m.entryExists(defaultAllowIPSetName, podIP))
+
+	netpolFoo := &networkingv1.NetworkPolicy{
+		ObjectMeta: metav1.ObjectMeta{
+			UID:       "netpol-foo",
+			Name:      "allow-from-default-2",
+			Namespace: "default",
+		},
+		Spec: networkingv1.NetworkPolicySpec{
+			PodSelector: metav1.LabelSelector{},
+			Ingress: []networkingv1.NetworkPolicyIngressRule{{
+				From: []networkingv1.NetworkPolicyPeer{{
+					PodSelector: &metav1.LabelSelector{},
+				}},
+			}},
+		},
+	}
+	controller.AddNetworkPolicy(netpolFoo)
+
+	controller.DeleteNetworkPolicy(netpolBar)
+	// netpolFoo dst-selects podFoo
+	require.False(t, m.entryExists(defaultAllowIPSetName, podIP))
+	controller.DeleteNetworkPolicy(netpolFoo)
+	// No netpol dst-selects podFoo
+	require.True(t, m.entryExists(defaultAllowIPSetName, podIP))
+}