Fix ensure_postup_dnat to strip-and-replace all DNAT rules idempotently

_get_dnat_container_ips() used a concatenating docker inspect format that
produced "invalid IP" when containers had multiple network attachments.
The old ensure_postup_dnat appended rather than replacing, so each update
call added a broken duplicate set of rules causing iptables to fail on
startup and tear down wg0 entirely.

Fix _get_dnat_container_ips to use a space separator in the format string
and validate each token as a real IP before accepting it.

Rewrite ensure_postup_dnat with _is_dnat_rule() helper: strips every
managed DNAT/FORWARD rule (any IP, port 53/80) on semicolon-split and
appends a single correct set — fully idempotent regardless of prior state.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

api/wireguard_manager.py

@@ -115,14 +115,22 @@ class WireGuardManager(BaseServiceManager):
 
     def _get_dnat_container_ips(self) -> tuple:
         """Return (dns_ip, caddy_ip) by inspecting running containers."""
+        import re as _re
+        _ip_re = _re.compile(r'^\d{1,3}(?:\.\d{1,3}){3}$')
+
         def _inspect(name, fallback):
             try:
                 r = subprocess.run(
                     ['docker', 'inspect', '--format',
-                     '{{range .NetworkSettings.Networks}}{{.IPAddress}}{{end}}', name],
+                     '{{range .NetworkSettings.Networks}}{{.IPAddress}} {{end}}', name],
                     capture_output=True, text=True, check=False)
-                return r.stdout.strip() or fallback
+                # Network range may return multiple IPs — pick the first valid one.
+                for candidate in r.stdout.split():
+                    candidate = candidate.strip()
+                    if candidate and _ip_re.match(candidate):
+                        return candidate
             except Exception:
+                pass
             return fallback
         return _inspect('cell-dns', '172.20.0.3'), _inspect('cell-caddy', '172.20.0.2')
 
@@ -176,11 +184,28 @@ class WireGuardManager(BaseServiceManager):
             f'sysctl -q net.ipv4.conf.all.rp_filter=1 || true\n'
         )
 
