Add ExternalIPPool API and update Egress API for failover support #2236

tnqn · 2021-06-03T10:41:57Z

Add ExternalIPPool CRD which defines one or multiple IP sets that can
be used in external network, and the Nodes that can hold the IPs.
Add an optional field "externalIPPool" to Egress spec which specifies
the IP Pool that the EgressIP should be allocated from.

Signed-off-by: Quan Tian qtian@vmware.com

codecov-commenter · 2021-06-03T14:41:06Z

Codecov Report

Merging #2236 (f7391b4) into main (382cd88) will decrease coverage by 21.69%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##             main    #2236       +/-   ##
===========================================
- Coverage   61.95%   40.25%   -21.70%     
===========================================
  Files         276      273        -3     
  Lines       21178    20506      -672     
===========================================
- Hits        13120     8255     -4865     
- Misses       6699    11094     +4395     
+ Partials     1359     1157      -202

Flag	Coverage Δ
kind-e2e-tests	`40.25% <100.00%> (-12.87%)`	⬇️
unit-tests	`?`

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

Impacted Files	Coverage Δ
pkg/apis/crd/v1alpha2/register.go	`85.71% <100.00%> (+2.38%)`	⬆️
pkg/agent/controller/egress/id_allocator.go	`0.00% <0.00%> (-100.00%)`	⬇️
pkg/controller/networkpolicy/crd_utils.go	`0.00% <0.00%> (-91.57%)`	⬇️
pkg/controller/networkpolicy/endpoint_querier.go	`2.85% <0.00%> (-88.58%)`	⬇️
...agent/flowexporter/connections/deny_connections.go	`7.14% <0.00%> (-85.72%)`	⬇️
pkg/controller/networkpolicy/status_controller.go	`0.00% <0.00%> (-85.17%)`	⬇️
...kg/controller/networkpolicy/antreanetworkpolicy.go	`0.00% <0.00%> (-85.00%)`	⬇️
pkg/controller/egress/controller.go	`0.00% <0.00%> (-83.88%)`	⬇️
pkg/agent/util/iptables/lock.go	`0.00% <0.00%> (-81.82%)`	⬇️
pkg/agent/controller/networkpolicy/priority.go	`8.49% <0.00%> (-81.70%)`	⬇️
... and 134 more

tnqn · 2021-06-03T15:20:00Z

/test-all

pkg/apis/crd/v1alpha2/types.go

jianjuns

A question on whether we always enable Node assignment or not.

jianjuns · 2021-06-07T18:07:00Z

pkg/apis/crd/v1alpha2/types.go

