[NPM] [BUG] Supporting multiple values under label selector MatchExpr #863

vakalapa · 2021-04-30T23:04:56Z

This PR is fix the bug of supporting multiple values in a LabelSelector match expressions.

 - key: pod
    operator: In
    values:
      - b
      - c

Today NPM considers the values of the above selector to be b AND c, but it should be b OR c

NPM now follows different design for podSelector and nameSpaceSelector.

For PodSelector, NPM has ipsets with immutable members like ips or ip,proto:port. Ipset allows creating 2nd level ipset ype list:set to be created with pod label ipsets as members and NPM will use this 2nd level ipset as the matchset criteria in rules.

For NameSpaceSelector, NPM uses ipsets with mutable members like other ipsets. Ipset does not allow adding list:set as members to list:set ipset, so 2nd level ipset cannot be used in case of nameSelectors. For this reason NPM will create combinations of nameSpaceSelectors and add individual rule for each combination.

…sions

…ontainer-networking into vakr/k8smultiparse

vakalapa · 2021-05-19T23:27:49Z

/azp run

azure-pipelines · 2021-05-19T23:28:19Z

Azure Pipelines successfully started running 2 pipeline(s).

npm/parseSelector_test.go

npm/parseSelector.go

JungukCho · 2021-05-20T21:28:13Z

npm/parseSelector.go

+		return []metav1.LabelSelector{*nsSelector}
+	}
+
+	// Now use the baseSelector and loop over multiValueMatchExprs to create all


Highly suggest moving codes from 185-192 to "zipMatchExprs". It will improve readability. Now, I need to track for loop in 190 with zipMatchExprs function which uses returned values as input again.

Or moving codes from "zipMatchExprs" to here. The function name of "zipMatchExprs" is not very intuitive at least to me. I think "FlattenNameSpaceSelector" function has a good name and combining all codes in the function is good to understand and not deep.

I modelled it as a recursive function, where we take in a slice and expand it using each expr.

npm/parseSelector.go

npm/translatePolicy.go

JungukCho · 2021-05-20T23:44:54Z

npm/translatePolicy.go

