Support allocating Egress IPs from ExternalIPPools #2237

tnqn · 2021-06-03T10:51:11Z

If users don't specify EgressIP of an Egress, antrea-controller will allocate one IP from the specified ExternalIPPool if it's available.

For #2128

codecov-commenter · 2021-06-03T15:57:49Z

Codecov Report

Merging #2237 (f40895b) into main (5953984) will increase coverage by 3.28%.
The diff coverage is 83.78%.

@@            Coverage Diff             @@
##             main    #2237      +/-   ##
==========================================
+ Coverage   61.73%   65.02%   +3.28%     
==========================================
  Files         276      277       +1     
  Lines       21306    21563     +257     
==========================================
+ Hits        13153    14021     +868     
+ Misses       6775     6101     -674     
- Partials     1378     1441      +63

Flag	Coverage Δ
e2e-tests	`50.35% <0.00%> (?)`
kind-e2e-tests	`52.29% <0.00%> (-0.64%)`	⬇️
unit-tests	`41.86% <83.78%> (+0.60%)`	⬆️

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

Impacted Files	Coverage Δ
pkg/controller/egress/ipallocator/allocator.go	`83.14% <83.14%> (ø)`
pkg/controller/egress/controller.go	`87.20% <84.11%> (+3.32%)`	⬆️
...gent/controller/networkpolicy/status_controller.go	`72.60% <0.00%> (-5.48%)`	⬇️
pkg/util/k8s/client.go	`46.34% <0.00%> (-2.68%)`	⬇️
pkg/agent/controller/traceflow/packetin.go	`64.97% <0.00%> (-1.69%)`	⬇️
...ntroller/networkpolicy/networkpolicy_controller.go	`69.45% <0.00%> (-0.65%)`	⬇️
pkg/ovs/openflow/ofctrl_bridge.go	`50.36% <0.00%> (+0.36%)`	⬆️
pkg/ovs/openflow/ofctrl_action.go	`50.42% <0.00%> (+0.42%)`	⬆️
pkg/agent/openflow/network_policy.go	`76.41% <0.00%> (+0.59%)`	⬆️
... and 45 more

pkg/controller/egress/controller.go

jianjuns · 2021-06-16T01:46:43Z

pkg/controller/egress/controller.go

+}
+
+// deleteEgress processes Egress DELETE events and deletes corresponding EgressGroup.
+func (c *EgressController) deleteExternalIPPool(obj interface{}) {


Should we enqueue Egresses here to reclaim the Egress IPs?

Good point. I was thinking whether we should have validationwebhook for externalIPPool so that one in use cannot be deleted, or just reclaim the IPs after the pool is deleted. Guess the latter one is more friendly to use. I could make the change.

Updated the code to reclaim IPs when pool is deleted.

I think refusing to delete an externalIPPool that's in use may be a more user-friendly solution actually, as it could help prevent mistakes.

I was thinking the scenario that user applying a manifest that has both egresses and externalIPPools. In current implementation, it's ok to create and delete egress and externalIPPool in any order. If we add the check, then externalIPPool can only be deleted after controller processes all egress delete events. But this may be not really important as pool is likely more persistent and your point of preventing mistakes makes sense to me. I'm ok with either strategy. @jianjuns do you have a strong opinion?

I have no strong opinion, but slightly prefer to allow deletion without check. We can have more and more such relations between CRDs; feel we should categorize some as weak references and allow deletion with the references. Pods applied the Egress might lose egress connectivity after the pool is deleted, but seems not very bad to me.
I saw APIs using either strategy.

BTW, I checked Tier resource already has the check that tier in use can not be deleted.

It's true, but we also have a check to prevent a policy from being created in a Tier that doesn't exist. Which we don't want to have for Egress.

I see merits to both approaches.

Allowing deletion:

symmetry with the creation case (one can create an Egress referencing a non-existing pool)

may be convenient if the user's intent is indeed to release all IPs at once

Preventing deletion:

prevent errors if the user mistakenly thought that the pool was no longer used

symmetry with what we would do for Pod IP pools, as in that case it wouldn't make sense IMO to allow deletion of a pool with IPs in use by Pods

What's the plan for supporting removing IP addresses from an EgressIPPool (e.g. removing an IP address range, which may be in use)? I think we should be consistent across both situations. If we don't plan to have a check for that case, I don't think we should have a check for deletion either.

Supporting removing or updating an existing IP address range requires more code to take care of cases. For example, changing 1.1.1.10-1.1.1.20 to 1.1.1.10-1.1.1.30 just means removing the former and adding the latter from code's perspective. We need to first know that the Egress IP's original ipRange is deleted (need to track which ipRange the IP is allocated from then) and then release the IP from the former pool and re-allocate them in the new pool. It can be done but I'm not sure if it's worth. One of my TODOs is to add a validatingwebhook to verify only new range can be added and existing ranges cannot be deleted. Guess to keep consistent experience, we propbably should have a check for deletion too.
@jianjuns @antoninbas how about let's start with simpler solution to get the core feature done first then consider optimizing the user experience, i.e. ipPool can expand and cannot shrink, ipPool can only be removed when it's no longer used.

Yes, in my mind we can just support extending the CIDRs, which is also the strategy of many other IPAM solutions I know.
Again, I feel we should finally categorize some relations/references as weak relations, and allow object deletion with a weak relation (and IMO Egress - IPPool relation can be weak).
I am fine to do fix validation later.

@jianjuns @antoninbas do you have any remaining comments want me to address in this PR? I plan to merge this first to make the regular operation work and will have a separate PR for validation enhancement.

pkg/controller/egress/controller.go

jianjuns · 2021-06-16T01:53:53Z

pkg/controller/egress/controller.go

+		c.releaseEgressIP(egress.Name, prevIP, prevIPPool)
+	}
+
+	// Skip allocating EgressIP if ExternalIPPool is not specified and returns whatever user specifies.