-    def ensure_postup_dnat(self) -> bool:
+    @staticmethod
-        """Update wg0.conf PostUp/PostDown to include DNS (53) and service (80) DNAT rules.
+    def _is_dnat_rule(token: str) -> bool:
+        """Return True if this semicolon-split token is a DNAT/FORWARD rule managed by us."""
+        t = token.strip()
+        if not t.startswith('iptables'):
+            return False
+        # PREROUTING DNAT on ports 53 or 80
+        if 'PREROUTING' in t and 'DNAT' in t and ('--dport 53' in t or '--dport 80' in t):
+            return True
+        # FORWARD accept to eth0 for ports 53 or 80 (service traffic forwarding)
+        if 'FORWARD' in t and '-o eth0' in t and ('--dport 53' in t or '--dport 80' in t):
+            return True
+        return False
 
-        Called at startup so rules persist across WireGuard interface restarts.
+    def ensure_postup_dnat(self) -> bool:
-        Returns True if the file was changed (caller should reload WG config).
+        """Rewrite wg0.conf PostUp/PostDown with the correct DNS/service DNAT rules.
+
+        Strips ALL managed DNAT and FORWARD rules (any IP, any port 53/80) and
+        replaces them with a single correct set for the current container IPs.
+        This is fully idempotent — stale IPs and duplicates are always cleaned.
+
+        Returns True if the file was changed.
         """
         cf = self._config_file()
         if not os.path.exists(cf):
@@ -189,44 +214,47 @@ class WireGuardManager(BaseServiceManager):
             content = f.read()
 
         dns_ip, caddy_ip = self._get_dnat_container_ips()
-        dnat_marker = f'--dport 53 -j DNAT --to-destination {dns_ip}:53'
-        if dnat_marker in content:
-            return False
 
         dnat_up = (
-            f'iptables -t nat -A PREROUTING -i %i -p udp --dport 53 -j DNAT --to-destination {dns_ip}:53; '
+            f'iptables -t nat -A PREROUTING -i %i -p udp --dport 53 -j DNAT --to-destination {dns_ip}:53'
-            f'iptables -t nat -A PREROUTING -i %i -p tcp --dport 53 -j DNAT --to-destination {dns_ip}:53; '
+            f'; iptables -t nat -A PREROUTING -i %i -p tcp --dport 53 -j DNAT --to-destination {dns_ip}:53'
-            f'iptables -t nat -A PREROUTING -i %i -p tcp --dport 80 -j DNAT --to-destination {caddy_ip}:80; '
+            f'; iptables -t nat -A PREROUTING -i %i -p tcp --dport 80 -j DNAT --to-destination {caddy_ip}:80'
-            f'iptables -I FORWARD -i %i -o eth0 -p tcp --dport 80 -j ACCEPT; '
+            f'; iptables -I FORWARD -i %i -o eth0 -p tcp --dport 80 -j ACCEPT'
-            f'iptables -I FORWARD -i %i -o eth0 -p udp --dport 53 -j ACCEPT; '
+            f'; iptables -I FORWARD -i %i -o eth0 -p udp --dport 53 -j ACCEPT'
-            f'iptables -I FORWARD -i %i -o eth0 -p tcp --dport 53 -j ACCEPT'
+            f'; iptables -I FORWARD -i %i -o eth0 -p tcp --dport 53 -j ACCEPT'
         )
         dnat_down = (
-            f'iptables -t nat -D PREROUTING -i %i -p udp --dport 53 -j DNAT --to-destination {dns_ip}:53 2>/dev/null || true; '
+            f'iptables -t nat -D PREROUTING -i %i -p udp --dport 53 -j DNAT --to-destination {dns_ip}:53 2>/dev/null || true'
-            f'iptables -t nat -D PREROUTING -i %i -p tcp --dport 53 -j DNAT --to-destination {dns_ip}:53 2>/dev/null || true; '
+            f'; iptables -t nat -D PREROUTING -i %i -p tcp --dport 53 -j DNAT --to-destination {dns_ip}:53 2>/dev/null || true'
-            f'iptables -t nat -D PREROUTING -i %i -p tcp --dport 80 -j DNAT --to-destination {caddy_ip}:80 2>/dev/null || true; '
+            f'; iptables -t nat -D PREROUTING -i %i -p tcp --dport 80 -j DNAT --to-destination {caddy_ip}:80 2>/dev/null || true'
-            f'iptables -D FORWARD -i %i -o eth0 -p tcp --dport 80 -j ACCEPT 2>/dev/null || true; '
+            f'; iptables -D FORWARD -i %i -o eth0 -p tcp --dport 80 -j ACCEPT 2>/dev/null || true'
-            f'iptables -D FORWARD -i %i -o eth0 -p udp --dport 53 -j ACCEPT 2>/dev/null || true; '
+            f'; iptables -D FORWARD -i %i -o eth0 -p udp --dport 53 -j ACCEPT 2>/dev/null || true'
-            f'iptables -D FORWARD -i %i -o eth0 -p tcp --dport 53 -j ACCEPT 2>/dev/null || true'
+            f'; iptables -D FORWARD -i %i -o eth0 -p tcp --dport 53 -j ACCEPT 2>/dev/null || true'
         )
 
         lines = content.split('\n')
         updated = []
         changed = False
         for line in lines:
-            if line.startswith('PostUp = ') and dnat_marker not in line:
+            if line.startswith('PostUp = ') or line.startswith('PostDown = '):
-                updated.append(line + '; ' + dnat_up)
+                prefix, _, rest = line.partition(' = ')
-                changed = True
+                # Split on ';', strip managed DNAT rules, rejoin
-            elif line.startswith('PostDown = ') and '--dport 53 -j DNAT' not in line:
+                tokens = [t for t in rest.split(';') if not self._is_dnat_rule(t)]
-                updated.append(line + '; ' + dnat_down)
+                clean = '; '.join(t.strip() for t in tokens if t.strip())
+                new_rules = dnat_up if line.startswith('PostUp') else dnat_down
+                new_line = f'{prefix} = {clean}; {new_rules}'
+                if new_line != line:
+                    updated.append(new_line)
                     changed = True
                 else:
                     updated.append(line)
+            else:
+                updated.append(line)
 
         if changed:
             with open(cf, 'w') as f:
                 f.write('\n'.join(updated))
-            logger.info(f'ensure_postup_dnat: updated wg0.conf with DNAT rules '
+            logger.info(f'ensure_postup_dnat: rewrote wg0.conf DNAT rules '
                         f'(dns={dns_ip}, caddy={caddy_ip})')
         return changed
 

tests/test_wireguard_manager.py

@@ -565,11 +565,13 @@ class TestWireGuardSysctlAndPortCheck(unittest.TestCase):
     def test_ensure_postup_dnat_idempotent_when_rules_present(self, mock_run):
         mock_run.return_value.returncode = 0
         mock_run.return_value.stdout = '172.20.0.3'
-        self._write_wg_conf_postup(
+        self._write_wg_conf_postup()
-            extra_postup='iptables -t nat -A PREROUTING -i %i -p udp --dport 53 -j DNAT --to-destination 172.20.0.3:53'
+        # First call: writes all 6 DNAT rules
-        )
+        first = self.wg.ensure_postup_dnat()
-        changed = self.wg.ensure_postup_dnat()
+        self.assertTrue(first)
-        self.assertFalse(changed)
+        # Second call: rules already correct, no change
+        second = self.wg.ensure_postup_dnat()
+        self.assertFalse(second)
 
     def test_ensure_postup_dnat_returns_false_when_no_conf(self):
         changed = self.wg.ensure_postup_dnat()