Add NetworkPolicyReference to controlplane NetworkPolicy #1258

tnqn · 2020-09-16T14:14:43Z

NetworkPolicy in controlplane API group is the object that consumed by antrea-agents. Both Antrea NetworkPolicy and K8s NetworkPolicy will be converted to it. Currently, the namespace and name of the original NetworkPolicy are copied to the controlplane NetworkPolicy and <Namespace>/<Name> is used as the key func. Therefore, one K8s NetworkPolicy may overwrite the controlplane NetworkPolicy mapping to a Antrea NetworkPolicy that has the same namespace and name.

To fix it completely, the original NetworkPolicy name will not be used as the controlplane NetworkPolicy name directly. However, antrea-agents still need to know the information about the original NetworkPolicy for multiple purposes, e.g. metrics and realization status report, and traceflow observation correlation.

To keep backward compatibility, this patch adds a new field "SourceRef" that includes the information about the original NetworkPolicy to the struct of controlplane NetworkPolicy. Instead of assuming controlplane NetworkPolicy has the same name as the original NetworkPolicy, antrea-agents consumes the new field to get the information about the original NetworkPolicy.

For #1173 and #1172

Compatibility impact: None

antrea-bot · 2020-09-16T14:14:55Z

Thanks for your PR.
Unit tests and code linters are run automatically every time the PR is updated.
E2e, conformance and network policy tests can only be triggered by a member of the vmware-tanzu organization. Regular contributors to the project should join the org.

The following commands are available:

/test-e2e: to trigger e2e tests.
/skip-e2e: to skip e2e tests.
/test-conformance: to trigger conformance tests.
/skip-conformance: to skip conformance tests.
/test-whole-conformance: to trigger all conformance tests on linux.
/skip-whole-conformance: to skip all conformance tests on linux.
/test-networkpolicy: to trigger networkpolicy tests.
/skip-networkpolicy: to skip networkpolicy tests.
/test-windows-conformance: to trigger windows conformance tests.
/skip-windows-conformance: to skip windows conformance tests.
/test-windows-networkpolicy: to trigger windows networkpolicy tests.
/skip-windows-networkpolicy: to skip windows networkpolicy tests.
/test-hw-offload: to trigger ovs hardware offload test.
/skip-hw-offload: to skip ovs hardware offload test.
/test-all: to trigger all tests (except whole conformance).
/skip-all: to skip all tests (except whole conformance).

codecov-commenter · 2020-09-16T14:18:50Z

Codecov Report

Merging #1258 into master will decrease coverage by 1.25%.
The diff coverage is 56.52%.

@@            Coverage Diff             @@
##           master    #1258      +/-   ##
==========================================
- Coverage   56.29%   55.04%   -1.26%     
==========================================
  Files         109      110       +1     
  Lines       12045    10597    -1448     
==========================================
- Hits         6781     5833     -948     
+ Misses       4673     4191     -482     
+ Partials      591      573      -18

Flag	Coverage Δ
#integration-tests	`44.89% <20.00%> (-1.22%)`	⬇️
#unit-tests	`41.90% <59.37%> (-0.64%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/agent/openflow/client.go	`32.85% <ø> (-2.75%)`	⬇️
pkg/apis/controlplane/helper.go	`0.00% <0.00%> (ø)`
pkg/apis/controlplane/v1beta1/helper.go	`0.00% <0.00%> (ø)`
pkg/agent/controller/networkpolicy/reconciler.go	`66.50% <20.83%> (-2.57%)`	⬇️
pkg/agent/openflow/network_policy.go	`73.70% <50.00%> (+0.08%)`	⬆️
pkg/agent/controller/networkpolicy/cache.go	`79.34% <100.00%> (+2.02%)`	⬆️
...ntroller/networkpolicy/networkpolicy_controller.go	`61.26% <100.00%> (+2.72%)`	⬆️
pkg/agent/types/networkpolicy.go	`11.11% <100.00%> (-5.56%)`	⬇️
...kg/controller/networkpolicy/antreanetworkpolicy.go	`64.28% <100.00%> (+3.17%)`	⬆️
...g/controller/networkpolicy/clusternetworkpolicy.go	`63.41% <100.00%> (+2.74%)`	⬆️
... and 112 more

