-
Notifications
You must be signed in to change notification settings - Fork 359
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
Do not apply Egress to traffic destined for ServiceCIDRs #5495
Conversation
When AntreaProxy is asked to skip some Services or is not running at all, Pod-to-Service traffic would be forwarded to Egress Node and be load-balanced remotely, as opposed to locally, which could incur performance issue and unexpected behaviors. This patch installs flows to prevent traffic destined for ServiceCIDRs from being SNAT'd. Signed-off-by: Quan Tian <qtian@vmware.com>
d7047a0
to
24b7699
Compare
/test-all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine with this change.
However, I wonder if instead, we should install flows for skipServices
to the AntreaProxy LB table in OVS, and send the packets directly to the host network (same behavior as when AntreaProxy is disabled). This would avoid adding Service flows to the SNAT table. I am being a bit vague here, since I am not super familiar with the current version of the pipeline. This approach may not make sense or be possible.
Thanks for the comment. I thought it over, and tend to keep the current approach for a few reasons, please let me know whether they make sense to you or not:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall, just a small question posted in the following.
func (c *EgressController) updateServiceCIDRs(stopCh <-chan struct{}) { | ||
timer := time.NewTimer(0) | ||
defer timer.Stop() | ||
<-timer.C // Consume the first tick. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A basic question: why consume the first tick? Is it for testing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The timer is used for retry. We don't want it to trigger InstallSNATBypassServiceFlows
before receiving a service CIDR update, which is destined to fail, so consume the first tick to avoid it.
@tnqn thanks for thinking it over, it makes sense to me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Yes, makes sense. |
/test-e2e |
1 similar comment
/test-e2e |
When AntreaProxy is asked to skip some Services or is not running at all, Pod-to-Service traffic would be forwarded to Egress Node and be load-balanced remotely, as opposed to locally, which could incur performance issue and unexpected behaviors.
This patch installs flows to prevent traffic destined for ServiceCIDRs from being SNAT'd.
Fixes #5494