Skip to content

Conversation

@vakalapa
Copy link
Contributor

@vakalapa vakalapa commented Nov 22, 2021

Reason for Change:

Adding support to reset all existing NPM ACL policies on all endpoints in windows DP.

Issue Fixed:

Requirements:

Notes:

@vakalapa vakalapa requested review from JungukCho, huntergregory and matmerr and removed request for matmerr November 22, 2021 19:14
epIDs := dp.getAllEndpointIDs()

// It is important to keep order to clean-up ACLs before ipsets. Otherwise we won't be able to delete ipsets referenced by ACLs
if err := dp.policyMgr.Reset(epIDs); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in resetDataPlane(), only difference between linux and windows is getting the endpoint IDs. Perhaps make getAllEndpointIDs() OS specific (call dp.initializeDP() within it) and keep pMgr and iMgr resets in generic dp.Reset()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will also need to initializeDP first in windows to be able to get a view of the endpoints in HNS, so it would be better to keep it as is.

// but if the bug is not solved then get all existing policies and remove relevant policies from list
// then apply remaining policies onto the endpoint
var aggregateErr error
noOfRulesToRemove := len(rulesToRemove)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: more conventional to say numOfRules?

@huntergregory huntergregory added the npm Related to NPM. label Nov 23, 2021
@vakalapa vakalapa merged commit d40a907 into master Dec 3, 2021
@vakalapa vakalapa deleted the vakr/windpreset branch December 3, 2021 00:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

npm Related to NPM.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants