Add EgressGroup API and Controller #1965

tnqn · 2021-03-17T16:01:47Z

This patch adds a controlplane API which provides List, Get, and Watch interface for EgressGroups. antrea-agents consume the API to get the Pods to which an Egress applies. Each agent only receives Pods running on its own Node.

For #1924

codecov-io · 2021-03-25T12:15:19Z

Codecov Report

Merging #1965 (0cdfef9) into main (46a2fc5) will decrease coverage by 0.50%.
The diff coverage is 20.85%.

@@            Coverage Diff             @@
##             main    #1965      +/-   ##
==========================================
- Coverage   65.40%   64.89%   -0.51%     
==========================================
  Files         197      201       +4     
  Lines       17192    17428     +236     
==========================================
+ Hits        11244    11310      +66     
- Misses       4778     4945     +167     
- Partials     1170     1173       +3

Flag	Coverage Δ
kind-e2e-tests	`56.10% <20.85%> (-0.36%)`	⬇️
unit-tests	`41.84% <0.00%> (-0.02%)`	⬇️

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

Impacted Files	Coverage Δ
pkg/apis/controlplane/v1beta1/conversion.go	`81.00% <ø> (ø)`
pkg/controller/egress/store/egressgroup.go	`0.00% <0.00%> (ø)`
pkg/controller/types/networkpolicy.go	`100.00% <ø> (ø)`
...piserver/registry/controlplane/egressgroup/rest.go	`15.38% <15.38%> (ø)`
pkg/controller/egress/controller.go	`26.04% <26.04%> (ø)`
pkg/apis/egress/v1alpha1/register.go	`75.00% <75.00%> (ø)`
pkg/apis/controlplane/register.go	`90.00% <100.00%> (+1.76%)`	⬆️
pkg/apis/controlplane/v1beta1/register.go	`94.11% <100.00%> (+1.26%)`	⬆️
pkg/apiserver/apiserver.go	`87.61% <100.00%> (+0.36%)`	⬆️
... and 6 more

jianjuns · 2021-04-01T20:40:20Z

build/yamls/base/crds.yml

+      served: true
+      storage: true
+      schema:
+        openAPIV3Schema:


We should add some validation here? Like SNAT IP is required, and its format.

Sure, I added this only for testing. Do you plan to include it in #1433? I think either that one or the last patch making the whole feature work should include it.

I plan to merge CRD types first. What you think?

jianjuns · 2021-04-01T20:45:13Z

pkg/controller/types/egress.go

