Skip to content

Commit

Permalink
datapath: Remove defunct --single-cluster-route flag
Browse files Browse the repository at this point in the history 
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] cilium#18426

Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
  • Loading branch information
@gandro @pjablonski123
gandro authored and pjablonski123 committed Dec 15, 2023
1 parent f105d08 commit 779f718
Show file tree
Hide file tree
Showing 8 changed files with 16 additions and 169 deletions.
1 change: 0 additions & 1 deletion Documentation/cmdref/cilium-agent.md
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions Documentation/operations/upgrade.rst
View file
Expand Up @@ -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
~~~~~~~~~~~~

Expand Down
4 changes: 0 additions & 4 deletions daemon/cmd/daemon_main.go
View file
Expand Up @@ -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)

Expand Down
62 changes: 14 additions & 48 deletions pkg/datapath/linux/node.go
View file
Expand Up @@ -1048,13 +1048,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
Expand Down Expand Up @@ -1123,13 +1121,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))
}
}

Expand All @@ -1150,18 +1146,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,
Expand Down Expand Up @@ -1305,30 +1289,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)
}
}
}
Expand Down
82 changes: 0 additions & 82 deletions pkg/datapath/linux/node_linux_test.go
View file
Expand Up @@ -250,70 +250,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")
Expand Down Expand Up @@ -3903,15 +3839,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,
Expand Down Expand Up @@ -4050,15 +3977,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,
Expand Down
15 changes: 0 additions & 15 deletions pkg/datapath/types/node.go
View file
Expand Up @@ -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.
//
Expand Down
1 change: 0 additions & 1 deletion pkg/nodediscovery/nodediscovery.go
View file
Expand Up @@ -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(),
Expand Down
18 changes: 0 additions & 18 deletions pkg/option/config.go
View file
Expand Up @@ -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"
Expand Down Expand Up @@ -1576,10 +1568,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
Expand Down Expand Up @@ -2847,11 +2835,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,
Expand Down Expand Up @@ -3148,7 +3131,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)
Expand Down

0 comments on commit 779f718

Please sign in to comment.