From 89e820d62e6dd67f6eb5f457d01b38c0e496bfd8 Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Tue, 15 May 2018 21:35:44 +0200 Subject: [PATCH 01/46] npc: Create WEAVE-NPC-EGRESS chain with the base rules The following rules are provisioned by weave-npc: -A WEAVE-NPC-EGRESS -m state --state RELATED,ESTABLISHED -j RETURN -A WEAVE-NPC-EGRESS -m set ! --match-set weave-local-pods src -j RETURN -A WEAVE-NPC-EGRESS -d 224.0.0.0/4 -j RETURN -A WEAVE-NPC-EGRESS -m state --state NEW -j WEAVE-NPC-EGRESS-DEFAULT -A WEAVE-NPC-EGRESS -m state --state NEW -m mark ! --mark 0x40000/0x40000 -j WEAVE-NPC-EGRESS-CUSTOM -A WEAVE-NPC-EGRESS -m state --state NEW -m mark ! --mark 0x40000/0x40000 -j NFLOG --nflog-group 86 -A WEAVE-NPC-EGRESS -m mark ! --mark 0x40000/0x40000 -j DROP -A WEAVE-NPC-EGRESS-CUSTOM -j MARK --set-xmark 0x40000/0x40000 -A WEAVE-NPC-EGRESS-CUSTOM -j RETURN -A WEAVE-NPC-EGRESS-DEFAULT -j MARK --set-xmark 0x40000/0x40000 -A WEAVE-NPC-EGRESS-DEFAULT -j RETURN For each rule we create two (mark and return). We cannot just accept a packet if it matches any rule, as a packet might need to traverse the ingress npc as well which happens later in the chain (in some cases we cannot detect whether packet is ingress or egress, so we need to check both chains). --- npc/constants.go | 5 ++++ prog/weave-npc/main.go | 65 ++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 67 insertions(+), 3 deletions(-) diff --git a/npc/constants.go b/npc/constants.go index 2722c57f..0e41dfe8 100644 --- a/npc/constants.go +++ b/npc/constants.go @@ -7,6 +7,11 @@ const ( DefaultChain = "WEAVE-NPC-DEFAULT" IngressChain = "WEAVE-NPC-INGRESS" + EgressChain = "WEAVE-NPC-EGRESS" + EgressDefaultChain = "WEAVE-NPC-EGRESS-DEFAULT" + EgressCustomChain = "WEAVE-NPC-EGRESS-CUSTOM" + EgressMark = "0x40000/0x40000" + IpsetNamePrefix = "weave-" LocalIpset = IpsetNamePrefix + "local-pods" diff --git a/prog/weave-npc/main.go b/prog/weave-npc/main.go index d9922de9..4edfe9f4 100644 --- a/prog/weave-npc/main.go +++ b/prog/weave-npc/main.go @@ -53,7 +53,23 @@ func resetIPTables(ipt *iptables.IPTables) error { return err } - return ipt.ClearChain(npc.TableFilter, npc.MainChain) + if err := ipt.ClearChain(npc.TableFilter, npc.MainChain); err != nil { + return err + } + + if err := ipt.ClearChain(npc.TableFilter, npc.EgressCustomChain); err != nil { + return err + } + + if err := ipt.ClearChain(npc.TableFilter, npc.EgressDefaultChain); err != nil { + return err + } + + if err := ipt.ClearChain(npc.TableFilter, npc.EgressChain); err != nil { + return err + } + + return nil } func resetIPSets(ips ipset.Interface) error { @@ -115,8 +131,51 @@ func createBaseRules(ipt *iptables.IPTables, ips ipset.Interface) error { if err := ips.Create(npc.LocalIpset, ipset.HashIP); err != nil { return err } - return ipt.Append(npc.TableFilter, npc.MainChain, - "-m", "set", "!", "--match-set", npc.LocalIpset, "dst", "-j", "ACCEPT") + if err := ipt.Append(npc.TableFilter, npc.MainChain, + "-m", "set", "!", "--match-set", npc.LocalIpset, "dst", "-j", "ACCEPT"); err != nil { + return err + } + + if err := ipt.Append(npc.TableFilter, npc.EgressChain, + "-m", "state", "--state", "RELATED,ESTABLISHED", "-j", "RETURN"); err != nil { + return err + } + if allowMcast { + if err := ipt.Append(npc.TableFilter, npc.EgressChain, + "-d", "224.0.0.0/4", "-j", "RETURN"); err != nil { + return err + } + } + if err := ipt.Append(npc.TableFilter, npc.EgressChain, + "-m", "state", "--state", "NEW", + "-j", string(npc.EgressDefaultChain)); err != nil { + return err + } + if err := ipt.Append(npc.TableFilter, npc.EgressChain, + "-m", "state", "--state", "NEW", + "-m", "set", "!", "--match-set", npc.LocalIpset, "src", + "-j", "RETURN"); err != nil { + return err + } + if err := ipt.Append(npc.TableFilter, npc.EgressChain, + "-m", "state", "--state", "NEW", + "-m", "mark", "!", "--mark", npc.EgressMark, + "-j", string(npc.EgressCustomChain)); err != nil { + return err + } + if err := ipt.Append(npc.TableFilter, npc.EgressChain, + "-m", "state", "--state", "NEW", + "-m", "mark", "!", "--mark", npc.EgressMark, + "-j", "NFLOG", "--nflog-group", "86"); err != nil { + return err + } + if err := ipt.Append(npc.TableFilter, npc.EgressChain, + "-m", "mark", "!", "--mark", npc.EgressMark, + "-j", "DROP"); err != nil { + return err + } + + return nil } func root(cmd *cobra.Command, args []string) { From cabc05b06c8b89b9b7406d53936b74780fa0111c Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Tue, 15 May 2018 21:45:09 +0200 Subject: [PATCH 02/46] npc: Add rule type (ingress, egress) --- npc/analyser.go | 16 ++++++++-------- npc/rule.go | 32 ++++++++++++++++++++++++-------- npc/rule_test.go | 2 +- 3 files changed, 33 insertions(+), 17 deletions(-) diff --git a/npc/analyser.go b/npc/analyser.go index 01cdf5ee..50246a38 100644 --- a/npc/analyser.go +++ b/npc/analyser.go @@ -45,13 +45,13 @@ func (ns *ns) analysePolicyLegacy(policy *extnapi.NetworkPolicy) ( // From is not provided, this rule matches all sources (traffic not restricted by source). if ingressRule.Ports == nil { // Ports is not provided, this rule matches all ports (traffic not restricted by port). - rule := newRuleSpec(nil, nil, dstSelector, nil) + rule := newRuleSpec(ingress, nil, nil, dstSelector, nil) rules[rule.key] = rule } else { // Ports is present and contains at least one item, then this rule allows traffic // only if the traffic matches at least one port in the ports list. withNormalisedProtoAndPortLegacy(ingressRule.Ports, func(proto, port string) { - rule := newRuleSpec(&proto, nil, dstSelector, &port) + rule := newRuleSpec(ingress, &proto, nil, dstSelector, &port) rules[rule.key] = rule }) } @@ -77,13 +77,13 @@ func (ns *ns) analysePolicyLegacy(policy *extnapi.NetworkPolicy) ( if ingressRule.Ports == nil { // Ports is not provided, this rule matches all ports (traffic not restricted by port). - rule := newRuleSpec(nil, srcSelector, dstSelector, nil) + rule := newRuleSpec(ingress, nil, srcSelector, dstSelector, nil) rules[rule.key] = rule } else { // Ports is present and contains at least one item, then this rule allows traffic // only if the traffic matches at least one port in the ports list. withNormalisedProtoAndPortLegacy(ingressRule.Ports, func(proto, port string) { - rule := newRuleSpec(&proto, srcSelector, dstSelector, &port) + rule := newRuleSpec(ingress, &proto, srcSelector, dstSelector, &port) rules[rule.key] = rule }) } @@ -128,11 +128,11 @@ func (ns *ns) analysePolicy(policy *networkingv1.NetworkPolicy) ( if allSources { if allPorts { - rule := newRuleSpec(nil, nil, dstSelector, nil) + rule := newRuleSpec(ingress, nil, nil, dstSelector, nil) rules[rule.key] = rule } else { withNormalisedProtoAndPort(ingressRule.Ports, func(proto, port string) { - rule := newRuleSpec(&proto, nil, dstSelector, &port) + rule := newRuleSpec(ingress, &proto, nil, dstSelector, &port) rules[rule.key] = rule }) } @@ -158,11 +158,11 @@ func (ns *ns) analysePolicy(policy *networkingv1.NetworkPolicy) ( } if allPorts { - rule := newRuleSpec(nil, srcSelector, dstSelector, nil) + rule := newRuleSpec(ingress, nil, srcSelector, dstSelector, nil) rules[rule.key] = rule } else { withNormalisedProtoAndPort(ingressRule.Ports, func(proto, port string) { - rule := newRuleSpec(&proto, srcSelector, dstSelector, &port) + rule := newRuleSpec(ingress, &proto, srcSelector, dstSelector, &port) rules[rule.key] = rule }) } diff --git a/npc/rule.go b/npc/rule.go index c0a334c0..f84884da 100644 --- a/npc/rule.go +++ b/npc/rule.go @@ -10,12 +10,20 @@ import ( "github.com/weaveworks/weave/npc/iptables" ) +type ruleType byte + +const ( + ingress ruleType = iota + egress +) + type ruleSpec struct { - key string - args []string + key string + args []string + ruleType ruleType } -func newRuleSpec(proto *string, srcHost *selectorSpec, dstHost *selectorSpec, dstPort *string) *ruleSpec { +func newRuleSpec(ruleType ruleType, proto *string, srcHost *selectorSpec, dstHost *selectorSpec, dstPort *string) *ruleSpec { args := []string{} if proto != nil { args = append(args, "-p", *proto) @@ -41,7 +49,7 @@ func newRuleSpec(proto *string, srcHost *selectorSpec, dstHost *selectorSpec, ds args = append(args, "-m", "comment", "--comment", fmt.Sprintf("%s -> %s", srcComment, dstComment)) key := strings.Join(args, " ") - return &ruleSpec{key, args} + return &ruleSpec{key, args, ruleType} } type ruleSet struct { @@ -58,8 +66,12 @@ func (rs *ruleSet) deprovision(user types.UID, current, desired map[string]*rule if _, found := desired[key]; !found { delete(rs.users[key], user) if len(rs.users[key]) == 0 { - common.Log.Infof("deleting rule: %v", spec.args) - if err := rs.ipt.Delete(TableFilter, IngressChain, spec.args...); err != nil { + chain := IngressChain + if spec.ruleType == egress { + chain = EgressCustomChain + } + common.Log.Infof("deleting rule %v from %q chain", spec.args, chain) + if err := rs.ipt.Delete(TableFilter, chain, spec.args...); err != nil { return err } delete(rs.users, key) @@ -74,8 +86,12 @@ func (rs *ruleSet) provision(user types.UID, current, desired map[string]*ruleSp for key, spec := range desired { if _, found := current[key]; !found { if _, found := rs.users[key]; !found { - common.Log.Infof("adding rule: %v", spec.args) - if err := rs.ipt.Append(TableFilter, IngressChain, spec.args...); err != nil { + chain := IngressChain + if spec.ruleType == egress { + chain = EgressCustomChain + } + common.Log.Infof("adding rule %v to %q chain", spec.args, chain) + if err := rs.ipt.Append(TableFilter, chain, spec.args...); err != nil { return err } rs.users[key] = make(map[types.UID]struct{}) diff --git a/npc/rule_test.go b/npc/rule_test.go index dee1802f..ab5850de 100644 --- a/npc/rule_test.go +++ b/npc/rule_test.go @@ -9,7 +9,7 @@ func TestRegressionHandleNil3095(t *testing.T) { // Test for handling nil values // https://github.com/weaveworks/weave/issues/3095 - rs := newRuleSpec(nil, nil, nil, nil) + rs := newRuleSpec(ingress, nil, nil, nil, nil) require.NotEqual(t, rs.key, "") From 29ec43bdc818fee724f2e4f0ffe9bfe50158894e Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Thu, 17 May 2018 18:21:39 +0200 Subject: [PATCH 03/46] npc: Rename default-allow ipset for ingress As we're going to introduce one for egress, we cannot use "default-allow" name for ingress. --- npc/controller_test.go | 64 +++++++++++++++++++++--------------------- npc/namespace.go | 34 +++++++++++----------- 2 files changed, 49 insertions(+), 49 deletions(-) diff --git a/npc/controller_test.go b/npc/controller_test.go index f7dd7fdb..6928fc52 100644 --- a/npc/controller_test.go +++ b/npc/controller_test.go @@ -196,10 +196,10 @@ func TestRegressionPolicyNamespaceOrdering3059(t *testing.T) { // Tests default-allow ipset behavior when running in non-legacy mode. func TestDefaultAllow(t *testing.T) { const ( - defaultAllowIPSetName = "weave-E.1.0W^NGSp]0_t5WwH/]gX@L" - fooPodIP = "10.32.0.10" - barPodIP = "10.32.0.11" - barPodNewIP = "10.32.0.12" + ingressDefaultAllowIPSetName = "weave-;rGqyMIl1HN^cfDki~Z$3]6!N" + fooPodIP = "10.32.0.10" + barPodIP = "10.32.0.11" + barPodNewIP = "10.32.0.12" ) m := newMockIPSet() @@ -213,7 +213,7 @@ func TestDefaultAllow(t *testing.T) { controller.AddNamespace(defaultNamespace) // Should create an ipset for default-allow - require.Contains(t, m.sets, defaultAllowIPSetName) + require.Contains(t, m.sets, ingressDefaultAllowIPSetName) podFoo := &coreapi.Pod{ ObjectMeta: metav1.ObjectMeta{ @@ -225,7 +225,7 @@ func TestDefaultAllow(t *testing.T) { controller.AddPod(podFoo) // Should add the foo pod to default-allow - require.True(t, m.entryExists(defaultAllowIPSetName, fooPodIP)) + require.True(t, m.entryExists(ingressDefaultAllowIPSetName, fooPodIP)) podBar := &coreapi.Pod{ ObjectMeta: metav1.ObjectMeta{ @@ -239,7 +239,7 @@ func TestDefaultAllow(t *testing.T) { controller.UpdatePod(podBarNoIP, podBar) // Should add the bar pod to default-allow - require.True(t, m.entryExists(defaultAllowIPSetName, barPodIP)) + require.True(t, m.entryExists(ingressDefaultAllowIPSetName, barPodIP)) // Allow access from the bar pod to the foo pod netpol := &networkingv1.NetworkPolicy{ @@ -261,51 +261,51 @@ func TestDefaultAllow(t *testing.T) { controller.AddNetworkPolicy(netpol) // Should remove the foo pod from default-allow as the netpol selects it - require.False(t, m.entryExists(defaultAllowIPSetName, fooPodIP)) - require.True(t, m.entryExists(defaultAllowIPSetName, barPodIP)) + require.False(t, m.entryExists(ingressDefaultAllowIPSetName, fooPodIP)) + require.True(t, m.entryExists(ingressDefaultAllowIPSetName, barPodIP)) podBarWithNewIP := *podBar podBarWithNewIP.Status.PodIP = barPodNewIP controller.UpdatePod(podBar, &podBarWithNewIP) // Should update IP addr of the bar pod in default-allow - require.False(t, m.entryExists(defaultAllowIPSetName, barPodIP)) - require.True(t, m.entryExists(defaultAllowIPSetName, barPodNewIP)) + require.False(t, m.entryExists(ingressDefaultAllowIPSetName, barPodIP)) + require.True(t, m.entryExists(ingressDefaultAllowIPSetName, barPodNewIP)) controller.UpdatePod(&podBarWithNewIP, podBarNoIP) // Should remove the bar pod from default-allow as it does not have any IP addr - require.False(t, m.entryExists(defaultAllowIPSetName, barPodNewIP)) + require.False(t, m.entryExists(ingressDefaultAllowIPSetName, barPodNewIP)) podFooWithNewLabel := *podFoo podFooWithNewLabel.ObjectMeta.Labels = map[string]string{"run": "new-foo"} controller.UpdatePod(podFoo, &podFooWithNewLabel) // Should bring back the foo pod to default-allow as it does not match dst of any netpol - require.True(t, m.entryExists(defaultAllowIPSetName, fooPodIP)) + require.True(t, m.entryExists(ingressDefaultAllowIPSetName, fooPodIP)) controller.UpdatePod(&podFooWithNewLabel, podFoo) // Should remove from default-allow as it matches the netpol after the update - require.False(t, m.entryExists(defaultAllowIPSetName, fooPodIP)) + require.False(t, m.entryExists(ingressDefaultAllowIPSetName, fooPodIP)) controller.DeleteNetworkPolicy(netpol) // Should bring back the foo pod to default-allow as no netpol selects it - require.True(t, m.entryExists(defaultAllowIPSetName, fooPodIP)) + require.True(t, m.entryExists(ingressDefaultAllowIPSetName, fooPodIP)) controller.DeletePod(podFoo) // Should remove foo pod from default-allow - require.False(t, m.entryExists(defaultAllowIPSetName, fooPodIP)) + require.False(t, m.entryExists(ingressDefaultAllowIPSetName, fooPodIP)) controller.DeleteNamespace(defaultNamespace) // Should remove default ipset - require.NotContains(t, m.sets, defaultAllowIPSetName) + require.NotContains(t, m.sets, ingressDefaultAllowIPSetName) } func TestOutOfOrderPodEvents(t *testing.T) { const ( - defaultAllowIPSetName = "weave-E.1.0W^NGSp]0_t5WwH/]gX@L" - runBarIPSetName = "weave-bZ~x=yBgzH)Ht()K*Uv3z{M]Y" - podIP = "10.32.0.10" + ingressDefaultAllowIPSetName = "weave-;rGqyMIl1HN^cfDki~Z$3]6!N" + runBarIPSetName = "weave-bZ~x=yBgzH)Ht()K*Uv3z{M]Y" + podIP = "10.32.0.10" ) m := newMockIPSet() @@ -328,7 +328,7 @@ func TestOutOfOrderPodEvents(t *testing.T) { controller.AddPod(podFoo) // Should be in default-allow as no netpol selects podFoo - require.True(t, m.entryExists(defaultAllowIPSetName, podIP)) + require.True(t, m.entryExists(ingressDefaultAllowIPSetName, podIP)) netpol := &networkingv1.NetworkPolicy{ ObjectMeta: metav1.ObjectMeta{ @@ -349,7 +349,7 @@ func TestOutOfOrderPodEvents(t *testing.T) { controller.AddNetworkPolicy(netpol) // Shouldn't be in default-allow as netpol above selects podFoo - require.False(t, m.entryExists(defaultAllowIPSetName, podIP)) + require.False(t, m.entryExists(ingressDefaultAllowIPSetName, podIP)) podBar := &coreapi.Pod{ ObjectMeta: metav1.ObjectMeta{ @@ -361,8 +361,8 @@ func TestOutOfOrderPodEvents(t *testing.T) { controller.AddPod(podBar) // Should be in default-allow as no netpol selects podBar - require.True(t, m.entryExists(defaultAllowIPSetName, podIP)) - require.True(t, m.Exist(podBar.ObjectMeta.UID, defaultAllowIPSetName, podIP)) + require.True(t, m.entryExists(ingressDefaultAllowIPSetName, podIP)) + require.True(t, m.Exist(podBar.ObjectMeta.UID, ingressDefaultAllowIPSetName, podIP)) // Should be in run=bar ipset require.True(t, m.entryExists(runBarIPSetName, podIP)) @@ -373,20 +373,20 @@ func TestOutOfOrderPodEvents(t *testing.T) { // Should be in default-allow as no netpol selects podBar and podFoo removal // should not affect podBar in default-allow - require.True(t, m.entryExists(defaultAllowIPSetName, podIP)) + require.True(t, m.entryExists(ingressDefaultAllowIPSetName, podIP)) controller.DeletePod(podBar) // Should remove from default-allow and run=bar ipsets - require.Equal(t, 0, len(m.sets[defaultAllowIPSetName].subSets)) + require.Equal(t, 0, len(m.sets[ingressDefaultAllowIPSetName].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" + ingressDefaultAllowIPSetName = "weave-;rGqyMIl1HN^cfDki~Z$3]6!N" + podIP = "10.32.0.10" ) m := newMockIPSet() @@ -408,7 +408,7 @@ func TestNewDstSelector(t *testing.T) { Status: coreapi.PodStatus{PodIP: podIP}} controller.AddPod(podFoo) - require.True(t, m.entryExists(defaultAllowIPSetName, podIP)) + require.True(t, m.entryExists(ingressDefaultAllowIPSetName, podIP)) netpolBar := &networkingv1.NetworkPolicy{ ObjectMeta: metav1.ObjectMeta{ @@ -428,7 +428,7 @@ func TestNewDstSelector(t *testing.T) { controller.AddNetworkPolicy(netpolBar) // netpolBar dst selector selects podFoo - require.False(t, m.entryExists(defaultAllowIPSetName, podIP)) + require.False(t, m.entryExists(ingressDefaultAllowIPSetName, podIP)) netpolFoo := &networkingv1.NetworkPolicy{ ObjectMeta: metav1.ObjectMeta{ @@ -449,8 +449,8 @@ func TestNewDstSelector(t *testing.T) { controller.DeleteNetworkPolicy(netpolBar) // netpolFoo dst-selects podFoo - require.False(t, m.entryExists(defaultAllowIPSetName, podIP)) + require.False(t, m.entryExists(ingressDefaultAllowIPSetName, podIP)) controller.DeleteNetworkPolicy(netpolFoo) // No netpol dst-selects podFoo - require.True(t, m.entryExists(defaultAllowIPSetName, podIP)) + require.True(t, m.entryExists(ingressDefaultAllowIPSetName, podIP)) } diff --git a/npc/namespace.go b/npc/namespace.go index 5dc3f46e..2fa91887 100644 --- a/npc/namespace.go +++ b/npc/namespace.go @@ -35,7 +35,7 @@ type ns struct { // stores IP addrs of pods which are not selected by any dst podSelector of // any netpol; used only in non-legacy mode and is used as a dst in // the WEAVE-NPC-DEFAULT iptables chain. - defaultAllowIPSet ipset.Name + ingressDefaultAllowIPSet ipset.Name nsSelectors *selectorSet podSelectors *selectorSet @@ -66,11 +66,11 @@ func newNS(name, nodeName string, legacy bool, ipt iptables.Interface, ips ipset ns.podSelectors = newSelectorSet(ips, ns.onNewPodSelector, ns.onNewDstPodSelector, ns.onDestroyDstPodSelector) if !legacy { - defaultAllowIPSet := ipset.Name(IpsetNamePrefix + shortName("default-allow:"+name)) - if err := ips.Create(defaultAllowIPSet, ipset.HashIP); err != nil { + ingressDefaultAllowIPSet := ipset.Name(IpsetNamePrefix + shortName("ingress-default-allow:"+name)) + if err := ips.Create(ingressDefaultAllowIPSet, ipset.HashIP); err != nil { return nil, err } - ns.defaultAllowIPSet = defaultAllowIPSet + ns.ingressDefaultAllowIPSet = ingressDefaultAllowIPSet } if err := ns.podSelectors.provision(ns.uid, nil, map[string]*selectorSpec{ns.allPods.key: ns.allPods}); err != nil { @@ -111,7 +111,7 @@ func (ns *ns) onNewDstPodSelector(selector *selector) error { if hasIP(pod) { // Remove the pod from default-allow if dst podselector matches the pod if selector.matches(pod.ObjectMeta.Labels) { - if err := ns.ips.DelEntry(pod.ObjectMeta.UID, ns.defaultAllowIPSet, pod.Status.PodIP); err != nil { + if err := ns.ips.DelEntry(pod.ObjectMeta.UID, ns.ingressDefaultAllowIPSet, pod.Status.PodIP); err != nil { return err } } @@ -150,7 +150,7 @@ func (ns *ns) addToDefaultAllowIfNoMatching(pod *coreapi.Pod) error { } } if !found { - if err := ns.ips.AddEntry(pod.ObjectMeta.UID, ns.defaultAllowIPSet, pod.Status.PodIP, podComment(pod)); err != nil { + if err := ns.ips.AddEntry(pod.ObjectMeta.UID, ns.ingressDefaultAllowIPSet, pod.Status.PodIP, podComment(pod)); err != nil { return err } } @@ -181,7 +181,7 @@ func (ns *ns) addPod(obj *coreapi.Pod) error { } // If there are no matching dst selectors, add the pod to default-allow if !ns.legacy && !found { - if err := ns.ips.AddEntry(obj.ObjectMeta.UID, ns.defaultAllowIPSet, obj.Status.PodIP, podComment(obj)); err != nil { + if err := ns.ips.AddEntry(obj.ObjectMeta.UID, ns.ingressDefaultAllowIPSet, obj.Status.PodIP, podComment(obj)); err != nil { return err } } @@ -203,7 +203,7 @@ func (ns *ns) updatePod(oldObj, newObj *coreapi.Pod) error { } if !ns.legacy { - if err := ns.ips.DelEntry(oldObj.ObjectMeta.UID, ns.defaultAllowIPSet, oldObj.Status.PodIP); err != nil { + if err := ns.ips.DelEntry(oldObj.ObjectMeta.UID, ns.ingressDefaultAllowIPSet, oldObj.Status.PodIP); err != nil { return err } } @@ -221,7 +221,7 @@ func (ns *ns) updatePod(oldObj, newObj *coreapi.Pod) error { } if !ns.legacy && !found { - if err := ns.ips.AddEntry(newObj.ObjectMeta.UID, ns.defaultAllowIPSet, newObj.Status.PodIP, podComment(newObj)); err != nil { + if err := ns.ips.AddEntry(newObj.ObjectMeta.UID, ns.ingressDefaultAllowIPSet, newObj.Status.PodIP, podComment(newObj)); err != nil { return err } } @@ -231,15 +231,15 @@ func (ns *ns) updatePod(oldObj, newObj *coreapi.Pod) error { if !ns.legacy && oldObj.Status.PodIP != newObj.Status.PodIP && - ns.ips.Exist(oldObj.ObjectMeta.UID, ns.defaultAllowIPSet, oldObj.Status.PodIP) { + ns.ips.Exist(oldObj.ObjectMeta.UID, ns.ingressDefaultAllowIPSet, oldObj.Status.PodIP) { // Instead of iterating over all selectors we check whether old pod IP // has been inserted into default-allow ipset to decide whether the IP // in the ipset has to be updated. - if err := ns.ips.DelEntry(oldObj.ObjectMeta.UID, ns.defaultAllowIPSet, oldObj.Status.PodIP); err != nil { + if err := ns.ips.DelEntry(oldObj.ObjectMeta.UID, ns.ingressDefaultAllowIPSet, oldObj.Status.PodIP); err != nil { return err } - if err := ns.ips.AddEntry(newObj.ObjectMeta.UID, ns.defaultAllowIPSet, newObj.Status.PodIP, podComment(newObj)); err != nil { + if err := ns.ips.AddEntry(newObj.ObjectMeta.UID, ns.ingressDefaultAllowIPSet, newObj.Status.PodIP, podComment(newObj)); err != nil { return err } } @@ -267,7 +267,7 @@ func (ns *ns) updatePod(oldObj, newObj *coreapi.Pod) error { if !ns.legacy && ns.podSelectors.dstSelectorExist(ps) { switch { case !oldMatch && newMatch: - if err := ns.ips.DelEntry(oldObj.ObjectMeta.UID, ns.defaultAllowIPSet, oldObj.Status.PodIP); err != nil { + if err := ns.ips.DelEntry(oldObj.ObjectMeta.UID, ns.ingressDefaultAllowIPSet, oldObj.Status.PodIP); err != nil { return err } case oldMatch && !newMatch: @@ -294,7 +294,7 @@ func (ns *ns) deletePod(obj *coreapi.Pod) error { } if !ns.legacy { - if err := ns.ips.DelEntry(obj.ObjectMeta.UID, ns.defaultAllowIPSet, obj.Status.PodIP); err != nil { + if err := ns.ips.DelEntry(obj.ObjectMeta.UID, ns.ingressDefaultAllowIPSet, obj.Status.PodIP); err != nil { return err } } @@ -381,7 +381,7 @@ func (ns *ns) ensureBypassRule() error { if ns.legacy { ipset = ns.allPods.ipsetName } else { - ipset = ns.defaultAllowIPSet + ipset = ns.ingressDefaultAllowIPSet } common.Log.Debugf("ensuring rule for DefaultAllow in namespace: %s, set %s", ns.name, ipset) @@ -393,7 +393,7 @@ func (ns *ns) deleteBypassRule() error { if ns.legacy { ipset = ns.allPods.ipsetName } else { - ipset = ns.defaultAllowIPSet + ipset = ns.ingressDefaultAllowIPSet } common.Log.Debugf("removing default rule in namespace: %s, set %s", ns.name, ipset) @@ -477,7 +477,7 @@ func (ns *ns) deleteNamespace(obj *coreapi.Namespace) error { } if !ns.legacy { - return ns.ips.Destroy(ns.defaultAllowIPSet) + return ns.ips.Destroy(ns.ingressDefaultAllowIPSet) } return nil From d474c1c26daeb4f57e4ff81d37d09514a460ca66 Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Sat, 19 May 2018 11:44:12 +0200 Subject: [PATCH 04/46] npc: Add policyType to selectorSpec This allows us to distinguish between ingress and egress dst selectors (required when managing entries in default-allow ipsets). --- npc/analyser.go | 14 +++++---- npc/controller.go | 2 +- npc/controller_test.go | 1 + npc/namespace.go | 36 ++++++++++++++--------- npc/selector.go | 65 ++++++++++++++++++++++++------------------ npc/types.go | 15 ++++++++++ 6 files changed, 84 insertions(+), 49 deletions(-) create mode 100644 npc/types.go diff --git a/npc/analyser.go b/npc/analyser.go index 50246a38..bd7203cf 100644 --- a/npc/analyser.go +++ b/npc/analyser.go @@ -24,7 +24,8 @@ func (ns *ns) analysePolicyLegacy(policy *extnapi.NetworkPolicy) ( podSelectors = make(map[string]*selectorSpec) rules = make(map[string]*ruleSpec) - dstSelector, err := newSelectorSpec(&policy.Spec.PodSelector, true, ns.name, ipset.HashIP) + policyTypes := []policyType{ingressPolicy} + dstSelector, err := newSelectorSpec(&policy.Spec.PodSelector, true, policyTypes, ns.name, ipset.HashIP) if err != nil { return nil, nil, nil, err } @@ -61,14 +62,14 @@ func (ns *ns) analysePolicyLegacy(policy *extnapi.NetworkPolicy) ( for _, peer := range ingressRule.From { var srcSelector *selectorSpec if peer.PodSelector != nil { - srcSelector, err = newSelectorSpec(peer.PodSelector, false, ns.name, ipset.HashIP) + srcSelector, err = newSelectorSpec(peer.PodSelector, false, nil, ns.name, ipset.HashIP) if err != nil { return nil, nil, nil, err } podSelectors[srcSelector.key] = srcSelector } if peer.NamespaceSelector != nil { - srcSelector, err = newSelectorSpec(peer.NamespaceSelector, false, "", ipset.ListSet) + srcSelector, err = newSelectorSpec(peer.NamespaceSelector, false, nil, "", ipset.ListSet) if err != nil { return nil, nil, nil, err } @@ -104,7 +105,8 @@ func (ns *ns) analysePolicy(policy *networkingv1.NetworkPolicy) ( rules = make(map[string]*ruleSpec) // If empty, matches all pods in a namespace - dstSelector, err := newSelectorSpec(&policy.Spec.PodSelector, true, ns.name, ipset.HashIP) + policyTypes := []policyType{ingressPolicy} + dstSelector, err := newSelectorSpec(&policy.Spec.PodSelector, true, policyTypes, ns.name, ipset.HashIP) if err != nil { return nil, nil, nil, err } @@ -143,14 +145,14 @@ func (ns *ns) analysePolicy(policy *networkingv1.NetworkPolicy) ( // NetworkPolicyPeer describes a peer to allow traffic from. // Exactly one of its fields must be specified. if peer.PodSelector != nil { - srcSelector, err = newSelectorSpec(peer.PodSelector, false, ns.name, ipset.HashIP) + srcSelector, err = newSelectorSpec(peer.PodSelector, false, nil, ns.name, ipset.HashIP) if err != nil { return nil, nil, nil, err } addIfNotExist(srcSelector, podSelectors) } else if peer.NamespaceSelector != nil { - srcSelector, err = newSelectorSpec(peer.NamespaceSelector, false, "", ipset.ListSet) + srcSelector, err = newSelectorSpec(peer.NamespaceSelector, false, nil, "", ipset.ListSet) if err != nil { return nil, nil, nil, err } diff --git a/npc/controller.go b/npc/controller.go index 6bd26b1e..4b3415a4 100644 --- a/npc/controller.go +++ b/npc/controller.go @@ -49,7 +49,7 @@ func New(nodeName string, legacy bool, ipt iptables.Interface, ips ipset.Interfa ips: ips, nss: make(map[string]*ns)} - doNothing := func(*selector) error { return nil } + doNothing := func(*selector, policyType) error { return nil } c.nsSelectors = newSelectorSet(ips, c.onNewNsSelector, doNothing, doNothing) return c diff --git a/npc/controller_test.go b/npc/controller_test.go index 6928fc52..0336416a 100644 --- a/npc/controller_test.go +++ b/npc/controller_test.go @@ -236,6 +236,7 @@ func TestDefaultAllow(t *testing.T) { Status: coreapi.PodStatus{PodIP: barPodIP}} podBarNoIP := &coreapi.Pod{ObjectMeta: podBar.ObjectMeta} controller.AddPod(podBarNoIP) + controller.UpdatePod(podBarNoIP, podBar) // Should add the bar pod to default-allow diff --git a/npc/namespace.go b/npc/namespace.go index 2fa91887..cf089c04 100644 --- a/npc/namespace.go +++ b/npc/namespace.go @@ -45,7 +45,7 @@ type ns struct { } func newNS(name, nodeName string, legacy bool, ipt iptables.Interface, ips ipset.Interface, nsSelectors *selectorSet) (*ns, error) { - allPods, err := newSelectorSpec(&metav1.LabelSelector{}, false, name, ipset.HashIP) + allPods, err := newSelectorSpec(&metav1.LabelSelector{}, false, nil, name, ipset.HashIP) if err != nil { return nil, err } @@ -102,7 +102,7 @@ func (ns *ns) onNewPodSelector(selector *selector) error { return nil } -func (ns *ns) onNewDstPodSelector(selector *selector) error { +func (ns *ns) onNewDstPodSelector(selector *selector, policyType policyType) error { if ns.legacy { return nil } @@ -110,8 +110,12 @@ func (ns *ns) onNewDstPodSelector(selector *selector) error { for _, pod := range ns.pods { if hasIP(pod) { // Remove the pod from default-allow if dst podselector matches the pod + ipset := ns.ingressDefaultAllowIPSet + if policyType == egressPolicy { + // TODO(brb) ipset = ns.egressDefaultAllowIPSet + } if selector.matches(pod.ObjectMeta.Labels) { - if err := ns.ips.DelEntry(pod.ObjectMeta.UID, ns.ingressDefaultAllowIPSet, pod.Status.PodIP); err != nil { + if err := ns.ips.DelEntry(pod.ObjectMeta.UID, ipset, pod.Status.PodIP); err != nil { return err } } @@ -121,7 +125,7 @@ func (ns *ns) onNewDstPodSelector(selector *selector) error { return nil } -func (ns *ns) onDestroyDstPodSelector(selector *selector) error { +func (ns *ns) onDestroyDstPodSelector(selector *selector, policyType policyType) error { if ns.legacy { return nil } @@ -129,7 +133,7 @@ func (ns *ns) onDestroyDstPodSelector(selector *selector) error { for _, pod := range ns.pods { if hasIP(pod) { if selector.matches(pod.ObjectMeta.Labels) { - if err := ns.addToDefaultAllowIfNoMatching(pod); err != nil { + if err := ns.addToDefaultAllowIfNoMatching(pod, policyType); err != nil { return err } } @@ -140,17 +144,21 @@ func (ns *ns) onDestroyDstPodSelector(selector *selector) error { } // Add pod IP addr to default-allow ipset if there are no matching dst selectors -func (ns *ns) addToDefaultAllowIfNoMatching(pod *coreapi.Pod) error { +func (ns *ns) addToDefaultAllowIfNoMatching(pod *coreapi.Pod, policyType policyType) error { found := false // TODO(mp) optimize (avoid iterating over selectors) by ref counting IP addrs. for _, s := range ns.podSelectors.entries { - if ns.podSelectors.dstSelectorExist(s) && s.matches(pod.ObjectMeta.Labels) { + if ns.podSelectors.dstSelectorExist(s, policyType) && s.matches(pod.ObjectMeta.Labels) { found = true break } } if !found { - if err := ns.ips.AddEntry(pod.ObjectMeta.UID, ns.ingressDefaultAllowIPSet, pod.Status.PodIP, podComment(pod)); err != nil { + ipset := ns.ingressDefaultAllowIPSet + if policyType == egressPolicy { + // TODO(brb) ipset = ns.egressDefaultAllowIPSet + } + if err := ns.ips.AddEntry(pod.ObjectMeta.UID, ipset, pod.Status.PodIP, podComment(pod)); err != nil { return err } } @@ -175,12 +183,12 @@ func (ns *ns) addPod(obj *coreapi.Pod) error { ns.ips.AddEntry(obj.ObjectMeta.UID, LocalIpset, obj.Status.PodIP, podComment(obj)) } - found, err := ns.podSelectors.addToMatching(obj.ObjectMeta.UID, obj.ObjectMeta.Labels, obj.Status.PodIP, podComment(obj)) + foundIngress, err := ns.podSelectors.addToMatching(obj.ObjectMeta.UID, obj.ObjectMeta.Labels, obj.Status.PodIP, podComment(obj)) if err != nil { return err } // If there are no matching dst selectors, add the pod to default-allow - if !ns.legacy && !found { + if !ns.legacy && !foundIngress { if err := ns.ips.AddEntry(obj.ObjectMeta.UID, ns.ingressDefaultAllowIPSet, obj.Status.PodIP, podComment(obj)); err != nil { return err } @@ -215,12 +223,12 @@ func (ns *ns) updatePod(oldObj, newObj *coreapi.Pod) error { if ns.checkLocalPod(newObj) { ns.ips.AddEntry(newObj.ObjectMeta.UID, LocalIpset, newObj.Status.PodIP, podComment(newObj)) } - found, err := ns.podSelectors.addToMatching(newObj.ObjectMeta.UID, newObj.ObjectMeta.Labels, newObj.Status.PodIP, podComment(newObj)) + foundIngress, err := ns.podSelectors.addToMatching(newObj.ObjectMeta.UID, newObj.ObjectMeta.Labels, newObj.Status.PodIP, podComment(newObj)) if err != nil { return err } - if !ns.legacy && !found { + if !ns.legacy && !foundIngress { if err := ns.ips.AddEntry(newObj.ObjectMeta.UID, ns.ingressDefaultAllowIPSet, newObj.Status.PodIP, podComment(newObj)); err != nil { return err } @@ -264,14 +272,14 @@ func (ns *ns) updatePod(oldObj, newObj *coreapi.Pod) error { } } - if !ns.legacy && ns.podSelectors.dstSelectorExist(ps) { + if !ns.legacy && ns.podSelectors.dstSelectorExist(ps, ingressPolicy) { switch { case !oldMatch && newMatch: if err := ns.ips.DelEntry(oldObj.ObjectMeta.UID, ns.ingressDefaultAllowIPSet, oldObj.Status.PodIP); err != nil { return err } case oldMatch && !newMatch: - if err := ns.addToDefaultAllowIfNoMatching(newObj); err != nil { + if err := ns.addToDefaultAllowIfNoMatching(newObj, ingressPolicy); err != nil { return err } } diff --git a/npc/selector.go b/npc/selector.go index 2a40e185..496cfd8f 100644 --- a/npc/selector.go +++ b/npc/selector.go @@ -10,25 +10,27 @@ import ( ) type selectorSpec struct { - key string // string representation (for hash keying/equality comparison) - selector labels.Selector // k8s Selector object (for matching) - dst bool // destination selector + key string // string representation (for hash keying/equality comparison) + selector labels.Selector // k8s Selector object (for matching) + dst bool // destination selector TODO(brb) not relevant + policyTypes []policyType // TODO(brb) repharse: netpol types (egress, ingress or both) which initiated the dst selector ipsetType ipset.Type // type of ipset to provision ipsetName ipset.Name // generated ipset name nsName string // Namespace name } -func newSelectorSpec(json *metav1.LabelSelector, dst bool, nsName string, ipsetType ipset.Type) (*selectorSpec, error) { +func newSelectorSpec(json *metav1.LabelSelector, dst bool, policyType []policyType, nsName string, ipsetType ipset.Type) (*selectorSpec, error) { selector, err := metav1.LabelSelectorAsSelector(json) if err != nil { return nil, err } key := selector.String() return &selectorSpec{ - key: key, - selector: selector, - dst: dst, + key: key, + selector: selector, + dst: dst, + policyTypes: policyType, // We prefix the selector string with the namespace name when generating // the shortname because you can specify the same selector in multiple // namespaces - we need those to map to distinct ipsets @@ -55,26 +57,28 @@ func (s *selector) delEntry(user types.UID, entry string) error { } type selectorFn func(selector *selector) error +type selectorWithPolicyTypeFn func(selector *selector, policyType policyType) error type selectorSet struct { ips ipset.Interface onNewSelector selectorFn // invoked after dst selector has been provisioned for the first time - onNewDstSelector selectorFn + onNewDstSelector selectorWithPolicyTypeFn // invoked after the last instance of dst selector has been deprovisioned - onDestroyDstSelector selectorFn + onDestroyDstSelector selectorWithPolicyTypeFn users map[string]map[types.UID]struct{} // list of users per selector entries map[string]*selector // We need to keep track of dst selector instances to be able to invoke // onNewDstSelector and onDestroyDstSelector callbacks at the right time; - // selectorSpec.Key -> count - dstSelectorsCount map[string]int + // selectorSpec.Key -> policyType -> count + // TODO(brb) dst -> target? + dstSelectorsCount map[string]map[policyType]int } -func newSelectorSet(ips ipset.Interface, onNewSelector, onNewDstSelector selectorFn, onDestroyDstSelector selectorFn) *selectorSet { +func newSelectorSet(ips ipset.Interface, onNewSelector selectorFn, onNewDstSelector, onDestroyDstSelector selectorWithPolicyTypeFn) *selectorSet { return &selectorSet{ ips: ips, onNewSelector: onNewSelector, @@ -82,22 +86,23 @@ func newSelectorSet(ips ipset.Interface, onNewSelector, onNewDstSelector selecto onDestroyDstSelector: onDestroyDstSelector, users: make(map[string]map[types.UID]struct{}), entries: make(map[string]*selector), - dstSelectorsCount: make(map[string]int)} + dstSelectorsCount: make(map[string]map[policyType]int)} } func (ss *selectorSet) addToMatching(user types.UID, labelMap map[string]string, entry string, comment string) (bool, error) { - found := false + foundIngress := false + // TODO(brb) foundEgress for _, s := range ss.entries { if s.matches(labelMap) { - if ss.dstSelectorExist(s) { - found = true + if ss.dstSelectorExist(s, ingressPolicy) { + foundIngress = true } if err := s.addEntry(user, entry, comment); err != nil { - return found, err + return foundIngress, err } } } - return found, nil + return foundIngress, nil } func (ss *selectorSet) delFromMatching(user types.UID, labelMap map[string]string, entry string) error { @@ -111,8 +116,8 @@ func (ss *selectorSet) delFromMatching(user types.UID, labelMap map[string]strin return nil } -func (ss *selectorSet) dstSelectorExist(s *selector) bool { - return ss.dstSelectorsCount[s.spec.key] > 0 +func (ss *selectorSet) dstSelectorExist(s *selector, policyType policyType) bool { + return ss.dstSelectorsCount[s.spec.key][policyType] > 0 } func (ss *selectorSet) deprovision(user types.UID, current, desired map[string]*selectorSpec) error { @@ -129,13 +134,14 @@ func (ss *selectorSet) deprovision(user types.UID, current, desired map[string]* delete(ss.users, key) } - if spec.dst { - ss.dstSelectorsCount[key]-- - if ss.dstSelectorsCount[key] == 0 { - if err := ss.onDestroyDstSelector(&selector{ss.ips, spec}); err != nil { + for _, policyType := range spec.policyTypes { + ss.dstSelectorsCount[key][policyType]-- + if ss.dstSelectorsCount[key][policyType] == 0 { + if err := ss.onDestroyDstSelector(&selector{ss.ips, spec}, policyType); err != nil { return err } } + // TODO(brb) delete(...) } } } @@ -160,10 +166,13 @@ func (ss *selectorSet) provision(user types.UID, current, desired map[string]*se } ss.users[key][user] = struct{}{} - if spec.dst { - ss.dstSelectorsCount[key]++ - if ss.dstSelectorsCount[key] == 1 { - if err := ss.onNewDstSelector(selector); err != nil { + for _, pt := range spec.policyTypes { + if _, found := ss.dstSelectorsCount[key]; !found { + ss.dstSelectorsCount[key] = make(map[policyType]int) + } + ss.dstSelectorsCount[key][pt]++ + if ss.dstSelectorsCount[key][pt] == 1 { + if err := ss.onNewDstSelector(selector, pt); err != nil { return err } } diff --git a/npc/types.go b/npc/types.go new file mode 100644 index 00000000..3b594ff9 --- /dev/null +++ b/npc/types.go @@ -0,0 +1,15 @@ +package npc + +type policyType byte + +const ( + ingressPolicy policyType = iota + egressPolicy +) + +func policyTypeStr(policyType policyType) string { + if policyType == egressPolicy { + return "egress" + } + return "ingress" +} From 79727a86fa2f94949abd09b77ff55f657cc7034c Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Sat, 19 May 2018 12:30:05 +0200 Subject: [PATCH 05/46] npc: Manage entries in egressDefaultAllowIPSet --- npc/namespace.go | 125 +++++++++++++++++++++++++++++++++++------------ npc/selector.go | 11 +++-- 2 files changed, 100 insertions(+), 36 deletions(-) diff --git a/npc/namespace.go b/npc/namespace.go index cf089c04..5b5607cd 100644 --- a/npc/namespace.go +++ b/npc/namespace.go @@ -34,8 +34,9 @@ type ns struct { // stores IP addrs of pods which are not selected by any dst podSelector of // any netpol; used only in non-legacy mode and is used as a dst in - // the WEAVE-NPC-DEFAULT iptables chain. + // the WEAVE-NPC-{INGRESS,EGRESS}-DEFAULT iptables chain. ingressDefaultAllowIPSet ipset.Name + egressDefaultAllowIPSet ipset.Name nsSelectors *selectorSet podSelectors *selectorSet @@ -71,6 +72,13 @@ func newNS(name, nodeName string, legacy bool, ipt iptables.Interface, ips ipset return nil, err } ns.ingressDefaultAllowIPSet = ingressDefaultAllowIPSet + + egressDefaultAllowIPSet := ipset.Name(IpsetNamePrefix + shortName("egress-default-allow:"+name)) + if err := ips.Create(egressDefaultAllowIPSet, ipset.HashIP); err != nil { + return nil, err + } + ns.egressDefaultAllowIPSet = egressDefaultAllowIPSet + } if err := ns.podSelectors.provision(ns.uid, nil, map[string]*selectorSpec{ns.allPods.key: ns.allPods}); err != nil { @@ -112,7 +120,7 @@ func (ns *ns) onNewDstPodSelector(selector *selector, policyType policyType) err // Remove the pod from default-allow if dst podselector matches the pod ipset := ns.ingressDefaultAllowIPSet if policyType == egressPolicy { - // TODO(brb) ipset = ns.egressDefaultAllowIPSet + ipset = ns.egressDefaultAllowIPSet } if selector.matches(pod.ObjectMeta.Labels) { if err := ns.ips.DelEntry(pod.ObjectMeta.UID, ipset, pod.Status.PodIP); err != nil { @@ -156,7 +164,7 @@ func (ns *ns) addToDefaultAllowIfNoMatching(pod *coreapi.Pod, policyType policyT if !found { ipset := ns.ingressDefaultAllowIPSet if policyType == egressPolicy { - // TODO(brb) ipset = ns.egressDefaultAllowIPSet + ipset = ns.egressDefaultAllowIPSet } if err := ns.ips.AddEntry(pod.ObjectMeta.UID, ipset, pod.Status.PodIP, podComment(pod)); err != nil { return err @@ -183,14 +191,21 @@ func (ns *ns) addPod(obj *coreapi.Pod) error { ns.ips.AddEntry(obj.ObjectMeta.UID, LocalIpset, obj.Status.PodIP, podComment(obj)) } - foundIngress, err := ns.podSelectors.addToMatching(obj.ObjectMeta.UID, obj.ObjectMeta.Labels, obj.Status.PodIP, podComment(obj)) + foundIngress, foundEgress, err := ns.podSelectors.addToMatching(obj.ObjectMeta.UID, obj.ObjectMeta.Labels, obj.Status.PodIP, podComment(obj)) if err != nil { return err } // If there are no matching dst selectors, add the pod to default-allow - if !ns.legacy && !foundIngress { - if err := ns.ips.AddEntry(obj.ObjectMeta.UID, ns.ingressDefaultAllowIPSet, obj.Status.PodIP, podComment(obj)); err != nil { - return err + if !ns.legacy { + if !foundIngress { + if err := ns.ips.AddEntry(obj.ObjectMeta.UID, ns.ingressDefaultAllowIPSet, obj.Status.PodIP, podComment(obj)); err != nil { + return err + } + } + if !foundEgress { + if err := ns.ips.AddEntry(obj.ObjectMeta.UID, ns.egressDefaultAllowIPSet, obj.Status.PodIP, podComment(obj)); err != nil { + return err + } } } @@ -214,6 +229,9 @@ func (ns *ns) updatePod(oldObj, newObj *coreapi.Pod) error { if err := ns.ips.DelEntry(oldObj.ObjectMeta.UID, ns.ingressDefaultAllowIPSet, oldObj.Status.PodIP); err != nil { return err } + if err := ns.ips.DelEntry(oldObj.ObjectMeta.UID, ns.egressDefaultAllowIPSet, oldObj.Status.PodIP); err != nil { + return err + } } return ns.podSelectors.delFromMatching(oldObj.ObjectMeta.UID, oldObj.ObjectMeta.Labels, oldObj.Status.PodIP) @@ -223,31 +241,32 @@ func (ns *ns) updatePod(oldObj, newObj *coreapi.Pod) error { if ns.checkLocalPod(newObj) { ns.ips.AddEntry(newObj.ObjectMeta.UID, LocalIpset, newObj.Status.PodIP, podComment(newObj)) } - foundIngress, err := ns.podSelectors.addToMatching(newObj.ObjectMeta.UID, newObj.ObjectMeta.Labels, newObj.Status.PodIP, podComment(newObj)) + foundIngress, foundEgress, err := ns.podSelectors.addToMatching(newObj.ObjectMeta.UID, newObj.ObjectMeta.Labels, newObj.Status.PodIP, podComment(newObj)) if err != nil { return err } - if !ns.legacy && !foundIngress { - if err := ns.ips.AddEntry(newObj.ObjectMeta.UID, ns.ingressDefaultAllowIPSet, newObj.Status.PodIP, podComment(newObj)); err != nil { - return err + if !ns.legacy { + if !foundIngress { + if err := ns.ips.AddEntry(newObj.ObjectMeta.UID, ns.ingressDefaultAllowIPSet, newObj.Status.PodIP, podComment(newObj)); err != nil { + return err + } + } + if !foundEgress { + if err := ns.ips.AddEntry(newObj.ObjectMeta.UID, ns.egressDefaultAllowIPSet, newObj.Status.PodIP, podComment(newObj)); err != nil { + return err + } } } return nil } - if !ns.legacy && - oldObj.Status.PodIP != newObj.Status.PodIP && - ns.ips.Exist(oldObj.ObjectMeta.UID, ns.ingressDefaultAllowIPSet, oldObj.Status.PodIP) { - // Instead of iterating over all selectors we check whether old pod IP - // has been inserted into default-allow ipset to decide whether the IP - // in the ipset has to be updated. - - if err := ns.ips.DelEntry(oldObj.ObjectMeta.UID, ns.ingressDefaultAllowIPSet, oldObj.Status.PodIP); err != nil { + if !ns.legacy && oldObj.Status.PodIP != newObj.Status.PodIP { + if err := ns.updateDefaultAllowIPSetEntry(oldObj, newObj, ns.ingressDefaultAllowIPSet); err != nil { return err } - if err := ns.ips.AddEntry(newObj.ObjectMeta.UID, ns.ingressDefaultAllowIPSet, newObj.Status.PodIP, podComment(newObj)); err != nil { + if err := ns.updateDefaultAllowIPSetEntry(oldObj, newObj, ns.egressDefaultAllowIPSet); err != nil { return err } } @@ -272,17 +291,35 @@ func (ns *ns) updatePod(oldObj, newObj *coreapi.Pod) error { } } - if !ns.legacy && ns.podSelectors.dstSelectorExist(ps, ingressPolicy) { - switch { - case !oldMatch && newMatch: - if err := ns.ips.DelEntry(oldObj.ObjectMeta.UID, ns.ingressDefaultAllowIPSet, oldObj.Status.PodIP); err != nil { - return err - } - case oldMatch && !newMatch: - if err := ns.addToDefaultAllowIfNoMatching(newObj, ingressPolicy); err != nil { - return err - } + if !ns.legacy { + if err := ns.addOrRemoveToDefaultAllowIPSet(ps, oldObj, newObj, oldMatch, newMatch, ingressPolicy); err != nil { + return err } + if err := ns.addOrRemoveToDefaultAllowIPSet(ps, oldObj, newObj, oldMatch, newMatch, egressPolicy); err != nil { + return err + } + } + } + } + + return nil +} + +func (ns *ns) addOrRemoveToDefaultAllowIPSet(ps *selector, oldObj, newObj *coreapi.Pod, oldMatch, newMatch bool, policyType policyType) error { + ipset := ns.ingressDefaultAllowIPSet + if policyType == egressPolicy { + ipset = ns.egressDefaultAllowIPSet + } + + if ns.podSelectors.dstSelectorExist(ps, policyType) { + switch { + case !oldMatch && newMatch: + if err := ns.ips.DelEntry(oldObj.ObjectMeta.UID, ipset, oldObj.Status.PodIP); err != nil { + return err + } + case oldMatch && !newMatch: + if err := ns.addToDefaultAllowIfNoMatching(newObj, policyType); err != nil { + return err } } } @@ -305,6 +342,9 @@ func (ns *ns) deletePod(obj *coreapi.Pod) error { if err := ns.ips.DelEntry(obj.ObjectMeta.UID, ns.ingressDefaultAllowIPSet, obj.Status.PodIP); err != nil { return err } + if err := ns.ips.DelEntry(obj.ObjectMeta.UID, ns.egressDefaultAllowIPSet, obj.Status.PodIP); err != nil { + return err + } } return ns.podSelectors.delFromMatching(obj.ObjectMeta.UID, obj.ObjectMeta.Labels, obj.Status.PodIP) @@ -380,6 +420,22 @@ func (ns *ns) deleteNetworkPolicy(obj interface{}) error { return ns.podSelectors.deprovision(uid, podSelectors, nil) } +func (ns *ns) updateDefaultAllowIPSetEntry(oldObj, newObj *coreapi.Pod, ipsetName ipset.Name) error { + // Instead of iterating over all selectors we check whether old pod IP + // has been inserted into default-allow ipset to decide whether the IP + // in the ipset has to be updated. + if ns.ips.Exist(oldObj.ObjectMeta.UID, ipsetName, oldObj.Status.PodIP) { + + if err := ns.ips.DelEntry(oldObj.ObjectMeta.UID, ipsetName, oldObj.Status.PodIP); err != nil { + return err + } + if err := ns.ips.AddEntry(newObj.ObjectMeta.UID, ipsetName, newObj.Status.PodIP, podComment(newObj)); err != nil { + return err + } + } + return nil +} + func bypassRule(nsIpsetName ipset.Name, namespace string) []string { return []string{"-m", "set", "--match-set", string(nsIpsetName), "dst", "-j", "ACCEPT", "-m", "comment", "--comment", "DefaultAllow isolation for namespace: " + namespace} } @@ -419,7 +475,7 @@ func (ns *ns) addNamespace(obj *coreapi.Namespace) error { } // Add namespace ipset to matching namespace selectors - _, err := ns.nsSelectors.addToMatching(obj.ObjectMeta.UID, obj.ObjectMeta.Labels, string(ns.allPods.ipsetName), namespaceComment(ns)) + _, _, err := ns.nsSelectors.addToMatching(obj.ObjectMeta.UID, obj.ObjectMeta.Labels, string(ns.allPods.ipsetName), namespaceComment(ns)) return err } @@ -485,7 +541,12 @@ func (ns *ns) deleteNamespace(obj *coreapi.Namespace) error { } if !ns.legacy { - return ns.ips.Destroy(ns.ingressDefaultAllowIPSet) + if err := ns.ips.Destroy(ns.ingressDefaultAllowIPSet); err != nil { + return err + } + if err := ns.ips.Destroy(ns.egressDefaultAllowIPSet); err != nil { + return err + } } return nil diff --git a/npc/selector.go b/npc/selector.go index 496cfd8f..1b080989 100644 --- a/npc/selector.go +++ b/npc/selector.go @@ -89,20 +89,23 @@ func newSelectorSet(ips ipset.Interface, onNewSelector selectorFn, onNewDstSelec dstSelectorsCount: make(map[string]map[policyType]int)} } -func (ss *selectorSet) addToMatching(user types.UID, labelMap map[string]string, entry string, comment string) (bool, error) { +func (ss *selectorSet) addToMatching(user types.UID, labelMap map[string]string, entry string, comment string) (bool, bool, error) { foundIngress := false - // TODO(brb) foundEgress + foundEgress := false for _, s := range ss.entries { if s.matches(labelMap) { if ss.dstSelectorExist(s, ingressPolicy) { foundIngress = true } + if ss.dstSelectorExist(s, egressPolicy) { + foundEgress = true + } if err := s.addEntry(user, entry, comment); err != nil { - return foundIngress, err + return foundIngress, foundEgress, err } } } - return foundIngress, nil + return foundIngress, foundEgress, nil } func (ss *selectorSet) delFromMatching(user types.UID, labelMap map[string]string, entry string) error { From f27d4d2117ea3b2e3a9b1cfa4042a111eca2b7c5 Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Sat, 19 May 2018 12:33:25 +0200 Subject: [PATCH 06/46] npc: Add defaultAllowIPSetName helper --- npc/namespace.go | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/npc/namespace.go b/npc/namespace.go index 5b5607cd..41fb6af3 100644 --- a/npc/namespace.go +++ b/npc/namespace.go @@ -118,10 +118,7 @@ func (ns *ns) onNewDstPodSelector(selector *selector, policyType policyType) err for _, pod := range ns.pods { if hasIP(pod) { // Remove the pod from default-allow if dst podselector matches the pod - ipset := ns.ingressDefaultAllowIPSet - if policyType == egressPolicy { - ipset = ns.egressDefaultAllowIPSet - } + ipset := ns.defaultAllowIPSetName(policyType) if selector.matches(pod.ObjectMeta.Labels) { if err := ns.ips.DelEntry(pod.ObjectMeta.UID, ipset, pod.Status.PodIP); err != nil { return err @@ -162,10 +159,7 @@ func (ns *ns) addToDefaultAllowIfNoMatching(pod *coreapi.Pod, policyType policyT } } if !found { - ipset := ns.ingressDefaultAllowIPSet - if policyType == egressPolicy { - ipset = ns.egressDefaultAllowIPSet - } + ipset := ns.defaultAllowIPSetName(policyType) if err := ns.ips.AddEntry(pod.ObjectMeta.UID, ipset, pod.Status.PodIP, podComment(pod)); err != nil { return err } @@ -306,11 +300,7 @@ func (ns *ns) updatePod(oldObj, newObj *coreapi.Pod) error { } func (ns *ns) addOrRemoveToDefaultAllowIPSet(ps *selector, oldObj, newObj *coreapi.Pod, oldMatch, newMatch bool, policyType policyType) error { - ipset := ns.ingressDefaultAllowIPSet - if policyType == egressPolicy { - ipset = ns.egressDefaultAllowIPSet - } - + ipset := ns.defaultAllowIPSetName(policyType) if ns.podSelectors.dstSelectorExist(ps, policyType) { switch { case !oldMatch && newMatch: @@ -323,7 +313,6 @@ func (ns *ns) addOrRemoveToDefaultAllowIPSet(ps *selector, oldObj, newObj *corea } } } - return nil } @@ -601,6 +590,14 @@ func podComment(pod *coreapi.Pod) string { return fmt.Sprintf("namespace: %s, pod: %s", pod.ObjectMeta.Namespace, pod.ObjectMeta.Name) } +func (ns *ns) defaultAllowIPSetName(pt policyType) ipset.Name { + ipset := ns.ingressDefaultAllowIPSet + if pt == egressPolicy { + ipset = ns.egressDefaultAllowIPSet + } + return ipset +} + func (ns *ns) analyse(obj interface{}) ( uid types.UID, rules map[string]*ruleSpec, From 5afcd0595d7467ad64437a5f674a04ecc17d93f4 Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Sat, 19 May 2018 15:51:41 +0200 Subject: [PATCH 07/46] npc: Parse PolicyTypes field when analysing a netpol --- npc/analyser.go | 10 +++++++++- npc/controller_test.go | 6 +++++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/npc/analyser.go b/npc/analyser.go index bd7203cf..0e98d8d6 100644 --- a/npc/analyser.go +++ b/npc/analyser.go @@ -103,9 +103,17 @@ func (ns *ns) analysePolicy(policy *networkingv1.NetworkPolicy) ( nsSelectors = make(map[string]*selectorSpec) podSelectors = make(map[string]*selectorSpec) rules = make(map[string]*ruleSpec) + policyTypes := make([]policyType, 0) + for _, pt := range policy.Spec.PolicyTypes { + if pt == networkingv1.PolicyTypeIngress { + policyTypes = append(policyTypes, ingressPolicy) + } + if pt == networkingv1.PolicyTypeEgress { + policyTypes = append(policyTypes, egressPolicy) + } + } // If empty, matches all pods in a namespace - policyTypes := []policyType{ingressPolicy} dstSelector, err := newSelectorSpec(&policy.Spec.PodSelector, true, policyTypes, ns.name, ipset.HashIP) if err != nil { return nil, nil, nil, err diff --git a/npc/controller_test.go b/npc/controller_test.go index 0336416a..1f7b9a79 100644 --- a/npc/controller_test.go +++ b/npc/controller_test.go @@ -144,6 +144,7 @@ func TestRegressionPolicyNamespaceOrdering3059(t *testing.T) { Namespace: "destination", }, Spec: networkingv1.NetworkPolicySpec{ + PolicyTypes: []networkingv1.PolicyType{networkingv1.PolicyTypeIngress}, Ingress: []networkingv1.NetworkPolicyIngressRule{ { From: []networkingv1.NetworkPolicyPeer{ @@ -249,6 +250,7 @@ func TestDefaultAllow(t *testing.T) { Namespace: "default", }, Spec: networkingv1.NetworkPolicySpec{ + PolicyTypes: []networkingv1.PolicyType{networkingv1.PolicyTypeIngress}, PodSelector: metav1.LabelSelector{MatchLabels: map[string]string{"run": "foo"}}, Ingress: []networkingv1.NetworkPolicyIngressRule{{ From: []networkingv1.NetworkPolicyPeer{{ @@ -299,7 +301,6 @@ func TestDefaultAllow(t *testing.T) { controller.DeleteNamespace(defaultNamespace) // Should remove default ipset require.NotContains(t, m.sets, ingressDefaultAllowIPSetName) - } func TestOutOfOrderPodEvents(t *testing.T) { @@ -337,6 +338,7 @@ func TestOutOfOrderPodEvents(t *testing.T) { Namespace: "default", }, Spec: networkingv1.NetworkPolicySpec{ + PolicyTypes: []networkingv1.PolicyType{networkingv1.PolicyTypeIngress}, PodSelector: metav1.LabelSelector{MatchLabels: map[string]string{"run": "foo"}}, Ingress: []networkingv1.NetworkPolicyIngressRule{{ From: []networkingv1.NetworkPolicyPeer{{ @@ -418,6 +420,7 @@ func TestNewDstSelector(t *testing.T) { Namespace: "default", }, Spec: networkingv1.NetworkPolicySpec{ + PolicyTypes: []networkingv1.PolicyType{networkingv1.PolicyTypeIngress}, PodSelector: metav1.LabelSelector{}, Ingress: []networkingv1.NetworkPolicyIngressRule{{ From: []networkingv1.NetworkPolicyPeer{{ @@ -438,6 +441,7 @@ func TestNewDstSelector(t *testing.T) { Namespace: "default", }, Spec: networkingv1.NetworkPolicySpec{ + PolicyTypes: []networkingv1.PolicyType{networkingv1.PolicyTypeIngress}, PodSelector: metav1.LabelSelector{}, Ingress: []networkingv1.NetworkPolicyIngressRule{{ From: []networkingv1.NetworkPolicyPeer{{ From 497a9c7fd0f0577140b048ee23a363fb4ce0e699 Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Mon, 21 May 2018 08:28:35 +0200 Subject: [PATCH 08/46] npc: Replace rulesType by policyType --- npc/analyser.go | 16 ++++++++-------- npc/rule.go | 35 +++++++++++++++-------------------- npc/rule_test.go | 2 +- 3 files changed, 24 insertions(+), 29 deletions(-) diff --git a/npc/analyser.go b/npc/analyser.go index 0e98d8d6..376ffa6b 100644 --- a/npc/analyser.go +++ b/npc/analyser.go @@ -46,13 +46,13 @@ func (ns *ns) analysePolicyLegacy(policy *extnapi.NetworkPolicy) ( // From is not provided, this rule matches all sources (traffic not restricted by source). if ingressRule.Ports == nil { // Ports is not provided, this rule matches all ports (traffic not restricted by port). - rule := newRuleSpec(ingress, nil, nil, dstSelector, nil) + rule := newRuleSpec(ingressPolicy, nil, nil, dstSelector, nil) rules[rule.key] = rule } else { // Ports is present and contains at least one item, then this rule allows traffic // only if the traffic matches at least one port in the ports list. withNormalisedProtoAndPortLegacy(ingressRule.Ports, func(proto, port string) { - rule := newRuleSpec(ingress, &proto, nil, dstSelector, &port) + rule := newRuleSpec(ingressPolicy, &proto, nil, dstSelector, &port) rules[rule.key] = rule }) } @@ -78,13 +78,13 @@ func (ns *ns) analysePolicyLegacy(policy *extnapi.NetworkPolicy) ( if ingressRule.Ports == nil { // Ports is not provided, this rule matches all ports (traffic not restricted by port). - rule := newRuleSpec(ingress, nil, srcSelector, dstSelector, nil) + rule := newRuleSpec(ingressPolicy, nil, srcSelector, dstSelector, nil) rules[rule.key] = rule } else { // Ports is present and contains at least one item, then this rule allows traffic // only if the traffic matches at least one port in the ports list. withNormalisedProtoAndPortLegacy(ingressRule.Ports, func(proto, port string) { - rule := newRuleSpec(ingress, &proto, srcSelector, dstSelector, &port) + rule := newRuleSpec(ingressPolicy, &proto, srcSelector, dstSelector, &port) rules[rule.key] = rule }) } @@ -138,11 +138,11 @@ func (ns *ns) analysePolicy(policy *networkingv1.NetworkPolicy) ( if allSources { if allPorts { - rule := newRuleSpec(ingress, nil, nil, dstSelector, nil) + rule := newRuleSpec(ingressPolicy, nil, nil, dstSelector, nil) rules[rule.key] = rule } else { withNormalisedProtoAndPort(ingressRule.Ports, func(proto, port string) { - rule := newRuleSpec(ingress, &proto, nil, dstSelector, &port) + rule := newRuleSpec(ingressPolicy, &proto, nil, dstSelector, &port) rules[rule.key] = rule }) } @@ -168,11 +168,11 @@ func (ns *ns) analysePolicy(policy *networkingv1.NetworkPolicy) ( } if allPorts { - rule := newRuleSpec(ingress, nil, srcSelector, dstSelector, nil) + rule := newRuleSpec(ingressPolicy, nil, srcSelector, dstSelector, nil) rules[rule.key] = rule } else { withNormalisedProtoAndPort(ingressRule.Ports, func(proto, port string) { - rule := newRuleSpec(ingress, &proto, srcSelector, dstSelector, &port) + rule := newRuleSpec(ingressPolicy, &proto, srcSelector, dstSelector, &port) rules[rule.key] = rule }) } diff --git a/npc/rule.go b/npc/rule.go index f84884da..b34ac11a 100644 --- a/npc/rule.go +++ b/npc/rule.go @@ -10,20 +10,13 @@ import ( "github.com/weaveworks/weave/npc/iptables" ) -type ruleType byte - -const ( - ingress ruleType = iota - egress -) - type ruleSpec struct { - key string - args []string - ruleType ruleType + key string + args []string + policyType policyType } -func newRuleSpec(ruleType ruleType, proto *string, srcHost *selectorSpec, dstHost *selectorSpec, dstPort *string) *ruleSpec { +func newRuleSpec(policyType policyType, proto *string, srcHost *selectorSpec, dstHost *selectorSpec, dstPort *string) *ruleSpec { args := []string{} if proto != nil { args = append(args, "-p", *proto) @@ -47,9 +40,10 @@ func newRuleSpec(ruleType ruleType, proto *string, srcHost *selectorSpec, dstHos } args = append(args, "-j", "ACCEPT") args = append(args, "-m", "comment", "--comment", fmt.Sprintf("%s -> %s", srcComment, dstComment)) + // TODO(brb) embed policyType into key key := strings.Join(args, " ") - return &ruleSpec{key, args, ruleType} + return &ruleSpec{key, args, policyType} } type ruleSet struct { @@ -66,10 +60,7 @@ func (rs *ruleSet) deprovision(user types.UID, current, desired map[string]*rule if _, found := desired[key]; !found { delete(rs.users[key], user) if len(rs.users[key]) == 0 { - chain := IngressChain - if spec.ruleType == egress { - chain = EgressCustomChain - } + chain := rulesChainByPolicyType(spec.policyType) common.Log.Infof("deleting rule %v from %q chain", spec.args, chain) if err := rs.ipt.Delete(TableFilter, chain, spec.args...); err != nil { return err @@ -86,10 +77,7 @@ func (rs *ruleSet) provision(user types.UID, current, desired map[string]*ruleSp for key, spec := range desired { if _, found := current[key]; !found { if _, found := rs.users[key]; !found { - chain := IngressChain - if spec.ruleType == egress { - chain = EgressCustomChain - } + chain := rulesChainByPolicyType(spec.policyType) common.Log.Infof("adding rule %v to %q chain", spec.args, chain) if err := rs.ipt.Append(TableFilter, chain, spec.args...); err != nil { return err @@ -102,3 +90,10 @@ func (rs *ruleSet) provision(user types.UID, current, desired map[string]*ruleSp return nil } + +func rulesChainByPolicyType(policyType policyType) string { + if policyType == egressPolicy { + return EgressCustomChain + } + return IngressChain +} diff --git a/npc/rule_test.go b/npc/rule_test.go index ab5850de..5fac10c7 100644 --- a/npc/rule_test.go +++ b/npc/rule_test.go @@ -9,7 +9,7 @@ func TestRegressionHandleNil3095(t *testing.T) { // Test for handling nil values // https://github.com/weaveworks/weave/issues/3095 - rs := newRuleSpec(ingress, nil, nil, nil, nil) + rs := newRuleSpec(ingressPolicy, nil, nil, nil, nil) require.NotEqual(t, rs.key, "") From 5e675f26f768bc61d98238b9854069e95e6156f5 Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Mon, 21 May 2018 08:40:06 +0200 Subject: [PATCH 09/46] npc: Embed policyType into rule This ensures two rules with the same rulespec but of different policyType will end up having different keys. Sharing the same key would prevent from provisioning the rules. --- npc/rule.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/npc/rule.go b/npc/rule.go index b34ac11a..f875af5e 100644 --- a/npc/rule.go +++ b/npc/rule.go @@ -39,8 +39,9 @@ func newRuleSpec(policyType policyType, proto *string, srcHost *selectorSpec, ds args = append(args, "--dport", *dstPort) } args = append(args, "-j", "ACCEPT") - args = append(args, "-m", "comment", "--comment", fmt.Sprintf("%s -> %s", srcComment, dstComment)) - // TODO(brb) embed policyType into key + // NOTE: if you remove the comment bellow, then embed `policyType` into `key`. + // Otherwise, the rule won't be provisioned if it exists for other policy type. + args = append(args, "-m", "comment", "--comment", fmt.Sprintf("%s -> %s (%s)", srcComment, dstComment, policyTypeStr(policyType))) key := strings.Join(args, " ") return &ruleSpec{key, args, policyType} From f02a11b27b83eb01deb05607e494ac9f8d88e492 Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Mon, 21 May 2018 08:50:09 +0200 Subject: [PATCH 10/46] npc: Analyse egress policy This is a copy-paste from the ingress part with small modifications. --- npc/analyser.go | 138 +++++++++++++++++++++++++++++++++--------------- 1 file changed, 95 insertions(+), 43 deletions(-) diff --git a/npc/analyser.go b/npc/analyser.go index 376ffa6b..94d45e8e 100644 --- a/npc/analyser.go +++ b/npc/analyser.go @@ -125,57 +125,109 @@ func (ns *ns) analysePolicy(policy *networkingv1.NetworkPolicy) ( // 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 { - return - } - - for _, ingressRule := range policy.Spec.Ingress { - // If Ports is empty or missing, this rule matches all ports - allPorts := ingressRule.Ports == nil || len(ingressRule.Ports) == 0 - // If From is empty or missing, this rule matches all sources - allSources := ingressRule.From == nil || len(ingressRule.From) == 0 - - if allSources { - if allPorts { - rule := newRuleSpec(ingressPolicy, nil, nil, dstSelector, nil) - rules[rule.key] = rule - } else { - withNormalisedProtoAndPort(ingressRule.Ports, func(proto, port string) { - rule := newRuleSpec(ingressPolicy, &proto, nil, dstSelector, &port) - rules[rule.key] = rule - }) - } - } else { - for _, peer := range ingressRule.From { - var srcSelector *selectorSpec - - // NetworkPolicyPeer describes a peer to allow traffic from. - // Exactly one of its fields must be specified. - if peer.PodSelector != nil { - srcSelector, err = newSelectorSpec(peer.PodSelector, false, nil, ns.name, ipset.HashIP) - if err != nil { - return nil, nil, nil, err - } - addIfNotExist(srcSelector, podSelectors) - - } else if peer.NamespaceSelector != nil { - srcSelector, err = newSelectorSpec(peer.NamespaceSelector, false, nil, "", ipset.ListSet) - if err != nil { - return nil, nil, nil, err - } - nsSelectors[srcSelector.key] = srcSelector - } + // If ingress is empty then this NetworkPolicy does not allow any ingress traffic + if policy.Spec.Ingress != nil && len(policy.Spec.Ingress) != 0 { + for _, ingressRule := range policy.Spec.Ingress { + // If Ports is empty or missing, this rule matches all ports + allPorts := ingressRule.Ports == nil || len(ingressRule.Ports) == 0 + // If From is empty or missing, this rule matches all sources + allSources := ingressRule.From == nil || len(ingressRule.From) == 0 + if allSources { if allPorts { - rule := newRuleSpec(ingressPolicy, nil, srcSelector, dstSelector, nil) + rule := newRuleSpec(ingressPolicy, nil, nil, dstSelector, nil) rules[rule.key] = rule } else { withNormalisedProtoAndPort(ingressRule.Ports, func(proto, port string) { - rule := newRuleSpec(ingressPolicy, &proto, srcSelector, dstSelector, &port) + rule := newRuleSpec(ingressPolicy, &proto, nil, dstSelector, &port) rules[rule.key] = rule }) } + } else { + for _, peer := range ingressRule.From { + var srcSelector *selectorSpec + + // NetworkPolicyPeer describes a peer to allow traffic from. + // Exactly one of its fields must be specified. + if peer.PodSelector != nil { + srcSelector, err = newSelectorSpec(peer.PodSelector, false, nil, ns.name, ipset.HashIP) + if err != nil { + return nil, nil, nil, err + } + addIfNotExist(srcSelector, podSelectors) + + } else if peer.NamespaceSelector != nil { + srcSelector, err = newSelectorSpec(peer.NamespaceSelector, false, nil, "", ipset.ListSet) + if err != nil { + return nil, nil, nil, err + } + nsSelectors[srcSelector.key] = srcSelector + } + + if allPorts { + rule := newRuleSpec(ingressPolicy, nil, srcSelector, dstSelector, nil) + rules[rule.key] = rule + } else { + withNormalisedProtoAndPort(ingressRule.Ports, func(proto, port string) { + rule := newRuleSpec(ingressPolicy, &proto, srcSelector, dstSelector, &port) + rules[rule.key] = rule + }) + } + } + } + } + } + + // If egress is empty then this NetworkPolicy does not allow any egress traffic + if policy.Spec.Egress != nil && len(policy.Spec.Egress) != 0 { + for _, egressRule := range policy.Spec.Egress { + // If Ports is empty or missing, this rule matches all ports + allPorts := egressRule.Ports == nil || len(egressRule.Ports) == 0 + // If To is empty or missing, this rule matches all destinations + allDestinations := egressRule.To == nil || len(egressRule.To) == 0 + + if allDestinations { + if allPorts { + rule := newRuleSpec(egressPolicy, nil, dstSelector, nil, nil) + rules[rule.key] = rule + } else { + withNormalisedProtoAndPort(egressRule.Ports, func(proto, port string) { + rule := newRuleSpec(egressPolicy, &proto, dstSelector, nil, &port) + rules[rule.key] = rule + }) + } + } else { + for _, peer := range egressRule.To { + // TODO(brb) s/selector/dstSelector/ after s/dstSelector/targetSelector/ + var selector *selectorSpec + + // NetworkPolicyPeer describes a peer to allow traffic from. + // Exactly one of its fields must be specified. + if peer.PodSelector != nil { + selector, err = newSelectorSpec(peer.PodSelector, false, nil, ns.name, ipset.HashIP) + if err != nil { + return nil, nil, nil, err + } + addIfNotExist(selector, podSelectors) + + } else if peer.NamespaceSelector != nil { + selector, err = newSelectorSpec(peer.NamespaceSelector, false, nil, "", ipset.ListSet) + if err != nil { + return nil, nil, nil, err + } + nsSelectors[selector.key] = selector + } + + if allPorts { + rule := newRuleSpec(egressPolicy, nil, dstSelector, selector, nil) + rules[rule.key] = rule + } else { + withNormalisedProtoAndPort(egressRule.Ports, func(proto, port string) { + rule := newRuleSpec(egressPolicy, &proto, dstSelector, selector, &port) + rules[rule.key] = rule + }) + } + } } } } From 8d1ee752d4e15c2b9e61a8a24ec3b070b4e1e139 Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Mon, 21 May 2018 09:38:28 +0200 Subject: [PATCH 11/46] npc: Rename dstSelector to targetSelector dstSelector becomes ambiguous once we introduce the egress. So, from now on, we call selector a target selector if it selects pods to which a netpol applies (aka NetworkPolicySpec.podSelector). --- npc/analyser.go | 47 ++++++++++++++++++------------------- npc/controller_test.go | 8 +++---- npc/namespace.go | 18 +++++++------- npc/selector.go | 53 +++++++++++++++++++++--------------------- 4 files changed, 62 insertions(+), 64 deletions(-) diff --git a/npc/analyser.go b/npc/analyser.go index 94d45e8e..51b64e81 100644 --- a/npc/analyser.go +++ b/npc/analyser.go @@ -25,11 +25,11 @@ func (ns *ns) analysePolicyLegacy(policy *extnapi.NetworkPolicy) ( rules = make(map[string]*ruleSpec) policyTypes := []policyType{ingressPolicy} - dstSelector, err := newSelectorSpec(&policy.Spec.PodSelector, true, policyTypes, ns.name, ipset.HashIP) + targetSelector, err := newSelectorSpec(&policy.Spec.PodSelector, true, policyTypes, ns.name, ipset.HashIP) if err != nil { return nil, nil, nil, err } - podSelectors[dstSelector.key] = dstSelector + podSelectors[targetSelector.key] = targetSelector for _, ingressRule := range policy.Spec.Ingress { if ingressRule.Ports != nil && len(ingressRule.Ports) == 0 { @@ -46,13 +46,13 @@ func (ns *ns) analysePolicyLegacy(policy *extnapi.NetworkPolicy) ( // From is not provided, this rule matches all sources (traffic not restricted by source). if ingressRule.Ports == nil { // Ports is not provided, this rule matches all ports (traffic not restricted by port). - rule := newRuleSpec(ingressPolicy, nil, nil, dstSelector, nil) + rule := newRuleSpec(ingressPolicy, nil, nil, targetSelector, nil) rules[rule.key] = rule } else { // Ports is present and contains at least one item, then this rule allows traffic // only if the traffic matches at least one port in the ports list. withNormalisedProtoAndPortLegacy(ingressRule.Ports, func(proto, port string) { - rule := newRuleSpec(ingressPolicy, &proto, nil, dstSelector, &port) + rule := newRuleSpec(ingressPolicy, &proto, nil, targetSelector, &port) rules[rule.key] = rule }) } @@ -78,13 +78,13 @@ func (ns *ns) analysePolicyLegacy(policy *extnapi.NetworkPolicy) ( if ingressRule.Ports == nil { // Ports is not provided, this rule matches all ports (traffic not restricted by port). - rule := newRuleSpec(ingressPolicy, nil, srcSelector, dstSelector, nil) + rule := newRuleSpec(ingressPolicy, nil, srcSelector, targetSelector, nil) rules[rule.key] = rule } else { // Ports is present and contains at least one item, then this rule allows traffic // only if the traffic matches at least one port in the ports list. withNormalisedProtoAndPortLegacy(ingressRule.Ports, func(proto, port string) { - rule := newRuleSpec(ingressPolicy, &proto, srcSelector, dstSelector, &port) + rule := newRuleSpec(ingressPolicy, &proto, srcSelector, targetSelector, &port) rules[rule.key] = rule }) } @@ -114,16 +114,16 @@ func (ns *ns) analysePolicy(policy *networkingv1.NetworkPolicy) ( } } // If empty, matches all pods in a namespace - dstSelector, err := newSelectorSpec(&policy.Spec.PodSelector, true, policyTypes, ns.name, ipset.HashIP) + targetSelector, err := newSelectorSpec(&policy.Spec.PodSelector, true, policyTypes, ns.name, ipset.HashIP) if err != nil { return nil, nil, nil, err } - // To prevent dstSelector being overwritten by a subsequent selector with + // To prevent targetSelector 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 + // Otherwise, targetSelector' "dst" property might be lost resulting in // an invalid content of the "default-allow" ipset. - addIfNotExist(dstSelector, podSelectors) + addIfNotExist(targetSelector, podSelectors) // If ingress is empty then this NetworkPolicy does not allow any ingress traffic if policy.Spec.Ingress != nil && len(policy.Spec.Ingress) != 0 { @@ -135,11 +135,11 @@ func (ns *ns) analysePolicy(policy *networkingv1.NetworkPolicy) ( if allSources { if allPorts { - rule := newRuleSpec(ingressPolicy, nil, nil, dstSelector, nil) + rule := newRuleSpec(ingressPolicy, nil, nil, targetSelector, nil) rules[rule.key] = rule } else { withNormalisedProtoAndPort(ingressRule.Ports, func(proto, port string) { - rule := newRuleSpec(ingressPolicy, &proto, nil, dstSelector, &port) + rule := newRuleSpec(ingressPolicy, &proto, nil, targetSelector, &port) rules[rule.key] = rule }) } @@ -165,11 +165,11 @@ func (ns *ns) analysePolicy(policy *networkingv1.NetworkPolicy) ( } if allPorts { - rule := newRuleSpec(ingressPolicy, nil, srcSelector, dstSelector, nil) + rule := newRuleSpec(ingressPolicy, nil, srcSelector, targetSelector, nil) rules[rule.key] = rule } else { withNormalisedProtoAndPort(ingressRule.Ports, func(proto, port string) { - rule := newRuleSpec(ingressPolicy, &proto, srcSelector, dstSelector, &port) + rule := newRuleSpec(ingressPolicy, &proto, srcSelector, targetSelector, &port) rules[rule.key] = rule }) } @@ -188,42 +188,41 @@ func (ns *ns) analysePolicy(policy *networkingv1.NetworkPolicy) ( if allDestinations { if allPorts { - rule := newRuleSpec(egressPolicy, nil, dstSelector, nil, nil) + rule := newRuleSpec(egressPolicy, nil, targetSelector, nil, nil) rules[rule.key] = rule } else { withNormalisedProtoAndPort(egressRule.Ports, func(proto, port string) { - rule := newRuleSpec(egressPolicy, &proto, dstSelector, nil, &port) + rule := newRuleSpec(egressPolicy, &proto, targetSelector, nil, &port) rules[rule.key] = rule }) } } else { for _, peer := range egressRule.To { - // TODO(brb) s/selector/dstSelector/ after s/dstSelector/targetSelector/ - var selector *selectorSpec + var dstSelector *selectorSpec // NetworkPolicyPeer describes a peer to allow traffic from. // Exactly one of its fields must be specified. if peer.PodSelector != nil { - selector, err = newSelectorSpec(peer.PodSelector, false, nil, ns.name, ipset.HashIP) + dstSelector, err = newSelectorSpec(peer.PodSelector, false, nil, ns.name, ipset.HashIP) if err != nil { return nil, nil, nil, err } - addIfNotExist(selector, podSelectors) + addIfNotExist(dstSelector, podSelectors) } else if peer.NamespaceSelector != nil { - selector, err = newSelectorSpec(peer.NamespaceSelector, false, nil, "", ipset.ListSet) + dstSelector, err = newSelectorSpec(peer.NamespaceSelector, false, nil, "", ipset.ListSet) if err != nil { return nil, nil, nil, err } - nsSelectors[selector.key] = selector + nsSelectors[dstSelector.key] = dstSelector } if allPorts { - rule := newRuleSpec(egressPolicy, nil, dstSelector, selector, nil) + rule := newRuleSpec(egressPolicy, nil, targetSelector, dstSelector, nil) rules[rule.key] = rule } else { withNormalisedProtoAndPort(egressRule.Ports, func(proto, port string) { - rule := newRuleSpec(egressPolicy, &proto, dstSelector, selector, &port) + rule := newRuleSpec(egressPolicy, &proto, targetSelector, dstSelector, &port) rules[rule.key] = rule }) } diff --git a/npc/controller_test.go b/npc/controller_test.go index 1f7b9a79..17001e86 100644 --- a/npc/controller_test.go +++ b/npc/controller_test.go @@ -386,7 +386,7 @@ func TestOutOfOrderPodEvents(t *testing.T) { } // Test case for https://github.com/weaveworks/weave/issues/3222 -func TestNewDstSelector(t *testing.T) { +func TestNewTargetSelector(t *testing.T) { const ( ingressDefaultAllowIPSetName = "weave-;rGqyMIl1HN^cfDki~Z$3]6!N" podIP = "10.32.0.10" @@ -431,7 +431,7 @@ func TestNewDstSelector(t *testing.T) { } controller.AddNetworkPolicy(netpolBar) - // netpolBar dst selector selects podFoo + // netpolBar target selector selects podFoo require.False(t, m.entryExists(ingressDefaultAllowIPSetName, podIP)) netpolFoo := &networkingv1.NetworkPolicy{ @@ -453,9 +453,9 @@ func TestNewDstSelector(t *testing.T) { controller.AddNetworkPolicy(netpolFoo) controller.DeleteNetworkPolicy(netpolBar) - // netpolFoo dst-selects podFoo + // netpolFoo target-selects podFoo require.False(t, m.entryExists(ingressDefaultAllowIPSetName, podIP)) controller.DeleteNetworkPolicy(netpolFoo) - // No netpol dst-selects podFoo + // No netpol target-selects podFoo require.True(t, m.entryExists(ingressDefaultAllowIPSetName, podIP)) } diff --git a/npc/namespace.go b/npc/namespace.go index 41fb6af3..80299d9f 100644 --- a/npc/namespace.go +++ b/npc/namespace.go @@ -32,8 +32,8 @@ type ns struct { uid types.UID // surrogate UID to own allPods selector allPods *selectorSpec // hash:ip ipset of all pod IPs in this namespace - // stores IP addrs of pods which are not selected by any dst podSelector of - // any netpol; used only in non-legacy mode and is used as a dst in + // stores IP addrs of pods which are not selected by any target podSelector of + // any netpol; used only in non-legacy mode and is used as a target in // the WEAVE-NPC-{INGRESS,EGRESS}-DEFAULT iptables chain. ingressDefaultAllowIPSet ipset.Name egressDefaultAllowIPSet ipset.Name @@ -64,7 +64,7 @@ func newNS(name, nodeName string, legacy bool, ipt iptables.Interface, ips ipset rules: newRuleSet(ipt), legacy: legacy} - ns.podSelectors = newSelectorSet(ips, ns.onNewPodSelector, ns.onNewDstPodSelector, ns.onDestroyDstPodSelector) + ns.podSelectors = newSelectorSet(ips, ns.onNewPodSelector, ns.onNewTargetPodSelector, ns.onDestroyTargetPodSelector) if !legacy { ingressDefaultAllowIPSet := ipset.Name(IpsetNamePrefix + shortName("ingress-default-allow:"+name)) @@ -110,7 +110,7 @@ func (ns *ns) onNewPodSelector(selector *selector) error { return nil } -func (ns *ns) onNewDstPodSelector(selector *selector, policyType policyType) error { +func (ns *ns) onNewTargetPodSelector(selector *selector, policyType policyType) error { if ns.legacy { return nil } @@ -130,7 +130,7 @@ func (ns *ns) onNewDstPodSelector(selector *selector, policyType policyType) err return nil } -func (ns *ns) onDestroyDstPodSelector(selector *selector, policyType policyType) error { +func (ns *ns) onDestroyTargetPodSelector(selector *selector, policyType policyType) error { if ns.legacy { return nil } @@ -148,12 +148,12 @@ func (ns *ns) onDestroyDstPodSelector(selector *selector, policyType policyType) return nil } -// Add pod IP addr to default-allow ipset if there are no matching dst selectors +// Add pod IP addr to default-allow ipset if there are no matching target selectors func (ns *ns) addToDefaultAllowIfNoMatching(pod *coreapi.Pod, policyType policyType) error { found := false // TODO(mp) optimize (avoid iterating over selectors) by ref counting IP addrs. for _, s := range ns.podSelectors.entries { - if ns.podSelectors.dstSelectorExist(s, policyType) && s.matches(pod.ObjectMeta.Labels) { + if ns.podSelectors.targetSelectorExist(s, policyType) && s.matches(pod.ObjectMeta.Labels) { found = true break } @@ -189,7 +189,7 @@ func (ns *ns) addPod(obj *coreapi.Pod) error { if err != nil { return err } - // If there are no matching dst selectors, add the pod to default-allow + // If there are no matching target selectors, add the pod to default-allow if !ns.legacy { if !foundIngress { if err := ns.ips.AddEntry(obj.ObjectMeta.UID, ns.ingressDefaultAllowIPSet, obj.Status.PodIP, podComment(obj)); err != nil { @@ -301,7 +301,7 @@ func (ns *ns) updatePod(oldObj, newObj *coreapi.Pod) error { func (ns *ns) addOrRemoveToDefaultAllowIPSet(ps *selector, oldObj, newObj *coreapi.Pod, oldMatch, newMatch bool, policyType policyType) error { ipset := ns.defaultAllowIPSetName(policyType) - if ns.podSelectors.dstSelectorExist(ps, policyType) { + if ns.podSelectors.targetSelectorExist(ps, policyType) { switch { case !oldMatch && newMatch: if err := ns.ips.DelEntry(oldObj.ObjectMeta.UID, ipset, oldObj.Status.PodIP); err != nil { diff --git a/npc/selector.go b/npc/selector.go index 1b080989..763191c1 100644 --- a/npc/selector.go +++ b/npc/selector.go @@ -13,7 +13,7 @@ type selectorSpec struct { key string // string representation (for hash keying/equality comparison) selector labels.Selector // k8s Selector object (for matching) dst bool // destination selector TODO(brb) not relevant - policyTypes []policyType // TODO(brb) repharse: netpol types (egress, ingress or both) which initiated the dst selector + policyTypes []policyType // TODO(brb) repharse: netpol types (egress, ingress or both) which initiated the target selector ipsetType ipset.Type // type of ipset to provision ipsetName ipset.Name // generated ipset name @@ -63,30 +63,29 @@ type selectorSet struct { ips ipset.Interface onNewSelector selectorFn - // invoked after dst selector has been provisioned for the first time - onNewDstSelector selectorWithPolicyTypeFn - // invoked after the last instance of dst selector has been deprovisioned - onDestroyDstSelector selectorWithPolicyTypeFn + // invoked after target selector has been provisioned for the first time + onNewTargetSelector selectorWithPolicyTypeFn + // invoked after the last instance of target selector has been deprovisioned + onDestroyTargetSelector selectorWithPolicyTypeFn users map[string]map[types.UID]struct{} // list of users per selector entries map[string]*selector - // We need to keep track of dst selector instances to be able to invoke - // onNewDstSelector and onDestroyDstSelector callbacks at the right time; + // We need to keep track of target selector instances to be able to invoke + // onNewTargetSelector and onDestroyTargetSelector callbacks at the right time; // selectorSpec.Key -> policyType -> count - // TODO(brb) dst -> target? - dstSelectorsCount map[string]map[policyType]int + targetSelectorsCount map[string]map[policyType]int } -func newSelectorSet(ips ipset.Interface, onNewSelector selectorFn, onNewDstSelector, onDestroyDstSelector selectorWithPolicyTypeFn) *selectorSet { +func newSelectorSet(ips ipset.Interface, onNewSelector selectorFn, onNewTargetSelector, onDestroyTargetSelector selectorWithPolicyTypeFn) *selectorSet { return &selectorSet{ - ips: ips, - onNewSelector: onNewSelector, - onNewDstSelector: onNewDstSelector, - onDestroyDstSelector: onDestroyDstSelector, + ips: ips, + onNewSelector: onNewSelector, + onNewTargetSelector: onNewTargetSelector, + onDestroyTargetSelector: onDestroyTargetSelector, users: make(map[string]map[types.UID]struct{}), entries: make(map[string]*selector), - dstSelectorsCount: make(map[string]map[policyType]int)} + targetSelectorsCount: make(map[string]map[policyType]int)} } func (ss *selectorSet) addToMatching(user types.UID, labelMap map[string]string, entry string, comment string) (bool, bool, error) { @@ -94,10 +93,10 @@ func (ss *selectorSet) addToMatching(user types.UID, labelMap map[string]string, foundEgress := false for _, s := range ss.entries { if s.matches(labelMap) { - if ss.dstSelectorExist(s, ingressPolicy) { + if ss.targetSelectorExist(s, ingressPolicy) { foundIngress = true } - if ss.dstSelectorExist(s, egressPolicy) { + if ss.targetSelectorExist(s, egressPolicy) { foundEgress = true } if err := s.addEntry(user, entry, comment); err != nil { @@ -119,8 +118,8 @@ func (ss *selectorSet) delFromMatching(user types.UID, labelMap map[string]strin return nil } -func (ss *selectorSet) dstSelectorExist(s *selector, policyType policyType) bool { - return ss.dstSelectorsCount[s.spec.key][policyType] > 0 +func (ss *selectorSet) targetSelectorExist(s *selector, policyType policyType) bool { + return ss.targetSelectorsCount[s.spec.key][policyType] > 0 } func (ss *selectorSet) deprovision(user types.UID, current, desired map[string]*selectorSpec) error { @@ -138,9 +137,9 @@ func (ss *selectorSet) deprovision(user types.UID, current, desired map[string]* } for _, policyType := range spec.policyTypes { - ss.dstSelectorsCount[key][policyType]-- - if ss.dstSelectorsCount[key][policyType] == 0 { - if err := ss.onDestroyDstSelector(&selector{ss.ips, spec}, policyType); err != nil { + ss.targetSelectorsCount[key][policyType]-- + if ss.targetSelectorsCount[key][policyType] == 0 { + if err := ss.onDestroyTargetSelector(&selector{ss.ips, spec}, policyType); err != nil { return err } } @@ -170,12 +169,12 @@ func (ss *selectorSet) provision(user types.UID, current, desired map[string]*se ss.users[key][user] = struct{}{} for _, pt := range spec.policyTypes { - if _, found := ss.dstSelectorsCount[key]; !found { - ss.dstSelectorsCount[key] = make(map[policyType]int) + if _, found := ss.targetSelectorsCount[key]; !found { + ss.targetSelectorsCount[key] = make(map[policyType]int) } - ss.dstSelectorsCount[key][pt]++ - if ss.dstSelectorsCount[key][pt] == 1 { - if err := ss.onNewDstSelector(selector, pt); err != nil { + ss.targetSelectorsCount[key][pt]++ + if ss.targetSelectorsCount[key][pt] == 1 { + if err := ss.onNewTargetSelector(selector, pt); err != nil { return err } } From 8904a4b14494df70217a076c74f96dd6c673876d Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Mon, 21 May 2018 09:54:43 +0200 Subject: [PATCH 12/46] npc: Get rid of selectorSpec.target We treat selectorSpec as a target selector if len(selectorSpec.policyTypes) > 0. --- npc/analyser.go | 16 ++++++++-------- npc/namespace.go | 2 +- npc/selector.go | 6 ++---- 3 files changed, 11 insertions(+), 13 deletions(-) diff --git a/npc/analyser.go b/npc/analyser.go index 51b64e81..7fb3313c 100644 --- a/npc/analyser.go +++ b/npc/analyser.go @@ -25,7 +25,7 @@ func (ns *ns) analysePolicyLegacy(policy *extnapi.NetworkPolicy) ( rules = make(map[string]*ruleSpec) policyTypes := []policyType{ingressPolicy} - targetSelector, err := newSelectorSpec(&policy.Spec.PodSelector, true, policyTypes, ns.name, ipset.HashIP) + targetSelector, err := newSelectorSpec(&policy.Spec.PodSelector, policyTypes, ns.name, ipset.HashIP) if err != nil { return nil, nil, nil, err } @@ -62,14 +62,14 @@ func (ns *ns) analysePolicyLegacy(policy *extnapi.NetworkPolicy) ( for _, peer := range ingressRule.From { var srcSelector *selectorSpec if peer.PodSelector != nil { - srcSelector, err = newSelectorSpec(peer.PodSelector, false, nil, ns.name, ipset.HashIP) + srcSelector, err = newSelectorSpec(peer.PodSelector, nil, ns.name, ipset.HashIP) if err != nil { return nil, nil, nil, err } podSelectors[srcSelector.key] = srcSelector } if peer.NamespaceSelector != nil { - srcSelector, err = newSelectorSpec(peer.NamespaceSelector, false, nil, "", ipset.ListSet) + srcSelector, err = newSelectorSpec(peer.NamespaceSelector, nil, "", ipset.ListSet) if err != nil { return nil, nil, nil, err } @@ -114,7 +114,7 @@ func (ns *ns) analysePolicy(policy *networkingv1.NetworkPolicy) ( } } // If empty, matches all pods in a namespace - targetSelector, err := newSelectorSpec(&policy.Spec.PodSelector, true, policyTypes, ns.name, ipset.HashIP) + targetSelector, err := newSelectorSpec(&policy.Spec.PodSelector, policyTypes, ns.name, ipset.HashIP) if err != nil { return nil, nil, nil, err } @@ -150,14 +150,14 @@ func (ns *ns) analysePolicy(policy *networkingv1.NetworkPolicy) ( // NetworkPolicyPeer describes a peer to allow traffic from. // Exactly one of its fields must be specified. if peer.PodSelector != nil { - srcSelector, err = newSelectorSpec(peer.PodSelector, false, nil, ns.name, ipset.HashIP) + srcSelector, err = newSelectorSpec(peer.PodSelector, nil, ns.name, ipset.HashIP) if err != nil { return nil, nil, nil, err } addIfNotExist(srcSelector, podSelectors) } else if peer.NamespaceSelector != nil { - srcSelector, err = newSelectorSpec(peer.NamespaceSelector, false, nil, "", ipset.ListSet) + srcSelector, err = newSelectorSpec(peer.NamespaceSelector, nil, "", ipset.ListSet) if err != nil { return nil, nil, nil, err } @@ -203,14 +203,14 @@ func (ns *ns) analysePolicy(policy *networkingv1.NetworkPolicy) ( // NetworkPolicyPeer describes a peer to allow traffic from. // Exactly one of its fields must be specified. if peer.PodSelector != nil { - dstSelector, err = newSelectorSpec(peer.PodSelector, false, nil, ns.name, ipset.HashIP) + dstSelector, err = newSelectorSpec(peer.PodSelector, nil, ns.name, ipset.HashIP) if err != nil { return nil, nil, nil, err } addIfNotExist(dstSelector, podSelectors) } else if peer.NamespaceSelector != nil { - dstSelector, err = newSelectorSpec(peer.NamespaceSelector, false, nil, "", ipset.ListSet) + dstSelector, err = newSelectorSpec(peer.NamespaceSelector, nil, "", ipset.ListSet) if err != nil { return nil, nil, nil, err } diff --git a/npc/namespace.go b/npc/namespace.go index 80299d9f..780f1174 100644 --- a/npc/namespace.go +++ b/npc/namespace.go @@ -46,7 +46,7 @@ type ns struct { } func newNS(name, nodeName string, legacy bool, ipt iptables.Interface, ips ipset.Interface, nsSelectors *selectorSet) (*ns, error) { - allPods, err := newSelectorSpec(&metav1.LabelSelector{}, false, nil, name, ipset.HashIP) + allPods, err := newSelectorSpec(&metav1.LabelSelector{}, nil, name, ipset.HashIP) if err != nil { return nil, err } diff --git a/npc/selector.go b/npc/selector.go index 763191c1..4affe0ca 100644 --- a/npc/selector.go +++ b/npc/selector.go @@ -12,15 +12,14 @@ import ( type selectorSpec struct { key string // string representation (for hash keying/equality comparison) selector labels.Selector // k8s Selector object (for matching) - dst bool // destination selector TODO(brb) not relevant - policyTypes []policyType // TODO(brb) repharse: netpol types (egress, ingress or both) which initiated the target selector + policyTypes []policyType // If non-empty, then selectorSpec is a target selector for given policyTypes. ipsetType ipset.Type // type of ipset to provision ipsetName ipset.Name // generated ipset name nsName string // Namespace name } -func newSelectorSpec(json *metav1.LabelSelector, dst bool, policyType []policyType, nsName string, ipsetType ipset.Type) (*selectorSpec, error) { +func newSelectorSpec(json *metav1.LabelSelector, policyType []policyType, nsName string, ipsetType ipset.Type) (*selectorSpec, error) { selector, err := metav1.LabelSelectorAsSelector(json) if err != nil { return nil, err @@ -29,7 +28,6 @@ func newSelectorSpec(json *metav1.LabelSelector, dst bool, policyType []policyTy return &selectorSpec{ key: key, selector: selector, - dst: dst, policyTypes: policyType, // We prefix the selector string with the namespace name when generating // the shortname because you can specify the same selector in multiple From 688328dc5793c7082925d44cae4110d92e2e3a19 Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Mon, 21 May 2018 15:35:16 +0200 Subject: [PATCH 13/46] npc: Steer traffic for egress processing We need two rules: * `-A INPUT -i weave -j WEAVE-NPC-EGRESS`: applies to packets destined to a local node. * `-A FORWARD -i weave -j WEAVE-NPC-EGRESS`: the rest. It can match an ingress packet. --- net/bridge.go | 17 +++++++++++++++-- weave | 4 ++++ 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/net/bridge.go b/net/bridge.go index 2f6d641f..07567a15 100644 --- a/net/bridge.go +++ b/net/bridge.go @@ -457,11 +457,24 @@ func configureIPTables(config *BridgeConfig, ips ipset.Interface) error { } if config.NPC { - // Steer traffic via the NPC - _ = ipt.NewChain("filter", "WEAVE-NPC") // ignore error because it might already exist + // Steer traffic via the NPC. + // Ignore error because chains might already exist + // TODO(brb): use constants for WEAVE-NPC* chains + _ = ipt.NewChain("filter", "WEAVE-NPC") + _ = ipt.NewChain("filter", "WEAVE-NPC-EGRESS") + + // TODO(brb): this rule might slow down packet processing; consider removing at it applies only + // to node-local traffic. + if err = ipt.AppendUnique("filter", "INPUT", "-i", config.WeaveBridgeName, "-j", "WEAVE-NPC-EGRESS"); err != nil { + return err + } + // If WEAVE-NPC chain doesn't exist then creating a rule in the chain will fail fwdRules = append(fwdRules, [][]string{ + {"-i", config.WeaveBridgeName, + "-m", "comment", "--comment", "NOTE: this must go before '-j KUBE-FORWARD'", + "-j", "WEAVE-NPC-EGRESS"}, {"-o", config.WeaveBridgeName, "-m", "comment", "--comment", "NOTE: this must go before '-j KUBE-FORWARD'", "-j", "WEAVE-NPC"}, diff --git a/weave b/weave index 56b42719..599afb94 100755 --- a/weave +++ b/weave @@ -496,6 +496,10 @@ destroy_bridge() { run_iptables -t filter -D FORWARD -o $BRIDGE -m state --state NEW -j NFLOG --nflog-group 86 2>/dev/null || true run_iptables -t filter -D FORWARD -o $BRIDGE -j DROP 2>/dev/null || true run_iptables -X WEAVE-NPC >/dev/null 2>&1 || true + run_iptables -F WEAVE-NPC-EGRESS >/dev/null 2>&1 || true + run_iptables -t filter -D FORWARD -o $BRIDGE -j WEAVE-NPC-EGRESS 2>/dev/null || true + run_iptables -t filter -D INPUT -i $BRIDGE -j WEAVE-NPC-EGRESS 2>/dev/null || true + run_iptables -X WEAVE-NPC-EGRESS >/dev/null 2>&1 || true run_iptables -F WEAVE-EXPOSE >/dev/null 2>&1 || true run_iptables -t filter -D FORWARD -o $BRIDGE -j WEAVE-EXPOSE 2>/dev/null || true From 9769c8d5b905808e0363f42506c4af19c275708e Mon Sep 17 00:00:00 2001 From: Kitt Hsu Date: Wed, 16 May 2018 16:58:18 +0800 Subject: [PATCH 14/46] support ipblock in npc NOTE(brb): This commit is cherry-picked from the kitt1987/ipblock branch. --- npc/ipblock.go | 120 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 120 insertions(+) create mode 100644 npc/ipblock.go diff --git a/npc/ipblock.go b/npc/ipblock.go new file mode 100644 index 00000000..fd5df974 --- /dev/null +++ b/npc/ipblock.go @@ -0,0 +1,120 @@ +package npc + +import ( + "github.com/weaveworks/weave/common" + "github.com/weaveworks/weave/npc/ipset" + networkingv1 "k8s.io/api/networking/v1" + "k8s.io/apimachinery/pkg/types" + "net" + "sort" + "strings" +) + +type ipBlock struct { + ipsetName ipset.Name // generated ipset name + spec *networkingv1.IPBlock +} + +func newIPBlock(ipb *networkingv1.IPBlock, ns string) (b *ipBlock, err error) { + if _, _, err = net.ParseCIDR(ipb.CIDR); err != nil { + return + } + + for _, ex := range ipb.Except { + if _, _, err = net.ParseCIDR(ex); err != nil { + return + } + } + + b = &ipBlock{spec: ipb} + + if len(ipb.Except) > 0 { + sortedExcept := ipb.Except + sort.Strings(sortedExcept) + b.ipsetName = ipset.Name(IpsetNamePrefix + shortName("except:"+strings.Join(sortedExcept, ""))) + } + + return +} + +//func (ipb ipBlock) GetRuleSpec() (spec *ruleSpec) { +// spec = &ruleSpec{ +// chain: IngressIPBlockChain, +// } +// +// if len(ipb.spec.Except) == 0 { +// spec.args = append(spec.args, "-s", ipb.spec.CIDR) +// spec.comment = fmt.Sprintf("cidr: %s", ipb.spec.CIDR) +// } else { +// spec.args = append(spec.args, "-s", ipb.spec.CIDR, "-m", "set", "!", "--match-set", +// string(ipb.ipsetName), "src") +// spec.comment = fmt.Sprintf("cidr: %s except [%s]", ipb.spec.CIDR, +// strings.Join(ipb.spec.Except, `,`)) +// } +// return +//} + +type ipBlockSet struct { + ips ipset.Interface + users map[string]int +} + +func newIPBlockSet(ips ipset.Interface) *ipBlockSet { + return &ipBlockSet{ + ips: ips, + users: make(map[string]int), + } +} + +func (s *ipBlockSet) deprovision(uid types.UID, current *ipBlock) (err error) { + if current == nil || len(current.ipsetName) == 0 { + return + } + + key := string(current.ipsetName) + if _, found := s.users[key]; !found { + return + } + + s.users[key]-- + if s.users[key] == 0 { + if err = s.ips.Destroy(current.ipsetName); err != nil { + return + } + + delete(s.users, key) + } + + return +} + +func (s *ipBlockSet) provision(uid types.UID, desired *ipBlock) (err error) { + if desired == nil || len(desired.ipsetName) == 0 { + return + } + + key := string(desired.ipsetName) + if _, found := s.users[key]; found { + common.Log.Debug() + s.users[key]++ + return + } + + err = s.ips.Create(desired.ipsetName, ipset.HashNet) + if err != nil { + return + } + + comment := `excepted from ` + desired.spec.CIDR + for _, excepted := range desired.spec.Except { + if err = s.ips.AddEntry(uid, desired.ipsetName, excepted, comment); err != nil { + if details := s.ips.Destroy(desired.ipsetName); details != nil { + common.Log.Warn("can't destroy ipset ", desired.ipsetName, ":", details) + } + return + } + } + + s.users[key] = 1 + return +} From 9198bb5a68ebcdbf890cceb22c336579c360e6b0 Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Tue, 22 May 2018 10:59:05 +0200 Subject: [PATCH 15/46] npc: Refactor npc/ipblock.go - No need to validate networkingv1.IPBlock fields as k8s does it. - Fix provisioning. --- npc/ipblock.go | 133 +++++++++++++++++++++++-------------------------- 1 file changed, 63 insertions(+), 70 deletions(-) diff --git a/npc/ipblock.go b/npc/ipblock.go index fd5df974..594af2b4 100644 --- a/npc/ipblock.go +++ b/npc/ipblock.go @@ -1,40 +1,32 @@ package npc import ( - "github.com/weaveworks/weave/common" - "github.com/weaveworks/weave/npc/ipset" - networkingv1 "k8s.io/api/networking/v1" - "k8s.io/apimachinery/pkg/types" - "net" "sort" "strings" + + networkingv1 "k8s.io/api/networking/v1" + "k8s.io/apimachinery/pkg/types" + + "github.com/weaveworks/weave/common" + "github.com/weaveworks/weave/npc/ipset" ) -type ipBlock struct { - ipsetName ipset.Name // generated ipset name - spec *networkingv1.IPBlock +type ipBlockSpec struct { + key string + ipsetName ipset.Name // ipset for storing excepted CIDRs + ipBlock *networkingv1.IPBlock } -func newIPBlock(ipb *networkingv1.IPBlock, ns string) (b *ipBlock, err error) { - if _, _, err = net.ParseCIDR(ipb.CIDR); err != nil { - return - } - - for _, ex := range ipb.Except { - if _, _, err = net.ParseCIDR(ex); err != nil { - return - } - } - - b = &ipBlock{spec: ipb} +func newIPBlockSpec(ipb *networkingv1.IPBlock) *ipBlockSpec { + spec := &ipBlockSpec{ipBlock: ipb} if len(ipb.Except) > 0 { - sortedExcept := ipb.Except - sort.Strings(sortedExcept) - b.ipsetName = ipset.Name(IpsetNamePrefix + shortName("except:"+strings.Join(sortedExcept, ""))) + sort.Strings(ipb.Except) + spec.key = strings.Join(ipb.Except, " ") + spec.ipsetName = ipset.Name(IpsetNamePrefix + shortName("ipblock-except:"+spec.key)) } - return + return spec } //func (ipb ipBlock) GetRuleSpec() (spec *ruleSpec) { @@ -56,65 +48,66 @@ func newIPBlock(ipb *networkingv1.IPBlock, ns string) (b *ipBlock, err error) { type ipBlockSet struct { ips ipset.Interface - users map[string]int + users map[string]map[types.UID]struct{} } func newIPBlockSet(ips ipset.Interface) *ipBlockSet { return &ipBlockSet{ ips: ips, - users: make(map[string]int), + users: make(map[string]map[types.UID]struct{}), } } -func (s *ipBlockSet) deprovision(uid types.UID, current *ipBlock) (err error) { - if current == nil || len(current.ipsetName) == 0 { - return - } - - key := string(current.ipsetName) - if _, found := s.users[key]; !found { - return - } - - s.users[key]-- - if s.users[key] == 0 { - if err = s.ips.Destroy(current.ipsetName); err != nil { - return +func (s *ipBlockSet) deprovision(user types.UID, current, desired map[string]*ipBlockSpec) error { + for key, spec := range current { + if key == "" { + continue } - delete(s.users, key) - } + if _, found := desired[key]; !found { + delete(s.users[key], user) + if len(s.users[key]) == 0 { + common.Log.Infof("destroying ipset: %#v", spec) + if err := s.ips.Destroy(spec.ipsetName); err != nil { + return err + } - return -} - -func (s *ipBlockSet) provision(uid types.UID, desired *ipBlock) (err error) { - if desired == nil || len(desired.ipsetName) == 0 { - return - } - - key := string(desired.ipsetName) - if _, found := s.users[key]; found { - common.Log.Debug() - s.users[key]++ - return - } - - err = s.ips.Create(desired.ipsetName, ipset.HashNet) - if err != nil { - return - } - - comment := `excepted from ` + desired.spec.CIDR - for _, excepted := range desired.spec.Except { - if err = s.ips.AddEntry(uid, desired.ipsetName, excepted, comment); err != nil { - if details := s.ips.Destroy(desired.ipsetName); details != nil { - common.Log.Warn("can't destroy ipset ", desired.ipsetName, ":", details) + delete(s.users, key) } - return } } - s.users[key] = 1 - return + return nil +} + +func (s *ipBlockSet) provision(user types.UID, current, desired map[string]*ipBlockSpec) (err error) { + for key, spec := range desired { + if key == "" { + // No need to provision an ipBlock with empty list of excepted CIDRs + // (i.e. no ipset is needed for the related iptables rule). + continue + } + + if _, found := current[key]; !found { + if _, found := s.users[key]; !found { + common.Log.Infof("creating ipset: %#v", spec) + if err := s.ips.Create(spec.ipsetName, ipset.HashNet); err != nil { + return err + } + + // TODO(brb) Pass comment to ips.Create instead. + comment := "excepted from " + spec.ipBlock.CIDR + for _, cidr := range spec.ipBlock.Except { + if err = s.ips.AddEntry(user, spec.ipsetName, cidr, comment); err != nil { + return err + } + } + + s.users[key] = make(map[types.UID]struct{}) + } + s.users[key][user] = struct{}{} + } + } + + return nil } From affdc6f5afd52d18f67ed6844ab02a86a3c14588 Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Tue, 22 May 2018 11:18:17 +0200 Subject: [PATCH 16/46] npc: Provision networkingv1.IPBlock (only for Egress) --- npc/analyser.go | 17 +++++++++++------ npc/namespace.go | 31 +++++++++++++++++++++++++------ 2 files changed, 36 insertions(+), 12 deletions(-) diff --git a/npc/analyser.go b/npc/analyser.go index 7fb3313c..8b825cdc 100644 --- a/npc/analyser.go +++ b/npc/analyser.go @@ -98,10 +98,12 @@ func (ns *ns) analysePolicyLegacy(policy *extnapi.NetworkPolicy) ( func (ns *ns) analysePolicy(policy *networkingv1.NetworkPolicy) ( rules map[string]*ruleSpec, nsSelectors, podSelectors map[string]*selectorSpec, + ipBlocks map[string]*ipBlockSpec, err error) { nsSelectors = make(map[string]*selectorSpec) podSelectors = make(map[string]*selectorSpec) + ipBlocks = make(map[string]*ipBlockSpec) rules = make(map[string]*ruleSpec) policyTypes := make([]policyType, 0) @@ -116,7 +118,7 @@ func (ns *ns) analysePolicy(policy *networkingv1.NetworkPolicy) ( // If empty, matches all pods in a namespace targetSelector, err := newSelectorSpec(&policy.Spec.PodSelector, policyTypes, ns.name, ipset.HashIP) if err != nil { - return nil, nil, nil, err + return nil, nil, nil, nil, err } // To prevent targetSelector being overwritten by a subsequent selector with @@ -152,14 +154,14 @@ func (ns *ns) analysePolicy(policy *networkingv1.NetworkPolicy) ( if peer.PodSelector != nil { srcSelector, err = newSelectorSpec(peer.PodSelector, nil, ns.name, ipset.HashIP) if err != nil { - return nil, nil, nil, err + return nil, nil, nil, nil, err } addIfNotExist(srcSelector, podSelectors) } else if peer.NamespaceSelector != nil { srcSelector, err = newSelectorSpec(peer.NamespaceSelector, nil, "", ipset.ListSet) if err != nil { - return nil, nil, nil, err + return nil, nil, nil, nil, err } nsSelectors[srcSelector.key] = srcSelector } @@ -205,16 +207,19 @@ func (ns *ns) analysePolicy(policy *networkingv1.NetworkPolicy) ( if peer.PodSelector != nil { dstSelector, err = newSelectorSpec(peer.PodSelector, nil, ns.name, ipset.HashIP) if err != nil { - return nil, nil, nil, err + return nil, nil, nil, nil, err } addIfNotExist(dstSelector, podSelectors) } else if peer.NamespaceSelector != nil { dstSelector, err = newSelectorSpec(peer.NamespaceSelector, nil, "", ipset.ListSet) if err != nil { - return nil, nil, nil, err + return nil, nil, nil, nil, err } nsSelectors[dstSelector.key] = dstSelector + } else if peer.IPBlock != nil { + ipBlock := newIPBlockSpec(peer.IPBlock) + ipBlocks[ipBlock.key] = ipBlock } if allPorts { @@ -231,7 +236,7 @@ func (ns *ns) analysePolicy(policy *networkingv1.NetworkPolicy) ( } } - return rules, nsSelectors, podSelectors, nil + return rules, nsSelectors, podSelectors, ipBlocks, nil } func addIfNotExist(s *selectorSpec, ss map[string]*selectorSpec) { diff --git a/npc/namespace.go b/npc/namespace.go index 780f1174..24a9012f 100644 --- a/npc/namespace.go +++ b/npc/namespace.go @@ -40,6 +40,7 @@ type ns struct { nsSelectors *selectorSet podSelectors *selectorSet + ipBlocks *ipBlockSet rules *ruleSet legacy bool @@ -61,6 +62,7 @@ func newNS(name, nodeName string, legacy bool, ipt iptables.Interface, ips ipset uid: uuid.NewUUID(), allPods: allPods, nsSelectors: nsSelectors, + ipBlocks: newIPBlockSet(ips), rules: newRuleSet(ipt), legacy: legacy} @@ -342,7 +344,7 @@ func (ns *ns) deletePod(obj *coreapi.Pod) error { func (ns *ns) addNetworkPolicy(obj interface{}) error { // Analyse policy, determine which rules and ipsets are required - uid, rules, nsSelectors, podSelectors, err := ns.analyse(obj) + uid, rules, nsSelectors, podSelectors, ipBlocks, err := ns.analyse(obj) if err != nil { return err } @@ -354,16 +356,19 @@ func (ns *ns) addNetworkPolicy(obj interface{}) error { if err := ns.podSelectors.provision(uid, nil, podSelectors); err != nil { return err } + if err := ns.ipBlocks.provision(uid, nil, ipBlocks); err != nil { + return err + } return ns.rules.provision(uid, nil, rules) } func (ns *ns) updateNetworkPolicy(oldObj, newObj interface{}) error { // Analyse the old and the new policy so we can determine differences - oldUID, oldRules, oldNsSelectors, oldPodSelectors, err := ns.analyse(oldObj) + oldUID, oldRules, oldNsSelectors, oldPodSelectors, oldIPBlocks, err := ns.analyse(oldObj) if err != nil { return err } - newUID, newRules, newNsSelectors, newPodSelectors, err := ns.analyse(newObj) + newUID, newRules, newNsSelectors, newPodSelectors, newIPBlocks, err := ns.analyse(newObj) if err != nil { return err } @@ -381,18 +386,24 @@ func (ns *ns) updateNetworkPolicy(oldObj, newObj interface{}) error { if err := ns.podSelectors.deprovision(oldUID, oldPodSelectors, newPodSelectors); err != nil { return err } + if err := ns.ipBlocks.deprovision(oldUID, oldIPBlocks, newIPBlocks); err != nil { + return err + } if err := ns.nsSelectors.provision(oldUID, oldNsSelectors, newNsSelectors); err != nil { return err } if err := ns.podSelectors.provision(oldUID, oldPodSelectors, newPodSelectors); err != nil { return err } + if err := ns.ipBlocks.provision(oldUID, oldIPBlocks, newIPBlocks); err != nil { + return err + } return ns.rules.provision(oldUID, oldRules, newRules) } func (ns *ns) deleteNetworkPolicy(obj interface{}) error { // Analyse network policy to free resources - uid, rules, nsSelectors, podSelectors, err := ns.analyse(obj) + uid, rules, nsSelectors, podSelectors, ipBlocks, err := ns.analyse(obj) if err != nil { return err } @@ -406,7 +417,14 @@ func (ns *ns) deleteNetworkPolicy(obj interface{}) error { if err := ns.nsSelectors.deprovision(uid, nsSelectors, nil); err != nil { return err } - return ns.podSelectors.deprovision(uid, podSelectors, nil) + if err := ns.podSelectors.deprovision(uid, podSelectors, nil); err != nil { + return err + } + if err := ns.ipBlocks.deprovision(uid, ipBlocks, nil); err != nil { + return err + } + + return nil } func (ns *ns) updateDefaultAllowIPSetEntry(oldObj, newObj *coreapi.Pod, ipsetName ipset.Name) error { @@ -602,6 +620,7 @@ func (ns *ns) analyse(obj interface{}) ( uid types.UID, rules map[string]*ruleSpec, nsSelectors, podSelectors map[string]*selectorSpec, + ipBlocks map[string]*ipBlockSpec, err error) { switch p := obj.(type) { @@ -622,7 +641,7 @@ func (ns *ns) analyse(obj interface{}) ( return } } else { - rules, nsSelectors, podSelectors, err = ns.analysePolicy(obj.(*networkingv1.NetworkPolicy)) + rules, nsSelectors, podSelectors, ipBlocks, err = ns.analysePolicy(obj.(*networkingv1.NetworkPolicy)) if err != nil { return } From 2e85018c261afd115569fc8b27e4a19531bbf8aa Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Tue, 22 May 2018 12:37:46 +0200 Subject: [PATCH 17/46] npc: Add ruleHost interface The idea for the interface is taken from the kitt1987/ipblock work. The interface is going to be implemented by selectorSpec and ipBlockSpec. --- npc/rule.go | 24 +++++++++++++++--------- npc/selector.go | 19 +++++++++++++++++++ 2 files changed, 34 insertions(+), 9 deletions(-) diff --git a/npc/rule.go b/npc/rule.go index f875af5e..24ac5b13 100644 --- a/npc/rule.go +++ b/npc/rule.go @@ -10,30 +10,36 @@ import ( "github.com/weaveworks/weave/npc/iptables" ) +type ruleHost interface { + // getRuleSpec returns source or destination specification and comment which + // are used in an iptables rule. + // + // If src=true then the rulespec is for source, otherwise - for destination. + getRuleSpec(src bool) ([]string, string) +} + type ruleSpec struct { key string args []string policyType policyType } -func newRuleSpec(policyType policyType, proto *string, srcHost *selectorSpec, dstHost *selectorSpec, dstPort *string) *ruleSpec { +func newRuleSpec(policyType policyType, proto *string, srcHost, dstHost ruleHost, dstPort *string) *ruleSpec { args := []string{} if proto != nil { args = append(args, "-p", *proto) } srcComment := "anywhere" if srcHost != nil { - args = append(args, "-m", "set", "--match-set", string(srcHost.ipsetName), "src") - if srcHost.nsName != "" { - srcComment = fmt.Sprintf("pods: namespace: %s, selector: %s", srcHost.nsName, srcHost.key) - } else { - srcComment = fmt.Sprintf("namespaces: selector: %s", srcHost.key) - } + rule, comment := srcHost.getRuleSpec(true) + args = append(args, rule...) + srcComment = comment } dstComment := "anywhere" if dstHost != nil { - args = append(args, "-m", "set", "--match-set", string(dstHost.ipsetName), "dst") - dstComment = fmt.Sprintf("pods: namespace: %s, selector: %s", dstHost.nsName, dstHost.key) + rule, comment := dstHost.getRuleSpec(false) + args = append(args, rule...) + dstComment = comment } if dstPort != nil { args = append(args, "--dport", *dstPort) diff --git a/npc/selector.go b/npc/selector.go index 4affe0ca..33f839a0 100644 --- a/npc/selector.go +++ b/npc/selector.go @@ -1,6 +1,8 @@ package npc import ( + "fmt" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/types" @@ -37,6 +39,23 @@ func newSelectorSpec(json *metav1.LabelSelector, policyType []policyType, nsName nsName: nsName}, nil } +func (spec *selectorSpec) getRuleSpec(src bool) ([]string, string) { + dir := "dst" + if src { + dir = "src" + } + rule := []string{"-m", "set", "--match-set", string(spec.ipsetName), dir} + + comment := "anywhere" + if spec.nsName != "" { + comment = fmt.Sprintf("pods: namespace: %s, selector: %s", spec.nsName, spec.key) + } else { + comment = fmt.Sprintf("namespaces: selector: %s", spec.key) + } + + return rule, comment +} + type selector struct { ips ipset.Interface spec *selectorSpec From 0a1ca30aae1fdcffbeae7abd2eed43d565d3d3fc Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Tue, 22 May 2018 12:44:54 +0200 Subject: [PATCH 18/46] npc: Add getRuleSpec for ipBlockSpec --- npc/ipblock.go | 34 ++++++++++++++++++---------------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/npc/ipblock.go b/npc/ipblock.go index 594af2b4..c40e46e2 100644 --- a/npc/ipblock.go +++ b/npc/ipblock.go @@ -1,6 +1,7 @@ package npc import ( + "fmt" "sort" "strings" @@ -29,22 +30,23 @@ func newIPBlockSpec(ipb *networkingv1.IPBlock) *ipBlockSpec { return spec } -//func (ipb ipBlock) GetRuleSpec() (spec *ruleSpec) { -// spec = &ruleSpec{ -// chain: IngressIPBlockChain, -// } -// -// if len(ipb.spec.Except) == 0 { -// spec.args = append(spec.args, "-s", ipb.spec.CIDR) -// spec.comment = fmt.Sprintf("cidr: %s", ipb.spec.CIDR) -// } else { -// spec.args = append(spec.args, "-s", ipb.spec.CIDR, "-m", "set", "!", "--match-set", -// string(ipb.ipsetName), "src") -// spec.comment = fmt.Sprintf("cidr: %s except [%s]", ipb.spec.CIDR, -// strings.Join(ipb.spec.Except, `,`)) -// } -// return -//} +func (spec *ipBlockSpec) getRuleSpec(src bool) ([]string, string) { + dir, dirOpt := "dst", "-d" + if src { + dir, dirOpt = "src", "-s" + } + + if spec.key == "" { + rule := []string{dirOpt, spec.ipBlock.CIDR} + comment := fmt.Sprintf("cidr: %s", spec.ipBlock.CIDR) + return rule, comment + } + + rule := []string{dirOpt, spec.ipBlock.CIDR, + "-m", "set", "!", "--match-set", string(spec.ipsetName), dir} + comment := fmt.Sprintf("cidr: %s except [%s]", spec.ipBlock.CIDR, spec.key) + return rule, comment +} type ipBlockSet struct { ips ipset.Interface From a5e0b3452dd5ec6c6e50862c050df6dacee20cbc Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Tue, 22 May 2018 12:55:45 +0200 Subject: [PATCH 19/46] npc: Create rules with IPBlock (egress only) --- npc/analyser.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/npc/analyser.go b/npc/analyser.go index 8b825cdc..48f358b5 100644 --- a/npc/analyser.go +++ b/npc/analyser.go @@ -201,6 +201,7 @@ func (ns *ns) analysePolicy(policy *networkingv1.NetworkPolicy) ( } else { for _, peer := range egressRule.To { var dstSelector *selectorSpec + var dstRuleHost ruleHost // NetworkPolicyPeer describes a peer to allow traffic from. // Exactly one of its fields must be specified. @@ -210,6 +211,7 @@ func (ns *ns) analysePolicy(policy *networkingv1.NetworkPolicy) ( return nil, nil, nil, nil, err } addIfNotExist(dstSelector, podSelectors) + dstRuleHost = dstSelector } else if peer.NamespaceSelector != nil { dstSelector, err = newSelectorSpec(peer.NamespaceSelector, nil, "", ipset.ListSet) @@ -217,17 +219,19 @@ func (ns *ns) analysePolicy(policy *networkingv1.NetworkPolicy) ( return nil, nil, nil, nil, err } nsSelectors[dstSelector.key] = dstSelector + dstRuleHost = dstSelector } else if peer.IPBlock != nil { ipBlock := newIPBlockSpec(peer.IPBlock) ipBlocks[ipBlock.key] = ipBlock + dstRuleHost = ipBlock } if allPorts { - rule := newRuleSpec(egressPolicy, nil, targetSelector, dstSelector, nil) + rule := newRuleSpec(egressPolicy, nil, targetSelector, dstRuleHost, nil) rules[rule.key] = rule } else { withNormalisedProtoAndPort(egressRule.Ports, func(proto, port string) { - rule := newRuleSpec(egressPolicy, &proto, targetSelector, dstSelector, &port) + rule := newRuleSpec(egressPolicy, &proto, targetSelector, dstRuleHost, &port) rules[rule.key] = rule }) } From b84fbf86ecf3c74fb73e8ed6ea70c0996fb80a4d Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Tue, 22 May 2018 13:21:29 +0200 Subject: [PATCH 20/46] npc: Add a dummy isNil method to ruleHost interface --- npc/ipblock.go | 4 ++++ npc/rule.go | 6 ++++-- npc/selector.go | 4 ++++ 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/npc/ipblock.go b/npc/ipblock.go index c40e46e2..0c5284ae 100644 --- a/npc/ipblock.go +++ b/npc/ipblock.go @@ -48,6 +48,10 @@ func (spec *ipBlockSpec) getRuleSpec(src bool) ([]string, string) { return rule, comment } +func (spec *ipBlockSpec) isNil() bool { + return spec == nil +} + type ipBlockSet struct { ips ipset.Interface users map[string]map[types.UID]struct{} diff --git a/npc/rule.go b/npc/rule.go index 24ac5b13..03de4170 100644 --- a/npc/rule.go +++ b/npc/rule.go @@ -16,6 +16,8 @@ type ruleHost interface { // // If src=true then the rulespec is for source, otherwise - for destination. getRuleSpec(src bool) ([]string, string) + // TODO(brb) Get rid + isNil() bool } type ruleSpec struct { @@ -30,13 +32,13 @@ func newRuleSpec(policyType policyType, proto *string, srcHost, dstHost ruleHost args = append(args, "-p", *proto) } srcComment := "anywhere" - if srcHost != nil { + if srcHost != nil && !srcHost.isNil() { rule, comment := srcHost.getRuleSpec(true) args = append(args, rule...) srcComment = comment } dstComment := "anywhere" - if dstHost != nil { + if dstHost != nil && !dstHost.isNil() { rule, comment := dstHost.getRuleSpec(false) args = append(args, rule...) dstComment = comment diff --git a/npc/selector.go b/npc/selector.go index 33f839a0..51eed64f 100644 --- a/npc/selector.go +++ b/npc/selector.go @@ -56,6 +56,10 @@ func (spec *selectorSpec) getRuleSpec(src bool) ([]string, string) { return rule, comment } +func (spec *selectorSpec) isNil() bool { + return spec == nil +} + type selector struct { ips ipset.Interface spec *selectorSpec From 35f1ba17e1441e0ba050154314803775601795ec Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Sat, 26 May 2018 16:51:17 +0200 Subject: [PATCH 21/46] npc: Provision egress rules --- npc/rule.go | 54 +++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 38 insertions(+), 16 deletions(-) diff --git a/npc/rule.go b/npc/rule.go index 03de4170..37238c72 100644 --- a/npc/rule.go +++ b/npc/rule.go @@ -46,7 +46,6 @@ func newRuleSpec(policyType policyType, proto *string, srcHost, dstHost ruleHost if dstPort != nil { args = append(args, "--dport", *dstPort) } - args = append(args, "-j", "ACCEPT") // NOTE: if you remove the comment bellow, then embed `policyType` into `key`. // Otherwise, the rule won't be provisioned if it exists for other policy type. args = append(args, "-m", "comment", "--comment", fmt.Sprintf("%s -> %s (%s)", srcComment, dstComment, policyTypeStr(policyType))) @@ -55,6 +54,31 @@ func newRuleSpec(policyType policyType, proto *string, srcHost, dstHost ruleHost return &ruleSpec{key, args, policyType} } +func (spec *ruleSpec) iptChain() string { + if spec.policyType == egressPolicy { + return EgressCustomChain + } + return IngressChain +} + +func (spec *ruleSpec) iptRuleSpecs() [][]string { + if spec.policyType == ingressPolicy { + rule := make([]string, len(spec.args)) + copy(rule, spec.args) + rule = append(rule, "-j", "ACCEPT") + return [][]string{rule} + } + + // egressPolicy + ruleMark := make([]string, len(spec.args)) + copy(ruleMark, spec.args) + ruleMark = append(ruleMark, "-j", "MARK", "--set-xmark", EgressMark) + ruleReturn := make([]string, len(spec.args)) + copy(ruleReturn, spec.args) + ruleReturn = append(ruleReturn, "-j", "RETURN") + return [][]string{ruleMark, ruleReturn} +} + type ruleSet struct { ipt iptables.Interface users map[string]map[types.UID]struct{} @@ -69,11 +93,14 @@ func (rs *ruleSet) deprovision(user types.UID, current, desired map[string]*rule if _, found := desired[key]; !found { delete(rs.users[key], user) if len(rs.users[key]) == 0 { - chain := rulesChainByPolicyType(spec.policyType) - common.Log.Infof("deleting rule %v from %q chain", spec.args, chain) - if err := rs.ipt.Delete(TableFilter, chain, spec.args...); err != nil { - return err + chain := spec.iptChain() + for _, rule := range spec.iptRuleSpecs() { + common.Log.Infof("deleting rule %v from %q chain", rule, chain) + if err := rs.ipt.Delete(TableFilter, chain, rule...); err != nil { + return err + } } + delete(rs.users, key) } } @@ -86,10 +113,12 @@ func (rs *ruleSet) provision(user types.UID, current, desired map[string]*ruleSp for key, spec := range desired { if _, found := current[key]; !found { if _, found := rs.users[key]; !found { - chain := rulesChainByPolicyType(spec.policyType) - common.Log.Infof("adding rule %v to %q chain", spec.args, chain) - if err := rs.ipt.Append(TableFilter, chain, spec.args...); err != nil { - return err + chain := spec.iptChain() + for _, rule := range spec.iptRuleSpecs() { + common.Log.Infof("adding rule %v to %q chain", rule, chain) + if err := rs.ipt.Append(TableFilter, chain, rule...); err != nil { + return err + } } rs.users[key] = make(map[types.UID]struct{}) } @@ -99,10 +128,3 @@ func (rs *ruleSet) provision(user types.UID, current, desired map[string]*ruleSp return nil } - -func rulesChainByPolicyType(policyType policyType) string { - if policyType == egressPolicy { - return EgressCustomChain - } - return IngressChain -} From 1f86ed55ba1e6e27cdae98d5bc021525c793e3de Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Sat, 26 May 2018 17:22:19 +0200 Subject: [PATCH 22/46] npc: Ensure default-allow rules for egress --- npc/namespace.go | 75 ++++++++++++++++++++++++++++++++++-------------- 1 file changed, 54 insertions(+), 21 deletions(-) diff --git a/npc/namespace.go b/npc/namespace.go index 24a9012f..f3a946e1 100644 --- a/npc/namespace.go +++ b/npc/namespace.go @@ -443,32 +443,65 @@ func (ns *ns) updateDefaultAllowIPSetEntry(oldObj, newObj *coreapi.Pod, ipsetNam return nil } -func bypassRule(nsIpsetName ipset.Name, namespace string) []string { +func bypassRuleIngress(nsIpsetName ipset.Name, namespace string) []string { return []string{"-m", "set", "--match-set", string(nsIpsetName), "dst", "-j", "ACCEPT", "-m", "comment", "--comment", "DefaultAllow isolation for namespace: " + namespace} } -func (ns *ns) ensureBypassRule() error { - var ipset ipset.Name - if ns.legacy { - ipset = ns.allPods.ipsetName - } else { - ipset = ns.ingressDefaultAllowIPSet +func bypassRuleEgress(nsIpsetName ipset.Name, namespace string) [][]string { + return [][]string{ + {"-m", "set", "--match-set", string(nsIpsetName), "src", "-j", "MARK", "--set-xmark", EgressMark, "-m", "comment", "--comment", "DefaultAllow isolation for namespace: " + namespace}, + {"-m", "set", "--match-set", string(nsIpsetName), "src", "-j", "RETURN", "-m", "comment", "--comment", "DefaultAllow isolation for namespace: " + namespace}, } - - common.Log.Debugf("ensuring rule for DefaultAllow in namespace: %s, set %s", ns.name, ipset) - return ns.ipt.Append(TableFilter, DefaultChain, bypassRule(ipset, ns.name)...) } -func (ns *ns) deleteBypassRule() error { - var ipset ipset.Name +func (ns *ns) ensureBypassRules() error { if ns.legacy { - ipset = ns.allPods.ipsetName - } else { - ipset = ns.ingressDefaultAllowIPSet + ipset := ns.allPods.ipsetName + common.Log.Debugf("ensuring rule for DefaultAllow in namespace: %s, set %s", ns.name, ipset) + if err := ns.ipt.Append(TableFilter, DefaultChain, bypassRuleIngress(ipset, ns.name)...); err != nil { + return err + } } - common.Log.Debugf("removing default rule in namespace: %s, set %s", ns.name, ipset) - return ns.ipt.Delete(TableFilter, DefaultChain, bypassRule(ipset, ns.name)...) + ipset := ns.ingressDefaultAllowIPSet + common.Log.Debugf("ensuring rule for ingress DefaultAllow in namespace: %s, set %s", ns.name, ipset) + if err := ns.ipt.Append(TableFilter, DefaultChain, bypassRuleIngress(ipset, ns.name)...); err != nil { + return err + } + ipset = ns.egressDefaultAllowIPSet + common.Log.Debugf("ensuring rules for egress DefaultAllow in namespace: %s, set %s", ns.name, ipset) + for _, rule := range bypassRuleEgress(ipset, ns.name) { + if err := ns.ipt.Append(TableFilter, EgressDefaultChain, rule...); err != nil { + return err + } + } + + return nil +} + +func (ns *ns) deleteBypassRules() error { + if ns.legacy { + ipset := ns.allPods.ipsetName + common.Log.Debugf("removing default rule in namespace: %s, set %s", ns.name, ipset) + if err := ns.ipt.Delete(TableFilter, DefaultChain, bypassRuleIngress(ipset, ns.name)...); err != nil { + return err + } + } + + ipset := ns.ingressDefaultAllowIPSet + common.Log.Debugf("removing default ingress rule in namespace: %s, set %s", ns.name, ipset) + if err := ns.ipt.Delete(TableFilter, DefaultChain, bypassRuleIngress(ipset, ns.name)...); err != nil { + return err + } + ipset = ns.egressDefaultAllowIPSet + common.Log.Debugf("removing default egress rule in namespace: %s, set %s", ns.name, ipset) + for _, rule := range bypassRuleEgress(ipset, ns.name) { + if err := ns.ipt.Delete(TableFilter, EgressDefaultChain, rule...); err != nil { + return err + } + } + + return nil } func (ns *ns) addNamespace(obj *coreapi.Namespace) error { @@ -476,7 +509,7 @@ func (ns *ns) addNamespace(obj *coreapi.Namespace) error { // Insert a rule to bypass policies if namespace is DefaultAllow if !ns.isDefaultDeny(obj) { - if err := ns.ensureBypassRule(); err != nil { + if err := ns.ensureBypassRules(); err != nil { return err } } @@ -496,12 +529,12 @@ 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 { - if err := ns.ensureBypassRule(); err != nil { + if err := ns.ensureBypassRules(); err != nil { return err } } if newDefaultDeny { - if err := ns.deleteBypassRule(); err != nil { + if err := ns.deleteBypassRules(); err != nil { return err } } @@ -536,7 +569,7 @@ func (ns *ns) deleteNamespace(obj *coreapi.Namespace) error { // Remove bypass rule if !ns.isDefaultDeny(obj) { - if err := ns.deleteBypassRule(); err != nil { + if err := ns.deleteBypassRules(); err != nil { return err } } From c0e2deb5a0d6bea3529c3bc8e677be22f4e3d3e7 Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Sat, 26 May 2018 20:16:33 +0200 Subject: [PATCH 23/46] npc: Add WEAVE-NPC-EGRESS-ACCEPT chain For improving readability of NPC rules. The chain marks to-be-allowed egress traffic. --- npc/constants.go | 1 + npc/namespace.go | 2 +- npc/rule.go | 2 +- prog/weave-npc/main.go | 9 +++++++++ 4 files changed, 12 insertions(+), 2 deletions(-) diff --git a/npc/constants.go b/npc/constants.go index 0e41dfe8..6c94dcc9 100644 --- a/npc/constants.go +++ b/npc/constants.go @@ -10,6 +10,7 @@ const ( EgressChain = "WEAVE-NPC-EGRESS" EgressDefaultChain = "WEAVE-NPC-EGRESS-DEFAULT" EgressCustomChain = "WEAVE-NPC-EGRESS-CUSTOM" + EgressMarkChain = "WEAVE-NPC-EGRESS-ACCEPT" EgressMark = "0x40000/0x40000" IpsetNamePrefix = "weave-" diff --git a/npc/namespace.go b/npc/namespace.go index f3a946e1..f3972474 100644 --- a/npc/namespace.go +++ b/npc/namespace.go @@ -449,7 +449,7 @@ func bypassRuleIngress(nsIpsetName ipset.Name, namespace string) []string { func bypassRuleEgress(nsIpsetName ipset.Name, namespace string) [][]string { return [][]string{ - {"-m", "set", "--match-set", string(nsIpsetName), "src", "-j", "MARK", "--set-xmark", EgressMark, "-m", "comment", "--comment", "DefaultAllow isolation for namespace: " + namespace}, + {"-m", "set", "--match-set", string(nsIpsetName), "src", "-j", EgressMarkChain, "-m", "comment", "--comment", "DefaultAllow isolation for namespace: " + namespace}, {"-m", "set", "--match-set", string(nsIpsetName), "src", "-j", "RETURN", "-m", "comment", "--comment", "DefaultAllow isolation for namespace: " + namespace}, } } diff --git a/npc/rule.go b/npc/rule.go index 37238c72..a08a3a20 100644 --- a/npc/rule.go +++ b/npc/rule.go @@ -72,7 +72,7 @@ func (spec *ruleSpec) iptRuleSpecs() [][]string { // egressPolicy ruleMark := make([]string, len(spec.args)) copy(ruleMark, spec.args) - ruleMark = append(ruleMark, "-j", "MARK", "--set-xmark", EgressMark) + ruleMark = append(ruleMark, "-j", EgressMarkChain) ruleReturn := make([]string, len(spec.args)) copy(ruleReturn, spec.args) ruleReturn = append(ruleReturn, "-j", "RETURN") diff --git a/prog/weave-npc/main.go b/prog/weave-npc/main.go index 4edfe9f4..dcda5813 100644 --- a/prog/weave-npc/main.go +++ b/prog/weave-npc/main.go @@ -57,6 +57,10 @@ func resetIPTables(ipt *iptables.IPTables) error { return err } + if err := ipt.ClearChain(npc.TableFilter, npc.EgressMarkChain); err != nil { + return err + } + if err := ipt.ClearChain(npc.TableFilter, npc.EgressCustomChain); err != nil { return err } @@ -136,6 +140,11 @@ func createBaseRules(ipt *iptables.IPTables, ips ipset.Interface) error { return err } + if err := ipt.Append(npc.TableFilter, npc.EgressMarkChain, + "-j", "MARK", "--set-xmark", npc.EgressMark); err != nil { + return err + } + if err := ipt.Append(npc.TableFilter, npc.EgressChain, "-m", "state", "--state", "RELATED,ESTABLISHED", "-j", "RETURN"); err != nil { return err From 6055431b6f2b0f878f5e31f577f3edd92dc279ad Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Sat, 26 May 2018 20:35:44 +0200 Subject: [PATCH 24/46] npc: Refactor bypassRules --- npc/namespace.go | 60 +++++++++++++++++++++++++----------------------- 1 file changed, 31 insertions(+), 29 deletions(-) diff --git a/npc/namespace.go b/npc/namespace.go index f3972474..a09d2e1e 100644 --- a/npc/namespace.go +++ b/npc/namespace.go @@ -443,36 +443,41 @@ func (ns *ns) updateDefaultAllowIPSetEntry(oldObj, newObj *coreapi.Pod, ipsetNam return nil } -func bypassRuleIngress(nsIpsetName ipset.Name, namespace string) []string { - return []string{"-m", "set", "--match-set", string(nsIpsetName), "dst", "-j", "ACCEPT", "-m", "comment", "--comment", "DefaultAllow isolation for namespace: " + namespace} +func bypassRules(namespace string, ingress, egress ipset.Name) map[string][][]string { + return map[string][][]string{ + DefaultChain: { + {"-m", "set", "--match-set", string(ingress), "dst", "-j", "ACCEPT", + "-m", "comment", "--comment", "DefaultAllow ingress isolation for namespace: " + namespace}, + }, + EgressDefaultChain: { + {"-m", "set", "--match-set", string(egress), "src", "-j", EgressMarkChain, + "-m", "comment", "--comment", "DefaultAllow egress isolation for namespace: " + namespace}, + {"-m", "set", "--match-set", string(egress), "src", "-j", "RETURN", + "-m", "comment", "--comment", "DefaultAllow egress isolation for namespace: " + namespace}, + }, + } } -func bypassRuleEgress(nsIpsetName ipset.Name, namespace string) [][]string { - return [][]string{ - {"-m", "set", "--match-set", string(nsIpsetName), "src", "-j", EgressMarkChain, "-m", "comment", "--comment", "DefaultAllow isolation for namespace: " + namespace}, - {"-m", "set", "--match-set", string(nsIpsetName), "src", "-j", "RETURN", "-m", "comment", "--comment", "DefaultAllow isolation for namespace: " + namespace}, - } +func bypassRuleLegacy(nsIpsetName ipset.Name, namespace string) []string { + return []string{"-m", "set", "--match-set", string(nsIpsetName), "dst", "-j", "ACCEPT", "-m", "comment", "--comment", "DefaultAllow isolation for namespace: " + namespace} } func (ns *ns) ensureBypassRules() error { if ns.legacy { ipset := ns.allPods.ipsetName common.Log.Debugf("ensuring rule for DefaultAllow in namespace: %s, set %s", ns.name, ipset) - if err := ns.ipt.Append(TableFilter, DefaultChain, bypassRuleIngress(ipset, ns.name)...); err != nil { + if err := ns.ipt.Append(TableFilter, DefaultChain, bypassRuleLegacy(ipset, ns.name)...); err != nil { return err } + return nil } - ipset := ns.ingressDefaultAllowIPSet - common.Log.Debugf("ensuring rule for ingress DefaultAllow in namespace: %s, set %s", ns.name, ipset) - if err := ns.ipt.Append(TableFilter, DefaultChain, bypassRuleIngress(ipset, ns.name)...); err != nil { - return err - } - ipset = ns.egressDefaultAllowIPSet - common.Log.Debugf("ensuring rules for egress DefaultAllow in namespace: %s, set %s", ns.name, ipset) - for _, rule := range bypassRuleEgress(ipset, ns.name) { - if err := ns.ipt.Append(TableFilter, EgressDefaultChain, rule...); err != nil { - return err + for chain, rules := range bypassRules(ns.name, ns.ingressDefaultAllowIPSet, ns.egressDefaultAllowIPSet) { + for _, rule := range rules { + common.Log.Debugf("adding rule for DefaultAllow in namespace: %s, chain: %s, %s", ns.name, chain, rule) + if err := ns.ipt.Append(TableFilter, chain, rule...); err != nil { + return err + } } } @@ -483,21 +488,18 @@ func (ns *ns) deleteBypassRules() error { if ns.legacy { ipset := ns.allPods.ipsetName common.Log.Debugf("removing default rule in namespace: %s, set %s", ns.name, ipset) - if err := ns.ipt.Delete(TableFilter, DefaultChain, bypassRuleIngress(ipset, ns.name)...); err != nil { + if err := ns.ipt.Delete(TableFilter, DefaultChain, bypassRuleLegacy(ipset, ns.name)...); err != nil { return err } + return nil } - ipset := ns.ingressDefaultAllowIPSet - common.Log.Debugf("removing default ingress rule in namespace: %s, set %s", ns.name, ipset) - if err := ns.ipt.Delete(TableFilter, DefaultChain, bypassRuleIngress(ipset, ns.name)...); err != nil { - return err - } - ipset = ns.egressDefaultAllowIPSet - common.Log.Debugf("removing default egress rule in namespace: %s, set %s", ns.name, ipset) - for _, rule := range bypassRuleEgress(ipset, ns.name) { - if err := ns.ipt.Delete(TableFilter, EgressDefaultChain, rule...); err != nil { - return err + for chain, rules := range bypassRules(ns.name, ns.ingressDefaultAllowIPSet, ns.egressDefaultAllowIPSet) { + for _, rule := range rules { + common.Log.Debugf("removing rule for DefaultAllow in namespace: %s, chain: %s, %s", ns.name, chain, rule) + if err := ns.ipt.Delete(TableFilter, chain, rule...); err != nil { + return err + } } } From 7da7ce85df37e126085ea1f49e1e2f04a3054b89 Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Mon, 28 May 2018 18:10:47 +0200 Subject: [PATCH 25/46] npc: Extend TestDefaultAllow unit test to include egress --- npc/controller_test.go | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/npc/controller_test.go b/npc/controller_test.go index 17001e86..2f5f0434 100644 --- a/npc/controller_test.go +++ b/npc/controller_test.go @@ -198,6 +198,7 @@ func TestRegressionPolicyNamespaceOrdering3059(t *testing.T) { func TestDefaultAllow(t *testing.T) { const ( ingressDefaultAllowIPSetName = "weave-;rGqyMIl1HN^cfDki~Z$3]6!N" + egressDefaultAllowIPSetName = "weave-s_+ChJId4Uy_$}G;WdH|~TK)I" fooPodIP = "10.32.0.10" barPodIP = "10.32.0.11" barPodNewIP = "10.32.0.12" @@ -215,6 +216,7 @@ func TestDefaultAllow(t *testing.T) { // Should create an ipset for default-allow require.Contains(t, m.sets, ingressDefaultAllowIPSetName) + require.Contains(t, m.sets, egressDefaultAllowIPSetName) podFoo := &coreapi.Pod{ ObjectMeta: metav1.ObjectMeta{ @@ -227,6 +229,7 @@ func TestDefaultAllow(t *testing.T) { // Should add the foo pod to default-allow require.True(t, m.entryExists(ingressDefaultAllowIPSetName, fooPodIP)) + require.True(t, m.entryExists(egressDefaultAllowIPSetName, fooPodIP)) podBar := &coreapi.Pod{ ObjectMeta: metav1.ObjectMeta{ @@ -242,6 +245,7 @@ func TestDefaultAllow(t *testing.T) { // Should add the bar pod to default-allow require.True(t, m.entryExists(ingressDefaultAllowIPSetName, barPodIP)) + require.True(t, m.entryExists(egressDefaultAllowIPSetName, barPodIP)) // Allow access from the bar pod to the foo pod netpol := &networkingv1.NetworkPolicy{ @@ -250,7 +254,10 @@ func TestDefaultAllow(t *testing.T) { Namespace: "default", }, Spec: networkingv1.NetworkPolicySpec{ - PolicyTypes: []networkingv1.PolicyType{networkingv1.PolicyTypeIngress}, + PolicyTypes: []networkingv1.PolicyType{ + networkingv1.PolicyTypeIngress, + networkingv1.PolicyTypeEgress, + }, PodSelector: metav1.LabelSelector{MatchLabels: map[string]string{"run": "foo"}}, Ingress: []networkingv1.NetworkPolicyIngressRule{{ From: []networkingv1.NetworkPolicyPeer{{ @@ -259,13 +266,20 @@ func TestDefaultAllow(t *testing.T) { }, }}, }}, + Egress: []networkingv1.NetworkPolicyEgressRule{{ + To: []networkingv1.NetworkPolicyPeer{{ + IPBlock: &networkingv1.IPBlock{CIDR: "192.168.48.0/24"}, + }}, + }}, }, } controller.AddNetworkPolicy(netpol) // Should remove the foo pod from default-allow as the netpol selects it require.False(t, m.entryExists(ingressDefaultAllowIPSetName, fooPodIP)) + require.False(t, m.entryExists(egressDefaultAllowIPSetName, fooPodIP)) require.True(t, m.entryExists(ingressDefaultAllowIPSetName, barPodIP)) + require.True(t, m.entryExists(egressDefaultAllowIPSetName, barPodIP)) podBarWithNewIP := *podBar podBarWithNewIP.Status.PodIP = barPodNewIP @@ -273,11 +287,14 @@ func TestDefaultAllow(t *testing.T) { // Should update IP addr of the bar pod in default-allow require.False(t, m.entryExists(ingressDefaultAllowIPSetName, barPodIP)) + require.False(t, m.entryExists(egressDefaultAllowIPSetName, barPodIP)) require.True(t, m.entryExists(ingressDefaultAllowIPSetName, barPodNewIP)) + require.True(t, m.entryExists(egressDefaultAllowIPSetName, barPodNewIP)) controller.UpdatePod(&podBarWithNewIP, podBarNoIP) // Should remove the bar pod from default-allow as it does not have any IP addr require.False(t, m.entryExists(ingressDefaultAllowIPSetName, barPodNewIP)) + require.False(t, m.entryExists(egressDefaultAllowIPSetName, barPodNewIP)) podFooWithNewLabel := *podFoo podFooWithNewLabel.ObjectMeta.Labels = map[string]string{"run": "new-foo"} @@ -285,22 +302,27 @@ func TestDefaultAllow(t *testing.T) { // Should bring back the foo pod to default-allow as it does not match dst of any netpol require.True(t, m.entryExists(ingressDefaultAllowIPSetName, fooPodIP)) + require.True(t, m.entryExists(egressDefaultAllowIPSetName, fooPodIP)) controller.UpdatePod(&podFooWithNewLabel, podFoo) // Should remove from default-allow as it matches the netpol after the update require.False(t, m.entryExists(ingressDefaultAllowIPSetName, fooPodIP)) + require.False(t, m.entryExists(egressDefaultAllowIPSetName, fooPodIP)) controller.DeleteNetworkPolicy(netpol) // Should bring back the foo pod to default-allow as no netpol selects it require.True(t, m.entryExists(ingressDefaultAllowIPSetName, fooPodIP)) + require.True(t, m.entryExists(egressDefaultAllowIPSetName, fooPodIP)) controller.DeletePod(podFoo) // Should remove foo pod from default-allow require.False(t, m.entryExists(ingressDefaultAllowIPSetName, fooPodIP)) + require.False(t, m.entryExists(egressDefaultAllowIPSetName, fooPodIP)) controller.DeleteNamespace(defaultNamespace) // Should remove default ipset require.NotContains(t, m.sets, ingressDefaultAllowIPSetName) + require.NotContains(t, m.sets, egressDefaultAllowIPSetName) } func TestOutOfOrderPodEvents(t *testing.T) { From cec3c8a846fa3d5752cd13ed23eefee58c85add3 Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Mon, 28 May 2018 18:56:50 +0200 Subject: [PATCH 26/46] npc: Add iptables mocked interface --- npc/controller_test.go | 42 ++++++++++++++++++++++++++++++++++++------ 1 file changed, 36 insertions(+), 6 deletions(-) diff --git a/npc/controller_test.go b/npc/controller_test.go index 2f5f0434..8c3dea5a 100644 --- a/npc/controller_test.go +++ b/npc/controller_test.go @@ -1,7 +1,9 @@ package npc import ( + "fmt" "log" + "strings" "testing" "github.com/pkg/errors" @@ -103,18 +105,46 @@ func (i *mockIPSet) List(prefix string) ([]ipset.Name, error) { } type mockIPTables struct { + rules map[string]map[string]struct{} // chain -> rulespec -> struct{} +} + +func newMockIPTables() *mockIPTables { + return &mockIPTables{rules: make(map[string]map[string]struct{})} } func (ipt *mockIPTables) Append(table, chain string, rulespec ...string) error { + if table != TableFilter { + return fmt.Errorf("invalid table: %q", table) + } + + if _, found := ipt.rules[chain]; !found { + ipt.rules[chain] = make(map[string]struct{}) + } + + rule := strings.Join(rulespec, " ") + if _, found := ipt.rules[chain][rule]; found { + return fmt.Errorf("rule already exists. chain: %q, rule: %q", chain, rule) + } + + ipt.rules[chain][rule] = struct{}{} + return nil } func (ipt *mockIPTables) Delete(table, chain string, rulespec ...string) error { + rule := strings.Join(rulespec, " ") + + if _, found := ipt.rules[chain][rule]; !found { + return fmt.Errorf("rule does not exist. chain: %q rule: %q", chain, rule) + } + + delete(ipt.rules[chain], rule) + return nil } func (ipt *mockIPTables) Insert(table, chain string, pos int, rulespec ...string) error { - return nil + return errors.New("Not Implemented") } func TestRegressionPolicyNamespaceOrdering3059(t *testing.T) { @@ -168,7 +198,7 @@ func TestRegressionPolicyNamespaceOrdering3059(t *testing.T) { // Namespaces first m := newMockIPSet() - controller := New("foo", false, &mockIPTables{}, &m) + controller := New("foo", false, newMockIPTables(), &m) const ( selectorIPSetName = "weave-I239Zp%sCvoVt*D6u=A!2]YEk" @@ -184,7 +214,7 @@ func TestRegressionPolicyNamespaceOrdering3059(t *testing.T) { // NetworkPolicy first m = newMockIPSet() - controller = New("foo", false, &mockIPTables{}, &m) + controller = New("foo", false, newMockIPTables(), &m) controller.AddNetworkPolicy(networkPolicy) @@ -205,7 +235,7 @@ func TestDefaultAllow(t *testing.T) { ) m := newMockIPSet() - controller := New("bar", false, &mockIPTables{}, &m) + controller := New("bar", false, newMockIPTables(), &m) defaultNamespace := &coreapi.Namespace{ ObjectMeta: metav1.ObjectMeta{ @@ -333,7 +363,7 @@ func TestOutOfOrderPodEvents(t *testing.T) { ) m := newMockIPSet() - controller := New("qux", false, &mockIPTables{}, &m) + controller := New("qux", false, newMockIPTables(), &m) defaultNamespace := &coreapi.Namespace{ ObjectMeta: metav1.ObjectMeta{ @@ -415,7 +445,7 @@ func TestNewTargetSelector(t *testing.T) { ) m := newMockIPSet() - controller := New("baz", false, &mockIPTables{}, &m) + controller := New("baz", false, newMockIPTables(), &m) defaultNamespace := &coreapi.Namespace{ ObjectMeta: metav1.ObjectMeta{ From a85a19e40a8f43e45680da1ee5a78ccb07a76726 Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Mon, 28 May 2018 19:51:47 +0200 Subject: [PATCH 27/46] npc: Add unit test for egress policy with ipblock --- npc/controller_test.go | 48 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/npc/controller_test.go b/npc/controller_test.go index 8c3dea5a..0a59479d 100644 --- a/npc/controller_test.go +++ b/npc/controller_test.go @@ -511,3 +511,51 @@ func TestNewTargetSelector(t *testing.T) { // No netpol target-selects podFoo require.True(t, m.entryExists(ingressDefaultAllowIPSetName, podIP)) } + +func TestEgressPolicyWithIPBlock(t *testing.T) { + const ( + fooPodIP = "10.32.0.10" + exceptIPSetName = "weave-LKf)d{$q[dkYlH3Ko6eFcdc7z" + ) + + m := newMockIPSet() + ipt := newMockIPTables() + controller := New("foo", false, ipt, &m) + + netpol := &networkingv1.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{ + UID: "netpol-foo", + Name: "allow-from-bar-to-foo", + Namespace: "default", + }, + Spec: networkingv1.NetworkPolicySpec{ + PolicyTypes: []networkingv1.PolicyType{ + networkingv1.PolicyTypeEgress, + }, + PodSelector: metav1.LabelSelector{MatchLabels: map[string]string{"run": "bar"}}, + Egress: []networkingv1.NetworkPolicyEgressRule{{ + To: []networkingv1.NetworkPolicyPeer{{ + IPBlock: &networkingv1.IPBlock{ + CIDR: "192.168.48.0/24", + Except: []string{ + "192.168.48.1/32", + "192.168.48.2/32", + }, + }, + }}, + }}, + }, + } + controller.AddNetworkPolicy(netpol) + + require.Equal(t, 2, len(m.sets[exceptIPSetName].subSets)) + require.True(t, m.entryExists(exceptIPSetName, "192.168.48.1/32")) + require.True(t, m.entryExists(exceptIPSetName, "192.168.48.2/32")) + + // Each egress rule is represented as two iptables rules (-J MARK and -J RETURN). + require.Equal(t, 2, len(ipt.rules[EgressCustomChain])) + for rule := range ipt.rules[EgressCustomChain] { + require.Contains(t, rule, "-d 192.168.48.0/24 -m set ! --match-set "+exceptIPSetName+" dst") + } + +} From 295ddc52482419647f80fcf46ef1151ffe47e116 Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Tue, 29 May 2018 14:34:41 +0200 Subject: [PATCH 28/46] npc: Try not to clear WEAVE-NPC-EGRESS chain First check whether the chain exists with the required rules, and if not - create it. If we flush each time we start weave-npc then we create a (short) time window during which all egress traffic is allowed. --- prog/weave-npc/main.go | 93 +++++++++++++++++++++++++----------------- 1 file changed, 56 insertions(+), 37 deletions(-) diff --git a/prog/weave-npc/main.go b/prog/weave-npc/main.go index dcda5813..a8663a18 100644 --- a/prog/weave-npc/main.go +++ b/prog/weave-npc/main.go @@ -3,9 +3,11 @@ package main import ( "os" "os/signal" + "strings" "syscall" "github.com/coreos/go-iptables/iptables" + "github.com/pkg/errors" "github.com/spf13/cobra" coreapi "k8s.io/api/core/v1" extnapi "k8s.io/api/extensions/v1beta1" @@ -69,9 +71,9 @@ func resetIPTables(ipt *iptables.IPTables) error { return err } - if err := ipt.ClearChain(npc.TableFilter, npc.EgressChain); err != nil { - return err - } + // We do not clear npc.EgressChain here because otherwise, in the case of restarting + // weave-npc process, all egress traffic is allowed for a short period of time. + // The chain is created in createBaseRules. return nil } @@ -97,6 +99,11 @@ func resetIPSets(ips ipset.Interface) error { } for _, s := range sets { + // LocalIPset might be used by WEAVE-NPC-EGRESS chain which we do not + // flush, so we cannot destroy it. + if s == npc.LocalIpset { + continue + } common.Log.Debugf("Destroying ipset '%s'", string(s)) if err := ips.Destroy(s); err != nil { common.Log.Errorf("Failed to destroy ipset '%s'", string(s)) @@ -145,44 +152,56 @@ func createBaseRules(ipt *iptables.IPTables, ips ipset.Interface) error { return err } - if err := ipt.Append(npc.TableFilter, npc.EgressChain, - "-m", "state", "--state", "RELATED,ESTABLISHED", "-j", "RETURN"); err != nil { - return err + ruleSpecs := [][]string{ + {"-m", "state", "--state", "RELATED,ESTABLISHED", "-j", "RETURN"}, + {"-m", "state", "--state", "NEW", "-m", "set", "!", "--match-set", npc.LocalIpset, "src", "-j", "RETURN"}, } if allowMcast { - if err := ipt.Append(npc.TableFilter, npc.EgressChain, - "-d", "224.0.0.0/4", "-j", "RETURN"); err != nil { - return err + ruleSpecs = append(ruleSpecs, []string{"-d", "224.0.0.0/4", "-j", "RETURN"}) + } + ruleSpecs = append(ruleSpecs, [][]string{ + {"-m", "state", "--state", "NEW", "-j", string(npc.EgressDefaultChain)}, + {"-m", "state", "--state", "NEW", "-m", "mark", "!", "--mark", npc.EgressMark, "-j", string(npc.EgressCustomChain)}, + {"-m", "state", "--state", "NEW", "-m", "mark", "!", "--mark", npc.EgressMark, "-j", "NFLOG", "--nflog-group", "86"}, + {"-m", "mark", "!", "--mark", npc.EgressMark, "-j", "DROP"}, + }...) + if err := addChainWithRules(ipt, npc.TableFilter, npc.EgressChain, ruleSpecs); err != nil { + return err + } + + return nil +} + +// addChainWithRules creates a chain and appends given rules to it. +// +// If the chain exists, but its rules are not the same as the given ones, the +// function will flush the chain and then will append the rules. +func addChainWithRules(ipt *iptables.IPTables, table, chain string, rulespecs [][]string) error { + // Create the chain ignoring any error because the chain might already exist + _ = ipt.NewChain(table, chain) + + currRuleSpecs, err := ipt.List(table, chain) + if err != nil { + return errors.Wrapf(err, "iptables -S. table: %q, chain: %q", table, chain) + } + + // First returned rule is "-N $(chain)", so ignore it + currRules := strings.Join(currRuleSpecs[1:], "\n") + rules := make([]string, 0) + for _, r := range rulespecs { + rules = append(rules, strings.Join(r, " ")) + } + reqRules := strings.Join(rules, "\n") + + if currRules == reqRules { + return nil + } + + for _, r := range rulespecs { + if err := ipt.Append(table, chain, r...); err != nil { + return errors.Wrapf(err, "iptables -A. table: %q, chain: %q, rule: %s", table, chain, r) } } - if err := ipt.Append(npc.TableFilter, npc.EgressChain, - "-m", "state", "--state", "NEW", - "-j", string(npc.EgressDefaultChain)); err != nil { - return err - } - if err := ipt.Append(npc.TableFilter, npc.EgressChain, - "-m", "state", "--state", "NEW", - "-m", "set", "!", "--match-set", npc.LocalIpset, "src", - "-j", "RETURN"); err != nil { - return err - } - if err := ipt.Append(npc.TableFilter, npc.EgressChain, - "-m", "state", "--state", "NEW", - "-m", "mark", "!", "--mark", npc.EgressMark, - "-j", string(npc.EgressCustomChain)); err != nil { - return err - } - if err := ipt.Append(npc.TableFilter, npc.EgressChain, - "-m", "state", "--state", "NEW", - "-m", "mark", "!", "--mark", npc.EgressMark, - "-j", "NFLOG", "--nflog-group", "86"); err != nil { - return err - } - if err := ipt.Append(npc.TableFilter, npc.EgressChain, - "-m", "mark", "!", "--mark", npc.EgressMark, - "-j", "DROP"); err != nil { - return err - } return nil } From a2d07c85dacb74f7529b08d6875ac5f3956aa79e Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Tue, 29 May 2018 14:58:48 +0200 Subject: [PATCH 29/46] npc: Remove WEAVE-NPC-EGRESS* chains upon "weave reset" Also, fix removal of rule which steers traffic for ingress processing. --- weave | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/weave b/weave index 599afb94..e5bb8b7d 100755 --- a/weave +++ b/weave @@ -492,14 +492,20 @@ destroy_bridge() { run_iptables -t filter -D FORWARD -o $BRIDGE -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT 2>/dev/null || true run_iptables -t filter -D FORWARD -i $BRIDGE -o $BRIDGE -j ACCEPT 2>/dev/null || true run_iptables -F WEAVE-NPC >/dev/null 2>&1 || true - run_iptables -t filter -D FORWARD -o $BRIDGE -j WEAVE-NPC 2>/dev/null || true + run_iptables -t filter -D FORWARD -o $BRIDGE -m comment --comment "NOTE: this must go before '-j KUBE-FORWARD'" -j WEAVE-NPC 2>/dev/null || true run_iptables -t filter -D FORWARD -o $BRIDGE -m state --state NEW -j NFLOG --nflog-group 86 2>/dev/null || true run_iptables -t filter -D FORWARD -o $BRIDGE -j DROP 2>/dev/null || true run_iptables -X WEAVE-NPC >/dev/null 2>&1 || true run_iptables -F WEAVE-NPC-EGRESS >/dev/null 2>&1 || true - run_iptables -t filter -D FORWARD -o $BRIDGE -j WEAVE-NPC-EGRESS 2>/dev/null || true + run_iptables -t filter -D FORWARD -i $BRIDGE -m comment --comment "NOTE: this must go before '-j KUBE-FORWARD'" -j WEAVE-NPC-EGRESS 2>/dev/null || true run_iptables -t filter -D INPUT -i $BRIDGE -j WEAVE-NPC-EGRESS 2>/dev/null || true run_iptables -X WEAVE-NPC-EGRESS >/dev/null 2>&1 || true + run_iptables -F WEAVE-NPC-EGRESS-DEFAULT >/dev/null 2>&1 || true + run_iptables -X WEAVE-NPC-EGRESS-DEFAULT >/dev/null 2>&1 || true + run_iptables -F WEAVE-NPC-EGRESS-CUSTOM >/dev/null 2>&1 || true + run_iptables -X WEAVE-NPC-EGRESS-CUSTOM >/dev/null 2>&1 || true + run_iptables -F WEAVE-NPC-EGRESS-ACCEPT >/dev/null 2>&1 || true + run_iptables -X WEAVE-NPC-EGRESS-ACCEPT >/dev/null 2>&1 || true run_iptables -F WEAVE-EXPOSE >/dev/null 2>&1 || true run_iptables -t filter -D FORWARD -o $BRIDGE -j WEAVE-EXPOSE 2>/dev/null || true From 86958be6cfb4690aa853fa66c66a69f062a1d38b Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Wed, 30 May 2018 13:12:44 +0200 Subject: [PATCH 30/46] npc: Add --use-legacy-netpol flag to weaver Egress does not work in the legacy netpol mode, so in the legacy case we don't have to create the egress traffic steering rules. Otherwise, some traffic will be blocked. --- net/bridge.go | 19 ++++++++++--------- prog/weaver/main.go | 1 + 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/net/bridge.go b/net/bridge.go index 07567a15..d511b757 100644 --- a/net/bridge.go +++ b/net/bridge.go @@ -216,6 +216,7 @@ type BridgeConfig struct { NoBridgedFastdp bool AWSVPC bool NPC bool + LegacyNPC bool MTU int Mac string Port int @@ -461,20 +462,20 @@ func configureIPTables(config *BridgeConfig, ips ipset.Interface) error { // Ignore error because chains might already exist // TODO(brb): use constants for WEAVE-NPC* chains _ = ipt.NewChain("filter", "WEAVE-NPC") - _ = ipt.NewChain("filter", "WEAVE-NPC-EGRESS") - - // TODO(brb): this rule might slow down packet processing; consider removing at it applies only - // to node-local traffic. - if err = ipt.AppendUnique("filter", "INPUT", "-i", config.WeaveBridgeName, "-j", "WEAVE-NPC-EGRESS"); err != nil { - return err + if !config.LegacyNPC { + _ = ipt.NewChain("filter", "WEAVE-NPC-EGRESS") + if err = ipt.AppendUnique("filter", "INPUT", "-i", config.WeaveBridgeName, "-j", "WEAVE-NPC-EGRESS"); err != nil { + return err + } + fwdRules = append(fwdRules, []string{ + "-i", config.WeaveBridgeName, + "-m", "comment", "--comment", "NOTE: this must go before '-j KUBE-FORWARD'", + "-j", "WEAVE-NPC-EGRESS"}) } // If WEAVE-NPC chain doesn't exist then creating a rule in the chain will fail fwdRules = append(fwdRules, [][]string{ - {"-i", config.WeaveBridgeName, - "-m", "comment", "--comment", "NOTE: this must go before '-j KUBE-FORWARD'", - "-j", "WEAVE-NPC-EGRESS"}, {"-o", config.WeaveBridgeName, "-m", "comment", "--comment", "NOTE: this must go before '-j KUBE-FORWARD'", "-j", "WEAVE-NPC"}, diff --git a/prog/weaver/main.go b/prog/weaver/main.go index 79c1b65a..93622c5a 100644 --- a/prog/weaver/main.go +++ b/prog/weaver/main.go @@ -182,6 +182,7 @@ func main() { mflag.StringVar(&bridgeConfig.WeaveBridgeName, []string{"-weave-bridge"}, "weave", "name of weave bridge") mflag.StringVar(&bridgeConfig.DockerBridgeName, []string{"-docker-bridge"}, "", "name of Docker bridge") mflag.BoolVar(&bridgeConfig.NPC, []string{"-expect-npc"}, false, "set up iptables rules for npc") + mflag.BoolVar(&bridgeConfig.LegacyNPC, []string{"-use-legacy-netpol"}, false, "use legacy network policies (pre k8s 1.7 vsn)") mflag.StringVar(&routerName, []string{"-name"}, "", "name of router (defaults to MAC of interface)") mflag.StringVar(&nickName, []string{"-nickname"}, "", "nickname of peer (defaults to hostname)") mflag.StringVar(&password, []string{"-password"}, "", "network password") From be8298926a4a2e5b4c476c641c9af979cc27cb00 Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Wed, 30 May 2018 13:20:43 +0200 Subject: [PATCH 31/46] npc: Pass --use-legacy-netpol to weaver in 840 --- test/840_weave_kube_3_test.sh | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/test/840_weave_kube_3_test.sh b/test/840_weave_kube_3_test.sh index 7ae42224..a2679203 100755 --- a/test/840_weave_kube_3_test.sh +++ b/test/840_weave_kube_3_test.sh @@ -44,9 +44,9 @@ for host in $HOSTS; do done if [ -n "$COVERAGE" ]; then - WEAVE_ENV_VARS="env:\\n - name: EXTRA_ARGS\\n value: \"-test.coverprofile=/home/weave/cover.prof --\"" + WEAVE_ENV_VARS="env:\\n - name: EXTRA_ARGS\\n value: \"-test.coverprofile=/home/weave/cover.prof -- --use-legacy-netpol\"" else - WEAVE_ENV_VARS="env:" + WEAVE_ENV_VARS="env:\\n - name: EXTRA_ARGS\\n value: \"--use-legacy-netpol\"" fi # Ensure Kubernetes uses locally built container images and inject code coverage environment variable (or do nothing depending on $COVERAGE): @@ -179,6 +179,12 @@ run_on $HOST1 "$KUBECTL run nettest-deny --labels=\"access=deny,run=nettest-deny denyPodName=$($SSH $HOST1 "$KUBECTL get pods -l run=nettest-deny -o go-template='{{(index .items 0).metadata.name}}'") assert_raises "! $SSH $HOST1 $KUBECTL exec $denyPodName -- curl -s -S -f -m 2 http://$DOMAIN:8080/status >/dev/null" +if [ -n "$COVERAGE" ]; then + WEAVE_ENV_VARS="env:\\n - name: EXTRA_ARGS\\n value: \"-test.coverprofile=/home/weave/cover.prof --\"" +else + WEAVE_ENV_VARS="env:" +fi + # Restart weave-net with npc in non-legacy mode $SSH $HOST1 "$KUBECTL delete ds weave-net -n=kube-system" sed -e "s%imagePullPolicy: Always%imagePullPolicy: Never%" \ From 18a6237ec7713161f6fde5712d8d83e26e6a829a Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Wed, 30 May 2018 20:42:19 +0200 Subject: [PATCH 32/46] npc: Check whether weave-local-pods exists before creating We do not destroy it anymore, thus the check. --- prog/weave-npc/main.go | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/prog/weave-npc/main.go b/prog/weave-npc/main.go index a8663a18..f8c6575d 100644 --- a/prog/weave-npc/main.go +++ b/prog/weave-npc/main.go @@ -139,9 +139,15 @@ func createBaseRules(ipt *iptables.IPTables, ips ipset.Interface) error { } // If the destination address is not any of the local pods, let it through - if err := ips.Create(npc.LocalIpset, ipset.HashIP); err != nil { + found, err := ipsetExist(ips, npc.LocalIpset) + if err != nil { return err } + if !found { + if err := ips.Create(npc.LocalIpset, ipset.HashIP); err != nil { + return err + } + } if err := ipt.Append(npc.TableFilter, npc.MainChain, "-m", "set", "!", "--match-set", npc.LocalIpset, "dst", "-j", "ACCEPT"); err != nil { return err @@ -206,6 +212,22 @@ func addChainWithRules(ipt *iptables.IPTables, table, chain string, rulespecs [] return nil } +// Dummy way to check whether a given ipset exists. +// TODO(brb) Use "ipset -exist create <..>" for our purpose instead (for some reasons +// creating an ipset with -exist fails). +func ipsetExist(ips ipset.Interface, name ipset.Name) (bool, error) { + sets, err := ips.List(string(name)) + if err != nil { + return false, err + } + for _, s := range sets { + if s == name { + return true, nil + } + } + return false, nil +} + func root(cmd *cobra.Command, args []string) { var npController cache.Controller From 9a63ade1b5b633f8c42b1f7eb7d9de3d6729cea4 Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Wed, 30 May 2018 20:57:21 +0200 Subject: [PATCH 33/46] npc: Do not create WEAVE-NPC-EGRESS* chains in legacy netpol mode --- prog/weave-npc/main.go | 44 ++++++++++++++++++++++-------------------- 1 file changed, 23 insertions(+), 21 deletions(-) diff --git a/prog/weave-npc/main.go b/prog/weave-npc/main.go index f8c6575d..276c90e7 100644 --- a/prog/weave-npc/main.go +++ b/prog/weave-npc/main.go @@ -114,7 +114,7 @@ func resetIPSets(ips ipset.Interface) error { return nil } -func createBaseRules(ipt *iptables.IPTables, ips ipset.Interface) error { +func createBaseRules(ipt *iptables.IPTables, ips ipset.Interface, legacy bool) error { // Configure main chain static rules if err := ipt.Append(npc.TableFilter, npc.MainChain, "-m", "state", "--state", "RELATED,ESTABLISHED", "-j", "ACCEPT"); err != nil { @@ -153,26 +153,28 @@ func createBaseRules(ipt *iptables.IPTables, ips ipset.Interface) error { return err } - if err := ipt.Append(npc.TableFilter, npc.EgressMarkChain, - "-j", "MARK", "--set-xmark", npc.EgressMark); err != nil { - return err - } + if !legacy { + if err := ipt.Append(npc.TableFilter, npc.EgressMarkChain, + "-j", "MARK", "--set-xmark", npc.EgressMark); err != nil { + return err + } - ruleSpecs := [][]string{ - {"-m", "state", "--state", "RELATED,ESTABLISHED", "-j", "RETURN"}, - {"-m", "state", "--state", "NEW", "-m", "set", "!", "--match-set", npc.LocalIpset, "src", "-j", "RETURN"}, - } - if allowMcast { - ruleSpecs = append(ruleSpecs, []string{"-d", "224.0.0.0/4", "-j", "RETURN"}) - } - ruleSpecs = append(ruleSpecs, [][]string{ - {"-m", "state", "--state", "NEW", "-j", string(npc.EgressDefaultChain)}, - {"-m", "state", "--state", "NEW", "-m", "mark", "!", "--mark", npc.EgressMark, "-j", string(npc.EgressCustomChain)}, - {"-m", "state", "--state", "NEW", "-m", "mark", "!", "--mark", npc.EgressMark, "-j", "NFLOG", "--nflog-group", "86"}, - {"-m", "mark", "!", "--mark", npc.EgressMark, "-j", "DROP"}, - }...) - if err := addChainWithRules(ipt, npc.TableFilter, npc.EgressChain, ruleSpecs); err != nil { - return err + ruleSpecs := [][]string{ + {"-m", "state", "--state", "RELATED,ESTABLISHED", "-j", "RETURN"}, + {"-m", "state", "--state", "NEW", "-m", "set", "!", "--match-set", npc.LocalIpset, "src", "-j", "RETURN"}, + } + if allowMcast { + ruleSpecs = append(ruleSpecs, []string{"-d", "224.0.0.0/4", "-j", "RETURN"}) + } + ruleSpecs = append(ruleSpecs, [][]string{ + {"-m", "state", "--state", "NEW", "-j", string(npc.EgressDefaultChain)}, + {"-m", "state", "--state", "NEW", "-m", "mark", "!", "--mark", npc.EgressMark, "-j", string(npc.EgressCustomChain)}, + {"-m", "state", "--state", "NEW", "-m", "mark", "!", "--mark", npc.EgressMark, "-j", "NFLOG", "--nflog-group", "86"}, + {"-m", "mark", "!", "--mark", npc.EgressMark, "-j", "DROP"}, + }...) + if err := addChainWithRules(ipt, npc.TableFilter, npc.EgressChain, ruleSpecs); err != nil { + return err + } } return nil @@ -266,7 +268,7 @@ func root(cmd *cobra.Command, args []string) { handleError(resetIPTables(ipt)) handleError(resetIPSets(ips)) - handleError(createBaseRules(ipt, ips)) + handleError(createBaseRules(ipt, ips, legacy)) npc := npc.New(nodeName, legacy, ipt, ips) From dee24aec6541ab6fa266dcd102f69eaab4098111 Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Thu, 7 Jun 2018 09:31:49 +0200 Subject: [PATCH 34/46] npc: Add integration test for egress Also, install dig (bind-tools) in the network-tester image. --- test/840_weave_kube_3_test.sh | 55 +++++++++++++++++++++++++++ test/images/network-tester/Dockerfile | 2 +- 2 files changed, 56 insertions(+), 1 deletion(-) diff --git a/test/840_weave_kube_3_test.sh b/test/840_weave_kube_3_test.sh index a2679203..a0001937 100755 --- a/test/840_weave_kube_3_test.sh +++ b/test/840_weave_kube_3_test.sh @@ -275,6 +275,61 @@ EOF assert_raises "$SSH $HOST1 curl -s -S -f -m 2 http://$VIRTUAL_IP/status >/dev/null" assert_raises "$SSH $HOST1 curl -s -S -f -m 2 http://$HOST2:31138/status >/dev/null" +# allow nettest-deny to access only to 8.8.8.0/24 +run_on $HOST1 "$KUBECTL apply -f -" </dev/null" +assert_raises "! $SSH $HOST1 $KUBECTL exec $denyPodName -- dig @8.8.8.4 google.com >/dev/null" +assert_raises "$SSH $HOST1 $KUBECTL exec $denyPodName -- dig @8.8.8.8 google.com >/dev/null" + +# also, allow nettest-deny to access nettest and kube-system (for kube-dns) +run_on $HOST1 "$KUBECTL label namespace kube-system name=kube-system" +run_on $HOST1 "$KUBECTL replace -f -" </dev/null" + # Passing --no-masq-local and setting externalTrafficPolicy to Local must preserve # the client source IP addr diff --git a/test/images/network-tester/Dockerfile b/test/images/network-tester/Dockerfile index 149b92c3..a7be5948 100644 --- a/test/images/network-tester/Dockerfile +++ b/test/images/network-tester/Dockerfile @@ -13,7 +13,7 @@ # limitations under the License. FROM bash -RUN apk add --no-cache curl +RUN apk add --no-cache curl bind-tools ADD webserver webserver EXPOSE 8080 ENTRYPOINT ["/webserver"] From 49224ec9f7c3be372cd64068850d7460ce83c397 Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Thu, 7 Jun 2018 10:29:54 +0200 Subject: [PATCH 35/46] npc: Rename policy{Ingress,Egress} to policyType{Ingress,Egress} --- npc/analyser.go | 30 +++++++++++++++--------------- npc/namespace.go | 6 +++--- npc/rule.go | 6 +++--- npc/rule_test.go | 2 +- npc/selector.go | 4 ++-- npc/types.go | 6 +++--- 6 files changed, 27 insertions(+), 27 deletions(-) diff --git a/npc/analyser.go b/npc/analyser.go index 48f358b5..6d44bac3 100644 --- a/npc/analyser.go +++ b/npc/analyser.go @@ -24,7 +24,7 @@ func (ns *ns) analysePolicyLegacy(policy *extnapi.NetworkPolicy) ( podSelectors = make(map[string]*selectorSpec) rules = make(map[string]*ruleSpec) - policyTypes := []policyType{ingressPolicy} + policyTypes := []policyType{policyTypeIngress} targetSelector, err := newSelectorSpec(&policy.Spec.PodSelector, policyTypes, ns.name, ipset.HashIP) if err != nil { return nil, nil, nil, err @@ -46,13 +46,13 @@ func (ns *ns) analysePolicyLegacy(policy *extnapi.NetworkPolicy) ( // From is not provided, this rule matches all sources (traffic not restricted by source). if ingressRule.Ports == nil { // Ports is not provided, this rule matches all ports (traffic not restricted by port). - rule := newRuleSpec(ingressPolicy, nil, nil, targetSelector, nil) + rule := newRuleSpec(policyTypeIngress, nil, nil, targetSelector, nil) rules[rule.key] = rule } else { // Ports is present and contains at least one item, then this rule allows traffic // only if the traffic matches at least one port in the ports list. withNormalisedProtoAndPortLegacy(ingressRule.Ports, func(proto, port string) { - rule := newRuleSpec(ingressPolicy, &proto, nil, targetSelector, &port) + rule := newRuleSpec(policyTypeIngress, &proto, nil, targetSelector, &port) rules[rule.key] = rule }) } @@ -78,13 +78,13 @@ func (ns *ns) analysePolicyLegacy(policy *extnapi.NetworkPolicy) ( if ingressRule.Ports == nil { // Ports is not provided, this rule matches all ports (traffic not restricted by port). - rule := newRuleSpec(ingressPolicy, nil, srcSelector, targetSelector, nil) + rule := newRuleSpec(policyTypeIngress, nil, srcSelector, targetSelector, nil) rules[rule.key] = rule } else { // Ports is present and contains at least one item, then this rule allows traffic // only if the traffic matches at least one port in the ports list. withNormalisedProtoAndPortLegacy(ingressRule.Ports, func(proto, port string) { - rule := newRuleSpec(ingressPolicy, &proto, srcSelector, targetSelector, &port) + rule := newRuleSpec(policyTypeIngress, &proto, srcSelector, targetSelector, &port) rules[rule.key] = rule }) } @@ -109,10 +109,10 @@ func (ns *ns) analysePolicy(policy *networkingv1.NetworkPolicy) ( for _, pt := range policy.Spec.PolicyTypes { if pt == networkingv1.PolicyTypeIngress { - policyTypes = append(policyTypes, ingressPolicy) + policyTypes = append(policyTypes, policyTypeIngress) } if pt == networkingv1.PolicyTypeEgress { - policyTypes = append(policyTypes, egressPolicy) + policyTypes = append(policyTypes, policyTypeEgress) } } // If empty, matches all pods in a namespace @@ -137,11 +137,11 @@ func (ns *ns) analysePolicy(policy *networkingv1.NetworkPolicy) ( if allSources { if allPorts { - rule := newRuleSpec(ingressPolicy, nil, nil, targetSelector, nil) + rule := newRuleSpec(policyTypeIngress, nil, nil, targetSelector, nil) rules[rule.key] = rule } else { withNormalisedProtoAndPort(ingressRule.Ports, func(proto, port string) { - rule := newRuleSpec(ingressPolicy, &proto, nil, targetSelector, &port) + rule := newRuleSpec(policyTypeIngress, &proto, nil, targetSelector, &port) rules[rule.key] = rule }) } @@ -167,11 +167,11 @@ func (ns *ns) analysePolicy(policy *networkingv1.NetworkPolicy) ( } if allPorts { - rule := newRuleSpec(ingressPolicy, nil, srcSelector, targetSelector, nil) + rule := newRuleSpec(policyTypeIngress, nil, srcSelector, targetSelector, nil) rules[rule.key] = rule } else { withNormalisedProtoAndPort(ingressRule.Ports, func(proto, port string) { - rule := newRuleSpec(ingressPolicy, &proto, srcSelector, targetSelector, &port) + rule := newRuleSpec(policyTypeIngress, &proto, srcSelector, targetSelector, &port) rules[rule.key] = rule }) } @@ -190,11 +190,11 @@ func (ns *ns) analysePolicy(policy *networkingv1.NetworkPolicy) ( if allDestinations { if allPorts { - rule := newRuleSpec(egressPolicy, nil, targetSelector, nil, nil) + rule := newRuleSpec(policyTypeEgress, nil, targetSelector, nil, nil) rules[rule.key] = rule } else { withNormalisedProtoAndPort(egressRule.Ports, func(proto, port string) { - rule := newRuleSpec(egressPolicy, &proto, targetSelector, nil, &port) + rule := newRuleSpec(policyTypeEgress, &proto, targetSelector, nil, &port) rules[rule.key] = rule }) } @@ -227,11 +227,11 @@ func (ns *ns) analysePolicy(policy *networkingv1.NetworkPolicy) ( } if allPorts { - rule := newRuleSpec(egressPolicy, nil, targetSelector, dstRuleHost, nil) + rule := newRuleSpec(policyTypeEgress, nil, targetSelector, dstRuleHost, nil) rules[rule.key] = rule } else { withNormalisedProtoAndPort(egressRule.Ports, func(proto, port string) { - rule := newRuleSpec(egressPolicy, &proto, targetSelector, dstRuleHost, &port) + rule := newRuleSpec(policyTypeEgress, &proto, targetSelector, dstRuleHost, &port) rules[rule.key] = rule }) } diff --git a/npc/namespace.go b/npc/namespace.go index a09d2e1e..01b9a892 100644 --- a/npc/namespace.go +++ b/npc/namespace.go @@ -288,10 +288,10 @@ func (ns *ns) updatePod(oldObj, newObj *coreapi.Pod) error { } if !ns.legacy { - if err := ns.addOrRemoveToDefaultAllowIPSet(ps, oldObj, newObj, oldMatch, newMatch, ingressPolicy); err != nil { + if err := ns.addOrRemoveToDefaultAllowIPSet(ps, oldObj, newObj, oldMatch, newMatch, policyTypeIngress); err != nil { return err } - if err := ns.addOrRemoveToDefaultAllowIPSet(ps, oldObj, newObj, oldMatch, newMatch, egressPolicy); err != nil { + if err := ns.addOrRemoveToDefaultAllowIPSet(ps, oldObj, newObj, oldMatch, newMatch, policyTypeEgress); err != nil { return err } } @@ -645,7 +645,7 @@ func podComment(pod *coreapi.Pod) string { func (ns *ns) defaultAllowIPSetName(pt policyType) ipset.Name { ipset := ns.ingressDefaultAllowIPSet - if pt == egressPolicy { + if pt == policyTypeEgress { ipset = ns.egressDefaultAllowIPSet } return ipset diff --git a/npc/rule.go b/npc/rule.go index a08a3a20..b90fd157 100644 --- a/npc/rule.go +++ b/npc/rule.go @@ -55,21 +55,21 @@ func newRuleSpec(policyType policyType, proto *string, srcHost, dstHost ruleHost } func (spec *ruleSpec) iptChain() string { - if spec.policyType == egressPolicy { + if spec.policyType == policyTypeEgress { return EgressCustomChain } return IngressChain } func (spec *ruleSpec) iptRuleSpecs() [][]string { - if spec.policyType == ingressPolicy { + if spec.policyType == policyTypeIngress { rule := make([]string, len(spec.args)) copy(rule, spec.args) rule = append(rule, "-j", "ACCEPT") return [][]string{rule} } - // egressPolicy + // policyTypeEgress ruleMark := make([]string, len(spec.args)) copy(ruleMark, spec.args) ruleMark = append(ruleMark, "-j", EgressMarkChain) diff --git a/npc/rule_test.go b/npc/rule_test.go index 5fac10c7..15535449 100644 --- a/npc/rule_test.go +++ b/npc/rule_test.go @@ -9,7 +9,7 @@ func TestRegressionHandleNil3095(t *testing.T) { // Test for handling nil values // https://github.com/weaveworks/weave/issues/3095 - rs := newRuleSpec(ingressPolicy, nil, nil, nil, nil) + rs := newRuleSpec(policyTypeIngress, nil, nil, nil, nil) require.NotEqual(t, rs.key, "") diff --git a/npc/selector.go b/npc/selector.go index 51eed64f..54848512 100644 --- a/npc/selector.go +++ b/npc/selector.go @@ -114,10 +114,10 @@ func (ss *selectorSet) addToMatching(user types.UID, labelMap map[string]string, foundEgress := false for _, s := range ss.entries { if s.matches(labelMap) { - if ss.targetSelectorExist(s, ingressPolicy) { + if ss.targetSelectorExist(s, policyTypeIngress) { foundIngress = true } - if ss.targetSelectorExist(s, egressPolicy) { + if ss.targetSelectorExist(s, policyTypeEgress) { foundEgress = true } if err := s.addEntry(user, entry, comment); err != nil { diff --git a/npc/types.go b/npc/types.go index 3b594ff9..608f07f0 100644 --- a/npc/types.go +++ b/npc/types.go @@ -3,12 +3,12 @@ package npc type policyType byte const ( - ingressPolicy policyType = iota - egressPolicy + policyTypeIngress policyType = iota + policyTypeEgress ) func policyTypeStr(policyType policyType) string { - if policyType == egressPolicy { + if policyType == policyTypeEgress { return "egress" } return "ingress" From cf3a3be8f25c136e59748837766f1a5a0f5f531e Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Thu, 7 Jun 2018 14:20:49 +0200 Subject: [PATCH 36/46] npc: Replace isNil by reflect.IsNil --- npc/ipblock.go | 4 ---- npc/rule.go | 7 +++---- npc/selector.go | 4 ---- 3 files changed, 3 insertions(+), 12 deletions(-) diff --git a/npc/ipblock.go b/npc/ipblock.go index 0c5284ae..c40e46e2 100644 --- a/npc/ipblock.go +++ b/npc/ipblock.go @@ -48,10 +48,6 @@ func (spec *ipBlockSpec) getRuleSpec(src bool) ([]string, string) { return rule, comment } -func (spec *ipBlockSpec) isNil() bool { - return spec == nil -} - type ipBlockSet struct { ips ipset.Interface users map[string]map[types.UID]struct{} diff --git a/npc/rule.go b/npc/rule.go index b90fd157..1edee770 100644 --- a/npc/rule.go +++ b/npc/rule.go @@ -2,6 +2,7 @@ package npc import ( "fmt" + "reflect" "strings" "k8s.io/apimachinery/pkg/types" @@ -16,8 +17,6 @@ type ruleHost interface { // // If src=true then the rulespec is for source, otherwise - for destination. getRuleSpec(src bool) ([]string, string) - // TODO(brb) Get rid - isNil() bool } type ruleSpec struct { @@ -32,13 +31,13 @@ func newRuleSpec(policyType policyType, proto *string, srcHost, dstHost ruleHost args = append(args, "-p", *proto) } srcComment := "anywhere" - if srcHost != nil && !srcHost.isNil() { + if srcHost != nil && !reflect.ValueOf(srcHost).IsNil() { rule, comment := srcHost.getRuleSpec(true) args = append(args, rule...) srcComment = comment } dstComment := "anywhere" - if dstHost != nil && !dstHost.isNil() { + if dstHost != nil && !reflect.ValueOf(dstHost).IsNil() { rule, comment := dstHost.getRuleSpec(false) args = append(args, rule...) dstComment = comment diff --git a/npc/selector.go b/npc/selector.go index 54848512..6280f26a 100644 --- a/npc/selector.go +++ b/npc/selector.go @@ -56,10 +56,6 @@ func (spec *selectorSpec) getRuleSpec(src bool) ([]string, string) { return rule, comment } -func (spec *selectorSpec) isNil() bool { - return spec == nil -} - type selector struct { ips ipset.Interface spec *selectorSpec From 07d31a85581cd3122f2c53a3c04f24d0980bc9dd Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Thu, 7 Jun 2018 14:50:39 +0200 Subject: [PATCH 37/46] npc: Get rid of the legacy netpol --- net/bridge.go | 17 +- npc/analyser.go | 91 -------- npc/controller.go | 7 +- npc/controller_test.go | 14 +- npc/namespace.go | 214 +++++-------------- prog/weave-kube/weave-daemonset-k8s-1.7.yaml | 9 - prog/weave-npc/main.go | 59 ++--- prog/weaver/main.go | 1 - site/kubernetes/kube-addon.md | 1 - test/840_weave_kube_3_test.sh | 39 ++-- 10 files changed, 111 insertions(+), 341 deletions(-) diff --git a/net/bridge.go b/net/bridge.go index d511b757..07aedc15 100644 --- a/net/bridge.go +++ b/net/bridge.go @@ -216,7 +216,6 @@ type BridgeConfig struct { NoBridgedFastdp bool AWSVPC bool NPC bool - LegacyNPC bool MTU int Mac string Port int @@ -462,16 +461,14 @@ func configureIPTables(config *BridgeConfig, ips ipset.Interface) error { // Ignore error because chains might already exist // TODO(brb): use constants for WEAVE-NPC* chains _ = ipt.NewChain("filter", "WEAVE-NPC") - if !config.LegacyNPC { - _ = ipt.NewChain("filter", "WEAVE-NPC-EGRESS") - if err = ipt.AppendUnique("filter", "INPUT", "-i", config.WeaveBridgeName, "-j", "WEAVE-NPC-EGRESS"); err != nil { - return err - } - fwdRules = append(fwdRules, []string{ - "-i", config.WeaveBridgeName, - "-m", "comment", "--comment", "NOTE: this must go before '-j KUBE-FORWARD'", - "-j", "WEAVE-NPC-EGRESS"}) + _ = ipt.NewChain("filter", "WEAVE-NPC-EGRESS") + if err = ipt.AppendUnique("filter", "INPUT", "-i", config.WeaveBridgeName, "-j", "WEAVE-NPC-EGRESS"); err != nil { + return err } + fwdRules = append(fwdRules, []string{ + "-i", config.WeaveBridgeName, + "-m", "comment", "--comment", "NOTE: this must go before '-j KUBE-FORWARD'", + "-j", "WEAVE-NPC-EGRESS"}) // If WEAVE-NPC chain doesn't exist then creating a rule in the chain will fail fwdRules = append(fwdRules, diff --git a/npc/analyser.go b/npc/analyser.go index 6d44bac3..2675ce1c 100644 --- a/npc/analyser.go +++ b/npc/analyser.go @@ -4,97 +4,12 @@ import ( "fmt" apiv1 "k8s.io/api/core/v1" - extnapi "k8s.io/api/extensions/v1beta1" networkingv1 "k8s.io/api/networking/v1" "k8s.io/apimachinery/pkg/util/intstr" "github.com/weaveworks/weave/net/ipset" ) -// analysePolicyLegacy is used to analyse extensions/v1beta1/NetworkPolicy (legacy) and -// implements pre-1.7 k8s netpol semantics, whilst analysePolicy - networking.k8s.io/v1/NetworkPolicy -// and 1.7 semantics. - -func (ns *ns) analysePolicyLegacy(policy *extnapi.NetworkPolicy) ( - rules map[string]*ruleSpec, - nsSelectors, podSelectors map[string]*selectorSpec, - err error) { - - nsSelectors = make(map[string]*selectorSpec) - podSelectors = make(map[string]*selectorSpec) - rules = make(map[string]*ruleSpec) - - policyTypes := []policyType{policyTypeIngress} - targetSelector, err := newSelectorSpec(&policy.Spec.PodSelector, policyTypes, ns.name, ipset.HashIP) - if err != nil { - return nil, nil, nil, err - } - podSelectors[targetSelector.key] = targetSelector - - for _, ingressRule := range policy.Spec.Ingress { - if ingressRule.Ports != nil && len(ingressRule.Ports) == 0 { - // Ports is empty, this rule matches no ports (no traffic matches). - continue - } - - if ingressRule.From != nil && len(ingressRule.From) == 0 { - // From is empty, this rule matches no sources (no traffic matches). - continue - } - - if ingressRule.From == nil { - // From is not provided, this rule matches all sources (traffic not restricted by source). - if ingressRule.Ports == nil { - // Ports is not provided, this rule matches all ports (traffic not restricted by port). - rule := newRuleSpec(policyTypeIngress, nil, nil, targetSelector, nil) - rules[rule.key] = rule - } else { - // Ports is present and contains at least one item, then this rule allows traffic - // only if the traffic matches at least one port in the ports list. - withNormalisedProtoAndPortLegacy(ingressRule.Ports, func(proto, port string) { - rule := newRuleSpec(policyTypeIngress, &proto, nil, targetSelector, &port) - rules[rule.key] = rule - }) - } - } else { - // From is present and contains at least on item, this rule allows traffic only if the - // traffic matches at least one item in the from list. - for _, peer := range ingressRule.From { - var srcSelector *selectorSpec - if peer.PodSelector != nil { - srcSelector, err = newSelectorSpec(peer.PodSelector, nil, ns.name, ipset.HashIP) - if err != nil { - return nil, nil, nil, err - } - podSelectors[srcSelector.key] = srcSelector - } - if peer.NamespaceSelector != nil { - srcSelector, err = newSelectorSpec(peer.NamespaceSelector, nil, "", ipset.ListSet) - if err != nil { - return nil, nil, nil, err - } - nsSelectors[srcSelector.key] = srcSelector - } - - if ingressRule.Ports == nil { - // Ports is not provided, this rule matches all ports (traffic not restricted by port). - rule := newRuleSpec(policyTypeIngress, nil, srcSelector, targetSelector, nil) - rules[rule.key] = rule - } else { - // Ports is present and contains at least one item, then this rule allows traffic - // only if the traffic matches at least one port in the ports list. - withNormalisedProtoAndPortLegacy(ingressRule.Ports, func(proto, port string) { - rule := newRuleSpec(policyTypeIngress, &proto, srcSelector, targetSelector, &port) - rules[rule.key] = rule - }) - } - } - } - } - - return rules, nsSelectors, podSelectors, nil -} - func (ns *ns) analysePolicy(policy *networkingv1.NetworkPolicy) ( rules map[string]*ruleSpec, nsSelectors, podSelectors map[string]*selectorSpec, @@ -249,12 +164,6 @@ func addIfNotExist(s *selectorSpec, ss map[string]*selectorSpec) { } } -func withNormalisedProtoAndPortLegacy(npps []extnapi.NetworkPolicyPort, f func(proto, port string)) { - for _, npp := range npps { - f(proto(npp.Protocol), port(npp.Port)) - } -} - func withNormalisedProtoAndPort(npps []networkingv1.NetworkPolicyPort, f func(proto, port string)) { for _, npp := range npps { f(proto(npp.Protocol), port(npp.Port)) diff --git a/npc/controller.go b/npc/controller.go index 4b3415a4..e8425993 100644 --- a/npc/controller.go +++ b/npc/controller.go @@ -37,14 +37,11 @@ type controller struct { nss map[string]*ns // ns name -> ns struct nsSelectors *selectorSet // selector string -> nsSelector - - legacy bool // use legacy network policy semantics (k8s pre-1.7) } -func New(nodeName string, legacy bool, ipt iptables.Interface, ips ipset.Interface) NetworkPolicyController { +func New(nodeName string, ipt iptables.Interface, ips ipset.Interface) NetworkPolicyController { c := &controller{ nodeName: nodeName, - legacy: legacy, ipt: ipt, ips: ips, nss: make(map[string]*ns)} @@ -71,7 +68,7 @@ func (npc *controller) onNewNsSelector(selector *selector) error { func (npc *controller) withNS(name string, f func(ns *ns) error) error { ns, found := npc.nss[name] if !found { - newNs, err := newNS(name, npc.nodeName, npc.legacy, npc.ipt, npc.ips, npc.nsSelectors) + newNs, err := newNS(name, npc.nodeName, npc.ipt, npc.ips, npc.nsSelectors) if err != nil { return err } diff --git a/npc/controller_test.go b/npc/controller_test.go index 0a59479d..fac426dd 100644 --- a/npc/controller_test.go +++ b/npc/controller_test.go @@ -198,7 +198,7 @@ func TestRegressionPolicyNamespaceOrdering3059(t *testing.T) { // Namespaces first m := newMockIPSet() - controller := New("foo", false, newMockIPTables(), &m) + controller := New("foo", newMockIPTables(), &m) const ( selectorIPSetName = "weave-I239Zp%sCvoVt*D6u=A!2]YEk" @@ -214,7 +214,7 @@ func TestRegressionPolicyNamespaceOrdering3059(t *testing.T) { // NetworkPolicy first m = newMockIPSet() - controller = New("foo", false, newMockIPTables(), &m) + controller = New("foo", newMockIPTables(), &m) controller.AddNetworkPolicy(networkPolicy) @@ -224,7 +224,7 @@ func TestRegressionPolicyNamespaceOrdering3059(t *testing.T) { require.Equal(t, true, len(m.sets[selectorIPSetName].subSets[sourceIPSetName]) > 0) } -// Tests default-allow ipset behavior when running in non-legacy mode. +// Tests default-allow ipset behavior func TestDefaultAllow(t *testing.T) { const ( ingressDefaultAllowIPSetName = "weave-;rGqyMIl1HN^cfDki~Z$3]6!N" @@ -235,7 +235,7 @@ func TestDefaultAllow(t *testing.T) { ) m := newMockIPSet() - controller := New("bar", false, newMockIPTables(), &m) + controller := New("bar", newMockIPTables(), &m) defaultNamespace := &coreapi.Namespace{ ObjectMeta: metav1.ObjectMeta{ @@ -363,7 +363,7 @@ func TestOutOfOrderPodEvents(t *testing.T) { ) m := newMockIPSet() - controller := New("qux", false, newMockIPTables(), &m) + controller := New("qux", newMockIPTables(), &m) defaultNamespace := &coreapi.Namespace{ ObjectMeta: metav1.ObjectMeta{ @@ -445,7 +445,7 @@ func TestNewTargetSelector(t *testing.T) { ) m := newMockIPSet() - controller := New("baz", false, newMockIPTables(), &m) + controller := New("baz", newMockIPTables(), &m) defaultNamespace := &coreapi.Namespace{ ObjectMeta: metav1.ObjectMeta{ @@ -520,7 +520,7 @@ func TestEgressPolicyWithIPBlock(t *testing.T) { m := newMockIPSet() ipt := newMockIPTables() - controller := New("foo", false, ipt, &m) + controller := New("foo", ipt, &m) netpol := &networkingv1.NetworkPolicy{ ObjectMeta: metav1.ObjectMeta{ diff --git a/npc/namespace.go b/npc/namespace.go index 01b9a892..9d43f1f8 100644 --- a/npc/namespace.go +++ b/npc/namespace.go @@ -1,7 +1,6 @@ package npc import ( - "encoding/json" "errors" "fmt" @@ -33,8 +32,8 @@ type ns struct { allPods *selectorSpec // hash:ip ipset of all pod IPs in this namespace // stores IP addrs of pods which are not selected by any target podSelector of - // any netpol; used only in non-legacy mode and is used as a target in - // the WEAVE-NPC-{INGRESS,EGRESS}-DEFAULT iptables chain. + // any netpol; used as a target in the WEAVE-NPC-{INGRESS,EGRESS}-DEFAULT + // iptables chains. ingressDefaultAllowIPSet ipset.Name egressDefaultAllowIPSet ipset.Name @@ -42,11 +41,9 @@ type ns struct { podSelectors *selectorSet ipBlocks *ipBlockSet rules *ruleSet - - legacy bool } -func newNS(name, nodeName string, legacy bool, ipt iptables.Interface, 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(&metav1.LabelSelector{}, nil, name, ipset.HashIP) if err != nil { return nil, err @@ -64,24 +61,21 @@ func newNS(name, nodeName string, legacy bool, ipt iptables.Interface, ips ipset nsSelectors: nsSelectors, ipBlocks: newIPBlockSet(ips), rules: newRuleSet(ipt), - legacy: legacy} + } ns.podSelectors = newSelectorSet(ips, ns.onNewPodSelector, ns.onNewTargetPodSelector, ns.onDestroyTargetPodSelector) - if !legacy { - ingressDefaultAllowIPSet := ipset.Name(IpsetNamePrefix + shortName("ingress-default-allow:"+name)) - if err := ips.Create(ingressDefaultAllowIPSet, ipset.HashIP); err != nil { - return nil, err - } - ns.ingressDefaultAllowIPSet = ingressDefaultAllowIPSet - - egressDefaultAllowIPSet := ipset.Name(IpsetNamePrefix + shortName("egress-default-allow:"+name)) - if err := ips.Create(egressDefaultAllowIPSet, ipset.HashIP); err != nil { - return nil, err - } - ns.egressDefaultAllowIPSet = egressDefaultAllowIPSet - + ingressDefaultAllowIPSet := ipset.Name(IpsetNamePrefix + shortName("ingress-default-allow:"+name)) + if err := ips.Create(ingressDefaultAllowIPSet, ipset.HashIP); err != nil { + return nil, err } + ns.ingressDefaultAllowIPSet = ingressDefaultAllowIPSet + + egressDefaultAllowIPSet := ipset.Name(IpsetNamePrefix + shortName("egress-default-allow:"+name)) + if err := ips.Create(egressDefaultAllowIPSet, ipset.HashIP); err != nil { + return nil, err + } + ns.egressDefaultAllowIPSet = egressDefaultAllowIPSet if err := ns.podSelectors.provision(ns.uid, nil, map[string]*selectorSpec{ns.allPods.key: ns.allPods}); err != nil { return nil, err @@ -113,10 +107,6 @@ func (ns *ns) onNewPodSelector(selector *selector) error { } func (ns *ns) onNewTargetPodSelector(selector *selector, policyType policyType) error { - if ns.legacy { - return nil - } - for _, pod := range ns.pods { if hasIP(pod) { // Remove the pod from default-allow if dst podselector matches the pod @@ -133,10 +123,6 @@ func (ns *ns) onNewTargetPodSelector(selector *selector, policyType policyType) } func (ns *ns) onDestroyTargetPodSelector(selector *selector, policyType policyType) error { - if ns.legacy { - return nil - } - for _, pod := range ns.pods { if hasIP(pod) { if selector.matches(pod.ObjectMeta.Labels) { @@ -192,16 +178,14 @@ func (ns *ns) addPod(obj *coreapi.Pod) error { return err } // If there are no matching target selectors, add the pod to default-allow - if !ns.legacy { - if !foundIngress { - if err := ns.ips.AddEntry(obj.ObjectMeta.UID, ns.ingressDefaultAllowIPSet, obj.Status.PodIP, podComment(obj)); err != nil { - return err - } + if !foundIngress { + if err := ns.ips.AddEntry(obj.ObjectMeta.UID, ns.ingressDefaultAllowIPSet, obj.Status.PodIP, podComment(obj)); err != nil { + return err } - if !foundEgress { - if err := ns.ips.AddEntry(obj.ObjectMeta.UID, ns.egressDefaultAllowIPSet, obj.Status.PodIP, podComment(obj)); err != nil { - return err - } + } + if !foundEgress { + if err := ns.ips.AddEntry(obj.ObjectMeta.UID, ns.egressDefaultAllowIPSet, obj.Status.PodIP, podComment(obj)); err != nil { + return err } } @@ -221,13 +205,11 @@ func (ns *ns) updatePod(oldObj, newObj *coreapi.Pod) error { ns.ips.DelEntry(oldObj.ObjectMeta.UID, LocalIpset, oldObj.Status.PodIP) } - if !ns.legacy { - if err := ns.ips.DelEntry(oldObj.ObjectMeta.UID, ns.ingressDefaultAllowIPSet, oldObj.Status.PodIP); err != nil { - return err - } - if err := ns.ips.DelEntry(oldObj.ObjectMeta.UID, ns.egressDefaultAllowIPSet, oldObj.Status.PodIP); err != nil { - return err - } + if err := ns.ips.DelEntry(oldObj.ObjectMeta.UID, ns.ingressDefaultAllowIPSet, oldObj.Status.PodIP); err != nil { + return err + } + if err := ns.ips.DelEntry(oldObj.ObjectMeta.UID, ns.egressDefaultAllowIPSet, oldObj.Status.PodIP); err != nil { + return err } return ns.podSelectors.delFromMatching(oldObj.ObjectMeta.UID, oldObj.ObjectMeta.Labels, oldObj.Status.PodIP) @@ -242,23 +224,21 @@ func (ns *ns) updatePod(oldObj, newObj *coreapi.Pod) error { return err } - if !ns.legacy { - if !foundIngress { - if err := ns.ips.AddEntry(newObj.ObjectMeta.UID, ns.ingressDefaultAllowIPSet, newObj.Status.PodIP, podComment(newObj)); err != nil { - return err - } + if !foundIngress { + if err := ns.ips.AddEntry(newObj.ObjectMeta.UID, ns.ingressDefaultAllowIPSet, newObj.Status.PodIP, podComment(newObj)); err != nil { + return err } - if !foundEgress { - if err := ns.ips.AddEntry(newObj.ObjectMeta.UID, ns.egressDefaultAllowIPSet, newObj.Status.PodIP, podComment(newObj)); err != nil { - return err - } + } + if !foundEgress { + if err := ns.ips.AddEntry(newObj.ObjectMeta.UID, ns.egressDefaultAllowIPSet, newObj.Status.PodIP, podComment(newObj)); err != nil { + return err } } return nil } - if !ns.legacy && oldObj.Status.PodIP != newObj.Status.PodIP { + if oldObj.Status.PodIP != newObj.Status.PodIP { if err := ns.updateDefaultAllowIPSetEntry(oldObj, newObj, ns.ingressDefaultAllowIPSet); err != nil { return err } @@ -287,13 +267,11 @@ func (ns *ns) updatePod(oldObj, newObj *coreapi.Pod) error { } } - if !ns.legacy { - if err := ns.addOrRemoveToDefaultAllowIPSet(ps, oldObj, newObj, oldMatch, newMatch, policyTypeIngress); err != nil { - return err - } - if err := ns.addOrRemoveToDefaultAllowIPSet(ps, oldObj, newObj, oldMatch, newMatch, policyTypeEgress); err != nil { - return err - } + if err := ns.addOrRemoveToDefaultAllowIPSet(ps, oldObj, newObj, oldMatch, newMatch, policyTypeIngress); err != nil { + return err + } + if err := ns.addOrRemoveToDefaultAllowIPSet(ps, oldObj, newObj, oldMatch, newMatch, policyTypeEgress); err != nil { + return err } } } @@ -329,13 +307,11 @@ func (ns *ns) deletePod(obj *coreapi.Pod) error { ns.ips.DelEntry(obj.ObjectMeta.UID, LocalIpset, obj.Status.PodIP) } - if !ns.legacy { - if err := ns.ips.DelEntry(obj.ObjectMeta.UID, ns.ingressDefaultAllowIPSet, obj.Status.PodIP); err != nil { - return err - } - if err := ns.ips.DelEntry(obj.ObjectMeta.UID, ns.egressDefaultAllowIPSet, obj.Status.PodIP); err != nil { - return err - } + if err := ns.ips.DelEntry(obj.ObjectMeta.UID, ns.ingressDefaultAllowIPSet, obj.Status.PodIP); err != nil { + return err + } + if err := ns.ips.DelEntry(obj.ObjectMeta.UID, ns.egressDefaultAllowIPSet, obj.Status.PodIP); err != nil { + return err } return ns.podSelectors.delFromMatching(obj.ObjectMeta.UID, obj.ObjectMeta.Labels, obj.Status.PodIP) @@ -458,20 +434,7 @@ func bypassRules(namespace string, ingress, egress ipset.Name) map[string][][]st } } -func bypassRuleLegacy(nsIpsetName ipset.Name, namespace string) []string { - return []string{"-m", "set", "--match-set", string(nsIpsetName), "dst", "-j", "ACCEPT", "-m", "comment", "--comment", "DefaultAllow isolation for namespace: " + namespace} -} - func (ns *ns) ensureBypassRules() error { - if ns.legacy { - ipset := ns.allPods.ipsetName - common.Log.Debugf("ensuring rule for DefaultAllow in namespace: %s, set %s", ns.name, ipset) - if err := ns.ipt.Append(TableFilter, DefaultChain, bypassRuleLegacy(ipset, ns.name)...); err != nil { - return err - } - return nil - } - for chain, rules := range bypassRules(ns.name, ns.ingressDefaultAllowIPSet, ns.egressDefaultAllowIPSet) { for _, rule := range rules { common.Log.Debugf("adding rule for DefaultAllow in namespace: %s, chain: %s, %s", ns.name, chain, rule) @@ -485,15 +448,6 @@ func (ns *ns) ensureBypassRules() error { } func (ns *ns) deleteBypassRules() error { - if ns.legacy { - ipset := ns.allPods.ipsetName - common.Log.Debugf("removing default rule in namespace: %s, set %s", ns.name, ipset) - if err := ns.ipt.Delete(TableFilter, DefaultChain, bypassRuleLegacy(ipset, ns.name)...); err != nil { - return err - } - return nil - } - for chain, rules := range bypassRules(ns.name, ns.ingressDefaultAllowIPSet, ns.egressDefaultAllowIPSet) { for _, rule := range rules { common.Log.Debugf("removing rule for DefaultAllow in namespace: %s, chain: %s, %s", ns.name, chain, rule) @@ -509,11 +463,9 @@ func (ns *ns) deleteBypassRules() error { func (ns *ns) addNamespace(obj *coreapi.Namespace) error { ns.namespace = obj - // Insert a rule to bypass policies if namespace is DefaultAllow - if !ns.isDefaultDeny(obj) { - if err := ns.ensureBypassRules(); err != nil { - return err - } + // Insert a rule to bypass policies + if err := ns.ensureBypassRules(); err != nil { + return err } // Add namespace ipset to matching namespace selectors @@ -524,24 +476,6 @@ func (ns *ns) addNamespace(obj *coreapi.Namespace) error { func (ns *ns) updateNamespace(oldObj, newObj *coreapi.Namespace) error { ns.namespace = newObj - // Update bypass rule if ingress default has changed - oldDefaultDeny := ns.isDefaultDeny(oldObj) - newDefaultDeny := ns.isDefaultDeny(newObj) - - if oldDefaultDeny != newDefaultDeny { - common.Log.Infof("namespace DefaultDeny changed from %t to %t", oldDefaultDeny, newDefaultDeny) - if oldDefaultDeny { - if err := ns.ensureBypassRules(); err != nil { - return err - } - } - if newDefaultDeny { - if err := ns.deleteBypassRules(); err != nil { - return err - } - } - } - // Re-evaluate namespace selector membership if labels have changed if !equals(oldObj.ObjectMeta.Labels, newObj.ObjectMeta.Labels) { for _, selector := range ns.nsSelectors.entries { @@ -570,10 +504,8 @@ func (ns *ns) deleteNamespace(obj *coreapi.Namespace) error { ns.namespace = nil // Remove bypass rule - if !ns.isDefaultDeny(obj) { - if err := ns.deleteBypassRules(); err != nil { - return err - } + if err := ns.deleteBypassRules(); err != nil { + return err } // Remove namespace ipset from any matching namespace selectors @@ -582,41 +514,16 @@ func (ns *ns) deleteNamespace(obj *coreapi.Namespace) error { return err } - if !ns.legacy { - if err := ns.ips.Destroy(ns.ingressDefaultAllowIPSet); err != nil { - return err - } - if err := ns.ips.Destroy(ns.egressDefaultAllowIPSet); err != nil { - return err - } + if err := ns.ips.Destroy(ns.ingressDefaultAllowIPSet); err != nil { + return err + } + if err := ns.ips.Destroy(ns.egressDefaultAllowIPSet); err != nil { + return err } return nil } -func (ns *ns) isDefaultDeny(namespace *coreapi.Namespace) bool { - nnpJSON, found := namespace.ObjectMeta.Annotations["net.beta.kubernetes.io/network-policy"] - if !found { - return false - } - - if !ns.legacy { - common.Log.Warn("DefaultDeny annotation is supported only in legacy mode (--use-legacy-netpol)") - return false - } - - var nnp NamespaceNetworkPolicy - if err := json.Unmarshal([]byte(nnpJSON), &nnp); err != nil { - common.Log.Warn("Ignoring network policy annotation: unmarshal failed:", err) - // If we can't understand the annotation, behave as if it isn't present - return false - } - - return nnp.Ingress != nil && - nnp.Ingress.Isolation != nil && - *(nnp.Ingress.Isolation) == DefaultDeny -} - func hasIP(pod *coreapi.Pod) bool { // Ensure pod isn't dead, has an IP address and isn't sharing the host network namespace return pod.Status.Phase != "Succeeded" && pod.Status.Phase != "Failed" && @@ -670,16 +577,9 @@ func (ns *ns) analyse(obj interface{}) ( ns.policies[uid] = obj // Analyse policy, determine which rules and ipsets are required - if ns.legacy { - rules, nsSelectors, podSelectors, err = ns.analysePolicyLegacy(obj.(*extnapi.NetworkPolicy)) - if err != nil { - return - } - } else { - rules, nsSelectors, podSelectors, ipBlocks, err = ns.analysePolicy(obj.(*networkingv1.NetworkPolicy)) - if err != nil { - return - } + rules, nsSelectors, podSelectors, ipBlocks, err = ns.analysePolicy(obj.(*networkingv1.NetworkPolicy)) + if err != nil { + return } return diff --git a/prog/weave-kube/weave-daemonset-k8s-1.7.yaml b/prog/weave-kube/weave-daemonset-k8s-1.7.yaml index 301e67ab..6f5ba5fc 100644 --- a/prog/weave-kube/weave-daemonset-k8s-1.7.yaml +++ b/prog/weave-kube/weave-daemonset-k8s-1.7.yaml @@ -25,14 +25,6 @@ items: - get - list - watch - - apiGroups: - - extensions - resources: - - networkpolicies - verbs: - - get - - list - - watch - apiGroups: - 'networking.k8s.io' resources: @@ -163,7 +155,6 @@ items: fieldPath: spec.nodeName image: 'weaveworks/weave-npc:latest' imagePullPolicy: Always -#npc-args resources: requests: cpu: 10m diff --git a/prog/weave-npc/main.go b/prog/weave-npc/main.go index 276c90e7..cce45b87 100644 --- a/prog/weave-npc/main.go +++ b/prog/weave-npc/main.go @@ -10,7 +10,6 @@ import ( "github.com/pkg/errors" "github.com/spf13/cobra" coreapi "k8s.io/api/core/v1" - extnapi "k8s.io/api/extensions/v1beta1" networkingv1 "k8s.io/api/networking/v1" "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/runtime" @@ -32,7 +31,6 @@ var ( logLevel string allowMcast bool nodeName string - legacy bool maxList int ) @@ -114,7 +112,7 @@ func resetIPSets(ips ipset.Interface) error { return nil } -func createBaseRules(ipt *iptables.IPTables, ips ipset.Interface, legacy bool) error { +func createBaseRules(ipt *iptables.IPTables, ips ipset.Interface) error { // Configure main chain static rules if err := ipt.Append(npc.TableFilter, npc.MainChain, "-m", "state", "--state", "RELATED,ESTABLISHED", "-j", "ACCEPT"); err != nil { @@ -153,28 +151,26 @@ func createBaseRules(ipt *iptables.IPTables, ips ipset.Interface, legacy bool) e return err } - if !legacy { - if err := ipt.Append(npc.TableFilter, npc.EgressMarkChain, - "-j", "MARK", "--set-xmark", npc.EgressMark); err != nil { - return err - } + if err := ipt.Append(npc.TableFilter, npc.EgressMarkChain, + "-j", "MARK", "--set-xmark", npc.EgressMark); err != nil { + return err + } - ruleSpecs := [][]string{ - {"-m", "state", "--state", "RELATED,ESTABLISHED", "-j", "RETURN"}, - {"-m", "state", "--state", "NEW", "-m", "set", "!", "--match-set", npc.LocalIpset, "src", "-j", "RETURN"}, - } - if allowMcast { - ruleSpecs = append(ruleSpecs, []string{"-d", "224.0.0.0/4", "-j", "RETURN"}) - } - ruleSpecs = append(ruleSpecs, [][]string{ - {"-m", "state", "--state", "NEW", "-j", string(npc.EgressDefaultChain)}, - {"-m", "state", "--state", "NEW", "-m", "mark", "!", "--mark", npc.EgressMark, "-j", string(npc.EgressCustomChain)}, - {"-m", "state", "--state", "NEW", "-m", "mark", "!", "--mark", npc.EgressMark, "-j", "NFLOG", "--nflog-group", "86"}, - {"-m", "mark", "!", "--mark", npc.EgressMark, "-j", "DROP"}, - }...) - if err := addChainWithRules(ipt, npc.TableFilter, npc.EgressChain, ruleSpecs); err != nil { - return err - } + ruleSpecs := [][]string{ + {"-m", "state", "--state", "RELATED,ESTABLISHED", "-j", "RETURN"}, + {"-m", "state", "--state", "NEW", "-m", "set", "!", "--match-set", npc.LocalIpset, "src", "-j", "RETURN"}, + } + if allowMcast { + ruleSpecs = append(ruleSpecs, []string{"-d", "224.0.0.0/4", "-j", "RETURN"}) + } + ruleSpecs = append(ruleSpecs, [][]string{ + {"-m", "state", "--state", "NEW", "-j", string(npc.EgressDefaultChain)}, + {"-m", "state", "--state", "NEW", "-m", "mark", "!", "--mark", npc.EgressMark, "-j", string(npc.EgressCustomChain)}, + {"-m", "state", "--state", "NEW", "-m", "mark", "!", "--mark", npc.EgressMark, "-j", "NFLOG", "--nflog-group", "86"}, + {"-m", "mark", "!", "--mark", npc.EgressMark, "-j", "DROP"}, + }...) + if err := addChainWithRules(ipt, npc.TableFilter, npc.EgressChain, ruleSpecs); err != nil { + return err } return nil @@ -243,10 +239,6 @@ func root(cmd *cobra.Command, args []string) { } common.Log.Infof("Starting Weaveworks NPC %s; node name %q", version, nodeName) - if legacy { - common.Log.Info("Running in legacy mode (k8s pre-1.7 network policy semantics)") - } - if err := metrics.Start(metricsAddr); err != nil { common.Log.Fatalf("Failed to start metrics: %v", err) } @@ -268,9 +260,9 @@ func root(cmd *cobra.Command, args []string) { handleError(resetIPTables(ipt)) handleError(resetIPSets(ips)) - handleError(createBaseRules(ipt, ips, legacy)) + handleError(createBaseRules(ipt, ips)) - npc := npc.New(nodeName, legacy, ipt, ips) + npc := npc.New(nodeName, ipt, ips) nsController := makeController(client.Core().RESTClient(), "namespaces", &coreapi.Namespace{}, cache.ResourceEventHandlerFuncs{ @@ -331,11 +323,7 @@ func root(cmd *cobra.Command, args []string) { handleError(npc.UpdateNetworkPolicy(old, new)) }, } - if legacy { - npController = makeController(client.Extensions().RESTClient(), "networkpolicies", &extnapi.NetworkPolicy{}, npHandlers) - } else { - npController = makeController(client.NetworkingV1().RESTClient(), "networkpolicies", &networkingv1.NetworkPolicy{}, npHandlers) - } + npController = makeController(client.NetworkingV1().RESTClient(), "networkpolicies", &networkingv1.NetworkPolicy{}, npHandlers) go nsController.Run(wait.NeverStop) go podController.Run(wait.NeverStop) @@ -356,7 +344,6 @@ func main() { rootCmd.PersistentFlags().StringVar(&logLevel, "log-level", "debug", "logging level (debug, info, warning, error)") rootCmd.PersistentFlags().BoolVar(&allowMcast, "allow-mcast", true, "allow all multicast traffic") rootCmd.PersistentFlags().StringVar(&nodeName, "node-name", "", "only generate rules that apply to this node") - rootCmd.PersistentFlags().BoolVar(&legacy, "use-legacy-netpol", false, "use legacy network policies (pre k8s 1.7 vsn)") rootCmd.PersistentFlags().IntVar(&maxList, "max-list-size", 1024, "maximum size of ipset list (for namespaces)") handleError(rootCmd.Execute()) diff --git a/prog/weaver/main.go b/prog/weaver/main.go index 93622c5a..79c1b65a 100644 --- a/prog/weaver/main.go +++ b/prog/weaver/main.go @@ -182,7 +182,6 @@ func main() { mflag.StringVar(&bridgeConfig.WeaveBridgeName, []string{"-weave-bridge"}, "weave", "name of weave bridge") mflag.StringVar(&bridgeConfig.DockerBridgeName, []string{"-docker-bridge"}, "", "name of Docker bridge") mflag.BoolVar(&bridgeConfig.NPC, []string{"-expect-npc"}, false, "set up iptables rules for npc") - mflag.BoolVar(&bridgeConfig.LegacyNPC, []string{"-use-legacy-netpol"}, false, "use legacy network policies (pre k8s 1.7 vsn)") mflag.StringVar(&routerName, []string{"-name"}, "", "name of router (defaults to MAC of interface)") mflag.StringVar(&nickName, []string{"-nickname"}, "", "nickname of peer (defaults to hostname)") mflag.StringVar(&password, []string{"-password"}, "", "network password") diff --git a/site/kubernetes/kube-addon.md b/site/kubernetes/kube-addon.md index fbb5c0b6..3c5880a4 100644 --- a/site/kubernetes/kube-addon.md +++ b/site/kubernetes/kube-addon.md @@ -370,7 +370,6 @@ You can customise the YAML you get from `cloud.weave.works` by passing some of W - `disable-npc`: boolean (`true|false`). Default: `false`. - `env.NAME=VALUE`: add environment variable `NAME` and set it to `VALUE`. - `seLinuxOptions.NAME=VALUE`: add SELinux option `NAME` and set it to `VALUE`, e.g. `seLinuxOptions.type=spc_t` - - `use-legacy-netpol`: use [legacy NetworkPolicy semantics](https://v1-6.docs.kubernetes.io/docs/api-reference/v1.6/#networkpolicy-v1beta1-extensions), boolean (`true|false`). Default: `true` for Kubernetes version <= 1.6, `false` for > 1.6. The list of variables you can set is: diff --git a/test/840_weave_kube_3_test.sh b/test/840_weave_kube_3_test.sh index a0001937..0efde493 100755 --- a/test/840_weave_kube_3_test.sh +++ b/test/840_weave_kube_3_test.sh @@ -44,15 +44,14 @@ for host in $HOSTS; do done if [ -n "$COVERAGE" ]; then - WEAVE_ENV_VARS="env:\\n - name: EXTRA_ARGS\\n value: \"-test.coverprofile=/home/weave/cover.prof -- --use-legacy-netpol\"" + WEAVE_ENV_VARS="env:\\n - name: EXTRA_ARGS\\n value: \"-test.coverprofile=/home/weave/cover.prof --\"" else - WEAVE_ENV_VARS="env:\\n - name: EXTRA_ARGS\\n value: \"--use-legacy-netpol\"" + WEAVE_ENV_VARS="env:" fi # Ensure Kubernetes uses locally built container images and inject code coverage environment variable (or do nothing depending on $COVERAGE): sed -e "s%imagePullPolicy: Always%imagePullPolicy: Never%" \ -e "s%env:%$WEAVE_ENV_VARS%" \ - -e "s%#npc-args% args:\n - '--use-legacy-netpol'%" \ "$(dirname "$0")/../prog/weave-kube/weave-daemonset-k8s-1.8.yaml" | run_on "$HOST1" "$KUBECTL apply -n kube-system -f -" sleep 5 @@ -76,10 +75,19 @@ assert "run_on $HOST1 ps aux | grep -c '[d]efunct'" "0" assert "run_on $HOST2 ps aux | grep -c '[d]efunct'" "0" assert "run_on $HOST3 ps aux | grep -c '[d]efunct'" "0" -# Set up a simple network policy so all our test pods can talk to each other -run_on $HOST1 "$KUBECTL annotate ns default net.beta.kubernetes.io/network-policy='{\"ingress\":{\"isolation\":\"DefaultDeny\"}}'" +# Set up simple network policies so all our test pods can talk to each other run_on $HOST1 "$KUBECTL apply -f -" </dev/null" -if [ -n "$COVERAGE" ]; then - WEAVE_ENV_VARS="env:\\n - name: EXTRA_ARGS\\n value: \"-test.coverprofile=/home/weave/cover.prof --\"" -else - WEAVE_ENV_VARS="env:" -fi - -# Restart weave-net with npc in non-legacy mode -$SSH $HOST1 "$KUBECTL delete ds weave-net -n=kube-system" -sed -e "s%imagePullPolicy: Always%imagePullPolicy: Never%" \ - -e "s%env:%$WEAVE_ENV_VARS%" \ - "$(dirname "$0")/../prog/weave-kube/weave-daemonset-k8s-1.8.yaml" | run_on "$HOST1" "$KUBECTL apply -n kube-system -f -" - -assert_raises 'wait_for_x check_all_pods_communicate pods' - -# nettest-deny should still not be able to reach nettest pods -assert_raises "! $SSH $HOST1 $KUBECTL exec $denyPodName -- curl -s -S -f -m 2 http://$DOMAIN:8080/status >/dev/null" - # check access via virtual IP VIRTUAL_IP=$($SSH $HOST1 $KUBECTL get service netvirt -o template --template={{.spec.clusterIP}}) assert_raises "$SSH $HOST1 $KUBECTL exec $podName -- curl -s -S -f -m 2 http://$VIRTUAL_IP/status >/dev/null" From 10a5b6fb5102d436af7764499ba13ad0a99f3233 Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Mon, 11 Jun 2018 11:47:23 +0200 Subject: [PATCH 38/46] npc: Use constants for chains --- net/bridge.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/net/bridge.go b/net/bridge.go index 07aedc15..40e85cb7 100644 --- a/net/bridge.go +++ b/net/bridge.go @@ -17,6 +17,7 @@ import ( "github.com/weaveworks/weave/ipam/tracker" "github.com/weaveworks/weave/net/address" "github.com/weaveworks/weave/net/ipset" + "github.com/weaveworks/weave/npc" ) /* This code implements three possible configurations to connect @@ -459,23 +460,22 @@ func configureIPTables(config *BridgeConfig, ips ipset.Interface) error { if config.NPC { // Steer traffic via the NPC. // Ignore error because chains might already exist - // TODO(brb): use constants for WEAVE-NPC* chains - _ = ipt.NewChain("filter", "WEAVE-NPC") - _ = ipt.NewChain("filter", "WEAVE-NPC-EGRESS") - if err = ipt.AppendUnique("filter", "INPUT", "-i", config.WeaveBridgeName, "-j", "WEAVE-NPC-EGRESS"); err != nil { + _ = ipt.NewChain("filter", npc.MainChain) + _ = ipt.NewChain("filter", npc.EgressChain) + if err = ipt.AppendUnique("filter", "INPUT", "-i", config.WeaveBridgeName, "-j", npc.EgressChain); err != nil { return err } fwdRules = append(fwdRules, []string{ "-i", config.WeaveBridgeName, "-m", "comment", "--comment", "NOTE: this must go before '-j KUBE-FORWARD'", - "-j", "WEAVE-NPC-EGRESS"}) + "-j", npc.EgressChain}) // If WEAVE-NPC chain doesn't exist then creating a rule in the chain will fail fwdRules = append(fwdRules, [][]string{ {"-o", config.WeaveBridgeName, "-m", "comment", "--comment", "NOTE: this must go before '-j KUBE-FORWARD'", - "-j", "WEAVE-NPC"}, + "-j", npc.MainChain}, {"-o", config.WeaveBridgeName, "-m", "state", "--state", "NEW", "-j", "NFLOG", "--nflog-group", "86"}, {"-o", config.WeaveBridgeName, "-j", "DROP"}, }...) From c5772c814c670290ef93e332facf535bdfdb95d5 Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Mon, 11 Jun 2018 11:52:35 +0200 Subject: [PATCH 39/46] npc: Add comments for traffic steering --- net/bridge.go | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/net/bridge.go b/net/bridge.go index 40e85cb7..acd2f1a9 100644 --- a/net/bridge.go +++ b/net/bridge.go @@ -459,20 +459,23 @@ func configureIPTables(config *BridgeConfig, ips ipset.Interface) error { if config.NPC { // Steer traffic via the NPC. + // Ignore error because chains might already exist _ = ipt.NewChain("filter", npc.MainChain) _ = ipt.NewChain("filter", npc.EgressChain) + + // Steer egress traffic destined to local node. if err = ipt.AppendUnique("filter", "INPUT", "-i", config.WeaveBridgeName, "-j", npc.EgressChain); err != nil { return err } - fwdRules = append(fwdRules, []string{ - "-i", config.WeaveBridgeName, - "-m", "comment", "--comment", "NOTE: this must go before '-j KUBE-FORWARD'", - "-j", npc.EgressChain}) - - // If WEAVE-NPC chain doesn't exist then creating a rule in the chain will fail fwdRules = append(fwdRules, [][]string{ + // Might include ingress traffic which is fine as long as we do not + // ACCEPT in WEAVE-NPC-EGRESS chain + {"-i", config.WeaveBridgeName, + "-m", "comment", "--comment", "NOTE: this must go before '-j KUBE-FORWARD'", + "-j", npc.EgressChain}, + // The following rules are for ingress NPC processing {"-o", config.WeaveBridgeName, "-m", "comment", "--comment", "NOTE: this must go before '-j KUBE-FORWARD'", "-j", npc.MainChain}, From 2c708b57a1cff076856257a67f5e16b14e891a97 Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Mon, 11 Jun 2018 11:58:17 +0200 Subject: [PATCH 40/46] npc: Improve analyser comments --- npc/analyser.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/npc/analyser.go b/npc/analyser.go index 2675ce1c..f94dde8f 100644 --- a/npc/analyser.go +++ b/npc/analyser.go @@ -38,8 +38,8 @@ func (ns *ns) analysePolicy(policy *networkingv1.NetworkPolicy) ( // To prevent targetSelector being overwritten by a subsequent selector with // the same key, addIfNotExist MUST be used when adding to podSelectors. - // Otherwise, targetSelector' "dst" property might be lost resulting in - // an invalid content of the "default-allow" ipset. + // Otherwise, policyTypes of the selector might be lost resulting in + // an invalid content in any "default-allow" ipset. addIfNotExist(targetSelector, podSelectors) // If ingress is empty then this NetworkPolicy does not allow any ingress traffic @@ -118,7 +118,7 @@ func (ns *ns) analysePolicy(policy *networkingv1.NetworkPolicy) ( var dstSelector *selectorSpec var dstRuleHost ruleHost - // NetworkPolicyPeer describes a peer to allow traffic from. + // NetworkPolicyPeer describes a peer to allow traffic to. // Exactly one of its fields must be specified. if peer.PodSelector != nil { dstSelector, err = newSelectorSpec(peer.PodSelector, nil, ns.name, ipset.HashIP) From c1ee358f663a22ba4eb4aeaa2b6a6c6cdcc6071f Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Mon, 11 Jun 2018 18:01:31 +0200 Subject: [PATCH 41/46] npc: Fix ipset package import path after rebase --- npc/ipblock.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/npc/ipblock.go b/npc/ipblock.go index c40e46e2..517ffb20 100644 --- a/npc/ipblock.go +++ b/npc/ipblock.go @@ -9,7 +9,7 @@ import ( "k8s.io/apimachinery/pkg/types" "github.com/weaveworks/weave/common" - "github.com/weaveworks/weave/npc/ipset" + "github.com/weaveworks/weave/net/ipset" ) type ipBlockSpec struct { From 1649080a35d3e831cc3e6d8f862fba4bf2f1fef4 Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Sun, 15 Jul 2018 10:17:01 +0200 Subject: [PATCH 42/46] npc: Add ns name into ipBlock ipset name Otherwise, the same ipBlock ipset can be used among different namespaces which results in panic as refcounting of ipsets happens only per namespace, so npc will try to create the same ipset twice. --- npc/analyser.go | 2 +- npc/controller_test.go | 41 +++++++++++++++++++++++++++++++++++++---- npc/ipblock.go | 7 ++++--- 3 files changed, 42 insertions(+), 8 deletions(-) diff --git a/npc/analyser.go b/npc/analyser.go index f94dde8f..a19bb711 100644 --- a/npc/analyser.go +++ b/npc/analyser.go @@ -136,7 +136,7 @@ func (ns *ns) analysePolicy(policy *networkingv1.NetworkPolicy) ( nsSelectors[dstSelector.key] = dstSelector dstRuleHost = dstSelector } else if peer.IPBlock != nil { - ipBlock := newIPBlockSpec(peer.IPBlock) + ipBlock := newIPBlockSpec(peer.IPBlock, ns.name) ipBlocks[ipBlock.key] = ipBlock dstRuleHost = ipBlock } diff --git a/npc/controller_test.go b/npc/controller_test.go index fac426dd..21488f03 100644 --- a/npc/controller_test.go +++ b/npc/controller_test.go @@ -514,15 +514,16 @@ func TestNewTargetSelector(t *testing.T) { func TestEgressPolicyWithIPBlock(t *testing.T) { const ( - fooPodIP = "10.32.0.10" - exceptIPSetName = "weave-LKf)d{$q[dkYlH3Ko6eFcdc7z" + fooPodIP = "10.32.0.10" + exceptIPSetName = "weave-j:W$5om!$8JS})buYAD7q^#sX" + exceptIPSetNameInNonDefault = "weave-|*P{aL2C@#N/1IZ!IvQItv_pt" ) m := newMockIPSet() ipt := newMockIPTables() controller := New("foo", ipt, &m) - netpol := &networkingv1.NetworkPolicy{ + netpolFoo := &networkingv1.NetworkPolicy{ ObjectMeta: metav1.ObjectMeta{ UID: "netpol-foo", Name: "allow-from-bar-to-foo", @@ -546,7 +547,8 @@ func TestEgressPolicyWithIPBlock(t *testing.T) { }}, }, } - controller.AddNetworkPolicy(netpol) + err := controller.AddNetworkPolicy(netpolFoo) + require.NoError(t, err) require.Equal(t, 2, len(m.sets[exceptIPSetName].subSets)) require.True(t, m.entryExists(exceptIPSetName, "192.168.48.1/32")) @@ -558,4 +560,35 @@ func TestEgressPolicyWithIPBlock(t *testing.T) { require.Contains(t, rule, "-d 192.168.48.0/24 -m set ! --match-set "+exceptIPSetName+" dst") } + // Check that we create a new ipset for the ipBlock bellow. An ipset with the + // same content already exists (created by netpolFoo), but we need to create + // a new one, as netpolBar is in a different namespace and weave-npc does + // object refcounting only within namespace boundaries. + netpolBar := &networkingv1.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{ + UID: "netpol-foo-bar", + Name: "allow-from-bar-to-foo-in-non-default", + Namespace: "non-default", + }, + Spec: networkingv1.NetworkPolicySpec{ + PolicyTypes: []networkingv1.PolicyType{ + networkingv1.PolicyTypeEgress, + }, + PodSelector: metav1.LabelSelector{MatchLabels: map[string]string{"run": "bar"}}, + Egress: []networkingv1.NetworkPolicyEgressRule{{ + To: []networkingv1.NetworkPolicyPeer{{ + IPBlock: &networkingv1.IPBlock{ + CIDR: "192.168.48.0/24", + Except: []string{ + "192.168.48.1/32", + "192.168.48.2/32", + }, + }, + }}, + }}, + }, + } + err = controller.AddNetworkPolicy(netpolBar) + require.NoError(t, err) + require.Equal(t, 2, len(m.sets[exceptIPSetNameInNonDefault].subSets)) } diff --git a/npc/ipblock.go b/npc/ipblock.go index 517ffb20..7d2579d0 100644 --- a/npc/ipblock.go +++ b/npc/ipblock.go @@ -16,15 +16,16 @@ type ipBlockSpec struct { key string ipsetName ipset.Name // ipset for storing excepted CIDRs ipBlock *networkingv1.IPBlock + nsName string // Namespace name } -func newIPBlockSpec(ipb *networkingv1.IPBlock) *ipBlockSpec { - spec := &ipBlockSpec{ipBlock: ipb} +func newIPBlockSpec(ipb *networkingv1.IPBlock, nsName string) *ipBlockSpec { + spec := &ipBlockSpec{ipBlock: ipb, nsName: nsName} if len(ipb.Except) > 0 { sort.Strings(ipb.Except) spec.key = strings.Join(ipb.Except, " ") - spec.ipsetName = ipset.Name(IpsetNamePrefix + shortName("ipblock-except:"+spec.key)) + spec.ipsetName = ipset.Name(IpsetNamePrefix + shortName(nsName+":"+"ipblock-except:"+spec.key)) } return spec From 6377aeabdd5bda59b7091489c3cecd968912565f Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Sat, 21 Jul 2018 15:56:14 +0200 Subject: [PATCH 43/46] npc: Do not ignore errors when creating chains Also, move ensureRulesAtTop to net/iptables.go --- net/bridge.go | 43 +++----------------------------- net/iptables.go | 65 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 40 deletions(-) create mode 100644 net/iptables.go diff --git a/net/bridge.go b/net/bridge.go index acd2f1a9..e6fb747c 100644 --- a/net/bridge.go +++ b/net/bridge.go @@ -460,9 +460,9 @@ func configureIPTables(config *BridgeConfig, ips ipset.Interface) error { if config.NPC { // Steer traffic via the NPC. - // Ignore error because chains might already exist - _ = ipt.NewChain("filter", npc.MainChain) - _ = ipt.NewChain("filter", npc.EgressChain) + if err = ensureChains(ipt, "filter", npc.MainChain, npc.EgressChain); err != nil { + return err + } // Steer egress traffic destined to local node. if err = ipt.AppendUnique("filter", "INPUT", "-i", config.WeaveBridgeName, "-j", npc.EgressChain); err != nil { @@ -584,43 +584,6 @@ func linkSetUpByName(linkName string) error { return netlink.LinkSetUp(link) } -// ensureRulesAtTop ensures the presence of given iptables rules. -// -// If any rule from the list is missing, the function deletes all given -// rules and re-inserts them at the top of the chain to ensure the order of the rules. -func ensureRulesAtTop(table, chain string, rulespecs [][]string, ipt *iptables.IPTables) error { - allFound := true - - for _, rs := range rulespecs { - found, err := ipt.Exists(table, chain, rs...) - if err != nil { - return errors.Wrapf(err, "ipt.Exists(%s, %s, %s)", table, chain, rs) - } - if !found { - allFound = false - break - } - } - - // All rules exist, do nothing. - if allFound { - return nil - } - - for pos, rs := range rulespecs { - // If any is missing, then delete all, as we need to preserve the order of - // given rules. Ignore errors, as rule might not exist. - if !allFound { - ipt.Delete(table, chain, rs...) - } - if err := ipt.Insert(table, chain, pos+1, rs...); err != nil { - return errors.Wrapf(err, "ipt.Append(%s, %s, %s)", table, chain, rs) - } - } - - return nil -} - func reexpose(config *BridgeConfig, log *logrus.Logger) error { // Get existing IP addrs of the weave bridge. // If the bridge hasn't been exposed, then this functions does nothing. diff --git a/net/iptables.go b/net/iptables.go new file mode 100644 index 00000000..63d5a2ed --- /dev/null +++ b/net/iptables.go @@ -0,0 +1,65 @@ +package net + +import ( + "github.com/coreos/go-iptables/iptables" + "github.com/pkg/errors" +) + +// ensureChains creates given chains if they do not exist. +func ensureChains(ipt *iptables.IPTables, table string, chains ...string) error { + existingChains, err := ipt.ListChains(table) + if err != nil { + return errors.Wrapf(err, "ipt.ListChains(%s)", table) + } + chainMap := make(map[string]struct{}) + for _, c := range existingChains { + chainMap[c] = struct{}{} + } + + for _, c := range chains { + if _, found := chainMap[c]; !found { + if err := ipt.NewChain(table, c); err != nil { + return errors.Wrapf(err, "ipt.NewChain(%s, %s)", table, c) + } + } + } + + return nil +} + +// ensureRulesAtTop ensures the presence of given iptables rules. +// +// If any rule from the list is missing, the function deletes all given +// rules and re-inserts them at the top of the chain to ensure the order of the rules. +func ensureRulesAtTop(table, chain string, rulespecs [][]string, ipt *iptables.IPTables) error { + allFound := true + + for _, rs := range rulespecs { + found, err := ipt.Exists(table, chain, rs...) + if err != nil { + return errors.Wrapf(err, "ipt.Exists(%s, %s, %s)", table, chain, rs) + } + if !found { + allFound = false + break + } + } + + // All rules exist, do nothing. + if allFound { + return nil + } + + for pos, rs := range rulespecs { + // If any is missing, then delete all, as we need to preserve the order of + // given rules. Ignore errors, as rule might not exist. + if !allFound { + ipt.Delete(table, chain, rs...) + } + if err := ipt.Insert(table, chain, pos+1, rs...); err != nil { + return errors.Wrapf(err, "ipt.Append(%s, %s, %s)", table, chain, rs) + } + } + + return nil +} From 70ff510e49ea597ef7217b36a5f63477ccbae249 Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Sat, 21 Jul 2018 16:01:11 +0200 Subject: [PATCH 44/46] npc: Document egress rules --- prog/weave-npc/main.go | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/prog/weave-npc/main.go b/prog/weave-npc/main.go index cce45b87..4c6868f7 100644 --- a/prog/weave-npc/main.go +++ b/prog/weave-npc/main.go @@ -156,6 +156,28 @@ func createBaseRules(ipt *iptables.IPTables, ips ipset.Interface) error { return err } + // Egress rules: + // + // -A WEAVE-NPC-EGRESS -m state --state RELATED,ESTABLISHED -j RETURN + // -A WEAVE-NPC-EGRESS -m set ! --match-set weave-local-pods src -j RETURN + // -A WEAVE-NPC-EGRESS -d 224.0.0.0/4 -j RETURN + // -A WEAVE-NPC-EGRESS -m state --state NEW -j WEAVE-NPC-EGRESS-DEFAULT + // -A WEAVE-NPC-EGRESS -m state --state NEW -m mark ! --mark 0x40000/0x40000 -j WEAVE-NPC-EGRESS-CUSTOM + // -A WEAVE-NPC-EGRESS -m state --state NEW -m mark ! --mark 0x40000/0x40000 -j NFLOG --nflog-group 86 + // -A WEAVE-NPC-EGRESS -m mark ! --mark 0x40000/0x40000 -j DROP + // + // -A WEAVE-NPC-EGRESS-CUSTOM -j MARK --set-xmark 0x40000/0x40000 + // -A WEAVE-NPC-EGRESS-CUSTOM -j RETURN + // + // -A WEAVE-NPC-EGRESS-DEFAULT -j MARK --set-xmark 0x40000/0x40000 + // -A WEAVE-NPC-EGRESS-DEFAULT -j RETURN + // + // For each rule we create two (mark and return). We cannot just accept + // a packet if it matches any rule, as a packet might need to traverse + // the ingress npc as well which happens later in the chain (in some cases + // we cannot detect whether packet is ingress or egress, so we need to + // check both chains). + ruleSpecs := [][]string{ {"-m", "state", "--state", "RELATED,ESTABLISHED", "-j", "RETURN"}, {"-m", "state", "--state", "NEW", "-m", "set", "!", "--match-set", npc.LocalIpset, "src", "-j", "RETURN"}, From 0ad2001ab457058545e53f0868e31ee33f50ac0a Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Sat, 21 Jul 2018 16:10:50 +0200 Subject: [PATCH 45/46] npc: ACCEPT already established connections in egress chain It's safe to accept such connections as they will be accepted in the ingress chain anyway. --- prog/weave-npc/main.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/prog/weave-npc/main.go b/prog/weave-npc/main.go index 4c6868f7..26fb79db 100644 --- a/prog/weave-npc/main.go +++ b/prog/weave-npc/main.go @@ -158,7 +158,7 @@ func createBaseRules(ipt *iptables.IPTables, ips ipset.Interface) error { // Egress rules: // - // -A WEAVE-NPC-EGRESS -m state --state RELATED,ESTABLISHED -j RETURN + // -A WEAVE-NPC-EGRESS -m state --state RELATED,ESTABLISHED -j ACCEPT // -A WEAVE-NPC-EGRESS -m set ! --match-set weave-local-pods src -j RETURN // -A WEAVE-NPC-EGRESS -d 224.0.0.0/4 -j RETURN // -A WEAVE-NPC-EGRESS -m state --state NEW -j WEAVE-NPC-EGRESS-DEFAULT @@ -179,7 +179,7 @@ func createBaseRules(ipt *iptables.IPTables, ips ipset.Interface) error { // check both chains). ruleSpecs := [][]string{ - {"-m", "state", "--state", "RELATED,ESTABLISHED", "-j", "RETURN"}, + {"-m", "state", "--state", "RELATED,ESTABLISHED", "-j", "ACCEPT"}, {"-m", "state", "--state", "NEW", "-m", "set", "!", "--match-set", npc.LocalIpset, "src", "-j", "RETURN"}, } if allowMcast { From e65cf32372b44ba06a201687d730f27a08f0fa5d Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Sat, 21 Jul 2018 17:00:47 +0200 Subject: [PATCH 46/46] npc: Flush chain if rules mismatch in AddChainWithRules Also, move the helper function to net/iptables.go. --- net/iptables.go | 41 +++++++++++++++++++++++++++++++++++++++++ prog/weave-npc/main.go | 39 ++------------------------------------- 2 files changed, 43 insertions(+), 37 deletions(-) diff --git a/net/iptables.go b/net/iptables.go index 63d5a2ed..a11ca533 100644 --- a/net/iptables.go +++ b/net/iptables.go @@ -1,10 +1,51 @@ package net import ( + "strings" + "github.com/coreos/go-iptables/iptables" "github.com/pkg/errors" ) +// AddChainWithRules creates a chain and appends given rules to it. +// +// If the chain exists, but its rules are not the same as the given ones, the +// function will flush the chain and then will append the rules. +func AddChainWithRules(ipt *iptables.IPTables, table, chain string, rulespecs [][]string) error { + if err := ensureChains(ipt, table, chain); err != nil { + return err + } + + currRuleSpecs, err := ipt.List(table, chain) + if err != nil { + return errors.Wrapf(err, "iptables -S. table: %q, chain: %q", table, chain) + } + + // First returned rule is "-N $(chain)", so ignore it + currRules := strings.Join(currRuleSpecs[1:], "\n") + rules := make([]string, 0) + for _, r := range rulespecs { + rules = append(rules, strings.Join(r, " ")) + } + reqRules := strings.Join(rules, "\n") + + if currRules == reqRules { + return nil + } + + if err := ipt.ClearChain(table, chain); err != nil { + return err + } + + for _, r := range rulespecs { + if err := ipt.Append(table, chain, r...); err != nil { + return errors.Wrapf(err, "iptables -A. table: %q, chain: %q, rule: %s", table, chain, r) + } + } + + return nil +} + // ensureChains creates given chains if they do not exist. func ensureChains(ipt *iptables.IPTables, table string, chains ...string) error { existingChains, err := ipt.ListChains(table) diff --git a/prog/weave-npc/main.go b/prog/weave-npc/main.go index 26fb79db..9d054206 100644 --- a/prog/weave-npc/main.go +++ b/prog/weave-npc/main.go @@ -3,11 +3,9 @@ package main import ( "os" "os/signal" - "strings" "syscall" "github.com/coreos/go-iptables/iptables" - "github.com/pkg/errors" "github.com/spf13/cobra" coreapi "k8s.io/api/core/v1" networkingv1 "k8s.io/api/networking/v1" @@ -19,6 +17,7 @@ import ( "k8s.io/client-go/tools/cache" "github.com/weaveworks/weave/common" + "github.com/weaveworks/weave/net" "github.com/weaveworks/weave/net/ipset" "github.com/weaveworks/weave/npc" "github.com/weaveworks/weave/npc/metrics" @@ -191,47 +190,13 @@ func createBaseRules(ipt *iptables.IPTables, ips ipset.Interface) error { {"-m", "state", "--state", "NEW", "-m", "mark", "!", "--mark", npc.EgressMark, "-j", "NFLOG", "--nflog-group", "86"}, {"-m", "mark", "!", "--mark", npc.EgressMark, "-j", "DROP"}, }...) - if err := addChainWithRules(ipt, npc.TableFilter, npc.EgressChain, ruleSpecs); err != nil { + if err := net.AddChainWithRules(ipt, npc.TableFilter, npc.EgressChain, ruleSpecs); err != nil { return err } return nil } -// addChainWithRules creates a chain and appends given rules to it. -// -// If the chain exists, but its rules are not the same as the given ones, the -// function will flush the chain and then will append the rules. -func addChainWithRules(ipt *iptables.IPTables, table, chain string, rulespecs [][]string) error { - // Create the chain ignoring any error because the chain might already exist - _ = ipt.NewChain(table, chain) - - currRuleSpecs, err := ipt.List(table, chain) - if err != nil { - return errors.Wrapf(err, "iptables -S. table: %q, chain: %q", table, chain) - } - - // First returned rule is "-N $(chain)", so ignore it - currRules := strings.Join(currRuleSpecs[1:], "\n") - rules := make([]string, 0) - for _, r := range rulespecs { - rules = append(rules, strings.Join(r, " ")) - } - reqRules := strings.Join(rules, "\n") - - if currRules == reqRules { - return nil - } - - for _, r := range rulespecs { - if err := ipt.Append(table, chain, r...); err != nil { - return errors.Wrapf(err, "iptables -A. table: %q, chain: %q, rule: %s", table, chain, r) - } - } - - return nil -} - // Dummy way to check whether a given ipset exists. // TODO(brb) Use "ipset -exist create <..>" for our purpose instead (for some reasons // creating an ipset with -exist fails).