fix: [NPM] Testing Changes and updates to Linux Policy Manager #1114

huntergregory · 2021-11-18T03:17:21Z

Linux DP wasn't working for integration tests before this PR. Now it does.

It will fail conformance tests though due to the first problem in the TODOs below.

TODO in followup PR:

fix problem: accept rules for iptables ingress chains jump to EGRESS instead of jumping to ALLOW-INGRESS-MARK chain
uncomment reboot code for Linux Policy Manager (reset and initialize Azure chains when we have 0 network policies)
call reconcile for policy manager within DP
getting the chain line number will return the first digit of the line number, not the whole number. So we might think position 15 is ok when we need position 1.

Notes on Changes

Linux Policy Manager fixes (in chain-management_linux.go):

make the jump from FORWARD chain to AZURE-NPM chain the first rule in the FORWARD chain
Don't throw an error if we fail to delete this jump rule when it doesn't exist
fix how we reset chains by only flushing/delete current chains (as opposed to creating/flushing/deleting all possible chains before)
Also, don't make an iptables-restore call when there are no azure chains currently

Fix DP:

uncomment code that initializes policy manager
same with resetting

Unit Testing:

verify all exec calls now
fix UTs for grep
consolidate tests in chain-management_linux_test.go into tests with subtests for better readability and maintenance

Integration Testing:

Better testing for network policies (had to update some test network policies which impacted tests in policymanager_linux_test.go)

huntergregory · 2021-11-18T18:48:10Z

/azp run

azure-pipelines · 2021-11-18T18:48:28Z

Azure Pipelines successfully started running 2 pipeline(s).

JungukCho

Can you apply comments based on previous comments in the function (may need to slightly change for v2) when creating NewDataPlane - https://github.com/Azure/azure-container-networking/blob/master/npm/pkg/dataplane/dataplane.go#L70 ?
I am not sure how windows works, but for linux it is good to comment them.

npm/pkg/dataplane/dataplane_test.go

npm/pkg/dataplane/policies/chain-management_linux.go

JungukCho · 2021-11-18T18:18:35Z

npm/pkg/dataplane/policies/chain-management_linux.go

 	deleteErrCode, deleteErr := pMgr.runIPTablesCommand(util.IptablesDeletionFlag, jumpFromForwardToAzureChainArgs...)
-	hadDeleteError := deleteErr != nil && deleteErrCode != couldntLoadTargetErrorCode
-	// TODO check rule doesn't exist error code instead. The first call of dp.Reset() we will have exit code 2 (couldn't load target) since AZURE-NPM won't exist
+	hadDeleteError := deleteErr != nil && deleteErrCode != couldntLoadTargetErrorCode // couldntLoadTargetErrorCode happens when AZURE-NPM chain doesn't exist (and hence the jump rule doesn't exist too)


nitpick: slightly better to put comment above code?

// couldntLoadTargetErrorCode happens when AZURE-NPM chain doesn't exist (and hence the jump rule doesn't exist too) hadDeleteError := deleteErr != nil && deleteErrCode != couldntLoadTargetErrorCode

It is so tough to handle iptables and ipset errors.
Can we update comments which includes couldntLoadTargetErrorCode error can be ignorable?
But, what if the error is not couldntLoadTargetErrorCode? In the code, now the code sends log message.
So, I guess it is ok to proceed. It would be helpful to add comments about it.

It is not scope of this PR, but just bring it up.
I just realized couldntLoadTargetErrorCode is not easy to know this is global variable.
I looked for local variables. We had this approach of the codes in several places, but good to come up with some ideas to easily recognize it.
somehow

chainsErr.NoTargetExist

wdyt?

Will update comments. I agree that we can do something like chainsErr.NoTargetExist later

npm/pkg/dataplane/policies/chain-management_linux.go

JungukCho · 2021-11-18T18:58:30Z

npm/pkg/dataplane/policies/policymanager.go

 )

-const reconcileChainTimeInMinutes = 5
+const reconcileTimeInMinutes = 5


I think for better management (e.g., naming or etc) of codes, we may sync how windows dataplane supports common functions. Now some of them are TODO in policymanager_windows.go, but would be helpful to put comments whether these are supported or not.

@vakalapa it would be nice if you walk through them and fille up them when you have time since I think you are the most knowledgeable in windows dataplane.

When we understand what reconcile in both side, we may change the name to better understand if it makes sense.

Atm, there are no plans to introduce a reconcile loop in windows DP. We do not have periodic tasks like chain management in windows and the error signatures in windows are virtually unusable so we cannot build any reconcile failure loop on it.

huntergregory · 2021-11-18T20:03:47Z

Can you apply comments based on previous comments in the function (may need to slightly change for v2) when creating NewDataPlane - https://github.com/Azure/azure-container-networking/blob/master/npm/pkg/dataplane/dataplane.go#L70 ? I am not sure how windows works, but for linux it is good to comment them.

So I see we should be resetting policy manager first, then ipset manager (I'll update this). Was there anything else the comment refers to?

…hains

JungukCho

lgtm!

JungukCho · 2021-11-18T21:19:15Z

npm/pkg/dataplane/policies/chain-management_linux.go

-	azureChainGrepPattern     string = "Chain AZURE-NPM"
-	minAzureChainStringLength int    = len(azureChainGrepPattern)
-	azureChainStartIndex      int    = 6
+	minLineNumberStringLength int = 3 // TODO transferred from iptm.go and not sure why this length is important, but will update the function its used in later anyways


Nitpick: curious, is it idiomatic to put comment just next to code?
If not, if the comment is long, it would be nice to put above the line since I think it is more readable.

But we can update it since we need to resolve the question.

just habit. I'll put comments on top now, but ya this variable will be deleted next PR anyways

JungukCho · 2021-11-18T21:26:34Z

Can you apply comments based on previous comments in the function (may need to slightly change for v2) when creating NewDataPlane - https://github.com/Azure/azure-container-networking/blob/master/npm/pkg/dataplane/dataplane.go#L70 ? I am not sure how windows works, but for linux it is good to comment them.

So I see we should be resetting policy manager first, then ipset manager (I'll update this). Was there anything else the comment refers to?

There is dependency. We have to delete iptables first and then delete ipset. If not, kernel complains ipset is being used. So, deleting iptables is failed. So, the previous comments (1 and 2) explain this hard dependency when starting NPM (basically reinitialization). Good to have comments for dependency for maintenance purpose.

	// It is important to keep order to clean-up iptables and ipset.
	// IPtables should be cleaned first to avoid failures to clean-up iptables due to "ipset is using in kernel" error
	// 1. clean-up NPM-related iptables information and then running periodic processes to keep iptables correct
	if err := c.iptMgr.UninitNpmChains(); err != nil {
		utilruntime.HandleError(fmt.Errorf("Failed to UninitNpmChains with err: %w", err))
	}

	// 2. then clean-up all NPM ipsets states
	if err := c.ipsMgr.DestroyNpmIpsets(); err != nil {
		utilruntime.HandleError(fmt.Errorf("Failed to DestroyNpmIpsets with err: %w", err))
	}

huntergregory · 2021-11-19T00:40:24Z

/azp run

azure-pipelines · 2021-11-19T00:40:40Z

Azure Pipelines successfully started running 2 pipeline(s).

huntergregory · 2021-11-19T05:20:36Z

/azp run

azure-pipelines · 2021-11-19T05:20:51Z

Azure Pipelines successfully started running 2 pipeline(s).

wip

0b0ad83

huntergregory added the npm Related to NPM. label Nov 18, 2021

huntergregory requested review from JungukCho, matmerr and vakalapa November 18, 2021 03:17

JungukCho reviewed Nov 18, 2021

View reviewed changes

address comments and simplify logic a bit for getting current azure c…

8e966e3

…hains

JungukCho previously approved these changes Nov 18, 2021

View reviewed changes

fix order of resetting acls and ipsets

bd2932e

huntergregory dismissed JungukCho’s stale review via bd2932e November 18, 2021 21:59

JungukCho approved these changes Nov 18, 2021

View reviewed changes

huntergregory merged commit 5d2da73 into master Nov 19, 2021

rbtr deleted the place-azure-first branch November 30, 2021 20:15

fix: [NPM] Testing Changes and updates to Linux Policy Manager #1114

fix: [NPM] Testing Changes and updates to Linux Policy Manager #1114

Uh oh!

Conversation

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

Uh oh!

Notes on Changes

Uh oh!

huntergregory commented Nov 18, 2021

Uh oh!

azure-pipelines bot commented Nov 18, 2021

Uh oh!

JungukCho left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

JungukCho Nov 18, 2021

Choose a reason for hiding this comment

Uh oh!

JungukCho Nov 18, 2021

Choose a reason for hiding this comment

Uh oh!

huntergregory Nov 18, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JungukCho Nov 18, 2021

Choose a reason for hiding this comment

Uh oh!

vakalapa Nov 18, 2021

Choose a reason for hiding this comment

Uh oh!

huntergregory commented Nov 18, 2021

Uh oh!

JungukCho left a comment

Choose a reason for hiding this comment

Uh oh!

JungukCho Nov 18, 2021

Choose a reason for hiding this comment

Uh oh!

huntergregory Nov 18, 2021

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

huntergregory commented Nov 19, 2021

Uh oh!

azure-pipelines bot commented Nov 19, 2021

Uh oh!

huntergregory commented Nov 19, 2021

Uh oh!

azure-pipelines bot commented Nov 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

huntergregory commented Nov 18, 2021 •

edited

Loading

JungukCho commented Nov 18, 2021 •

edited

Loading