Skip to content

dockerd: initialize network config once #29963

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Cryptoteep wants to merge 2 commits into
base: master
Choose a base branch
from
+36 −6
Open

dockerd: initialize network config once #29963

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b27e3c4
dockerd: initialize network config once
Cryptoteep Jul 10, 2026
5c080c8
dockerd: fix firewall membership check silencing its own read
Cryptoteep Jul 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions utils/dockerd/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ include $(TOPDIR)/rules.mk

PKG_NAME:=dockerd
PKG_VERSION:=29.6.1
PKG_RELEASE:=1
PKG_RELEASE:=2
PKG_LICENSE:=Apache-2.0
PKG_LICENSE_FILES:=LICENSE
PKG_CPE_ID:=cpe:/a:mobyproject:moby
Expand Down Expand Up @@ -133,6 +133,9 @@ define Package/dockerd/install
$(INSTALL_DIR) $(1)/etc/init.d
$(INSTALL_BIN) ./files/dockerd.init $(1)/etc/init.d/dockerd

$(INSTALL_DIR) $(1)/etc/uci-defaults
$(INSTALL_BIN) ./files/dockerd.defaults $(1)/etc/uci-defaults/dockerd

$(INSTALL_DIR) $(1)/etc/config
$(INSTALL_CONF) ./files/etc/config/dockerd $(1)/etc/config/dockerd

Expand All @@ -146,7 +149,6 @@ define Package/dockerd/postinst
#!/bin/sh
[ -n "$$IPKG_INSTROOT" ] || {
/etc/init.d/dockerd enable
/etc/init.d/dockerd uciadd
/etc/init.d/dockerd start
}
endef
Expand Down
3 changes: 3 additions & 0 deletions utils/dockerd/files/dockerd.defaults
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
#!/bin/sh

/etc/init.d/dockerd uciadd
12 changes: 8 additions & 4 deletions utils/dockerd/files/dockerd.init
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,14 @@ find_network_device() {
}

boot() {
uciadd
rc_procd start_service

@openwrt-ai openwrt-ai Jul 10, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing uciadd from boot() looks like it regresses the case where dockerd is baked into the firmware image rather than installed at runtime.

Package/dockerd/postinst runs uciadd only inside the [ -n "$IPKG_INSTROOT" ] || { ... } guard, so it is skipped during image build (ImageBuilder/buildroot), where IPKG_INSTROOT is set. The build system's default_postinst re-runs only /etc/uci-defaults/ scripts on first boot, not the package's postinst-pkg body, so the guarded uciadd never runs on such systems. boot() was therefore the only path that created the docker network interface, docker0 bridge device and docker firewall zone on first boot for image-preinstalled dockerd; it was idempotent for runtime installs because uciadd gates every mutation behind if ! uci_quiet get ....

With this change, a device that ships dockerd in its image would boot without the docker network/firewall UCI config ever being created. This lines up with the maintainer's doubt in the PR thread ("I'm not sure the fix is as simple as removing from boot").

If the goal is only to avoid the reload_config at boot (the docker_devices redefinition), consider addressing that specific reload/idempotency issue instead of dropping config creation entirely — e.g. keep the config-creation on first boot but skip reload_config, or move the one-time creation into an /etc/uci-defaults/ script so it is deferred to first boot for image-preinstalled systems. Could you confirm how image-preinstalled installs are expected to get the docker network/firewall config after this change?

Generated by Claude Code

@G-M0N3Y-2503 G-M0N3Y-2503 Jul 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the idempotency is the issue, because we are dealing with the firewall the reload was to ensure the firewall is in the expected shape before dockerd starts if I recall correctly.
I'm not sure why this is an issue all of a sudden, or why reloading an unchanged firewall breaks anything.

@Cryptoteep Cryptoteep Jul 16, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The failure mode is described in #29962: on a device where the docker zone already exists, every boot goes uciadd → reload_config → fw4 reload, and that reload fails with Error: redefinition of symbol 'docker_devices', leaving fw4 running with no instances (reported on 25.12.4 with iptables=0). So the reload is not a harmless no-op — it re-runs fw4 against state where dockerd's nftables objects already exist.

This PR keeps the boot-time uciadd, so the firewall is still ensured to be in the expected shape before dockerd starts (and image-preinstalled setups still get initialized on first boot). The only change is that reload_config is skipped when uciadd didn't actually modify UCI — i.e. the everyday reboot path no longer touches fw4 at all.

}

