Rework the interface to libnet/internal/nftables

Add nftables.Modifier, to hold a queue of commands that can be applied
using Modifier.Apply. No updates are made to the underlying Table
until Apply is called, errors in the queue if commands are deferred
until Apply.

This has the advantages that:
- less error handling is needed in code that generates update commands
- it's transactional, without needing explicit transactions

Minor disadvantages are that t's slightly more difficult to debug updates,
as it's no longer possible to step through the call making an update to
the Table manipulation in a debugger - and errors in the command, and
errors like trying to update a nonexistent chain/set/vmap, deleting an
object that doesn't exist or creating a duplicate are not reported
until the updates are applied (so, it's a little less clear where
the update came from).

Signed-off-by: Rob Murray <rob.murray@docker.com>

Author: robmry

libnetwork/drivers/bridge/internal/nftabler/endpoint.go

@@ -25,18 +25,24 @@ func (n *network) DelEndpoint(ctx context.Context, epIPv4, epIPv6 netip.Addr) er
 
 func (n *network) modEndpoint(ctx context.Context, epIPv4, epIPv6 netip.Addr, enable bool) error {
 	if n.fw.config.IPv4 && epIPv4.IsValid() {
-		if err := n.filterDirectAccess(ctx, n.fw.table4, n.config.Config4, epIPv4, enable); err != nil {
-			return err
+		tm := n.fw.table4.Modifier()
+		updater := tm.Create
+		if !enable {
+			updater = tm.Delete
 		}
-		if err := nftApply(ctx, n.fw.table4); err != nil {
+		n.filterDirectAccess(updater, tm.Family(), n.config.Config4, epIPv4)
+		if err := tm.Apply(ctx); err != nil {
 			return fmt.Errorf("adding rules for bridge %s: %w", n.config.IfName, err)
 		}
 	}
 	if n.fw.config.IPv6 && epIPv6.IsValid() {
-		if err := n.filterDirectAccess(ctx, n.fw.table6, n.config.Config6, epIPv6, enable); err != nil {
-			return err
+		tm := n.fw.table6.Modifier()
+		updater := tm.Create
+		if !enable {
+			updater = tm.Delete
 		}
-		if err := nftApply(ctx, n.fw.table6); err != nil {
+		n.filterDirectAccess(updater, tm.Family(), n.config.Config6, epIPv6)
+		if err := tm.Apply(ctx); err != nil {
 			return fmt.Errorf("adding rules for bridge %s: %w", n.config.IfName, err)
 		}
 	}
@@ -53,17 +59,21 @@ func (n *network) modEndpoint(ctx context.Context, epIPv4, epIPv6 netip.Addr, en
 //     kernel support).
 //
 // Packets originating on the bridge's own interface and addressed directly to the
-// container are allowed - the host always has direct access to its own containers
-// (it doesn't need to use the port mapped to its own addresses, although it can).
+// container are allowed - the host always has direct access to its own containers.
+// (It doesn't need to use the port mapped to its own addresses, although it can.)
 //
 // "Trusted interfaces" are treated in the same way as the bridge itself.
-func (n *network) filterDirectAccess(ctx context.Context, table nftables.TableRef, conf firewaller.NetworkConfigFam, epIP netip.Addr, enable bool) error {
+func (n *network) filterDirectAccess(updater func(nftables.Obj), fam nftables.Family, conf firewaller.NetworkConfigFam, epIP netip.Addr) {
 	if n.config.Internal || conf.Unprotected || conf.Routed || n.fw.config.AllowDirectRouting {
-		return nil
+		return
 	}
-	updater := table.ChainUpdateFunc(ctx, rawPreroutingChain, enable)
 	ifNames := strings.Join(n.config.TrustedHostInterfaces, ", ")
-	return updater(ctx, rawPreroutingPortsRuleGroup,
-		`%s daddr %s iifname != { %s, %s } counter drop comment "DROP DIRECT ACCESS"`,
-		table.Family(), epIP, n.config.IfName, ifNames)
+	updater(nftables.Rule{
+		Chain: rawPreroutingChain,
+		Group: rawPreroutingPortsRuleGroup,
+		Rule: []string{
+			string(fam), "daddr", epIP.String(),
+			"iifname != {", n.config.IfName, ",", ifNames, `} counter drop comment "DROP DIRECT ACCESS"`,
+		},
+	})
 }

libnetwork/drivers/bridge/internal/nftabler/link.go

@@ -7,8 +7,10 @@ import (
 	"errors"
 	"fmt"
 	"net/netip"
+	"strconv"
 
 	"github.com/containerd/log"
+	"github.com/docker/docker/libnetwork/internal/nftables"
 	"github.com/docker/docker/libnetwork/types"
 )
 
@@ -24,44 +26,47 @@ func (n *network) AddLink(ctx context.Context, parentIP, childIP netip.Addr, por
 		n.fw.cleaner.DelLink(ctx, n.config, parentIP, childIP, ports)
 	}
 
-	chain := n.fw.table4.Chain(ctx, chainFilterFwdIn(n.config.IfName))
+	tm := n.fw.table4.Modifier()
 	for _, port := range ports {
-		for _, rule := range legacyLinkRules(parentIP, childIP, port) {
-			if err := chain.AppendRule(ctx, fwdInLegacyLinksRuleGroup, rule); err != nil {
-				return err
-			}
-		}
+		updateLegacyLinkRules(tm.Create, chainFilterFwdIn(n.config.IfName), parentIP, childIP, port)
 	}
-	if err := nftApply(ctx, n.fw.table4); err != nil {
+	if err := tm.Apply(ctx); err != nil {
 		return fmt.Errorf("adding rules for bridge %s: %w", n.config.IfName, err)
 	}
 	return nil
 }
 
 func (n *network) DelLink(ctx context.Context, parentIP, childIP netip.Addr, ports []types.TransportPort) {
-	chain := n.fw.table4.Chain(ctx, chainFilterFwdIn(n.config.IfName))
+	tm := n.fw.table4.Modifier()
 	for _, port := range ports {
-		for _, rule := range legacyLinkRules(parentIP, childIP, port) {
-			if err := chain.DeleteRule(ctx, fwdInLegacyLinksRuleGroup, rule); err != nil {
-				log.G(ctx).WithFields(log.Fields{
-					"rule":  rule,
-					"error": err,
-				}).Warn("Failed to remove link between containers")
-			}
-		}
+		updateLegacyLinkRules(tm.Delete, chainFilterFwdIn(n.config.IfName), parentIP, childIP, port)
 	}
-	if err := nftApply(ctx, n.fw.table4); err != nil {
+	if err := tm.Apply(ctx); err != nil {
 		log.G(ctx).WithError(err).Warn("Removing link, failed to update nftables")
 	}
 }
 
-func legacyLinkRules(parentIP, childIP netip.Addr, port types.TransportPort) []string {
+func updateLegacyLinkRules(updater func(command nftables.Obj), chainName string, parentIP, childIP netip.Addr, port types.TransportPort) {
 	// TODO(robmry) - could combine rules for each proto by using an anonymous set.
-	return []string{
-		// Match the iptables implementation, but without checking iifname/oifname (not needed
-		// because the addresses belong to the bridge).
-		fmt.Sprintf("ip saddr %s ip daddr %s %s dport %d counter accept", parentIP.Unmap(), childIP.Unmap(), port.Proto, port.Port),
-		// Conntrack will allow responses. So, this must be to allow unsolicited packets from an exposed port.
-		fmt.Sprintf("ip daddr %s ip saddr %s %s sport %d counter accept", parentIP.Unmap(), childIP.Unmap(), port.Proto, port.Port),
-	}
+	// Match the iptables implementation, but without checking iifname/oifname (not needed
+	// because the addresses belong to the bridge).
+	updater(nftables.Rule{
+		Chain: chainName,
+		Group: fwdInLegacyLinksRuleGroup,
+		Rule: []string{
+			"ip saddr", parentIP.Unmap().String(),
+			"ip daddr", childIP.Unmap().String(), port.Proto.String(), "dport", strconv.Itoa(int(port.Port)),
+			"counter accept",
+		},
+	})
+	// Conntrack will allow responses. So, this must be to allow unsolicited packets from an exposed port.
+	updater(nftables.Rule{
+		Chain: chainName,
+		Group: fwdInLegacyLinksRuleGroup,
+		Rule: []string{
+			"ip daddr", parentIP.Unmap().String(),
+			"ip saddr", childIP.Unmap().String(), port.Proto.String(), "sport", strconv.Itoa(int(port.Port)),
+			"counter accept",
+		},
+	})
 }