From 42c10b94dffb138cf46c77a1bc6c8a0f6fdd82ca Mon Sep 17 00:00:00 2001 From: vakr Date: Wed, 7 Jul 2021 14:29:27 -0700 Subject: [PATCH 1/3] Adding sort in comment for deterministic behavior --- npm/translatePolicy.go | 12 ++++++++---- npm/translatePolicy_test.go | 4 ++-- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/npm/translatePolicy.go b/npm/translatePolicy.go index d63538b449..6f046bf6bf 100644 --- a/npm/translatePolicy.go +++ b/npm/translatePolicy.go @@ -3,7 +3,9 @@ package npm import ( + "sort" "strconv" + "strings" "github.com/Azure/azure-container-networking/log" "github.com/Azure/azure-container-networking/npm/iptm" @@ -191,7 +193,7 @@ func craftPartialIptablesCommentFromSelector(ns string, selector *metav1.LabelSe ops = append(ops, op) } - var comment, prefix, postfix string + var prefix, postfix string if isNamespaceSelector { prefix = "ns-" } else { @@ -200,11 +202,13 @@ func craftPartialIptablesCommentFromSelector(ns string, selector *metav1.LabelSe } } - for i, _ := range labelsWithoutOps { - comment += prefix + ops[i] + labelsWithoutOps[i] - comment += "-AND-" + comments := []string{} + for i := range labelsWithoutOps { + comments = append(comments, prefix+ops[i]+labelsWithoutOps[i]) } + sort.Strings(comments) + comment := strings.Join(comments, "-AND-") + "-AND-" return comment[:len(comment)-len("-AND-")] + postfix } diff --git a/npm/translatePolicy_test.go b/npm/translatePolicy_test.go index 27d16018a4..289cacdc89 100644 --- a/npm/translatePolicy_test.go +++ b/npm/translatePolicy_test.go @@ -2473,7 +2473,7 @@ func TestAllowMultiplePodSelectors(t *testing.T) { util.IptablesModuleFlag, util.IptablesCommentModuleFlag, util.IptablesCommentFlag, - "ALLOW-ns-!ns:netpol-4537-x-AND-pod:b:c-AND-app:test:int-TO-pod:a:x-IN-ns-netpol-4537-x", + "ALLOW-ns-!ns:netpol-4537-x-AND-app:test:int-AND-pod:b:c-TO-pod:a:x-IN-ns-netpol-4537-x", }, }, &iptm.IptEntry{ @@ -2512,7 +2512,7 @@ func TestAllowMultiplePodSelectors(t *testing.T) { util.IptablesModuleFlag, util.IptablesCommentModuleFlag, util.IptablesCommentFlag, - "ALLOW-ns-!ns:netpol-4537-y-AND-pod:b:c-AND-app:test:int-TO-pod:a:x-IN-ns-netpol-4537-x", + "ALLOW-ns-!ns:netpol-4537-y-AND-app:test:int-AND-pod:b:c-TO-pod:a:x-IN-ns-netpol-4537-x", }, }, } From 366170c52ad9e1cb7430cfdc698e9efc779faf29 Mon Sep 17 00:00:00 2001 From: vakr Date: Wed, 7 Jul 2021 15:46:18 -0700 Subject: [PATCH 2/3] Fixing some other UTs' comments --- npm/translatePolicy_test.go | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/npm/translatePolicy_test.go b/npm/translatePolicy_test.go index 289cacdc89..527c3a89e7 100644 --- a/npm/translatePolicy_test.go +++ b/npm/translatePolicy_test.go @@ -343,7 +343,7 @@ func TestCraftPartialIptablesCommentFromSelector(t *testing.T) { }, } comment = craftPartialIptablesCommentFromSelector("testnamespace", selector, false) - expectedComment = "k0:v0-AND-!k2-AND-k1:v10:v11-IN-ns-testnamespace" + expectedComment = "!k2-AND-k0:v0-AND-k1:v10:v11-IN-ns-testnamespace" if comment != expectedComment { t.Errorf("TestCraftPartialIptablesCommentFromSelector failed @ normal selector comparison") t.Errorf("comment:\n%v", comment) @@ -371,7 +371,7 @@ func TestCraftPartialIptablesCommentFromSelector(t *testing.T) { }, } comment = craftPartialIptablesCommentFromSelector("", nsSelector, true) - expectedComment = "ns-k0:v0-AND-ns-!k2-AND-ns-k1:v10:v11" + expectedComment = "ns-!k2-AND-ns-k0:v0-AND-ns-k1:v10:v11" if comment != expectedComment { t.Errorf("TestCraftPartialIptablesCommentFromSelector failed @ namespace selector comparison") t.Errorf("comment:\n%v", comment) @@ -425,7 +425,7 @@ func TestGetDefaultDropEntries(t *testing.T) { util.IptablesModuleFlag, util.IptablesCommentModuleFlag, util.IptablesCommentFlag, - "DROP-ALL-TO-context:dev-AND-!testNotIn:frontend-IN-ns-testnamespace", + "DROP-ALL-TO-!testNotIn:frontend-AND-context:dev-IN-ns-testnamespace", }, }, } @@ -465,7 +465,7 @@ func TestGetDefaultDropEntries(t *testing.T) { util.IptablesModuleFlag, util.IptablesCommentModuleFlag, util.IptablesCommentFlag, - "DROP-ALL-FROM-context:dev-AND-!testNotIn:frontend-IN-ns-testnamespace", + "DROP-ALL-FROM-!testNotIn:frontend-AND-context:dev-IN-ns-testnamespace", }, }, } @@ -505,7 +505,7 @@ func TestGetDefaultDropEntries(t *testing.T) { util.IptablesModuleFlag, util.IptablesCommentModuleFlag, util.IptablesCommentFlag, - "DROP-ALL-TO-context:dev-AND-!testNotIn:frontend-IN-ns-testnamespace", + "DROP-ALL-TO-!testNotIn:frontend-AND-context:dev-IN-ns-testnamespace", }, }, &iptm.IptEntry{ @@ -532,7 +532,7 @@ func TestGetDefaultDropEntries(t *testing.T) { util.IptablesModuleFlag, util.IptablesCommentModuleFlag, util.IptablesCommentFlag, - "DROP-ALL-FROM-context:dev-AND-!testNotIn:frontend-IN-ns-testnamespace", + "DROP-ALL-FROM-!testNotIn:frontend-AND-context:dev-IN-ns-testnamespace", }, }, } @@ -717,7 +717,7 @@ func TestTranslateIngress(t *testing.T) { util.IptablesModuleFlag, util.IptablesCommentModuleFlag, util.IptablesCommentFlag, - "ALLOW-app:db-AND-testIn:frontend-IN-ns-testnamespace-AND-TCP-PORT-6783-TO-context:dev-AND-!testNotIn:frontend-IN-ns-testnamespace", + "ALLOW-app:db-AND-testIn:frontend-IN-ns-testnamespace-AND-TCP-PORT-6783-TO-!testNotIn:frontend-AND-context:dev-IN-ns-testnamespace", }, }, &iptm.IptEntry{ @@ -760,7 +760,7 @@ func TestTranslateIngress(t *testing.T) { util.IptablesModuleFlag, util.IptablesCommentModuleFlag, util.IptablesCommentFlag, - "ALLOW-ns-ns:dev-AND-ns-testIn:frontendns-AND-TCP-PORT-6783-TO-context:dev-AND-!testNotIn:frontend-IN-ns-testnamespace", + "ALLOW-ns-ns:dev-AND-ns-testIn:frontendns-AND-TCP-PORT-6783-TO-!testNotIn:frontend-AND-context:dev-IN-ns-testnamespace", }, }, &iptm.IptEntry{ @@ -814,7 +814,7 @@ func TestTranslateIngress(t *testing.T) { util.IptablesModuleFlag, util.IptablesCommentModuleFlag, util.IptablesCommentFlag, - "ALLOW-ns-planet:earth-AND-ns-keyExists-AND-region:northpole-AND-!k-AND-TCP-PORT-6783-TO-context:dev-AND-!testNotIn:frontend-IN-ns-testnamespace", + "ALLOW-ns-keyExists-AND-ns-planet:earth-AND-!k-AND-region:northpole-AND-TCP-PORT-6783-TO-!testNotIn:frontend-AND-context:dev-IN-ns-testnamespace", }, }, } @@ -1000,7 +1000,7 @@ func TestTranslateEgress(t *testing.T) { util.IptablesModuleFlag, util.IptablesCommentModuleFlag, util.IptablesCommentFlag, - "ALLOW-app:db-AND-testIn:frontend-IN-ns-testnamespace-AND-TCP-PORT-6783-FROM-context:dev-AND-!testNotIn:frontend-IN-ns-testnamespace", + "ALLOW-app:db-AND-testIn:frontend-IN-ns-testnamespace-AND-TCP-PORT-6783-FROM-!testNotIn:frontend-AND-context:dev-IN-ns-testnamespace", }, }, &iptm.IptEntry{ @@ -1043,7 +1043,7 @@ func TestTranslateEgress(t *testing.T) { util.IptablesModuleFlag, util.IptablesCommentModuleFlag, util.IptablesCommentFlag, - "ALLOW-ns-ns:dev-AND-ns-testIn:frontendns-AND-TCP-PORT-6783-FROM-context:dev-AND-!testNotIn:frontend-IN-ns-testnamespace", + "ALLOW-ns-ns:dev-AND-ns-testIn:frontendns-AND-TCP-PORT-6783-FROM-!testNotIn:frontend-AND-context:dev-IN-ns-testnamespace", }, }, &iptm.IptEntry{ @@ -1097,7 +1097,7 @@ func TestTranslateEgress(t *testing.T) { util.IptablesModuleFlag, util.IptablesCommentModuleFlag, util.IptablesCommentFlag, - "ALLOW-context:dev-AND-!testNotIn:frontend-IN-ns-testnamespace-TO-ns-planet:earth-AND-ns-keyExists-AND-region:northpole-AND-!k-AND-TCP-PORT-6783", + "ALLOW-!testNotIn:frontend-AND-context:dev-IN-ns-testnamespace-TO-ns-keyExists-AND-ns-planet:earth-AND-!k-AND-region:northpole-AND-TCP-PORT-6783", }, }, } @@ -1588,7 +1588,7 @@ func TestAllowNamespaceDevToAppFrontend(t *testing.T) { util.IptablesModuleFlag, util.IptablesCommentModuleFlag, util.IptablesCommentFlag, - "ALLOW-ns-namespace:dev-AND-ns-!namespace:test0-TO-app:frontend-IN-ns-testnamespace", + "ALLOW-ns-!namespace:test0-AND-ns-namespace:dev-TO-app:frontend-IN-ns-testnamespace", }, }, &iptm.IptEntry{ @@ -1622,7 +1622,7 @@ func TestAllowNamespaceDevToAppFrontend(t *testing.T) { util.IptablesModuleFlag, util.IptablesCommentModuleFlag, util.IptablesCommentFlag, - "ALLOW-ns-namespace:dev-AND-ns-!namespace:test1-TO-app:frontend-IN-ns-testnamespace", + "ALLOW-ns-!namespace:test1-AND-ns-namespace:dev-TO-app:frontend-IN-ns-testnamespace", }, }, &iptm.IptEntry{ @@ -1731,7 +1731,7 @@ func TestAllowAllToK0AndK1AndAppFrontend(t *testing.T) { util.IptablesModuleFlag, util.IptablesCommentModuleFlag, util.IptablesCommentFlag, - "ALLOW-all-namespaces-TO-app:frontend-AND-!k0-AND-k1:v0:v1-IN-ns-testnamespace", + "ALLOW-all-namespaces-TO-!k0-AND-app:frontend-AND-k1:v0:v1-IN-ns-testnamespace", }, }, &iptm.IptEntry{ @@ -1763,7 +1763,7 @@ func TestAllowAllToK0AndK1AndAppFrontend(t *testing.T) { util.IptablesModuleFlag, util.IptablesCommentModuleFlag, util.IptablesCommentFlag, - "DROP-ALL-TO-app:frontend-AND-!k0-AND-k1:v0:v1-IN-ns-testnamespace", + "DROP-ALL-TO-!k0-AND-app:frontend-AND-k1:v0:v1-IN-ns-testnamespace", }, }, } From 4fbc9e3e292751eff820e9438065345f116ada34 Mon Sep 17 00:00:00 2001 From: vakr Date: Thu, 8 Jul 2021 08:56:53 -0700 Subject: [PATCH 3/3] addressing some comments --- npm/translatePolicy.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/npm/translatePolicy.go b/npm/translatePolicy.go index 6f046bf6bf..0c981aac04 100644 --- a/npm/translatePolicy.go +++ b/npm/translatePolicy.go @@ -208,8 +208,7 @@ func craftPartialIptablesCommentFromSelector(ns string, selector *metav1.LabelSe } sort.Strings(comments) - comment := strings.Join(comments, "-AND-") + "-AND-" - return comment[:len(comment)-len("-AND-")] + postfix + return strings.Join(comments, "-AND-") + postfix } func appendSelectorLabelsToLists(lists, listLabelsWithMembers map[string][]string, isNamespaceSelector bool) {