fix(sidecars): apply_routes_change targets the bundle + SIGHUP forwarding
Two bugs surfaced when applying an egress route change:
1. egress_apply.py still targeted claude-bottle-egress-<slug> —
the legacy per-sidecar container that no longer exists (it's
a docker-network alias on the bundle now). Switched it to
sidecar_bundle_container_name(slug), matching the chunk-5
fix already made to pipelock_apply.py.
2. `docker kill --signal HUP <bundle>` lands SIGHUP on the
supervisor (PID 1 in the bundle), which previously had no
SIGHUP handler — the signal was ignored. Added
`_Supervisor.forward_signal(sig, daemon_name)` and a SIGHUP
handler in main() that forwards to the egress daemon so
mitmdump's addon reload still works under the bundle.
Tests:
- New _Supervisor.forward_signal cases: forwards to the named
child (Python subprocess as the SIGHUP target — bash trap +
stdout=PIPE deferral interferes with the production-style
test); unknown-daemon name is a no-op.
Stale-reference cleanup (separate issue surfaced while looking
at this):
- claude_bottle/{egress,git_gate,egress_addon,
egress_addon_core,supervise_server}.py: Dockerfile.egress /
Dockerfile.git-gate / Dockerfile.supervise references updated
to Dockerfile.sidecars (the old per-sidecar Dockerfiles were
deleted in PRD 0024 chunk 5).
- tests/README.md: dropped the entry for
test_pipelock_sidecar_smoke (deleted in chunk 3) and added
the new bundle integration tests.
- git_gate.py: stale `DockerGitGate.start via docker cp`
reference (the method was deleted in chunk 3) rewritten to
the bind-mount path the renderer uses now.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
@@ -31,7 +31,7 @@ from ...egress import EGRESS_ROUTES_IN_CONTAINER
|
|||||||
from ...egress_addon_core import load_routes
|
from ...egress_addon_core import load_routes
|
||||||
from ...yaml_subset import YamlSubsetError, parse_yaml_subset
|
from ...yaml_subset import YamlSubsetError, parse_yaml_subset
|
||||||
from .bottle_state import egress_state_dir
|
from .bottle_state import egress_state_dir
|
||||||
from .egress import egress_container_name
|
from .sidecar_bundle import sidecar_bundle_container_name
|
||||||
from .pipelock_apply import (
|
from .pipelock_apply import (
|
||||||
PipelockApplyError,
|
PipelockApplyError,
|
||||||
apply_allowlist_change,
|
apply_allowlist_change,
|
||||||
@@ -81,7 +81,7 @@ def fetch_current_routes(slug: str) -> str:
|
|||||||
for `slug`. Returns the file content as a string. Raises
|
for `slug`. Returns the file content as a string. Raises
|
||||||
EgressApplyError if the sidecar isn't reachable or the read
|
EgressApplyError if the sidecar isn't reachable or the read
|
||||||
fails."""
|
fails."""
|
||||||
container = egress_container_name(slug)
|
container = sidecar_bundle_container_name(slug)
|
||||||
r = subprocess.run(
|
r = subprocess.run(
|
||||||
["docker", "exec", container, "cat", EGRESS_ROUTES_IN_CONTAINER],
|
["docker", "exec", container, "cat", EGRESS_ROUTES_IN_CONTAINER],
|
||||||
capture_output=True, text=True, check=False,
|
capture_output=True, text=True, check=False,
|
||||||
@@ -185,7 +185,7 @@ def apply_routes_change(slug: str, new_content: str) -> tuple[str, str]:
|
|||||||
|
|
||||||
Returns (before, after) where `after` == `new_content`. Raises
|
Returns (before, after) where `after` == `new_content`. Raises
|
||||||
EgressApplyError on any step."""
|
EgressApplyError on any step."""
|
||||||
container = egress_container_name(slug)
|
container = sidecar_bundle_container_name(slug)
|
||||||
before = fetch_current_routes(slug)
|
before = fetch_current_routes(slug)
|
||||||
validate_routes_content(new_content)
|
validate_routes_content(new_content)
|
||||||
|
|
||||||
|
|||||||
@@ -40,7 +40,7 @@ from .manifest import Bottle
|
|||||||
EGRESS_HOSTNAME = "egress"
|
EGRESS_HOSTNAME = "egress"
|
||||||
|
|
||||||
# In-container path the addon reads. Pre-created in
|
# In-container path the addon reads. Pre-created in
|
||||||
# `Dockerfile.egress` so the host bind-mount can drop the file
|
# `Dockerfile.sidecars` so the host bind-mount can drop the file
|
||||||
# directly. Content is YAML (hand-rolled by `egress_render_routes`
|
# directly. Content is YAML (hand-rolled by `egress_render_routes`
|
||||||
# in the style of `pipelock_render_yaml`, parsed by `yaml_subset`
|
# in the style of `pipelock_render_yaml`, parsed by `yaml_subset`
|
||||||
# inside the addon).
|
# inside the addon).
|
||||||
|
|||||||
@@ -18,7 +18,7 @@ This file imports `mitmproxy` and is never imported on the host —
|
|||||||
mitmproxy is a container-only dependency. The host's tests target
|
mitmproxy is a container-only dependency. The host's tests target
|
||||||
`egress_addon_core`.
|
`egress_addon_core`.
|
||||||
|
|
||||||
Dockerfile.egress copies both this file and
|
Dockerfile.sidecars copies both this file and
|
||||||
`egress_addon_core.py` flat into `/app/`; the absolute import
|
`egress_addon_core.py` flat into `/app/`; the absolute import
|
||||||
below works because mitmdump runs with `/app` on its sys.path. The
|
below works because mitmdump runs with `/app` on its sys.path. The
|
||||||
parallel file in the package source tree (claude_bottle/) is the
|
parallel file in the package source tree (claude_bottle/) is the
|
||||||
|
|||||||
@@ -7,8 +7,8 @@ exercise the parse + decision functions without depending on the
|
|||||||
container.
|
container.
|
||||||
|
|
||||||
Imports: stdlib + `yaml_subset` (which is itself stdlib-only and
|
Imports: stdlib + `yaml_subset` (which is itself stdlib-only and
|
||||||
ships flat into the egress image alongside this file — see
|
ships flat into the sidecar bundle image alongside this file —
|
||||||
`Dockerfile.egress`).
|
see `Dockerfile.sidecars`).
|
||||||
"""
|
"""
|
||||||
|
|
||||||
from __future__ import annotations
|
from __future__ import annotations
|
||||||
@@ -16,13 +16,11 @@ from __future__ import annotations
|
|||||||
import typing
|
import typing
|
||||||
from dataclasses import dataclass
|
from dataclasses import dataclass
|
||||||
|
|
||||||
# Absolute import — `yaml_subset.py` is copied flat into the egress
|
# Absolute import — `yaml_subset.py` is copied flat into the bundle
|
||||||
# image's `/app/` next to this file (via `Dockerfile.egress`). The
|
# image's `/app/` next to this file (via `Dockerfile.sidecars`).
|
||||||
# host-side unit tests run with the repo on sys.path, where the
|
# The host-side unit tests run with the repo on sys.path, where the
|
||||||
# bare `yaml_subset` module also resolves because
|
# import resolves under the `claude_bottle` package. The try/except
|
||||||
# `claude_bottle/yaml_subset.py` shadows it at import time... actually
|
# shim picks whichever import works.
|
||||||
# no, on host the module lives under the `claude_bottle` package.
|
|
||||||
# The try/except shim picks whichever import works.
|
|
||||||
try:
|
try:
|
||||||
from yaml_subset import YamlSubsetError, parse_yaml_subset # type: ignore[import-not-found]
|
from yaml_subset import YamlSubsetError, parse_yaml_subset # type: ignore[import-not-found]
|
||||||
except ImportError: # pragma: no cover - host-side path
|
except ImportError: # pragma: no cover - host-side path
|
||||||
|
|||||||
@@ -147,12 +147,12 @@ def git_gate_known_hosts_line(host: str, port: str, key: str) -> str:
|
|||||||
|
|
||||||
|
|
||||||
def git_gate_render_entrypoint(upstreams: tuple[GitGateUpstream, ...]) -> str:
|
def git_gate_render_entrypoint(upstreams: tuple[GitGateUpstream, ...]) -> str:
|
||||||
"""Posix-sh entrypoint (alpine ash). One `init_repo` call per
|
"""Posix-sh entrypoint. One `init_repo` call per upstream, then
|
||||||
upstream, then `exec git daemon`. The function reads
|
`exec git daemon`. The function reads
|
||||||
`/git-gate/creds/<name>-{key,known_hosts}` (laid down by
|
`/git-gate/creds/<name>-{key,known_hosts}` (bind-mounted into
|
||||||
`DockerGitGate.start` via docker cp) and wires them into each
|
the bundle by the renderer) and wires them into each bare repo's
|
||||||
bare repo's config; the access-hook + pre-receive hook pick those
|
config; the access-hook + pre-receive hook pick those paths up
|
||||||
paths up at fetch / push time."""
|
at fetch / push time."""
|
||||||
lines = [
|
lines = [
|
||||||
"#!/bin/sh",
|
"#!/bin/sh",
|
||||||
"set -eu",
|
"set -eu",
|
||||||
|
|||||||
@@ -200,6 +200,30 @@ class _Supervisor:
|
|||||||
the operator gets that code on container exit."""
|
the operator gets that code on container exit."""
|
||||||
return max((p.returncode for _, p in self.procs), default=0)
|
return max((p.returncode for _, p in self.procs), default=0)
|
||||||
|
|
||||||
|
def forward_signal(self, sig: int, daemon_name: str) -> bool:
|
||||||
|
"""Forward a signal to one named child. Used by the SIGHUP
|
||||||
|
path: egress_apply.py runs `docker kill --signal HUP
|
||||||
|
<bundle>`, the host kernel delivers SIGHUP to PID 1 (this
|
||||||
|
supervisor), and we relay it to mitmdump so it reloads
|
||||||
|
its addon's routes.yaml. Returns True iff a live child by
|
||||||
|
that name was signaled."""
|
||||||
|
for spec, p in self.procs:
|
||||||
|
if spec.name != daemon_name:
|
||||||
|
continue
|
||||||
|
if p.poll() is not None:
|
||||||
|
_log(
|
||||||
|
f"SIGHUP for {daemon_name} dropped; daemon "
|
||||||
|
f"already exited (rc={p.returncode})"
|
||||||
|
)
|
||||||
|
return False
|
||||||
|
try:
|
||||||
|
p.send_signal(sig)
|
||||||
|
except ProcessLookupError:
|
||||||
|
return False
|
||||||
|
_log(f"forwarded {signal.Signals(sig).name} to {daemon_name}")
|
||||||
|
return True
|
||||||
|
return False
|
||||||
|
|
||||||
|
|
||||||
def main(argv: Sequence[str] | None = None) -> int:
|
def main(argv: Sequence[str] | None = None) -> int:
|
||||||
del argv # no flags yet; env-driven only
|
del argv # no flags yet; env-driven only
|
||||||
@@ -213,6 +237,14 @@ def main(argv: Sequence[str] | None = None) -> int:
|
|||||||
|
|
||||||
signal.signal(signal.SIGTERM, lambda *_: sup.request_shutdown("SIGTERM"))
|
signal.signal(signal.SIGTERM, lambda *_: sup.request_shutdown("SIGTERM"))
|
||||||
signal.signal(signal.SIGINT, lambda *_: sup.request_shutdown("SIGINT"))
|
signal.signal(signal.SIGINT, lambda *_: sup.request_shutdown("SIGINT"))
|
||||||
|
# SIGHUP reload path: routes.yaml + pipelock.yaml hot-reloads
|
||||||
|
# in egress_apply.py / pipelock_apply.py issue `docker kill
|
||||||
|
# --signal HUP <bundle>`. The kernel delivers SIGHUP to PID 1
|
||||||
|
# (this supervisor); we forward it to the daemon that knows
|
||||||
|
# how to reload (egress = mitmdump-reload-addons; pipelock has
|
||||||
|
# no in-process reload, so the pipelock-apply path uses
|
||||||
|
# `docker restart` instead).
|
||||||
|
signal.signal(signal.SIGHUP, lambda *_: sup.forward_signal(signal.SIGHUP, "egress"))
|
||||||
|
|
||||||
while not sup.tick():
|
while not sup.tick():
|
||||||
time.sleep(_POLL_INTERVAL)
|
time.sleep(_POLL_INTERVAL)
|
||||||
|
|||||||
@@ -42,8 +42,8 @@ import urllib.request
|
|||||||
from dataclasses import dataclass
|
from dataclasses import dataclass
|
||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
|
|
||||||
# Same-directory import inside the container; `supervise.py` is COPYed
|
# Same-directory import inside the bundle container; `supervise.py`
|
||||||
# alongside this file by Dockerfile.supervise.
|
# is COPYed alongside this file by Dockerfile.sidecars.
|
||||||
import supervise as _sv
|
import supervise as _sv
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
+8
-6
@@ -40,15 +40,17 @@ Discovery is invoked with `-t .` (top-level dir = repo root) so the
|
|||||||
|
|
||||||
## What the integration tests cover
|
## What the integration tests cover
|
||||||
|
|
||||||
- `test_pipelock_sidecar_smoke.py` — drives `DockerPipelockProxy.prepare`
|
|
||||||
+ `.start` (the production code path) against a real Docker daemon and
|
|
||||||
probes the sidecar's `/health` from an in-network curl container.
|
|
||||||
- `test_dry_run_plan.py` — `cli.py start --dry-run --format=json` emits
|
- `test_dry_run_plan.py` — `cli.py start --dry-run --format=json` emits
|
||||||
a structured plan that contains the resolved egress allowlist and
|
a structured plan that contains the resolved egress allowlist and
|
||||||
the bottle's runtime, and creates zero Docker resources.
|
the bottle's runtime, and creates zero Docker resources.
|
||||||
- `test_orphan_cleanup.py` — `network_remove` and `PipelockProxy.stop`
|
- `test_orphan_cleanup.py` — `network_remove` is idempotent against
|
||||||
are idempotent against missing resources, so the EXIT trap can call
|
missing resources, so the EXIT trap can call it unconditionally.
|
||||||
them unconditionally.
|
- `test_sidecar_bundle_image.py` — builds Dockerfile.sidecars and
|
||||||
|
probes that pipelock / gitleaks / mitmdump / supervise are all
|
||||||
|
reachable inside the bundle.
|
||||||
|
- `test_sidecar_bundle_compose.py` — end-to-end compose-up of an
|
||||||
|
agent + bundle pair; verifies the agent reaches the bundle via
|
||||||
|
the legacy network aliases.
|
||||||
|
|
||||||
## Canaries
|
## Canaries
|
||||||
|
|
||||||
|
|||||||
@@ -174,6 +174,59 @@ class TestSupervisor(unittest.TestCase):
|
|||||||
self.assertEqual(2, rc)
|
self.assertEqual(2, rc)
|
||||||
self.assertIsNone(sup.shutdown_at)
|
self.assertIsNone(sup.shutdown_at)
|
||||||
|
|
||||||
|
def test_forward_signal_to_named_child(self):
|
||||||
|
# SIGHUP needs to reach mitmdump inside the bundle so
|
||||||
|
# routes.yaml reloads (egress_apply.py issues `docker kill
|
||||||
|
# --signal HUP <bundle>`). The supervisor forwards by daemon
|
||||||
|
# name.
|
||||||
|
#
|
||||||
|
# The child is Python (not a shell-with-trap) on purpose:
|
||||||
|
# bash on macOS defers trap execution while a foreground
|
||||||
|
# builtin like `sleep` is running and stdout is a pipe,
|
||||||
|
# which makes shell-trap test fixtures flaky. The
|
||||||
|
# production code path (mitmdump as the bundle child)
|
||||||
|
# doesn't have a shell in between — egress_entrypoint.sh
|
||||||
|
# `exec`s mitmdump, so SIGHUP lands directly on mitmdump.
|
||||||
|
sighup_marker = (
|
||||||
|
sys.executable, "-c",
|
||||||
|
"import signal, sys, time\n"
|
||||||
|
"def _h(*_): sys.exit(42)\n"
|
||||||
|
"signal.signal(signal.SIGHUP, _h)\n"
|
||||||
|
"while True: time.sleep(0.1)\n",
|
||||||
|
)
|
||||||
|
specs = [
|
||||||
|
_DaemonSpec("egress", sighup_marker),
|
||||||
|
_DaemonSpec("other", ("/bin/sleep", "30")),
|
||||||
|
]
|
||||||
|
sup = _Supervisor(specs)
|
||||||
|
sup.start_all()
|
||||||
|
time.sleep(0.3) # let Python install the handler
|
||||||
|
|
||||||
|
delivered = sup.forward_signal(signal.SIGHUP, "egress")
|
||||||
|
self.assertTrue(delivered)
|
||||||
|
|
||||||
|
deadline = time.monotonic() + 3.0
|
||||||
|
while time.monotonic() < deadline:
|
||||||
|
if sup.procs[0][1].poll() is not None:
|
||||||
|
break
|
||||||
|
time.sleep(0.05)
|
||||||
|
self.assertEqual(42, sup.procs[0][1].returncode,
|
||||||
|
"egress did not see SIGHUP")
|
||||||
|
# The other daemon is untouched.
|
||||||
|
self.assertIsNone(sup.procs[1][1].poll())
|
||||||
|
|
||||||
|
sup.request_shutdown(reason="cleanup")
|
||||||
|
self._drive(sup)
|
||||||
|
|
||||||
|
def test_forward_signal_unknown_daemon_no_op(self):
|
||||||
|
specs = [_DaemonSpec("a", ("/bin/sleep", "30"))]
|
||||||
|
sup = _Supervisor(specs)
|
||||||
|
sup.start_all()
|
||||||
|
delivered = sup.forward_signal(signal.SIGHUP, "ghost")
|
||||||
|
self.assertFalse(delivered)
|
||||||
|
sup.request_shutdown(reason="cleanup")
|
||||||
|
self._drive(sup)
|
||||||
|
|
||||||
def test_shutdown_after_start_terminates_children(self):
|
def test_shutdown_after_start_terminates_children(self):
|
||||||
# Two long-running children. Caller requests shutdown;
|
# Two long-running children. Caller requests shutdown;
|
||||||
# both should receive SIGTERM and exit. exit_code() is
|
# both should receive SIGTERM and exit. exit_code() is
|
||||||
|
|||||||
Reference in New Issue
Block a user