Move Cidr translation from iptable to ipset. #582

csfmomo · 2020-06-16T07:20:29Z

What this PR does / why we need it:
Ipset is an extension for iptable to store and look up ip blocks in an efficient hash way. Move ip cidr related rule to ipset will improve npm performance.

Which issue this PR fixes
It fixes perf issue when large amount of ip cidr rule got applied.

Special notes for your reviewer:
Examples of ipset and iptables rules with this change.

Release note:

codecov · 2020-06-16T07:25:04Z

Codecov Report

Merging #582 into master will increase coverage by 3.92%.
The diff coverage is 93.33%.

@@            Coverage Diff             @@
##           master     #582      +/-   ##
==========================================
+ Coverage   49.08%   53.00%   +3.92%     
==========================================
  Files          28       23       -5     
  Lines        3449     2858     -591     
==========================================
- Hits         1693     1515     -178     
+ Misses       1466     1071     -395     
+ Partials      290      272      -18

jaer-tsun · 2020-06-16T17:20:52Z

npm/nwpolicy.go

+func createCidrsRule(ingressOrEgress string, policyName string, ipsetEntries [][]string, ipsMgr *ipsm.IpsetManager) {
+	spec := append([]string{util.IpsetNetHashFlag, util.IpsetMaxelemName, util.IpsetMaxelemNum})
+	for i, ipCidrSet := range ipsetEntries {
+		if ipCidrSet == nil || len(ipCidrSet) == 0 {


translatepolicy should already remove empty sets

No, it didn't.
For the existing return object, including sets, namedPorts, lists, they are []string type and called UniqueStrSlice to remove empty set. However, for ipsetEntries, it's [][]string type and didn't do drop empty entries before returning. I intentionally didn't do that to keep index order for simplicity for deleting ipset when network policy rule updates.

jaer-tsun · 2020-06-16T17:35:26Z

npm/nwpolicy.go

+
+func removeCidrsRule(ingressOrEgress string, policyName string, ipsetEntries [][]string, ipsMgr *ipsm.IpsetManager) {
+	for i, ipCidrSet := range ipsetEntries {
+		if ipCidrSet == nil || len(ipCidrSet) == 0 {


I don't think this func will work as intended since the length of the set includes all the entries to be deleted

What I intend to do here is, delete ipset npm created when translate cidrs rule. Length of the set includes cidrs belong to this rule.
It works as intended. I test it by updating same network policy rules. It will remove and update the target ipset.

jaer-tsun · 2020-06-16T17:36:32Z

npm/nwpolicy.go

+		if ipCidrSet == nil || len(ipCidrSet) == 0 {
+			continue
+		}
+		setName := policyName + "-cidr" + strconv.Itoa(i) + ingressOrEgress


setName should include namespace

OK, I'll add ns but remove cidr for setName.

jaer-tsun · 2020-06-16T17:40:35Z

npm/translatePolicy.go

 		resultSets = append(resultSets, ingressSets...)
 		resultNamedPorts = append(resultNamedPorts, ingressNamedPorts...)
 		resultLists = append(resultLists, ingressLists...)
+		resultIngressIPCidrs = ingressIPCidrs


why do we copy?

No need to copy. Let me remove it.
Copied it following previous coding style but is't not needed here.

jaer-tsun · 2020-06-16T17:40:42Z

npm/translatePolicy.go

 		resultSets = append(resultSets, egressSets...)
 		resultNamedPorts = append(resultNamedPorts, egressNamedPorts...)
 		resultLists = append(resultLists, egressLists...)
+		resultEgressIPCidrs = egressIPCidrs


why do we copy?

Same as comment in line 1515.

jaer-tsun · 2020-06-16T17:40:53Z

npm/translatePolicy.go

 			resultSets = append(resultSets, ingressSets...)
 			resultNamedPorts = append(resultNamedPorts, ingressNamedPorts...)
 			resultLists = append(resultLists, ingressLists...)
+			resultIngressIPCidrs = ingressIPCidrs


why do we copy?

ingressIPCidrs is a local variable which under for loop. If we want to return it outside the for loop, we have to copy it to a non-local variable. Copy it to resultIngressIPCidrs to keep consistency with existing code.

jaer-tsun · 2020-06-16T17:41:02Z

npm/translatePolicy.go

 			resultSets = append(resultSets, egressSets...)
 			resultNamedPorts = append(resultNamedPorts, egressNamedPorts...)
 			resultLists = append(resultLists, egressLists...)
+			resultEgressIPCidrs = egressIPCidrs


why do we copy?

egressIPCidrs is a local variable which under for loop. If we want to return it outside the for loop, we have to copy it to a non-local variable. Copy it to resultEgressIPCidrs to keep consistency with existing code.

jaer-tsun · 2020-06-16T17:44:40Z

npm/translatePolicy.go

 			if toRule.IPBlock != nil {
 				if len(toRule.IPBlock.CIDR) > 0 {
+					ipCidrs[i] = append(ipCidrs[i], toRule.IPBlock.CIDR)
+					cidrIpsetName := name + "-cidr" + strconv.Itoa(i) + "out"


no need for "cidr" in name to hash

jaer-tsun · 2020-06-16T18:00:45Z

npm/translatePolicy.go

 			if fromRule.IPBlock != nil {
 				if len(fromRule.IPBlock.CIDR) > 0 {
+					ipCidrs[i] = append(ipCidrs[i], fromRule.IPBlock.CIDR)
+					cidrIpsetName := name + "-cidr" + strconv.Itoa(i) + "in"


no need for "-cidr", but add namespace

What about keeping the cidr to be more explicit? We just want this name to be unique.
At least we have to worry other ipset formed by podSelector or namespaceSelector from the same networkPolicyPeer in the future.

type NetworkPolicyPeer struct { PodSelector *metav1.LabelSelector NamespaceSelector *metav1.LabelSelector IPBlock *IPBlock }

Yes, keep cidr will make it more explicit but it's not a hard request for now. Ipset also has a limitation for the name string length.
Current NPM keeps ipset for pod and namespace already. For pod, npm uses pod label key, pod label key and value as ipset name. For namespace, upm uses the name of namespaces as ipset name. They are not conflicting with IPBlock ipset name.

jaer-tsun · 2020-06-16T18:26:14Z

npm/translatePolicy.go

+					cidrIpsetName := name + "-cidr" + strconv.Itoa(i) + "in"
+					if len(fromRule.IPBlock.Except) > 0 {
+						for _, except := range fromRule.IPBlock.Except {
+							ipCidrs[i] = append(ipCidrs[i], except + " " + util.IpsetNomatch)


we shouldn't have no matches since we're moving to allow based style

We can do it in next PR, let's have this PR focus on ip table to ip set change.

Maybe a TODO comment can help move this work to our next PR.

Let me add TODO comment.

mainred

Left a few comments, but to not block your work, I'll approve your work and leave you and Jaeryn make the last decision

mainred · 2020-06-17T08:01:45Z

npm/ipsm/ipsm.go

 		operationFlag: util.IpsetCreationFlag,
 		set:           util.GetHashedName(listName),
-		spec:          util.IpsetSetListFlag,
+		spec:          append([]string{util.IpsetSetListFlag}),


Suggested change

spec: append([]string{util.IpsetSetListFlag}),

spec: []string{util.IpsetSetListFlag},

mainred · 2020-06-17T08:02:31Z

npm/ipsm/ipsm.go

 		operationFlag: util.IpsetAppendFlag,
 		set:           util.GetHashedName(listName),
-		spec:          util.GetHashedName(setName),
+		spec:          append([]string{util.GetHashedName(setName)}),


mainred · 2020-06-17T08:02:50Z

npm/ipsm/ipsm.go

 		operationFlag: util.IpsetDeletionFlag,
 		set:           hashedListName,
-		spec:          hashedSetName,
+		spec:          append([]string{hashedSetName}),


mainred · 2020-06-17T09:09:45Z

npm/nwpolicy.go

 	return nil
 }
+
+func createCidrsRule(ingressOrEgress string, policyName string, ipsetEntries [][]string, ipsMgr *ipsm.IpsetManager) {


what about moving createCidrsRule and removeCidrsRule to ipsm.IpsetManager?

what about moving createCidrsRule and removeCidrsRule to ipsm.IpsetManager?

createCidrsRule and removeCidrsRule are only needed during AddPolicy function which locate at nwpolicy.go. They won't be reused any where else. I would like to keep them inside nwpolicy.go. They reuse the functions of CreateSet and AddToSet in ipsm.IpsetManager. ipsm keeps more generic functions.

mainred · 2020-06-17T09:16:33Z

npm/translatePolicy.go

 			if fromRule.IPBlock != nil {
 				if len(fromRule.IPBlock.CIDR) > 0 {
+					ipCidrs[i] = append(ipCidrs[i], fromRule.IPBlock.CIDR)
+					cidrIpsetName := name + "-cidr" + strconv.Itoa(i) + "in"


What about keeping the cidr to be more explicit? We just want this name to be unique.
At least we have to worry other ipset formed by podSelector or namespaceSelector from the same networkPolicyPeer in the future.

type NetworkPolicyPeer struct { PodSelector *metav1.LabelSelector NamespaceSelector *metav1.LabelSelector IPBlock *IPBlock }

mainred · 2020-06-17T09:17:44Z

npm/translatePolicy.go

+					cidrIpsetName := name + "-cidr" + strconv.Itoa(i) + "in"
+					if len(fromRule.IPBlock.Except) > 0 {
+						for _, except := range fromRule.IPBlock.Except {
+							ipCidrs[i] = append(ipCidrs[i], except + " " + util.IpsetNomatch)


Maybe a TODO comment can help move this work to our next PR.

mainred · 2020-06-17T09:20:10Z

npm/translatePolicy.go

 }

-func translateIngress(ns string, targetSelector metav1.LabelSelector, rules []networkingv1.NetworkPolicyIngressRule) ([]string, []string, []string, []*iptm.IptEntry) {
+func translateIngress(ns string, name string, targetSelector metav1.LabelSelector, rules []networkingv1.NetworkPolicyIngressRule) ([]string, []string, []string, [][]string, []*iptm.IptEntry) {


policyName to be more explicit?

mainred · 2020-06-17T09:31:29Z

npm/translatePolicy.go

+						}
+						addedIngressFromEntry = true
+					}
+					if j != 0 {


Besides IPBLocks, we also have podselector and namespace selector in the following steps, so we cannot continue to next rule item.

We may later refactor this lengthy function to be more clean and easy to maintain.
CC @jaer-tsun

Besides IPBLocks, we also have podselector and namespace selector in the following steps, so we cannot continue to next rule item.

We can continue. Please note the continue condition is when j != 0 which means the podselector and namespace selector has already been added in iptable entry when j = 0. It's a key point to improve our performance. When j = 0 which means npm loop through the 1st item in ipBlock, npm will create a cidr ipset, create a iptable entry. When j > 0, npm just need to add cidr to ipset entry, no need to create iptable entries anymore.

We may later refactor this lengthy function to be more clean and easy to maintain.
CC @jaer-tsun

Agree, I drafted a PR to refactor translatePolicy.go to remove duplicated code between translateIngress and translateEgress and make it concise.

mainred · 2020-06-17T09:36:50Z

npm/translatePolicy.go

 }

-func translateEgress(ns string, targetSelector metav1.LabelSelector, rules []networkingv1.NetworkPolicyEgressRule) ([]string, []string, []string, []*iptm.IptEntry) {
+func translateEgress(ns string, name string, targetSelector metav1.LabelSelector, rules []networkingv1.NetworkPolicyEgressRule) ([]string, []string, []string, [][]string, []*iptm.IptEntry) {


Suggested change

func translateEgress(ns string, name string, targetSelector metav1.LabelSelector, rules []networkingv1.NetworkPolicyEgressRule) ([]string, []string, []string, [][]string, []*iptm.IptEntry) {

func translateEgress(ns string, policyName string, targetSelector metav1.LabelSelector, rules []networkingv1.NetworkPolicyEgressRule) ([]string, []string, []string, [][]string, []*iptm.IptEntry) {

mainred

gofmt is also needed.

csfmomo · 2020-06-18T04:33:43Z

gofmt

Let me open a separate PR for gofmt.

jaer-tsun

/lgtm

Move Cidr translation from iptable to ipset.

f747a4b

csfmomo requested a review from jaer-tsun June 16, 2020 07:20

jaer-tsun suggested changes Jun 16, 2020

View reviewed changes

jaer-tsun reviewed Jun 16, 2020

View reviewed changes

mainred previously approved these changes Jun 17, 2020

View reviewed changes

mainred reviewed Jun 17, 2020

View reviewed changes

Move Cidr translation from iptable to ipset.

15716a6

csfmomo dismissed mainred’s stale review via 15716a6 June 17, 2020 15:28

Move Cidr translation from iptable to ipset.

46bb100

jaer-tsun approved these changes Jun 22, 2020

View reviewed changes

csfmomo merged commit 19fb3e6 into master Jun 22, 2020

csfmomo deleted the moveToIpsetRebased branch July 10, 2020 21:45

csfmomo restored the moveToIpsetRebased branch July 10, 2020 21:45

csfmomo deleted the moveToIpsetRebased branch July 10, 2020 21:46

	spec: append([]string{util.IpsetSetListFlag}),
	spec: []string{util.IpsetSetListFlag},

	func translateEgress(ns string, name string, targetSelector metav1.LabelSelector, rules []networkingv1.NetworkPolicyEgressRule) ([]string, []string, []string, [][]string, []*iptm.IptEntry) {
	func translateEgress(ns string, policyName string, targetSelector metav1.LabelSelector, rules []networkingv1.NetworkPolicyEgressRule) ([]string, []string, []string, [][]string, []*iptm.IptEntry) {

Move Cidr translation from iptable to ipset. #582

Move Cidr translation from iptable to ipset. #582

Uh oh!

Conversation

csfmomo commented Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csfmomo Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csfmomo Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csfmomo Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mainred left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csfmomo commented Jun 16, 2020 •

edited

Loading

codecov bot commented Jun 16, 2020 •

edited

Loading

csfmomo Jun 16, 2020 •

edited

Loading

csfmomo Jun 16, 2020 •

edited

Loading

csfmomo Jun 16, 2020 •

edited

Loading