fix: [NPM] adding fixes to windows DP after first run #1100

vakalapa · 2021-11-12T00:37:31Z

Reason for Change:

Splitting SetPolicy calls into HNS into 2, one for Nested and one for 1st level sets.
Adding state to FakeHNS to help emulate HNS behavior
Adding IPSet Manager Windows UTs
Adding Policy Manager Windows UTs.

Issue Fixed:

Requirements:

uses conventional commit messages
includes documentation
adds unit tests

Notes:

…er for nested sets

npm/pkg/dataplane/ipsets/ipsetmanager_windows_test.go

npm/pkg/dataplane/ipsets/ipsetmanager_test.go

npm/pkg/dataplane/ipsets/testutils.go

huntergregory · 2021-11-18T23:10:35Z

npm/pkg/dataplane/ipsets/ipsetmanager_test.go

 	assert.Equal(t, util.GetHashedName(setMetadata.GetPrefixName()), set.HashedName)
+
+	err := iMgr.applyIPSets()
+	require.NoError(t, err)


Eventually change each test to have this:

toAddOrUpdateSetNames := []string{} assertEqualContentsTestHelper(t, toAddOrUpdateSetNames, iMgr.toAddOrUpdateCache) toDeleteSetNames := []string{} assertEqualContentsTestHelper(t, toAddOrUpdateSetNames, iMgr.toAddOrUpdateCache) calls := GetApplyIPSetTestCalls(toAdd, toDelete) ioshim := common.NewMockIOShim(calls) defer ioshim.VerifyCalls(t, calls) iMgr := NewIPSetManager(ioshim) ... iMgr.ApplyIPSets()

for another PR

huntergregory · 2021-11-18T23:18:39Z

npm/pkg/dataplane/policies/policymanager.go

 	return nil
 }

+type PolicyManagerCfg struct {


note to add this in Constructor for pMgr in another PR (will touch code in dataplane.go, dataplane_test.go though)

matmerr · 2021-11-19T20:10:25Z

test/integration/npm/main.go

 }

