feat: replace hardcoded service names with ServiceRegistry-driven Caddy and CoreDNS config

Previously, CaddyManager and NetworkManager contained hardcoded lists of
service names (calendar, files, mail, webdav, etc.), meaning every new
service required a code change to appear in Caddy routes and DNS records.
Now both managers accept a service_registry parameter and derive their
service lists dynamically from the registry at runtime.

- CaddyManager: new _build_registry_service_routes() and
  _http01_service_pairs() methods pull routes from the registry
- NetworkManager: new _get_service_subdomains() method returns registry
  subdomains with a hardcoded fallback when no registry is wired in;
  _build_dns_records, stale-record detection, and service name sets all
  use the registry
- managers.py: service_registry constructed before network_manager so it
  can be injected into both CaddyManager and NetworkManager
- service_registry.py: validation chokepoint in get_caddy_routes() rejects
  invalid subdomain/backend values and reserved service names
- service_store_manager.py: _validate_manifest now validates top-level
  subdomain, backend, extra_subdomains, and extra_backends fields
- tests: 24 new tests covering registry-driven routing and DNS subdomain
  generation (test_caddy_registry_integration.py)

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

tests/test_caddy_registry_integration.py

@@ -0,0 +1,528 @@
+"""Integration tests for registry-driven CaddyManager and NetworkManager routing.
+
+These tests cover the new registry path introduced in Step 5 of the PIC Services
+Architecture.  The no-registry (fallback) paths are already covered by
+test_caddy_manager.py and test_network_manager.py.
+"""
+
+import os
+import sys
+import shutil
+import tempfile
+import unittest
+from unittest.mock import MagicMock
+
+sys.path.insert(0, os.path.join(os.path.dirname(__file__), '..', 'api'))
+
+from caddy_manager import CaddyManager  # noqa: E402
+from network_manager import NetworkManager  # noqa: E402
+
+
+# ---------------------------------------------------------------------------
+# Shared helpers
+# ---------------------------------------------------------------------------
+
+def _mgr_with_registry(registry=None):
+    """Build a CaddyManager wired to an optional mock registry."""
+    cm = MagicMock()
+    cm.get_identity.return_value = {}
+    return CaddyManager(config_manager=cm, service_registry=registry)
+
+
+def _mock_registry():
+    """Return a mock ServiceRegistry that reproduces the 3 builtin service routes."""
+    reg = MagicMock()
+    reg.get_caddy_routes.return_value = [
+        {
+            'service_id': 'calendar',
+            'subdomain': 'calendar',
+            'backend': 'cell-radicale:5232',
+            'extra_subdomains': [],
+            'extra_backends': {},
+        },
+        {
+            'service_id': 'email',
+            'subdomain': 'mail',
+            'backend': 'cell-rainloop:8888',
+            'extra_subdomains': ['webmail'],
+            'extra_backends': {},
+        },
+        {
+            'service_id': 'files',
+            'subdomain': 'files',
+            'backend': 'cell-filegator:8080',
+            'extra_subdomains': ['webdav'],
+            'extra_backends': {'webdav': 'cell-webdav:80'},
+        },
+    ]
+    return reg
+
+
+def _nm(registry=None):
+    """Build a NetworkManager backed by temp dirs and an optional mock registry."""
+    tmpdir = tempfile.mkdtemp()
+    nm = NetworkManager(
+        data_dir=os.path.join(tmpdir, 'data'),
+        config_dir=os.path.join(tmpdir, 'config'),
+        service_registry=registry,
+    )
+    nm._tmpdir = tmpdir  # stash so the caller can clean up
+    return nm
+
+
+# ---------------------------------------------------------------------------
+# TestBuildRegistryServiceRoutes
+# ---------------------------------------------------------------------------
+
+class TestBuildRegistryServiceRoutes(unittest.TestCase):
+
+    def test_returns_hardcoded_when_no_registry(self):
+        """service_registry=None produces the same output as _build_core_service_routes."""
+        mgr = _mgr_with_registry(registry=None)
+        domain = 'alpha.pic.ngo'
+        result = mgr._build_registry_service_routes(domain)
+        expected = CaddyManager._build_core_service_routes(domain)
+        self.assertEqual(result, expected)
+
+    def test_returns_hardcoded_when_registry_empty(self):
+        """An empty route list from the registry falls back to hardcoded."""
+        reg = MagicMock()
+        reg.get_caddy_routes.return_value = []
+        mgr = _mgr_with_registry(registry=reg)
+        domain = 'alpha.pic.ngo'
+        result = mgr._build_registry_service_routes(domain)
+        expected = CaddyManager._build_core_service_routes(domain)
+        self.assertEqual(result, expected)
+
+    def test_registry_error_falls_back(self):
+        """When get_caddy_routes raises, output equals _build_core_service_routes."""
+        reg = MagicMock()
+        reg.get_caddy_routes.side_effect = Exception('registry unavailable')
+        mgr = _mgr_with_registry(registry=reg)
+        domain = 'alpha.pic.ngo'
+        result = mgr._build_registry_service_routes(domain)
+        expected = CaddyManager._build_core_service_routes(domain)
+        self.assertEqual(result, expected)
+
+    def test_single_service_no_extras(self):
+        """One service with no extra_subdomains produces one matcher + handle + api block."""
+        reg = MagicMock()
+        reg.get_caddy_routes.return_value = [
+            {
+                'service_id': 'calendar',
+                'subdomain': 'calendar',
+                'backend': 'cell-radicale:5232',
+                'extra_subdomains': [],
+                'extra_backends': {},
+            }
+        ]
+        mgr = _mgr_with_registry(registry=reg)
+        result = mgr._build_registry_service_routes('test.cell')
+        self.assertIn('@calendar host calendar.test.cell', result)
+        self.assertIn('reverse_proxy cell-radicale:5232', result)
+        self.assertIn('@api host api.test.cell', result)
+        self.assertIn('reverse_proxy cell-api:3000', result)
+        # Only two named-matcher definition lines: @calendar and @api
+        matcher_lines = [l for l in result.splitlines() if l.strip().startswith('@') and 'host' in l]
+        self.assertEqual(len(matcher_lines), 2)
+
+    def test_extra_subdomain_same_backend(self):
+        """An extra_subdomain NOT in extra_backends shares the primary matcher host line."""
+        reg = MagicMock()
+        reg.get_caddy_routes.return_value = [
+            {
+                'service_id': 'email',
+                'subdomain': 'mail',
+                'backend': 'cell-rainloop:8888',
+                'extra_subdomains': ['webmail'],
+                'extra_backends': {},  # webmail not listed → shares backend
+            }
+        ]
+        mgr = _mgr_with_registry(registry=reg)
+        result = mgr._build_registry_service_routes('test.cell')
+        # Both subdomains appear in the same host matcher line
+        self.assertIn('@mail host mail.test.cell webmail.test.cell', result)
+        # Only one reverse_proxy for cell-rainloop (shared block)
+        self.assertEqual(result.count('reverse_proxy cell-rainloop:8888'), 1)
+        # No separate @webmail block
+        self.assertNotIn('@webmail host', result)
+
+    def test_extra_subdomain_different_backend(self):
+        """An extra_subdomain listed in extra_backends gets its own matcher + handle block."""
+        reg = MagicMock()
+        reg.get_caddy_routes.return_value = [
+            {
+                'service_id': 'files',
+                'subdomain': 'files',
+                'backend': 'cell-filegator:8080',
+                'extra_subdomains': ['webdav'],
+                'extra_backends': {'webdav': 'cell-webdav:80'},
+            }
+        ]
+        mgr = _mgr_with_registry(registry=reg)
+        result = mgr._build_registry_service_routes('test.cell')
+        # files gets its own block (webdav not in shared list)
+        self.assertIn('@files host files.test.cell', result)
+        self.assertIn('reverse_proxy cell-filegator:8080', result)
+        # webdav gets a separate block
+        self.assertIn('@webdav host webdav.test.cell', result)
+        self.assertIn('reverse_proxy cell-webdav:80', result)
+        # webdav must NOT appear in the @files host line
+        files_line = [l for l in result.splitlines() if '@files host' in l][0]
+        self.assertNotIn('webdav', files_line)
+
+    def test_api_always_appended(self):
+        """The @api block is always the last block even when registry has no api entry."""
+        reg = _mock_registry()
+        mgr = _mgr_with_registry(registry=reg)
+        result = mgr._build_registry_service_routes('alpha.pic.ngo')
+        self.assertIn('@api host api.alpha.pic.ngo', result)
+        self.assertIn('reverse_proxy cell-api:3000', result)
+        # api block is at the end
+        api_idx = result.rfind('@api')
+        other_matchers = ['@calendar', '@mail', '@files', '@webdav']
+        for m in other_matchers:
+            self.assertLess(result.index(m), api_idx,
+                            f'{m} should appear before @api')
+
+    def test_api_not_duplicated_when_registry_returns_api(self):
+        """Even if registry somehow returns an 'api' route, the injected api block is cell-api:3000."""
+        reg = MagicMock()
+        reg.get_caddy_routes.return_value = [
+            {
+                'service_id': 'api',
+                'subdomain': 'api',
+                'backend': 'cell-other:9999',  # wrong backend — should be overridden
+                'extra_subdomains': [],
+                'extra_backends': {},
+            }
+        ]
+        mgr = _mgr_with_registry(registry=reg)
+        result = mgr._build_registry_service_routes('test.cell')
+        # The infrastructure api block is always appended with the canonical backend
+        self.assertIn('reverse_proxy cell-api:3000', result)
+        # api host matcher appears at least once (from registry AND from append)
+        self.assertGreaterEqual(result.count('@api host api.test.cell'), 1)
+
+
+# ---------------------------------------------------------------------------
+# TestHttp01ServicePairs
+# ---------------------------------------------------------------------------
+
+class TestHttp01ServicePairs(unittest.TestCase):
+
+    def test_pairs_from_registry(self):
+        """With the 3 builtins the pairs list matches expected (subdomain, backend) tuples."""
+        reg = _mock_registry()
+        mgr = _mgr_with_registry(registry=reg)
+        pairs = mgr._http01_service_pairs()
+        pairs_dict = dict(pairs)
+        self.assertEqual(pairs_dict['calendar'], 'cell-radicale:5232')
+        self.assertEqual(pairs_dict['mail'], 'cell-rainloop:8888')
+        self.assertEqual(pairs_dict['webmail'], 'cell-rainloop:8888')
+        self.assertEqual(pairs_dict['files'], 'cell-filegator:8080')
+        self.assertEqual(pairs_dict['webdav'], 'cell-webdav:80')
+        self.assertEqual(pairs_dict['api'], 'cell-api:3000')
+
+    def test_webdav_gets_own_backend(self):
+        """webdav must map to cell-webdav:80, not to cell-filegator:8080."""
+        reg = _mock_registry()
+        mgr = _mgr_with_registry(registry=reg)
+        pairs = mgr._http01_service_pairs()
+        webdav_entry = next((b for s, b in pairs if s == 'webdav'), None)
+        self.assertIsNotNone(webdav_entry)
+        self.assertEqual(webdav_entry, 'cell-webdav:80')
+        self.assertNotEqual(webdav_entry, 'cell-filegator:8080')
+
+    def test_fallback_when_no_registry(self):
+        """Without a registry the hardcoded pairs are returned, including api."""
+        mgr = _mgr_with_registry(registry=None)
+        pairs = mgr._http01_service_pairs()
+        subdomains = [s for s, _ in pairs]
+        self.assertIn('calendar', subdomains)
+        self.assertIn('mail', subdomains)
+        self.assertIn('webmail', subdomains)
+        self.assertIn('files', subdomains)
+        self.assertIn('webdav', subdomains)
+        self.assertIn('api', subdomains)
+
+    def test_fallback_when_registry_error(self):
+        """When get_caddy_routes raises, falls back to hardcoded pairs."""
+        reg = MagicMock()
+        reg.get_caddy_routes.side_effect = RuntimeError('boom')
+        mgr = _mgr_with_registry(registry=reg)
+        pairs = mgr._http01_service_pairs()
+        subdomains = [s for s, _ in pairs]
+        self.assertIn('calendar', subdomains)
+        self.assertIn('api', subdomains)
+
+
+# ---------------------------------------------------------------------------
+# TestCaddyfileWithRegistry
+# ---------------------------------------------------------------------------
+
+class TestCaddyfileWithRegistry(unittest.TestCase):
+
+    def _generate(self, domain_mode, cell_name='alpha', domain_name=None,
+                  registry=None, services=None):
+        reg = registry if registry is not None else _mock_registry()
+        mgr = _mgr_with_registry(registry=reg)
+        identity = {'cell_name': cell_name, 'domain_mode': domain_mode}
+        if domain_name:
+            identity['domain_name'] = domain_name
+        return mgr.generate_caddyfile(identity, services or [])
+
+    def test_pic_ngo_with_registry_has_correct_routes(self):
+        """pic_ngo Caddyfile has all service matchers with correct subdomains and backends."""
+        out = self._generate('pic_ngo', cell_name='alpha')
+        # calendar
+        self.assertIn('@calendar host calendar.alpha.pic.ngo', out)
+        self.assertIn('reverse_proxy cell-radicale:5232', out)
+        # mail + webmail share one matcher
+        self.assertIn('@mail host mail.alpha.pic.ngo webmail.alpha.pic.ngo', out)
+        self.assertIn('reverse_proxy cell-rainloop:8888', out)
+        # files
+        self.assertIn('@files host files.alpha.pic.ngo', out)
+        self.assertIn('reverse_proxy cell-filegator:8080', out)
+        # webdav separate block
+        self.assertIn('@webdav host webdav.alpha.pic.ngo', out)
+        self.assertIn('reverse_proxy cell-webdav:80', out)
+        # api always present
+        self.assertIn('@api host api.alpha.pic.ngo', out)
+        self.assertIn('reverse_proxy cell-api:3000', out)
+
+    def test_cloudflare_with_registry_uses_registry_routes(self):
+        """cloudflare Caddyfile routes are sourced from registry, not hardcoded."""
+        out = self._generate('cloudflare', cell_name='beta',
+                             domain_name='example.com')
+        self.assertIn('@calendar host calendar.example.com', out)
+        self.assertIn('@mail host mail.example.com webmail.example.com', out)
+        self.assertIn('@files host files.example.com', out)
+        self.assertIn('@webdav host webdav.example.com', out)
+        self.assertIn('@api host api.example.com', out)
+        # Correct DNS plugin block is still present
+        self.assertIn('dns cloudflare {$CF_API_TOKEN}', out)
+
+    def test_duckdns_with_registry_uses_registry_routes(self):
+        """duckdns Caddyfile routes are sourced from registry."""
+        out = self._generate('duckdns', cell_name='gamma')
+        self.assertIn('@calendar host calendar.gamma.duckdns.org', out)
+        self.assertIn('@api host api.gamma.duckdns.org', out)
+        self.assertIn('dns duckdns {$DUCKDNS_TOKEN}', out)
+
+    def test_http01_with_registry_has_per_host_blocks(self):
+        """http01 Caddyfile has individual per-host blocks for every service subdomain."""
+        out = self._generate('http01', cell_name='delta',
+                             domain_name='delta.noip.me')
+        self.assertIn('calendar.delta.noip.me {', out)
+        self.assertIn('mail.delta.noip.me {', out)
+        self.assertIn('webmail.delta.noip.me {', out)
+        self.assertIn('files.delta.noip.me {', out)
+        self.assertIn('webdav.delta.noip.me {', out)
+        self.assertIn('api.delta.noip.me {', out)
+        # Correct backends
+        self.assertIn('reverse_proxy cell-radicale:5232', out)
+        self.assertIn('reverse_proxy cell-rainloop:8888', out)
+        self.assertIn('reverse_proxy cell-filegator:8080', out)
+        self.assertIn('reverse_proxy cell-webdav:80', out)
+
+    def test_pic_ngo_fallback_when_registry_empty(self):
+        """pic_ngo falls back to hardcoded routes when registry returns empty list."""
+        reg = MagicMock()
+        reg.get_caddy_routes.return_value = []
+        out = self._generate('pic_ngo', cell_name='alpha', registry=reg)
+        # Hardcoded routes should appear
+        self.assertIn('@calendar host calendar.alpha.pic.ngo', out)
+        self.assertIn('@mail host mail.alpha.pic.ngo webmail.alpha.pic.ngo', out)
+
+
+# ---------------------------------------------------------------------------
+# TestNetworkManagerGetServiceSubdomains
+# ---------------------------------------------------------------------------
+
+class TestNetworkManagerGetServiceSubdomains(unittest.TestCase):
+
+    def setUp(self):
+        self.managers = []
+
+    def tearDown(self):
+        for nm in self.managers:
+            shutil.rmtree(nm._tmpdir, ignore_errors=True)
+
+    def _make(self, registry=None):
+        nm = _nm(registry=registry)
+        self.managers.append(nm)
+        return nm
+
+    def test_no_registry_returns_hardcoded(self):
+        """Without a registry the hardcoded service subdomain list is returned."""
+        nm = self._make(registry=None)
+        subs = nm._get_service_subdomains()
+        self.assertCountEqual(subs, ['calendar', 'files', 'mail', 'webmail', 'webdav'])
+
+    def test_registry_returns_all_subdomains(self):
+        """Primary + extra_subdomains from all routes are returned."""
+        reg = _mock_registry()
+        nm = self._make(registry=reg)
+        subs = nm._get_service_subdomains()
+        # calendar (primary), mail (primary), webmail (extra), files (primary), webdav (extra)
+        for expected in ('calendar', 'mail', 'webmail', 'files', 'webdav'):
+            self.assertIn(expected, subs)
+
+    def test_registry_error_falls_back(self):
+        """When get_caddy_routes raises, hardcoded list is returned."""
+        reg = MagicMock()
+        reg.get_caddy_routes.side_effect = Exception('broken registry')
+        nm = self._make(registry=reg)
+        subs = nm._get_service_subdomains()
+        self.assertCountEqual(subs, ['calendar', 'files', 'mail', 'webmail', 'webdav'])
+
+    def test_registry_extra_subdomains_included(self):
+        """extra_subdomains from each route are included in the returned list."""
+        reg = MagicMock()
+        reg.get_caddy_routes.return_value = [
+            {
+                'service_id': 'files',
+                'subdomain': 'files',
+                'backend': 'cell-filegator:8080',
+                'extra_subdomains': ['webdav', 'dav'],
+                'extra_backends': {},
+            }
+        ]
+        nm = self._make(registry=reg)
+        subs = nm._get_service_subdomains()
+        self.assertIn('files', subs)
+        self.assertIn('webdav', subs)
+        self.assertIn('dav', subs)
+
+    def test_build_dns_records_with_registry(self):
+        """All registry subdomains appear as A records in _build_dns_records output."""
+        reg = _mock_registry()
+        nm = self._make(registry=reg)
+        # Override WG IP lookup so we get a predictable value
+        nm._get_wg_server_ip = lambda: '10.0.0.1'
+        records = nm._build_dns_records('mycell', '172.20.0.0/16')
+        names = [r['name'] for r in records]
+        for expected in ('mycell', 'api', 'webui', 'calendar', 'mail',
+                         'webmail', 'files', 'webdav'):
+            self.assertIn(expected, names,
+                          f'{expected!r} should be in DNS records but is not')
+        # All records must point to the WG server IP
+        for r in records:
+            self.assertEqual(r['value'], '10.0.0.1')
+            self.assertEqual(r['type'], 'A')
+
+
+# ---------------------------------------------------------------------------
+# TestNetworkManagerStaleSet
+# ---------------------------------------------------------------------------
+
+class TestNetworkManagerStaleSet(unittest.TestCase):
+    """Verify that registry subdomains drive stale record cleanup in update_split_horizon_zone."""
+
+    def setUp(self):
+        self.test_dir = tempfile.mkdtemp()
+        data_dir = os.path.join(self.test_dir, 'data')
+        config_dir = os.path.join(self.test_dir, 'config')
+        os.makedirs(os.path.join(data_dir, 'dns'), exist_ok=True)
+        os.makedirs(os.path.join(config_dir, 'dns'), exist_ok=True)
+        self.reg = _mock_registry()
+        self.nm = NetworkManager(
+            data_dir=data_dir,
+            config_dir=config_dir,
+            service_registry=self.reg,
+        )
+
+    def tearDown(self):
+        shutil.rmtree(self.test_dir, ignore_errors=True)
+
+    def _write_zone(self, zone_name: str, content: str):
+        path = os.path.join(self.nm.dns_zones_dir, f'{zone_name}.zone')
+        with open(path, 'w') as f:
+            f.write(content)
+
+    def test_stale_set_includes_registry_subdomains(self):
+        """Registry subdomains (calendar, mail, webmail, files, webdav) are treated as
+        stale service records and removed from the parent zone during
+        update_split_horizon_zone."""
+        import subprocess
+        # Build a parent zone with stale service records that the registry knows about
+        stale_records = [
+            {'name': 'pic2',     'type': 'A', 'value': '10.0.0.1'},
+            {'name': 'api',      'type': 'A', 'value': '10.0.0.1'},
+            {'name': 'webui',    'type': 'A', 'value': '10.0.0.1'},
+            {'name': 'calendar', 'type': 'A', 'value': '10.0.0.1'},
+            {'name': 'mail',     'type': 'A', 'value': '10.0.0.1'},
+            {'name': 'webmail',  'type': 'A', 'value': '10.0.0.1'},
+            {'name': 'files',    'type': 'A', 'value': '10.0.0.1'},
+            {'name': 'webdav',   'type': 'A', 'value': '10.0.0.1'},
+        ]
+        from unittest.mock import patch
+        with patch('subprocess.run'):
+            self.nm.update_dns_zone('pic.ngo', stale_records)
+            self.nm.update_split_horizon_zone(
+                'pic2.pic.ngo', '172.20.0.2', primary_domain='pic.ngo'
+            )
+
+        parent_zone = os.path.join(self.nm.dns_zones_dir, 'pic.ngo.zone')
+        content = open(parent_zone).read()
+
+        # All registry subdomains must be gone
+        for stale in ('api', 'webui', 'calendar', 'mail', 'webmail', 'files', 'webdav'):
+            # Check that no line *starts* with the stale name (to avoid false positives
+            # on SOA/NS lines that may contain the zone name as a suffix)
+            lines_with_stale = [
+                l for l in content.splitlines()
+                if l.startswith(stale + ' ') or l.startswith(stale + '\t')
+            ]
+            self.assertEqual(
+                lines_with_stale, [],
+                f'Stale record {stale!r} should have been removed from pic.ngo zone'
+            )
+
+    def test_stale_set_uses_registry_not_hardcoded(self):
+        """When a registry provides a custom subdomain, it is treated as stale too."""
+        custom_reg = MagicMock()
+        custom_reg.get_caddy_routes.return_value = [
+            {
+                'service_id': 'chat',
+                'subdomain': 'chat',
+                'backend': 'cell-chat:9000',
+                'extra_subdomains': ['im'],
+                'extra_backends': {},
+            }
+        ]
+        data_dir = os.path.join(self.test_dir, 'data2')
+        config_dir = os.path.join(self.test_dir, 'config2')
+        os.makedirs(os.path.join(data_dir, 'dns'), exist_ok=True)
+        os.makedirs(os.path.join(config_dir, 'dns'), exist_ok=True)
+        nm = NetworkManager(data_dir=data_dir, config_dir=config_dir,
+                            service_registry=custom_reg)
+
+        stale_records = [
+            {'name': 'pic3',  'type': 'A', 'value': '10.0.0.1'},
+            {'name': 'chat',  'type': 'A', 'value': '10.0.0.1'},
+            {'name': 'im',    'type': 'A', 'value': '10.0.0.1'},
+        ]
+        from unittest.mock import patch
+        with patch('subprocess.run'):
+            nm.update_dns_zone('pic.ngo', stale_records)
+            nm.update_split_horizon_zone(
+                'pic3.pic.ngo', '172.20.0.2', primary_domain='pic.ngo'
+            )
+
+        parent_zone = os.path.join(nm.dns_zones_dir, 'pic.ngo.zone')
+        content = open(parent_zone).read()
+        for stale in ('chat', 'im'):
+            lines_with_stale = [
+                l for l in content.splitlines()
+                if l.startswith(stale + ' ') or l.startswith(stale + '\t')
+            ]
+            self.assertEqual(
+                lines_with_stale, [],
+                f'Custom registry subdomain {stale!r} should have been removed'
+            )
+
+
+if __name__ == '__main__':
+    unittest.main()