ToServices support #2755

GraysonWu · 2021-09-12T22:29:55Z

This PR adds ToServices feature, which allows users to apply an ACNP/ANP on a Service.
Use this PR to replace PR #2538 with the new approach brought up in PR #2538.
The new approach will use groupID assigned to the Service by AntreaProxy to match traffic, which means that ToServices can only be used when AntreaProxy is enabled and this Service must have at least one clusterIP.
Also, this implementation use groupID to match traffic, thus the policies will not be enforced when appliedTo workloads connect to Service Endpoints directly. In order to enforce policies on directly Endpoints traffic, one fallback is using ClusterGroup with ServiceReference added in PR #1797.

Add ToServices field in ACNP and ANP.
In ServiceLBTable, load OVS groupID to reg7.
Use OVS groupID of Service to do the conj match in the egress table.
Add a channel between proxier and networkpolicy controller for Service groupID update events.

Signed-off-by: wgrayson wgrayson@vmware.com

codecov-commenter · 2021-09-12T23:11:37Z

Codecov Report

Merging #2755 (59205cf) into main (8336f7a) will increase coverage by 0.05%.
The diff coverage is 80.58%.

@@            Coverage Diff             @@
##             main    #2755      +/-   ##
==========================================
+ Coverage   61.59%   61.64%   +0.05%     
==========================================
  Files         283      283              
  Lines       23644    23729      +85     
==========================================
+ Hits        14563    14628      +65     
- Misses       7512     7524      +12     
- Partials     1569     1577       +8

Flag	Coverage Δ
kind-e2e-tests	`49.29% <62.13%> (+0.03%)`	⬆️
unit-tests	`40.88% <50.63%> (+0.03%)`	⬆️

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

Impacted Files	Coverage Δ
pkg/agent/types/networkpolicy.go	`83.33% <ø> (ø)`
pkg/apis/controlplane/types.go	`0.00% <ø> (ø)`
pkg/controller/networkpolicy/validate.go	`21.19% <0.00%> (-0.33%)`	⬇️
pkg/agent/proxy/proxier.go	`60.20% <40.00%> (ø)`
pkg/agent/controller/networkpolicy/cache.go	`86.54% <76.92%> (-0.44%)`	⬇️
pkg/agent/controller/networkpolicy/reconciler.go	`76.98% <76.92%> (-0.31%)`	⬇️
pkg/agent/openflow/network_policy.go	`83.19% <80.00%> (-0.03%)`	⬇️
pkg/agent/proxy/types/groupcounter.go	`94.33% <91.66%> (+4.33%)`	⬆️
...ntroller/networkpolicy/networkpolicy_controller.go	`71.25% <100.00%> (ø)`
pkg/agent/openflow/pipeline.go	`78.33% <100.00%> (+0.06%)`	⬆️
... and 13 more

pkg/controller/networkpolicy/validate.go

antoninbas

The commit message / PR description should mention the "caveat" of this approach: the policies will not be enforced when appliedTo workloads connect to Service Endpoints directly.

pkg/apis/crd/v1alpha1/types.go

pkg/controller/networkpolicy/validate.go

GraysonWu · 2021-09-23T21:13:12Z

Updated the commit message and the PR description.

pkg/agent/controller/networkpolicy/cache.go

pkg/agent/openflow/pipeline.go

pkg/apis/controlplane/types.go

pkg/apis/controlplane/v1beta2/types.go

pkg/controller/networkpolicy/mutate.go

pkg/controller/networkpolicy/validate.go

docs/antrea-network-policy.md

abhiraut · 2021-10-01T18:09:50Z

pkg/apis/crd/v1alpha1/types.go

