-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
egressgw: change special values for gatewayIP #24449
egressgw: change special values for gatewayIP #24449
Conversation
78f2f59
to
3a771fe
Compare
I know this needs further polishing on test and possibly documentation, but I opened this to get initial feedback if this look like the right way to do this and whether or not traffic on egressgw that does not match any node should be dropped by default or not (or maybe this behavior should be guarded under a non default option?). I assumed this was the default behavior and had a bad surprise on my home setup (where I use this as some sort of proxy to reach the external world on some specific workloads). |
47d318a
to
5b0e2ff
Compare
7b8109c
to
6dd2492
Compare
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 change looks reasonable however, I'd like either @julianwiedmann or @jibi to give a final ack for full context.
Thanks for the PR @MrFreezeex! Couple of comments below. First I think 2158822e021025b4c25ac0d054aef1a14de2cbf0 warrants its own PR as that's a separate bug fix. Next, for the other 2 commits, I'm not opposed to the idea of explicitly dropping traffic selected by a CEGP that doesn't match any node (I think that's desirable) but this will cause some issues during upgrades/downgrades of the agent. For example:
One way to work around this would be to introduce the change over a couple of releases:
but I need to think a bit more here (as that could still cause some drops for example when downgrading from |
Sure I will do that.
Migration is a very good point that I indeed didn't have in mind! I am thinking about an alternative solution though, we could just say that |
Here is the separated PR for the first commit: #24646 |
I thought about that as well, but as you pointed out it makes things less clear, so not 100% sure we want to go this way 🤔 |
Aren't those values implementation details? We dump them in the CLI commands, but maybe that's our mistake and we should convert them to e.g. |
Yep afaik they are and never shown except with |
Ok actually I might have another idea. We could add an optional settings disabled by default to block traffic if egressgw doesn't match anything (disabled by default for now). If enabled it will block |
Honestly, if it's just a matter of code readability, then I think that's a job for clear constant names (e.g., |
761161b
to
eed99de
Compare
/test |
That worked to fix most of the failures, the only one left is for the new end to end test:
and the reason for that is that the source pods are not deployed (as we are using a separate context for that test). Moving |
4f13543
to
d27b87e
Compare
Oh yes indeed sorry for that 🤦♂️, I was struggling to find the root cause for this, thanks! |
/test |
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.
Looks good now, thanks for bearing with me through the few iterations and for all the extra tests!
With regard to part 2: I don't think we need to wait for anything in particular, we can already start getting that into master, and label it for backport only once this PR is backported and released in v1.13
d27b87e
to
5702bdd
Compare
0.0.0.0 now represents that the gateway has not been found (while it previously meant gateway not found AND excluded CIDR). While 0.0.0.1 now represents an excluded CIDR. Future special values should all be included in the 0.0.0.0/8 range. Both values (0.0.0.0, 0.0.0.1) currently behave like excluded CIDR and skip the egressgw NAT-ing while not dropping the packet for backward compatibilities during Cilium upgrade. In a future release gateway not found (0.0.0.0) will drop the traffic. Signed-off-by: Arthur Outhenin-Chalandre <arthur@cri.epita.fr>
Add a new test to check the case whenever the egress gateway doesn't match any gateway. Signed-off-by: Arthur Outhenin-Chalandre <arthur@cri.epita.fr>
Signed-off-by: Arthur Outhenin-Chalandre <arthur@cri.epita.fr>
5702bdd
to
54e37f3
Compare
Thanks for bearing with me too and for all your guidance :D.
Sure 👍, I can prepare something for it as soon as this gets merged! :) |
/test |
@jibi Unfortunately this does not apply cleanly to v1.13. Marking with |
#24849 is merged, updating the label to |
Please ensure your pull request adheres to the following guidelines:
For first time contributors, read Submitting a pull request
All code is covered by unit and/or runtime tests where feasible.
All commits contain a well written commit description including a title,
description and a
Fixes: #XXX
line if the commit addresses a particularGitHub issue.
If your commit description contains a
Fixes: <commit-id>
tag, thenplease add the commit author[s] as reviewer[s] to this issue.
All commits are signed off. See the section Developer’s Certificate of Origin
Provide a title or release-note blurb suitable for the release notes.
Are you a user of Cilium? Please add yourself to the Users doc
Thanks for contributing!
Change excluded CIDR "special" value of gatewayIP from 0.0.0.0 -> 0.0.0.1