From c33cd2657c579d4bff8e3b48da97b7926a04b5f3 Mon Sep 17 00:00:00 2001 From: Mike Bryant Date: Tue, 11 Jul 2017 17:43:24 +0100 Subject: [PATCH 1/3] Refactor iptables usage in weave-npc to make it testable --- npc/controller.go | 6 +++--- npc/iptables/iptables.go | 7 +++++++ npc/namespace.go | 6 +++--- npc/rule.go | 6 +++--- 4 files changed, 16 insertions(+), 9 deletions(-) create mode 100644 npc/iptables/iptables.go diff --git a/npc/controller.go b/npc/controller.go index c2a10510..a0ad6c78 100644 --- a/npc/controller.go +++ b/npc/controller.go @@ -3,13 +3,13 @@ package npc import ( "sync" - "github.com/coreos/go-iptables/iptables" "github.com/pkg/errors" coreapi "k8s.io/client-go/pkg/api/v1" extnapi "k8s.io/client-go/pkg/apis/extensions/v1beta1" "github.com/weaveworks/weave/common" "github.com/weaveworks/weave/npc/ipset" + "github.com/weaveworks/weave/npc/iptables" ) type NetworkPolicyController interface { @@ -31,14 +31,14 @@ type controller struct { nodeName string // my node name - ipt *iptables.IPTables + ipt iptables.Interface ips ipset.Interface nss map[string]*ns // ns name -> ns struct nsSelectors *selectorSet // selector string -> nsSelector } -func New(nodeName string, ipt *iptables.IPTables, ips ipset.Interface) NetworkPolicyController { +func New(nodeName string, ipt iptables.Interface, ips ipset.Interface) NetworkPolicyController { c := &controller{ nodeName: nodeName, ipt: ipt, diff --git a/npc/iptables/iptables.go b/npc/iptables/iptables.go new file mode 100644 index 00000000..771a5f6c --- /dev/null +++ b/npc/iptables/iptables.go @@ -0,0 +1,7 @@ +package iptables + +type Interface interface { + Append(table, chain string, rulespec ...string) error + Delete(table, chain string, rulespec ...string) error + Insert(table, chain string, pos int, rulespec ...string) error +} diff --git a/npc/namespace.go b/npc/namespace.go index e340edc9..3749ed36 100644 --- a/npc/namespace.go +++ b/npc/namespace.go @@ -3,7 +3,6 @@ package npc import ( "encoding/json" - "github.com/coreos/go-iptables/iptables" "k8s.io/client-go/pkg/api/unversioned" coreapi "k8s.io/client-go/pkg/api/v1" extnapi "k8s.io/client-go/pkg/apis/extensions/v1beta1" @@ -12,10 +11,11 @@ import ( "github.com/weaveworks/weave/common" "github.com/weaveworks/weave/npc/ipset" + "github.com/weaveworks/weave/npc/iptables" ) type ns struct { - ipt *iptables.IPTables // interface to iptables + ipt iptables.Interface // interface to iptables ips ipset.Interface // interface to ipset name string // k8s Namespace name @@ -32,7 +32,7 @@ type ns struct { rules *ruleSet } -func newNS(name, nodeName string, ipt *iptables.IPTables, ips ipset.Interface, nsSelectors *selectorSet) (*ns, error) { +func newNS(name, nodeName string, ipt iptables.Interface, ips ipset.Interface, nsSelectors *selectorSet) (*ns, error) { allPods, err := newSelectorSpec(&unversioned.LabelSelector{}, name, ipset.HashIP) if err != nil { return nil, err diff --git a/npc/rule.go b/npc/rule.go index 709e74d3..07854424 100644 --- a/npc/rule.go +++ b/npc/rule.go @@ -3,10 +3,10 @@ package npc import ( "strings" - "github.com/coreos/go-iptables/iptables" "k8s.io/client-go/pkg/types" "github.com/weaveworks/weave/common" + "github.com/weaveworks/weave/npc/iptables" ) type ruleSpec struct { @@ -35,11 +35,11 @@ func newRuleSpec(proto *string, srcHost *selectorSpec, dstHost *selectorSpec, ds } type ruleSet struct { - ipt *iptables.IPTables + ipt iptables.Interface users map[string]map[types.UID]struct{} } -func newRuleSet(ipt *iptables.IPTables) *ruleSet { +func newRuleSet(ipt iptables.Interface) *ruleSet { return &ruleSet{ipt, make(map[string]map[types.UID]struct{})} } From 219b03cb6bc8373b7ee54435c696412818fa3e32 Mon Sep 17 00:00:00 2001 From: Mike Bryant Date: Tue, 11 Jul 2017 17:44:40 +0100 Subject: [PATCH 2/3] Add regression test for weave-npc race condition --- npc/controller_test.go | 182 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 182 insertions(+) create mode 100644 npc/controller_test.go diff --git a/npc/controller_test.go b/npc/controller_test.go new file mode 100644 index 00000000..2c179dde --- /dev/null +++ b/npc/controller_test.go @@ -0,0 +1,182 @@ +package npc + +import ( + "log" + "testing" + + "github.com/pkg/errors" + "github.com/stretchr/testify/require" + "github.com/weaveworks/weave/npc/ipset" + unversionedapi "k8s.io/client-go/pkg/api/unversioned" + coreapi "k8s.io/client-go/pkg/api/v1" + extnapi "k8s.io/client-go/pkg/apis/extensions/v1beta1" + "k8s.io/client-go/pkg/util/intstr" +) + +type mockSet struct { + name ipset.Name + setType ipset.Type + subSets map[string]bool +} + +type mockIPSet struct { + sets map[string]mockSet +} + +func newMockIPSet() mockIPSet { + i := mockIPSet{ + sets: make(map[string]mockSet), + } + + return i +} + +func (i *mockIPSet) Create(ipsetName ipset.Name, ipsetType ipset.Type) error { + if _, ok := i.sets[string(ipsetName)]; ok { + return errors.Errorf("ipset %s already exists", ipsetName) + } + i.sets[string(ipsetName)] = mockSet{name: ipsetName, setType: ipsetType, subSets: make(map[string]bool)} + return nil +} + +func (i *mockIPSet) AddEntry(ipsetName ipset.Name, entry string) error { + log.Printf("adding entry %s to %s", entry, ipsetName) + if _, ok := i.sets[entry]; !ok { + return errors.Errorf("ipset %s does not exist", entry) + } + if _, ok := i.sets[string(ipsetName)].subSets[entry]; ok { + return errors.Errorf("ipset %s is already a member of %s", entry, ipsetName) + } + i.sets[string(ipsetName)].subSets[entry] = true + + return nil +} + +func (i *mockIPSet) DelEntry(ipsetName ipset.Name, entry string) error { + log.Printf("deleting entry %s from %s", entry, ipsetName) + if _, ok := i.sets[string(ipsetName)]; !ok { + return errors.Errorf("ipset %s does not exist", ipsetName) + } + if _, ok := i.sets[string(ipsetName)].subSets[entry]; !ok { + return errors.Errorf("ipset %s is not a member of %s", entry, ipsetName) + } + delete(i.sets[string(ipsetName)].subSets, entry) + + return nil +} + +func (i *mockIPSet) Flush(ipsetName ipset.Name) error { + return errors.New("Not Implemented") +} + +func (i *mockIPSet) FlushAll() error { + return errors.New("Not Implemented") +} + +func (i *mockIPSet) Destroy(ipsetName ipset.Name) error { + if _, ok := i.sets[string(ipsetName)]; !ok { + return errors.Errorf("ipset %s does not exist", ipsetName) + } + delete(i.sets, string(ipsetName)) + return nil +} + +func (i *mockIPSet) DestroyAll() error { + return errors.New("Not Implemented") +} + +func (i *mockIPSet) List(prefix string) ([]ipset.Name, error) { + return []ipset.Name{}, errors.New("Not Implemented") +} + +type mockIPTables struct { +} + +func (ipt *mockIPTables) Append(table, chain string, rulespec ...string) error { + return nil +} + +func (ipt *mockIPTables) Delete(table, chain string, rulespec ...string) error { + return nil +} + +func (ipt *mockIPTables) Insert(table, chain string, pos int, rulespec ...string) error { + return nil +} + +func TestRegressionPolicyNamespaceOrdering3059(t *testing.T) { + // Test for race condition between namespace and networkpolicy events + // https://github.com/weaveworks/weave/issues/3059 + + sourceNamespace := &coreapi.Namespace{ + ObjectMeta: coreapi.ObjectMeta{ + Name: "source", + Labels: map[string]string{ + "app": "source", + }, + }, + } + + destinationNamespace := &coreapi.Namespace{ + ObjectMeta: coreapi.ObjectMeta{ + Name: "destination", + }, + } + + port := intstr.FromInt(12345) + + networkPolicy := &extnapi.NetworkPolicy{ + ObjectMeta: coreapi.ObjectMeta{ + Name: "network-policy", + Namespace: "destination", + }, + Spec: extnapi.NetworkPolicySpec{ + Ingress: []extnapi.NetworkPolicyIngressRule{ + { + From: []extnapi.NetworkPolicyPeer{ + { + NamespaceSelector: &unversionedapi.LabelSelector{ + MatchLabels: map[string]string{ + "app": "source", + }, + }, + }, + }, + Ports: []extnapi.NetworkPolicyPort{ + { + Port: &port, + }, + }, + }, + }, + }, + } + + // Namespaces first + m := newMockIPSet() + controller := New("foo", &mockIPTables{}, &m) + + const ( + selectorIPSetName = "weave-I239Zp%sCvoVt*D6u=A!2]YEk" + sourceIPSetName = "weave-HboJG1fGgG]/SR%k9H#hv5e96" + ) + + controller.AddNamespace(sourceNamespace) + controller.AddNamespace(destinationNamespace) + + controller.AddNetworkPolicy(networkPolicy) + + require.Equal(t, true, m.sets[selectorIPSetName].subSets[sourceIPSetName]) + + // NetworkPolicy first + m = newMockIPSet() + controller = New("foo", &mockIPTables{}, &m) + + controller.AddNetworkPolicy(networkPolicy) + + controller.AddNamespace(sourceNamespace) + controller.AddNamespace(destinationNamespace) + + require.Equal(t, true, m.sets[selectorIPSetName].subSets[sourceIPSetName]) + +} From cbc3d5b04c2434f577ee74c537642ab9bfde010c Mon Sep 17 00:00:00 2001 From: Mike Bryant Date: Wed, 12 Jul 2017 11:16:42 +0100 Subject: [PATCH 3/3] Don't bypass selector provisioning when not default deny If we have namespaces Source and Destination, and Destination contains a NetworkPolicy allowing access from Source, we need to provision the ipsets for Source and add them to relevant selectors, even though Source is DefaultAllow. This also fixes an issue where if the Namespace labels are updated, that would be ignored. Fixes #3059 --- npc/namespace.go | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/npc/namespace.go b/npc/namespace.go index 3749ed36..e0cce644 100644 --- a/npc/namespace.go +++ b/npc/namespace.go @@ -265,7 +265,9 @@ func (ns *ns) addNamespace(obj *coreapi.Namespace) error { // Insert a rule to bypass policies if namespace is DefaultAllow if !isDefaultDeny(obj) { - return ns.ensureBypassRule(ns.allPods.ipsetName) + if err := ns.ensureBypassRule(ns.allPods.ipsetName); err != nil { + return err + } } // Add namespace ipset to matching namespace selectors @@ -282,10 +284,14 @@ func (ns *ns) updateNamespace(oldObj, newObj *coreapi.Namespace) error { if oldDefaultDeny != newDefaultDeny { common.Log.Infof("namespace DefaultDeny changed from %t to %t", oldDefaultDeny, newDefaultDeny) if oldDefaultDeny { - return ns.ensureBypassRule(ns.allPods.ipsetName) + if err := ns.ensureBypassRule(ns.allPods.ipsetName); err != nil { + return err + } } if newDefaultDeny { - return ns.deleteBypassRule(ns.allPods.ipsetName) + if err := ns.deleteBypassRule(ns.allPods.ipsetName); err != nil { + return err + } } } @@ -318,7 +324,9 @@ func (ns *ns) deleteNamespace(obj *coreapi.Namespace) error { // Remove bypass rule if !isDefaultDeny(obj) { - return ns.deleteBypassRule(ns.allPods.ipsetName) + if err := ns.deleteBypassRule(ns.allPods.ipsetName); err != nil { + return err + } } // Remove namespace ipset from any matching namespace selectors