What will happen if user originally specified an Egress IP, and then updated the Spec to add an IPPool, while keeping the Egress IP in the Spec unchanged?
Will we resign the Egress IP to a Node?

What will happen if user originally specified an Egress IP, and then updated the Spec to add an IPPool, while keeping the Egress IP in the Spec unchanged?

Currently controller will only log error that the requested IP is not in the Pool. We could improve it by adding a validationwebhook which rejects the update if the IP doesn't match the pool.

Will we resign the Egress IP to a Node?

Good question. Just thought about it after you raised it. I think it will have the problem if we don't add the validationwebhook. Or we have to check if the EgressIP match the Pool in agent too.

Added TODO for the validation webhook. Will add it with a separate PR.

tnqn

thanks @jianjuns for the review.

pkg/controller/egress/controller.go

tnqn · 2021-06-16T02:36:34Z

pkg/controller/egress/controller.go

+		c.releaseEgressIP(egress.Name, prevIP, prevIPPool)
+	}
+
+	// Skip allocating EgressIP if ExternalIPPool is not specified and returns whatever user specifies.


What will happen if user originally specified an Egress IP, and then updated the Spec to add an IPPool, while keeping the Egress IP in the Spec unchanged?

Currently controller will only log error that the requested IP is not in the Pool. We could improve it by adding a validationwebhook which rejects the update if the IP doesn't match the pool.

Will we resign the Egress IP to a Node?

Good question. Just thought about it after you raised it. I think it will have the problem if we don't add the validationwebhook. Or we have to check if the EgressIP match the Pool in agent too.

tnqn · 2021-06-16T02:44:30Z

pkg/controller/egress/controller.go

