-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
v1.13 backports 2023-04-20 #24999
v1.13 backports 2023-04-20 #24999
Conversation
[ upstream commit a292a75 ] Switch to dropping traffic when no gateway are found for an egressgw instead of the previous behavior consisting of allowing traffic without the snat. It also adds a new drop reason (DROP_NO_EGRESS_GATEWAY) for this specific case. Signed-off-by: Arthur Outhenin-Chalandre <arthur@cri.epita.fr>
Hi @jibi, I made the backport for the egressgw change. Hopefully it's correct 🙈 :D. |
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.
whoa, thanks for taking care of this! 💯 🚀
@@ -353,6 +353,7 @@ enum DropReason { | |||
NAT46 = 187; | |||
NAT64 = 188; | |||
AUTH_REQUIRED = 189; | |||
NO_EGRESS_GATEWAY = 194; |
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.
should we keep it sequential here?
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.
cc @rolinh, not really sure what's better for backports
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.
should we keep it sequential here?
No we shouldn't, we need the same value as in #24835. Otherwise API consumers (e.g. Hubble CLI) could be confused as DropReason 190 would be ambiguous depending on the Cilium version, i.e. CT_NO_MAP_FOUND
(Cilium v1.14+) vs NO_EGRESS_GATEWAY
(Cilium v1.13).
@@ -304,6 +304,12 @@ Annotations: | |||
|
|||
.. _1.13_upgrade_notes: | |||
|
|||
1.13.3+ Upgrade Notes |
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.
👌
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.
Thanks @MrFreezeex, Hubble changes LGTM.
@@ -353,6 +353,7 @@ enum DropReason { | |||
NAT46 = 187; | |||
NAT64 = 188; | |||
AUTH_REQUIRED = 189; | |||
NO_EGRESS_GATEWAY = 194; |
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.
should we keep it sequential here?
No we shouldn't, we need the same value as in #24835. Otherwise API consumers (e.g. Hubble CLI) could be confused as DropReason 190 would be ambiguous depending on the Cilium version, i.e. CT_NO_MAP_FOUND
(Cilium v1.14+) vs NO_EGRESS_GATEWAY
(Cilium v1.13).
/test-backport-1.13 Job 'Cilium-PR-K8s-1.20-kernel-4.19' failed: Click to show.Test Name
Failure Output
Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.20-kernel-4.19/1897/ If it is a flake and a GitHub issue doesn't already exist to track it, comment Job 'Cilium-PR-K8s-1.20-kernel-4.19' failed: Click to show.Test Name
Failure Output
Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.20-kernel-4.19/1898/ If it is a flake and a GitHub issue doesn't already exist to track it, comment |
/mlh new-flake Cilium-PR-K8s-1.20-kernel-4.19 👍 created #25014 |
/test-1.20-4.19 |
Once this PR is merged, you can update the PR labels via: