feat: [NPM] NPM v2 network policy controller and UTs for all v2 controllers #1082
Conversation
vakalapa
commented
Nov 3, 2021
vakalapa
commented
- Network policy controller v2 using generic DP APIs
- All V2 Controller UTs with GDPI mock
vakalapa
requested review from
JungukCho,
huntergregory,
matmerr and
neaggarwMS
huntergregory
left a comment
huntergregory
left a comment
There was a problem hiding this comment.
Wrote a note about updating policies versus removing/adding. We'll have to decide whether we want to track Prometheus policy metrics in controller pods, dp daemons, or both.
npm/pkg/controlplane/controllers/v2/networkPolicyController_test.go
Outdated
Show resolved
Hide resolved
| // 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.rawNpMap[netpolKey] = netPolObj | ||
| metrics.IncNumPolicies() |
There was a problem hiding this comment.
should we track policy metrics in the controller or DP? IPSet metrics must be tracked in the DP since the logic is only feasible in the ipset manager when we apply sets on need in the kernel
There was a problem hiding this comment.
I am okay with either approaches, since the object is just one and logic is simple if we do it in controller, since we will only increment if DP sends a no error signal. In DP we can have more robust metrics to determine, how many endpoints have a policy, how many rules etc .
There was a problem hiding this comment.
In DP policy may not make sense, but rules do. so we should have a metric for rules in DP, wdyt ?
There was a problem hiding this comment.
Makes sense to have rules in DP. Good with the code as is, but if the number of policies is the only metric that the control plane could take care of, then perhaps we just take care of that metric in DP only for simplicity
vakalapa
changed the title
|
To qualify the prometheus discussion, currently we have cluster-level metrics (e.g. num policies, sets, entries)j and node-level metrics (execution times). These metrics are hosted on different HTTP endpoints, and the cluster-level ones are redundant across all daemonsets. |
|
|
||
| // nodename in NewPodMetadata is nil so UpdatePod is ignored | ||
| podMetadata := dataplane.NewPodMetadata(podKey, namedPortIpsetEntry, "") | ||
| podMetadata := dataplane.NewPodMetadata(podKey, namedPortIpsetEntry, nodeName) |
There was a problem hiding this comment.
Curious - why do we have NodeName? Is it because of endpointPolicy of windows?
There was a problem hiding this comment.
Yes, in linux this will be no-op, this was the only major difference between linux and windows controllers, and i felt this did not have enough usage to be abstracted out and be maintained for both linux and windows.
| cachedPodMetadata := dataplane.NewPodMetadata(podKey, cachedNpmPod.PodIP, newPodMetadata.NodeName) | ||
|
|
||
| if err = c.dp.RemoveFromSets([]*ipsets.IPSetMetadata{ipsets.NewIPSetMetadata(podIPSetName, ipsets.KeyLabelOfPod)}, cachedPodMetadata); err != nil { | ||
| toRemoveSet := ipsets.NewIPSetMetadata(podIPSetName, ipsets.KeyLabelOfPod) |
JungukCho
left a comment
JungukCho
left a comment
There was a problem hiding this comment.
lgtm, but can you double check DeepEqual with UTs before merging it.
| // In this updateNetworkPolicy event, | ||
| // newNetPol was updated with states which netPolController does not need to reconcile. | ||
| if reflect.DeepEqual(*cachedNetPolSpecObj, netPolObj.Spec) { | ||
| if reflect.DeepEqual(cachedNetPolSpecObj, &netPolObj.Spec) { |
There was a problem hiding this comment.
Curious - Are their result different?
reflect.DeepEqual(*cachedNetPolSpecObj, netPolObj.Spec) and reflect.DeepEqual(cachedNetPolSpecObj, &netPolObj.Spec)
There was a problem hiding this comment.
Yeah , did not want to pass in full objs and replicating them inside deep inside, so decided to send in pointers.
| } | ||
|
|
||
| cachedNetPolObj, netPolExists := c.rawNpMap[key] | ||
| cachedNetPolSpecObj, netPolExists := c.rawNpMap[key] |
There was a problem hiding this comment.
I worried about updating name and namespace , but they are immutable.
There was a problem hiding this comment.
Yes and our Key has namespace and name in it.
|
@JungukCho TestUpdateNetworkPolicy validated that scenario and you can see updatePolicy is called only once. |
…ollers (Azure#1082) * adding a legacy build command * Adding all v2 controller test files * v2 podcontroller changes * completing all pod v2 controllers uts * Adding netpol v2 controller UTs * Removing unused make file command * Fixing lints and correcting a test case * Fixing an error in expected values * dealing with flaky tests * Fixing an issue with HCN vendor, until we wait for the fix to be rolled out * Addressing some comments * Removing addPolicy call and relying on updatepolicy * Saving only spec of netpol and not whole object * changing name of rawNPMap to rawNPSpecMap * changing name of rawNPMap to rawNPSpecMap * Deep equal type for spec was not equal corrected the pointers * Deep equal type for spec was not equal corrected the pointers
