From 8d1ee752d4e15c2b9e61a8a24ec3b070b4e1e139 Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Mon, 21 May 2018 09:38:28 +0200 Subject: [PATCH] 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 } }