-func printAndWait() {
+func printAndWait(wait bool) {


if wait is false, function name could be simplified

matmerr · 2021-11-19T20:16:33Z

common/ioshim_windows.go

+	}
+}
+
+func NewMockIOShimWithFakeHNS(hns *hnswrapper.Hnsv2wrapperFake) *IOShim {


should probably omit these fakes from coverage metrics

JungukCho

You can resolve it in your next PR if they make sense.

JungukCho · 2021-11-19T21:55:06Z

network/hnswrapper/hnsv2wrapperfake.go


+const networkName = "azure"
+
+var errorFakeHNS = errors.New("errorFakeHNS Error")


nitpick: seems Error is unnecessary for newErrorFakeHNS since errorFakeHNS has error context already, but it is ok as is.

JungukCho · 2021-11-19T21:55:39Z

network/hnswrapper/hnsv2wrapperfake.go

+
 type Hnsv2wrapperFake struct {
+	Cache FakeHNSCache
+	*sync.Mutex


curious: special reason for pointer for sync.Mutex?

Lint was not happy that i was passing a value and not reference.

Interesting.. good to know why.

I haven't looked at the usage, but probably because you should never copy a sync.Mutex. In a fake this might be okay, but I would really discourage references to mutexes because any copy (like if you pass the struct by val in to parallel goroutines) of the object can lock all of them

JungukCho · 2021-11-19T22:01:46Z

network/hnswrapper/hnsv2wrapperfake.go

+		if err != nil {
+			return newErrorFakeHNS(err.Error())
+		}
+		for _, setPolSetting := range setPolSettings.Policies {


nitpick: seem better HNSNetPol instead of setPolSetting

JungukCho · 2021-11-19T22:05:47Z

network/hnswrapper/hnsv2wrapperfake.go

+		if err != nil {
+			return newErrorFakeHNS(err.Error())
+		}
+		for _, newPolicy := range setPolSettings.Policies {


May be better to use consistent variables for newPolicy if there is no special contexts. Line 69 in this file uses a different name.

JungukCho · 2021-11-19T22:10:57Z

network/hnswrapper/hnsv2wrapperfake.go

+				return newErrorFakeHNS(err.Error())
+			}
+			if _, ok := networkCache.Policies[setpol.Id]; !ok {
+				return newErrorFakeHNS(fmt.Sprintf("[FakeHNS] could not find %s ipset", setpol.Name))


nitpick: not sure sometime it has [FakeHNS] or not.
It looks overlapped with

var errorFakeHNS = errors.New("errorFakeHNS Error")

JungukCho · 2021-11-19T22:15:57Z

network/hnswrapper/hnsv2wrapperfake.go

+func (fCache FakeHNSCache) ACLPolicies(epList map[string]string, policyID string) (map[string][]*FakeEndpointPolicy, error) {
+	aclPols := make(map[string][]*FakeEndpointPolicy)
+	for ip, epID := range epList {
+		epCache, ok := fCache.endpoints[epID]


nitpick: epCache -> cachedEP or just ep since I checked whether fCache.endpoints[epID] return another map ?

JungukCho · 2021-11-19T22:34:17Z

network/hnswrapper/hnsv2wrapperfake.go

+func (fEndpoint *FakeHostComputeEndpoint) RemovePolicy(toRemovePol *FakeEndpointPolicy) error {
+	for i, policy := range fEndpoint.Policies {
+		if reflect.DeepEqual(policy, toRemovePol) {
+			fEndpoint.Policies = append(fEndpoint.Policies[:i], fEndpoint.Policies[i+1:]...)


nitpick: may do swap if ordering is not issue.
For example,

if i != len(fEndpoint.Policies) { // replace it with last one fEndpoint.Policies[i] = fEndpoint.Policies[len(fEndpoint.Policies)-1] } // remove last element. fEndpoint.Policies = fEndpoint.Policies[:i-1] return nil }

JungukCho · 2021-11-19T22:37:42Z

npm/pkg/dataplane/policies/policy_windows.go

 )

+const (
+	blockRulePriotity = 3000


May be beneficial to put comments why they are 3000 and 222 for code maintenance.

JungukCho · 2021-11-19T22:40:08Z

npm/pkg/dataplane/policies/policy_windows.go

 	policySettings.LocalAddresses = srcListStr
 	policySettings.RemoteAddresses = dstListStr
 	policySettings.RemotePorts = dstPortStr
+	policySettings.LocalPorts = ""


It looks unnecessary. policySettings has default value (empty string) for string.

Just a preventive measure, can remove it.

JungukCho · 2021-11-19T22:40:13Z

npm/pkg/dataplane/policies/policy_windows.go

 	if policySettings.Direction == hcn.DirectionTypeOut {
 		policySettings.LocalAddresses = dstListStr
 		policySettings.LocalPorts = dstPortStr
+		policySettings.RemotePorts = ""


It looks unnecessary. policySettings has default value (empty string) for string.

This is needed unlike the previous comment because the we are first setting the policy remote ports for In and then changing to out, so to override the previous assignment we are re-assigning this value to empty string.

I guess it will be easier to understand to have if else based on hcn.DirectionTypeOut

if policySettings.Direction == hcn.DirectionTypeOut { policySettings.LocalAddresses = dstListStr policySettings.RemoteAddresses = srcListStr policySettings.LocalPorts = dstPortStr }else { // maybe In? policySettings.LocalAddresses = srcListStr policySettings.RemoteAddresses = dstListStr policySettings.RemotePorts = dstPortStr }

If I missed something, stay with current code, but slightly adjust order in if condition to have the same order above assignment since it helps understand what wat changed.

policySettings.LocalAddresses = srcListStr policySettings.RemoteAddresses = dstListStr policySettings.RemotePorts = dstPortStr policySettings.LocalPorts = "" if policySettings.Direction == hcn.DirectionTypeOut { policySettings.LocalAddresses = dstListStr policySettings.RemoteAddresses = srcListStr policySettings.RemotePorts = "" policySettings.LocalPorts = dstPortStr }

But do you see a huge readability issue with the way it is currently ? I feel like by explicitly assigning to "" it is clear what to expect from a ACL policy in a given direction and removes any assumptions ? wdyt ?

Vamsi Kalapala and others added 9 commits November 9, 2021 14:35

[NPM] Expanding HNS fake usage with internal state

e141679

adding some test cases for windows DP

8901c91

adding some test cases for windows DP

b613dda

Correcting some issues with windows DP

1185dc2

Splitting each modify call into two, one for 1st level sets and anoth…

82165c4

…er for nested sets

Fixing a build issue in linux

e56d038

Enhancing windows ipset tests

7bb6088

merging master

a575395

Adding ipset mgr windows tests

bfa9d3b

vakalapa requested review from JungukCho, huntergregory and matmerr November 16, 2021 23:02

fixing a build issue

81fbf8d

huntergregory reviewed Nov 16, 2021

View reviewed changes

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

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

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

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

vakalapa added 4 commits November 18, 2021 14:16

Adding in ACL verify code

45a3d35

merging master

b7e177f

addressing some lint issues

666b510

addressing some lint issues

b3fca1a

huntergregory reviewed Nov 18, 2021

View reviewed changes

removing apply ipset in generic ipsetMgr tests

692e534

huntergregory reviewed Nov 18, 2021

View reviewed changes

vakalapa added 3 commits November 18, 2021 16:37

fixing a build issue with windows

ca897eb

Merging master and resolving conflicts

4684031

fixing windows build issue

0a114ca

matmerr approved these changes Nov 19, 2021

View reviewed changes

JungukCho reviewed Nov 19, 2021

View reviewed changes

vakalapa merged commit ca3457e into master Nov 19, 2021

vakalapa deleted the vakr/winpoliciesreset branch November 19, 2021 22:57


		const networkName = "azure"

		var errorFakeHNS = errors.New("errorFakeHNS Error")

fix: [NPM] adding fixes to windows DP after first run #1100

fix: [NPM] adding fixes to windows DP after first run #1100

Uh oh!

Conversation

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

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!

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!

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!

rbtr Nov 22, 2021 • 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

vakalapa commented Nov 12, 2021 •

edited

Loading

rbtr Nov 22, 2021 •

edited

Loading