-					for i, _ := range nsLabelsWithoutOps {
-						// Add namespaces prefix to distinguish namespace ipset lists and pod ipsets
-						nsLabelsWithoutOps[i] = "ns-" + nsLabelsWithoutOps[i]
+				for _, nsSelector := range FlattenNameSpaceSelector(fromRule.NamespaceSelector) {


Prefer to splitting this one line to two lines to understand what it returns. Same to all in this file, but not strong opinion.

npm/parseSelector.go

JungukCho · 2021-07-01T19:48:21Z

npm/parseSelector.go

-	// for _, egressRule := range nsSelector {
-	// 	log.Logf("%+v", egressRule)
-	// }
+	/*


I am not sure we need these long comments. I think it was well explained in comments (140-142) and is better to add concise explanation in the comments (140-142) based on summary of these long comments.
For example, matchExpressions "ns in (netpol-x, netpol-y)" is converted to multiple matchExpressions [
"ns in (netpol-x)", "ns in (netpol-y)"], then translate policy will replicate each of these nsSelectors to add two different rules in iptables, resulting in OR condition between the values.

This comes back to what we discussed in our meeting today, having a visual representation of what the YAML looks like and what the transformation looks like will help other developers to quickly understand. I would like to keep this as is if its ok.

To be clear, I suggested putting comprehensive information (e.g., pods yaml and netpol yaml which shows the problem in our NPM) to reproduce the problem, which helps to understand big picture at the first. T
his code is very straightforward. So, I don't think we need this, but if you want to keep it, I don't have strong opinion.

JungukCho · 2021-07-01T19:56:02Z

npm/parseSelector_test.go

 		t.Errorf("TestFlattenNameSpaceSelector failed @ 2nd selector length check %+v", testSelectors)
 	}
+
+	expectedSelectors := []metav1.LabelSelector{


I think we can make these tests concise and simple for all unit tests in this function and improve readability as well.
There are many things are repeated (e.g., LabelSelectorRequirement and MatchLabels).
We can make it better. One example is below.

matchLabels := map[string]string{ "c": "d", "a": "b", } then use matchLabels whenever creating &metav1.LabelSelector.

added a common matchLabel to help with readability

JungukCho · 2021-07-01T20:01:40Z

npm/parseSelector_test.go

 		t.Errorf("TestFlattenNameSpaceSelector failed @ 1st selector length check %+v", testSelectors)
 	}

+	if !reflect.DeepEqual(testSelectors[0], *firstSelector) {


There are multiple time to check length of returned values from FlattenNameSpaceSelector and deepEqual in this function and TestFlattenNameSpaceSelectorWoMatchLabels function. So, I think it is better to make one helper function to check them together.

Makes sense. but they are only a couple and i feel the code is self explanatory in itself,

JungukCho · 2021-07-01T20:04:50Z

npm/parseSelector_test.go

+}
+
+func TestFlattenNameSpaceSelectorWoMatchLabels(t *testing.T) {
+	firstSelector := &metav1.LabelSelector{


Same here. You can make it concise and better readability by using variables to avoid unnecessary repeat.

JungukCho · 2021-07-01T20:19:49Z

npm/networkPolicyController.go

+		if err = ipsMgr.CreateList(listKey); err != nil {
+			return fmt.Errorf("[syncAddAndUpdateNetPol] Error: creating ipset list %s with err: %v", listKey, err)
+		}
+		ipsMgr.IpSetReferIncOrDec(listKey, util.IpsetSetListFlag, ipsm.IncrementOp)


In this case, I prefer to having another function which functions "CreateList and IPSetReferIncOrDec". It will be easy to remember and help to encapsulate details.
I assume that your next PR is about those reference counts, we can revisit this in the PR.

JungukCho · 2021-07-01T20:22:08Z

npm/ipsm/ipsm.go

 	return nil
 }

+// Clean removes all the empty sets & lists under the namespace.


When do we use this function?

Clean() function is not being currently, this is for future work items.

I would suggest not to add function which is not used in this PR. I think this is a good practice.
There are multiple reasons.

Reviewer do not need to spend time.

Add the function with the PR for your future work items. So, it will be well aligned and easy to understand PR.

There are possibility that this function will not be used in the future. At that time, this function just stays for long time.

I think you have future work item to use this function. So, add this function in the PR.

JungukCho · 2021-07-01T20:24:25Z

npm/ipsm/ipsm.go


-// IpSetReferIncOrDec checks if an element exists in setMap/listMap and then increases or decreases tis refer count.
+// IpSetReferIncOrDec checks if an element exists in setMap/listMap and then increases or decreases this referCount.
 func (ipsMgr *IpsetManager) IpSetReferIncOrDec(ipsetName string, kind string, countOperation ReferCountOperation) {


Do you have plan to extend this function for other types in addition to "list" as well?
In this PR, kind is always "list".

Yes in the next work item(refer coutn) we will extend support of this IncDec to SetMap where we will track the use of iptable rules and their references to a given ipset. we will track both kinds of ipsets.

JungukCho

Thank you for applying comments. Look forward to your next PR.

vakalapa · 2021-07-07T16:05:07Z

Merging, we got a green in Cyclonus. https://github.com/Azure/azure-container-networking/pull/863/checks?check_run_id=3004138884

vakalapa added 4 commits April 27, 2021 14:01

basic scenario and investigation

08dc844

adding some basic imcrements to yaml

ad6a2dd

First pass at adding 2nd level ipsets for multi value selector expres…

6668117

…sions

resolving some references for drop match fields

ac135bf

vakalapa added bug npm Related to NPM. labels Apr 30, 2021

vakalapa added 3 commits May 3, 2021 15:19

Adding some fixes around nil checks

08916ba

ignoring 1 value expr to be added to list mems

e031817

not adding lists to the setMap

8180aae

vakalapa linked an issue May 4, 2021 that may be closed by this pull request

NetworkPolicy should enforce policy based on Multiple PodSelectors and NamespaceSelector #851

Closed

vakalapa added 3 commits May 4, 2021 11:54

correcting some UTs

6c7ae92

Correcting UTs.

5299dc5

fixing all UTs

bf83fd4

vakalapa marked this pull request as ready for review May 5, 2021 02:11

vakalapa requested review from JungukCho, matmerr and neaggarwMS May 5, 2021 02:11

vakalapa added 13 commits May 5, 2021 10:55

Adding and cleaning some comments

928d281

basic scenario and investigation

0553248

adding some basic imcrements to yaml

661bbb4

First pass at adding 2nd level ipsets for multi value selector expres…

7fc6419

…sions

resolving some references for drop match fields

b8cd4b1

Adding some fixes around nil checks

4f7a8aa

ignoring 1 value expr to be added to list mems

d5cd6c3

not adding lists to the setMap

96bc1b7

correcting some UTs

e066429

Correcting UTs.

9c8af4c

fixing all UTs

1227743

Adding and cleaning some comments

fbb0bed

Merge branch 'vakr/k8smultiparse' of https://github.com/Azure/azure-c…

28f3166

…ontainer-networking into vakr/k8smultiparse