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

Allow Node SNAT for Static Egress case #6831

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jainpulkit22
Copy link
Contributor

Implemented best effort scenario, where in case of static Egress also, if there is no egress node then the packets will be sent using normal Node SNAT, as in case of dynamic Egress.

For #6228.

@rajnkamr rajnkamr added this to the Antrea v2.3 release milestone Nov 26, 2024
@rajnkamr rajnkamr added the area/transit/egress Issues or PRs related to Egress (SNAT for traffic egressing the cluster). label Nov 26, 2024
@jainpulkit22 jainpulkit22 force-pushed the egress-record-update branch 11 times, most recently from 658ceb3 to 8dd6cfb Compare November 28, 2024 10:01
@jainpulkit22 jainpulkit22 force-pushed the egress-record-update branch 2 times, most recently from be099f9 to 1f5c419 Compare December 5, 2024 07:29
@jainpulkit22 jainpulkit22 force-pushed the egress-record-update branch 2 times, most recently from 62ff87c to c0bc9dd Compare December 11, 2024 10:24
@antoninbas antoninbas requested a review from tnqn December 11, 2024 22:15
pkg/agent/controller/egress/egress_controller.go Outdated Show resolved Hide resolved
eState.pods.Insert(pod)
stalePods.Delete(pod)
egress, _ = c.egressLister.Get(egressName)
if egress.Status.EgressNode != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

As I pointed out before, I do feel like it is strange to consume .Status.EgressNode as part of the controller, and I am not sure this feature is very useful, but I will let @tnqn review & decide.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if this is an improvement either. Note that if this behavior change isn't wanted by all users, it would satisfy some while breaking others. The issue being addressed doesn't come from end users so I would prefer keeping it as was.
Instead of changing the behavior from one to another, I would prefer making it explicit and configurable, letting user know and decide what would happen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay so in that case we can have a config variable set by user in antrea.yml and if that i set then we can allow normal Node SNAT if static egress is not assigned to any node, and if the variable is not set or is false then we will keep the behaviour as it is.
Is it what is expected @antoninbas @tnqn

@jainpulkit22 jainpulkit22 force-pushed the egress-record-update branch 3 times, most recently from 7267a38 to 592fec7 Compare December 12, 2024 06:57
Implemented best effort scenario, where in case of
static Egress also, if there is no egress node then
the packets will be sent using normal Node SNAT, as
in case of dynamic Egress.

Signed-off-by: Pulkit Jain <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/transit/egress Issues or PRs related to Egress (SNAT for traffic egressing the cluster).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants