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

Update Egress API to support multiple Egress IPs and pools #4603

Merged
merged 1 commit into from
May 4, 2023

Conversation

tnqn
Copy link
Member

@tnqn tnqn commented Feb 6, 2023

Nodes in a cluster could reside in multiple subnets, but the egress IP needs to be routable in the underlay network and usually resides in the same subnet of the Node hosting it. Therefore, there may be a situation where no available nodes are eligible to host an egress IP address if all Nodes in a subnet are down, interrupting egress traffic of the workloads that use the IP.

As the first step of supporting the above scenario, the patch extends the Egress IP to support multiple Egress IPs and pools so that one Egress IP can be configured for each subnet, making Egress failover across the whole cluster possible.

Besides, it also adds a field, status.egressIP, to represent the effective Egress IP. When there is no eligible Node for any of the Egress IPs, the field will be empty.

@codecov
Copy link

codecov bot commented Feb 6, 2023

Codecov Report

Merging #4603 (567e2b3) into main (272b56c) will decrease coverage by 3.12%.
The diff coverage is 56.66%.

❗ Current head 567e2b3 differs from pull request most recent head 58e97ac. Consider uploading reports for the commit 58e97ac to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4603      +/-   ##
==========================================
- Coverage   71.99%   68.87%   -3.12%     
==========================================
  Files         406      403       -3     
  Lines       60856    59797    -1059     
==========================================
- Hits        43812    41187    -2625     
- Misses      14132    15773    +1641     
+ Partials     2912     2837      -75     
Flag Coverage Δ *Carryforward flag
e2e-tests 38.33% <ø> (-0.05%) ⬇️ Carriedforward from d5dd02e
integration-tests 34.45% <ø> (+0.62%) ⬆️
kind-e2e-tests 41.03% <50.00%> (-6.35%) ⬇️
unit-tests 59.92% <56.66%> (-2.79%) ⬇️

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files Coverage Δ
pkg/controller/egress/controller.go 87.50% <50.00%> (-1.20%) ⬇️
pkg/agent/controller/egress/egress_controller.go 83.68% <59.09%> (+0.03%) ⬆️

... and 106 files with indirect coverage changes

@tnqn tnqn force-pushed the multi-egress-ips branch 4 times, most recently from 62ccdf8 to 4f70047 Compare February 9, 2023 09:09
@tnqn tnqn force-pushed the multi-egress-ips branch 2 times, most recently from 7e3fedc to 5ee642c Compare February 16, 2023 13:10
@tnqn tnqn force-pushed the multi-egress-ips branch 2 times, most recently from 5e847e1 to 567e2b3 Compare February 24, 2023 08:09
@tnqn tnqn changed the title [WIP] Extend Egress API to support multiple Egress IPs Extend Egress data structure to support multiple Egress IPs Apr 23, 2023
@tnqn
Copy link
Member Author

tnqn commented Apr 23, 2023

/test-all

@tnqn tnqn marked this pull request as ready for review April 23, 2023 11:35
@XinShuYang
Copy link
Contributor

/test-all

@tnqn tnqn closed this Apr 26, 2023
@tnqn tnqn reopened this Apr 28, 2023
@tnqn tnqn changed the title Extend Egress data structure to support multiple Egress IPs Update Egress API to support multiple Egress IPs and pools Apr 28, 2023
@tnqn
Copy link
Member Author

tnqn commented Apr 28, 2023

/test-all

