Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add EgressGroup API and Controller #1965

Merged
merged 1 commit into from
Apr 6, 2021
Merged

Conversation

tnqn
Copy link
Member

@tnqn tnqn commented Mar 17, 2021

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

Codecov Report

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

Impacted file tree graph

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

@tnqn tnqn force-pushed the egress-controlplane branch 3 times, most recently from 6e22f26 to d8dab17 Compare March 26, 2021 16:01
@antoninbas antoninbas added this to the Antrea v1.0 release milestone Mar 30, 2021
@tnqn tnqn force-pushed the egress-controlplane branch 3 times, most recently from 3d50cdd to 0f6830c Compare April 1, 2021 11:07
@tnqn tnqn changed the title [WIP] Add EgressGroup API and Controller Add EgressGroup API and Controller Apr 1, 2021
@tnqn tnqn marked this pull request as ready for review April 1, 2021 11:10
@tnqn tnqn force-pushed the egress-controlplane branch 2 times, most recently from 7e1326d to 27747ee Compare April 1, 2021 11:28
served: true
storage: true
schema:
openAPIV3Schema:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

)

// EgressGroup describes a set of GroupMembers to apply Egress to.
type EgressGroup struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about adding a comment here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

// 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds great!

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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, will do

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can do later.

Copy link
Member Author

@tnqn tnqn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jianjuns thanks for reviewing.

served: true
storage: true
schema:
openAPIV3Schema:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

)

// EgressGroup describes a set of GroupMembers to apply Egress to.
type EgressGroup struct {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

// 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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, will do

@tnqn tnqn force-pushed the egress-controlplane branch 2 times, most recently from cc9483e to cc651cb Compare April 5, 2021 15:11
jianjuns
jianjuns previously approved these changes Apr 5, 2021
@tnqn
Copy link
Member Author

tnqn commented Apr 5, 2021

/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
Copy link
Member Author

tnqn commented Apr 5, 2021

/test-all

@tnqn
Copy link
Member Author

tnqn commented Apr 6, 2021

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

@tnqn tnqn merged commit 5752f96 into antrea-io:main Apr 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants