From 50416cf892d294dca7e5df611ce136bd011f197b Mon Sep 17 00:00:00 2001 From: Cory Snider Date: Wed, 4 Nov 2026 12:12:09 -0500 Subject: [PATCH] libnetwork/d/bridge: remove leftover DOCKER chain When the daemon started with the nftables backend after a release older than 28.0.0 had run with iptables, NewCleaner deleted the jumps from filter-FORWARD to DOCKER-FORWARD and DOCKER-ISOLATION-STAGE-1, then removed Docker's chains. Those releases also jumped from FORWARD to the filter table's DOCKER chain for each bridge ("-o docker0 -j DOCKER"). iptables refuses to delete a chain that a rule still jumps to, and removeIPChains ignores the error. The cleaner's DelNetwork deleted the per-bridge jump later, when the bridge driver restored the network, but nothing tried to remove the chain again. The empty DOCKER chain remained. When the bridge driver has restored its networks and endpoints, call the cleaner's new Done method, which removes the chains again. Signed-off-by: Cory Snider --- .../libnetwork/drivers/bridge/bridge_store.go | 6 +- .../bridge/internal/firewaller/firewaller.go | 5 + .../bridge/internal/iptabler/cleaner.go | 22 +++- .../bridge/internal/iptabler/iptabler_test.go | 10 +- .../bridge/internal/nftabler/cleaner.go | 9 ++ .../network/bridge/bridge_linux_test.go | 104 ++++++++++++++++++ 6 files changed, 149 insertions(+), 7 deletions(-) diff --git a/daemon/libnetwork/drivers/bridge/bridge_store.go b/daemon/libnetwork/drivers/bridge/bridge_store.go index 1bf0f5521e..4ec532fcef 100644 --- a/daemon/libnetwork/drivers/bridge/bridge_store.go +++ b/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 diff --git a/daemon/libnetwork/drivers/bridge/internal/firewaller/firewaller.go b/daemon/libnetwork/drivers/bridge/internal/firewaller/firewaller.go index 31320fff7d..4237ac2c2d 100644 --- a/daemon/libnetwork/drivers/bridge/internal/firewaller/firewaller.go +++ b/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 diff --git a/daemon/libnetwork/drivers/bridge/internal/iptabler/cleaner.go b/daemon/libnetwork/drivers/bridge/internal/iptabler/cleaner.go index 71329684b6..b6a7312534 100644 --- a/daemon/libnetwork/drivers/bridge/internal/iptabler/cleaner.go +++ b/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,7 +63,7 @@ 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) { @@ -102,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 -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) + } +} diff --git a/daemon/libnetwork/drivers/bridge/internal/iptabler/iptabler_test.go b/daemon/libnetwork/drivers/bridge/internal/iptabler/iptabler_test.go index b09a62de5d..61efe9f533 100644 --- a/daemon/libnetwork/drivers/bridge/internal/iptabler/iptabler_test.go +++ b/daemon/libnetwork/drivers/bridge/internal/iptabler/iptabler_test.go @@ -106,7 +106,9 @@ 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. +// 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") @@ -149,6 +151,12 @@ func TestCleanerPerBridgeRules(t *testing.T) { 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. diff --git a/daemon/libnetwork/drivers/bridge/internal/nftabler/cleaner.go b/daemon/libnetwork/drivers/bridge/internal/nftabler/cleaner.go index 579849f955..227dfe975b 100644 --- a/daemon/libnetwork/drivers/bridge/internal/nftabler/cleaner.go +++ b/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 + } +} diff --git a/integration/network/bridge/bridge_linux_test.go b/integration/network/bridge/bridge_linux_test.go index ac8101cacd..68bba9e690 100644 --- a/integration/network/bridge/bridge_linux_test.go +++ b/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)