refactor: [NPM] General translation logic for egress and ingress #1106

JungukCho · 2021-11-12T21:53:43Z

Reason for Change:

This PR is to support egress and generalizes translation for egress and ingress.

Issue Fixed:

Requirements:

uses conventional commit messages
includes documentation
adds unit tests

Notes:

[NPM] General translation logic for linux and windows [NPM] General translation logic for linux and windows #1055
refactor: [NPM] parsing label selector for general translation logic refactor: [NPM] parsing label selector for general translation logic #1077
refactor: [NPM] General translation logic (mainly clean-up codes and correct bugs) refactor: [NPM] General translation logic (mainly clean-up codes and correct bugs) #1105

As @huntergregory summarized, next PR will deal with below lists.

switching to PeerList instead of SrcList and DstList like you suggested
deciding what to do for pod selector structs

huntergregory

Just curious, should some srclist become dstlist for egress?

I should have noticed this last PR, but can we consolidate some structs/fields? Right now, we have the NPMNetworkPolicy.PodSelectorList, which is idential to information in NPMNetworkPolicy.PodSelectorIPSets with an additional included bool for each set. We also have labelSelector struct in parseSelector.go which is identical and copied over to the two structures above. We can consolidate these three duplicate structures/fields into a into a new struct in the policy package to prevent duplication and reduce the amount of code (less complex and less bug-prone):

Proposal

Remove the labelSelector struct and NPMNetworkPolicy.PodSelectorList field, and make the type of NPMNetworkPolicy.PodSelectorIPSets as IncludedTranslatedIPSet

type IncludedTranslatedIPSet struct {
    Included bool
    ipsets  *[]TranslatedIPSet
}

To limit scope of this PR, we can leave the old NPMNetworkPolicy fields and add this as a new field. That way, current DP is unaffected, and we can follow up with a PR in DP.

huntergregory · 2021-11-13T00:44:00Z

npm/pkg/controlplane/translation/translatePolicy.go

+		acl := policies.NewACLPolicy(npmNetPol.NameSpace, npmNetPol.Name, policies.Allowed, direction)
+		ruleIPSets, setInfo := allowAllInternal(matchType)
+		npmNetPol.RuleIPSets = append(npmNetPol.RuleIPSets, ruleIPSets)
+		acl.SrcList = append(acl.SrcList, setInfo)


should this be dstlist for egress?

huntergregory · 2021-11-13T00:46:28Z

npm/pkg/controlplane/translation/translatePolicy.go

-					nsSelectorIPSets, nsSrcList := nameSpaceSelector(policies.SrcMatch, &flattenNSSelctor[i])
-					npmNetPol.RuleIPSets = append(npmNetPol.RuleIPSets, nsSelectorIPSets...)
-					peerAndPortRule(npmNetPol, rule.Ports, nsSrcList)
+	// #2. From fields exist in rule


nitpick: from or to field exists

huntergregory · 2021-11-13T01:16:14Z

npm/pkg/controlplane/translation/translatePolicy.go