+}
+
+// deleteEgress processes Egress DELETE events and deletes corresponding EgressGroup.
+func (c *EgressController) deleteExternalIPPool(obj interface{}) {


Good point. I was thinking whether we should have validationwebhook for externalIPPool so that one in use cannot be deleted, or just reclaim the IPs after the pool is deleted. Guess the latter one is more friendly to use. I could make the change.

pkg/controller/egress/controller.go

antoninbas · 2021-06-16T21:15:33Z

pkg/controller/egress/controller.go

+}
+
+// deleteEgress processes Egress DELETE events and deletes corresponding EgressGroup.
+func (c *EgressController) deleteExternalIPPool(obj interface{}) {


I think refusing to delete an externalIPPool that's in use may be a more user-friendly solution actually, as it could help prevent mistakes.

pkg/controller/egress/controller.go

pkg/controller/egress/controller_test.go

tnqn

@antoninbas thanks for the review and catching the locking error. I have addressed all comments except how to deal with externalIPPool removal. Want to achieve consensus first. I plan to have a separate PR for all validatingWebhook planned in the TODOs.

pkg/controller/egress/controller.go

pkg/controller/egress/controller_test.go

tnqn · 2021-06-23T03:43:09Z

/test-all

If users don't specify EgressIP of an Egress, antrea-controller will allocate one IP from the specified ExternalIPPool if it's available. Signed-off-by: Quan Tian <qtian@vmware.com>

antoninbas

LGTM

tnqn · 2021-06-23T16:36:08Z

/test-all

tnqn force-pushed the egress-allocation branch 6 times, most recently from eee68d8 to ee0754c Compare June 3, 2021 15:28

tnqn changed the title ~~Support Allocating Egress IPs from ExternalIPPools~~ Support allocating Egress IPs from ExternalIPPools Jun 3, 2021

tnqn marked this pull request as draft June 3, 2021 16:03

tnqn force-pushed the egress-allocation branch 4 times, most recently from 23cd8e3 to 48ca2dc Compare June 15, 2021 16:44

tnqn marked this pull request as ready for review June 15, 2021 16:44

tnqn requested review from jianjuns, antoninbas and wenqiq June 15, 2021 16:44

tnqn force-pushed the egress-allocation branch from 48ca2dc to b93de0b Compare June 15, 2021 16:57

jianjuns reviewed Jun 16, 2021

View reviewed changes

tnqn commented Jun 16, 2021

View reviewed changes

tnqn force-pushed the egress-allocation branch 3 times, most recently from f9e95af to 69e0b33 Compare June 16, 2021 16:36

antoninbas reviewed Jun 16, 2021

View reviewed changes

tnqn commented Jun 17, 2021

View reviewed changes

Support allocating Egress IPs from ExternalIPPools

22780dc

If users don't specify EgressIP of an Egress, antrea-controller will allocate one IP from the specified ExternalIPPool if it's available. Signed-off-by: Quan Tian <qtian@vmware.com>

tnqn force-pushed the egress-allocation branch from f40895b to 22780dc Compare June 23, 2021 13:21

antoninbas approved these changes Jun 23, 2021

View reviewed changes

jianjuns approved these changes Jun 23, 2021

View reviewed changes

tnqn merged commit ef14ad1 into antrea-io:main Jun 24, 2021

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

Support allocating Egress IPs from ExternalIPPools #2237

Support allocating Egress IPs from ExternalIPPools #2237

tnqn commented Jun 3, 2021 •

edited

Loading

codecov-commenter commented Jun 3, 2021 •

edited

Loading

jianjuns Jun 16, 2021

tnqn Jun 16, 2021

tnqn Jun 16, 2021

antoninbas Jun 16, 2021

tnqn Jun 17, 2021

jianjuns Jun 18, 2021

antoninbas Jun 18, 2021

tnqn Jun 18, 2021

jianjuns Jun 21, 2021 •

edited

Loading

tnqn Jun 23, 2021

jianjuns Jun 16, 2021

tnqn Jun 16, 2021

tnqn Jun 16, 2021

tnqn left a comment

tnqn Jun 16, 2021

tnqn Jun 16, 2021

antoninbas Jun 16, 2021

tnqn left a comment

tnqn commented Jun 23, 2021

antoninbas left a comment

tnqn commented Jun 23, 2021

Support allocating Egress IPs from ExternalIPPools #2237

Support allocating Egress IPs from ExternalIPPools #2237

Conversation

tnqn commented Jun 3, 2021 • edited Loading

codecov-commenter commented Jun 3, 2021 • edited Loading

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

jianjuns Jun 21, 2021 • edited Loading

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

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Jun 23, 2021

antoninbas left a comment

Choose a reason for hiding this comment

tnqn commented Jun 23, 2021

tnqn commented Jun 3, 2021 •

edited

Loading

codecov-commenter commented Jun 3, 2021 •

edited

Loading

jianjuns Jun 21, 2021 •

edited

Loading