uciadd() {
local iface="${1}"
local device="${2}"
local zone="${3}"
local changed=0

[ -z "${iface}" ] && {
iface="docker"
Expand All @@ -69,6 +69,7 @@ uciadd() {
uci_quiet set network.@interface[-1].proto="none"
uci_quiet set network.@interface[-1].auto="0"
uci_quiet commit network
changed=1
fi

# Add docker bridge device
Expand All @@ -78,6 +79,7 @@ uciadd() {
uci_quiet set network.@device[-1].type="bridge"
uci_quiet set network.@device[-1].name="${device}"
uci_quiet commit network
changed=1
else
logger -t "dockerd-init" -p notice "Bridge device '${device}' already defined in network config"
fi
Expand All @@ -92,16 +94,18 @@ uciadd() {
uci_quiet set firewall.@zone[-1].forward="ACCEPT"
uci_quiet set firewall.@zone[-1].name="${zone}"
uci_quiet commit firewall
changed=1
fi

# Add interface to firewall zone
if uci_quiet get firewall.${zone}; then
uci_quiet del_list firewall.${zone}.network="${iface}"
if uci_quiet get firewall.${zone} \
&& ! uci_quiet get firewall.${zone}.network | tr ' ' '\n' | grep -Fxq "${iface}"; then

@openwrt-ai openwrt-ai Jul 12, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This membership check never sees the list, so the reload it is meant to gate still fires on every boot. uci_quiet sends its stdout to /dev/null (uci -q "${@}" >/dev/null), so uci_quiet get firewall.${zone}.network | tr ... | grep -Fxq "${iface}" reads an empty stream, grep never matches, and the ! makes the second condition always true. On a normal reboot the zone already exists, so this branch runs add_list + commit firewall every time, sets changed=1, and calls reload_config — defeating the PR's goal of skipping the reload when nothing changed. It also appends a duplicate ${iface} to firewall.${zone}.network on each boot, since uci add_list does not deduplicate (that is why the previous code paired del_list with add_list).

Use the non-silencing form for the read so its output reaches the pipe:

Suggested change
if uci_quiet get firewall.${zone} \
&& ! uci_quiet get firewall.${zone}.network | tr ' ' '\n' | grep -Fxq "${iface}"; then
if uci_quiet get firewall.${zone} \
&& ! uci -q get firewall.${zone}.network | tr ' ' '\n' | grep -Fxq "${iface}"; then

Generated by Claude Code

@openwrt-ai openwrt-ai Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The membership check reads from uci_quiet, which reintroduces a bug that a prior revision had fixed. uci_quiet redirects stdout to /dev/null (uci -q "${@}" >/dev/null, line 13), so uci_quiet get firewall.${zone}.network puts nothing on the pipe: tr/grep see an empty stream, grep -Fxq never matches, and ! makes the condition always true. On a device where the docker zone already exists, this runs add_list + commit firewall and sets changed=1 on every uciadd invocation, so reload_config fires even when nothing changed — defeating this commit's stated goal of reloading only when UCI actually changes (and re-adding a duplicate ${iface}, since uci add_list does not deduplicate). Use the non-silencing read so its output reaches the pipe:

Suggested change
&& ! uci_quiet get firewall.${zone}.network | tr ' ' '\n' | grep -Fxq "${iface}"; then
&& ! uci -q get firewall.${zone}.network | tr ' ' '\n' | grep -Fxq "${iface}"; then

Generated by Claude Code

uci_quiet add_list firewall.${zone}.network="${iface}"
uci_quiet commit firewall
changed=1
fi

reload_config
[ "${changed}" -eq 0 ] || reload_config
}

ucidel() {
Expand Down
21 changes: 21 additions & 0 deletions utils/dockerd/test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
#!/bin/sh

# shellcheck shell=busybox

init_script="${DOCKERD_INIT_SCRIPT:-/etc/init.d/dockerd}"
boot_body="$(sed -n '/^boot() {$/,/^}$/p' "${init_script}")"

echo "${boot_body}" | grep -q 'rc_procd start_service' || {
echo "dockerd boot() does not start the service" >&2
exit 1
}

if echo "${boot_body}" | grep -qw uciadd; then
echo "dockerd boot() must not re-run one-time UCI initialization" >&2
exit 1
fi

grep -q '\[ "${changed}" -eq 0 \] || reload_config' "${init_script}" || {
echo "dockerd uciadd() must reload only after changing UCI" >&2
exit 1
}