Drop the use of ipset #49530

robmry · 2025-02-24T12:35:32Z

- What I did

Commit 0546d90 introduced the use of ipset to reduce the number of rules that need to be processed per-packet, and make the code a bit simpler.

But, docker's used on embedded kernels compiled without support for ipset, so the change is too disruptive.

- How I did it

Replace the two ipset rules with a new chain that writes out the rule's actions long-hand. So ..

This rule:

      -A FORWARD -m set --match-set docker-ext-bridges-v4 dst \
        -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT

Is transformed into a per-bridge rule in new chain DOCKER-CT:

      -A DOCKER-FORWARD -j DOCKER-CT
      -A DOCKER-CT -o docker0 -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT
      -A DOCKER-CT -o bridge1 -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT

And:

      -A FORWARD -m set --match-set docker-ext-bridges-v4 dst -j DOCKER

Is transformed into a per-bridge rule in new chain DOCKER-BRIDGE:

      -A DOCKER-FORWARD -j DOCKER-BRIDGE
      -A DOCKER-BRIDGE -o docker0 -j DOCKER
      -A DOCKER-BRIDGE -o bridge1 -j DOCKER

- How to verify it

The usual tests.

Ran 28.0.0 with some networks, stopped it and updated to this PR, checked that the ipset rules were removed.

- Human readable description for the release notes

- Remove dependency on kernel modules `ip_set`, `ip_set_hash_net` and `netfilter_xt_set`.
  - The dependency was introduced in release 28.0.0 but proved too disruptive. The iptables rules using these modules have been replaced.

akerouanton

LGTM

akerouanton · 2025-02-25T13:48:06Z

integration/network/bridge/iptablesdoc/generated/new-daemon.md

     network that's made it through the DOCKER and isolation chains, whether the
     destination is external or another network.

+The DOCKER-CT chain is an early ACCEPT for any RELATED,ESTABLISHED traffic to a


This reminds me that we're lacking a proper --ctstate INVALID -j DROP but IIRC it could cause issues for some Docker users (although I don't recall the exact details). That's probably something we should add in the next major. I can raise a PR if you want.

Yes, probably best not to try to tackle that for 28.0.1 and risk introducing new issues.

thaJeztah · 2025-02-25T14:32:47Z

I guess after this one, we can also revert the changes in check-config.sh ?

contrib/check-config: add ipset related flags #49510

thaJeztah

code LGTM

(but I'm an iptables illiterate, so don't hold me to those parts)

thaJeztah · 2025-02-25T14:35:42Z

libnetwork/drivers/bridge/setup_ip_tables_linux.go

+	defer func() {
+		if retErr != nil {
+			if err := iptable.RemoveExistingChain(DockerBridgeChain, iptables.Filter); err != nil {
+				log.G(context.TODO()).Warnf("failed on removing iptables FILTER chain %s on cleanup: %v", DockerBridgeChain, err)


For a follow-up, we could use structured logs (at least WithError() for these. (and at some point see where we / if we can punch through contexts).

Yes, there are lots of log lines following the same pattern in that function, we could sort them all out separately. (Or, not, because hopefully we'll delete all the iptables code soon ... first though, it's being moved to a separate package, to make it possible to swap in nftables / firewalld backends - that's in-progress, so I'd rather not make lots of changes here that might get lost when I rebase.

We haven't plumbed in context for any of the bridge driver's initialisation code (or most/all of libnetwork's initialisation code). We could also do that separately.

thaJeztah · 2025-02-25T14:37:57Z

libnetwork/drivers/bridge/setup_ip_tables_linux.go


+	_, err = iptable.NewChain(DockerBridgeChain, iptables.Filter)
+	if err != nil {
+		return fmt.Errorf("failed to create FILTER chain %s: %v", DockerBridgeChain, err)


I'm guessing the underlying error would never be relevant to us, beyond reporting "something failed"? (looking at %v vs %w)

Right, I don't think we'd want to look at the underlying error ... most of the errors in that function use %v, so we could swap them for %w in a follow up.

Commit 0546d90 introduced the use of ipset to reduce the number of rules that need to be processed per-packet, and make the code a bit simpler. But, docker's used on embedded kernels compiled without support for ipset, so the change is too disruptive. Replace the two ipset rules with a new chain that writes out the rule's actions long-hand. So .. This rule: -A FORWARD -m set --match-set docker-ext-bridges-v4 dst \ -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT Is transformed into a per-bridge rule in new chain DOCKER-CT: -A DOCKER-FORWARD -j DOCKER-CT -A DOCKER-CT -o docker0 -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT -A DOCKER-CT -o bridge1 -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT And: -A FORWARD -m set --match-set docker-ext-bridges-v4 dst -j DOCKER Is transformed into a per-bridge rule in new chain DOCKER-BRIDGE: -A DOCKER-FORWARD -j DOCKER-BRIDGE -A DOCKER-BRIDGE -o docker0 -j DOCKER -A DOCKER-BRIDGE -o bridge1 -j DOCKER Signed-off-by: Rob Murray <[email protected]>

robmry · 2025-02-25T15:26:22Z

Rebased, and reworded some text in the iptablesdoc template (@vvoland) ...

-The FORWARD chain rules are numbered in the output above, they are:
+The FORWARD chain rules, explained in the order they appear in the output above, are:

-are appended. Referring to the numbers in the iptables output ...
+are appended. The DOCKER-FORWARD chain rules, explained in the order they appear in
+the output above, are:

robmry · 2025-02-25T15:46:13Z

I guess after this one, we can also revert the changes in check-config.sh ?

contrib/check-config: add ipset related flags #49510

Oh, yes - will do.

vvoland

LGTM

robmry force-pushed the disable_ip_set branch from effdb11 to 6bfcbec Compare February 24, 2025 17:06

robmry changed the title ~~Experimental - disable ip set~~ Drop the use of ipset Feb 24, 2025

robmry force-pushed the disable_ip_set branch from 6bfcbec to a6e157c Compare February 24, 2025 17:11

robmry self-assigned this Feb 24, 2025

robmry added impact/changelog kind/bugfix PR's that fix bugs area/networking/firewalling Networking area/networking/d/bridge Networking version/28.0 labels Feb 24, 2025

robmry added this to the 28.0.1 milestone Feb 24, 2025

robmry force-pushed the disable_ip_set branch 4 times, most recently from 673e09a to 14c96a5 Compare February 24, 2025 21:08

This was referenced Feb 24, 2025

Jump to DOCKER-INGRESS from DOCKER-FORWARD #49538

Merged

Add chain DOCKER-FORWARD #49518

Merged

robmry force-pushed the disable_ip_set branch from 14c96a5 to 20751f1 Compare February 25, 2025 11:26

robmry requested review from akerouanton, thaJeztah and vvoland February 25, 2025 11:27

robmry marked this pull request as ready for review February 25, 2025 11:27

robmry mentioned this pull request Feb 25, 2025

Docker version 28.0.0, build af898ab configures host network with no network access: docker/for-linux#1521

Closed

akerouanton approved these changes Feb 25, 2025

View reviewed changes

thaJeztah approved these changes Feb 25, 2025

View reviewed changes

robmry force-pushed the disable_ip_set branch from 20751f1 to 76417bf Compare February 25, 2025 15:24

vvoland approved these changes Feb 25, 2025

View reviewed changes

thaJeztah merged commit f92fdfe into moby:master Feb 25, 2025
150 checks passed

robmry mentioned this pull request Feb 25, 2025

Revert "contrib/check-config: add ipset related flags" #49545

Merged

vvoland added process/cherry-pick process/cherry-pick/28.x process/cherry-picked area/networking Networking and removed process/cherry-pick process/cherry-pick/28.x labels Feb 26, 2025

robmry deleted the disable_ip_set branch February 26, 2025 14:45

thaJeztah mentioned this pull request Mar 5, 2025

Flush iptables chains DOCKER-CT, DOCKER-BRIDGE on startup #49582

Merged

jtgasper3 mentioned this pull request Mar 9, 2025

Bump github.com/docker/docker from 27.5.1+incompatible to 28.0.1+incompatible jtgasper3/swarm-visualizer#9

Merged

Drop the use of ipset #49530

Drop the use of ipset #49530

Uh oh!

Conversation

robmry commented Feb 24, 2025 • edited by vvoland Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akerouanton left a comment

Choose a reason for hiding this comment

Uh oh!

akerouanton Feb 25, 2025

Choose a reason for hiding this comment

Uh oh!

robmry Feb 25, 2025

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Feb 25, 2025

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah Feb 25, 2025

Choose a reason for hiding this comment

Uh oh!

robmry Feb 25, 2025

Choose a reason for hiding this comment

Uh oh!

thaJeztah Feb 25, 2025

Choose a reason for hiding this comment

Uh oh!

robmry Feb 25, 2025

Choose a reason for hiding this comment

Uh oh!

robmry commented Feb 25, 2025

Uh oh!

robmry commented Feb 25, 2025

Uh oh!

vvoland left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

robmry commented Feb 24, 2025 •

edited by vvoland

Loading