AppliedTo Per Rule for Antrea-native Policies #1396

Dyanngg · 2020-10-16T05:35:43Z

This PR fixes #1327. Please refer to the issue for more information on this feature.

The following changes are made in terms of implementation:

Add admission webhook validation to ensure that the appliedTo is set at either spec level or rule level, but not both.
Add a AppliedToPerRule field in multiple policy rule structs to indicate which level appliedTo was set.
For the internalNP, AppliedToGroups are set as spec.appliedTo or the union of all rule.appliedTo. The original span calculations and stale appliedToGroup cleanups would seamlessly work.
In agent, use the AppliedToPerRule flag to determine appliedTo for each PolicyRule.

antrea-bot · 2020-10-16T05:35:59Z

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-io · 2020-10-16T05:40:19Z

Codecov Report

Merging #1396 (c85e45e) into master (9d3d10b) will decrease coverage by 0.79%.
The diff coverage is 60.67%.

@@            Coverage Diff             @@
##           master    #1396      +/-   ##
==========================================
- Coverage   63.31%   62.52%   -0.80%     
==========================================
  Files         170      181      +11     
  Lines       14250    15499    +1249     
==========================================
+ Hits         9023     9691     +668     
- Misses       4292     4786     +494     
- Partials      935     1022      +87

Flag	Coverage Δ
e2e-tests	`25.88% <21.37%> (?)`
kind-e2e-tests	`53.00% <54.37%> (-2.38%)`	⬇️
unit-tests	`40.66% <29.51%> (-0.62%)`	⬇️

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

Impacted Files	Coverage Δ
cmd/antrea-agent/agent.go	`0.00% <0.00%> (ø)`
.../agent/apiserver/handlers/networkpolicy/handler.go	`58.33% <ø> (ø)`
...gent/controller/noderoute/node_route_controller.go	`45.83% <0.00%> (-0.64%)`	⬇️
pkg/agent/stats/collector.go	`97.72% <ø> (ø)`
pkg/antctl/antctl.go	`100.00% <ø> (ø)`
pkg/antctl/transform/controllerinfo/transform.go	`0.00% <ø> (ø)`
pkg/antctl/transform/version/transform.go	`44.82% <ø> (ø)`
pkg/controller/networkpolicy/tier.go	`90.00% <ø> (ø)`
pkg/controller/types/networkpolicy.go	`100.00% <ø> (ø)`
pkg/features/antrea_features.go	`16.66% <ø> (ø)`
... and 87 more

Dyanngg · 2020-10-29T17:33:22Z

/test-all

antrea-bot · 2020-11-06T00:56:23Z

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-all-features-conformance: to trigger conformance tests with all alpha features enabled.
/skip-all-features-conformance: to skip conformance tests with all alpha features enabled.
/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).

Dyanngg · 2020-12-09T21:46:24Z

/test-all

antoninbas

some comments, but I am not the best person to review NP changes in the controller & agent

pkg/agent/controller/networkpolicy/cache_test.go

pkg/apis/controlplane/types.go

pkg/apis/controlplane/v1beta1/conversion.go

pkg/controller/networkpolicy/antreanetworkpolicy.go

pkg/controller/networkpolicy/clusternetworkpolicy.go

pkg/controller/networkpolicy/validate.go

test/e2e/antreapolicy_test.go

antoninbas · 2020-12-09T21:43:11Z

test/e2e/utils/cnpspecbuilder.go

+	PodSelectorMatchExp *[]metav1.LabelSelectorRequirement
+	NSSelectorMatchExp  *[]metav1.LabelSelectorRequirement


Not really introduced by this PR, but I don't recall why these have to be pointers, while PodSelector / NSSelector do not. Do you know the reason?

I'm not sure... Maybe because Maps are already passed by reference?

that may make sense for function parameters (except if we never mutate the slice), but not really for fields in a struct

maybe you can punt this to the later commit you mentioned in https://github.com/vmware-tanzu/antrea/pull/1396/files#r539671256

Opened #1647 for this

