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)