From 14ff4fe1868aa7583f863ed5b839f4aee3937e16 Mon Sep 17 00:00:00 2001 From: claude Date: Mon, 20 Jul 2026 22:46:09 +0000 Subject: [PATCH] fix(lifecycle): build gateway before infra and fix orchestrator port mapping MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - `_build_images()` now builds Dockerfile.gateway → Dockerfile.orchestrator → Dockerfile.infra in order; Dockerfile.infra starts FROM bot-bottle-gateway so the base must exist on clean hosts. - Publish mapping corrected from `self.port:self.port` to `self.port:DEFAULT_PORT` (8099) — gateway_init hardcodes the orchestrator on port 8099 inside the container, so the host-side published port must map to that fixed internal port. - BOT_BOTTLE_ORCHESTRATOR_URL inside the container now always points to 127.0.0.1:8099, not self.port, since gateway daemons reach the orchestrator over loopback at the fixed internal port. - Update test_ensure_running_builds_both_images → _all_images for the new three-step build sequence; add test_publish_maps_host_port_to_fixed_internal_port to lock in the port-mapping fix. Addresses the P1 findings from the didericis-codex review on PR #432. --- bot_bottle/orchestrator/lifecycle.py | 16 ++++++---- tests/unit/test_orchestrator_lifecycle.py | 36 ++++++++++++++++++----- 2 files changed, 40 insertions(+), 12 deletions(-) diff --git a/bot_bottle/orchestrator/lifecycle.py b/bot_bottle/orchestrator/lifecycle.py index 7c06801..5cfd973 100644 --- a/bot_bottle/orchestrator/lifecycle.py +++ b/bot_bottle/orchestrator/lifecycle.py @@ -27,6 +27,8 @@ from ..paths import CONTROL_PLANE_TOKEN_ENV, bot_bottle_root, host_control_plane from ..supervise import DB_PATH_IN_CONTAINER from .gateway import ( GATEWAY_CA_VOLUME, + GATEWAY_DOCKERFILE, + GATEWAY_IMAGE, GATEWAY_NETWORK, GatewayError, MITMPROXY_HOME, @@ -160,9 +162,10 @@ class OrchestratorService: ) def _build_images(self) -> None: - """Build the orchestrator image (build intermediate), then the infra - image. Both are cache-aware: a no-op when nothing changed.""" + """Build the gateway base, the orchestrator intermediate, then the + infra image. All are cache-aware: a no-op when nothing changed.""" for tag, dockerfile in ( + (GATEWAY_IMAGE, GATEWAY_DOCKERFILE), (ORCHESTRATOR_IMAGE, ORCHESTRATOR_DOCKERFILE), (self.image, INFRA_DOCKERFILE), ): @@ -188,7 +191,9 @@ class OrchestratorService: "--label", f"{INFRA_SOURCE_HASH_LABEL}={current_hash}", "--network", self.network, # Host CLI reaches the control plane here (loopback only). - "--publish", f"127.0.0.1:{self.port}:{self.port}", + # gateway_init always starts the orchestrator on DEFAULT_PORT (8099) + # inside the container; self.port is the host-side published port. + "--publish", f"127.0.0.1:{self.port}:{DEFAULT_PORT}", # Persist the mitmproxy CA so it survives container recreation. "--volume", f"{GATEWAY_CA_VOLUME}:{MITMPROXY_HOME}", # Shared supervise DB (same file the operator reads over HTTP). @@ -206,8 +211,9 @@ class OrchestratorService: # Control-plane secret: required by the orchestrator (to enforce) # and by the gateway daemons (to present on /resolve calls). "--env", CONTROL_PLANE_TOKEN_ENV, - # Gateway daemons reach the orchestrator over loopback. - "--env", f"BOT_BOTTLE_ORCHESTRATOR_URL=http://127.0.0.1:{self.port}", + # Gateway daemons reach the orchestrator over loopback at its + # fixed internal port (DEFAULT_PORT), independent of self.port. + "--env", f"BOT_BOTTLE_ORCHESTRATOR_URL=http://127.0.0.1:{DEFAULT_PORT}", # Opt the orchestrator into gateway_init's supervise tree. "--env", f"BOT_BOTTLE_GATEWAY_DAEMONS={_INFRA_DAEMONS}", self.image, diff --git a/tests/unit/test_orchestrator_lifecycle.py b/tests/unit/test_orchestrator_lifecycle.py index 5afdd45..6665a33 100644 --- a/tests/unit/test_orchestrator_lifecycle.py +++ b/tests/unit/test_orchestrator_lifecycle.py @@ -116,7 +116,7 @@ class TestOrchestratorService(unittest.TestCase): daemons_flag = "BOT_BOTTLE_GATEWAY_DAEMONS=egress,git-http,supervise,orchestrator" self.assertIn("orchestrator", argv[argv.index(daemons_flag)]) - def test_ensure_running_builds_both_images(self) -> None: + def test_ensure_running_builds_all_images(self) -> None: calls: list[list[str]] = [] def fake(argv: list[str], **_kw: object) -> Mock: @@ -129,14 +129,36 @@ class TestOrchestratorService(unittest.TestCase): patch(_RUN, side_effect=fake), patch(_SLEEP): self.svc.ensure_running() builds = [c for c in calls if c[:2] == ["docker", "build"]] - # Orchestrator (build intermediate) + infra image both built. - self.assertEqual(2, len(builds)) + # Gateway base + orchestrator intermediate + infra image — all three built. + self.assertEqual(3, len(builds)) dockerfiles = [next(a for a in b if "Dockerfile" in a) for b in builds] - self.assertIn("Dockerfile.orchestrator", dockerfiles[0]) - self.assertIn("Dockerfile.infra", dockerfiles[1]) - # Images are distinct — the point of the split. + self.assertIn("Dockerfile.gateway", dockerfiles[0]) + self.assertIn("Dockerfile.orchestrator", dockerfiles[1]) + self.assertIn("Dockerfile.infra", dockerfiles[2]) + # All three images are distinct. tags = [b[b.index("-t") + 1] for b in builds] - self.assertNotEqual(tags[0], tags[1]) + self.assertEqual(3, len(set(tags))) + + def test_publish_maps_host_port_to_fixed_internal_port(self) -> None: + """A non-default self.port is published to the fixed internal port 8099, + not to self.port:self.port — the orchestrator always listens on 8099.""" + calls: list[list[str]] = [] + + def fake(argv: list[str], **_kw: object) -> Mock: + calls.append(argv) + if argv[:2] == ["docker", "ps"]: + return _proc(stdout="") + return _proc() + + svc = OrchestratorService(port=20001) + with patch(_URLOPEN, side_effect=[urllib.error.URLError("down"), _health(200)]), \ + patch(_RUN, side_effect=fake), patch(_SLEEP): + svc.ensure_running() + runs = [c for c in calls if c[:2] == ["docker", "run"]] + argv = runs[0] + self.assertEqual("127.0.0.1:20001:8099", argv[argv.index("--publish") + 1]) + orch_url = next(a for a in argv if "BOT_BOTTLE_ORCHESTRATOR_URL" in a) + self.assertIn(":8099", orch_url) def test_ensure_running_raises_on_timeout(self) -> None: with patch(_URLOPEN, side_effect=urllib.error.URLError("down")), \