[NPM] Windows Policy Manager changes for OS22 #1062

vakalapa · 2021-10-15T23:26:38Z

This PR consists of following changes:

Set Policies:

Reset and initialize of Windows SetPolicies
Add, update and delete events of SetPolicies

ACL Policies:

Add, Update and Delete events of Set Policies

npm/pkg/dataplane/policies/policymanager_windows.go

vakalapa · 2021-10-20T18:24:45Z

npm/pkg/dataplane/ipsets/ipsetmanager.go

+	for setName := range iMgr.toDeleteCache {
+		_, ok := iMgr.toAddOrUpdateCache[setName]
+		if ok {
+			delete(iMgr.toDeleteCache, setName)


do not delete but Log an error to make sure everything is working as expected

huntergregory

added nitpicks. might follow up after reading the rest

npm/pkg/dataplane/policies/policy_windows.go

huntergregory · 2021-10-20T19:48:49Z

npm/pkg/dataplane/policies/policy_windows.go

+		}
+	}
+
+	if !acl.checkIPSets() {


confused what checkIPSets is supposed to do since it looks at more than namedports

This is checking if any of the unsupported translation features are used for ACL, if so we will ignore applying this policy

I guess to clarify I'm confused why we return ErrNamedPortsNotSupported when the check looks at more than named ports

huntergregory

notes on policymanager

npm/pkg/dataplane/policies/policymanager_windows.go

huntergregory · 2021-10-21T02:22:43Z

npm/pkg/dataplane/policies/policymanager_windows.go

+}
+
+func (epBuilder *endpointPolicyBuilder) compareAndRemovePolicies(rulesToRemove []*NPMACLPolSettings) error {
+	lenOfRulesToRemove := len(rulesToRemove)


Should we do this more efficiently? current runtime is O(R A^2) where R is # to remove and A is total # acls. Could also update next iteration

We could make it O(R A) w/ a linked list for the builder [O(1) removal], or O(R) w/ a set for the builder [O(1) removal and existence check]

Optimized it to be o(n) now, We will not be using unique ID for all policies, we can get away with using unique for a group of ACL generated from one network policy

huntergregory · 2021-10-21T02:29:06Z

npm/pkg/dataplane/policies/policymanager_windows.go

+			}
+		}
+	}
+	if lenOfRulesToRemove > 0 {


can just return an error if it doesn't exist in loop if we don't care about how many don't exist

npm/pkg/dataplane/policies/policymanager_windows.go

…2 control plane

huntergregory

notes on ipsetmanager

npm/pkg/dataplane/ipsets/ipsetmanager.go

npm/pkg/dataplane/ipsets/ipsetmanager_windows.go

vakalapa · 2021-10-25T19:43:50Z

/azp run

azure-pipelines · 2021-10-25T19:44:07Z

Azure Pipelines successfully started running 2 pipeline(s).

huntergregory · 2021-10-25T21:54:56Z

when do we want to delete ipsets from our overall cache? Curious about the bolded situation below. For ApplyOnNeed ipset mode, should we remove the ipset from the overall cache if references become 0? Since creating an empty ipset basically costs the same as checking if it exists, I think we would work at the same speed with optimal memory usage. Also noticing that if we use the ApplyOnNeed mode, then we would never need to use a CreateIPSet() or DeleteIPSet() call.

the contract for DP around policies is:

AddPolicy: for each TranslatedIPSet, we create the set (and any member sets) if it doesn't exist, keeping old members and adding any new members within the TranslatedIPSet
RemovePolicy: for each TranslatedIPSet, we try to delete the set (and any member sets??); however, we won't delete a set if it's referenced anywhere (by other policies or other ipsets)

…licies in order

vakalapa · 2021-10-26T23:07:48Z

@huntergregory As we discussed offline, i am inline with proactively deleting an IPSet when it has no references and no memebers. On a removeFromSets, RemoveFromLists call at the end, we can check for ipset.CanBeDeleted() and if yes add it to delete cache, Anyway when the controller needs this ipsets, it is going to either create, or our AddTo* calls will create ipset if needed.

vakalapa · 2021-10-27T18:26:43Z

npm/pkg/dataplane/ipsets/ipsetmanager_windows.go

+	idx := 0
+	policySettingsOrder := []hcn.SetPolicyType{SetPolicyTypeNestedIPSet, hcn.SetPolicyTypeIpSet}
+	if operation == hcn.RequestTypeRemove {
+		policySettingsOrder = []hcn.SetPolicyType{hcn.SetPolicyTypeIpSet, SetPolicyTypeNestedIPSet}


Add comments here

huntergregory · 2021-10-27T19:34:01Z

when do we want to delete ipsets from our overall cache? Curious about the bolded situation below. For ApplyOnNeed ipset mode, should we remove the ipset from the overall cache if references become 0? Since creating an empty ipset basically costs the same as checking if it exists, I think we would work at the same speed with optimal memory usage. Also noticing that if we use the ApplyOnNeed mode, then we would never need to use a CreateIPSet() or DeleteIPSet() call.

the contract for DP around policies is:

AddPolicy: for each TranslatedIPSet, we create the set (and any member sets) if it doesn't exist, keeping old members and adding any new members within the TranslatedIPSet

RemovePolicy: for each TranslatedIPSet, we try to delete the set (and any member sets??); however, we won't delete a set if it's referenced anywhere (by other policies or other ipsets)

Update to delete from cache when we delete from kernel in another PR

huntergregory

looks good. Can we move some policy checks to generic pMgr? (noted in comments)

npm/npm.go

npm/pkg/dataplane/dataplane_windows.go

huntergregory · 2021-10-27T20:23:01Z

npm/pkg/dataplane/policies/policy.go

 // or a single HNS rule in windows
 type ACLPolicy struct {
 	// PolicyID is the rules name with a given network policy
+	// PolicyID will be same for all ACLs in a Network Policy


do we ever use this back-reference?

Yes we are using this policyId in policymanager windows, to equate what policies are applied in HNS while updating or removing.

npm/pkg/dataplane/policies/policy_windows.go

npm/pkg/dataplane/policies/policymanager_windows.go

huntergregory · 2021-10-27T21:02:23Z

npm/pkg/dataplane/policies/policymanager_windows.go

+			continue
+		}
+
+		epBuilder.compareAndRemovePolicies(rulesToRemove[0].Id, len(rulesToRemove))


just removing the first rule?

nevermind, I see that they all have the same ID. Perhaps pass in the whole rulesToRemove as an arg instead?

Yeah i did that first, but there is no reason to send the whole rule set, we just need the ACl Id of one and since all the ACL IDs in a policy are same, we can get away with it. If tomorrow we do add unique ACl Ids then we can send in the full rulesToRemove obj

npm/pkg/dataplane/policies/policymanager_windows.go

huntergregory

🚀

[NPM] Windows Policy Manager changes for OS22

a11cf9e

vakalapa added enhancement npm Related to NPM. labels Oct 15, 2021

Vamsi Kalapala added 9 commits October 15, 2021 16:27

Merge branch 'master' into vakr/npmwindowslogic

c6cc5fd

Adding new NPM ACLSettings with ID

8bb02a7

first pass on both add and remove policies

5b50744

merge conflict

f43a2b9

fixing a merge issue

5fa1cfd

Working 1st level Setpolicy CRUD operations

44224fd

have NPMACl to HNSACL conversion logic ready

799cc22

updating policy endpoints only after adding policy to an endpoint

8f84627

updating policy endpoints only after adding policy to an endpoint

ba2c798

vakalapa commented Oct 20, 2021

View reviewed changes

npm/pkg/dataplane/policies/policymanager_windows.go Show resolved Hide resolved

vakalapa commented Oct 20, 2021

View reviewed changes

Vamsi Kalapala added 4 commits October 20, 2021 13:01

Merge branch 'master' into vakr/npmwindowslogic

cc25352

fixing a build issue

59ddfbb

fixing merge conflicts

e54ddfc

fixing issue in linux files

002707a

huntergregory reviewed Oct 21, 2021

View reviewed changes

Addressing some comments and also completing some integrations with V…

33693d9

…2 control plane

huntergregory reviewed Oct 22, 2021

View reviewed changes

Vamsi Kalapala added 4 commits October 22, 2021 16:11

Updating policy ID logic and update pod

0d268d7

Updating policy ID logic and update pod

24c7ce5

Addressing some comments

d1f7f5b

adding basic reset bits

9b27bf3

Vamsi Kalapala added 2 commits October 25, 2021 13:46

fixnig build issue in linux

5d29953

fixnig build issue in linux

5a484c7

Vamsi Kalapala added 2 commits October 25, 2021 14:14

Fixing the _linux_test.go build failures

0563e2b

fix lints

a1da271

Addressing some comments and correcting windows logic to apply set po…

66b091b

…licies in order

cleaning up logic for calculating set policies

35ff0f0

vakalapa marked this pull request as ready for review October 26, 2021 23:08

vakalapa requested review from JungukCho and matmerr October 26, 2021 23:08

vakalapa commented Oct 27, 2021

View reviewed changes

huntergregory reviewed Oct 27, 2021

View reviewed changes

Applying some feedback.

95e0995

huntergregory previously approved these changes Oct 28, 2021

View reviewed changes

fixing a failing test and panic

5f8ed65

vakalapa dismissed huntergregory’s stale review via 5f8ed65 October 28, 2021 00:13

huntergregory approved these changes Oct 28, 2021

View reviewed changes

vakalapa merged commit 9850758 into master Oct 28, 2021

vakalapa deleted the vakr/npmwindowslogic branch October 28, 2021 15:50

[NPM] Windows Policy Manager changes for OS22 #1062

[NPM] Windows Policy Manager changes for OS22 #1062

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huntergregory left a comment

Choose a reason for hiding this comment

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!

huntergregory left a comment

Choose a reason for hiding this comment

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!

Uh oh!

Uh oh!

Uh oh!

huntergregory left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vakalapa commented Oct 25, 2021

Uh oh!

azure-pipelines bot commented Oct 25, 2021

Uh oh!

huntergregory commented Oct 25, 2021

Uh oh!

vakalapa commented Oct 26, 2021

Uh oh!

vakalapa Oct 27, 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!

huntergregory commented Oct 27, 2021

Uh oh!

huntergregory left a comment

Choose a reason for hiding this comment

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!

Uh oh!

Choose a reason for hiding this comment

vakalapa commented Oct 15, 2021 •

edited

Loading

vakalapa Oct 27, 2021 •

edited

Loading