Skip to content

Commit

Permalink
All-Ones netmask is not needed for ACLs or flow logging
Browse files Browse the repository at this point in the history 
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 <milan@zededa.com>
  • Loading branch information
@milan-zededa @eriknordmark
milan-zededa authored and eriknordmark committed Nov 22, 2024
1 parent 0b67531 commit a73c78d
Show file tree
Hide file tree
Showing 6 changed files with 32 additions and 163 deletions.
76 changes: 13 additions & 63 deletions pkg/pillar/nireconciler/genericitems/dnsmasq.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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) &&
Expand Down Expand Up @@ -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)
Expand All @@ -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...)
}
Expand Down Expand Up @@ -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) &&
Expand All @@ -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)
}
}
Expand All @@ -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",
Expand Down
26 changes: 4 additions & 22 deletions pkg/pillar/nireconciler/genericitems/dnsmasq_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
Expand Down
5 changes: 2 additions & 3 deletions pkg/pillar/nireconciler/linux_config.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -773,7 +773,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
Expand Down Expand Up @@ -1082,8 +1082,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,
Expand Down
23 changes: 1 addition & 22 deletions pkg/pillar/nireconciler/linux_reconciler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,6 @@ type LinuxNIReconciler struct {
exportIntendedState bool
withKubernetesNetworking bool

// From GCP
disableAllOnesNetmask bool

reconcileMu sync.Mutex
currentState dg.Graph
intendedState dg.Graph
Expand Down Expand Up @@ -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.
Expand Down
58 changes: 6 additions & 52 deletions pkg/pillar/nireconciler/linux_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2081,42 +2081,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)
Expand Down Expand Up @@ -2781,32 +2745,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"),
Expand Down Expand Up @@ -2862,14 +2816,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)
Expand Down
7 changes: 6 additions & 1 deletion pkg/pillar/types/global.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit a73c78d

Please sign in to comment.