Merge pull request #54129 from corhere/fix-nftables-cleanup-legacy-chains

libnetwork/d/bridge: fix cleanup of 27.x iptables

daemon/libnetwork/drivers/bridge/bridge_store.go

@@ -41,10 +41,10 @@ func (d *driver) initStore() error {
 		return err
 	}
 
-	// If there's a firewall cleaner, it's done its job by cleaning up rules
-	// belonging to the restored networks. So, drop it.
+	// If there's a firewall cleaner, it's cleaned up rules belonging to the
+	// restored networks. Let it finish, then drop it.
 	if nft, ok := d.firewaller.(*nftabler.Nftabler); ok {
-		nft.SetFirewallCleaner(nil)
+		nft.FinishCleanup(context.TODO())
 	}
 
 	return nil

daemon/libnetwork/drivers/bridge/internal/firewaller/firewaller.go

@@ -138,6 +138,11 @@ type FirewallCleaner interface {
 	// DelPorts removes firewall rules associated with the specified port bindings.
 	// It should be called by the new Firewaller when adding new port mappings.
 	DelPorts(ctx context.Context, nc NetworkConfig, pbs []types.PortBinding)
+	// Done deletes rules that could only be deleted after the calls to
+	// DelNetwork, DelEndpoint and DelPorts for restored networks and
+	// endpoints. It should be called by the new Firewaller once they have
+	// been restored.
+	Done(ctx context.Context)
 	// DelLink removes firewall rules associated with a legacy link.
 	// It should be called by the new Firewaller when adding a new legacy link.
 	// (Excluded from the interface at present, it's not required by any current

daemon/libnetwork/drivers/bridge/internal/iptabler/cleaner.go

@@ -13,7 +13,9 @@ import (
 )
 
 type iptablesCleaner struct {
-	config firewaller.Config
+	config   firewaller.Config
+	cleaned4 bool
+	cleaned6 bool
 }
 
 // NewCleaner checks for iptables rules left behind by an old daemon that was using
@@ -27,7 +29,8 @@ type iptablesCleaner struct {
 // this stage, interface names, addresses, etc. are unknown. So, also return
 // FirewallCleaner that the new Firewaller can use to delete those rules while
 // it's setting up those networks/ports (probably on replay from persistent
-// storage).
+// storage). A chain that one of those rules jumps to can't be removed until
+// the rule is deleted, so the FirewallCleaner's Done removes the chains again.
 //
 // If there are no old rules to clean up, return nil.
 func NewCleaner(ctx context.Context, config firewaller.Config) firewaller.FirewallCleaner {
@@ -60,24 +63,25 @@ func NewCleaner(ctx context.Context, config firewaller.Config) firewaller.Firewa
 	if !cleaned4 && !cleaned6 {
 		return nil
 	}
-	return &iptablesCleaner{config: config}
+	return &iptablesCleaner{config: config, cleaned4: cleaned4, cleaned6: cleaned6}
 }
 
 func (ic iptablesCleaner) DelNetwork(ctx context.Context, nc firewaller.NetworkConfig) {
-	if nc.Internal {
-		return
-	}
 	n := network{
 		config: nc,
 		ipt:    &Iptabler{config: ic.config},
 	}
 	if ic.config.IPv4 && nc.Config4.Prefix.IsValid() {
 		_ = deleteLegacyFilterRules(iptables.IPv4, nc.IfName)
-		_ = n.setupNonInternalNetworkRules(ctx, iptables.IPv4, nc.Config4, false)
+		if !nc.Internal {
+			_ = n.setupNonInternalNetworkRules(ctx, iptables.IPv4, nc.Config4, false)
+		}
 	}
 	if ic.config.IPv6 && nc.Config6.Prefix.IsValid() {
 		_ = deleteLegacyFilterRules(iptables.IPv6, nc.IfName)
-		_ = n.setupNonInternalNetworkRules(ctx, iptables.IPv6, nc.Config6, false)
+		if !nc.Internal {
+			_ = n.setupNonInternalNetworkRules(ctx, iptables.IPv6, nc.Config6, false)
+		}
 	}
 }
 
@@ -101,3 +105,16 @@ func (ic iptablesCleaner) DelPorts(ctx context.Context, nc firewaller.NetworkCon
 	}
 	_ = n.DelPorts(ctx, pbs)
 }
+
+// Done removes the chains that NewCleaner couldn't remove. Daemons older than
+// 28.0.0 jumped from filter-FORWARD to DOCKER for each bridge ("-o <bridge> -j
+// DOCKER"), and iptables can't remove a chain that a rule jumps to. DelNetwork
+// deletes those jumps for restored networks.
+func (ic iptablesCleaner) Done(ctx context.Context) {
+	if ic.cleaned4 {
+		removeIPChains(ctx, iptables.IPv4)
+	}
+	if ic.cleaned6 {
+		removeIPChains(ctx, iptables.IPv6)
+	}
+}

daemon/libnetwork/drivers/bridge/internal/iptabler/iptabler_test.go

@@ -104,6 +104,61 @@ func TestCleanupOldIngressChains(t *testing.T) {
 	}
 }
 