+type ExternalIPPoolSpec struct {
+	// The IP ranges of this IP pool, e.g. 10.10.0.0/24, 10.10.10.2-10.10.10.20, 10.10.10.30-10.10.10.30.
+	IPRanges []IPRange `json:"ipRanges"`
+	// The Nodes that the external IPs can be assigned to. If empty, it means all Nodes.


If it is nil, do you think we can say it means auto-assignement is not enabled?

I feel it wouldn't be very useful in practice. It seems if users want to manage the egress IPs themselves, providing an ExternalIPPool here and associate it with the Egresses is not as convenient as just specifying the IP in Egress directly. Some benefits when using nil to represent all Nodes in my mind:

For a cluster that all nodes are in same subnet, it's more convinient to use nil to select all Nodes as the Egress Nodes. I don't know a typical expression that selects all Nodes without adding labels to them. (unless using NotIn and DoesNotExist with some meaningless key/value)

Have a clear boundary between enabling assignment/failover and disabling it by the externalIPPool field.

Keep backwards-compatible with the previous static Egresses.

What you said make sense to me. But do you think it is a convention to use empty selector (but not nil) to express selecting all?

It seems that it's quite unusual for a nil selector to mean "select all". Is there an example in the K8s API of such behavior? On the other hand, it seems that empty (but not nil) to select all is fairly common. My preference if we do not think there will be other mechanisms / fields for selection would be to make this field not optional, with empty meaning "select all" (like the PodSelector field in the NetworkPolicy Spec). As Jianjun mentioned in the meeting, if we plan to have other selector fields (potentially mutually-exclusive), that's potentially a different story...

If we do go with a pointer, Jianjun's proposal kind of makes sense to me (disabling auto-assignment for nil): the user is responsible for assigning IP addresses to Nodes, but doesn't have to manage individual IPs for different Egress resources? Still, if we don't need other selectors in the future and this is not an actual use case, a non optional field makes more sense to me.

I missed the difference between nil and empty, thought they are same somehow. Yes, what you suggested makes sense. Empty is more common to to represent selecting All. I have made the field mandatory in the latest patch. If we have other fields to select Nodes in the future, I think changing a mandatory field to optional is compatible.

I think last time we discussed changing from mandatory to optional is not a compatible change. But anyway if we see no use case to make it optional, we can keep the current design (otherwise maybe change back to a pointer, but validating it cannot be nil?).

I also believe that changing from mandatory to optional is not considered a compatible changes, since it can break older clients which read ExternalIPPoolSpec resources created by a newer client? https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api_changes.md#on-compatibility

Given that this is an alpha API, maybe we should go with what makes the most sense right now, which I would say is making the field mandatory (current proposal).

My mistake, was considering the server side storage only. Agree we should keep what makes the most sense right now when we don't have clear idea about how to extend it.

antoninbas · 2021-06-08T04:33:49Z

pkg/apis/crd/v1alpha2/types.go

+	// ExternalIPPool specifies the IP Pool that the EgressIP should be allocated from.
+	// If it's set, the allocated EgressIP will be assigned to a Node specified by the Pool and it will failover to
+	// another Node if the assigned Node becomes unavailable.
+	// If it's set with EgressIP, the EgressIP must be in the IP Pool.
+	ExternalIPPool string `json:"externalIPPool"`


Should we have an EgressStatus to report which IP has been assigned (e.g. for troubleshooting) to the Egress resource?

The assigned IP will be set to the above EgressIP field. Does it make sense to you? This is similar to Service ClusterIP. It can be specified manually or assigned by server, the field is kept in Spec for unification.

BTW, for troubleshooting and visibility, there is a plan to add a nodeName field to status after the major function is done.

Sounds good. If we want to get fancy, we can even add a status to the EgressIPPool to report the number of allocated vs free IPs?

Good suggestion, will create an issue to track it.

pkg/apis/crd/v1alpha2/types.go

tnqn

Thanks @jianjuns and @antoninbas for the suggestions.

tnqn · 2021-06-08T13:47:46Z

pkg/apis/crd/v1alpha2/types.go

+	// ExternalIPPool specifies the IP Pool that the EgressIP should be allocated from.
+	// If it's set, the allocated EgressIP will be assigned to a Node specified by the Pool and it will failover to
+	// another Node if the assigned Node becomes unavailable.
+	// If it's set with EgressIP, the EgressIP must be in the IP Pool.
+	ExternalIPPool string `json:"externalIPPool"`


The assigned IP will be set to the above EgressIP field. Does it make sense to you? This is similar to Service ClusterIP. It can be specified manually or assigned by server, the field is kept in Spec for unification.

pkg/apis/crd/v1alpha2/types.go

tnqn · 2021-06-08T14:20:45Z

pkg/apis/crd/v1alpha2/types.go

+type ExternalIPPoolSpec struct {
+	// The IP ranges of this IP pool, e.g. 10.10.0.0/24, 10.10.10.2-10.10.10.20, 10.10.10.30-10.10.10.30.
+	IPRanges []IPRange `json:"ipRanges"`
+	// The Nodes that the external IPs can be assigned to. If empty, it means all Nodes.


I missed the difference between nil and empty, thought they are same somehow. Yes, what you suggested makes sense. Empty is more common to to represent selecting All. I have made the field mandatory in the latest patch. If we have other fields to select Nodes in the future, I think changing a mandatory field to optional is compatible.

jianjuns · 2021-06-08T17:21:59Z

pkg/apis/crd/v1alpha2/types.go

+type ExternalIPPoolSpec struct {
+	// The IP ranges of this IP pool, e.g. 10.10.0.0/24, 10.10.10.2-10.10.10.20, 10.10.10.30-10.10.10.30.
+	IPRanges []IPRange `json:"ipRanges"`
+	// The Nodes that the external IPs can be assigned to. If empty, it means all Nodes.


I think last time we discussed changing from mandatory to optional is not a compatible change. But anyway if we see no use case to make it optional, we can keep the current design (otherwise maybe change back to a pointer, but validating it cannot be nil?).

antoninbas · 2021-06-08T21:03:28Z

pkg/apis/crd/v1alpha2/types.go

+// +genclient:noStatus
+// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
+
+// ExternalIPPool defines one or multiple IP sets that can be used in external network. For instance, the IPs can be


"in the external network"?
or "in external networks"?

antoninbas · 2021-06-08T21:12:44Z

pkg/apis/crd/v1alpha2/types.go

+type ExternalIPPoolSpec struct {
+	// The IP ranges of this IP pool, e.g. 10.10.0.0/24, 10.10.10.2-10.10.10.20, 10.10.10.30-10.10.10.30.
+	IPRanges []IPRange `json:"ipRanges"`
+	// The Nodes that the external IPs can be assigned to. If empty, it means all Nodes.


I also believe that changing from mandatory to optional is not considered a compatible changes, since it can break older clients which read ExternalIPPoolSpec resources created by a newer client? https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api_changes.md#on-compatibility

Given that this is an alpha API, maybe we should go with what makes the most sense right now, which I would say is making the field mandatory (current proposal).

1. Add ExternalIPPool CRD which defines one or multiple IP sets that can be used in external network, and the Nodes that can hold the IPs. 2. Add an optional field "externalIPPool" to Egress spec which specifies the IP Pool that the EgressIP should be allocated from. Signed-off-by: Quan Tian <qtian@vmware.com>

tnqn · 2021-06-09T15:34:15Z

/test-all

tnqn · 2021-06-10T03:23:54Z

/test-networkpolicy
/test-e2e
/test-conformance

tnqn mentioned this pull request Jun 3, 2021

Support allocating Egress IPs from ExternalIPPools #2237

Merged

tnqn force-pushed the egress-api branch 2 times, most recently from c59fc68 to 26b76b6 Compare June 3, 2021 14:34

tnqn force-pushed the egress-api branch from 26b76b6 to 13e0f36 Compare June 3, 2021 15:22

tnqn requested review from jianjuns, antoninbas and wenqiq June 3, 2021 15:24

tnqn mentioned this pull request Jun 3, 2021

Support EgressIP assigning and failover in antrea-agent #2186

Merged

jianjuns reviewed Jun 4, 2021

View reviewed changes

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

jianjuns reviewed Jun 7, 2021

View reviewed changes

antoninbas reviewed Jun 8, 2021

View reviewed changes

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

tnqn commented Jun 8, 2021

View reviewed changes

jianjuns previously approved these changes Jun 8, 2021

View reviewed changes

antoninbas previously approved these changes Jun 8, 2021

View reviewed changes

tnqn dismissed stale reviews from antoninbas and jianjuns via 1ab4fad June 9, 2021 11:05

tnqn force-pushed the egress-api branch from a740b4e to 1ab4fad Compare June 9, 2021 11:05

antoninbas approved these changes Jun 9, 2021

View reviewed changes

tnqn merged commit 4285e7c into antrea-io:main Jun 10, 2021

tnqn mentioned this pull request Jul 23, 2021

Report the number of allocated vs free IPs to status of the EgressIPPool? #2454

Closed

tnqn mentioned this pull request Aug 24, 2021

Support automatic failover for Egress #2128

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ExternalIPPool API and update Egress API for failover support #2236

Add ExternalIPPool API and update Egress API for failover support #2236

tnqn commented Jun 3, 2021

codecov-commenter commented Jun 3, 2021 •

edited

Loading

tnqn commented Jun 3, 2021

jianjuns left a comment

jianjuns Jun 7, 2021

tnqn Jun 8, 2021

jianjuns Jun 8, 2021

antoninbas Jun 8, 2021

antoninbas Jun 8, 2021

tnqn Jun 8, 2021

jianjuns Jun 8, 2021

antoninbas Jun 8, 2021

tnqn Jun 9, 2021

antoninbas Jun 8, 2021

tnqn Jun 8, 2021

tnqn Jun 8, 2021

antoninbas Jun 8, 2021

tnqn Jun 9, 2021

tnqn left a comment

tnqn Jun 8, 2021

tnqn Jun 8, 2021

jianjuns Jun 8, 2021

antoninbas Jun 8, 2021

tnqn Jun 9, 2021

antoninbas Jun 8, 2021

tnqn commented Jun 9, 2021

tnqn commented Jun 10, 2021

Add ExternalIPPool API and update Egress API for failover support #2236

Add ExternalIPPool API and update Egress API for failover support #2236

Conversation

tnqn commented Jun 3, 2021

codecov-commenter commented Jun 3, 2021 • edited Loading

Codecov Report

tnqn commented Jun 3, 2021

jianjuns 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn commented Jun 9, 2021

tnqn commented Jun 10, 2021

codecov-commenter commented Jun 3, 2021 •

edited

Loading