[NPM] support network policy controller and its unit tests #849

JungukCho · 2021-04-06T18:42:45Z

Reason for Change:

This PR is mainly to add network policy controller to make dealing with network policy event reliable.
Add unit tests for network policy controller to test various network policy events.
Cleans up previous codes (e.g., not used logics).
Put comments of existing and new codes as many as possible.

Issue Fixed:

Retry reconcile loop if there is failed
Fix incorrect return error.

Requirements:

uses conventional commit messages
includes documentation
adds unit tests

Notes:

codecov · 2021-04-06T18:51:50Z

Codecov Report

Merging #849 (b7eb15c) into master (cf7d8a5) will increase coverage by 0.40%.
The diff coverage is 46.57%.

@@            Coverage Diff             @@
##           master     #849      +/-   ##
==========================================
+ Coverage   42.10%   42.51%   +0.40%     
==========================================
  Files         158      158              
  Lines       15025    15138     +113     
==========================================
+ Hits         6327     6436     +109     
+ Misses       7926     7913      -13     
- Partials      772      789      +17

…l and uninstall default Azure NPM chain.

…unction

…iables

…more test cases (e.g., update and adding multiple network policies)

…work policies. Update unit tests to test more network policies events

vakalapa · 2021-04-07T15:51:37Z

npm/networkPolicyController.go

+		clientset:              clientset,
+		netPolLister:           npInformer.Lister(),
+		netPolListerSynced:     npInformer.Informer().HasSynced,
+		workqueue:              workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "NetPols"),


expand netpols to NetworkPolicy while naming the WQ.

vakalapa · 2021-04-07T15:52:31Z

npm/networkPolicyController.go

+
+	npInformer.Informer().AddEventHandler(
+		cache.ResourceEventHandlerFuncs{
+			AddFunc:    netPolController.addNetPol,


lets try to have function named too as AddNetworkPolicy or UpdateNetworkPOlicy. Netpol is our lingo and may confuse some folks.

ack. For consistency, I also changed deleteNetPol to deleteNetworkPolicy. deleteNetworkPolicy -> cleanUpNetworkPolicy

npm/networkPolicyController.go

vakalapa · 2021-04-07T16:00:28Z

npm/networkPolicyController.go

+
+	// (TODO): Reduce scope of lock later
+	c.npMgr.Lock()
+	defer c.npMgr.Unlock()


We can optimize this lock from defer to explicitly mentioning below RawNpMap exists check. thta way we are not locking when Add is being performed. Probably we can expand this to other controllers later.

Good point. Another important thing is we need to decouple all shared maps (e.g., RawNpMap, etc) from npm.go to each controller and then carefully walk through them again. Even we may encapsulate each map with lock as a struct. Let's understand each maps in details and start this process as another PR.

npm/networkPolicyController.go

vakalapa · 2021-04-07T16:19:43Z

npm/networkPolicyController_test.go

+		f.t.Errorf("ipSetSave failed @ ipsMgr.Save")
+	}
+}
+func (f *netPolFixture) ipSetRestore(ipsetConfigFile string) {


We can reuse ipsetsave and restore from other tests right ?

Definitely. we can. I would suggest we left them separately now and see them after reviewing and thinking overall structure of our codes since as we discussed, we may decouple each controller into each package later.

vakalapa · 2021-04-07T16:20:59Z

npm/networkPolicyController_test.go

+	f.kubeInformer.Start(stopCh)
+}
+
+func (f *netPolFixture) ipSetSave(ipsetConfigFile string) {


with this, we will need IpTableSave and IpTableRestore, since each test will be manipulating iptable rules, we should clean them up once done

Good point. Added two methods for iptables management. Now the methods are not used in networkPolicyController_test.go due to npm_test.go. We will use them later.

npm/networkPolicyController.go

matmerr · 2021-04-08T18:51:16Z

/azp run

azure-pipelines · 2021-04-08T18:51:29Z

Azure Pipelines successfully started running 1 pipeline(s).

…policy cache in a right place. Correct prometheus metric code.

vakalapa

lgtm 🚀

…hod parameter in cleanUpNetworkPolicy

vakalapa · 2021-04-12T22:53:07Z

npm/networkPolicyController.go

+			// #1 No item in RawNpMap, which means network policy with the cachedNetPolKey is not applied. Thus, do not need to clean up process.
+			// #2 item in RawNpMap exists, which means network policy with the cachedNetPolKey is applied . Thus, Need to clean up process.
+			cachedNetPolKey := util.GetNSNameWithPrefix(key)
+			_, cachedNetPolObjExists := c.npMgr.RawNpMap[cachedNetPolKey]


Do we have to check if the cahedObj exists here ? Because the first check we do in cleanUpnetworkPolicy is if the cachedObj exists, i think we can safely remove this check. Any reason for leaving this check here ?

Good catch. Removed the redundant check.

npm/networkPolicyController.go

vakalapa · 2021-04-13T15:33:12Z

npm/npm.go

-		},
-	)
+	// create network policy controller
+	npMgr.netPolController = NewNetworkPolicyController(informerFactory.Networking().V1().NetworkPolicies(), clientset, npMgr)


