[NPM] RETURN early on MARK in DROP chains #881

vakalapa · 2021-05-26T18:28:52Z

If there are multiple policies applied on a cluster with TO rules in one and PORT rules in another selecting the same namespace and pod selector, then some TO traffic is being dropped. The reason for that is in PORT chains we are adding TO/FROM chain rules and DROP rules, event though TO/FORM chain MARKs the packet and returns it, since there are no RETURNs in PORT after the TO/FROM is called, we are sending the packet to DROP chains and dropping the traffic.

This fix adds a RETURN rule at the top of both DROP chains to return, in future we should check if we can reduce the TO/FROM rules and DROP rules in PORT chain

Along with that, with this PR, we will now be only adding a single JUMP to either TO/FROM chain in PORT chain for all traffic. We will not be adding any DROP rules in PORT or TO/FROM chains ! This will reduce the number jump rules drastically, keeps a simple logic on where the MARK rules are hit and where the DROP rules are hit.

JungukCho

Can you please add a detailed explanation to describe what was an issue and how "early on Mark" resolves the issue with one example yaml? Since I only understood what code does (e.g., control order of rules (e.g., append or insert) based on type of chain), I cannot quite map what it solved. It will help me to understand this PR. Since this PR is linked with #870 , it would be nice to use the example. Will be very appreciate to show iptables changes as well.

Also are there valid unit test for this?

vakalapa · 2021-06-01T16:11:03Z

/azp run

azure-pipelines · 2021-06-01T16:11:18Z

Azure Pipelines successfully started running 2 pipeline(s).

JungukCho

I would like to point out exact codes, but not sure where they are.
However, after playing with this PR, in high-level, this PR removes "AZURE-NPM-EGRESS(or INGRESS)-DROPS" chains from "AZURE-NPM-EGRESS (or INGRESS)-PORT" chain to resolve packet drop issues.

One thing that I am not sure is to put "RETURN" in each "AZURE-NPM-EGRESS (or INGRESS)-DROPS" chains is necessary.
IPtables output from example yaml which enables reproducing packet drop issues. Italic parts seems redundant.

Or are there something which I missed?

Chain AZURE-NPM-EGRESS (1 references)
pkts bytes target prot opt in out source destination
0 0 AZURE-NPM-EGRESS-PORT all -- * * 0.0.0.0/0 0.0.0.0/0
0 0 RETURN all -- * * 0.0.0.0/0 0.0.0.0/0 mark match 0x3000 / RETURN-on-EGRESS-and-INGRESS-mark-0x3000 /
0 0 RETURN all -- * * 0.0.0.0/0 0.0.0.0/0 mark match 0x1000 / RETURN-on-EGRESS-mark-0x1000 */*
0 0 AZURE-NPM-EGRESS-DROPS all -- * * 0.0.0.0/0 0.0.0.0/0

Chain AZURE-NPM-EGRESS-DROPS (1 references)
pkts bytes target prot opt in out source destination
0 0 RETURN all -- * * 0.0.0.0/0 0.0.0.0/0 mark match 0x3000 / _RETURN-on-EGRESS-and-INGRESS-mark-0x3000 /
0 0 RETURN all -- * * 0.0.0.0/0 0.0.0.0/0 mark match 0x1000 / RETURN-on-EGRESS-mark-0x1000 /
0 0 DROP all -- * * 0.0.0.0/0 0.0.0.0/0 match-set azure-npm-784554818 src /* DROP-ALL-FROM-ns-default /_
0 0 DROP all -- * * 0.0.0.0/0 0.0.0.0/0 match-set azure-npm-784554818 src / DROP-ALL-FROM-ns-default */

JungukCho · 2021-06-02T22:49:39Z

npm/iptm/iptm.go

 }

