From f5d676e6c10d19b137979ee5e297c152d1011978 Mon Sep 17 00:00:00 2001 From: Vishal Rana Date: Thu, 27 Aug 2026 08:31:08 -0700 Subject: [PATCH 1/2] fix(proxy): align discovery container name Match the running container name to the published image while removing legacy onebox-proxy-discovery instances during teardown. --- internal/engine/ops.go | 4 +++- internal/engine/ops_test.go | 4 +++- internal/proxy/proxy.go | 7 ++++--- internal/proxy/proxy_test.go | 2 +- 4 files changed, 11 insertions(+), 6 deletions(-) diff --git a/internal/engine/ops.go b/internal/engine/ops.go index ef49cde8..aea8253e 100644 --- a/internal/engine/ops.go +++ b/internal/engine/ops.go @@ -215,8 +215,10 @@ func (e *Engine) Destroy(ctx context.Context, removeVolumes, removeProxy bool) e down := "if [ -f " + q(hp.Compose) + " ]; then docker compose -p " + proxy.Project + " -f " + q(hp.Compose) + " down || exit $?; fi; " + "proxy_orphans=$(docker ps -aq --filter name=^" + proxy.ContainerName + "$) || exit $?; " + "discovery_orphans=$(docker ps -aq --filter name=^" + proxy.DiscoveryContainerName + "$) || exit $?; " + + "legacy_discovery_orphans=$(docker ps -aq --filter name=^" + proxy.LegacyDiscoveryContainerName + "$) || exit $?; " + "if [ -n \"$proxy_orphans\" ]; then docker rm -f $proxy_orphans || exit $?; fi; " + - "if [ -n \"$discovery_orphans\" ]; then docker rm -f $discovery_orphans; fi" + "if [ -n \"$discovery_orphans\" ]; then docker rm -f $discovery_orphans || exit $?; fi; " + + "if [ -n \"$legacy_discovery_orphans\" ]; then docker rm -f $legacy_discovery_orphans; fi" if res, err := e.hostMutate(ctx, down); err != nil { return err } else if res.ExitCode != 0 { diff --git a/internal/engine/ops_test.go b/internal/engine/ops_test.go index 820fc9e0..e4546b7b 100644 --- a/internal/engine/ops_test.go +++ b/internal/engine/ops_test.go @@ -340,7 +340,9 @@ func TestDestroyProxyTeardownForSoleOwner(t *testing.T) { if !strings.Contains(seq, "docker compose -p onebox-proxy -f '/var/lib/ob/_host/proxy/compose.yaml' down") { t.Fatalf("sole owner with --proxy must tear the proxy down:\n%s", seq) } - for _, name := range []string{"name=^onebox-proxy$", "name=^onebox-proxy-discovery$"} { + for _, name := range []string{ + "name=^onebox-proxy$", "name=^onebox-discovery$", "name=^onebox-proxy-discovery$", + } { if !strings.Contains(seq, name) { t.Fatalf("proxy teardown must sweep orphan %s even when Compose state is missing:\n%s", name, seq) } diff --git a/internal/proxy/proxy.go b/internal/proxy/proxy.go index 6f330e44..cd81110b 100644 --- a/internal/proxy/proxy.go +++ b/internal/proxy/proxy.go @@ -39,9 +39,10 @@ const ( DiscoveryImageRepository = "ghcr.io/labstack/onebox-discovery" // Project is the compose project name; ContainerName the fixed container // name — both host-global, which is the point. - Project = app.ProxyProject - ContainerName = app.ProxyProject - DiscoveryContainerName = app.ProxyProject + "-discovery" + Project = app.ProxyProject + ContainerName = app.ProxyProject + DiscoveryContainerName = "onebox-discovery" + LegacyDiscoveryContainerName = app.ProxyProject + "-discovery" ) var releaseVersion = regexp.MustCompile(`^v[0-9]{4}\.[0-9]{1,2}\.[0-9]+$`) diff --git a/internal/proxy/proxy_test.go b/internal/proxy/proxy_test.go index e508d079..d565eeca 100644 --- a/internal/proxy/proxy_test.go +++ b/internal/proxy/proxy_test.go @@ -55,7 +55,7 @@ func TestRenderCompose(t *testing.T) { "container_name: onebox-proxy", "image: traefik:v3.7", `"80:80"`, `"443:443"`, - "container_name: onebox-proxy-discovery", + "container_name: onebox-discovery", "image: ghcr.io/labstack/onebox-discovery:edge", "./config:/etc/traefik:ro", "./dynamic:/etc/traefik/dynamic:ro", From b971a8c61d656da54f7433ccb2c49e544e60bb78 Mon Sep 17 00:00:00 2001 From: Vishal Rana Date: Thu, 27 Aug 2026 09:00:32 -0700 Subject: [PATCH 2/2] fix(proxy): verify orphan ownership Exact container names can be reused by unrelated workloads. Require generated Compose project and service labels before force-removing fallback matches. --- internal/engine/ops.go | 35 ++++++++++++++++------------------- internal/engine/ops_test.go | 10 ++++++---- 2 files changed, 22 insertions(+), 23 deletions(-) diff --git a/internal/engine/ops.go b/internal/engine/ops.go index aea8253e..137e0309 100644 --- a/internal/engine/ops.go +++ b/internal/engine/ops.go @@ -190,32 +190,29 @@ func (e *Engine) Destroy(ctx context.Context, removeVolumes, removeProxy bool) e // ownership released with something still holding :80/:443, and // the compose file deleted so no ob command can reach it. // - // --remove-orphans is deliberately absent for the same reason - // the sweep is name-anchored: it removes every container carrying - // the project label that the file does not declare, which is the - // seizure described below. The sweep already covers ob's own - // orphan, which is the only container ob created. + // --remove-orphans is deliberately absent because a project label + // alone is not proof of ownership: a user can independently run + // `docker compose -p onebox-proxy up`. A fixed name alone is not + // proof either: after a rename (or on a host installed later), the + // old name is free for an unrelated container to use. // - // It matches ob's own fixed container names, not the project - // label: `docker compose -p onebox-proxy up` run by a user for their - // own reasons carries that label too, and force-removing their - // containers is the foreign-resource seizure preflight refuses - // everywhere else. The compose file declares these two fixed names, - // so together they are complete for ob. + // The fallback therefore requires the exact generated container + // name AND the Compose project/service labels emitted by ob's + // generated two-service document. This still finds ob containers + // after the file or Compose state disappears without seizing a + // same-named foreign container. // // Without `|| exit $?` a failed docker ps yields an empty list, // the if is skipped, and teardown "succeeds" — the very outcome // this exists to prevent. // - // Reaching here implies the proxy is managed: Destroy refuses - // --proxy for an unmanaged one before any of this runs. That is - // what makes the name sweep safe — on an unmanaged host ob never - // created a onebox-proxy container, so anything answering to the - // name would be the operator's. + // Reaching here implies the proxy is managed, but that proves only + // the requested operation's scope. The filters below separately + // prove ownership of every container selected for force-removal. down := "if [ -f " + q(hp.Compose) + " ]; then docker compose -p " + proxy.Project + " -f " + q(hp.Compose) + " down || exit $?; fi; " + - "proxy_orphans=$(docker ps -aq --filter name=^" + proxy.ContainerName + "$) || exit $?; " + - "discovery_orphans=$(docker ps -aq --filter name=^" + proxy.DiscoveryContainerName + "$) || exit $?; " + - "legacy_discovery_orphans=$(docker ps -aq --filter name=^" + proxy.LegacyDiscoveryContainerName + "$) || exit $?; " + + "proxy_orphans=$(docker ps -aq --filter name=^" + proxy.ContainerName + "$ --filter label=com.docker.compose.project=" + proxy.Project + " --filter label=com.docker.compose.service=proxy) || exit $?; " + + "discovery_orphans=$(docker ps -aq --filter name=^" + proxy.DiscoveryContainerName + "$ --filter label=com.docker.compose.project=" + proxy.Project + " --filter label=com.docker.compose.service=discovery) || exit $?; " + + "legacy_discovery_orphans=$(docker ps -aq --filter name=^" + proxy.LegacyDiscoveryContainerName + "$ --filter label=com.docker.compose.project=" + proxy.Project + " --filter label=com.docker.compose.service=discovery) || exit $?; " + "if [ -n \"$proxy_orphans\" ]; then docker rm -f $proxy_orphans || exit $?; fi; " + "if [ -n \"$discovery_orphans\" ]; then docker rm -f $discovery_orphans || exit $?; fi; " + "if [ -n \"$legacy_discovery_orphans\" ]; then docker rm -f $legacy_discovery_orphans; fi" diff --git a/internal/engine/ops_test.go b/internal/engine/ops_test.go index e4546b7b..b110b59e 100644 --- a/internal/engine/ops_test.go +++ b/internal/engine/ops_test.go @@ -340,11 +340,13 @@ func TestDestroyProxyTeardownForSoleOwner(t *testing.T) { if !strings.Contains(seq, "docker compose -p onebox-proxy -f '/var/lib/ob/_host/proxy/compose.yaml' down") { t.Fatalf("sole owner with --proxy must tear the proxy down:\n%s", seq) } - for _, name := range []string{ - "name=^onebox-proxy$", "name=^onebox-discovery$", "name=^onebox-proxy-discovery$", + for _, selector := range []string{ + "name=^onebox-proxy$ --filter label=com.docker.compose.project=onebox-proxy --filter label=com.docker.compose.service=proxy", + "name=^onebox-discovery$ --filter label=com.docker.compose.project=onebox-proxy --filter label=com.docker.compose.service=discovery", + "name=^onebox-proxy-discovery$ --filter label=com.docker.compose.project=onebox-proxy --filter label=com.docker.compose.service=discovery", } { - if !strings.Contains(seq, name) { - t.Fatalf("proxy teardown must sweep orphan %s even when Compose state is missing:\n%s", name, seq) + if !strings.Contains(seq, selector) { + t.Fatalf("proxy teardown must sweep owned orphan %s even when Compose state is missing:\n%s", selector, seq) } } if !strings.Contains(seq, "rm -rf '/var/lib/ob/_host/proxy'") {