[NPM] 🐞 {fix} error creating IPsets when same string for ports and labels is used #734

vakalapa · 2020-11-18T04:19:00Z

Reason for Change:

NPM creates IPsets for the following today:

NameSpace
Label
LabelKey:Value
NamedPorts
CIDR blocks

Namespace and namespace labels are getting a ns- prefix helping NPM to distinguish between PodLabels and NameSpaceLabels. But there are several instances where the IPSet names for Labels and NamedPorts are surfacing. This occurs when a podLabel http:test and a namedPort http is created. An example can be seen in #733 . NPM hashes http from label and creates a IpSet with this hash, then when it is adding namedPort http it would hash to the same value and it logs this below error.

Stderr: [exit status 1, ipset v6.34: Syntax error: Elem separator in <IP_ADDR>,<PORT_NUM>, but settype hash:net supports none.]

Using ":" as prefix delimiter as k8s does not allow using a ":" in label names.

Issue Fixed:
#733

Further Enhancements
Name clash probability is still high. Work item is added to have a robust logic while adding IpSets

uses conventional commit messages
includes documentation
adds unit tests

csfmomo · 2020-11-18T04:56:51Z

npm/pod_test.go

 	if err := npMgr.AddPod(podObj); err != nil {
 		t.Errorf("TestAddPod failed @ AddPod")
 	}