+type ServiceReference struct {
+	// Name of the Service
+	Name string `json:"name,omitempty"`
+	// Namespace of the Service


maybe add the requirements of these? Name is required. Namespace is required for ACNP usage

ACNP and ANP both use this struct. I'm not sure how to do "Namespace is required for ACNP usage". But I have this requirement set in crds.yml.

pkg/controller/networkpolicy/mutate.go

tnqn

The commit message format wouldn't look nice in many git tools. Could you follow the above link to improve it? More specifically, please wrap the body at 72 characters.

pkg/agent/proxy/types/groupcounter.go

docs/antrea-network-policy.md

tnqn

Code LGTM. I suggest to remove the part that talks about the old approach from the commit message as it's never merged to main branch and could confuse people to think the PR is updating the implementation of a feature that was implemented in another way. The commit message could just focus on what this PR does.

This PR adds ToServices feature, which allows users to apply an ACNP/ANP on a Service. This PR uses groupID assigned to the Service by AntreaProxy to match traffic, which means that ToServices can only be used when AntreaProxy is enabled and this Service must have at least one clusterIP. Also, this PR use groupID to match traffic, thus the policies will not be enforced when appliedTo workloads connect to Service Endpoints directly. In order to enforce policies on directly Endpoints traffic, one fallback is using ClusterGroup with ServiceReference added in PR antrea-io#1797. What this PR did: 1. Add ToServices field in ACNP and ANP. 2. In ServiceLBTable, load OVS groupID to reg7. 3. Use OVS groupID of Service to do the conj match in the egress table. 4. Add a channel between proxier and networkpolicy controller for Service groupID update events. Signed-off-by: wgrayson <wgrayson@vmware.com>

GraysonWu · 2021-10-20T08:54:58Z

@tnqn Good suggestion. Commit message changed.

tnqn

LGTM

tnqn · 2021-10-20T09:02:09Z

/test-all

tnqn · 2021-10-20T16:02:44Z

e2e failed:

E1020 14:14:40.818524 16403 antreapolicy_test.go:101] ClusterNetworkPolicy.crd.antrea.io "test-log-acnp-deny" is invalid: spec.egress.toServices: Invalid value: "null": spec.egress.toServices in body must be of type array: "null"

GraysonWu · 2021-10-20T22:15:40Z

@tnqn The reason for those failures is that before k8s v1.20 null values for fields that don't specify the nullable flag won't be pruned. So if we use ToServices with a null value that is not of type array, it will be a problem. Starting from k8s v1.20, those fields will be pruned. Details: https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#defaulting-and-nullable

So I pushed a commit making ToServices field omitempty. In this case, we can prune ToServices with a null value during struct->json process.

We didn't meet this issue on to/from field before while to/from is not omitempty, because in our e2e test framework we set 'to/from' to an empty array instead of a null value. Maybe we should also consider making to/from omitempty in the future.

Thanks for @antoninbas 's help during the investigation.

GraysonWu · 2021-10-20T22:16:43Z

/test-all
/test-ipv6-e2e
/test-ipv6-only-e2e

GraysonWu · 2021-10-21T00:43:18Z

/test-all
/test-ipv6-e2e
/test-ipv6-only-e2e

Signed-off-by: wgrayson <wgrayson@vmware.com>

GraysonWu · 2021-10-21T02:04:05Z

/test-all
/test-ipv6-e2e
/test-ipv6-only-e2e

tnqn

LGTM

GraysonWu · 2021-10-21T05:58:16Z

/test-ipv6-e2e
/test-ipv6-only-e2e

GraysonWu mentioned this pull request Sep 12, 2021

ToService support [deprecated] #2538

Closed

GraysonWu force-pushed the to-services branch 5 times, most recently from cab7532 to 6606fb6 Compare September 16, 2021 02:26

tnqn reviewed Sep 16, 2021

View reviewed changes

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

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

GraysonWu force-pushed the to-services branch from 6606fb6 to 58bbc3a Compare September 16, 2021 18:57

GraysonWu requested review from abhiraut, tnqn, antoninbas and jianjuns September 16, 2021 18:59

GraysonWu force-pushed the to-services branch 5 times, most recently from 064c606 to 4f4ea69 Compare September 22, 2021 23:19

antoninbas reviewed Sep 23, 2021

View reviewed changes

pkg/apis/crd/v1alpha1/types.go Outdated Show resolved Hide resolved

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

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

GraysonWu force-pushed the to-services branch from 4f4ea69 to 96335a1 Compare September 23, 2021 21:12

GraysonWu force-pushed the to-services branch 2 times, most recently from 195d2ee to 7e2299d Compare September 30, 2021 01:33

GraysonWu requested a review from antoninbas September 30, 2021 01:34

GraysonWu force-pushed the to-services branch from 7e2299d to fa5820f Compare September 30, 2021 03:07

