From ba00f593876c6712dc9c9eb21fecb59f2b33fc56 Mon Sep 17 00:00:00 2001 From: Junguk Cho Date: Fri, 16 Jul 2021 16:01:14 -0700 Subject: [PATCH 1/3] Make order of iptable entries in UT determinisitc in case of MultipleLabelsToMultipleLabels --- npm/translatePolicy_test.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/npm/translatePolicy_test.go b/npm/translatePolicy_test.go index 245528f4d7..9ef129b51f 100644 --- a/npm/translatePolicy_test.go +++ b/npm/translatePolicy_test.go @@ -4,6 +4,7 @@ import ( "encoding/json" "io/ioutil" "reflect" + "sort" "testing" "github.com/Azure/azure-container-networking/npm/iptm" @@ -2519,6 +2520,15 @@ func TestAllowMultiplePodSelectors(t *testing.T) { expectedIptEntries = append(expectedIptEntries, nonKubeSystemEntries...) // has egress, but empty map means allow all expectedIptEntries = append(expectedIptEntries, getDefaultDropEntries("netpol-4537-x", multiPodSlector.Spec.PodSelector, true, false)...) + + // Since the order of all ipset values in Specs is not guaranteed in MultiplePodSelectors case, sorting Specs is necessary before comparing them. + if len(iptEntries) == len(expectedIptEntries) { + for i := 0; i < len(expectedIptEntries); i++ { + sort.Strings(iptEntries[i].Specs) + sort.Strings(expectedIptEntries[i].Specs) + } + } + if !reflect.DeepEqual(iptEntries, expectedIptEntries) { t.Errorf("translatedPolicy failed @ allow-ns-y-z-pod-b-c policy comparison") marshalledIptEntries, _ := json.Marshal(iptEntries) From bf162f2c4436d851188803376e3fa250381c213d Mon Sep 17 00:00:00 2001 From: Junguk Cho Date: Tue, 20 Jul 2021 18:16:31 -0700 Subject: [PATCH 2/3] Only Sort ipsets according to direction in ipetries before comparing results --- .../allow-ns-y-z-pod-b-c-with-namedPort.yaml | 39 ++++++ npm/translatePolicy.go | 3 +- npm/translatePolicy_test.go | 132 ++++++++++++++---- npm/util/util.go | 10 +- 4 files changed, 147 insertions(+), 37 deletions(-) create mode 100644 npm/testpolicies/allow-ns-y-z-pod-b-c-with-namedPort.yaml diff --git a/npm/testpolicies/allow-ns-y-z-pod-b-c-with-namedPort.yaml b/npm/testpolicies/allow-ns-y-z-pod-b-c-with-namedPort.yaml new file mode 100644 index 0000000000..49f24ee133 --- /dev/null +++ b/npm/testpolicies/allow-ns-y-z-pod-b-c-with-namedPort.yaml @@ -0,0 +1,39 @@ +apiVersion: networking.k8s.io/v1 +kind: NetworkPolicy +metadata: + name: allow-ns-y-z-pod-b-c + namespace: netpol-4537-x +spec: + ingress: + - from: + - namespaceSelector: + matchExpressions: + - key: ns + operator: NotIn + values: + - netpol-4537-x + - netpol-4537-y + podSelector: + matchExpressions: + - key: pod + operator: In + values: + - b + - c + - key: app + operator: In + values: + - test + - int + ports: + - port: serve-80 + protocol: TCP + podSelector: + matchExpressions: + - key: pod + operator: In + values: + - a + - x + policyTypes: + - Ingress diff --git a/npm/translatePolicy.go b/npm/translatePolicy.go index 6efaf335eb..b85ed32ca6 100644 --- a/npm/translatePolicy.go +++ b/npm/translatePolicy.go @@ -1700,10 +1700,9 @@ func translatePolicy(npObj *networkingv1.NetworkPolicy) ([]string, []string, map } entries = append(entries, getDefaultDropEntries(npNs, npObj.Spec.PodSelector, hasIngress, hasEgress)...) - resultSets = util.UniqueStrSlice(resultSets) for resultListKey, resultLists := range resultListMap { resultListMap[resultListKey] = util.UniqueStrSlice(resultLists) } - return resultSets, resultNamedPorts, resultListMap, resultIngressIPCidrs, resultEgressIPCidrs, entries + return util.UniqueStrSlice(resultSets), util.UniqueStrSlice(resultNamedPorts), resultListMap, resultIngressIPCidrs, resultEgressIPCidrs, entries } diff --git a/npm/translatePolicy_test.go b/npm/translatePolicy_test.go index 9ef129b51f..44173a0a8e 100644 --- a/npm/translatePolicy_test.go +++ b/npm/translatePolicy_test.go @@ -2,6 +2,7 @@ package npm import ( "encoding/json" + "fmt" "io/ioutil" "reflect" "sort" @@ -2389,14 +2390,79 @@ func TestAllowAllFromAppBackend(t *testing.T) { } } +// sortedIpSetMap returns a map which has direction and sorted ipsets as key and values respectively. +func sortedIpSetMap(specs []string) map[string][]string { + var prevSpec string + namedPortIpSet := fmt.Sprintf("%s,%s", util.IptablesDstFlag, util.IptablesDstFlag) + ipSetMap := make(map[string][]string) + + // Direction is always followed after ipset + for _, spec := range specs { + if spec == util.IptablesDstFlag || spec == util.IptablesSrcFlag || spec == namedPortIpSet { + ipSetMap[spec] = append(ipSetMap[spec], prevSpec) + } + prevSpec = spec + } + + // sorting ipsets + for _, ipsets := range ipSetMap { + sort.Strings(ipsets) + } + return ipSetMap +} + +// equalSetNamesInIptEntries checks whether ipset is the same or not after sorting them +func equalIPSetsInIptEntries(iptEntries, expectedIptEntries []*iptm.IptEntry, t *testing.T) bool { + if len(iptEntries) != len(expectedIptEntries) { + return false + } + + for i := 0; i < len(expectedIptEntries); i++ { + IpSetMap := sortedIpSetMap(iptEntries[i].Specs) + expectedIpsetMap := sortedIpSetMap(expectedIptEntries[i].Specs) + + if !reflect.DeepEqual(IpSetMap, expectedIpsetMap) { + t.Errorf("Ipsets are different\n got %+v\n want %+v", IpSetMap, expectedIpsetMap) + return false + } + } + + return true +} + +// check all returned values from translation function against expected values +func checkNetPolTranslationResult(netPolPolicy string, sets, expectedSets []string, lists, expectedLists map[string][]string, iptEntries, expectedIptEntries []*iptm.IptEntry, t *testing.T) bool { + if !util.CompareSlices(sets, expectedSets) { + t.Errorf("translatedPolicy failed @ %s\n sets: %v\n expectedSets: %v", netPolPolicy, sets, expectedSets) + return false + } + + // TODO(jungukcho): check whether this (map) is the same issue or not before merging it to master + if !reflect.DeepEqual(lists, expectedLists) { + t.Errorf("translatedPolicy failed @ %s\n lists: %v\n expectedLists: %v", netPolPolicy, lists, expectedLists) + return false + } + + if !reflect.DeepEqual(iptEntries, expectedIptEntries) { + if !equalIPSetsInIptEntries(iptEntries, expectedIptEntries, t) { + marshalledIptEntries, _ := json.Marshal(iptEntries) + marshalledExpectedIptEntries, _ := json.Marshal(expectedIptEntries) + t.Errorf("translatedPolicy failed @ %s\n iptEntries: %s\n expectedIptEntries: %s", netPolPolicy, marshalledIptEntries, marshalledExpectedIptEntries) + return false + } + } + + return true +} func TestAllowMultiplePodSelectors(t *testing.T) { - multiPodSlector, err := readPolicyYaml("testpolicies/allow-ns-y-z-pod-b-c.yaml") + // TODO(jungukcho): need to set util.IsNewNwPolicyVerFlag as true. It is a very strong dependency. Need to remove this dependency. + util.IsNewNwPolicyVerFlag = true + netPolFile := "allow-ns-y-z-pod-b-c.yaml" + multiPodSlector, err := readPolicyYaml(fmt.Sprintf("testpolicies/%s", netPolFile)) if err != nil { t.Fatal(err) } - util.IsNewNwPolicyVerFlag = true - sets, _, lists, _, _, iptEntries := translatePolicy(multiPodSlector) expectedSets := []string{ @@ -2408,11 +2474,6 @@ func TestAllowMultiplePodSelectors(t *testing.T) { "app:test", "app:int", } - if !util.CompareSlices(sets, expectedSets) { - t.Errorf("translatedPolicy failed @ allow-ns-y-z-pod-b-c sets comparison") - t.Errorf("sets: %v", sets) - t.Errorf("expectedSets: %v", expectedSets) - } expectedLists := map[string][]string{ "app:test:int": { @@ -2430,14 +2491,8 @@ func TestAllowMultiplePodSelectors(t *testing.T) { "pod:c", }, } - if !reflect.DeepEqual(lists, expectedLists) { - t.Errorf("translatedPolicy failed @ allow-ns-y-z-pod-b-c lists comparison") - t.Errorf("lists: %v", lists) - t.Errorf("expectedLists: %v", expectedLists) - } - expectedIptEntries := []*iptm.IptEntry{} - nonKubeSystemEntries := []*iptm.IptEntry{ + expectedIptEntries := []*iptm.IptEntry{ { Chain: util.IptablesAzureIngressFromChain, Specs: []string{ @@ -2517,24 +2572,43 @@ func TestAllowMultiplePodSelectors(t *testing.T) { }, }, } - expectedIptEntries = append(expectedIptEntries, nonKubeSystemEntries...) - // has egress, but empty map means allow all expectedIptEntries = append(expectedIptEntries, getDefaultDropEntries("netpol-4537-x", multiPodSlector.Spec.PodSelector, true, false)...) - // Since the order of all ipset values in Specs is not guaranteed in MultiplePodSelectors case, sorting Specs is necessary before comparing them. - if len(iptEntries) == len(expectedIptEntries) { - for i := 0; i < len(expectedIptEntries); i++ { - sort.Strings(iptEntries[i].Specs) - sort.Strings(expectedIptEntries[i].Specs) - } + if !checkNetPolTranslationResult(netPolFile, sets, expectedSets, lists, expectedLists, iptEntries, expectedIptEntries, t) { + t.Errorf("translatedPolicy failed @ %s", netPolFile) } - if !reflect.DeepEqual(iptEntries, expectedIptEntries) { - t.Errorf("translatedPolicy failed @ allow-ns-y-z-pod-b-c policy comparison") - marshalledIptEntries, _ := json.Marshal(iptEntries) - marshalledExpectedIptEntries, _ := json.Marshal(expectedIptEntries) - t.Errorf("iptEntries: %s", marshalledIptEntries) - t.Errorf("expectedIptEntries: %s", marshalledExpectedIptEntries) + netPolFile = "allow-ns-y-z-pod-b-c-with-namedPort.yaml" + multiPodSlector, err = readPolicyYaml(fmt.Sprintf("testpolicies/%s", netPolFile)) + if err != nil { + t.Fatal(err) + } + + var namedPorts []string + sets, namedPorts, lists, _, _, iptEntries = translatePolicy(multiPodSlector) + + // reuse previously used ipentries + entriesForNamePort := []string{ + util.IptablesModuleFlag, + util.IptablesSetModuleFlag, + util.IptablesMatchSetFlag, + util.GetHashedName("namedport:serve-80"), + util.IptablesDstFlag + "," + util.IptablesDstFlag, + util.IptablesJumpFlag, + } + // Do not add namedPort entries in last drop rule + for i := 0; i < len(expectedIptEntries)-1; i++ { + expectedIptEntries[i].Specs = append(entriesForNamePort, expectedIptEntries[i].Specs...) + } + + // compared NamedPort results + expectedNamedPorts := []string{"namedport:serve-80"} + if !reflect.DeepEqual(namedPorts, expectedNamedPorts) { + t.Errorf("translatedPolicy failed namedPort @ %s comparison\n sets: %v\n expectedSets %v", netPolFile, namedPorts, expectedNamedPorts) + } + + if !checkNetPolTranslationResult(netPolFile, sets, expectedSets, lists, expectedLists, iptEntries, expectedIptEntries, t) { + t.Errorf("translatedPolicy failed @ %s", netPolFile) } } diff --git a/npm/util/util.go b/npm/util/util.go index 09d7b7d0d1..0112f64517 100644 --- a/npm/util/util.go +++ b/npm/util/util.go @@ -6,6 +6,7 @@ import ( "fmt" "hash/fnv" "os" + "reflect" "regexp" "sort" "strconv" @@ -334,10 +335,7 @@ func StrExistsInSlice(items []string, val string) bool { } func CompareSlices(list1, list2 []string) bool { - for _, item := range list1 { - if !StrExistsInSlice(list2, item) { - return false - } - } - return true + sort.Strings(list1) + sort.Strings(list2) + return reflect.DeepEqual(list1, list2) } From 9c8393ecff7fe686b79c890810eda76f354ac984 Mon Sep 17 00:00:00 2001 From: Junguk Cho Date: Tue, 20 Jul 2021 18:36:19 -0700 Subject: [PATCH 3/3] Correct the name of example network policy --- npm/testpolicies/allow-ns-y-z-pod-b-c-with-namedPort.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/npm/testpolicies/allow-ns-y-z-pod-b-c-with-namedPort.yaml b/npm/testpolicies/allow-ns-y-z-pod-b-c-with-namedPort.yaml index 49f24ee133..b42eb7d367 100644 --- a/npm/testpolicies/allow-ns-y-z-pod-b-c-with-namedPort.yaml +++ b/npm/testpolicies/allow-ns-y-z-pod-b-c-with-namedPort.yaml @@ -1,7 +1,7 @@ apiVersion: networking.k8s.io/v1 kind: NetworkPolicy metadata: - name: allow-ns-y-z-pod-b-c + name: allow-ns-y-z-pod-b-c-with-namedPort namespace: netpol-4537-x spec: ingress: