From 3f2a441dfd8075ea49958e9f9825e07365b9ff41 Mon Sep 17 00:00:00 2001 From: Cory Snider Date: Wed, 4 Nov 2026 12:10:04 -0500 Subject: [PATCH] libnetwork/d/bridge: clean up legacy FORWARD rules When the daemon started with the nftables backend after a release older than 28.0.1 had run with iptables, the iptables cleaner's DelNetwork left the per-bridge rules those releases put in filter-FORWARD: "-i docker0 ! -o docker0 -j ACCEPT" and the ICC rules. The iptables backend deleted them in setupNonInternalNetworkRules and setIcc, but only when adding a network's rules, and DelNetwork returned early for an internal network. Delete all the legacy per-bridge FORWARD rules in deleteLegacyFilterRules, which the iptables backend and the cleaner call for every network. With the iptables backend, setupIPTables deletes the legacy rules after calling setupNonInternalNetworkRules, which used to delete "-i docker0 ! -o docker0 -j ACCEPT" before adding the DOCKER-FORWARD rule that replaces it. Now, for a moment, both rules are present. Both accept outgoing packets from the bridge. Signed-off-by: Cory Snider --- .../bridge/internal/iptabler/cleaner.go | 11 +-- .../bridge/internal/iptabler/iptabler_test.go | 47 ++++++++++++ .../bridge/internal/iptabler/network.go | 76 +++++-------------- 3 files changed, 74 insertions(+), 60 deletions(-) diff --git a/daemon/libnetwork/drivers/bridge/internal/iptabler/cleaner.go b/daemon/libnetwork/drivers/bridge/internal/iptabler/cleaner.go index 3da6b18212..71329684b6 100644 --- a/daemon/libnetwork/drivers/bridge/internal/iptabler/cleaner.go +++ b/daemon/libnetwork/drivers/bridge/internal/iptabler/cleaner.go @@ -64,20 +64,21 @@ func NewCleaner(ctx context.Context, config firewaller.Config) firewaller.Firewa } 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) + } } } diff --git a/daemon/libnetwork/drivers/bridge/internal/iptabler/iptabler_test.go b/daemon/libnetwork/drivers/bridge/internal/iptabler/iptabler_test.go index 94537439fe..b09a62de5d 100644 --- a/daemon/libnetwork/drivers/bridge/internal/iptabler/iptabler_test.go +++ b/daemon/libnetwork/drivers/bridge/internal/iptabler/iptabler_test.go @@ -104,6 +104,53 @@ 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. +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) + } +} + // 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") diff --git a/daemon/libnetwork/drivers/bridge/internal/iptabler/network.go b/daemon/libnetwork/drivers/bridge/internal/iptabler/network.go index 9912ef4e77..9dd0a684c9 100644 --- a/daemon/libnetwork/drivers/bridge/internal/iptabler/network.go +++ b/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 }