[NPM] Generic Dataplane interface for windows and linux #984

vakalapa · 2021-08-30T18:56:47Z

Reason for Change:

This PR helps outline the DataPlane interface which acts as an abstraction layer for similar functionalities in Windows and Linux.

a. Addition of new dataplane APIs for control plane to use
b. specific build target functions to convert ipsets and acl policies accoridngly

Issue Fixed:

Requirements:

uses conventional commit messages
includes documentation
adds unit tests

Notes:

npm/pkg/dataplane/dataplane.go

vakalapa · 2021-09-01T19:44:00Z

/azp run

azure-pipelines · 2021-09-01T19:44:15Z

Azure Pipelines successfully started running 2 pipeline(s).

npm/pkg/dataplane/ipsets/ipset.go

huntergregory · 2021-09-01T22:32:15Z

npm/pkg/dataplane/ipsets/ipset.go

+	set.IpsetReferCount--
+}
+
+func (set *IPSet) AddSelectorReference(netPolName string) {


different arg name (below func too)?

essentially what we are adding is the network policy name if its a selector ref or a normal ref.

npm/pkg/dataplane/ipsets/ipset.go

npm/pkg/dataplane/dataplane.go

matmerr

some comments

npm/pkg/dataplane/dataplane.go

npm/pkg/dataplane/policies/policymanager.go

npm/pkg/dataplane/dataplane_windows.go

npm/pkg/dataplane/dataplane.go

matmerr · 2021-09-02T00:00:57Z

npm/pkg/dataplane/ipsets/ipset.go

+
+const (
+	ListSet SetKind = "list"
+	HashSet SetKind = "set"


tbh I think there's room for what defines a set here, taking linux vernacular could be misleading in windows, but I think in terms of platform agnostic, what about something like hashset is "set" and list is "superset" since it contains only "sets"(?)

I like this "superset" since it is intuitive. But keeping list since we have been used to calling it as list. We can use "nestedset" which windows is using ?

npm/pkg/dataplane/ipsets/ipset.go

npm/util/util.go

matmerr · 2021-09-07T20:20:04Z

npm/pkg/dataplane/ipsets/ipset.go

+	}
+}
+
+func getSetKind(setType SetType) SetKind {


is this still needed with properties?

Yes we derive the kind from type of set input.

JungukCho

Before I put other comments, there are structural (i.e., architectural) comments of this PR.
I may misunderstand the codes and intentions, but some of them are different from what I expected.

I thought interface will be used for ipsetmanager and dataplane to abstract linux and windows. But it seems you use composition with concrete struct instead of interface. I think both ipsetmanager and dataplane are nicely abstract with interface (Example in npm - https://github.com/Azure/azure-container-networking/blob/master/npm/iptm/ioshim.go).
Do I know why you decide this direction?
There are many almost inline method (just one or two lines) for exposed members. In this case, if you want to keep methods, I think non-capitalizing members if possible since it is good to maintain code as our code base becomes bigger and bigger. In addition, some of them can be removed with interface direction instead of concrete struct.
I may be wrong, but I think function or method having one line is not very useful. wdyt?
It would be nice to have UTs for ipsetmanager_windows.go and dataplane_windows.go in this PR to guarantee their functionality, but as you said, it will be next PR. So, I think it is ok in this PR.
While I read this PR, there are many common variables and const with debugTools. I think it is good to remove redundant declarations by checking debugTools code carefully. One examaple, is ipset.go and rule.pb.go generated from rule.proto.

vakalapa · 2021-09-08T19:38:42Z

@JungukCho thanks foe the comments

I felt there is not much abstraction to be made with interfaces. As part of our design exercise we did find that most of the core logic is same for both dataplanes, only the dataplane specific artifact conversions and application is different. So i took the https://golang.org/src/os/executable.go approach. The main executable package contains a publicly exposed function Executable() and each dataplane specific file will implement the executable() like https://golang.org/src/os/executable_windows.go. I felt this is much easier to read as the flow of code is pretty straight forward. Please let me know if you think there is any issue with this approach
Sure, most of the dataplane.go function may stay inline like above executable approach but some will get filled up with abstractions needed for both dataplanes. Also a single dataplane call will make our lifes easier in terms extending dataplane with a gprc call in between later on for operator model. Like you mentioned i will try to see if i can non-capitalize any members.
Agreed, tests are important and will definitely be one focus area when adding windows dataplane support.
I did not want to increase the scope of this PR. We do have a debug tools work item in pipeline and as part of that work item and also as part of dataplane integration we should be able to clean pkg/dataplane all together. Will be a incremental process. Idea is to not touch any existing code as much possible.

npm/pkg/dataplane/dataplane.go

npm/pkg/dataplane/dataplane_linux.go

npm/pkg/dataplane/ipsets/ipset.go

npm/pkg/dataplane/policies/policy_windows.go

npm/pkg/dataplane/policies/policy.go

JungukCho · 2021-09-08T20:01:09Z

npm/pkg/dataplane/policies/policy_windows.go

+		return hcn.DirectionTypeIn
+	case Egress:
+		return hcn.DirectionTypeOut
+	case Both:


seems this case is not necessary.

Windows dataplane does accept rules on both directions, in case of network policies with both ingress and egress rules i can use a DENY all with BOTH, that way we get away with single rule than 2

I meant case Both is not necessary based on logic. Anyway, by default it returns ""

JungukCho · 2021-09-08T21:28:30Z

@JungukCho thanks foe the comments

I felt there is not much abstraction to be made with interfaces. As part of our design exercise we did find that most of the core logic is same for both dataplanes, only the dataplane specific artifact conversions and application is different. So i took the https://golang.org/src/os/executable.go approach. The main executable package contains a publicly exposed function Executable() and each dataplane specific file will implement the executable() like https://golang.org/src/os/executable_windows.go. I felt this is much easier to read as the flow of code is pretty straight forward.
Please let me know if you think there is any issue with this approach

If they are almost identical, this approach seems reasonable. How about Dataplane? I think they are pretty different implementations. I will first take a look at your reference. I may find better patterns instead of interface. I will let you know if I still prefer to interface and we can discuss more.

Sure, most of the dataplane.go function may stay inline like above executable approach but some will get filled up with abstractions needed for both dataplanes. Also a single dataplane call will make our lifes easier in terms extending dataplane with a gprc call in between later on for operator model. Like you mentioned i will try to see if i can non-capitalize any members.

Still I think those inline functions will be nicely removed with interface. Further more dataplane-specific functions will be added in next PRs. So, it will increase more these inline functions. I will check your above reference and let you know.
I think in this stage thinking a grpc call in design is not very helpful since it is too future. wdyt?

Agreed, tests are important and will definitely be one focus area when adding windows dataplane support.

I did not want to increase the scope of this PR. We do have a debug tools work item in pipeline and as part of that work item and also as part of dataplane integration we should be able to clean pkg/dataplane all together. Will be a incremental process. Idea is to not touch any existing code as much possible.

I am not sure it increases scope of the PR. I meant checking the existing variables and then reusing them to avoid redundant codes in this PR without touching existing logics.

npm/pkg/dataplane/dataplane.go

JungukCho

LGTM. Thank you for applying comments.
This PR will be foundation of next NPM!

matmerr

🚀

matmerr

🚀

JungukCho

lgtm

vakalapa added 2 commits August 27, 2021 14:49

adding initial dataplane interface

7a4dbf8

adding some skeleton code for crud functions

4385d46

vakalapa added npm Related to NPM. enhancement work-in-progress labels Aug 30, 2021

vakalapa marked this pull request as draft August 30, 2021 18:58

vakalapa added 2 commits August 30, 2021 14:30

adding dirty cache initial support for ipset manager

013aa67

correcting some go lints

3e32783

huntergregory reviewed Aug 30, 2021

View reviewed changes

npm/pkg/dataplane/dataplane.go Outdated Show resolved Hide resolved

vakalapa and others added 2 commits August 30, 2021 16:24

Adding some skeleton around os specific files

a907ea2

removing interface and adding some network logic

58d7782

vakalapa marked this pull request as ready for review August 31, 2021 21:03

vakalapa removed the work-in-progress label Aug 31, 2021

vakalapa requested review from JungukCho and matmerr September 1, 2021 16:59

adding apply ipsets logic

5c5403e

vakalapa requested a review from neaggarwMS September 1, 2021 20:18

huntergregory reviewed Sep 1, 2021

View reviewed changes

matmerr reviewed Sep 2, 2021

View reviewed changes

npm/pkg/dataplane/ipsets/ipset.go Show resolved Hide resolved

Vamsi Kalapala added 2 commits September 1, 2021 22:14

Addressing some comments and also adding comments to code

6c5afdc

Fixing some golints

cf8e556

huntergregory reviewed Sep 2, 2021

View reviewed changes

npm/pkg/dataplane/ipsets/ipset.go Show resolved Hide resolved

Vamsi Kalapala added 3 commits September 2, 2021 11:34

addressing some comments

bfff6a5

adding some golint checks

4fe2916

Adding a new field setProperty and also adding structure for policies

9441d08

matmerr reviewed Sep 7, 2021

View reviewed changes

applying some comments

14ef65f

Vamsi Kalapala added 2 commits September 7, 2021 14:56

Adding some comments

57d78af

Adding some test cases

f702d88

JungukCho reviewed Sep 8, 2021

View reviewed changes

huntergregory reviewed Sep 8, 2021

View reviewed changes

npm/pkg/dataplane/dataplane.go Outdated Show resolved Hide resolved

Vamsi Kalapala added 7 commits September 8, 2021 16:09

Addressing some comments

a9dfbc3

Addressing more comments

a5cbd0b

resolving some comments

c921c1d

resolving some comments

af60247

fixing some comments

987f820

removing lock on policymap

ae527f4

fixing some golints

d0eb0c6

JungukCho previously approved these changes Sep 10, 2021

View reviewed changes

matmerr previously approved these changes Sep 10, 2021

View reviewed changes

Vamsi Kalapala added 3 commits September 10, 2021 15:45

Merge branch 'master' into vakr/dataplaneinterface

15cae9a

Merge branch 'master' into vakr/dataplaneinterface

7c200c5

merging with master

607ba43

vakalapa dismissed stale reviews from matmerr and JungukCho via 607ba43 September 10, 2021 23:06

huntergregory previously approved these changes Sep 10, 2021

View reviewed changes

fixingsome wrap checks

942f894

vakalapa dismissed huntergregory’s stale review via 942f894 September 10, 2021 23:26

fixing lints

50315e6

matmerr approved these changes Sep 10, 2021

View reviewed changes

huntergregory approved these changes Sep 11, 2021

View reviewed changes

JungukCho approved these changes Sep 11, 2021

View reviewed changes

vakalapa merged commit 01c12d8 into master Sep 13, 2021

vakalapa deleted the vakr/dataplaneinterface branch September 13, 2021 02:43

[NPM] Generic Dataplane interface for windows and linux #984

[NPM] Generic Dataplane interface for windows and linux #984

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Uh oh!

vakalapa commented Sep 1, 2021

Uh oh!

azure-pipelines bot commented Sep 1, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matmerr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JungukCho left a comment

Choose a reason for hiding this comment

Uh oh!

vakalapa commented Sep 8, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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 Sep 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

JungukCho left a comment

Choose a reason for hiding this comment

Uh oh!

matmerr left a comment

Choose a reason for hiding this comment

Uh oh!

matmerr left a comment

Choose a reason for hiding this comment

Uh oh!

JungukCho left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

vakalapa commented Aug 30, 2021 •

edited

Loading

JungukCho commented Sep 8, 2021 •

edited

Loading