From ded61d43182f18f7fc972d5aa9bd0d52e5103942 Mon Sep 17 00:00:00 2001 From: Milan Lenco Date: Wed, 6 Nov 2024 16:50:00 +0100 Subject: [PATCH] All-Ones netmask is not needed for ACLs or flow logging The DHCP server deployed by EVE for Local Network Instances assigns IP addresses to applications with a /32 netmask (all-ones) and uses the Classless Static Route Option (RFC 3442) to configure static routes for the NI subnet. This setup enforces routing even for east-west (app-to-app) traffic, which would otherwise only be forwarded if the actual NI subnet mask (e.g., /24) were used. This approach was historically implemented to prevent east-west traffic from bypassing ACLs and to ensure that connection tracking (conntrack) entries were created for flow logging purposes. However, it became unnecessary after we enabled the "net.bridge.bridge-nf-call-iptables" option, which ensures that even traffic forwarded by a bridge within EVE passes through iptables filtering and has conntrack entries created. Using a /32 netmask now offers no added value and has some drawbacks. First, applications may use DHCP servers with the Classless Route option disabled, resulting in obtaining all-ones netmask with no reachable destinations due to missing connected routes. Second, enforcing routing adds extra packet processing steps for traffic that could be directly forwarded between applications, increasing overhead and reducing network performance. We previously added an option to disable the all-ones netmask (while still keeping it enabled by default), but this has increased code complexity since it requires two distinct routing configurations to manage (and test). I propose removing the all-ones netmask configuration altogether to simplify the code and reduce packet processing overhead. While some may consider this a breaking change, I believe the change in the netmask should not impact applications as long as IP addresses are preserved and the overall routing/bridging functionality remains consistent across EVE upgrades (the set of reachable destinations does not change). Signed-off-by: Milan Lenco --- .../nireconciler/genericitems/dnsmasq.go | 76 ++++--------------- .../nireconciler/genericitems/dnsmasq_test.go | 20 +---- pkg/pillar/nireconciler/linux_config.go | 5 +- pkg/pillar/nireconciler/linux_reconciler.go | 23 +----- pkg/pillar/nireconciler/linux_test.go | 58 ++------------ pkg/pillar/types/global.go | 4 - pkg/pillar/types/global_test.go | 1 - 7 files changed, 23 insertions(+), 164 deletions(-) diff --git a/pkg/pillar/nireconciler/genericitems/dnsmasq.go b/pkg/pillar/nireconciler/genericitems/dnsmasq.go index 0d820bdb8db..ec9c68c19a1 100644 --- a/pkg/pillar/nireconciler/genericitems/dnsmasq.go +++ b/pkg/pillar/nireconciler/genericitems/dnsmasq.go @@ -56,12 +56,6 @@ type Dnsmasq struct { type DHCPServer struct { // Subnet : network address + netmask (IPv4 or IPv6). Subnet *net.IPNet - // AllOnesNetmask : if enabled, DHCP server will advertise netmask with all bits - // set to one (e.g. /32 for IPv4) instead of using the actual netmask from Subnet. - // This together with Classless routes (routing traffic for the actual Subnet) - // can be used to force all traffic to go through the configured GatewayIP - // (where ACLs could be applied). - AllOnesNetmask bool // IPRange : a range of IP addresses to allocate from. // Not applicable for IPv6 (SLAAC is used instead). IPRange IPRange @@ -92,10 +86,10 @@ type DHCPServer struct { // String describes DHCPServer config. func (d DHCPServer) String() string { - return fmt.Sprintf("DHCPServer: {subnet: %s, allOnesNetmask: %t, ipRange: <%s-%s>, "+ + return fmt.Sprintf("DHCPServer: {subnet: %s, ipRange: <%s-%s>, "+ "gatewayIP: %s, withDefaultRoute: %t, domainName: %s, dnsServers: %v, ntpServers: %v, "+ "staticEntries: %v, propagateRoutes: %v, MTU: %d}", - d.Subnet, d.AllOnesNetmask, d.IPRange.FromIP, d.IPRange.ToIP, d.GatewayIP, + d.Subnet, d.IPRange.FromIP, d.IPRange.ToIP, d.GatewayIP, d.WithDefaultRoute, d.DomainName, d.DNSServers, d.NTPServers, d.StaticEntries, d.PropagateRoutes, d.MTU) } @@ -103,7 +97,6 @@ func (d DHCPServer) String() string { // Equal compares two DHCPServer instances func (d DHCPServer) Equal(d2 DHCPServer, withStaticEntries bool) bool { return netutils.EqualIPNets(d.Subnet, d2.Subnet) && - d.AllOnesNetmask == d2.AllOnesNetmask && netutils.EqualIPs(d.IPRange.FromIP, d2.IPRange.FromIP) && netutils.EqualIPs(d.IPRange.ToIP, d2.IPRange.ToIP) && netutils.EqualIPs(d.GatewayIP, d2.GatewayIP) && @@ -637,11 +630,6 @@ func (c *DnsmasqConfigurator) CreateDnsmasqConfig(buffer io.Writer, dnsmasq Dnsm if subnet != nil { ipv4Netmask = net.IP(subnet.Mask).String() altIPv4Netmask := ipv4Netmask - if gatewayIP != nil && dnsmasq.DHCPServer.AllOnesNetmask { - // Network prefix "255.255.255.255" will force packets to go through - // dom0 virtual router that makes the packets pass through ACLs and flow log. - altIPv4Netmask = "255.255.255.255" - } if _, err := io.WriteString(buffer, fmt.Sprintf("dhcp-option=option:netmask,%s\n", altIPv4Netmask)); err != nil { return writeErr(err) @@ -651,29 +639,12 @@ func (c *DnsmasqConfigurator) CreateDnsmasqConfig(buffer io.Writer, dnsmasq Dnsm // Prepare the set of all static routes to propagate to applications. var staticRoutes []IPRoute if !isIPv6 { - if dnsmasq.DHCPServer.AllOnesNetmask { - // Make the gateway reachable using a connected route. - if gatewayIP != nil { - staticRoutes = append(staticRoutes, IPRoute{ - DstNetwork: netutils.HostSubnet(gatewayIP), - Gateway: net.IP{0, 0, 0, 0}, - }) - } - if subnet != nil && gatewayIP != nil { - staticRoutes = append(staticRoutes, IPRoute{ - DstNetwork: subnet, - Gateway: gatewayIP, - }) - } - } else { - // With all-ones netmask disabled we use forwarding between apps - // on the same NI. - if subnet != nil { - staticRoutes = append(staticRoutes, IPRoute{ - DstNetwork: subnet, - Gateway: net.IP{0, 0, 0, 0}, - }) - } + // Use L2-forwarding between apps on the same NI. + if subnet != nil { + staticRoutes = append(staticRoutes, IPRoute{ + DstNetwork: subnet, + Gateway: net.IP{0, 0, 0, 0}, + }) } staticRoutes = append(staticRoutes, dnsmasq.DHCPServer.PropagateRoutes...) } @@ -717,9 +688,9 @@ func (c *DnsmasqConfigurator) CreateDnsmasqConfig(buffer io.Writer, dnsmasq Dnsm } } - // For flow-logging purposes, we prefer routing inside the host over forwarding, - // even between apps on the same subnet. The only exception is when all-ones netmask - // for app interfaces is disabled, then we are unable to enforce routing between apps. + // Apply static routes to all endpoints and separately to individual gateways. + // This is to make sure that gateway-app will not receive route that uses app's + // own local IP as the gateway. var appGateways []net.IP for _, ipRoute := range staticRoutes { if subnet != nil && subnet.Contains(ipRoute.Gateway) && @@ -728,30 +699,10 @@ func (c *DnsmasqConfigurator) CreateDnsmasqConfig(buffer io.Writer, dnsmasq Dnsm } } appGateways = generics.FilterDuplicatesFn(appGateways, netutils.EqualIPs) - enforceRouting := func(route IPRoute) IPRoute { - if gatewayIP == nil || !dnsmasq.DHCPServer.AllOnesNetmask { - // Forwarding from app to app is inevitable. - return route - } - if !generics.ContainsItemFn(appGateways, route.Gateway, netutils.EqualIPs) { - // Not from app to app (i.e. already routed). - return route - } - // Traffic will go: app->bridge(L3)->app - return IPRoute{ - DstNetwork: route.DstNetwork, - Gateway: gatewayIP, - } - } - - // Apply static routes to all endpoints and separately to individual gateways. - // This is to make sure that gateway-app will not receive route that uses app's - // own local IP as the gateway. - epRoutes := generics.MapList(staticRoutes, enforceRouting) - if len(epRoutes) > 0 { + if len(staticRoutes) > 0 { if _, err := io.WriteString(buffer, fmt.Sprintf("dhcp-option=tag:%s,option:classless-static-route,%s\n", - endpointTag, c.formatRoutesForConfig(epRoutes))); err != nil { + endpointTag, c.formatRoutesForConfig(staticRoutes))); err != nil { return writeErr(err) } } @@ -761,7 +712,6 @@ func (c *DnsmasqConfigurator) CreateDnsmasqConfig(buffer io.Writer, dnsmasq Dnsm return !netutils.EqualIPs(route.Gateway, appGatewayIP) } gwRoutes := generics.FilterList(staticRoutes, isRouteValid) - gwRoutes = generics.MapList(gwRoutes, enforceRouting) if len(gwRoutes) > 0 { if _, err := io.WriteString(buffer, fmt.Sprintf("dhcp-option=tag:%s,option:classless-static-route,%s\n", diff --git a/pkg/pillar/nireconciler/genericitems/dnsmasq_test.go b/pkg/pillar/nireconciler/genericitems/dnsmasq_test.go index 96d0666a3d8..a1750cb32a0 100644 --- a/pkg/pillar/nireconciler/genericitems/dnsmasq_test.go +++ b/pkg/pillar/nireconciler/genericitems/dnsmasq_test.go @@ -36,8 +36,7 @@ func exampleDnsmasqParams() genericitems.Dnsmasq { dnsmasq.ListenIf.IfName = "br0" _, subnet, _ := net.ParseCIDR("10.0.0.0/24") dnsmasq.DHCPServer = genericitems.DHCPServer{ - Subnet: subnet, - AllOnesNetmask: true, + Subnet: subnet, IPRange: genericitems.IPRange{ FromIP: net.IP{10, 0, 0, 2}, ToIP: net.IP{10, 0, 0, 123}, @@ -210,23 +209,6 @@ func TestCreateDnsmasqConfigWithoutGateway(t *testing.T) { } } -func TestCreateDnsmasqConfigWithDisabledAllOnesNetmask(t *testing.T) { - t.Parallel() - - dnsmasq := exampleDnsmasqParams() - dnsmasq.DHCPServer.AllOnesNetmask = false - config := createDnsmasqConfig(dnsmasq) - - dhcpRangeRex := "(?m)^dhcp-range=10.0.0.2,10.0.0.123,255.255.255.0,60m$" - ok, err := regexp.MatchString(dhcpRangeRex, config) - if err != nil { - panic(err) - } - if !ok { - t.Fatalf("expected to match '%s', but got '%s'", dhcpRangeRex, config) - } -} - func TestRunDnsmasqInvalidDhcpRange(t *testing.T) { t.Parallel() line, err := configurator.CreateDHCPv4RangeConfig(nil, nil) diff --git a/pkg/pillar/nireconciler/linux_config.go b/pkg/pillar/nireconciler/linux_config.go index 293d15a6b11..515217d8e46 100644 --- a/pkg/pillar/nireconciler/linux_config.go +++ b/pkg/pillar/nireconciler/linux_config.go @@ -746,7 +746,7 @@ func (r *LinuxNIReconciler) getIntendedNIL3Cfg(niID uuid.UUID) dg.Graph { continue } isAppGW := gateway != nil && r.getNISubnet(ni).Contains(gateway) - if isAppGW && r.disableAllOnesNetmask { + if isAppGW { // Route is not needed inside the host, traffic is just forwarded // by the bridge. continue @@ -1055,8 +1055,7 @@ func (r *LinuxNIReconciler) getIntendedDnsmasqCfg(niID uuid.UUID) (items []dg.It } propagateRoutes = generics.FilterDuplicatesFn(propagateRoutes, generic.EqualIPRoutes) dhcpCfg := generic.DHCPServer{ - Subnet: r.getNISubnet(ni), - AllOnesNetmask: !r.disableAllOnesNetmask, + Subnet: r.getNISubnet(ni), IPRange: generic.IPRange{ FromIP: ni.config.DhcpRange.Start, ToIP: ni.config.DhcpRange.End, diff --git a/pkg/pillar/nireconciler/linux_reconciler.go b/pkg/pillar/nireconciler/linux_reconciler.go index f0eaedfdc43..23fc8d0ba11 100644 --- a/pkg/pillar/nireconciler/linux_reconciler.go +++ b/pkg/pillar/nireconciler/linux_reconciler.go @@ -67,9 +67,6 @@ type LinuxNIReconciler struct { exportIntendedState bool withKubernetesNetworking bool - // From GCP - disableAllOnesNetmask bool - reconcileMu sync.Mutex currentState dg.Graph intendedState dg.Graph @@ -801,25 +798,7 @@ func (r *LinuxNIReconciler) ResumeReconcile(ctx context.Context) { // ApplyUpdatedGCP : apply change in the global config properties. func (r *LinuxNIReconciler) ApplyUpdatedGCP(ctx context.Context, newGCP types.ConfigItemValueMap) { - contWatcher := r.pauseWatcher() - defer contWatcher() - disableAllOnesNetmask := newGCP.GlobalValueBool(types.DisableDHCPAllOnesNetMask) - if r.disableAllOnesNetmask == disableAllOnesNetmask { - // No change in GCP relevant for network instances. - return - } - r.disableAllOnesNetmask = disableAllOnesNetmask - for niID, ni := range r.nis { - if ni.config.Type == types.NetworkInstanceTypeSwitch { - // Not running DHCP server for switch NI inside EVE. - continue - } - r.scheduleNICfgRebuild(niID, - fmt.Sprintf("global config property %s changed to %t", - types.DisableDHCPAllOnesNetMask, r.disableAllOnesNetmask)) - } - updates := r.reconcile(ctx) - r.publishReconcilerUpdates(updates...) + // NOOP } // AddNI : create this new network instance inside the network stack. diff --git a/pkg/pillar/nireconciler/linux_test.go b/pkg/pillar/nireconciler/linux_test.go index 4c6ac123320..ad95a1dcf1d 100644 --- a/pkg/pillar/nireconciler/linux_test.go +++ b/pkg/pillar/nireconciler/linux_test.go @@ -2067,42 +2067,6 @@ func TestIPv4LocalAndSwitchNIsWithFlowlogging(test *testing.T) { ni5Config.EnableFlowlog = false } -func TestDisableAllOnesMask(test *testing.T) { - t := initTest(test, false) - networkMonitor.AddOrUpdateInterface(eth0) - networkMonitor.UpdateRoutes(eth0Routes) - ctx := reconciler.MockRun(context.Background()) - updatesCh := niReconciler.WatchReconcilerUpdates() - niReconciler.RunInitialReconcile(ctx) - - // Create local network instance. - _, err := niReconciler.AddNI(ctx, ni1Config, ni1Bridge) - t.Expect(err).ToNot(HaveOccurred()) - var recUpdate nirec.ReconcilerUpdate - t.Eventually(updatesCh).Should(Receive(&recUpdate)) - t.Expect(recUpdate.UpdateType).To(Equal(nirec.NIReconcileStatusChanged)) - networkMonitor.AddOrUpdateInterface(ni1BridgeIf) - - // dnsmasq should advertise mask with all bits set to one. - dnsmasqConf := itemDescription(dg.Reference( - genericitems.Dnsmasq{ListenIf: genericitems.NetworkIf{IfName: "bn1"}})) - t.Expect(dnsmasqConf).To(ContainSubstring("allOnesNetmask: true")) - - // Update global config to disable all ones mask. - gcp := types.DefaultConfigItemValueMap() - gcp.SetGlobalValueBool(types.DisableDHCPAllOnesNetMask, true) - niReconciler.ApplyUpdatedGCP(ctx, *gcp) - - // dnsmasq should now use the mask configured for the NI subnet. - dnsmasqConf = itemDescription(dg.Reference( - genericitems.Dnsmasq{ListenIf: genericitems.NetworkIf{IfName: "bn1"}})) - t.Expect(dnsmasqConf).To(ContainSubstring("allOnesNetmask: false")) - - // Delete network instance - _, err = niReconciler.DelNI(ctx, ni1UUID.UUID) - t.Expect(err).ToNot(HaveOccurred()) -} - func TestIPv6LocalAndSwitchNIs(test *testing.T) { t := initTest(test, false) networkMonitor.AddOrUpdateInterface(keth2) @@ -2767,7 +2731,9 @@ func TestStaticAndConnectedRoutes(test *testing.T) { } recStatus, err := niReconciler.UpdateNI(ctx, ni5Config, ni5Bridge) t.Expect(err).ToNot(HaveOccurred()) - t.Expect(recStatus.Routes).To(HaveLen(5)) + // The list does not include routes with application as a gateway. In those cases, + // the traffic is simply forwarded by the bridge, not routed. + t.Expect(recStatus.Routes).To(HaveLen(3)) t.Expect(recStatus.Routes[0].Equal(types.IPRouteInfo{ IPVersion: types.AddressTypeIPV4, DstNetwork: ipAddressWithPrefix("0.0.0.0/0"), @@ -2775,24 +2741,12 @@ func TestStaticAndConnectedRoutes(test *testing.T) { OutputPort: "ethernet1", })).To(BeTrue()) t.Expect(recStatus.Routes[1].Equal(types.IPRouteInfo{ - IPVersion: types.AddressTypeIPV4, - DstNetwork: ipAddressWithPrefix("10.50.5.0/30"), - Gateway: ipAddress("10.10.20.2"), - GatewayApp: app2UUID.UUID, - })).To(BeTrue()) - t.Expect(recStatus.Routes[2].Equal(types.IPRouteInfo{ - IPVersion: types.AddressTypeIPV4, - DstNetwork: ipAddressWithPrefix("10.50.19.0/30"), - Gateway: ipAddress("10.10.20.2"), - GatewayApp: app2UUID.UUID, - })).To(BeTrue()) - t.Expect(recStatus.Routes[3].Equal(types.IPRouteInfo{ IPVersion: types.AddressTypeIPV4, DstNetwork: ipAddressWithPrefix("10.50.14.0/26"), Gateway: ipAddress("172.30.30.15"), OutputPort: "ethernet3", })).To(BeTrue()) - t.Expect(recStatus.Routes[4].Equal(types.IPRouteInfo{ + t.Expect(recStatus.Routes[2].Equal(types.IPRouteInfo{ IPVersion: types.AddressTypeIPV4, DstNetwork: ipAddressWithPrefix("10.50.1.0/24"), Gateway: ipAddress("172.20.1.1"), @@ -2848,14 +2802,14 @@ func TestStaticAndConnectedRoutes(test *testing.T) { return false } return route.Table == 801 - })).To(Equal(2 + 1)) // subnet route + unreachable route + 1 static route + })).To(Equal(2)) // only IPv4 + IPv6 unreachable routes (N1 is air-gapped) t.Expect(itemCount(func(item dg.Item) bool { route, isRoute := item.(linuxitems.Route) if !isRoute { return false } return route.Table == 805 - })).To(Equal(2 + 5)) // + 5 static routes + })).To(Equal(2 + 3)) // unreachable IPv4/IPv6 routes + static routes // Disconnect the application. appStatus, err = niReconciler.DelAppConn(ctx, app2UUID.UUID) diff --git a/pkg/pillar/types/global.go b/pkg/pillar/types/global.go index 533e208dc20..ffed09c4327 100644 --- a/pkg/pillar/types/global.go +++ b/pkg/pillar/types/global.go @@ -298,9 +298,6 @@ const ( // FmlCustomResolution global setting key FmlCustomResolution GlobalSettingKey = "app.fml.resolution" - // XXX Temporary flag to disable RFC 3442 classless static route usage - DisableDHCPAllOnesNetMask GlobalSettingKey = "debug.disable.dhcp.all-ones.netmask" - // ProcessCloudInitMultiPart to help VMs which do not handle mime multi-part themselves ProcessCloudInitMultiPart GlobalSettingKey = "process.cloud-init.multipart" @@ -963,7 +960,6 @@ func NewConfigItemSpecMap() ConfigItemSpecMap { configItemSpecMap.AddBoolItem(IgnoreMemoryCheckForApps, false) configItemSpecMap.AddBoolItem(IgnoreDiskCheckForApps, false) configItemSpecMap.AddBoolItem(AllowLogFastupload, false) - configItemSpecMap.AddBoolItem(DisableDHCPAllOnesNetMask, false) configItemSpecMap.AddBoolItem(ProcessCloudInitMultiPart, false) configItemSpecMap.AddBoolItem(ConsoleAccess, true) // Controller likely default to false configItemSpecMap.AddBoolItem(VncShimVMAccess, false) diff --git a/pkg/pillar/types/global_test.go b/pkg/pillar/types/global_test.go index 444b41917c6..cf302687018 100644 --- a/pkg/pillar/types/global_test.go +++ b/pkg/pillar/types/global_test.go @@ -207,7 +207,6 @@ func TestNewConfigItemSpecMap(t *testing.T) { SyslogLogLevel, KernelLogLevel, FmlCustomResolution, - DisableDHCPAllOnesNetMask, ProcessCloudInitMultiPart, NetDumpEnable, NetDumpTopicMaxCount,