+func isDropsChain(chainName string) bool {
+	for _, chain := range IptablesAzureDropsChainList {


Prefer to using "if condition" instead of "for loop with IptableAzureDropsChainList" since there are only two elements.

ok let me fix it

neaggarwMS · 2021-06-02T23:20:58Z

npm/iptm/helper.go

 		getAzureNPMAcceptChainRules,
 		getAzureNPMIngressChainRules,
 		getAzureNPMIngressPortChainRules,
-		getAzureNPMIngressFromChainRules,


We still need these chains no?

yeah we will need the chains, this is just removing the default RETURN rules added in these chains

Shall we change the function name then? GetAllChainsAndRules is misleading

vakalapa · 2021-06-02T23:27:18Z

I would like to point out exact codes, but not sure where they are.
However, after playing with this PR, in high-level, this PR removes "AZURE-NPM-EGRESS(or INGRESS)-DROPS" chains from "AZURE-NPM-EGRESS (or INGRESS)-PORT" chain to resolve packet drop issues.

One thing that I am not sure is to put "RETURN" in each "AZURE-NPM-EGRESS (or INGRESS)-DROPS" chains is necessary.
IPtables output from example yaml which enables reproducing packet drop issues. Italic parts seems redundant.

The redundant RETURN rules in DROP chains are a form contingency for future chain ordering when we call DROP chains directly unlike today. These rules ensure any future changes will insure us from dropping relevant MARK'ed packets (whcih has caused this issue in the first place)

JungukCho · 2021-06-03T00:05:22Z

npm/iptm/iptm.go

 	}

+	// (TODO) need a review of all chains to make sure we are not adding duplicate rules.
+	// NPM should not add multiple TO/FROM rules along with DROPs in PORT chains,


Still need these comments? Is it resolved in this PR?

Yes we will not need this comment as we are taking care of these duplicate rules in this PR.

neaggarwMS · 2021-06-03T16:50:32Z

// getAzureNPMEgressPortChainRules returns rules for AZURE-NPM-INGRESS-PORT

nit: typo

Refers to: npm/iptm/helper.go:244 in c41f87a. [](commit_id = c41f87a, deletion_comment = False)

JungukCho

One concern in this PR is I am not 100% sure that whether it does allow invalid packets or not allow valid packets when various multiple network policies are applied in AZURE-NPM-INGRESS(or EGRESS)-DROPS chain while I think this PR does not change those behavior from previous code base and getDefaultDropEntries() guarantees that one.

Do you have any thoughts?

JungukCho · 2021-06-09T16:01:59Z

npm/iptm/iptm.go

-	if entry.IsJumpEntry {
+	// Since there is a RETURN statement added to each DROP chain, we need to make sure
+	// any new DROP rule added to ingress or egress DROPS chain is added at the BOTTOM
+	if entry.IsJumpEntry || isDropsChain(entry.Chain) {


NPM does not need entry.IsJumpEntry anymore with this PR.
So, please confirm in NPM code. If we confirm this, we can remove this if condition.
Also, review and update translatePolicy_test.go with this change accordingly. If other codes also impacted, please check their UTs as well.

As I put comment about this before, RETURNs in "AZURE-NPM-EGRESS (or INGRESS)-DROPS" chains are redundant.

If we confirm this, it seems we only need below block in iptm.go with this PR.

iptMgr.OperationFlag = util.IptablesInsertionFlag.

Let me know if I miss something.

You are right, we do not need to append when the jumpentry is true, since we are now removing all jump entries and are relying on the default rules to help with that jump transitions to next chains.

As far as the redundant RETURN rule in DROP chains are concerned, i feel strongly to keep those rules in place as a contingency for any later changes. With these redundant rules, we are ensuring that no matter what we will NEVER DROP a MARK'ed packet and this assurance helps are from hitting any other bugs in the future.

One concern in this PR is I am not 100% sure that whether it does allow invalid packets or not allow valid packets when various multiple network policies are applied in AZURE-NPM-INGRESS(or EGRESS)-DROPS chain while I think this PR does not change those behavior from previous code base and getDefaultDropEntries() guarantees that one.

Do you have any thoughts?

I think overall this is a better design because, keeping the translation logic aside, while evaluating rule we are making sure that a MARKED packet will never be dropped and a unmarked packet will be dropped on certain conditions. This simplifies the logic from before where we have multiple avenue to DROPs chain before we evaluated all MARK rules in a given direction (which was wrong).

JungukCho · 2021-06-14T22:56:21Z

Thank you for applying comments!

One more suggestion is to remove "IsJumpEntry" member from "IptEntry" struct (https://github.com/Azure/azure-container-networking/blob/master/npm/iptm/iptm.go#L50) with this PR since the "IsJumpEntry" member is not needed anymore.
Are there reasons to keep it which I missed?

Except for above suggestion, it looks good to me.

JungukCho

LGTM.

I think this PR has functional improvement, but I assume it also has data plane latency improvement. So, while it is not urgent now, it would be good to have some measurements to see data plane improvements with this PR later.

vakalapa · 2021-06-14T23:47:58Z

/azp run

azure-pipelines · 2021-06-14T23:48:13Z

Azure Pipelines successfully started running 2 pipeline(s).

vakalapa · 2021-06-15T16:59:29Z

/azp run

azure-pipelines · 2021-06-15T16:59:45Z

Azure Pipelines successfully started running 2 pipeline(s).

Returning in DROP chains

757478d

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

[NPM] TO/FROM rule and port rule on same PodSelector in multiple policies is having issues. #870

Closed

vakalapa added npm Related to NPM. bug labels May 26, 2021

vakalapa requested review from JungukCho, matmerr and neaggarwMS May 26, 2021 18:49

adding a comment about future cleanup of chains:

e47ed50

JungukCho reviewed May 26, 2021

View reviewed changes

Removing duplicate rules in PORT chains of TO/FROM and DROP chain jumps

eaccc18

Adding a image of new chains behavior

ab5fb9c

JungukCho reviewed Jun 2, 2021

View reviewed changes

neaggarwMS reviewed Jun 2, 2021

View reviewed changes

Addressing comments

c41f87a

JungukCho reviewed Jun 3, 2021

View reviewed changes

JungukCho reviewed Jun 9, 2021

View reviewed changes

vakalapa added 2 commits June 11, 2021 16:34

addressing some comments

f2d97e2

Correcting some UTs to not have the jump rules

482304b

removing jump flag

bdae159

JungukCho approved these changes Jun 14, 2021

View reviewed changes

vakalapa merged commit c2b2db1 into master Jun 15, 2021

vakalapa deleted the vakr/fixtofromruledrop branch June 15, 2021 17:06

[NPM] RETURN early on MARK in DROP chains #881

[NPM] RETURN early on MARK in DROP chains #881

Uh oh!

Conversation

vakalapa commented May 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JungukCho left a comment

Choose a reason for hiding this comment

Uh oh!

vakalapa commented Jun 1, 2021

Uh oh!

azure-pipelines bot commented Jun 1, 2021

Uh oh!

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

vakalapa commented Jun 2, 2021 • 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!

neaggarwMS commented Jun 3, 2021

Uh oh!

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

JungukCho commented Jun 14, 2021

Uh oh!

JungukCho left a comment

Choose a reason for hiding this comment

Uh oh!

vakalapa commented Jun 14, 2021

Uh oh!

azure-pipelines bot commented Jun 14, 2021

Uh oh!

vakalapa commented Jun 15, 2021

Uh oh!

azure-pipelines bot commented Jun 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vakalapa commented May 26, 2021 •

edited

Loading

vakalapa commented Jun 2, 2021 •

edited

Loading