Add logic to hanle different order between namespace selectoer, pod selector, ip block rules. (#604)

csfmomo · web-flow · commit 14e8a980ccdf · 2020-07-08T11:08:39.000-07:00
diff --git a/npm/testpolicies/complex-policy-diff-order.yaml 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
+
+          
diff --git 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
@@ -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)

Original file line number	Diff line number	Diff line change
`@@ -159,6 +159,7 @@ func translateIngress(ns string, policyName string, targetSelector metav1.LabelS`
`159`	`159`	`ipCidrs [][]string`
`160`	`160`	`entries []*iptm.IptEntry`
`161`	`161`	`fromRuleEntries []*iptm.IptEntry`
	`162`	`+ addedCidrEntry bool // all cidr entry will be added in one set per from/to rule`
`162`	`163`	`addedIngressFromEntry, addedPortEntry bool // add drop entries at the end of the chain when there are non ALLOW-ALL* rules`
`163`	`164`	`)`
`164`	`165`
`@@ -291,7 +292,7 @@ func translateIngress(ns string, policyName string, targetSelector metav1.LabelS`
`291`	`292`	`}`
`292`	`293`	`addedIngressFromEntry = true`
`293`	`294`	`}`
`294`		`- if j != 0 {`
	`295`	`+ if j != 0 && addedCidrEntry {`
`295`	`296`	`continue`
`296`	`297`	`}`
`297`	`298`	`if portRuleExists {`
`@@ -324,7 +325,7 @@ func translateIngress(ns string, policyName string, targetSelector metav1.LabelS`
`324`	`325`	`util.IptablesCommentModuleFlag,`
`325`	`326`	`util.IptablesCommentFlag,`
`326`	`327`	`"ALLOW-"+cidrIpsetName+`
`327`		`- "-:-"+craftPartialIptablesCommentFromPort(portRule, util.IptablesDstPortFlag)+`
	`328`	`+ "-AND-"+craftPartialIptablesCommentFromPort(portRule, util.IptablesDstPortFlag)+`
`328`	`329`	`"-TO-"+targetSelectorComment,`
`329`	`330`	`)`
`330`	`331`	`fromRuleEntries = append(fromRuleEntries, entry)`
`@@ -353,19 +354,19 @@ func translateIngress(ns string, policyName string, targetSelector metav1.LabelS`
`353`	`354`	`util.IptablesCommentModuleFlag,`
`354`	`355`	`util.IptablesCommentFlag,`
`355`	`356`	`"ALLOW-"+cidrIpsetName+`
`356`		`- "-:-"+craftPartialIptablesCommentFromPort(portRule, util.IptablesDstPortFlag)+`
	`357`	`+ "-AND-"+craftPartialIptablesCommentFromPort(portRule, util.IptablesDstPortFlag)+`
`357`	`358`	`"-TO-"+targetSelectorComment,`
`358`	`359`	`)`
`359`	`360`	`fromRuleEntries = append(fromRuleEntries, entry)`
`360`	`361`	`}`
`361`	`362`	`}`
`362`	`363`	`} else {`
`363`		`- cidrEntry := &iptm.IptEntry{`
	`364`	`+ entry := &iptm.IptEntry{`
`364`	`365`	`Chain: util.IptablesAzureIngressFromChain,`
`365`	`366`	`Specs: append([]string(nil), targetSelectorIptEntrySpec...),`
`366`	`367`	`}`
`367`		`- cidrEntry.Specs = append(`
`368`		`- cidrEntry.Specs,`
	`368`	`+ entry.Specs = append(`
	`369`	`+ entry.Specs,`
`369`	`370`	`util.IptablesModuleFlag,`
`370`	`371`	`util.IptablesSetModuleFlag,`
`371`	`372`	`util.IptablesMatchSetFlag,`
`@@ -379,9 +380,10 @@ func translateIngress(ns string, policyName string, targetSelector metav1.LabelS`
`379`	`380`	`"ALLOW-"+cidrIpsetName+`
`380`	`381`	`"-TO-"+targetSelectorComment,`
`381`	`382`	`)`
`382`		`- fromRuleEntries = append(fromRuleEntries, cidrEntry)`
	`383`	`+ fromRuleEntries = append(fromRuleEntries, entry)`
`383`	`384`	`addedIngressFromEntry = true`
`384`	`385`	`}`
	`386`	`+ addedCidrEntry = true`
`385`	`387`	`}`
`386`	`388`	`continue`
`387`	`389`	`}`
`@@ -799,6 +801,7 @@ func translateEgress(ns string, policyName string, targetSelector metav1.LabelSe`
`799`	`801`	`ipCidrs [][]string`
`800`	`802`	`entries []*iptm.IptEntry`
`801`	`803`	`toRuleEntries []*iptm.IptEntry`
	`804`	`+ addedCidrEntry bool // all cidr entry will be added in one set per from/to rule`