jianjuns
jianjuns previously approved these changes Apr 28, 2023
// ExternalIPPool specifies the IP Pool that the EgressIP should be allocated from.
// If it is empty, the specified EgressIP must be assigned to a Node manually.
// If it is non-empty, the EgressIP will be assigned to a Node specified by the pool automatically and will failover
// to a different Node when the Node becomes unreachable.
ExternalIPPool string `json:"externalIPPool"`
ExternalIPPool string `json:"externalIPPool,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can deprecate this field in a future release?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean both EgressIP and ExternalIPPool, the singular fields?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes.

@@ -213,11 +216,18 @@ type EgressSpec struct {
// If ExternalIPPool is non-empty, it can be empty and will be assigned by Antrea automatically.
// If both ExternalIPPool and EgressIP are non-empty, the IP must be in the pool.
EgressIP string `json:"egressIP,omitempty"`
// EgressIPs specifies multiple SNAT IP addresses for the selected workloads.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we require each IP must be from different ExternalIPPools?

Copy link
Member Author

Choose a reason for hiding this comment

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

We probably should check the uniqueness of the pools because:

  1. Using same ExternalIPPools multiple times is meaningless
  2. It may cause trouble to the implementation, e.g. we might use map to track pool -> ip allocation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. Would you add comments for that?

Copy link
Member Author

Choose a reason for hiding this comment

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

updated the comment of ExternalIPPools

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we say IPs must be in ExternalIPPools? And move "Entries with the same index in EgressIPs and ExternalIPPools are correlated." 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.

But we could also support IPs not allocated from pools. For example, users could get HA support when they have two Nodes that have public access and just use two Node IPs as the Egress IPs 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.

@jianjuns If there are more comments about the fields' comment, we could update with a following up PR or when we officially support multiple IPs. I will merge it first to proceed.

antoninbas
antoninbas previously approved these changes Apr 28, 2023
Nodes in a cluster could reside in multiple subnets, but the egress IP
needs to be routable in the underlay network and usually resides in the
same subnet of the Node hosting it. Therefore, there may be a situation
where no available nodes are eligible to host an egress IP address if
all Nodes in a subnet are down, interrupting egress traffic of the
workloads that use the IP.

As the first step of supporting the above scenario, the patch extends
the Egress IP to support multiple Egress IPs and pools so that one
Egress IP can be configured for each subnet, making Egress failover
across the whole cluster possible.

Besides, it also adds a field, `status.egressIP`, to represent the
effective Egress IP. When there is no eligible Node for any of the
Egress IPs, the field will be empty.

Signed-off-by: Quan Tian <qtian@vmware.com>
@tnqn tnqn dismissed stale reviews from antoninbas and jianjuns via 1a8d0fe May 4, 2023 03:11
@@ -213,11 +216,18 @@ type EgressSpec struct {
// If ExternalIPPool is non-empty, it can be empty and will be assigned by Antrea automatically.
// If both ExternalIPPool and EgressIP are non-empty, the IP must be in the pool.
EgressIP string `json:"egressIP,omitempty"`
// EgressIPs specifies multiple SNAT IP addresses for the selected workloads.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we say IPs must be in ExternalIPPools? And move "Entries with the same index in EgressIPs and ExternalIPPools are correlated." here?

@tnqn
Copy link
Member Author

tnqn commented May 4, 2023

/skip-all

@tnqn tnqn merged commit e635aef into antrea-io:main May 4, 2023
@tnqn tnqn deleted the multi-egress-ips branch May 4, 2023 09:28
rajnkamr pushed a commit to rajnkamr/antrea that referenced this pull request May 4, 2023
…#4603)

Nodes in a cluster could reside in multiple subnets, but the egress IP
needs to be routable in the underlay network and usually resides in the
same subnet of the Node hosting it. Therefore, there may be a situation
where no available nodes are eligible to host an egress IP address if
all Nodes in a subnet are down, interrupting egress traffic of the
workloads that use the IP.

As the first step of supporting the above scenario, the patch extends
the Egress IP to support multiple Egress IPs and pools so that one
Egress IP can be configured for each subnet, making Egress failover
across the whole cluster possible.

Besides, it also adds a field, `status.egressIP`, to represent the
effective Egress IP. When there is no eligible Node for any of the
Egress IPs, the field will be empty.

Signed-off-by: Quan Tian <qtian@vmware.com>
ceclinux pushed a commit to ceclinux/antrea that referenced this pull request Jun 5, 2023
…#4603)

Nodes in a cluster could reside in multiple subnets, but the egress IP
needs to be routable in the underlay network and usually resides in the
same subnet of the Node hosting it. Therefore, there may be a situation
where no available nodes are eligible to host an egress IP address if
all Nodes in a subnet are down, interrupting egress traffic of the
workloads that use the IP.

As the first step of supporting the above scenario, the patch extends
the Egress IP to support multiple Egress IPs and pools so that one
Egress IP can be configured for each subnet, making Egress failover
across the whole cluster possible.

Besides, it also adds a field, `status.egressIP`, to represent the
effective Egress IP. When there is no eligible Node for any of the
Egress IPs, the field will be empty.

Signed-off-by: Quan Tian <qtian@vmware.com>
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.

4 participants