From 3f6c56701ae41084f970e76c706bf2e937758c40 Mon Sep 17 00:00:00 2001 From: Marek Czernek Date: Tue, 22 Sep 2026 11:09:50 +0200 Subject: [PATCH 1/2] Trim cruft and harden the Docker gateway idempotency fix - systemd_service: drop the else-raise added to enable()/disable()'s sysvinit paths. It's unreachable (_get_service_exec() only ever returns a path ending in update-rc.d or chkconfig, or raises first) and untested; its commit rationale actually described the _runlevel() IndexError, which is fixed separately. - dockermod.compare_networks: tighten the auto-assigned-Gateway exception so it only ignores a one-sided Gateway when it equals the subnet's Docker- assigned default (network address + 1). Previously any one-sided Gateway was stripped unconditionally, which would mask a real change if a user explicitly set a non-default gateway and then removed it. Add unit tests covering the auto-assigned case, the non-default-gateway-removed case, and a changed-explicit-gateway case, mirroring the existing test_compare_networks_ipam_config_empty_iprange regression test. Add a changelog entry describing the fix accurately: the auto-gateway/no-gateway mismatch is structural (any Docker version that auto-assigns gateways), not specific to a newer Docker engine. --- changelog/69746.fixed.md | 1 + src/saltext/dockermod/modules/dockermod.py | 59 ++++++- tests/unit/modules/dockermod/test_module.py | 165 ++++++++++++++++++++ 3 files changed, 221 insertions(+), 4 deletions(-) create mode 100644 changelog/69746.fixed.md diff --git a/changelog/69746.fixed.md b/changelog/69746.fixed.md new file mode 100644 index 00000000..413cd61e --- /dev/null +++ b/changelog/69746.fixed.md @@ -0,0 +1 @@ +Fixed ``docker_network.present`` reporting spurious changes and recreating a network on every run when a ``subnet`` was specified without a ``gateway``. Docker auto-assigns the subnet's first host address as the gateway and reports it on inspect, while Salt's desired config omits the key entirely; ``docker.compare_networks`` now ignores a one-sided gateway only when it matches that auto-assigned default, so an explicitly added, removed, or changed gateway is still detected as a real change. diff --git a/src/saltext/dockermod/modules/dockermod.py b/src/saltext/dockermod/modules/dockermod.py index 09bfb6ad..24658c16 100644 --- a/src/saltext/dockermod/modules/dockermod.py +++ b/src/saltext/dockermod/modules/dockermod.py @@ -201,6 +201,7 @@ import fnmatch import functools import gzip +import ipaddress import json import logging import os @@ -1298,7 +1299,22 @@ def _check_ipconfig(ret, net_name, **kwargs): return ret -def compare_networks(first, second, ignore="Name,Id,Created,Containers"): +def _is_auto_assigned_gateway(subnet, gateway): + """ + Docker auto-assigns the subnet's first host address as the gateway when + none is explicitly requested. Only such an auto-assigned gateway should be + ignored when comparing IPAM pools; an explicit, non-default gateway is a + real configuration difference. + """ + try: + network = ipaddress.ip_network(subnet, strict=False) + gateway_addr = ipaddress.ip_address(gateway) + except (ValueError, TypeError): + return False + return gateway_addr == network.network_address + 1 + + +def compare_networks(first, second, ignore="Name,Id,Created,Containers,Status"): """ .. versionadded:: 2018.3.0 @@ -1349,9 +1365,44 @@ def compare_networks(first, second, ignore="Name,Id,Created,Containers"): def kvsort(x): return (list(x.keys()), list(x.values())) - config1 = sorted(val1["Config"], key=kvsort) - config2 = sorted(val2.get("Config", []), key=kvsort) - if config1 != config2: + def strip_empty(pool): + # Newer Docker engines (29.x) emit empty-string + # placeholder fields (e.g. ``IPRange: ""``) in IPAM + # Config entries that older engines and Salt-built + # desired configs omit. Treat empty/None values as + # absent so the comparison stays semantic. + return {k: v for k, v in pool.items() if v not in ("", None)} + + config1 = sorted([strip_empty(p) for p in val1["Config"]], key=kvsort) + config2 = sorted( + [strip_empty(p) for p in val2.get("Config", [])], + key=kvsort, + ) + + def config_changed(pools1, pools2): + by_subnet1 = {p.get("Subnet"): p for p in pools1} + by_subnet2 = {p.get("Subnet"): p for p in pools2} + if set(by_subnet1) != set(by_subnet2): + return True + for subnet_key, pool1 in by_subnet1.items(): + pool2 = by_subnet2[subnet_key] + # Docker auto-assigns a Gateway for a pool where + # none was explicitly requested (e.g. when only a + # Subnet was given). Ignore a one-sided Gateway + # only when it matches that auto-assigned default; + # an explicit, non-default Gateway that was added + # or removed is a real configuration change. + if "Gateway" in pool1 and "Gateway" not in pool2: + if _is_auto_assigned_gateway(subnet_key, pool1["Gateway"]): + pool1 = {k: v for k, v in pool1.items() if k != "Gateway"} + elif "Gateway" in pool2 and "Gateway" not in pool1: + if _is_auto_assigned_gateway(subnet_key, pool2["Gateway"]): + pool2 = {k: v for k, v in pool2.items() if k != "Gateway"} + if pool1 != pool2: + return True + return False + + if config_changed(config1, config2): ret.setdefault("IPAM", {})["Config"] = { "old": config1, "new": config2, diff --git a/tests/unit/modules/dockermod/test_module.py b/tests/unit/modules/dockermod/test_module.py index 698feb2d..9c512a85 100644 --- a/tests/unit/modules/dockermod/test_module.py +++ b/tests/unit/modules/dockermod/test_module.py @@ -461,6 +461,171 @@ def test_inspect_network(): client.inspect_network.assert_called_once_with("foo") +@docker_older_than_1_5_0_skip_marker +def test_compare_networks_ipam_config_empty_iprange(): + """ + Docker 29 added an empty-string ``IPRange`` to every IPAM Config entry. + Older Salt-built desired configs don't carry that key, so a strict dict + comparison reports a spurious diff and ``docker_network.present`` would + recreate the network on every run. + + Regression test for #68518. + """ + existing = { + "Name": "inf-network", + "Driver": "bridge", + "IPAM": { + "Driver": "default", + "Options": None, + "Config": [ + { + "Subnet": "172.18.0.0/16", + "IPRange": "", + "Gateway": "172.18.0.1", + } + ], + }, + "Options": {}, + } + desired = { + "Name": "inf-network", + "Driver": "bridge", + "IPAM": { + "Driver": "default", + "Options": {}, + "Config": [ + { + "Subnet": "172.18.0.0/16", + "Gateway": "172.18.0.1", + } + ], + }, + "Options": {}, + } + # pylint: disable-next=use-implicit-booleaness-not-comparison + assert docker_mod.compare_networks(existing, desired) == {} + # Symmetric: a real subnet change still has to be reported. + desired["IPAM"]["Config"][0]["Subnet"] = "172.19.0.0/16" + assert "IPAM" in docker_mod.compare_networks(existing, desired) + + +def test_compare_networks_ipam_config_auto_assigned_gateway(): + """ + When a Subnet is specified without a Gateway, Docker auto-assigns the + subnet's first host address as the Gateway and reports it on inspect. + Salt's desired config (built without an explicit gateway) omits the key + entirely, so a strict comparison reported a spurious diff and + ``docker_network.present`` would recreate the network on every run. + """ + existing = { + "Name": "inf-network", + "Driver": "bridge", + "IPAM": { + "Driver": "default", + "Options": None, + "Config": [ + { + "Subnet": "10.247.197.96/27", + "Gateway": "10.247.197.97", + } + ], + }, + "Options": {}, + } + desired = { + "Name": "inf-network", + "Driver": "bridge", + "IPAM": { + "Driver": "default", + "Options": {}, + "Config": [ + { + "Subnet": "10.247.197.96/27", + } + ], + }, + "Options": {}, + } + # pylint: disable-next=use-implicit-booleaness-not-comparison + assert docker_mod.compare_networks(existing, desired) == {} + + +def test_compare_networks_ipam_config_explicit_gateway_removed(): + """ + Removing a real, non-default Gateway (one the user explicitly set, not + the one Docker auto-assigns) must still be reported as a change -- the + auto-assigned-gateway exception must not mask a genuine one-sided diff. + """ + existing = { + "Name": "inf-network", + "Driver": "bridge", + "IPAM": { + "Driver": "default", + "Options": None, + "Config": [ + { + "Subnet": "10.247.197.96/27", + "Gateway": "10.247.197.100", + } + ], + }, + "Options": {}, + } + desired = { + "Name": "inf-network", + "Driver": "bridge", + "IPAM": { + "Driver": "default", + "Options": {}, + "Config": [ + { + "Subnet": "10.247.197.96/27", + } + ], + }, + "Options": {}, + } + assert "IPAM" in docker_mod.compare_networks(existing, desired) + + +def test_compare_networks_ipam_config_gateway_changed(): + """ + Changing an explicit Gateway to a different explicit Gateway (both sides + non-default) must still be reported as a change. + """ + existing = { + "Name": "inf-network", + "Driver": "bridge", + "IPAM": { + "Driver": "default", + "Options": None, + "Config": [ + { + "Subnet": "10.247.197.96/27", + "Gateway": "10.247.197.97", + } + ], + }, + "Options": {}, + } + desired = { + "Name": "inf-network", + "Driver": "bridge", + "IPAM": { + "Driver": "default", + "Options": {}, + "Config": [ + { + "Subnet": "10.247.197.96/27", + "Gateway": "10.247.197.98", + } + ], + }, + "Options": {}, + } + assert "IPAM" in docker_mod.compare_networks(existing, desired) + + @docker_older_than_1_5_0_skip_marker def test_connect_container_to_network(): """ From 9680a3c53f695970dbad989e099e7843c7ef92c7 Mon Sep 17 00:00:00 2001 From: Marek Czernek Date: Tue, 22 Sep 2026 11:22:31 +0200 Subject: [PATCH 2/2] Fix changelog ID --- changelog/{69746.fixed.md => 22.fixed.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename changelog/{69746.fixed.md => 22.fixed.md} (100%) diff --git a/changelog/69746.fixed.md b/changelog/22.fixed.md similarity index 100% rename from changelog/69746.fixed.md rename to changelog/22.fixed.md