`802`	`805`	`addedEgressToEntry, addedPortEntry bool // add drop entry when there are non ALLOW-ALL* rules`
`803`	`806`	`)`
`804`	`807`
`@@ -927,7 +930,7 @@ func translateEgress(ns string, policyName string, targetSelector metav1.LabelSe`
`927`	`930`	`}`
`928`	`931`	`addedEgressToEntry = true`
`929`	`932`	`}`
`930`		`- if j != 0 {`
	`933`	`+ if j != 0 && addedCidrEntry {`
`931`	`934`	`continue`
`932`	`935`	`}`
`933`	`936`	`if portRuleExists {`
`@@ -960,7 +963,7 @@ func translateEgress(ns string, policyName string, targetSelector metav1.LabelSe`
`960`	`963`	`util.IptablesCommentModuleFlag,`
`961`	`964`	`util.IptablesCommentFlag,`
`962`	`965`	`"ALLOW-"+cidrIpsetName+`
`963`		`- "-:-"+craftPartialIptablesCommentFromPort(portRule, util.IptablesDstPortFlag)+`
	`966`	`+ "-AND-"+craftPartialIptablesCommentFromPort(portRule, util.IptablesDstPortFlag)+`
`964`	`967`	`"-FROM-"+targetSelectorComment,`
`965`	`968`	`)`
`966`	`969`	`toRuleEntries = append(toRuleEntries, entry)`
`@@ -989,30 +992,30 @@ func translateEgress(ns string, policyName string, targetSelector metav1.LabelSe`
`989`	`992`	`util.IptablesCommentModuleFlag,`
`990`	`993`	`util.IptablesCommentFlag,`
`991`	`994`	`"ALLOW-"+cidrIpsetName+`
`992`		`- "-:-"+craftPartialIptablesCommentFromPort(portRule, util.IptablesDstPortFlag)+`
	`995`	`+ "-AND-"+craftPartialIptablesCommentFromPort(portRule, util.IptablesDstPortFlag)+`
`993`	`996`	`"-FROM-"+targetSelectorComment,`
`994`	`997`	`)`
`995`	`998`	`toRuleEntries = append(toRuleEntries, entry)`
`996`	`999`	`}`
`997`	`1000`	`}`
`998`	`1001`	`} else {`
`999`		`- cidrEntry := &iptm.IptEntry{`
	`1002`	`+ entry := &iptm.IptEntry{`
`1000`	`1003`	`Chain: util.IptablesAzureEgressToChain,`
`1001`	`1004`	`}`
`1002`		`- cidrEntry.Specs = append(`
`1003`		`- cidrEntry.Specs,`
	`1005`	`+ entry.Specs = append(`
	`1006`	`+ entry.Specs,`
`1004`	`1007`	`targetSelectorIptEntrySpec...,`
`1005`	`1008`	`)`
`1006`		`- cidrEntry.Specs = append(`
`1007`		`- cidrEntry.Specs,`
	`1009`	`+ entry.Specs = append(`
	`1010`	`+ entry.Specs,`
`1008`	`1011`	`util.IptablesModuleFlag,`
`1009`	`1012`	`util.IptablesSetModuleFlag,`
`1010`	`1013`	`util.IptablesMatchSetFlag,`
`1011`	`1014`	`util.GetHashedName(cidrIpsetName),`
`1012`	`1015`	`util.IptablesDstFlag,`
`1013`	`1016`	`)`
`1014`		`- cidrEntry.Specs = append(`
`1015`		`- cidrEntry.Specs,`
	`1017`	`+ entry.Specs = append(`
	`1018`	`+ entry.Specs,`
`1016`	`1019`	`util.IptablesJumpFlag,`
`1017`	`1020`	`util.IptablesAccept,`
`1018`	`1021`	`util.IptablesModuleFlag,`
`@@ -1021,9 +1024,10 @@ func translateEgress(ns string, policyName string, targetSelector metav1.LabelSe`
`1021`	`1024`	`"ALLOW-"+cidrIpsetName+`
`1022`	`1025`	`"-FROM-"+targetSelectorComment,`
`1023`	`1026`	`)`
`1024`		`- toRuleEntries = append(toRuleEntries, cidrEntry)`
	`1027`	`+ toRuleEntries = append(toRuleEntries, entry)`
`1025`	`1028`	`addedEgressToEntry = true`
`1026`	`1029`	`}`
	`1030`	`+ addedCidrEntry = true`
`1027`	`1031`	`}`
`1028`	`1032`	`continue`
`1029`	`1033`	`}`