tnqn reviewed Sep 30, 2021

View reviewed changes

abhiraut reviewed Oct 1, 2021

View reviewed changes

GraysonWu force-pushed the to-services branch 2 times, most recently from b9df198 to b38153d Compare October 2, 2021 00:40

GraysonWu requested a review from tnqn October 2, 2021 00:40

GraysonWu force-pushed the to-services branch from ac3d046 to 696cfa5 Compare October 19, 2021 18:53

GraysonWu requested a review from tnqn October 19, 2021 19:09

GraysonWu force-pushed the to-services branch 2 times, most recently from 55044ec to e73b47f Compare October 19, 2021 22:21

tnqn reviewed Oct 20, 2021

View reviewed changes

pkg/agent/proxy/types/groupcounter.go Outdated Show resolved Hide resolved

docs/antrea-network-policy.md Outdated Show resolved Hide resolved

GraysonWu force-pushed the to-services branch from e73b47f to 2755fe3 Compare October 20, 2021 08:25

GraysonWu requested a review from tnqn October 20, 2021 08:26

GraysonWu force-pushed the to-services branch from 2755fe3 to b5865e9 Compare October 20, 2021 08:35

tnqn reviewed Oct 20, 2021

View reviewed changes

GraysonWu force-pushed the to-services branch from b5865e9 to d955364 Compare October 20, 2021 08:53

tnqn previously approved these changes Oct 20, 2021

View reviewed changes

GraysonWu dismissed tnqn’s stale review via fe086ee October 20, 2021 21:50

GraysonWu requested a review from tnqn October 20, 2021 22:19

GraysonWu force-pushed the to-services branch from fe086ee to d994630 Compare October 21, 2021 00:37

Make ToServices omitempty

59205cf

Signed-off-by: wgrayson <wgrayson@vmware.com>

GraysonWu force-pushed the to-services branch from d994630 to 59205cf Compare October 21, 2021 01:08

tnqn approved these changes Oct 21, 2021

View reviewed changes

tnqn merged commit aec3e8b into antrea-io:main Oct 21, 2021

GraysonWu mentioned this pull request Oct 22, 2021

Make to/from field omitempty #2925

Closed

tnqn mentioned this pull request Feb 14, 2022

Add service account selector support in ACNP #3044

Merged

hongliangl mentioned this pull request Nov 1, 2023

Report NetworkPolicy destined for Services as invalid when proxyAll is disabled #5591

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ToServices support #2755

ToServices support #2755

GraysonWu commented Sep 12, 2021 •

edited

Loading

codecov-commenter commented Sep 12, 2021 •

edited

Loading

antoninbas left a comment

GraysonWu commented Sep 23, 2021

abhiraut Oct 1, 2021

GraysonWu Oct 2, 2021

tnqn left a comment

tnqn left a comment

GraysonWu commented Oct 20, 2021 •

edited

Loading

tnqn left a comment

tnqn commented Oct 20, 2021

tnqn commented Oct 20, 2021

GraysonWu commented Oct 20, 2021

GraysonWu commented Oct 20, 2021

GraysonWu commented Oct 21, 2021

GraysonWu commented Oct 21, 2021

tnqn left a comment

GraysonWu commented Oct 21, 2021

ToServices support #2755

ToServices support #2755

Conversation

GraysonWu commented Sep 12, 2021 • edited Loading

codecov-commenter commented Sep 12, 2021 • edited Loading

Codecov Report

antoninbas left a comment

Choose a reason for hiding this comment

GraysonWu commented Sep 23, 2021

abhiraut Oct 1, 2021

Choose a reason for hiding this comment

GraysonWu Oct 2, 2021

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

GraysonWu commented Oct 20, 2021 • edited Loading

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Oct 20, 2021

tnqn commented Oct 20, 2021

GraysonWu commented Oct 20, 2021

GraysonWu commented Oct 20, 2021

GraysonWu commented Oct 21, 2021

GraysonWu commented Oct 21, 2021

tnqn left a comment

Choose a reason for hiding this comment

GraysonWu commented Oct 21, 2021

GraysonWu commented Sep 12, 2021 •

edited

Loading

codecov-commenter commented Sep 12, 2021 •

edited

Loading

GraysonWu commented Oct 20, 2021 •

edited

Loading