tnqn · 2020-09-16T15:30:41Z

/test-all

abhiraut

lgtm

pkg/controller/networkpolicy/antreanetworkpolicy_test.go

pkg/controller/types/networkpolicy.go

abhiraut · 2020-09-16T23:39:03Z

/cc @suwang48404 .. note this changes the message of internal network policies

Dyanngg · 2020-09-17T01:05:58Z

pkg/agent/controller/networkpolicy/cache.go

+	var rules []*rule
+	for _, obj := range objs {
+		rule := obj.(*rule)
+		if rule.PolicyRef.Name == npName {


How is K8s NP and ANP differentiated here? If there were NP and ANP with same ns/name, wouldn't rules for both those policies be added to rules still?

Good catch, I tried to fix it but it seems not a simple fix as antctl never expects Antrea Policies before. I filed #1262 and listed existing issues. As this interface is consumed by antctl only, I would prefer addressing it with a separate PR for antctl. What do you think?

I agree. cc @GraysonWu as he is working on antctl support for ClusterNetworkPolicy. This PR and issue #1262 should be relevant

i noticed this too.. but since we have not yet implemented antctl so it won't make sense to handle it in this PR
@tnqn i already raised #927 issue long time back.. will dup your issue to it as Grayson is already assigned the initial PR

thanks @Dyanngg @abhiraut @GraysonWu for taking care of this.

tnqn · 2020-09-17T15:06:02Z

/test-all

jianjuns

Question - this is not a compatible API changes right? @tnqn

abhiraut · 2020-09-17T23:26:14Z

Question - this is not a compatible API changes right? @tnqn

i think this is one case where we may need to handle versioning ?

tnqn · 2020-09-18T04:18:44Z

@jianjuns @abhiraut I added compatibility impact to the PR's description and proposed a staged solution, please see if it makes sense for you. I think at least it's riskless to do 1 for now and we could decide whether it's worth to do 2 and 3 later.

tnqn · 2020-09-18T15:41:11Z

@jianjuns @abhiraut I'm going to update PR's description to reflect the latest compatibility impact, posting previous impact and proposal here:

Compatibility impact if we change controlplane NetworkPolicy to cluster scope and use UID as name directly:

This change is backward-compatible for antrea-agent.
Older antrea-agents can communicate with new antrea-controller as nothing has changed to the rule and antrea-agents never filtered policies by namespace.
A few logging and messages of Traceflow observations will be impacted as "Namespace" is no longer set and "Name" is changed to the UID of the original NetworkPolicy, for example:

# the original log
NetworkPolicy default/allow-dns applied to Pods on this Node
# older agents would become
NetworkPolicy /561e923c-a9c6-43b6-80d8-fda1427e4b98 applied to Pods on this Node
$ new agents would become
NetworkPolicy K8sNetworkPolicy:default/allow-dns applied to Pods on this Node

It's not completely backward-compatible for antctl. Filtering by namespace and/or name will be broken because of the namespace scope change.

# antctl-0.9.3 get netpol
NAMESPACE NAME                                 APPLIED-TO                           RULES
<NONE>    561e923c-a9c6-43b6-80d8-fda1427e4b98 c4c59cfe-9160-5de5-a85b-01a58d11963e 1
# antctl-0.9.3 get netpol 561e923c-a9c6-43b6-80d8-fda1427e4b98
Error: an empty namespace may not be set when a resource name is provided
# antctl-0.9.3 get netpol -n default
Error: Not Found: the server could not find the requested resource

If we want to keep complete backward-compatibility for all clients, maybe we could do the following:

For this release, we only add sourceRef field to v1beta1 and do not make any change to the content of namespace, name, and namespace scope. So features that need to know kinds of NetworkPolicies can get what they need without any other clients affected.
In next release, we add v1beta2 version that changes the resource scope to cluster and use the original UID as the resource's name.
After v1beta1 clients are out of support windows, we remove v1beta1.

tnqn · 2020-09-18T16:37:55Z

/test-all

jianjuns · 2020-09-18T19:05:46Z

@tnqn I agreed with the two-stages approach you proposed. We can decide later how to address other issues and whether to increase API group version.

antoninbas · 2020-09-18T19:37:11Z

The approach looks good, but I'd like to understand how this will fix #1173 in v0.10? It seems that ANP and K8s NP can still overwrite each other in the store as we are still using the same Name & Namespace in the internal policy object and we didn't update the store key function?

tnqn · 2020-09-21T04:35:30Z

The approach looks good, but I'd like to understand how this will fix #1173 in v0.10? It seems that ANP and K8s NP can still overwrite each other in the store as we are still using the same Name & Namespace in the internal policy object and we didn't update the store key function?

@antoninbas It cannot fix #1173 and is only the first step that makes current internal API consumers not rely on the name and namespace of controlplane networkpolicy, and a prerequisite of supporting NetworkPolicy metrics/status. Changing key function or the name of controlplane networkpolicy may introduce little troubles to upgrade and backward compatibility as listed in #1258 (comment), though none of them are really serious, and we don't know its impact on external API consumers (cloud case) yet. As ANP is still alpha, do you think we could defer the fix to next release?

antoninbas · 2020-09-21T17:46:08Z

@tnqn yes, sounds good. I was just double-checking that this was not indeed a fix

antoninbas

just one suggestion to improve a comment, otherwise LGTM

antoninbas · 2020-09-21T18:21:31Z

pkg/agent/controller/networkpolicy/cache.go

+	// Reference to the original NetworkPolicy that the rule belongs to.
+	// Note it has different meaning from PolicyUID, PolicyName, and
+	// PolicyNamespace which are the metadata of controlplane NetworkPolicy.
+	// Although they are same for now, it might change in future, features that
+	// need the information of the original NetworkPolicy should use SourceRef.
+	SourceRef *v1beta1.NetworkPolicyReference


it has a different meaning
which are the metadata fields of the corresponding controlplane NetworkPolicy
it might change in the future

done, thanks!

abhiraut · 2020-09-21T21:57:08Z

@jianjuns @abhiraut I'm going to update PR's description to reflect the latest compatibility impact, posting previous impact and proposal here:

Compatibility impact if we change controlplane NetworkPolicy to cluster scope and use UID as name directly:

This change is backward-compatible for antrea-agent.
Older antrea-agents can communicate with new antrea-controller as nothing has changed to the rule and antrea-agents never filtered policies by namespace.
A few logging and messages of Traceflow observations will be impacted as "Namespace" is no longer set and "Name" is changed to the UID of the original NetworkPolicy, for example:
# the original log
NetworkPolicy default/allow-dns applied to Pods on this Node
# older agents would become
NetworkPolicy /561e923c-a9c6-43b6-80d8-fda1427e4b98 applied to Pods on this Node
$ new agents would become
NetworkPolicy K8sNetworkPolicy:default/allow-dns applied to Pods on this Node
It's not completely backward-compatible for antctl. Filtering by namespace and/or name will be broken because of the namespace scope change.
# antctl-0.9.3 get netpol
NAMESPACE NAME                                 APPLIED-TO                           RULES
<NONE>    561e923c-a9c6-43b6-80d8-fda1427e4b98 c4c59cfe-9160-5de5-a85b-01a58d11963e 1
# antctl-0.9.3 get netpol 561e923c-a9c6-43b6-80d8-fda1427e4b98
Error: an empty namespace may not be set when a resource name is provided
# antctl-0.9.3 get netpol -n default
Error: Not Found: the server could not find the requested resource
If we want to keep complete backward-compatibility for all clients, maybe we could do the following:

For this release, we only add sourceRef field to v1beta1 and do not make any change to the content of namespace, name, and namespace scope. So features that need to know kinds of NetworkPolicies can get what they need without any other clients affected.

In next release, we add v1beta2 version that changes the resource scope to cluster and use the original UID as the resource's name.

After v1beta1 clients are out of support windows, we remove v1beta1.

I was really hoping we could fix the issue in this release but i understand that it may not be enough time for us to implement v1beta2 versioning here.. lets target it for 0.11 .. in the middle of reviewing this PR

abhiraut

one question. lgtm otherwise

abhiraut · 2020-09-21T21:59:33Z

pkg/agent/controller/networkpolicy/cache.go

 func (r *CompletedRule) isAntreaNetworkPolicyRule() bool {
-	return r.PolicyPriority != nil
+	return r.SourceRef.Type != v1beta1.K8sNetworkPolicy


do we need to be careful in terms of using this field right after upgrade .. ie what happens if the controller was not yet upgraded and agent was ? i.e. is sourceRef guaranteed to be not nil in such cases?

If upgrading by applying yaml, the old controller pod should be removed immediately so there should be no chance that new agent can talk to old controller. For other integrated solutions, we also assume controller is upgraded first. We may need a doc like https://kubernetes.io/docs/setup/release/version-skew-policy/#supported-component-upgrade-order to document it officially. It could be a part of #1273 which is tracking the version policy.

abhiraut

i think we recommend controller upgrade first followed by agents.. so we should be good

NetworkPolicy in controlplane API group is the object that consumed by antrea-agents. Both Antrea NetworkPolicy and K8s NetworkPolicy will be converted to it. Currently, the namespace and name of the original NetworkPolicy are copied to the controlplane NetworkPolicy and <Namespace>/<Name> is used as the key func. Therefore, one K8s NetworkPolicy may overwrite the controlplane NetworkPolicy mapping to a Antrea NetworkPolicy that has the same namespace and name. To fix it completely, the original NetworkPolicy name will not be used as the controlplane NetworkPolicy name directly. However, antrea-agents still need to know the information about the original NetworkPolicy for multiple purposes, e.g. metrics and realization status report, and traceflow observation correlation. To keep backward compatibility, this patch adds a new field "SourceRef" that includes the information about the original NetworkPolicy to the struct of controlplane NetworkPolicy. Instead of assuming controlplane NetworkPolicy has the same name as the original NetworkPolicy, antrea-agents consumes the new field to get the information about the original NetworkPolicy.

tnqn · 2020-09-22T02:46:57Z

/test-all

jianjuns

LGTM

tnqn · 2020-09-22T04:08:06Z

/test-windows-conformance
/test-windows-networkpolicy

lzhecheng · 2020-09-22T04:48:59Z

/test-windows-conformance

tnqn · 2020-09-22T06:44:11Z

/test-windows-networkpolicy

tnqn · 2020-09-22T07:15:45Z

/test-windows-networkpolicy

vmwclabot added the cla-not-required label Sep 16, 2020

tnqn added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API. label Sep 16, 2020

tnqn force-pushed the np-ref branch from 6385292 to 904615f Compare September 16, 2020 14:15

tnqn force-pushed the np-ref branch 2 times, most recently from dba53fe to 1e09de0 Compare September 16, 2020 15:11

tnqn requested review from Dyanngg, abhiraut, jianjuns and antoninbas September 16, 2020 15:12

tnqn added this to the Antrea v0.10.0 release milestone Sep 16, 2020

tnqn force-pushed the np-ref branch from 1e09de0 to f7ba9bb Compare September 16, 2020 16:08

tnqn mentioned this pull request Sep 16, 2020

Add NetworkPolicy metrics API and controlplane stats API #1221

Merged

abhiraut previously approved these changes Sep 16, 2020

View reviewed changes

pkg/controller/networkpolicy/antreanetworkpolicy_test.go Outdated Show resolved Hide resolved

pkg/controller/types/networkpolicy.go Outdated Show resolved Hide resolved

Dyanngg reviewed Sep 17, 2020

View reviewed changes

tnqn dismissed abhiraut’s stale review via a3a1873 September 17, 2020 07:28

tnqn force-pushed the np-ref branch 4 times, most recently from c9a9367 to 0633e8f Compare September 17, 2020 09:18

jianjuns reviewed Sep 17, 2020

View reviewed changes

Dyanngg mentioned this pull request Sep 18, 2020

[Antctl]: Support commands to query AntreaPolicy types (CNP/ANP) #927

Closed

tnqn force-pushed the np-ref branch 2 times, most recently from 2da539d to 3fa5ac8 Compare September 18, 2020 09:56

tnqn force-pushed the np-ref branch 3 times, most recently from b117ec1 to 9bec8cd Compare September 18, 2020 15:02

tnqn changed the title ~~Fix name collision between Antrea NetworkPolicy and K8s NetworkPolicy~~ Add NetworkPolicyReference to controlplane NetworkPolicy Sep 18, 2020

antoninbas previously approved these changes Sep 21, 2020

View reviewed changes

abhiraut reviewed Sep 21, 2020

View reviewed changes

abhiraut previously approved these changes Sep 21, 2020

View reviewed changes

tnqn dismissed stale reviews from abhiraut and antoninbas via 931320f September 22, 2020 02:28

tnqn force-pushed the np-ref branch from 9bec8cd to 931320f Compare September 22, 2020 02:28

jianjuns approved these changes Sep 22, 2020

View reviewed changes

antoninbas approved these changes Sep 22, 2020

View reviewed changes

Dyanngg approved these changes Sep 22, 2020

View reviewed changes

tnqn merged commit fa893b9 into antrea-io:master Sep 22, 2020

antoninbas mentioned this pull request Sep 23, 2020

Antrea NetworkPolicy and K8s NetworkPolicy collides if have the same name and namespace #1173

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add NetworkPolicyReference to controlplane NetworkPolicy #1258

Add NetworkPolicyReference to controlplane NetworkPolicy #1258

tnqn commented Sep 16, 2020 •

edited

Loading

antrea-bot commented Sep 16, 2020

codecov-commenter commented Sep 16, 2020 •

edited

Loading

tnqn commented Sep 16, 2020

abhiraut left a comment

abhiraut commented Sep 16, 2020

Dyanngg Sep 17, 2020

tnqn Sep 17, 2020

Dyanngg Sep 17, 2020

abhiraut Sep 17, 2020

tnqn Sep 18, 2020

tnqn commented Sep 17, 2020

jianjuns left a comment

abhiraut commented Sep 17, 2020

tnqn commented Sep 18, 2020

tnqn commented Sep 18, 2020 •

edited

Loading

tnqn commented Sep 18, 2020

jianjuns commented Sep 18, 2020

antoninbas commented Sep 18, 2020

tnqn commented Sep 21, 2020

antoninbas commented Sep 21, 2020

antoninbas left a comment

antoninbas Sep 21, 2020

tnqn Sep 22, 2020

abhiraut commented Sep 21, 2020

abhiraut left a comment

abhiraut Sep 21, 2020

tnqn Sep 22, 2020

abhiraut left a comment

tnqn commented Sep 22, 2020

jianjuns left a comment

tnqn commented Sep 22, 2020

lzhecheng commented Sep 22, 2020

tnqn commented Sep 22, 2020

tnqn commented Sep 22, 2020

Add NetworkPolicyReference to controlplane NetworkPolicy #1258

Add NetworkPolicyReference to controlplane NetworkPolicy #1258

Conversation

tnqn commented Sep 16, 2020 • edited Loading

antrea-bot commented Sep 16, 2020

codecov-commenter commented Sep 16, 2020 • edited Loading

Codecov Report

tnqn commented Sep 16, 2020

abhiraut left a comment

Choose a reason for hiding this comment

abhiraut commented Sep 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn commented Sep 17, 2020

jianjuns left a comment

Choose a reason for hiding this comment

abhiraut commented Sep 17, 2020

tnqn commented Sep 18, 2020

tnqn commented Sep 18, 2020 • edited Loading

tnqn commented Sep 18, 2020

jianjuns commented Sep 18, 2020

antoninbas commented Sep 18, 2020

tnqn commented Sep 21, 2020

antoninbas commented Sep 21, 2020

antoninbas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhiraut commented Sep 21, 2020

abhiraut left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhiraut left a comment

Choose a reason for hiding this comment

tnqn commented Sep 22, 2020

jianjuns left a comment

Choose a reason for hiding this comment

tnqn commented Sep 22, 2020

lzhecheng commented Sep 22, 2020

tnqn commented Sep 22, 2020

tnqn commented Sep 22, 2020

tnqn commented Sep 16, 2020 •

edited

Loading

codecov-commenter commented Sep 16, 2020 •

edited

Loading

tnqn commented Sep 18, 2020 •

edited

Loading