+	if !ipsMgr.Exists(util.GetHashedName("app:test-pod"), "1.2.3.4,8080", "") {


Will it be port:app:test-pod?

Yes thank you.

codecov · 2020-11-18T07:51:05Z

Codecov Report

Merging #734 (3aba506) into master (28207fc) will increase coverage by 0.29%.
The diff coverage is 47.12%.

@@            Coverage Diff             @@
##           master     #734      +/-   ##
==========================================
+ Coverage   39.06%   39.36%   +0.29%     
==========================================
  Files          83       83              
  Lines       10713    10827     +114     
==========================================
+ Hits         4185     4262      +77     
- Misses       6022     6061      +39     
+ Partials      506      504       -2

neaggarwMS · 2020-11-18T17:07:18Z

npm/npm.go

 	iptMgr.UninitNpmChains()

+	log.Logf("Azure-NPM creating, cleaning existing IPSets")
+	destroyErr := ipsm.NewIpsetManager().Destroy()


Wont this cleanup all the ipsets from that node? We should only clean the ones created by Azure-NPM. Also ignore the error as this is best effort to clean the old ipsets.

As we discussed offline, NPM is not saving state any state while updating, to avoid any leak of ipsets, we will destroy everything. I logged a work item to add state fullness to this logic.

Agree with Neha here, an ipset flush wipes out all ipsets, including ones NPM may have not created, this is where we need to iterate through all ipsets that contain azure-npm

Added support to just delete azure-npm-(numerics) ipsets and lists

neaggarwMS · 2020-11-18T17:09:51Z

npm/util/const.go

 	IpsetNomatch string = "nomatch"
+
+	//Prefixes for ipsets
+	NamedPortIPSetPrefix string = "port:"


Can we rename the value from port: to portname: (or namedport:):, port seems very common

neaggarwMS · 2020-11-18T17:19:05Z

npm/translatePolicy.go

 						for _, portRule := range rule.Ports {
 							if portRule.Port != nil && portRule.Port.IntValue() == 0 {
-								portName := portRule.Port.String()
+								portName := util.NamedPortIPSetPrefix + portRule.Port.String()


Can we confirm this? We can still have a port rule without a named port, right?

Okay checked the logic, all port strings are getting appending to Namedports and in the end we call util.DropEmptyFields(namedPorts) (Line: 1438). This is weird. But with the updated logic empty port strings will not be dropped. We should only add if portRule.port.String() is not empty. This should have been the logic at first place instead of double parsing :-/

We should still add a test with policy which has port rules (without named ports) and one without

NPM is allowing this below network policy, corrected the behavior now.

ingress: - from: - podSelector: matchLabels: app: backend ports: - port: ""

…l case

neaggarwMS

Left minor comments, rest looks good.

neaggarwMS · 2020-11-19T05:36:34Z

npm/ipsm/ipsm.go

+		entry.operationFlag = util.IpsetDestroyFlag
+		entry.set = ipsetName
+		if _, err := ipsMgr.Run(entry); err != nil {
+			metrics.SendErrorMetric(util.IpsmID, "Error: failed to destroy ipset %s", ipsetName)


Can you add the FunctionName in the log statement for better debugability: DestroyNpmIpsets (We dont print the FileName/Line# in the Geneva output, although we should.)

neaggarwMS · 2020-11-19T05:41:18Z

npm/ipsm/ipsm.go

+
+		if _, err := ipsMgr.Run(entry); err != nil {
+			metrics.SendErrorMetric(util.IpsmID, "Error: failed to flush ipset %s", ipsetName)
+			log.Logf("Error: failed to flush ipset %s", ipsetName)


SendErrorMetric also logs the error. Actually that is an incorrect functionName it should be SendErrorLog or SendLog

neaggarwMS · 2020-11-19T05:41:32Z

npm/ipsm/ipsm.go

+		entry.set = ipsetName
+		if _, err := ipsMgr.Run(entry); err != nil {
+			metrics.SendErrorMetric(util.IpsmID, "Error: failed to destroy ipset %s", ipsetName)
+			log.Logf("Error: failed to destroy ipset %s", ipsetName)


Again remove the duplicate logF line

neaggarwMS · 2020-11-19T05:46:58Z

npm/translatePolicy.go

 }

+func getPortType(portRule networkingv1.NetworkPolicyPort) string {
+	if portRule.Port == nil {


nit: We can combine 1st and last check
if portRule.Port == nil || portRule.Port.IntValue() != 0
{
return "validport"
} else ...

csfmomo · 2020-11-19T22:01:18Z

npm/npm.go

+	log.Logf("Azure-NPM creating, cleaning existing Azure NPM IPSets")
+	destroyErr := ipsm.NewIpsetManager().DestroyNpmIpsets()
+	if destroyErr != nil {
+		log.Logf("Azure-NPM error occurred while destroying existing IPSets err: %s", destroyErr.Error())


nit, wondering, if DestroyNpmIpsets did metrics.SendErrorMetric inside its function, whether do we want to log here again.

csfmomo · 2020-11-19T22:04:13Z

npm/ipsm/ipsm.go

+	for _, matchedItem := range ipsetRegexSlice {
+		if len(matchedItem) == 2 {
+			itemString := string(matchedItem[1])
+			if strings.Contains(itemString, "azure-npm") {


nit, do we want to use a const for "azure-npm"

matmerr · 2020-11-19T23:05:48Z

npm/translatePolicy.go


+func getPortType(portRule networkingv1.NetworkPolicyPort) string {
+	if portRule.Port == nil || portRule.Port.IntValue() != 0 {
+		return "validport"


can we make these consts in case we end up using this elsewhere?

vakalapa added 2 commits November 17, 2020 20:14

Adding port_ format for named ports

985f077

Cleaning existing ipsets incase of a upgrade

87b0dc3

vakalapa marked this pull request as ready for review November 18, 2020 04:42

vakalapa requested review from csfmomo, matmerr, neaggarwMS and paulgmiller November 18, 2020 04:42

vakalapa added 2 commits November 17, 2020 20:49

Changing the delimiter for port prefix

0c692a2

Some basic formatting

422add6

vakalapa linked an issue Nov 18, 2020 that may be closed by this pull request

[NPM] issue with using same string for namedPort and labelKey #733

Closed

csfmomo reviewed Nov 18, 2020

View reviewed changes

Adding fixes to testcases

192023c

neaggarwMS reviewed Nov 18, 2020

View reviewed changes

vakalapa added 6 commits November 18, 2020 12:22

Addressing comments

c16bc13

Adding mitigation for empty string in named port

3806028

Changing port nil behavior. NPM does partial rule handling in port ni…

c9df942

…l case

removing exists check, as setmap is not accessible from this test

b230958

Adding support to delete only azure-npm ipsets

d5b0471

Adding support to delete only azure-npm ipsets

f564ec9

neaggarwMS reviewed Nov 19, 2020

View reviewed changes

Addressing comments

a8a8e94

csfmomo reviewed Nov 19, 2020

View reviewed changes

csfmomo previously approved these changes Nov 19, 2020

View reviewed changes

Changing azure-npm to const flag and cleaning up un wanted error log

013a36b

vakalapa dismissed csfmomo’s stale review via 013a36b November 19, 2020 22:10

csfmomo previously approved these changes Nov 19, 2020

View reviewed changes

matmerr reviewed Nov 19, 2020

View reviewed changes

neaggarwMS previously approved these changes Nov 20, 2020

View reviewed changes

Changing the make entries to 0

3aba506

vakalapa dismissed stale reviews from neaggarwMS and csfmomo via 3aba506 November 20, 2020 01:23

csfmomo approved these changes Nov 20, 2020

View reviewed changes

vakalapa merged commit 8ae7b8a into master Nov 20, 2020

vakalapa deleted the vakr/npm_port_name branch November 20, 2020 07:49

[NPM] 🐞 {fix} error creating IPsets when same string for ports and labels is used #734

[NPM] 🐞 {fix} error creating IPsets when same string for ports and labels is used #734

Uh oh!

Conversation

vakalapa commented Nov 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Nov 18, 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!

neaggarwMS Nov 18, 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!

neaggarwMS 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!

csfmomo Nov 19, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vakalapa commented Nov 18, 2020 •

edited

Loading

codecov bot commented Nov 18, 2020 •

edited

Loading

neaggarwMS Nov 18, 2020 •

edited

Loading

csfmomo Nov 19, 2020 •

edited

Loading