Can we not replace "informerFactory.Networking().V1().NetworkPolicies()" with npInformer (L213) as they are same ?

I would like to remove npInformer from NetworkPolicyManager struct in npm.go. Let me first check whether I can remove npInformer member. If not, I can use npInfomer.

npInformer member in NetworkPolicyManager struct is also used in Start method while it may be not necessary to keep the member. I changed it to "npInformer".

vakalapa · 2021-04-13T15:40:37Z

npm/networkPolicyController_test.go

+	testCases := []expectedNetPolValues{
+		{1, 2, 0, true, true},
+	}
+	checkNetPolTestResult("TestAddMultipleNetPols", f, testCases)


In the old nwpolicy_test, we were checking if the counters in Prometheus were correct, we are missing that check here, can we add back checks like below

newGaugeVal, err3 := promutil.GetValue(metrics.NumPolicies)
newCountVal, err4 := promutil.GetCountValue(metrics.AddPolicyExecTime)

Good catch. Will add them.

npm/networkPolicyController.go

neaggarwMS · 2021-04-13T16:34:33Z

npm/networkPolicyController.go

+}
+
+// needSync checks whether the obj is valid network policy object.
+func (c *networkPolicyController) needSync(obj interface{}) (string, error) {


Nit: This also returns the Key...so shall we rename it to GetNetworkPolicyKey (as the only validation it does is the networkpolicy object and not really if this is a valid update.

Good suggestion. applied.

neaggarwMS · 2021-04-13T16:35:39Z

npm/networkPolicyController.go

+		// if network policy does not have different states against lastly applied states stored in cachedNetPolObj,
+		// netPolController does not need to reconcile this update.
+		// in this updateNetworkPolicy event, newNetPol was updated with states which netPolController does not need to reconcile.
+		if isSameNetworkPolicy(cachedNetPolObj, newNetPol) {


This can also be called as needSync, but I'm fine with isSameNetworkPolicy

Now I would like to stick to isSameNetworkPolicy name since it is related to isSamePolicy function in parsePolicy.go and ProcessedNpMap data structure. So, when revisiting this issue, I will consider suggestion.

vakalapa · 2021-04-13T16:34:40Z

npm/networkPolicyController.go

+	// Cache network object first before applying ipsets and iptables.
+	// If error happens while applying ipsets and iptables,
+	// the key is re-queued in workqueue and process this function again, which eventually meets desired states of network policy
+	c.npMgr.RawNpMap[cachedNetPolKey] = netPolObj


We will need a deepcopy of this here ! we are not removing references from the sharedinformer cache

I think current code is better in terms of memory and computations. If we use deepcopy network policyhere, we will always have two network policy objects and increase computations for deepcopy operation. But, if we use reference of network policy from sharedinformer cache, it will be either one or two objects eventually and no computation overhead for deepcopy.

neaggarwMS · 2021-04-13T16:36:37Z

npm/networkPolicyController.go

+	netPolCachedKey := util.GetNSNameWithPrefix(key)
+
+	// (TODO): need to decouple this lock from npMgr if possible
+	c.npMgr.Lock()


Shouldnt we take a ReadLock? Same for above

Good catch. We can use readlock here, but current NPM code base is using global Mutex. When we decouple global Mutex from our code, we can take your suggestion. It will be different PR.

neaggarwMS · 2021-04-13T16:39:03Z

npm/networkPolicyController.go

+		// TODO : may consider using "c.queue.AddAfter(key, *requeueAfter)" according to error type later
+		if err := c.syncNetPol(key); err != nil {
+			// Put the item back on the workqueue to handle any transient errors.
+			c.workqueue.AddRateLimited(key)


c.workqueue.AddRateLimited(key) [](start = 3, length = 31)

Why do we need to do that? Simply dont call c.WorkQueue.Done in linie#196 (You should call that only if err!=nil)

We want to put ratelimit when reconcile is failed not to try it again so fast. https://pkg.go.dev/k8s.io/client-go/util/workqueue#RateLimitingInterface

neaggarwMS · 2021-04-13T16:39:23Z

npm/networkPolicyController.go

+		}
+		// Finally, if no error occurs we Forget this item so it does not
+		// get queued again until another change happens.
+		c.workqueue.Forget(obj)


How it is different from c.Workqueue.Done?

// We call Done here so the workqueue knows we have finished processing this item. We also must remember to call Forget if we do not want this work item being re-queued. For example, we do not call Forget if a transient error occurs, instead the item is put back on the workqueue and attempted again after a back-off period. - Refer to
https://github.com/kubernetes/sample-controller/blob/master/controller.go#L196

neaggarwMS · 2021-04-13T17:31:16Z

npm/networkPolicyController.go

+	}
+
+	err := func(obj interface{}) error {
+		defer c.workqueue.Done(obj)


defer c.workqueue.Done(obj) [](start = 2, length = 27)

c.workqueue.Done(obj) if err != nil { c.workqueue.AddRateLimited(key) }

I think we agree to stick to current code.

…etrics. Applied better naming and removed redundancy codes.

…age.

vakalapa · 2021-04-13T23:13:18Z

/azp run

azure-pipelines · 2021-04-13T23:13:28Z

Azure Pipelines successfully started running 1 pipeline(s).

* first version of network policy controller and its unit tests * update reconcile and deleteNetworkPolicy function to correctly install and uninstall default Azure NPM chain. * To explicitly manage default Azure NPM chain in deleteNetworkPolicy function * correct comments and delete unused variable * fix missed returing errors in codes * Correct to check DeletionTimestamp and DeletionGracePeriodSeconds variables * removed placeholder functions in network policy controoler and added more test cases (e.g., update and adding multiple network policies) * - applied comments (use explict names, locating lock in a better place) * add two methods to save and restore iptables in unit test * comment out unused function * early filter in updateNetworkPolicy function if they are the same network policies. Update unit tests to test more network policies events * - start using klog package instead of log package * remove unneeded defer for lock * Locate of adding and deleting network policy object from our network policy cache in a right place. Correct prometheus metric code. * use cached network policy key instead of network policy object as method parameter in cleanUpNetworkPolicy * remove redundant check * Remove ns- prefix as key in RawNpMap. Update UT to check prometheus metrics. Applied better naming and removed redundancy codes. * minor update for varialbe names * remove dependency between UT by re-initializing metrics. Correct message.

first version of network policy controller and its unit tests

758e0cb

JungukCho added enhancement npm Related to NPM. labels Apr 6, 2021

JungukCho requested review from matmerr, neaggarwMS, thatmattlong and vakalapa April 6, 2021 18:42

JungukCho added 10 commits April 6, 2021 14:16

update reconcile and deleteNetworkPolicy function to correctly instal…

c7aa535

…l and uninstall default Azure NPM chain.

To explicitly manage default Azure NPM chain in deleteNetworkPolicy f…

d8a4d2b

…unction

correct comments and delete unused variable

96a5bde

fix missed returing errors in codes

3b6fbc0

Correct to check DeletionTimestamp and DeletionGracePeriodSeconds var…

0bb785b

…iables

removed placeholder functions in network policy controoler and added …

c3a1969

…more test cases (e.g., update and adding multiple network policies)

- applied comments (use explict names, locating lock in a better place)

928aee6

add two methods to save and restore iptables in unit test

faad159

comment out unused function

f5ac80e

early filter in updateNetworkPolicy function if they are the same net…

50d1e98

…work policies. Update unit tests to test more network policies events

vakalapa reviewed Apr 7, 2021

View reviewed changes

npm/networkPolicyController.go Outdated Show resolved Hide resolved

npm/networkPolicyController.go Outdated Show resolved Hide resolved

JungukCho added 2 commits April 7, 2021 15:59

- start using klog package instead of log package

7db79d0

remove unneeded defer for lock

b9ea448

Locate of adding and deleting network policy object from our network …

2850c34

…policy cache in a right place. Correct prometheus metric code.

vakalapa previously approved these changes Apr 9, 2021

View reviewed changes

use cached network policy key instead of network policy object as met…

761a974

…hod parameter in cleanUpNetworkPolicy

JungukCho dismissed vakalapa’s stale review via 761a974 April 9, 2021 23:24

vakalapa reviewed Apr 12, 2021

View reviewed changes

vakalapa requested changes Apr 12, 2021

View reviewed changes

npm/networkPolicyController.go Show resolved Hide resolved

remove redundant check

b6fb6a2

vakalapa reviewed Apr 13, 2021

View reviewed changes

neaggarwMS reviewed Apr 13, 2021

View reviewed changes

vakalapa reviewed Apr 13, 2021

View reviewed changes

neaggarwMS reviewed Apr 13, 2021

View reviewed changes

JungukCho added 3 commits April 13, 2021 13:38

Remove ns- prefix as key in RawNpMap. Update UT to check prometheus m…

81b8fb1

…etrics. Applied better naming and removed redundancy codes.

minor update for varialbe names

9551acf

remove dependency between UT by re-initializing metrics. Correct mess…

b7eb15c

…age.

vakalapa approved these changes Apr 14, 2021

View reviewed changes

JungukCho merged commit d816931 into Azure:master Apr 14, 2021

[NPM] support network policy controller and its unit tests #849

[NPM] support network policy controller and its unit tests #849

Uh oh!

Conversation

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

Uh oh!

Uh oh!

codecov bot commented Apr 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

matmerr commented Apr 8, 2021

Uh oh!

azure-pipelines bot commented Apr 8, 2021

Uh oh!

vakalapa 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!

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!

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!

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 Apr 6, 2021 •

edited

Loading

codecov bot commented Apr 6, 2021 •

edited

Loading