cmd: Disable local node routes when endpoint routes are enabled #28324
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gandro
added
area/daemon
gandro
requested review from
youngnick,
sayboras,
christarazi,
learnitall and
viktor-kurchenko
|
/test |
christarazi
approved these changes
Sep 28, 2023
viktor-kurchenko
approved these changes
Sep 29, 2023
sayboras
approved these changes
Sep 29, 2023
learnitall
requested changes
Sep 29, 2023
There was a problem hiding this comment.
Thank you for the detailed PR description. The doc changes you included look great, but there is one extra place that I believe still needs an update- similar to the multi-pool IPAM docs:
cilium/Documentation/network/concepts/routing.rst
Lines 288 to 299 in bb6decf
Can you please update this page and then re-request a review from me?
|
@learnitall I've updated the highlighted section as follows: diff --git a/Documentation/network/concepts/routing.rst b/Documentation/network/concepts/routing.rst
index 002407eaa24d..1e912248d477 100644
--- a/Documentation/network/concepts/routing.rst
+++ b/Documentation/network/concepts/routing.rst
@@ -296,7 +296,7 @@ The following configuration options must be set to run the datapath on GKE:
* ``ipam: kubernetes``: Enable :ref:`k8s_hostscope` IPAM
* ``routing-mode: native``: Enable native routing mode
* ``enable-endpoint-routes: true``: Enable per-endpoint routing on the node
- * ``enable-local-node-route: false``: Disable installation of the local node route
+ (automatically disables the local node route).
* ``ipv4-native-routing-cidr: x.x.x.x/y``: Set the CIDR in which native routing
is supported. |
gandro
force-pushed
the
pr/gandro/auto-disable-local-node-routes
branch
from
7455cbb
to
579dcd4
Compare
The command-line description of `enable-endpoint-routes` states: "Use per endpoint routes instead of routing via cilium_host". This description however has not been true: Even with per-endpoint routes enabled, Cilium would still install the local node route unless explicitly disabled. Not only is the local node route (which installs a single route based on the local pod CIDR) redundant to per-endpoint routes (which installs a route per endpoint), it also does not work on IPAM modes that do not have a local pod CIDR, such as ENI or Azure. On Azure, GKE, and in the MultiPool CI, we already disabled the local node route explicitly when enabling per-endpoint routes, but in many other cases (ENI, as well as most of our CI (see github/actions/cilium-config)) we forgot to disable local node routes explicitly when per-endpoint routes are enabled. Therefore, this commit improves UX by automatically disabling the local node route if per-endpoint routes are enabled. This approach was discussed at the Cilium Community meeting on June 7, 2023. As an alternative to this change, we could also introduce a new tri-mode flag (i.e. "per-endpoint routes", "local node route", "none"), but such a change might introduce unnecessary churn if we decide to remove the local node routes further down the road. Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
gandro
force-pushed
the
pr/gandro/auto-disable-local-node-routes
branch
from
579dcd4
to
da9758f
Compare
learnitall
approved these changes
Oct 2, 2023
maintainer-s-little-helper
bot
added
the
ready-to-merge
Closed
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The command-line description of
enable-endpoint-routesstates: "Use per endpoint routes instead of routing via cilium_host". This description however has not been true: Even with per-endpoint routes enabled, Cilium would still install the local node route (i.e. "routing via cilium_host") unless explicitly disabled.Not only is the local node route (which installs a single route based on the local pod CIDR) redundant to per-endpoint routes (which installs a route per endpoint), it also does not work on IPAM modes that do not have a local pod CIDR, such as ENI or Azure.
On Azure, GKE, and in the MultiPool CI, we already disabled the local node route explicitly when enabling per-endpoint routes, but in many other cases (ENI, as well as most of our CI (see
github/actions/cilium-config)) we forgot to disable local node routes explicitly when per-endpoint routes are enabled. Therefore, this commit improves UX by automatically disabling the local node route if per-endpoint routes are enabled. This approach was discussed at the Cilium Community meeting on June 7, 2023.
As an alternative to this change, we could also introduce a new tri-mode flag (i.e. "per-endpoint routes", "local node route", "none"), but such a change might introduce unnecessary churn if we decide to remove the local node routes further down the road.