All-Ones netmask is not needed for ACLs or flow logging

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,18 +86,17 @@ 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)
 }
 
 // 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",

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},
@@ -156,10 +155,10 @@ hostsdir=/run/zedrouter/hosts.br0
 dhcp-hostsdir=/run/zedrouter/dhcp-hosts.br0
 dhcp-option=option:dns-server,10.0.0.1,1.1.1.1
 dhcp-option=option:ntp-server,94.130.35.4,94.16.114.254
-dhcp-option=option:netmask,255.255.255.255
+dhcp-option=option:netmask,255.255.255.0
 dhcp-option=option:router,10.0.0.1
-dhcp-option=tag:endpoint,option:classless-static-route,10.0.0.1/32,0.0.0.0,10.0.0.0/24,10.0.0.1,192.168.1.0/24,10.0.0.1,172.30.0.0/16,10.0.0.1,0.0.0.0/0,10.0.0.1
-dhcp-option=tag:gateway-10-0-0-100,option:classless-static-route,10.0.0.1/32,0.0.0.0,10.0.0.0/24,10.0.0.1,192.168.1.0/24,10.0.0.1,0.0.0.0/0,10.0.0.1
+dhcp-option=tag:endpoint,option:classless-static-route,10.0.0.0/24,0.0.0.0,192.168.1.0/24,10.0.0.1,172.30.0.0/16,10.0.0.100,0.0.0.0/0,10.0.0.1
+dhcp-option=tag:gateway-10-0-0-100,option:classless-static-route,10.0.0.0/24,0.0.0.0,192.168.1.0/24,10.0.0.1,0.0.0.0/0,10.0.0.1
 dhcp-range=10.0.0.2,10.0.0.123,255.255.255.0,60m
 `
 	if configExpected != config {
@@ -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)

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,

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.

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,32 +2731,22 @@ 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"),
 		Gateway:    ipAddress("172.20.0.1"),
 		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)

pkg/pillar/types/global.go

@@ -298,7 +298,12 @@ const (
 	// FmlCustomResolution global setting key
 	FmlCustomResolution GlobalSettingKey = "app.fml.resolution"
 
-	// XXX Temporary flag to disable RFC 3442 classless static route usage
+	// DisableDHCPAllOnesNetMask option is deprecated and has no effect.
+	// Zedrouter no longer uses the all-ones netmask as it adds unnecessary complexity,
+	// causes confusion for some applications, and is no longer required for any EVE
+	// functionality (previously it was supposedly needed for ACLs and flow logging).
+	// We keep the option defined to avoid reporting errors in ZInfoDevice.ConfigItemStatus
+	// for older deployments where this option is still configured.
 	DisableDHCPAllOnesNetMask GlobalSettingKey = "debug.disable.dhcp.all-ones.netmask"
 
 	// ProcessCloudInitMultiPart to help VMs which do not handle mime multi-part themselves