test/e2e/utils/anpspecbuilder.go

Dyanngg · 2020-12-10T06:51:27Z

/test-all

tnqn

LGTM overall

pkg/controller/networkpolicy/validate.go

pkg/agent/controller/networkpolicy/cache.go

Dyanngg · 2020-12-10T21:00:10Z

/test-all

jianjuns · 2020-12-11T01:43:16Z

pkg/controller/networkpolicy/validate.go

+	}
+	numAppliedToInRules := countAppliedToInRules(ingress) + countAppliedToInRules(egress)
+	if appliedToInSpec && (numAppliedToInRules > 0) {
+		return "appliedTo should not be set in both spec and rules", false


Sorry, I must have missed some discussions. But why appliedTo cannot be on both rules and NP?
@abhiraut

i think we decided to keep the implementation simple and not support a hybrid AppliedTo .. initially i thought we could inherit the appliedTo from the spec in to the rule if no AppliedTo set on the rule.. i am not sure if the implementation becomes too complex to support a hybrid setup?

Spoke offline, at this moment we will keep the current implementation and disallow the hybrid case

antoninbas

LGTM, but please get an approval from @abhiraut or @tnqn as well.

antoninbas · 2020-12-11T02:48:25Z

pkg/controller/networkpolicy/store/networkpolicy.go

@@ -111,7 +111,9 @@ func ToNetworkPolicyMsg(in *types.NetworkPolicy, out *controlplane.NetworkPolicy
 	}
 	// Since stored objects are immutable, we just reference the fields here.
 	out.Rules = in.Rules
-	out.AppliedToGroups = in.AppliedToGroups
+	if !in.AppliedToPerRule {


maybe add a small comment for this

Dyanngg · 2020-12-14T20:08:29Z

/test-all

jianjuns · 2020-12-15T01:32:03Z

pkg/apis/security/v1alpha1/types.go

+	// Select workloads on which this rule will be applied to. Cannot be set in
+	// conjunction with NetworkPolicySpec/ClusterNetworkPolicySpec.AppliedTo.
+	// +optional
+	AppliedTo []NetworkPolicyPeer `json:"appliedTo,omitempty"`


I am not suggesting any change for now. But want to mention I would consider to define a separate struct for AppliedTo instead of using NetworkPolicyPeer, that can be shared by different types of policies, and can remove the requirements of validating unsupported fields of NetworkPolicyPeer.
Check my idea here: https://github.com/vmware-tanzu/antrea/pull/1433/files#diff-820e873e5691a9f56ffc35887e7e006745a9ff4b0c547b994687f06a9db3919fR79. @abhiraut

I'm working on integrating the ClusterGroup with ACNP/ANP. the struct you are proposing is similar to a Group/ClusterGroup.. do you want to reuse the same CRD for snat policy?

see https://github.com/vmware-tanzu/antrea/pull/1639/files#diff-29b0b716ba012862166e3d02f34ebf73408821432978f99e8dec1ce4911b0e65R93

I think we want to support multiple groups and maybe mix of groups and selectors for one policy. Then we still need an appliedTo struct for that?

pkg/controller/networkpolicy/antreanetworkpolicy.go

Dyanngg · 2020-12-15T03:52:31Z

/test-all

tnqn

LGTM
But I'd like to confirm that we allowed creating policies that has empty appliedTo before and the change will make there will be at least one appliedTo in either policy spec or all of rule spec, right?

Dyanngg · 2020-12-15T18:29:05Z

LGTM
But I'd like to confirm that we allowed creating policies that has empty appliedTo before and the change will make there will be at least one appliedTo in either policy spec or all of rule spec, right?

Correct. #1359 ensures that for Antrea-native policies, the appliedTo field has to be specified at policy level, though does not require it to not be an empty list. But realistically I believe it makes sense to check that there is at least one appliedTo in either policy spec or all of rule spec.

abhiraut · 2020-12-15T18:53:55Z

btw I am still wondering if we should support appliedTo per rule with regular selectors or keep it for incoming Group CRDs? or allow both? in that case we may need to propose two fields
ie

appliedTo:
  - podSelector: {}

or

appliedTo:
  - group: <groupName>

@jianjuns any thoughts on this?

abhiraut · 2020-12-15T19:00:19Z

btw I am still wondering if we should support appliedTo per rule with regular selectors or keep it for incoming Group CRDs? or allow both? in that case we may need to propose two fields
ie
appliedTo:
  - podSelector: {}
or
appliedTo:
  - group: <groupName>
@jianjuns any thoughts on this?

noticed @jianjuns latest reply.. so we need to support hybrid, in that case maybe we need to formally propose the AppliedTo struct as Jianjun was suggesting.
And I assume we introduce a new "appliedTo" like field for groups then?

jianjuns · 2020-12-15T19:00:42Z

@abhiraut: what is the plan for NP appliedTo? We should NP and rule the same right?
Again do you think we should have a separate AppliedTo struct that includes all selectors and groups?

jianjuns · 2020-12-15T19:01:29Z

Guess I need to understand your ideas first.

abhiraut · 2020-12-15T21:24:59Z

Guess I need to understand your ideas first.

i was thinking of the following, if we want to support both groups and selectors

type AppliedTo struct {
  PodSelector *metav1.LabelSelector
  NamespaceSelector *metav1.LabelSelector
  Group *string // Reference to the Group/ClusterGroup
}

although this would still require some validation on the referenced Group to not have invalid selectors like IPBlock

jianjuns · 2020-12-15T22:27:52Z

For NP appliedTo, do you plan to support both group and selectors?

Another question - do we want to support multiple groups?

abhiraut · 2020-12-16T02:20:24Z

For NP appliedTo, do you plan to support both group and selectors?

Another question - do we want to support multiple groups?

does the following make sense:

type AppliedTo struct {
  PodSelector *metav1.LabelSelector
  NamespaceSelector *metav1.LabelSelector
  Group *string // Reference to the Group/ClusterGroup
}

type NetworkPolicySpec struct {
  AppliedTo []AppliedTo
  // other fields
  ...
}

type Rule struct {
  AppliedTo []AppliedTo
  // other fields
  ...
}

antoninbas

LGTM

I believe any change of type for the AppliedTo fields would belong in a separate PR (requires an API version change).

We should consider adding unit tests for conversion functions as we introduce more to make sure they behave correctly. I believe upstream K8s does the same.

antoninbas · 2020-12-17T19:04:40Z

pkg/apis/controlplane/types.go

@@ -190,6 +191,10 @@ type NetworkPolicyRule struct {
 	// EnableLogging is used to indicate if agent should generate logs
 	// when rules are matched. Should be default to false.
 	EnableLogging bool
+	// AppliedToGroups is a list of names of AppliedToGroups to which this rule applies.
+	// Cannot be set in conjunction with NetworkPolicy.AppliedToGroups of the NetworkPolicy
+	// that this Rule is referred to.


that this Rule belongs to

jianjuns · 2020-12-17T19:16:24Z

@abhiraut I feel "[]AppliedTo" looks strange. ApplitedTo sounds like a singular to include all entities.

Of course the AppliedTo strcut change should not be in this PR (we are just using this PR to discuss and probably let us move to another thread). I think everyone is on the same page here.

* Add appliedTo field in rules and update validationWebook * Add appliedTo per rule handling in controller and agent * Add omitempty to appliedTo fields and address test failures * Add testcases for appliedTo per rule * Address comments

Dyanngg added this to the Antrea v0.11.0 release milestone Oct 16, 2020

Dyanngg requested review from abhiraut, tnqn and jianjuns October 16, 2020 05:35

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

Dyanngg force-pushed the applied-to-per-rule branch from 1e886f1 to 8a1828e Compare October 16, 2020 05:55

Dyanngg force-pushed the applied-to-per-rule branch from 8a1828e to c6e0e63 Compare October 29, 2020 04:57

Dyanngg modified the milestones: Antrea v0.11.0 release, Antrea v0.12.0 release Nov 3, 2020

Dyanngg force-pushed the applied-to-per-rule branch from c6e0e63 to 89d8c3d Compare November 6, 2020 00:56

Dyanngg force-pushed the applied-to-per-rule branch from 89d8c3d to 2405cca Compare November 6, 2020 01:53

Dyanngg force-pushed the applied-to-per-rule branch 4 times, most recently from f09ae26 to 349e3b2 Compare December 8, 2020 23:57

Dyanngg requested a review from antoninbas December 9, 2020 18:46

antoninbas reviewed Dec 9, 2020

View reviewed changes

Dyanngg force-pushed the applied-to-per-rule branch 2 times, most recently from 369a403 to b41f200 Compare December 10, 2020 05:35

tnqn reviewed Dec 10, 2020

View reviewed changes

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

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

pkg/agent/controller/networkpolicy/cache.go Outdated Show resolved Hide resolved

Dyanngg force-pushed the applied-to-per-rule branch 2 times, most recently from 8dfc7ed to 72032c3 Compare December 10, 2020 19:07

Dyanngg mentioned this pull request Dec 10, 2020

E2E improvements and fixes for Antrea-native Policy #1647

Merged

Dyanngg force-pushed the applied-to-per-rule branch from 0310e18 to 3e958d0 Compare December 11, 2020 01:34

jianjuns reviewed Dec 11, 2020

View reviewed changes

antoninbas reviewed Dec 11, 2020

View reviewed changes

Dyanngg added 4 commits December 14, 2020 11:07

Add appliedTo field in rules and update validationWebook

0f42cff

Add appliedTo per rule handling in controller and agent

af0e75a

Add omitempty to appliedTo fields and address test failures

7b12e2c

Add testcases for appliedTo per rule

f6cae81

Dyanngg force-pushed the applied-to-per-rule branch from 3e958d0 to e7e4dbc Compare December 14, 2020 19:25

jianjuns reviewed Dec 15, 2020

View reviewed changes

abhiraut reviewed Dec 15, 2020

View reviewed changes

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

Address comments

c85e45e

Dyanngg force-pushed the applied-to-per-rule branch from e7e4dbc to c85e45e Compare December 15, 2020 03:50

tnqn approved these changes Dec 15, 2020

View reviewed changes

Dyanngg requested a review from abhiraut December 15, 2020 18:29

antoninbas approved these changes Dec 17, 2020

View reviewed changes

Dyanngg merged commit 2f55e19 into antrea-io:master Dec 17, 2020

		PodSelectorMatchExp *[]metav1.LabelSelectorRequirement
		NSSelectorMatchExp *[]metav1.LabelSelectorRequirement

AppliedTo Per Rule for Antrea-native Policies #1396

AppliedTo Per Rule for Antrea-native Policies #1396

Conversation

Dyanngg commented Oct 16, 2020

antrea-bot commented Oct 16, 2020

codecov-io commented Oct 16, 2020 • edited

Codecov Report

Dyanngg commented Oct 29, 2020

antrea-bot commented Nov 6, 2020

Dyanngg commented Dec 9, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dyanngg commented Dec 10, 2020

tnqn left a comment

Choose a reason for hiding this comment

Dyanngg commented Dec 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Dyanngg commented Dec 14, 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

Dyanngg commented Dec 15, 2020

tnqn left a comment

Choose a reason for hiding this comment

Dyanngg commented Dec 15, 2020

abhiraut commented Dec 15, 2020 • edited

abhiraut commented Dec 15, 2020

jianjuns commented Dec 15, 2020

jianjuns commented Dec 15, 2020

abhiraut commented Dec 15, 2020 • edited

jianjuns commented Dec 15, 2020

abhiraut commented Dec 16, 2020

antoninbas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jianjuns commented Dec 17, 2020

codecov-io commented Oct 16, 2020 •

edited

abhiraut commented Dec 15, 2020 •

edited

abhiraut commented Dec 15, 2020 •

edited