From d2ebd41e40c033e89da46e8c5ea20912c9158573 Mon Sep 17 00:00:00 2001 From: Sebastian Wicki Date: Thu, 16 Nov 2023 11:02:28 +0100 Subject: [PATCH] datapath: Remove defunct `--single-cluster-route` flag This commit removes the `single-cluster-route` flag. Contrary to it's description, it never [1] installed a cluster-wide route for `cilium_host`. Instead, it installed a route for the local alloc CIDR (aka local PodCIDR), virtually identical to the (enabled by default) `enable-local-node-route`. The only difference being that `single-cluster-route` sets the MTU. This means that the flag (which has not been referenced anywhere in the last four years) likely never worked as described. Marco Iorio recently tested the flag and found that the flag (when enabled) breaks node-to-pod and nodeport traffic. In addition, since the route conflicts with the local node route, we also found that the "single cluster route" was in fact overwritten by the "local node route". The only other effect the flag has is that it disables per-node routes, but those are needed for node-to-pod traffic. Removal has already been discussed two years ago [1]. Given that the flag has remained broken ever since and there have not been any bug reports at all, it is assumed that no one is actually using it. It is also not documented anywhere outside of the cmdref. Therefore, it is removed without prior deprecation. [1] https://github.com/cilium/cilium/pull/18426 Signed-off-by: Sebastian Wicki --- Documentation/cmdref/cilium-agent.md | 1 - Documentation/operations/upgrade.rst | 2 + daemon/cmd/daemon_main.go | 4 -- pkg/datapath/linux/node.go | 62 +++++--------------- pkg/datapath/linux/node_linux_test.go | 82 --------------------------- pkg/datapath/types/node.go | 15 ----- pkg/nodediscovery/nodediscovery.go | 1 - pkg/option/config.go | 18 ------ 8 files changed, 16 insertions(+), 169 deletions(-) diff --git a/Documentation/cmdref/cilium-agent.md b/Documentation/cmdref/cilium-agent.md index 6aa6216bb652..77c3e5d6d678 100644 --- a/Documentation/cmdref/cilium-agent.md +++ b/Documentation/cmdref/cilium-agent.md @@ -304,7 +304,6 @@ cilium-agent [flags] --route-metric int Overwrite the metric used by cilium when adding routes to its 'cilium_host' device --routing-mode string Routing mode ("native" or "tunnel") (default "tunnel") --sidecar-istio-proxy-image string Regular expression matching compatible Istio sidecar istio-proxy container image names (default "cilium/istio_proxy") - --single-cluster-route Use a single cluster route instead of per node routes --socket-path string Sets daemon's socket path to listen for connections (default "/var/run/cilium/cilium.sock") --state-dir string Directory path to store runtime state (default "/var/run/cilium") --tofqdns-dns-reject-response-code string DNS response code for rejecting DNS requests, available options are '[nameError refused]' (default "refused") diff --git a/Documentation/operations/upgrade.rst b/Documentation/operations/upgrade.rst index 783d5ec7014f..17a489e46778 100644 --- a/Documentation/operations/upgrade.rst +++ b/Documentation/operations/upgrade.rst @@ -373,6 +373,8 @@ Removed Options ``tunnel=disabled``). To configure the tunneling protocol, set ``tunnel-protocol=vxlan|geneve`` (previously ``tunnel=vxlan|geneve``). +* The long defunct and undocumented ``single-cluster-route`` flag has been removed. + Helm Options ~~~~~~~~~~~~ diff --git a/daemon/cmd/daemon_main.go b/daemon/cmd/daemon_main.go index 3e5e769b4d02..46b6446d8912 100644 --- a/daemon/cmd/daemon_main.go +++ b/daemon/cmd/daemon_main.go @@ -747,10 +747,6 @@ func InitGlobalFlags(cmd *cobra.Command, vp *viper.Viper) { "Regular expression matching compatible Istio sidecar istio-proxy container image names") option.BindEnv(vp, option.SidecarIstioProxyImage) - flags.Bool(option.SingleClusterRouteName, false, - "Use a single cluster route instead of per node routes") - option.BindEnv(vp, option.SingleClusterRouteName) - flags.String(option.SocketPath, defaults.SockPath, "Sets daemon's socket path to listen for connections") option.BindEnv(vp, option.SocketPath) diff --git a/pkg/datapath/linux/node.go b/pkg/datapath/linux/node.go index 6c41f2ee1aef..e3968aa7a044 100644 --- a/pkg/datapath/linux/node.go +++ b/pkg/datapath/linux/node.go @@ -1047,13 +1047,11 @@ func (n *linuxNodeHandler) nodeUpdate(oldNode, newNode *nodeTypes.Node, firstAdd // Not a typo, the IPv4 host IP is used to build the IPv6 overlay errs = errors.Join(errs, updateTunnelMapping(oldPrefixCluster6, newPrefixCluster6, oldIP4, newIP4, firstAddition, n.nodeConfig.EnableIPv6, oldKey, newKey)) - if !n.nodeConfig.UseSingleClusterRoute { - if err := n.updateOrRemoveNodeRoutes(oldAllIP4AllocCidrs, newAllIP4AllocCidrs, isLocalNode); err != nil { - errs = errors.Join(errs, fmt.Errorf("failed to enable encapsulation: single cluster routes: ipv4: %w", err)) - } - if err := n.updateOrRemoveNodeRoutes(oldAllIP6AllocCidrs, newAllIP6AllocCidrs, isLocalNode); err != nil { - errs = errors.Join(errs, fmt.Errorf("failed to enable encapsulation: single cluster routes: ipv6: %w", err)) - } + if err := n.updateOrRemoveNodeRoutes(oldAllIP4AllocCidrs, newAllIP4AllocCidrs, isLocalNode); err != nil { + errs = errors.Join(errs, fmt.Errorf("failed to enable encapsulation: single cluster routes: ipv4: %w", err)) + } + if err := n.updateOrRemoveNodeRoutes(oldAllIP6AllocCidrs, newAllIP6AllocCidrs, isLocalNode); err != nil { + errs = errors.Join(errs, fmt.Errorf("failed to enable encapsulation: single cluster routes: ipv6: %w", err)) } return errs @@ -1122,13 +1120,11 @@ func (n *linuxNodeHandler) nodeDelete(oldNode *nodeTypes.Node) error { errs = errors.Join(errs, fmt.Errorf("failed to remove old encapsulation config: deleting tunnel mapping for ipv6: %w", err)) } - if !n.nodeConfig.UseSingleClusterRoute { - if err := n.deleteNodeRoute(oldNode.IPv4AllocCIDR, false); err != nil { - errs = errors.Join(errs, fmt.Errorf("failed to remove old encapsulation config: deleting old single cluster node route for ipv4: %w", err)) - } - if err := n.deleteNodeRoute(oldNode.IPv6AllocCIDR, false); err != nil { - errs = errors.Join(errs, fmt.Errorf("failed to remove old encapsulation config: deleting old single cluster node route for ipv6: %w", err)) - } + if err := n.deleteNodeRoute(oldNode.IPv4AllocCIDR, false); err != nil { + errs = errors.Join(errs, fmt.Errorf("failed to remove old encapsulation config: deleting old single cluster node route for ipv4: %w", err)) + } + if err := n.deleteNodeRoute(oldNode.IPv6AllocCIDR, false); err != nil { + errs = errors.Join(errs, fmt.Errorf("failed to remove old encapsulation config: deleting old single cluster node route for ipv6: %w", err)) } } @@ -1149,18 +1145,6 @@ func (n *linuxNodeHandler) nodeDelete(oldNode *nodeTypes.Node) error { return errs } -func (n *linuxNodeHandler) updateOrRemoveClusterRoute(addressing datapath.NodeAddressingFamily, addressFamilyEnabled bool) error { - allocCIDR := addressing.AllocationCIDR() - if addressFamilyEnabled { - return n.updateNodeRoute(allocCIDR, addressFamilyEnabled, false) - } - if rt, _ := n.lookupNodeRoute(allocCIDR, false); rt != nil { - return n.deleteNodeRoute(allocCIDR, false) - } - - return nil -} - func (n *linuxNodeHandler) replaceHostRules() error { rule := route.Rule{ Priority: 1, @@ -1300,30 +1284,12 @@ func (n *linuxNodeHandler) NodeConfigurationChanged(newConfig datapath.LocalNode } var errs error - if newConfig.UseSingleClusterRoute { - if err := n.updateOrRemoveClusterRoute(n.nodeAddressing.IPv4(), newConfig.EnableIPv4); err != nil { - errs = errors.Join(errs, fmt.Errorf("failed to update or remove IPv4 cluster route: %w", err)) - } - if err := n.updateOrRemoveClusterRoute(n.nodeAddressing.IPv6(), newConfig.EnableIPv6); err != nil { - errs = errors.Join(errs, fmt.Errorf("failed to update or remove IPv6 cluster route: %w", err)) - } - } else if prevConfig.UseSingleClusterRoute { - // single cluster route has been disabled, remove route - if err := n.deleteNodeRoute(n.nodeAddressing.IPv4().AllocationCIDR(), false); err != nil { - errs = errors.Join(errs, err) - } - if err := n.deleteNodeRoute(n.nodeAddressing.IPv6().AllocationCIDR(), false); err != nil { - errs = errors.Join(errs, err) - } - } - if !n.isInitialized { n.isInitialized = true - if !n.nodeConfig.UseSingleClusterRoute { - for _, unlinkedNode := range n.nodes { - if err := n.nodeUpdate(nil, unlinkedNode, true); err != nil { - errs = errors.Join(errs, err) - } + + for _, unlinkedNode := range n.nodes { + if err := n.nodeUpdate(nil, unlinkedNode, true); err != nil { + errs = errors.Join(errs, err) } } } diff --git a/pkg/datapath/linux/node_linux_test.go b/pkg/datapath/linux/node_linux_test.go index 464169d86292..33a8d96ae0e6 100644 --- a/pkg/datapath/linux/node_linux_test.go +++ b/pkg/datapath/linux/node_linux_test.go @@ -249,70 +249,6 @@ func (s *linuxPrivilegedBaseTestSuite) TestUpdateNodeRoute(c *check.C) { } } -func (s *linuxPrivilegedBaseTestSuite) TestSingleClusterPrefix(c *check.C) { - dpConfig := DatapathConfiguration{HostDevice: dummyHostDeviceName} - - linuxNodeHandler := NewNodeHandler(dpConfig, s.nodeAddressing, nil) - c.Assert(linuxNodeHandler, check.Not(check.IsNil)) - - // enable as per test definition - err := linuxNodeHandler.NodeConfigurationChanged(datapath.LocalNodeConfiguration{ - UseSingleClusterRoute: true, - EnableIPv4: s.enableIPv4, - EnableIPv6: s.enableIPv6, - }) - c.Assert(err, check.IsNil) - - if s.enableIPv4 { - foundRoute, err := linuxNodeHandler.lookupNodeRoute(s.nodeAddressing.IPv4().AllocationCIDR(), false) - c.Assert(err, check.IsNil) - c.Assert(foundRoute, check.Not(check.IsNil)) - } - - if s.enableIPv6 { - foundRoute, err := linuxNodeHandler.lookupNodeRoute(s.nodeAddressing.IPv6().AllocationCIDR(), false) - c.Assert(err, check.IsNil) - c.Assert(foundRoute, check.Not(check.IsNil)) - } - - // disable ipv4, enable ipv6. addressing may not be available for IPv6 - err = linuxNodeHandler.NodeConfigurationChanged(datapath.LocalNodeConfiguration{ - UseSingleClusterRoute: true, - EnableIPv6: true, - }) - c.Assert(err, check.IsNil) - - foundRoute, err := linuxNodeHandler.lookupNodeRoute(s.nodeAddressing.IPv4().AllocationCIDR(), false) - c.Assert(err, check.IsNil) - c.Assert(foundRoute, check.IsNil) - - if s.enableIPv6 { - foundRoute, err := linuxNodeHandler.lookupNodeRoute(s.nodeAddressing.IPv6().AllocationCIDR(), false) - c.Assert(err, check.IsNil) - c.Assert(foundRoute, check.Not(check.IsNil)) - } - - // enable ipv4, enable ipv6, addressing may not be available - err = linuxNodeHandler.NodeConfigurationChanged(datapath.LocalNodeConfiguration{ - UseSingleClusterRoute: true, - EnableIPv6: true, - EnableIPv4: true, - }) - c.Assert(err, check.IsNil) - - if s.enableIPv4 { - foundRoute, err := linuxNodeHandler.lookupNodeRoute(s.nodeAddressing.IPv4().AllocationCIDR(), false) - c.Assert(err, check.IsNil) - c.Assert(foundRoute, check.Not(check.IsNil)) - } - - if s.enableIPv6 { - foundRoute, err := linuxNodeHandler.lookupNodeRoute(s.nodeAddressing.IPv6().AllocationCIDR(), false) - c.Assert(err, check.IsNil) - c.Assert(foundRoute, check.Not(check.IsNil)) - } -} - func (s *linuxPrivilegedBaseTestSuite) TestAuxiliaryPrefixes(c *check.C) { net1 := cidr.MustParseCIDR("30.30.0.0/24") net2 := cidr.MustParseCIDR("cafe:f00d::/112") @@ -3893,15 +3829,6 @@ func (s *linuxPrivilegedBaseTestSuite) BenchmarkNodeUpdateEncap(c *check.C) { }) } -func (s *linuxPrivilegedBaseTestSuite) BenchmarkNodeUpdateEncapSingleClusterRoute(c *check.C) { - s.benchmarkNodeUpdate(c, datapath.LocalNodeConfiguration{ - EnableIPv4: s.enableIPv4, - EnableIPv6: s.enableIPv6, - EnableEncapsulation: true, - UseSingleClusterRoute: true, - }) -} - func (s *linuxPrivilegedBaseTestSuite) BenchmarkNodeUpdateDirectRoute(c *check.C) { s.benchmarkNodeUpdate(c, datapath.LocalNodeConfiguration{ EnableIPv4: s.enableIPv4, @@ -4040,15 +3967,6 @@ func (s *linuxPrivilegedBaseTestSuite) BenchmarkNodeValidateImplementationEncap( }) } -func (s *linuxPrivilegedBaseTestSuite) BenchmarkNodeValidateImplementationEncapSingleCluster(c *check.C) { - s.benchmarkNodeValidateImplementation(c, datapath.LocalNodeConfiguration{ - EnableIPv4: s.enableIPv4, - EnableIPv6: s.enableIPv6, - EnableEncapsulation: true, - UseSingleClusterRoute: true, - }) -} - func (s *linuxPrivilegedBaseTestSuite) BenchmarkNodeValidateImplementationDirectRoute(c *check.C) { s.benchmarkNodeValidateImplementation(c, datapath.LocalNodeConfiguration{ EnableIPv4: s.enableIPv4, diff --git a/pkg/datapath/types/node.go b/pkg/datapath/types/node.go index 4320d927ebd0..e1e38809e45c 100644 --- a/pkg/datapath/types/node.go +++ b/pkg/datapath/types/node.go @@ -42,21 +42,6 @@ type LocalNodeConfiguration struct { // subsequent calls to NodeConfigurationChanged(). EnableIPv6 bool - // UseSingleClusterRoute enables the use of a single cluster-wide route - // to direct traffic from the host into the Cilium datapath. This - // avoids the requirement to install a separate route for each node - // CIDR and can thus improve the overhead when operating large clusters - // with significant node event churn due to auto-scaling. - // - // Use of UseSingleClusterRoute must be compatible with - // EnableAutoDirectRouting. When both are enabled, any direct node - // route must take precedence over the cluster-wide route as per LPM - // routing definition. - // - // This field is mutable. The implementation of - // NodeConfigurationChanged() must adjust the routes accordingly. - UseSingleClusterRoute bool - // EnableEncapsulation enables use of encapsulation in communication // between nodes. // diff --git a/pkg/nodediscovery/nodediscovery.go b/pkg/nodediscovery/nodediscovery.go index ae889adc303b..1b3080963477 100644 --- a/pkg/nodediscovery/nodediscovery.go +++ b/pkg/nodediscovery/nodediscovery.go @@ -115,7 +115,6 @@ func NewNodeDiscovery(manager nodemanager.NodeManager, clientset client.Clientse Manager: manager, LocalConfig: datapath.LocalNodeConfiguration{ MtuConfig: mtuConfig, - UseSingleClusterRoute: option.Config.UseSingleClusterRoute, EnableIPv4: option.Config.EnableIPv4, EnableIPv6: option.Config.EnableIPv6, EnableEncapsulation: option.Config.TunnelingEnabled(), diff --git a/pkg/option/config.go b/pkg/option/config.go index 51ef3cc31895..8122b15eb825 100644 --- a/pkg/option/config.go +++ b/pkg/option/config.go @@ -504,14 +504,6 @@ const ( // RoutingMode is the name of the option to choose between native routing and tunneling mode RoutingMode = "routing-mode" - // SingleClusterRouteName is the name of the SingleClusterRoute option - // - // SingleClusterRoute enables use of a single route covering the entire - // cluster CIDR to point to the cilium_host interface instead of using - // a separate route for each cluster node CIDR. This option is not - // compatible with --routing-mode=native - SingleClusterRouteName = "single-cluster-route" - // MaxInternalTimerDelay sets a maximum on all periodic timers in // the agent in order to flush out timer-related bugs in the agent. MaxInternalTimerDelay = "max-internal-timer-delay" @@ -1570,10 +1562,6 @@ type DaemonConfig struct { // RunInterval. Zero means unlimited. MaxControllerInterval int - // UseSingleClusterRoute specifies whether to use a single cluster route - // instead of per-node routes. - UseSingleClusterRoute bool - // HTTPNormalizePath switches on Envoy HTTP path normalization options, which currently // includes RFC 3986 path normalization, Envoy merge slashes option, and unescaping and // redirecting for paths that contain escaped slashes. These are necessary to keep path based @@ -2833,11 +2821,6 @@ func (c *DaemonConfig) Validate(vp *viper.Viper) error { c.RoutingMode, RoutingModeTunnel, RoutingModeNative) } - if c.RoutingMode == RoutingModeNative && c.UseSingleClusterRoute { - return fmt.Errorf("option --%s cannot be used in combination with --%s=%s", - SingleClusterRouteName, RoutingMode, RoutingModeNative) - } - cinfo := clustermeshTypes.ClusterInfo{ ID: c.ClusterID, Name: c.ClusterName, @@ -3134,7 +3117,6 @@ func (c *DaemonConfig) Populate(vp *viper.Viper) { c.RunDir = vp.GetString(StateDir) c.ExternalEnvoyProxy = vp.GetBool(ExternalEnvoyProxy) c.SidecarIstioProxyImage = vp.GetString(SidecarIstioProxyImage) - c.UseSingleClusterRoute = vp.GetBool(SingleClusterRouteName) c.SocketPath = vp.GetString(SocketPath) c.TracePayloadlen = vp.GetInt(TracePayloadlen) c.Version = vp.GetString(Version)