+// TestCleanerPerBridgeRules checks that the cleaner's DelNetwork deletes the
+// per-bridge rules that daemons older than 28.0.1 left in filter-FORWARD, for
+// internal and non-internal networks. Then, that Done removes the filter
+// table's DOCKER chain, which NewCleaner can't remove while filter-FORWARD
+// has per-bridge jumps to it.
+func TestCleanerPerBridgeRules(t *testing.T) {
+	skip.If(t, iptables.UsingFirewalld(), "firewalld is running in the host netns, it can't modify rules in the test's netns")
+
+	defer netnsutils.SetupTestOSContext(t)()
+	versions := []iptables.IPVersion{iptables.IPv4, iptables.IPv6}
+	for _, version := range versions {
+		iptable := iptables.GetIptable(version)
+		for _, args := range [][]string{
+			{"-N", dockerChain},
+			{"-N", isolationChain1},
+			{"-A", "FORWARD", "-j", isolationChain1},
+			{"-A", "FORWARD", "-o", "br-restored", "-m", "conntrack", "--ctstate", "RELATED,ESTABLISHED", "-j", "ACCEPT"},
+			{"-A", "FORWARD", "-o", "br-restored", "-j", dockerChain},
+			{"-A", "FORWARD", "-i", "br-restored", "!", "-o", "br-restored", "-j", "ACCEPT"},
+			{"-A", "FORWARD", "-i", "br-restored", "-o", "br-restored", "-j", "ACCEPT"},
+			{"-A", "FORWARD", "-i", "br-internal", "-o", "br-internal", "-j", "ACCEPT"},
+		} {
+			out, err := iptable.Raw(args...)
+			assert.NilError(t, err, "version:%s args:%v out:%s", version, args, out)
+		}
+	}
+
+	fc := NewCleaner(t.Context(), firewaller.Config{IPv4: true, IPv6: true})
+	assert.Assert(t, fc != nil)
+	for _, version := range versions {
+		iptable := iptables.GetIptable(version)
+		assert.Check(t, !iptable.ExistChain(isolationChain1, iptables.Filter), "version:%s", version)
+	}
+
+	for _, nc := range []firewaller.NetworkConfig{
+		{IfName: "br-restored", ICC: true, Masquerade: true},
+		{IfName: "br-internal", ICC: true, Internal: true},
+	} {
+		nc.Config4.Prefix = netip.MustParsePrefix("192.0.2.0/24")
+		nc.Config6.Prefix = netip.MustParsePrefix("2001:db8::/64")
+		fc.DelNetwork(t.Context(), nc)
+	}
+	for _, version := range versions {
+		out, err := iptables.GetIptable(version).Raw("-S", "FORWARD")
+		assert.NilError(t, err)
+		assert.Check(t, is.Equal(string(out), "-P FORWARD ACCEPT\n"), "version:%s", version)
+	}
+
+	fc.Done(t.Context())
+	for _, version := range versions {
+		iptable := iptables.GetIptable(version)
+		assert.Check(t, !iptable.ExistChain(dockerChain, iptables.Filter), "version:%s", version)
+	}
+}
+
 // TestIptabler tests combinations of firewaller options against golden results.
 func TestIptabler(t *testing.T) {
 	skip.If(t, iptables.UsingFirewalld(), "firewalld is running in the host netns, it can't modify rules in the test's netns")

daemon/libnetwork/drivers/bridge/internal/iptabler/network.go

@@ -89,6 +89,10 @@ func (n *network) setupIPTables(ctx context.Context, ipVersion iptables.IPVersio
 		n.registerCleanFunc(func() error {
 			return setupInternalNetworkRules(ctx, n.config.IfName, config.Prefix, n.config.ICC, false)
 		})
+
+		if err := deleteLegacyFilterRules(ipVersion, n.config.IfName); err != nil {
+			return fmt.Errorf("failed to delete legacy rules in filter-FORWARD: %w", err)
+		}
 	} else {
 		if err := n.setupNonInternalNetworkRules(ctx, ipVersion, config, true); err != nil {
 			return fmt.Errorf("Failed to Setup IP tables: %w", err)
@@ -219,43 +223,27 @@ func addNATJumpRules(ipVer iptables.IPVersion, hairpinMode, enable bool) error {
 	return nil
 }
 
-// deleteLegacyFilterRules removes the legacy per-bridge rules from the filter-FORWARD
-// chain. This is required for users upgrading the Engine to v28.0.
+// deleteLegacyFilterRules removes the per-bridge rules that daemons older than
+// 28.0.1 added to the filter-FORWARD chain. The rules that replaced them are in
+// Docker's own chains.
 // TODO(aker): drop this function once Mirantis latest LTS is v28.0 (or higher).
 func deleteLegacyFilterRules(ipVer iptables.IPVersion, bridgeName string) error {
-	iptable := iptables.GetIptable(ipVer)
-	// Delete legacy per-bridge jump to the DOCKER chain from the FORWARD chain, if it exists.
-	// These rules have been replaced by an ipset-matching rule.
-	link := []string{
-		"-o", bridgeName,
-		"-j", dockerChain,
-	}
-	if iptable.Exists(iptables.Filter, "FORWARD", link...) {
-		del := append([]string{string(iptables.Delete), "FORWARD"}, link...)
-		if output, err := iptable.Raw(del...); err != nil {
-			return err
-		} else if len(output) != 0 {
-			return fmt.Errorf("could not delete linking rule from %s-%s: %s", iptables.Filter, dockerChain, output)
+	for _, args := range [][]string{
+		// Replaced by an ipset-matching rule in 28.0.0.
+		{"-o", bridgeName, "-j", dockerChain},
+		{"-o", bridgeName, "-m", "conntrack", "--ctstate", "RELATED,ESTABLISHED", "-j", "ACCEPT"},
+		// Outgoing packets, without and with ICC.
+		{"-i", bridgeName, "!", "-o", bridgeName, "-j", "ACCEPT"},
+		{"-i", bridgeName, "-j", "ACCEPT"},
+		// ICC.
+		{"-i", bridgeName, "-o", bridgeName, "-j", "ACCEPT"},
+		{"-i", bridgeName, "-o", bridgeName, "-j", "DROP"},
+	} {
+		rule := iptables.Rule{IPVer: ipVer, Table: iptables.Filter, Chain: "FORWARD", Args: args}
+		if err := rule.Delete(); err != nil {
+			return fmt.Errorf("deleting legacy rule %q: %w", rule, err)
 		}
 	}
-
-	// Delete legacy per-bridge related/established rule if it exists. These rules
-	// have been replaced by an ipset-matching rule.
-	establish := []string{
-		"-o", bridgeName,
-		"-m", "conntrack",
-		"--ctstate", "RELATED,ESTABLISHED",
-		"-j", "ACCEPT",
-	}
-	if iptable.Exists(iptables.Filter, "FORWARD", establish...) {
-		del := append([]string{string(iptables.Delete), "FORWARD"}, establish...)
-		if output, err := iptable.Raw(del...); err != nil {
-			return err
-		} else if len(output) != 0 {
-			return fmt.Errorf("could not delete establish rule from %s-%s: %s", iptables.Filter, dockerChain, output)
-		}
-	}
-
 	return nil
 }
 
@@ -371,12 +359,6 @@ func (n *network) setupNonInternalNetworkRules(ctx context.Context, ipVer iptabl
 		"!", "-o", n.config.IfName,
 		"-j", "ACCEPT",
 	}}
-	// If there's a version of outRuleNoICC in the FORWARD chain, created by moby 28.0.0 or older, delete it.
-	if enable {
-		if err := outRuleNoICC.WithChain("FORWARD").Delete(); err != nil {
-			return fmt.Errorf("deleting FORWARD chain outRuleNoICC: %w", err)
-		}
-	}
 	if n.config.ICC {
 		// Accept outgoing traffic to anywhere, including other containers on this bridge.
 		outRuleICC := iptables.Rule{IPVer: ipVer, Table: iptables.Filter, Chain: dockerForwardChain, Args: []string{
@@ -386,12 +368,6 @@ func (n *network) setupNonInternalNetworkRules(ctx context.Context, ipVer iptabl
 		if err := appendOrDelChainRule(outRuleICC, "ACCEPT OUTGOING", enable); err != nil {
 			return err
 		}
-		// If there's a version of outRuleICC in the FORWARD chain, created by moby 28.0.0 or older, delete it.
-		if enable {
-			if err := outRuleICC.WithChain("FORWARD").Delete(); err != nil {
-				return fmt.Errorf("deleting FORWARD chain outRuleICC: %w", err)
-			}
-		}
 	} else {
 		// Accept outgoing traffic to anywhere, apart from other containers on this bridge.
 		// setIcc added a DROP rule for ICC traffic.
@@ -432,16 +408,6 @@ func setIcc(ctx context.Context, version iptables.IPVersion, bridgeIface string,
 			log.G(ctx).WithError(err).Warn("Failed to delete ICC drop rule")
 		}
 	}
-
-	// Delete rules that may have been inserted into the FORWARD chain by moby 28.0.0 or older.
-	if insert {
-		if err := acceptRule.WithChain("FORWARD").Delete(); err != nil {
-			return fmt.Errorf("deleting FORWARD chain accept rule: %w", err)
-		}
-		if err := dropRule.WithChain("FORWARD").Delete(); err != nil {
-			return fmt.Errorf("deleting FORWARD chain drop rule: %w", err)
-		}
-	}
 	return nil
 }
 

daemon/libnetwork/drivers/bridge/internal/nftabler/cleaner.go

@@ -37,3 +37,12 @@ func tryCleanup(ctx context.Context, family nftables.Family, label string) {
 func (nft *Nftabler) SetFirewallCleaner(fc firewaller.FirewallCleaner) {
 	nft.cleaner = fc
 }
+
+// FinishCleanup tells the FirewallCleaner, if there is one, that networks and
+// endpoints have been restored. Then, it drops the FirewallCleaner.
+func (nft *Nftabler) FinishCleanup(ctx context.Context) {
+	if nft.cleaner != nil {
+		nft.cleaner.Done(ctx)
+		nft.cleaner = nil
+	}
+}

integration/network/bridge/bridge_linux_test.go

@@ -1125,6 +1125,110 @@ func TestFirewallBackendSwitch(t *testing.T) {
 	assert.Check(t, !nftablesTablesExist(), "nftables tables exist after running with iptables")
 }
 
+// TestFirewallBackendSwitchFrom27 checks that, when the daemon starts with the
+// nftables backend after a 27.x release ran with iptables, it deletes the 27.x
+// release's chains and rules. In that layout, the filter-FORWARD chain has
+// per-bridge rules, including a jump to the DOCKER chain.
+func TestFirewallBackendSwitchFrom27(t *testing.T) {
+	skip.If(t, testEnv.IsRootless, "rootless has its own netns")
+	skip.If(t, networking.FirewalldRunning(), "can't use firewalld in host netns to add rules in L3Segment")
+	_ = setupTest(t)
+
+	// Run in a clean netns.
+	addr4 := netip.MustParseAddr("192.168.126.1")
+	addr6 := netip.MustParseAddr("fd76:c828:41fa::1")
+	l3 := networking.NewL3Segment(t, "test-fwbe27",
+		netip.PrefixFrom(addr4, 24),
+		netip.PrefixFrom(addr6, 64),
+	)
+	defer l3.Destroy(t)
+
+	addr4, addr6 = addr4.Next(), addr6.Next()
+	const hostname = "fwbe27"
+	l3.AddHost(t, hostname, hostname+"-netns", "eth0",
+		netip.PrefixFrom(addr4, 24),
+		netip.PrefixFrom(addr6, 64),
+	)
+	host := l3.Hosts[hostname]
+
+	// Run without OTel because there's no routing from this netns for it.
+	d := daemon.New(t, daemon.WithEnvVars("OTEL_EXPORTER_OTLP_ENDPOINT="))
+
+	// A start with the iptables backend leaves the default bridge network in
+	// the bridge driver's store, so the next start restores it, as it would
+	// after an upgrade from 27.x.
+	host.Do(t, func() {
+		d.Start(t, "--firewall-backend=iptables")
+		d.Stop(t)
+	})
+
+	// Replace the rules with those a 27.x release created for the default
+	// bridge network, from the first generated copy of
+	// iptablesdoc/generated/new-daemon.md. A rule with cmd set is only for
+	// that command. The default bridge network had no IPv6, so ip6tables has
+	// no per-bridge rules.
+	rules27 := []struct {
+		cmd   string
+		table string
+		args  []string
+	}{
+		{table: "filter", args: []string{"-N", "DOCKER"}},
+		{table: "filter", args: []string{"-N", "DOCKER-ISOLATION-STAGE-1"}},
+		{table: "filter", args: []string{"-N", "DOCKER-ISOLATION-STAGE-2"}},
+		{table: "filter", args: []string{"-N", "DOCKER-USER"}},
+		{table: "filter", args: []string{"-A", "FORWARD", "-j", "DOCKER-USER"}},
+		{table: "filter", args: []string{"-A", "FORWARD", "-j", "DOCKER-ISOLATION-STAGE-1"}},
+		{cmd: "iptables", table: "filter", args: []string{"-A", "FORWARD", "-o", "docker0", "-m", "conntrack", "--ctstate", "RELATED,ESTABLISHED", "-j", "ACCEPT"}},
+		{cmd: "iptables", table: "filter", args: []string{"-A", "FORWARD", "-o", "docker0", "-j", "DOCKER"}},
+		{cmd: "iptables", table: "filter", args: []string{"-A", "FORWARD", "-i", "docker0", "!", "-o", "docker0", "-j", "ACCEPT"}},
+		{cmd: "iptables", table: "filter", args: []string{"-A", "FORWARD", "-i", "docker0", "-o", "docker0", "-j", "ACCEPT"}},
+		{cmd: "iptables", table: "filter", args: []string{"-A", "DOCKER-ISOLATION-STAGE-1", "-i", "docker0", "!", "-o", "docker0", "-j", "DOCKER-ISOLATION-STAGE-2"}},
+		{table: "filter", args: []string{"-A", "DOCKER-ISOLATION-STAGE-1", "-j", "RETURN"}},
+		{cmd: "iptables", table: "filter", args: []string{"-A", "DOCKER-ISOLATION-STAGE-2", "-o", "docker0", "-j", "DROP"}},
+		{table: "filter", args: []string{"-A", "DOCKER-ISOLATION-STAGE-2", "-j", "RETURN"}},
+		{table: "filter", args: []string{"-A", "DOCKER-USER", "-j", "RETURN"}},
+		{table: "nat", args: []string{"-N", "DOCKER"}},
+		{table: "nat", args: []string{"-A", "PREROUTING", "-m", "addrtype", "--dst-type", "LOCAL", "-j", "DOCKER"}},
+		{cmd: "iptables", table: "nat", args: []string{"-A", "OUTPUT", "!", "-d", "127.0.0.0/8", "-m", "addrtype", "--dst-type", "LOCAL", "-j", "DOCKER"}},
+		{cmd: "ip6tables", table: "nat", args: []string{"-A", "OUTPUT", "!", "-d", "::1/128", "-m", "addrtype", "--dst-type", "LOCAL", "-j", "DOCKER"}},
+		{cmd: "iptables", table: "nat", args: []string{"-A", "POSTROUTING", "-s", "172.17.0.0/16", "!", "-o", "docker0", "-j", "MASQUERADE"}},
+		{cmd: "iptables", table: "nat", args: []string{"-A", "DOCKER", "-i", "docker0", "-j", "RETURN"}},
+	}
+	for _, cmd := range []string{"iptables", "ip6tables"} {
+		for _, table := range []string{"filter", "nat"} {
+			host.MustRun(t, cmd, "-t", table, "-F")
+			host.MustRun(t, cmd, "-t", table, "-X")
+		}
+		for _, r := range rules27 {
+			if r.cmd != "" && r.cmd != cmd {
+				continue
+			}
+			host.MustRun(t, cmd, append([]string{"-t", r.table}, r.args...)...)
+		}
+	}
+
+	host.Do(t, func() {
+		d.Start(t, "--firewall-backend=nftables")
+		d.Stop(t)
+	})
+
+	// The daemon doesn't delete DOCKER-USER, or the jump to it. Everything
+	// else in the 27.x rules is Docker's, and must be gone.
+	for _, cmd := range []string{"iptables-save", "ip6tables-save"} {
+		dump := host.MustRun(t, cmd)
+		var remaining []string
+		for line := range strings.SplitSeq(dump, "\n") {
+			if strings.Contains(line, "DOCKER-USER") {
+				continue
+			}
+			if strings.HasPrefix(line, ":DOCKER") || strings.HasPrefix(line, "-A ") {
+				remaining = append(remaining, line)
+			}
+		}
+		assert.Check(t, is.Len(remaining, 0), "%s:\n%s", cmd, dump)
+	}
+}
+
 func TestEmptyPortBindingsBC(t *testing.T) {
 	ctx := setupTest(t)
 	d := daemon.New(t)