From 05d1464eb33afaf9e83165e755cc2b5c97af5e10 Mon Sep 17 00:00:00 2001 From: Shufang Date: Mon, 29 Jun 2020 16:45:59 -0700 Subject: [PATCH] Add logic to hanle different order between namespace selectoer, pod selector, ip block rules. --- .../complex-policy-diff-order.yaml | 38 ++++++++++++++++++ npm/translatePolicy.go | 40 ++++++++++--------- npm/translatePolicy_test.go | 16 ++++---- 3 files changed, 69 insertions(+), 25 deletions(-) create mode 100644 npm/testpolicies/complex-policy-diff-order.yaml diff --git a/npm/testpolicies/complex-policy-diff-order.yaml b/npm/testpolicies/complex-policy-diff-order.yaml new file mode 100644 index 0000000000..b1a5c14455 --- /dev/null +++ b/npm/testpolicies/complex-policy-diff-order.yaml @@ -0,0 +1,38 @@ +apiVersion: networking.k8s.io/v1 +kind: NetworkPolicy +metadata: + name: k8s-example-policy + namespace: default +spec: + podSelector: + matchLabels: + role: db + policyTypes: + - Ingress + - Egress + ingress: + - from: + - namespaceSelector: + matchLabels: + project: myproject + - podSelector: + matchLabels: + role: frontend + - ipBlock: + cidr: 172.17.0.0/16 + except: + - 172.17.1.0/24 + ports: + - protocol: TCP + port: 6379 + egress: + - to: + - ipBlock: + cidr: 10.0.0.0/24 + except: + - 10.0.0.1/32 + ports: + - protocol: TCP + port: 5978 + + \ No newline at end of file diff --git a/npm/translatePolicy.go b/npm/translatePolicy.go index 2420940247..a75cfd53b2 100644 --- a/npm/translatePolicy.go +++ b/npm/translatePolicy.go @@ -159,6 +159,7 @@ func translateIngress(ns string, policyName string, targetSelector metav1.LabelS ipCidrs [][]string entries []*iptm.IptEntry fromRuleEntries []*iptm.IptEntry + addedCidrEntry bool // all cidr entry will be added in one set per from/to rule addedIngressFromEntry, addedPortEntry bool // add drop entries at the end of the chain when there are non ALLOW-ALL* rules ) @@ -291,7 +292,7 @@ func translateIngress(ns string, policyName string, targetSelector metav1.LabelS } addedIngressFromEntry = true } - if j != 0 { + if j != 0 && addedCidrEntry { continue } if portRuleExists { @@ -324,7 +325,7 @@ func translateIngress(ns string, policyName string, targetSelector metav1.LabelS util.IptablesCommentModuleFlag, util.IptablesCommentFlag, "ALLOW-"+cidrIpsetName+ - "-:-"+craftPartialIptablesCommentFromPort(portRule, util.IptablesDstPortFlag)+ + "-AND-"+craftPartialIptablesCommentFromPort(portRule, util.IptablesDstPortFlag)+ "-TO-"+targetSelectorComment, ) fromRuleEntries = append(fromRuleEntries, entry) @@ -353,19 +354,19 @@ func translateIngress(ns string, policyName string, targetSelector metav1.LabelS util.IptablesCommentModuleFlag, util.IptablesCommentFlag, "ALLOW-"+cidrIpsetName+ - "-:-"+craftPartialIptablesCommentFromPort(portRule, util.IptablesDstPortFlag)+ + "-AND-"+craftPartialIptablesCommentFromPort(portRule, util.IptablesDstPortFlag)+ "-TO-"+targetSelectorComment, ) fromRuleEntries = append(fromRuleEntries, entry) } } } else { - cidrEntry := &iptm.IptEntry{ + entry := &iptm.IptEntry{ Chain: util.IptablesAzureIngressFromChain, Specs: append([]string(nil), targetSelectorIptEntrySpec...), } - cidrEntry.Specs = append( - cidrEntry.Specs, + entry.Specs = append( + entry.Specs, util.IptablesModuleFlag, util.IptablesSetModuleFlag, util.IptablesMatchSetFlag, @@ -379,9 +380,10 @@ func translateIngress(ns string, policyName string, targetSelector metav1.LabelS "ALLOW-"+cidrIpsetName+ "-TO-"+targetSelectorComment, ) - fromRuleEntries = append(fromRuleEntries, cidrEntry) + fromRuleEntries = append(fromRuleEntries, entry) addedIngressFromEntry = true } + addedCidrEntry = true } continue } @@ -799,6 +801,7 @@ func translateEgress(ns string, policyName string, targetSelector metav1.LabelSe ipCidrs [][]string entries []*iptm.IptEntry toRuleEntries []*iptm.IptEntry + addedCidrEntry bool // all cidr entry will be added in one set per from/to rule addedEgressToEntry, addedPortEntry bool // add drop entry when there are non ALLOW-ALL* rules ) @@ -927,7 +930,7 @@ func translateEgress(ns string, policyName string, targetSelector metav1.LabelSe } addedEgressToEntry = true } - if j != 0 { + if j != 0 && addedCidrEntry { continue } if portRuleExists { @@ -960,7 +963,7 @@ func translateEgress(ns string, policyName string, targetSelector metav1.LabelSe util.IptablesCommentModuleFlag, util.IptablesCommentFlag, "ALLOW-"+cidrIpsetName+ - "-:-"+craftPartialIptablesCommentFromPort(portRule, util.IptablesDstPortFlag)+ + "-AND-"+craftPartialIptablesCommentFromPort(portRule, util.IptablesDstPortFlag)+ "-FROM-"+targetSelectorComment, ) toRuleEntries = append(toRuleEntries, entry) @@ -989,30 +992,30 @@ func translateEgress(ns string, policyName string, targetSelector metav1.LabelSe util.IptablesCommentModuleFlag, util.IptablesCommentFlag, "ALLOW-"+cidrIpsetName+ - "-:-"+craftPartialIptablesCommentFromPort(portRule, util.IptablesDstPortFlag)+ + "-AND-"+craftPartialIptablesCommentFromPort(portRule, util.IptablesDstPortFlag)+ "-FROM-"+targetSelectorComment, ) toRuleEntries = append(toRuleEntries, entry) } } } else { - cidrEntry := &iptm.IptEntry{ + entry := &iptm.IptEntry{ Chain: util.IptablesAzureEgressToChain, } - cidrEntry.Specs = append( - cidrEntry.Specs, + entry.Specs = append( + entry.Specs, targetSelectorIptEntrySpec..., ) - cidrEntry.Specs = append( - cidrEntry.Specs, + entry.Specs = append( + entry.Specs, util.IptablesModuleFlag, util.IptablesSetModuleFlag, util.IptablesMatchSetFlag, util.GetHashedName(cidrIpsetName), util.IptablesDstFlag, ) - cidrEntry.Specs = append( - cidrEntry.Specs, + entry.Specs = append( + entry.Specs, util.IptablesJumpFlag, util.IptablesAccept, util.IptablesModuleFlag, @@ -1021,9 +1024,10 @@ func translateEgress(ns string, policyName string, targetSelector metav1.LabelSe "ALLOW-"+cidrIpsetName+ "-FROM-"+targetSelectorComment, ) - toRuleEntries = append(toRuleEntries, cidrEntry) + toRuleEntries = append(toRuleEntries, entry) addedEgressToEntry = true } + addedCidrEntry = true } continue } diff --git a/npm/translatePolicy_test.go b/npm/translatePolicy_test.go index 3d191ef78e..fcb924a9e4 100644 --- a/npm/translatePolicy_test.go +++ b/npm/translatePolicy_test.go @@ -2914,18 +2914,20 @@ func TestAllowAppFrontendToTCPPort53UDPPort53Policy(t *testing.T) { func TestComplexPolicy(t *testing.T) { k8sExamplePolicy, err := readPolicyYaml("testpolicies/complex-policy.yaml") + k8sExamplePolicyDiffOrder, err := readPolicyYaml("testpolicies/complex-policy-diff-order.yaml") if err != nil { t.Fatal(err) } sets, _, lists, ingressIPCidrs, egressIPCidrs, iptEntries := translatePolicy(k8sExamplePolicy) + setsDiffOrder, _, listsDiffOrder, ingressIPCidrsDiffOrder, egressIPCidrsDiffOrder, iptEntriesDiffOrder := translatePolicy(k8sExamplePolicyDiffOrder) expectedSets := []string{ "role:db", "ns-default", "role:frontend", } - if !reflect.DeepEqual(sets, expectedSets) { + if !reflect.DeepEqual(sets, expectedSets) || !reflect.DeepEqual(setsDiffOrder, expectedSets) { t.Errorf("translatedPolicy failed @ k8s-example-policy sets comparison") t.Errorf("sets: %v", sets) t.Errorf("expectedSets: %v", expectedSets) @@ -2934,7 +2936,7 @@ func TestComplexPolicy(t *testing.T) { expectedLists := []string{ "ns-project:myproject", } - if !reflect.DeepEqual(lists, expectedLists) { + if !reflect.DeepEqual(lists, expectedLists) || !reflect.DeepEqual(listsDiffOrder, expectedLists) { t.Errorf("translatedPolicy failed @ k8s-example-policy lists comparison") t.Errorf("lists: %v", lists) t.Errorf("expectedLists: %v", expectedLists) @@ -2948,13 +2950,13 @@ func TestComplexPolicy(t *testing.T) { {"", "10.0.0.0/24", "10.0.0.1/32nomatch"}, } - if !reflect.DeepEqual(ingressIPCidrs, expectedIngressIPCidrs) { + if !reflect.DeepEqual(ingressIPCidrs, expectedIngressIPCidrs) || !reflect.DeepEqual(ingressIPCidrsDiffOrder, expectedIngressIPCidrs){ t.Errorf("translatedPolicy failed @ k8s-example-policy ingress IP Cidrs comparison") t.Errorf("ingress IP Cidrs: %v", ingressIPCidrs) t.Errorf("expected ingress IP Cidrs: %v", expectedIngressIPCidrs) } - if !reflect.DeepEqual(egressIPCidrs, expectedEgressIPCidrs) { + if !reflect.DeepEqual(egressIPCidrs, expectedEgressIPCidrs) || !reflect.DeepEqual(egressIPCidrsDiffOrder, expectedEgressIPCidrs) { t.Errorf("translatedPolicy failed @ k8s-example-policy egress IP Cidrs comparison") t.Errorf("egress IP Cidrs: %v", egressIPCidrs) t.Errorf("expected egress IP Cidrs: %v", expectedEgressIPCidrs) @@ -2991,7 +2993,7 @@ func TestComplexPolicy(t *testing.T) { util.IptablesModuleFlag, util.IptablesCommentModuleFlag, util.IptablesCommentFlag, - "ALLOW-" + cidrIngressIpsetName + "-:-TCP-PORT-6379-TO-role:db-IN-ns-default", + "ALLOW-" + cidrIngressIpsetName + "-AND-TCP-PORT-6379-TO-role:db-IN-ns-default", }, }, &iptm.IptEntry{ @@ -3130,7 +3132,7 @@ func TestComplexPolicy(t *testing.T) { util.IptablesModuleFlag, util.IptablesCommentModuleFlag, util.IptablesCommentFlag, - "ALLOW-" + cidrEgressIpsetName + "-:-TCP-PORT-5978-FROM-role:db-IN-ns-default", + "ALLOW-" + cidrEgressIpsetName + "-AND-TCP-PORT-5978-FROM-role:db-IN-ns-default", }, }, &iptm.IptEntry{ @@ -3222,7 +3224,7 @@ func TestComplexPolicy(t *testing.T) { } expectedIptEntries = append(expectedIptEntries, nonKubeSystemEntries...) expectedIptEntries = append(expectedIptEntries, getDefaultDropEntries("testnamespace", k8sExamplePolicy.Spec.PodSelector, false, false)...) - if !reflect.DeepEqual(iptEntries, expectedIptEntries) { + if !reflect.DeepEqual(iptEntries, expectedIptEntries) || !reflect.DeepEqual(iptEntriesDiffOrder, expectedIptEntries) { t.Errorf("translatedPolicy failed @ k8s-example-policy policy comparison") marshalledIptEntries, _ := json.Marshal(iptEntries) marshalledExpectedIptEntries, _ := json.Marshal(expectedIptEntries)