+		// to handle multiple values in matchExpressions spec.
+		flattenNSSelctor := flattenNameSpaceSelector(peer.NamespaceSelector)
+		for i := range flattenNSSelctor {
+			nsSelectorIPSets, nsSrcList := nameSpaceSelector(matchType, &flattenNSSelctor[i])


nitpick: flattenNSSelector

huntergregory · 2021-11-13T01:19:19Z

npm/pkg/controlplane/translation/translatePolicy.go

+	for i, rule := range ingress {
+		translateRule(npmNetPol, policies.Ingress, policies.SrcMatch, i, rule.Ports, rule.From)
+	}
+	// Except for allow all traffic case in #1, the rest of them should not have default drop rules.


nitpick: should this say "should have default drop rule"? (same with comment in ingress == nil check)

same for egress function

Yes. Good catch!

huntergregory · 2021-11-13T01:19:39Z

npm/pkg/controlplane/translation/translatePolicy.go

+	}
+
+	// #2. If ingress is nil (in yaml file, it is specified with '[]'), it means "Deny all" - it does not allow receiving any traffic from others.
+	if ingress == nil {


we could remove this check since the for loop will do nothing if ingress is nil

similar for egress function

You're right. Initially, I did it and functionally there is no different, but to better understanding code and maintenance of code, I explicitly put two exceptional cases (e.g., allAllow and allDeny).
In this translation package, there are some codes which can be reduced, but just make different functions as well for better understanding (e.g., parsePodSelector or parseNSSelector, etc).

I am fine to either way (current one or merging one).
Please let me know if you think merging (only for loop) is better.
I can revise and put comment about it.

For understanding and maintenance, makes sense to keep. Having an extra if-check shouldn't hurt performance anyways

huntergregory · 2021-11-13T01:27:50Z

npm/pkg/dataplane/policies/policy.go

 const (
-	SrcMatch    MatchType = 0
-	DstMatch    MatchType = 1
+	SrcMatch MatchType = 0


I'd like to change this type to string for easier debugging from print statements. I could do that in my PR or you could

I am fine if you have ideas to better code, but do we have matchTypeStrings map just below this code?

https://github.com/Azure/azure-container-networking/blob/master/npm/pkg/dataplane/policies/policy.go#L191
Always glad if you can make better one. If so, please make different PR for them. I think it used in several place and easy to have one simple PR.

@huntergregory There is one question about DstDstMatch (https://github.com/Azure/azure-container-networking/blob/master/npm/pkg/dataplane/policies/policy.go#L188)

// TODO(jungukcho) Question - Why it is 3, not 2? DstDstMatch MatchType = 3

I had all the combinations before (SrcSrcMatch was 2), so it's leftover from that.

I agree, since there's a map already, this should be a separate PR change

If so, I will revise it to below code.

// Possible MatchTypes. const ( SrcMatch MatchType = 0 DstMatch MatchType = 1 // MatchTypes with 2 locations (e.g. DstDst) are for ip and port respectively. DstDstMatch MatchType = 2 // This is used for podSelector under spec. It can be Src or Dst based on existence of ingress or egress rule. EitherMatch MatchType = 3 )

huntergregory · 2021-11-13T01:38:36Z

npm/pkg/controlplane/translation/translatePolicy.go

-// (e.g., IPBlock, podSelector, namespaceSelector, or both podSelector and namespaceSelector)
-func peerAndPortRule(npmNetPol *policies.NPMNetworkPolicy, ports []networkingv1.NetworkPolicyPort, setInfo []policies.SetInfo) {
+// (e.g., IPBlock, podSelector, namespaceSelector, or both podSelector and namespaceSelector).
+func peerAndPortRule(npmNetPol *policies.NPMNetworkPolicy, direction policies.Direction, ports []networkingv1.NetworkPolicyPort, setInfo []policies.SetInfo) {


should this affect the acl dstlist for egress? if so should modify the variable names used as args to calls to this func to be something like srcOrDstList

You confirmed ports are always for the destination for either ingress/egress, right?

Good catch.
I will walk through dstlist and srclist.

Yes. Confirmed that ports are always for destination.

Revised them now with a method.

However, it seems we can do better job with only one PeerList instead of having two SrcList and DstList.
I put the comment about it, but want to do it next PR even though it makes sense since it needs to change in multiple places. wdyt?

JungukCho · 2021-11-15T17:43:40Z

Just curious, should some srclist become dstlist for egress?

I will walk through srcList and dstList again.

I should have noticed this last PR, but can we consolidate some structs/fields? Right now, we have the NPMNetworkPolicy.PodSelectorList, which is idential to information in NPMNetworkPolicy.PodSelectorIPSets with an additional included bool for each set. We also have labelSelector struct in parseSelector.go which is identical and copied over to the two structures above. We can consolidate these three duplicate structures/fields into a into a new struct in the policy package to prevent duplication and reduce the amount of code (less complex and less bug-prone):

Proposal

Remove the labelSelector struct and NPMNetworkPolicy.PodSelectorList field, and make the type of NPMNetworkPolicy.PodSelectorIPSets as IncludedTranslatedIPSet
type IncludedTranslatedIPSet struct {
    Included bool
    ipsets  *[]TranslatedIPSet
}

Yes. Good suggestion.
I think you mentioned this in previous PR, I put it in some notes, but missed it in this PR.
Let me walk through this issue again and get back to you.

To limit scope of this PR, we can leave the old NPMNetworkPolicy fields and add this as a new field. That way, current DP is unaffected, and we can follow up with a PR in DP.
As you said, it would be good to a single PR to add and remove related codes if we decide to revise the struct. wdyt?

huntergregory · 2021-11-15T18:38:34Z

To limit scope of this PR, we can leave the old NPMNetworkPolicy fields and add this as a new field. That way, current DP is unaffected, and we can follow up with a PR in DP.

As you said, it would be good to a single PR to add and remove related codes if we decide to revise the struct. wdyt?

If we change later, we can get to integration testing quicker, but then we might notice bugs and have to make fixes based on structs that we won't use later. I don't have a strong opinion.

JungukCho · 2021-11-15T18:55:22Z

To limit scope of this PR, we can leave the old NPMNetworkPolicy fields and add this as a new field. That way, current DP is unaffected, and we can follow up with a PR in DP.

As you said, it would be good to a single PR to add and remove related codes if we decide to revise the struct. wdyt?

If we change later, we can get to integration testing quicker, but then we might notice bugs and have to make fixes based on structs that we won't use later. I don't have a strong opinion.

Anyway if we change some codes, we need to do integration test again even though it is non-functional change (and we don't waste time your point - but then we might notice bugs and have to make fixes based on structs that we won't use later.). So, let's complete some necessary code change and PRs first and then plan integration test. We don't need to rush.

…lues

JungukCho · 2021-11-15T21:26:25Z

To limit scope of this PR, we can leave the old NPMNetworkPolicy fields and add this as a new field. That way, current DP is unaffected, and we can follow up with a PR in DP.

As you said, it would be good to a single PR to add and remove related codes if we decide to revise the struct. wdyt?

If we change later, we can get to integration testing quicker, but then we might notice bugs and have to make fixes based on structs that we won't use later. I don't have a strong opinion.

Anyway if we change some codes, we need to do integration test again even though it is non-functional change (and we don't waste time your point - but then we might notice bugs and have to make fixes based on structs that we won't use later.). So, let's complete some necessary code change and PRs first and then plan integration test. We don't need to rush.

Just curious, should some srclist become dstlist for egress?

I will walk through srcList and dstList again.
I should have noticed this last PR, but can we consolidate some structs/fields? Right now, we have the NPMNetworkPolicy.PodSelectorList, which is idential to information in NPMNetworkPolicy.PodSelectorIPSets with an additional included bool for each set. We also have labelSelector struct in parseSelector.go which is identical and copied over to the two structures above. We can consolidate these three duplicate structures/fields into a into a new struct in the policy package to prevent duplication and reduce the amount of code (less complex and less bug-prone):

Proposal

Remove the labelSelector struct and NPMNetworkPolicy.PodSelectorList field, and make the type of NPMNetworkPolicy.PodSelectorIPSets as IncludedTranslatedIPSet
type IncludedTranslatedIPSet struct {
    Included bool
    ipsets  *[]TranslatedIPSet
}
Yes. Good suggestion. I think you mentioned this in previous PR, I put it in some notes, but missed it in this PR. Let me walk through this issue again and get back to you.

To limit scope of this PR, we can leave the old NPMNetworkPolicy fields and add this as a new field. That way, current DP is unaffected, and we can follow up with a PR in DP.
As you said, it would be good to a single PR to add and remove related codes if we decide to revise the struct. wdyt?

I walked through the code (including linux dataplane) to see how it uses data structures and think I prefer to having two members (i.e., PodSelectorIPSets and PodSelectorList) since two data structures have different data when there is multiple values in podSelector (Check this UT -

azure-container-networking/npm/pkg/controlplane/translation/translatePolicy_test.go

Line 618 in 306b665

podSelectorIPSets: []*ipsets.TranslatedIPSet{

) and seems it is not easy to induce them in data plane side.

labelSelector is a local data structure used in only translation logic to create IPSet and List based on namespaceSelector and podSelector.

So, I am fine to a current code, but if you have another opinion, please let me know.
We can revise it if needed.

huntergregory

Looks good to me. We have some cleanup work to do later in Translation & Dataplane:

switching to PeerList instead of SrcList and DstList like you suggested
deciding what to do for pod selector structs

vakalapa · 2021-11-15T22:45:55Z

npm/pkg/controlplane/translation/translatePolicy.go

+	ports []networkingv1.NetworkPolicyPort, peers []networkingv1.NetworkPolicyPeer) {
+	// TODO(jungukcho): need to clean up it.
+	// Leave allowExternal variable now while the condition is checked before calling this function.
+	allowExternal, portRuleExists, fromRuleExists := ruleExists(ports, peers)


nit: fromRuleExists to peerRuleExists

JungukCho · 2021-11-15T23:05:36Z

Looks good to me. We have some cleanup work to do later in Translation & Dataplane:

switching to PeerList instead of SrcList and DstList like you suggested

deciding what to do for pod selector structs

Thank you for summary. Will add them in a note in this PR as well and revisit them in a next PR.

JungukCho added 5 commits November 12, 2021 13:50

Generallize function for ingress and egress

1f15dc5

Use function to support both ingress and egress and update UTs

406d650

Simplify UTs and correct comments

74dbd23

Add more comments and correct policies.MatchType

c40deed

Reorganize codes to better understand and simplify UTs

ff82f91

JungukCho requested review from huntergregory, matmerr, neaggarwMS and vakalapa November 12, 2021 21:53

JungukCho added the npm Related to NPM. label Nov 12, 2021

huntergregory suggested changes Nov 13, 2021

View reviewed changes

Addresses comments (e.g., fix typo and correct wrong comments)

9a828c9

Address comments (e.g., srcList and dstList) and correct MatchType va…

306b665

…lues

huntergregory previously approved these changes Nov 15, 2021

View reviewed changes

vakalapa reviewed Nov 15, 2021

View reviewed changes

Address comments (fromRulesExists -> peerRuleExists)

174a14e

JungukCho dismissed huntergregory’s stale review via 174a14e November 15, 2021 23:04

huntergregory approved these changes Nov 16, 2021

View reviewed changes

JungukCho merged commit d7dcc64 into Azure:master Nov 16, 2021

JungukCho mentioned this pull request Nov 18, 2021

refacto: [NPM] Handle 0.0.0.0/0 ipblock and de-duplicated elements in except field #1109

Merged

3 tasks

refactor: [NPM] General translation logic for egress and ingress #1106

refactor: [NPM] General translation logic for egress and ingress #1106

Uh oh!

Conversation

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

Uh oh!

Uh oh!

huntergregory left a comment

Choose a reason for hiding this comment

Proposal

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!

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!

JungukCho commented Nov 15, 2021

Proposal

Uh oh!

huntergregory commented Nov 15, 2021

Uh oh!

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

Uh oh!

Uh oh!

JungukCho commented Nov 15, 2021

Proposal

Uh oh!

huntergregory left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JungukCho commented Nov 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

JungukCho commented Nov 12, 2021 •

edited

Loading

JungukCho commented Nov 15, 2021 •

edited

Loading