Trim cruft and harden the Docker gateway idempotency fix

changelog/22.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.

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,

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():
     """