+)
+
+// EgressGroup describes a set of GroupMembers to apply Egress to.
+type EgressGroup struct {


We cannot share a group struct with NetworkPolicy? If in future that is possible, maybe give a generic name for it, like AppliedGroup?

It could share it with NetworkPolicy. I didn't do it to avoid touching NetworkPolicy stuff in this PR. If you prefer, I could move AppliedToGroup from networkpolicy.go to group.go in this PR and reuse it. You mean AppliedGroup or AppliedToGroup?

I meant AppliedToGroup. If you want to reduce the change, we can refactor later - not much time left for this release.

Do you think we can rename it to AppliedToGroup if later we want to unify with NetworkPolicy AppliedToGroup?

Yes, it can. Should we do it when unifying with NetworkPolicy AppliedToGroup to avoid confusion that the API is named EgressGroup while its stored struct is AppliedToGroup?

How about adding a comment here.

jianjuns · 2021-04-01T23:41:01Z

pkg/controller/egress/store/egressgroup.go

+	// The previous version of the stored EgressGroup.
+	PrevGroup *types.EgressGroup
+	// The current version of the transferred EgressGroup, which will be used in Added events.
+	CurrObject *controlplane.EgressGroup


I might have missed something, but I did not see CurrObject/PrevObject are used in the code of this PR?

Thanks for catching it. They are not needed. I forgot to remove them, so do PatchObject.

jianjuns · 2021-04-02T01:16:19Z

pkg/controller/egress/controller.go

+		// install its SNAT rule right after the Pod's CNI request is processed, which just requires a notification from
+		// CNIServer to the agent's EgressController. However the current notification mechanism (the entityUpdate
+		// channel) allows only single consumer. Once it allows multiple consumers, we can change the condition to
+		// include scheduled Pods that have no IPs.


Sounds great!

jianjuns · 2021-04-02T01:21:32Z

pkg/controller/egress/controller.go

+	curEgress := cur.(*egressv1alpha1.Egress)
+	klog.Infof("Processing Egress %s UPDATE event with selector (%s)", curEgress.Name, curEgress.Spec.AppliedTo)
+	// Do nothing if AppliedTo doesn't change.
+	if reflect.DeepEqual(oldEgress.Spec.AppliedTo, curEgress.Spec.AppliedTo) {


Do you think we can optimize by defining our own comparison funcs for AppliedTo?

Sure, will do

We can do later.

tnqn

@jianjuns thanks for reviewing.

tnqn · 2021-04-02T02:08:00Z

build/yamls/base/crds.yml

+      served: true
+      storage: true
+      schema:
+        openAPIV3Schema:


Sure, I added this only for testing. Do you plan to include it in #1433? I think either that one or the last patch making the whole feature work should include it.

tnqn · 2021-04-02T02:13:01Z

pkg/controller/types/egress.go

+)
+
+// EgressGroup describes a set of GroupMembers to apply Egress to.
+type EgressGroup struct {


It could share it with NetworkPolicy. I didn't do it to avoid touching NetworkPolicy stuff in this PR. If you prefer, I could move AppliedToGroup from networkpolicy.go to group.go in this PR and reuse it. You mean AppliedGroup or AppliedToGroup?

tnqn · 2021-04-02T02:16:06Z

pkg/controller/egress/store/egressgroup.go

+	// The previous version of the stored EgressGroup.
+	PrevGroup *types.EgressGroup
+	// The current version of the transferred EgressGroup, which will be used in Added events.
+	CurrObject *controlplane.EgressGroup


Thanks for catching it. They are not needed. I forgot to remove them, so do PatchObject.

tnqn · 2021-04-02T02:18:50Z

pkg/controller/egress/controller.go

+	curEgress := cur.(*egressv1alpha1.Egress)
+	klog.Infof("Processing Egress %s UPDATE event with selector (%s)", curEgress.Name, curEgress.Spec.AppliedTo)
+	// Do nothing if AppliedTo doesn't change.
+	if reflect.DeepEqual(oldEgress.Spec.AppliedTo, curEgress.Spec.AppliedTo) {


Sure, will do

tnqn · 2021-04-05T17:45:44Z

/test-all

This patch adds a controlplane API which provides List, Get, and Watch interface for EgressGroups. antrea-agents consume the API to get the Pods to which an Egress applies. Each agent only receives Pods running on its own Node.

tnqn · 2021-04-05T18:11:54Z

/test-all

tnqn · 2021-04-06T02:02:14Z

/test-integration
/test-windows-e2e
/test-windows-conformance

vmwclabot added the cla-not-required label Mar 17, 2021

tnqn force-pushed the egress-controlplane branch from 6655a8f to 178405b Compare March 17, 2021 16:08

tnqn mentioned this pull request Mar 18, 2021

Egress Policy v1alpha1 implementation #1924

Closed

5 tasks

tnqn force-pushed the egress-controlplane branch 2 times, most recently from 67a05e1 to 8d715b1 Compare March 25, 2021 10:22

tnqn force-pushed the egress-controlplane branch from 8d715b1 to 0cdfef9 Compare March 25, 2021 10:45

tnqn force-pushed the egress-controlplane branch 3 times, most recently from 6e22f26 to d8dab17 Compare March 26, 2021 16:01

antoninbas added this to the Antrea v1.0 release milestone Mar 30, 2021

tnqn force-pushed the egress-controlplane branch 3 times, most recently from 3d50cdd to 0f6830c Compare April 1, 2021 11:07

tnqn changed the title ~~[WIP] Add EgressGroup API and Controller~~ Add EgressGroup API and Controller Apr 1, 2021

tnqn marked this pull request as ready for review April 1, 2021 11:10

tnqn force-pushed the egress-controlplane branch 2 times, most recently from 7e1326d to 27747ee Compare April 1, 2021 11:28

jianjuns requested review from jianjuns and Dyanngg April 1, 2021 18:32

jianjuns reviewed Apr 2, 2021

View reviewed changes

tnqn commented Apr 2, 2021

View reviewed changes

tnqn force-pushed the egress-controlplane branch 2 times, most recently from cc9483e to cc651cb Compare April 5, 2021 15:11

jianjuns previously approved these changes Apr 5, 2021

View reviewed changes

tnqn dismissed jianjuns’s stale review via 93cf251 April 5, 2021 17:45

tnqn force-pushed the egress-controlplane branch from cc651cb to 93cf251 Compare April 5, 2021 17:45

Add EgressGroup API and Controller

73f53ca

This patch adds a controlplane API which provides List, Get, and Watch interface for EgressGroups. antrea-agents consume the API to get the Pods to which an Egress applies. Each agent only receives Pods running on its own Node.

tnqn force-pushed the egress-controlplane branch from 93cf251 to 73f53ca Compare April 5, 2021 17:56

jianjuns approved these changes Apr 5, 2021

View reviewed changes

tnqn merged commit 5752f96 into antrea-io:main Apr 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add EgressGroup API and Controller #1965

Add EgressGroup API and Controller #1965

tnqn commented Mar 17, 2021 •

edited

Loading

codecov-io commented Mar 25, 2021

jianjuns Apr 1, 2021

tnqn Apr 2, 2021

jianjuns Apr 2, 2021

jianjuns Apr 1, 2021

tnqn Apr 2, 2021

jianjuns Apr 2, 2021

jianjuns Apr 5, 2021

tnqn Apr 5, 2021

jianjuns Apr 5, 2021

tnqn Apr 5, 2021

jianjuns Apr 1, 2021

tnqn Apr 2, 2021

jianjuns Apr 2, 2021

jianjuns Apr 2, 2021

tnqn Apr 2, 2021

jianjuns Apr 2, 2021

tnqn left a comment

tnqn Apr 2, 2021

tnqn Apr 2, 2021

tnqn Apr 2, 2021

tnqn Apr 2, 2021

tnqn commented Apr 5, 2021

tnqn commented Apr 5, 2021

tnqn commented Apr 6, 2021

Add EgressGroup API and Controller #1965

Add EgressGroup API and Controller #1965

Conversation

tnqn commented Mar 17, 2021 • edited Loading

codecov-io commented Mar 25, 2021

Codecov Report

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

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

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

tnqn commented Apr 5, 2021

tnqn commented Apr 5, 2021

tnqn commented Apr 6, 2021

tnqn commented Mar 17, 2021 •

edited

Loading