diff --git a/cmd/ob/commands.go b/cmd/ob/commands.go index 6cc92e17..39b675d7 100644 --- a/cmd/ob/commands.go +++ b/cmd/ob/commands.go @@ -337,6 +337,10 @@ func renderDeployPlan(cmd *cobra.Command, u *ui.UI, plan onebox.DeployPlan) { u.Infof("compose unchanged (release labels aside) — but the payload differs (env files / secrets); deploying ships it") } + if plan.ProxyAction != "" { + u.Infof("proxy: %s", plan.ProxyAction) + } + u.Println(u.Bold("operation:")) u.Println(fmt.Sprintf(" risk=%s reversibility=%s approval=%s", plan.Operation.Risk, plan.Operation.Reversibility, plan.Operation.Approval)) diff --git a/cmd/ob/commands_test.go b/cmd/ob/commands_test.go index ea26a391..e313aece 100644 --- a/cmd/ob/commands_test.go +++ b/cmd/ob/commands_test.go @@ -17,6 +17,7 @@ import ( "github.com/labstack/onebox/internal/app" "github.com/labstack/onebox/internal/onebox" + "github.com/labstack/onebox/internal/ui" ) func TestConfirmDefaultsToNo(t *testing.T) { @@ -191,3 +192,14 @@ func TestNotifyOutcome(t *testing.T) { t.Fatal("nil notify must be a no-op") } } + +func TestRenderDeployPlanShowsProxyAction(t *testing.T) { + var out bytes.Buffer + cmd := &cobra.Command{} + cmd.SetOut(&out) + plan := onebox.DeployPlan{ProxyAction: "restart Traefik (configuration changed)"} + renderDeployPlan(cmd, ui.New(&out, false), plan) + if !strings.Contains(out.String(), "proxy: restart Traefik (configuration changed)") { + t.Fatalf("proxy action missing: %q", out.String()) + } +} diff --git a/docs/plans/2026-10-17-zero-downtime-gaps.md b/docs/plans/2026-10-17-zero-downtime-gaps.md new file mode 100644 index 00000000..a7206896 --- /dev/null +++ b/docs/plans/2026-10-17-zero-downtime-gaps.md @@ -0,0 +1,363 @@ +# Closing avoidable downtime in Onebox + +Status: implemented, 2026-10-17. Reviewed by Codex. Implementation plan: +`docs/superpowers/plans/2026-10-17-zero-downtime-gaps.md`. + +Goal: remove downtime Onebox causes without need, and make the rolling +guarantee hold when the routing layer misbehaves. Downtime inherent to one box +with single-instance services stays, and is documented. + +What changed from v1: + +- Proxy: an explicit state machine, an invalidate-first marker, fresh-output + evidence, and no Traefik restart on a discovery repair. +- Services: a `--timeout` on apply and a graceful destroy, beside the rendered + grace period. +- Rolling: the gate runs before every retirement and before success (covers + resume), matches the expected proxy services, and fails rather than warns. +- New: the deploy plan names the proxy action. +- New: recovery restores the predecessor before removing serving containers. +- The rejection of dynamic hot reload stays, with a corrected rationale. + +--- + +## 1. Proxy: restart Traefik only when Traefik's inputs change + +### Problem + +The applied-state digest covers config files, dynamic files and the rendered +`compose.yaml` (`internal/proxy/proxy.go:484`, `:522`). The compose file pins +discovery to the runner release (`internal/proxy/proxy.go:52`; development +builds use `:edge`). A new release therefore changes the digest, and +`EnsureProxy` swaps both directories, force-recreates discovery, and restarts +an unchanged Traefik container (`internal/engine/proxy.go:211`). + +Two existing defects share the code path: + +- A missing discovery container or output with an unchanged digest takes the + same path, so repairing discovery restarts a healthy Traefik. +- The marker is written after health but never invalidated before the swap. + Interrupted apply of B, then a re-apply of A: the marker still reads A, so + the run is a no-op against B's files on disk. + +### Design + +Staging returns a pair `{Config, Compose}`. One type owns its encoding +(` ` on one line) and its parsing. Anything unparsable, +including the legacy one-digest marker, is "unknown". + +`EnsureProxy` decides from the staged pair, the marker, and observation: + +- **No-op.** Marker equals staged, Traefik and discovery running, output + present. +- **Config path.** `marker.Config` differs or is unknown: + 1. Delete the marker. + 2. Stage, swap directories and `compose.yaml` (as today). + 3. Force-recreate discovery. Wait for `onebox.yml`. The swap leaves no output + file, so its presence proves the new controller published. + 4. `up -d proxy`. Restart Traefik if Compose did not recreate it. + 5. Wait for health. Write the marker atomically (temporary file, `mv`). +- **Compose or repair path.** Config equal; Compose differs, or a container or + the output is missing: + 1. Delete the marker. + 2. Replace `compose.yaml` atomically. No directory swap. + 3. Record the discovery container id and the output's inode. Bring + discovery up: `--force-recreate` when it is not running or the output is + missing (a running controller that lost its output will not rewrite it + by itself), plain `up -d` otherwise, which recreates it only on drift. + Then `up -d proxy`, which recreates Traefik only if its own block drifted. + 4. If the discovery id changed or the output was missing, wait until the + output appears or its inode changes. Discovery writes through + temporary file and rename on every reconcile, including startup + (`cmd/onebox-discovery/main.go:42`, `internal/discovery/docker.go:170`), + so a new inode proves the new controller published. + 5. Never `docker restart`. If Compose recreated Traefik, wait for health. + 6. Write the marker atomically. + +The restart rule is now one line: restart only on the config path. + +`ob status` parses the pair and says which half drifted, instead of printing +the first 8 characters of one string (`internal/engine/proxystatus.go:294`, +`internal/engine/status_snapshot.go:453`). + +A legacy marker takes the config path once: one last restart per host. + +### Assumption to verify + +The compose path relies on Compose not recreating `proxy` when its dependency +`discovery` is recreated (`depends_on` with `condition: service_started`, no +`restart: true`), and on `depends_on` being outside the service hash. That +holds in the Compose v2.39.4 source. Onebox sets no Compose version floor +(`internal/app/prerequisites.go:120`). Verify on the target hosts' Compose +version before release; an end-to-end test should assert the Traefik +container id survives a discovery image change. + +### Why not a separate Compose project for discovery + +It needs a migration of the live `onebox-discovery` container, plus status, +destroy, preflight and naming-guard changes, and it solves none of the state +transitions above. + +--- + +## 2. Managed services: a clean shutdown window + +### Problem + +Rendered service documents set no `stop_grace_period` +(`internal/app/services.go:367`), so Docker kills after 10s. PostgreSQL fast +shutdown writes a checkpoint; durable Redis and Valkey write on shutdown. +Either can exceed 10s, and a kill makes the restart slower. Destroy uses +`docker rm -f` (`internal/engine/services.go:482`) and skips shutdown entirely. + +### Design + +One constant, `serviceStopGrace = 2m`, used in three places: + +- Rendered `stop_grace_period`. Covers reboots and manual stops. +- `docker compose ... up -d --timeout 120` in service apply + (`internal/engine/services.go:141`). Protects the first recreate, because + existing containers still carry the 10s default. +- Destroy: `docker stop -t 120`, then `docker rm -f`. The label sweep stays, + so it works without the Compose file. + +This is a bounded policy, not a promise that every database finishes in time. +Apply handles services one at a time, so the worst case grows per service. The +90s health budget starts after Compose returns and does not change. + +Rollout: every managed service is recreated once on its next apply, and +PostgreSQL services with declared extensions on their next deploy. Release +note, as with #220. Contract goldens change +(`internal/app/equivalence_test.go:111`). + +--- + +## 3. Rolling: gate on published routing, not only a fixed sleep + +### Problem + +After a newcomer is healthy, `retireContainer` sleeps a fixed 3s and retires an +old replica (`internal/engine/roll.go:263`). Nothing checks that discovery +published the newcomer. Resume adopts running newcomers without any check +(`internal/engine/roll.go:145`, `:163`). If discovery is down, `onebox.yml` +stays frozen: it keeps old addresses and never gains the newcomer, so the +route ends on dead endpoints while the deploy reports success. + +### Design + +Only when the proxy is managed and the workload has routes. + +`published(container)` reads and parses `onebox.yml`, takes the container's +ingress address with discovery's own selection rule (shared helper, from +`internal/discovery/docker.go:102`), and checks it is a server of every +expected service, `ProxyServiceFor(workload, i)` (`internal/app/generate.go:543`). + +- **Gate A**, before any retirement and before reporting success: every + newcomer is healthy and published. Covers fresh surges and resumed + adoption. Bound 30s. On failure, a newcomer created in this pass is removed, + adopted ones are left, and old replicas keep serving. +- **Gate B**, after the drain flip and before any signal or stop: the drained + address is absent from the expected services. On failure, stop the roll + with the container still running and draining. `ob resume` retries. +- An unreadable, unparsable or cancelled read never counts as presence or + absence. +- Unguardable health checks (no shell in the image, + `internal/engine/roll.go:275`) cannot drain, so Gate B does not apply. The + existing log line stays. +- `ConvergeBuffer` stays after each gate. Published is not loaded: Traefik's + file watch and throttle (2s default, changeable by custom static config) are + not observable without the Traefik API. Documented as an assumption. + +Residual risk, accepted: a frozen document plus Docker reusing a just-freed +address could pass Gate A falsely. Docker allocates addresses serially, so +immediate reuse is unlikely. + +--- + +## 4. The deploy plan names the proxy action + +Today the plan diffs only the application Compose +(`internal/onebox/plan_deploy.go:128`), and the operation graph has no proxy +step (`internal/onebox/operation_graph.go:36`). A deploy can restart Traefik +without the plan saying so. + +Add one plan line, computed with the staging `ob status` already does: +`proxy: unchanged`, `proxy: update controller (no restart)`, or +`proxy: restart (configuration changed)`. + +--- + +## 5. Recovery: restore before removing serving containers + +Recovery force-removes every container of the interrupted release +(`internal/engine/recovery.go:169`) before restoring the predecessor +(`:171`). If the interrupted release was serving, `ob abort` has a gap. + +All releases share one Compose project (`internal/engine/roll.go:25`), so a +roll of the predecessor already treats the interrupted release's containers of +that workload as old replicas and drains and retires them. + +New order: + +1. Remove the interrupted release's containers that are not serving a + workload the predecessor will roll: anything not running, job and hook + containers, containers of `recreate` workloads, and any container holding + the transient rollout name (`TransientContainer`), which the predecessor's + roll needs. With no predecessor, remove everything (today's behavior). +2. Restore the predecessor. A workload counts as already restored only when + its containers match the predecessor's revision and none of them belongs + to the interrupted release; matching revision alone is not enough, because + step 3 would then remove the only containers it has. Otherwise the + predecessor rolls it, which drains and retires the interrupted containers + that were kept. +3. Sweep every remaining interrupted-release container (today's removal). +4. Verify, then the existing postcondition. + +Lands last, after the rolling gate, because the restore relies on adopted +newcomers being admitted correctly. + +--- + +## 6. Review round decisions (2026-10-17) + +A whole-branch review of the first implementation found these gaps. Each +decision below amends the section it names. + +- **R1, withdrawal (amends §3 Gate B).** Withdrawal is checked whenever the + proxy is managed, whatever the new role's routes are. A drained container + with no address on the proxy network is withdrawn: it cannot be a server. + Otherwise it is withdrawn when its address is a server of no service in + the document, not only of the new role's expected services. Adding a + route, removing one, or changing its protocol then retires old replicas + normally. Gate A keeps checking the new role's expected services. +- **R2, converge marker (amends §1).** The converge path never touches + `config/` or `dynamic/`, so it keeps the Config half: it writes + ` sha256:<64 zeros>` atomically instead of deleting the + marker. A failed or interrupted converge is retried as a converge, not as + a configuration apply with a Traefik restart. The config path still + deletes the marker. +- **R3, fresh publication on the converge path (amends §1).** Running + discovery rewrites `onebox.yml` on every container event, so an inode + change alone does not prove a new controller published. The converge path + stops discovery, samples the inode, brings discovery up (forced when it + was missing or the output was missing), then waits for a new inode. + Traefik keeps serving the last document meanwhile. +- **R4, Traefik replacement is named (amends §1, §4).** The observation + compares the live and rendered `services.proxy` blocks (parsed YAML; an + unreadable live file counts as changed). A compose drift that changes + Traefik's own block is the action `recreate Traefik (its container + definition changed)`. It runs exactly like the converge path. The plan says + `start Traefik` when Traefik is not running. +- **R5, the config path keeps serving until the restart (amends §1).** Live + `config/` and `dynamic/` are renamed into the staged directory instead of + deleted. Traefik's bind mounts follow the renamed directories, so it keeps + its routes until it is restarted or recreated. The retired directories go + with the staged directory after health; on failure they stay for the next + run, which restarts Traefik. +- **R6.** The converge path uploads only `compose.yaml`, never `config/` + (which can hold `.env` credentials). +- **R7, single-replica recovery (amends §5).** A running interrupted + container holding the transient rollout name is renamed to the lowest free + slot name instead of removed, so it keeps serving and frees the name the + predecessor's roll needs. Before restoring, the drain file is removed from + the predecessor's running replicas, so a replica drained by the + interrupted roll becomes healthy again and can be admitted. +- **R8, a failed replace stays a replace (amends R2, R4).** The replace path + writes its own pending digest, `sha256:<63 zeros>1`, and a marker carrying + it counts as a Traefik definition change. A retry after the live + `compose.yaml` was already moved is planned and labelled as a replace. +- **R9, discovery is never left stopped (amends R3).** The discovery image is + pulled when missing before discovery is stopped. If any step after the + stop fails, the existing `onebox-discovery` container is started again, + best effort, before the error returns. +- **R10.** The unchanged path also removes stale `.staged-*` directories: a + committed final marker means Traefik already serves the live directories. +- **R11.** The dead `withdrawnFrom` goes. Recovery gains a test for the surge + case: the predecessor replica is gone and the interrupted newcomer is the + only healthy container. +- **R12, discovery recovery on every path (amends R9).** The apply path also + pulls the discovery image before force-recreating discovery, and counts as + having stopped it. The fallback after a failure is `docker start + onebox-discovery`, then `compose up -d discovery` against the live + `compose.yaml` when the container no longer exists. +- **R13, the approved proxy action is binding (amends §4).** Executing a + sealed deploy plan recomputes the proxy plan line before the proxy step + and refuses, as drift, when it differs from the plan's `proxy:` line. + Paths without a plan (`ob proxy apply`, bootstrap, resume) do not check. +- **R14, a re-pulled Traefik image counts as a definition change.** When + Traefik is running, the observation compares its container's image ID + with the local image ID for the configured tag; a difference, or a tag + absent locally, sets `TraefikChanged`. +- **R15, failed uploads do not accumulate.** At the start of a run, staged + directories with no `retired/` inside are removed; those with retired + configuration stay until an apply succeeds. +- **R16, withdrawn.** Admitting each newcomer once let a roll retire every old + replica and report success after an admitted newcomer failed under load, + breaking §3 Gate A. Gate A re-checks every newcomer before each retirement + and before success; the extra round trips are the price of the guarantee. + **R17.** Routing errors name the discovery container through + `proxy.DiscoveryContainerName`. +- **R18, the marker always follows health.** The converge and replace paths + wait for Traefik's health before committing the final identity, even when + the container was not recreated in this run. +- **R19.** The apply path pulls the discovery image right after the drift + check, before it invalidates the marker or swaps directories. The discovery + fallback has its own 30s budget. The Traefik image for the comparison is + read from the rendered compose, not defaulted twice. +- **Polish left as is:** `ob status` does not show the R14 image signal; both + pending markers render as compose drift; the binding compares the plan + line's text (plans expire, so a rewording only refuses outstanding plans); + the unchanged path runs two cleanups. +- **Kept by design after the second code review:** a drain that never takes + effect fails the roll instead of stopping a routed replica (§3 Gate B); the + slot search in recovery stays, since a placeholder name would add a host + object name. +- **Accepted:** after a failed proxy apply, a roll that skips the proxy step + (`ob secrets push`, recovery) can read a routing document Traefik is not + serving. Run `ob proxy apply` first; `ob status` shows the proxy as + drifted. Refusing such rolls outright would also block `ob abort` during + an incident. +- **Operator check additions:** on each target host, `compose stop` of a + service leaves a `depends_on` dependent running, and `up -d` of the + stopped service keeps the dependent's container. +- **Deferred:** an end-to-end assertion that Traefik's container id survives + a discovery image change. The Compose behavior was verified locally on + Compose 5.6.0; the release operator check covers the target hosts. + +## Rejected: hot reload of authored dynamic files + +Per-file updates in `dynamic/` can expose a half-updated set to Traefik's +directory rebuild. Today's swap is not transactional either, and dynamic +changes do not inherently need a restart: a single generated document, +replaced atomically inside a stable directory as discovery already does, would +work. That is a separate feature. This patch keeps the restart. + +--- + +## Downtime that remains, by design + +- Any changed managed-service configuration restarts that service's single + container. +- Any change to Traefik's static, dynamic or authored configuration, image or + ports restarts or recreates Traefik. +- `recreate` workloads keep their stated gap. +- Host events: Docker daemon restart, reboot, host failure. + +## Tests + +- Proxy: compose-only change (no swap, no restart, marker updated); config + change (today's sequence); legacy marker; discovery repair without restart; + stale output rejected; interruption between each swap step, then a re-apply + of the old input; status shows which half drifted. Keep the existing + missing-controller and missing-output tests (`internal/engine/proxy_test.go:203`, + `:286`) and the marker-after-health assertion. +- Services: rendered grace period; `--timeout` on apply; graceful destroy; + goldens regenerated. +- Rolling: unpublished newcomer fails and old replicas keep serving; adopted + unhealthy or unpublished newcomer blocks retirement; withdrawal timeout + stops before signal; unreadable document is neither presence nor absence; + unguardable health keeps today's path. Keep the resume adoption test + (`internal/engine/roll_test.go:174`). +- Plan: each of the three proxy lines. +- Recovery: a serving interrupted release is drained by the predecessor's + roll, not force-removed first; no predecessor keeps today's removal. diff --git a/docs/superpowers/plans/2026-10-17-zero-downtime-gaps.md b/docs/superpowers/plans/2026-10-17-zero-downtime-gaps.md new file mode 100644 index 00000000..54d8c5ae --- /dev/null +++ b/docs/superpowers/plans/2026-10-17-zero-downtime-gaps.md @@ -0,0 +1,644 @@ +# Zero-Downtime Gaps Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Remove the downtime Onebox causes without need: Traefik restarts on every `ob` upgrade and on discovery repair, killed database shutdowns, rolls that retire the last routed replica, and recovery that removes serving containers before restoring. + +**Architecture:** The proxy's applied-state marker splits into a `{Config, Compose}` pair, and a pure decision function picks one of three converge paths; only the configuration path restarts Traefik. Managed services get one shutdown grace constant used by render, apply and destroy. Rolling gains two gates that read the discovery controller's published `onebox.yml`: a newcomer must be published before any old replica retires, and a drained replica must be withdrawn before it is stopped. Recovery restores the predecessor before sweeping the interrupted release. + +**Tech Stack:** Go 1.27.2, Docker Compose v2, Traefik v3 file provider, `transport.Fake` for unit tests, `virtualClock` (`internal/engine/drain_budget_test.go`) for timeout tests. + +**Spec:** `docs/plans/2026-10-17-zero-downtime-gaps.md` + +## Global Constraints + +- Work on a branch, never `main`. Breaking changes are acceptable; the only compatibility rule is that a legacy one-digest marker takes the configuration path once. +- Marker format: ` ` on one line, each `sha256:<64 lowercase hex>`. Written atomically: `printf '%s\n' > config.hash.tmp && mv -f config.hash.tmp config.hash`. Deleted (`rm -f`) before any path writes to the host proxy directory. +- Publication wait: the existing 30 × 1s loop, now keyed on the output file's inode. +- `app.ServiceStopGrace = 2 * time.Minute`. Rendered as `stop_grace_period: 2m0s`; passed as `--timeout 120` and `docker stop -t 120`. +- `routePublishBudget = 30 * time.Second`. `ConvergeBuffer` stays 3s and stays in place after each gate. +- Routing gates apply only when `Spec.Proxy.Managed` and the workload has routes. Health admission of adopted newcomers applies to every rolling workload. +- An unreadable, failed or unparsable `onebox.yml` read never counts as presence or absence. +- No new host object names. Existing names: `onebox-proxy`, `onebox-discovery`, `/var/lib/onebox/_host/proxy/{config.hash,compose.yaml,config,dynamic}`. +- Gate before every commit: `go build ./... && go vet ./... && go test ./...`. Final task also runs `just check` and `just env-namespace`. +- Commits: conventional (`fix(proxy): …`, `fix(services): …`, `fix(rolling): …`, `fix(recovery): …`), body says why and what an operator sees on rollout. + +## Review Focus + +1. Output deleted by hand while discovery runs and the marker matches: the converge path must force-recreate discovery, never wait on a controller that will not rewrite. Test in Task 3. +2. Empty, truncated or legacy marker: configuration path, never "unchanged". Tests in Tasks 2 and 3. +3. A workload with one HTTP and one TCP route: published only when its address is a server of both services. Test in Task 7. +4. IPv6-only ingress address: `http://[fd00::5]:3000` matches `fd00::5`. Test in Task 7. +5. `cat onebox.yml` fails during withdrawal: not withdrawn, roll stops, no `docker stop`. Test in Task 9. + +--- + +## Slice 1: Proxy converge + +### Task 1: Split proxy identity + +**Files:** +- Create: `internal/proxy/identity.go`, `internal/proxy/identity_test.go` +- Modify: `internal/proxy/proxy.go:331-523` (`Stage`, `StageForApp`, `StageForAppManaged`, `stageDefaultManaged`) +- Modify (compile only): `internal/engine/proxy.go:41`, `internal/engine/proxystatus.go:187`, `internal/engine/proxy_test.go:26-49` (`proxyFixture`) + +**Interfaces:** +- Produces: `type Identity struct{ Config, Compose string }`; `func (i Identity) String() string`; `func ParseIdentity(s string) (Identity, bool)`. `Stage`, `StageForApp`, `StageForAppManaged` return `(Identity, error)`. `proxyFixture` returns `(*Engine, proxy.Identity, *bytes.Buffer)`. + +- [ ] **Step 1: Write the failing tests** in `internal/proxy/identity_test.go` + +```go +func TestParseIdentityRoundTrip(t *testing.T) { + id := Identity{Config: "sha256:" + strings.Repeat("a", 64), Compose: "sha256:" + strings.Repeat("b", 64)} + got, ok := ParseIdentity(id.String() + "\n") + if !ok || got != id { t.Fatalf("got %+v %v", got, ok) } +} + +func TestParseIdentityRejectsLegacyAndGarbage(t *testing.T) { + a := "sha256:" + strings.Repeat("a", 64) + for _, s := range []string{"", "deadbeef", a, a + " " + a + " " + a, a + " sha256:XYZ", strings.Repeat("a", 64) + " " + a} { + if _, ok := ParseIdentity(s); ok { t.Errorf("accepted %q", s) } + } +} + +func TestStageSeparatesComposeFromConfig(t *testing.T) { + // same proxy.config dir staged with DiscoveryImage("v2026.10.4-alpha") and ("v2026.10.5-alpha") + // want: Config equal, Compose different +} + +func TestStageConfigChangeMovesOnlyConfig(t *testing.T) { + // traefik.yml body changed between two stagings, same discovery image + // want: Config different, Compose equal +} +``` + +- [ ] **Step 2: Run** `go test ./internal/proxy -run 'Identity|StageSeparates|StageConfigChange'` — Expected: FAIL, `undefined: Identity`. + +- [ ] **Step 3: Implement.** `Config` hashes `newProxyConfigHash()` plus every staged file and the managed dynamic file (today's hash minus the compose bytes). `Compose` is `"sha256:" + hex(sha256(compose))`. Both carry the `sha256:` prefix on both staging paths (today only `stageDefaultManaged` does). Callers compile with `id.String()` until Tasks 3 and 4 replace them; `proxyFixture` and every test that sends `hash + "\n"` send `id.String() + "\n"`. + +- [ ] **Step 4: Run** `go build ./... && go vet ./... && go test ./...` — Expected: PASS. + +- [ ] **Step 5: Commit** `refactor(proxy): stage a config digest and a compose digest` + +### Task 2: Observe and decide + +**Files:** +- Create: `internal/engine/proxy_converge.go`, `internal/engine/proxy_converge_test.go` + +**Interfaces:** +- Consumes: `proxy.Identity`, `proxy.ParseIdentity` (Task 1). +- Produces: + - `type proxyObservation struct { Applied proxy.Identity; AppliedKnown bool; ProxyIDs, DiscoveryIDs []string; OutputInode string }` — `OutputInode` is `""` when `dynamic/onebox.yml` is absent or empty. + - `type proxyAction int` with `proxyUnchanged`, `proxyConverge` (compose drift or repair, never restarts Traefik), `proxyApply` (configuration changed, restarts Traefik). + - `func decideProxy(o proxyObservation, staged proxy.Identity) proxyAction` + - `func (a proxyAction) String() string` → `"unchanged"`, `"update changed containers only, no restart"`, `"restart Traefik (configuration changed)"`. + - `func (e *Engine) stageProxy(dir string) (proxy.Identity, error)` — the `StageForAppManaged` call now at `internal/engine/proxy.go:30-44`, including `Proxy.Config` resolution against `LocalDir`. + - `func (e *Engine) observeProxy(ctx context.Context) (proxyObservation, error)` — read-only: `cat` of the marker, `proxyContainerIDs`, `discoveryContainerIDs`, `outputInodeCommand`. + - `func outputInodeCommand(path string) string` → `if [ -s P ]; then stat -c %i P; fi` with `P` quoted by `q`. + +- [ ] **Step 1: Write the failing tests** + +```go +func TestDecideProxy(t *testing.T) { + staged := proxy.Identity{Config: digestA, Compose: digestB} + running := proxyObservation{Applied: staged, AppliedKnown: true, ProxyIDs: []string{"PX1"}, DiscoveryIDs: []string{"D1"}, OutputInode: "12"} + // rows: name, mutate(o), want + // "unknown marker" AppliedKnown=false → proxyApply + // "config drift" Applied.Config=digestC → proxyApply + // "compose drift" Applied.Compose=digestC → proxyConverge + // "traefik missing" ProxyIDs=nil → proxyConverge + // "discovery missing" DiscoveryIDs=nil → proxyConverge + // "output missing" OutputInode="" → proxyConverge + // "all present and equal" (none) → proxyUnchanged +} + +func TestObserveProxyIsReadOnly(t *testing.T) { + // fake answers marker, both ps queries, inode; after observeProxy no command + // contains "rm ", "mv ", "mkdir", "up -d", "docker restart" or "> " +} +``` + +- [ ] **Step 2: Run** `go test ./internal/engine -run 'DecideProxy|ObserveProxy'` — Expected: FAIL, undefined. + +- [ ] **Step 3: Implement** the interfaces above. A marker that fails `ParseIdentity` sets `AppliedKnown=false`. + +- [ ] **Step 4: Run** the gate — Expected: PASS. + +- [ ] **Step 5: Commit** `refactor(proxy): observe the host proxy and decide its converge path` + +### Task 3: Converge without needless Traefik restarts + +**Files:** +- Modify: `internal/engine/proxy.go:24-232` (`EnsureProxy`) +- Test: `internal/engine/proxy_test.go` + +**Interfaces:** +- Consumes: `stageProxy`, `observeProxy`, `decideProxy`, `outputInodeCommand` (Task 2). +- Produces: `func (e *Engine) waitForPublication(ctx context.Context, previousInode string) error` (30 × 1s until the output is non-empty and its inode differs from `previousInode`; error text contains `did not publish` and `docker logs onebox-discovery`); `func (e *Engine) commitProxyIdentity(ctx context.Context, id proxy.Identity) error` (atomic marker write). + +- [ ] **Step 1: Verify the Compose assumption** on the local Docker host: + +```bash +d=$(mktemp -d) && cd "$d" +cat > compose.yaml <<'EOF' +services: + a: {image: busybox:1.36, command: sleep 3600} + b: {image: busybox:1.36, command: sleep 3600, depends_on: {a: {condition: service_started}}} +EOF +docker compose -p obcheck up -d && B1=$(docker compose -p obcheck ps -q b) +sed -i.bak 's/a: {image: busybox:1.36/a: {image: busybox:1.37/' compose.yaml +docker compose -p obcheck up -d a && docker compose -p obcheck up -d b && B2=$(docker compose -p obcheck ps -q b) +docker compose version; test "$B1" = "$B2" && echo SAME || echo RECREATED +docker compose -p obcheck down +``` + +Expected: `SAME`. On `RECREATED`, stop and report: the converge path's premise fails. Record the `docker compose version` output in the commit body. + +- [ ] **Step 2: Write the failing tests** (fakes built on `proxyPS`; marker read is `cat '/var/lib/onebox/_host/proxy/config.hash'`; inode read is `outputInodeCommand('/var/lib/onebox/_host/proxy/dynamic/onebox.yml')`) + +```go +func TestEnsureProxyComposeOnlyChangeSkipsSwapAndRestart(t *testing.T) { + // marker = {staged.Config, other compose}; discovery D1 → D2 after `up -d discovery`; inode "100" → "101" + // want: contains "/compose.yaml' '/var/lib/onebox/_host/proxy/compose.yaml'" + // want: not contains "/config' '/var/lib/onebox/_host/proxy/config'", "docker restart", "--force-recreate" + // want: "mv -f '/var/lib/onebox/_host/proxy/config.hash.tmp'" after the last health inspect +} + +func TestEnsureProxyMissingOutputForceRecreatesDiscoveryWithoutRestart(t *testing.T) { + // marker == staged, both running, inode "" → "200" after force-recreate + // want: contains "up -d --force-recreate discovery"; not contains "docker restart" +} + +func TestEnsureProxyComposePathRejectsStaleOutput(t *testing.T) { + // discovery id changes, inode stays "100" + // want: error contains "did not publish"; no "config.hash.tmp" write +} + +func TestEnsureProxyMissingMarkerTakesConfigPath(t *testing.T) { + // marker read returns "" with both containers running and output present + // want: config swap command present +} +``` + +Update existing tests: +- `TestEnsureProxyUnchangedIsNoOp`: marker `id.String()`, inode `"12"`. +- `TestEnsureProxyRepairsMissingDiscoveryController` and `TestEnsureProxyRepairsMissingDiscoveryOutput`: also assert no `docker restart` and no config swap. +- `TestEnsureProxyConfigOnlyChangeRestarts`: `rm -f '/var/lib/onebox/_host/proxy/config.hash'` precedes the upload; the marker assertion targets the `config.hash.tmp` `mv`. +- `TestEnsureProxyFailedConvergeLeavesHashUnwritten`: assert the `rm -f` ran and no `config.hash.tmp` write did. + +- [ ] **Step 3: Run** `go test ./internal/engine -run EnsureProxy` — Expected: the new tests FAIL. + +- [ ] **Step 4: Implement.** After the existing lock, journal and directory preparation, `observeProxy`, then switch on `decideProxy`: + - `proxyUnchanged`: log `proxy: unchanged and running — not touched`, return. + - Both other paths: keep today's live/planned compose diff display once, then `rm -f` the marker before any upload. + - `proxyApply`: today's staged upload and swap, force-recreate discovery, `waitForPublication(ctx, "")` (the swap removed the old output), `up -d proxy`, restart only if the container id did not change, health, `commitProxyIdentity`. + - `proxyConverge`: upload the staging dir, `mv -f` only its `compose.yaml` over `hp.Compose`, remove the staged dir; `up -d --force-recreate discovery` when `DiscoveryIDs` is empty or `OutputInode` is `""`, else `up -d discovery`; if the discovery id changed or was forced, `waitForPublication(ctx, o.OutputInode)`; `up -d proxy`; if the Traefik id changed or was absent, wait for health; never `docker restart`; `commitProxyIdentity`. + +- [ ] **Step 5: Run** the gate — Expected: PASS. + +- [ ] **Step 6: Commit** `fix(proxy): restart Traefik only when its configuration changes` — body: `ob` upgrades and discovery repair no longer drop routes; the first apply on each host after upgrading still restarts once (legacy marker); Compose version verified in Step 1. + +### Task 4: Status names the drifted half + +**Files:** +- Modify: `internal/engine/proxystatus.go:180-200` (local stage via `stageProxy`), `:280-303` (render), `internal/engine/status_snapshot.go:450-460` +- Test: `internal/engine/proxystatus_test.go`, status snapshot tests + +**Interfaces:** +- Consumes: `stageProxy` (Task 2), `proxy.ParseIdentity` (Task 1). +- Produces: `func proxyDriftParts(applied string, local proxy.Identity) []string` → `nil` in sync, else `["config"]`, `["compose"]` or `["config", "compose"]`; an unparsable marker yields both. `func shortDigest(d string) string` → first 8 hex characters after `sha256:`. + +- [ ] **Step 1: Write the failing tests** + +```go +func TestProxyDriftParts(t *testing.T) { + // equal → nil; compose differs → ["compose"]; config differs → ["config"]; + // "deadbeef" → ["config","compose"] +} + +func TestProxyStatusNamesComposeDrift(t *testing.T) { + // applied = {local.Config, other}.String() + // output contains "DRIFTED ⚠ (compose)" and not "(config" +} +``` + +- [ ] **Step 2: Run** `go test ./internal/engine -run 'ProxyDrift|ProxyStatus'` — Expected: FAIL. + +- [ ] **Step 3: Implement.** In sync: `config compose (in sync)`. Drift: `DRIFTED ⚠ () — `ob proxy apply``. The JSON snapshot keeps `LocalConfigHash` (now `local.String()`) and its applied field, and decides divergence with `proxyDriftParts`. + +- [ ] **Step 4: Run** the gate — Expected: PASS. + +- [ ] **Step 5: Commit** `fix(status): say which half of the proxy drifted` + +--- + +## Slice 2: Plan visibility + +### Task 5: The deploy plan names the proxy action + +**Files:** +- Modify: `internal/engine/proxy_converge.go`, `internal/onebox/execution_types.go:53-62`, `internal/onebox/plan_deploy.go` (after `e.Refresh`), `cmd/ob/commands.go:323-337` (`renderDeployPlan`) +- Test: `internal/engine/proxy_converge_test.go`, `cmd/ob/commands_test.go` + +**Interfaces:** +- Consumes: `stageProxy`, `observeProxy`, `decideProxy` (Task 2). +- Produces: `func (e *Engine) PlanProxy(ctx context.Context) (string, error)` → `""` when the proxy is not managed, else `decideProxy(...).String()`. `DeployPlan.ProxyAction string` with tag `json:"proxy_action,omitempty"`. + +- [ ] **Step 1: Write the failing tests** + +```go +func TestPlanProxyReportsEachAction(t *testing.T) { + // unchanged → "unchanged"; compose drift → "update changed containers only, no restart"; + // config drift → "restart Traefik (configuration changed)"; unmanaged → "" + // every case: no mutating command (as in TestObserveProxyIsReadOnly) +} + +func TestRenderDeployPlanShowsProxyAction(t *testing.T) { + // plan with ProxyAction "restart Traefik (configuration changed)" + // output contains "proxy: restart Traefik (configuration changed)" +} +``` + +- [ ] **Step 2: Run** `go test ./internal/engine ./cmd/ob -run 'PlanProxy|ProxyAction'` — Expected: FAIL. + +- [ ] **Step 3: Implement.** `PlanDeploy` sets `ProxyAction` before the plan digest is computed, so the digest covers it. `renderDeployPlan` prints `proxy: ` with `u.Infof` right after the diff / no-op block. + +- [ ] **Step 4: Run** the gate — Expected: PASS. + +- [ ] **Step 5: Commit** `feat(plan): show what a deploy does to the proxy` + +--- + +## Slice 3: Managed services + +### Task 6: A clean shutdown window + +**Files:** +- Modify: `internal/app/services.go:367-380` (render), `internal/engine/services.go:141` (apply), `internal/engine/services.go:497` (`removeServices`) +- Modify: `internal/app/testdata/contract-verdicts.json` (regenerated), `site/src/content/docs/guides/add-a-database.mdx` +- Test: `internal/app/services_test.go`; create `internal/engine/services_test.go` + +**Interfaces:** +- Produces: `app.ServiceStopGrace = 2 * time.Minute`. + +- [ ] **Step 1: Write the failing tests** + +```go +func TestRenderedServicesCarryStopGrace(t *testing.T) { + // postgres and redis services rendered: each document has stop_grace_period: 2m0s +} + +func TestApplyServicesPassesStopTimeout(t *testing.T) { + // commands contain "up -d --remove-orphans --timeout 120" +} + +func TestRemoveServicesStopsGracefullyBeforeRemoval(t *testing.T) { + // for container SVC1: command contains "docker stop -t 120 SVC1 >/dev/null 2>&1; docker rm -f SVC1" +} +``` + +- [ ] **Step 2: Run** `go test ./internal/app ./internal/engine -run 'StopGrace|StopTimeout|StopsGracefully'` — Expected: FAIL. + +- [ ] **Step 3: Implement.** Render `"stop_grace_period": ServiceStopGrace.String()`. Apply and destroy derive 120 from `int(app.ServiceStopGrace.Seconds())`. Destroy's `rm -f` result still decides success; the stop is best effort. + +- [ ] **Step 4: Regenerate goldens.** Run `ONEBOX_UPDATE_VERDICTS=1 go test ./internal/app -run TestContractDidNotMove`, then `git diff --stat internal/app/testdata` — Expected: only `contract-verdicts.json` changes. + +- [ ] **Step 5: Docs.** In `add-a-database.mdx`, beside the `/dev/shm` paragraph: services get up to 2 minutes to shut down cleanly; the first `ob service apply` after upgrading recreates every service, and deploy does so for PostgreSQL services with declared extensions. + +- [ ] **Step 6: Run** the gate — Expected: PASS. + +- [ ] **Step 7: Commit** `fix(services): give managed services two minutes to shut down` — body: a 10s kill forced crash recovery or AOF replay; rollout recreates each service once, as with #220. + +--- + +## Slice 4: Rolling routing gates + +### Task 7: Read published routing + +**Files:** +- Modify: `internal/discovery/docker.go:102-107` +- Create: `internal/engine/routing.go`, `internal/engine/routing_test.go` +- Test: `internal/discovery/discovery_test.go` + +**Interfaces:** +- Produces: + - `func discovery.EndpointAddress(ipv4, ipv6 string) string` — `ipv4` when it parses as an IP, else `ipv6`; `inspect` uses it. + - `const routePublishBudget = 30 * time.Second` + - `func (e *Engine) routeGated(role app.Workload) bool` + - `func routedIn(doc discovery.Dynamic, n app.Names, workload string, role app.Workload, address string) bool` — address is a server of every expected service. + - `func withdrawnFrom(doc discovery.Dynamic, n app.Names, workload string, role app.Workload, address string) bool` — address is a server of none. + - Expected services: for route `i`, `n.ProxyServiceFor(workload, i)`, under `TCP.Services` when `Protocol == "tcp"`, else `HTTP.Services`. Hosts compare by `net.ParseIP(...).Equal`; HTTP hosts come from `url.Parse(URL).Hostname()`, TCP hosts from `net.SplitHostPort(Address)`. + - `func (e *Engine) routeAddress(ctx context.Context, id string) (string, error)` — `docker inspect -f '{{with index .NetworkSettings.Networks ""}}{{.IPAddress}} {{.GlobalIPv6Address}}{{end}}' `, through `EndpointAddress`; empty is an error naming the network. + - `func (e *Engine) readPublishedRoutes(ctx context.Context) (discovery.Dynamic, error)` — `cat` of `proxy.HostPaths(e.names()).Dynamic + "/onebox.yml"`; non-zero exit or YAML error is an error. + - `func (e *Engine) awaitRouting(ctx context.Context, workload string, role app.Workload, id string, routed bool, pollEvery time.Duration) error` — resolves the address once, polls until `routedIn` (routed) or `withdrawnFrom` (not routed) holds, deadline `e.Opts.Now() + routePublishBudget`. Errors: routed → contains `did not publish` and `docker logs onebox-discovery`; withdrawn → contains `still routes` and `ob resume`; both include the last read error when there was one. + +- [ ] **Step 1: Write the failing tests** + +```go +func TestEndpointAddressPrefersIPv4(t *testing.T) // ("10.0.0.5","fd00::5") → "10.0.0.5"; ("","fd00::5") → "fd00::5" +func TestRoutedInRequiresEveryExpectedService(t *testing.T) // web: http:3000 + tcp:5432; only the HTTP server lists 10.0.0.5 → false; add TCP "10.0.0.5:5432" → true +func TestRoutedInMatchesIPv6(t *testing.T) // "http://[fd00::5]:3000" vs "fd00::5" → true +func TestWithdrawnFromEveryService(t *testing.T) // absent everywhere → true; in one service → false +func TestRouteGatedOnlyForManagedProxyWithRoutes(t *testing.T) +func TestAwaitRoutingTreatsReadFailureAsNeither(t *testing.T) // cat exits 1 every time; virtualClock; both modes return errors, never nil +``` + +- [ ] **Step 2: Run** `go test ./internal/discovery ./internal/engine -run 'EndpointAddress|RoutedIn|WithdrawnFrom|RouteGated|AwaitRouting'` — Expected: FAIL. + +- [ ] **Step 3: Implement** the interfaces above. + +- [ ] **Step 4: Run** the gate — Expected: PASS. + +- [ ] **Step 5: Commit** `feat(rolling): read the routes discovery publishes` + +### Task 8: Admit newcomers before retiring anything + +**Files:** +- Modify: `internal/engine/roll.go:132-258` (`rollRoleForRelease`) +- Test: `internal/engine/roll_test.go` + +**Interfaces:** +- Consumes: `routeGated`, `awaitRouting` (Task 7). +- Produces: `func (e *Engine) admitNewcomers(ctx context.Context, workload string, role app.Workload, ids []string, within, pollEvery time.Duration) error` — each id must pass `waitHealth(id, "healthy", within, pollEvery)`, then `awaitRouting(..., true, ...)` when gated. It never removes a container. + +- [ ] **Step 1: Write the failing tests.** Add a helper `routedRollFake(base *transport.Fake, publishFrom map[string]int)` that answers the `routeAddress` inspect with `10.0.0.` per id and the `onebox.yml` read with a document listing each id from its N-th read on; the config sets `Proxy.Managed = true` and one HTTP route on `web`. + +```go +func TestRollRetiresOldOnlyAfterNewcomerPublished(t *testing.T) // NEW1 published from read 3: "docker exec OLD1 … touch" comes after the 3rd onebox.yml read +func TestRollRemovesNewcomerNeverPublished(t *testing.T) // never published: error contains "did not publish"; "docker rm -f NEW1" present; no "docker exec OLD1" +func TestRollResumeAdmitsAdoptedNewcomerBeforeRetiring(t *testing.T) // resume=true, NEW1 health stays "starting": error; no "docker exec OLD1"; no "docker rm -f NEW1" +func TestRollUnmanagedProxyReadsNoRoutes(t *testing.T) // testConfig(): no command reads onebox.yml +``` + +- [ ] **Step 2: Run** `go test ./internal/engine -run 'TestRoll(RetiresOld|RemovesNewcomer|ResumeAdmits|UnmanagedProxy)'` — Expected: FAIL. + +- [ ] **Step 3: Implement.** In the surge block, after `waitHealth` succeeds and when gated, `awaitRouting` the new id; on failure remove it exactly as the health failure does (`roll.go:233-235`). Call `admitNewcomers` once per pass immediately before retiring a surplus (`news[:desired]`), before retiring an old (`news`), and before the success `break` (`news`). + +- [ ] **Step 4: Run** the gate — Expected: PASS, including `TestRoll*` resume adoption (`roll_test.go:174`). + +- [ ] **Step 5: Commit** `fix(rolling): never retire a replica before its replacement is routed` + +### Task 9: Withdraw before stopping + +**Files:** +- Modify: `internal/engine/roll.go:260-312` (`retireContainer`), callers `roll.go:169`, `roll.go:242`, `internal/engine/retired_workloads.go:43` +- Test: `internal/engine/roll_test.go` + +**Interfaces:** +- Consumes: `routeGated`, `awaitRouting` (Task 7). +- Produces: `retireContainer(ctx context.Context, workload string, role app.Workload, id string, pollEvery time.Duration) error`. + +- [ ] **Step 1: Write the failing tests** (call `retireContainer` directly on a routed fake) + +```go +func TestRetireWaitsForWithdrawalBeforeStop(t *testing.T) // OLD1 listed for 2 reads, then absent: "docker stop -t" OLD1 after the 3rd read +func TestRetireStopsWhenWithdrawalTimesOut(t *testing.T) // always listed, virtualClock: error contains "still routes"; no "docker stop" OLD1 +func TestRetireUnreadableRoutesAreNotWithdrawal(t *testing.T) // cat exits 1 every time: error; no "docker stop" OLD1 +func TestRetireUnguardedHealthcheckSkipsWithdrawalGate(t *testing.T) // exec-form healthcheck: no onebox.yml read; stop proceeds after the buffers +``` + +- [ ] **Step 2: Run** `go test ./internal/engine -run TestRetire` — Expected: FAIL. + +- [ ] **Step 3: Implement.** In the guarded branch, after the drain flip wait and before the second `ConvergeBuffer` sleep, `awaitRouting(ctx, workload, role, id, false, pollEvery)` when gated; return its error. The unguarded branch is unchanged. + +- [ ] **Step 4: Run** the gate — Expected: PASS. + +- [ ] **Step 5: Commit** `fix(rolling): stop a replica only after the proxy has dropped it` + +--- + +## Slice 5: Recovery + +### Task 10: Restore before removing serving containers + +**Files:** +- Modify: `internal/engine/recovery.go:169-186` (`recoverInterrupted` order), add `removeInterruptedNonServing` beside `removeExactReleaseContainers` +- Test: `internal/engine/recovery_test.go` + +**Interfaces:** +- Consumes: `admitNewcomers` (Task 8) through `RollRole`. +- Produces: `func (e *Engine) removeInterruptedNonServing(ctx context.Context, interruptedID string, previous *Engine) error`. Lists `docker ps -a --filter label=onebox.app= --filter label=onebox.release= --format '{{.ID}}|{{.Label "com.docker.compose.service"}}|{{.Names}}|{{.State}}'`. Keeps a container only when `previous != nil`, its state is `running`, `previous.Spec.Workloads[service]` exists with `Mode() == "rolling"`, and its name is not `e.names().TransientContainer(service)`. Removes the rest with the existing guarded `docker rm -f` command. + +- [ ] **Step 1: Write the failing tests** + +```go +func TestRecoveryRollsPredecessorBeforeRemovingServingContainer(t *testing.T) { + // interrupted release owns NEW1 (running, service web, slot name) + // want: first "--scale web=" precedes every command that stops or removes NEW1 +} + +func TestRecoveryWithoutPredecessorRemovesEverythingFirst(t *testing.T) { + // previous == nil: every interrupted container removed before finalization, as today +} +``` + +Update `TestRecoveryRetryKeepsCheckpointUntilHealthyAndSweepsStaleRoles` so its fake answers the new listing format: `STALE1` (service not in the predecessor) is removed before restore; `NEW1` is retired by the roll or swept after it. Its verify-phase and checkpoint assertions stay. + +- [ ] **Step 2: Run** `go test ./internal/engine -run TestRecovery` — Expected: the new tests FAIL. + +Also, decided during implementation: `restoreReleaseRoles(ctx context.Context, previous *Engine, previousID, interruptedID string) error` retains a workload only when `retainableWorkload(...)` holds and no `observed[role]` container has `release == interruptedID`. + +```go +func TestRestoreReleaseRolesRollsInterruptedOwnedMatchingRevision(t *testing.T) { + // web's only container carries the predecessor's revision but the interrupted release label + // want: a "--scale web=" surge, and that container drained and retired by the roll, not retained +} +``` + +`TestRestoreReleaseRolesRetainsMatchingTargetRevisions` passes the new argument and keeps its assertions. + +- [ ] **Step 3: Implement** the order: `removeInterruptedNonServing` (phase `cleanup`), `restoreReleaseRoles` (phase `restore`), `removeExactReleaseContainers` (phase `sweep`), `Verify` (phase `verify`), `assertNoReleaseContainers` (phase `cleanup-postcondition`), then finalization unchanged. + +- [ ] **Step 4: Run** the gate — Expected: PASS. + +- [ ] **Step 5: Commit** `fix(recovery): restore the predecessor before removing serving containers` + +--- + +### Task 11: Docs and final gates + +**Files:** +- Modify: `site/src/content/docs/explanation/safety-envelope.mdx` (rolling and proxy paragraphs), `docs/plans/2026-10-17-zero-downtime-gaps.md` (status: implemented) + +- [ ] **Step 1: Docs.** In `safety-envelope.mdx`: a roll retires a replica only after discovery has published its replacement and stops it only after the proxy has dropped it; Traefik restarts only when its configuration changes, not on `ob` upgrades; the plan's `proxy:` line says which. + +- [ ] **Step 2: Run** `just check && just env-namespace` — Expected: PASS. If `site-build` reports missing packages, run `just site-install` first. + +- [ ] **Step 3: Commit** `docs: describe the routing gates and when Traefik restarts` + +- [ ] **Step 4: Operator check before release.** Record `docker compose version` on each target host. If any is older than the version recorded in Task 3 Step 1, rerun that script there before releasing. Also check, on each host, that `docker compose stop a` leaves a `depends_on` dependent `b` running and that `up -d a` keeps `b`'s container; that `docker inspect -f '{{.Image}}'` of a container equals `docker image inspect -f '{{.Id}}'` of its tag; and that `up -d` recreates a container after its tag is re-pulled to a new image. + +--- + +## Slice 6: Review round (spec §6) + +Same gate and commit discipline. Every existing assertion stays; a test that the review found vacuous is tightened, never deleted. + +### Task 12: Withdrawal that survives route changes (R1) + +**Files:** `internal/engine/routing.go`, `internal/engine/roll.go:295-340`, `internal/engine/roll_test.go`, `internal/engine/routing_test.go` + +**Interfaces:** +- `func withdrawnFromAll(doc discovery.Dynamic, address string) bool` — address is a server of no HTTP and no TCP service in the document. Replaces `withdrawnFrom` in the withdrawal gate. +- `routeAddress` returns `("", nil)` when the container has no address on the proxy network; the routed gate still treats that as an error. +- `retireContainer` gates withdrawal on `e.Spec.Proxy.Managed` alone. + +- [ ] **Tests:** + +```go +func TestRetireOldReplicaOffProxyNetworkIsWithdrawn(t *testing.T) // inspect prints " ": stop proceeds; no onebox.yml wait +func TestRetireWaitsWhileOldReplicaServesARemovedRoute(t *testing.T) // new role has no routes; OLD1 listed under onebox-web for 2 reads: stop after the 3rd +func TestRetireWaitsWhileOldReplicaServesAnotherProtocol(t *testing.T) // new role http; OLD1 listed under a TCP service: waits, then error on timeout +func TestRollResumeBlocksAdoptedHealthyUnpublishedNewcomer(t *testing.T) // managed proxy, adopted NEW1 healthy, never published: error; no "docker exec OLD1" +func TestRollWithdrawsOldReplicaInsideFullRoll(t *testing.T) // routedRollFake lists OLD1 until drained: "docker stop" OLD1 after the read without it +``` + +- [ ] **Commit** `fix(rolling): withdraw old replicas whatever the new routes are` + +### Task 13: Converge without a restart on retry, with fresh evidence (R2, R3, R5, R6) + +**Files:** `internal/engine/proxy.go:85-230`, `internal/engine/proxy_test.go` + +**Interfaces:** +- `const pendingComposeDigest = "sha256:" + 64 × "0"`; the converge path calls `commitProxyIdentity(ctx, proxy.Identity{Config: o.Applied.Config, Compose: pendingComposeDigest})` where it now deletes the marker. +- Converge path: upload a staging dir holding only `compose.yaml`; `compose stop discovery`; sample `outputInodeCommand`; `up -d discovery` (with `--force-recreate` when discovery or its output was missing); `waitForPublication(ctx, sampledInode)`; `up -d proxy`; no restart. +- Config path swap: move live `config` and `dynamic` (when present) into `/retired/`, then move the staged ones in; no `rm -rf` of live directories. The staged dir (with `retired/`) is removed by the deferred cleanup only when the apply succeeded. + +- [ ] **Tests** (and tighten the review's vacuous ones): + +```go +func TestEnsureProxyFailedConvergeRetriesAsConverge(t *testing.T) // converge fails at "up -d discovery"; marker holds " sha256:000…"; second run: no config swap, no "docker restart" +func TestEnsureProxyConvergeStopsDiscoveryBeforeSampling(t *testing.T) // "stop discovery" precedes the inode sample, which precedes "up -d discovery" +func TestEnsureProxyConvergeUploadsOnlyCompose(t *testing.T) // the uploaded dir holds compose.yaml only +func TestEnsureProxyConfigSwapRetiresLiveDirsInsteadOfDeleting(t *testing.T) // no "rm -rf '/var/lib/onebox/_host/proxy/config'"; "mv … /retired/config" present +``` + +Tighten: `TestEnsureProxyComposeOnlyChangeSkipsSwapAndRestart` asserts the marker write after the last discovery publication wait (not after a `docker inspect` that path never runs); `TestEnsureProxyMissingOutputForceRecreatesDiscoveryWithoutRestart` returns `""` before and a new inode after; `proxyPS` answers the discovery query with its own id. + +- [ ] **Commit** `fix(proxy): retry a converge as a converge and keep routes until restart` + +### Task 14: Name a Traefik replacement in the plan (R4) + +**Files:** `internal/engine/proxy_converge.go`, `internal/engine/proxy.go`, `internal/engine/proxy_converge_test.go` + +**Interfaces:** +- `func traefikDefinitionChanged(live, rendered []byte) bool` — parse both as YAML; compare `services.proxy` and top-level `networks` with `reflect.DeepEqual`; parse error or empty live → `true`. +- `observeProxy(ctx context.Context, rendered []byte) (proxyObservation, error)` also reads the live `compose.yaml` and sets `TraefikChanged bool`. +- New action `proxyReplace`, chosen by `decideProxy` when it would choose `proxyConverge` and `TraefikChanged` is true. `EnsureProxy` runs it exactly as `proxyConverge`. `String()` → `"recreate Traefik (its container definition changed)"`. +- `func proxyPlanLine(o proxyObservation, a proxyAction) string` → `"start Traefik"` when `len(o.ProxyIDs) == 0`, else `a.String()`. `PlanProxy` returns it. + +- [ ] **Tests:** + +```go +func TestTraefikDefinitionChanged(t *testing.T) // discovery image only → false; traefik image → true; ports → true; unparsable live → true +func TestDecideProxyNamesTraefikReplacement(t *testing.T) // compose drift + TraefikChanged → proxyReplace +func TestPlanProxySaysStartWhenTraefikMissing(t *testing.T) +``` + +- [ ] **Commit** `fix(plan): say when Traefik itself will be replaced` + +### Task 15: Single-replica recovery keeps serving (R7) + +**Files:** `internal/engine/recovery.go`, `internal/engine/recovery_test.go` + +**Interfaces:** +- In `removeInterruptedNonServing`: a running container of a rolling predecessor workload named `TransientContainer(service)` is renamed with `renameContainer` to the lowest `Container(service, i)`, `i ≥ 1`, that no container (any state) holds; it is then kept like any other serving container. +- `func (e *Engine) undrainPredecessorReplicas(ctx context.Context, previous *Engine, previousID string) error` — for each running container labelled `onebox.release=previousID` of a rolling workload, `docker exec rm -f /tmp/onebox-drain` (`app.DrainFile`), best effort; called right before `restoreReleaseRoles`. + +- [ ] **Tests:** + +```go +func TestRecoverySingleReplicaKeepsInterruptedNewcomerServing(t *testing.T) +// replicas: 1. NEW1 (interrupted release) holds the transient name and is healthy; OLD1 (predecessor) is running and drained. +// want, in order: NEW1 renamed to the free slot (never `docker rm -f NEW1` before OLD1 is admitted); +// `docker exec OLD1 rm -f /tmp/onebox-drain`; OLD1 adopted and admitted (healthy, and published when the proxy is managed); +// only then NEW1 drained, stopped and removed. No surge: the predecessor's own replica is adopted. +``` + +Corrected during implementation: the predecessor's running replica is adopted, not replaced, so the roll does not surge. + +Fix `TestRecoveryRollsPredecessorBeforeRemovingServingContainer` to use the fake's own `OLD1` listing and assert touch → stop → rm of `OLD1` with `NEW1` surviving. + +- [ ] **Commit** `fix(recovery): keep a single-replica workload serving through recovery` + +### Task 16: Docs for the review round + +**Files:** `site/src/content/docs/explanation/safety-envelope.mdx` + +- [ ] Correct the proxy sentence: Traefik restarts when its configuration changes and is recreated when its own image or ports change; `ob` upgrades and discovery repair do neither otherwise; the plan's `proxy:` line says which. State the assumption that Traefik loads published routes within the 3s converge buffer. State that the first proxy apply after upgrading restarts Traefik once and the first `ob service apply` recreates each managed service once. +- [ ] **Commit** `docs: describe proxy actions and the one-time upgrade restarts` + +### Task 17: Second review round (spec §6 R8-R11) + +**Files:** `internal/engine/proxy.go`, `internal/engine/proxy_converge.go`, `internal/engine/routing.go`, `internal/engine/recovery_test.go`, and their tests + +**Interfaces:** +- `const pendingReplaceDigest = "sha256:" + 63 × "0" + "1"`. The replace path commits `{Config: o.Applied.Config, Compose: pendingReplaceDigest}`; the converge path keeps `pendingComposeDigest`. `observeProxy` sets `TraefikChanged = true` when `Applied.Compose == pendingReplaceDigest`. +- Before `compose stop discovery`: `docker image inspect >/dev/null 2>&1 || docker pull ` with ` = proxy.DiscoveryImage(e.Opts.Runner.Version)`; a failed pull returns before the stop. +- After the stop, any returned error first runs `docker start onebox-discovery` (best effort; its result is logged, not returned). +- The `proxyUnchanged` branch runs the same stale `.staged-*` cleanup the success path runs. +- Remove `withdrawnFrom` and its test. + +- [ ] **Tests:** + +```go +func TestEnsureProxyFailedReplaceIsPlannedAsReplace(t *testing.T) // replace fails at "up -d discovery": marker holds pendingReplaceDigest; PlanProxy then says "recreate Traefik (its container definition changed)" +func TestEnsureProxyPullsDiscoveryBeforeStopping(t *testing.T) // image missing: "docker pull" precedes "stop discovery"; failed pull: no "stop discovery" +func TestEnsureProxyRestartsDiscoveryAfterFailedUp(t *testing.T) // "up -d discovery" fails: "docker start onebox-discovery" runs before the error returns +func TestEnsureProxyUnchangedCleansStaleStaging(t *testing.T) // unchanged path runs the ".staged-*" cleanup +func TestRecoverySurgesWhenOnlyInterruptedNewcomerServes(t *testing.T) // replicas: 1, no predecessor replica, NEW1 holds the transient name: NEW1 renamed to slot 1, predecessor surge takes the transient name, then NEW1 drained, stopped, removed +``` + +- [ ] **Gate:** `go build ./... && go vet ./... && go test -p 2 ./...`; `golangci-lint run ./...` 0 issues. +- [ ] **Commit** `fix(proxy): never leave discovery stopped and keep a failed replace labelled` + +### Task 18: Code review round (spec §6 R12-R17) + +**Files:** `internal/engine/proxy.go`, `internal/engine/proxy_converge.go`, `internal/engine/roll.go`, `internal/engine/routing.go`, the deploy execution path that sets engine options from a sealed plan (`internal/onebox/execute.go`, beside `options.WorkloadPlans`), and their tests + +**Interfaces:** +- R12: the apply path runs the same `docker image inspect >/dev/null 2>&1 || docker pull ` before `up -d --force-recreate discovery` and sets `discoveryStopped = true` before it. The deferred fallback runs `docker start onebox-discovery`; when that exits non-zero, `docker compose -p onebox-proxy -f up -d discovery`. Both best effort, logged. +- R13: engine option `ExpectedProxyAction string`; set from `plan.ProxyAction` when executing a sealed deploy plan. When non-empty, `EnsureProxy` computes `proxyPlanLine(o, action)` after `observeProxy` and returns a drift error naming both lines when they differ, before any mutation. +- R14: in `observeProxy`, when `len(ProxyIDs) > 0`: `docker inspect -f '{{.Image}}' ` versus `docker image inspect -f '{{.Id}}' ` (the image `RenderComposeForApp` uses); differ or tag missing → `TraefikChanged = true`. +- R15: before observing, remove `.staged-*` directories that contain no `retired/`. +- R16: `rollRoleForRelease` keeps an `admitted map[string]bool`; `admitNewcomers` skips ids already admitted in this invocation and records the ones it admits. +- R17: routing error text uses `proxy.DiscoveryContainerName`. + +- [ ] **Tests:** + +```go +func TestEnsureProxyApplyRestoresDiscoveryWhenRecreateFails(t *testing.T) // force-recreate fails, "docker start" exits 1: "compose … up -d discovery" fallback runs +func TestEnsureProxyApplyPullsDiscoveryBeforeRecreate(t *testing.T) +func TestEnsureProxyRefusesPlannedActionDrift(t *testing.T) // ExpectedProxyAction "unchanged", observation says restart: error, no mutation after observe +func TestObserveProxyFlagsRepulledTraefikImage(t *testing.T) // container image ID ≠ tag ID → TraefikChanged; equal → false (definition otherwise equal) +func TestEnsureProxyRemovesFailedUploadsButKeepsRetired(t *testing.T) +func TestRollAdmitsEachNewcomerOnce(t *testing.T) // replicas: 3: each newcomer's health inspect for admission runs once +``` + +Plus one execution-path test that a sealed plan's `ProxyAction` reaches `ExpectedProxyAction`. + +- [ ] **Gate:** `go build ./... && go vet ./... && go test -p 2 ./...`; `golangci-lint run ./...` 0 issues. +- [ ] **Commit(s)**: `fix(proxy): restore discovery on every path and bind the planned proxy action`, `perf(rolling): admit each newcomer once`. + +### Task 19: Final review round (spec §6 R16 withdrawn, R18, R19) + +**Files:** `internal/engine/roll.go`, `internal/engine/proxy.go`, `internal/engine/proxy_converge.go`, their tests, `internal/onebox/proxy_execute_test.go` + +- Revert the R16 behavior: `admitNewcomers` re-checks every newcomer it is given; remove the `admitted` map and `TestRollAdmitsEachNewcomerOnce`. +- R18: on `proxyConverge` and `proxyReplace`, `waitHealth(ids[0], "healthy", 180s, 5s)` always runs before `commitProxyIdentity`. +- R19: move the apply path's discovery image pull to right after the drift check; give the discovery fallback its own 30s context and path-neutral log text; take the Traefik image for the R14 comparison from `services.proxy.image` in the rendered compose. + +- [ ] **Tests:** + +```go +func TestRollStopsWhenAdmittedNewcomerFailsLater(t *testing.T) // steady 3: NEW1 turns unhealthy after OLD1 is removed: error; OLD2, OLD3 still serving +func TestRollKeepsLastHealthyOldWhenNewcomersFail(t *testing.T) // 3→2: both newcomers unhealthy after OLD2 is removed: error; OLD3 not retired +func TestEnsureProxyRetryWaitsForHealthBeforeCommit(t *testing.T) // pending marker, PX1 unhealthy, not recreated: error; no final marker +func TestEnsureProxyProceedsWhenPlannedActionMatches(t *testing.T) // engine converge with matching ExpectedProxyAction: commits +func TestExecuteProceedsWhenSealedProxyActionMatches(t *testing.T) // onebox: deploy reaches transfer; no-op path succeeds +func TestEnsureProxyFallbackSkipsComposeWhenStartSucceeds(t *testing.T) // exactly one "up -d discovery" (the failed one) +func TestEnsureProxyApplyPullsBeforeInvalidatingMarker(t *testing.T) // pull precedes "rm -f …config.hash" +``` + +- [ ] **Gate:** `go build ./... && go vet ./... && go test -p 2 ./...`; `golangci-lint run ./...` 0 issues. +- [ ] **Commit(s):** `fix(rolling): re-check every newcomer before each retirement`, `fix(proxy): commit the marker only after health on every path`. diff --git a/internal/app/services.go b/internal/app/services.go index a40c76fd..0f395874 100644 --- a/internal/app/services.go +++ b/internal/app/services.go @@ -4,12 +4,16 @@ import ( "fmt" "sort" "strings" + "time" "github.com/compose-spec/compose-go/v2/types" "github.com/labstack/onebox/internal/shellquote" ) +// ServiceStopGrace gives managed services time to flush their durable state. +const ServiceStopGrace = 2 * time.Minute + // Supporting services are the reason this tool exists. Nearly every // application needs a database and a cache, nobody's application *is* the // database, and writing the same Postgres service block into every project is @@ -365,9 +369,10 @@ func (p *Spec) renderService(n Names, name string, s Service, selectedImage stri return nil, err } svc := map[string]any{ - "image": image, - "restart": "unless-stopped", - "container_name": n.ServiceContainer(name), + "image": image, + "restart": "unless-stopped", + "stop_grace_period": ServiceStopGrace.String(), + "container_name": n.ServiceContainer(name), "labels": map[string]any{ "onebox.app": p.Name, "onebox.service": name, diff --git a/internal/app/services_test.go b/internal/app/services_test.go index 74dd1af4..9ad3050f 100644 --- a/internal/app/services_test.go +++ b/internal/app/services_test.go @@ -646,3 +646,14 @@ func TestEphemeralServiceCannotDeclareBackup(t *testing.T) { t.Fatalf("refusal does not explain itself: %v", err) } } + +func TestRenderedServicesCarryStopGrace(t *testing.T) { + for _, driver := range []string{"postgres", "redis"} { + t.Run(driver, func(t *testing.T) { + doc := renderStore(t, "services: {store: {driver: "+driver+", version: '17'}}\n") + if !strings.Contains(doc, "stop_grace_period: 2m0s") { + t.Fatalf("service has no two-minute shutdown window:\n%s", doc) + } + }) + } +} diff --git a/internal/app/testdata/contract-verdicts.json b/internal/app/testdata/contract-verdicts.json index 8017da16..2e456206 100644 --- a/internal/app/testdata/contract-verdicts.json +++ b/internal/app/testdata/contract-verdicts.json @@ -392,17 +392,17 @@ { "case": "conformance/service backup policy", "loads": true, - "digest": "73135190e9f9fae42347e107f496211e4d2f75bfe5ff934ead0096f4f3571a19 postgres=ee21ed4fc49969fb" + "digest": "73135190e9f9fae42347e107f496211e4d2f75bfe5ff934ead0096f4f3571a19 postgres=d48aeac2a592c834" }, { "case": "conformance/service scalar", "loads": true, - "digest": "f6ddf429bf783de806d4b1475df352d4b3ff85210dfe379804248dd3fdd0b80a postgres=ff6eeff3e4bbd9bd" + "digest": "f6ddf429bf783de806d4b1475df352d4b3ff85210dfe379804248dd3fdd0b80a postgres=45f65def6f3b45e9" }, { "case": "conformance/settings key that is a real driver flag", "loads": true, - "digest": "f6ddf429bf783de806d4b1475df352d4b3ff85210dfe379804248dd3fdd0b80a redis=fd038e9c5b5c3b09" + "digest": "f6ddf429bf783de806d4b1475df352d4b3ff85210dfe379804248dd3fdd0b80a redis=c4d310541968b4d6" }, { "case": "conformance/settings key with a shell metacharacter", @@ -532,12 +532,12 @@ { "case": "corpus/authentik.yml", "loads": true, - "digest": "e82f2e9eac87e3494cf0aa86b80f91a60f363f0af7aac28bb45e1ec6ed2e40fb postgres=4d380770b261847a redis=53bad39e8f359a02" + "digest": "e82f2e9eac87e3494cf0aa86b80f91a60f363f0af7aac28bb45e1ec6ed2e40fb postgres=f81c980270118d21 redis=4f8002b0a56335d5" }, { "case": "corpus/ext-authentik-managed.yml", "loads": true, - "digest": "10748708444a383dc2e587286c13f06a4f3ea218835e2eeab1af83ed312ec741 postgres=4d380770b261847a redis=53bad39e8f359a02" + "digest": "10748708444a383dc2e587286c13f06a4f3ea218835e2eeab1af83ed312ec741 postgres=f81c980270118d21 redis=4f8002b0a56335d5" }, { "case": "corpus/ext-authentik.yml", @@ -552,7 +552,7 @@ { "case": "corpus/ext-gitea.yml", "loads": true, - "digest": "f6028df90e48b43f68c3375115ef402255ebd0aadf0ddde9eb9e2d538598cb65 postgres=105be1b298f59e44" + "digest": "f6028df90e48b43f68c3375115ef402255ebd0aadf0ddde9eb9e2d538598cb65 postgres=24bccf98808db97d" }, { "case": "corpus/ext-immich-sourced.yml", @@ -567,7 +567,7 @@ { "case": "corpus/ext-n8n.yml", "loads": true, - "digest": "3831bd9a52d01deada4936e218fc8ba33c96a0b1d939e6bf503f3fbda8ba0895 postgres=12dc8bcbdff40808 redis=83f7ec43501bb1f4" + "digest": "3831bd9a52d01deada4936e218fc8ba33c96a0b1d939e6bf503f3fbda8ba0895 postgres=5b23cbde31e9dc29 redis=0644b31adce178f9" }, { "case": "corpus/ext-paperless.yml", @@ -577,17 +577,17 @@ { "case": "corpus/ext-plausible.yml", "loads": true, - "digest": "4574810017062b8d857ad2c43597b484fe88e6b4198f75ec74c2d705cbcf17d1 events=79a3b4327069a1dc postgres=1aea911e88a3631c" + "digest": "4574810017062b8d857ad2c43597b484fe88e6b4198f75ec74c2d705cbcf17d1 events=47324284a6e77538 postgres=361a106a8e2ffde3" }, { "case": "corpus/ext-umami.yml", "loads": true, - "digest": "fd193a8c2f51cbe206e2e038ccd277c3e6785c84912b5967a2afd58c1320ac13 postgres=47bbd25b517473d9" + "digest": "fd193a8c2f51cbe206e2e038ccd277c3e6785c84912b5967a2afd58c1320ac13 postgres=d07ceb7c2c0392b1" }, { "case": "corpus/ghost.yml", "loads": true, - "digest": "7ab89c09054e821edfb0edd5c014b488ca06785d052f4178a688d4008961e1ad mysql=6dc2ab74dac73160" + "digest": "7ab89c09054e821edfb0edd5c014b488ca06785d052f4178a688d4008961e1ad mysql=393016b70855b2cd" }, { "case": "corpus/gitea.yml", @@ -622,7 +622,7 @@ { "case": "corpus/penpot.yml", "loads": true, - "digest": "1cd500aae4deac5cf744dec3f1222b13aec493457de4b6069a3449126ad70210 postgres=8a2a89524e0749e0 redis=e6f4fb5264fe75ae" + "digest": "1cd500aae4deac5cf744dec3f1222b13aec493457de4b6069a3449126ad70210 postgres=e98b19cff65fe766 redis=f8e5a12917a49994" }, { "case": "corpus/pursue.yml", @@ -637,7 +637,7 @@ { "case": "corpus/rocketchat.yml", "loads": true, - "digest": "4a091cd29c4a572eb57fcc9cd68edb76fbd56eb88ce16aab29efb30cb16af69f mongodb=09562fa5123be0a1" + "digest": "4a091cd29c4a572eb57fcc9cd68edb76fbd56eb88ce16aab29efb30cb16af69f mongodb=ada71cea467aa56d" }, { "case": "corpus/umami.yml", diff --git a/internal/discovery/discovery_test.go b/internal/discovery/discovery_test.go index 9af130b5..3f768209 100644 --- a/internal/discovery/discovery_test.go +++ b/internal/discovery/discovery_test.go @@ -160,3 +160,11 @@ func TestWriteAtomicProducesTraefikYAML(t *testing.T) { t.Fatalf("temporary files remain: %v", matches) } } + +func TestEndpointAddressPrefersIPv4(t *testing.T) { + for _, tc := range []struct{ v4, v6, want string }{{"10.0.0.5", "fd00::5", "10.0.0.5"}, {"", "fd00::5", "fd00::5"}, {"invalid", "fd00::5", "fd00::5"}} { + if got := EndpointAddress(tc.v4, tc.v6); got != tc.want { + t.Fatalf("EndpointAddress = %q, want %q", got, tc.want) + } + } +} diff --git a/internal/discovery/docker.go b/internal/discovery/docker.go index 72d72d44..2ed248c3 100644 --- a/internal/discovery/docker.go +++ b/internal/discovery/docker.go @@ -100,15 +100,19 @@ func (c *Client) inspect(ctx context.Context, id string) (Container, error) { container.Health = raw.State.Health.Status } for name, endpoint := range raw.NetworkSettings.Networks { - address := endpoint.IPAddress - if net.ParseIP(address) == nil { - address = endpoint.GlobalIPv6Address - } - container.Networks[name] = address + container.Networks[name] = EndpointAddress(endpoint.IPAddress, endpoint.GlobalIPv6Address) } return container, nil } +// EndpointAddress selects the same network address for discovery and rollout gates. +func EndpointAddress(ipv4, ipv6 string) string { + if net.ParseIP(ipv4) != nil { + return ipv4 + } + return ipv6 +} + func (c *Client) getJSON(ctx context.Context, path string, target any) error { req, err := http.NewRequestWithContext(ctx, http.MethodGet, "http://docker"+path, nil) if err != nil { diff --git a/internal/engine/deploy_test.go b/internal/engine/deploy_test.go index 26141a3a..4021b9af 100644 --- a/internal/engine/deploy_test.go +++ b/internal/engine/deploy_test.go @@ -71,9 +71,23 @@ func happyFake() *transport.Fake { scaleCount, recreateCount := 0, 0 initialScaleCount, initialRecreateCount := 0, 0 seenExactReleaseQuery := false + recoveryRelease := "" + interruptedOldGone := false newGone, workerGone := false, false name := map[string]string{"OLD1": "web"} for _, c := range f.Commands { + if recoveryRelease == "" && strings.Contains(c, "docker ps -a --filter label=onebox.app=") && strings.Contains(c, "label=onebox.release=") { + _, releaseFilter, _ := strings.Cut(c, "label=onebox.release=") + recoveryRelease = strings.Fields(releaseFilter)[0] + // Recovery rolls the predecessor into NEW1/W1; the interrupted + // web replica becomes OLD1 in this fixture's next roll. + scaled, oldGone, drained = false, false, false + scaleCount, recreateCount = 0, 0 + initialScaleCount, initialRecreateCount = 0, 0 + newGone, workerGone = false, true + name = map[string]string{"OLD1": "web"} + seenExactReleaseQuery = true + } if strings.Contains(c, "docker ps -aq") && strings.Contains(c, "label=onebox.app=") && strings.Contains(c, "label=onebox.release=") { seenExactReleaseQuery = true } @@ -90,8 +104,11 @@ func happyFake() *transport.Fake { initialRecreateCount++ } } - if strings.Contains(c, "docker rm OLD1") { + if strings.Contains(c, "docker rm OLD1") || strings.Contains(c, "docker rm -f OLD1") { oldGone = true + if recoveryRelease != "" { + interruptedOldGone = true + } } if strings.Contains(c, "docker rm -f NEW1") { newGone = true @@ -114,6 +131,11 @@ func happyFake() *transport.Fake { return fs[len(fs)-1] } switch { + case strings.Contains(cmd, "docker ps -a --filter label=onebox.app=") && strings.Contains(cmd, "label=onebox.release="): + if !interruptedOldGone { + return transport.Result{Stdout: "OLD1|web|sample-web-1|running"}, true + } + return transport.Result{}, true // Compose counts stopped replicas toward --scale, so the roll asks for // them separately; these fakes model none. case strings.Contains(cmd, "status=exited"): @@ -180,6 +202,12 @@ func happyFake() *transport.Fake { } return transport.Result{}, true case strings.Contains(cmd, "docker ps -aq") && strings.Contains(cmd, "label=onebox.app=") && strings.Contains(cmd, "label=onebox.release="): + if recoveryRelease != "" && strings.Contains(cmd, "label=onebox.release="+recoveryRelease) { + if !interruptedOldGone { + return transport.Result{Stdout: "OLD1\n"}, true + } + return transport.Result{}, true + } var ids []string if initialScaleCount > 0 && !newGone { ids = append(ids, "NEW1") @@ -299,7 +327,17 @@ func TestDeployUpgradesManagedProxyAfterReadOnlyPreconditions(t *testing.T) { f := happyFake() base := f.Dynamic proxyRuntime := legacyProxyPS(f) + drained := false f.Dynamic = func(command string) (transport.Result, bool) { + if strings.Contains(command, "docker exec OLD1 touch ") { + drained = true + } + if strings.HasPrefix(command, "cat ") && strings.Contains(command, "/dynamic/onebox.yml") { + if !drained { + return transport.Result{Stdout: "http:\n services:\n previous:\n loadBalancer:\n servers:\n - url: http://172.20.0.5:3000\n"}, true + } + return transport.Result{Stdout: "{}\n"}, true + } if result, ok := proxyRuntime(command); ok { return result, true } diff --git a/internal/engine/engine.go b/internal/engine/engine.go index f9017911..0b4bc542 100644 --- a/internal/engine/engine.go +++ b/internal/engine/engine.go @@ -76,6 +76,8 @@ type Options struct { // WorkloadPlans are the sealed per-workload actions approved for this // deployment. An absent entry preserves the historical release behavior. WorkloadPlans map[string]WorkloadPlan + // ExpectedProxyAction is the proxy plan line approved for this deployment. + ExpectedProxyAction string // UI is the output layer; built from Out+Verbose when unset (cmd shares // one instance so the command log and narrative interleave in order). UI *ui.UI diff --git a/internal/engine/proxy.go b/internal/engine/proxy.go index 4125246d..1c29bd88 100644 --- a/internal/engine/proxy.go +++ b/internal/engine/proxy.go @@ -16,6 +16,11 @@ import ( "github.com/labstack/onebox/internal/proxy" ) +const ( + pendingComposeDigest = "sha256:0000000000000000000000000000000000000000000000000000000000000000" + pendingReplaceDigest = "sha256:0000000000000000000000000000000000000000000000000000000000000001" +) + // EnsureProxy converges the HOST-scoped managed proxy (design: one Traefik // per host, owned by its sole Onebox application). Idempotent and ACME-safe: an // unchanged proxy is never touched; a compose change recreates the container @@ -26,24 +31,16 @@ func (e *Engine) EnsureProxy(ctx context.Context, deployID string, breakLock boo return nil } hp := proxy.HostPaths(e.names()) - // Empty stays empty: joining it with the project directory would point at - // the repository root and ask it to be Traefik's config. - localCfg := e.Spec.Proxy.Config - if localCfg != "" && !filepath.IsAbs(localCfg) { - localCfg = filepath.Join(e.Opts.LocalDir, localCfg) - } staging, err := os.MkdirTemp("", "onebox-proxy") if err != nil { return err } defer os.RemoveAll(staging) - discoveryImage := proxy.DiscoveryImage(e.Opts.Runner.Version) - hash, err := proxy.StageForAppManaged(localCfg, staging, e.Spec.Proxy.Image, discoveryImage, - e.Spec.Name, e.Spec.Proxy.Network, e.Spec.Proxy.Entrypoints, e.Spec.HasExactTerminatingTLS(), - e.Spec.HasWildcardTerminatingTLS(), e.Spec.Proxy.DNSChallenge) + id, err := e.stageProxy(staging) if err != nil { return err } + hash := id.String() rendered, err := os.ReadFile(filepath.Join(staging, "compose.yaml")) if err != nil { return err @@ -57,6 +54,43 @@ func (e *Engine) EnsureProxy(ctx context.Context, deployID string, breakLock boo return err } defer e.releaseHostLock(ctx) + discoveryStopped := false + defer func() { + if err == nil || !discoveryStopped { + return + } + fallbackContext, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + res, startErr := e.hostMutate(fallbackContext, "docker start "+proxy.DiscoveryContainerName) + if startErr == nil && res.ExitCode != 0 { + startErr = fmt.Errorf("exit %d: %s", res.ExitCode, strings.TrimSpace(res.Stderr)) + } + if startErr != nil { + e.logf("proxy: discovery not restarted after failure: %v", startErr) + } else { + e.logf("proxy: discovery restarted after failure") + } + if res.ExitCode != 0 && startErr != nil { + res, upErr := e.hostMutate(fallbackContext, "docker compose -p "+proxy.Project+" -f "+q(hp.Compose)+" up -d discovery") + if upErr == nil && res.ExitCode != 0 { + upErr = fmt.Errorf("exit %d: %s", res.ExitCode, strings.TrimSpace(res.Stderr)) + } + if upErr != nil { + e.logf("proxy: discovery not recreated after failure: %v", upErr) + } else { + e.logf("proxy: discovery recreated after failure") + } + } + }() + applied := false + defer func() { + if err != nil || !applied { + return + } + cleanupContext, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + _, _ = e.hostMutate(cleanupContext, "find "+q(hp.Dir)+" -mindepth 1 -maxdepth 1 -type d -name '.staged-*' -exec rm -rf -- {} + 2>/dev/null || true") + }() jw := &journal.Writer{T: e.T, Dir: e.names().HostJournalDir(), DeployID: deployID, Operator: journal.DefaultOperator(), GitSHA: e.Opts.GitSHA, ConfigHash: e.Opts.ConfigHash, Runner: &e.Opts.Runner} if err := jw.Append(ctx, journal.Record{Phase: "proxy-apply", Event: "start", Detail: "hash=" + hash}); err != nil { return fmt.Errorf("journal proxy apply start: %w", err) @@ -73,14 +107,37 @@ func (e *Engine) EnsureProxy(ctx context.Context, deployID string, breakLock boo } e.pruneHostJournal(ctx) }() - res, err := e.hostMutate(ctx, "find "+q(hp.Dir)+" -mindepth 1 -maxdepth 1 -type d -name '.staged-*' -exec rm -rf -- {} + 2>/dev/null || true") + o, err := e.observeProxy(ctx, rendered) if err != nil { - return fmt.Errorf("clean stale proxy staging: %w", err) + return err } - if res.ExitCode != 0 { - return fmt.Errorf("clean stale proxy staging: %s", strings.TrimSpace(res.Stderr)) + action := decideProxy(o, id) + if expected := e.Opts.ExpectedProxyAction; expected != "" { + if current := proxyPlanLine(o, action); current != expected { + return fmt.Errorf("proxy action changed since plan: planned %q, current %q — re-plan", expected, current) + } + } + pullDiscovery := func() error { + image := q(proxy.DiscoveryImage(e.Opts.Runner.Version)) + if res, err := e.hostMutate(ctx, "docker image inspect "+image+" >/dev/null 2>&1 || docker pull "+image); err != nil { + return err + } else if res.ExitCode != 0 { + return fmt.Errorf("proxy discovery image: %s", strings.TrimSpace(res.Stderr)) + } + return nil + } + if action == proxyApply { + if err := pullDiscovery(); err != nil { + return err + } + } + cleanupUploads := "find " + q(hp.Dir) + " -mindepth 1 -maxdepth 1 -type d -name " + q(".staged-*") + + " ! -exec test -d " + q("{}/retired") + " \\; -exec rm -rf -- {} + 2>/dev/null || true" + if res, err := e.hostMutate(ctx, cleanupUploads); err != nil { + return fmt.Errorf("clean stale proxy uploads: %w", err) + } else if res.ExitCode != 0 { + return fmt.Errorf("clean stale proxy uploads: %s", strings.TrimSpace(res.Stderr)) } - acmeJSON := hp.Acme + "/acme.json" prepareDirs := "mkdir -p " + q(hp.Acme) + " " + q(hp.Dynamic) + " && { test -f " + q(acmeJSON) + " || (touch " + q(acmeJSON) + " && chmod 600 " + q(acmeJSON) + "); }" + @@ -92,31 +149,14 @@ func (e *Engine) EnsureProxy(ctx context.Context, deployID string, breakLock boo return fmt.Errorf("host proxy dirs: %s", res.Stderr) } - res, err = e.T.Run(ctx, "cat "+q(hp.Hash)+" 2>/dev/null || true") - if err != nil { - return err - } - remoteHash := strings.TrimSpace(res.Stdout) - ids, err := e.proxyContainerIDs(ctx) - if err != nil { - return err - } - discoveryIDs, err := e.discoveryContainerIDs(ctx) - if err != nil { - return err - } - res, err = e.T.Run(ctx, "test -s "+q(hp.Dynamic+"/onebox.yml")) - if err != nil { - return err - } - discoveryReady := res.ExitCode == 0 - - if remoteHash == hash && len(ids) > 0 && len(discoveryIDs) > 0 && discoveryReady { + switch action { + case proxyUnchanged: + applied = true e.logf("proxy: unchanged and running — not touched") return nil } - res, err = e.T.Run(ctx, "cat "+q(hp.Compose)+" 2>/dev/null || true") + res, err := e.T.Run(ctx, "cat "+q(hp.Compose)+" 2>/dev/null || true") if err != nil { return err } @@ -129,86 +169,116 @@ func (e *Engine) EnsureProxy(ctx context.Context, deployID string, breakLock boo }) e.ui.Diff(diff) } - if remoteHash != "" && remoteHash != hash { - // config content is never diffed or printed — .env may hold secrets; - // only hashes travel - e.logf("proxy: config changed (%.8s → %.8s)", remoteHash, hash) - } - - if remoteHash != hash || remoteCompose == "" { - // Stage remotely, then swap: the live container bind-mounts ConfigDir, - // so it must never see a half-written dir for the (seconds-long) upload - // window — only for the instant of the rm+mv. Stale files can't linger - // either (upload is additive tar; the swap replaces the whole dir). - stagedDir := hp.Dir + "/.staged-" + e.hostLockToken - defer func() { - cleanupContext, cancel := context.WithTimeout(context.Background(), 5*time.Second) - defer cancel() - _, _ = e.hostMutate(cleanupContext, "rm -rf "+q(stagedDir)) - }() - res, err = e.hostMutate(ctx, "rm -rf "+q(stagedDir)) - if err != nil { - return fmt.Errorf("clear proxy staging: %w", err) - } - if res.ExitCode != 0 { - return fmt.Errorf("clear proxy staging: %s", strings.TrimSpace(res.Stderr)) + if action == proxyConverge || action == proxyReplace { + pending := proxy.Identity{Config: o.Applied.Config, Compose: pendingComposeDigest} + if action == proxyReplace { + pending.Compose = pendingReplaceDigest } - if err := e.T.Upload(ctx, staging, stagedDir); err != nil { + if err := e.commitProxyIdentity(ctx, pending); err != nil { return err } - swap := "rm -rf " + q(hp.ConfigDir) + - " " + q(hp.Dynamic) + - " && mv " + q(stagedDir+"/config") + " " + q(hp.ConfigDir) + - " && mv " + q(stagedDir+"/dynamic") + " " + q(hp.Dynamic) + - " && mv -f " + q(stagedDir+"/compose.yaml") + " " + q(hp.Compose) + - " && chown -R 0:65532 " + q(hp.ConfigDir) + " " + q(hp.Dynamic) + - " && find " + q(hp.ConfigDir) + " " + q(hp.Dynamic) + " -type d -exec chmod 750 {} +" + - " && find " + q(hp.ConfigDir) + " " + q(hp.Dynamic) + " -type f -exec chmod 640 {} +" + - " && rm -rf " + q(stagedDir) - res, err = e.hostMutate(ctx, swap) + composeStaging, err := os.MkdirTemp("", "onebox-proxy-compose") if err != nil { - return fmt.Errorf("swap proxy config: %w", err) + return err } - if res.ExitCode != 0 { - return fmt.Errorf("swap proxy config: %s", strings.TrimSpace(res.Stderr)) + defer os.RemoveAll(composeStaging) + if err := os.WriteFile(filepath.Join(composeStaging, "compose.yaml"), rendered, 0o600); err != nil { + return err + } + staging = composeStaging + } else { + if res, err := e.hostMutate(ctx, "rm -f "+q(hp.Hash)); err != nil { + return err + } else if res.ExitCode != 0 { + return fmt.Errorf("invalidate proxy hash: %s", strings.TrimSpace(res.Stderr)) } } + // Retire the live directories without deleting their contents: Traefik's + // bind mounts keep serving them until the restart succeeds. + stagedDir := hp.Dir + "/.staged-" + e.hostLockToken + res, err = e.hostMutate(ctx, "rm -rf "+q(stagedDir)) + if err != nil { + return fmt.Errorf("clear proxy staging: %w", err) + } + if res.ExitCode != 0 { + return fmt.Errorf("clear proxy staging: %s", strings.TrimSpace(res.Stderr)) + } + if err := e.T.Upload(ctx, staging, stagedDir); err != nil { + return err + } + swap := "mkdir -p " + q(stagedDir+"/retired") + + " && { if [ -d " + q(hp.ConfigDir) + " ]; then mv " + q(hp.ConfigDir) + " " + q(stagedDir+"/retired/config") + "; fi; }" + + " && { if [ -d " + q(hp.Dynamic) + " ]; then mv " + q(hp.Dynamic) + " " + q(stagedDir+"/retired/dynamic") + "; fi; }" + + " && mv " + q(stagedDir+"/config") + " " + q(hp.ConfigDir) + + " && mv " + q(stagedDir+"/dynamic") + " " + q(hp.Dynamic) + + " && mv -f " + q(stagedDir+"/compose.yaml") + " " + q(hp.Compose) + + " && chown -R 0:65532 " + q(hp.ConfigDir) + " " + q(hp.Dynamic) + + " && find " + q(hp.ConfigDir) + " " + q(hp.Dynamic) + " -type d -exec chmod 750 {} +" + + " && find " + q(hp.ConfigDir) + " " + q(hp.Dynamic) + " -type f -exec chmod 640 {} +" + if action == proxyConverge || action == proxyReplace { + swap = "mv -f " + q(stagedDir+"/compose.yaml") + " " + q(hp.Compose) + } + res, err = e.hostMutate(ctx, swap) + if err != nil { + return fmt.Errorf("swap proxy config: %w", err) + } + if res.ExitCode != 0 { + return fmt.Errorf("swap proxy config: %s", strings.TrimSpace(res.Stderr)) + } - // Converge by observation, not by disk state (a crash may have left the - // files new but the container old): `up -d` recreates only when the - // running container diverges from the compose file; if it didn't recreate, - // the change was config-only — restart so the static config reloads. - before := idSet(ids) + before := idSet(o.ProxyIDs) e.logf("proxy: converging") compose := "docker compose -p " + proxy.Project + " -f " + q(hp.Compose) - // The discovery bind mount points at a directory inode. Proxy apply swaps - // that directory atomically, so discovery must be recreated to bind the new - // inode before Traefik is started or restarted against it. - if res, err := e.hostMutate(ctx, compose+" up -d --force-recreate discovery"); err != nil { + // A config swap replaces discovery's bind-mounted directory inode. + forceDiscovery := action == proxyApply || len(o.DiscoveryIDs) == 0 || o.OutputInode == "" + previousInode := "" + if action == proxyConverge || action == proxyReplace { + if err := pullDiscovery(); err != nil { + return err + } + discoveryStopped = true + if res, err := e.hostMutate(ctx, compose+" stop discovery"); err != nil { + return err + } else if res.ExitCode != 0 { + return fmt.Errorf("proxy discovery stop: %s", strings.TrimSpace(res.Stderr)) + } + res, err := e.T.Run(ctx, outputInodeCommand(hp.Dynamic+"/onebox.yml")) + if err != nil { + return err + } + if res.ExitCode != 0 { + return fmt.Errorf("proxy discovery output: %s", strings.TrimSpace(res.Stderr)) + } + previousInode = strings.TrimSpace(res.Stdout) + } + discoveryUp := compose + " up -d discovery" + if forceDiscovery { + discoveryUp = compose + " up -d --force-recreate discovery" + } + if action == proxyApply { + discoveryStopped = true + } + if res, err := e.hostMutate(ctx, discoveryUp); err != nil { return err } else if res.ExitCode != 0 { return fmt.Errorf("proxy discovery up: %s", strings.TrimSpace(res.Stderr)) } - ready := "i=0; while [ $i -lt 30 ]; do test -s " + q(hp.Dynamic+"/onebox.yml") + - " && exit 0; i=$((i+1)); sleep 1; done; exit 1" - if res, err := e.T.Run(ctx, ready); err != nil { + if err := e.waitForPublication(ctx, previousInode); err != nil { return err - } else if res.ExitCode != 0 { - return fmt.Errorf("proxy discovery did not publish routing state; inspect `docker logs %s`", proxy.DiscoveryContainerName) } if res, err := e.hostMutate(ctx, compose+" up -d proxy"); err != nil { return err } else if res.ExitCode != 0 { return fmt.Errorf("proxy up: %s", strings.TrimSpace(res.Stderr)) } - ids, err = e.proxyContainerIDs(ctx) + ids, err := e.proxyContainerIDs(ctx) if err != nil { return err } if len(ids) == 0 { return fmt.Errorf("proxy: no container after converge") } - if before[ids[0]] { + if action == proxyApply && before[ids[0]] { e.logf("proxy: container not recreated — restarting %s to load the new config", proxy.ContainerName) if res, err := e.hostMutate(ctx, "docker restart "+proxy.ContainerName); err != nil { return err @@ -219,16 +289,36 @@ func (e *Engine) EnsureProxy(ctx context.Context, deployID string, breakLock boo if err := e.waitHealth(ctx, ids[0], "healthy", 180*time.Second, 5*time.Second); err != nil { return fmt.Errorf("proxy never became healthy (the Traefik static configuration must enable ping: {}): %w", err) } - // the applied-state marker — written ONLY after health confirms, so an - // interrupted converge is retried, never mistaken for "unchanged" - res, err = e.hostMutate(ctx, "echo "+q(hash)+" > "+q(hp.Hash)) - if err != nil { - return fmt.Errorf("write proxy hash: %w", err) + if err := e.commitProxyIdentity(ctx, id); err != nil { + return err } - if res.ExitCode != 0 { + applied = true + e.logf("proxy: healthy at config %s compose %s", shortDigest(id.Config), shortDigest(id.Compose)) + return nil +} + +func (e *Engine) waitForPublication(ctx context.Context, previousInode string) error { + path := proxy.HostPaths(e.names()).Dynamic + "/onebox.yml" + ready := "i=0; while [ $i -lt 30 ]; do inode=$(" + outputInodeCommand(path) + + ") && [ -n \"$inode\" ] && [ \"$inode\" != " + q(previousInode) + + " ] && exit 0; i=$((i+1)); sleep 1; done; exit 1" + if res, err := e.T.Run(ctx, ready); err != nil { + return err + } else if res.ExitCode != 0 { + return fmt.Errorf("proxy discovery did not publish routing state; inspect `docker logs %s`", proxy.DiscoveryContainerName) + } + return nil +} + +func (e *Engine) commitProxyIdentity(ctx context.Context, id proxy.Identity) error { + path := proxy.HostPaths(e.names()).Hash + command := "printf '%s\\n' " + q(id.String()) + " > " + q(path+".tmp") + + " && mv -f " + q(path+".tmp") + " " + q(path) + if res, err := e.hostMutate(ctx, command); err != nil { + return fmt.Errorf("write proxy hash: %w", err) + } else if res.ExitCode != 0 { return fmt.Errorf("write proxy hash: %s", strings.TrimSpace(res.Stderr)) } - e.logf("proxy: healthy at config %.8s", hash) return nil } diff --git a/internal/engine/proxy_converge.go b/internal/engine/proxy_converge.go new file mode 100644 index 00000000..c94fcbf4 --- /dev/null +++ b/internal/engine/proxy_converge.go @@ -0,0 +1,181 @@ +package engine + +import ( + "context" + "fmt" + "os" + "path/filepath" + "reflect" + "strings" + + "gopkg.in/yaml.v3" + + "github.com/labstack/onebox/internal/proxy" +) + +type proxyObservation struct { + Applied proxy.Identity + AppliedKnown bool + ProxyIDs, DiscoveryIDs []string + OutputInode string + TraefikChanged bool +} + +type proxyAction int + +const ( + proxyUnchanged proxyAction = iota + proxyConverge + proxyReplace + proxyApply +) + +func decideProxy(o proxyObservation, staged proxy.Identity) proxyAction { + if !o.AppliedKnown || o.Applied.Config != staged.Config { + return proxyApply + } + if o.TraefikChanged || o.Applied.Compose != staged.Compose || len(o.ProxyIDs) == 0 || len(o.DiscoveryIDs) == 0 || o.OutputInode == "" { + if o.TraefikChanged { + return proxyReplace + } + return proxyConverge + } + return proxyUnchanged +} + +func (a proxyAction) String() string { + switch a { + case proxyUnchanged: + return "unchanged" + case proxyConverge: + return "update changed containers only, no restart" + case proxyReplace: + return "recreate Traefik (its container definition changed)" + case proxyApply: + return "restart Traefik (configuration changed)" + default: + return "unknown" + } +} + +func (e *Engine) stageProxy(dir string) (proxy.Identity, error) { + // Empty stays empty: the project directory is not Traefik's config. + localCfg := e.Spec.Proxy.Config + if localCfg != "" && !filepath.IsAbs(localCfg) { + localCfg = filepath.Join(e.Opts.LocalDir, localCfg) + } + discoveryImage := proxy.DiscoveryImage(e.Opts.Runner.Version) + return proxy.StageForAppManaged(localCfg, dir, e.Spec.Proxy.Image, discoveryImage, + e.Spec.Name, e.Spec.Proxy.Network, e.Spec.Proxy.Entrypoints, e.Spec.HasExactTerminatingTLS(), + e.Spec.HasWildcardTerminatingTLS(), e.Spec.Proxy.DNSChallenge) +} + +func traefikDefinitionChanged(live, rendered []byte) bool { + if len(strings.TrimSpace(string(live))) == 0 { + return true + } + var before, after struct { + Services map[string]any + Networks any + } + if yaml.Unmarshal(live, &before) != nil || yaml.Unmarshal(rendered, &after) != nil { + return true + } + return !reflect.DeepEqual(before.Services["proxy"], after.Services["proxy"]) || + !reflect.DeepEqual(before.Networks, after.Networks) +} + +func (e *Engine) observeProxy(ctx context.Context, rendered []byte) (proxyObservation, error) { + var o proxyObservation + hp := proxy.HostPaths(e.names()) + marker, err := e.T.Run(ctx, "cat "+q(hp.Hash)) + if err != nil { + return o, err + } + if marker.ExitCode == 0 { + o.Applied, o.AppliedKnown = proxy.ParseIdentity(marker.Stdout) + } + compose, err := e.T.Run(ctx, "cat "+q(hp.Compose)) + if err != nil { + return o, err + } + o.TraefikChanged = o.Applied.Compose == pendingReplaceDigest || compose.ExitCode != 0 || traefikDefinitionChanged([]byte(compose.Stdout), rendered) + o.ProxyIDs, err = e.proxyContainerIDs(ctx) + if err != nil { + return o, err + } + if len(o.ProxyIDs) > 0 { + var planned struct { + Services map[string]struct { + Image string + } + } + if err := yaml.Unmarshal(rendered, &planned); err != nil { + return o, fmt.Errorf("read rendered proxy image: %w", err) + } + image := planned.Services["proxy"].Image + if image == "" { + return o, fmt.Errorf("read rendered proxy image: services.proxy.image is empty") + } + running, err := e.T.Run(ctx, "docker inspect -f '{{.Image}}' "+q(o.ProxyIDs[0])) + if err != nil { + return o, err + } + if running.ExitCode != 0 { + return o, fmt.Errorf("read proxy image: exit %d: %s", running.ExitCode, strings.TrimSpace(running.Stderr)) + } + local, err := e.T.Run(ctx, "docker image inspect -f '{{.Id}}' "+q(image)) + if err != nil { + return o, err + } + o.TraefikChanged = o.TraefikChanged || local.ExitCode != 0 || strings.TrimSpace(running.Stdout) != strings.TrimSpace(local.Stdout) + } + o.DiscoveryIDs, err = e.discoveryContainerIDs(ctx) + if err != nil { + return o, err + } + output, err := e.T.Run(ctx, outputInodeCommand(hp.Dynamic+"/onebox.yml")) + if err != nil { + return o, err + } + if output.ExitCode != 0 { + return o, fmt.Errorf("read proxy output inode: exit %d: %s", output.ExitCode, strings.TrimSpace(output.Stderr)) + } + o.OutputInode = strings.TrimSpace(output.Stdout) + return o, nil +} + +func outputInodeCommand(path string) string { + return "if [ -s " + q(path) + " ]; then stat -c %i " + q(path) + "; fi" +} + +func proxyPlanLine(o proxyObservation, a proxyAction) string { + if len(o.ProxyIDs) == 0 { + return "start Traefik" + } + return a.String() +} + +func (e *Engine) PlanProxy(ctx context.Context) (string, error) { + if !e.Spec.Proxy.Managed { + return "", nil + } + staging, err := os.MkdirTemp("", "onebox-proxy") + if err != nil { + return "", err + } + defer os.RemoveAll(staging) + staged, err := e.stageProxy(staging) + if err != nil { + return "", err + } + rendered, err := os.ReadFile(filepath.Join(staging, "compose.yaml")) + if err != nil { + return "", err + } + observed, err := e.observeProxy(ctx, rendered) + if err != nil { + return "", err + } + return proxyPlanLine(observed, decideProxy(observed, staged)), nil +} diff --git a/internal/engine/proxy_converge_test.go b/internal/engine/proxy_converge_test.go new file mode 100644 index 00000000..22a1dcd7 --- /dev/null +++ b/internal/engine/proxy_converge_test.go @@ -0,0 +1,979 @@ +package engine + +import ( + "context" + "errors" + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/labstack/onebox/internal/proxy" + "github.com/labstack/onebox/internal/transport" +) + +func testProxyCompose(t *testing.T, e *Engine) []byte { + t.Helper() + dir := t.TempDir() + if _, err := e.stageProxy(dir); err != nil { + t.Fatal(err) + } + rendered, err := os.ReadFile(filepath.Join(dir, "compose.yaml")) + if err != nil { + t.Fatal(err) + } + return rendered +} + +func TestEnsureProxyRetryWaitsForHealthBeforeCommit(t *testing.T) { + for _, digest := range []string{pendingComposeDigest, pendingReplaceDigest} { + t.Run(digest, func(t *testing.T) { + f := &transport.Fake{} + e, staged, _ := proxyFixture(t, f) + pending := proxy.Identity{Config: staged.Config, Compose: digest} + live := string(testProxyCompose(t, e)) + clock := &virtualClock{} + e.Opts.Now, e.Opts.Sleep = clock.now, clock.sleep + e.Opts.Wait = func(ctx context.Context, d time.Duration) error { clock.sleep(d); return ctx.Err() } + ps := proxyPS(f, true) + healthReads := 0 + f.Dynamic = func(cmd string) (transport.Result, bool) { + switch { + case cmd == "cat '/var/lib/onebox/_host/proxy/config.hash'": + return transport.Result{Stdout: pending.String()}, true + case strings.HasPrefix(cmd, "cat '/var/lib/onebox/_host/proxy/compose.yaml'"): + return transport.Result{Stdout: live}, true + case strings.Contains(cmd, "Health.Status") && strings.HasSuffix(cmd, "PX1"): + healthReads++ + return transport.Result{Stdout: "unhealthy"}, true + } + return ps(cmd) + } + err := e.EnsureProxy(context.Background(), "unhealthy-retry", false) + seq := strings.Join(f.Commands, "\n") + if err == nil || !strings.Contains(err.Error(), "proxy never became healthy") || healthReads == 0 { + t.Fatalf("retry must wait for unchanged PX1 to become healthy: %v\n%s", err, seq) + } + if strings.Contains(seq, testProxyIdentityWrite(staged)) || strings.Contains(seq, "docker restart") { + t.Fatalf("unhealthy retry must keep its pending identity without restarting Traefik:\n%s", seq) + } + }) + } +} + +func TestEnsureProxyProceedsWhenPlannedActionMatches(t *testing.T) { + f := &transport.Fake{} + e, staged, _ := proxyFixture(t, f) + pending := proxy.Identity{Config: staged.Config, Compose: pendingComposeDigest} + live := string(testProxyCompose(t, e)) + e.Opts.ExpectedProxyAction = proxyConverge.String() + clock := &virtualClock{} + e.Opts.Now, e.Opts.Sleep = clock.now, clock.sleep + e.Opts.Wait = func(ctx context.Context, d time.Duration) error { clock.sleep(d); return ctx.Err() } + ps := proxyPS(f, true) + healthReads := 0 + f.Dynamic = func(cmd string) (transport.Result, bool) { + switch { + case cmd == "cat '/var/lib/onebox/_host/proxy/config.hash'": + return transport.Result{Stdout: pending.String()}, true + case strings.HasPrefix(cmd, "cat '/var/lib/onebox/_host/proxy/compose.yaml'"): + return transport.Result{Stdout: live}, true + case strings.Contains(cmd, "Health.Status") && strings.HasSuffix(cmd, "PX1"): + healthReads++ + if healthReads == 1 { + return transport.Result{Stdout: "starting"}, true + } + return transport.Result{Stdout: "healthy"}, true + } + return ps(cmd) + } + if err := e.EnsureProxy(context.Background(), "matching-proxy-plan", false); err != nil { + t.Fatal(err) + } + seq := strings.Join(f.Commands, "\n") + health := strings.LastIndex(seq, "docker inspect -f '{{if .State.Health}}") + commit := strings.Index(seq, testProxyIdentityWrite(staged)) + if healthReads != 2 || health < 0 || commit < health { + t.Fatalf("matching plan must publish and wait for health before committing:\n%s", seq) + } + if strings.Contains(seq, "docker restart") || strings.Contains(seq, "/retired/") { + t.Fatalf("matching converge must keep Traefik and its configuration:\n%s", seq) + } +} + +type proxyRunObserver struct { + *transport.Fake + inspect func(context.Context, string) +} + +func (f *proxyRunObserver) Run(ctx context.Context, cmd string) (transport.Result, error) { + f.inspect(ctx, cmd) + return f.Fake.Run(ctx, cmd) +} + +func TestEnsureProxyFallbackSkipsComposeWhenStartSucceeds(t *testing.T) { + f := &transport.Fake{} + e, staged, out := proxyFixture(t, f) + pending := proxy.Identity{Config: staged.Config, Compose: pendingComposeDigest} + live := string(testProxyCompose(t, e)) + ps := proxyPS(f, true) + f.Dynamic = func(cmd string) (transport.Result, bool) { + switch { + case cmd == "cat '/var/lib/onebox/_host/proxy/config.hash'": + return transport.Result{Stdout: pending.String()}, true + case strings.HasPrefix(cmd, "cat '/var/lib/onebox/_host/proxy/compose.yaml'"): + return transport.Result{Stdout: live}, true + case strings.Contains(cmd, " up -d discovery"): + return transport.Result{ExitCode: 1, Stderr: "discovery up failed"}, true + } + return ps(cmd) + } + started := false + e.T = &proxyRunObserver{Fake: f, inspect: func(ctx context.Context, cmd string) { + if strings.Contains(cmd, "docker start onebox-discovery") { + started = true + deadline, ok := ctx.Deadline() + if remaining := time.Until(deadline); !ok || remaining <= 25*time.Second || remaining > 30*time.Second { + t.Errorf("discovery fallback needs its own 30s context: remaining=%v", remaining) + } + } + }} + err := e.EnsureProxy(context.Background(), "fallback-start", false) + seq := strings.Join(f.Commands, "\n") + if err == nil || !strings.Contains(err.Error(), "discovery up failed") || !started || strings.Count(seq, " up -d discovery") != 1 { + t.Fatalf("successful start must skip the Compose fallback: %v\n%s", err, seq) + } + if strings.Contains(out.String(), "failed converge") || !strings.Contains(out.String(), "discovery restarted after failure") { + t.Fatalf("fallback result must use path-neutral log text:\n%s", out) + } +} + +func TestEnsureProxyApplyPullsBeforeInvalidatingMarker(t *testing.T) { + for _, pullFails := range []bool{false, true} { + t.Run(fmt.Sprint(pullFails), func(t *testing.T) { + f := &transport.Fake{} + e, applied, _ := proxyFixture(t, f) + applied.Config = "sha256:" + strings.Repeat("a", 64) + live := string(testProxyCompose(t, e)) + image := q(proxy.DiscoveryImage(e.Opts.Runner.Version)) + ensureImage := "docker image inspect " + image + " >/dev/null 2>&1 || docker pull " + image + ps := proxyPS(f, true) + f.Dynamic = func(cmd string) (transport.Result, bool) { + switch { + case cmd == "cat '/var/lib/onebox/_host/proxy/config.hash'": + return transport.Result{Stdout: applied.String()}, true + case strings.HasPrefix(cmd, "cat '/var/lib/onebox/_host/proxy/compose.yaml'"): + return transport.Result{Stdout: live}, true + case strings.Contains(cmd, ensureImage): + if pullFails { + return transport.Result{ExitCode: 1, Stderr: "cannot pull discovery"}, true + } + return transport.Result{}, true + } + return ps(cmd) + } + err := e.EnsureProxy(context.Background(), "early-apply-pull", false) + seq := strings.Join(f.Commands, "\n") + pull := strings.Index(seq, ensureImage) + invalidate := strings.Index(seq, "rm -f '/var/lib/onebox/_host/proxy/config.hash'") + cleanup := strings.Index(seq, "-name '.staged-*'") + prepare := strings.Index(seq, "mkdir -p '/var/lib/onebox/_host/proxy/acme'") + if pull < 0 || pullFails && (err == nil || !strings.Contains(err.Error(), "cannot pull discovery")) || !pullFails && err != nil { + t.Fatalf("apply discovery pull: %v\n%s", err, seq) + } + if pullFails { + if invalidate >= 0 || cleanup >= 0 || prepare >= 0 || len(f.Uploads) != 0 { + t.Fatalf("failed pull must preserve the marker and directories:\n%s", seq) + } + } else if invalidate < pull || cleanup < pull || prepare < pull { + t.Fatalf("apply must pull immediately after drift validation:\n%s", seq) + } + }) + } +} + +func TestEnsureProxyApplyRestoresDiscoveryWhenRecreateFails(t *testing.T) { + for _, fallbackFails := range []bool{false, true} { + t.Run(fmt.Sprint(fallbackFails), func(t *testing.T) { + f := &transport.Fake{} + e, _, out := proxyFixture(t, f) + ps := proxyPS(f, true) + f.Dynamic = func(cmd string) (transport.Result, bool) { + switch { + case strings.Contains(cmd, "up -d --force-recreate discovery"): + return transport.Result{ExitCode: 1, Stderr: "recreate failed"}, true + case strings.Contains(cmd, "docker start onebox-discovery"): + return transport.Result{ExitCode: 1, Stderr: "container no longer exists"}, true + case strings.Contains(cmd, "up -d discovery"): + if fallbackFails { + return transport.Result{ExitCode: 1, Stderr: "fallback up failed"}, true + } + return transport.Result{}, true + } + return ps(cmd) + } + err := e.EnsureProxy(context.Background(), "failed-apply", false) + seq := strings.Join(f.Commands, "\n") + recreate, start, fallback := strings.Index(seq, "up -d --force-recreate discovery"), strings.Index(seq, "docker start onebox-discovery"), strings.LastIndex(seq, "up -d discovery") + if err == nil || !strings.Contains(err.Error(), "recreate failed") || recreate < 0 || start < recreate || fallback < start { + t.Fatalf("failed recreate must start, then recreate a missing discovery container: %v\n%s", err, seq) + } + if fallbackFails && (!strings.Contains(out.String(), "fallback up failed") || strings.Contains(err.Error(), "fallback up failed")) { + t.Fatalf("fallback failure must be logged without replacing the apply error: %v\n%s", err, out) + } + }) + } +} + +func TestEnsureProxyApplyPullsDiscoveryBeforeRecreate(t *testing.T) { + for _, pullFails := range []bool{false, true} { + t.Run(fmt.Sprint(pullFails), func(t *testing.T) { + f := &transport.Fake{} + e, _, _ := proxyFixture(t, f) + e.Opts.Runner.Version = "v1.2.3" + image := q(proxy.DiscoveryImage(e.Opts.Runner.Version)) + ensureImage := "docker image inspect " + image + " >/dev/null 2>&1 || docker pull " + image + ps := proxyPS(f, true) + f.Dynamic = func(cmd string) (transport.Result, bool) { + if strings.Contains(cmd, ensureImage) { + if pullFails { + return transport.Result{ExitCode: 1, Stderr: "discovery pull failed"}, true + } + return transport.Result{}, true + } + return ps(cmd) + } + err := e.EnsureProxy(context.Background(), "apply-pull", false) + seq := strings.Join(f.Commands, "\n") + pull, recreate := strings.Index(seq, ensureImage), strings.Index(seq, "up -d --force-recreate discovery") + if pull < 0 { + t.Fatalf("apply must make the discovery image available:\n%s", seq) + } + if pullFails { + if err == nil || !strings.Contains(err.Error(), "discovery pull failed") || recreate >= 0 { + t.Fatalf("failed pull must precede any recreate: %v\n%s", err, seq) + } + } else if err != nil || recreate < pull { + t.Fatalf("apply must pull before recreating discovery: %v\n%s", err, seq) + } + }) + } +} + +func TestEnsureProxyRefusesPlannedActionDrift(t *testing.T) { + for _, fresh := range []bool{false, true} { + t.Run(fmt.Sprintf("fresh=%v", fresh), func(t *testing.T) { + f := &transport.Fake{} + e, applied, _ := proxyFixture(t, f) + e.Opts.ExpectedProxyAction = "unchanged" + rendered := string(testProxyCompose(t, e)) + applied.Config = "sha256:" + strings.Repeat("a", 64) + ps := proxyPS(f, !fresh) + observed := false + f.Dynamic = func(cmd string) (transport.Result, bool) { + switch cmd { + case "cat '/var/lib/onebox/_host/proxy/config.hash'": + if fresh { + return transport.Result{ExitCode: 1}, true + } + return transport.Result{Stdout: applied.String()}, true + case "cat '/var/lib/onebox/_host/proxy/compose.yaml'": + if fresh { + return transport.Result{ExitCode: 1}, true + } + return transport.Result{Stdout: rendered}, true + case outputInodeCommand("/var/lib/onebox/_host/proxy/dynamic/onebox.yml"): + observed = true + if fresh { + return transport.Result{}, true + } + return transport.Result{Stdout: "12"}, true + } + return ps(cmd) + } + err := e.EnsureProxy(context.Background(), "proxy-drift", false) + current := "restart Traefik (configuration changed)" + if fresh { + current = "start Traefik" + } + if err == nil || !observed || !strings.Contains(err.Error(), "unchanged") || !strings.Contains(err.Error(), current) || !strings.Contains(err.Error(), "re-plan") { + t.Fatalf("proxy action drift must name both plan lines and re-plan guidance: %v", err) + } + seq := strings.Join(f.Commands, "\n") + for _, forbidden := range []string{"/proxy/acme", "-name '.staged-*'", "config.hash.tmp", "rm -f '/var/lib/onebox/_host/proxy/config.hash'", "docker pull", "up -d", " stop discovery"} { + if strings.Contains(seq, forbidden) { + t.Fatalf("refused plan mutated the host, found %s:\n%s", forbidden, seq) + } + } + if len(f.Uploads) != 0 { + t.Fatalf("refused plan uploaded proxy configuration: %v", f.Uploads) + } + }) + } +} + +func TestObserveProxyFlagsRepulledTraefikImage(t *testing.T) { + for _, configured := range []string{"", "registry.example.com/traefik:custom"} { + for _, state := range []string{"equal", "different", "missing", "rendered"} { + t.Run(configured+"/"+state, func(t *testing.T) { + f := &transport.Fake{} + e, id, _ := proxyFixture(t, f) + e.Spec.Proxy.Image = configured + rendered := testProxyCompose(t, e) + id.Compose = HashBytes(rendered) + image := configured + if image == "" { + image = proxy.DefaultImage + } + if state == "rendered" { + e.Spec.Proxy.Image = "registry.example.com/not-the-rendered-image:changed" + } + ps := proxyPS(f, true) + inspectedContainer, inspectedImage := false, false + f.Dynamic = func(cmd string) (transport.Result, bool) { + switch { + case cmd == "cat '/var/lib/onebox/_host/proxy/config.hash'": + return transport.Result{Stdout: id.String()}, true + case cmd == "cat '/var/lib/onebox/_host/proxy/compose.yaml'": + return transport.Result{Stdout: string(rendered)}, true + case strings.HasPrefix(cmd, "docker inspect -f '{{.Image}}'"): + inspectedContainer = true + return transport.Result{Stdout: "sha256:running\n"}, true + case strings.HasPrefix(cmd, "docker image inspect -f '{{.Id}}'"): + inspectedImage = true + if !strings.HasSuffix(cmd, q(image)) { + t.Fatalf("inspected the wrong configured image: %s", cmd) + } + if state == "missing" { + return transport.Result{ExitCode: 1}, true + } + digest := "sha256:running\n" + if state == "different" { + digest = "sha256:repulled\n" + } + return transport.Result{Stdout: digest}, true + } + return ps(cmd) + } + o, err := e.observeProxy(context.Background(), rendered) + changed := state == "different" || state == "missing" + if err != nil || !inspectedContainer || !inspectedImage || o.TraefikChanged != changed { + t.Fatalf("repulled image observation = %+v, %v", o, err) + } + want := proxyUnchanged + if changed { + want = proxyReplace + } + if got := decideProxy(o, id); got != want { + t.Fatalf("image drift action = %v, want %v", got, want) + } + }) + } + } +} + +func runProxyStagingCleanup(t *testing.T, command, dir string) { + t.Helper() + start := strings.Index(command, "find '/var/lib/onebox/_host/proxy'") + if start < 0 { + t.Fatalf("no staging cleanup: %s", command) + } + script, _, ok := strings.Cut(command[start:], "; else echo onebox-host-fenced") + if !ok { + t.Fatalf("staging cleanup must hold the host lock: %s", command) + } + script = strings.ReplaceAll(script, q("/var/lib/onebox/_host/proxy"), q(dir)) + if output, err := exec.CommandContext(t.Context(), "sh", "-c", script).CombinedOutput(); err != nil { + t.Fatalf("staging cleanup: %v\n%s", err, output) + } +} + +func TestEnsureProxyRemovesFailedUploadsButKeepsRetired(t *testing.T) { + dir := t.TempDir() + for _, name := range []string{".staged-upload", ".staged-config/retired", "unrelated"} { + if err := os.MkdirAll(filepath.Join(dir, name), 0o755); err != nil { + t.Fatal(err) + } + } + f := &transport.Fake{} + e, _, _ := proxyFixture(t, f) + ps := proxyPS(f, true) + cleaned := false + f.Dynamic = func(cmd string) (transport.Result, bool) { + if strings.Contains(cmd, "-name '.staged-*'") { + runProxyStagingCleanup(t, cmd, dir) + cleaned = true + return transport.Result{}, true + } + if strings.Contains(cmd, "mkdir -p '/var/lib/onebox/_host/proxy/acme'") { + return transport.Result{ExitCode: 1, Stderr: "cannot prepare directories"}, true + } + return ps(cmd) + } + if err := e.EnsureProxy(context.Background(), "clean-upload", false); err == nil { + t.Fatal("failed directory preparation must error") + } + if _, err := os.Stat(filepath.Join(dir, ".staged-upload")); !cleaned || !os.IsNotExist(err) { + t.Fatalf("failed upload staging survived cleanup: cleaned=%v err=%v", cleaned, err) + } + for _, name := range []string{".staged-config/retired", "unrelated"} { + if _, err := os.Stat(filepath.Join(dir, name)); err != nil { + t.Fatalf("startup cleanup removed protected %s: %v", name, err) + } + } +} + +func TestEnsureProxyFailedReplaceIsPlannedAsReplace(t *testing.T) { + f := &transport.Fake{} + e, staged, _ := proxyFixture(t, f) + rendered := string(testProxyCompose(t, e)) + live := strings.Replace(rendered, "image: ", "image: old-", 1) + applied := staged + applied.Compose = "sha256:" + strings.Repeat("a", 64) + pending := proxy.Identity{Config: applied.Config, Compose: "sha256:" + strings.Repeat("0", 63) + "1"} + oldPending := proxy.Identity{Config: applied.Config, Compose: pendingComposeDigest} + marker := applied.String() + fail := true + ps := proxyPS(f, true) + f.Dynamic = func(cmd string) (transport.Result, bool) { + switch { + case cmd == "cat '/var/lib/onebox/_host/proxy/config.hash'": + return transport.Result{Stdout: marker}, true + case strings.HasPrefix(cmd, "cat '/var/lib/onebox/_host/proxy/compose.yaml'"): + return transport.Result{Stdout: live}, true + case strings.Contains(cmd, "/compose.yaml' '/var/lib/onebox/_host/proxy/compose.yaml'"): + live = rendered + return transport.Result{}, true + case strings.Contains(cmd, "up -d discovery") && fail: + return transport.Result{ExitCode: 1, Stderr: "cannot start discovery"}, true + } + for _, id := range []proxy.Identity{pending, oldPending, staged} { + if strings.Contains(cmd, testProxyIdentityWrite(id)) { + marker = id.String() + return transport.Result{}, true + } + } + return ps(cmd) + } + if err := e.EnsureProxy(context.Background(), "failed-replace", false); err == nil { + t.Fatal("failed discovery up must error") + } + if marker != pending.String() || live != rendered || strings.Contains(strings.Join(f.Commands, "\n"), testProxyIdentityWrite(staged)) { + t.Errorf("failed replacement must leave the new Compose and only its pending marker: %q", marker) + } + line, err := e.PlanProxy(context.Background()) + if err != nil || line != "recreate Traefik (its container definition changed)" { + t.Fatalf("failed replacement plan = %q, %v", line, err) + } + beforeRetry := len(f.Commands) + fail = false + if err := e.EnsureProxy(context.Background(), "retry-replace", false); err != nil { + t.Fatal(err) + } + retry := strings.Join(f.Commands[beforeRetry:], "\n") + if !strings.Contains(retry, testProxyIdentityWrite(pending)) || marker != staged.String() || + strings.Contains(retry, "docker restart") || strings.Contains(retry, "/retired/") { + t.Fatalf("retry must remain a replacement before committing the final identity:\n%s", retry) + } +} + +func TestEnsureProxyPullsDiscoveryBeforeStopping(t *testing.T) { + for _, replace := range []bool{false, true} { + for _, pullFails := range []bool{false, true} { + t.Run(fmt.Sprintf("replace=%v/pullFails=%v", replace, pullFails), func(t *testing.T) { + f := &transport.Fake{} + e, applied, _ := proxyFixture(t, f) + e.Opts.Runner.Version = "v1.2.3" + live := string(testProxyCompose(t, e)) + if replace { + live = strings.Replace(live, "image: ", "image: old-", 1) + } + applied.Compose = "sha256:" + strings.Repeat("a", 64) + image := proxy.DiscoveryImage(e.Opts.Runner.Version) + ensureImage := "docker image inspect " + q(image) + " >/dev/null 2>&1 || docker pull " + q(image) + ps := proxyPS(f, true) + f.Dynamic = func(cmd string) (transport.Result, bool) { + switch { + case cmd == "cat '/var/lib/onebox/_host/proxy/config.hash'": + return transport.Result{Stdout: applied.String()}, true + case strings.HasPrefix(cmd, "cat '/var/lib/onebox/_host/proxy/compose.yaml'"): + return transport.Result{Stdout: live}, true + case strings.Contains(cmd, ensureImage): + if pullFails { + return transport.Result{ExitCode: 1, Stderr: "cannot pull discovery"}, true + } + return transport.Result{}, true // missing image was pulled + } + return ps(cmd) + } + err := e.EnsureProxy(context.Background(), "pull-discovery", false) + seq := strings.Join(f.Commands, "\n") + pull, stop := strings.Index(seq, ensureImage), strings.Index(seq, " stop discovery") + if pull < 0 { + t.Fatalf("discovery image must be available before stopping:\n%s", seq) + } + if pullFails { + if err == nil || !strings.Contains(err.Error(), "cannot pull discovery") || stop >= 0 || strings.Contains(seq, "up -d discovery") || strings.Contains(seq, "docker start onebox-discovery") { + t.Fatalf("failed pull must return before stopping discovery: %v\n%s", err, seq) + } + } else if err != nil || stop < pull || strings.Contains(seq, "docker start onebox-discovery") { + t.Fatalf("pull must precede stop without a fallback start on success: %v\n%s", err, seq) + } + }) + } + } +} + +func TestEnsureProxyRestartsDiscoveryAfterFailedUp(t *testing.T) { + for _, step := range []string{"up", "transport", "inode", "publication", "journal", "start fails"} { + t.Run(step, func(t *testing.T) { + f := &transport.Fake{} + e, applied, out := proxyFixture(t, f) + live := string(testProxyCompose(t, e)) + applied.Compose = "sha256:" + strings.Repeat("a", 64) + ps := proxyPS(f, true) + stopped, startAttempted := false, false + transportErr := errors.New("discovery transport failed") + if step == "transport" { + f.Err = func(cmd string) error { + if strings.Contains(cmd, "up -d discovery") { + return transportErr + } + return nil + } + } + f.Dynamic = func(cmd string) (transport.Result, bool) { + switch { + case cmd == "cat '/var/lib/onebox/_host/proxy/config.hash'": + return transport.Result{Stdout: applied.String()}, true + case strings.HasPrefix(cmd, "cat '/var/lib/onebox/_host/proxy/compose.yaml'"): + return transport.Result{Stdout: live}, true + case strings.Contains(cmd, " stop discovery"): + stopped = true + return transport.Result{}, true + case strings.Contains(cmd, "docker start onebox-discovery"): + startAttempted = true + if step == "start fails" { + return transport.Result{ExitCode: 1, Stderr: "cannot restart discovery"}, true + } + return transport.Result{}, true + case strings.Contains(cmd, "up -d discovery") && (step == "up" || step == "start fails"): + return transport.Result{ExitCode: 1, Stderr: "cannot start discovery"}, true + case cmd == outputInodeCommand("/var/lib/onebox/_host/proxy/dynamic/onebox.yml") && stopped && step == "inode": + return transport.Result{ExitCode: 1, Stderr: "cannot read inode"}, true + case strings.HasPrefix(cmd, "i=0;") && step == "publication": + return transport.Result{ExitCode: 1}, true + case strings.Contains(cmd, "\"phase\":\"proxy-apply\",\"event\":\"finish\"") && step == "journal": + return transport.Result{ExitCode: 1, Stderr: "cannot finish journal"}, true + } + return ps(cmd) + } + err := e.EnsureProxy(context.Background(), "restart-discovery", false) + if err == nil || !stopped || !startAttempted { + t.Fatalf("error after stop must restart discovery before returning: %v\n%s", err, strings.Join(f.Commands, "\n")) + } + if step == "transport" && !errors.Is(err, transportErr) { + t.Fatalf("lost original transport error: %v", err) + } + if step == "start fails" && (!strings.Contains(out.String(), "cannot restart discovery") || strings.Contains(err.Error(), "cannot restart discovery") || !strings.Contains(err.Error(), "cannot start discovery")) { + t.Fatalf("fallback failure must be logged without replacing the original error: %v\n%s", err, out) + } + }) + } +} + +func TestEnsureProxyUnchangedCleansStaleStaging(t *testing.T) { + f := &transport.Fake{} + e, id, _ := proxyFixture(t, f) + rendered := string(testProxyCompose(t, e)) + ps := proxyPS(f, true) + f.Dynamic = func(cmd string) (transport.Result, bool) { + switch cmd { + case "cat '/var/lib/onebox/_host/proxy/config.hash'": + return transport.Result{Stdout: id.String()}, true + case "cat '/var/lib/onebox/_host/proxy/compose.yaml'": + return transport.Result{Stdout: rendered}, true + } + return ps(cmd) + } + if err := e.EnsureProxy(context.Background(), "unchanged-cleanup", false); err != nil { + t.Fatal(err) + } + seq := strings.Join(f.Commands, "\n") + if !strings.Contains(seq, "-name '.staged-*' -exec rm -rf -- {} +") { + t.Fatalf("unchanged proxy must clean stale staging:\n%s", seq) + } + if len(f.Uploads) != 0 || strings.Contains(seq, "up -d") || strings.Contains(seq, "docker restart") { + t.Fatalf("unchanged proxy must only clean stale staging:\n%s", seq) + } +} + +func TestTraefikDefinitionChanged(t *testing.T) { + live := "services:\n proxy:\n image: traefik:v3.6\n ports: [80:80]\n discovery:\n image: discovery:old\nnetworks:\n proxy:\n external: true\n" + for _, tt := range []struct { + name string + live string + rendered string + want bool + }{ + {"same", live, live, false}, + {"discovery image", live, strings.Replace(live, "discovery:old", "discovery:new", 1), false}, + {"traefik image", live, strings.Replace(live, "traefik:v3.6", "traefik:v3.7", 1), true}, + {"ports", live, strings.Replace(live, "80:80", "8080:80", 1), true}, + {"networks", live, strings.Replace(live, "external: true", "external: false", 1), true}, + {"formatting", live, "# compose\n" + strings.Replace(live, "ports: [80:80]", "ports:\n - 80:80", 1), false}, + {"invalid live", "services: [", live, true}, + {"invalid rendered", live, "services: [", true}, + {"empty live", "", live, true}, + {"blank live", " \n", live, true}, + } { + t.Run(tt.name, func(t *testing.T) { + if got := traefikDefinitionChanged([]byte(tt.live), []byte(tt.rendered)); got != tt.want { + t.Fatalf("traefikDefinitionChanged = %v, want %v", got, tt.want) + } + }) + } +} + +func TestDecideProxyNamesTraefikReplacement(t *testing.T) { + digestA := "sha256:" + strings.Repeat("a", 64) + digestB := "sha256:" + strings.Repeat("b", 64) + staged := proxy.Identity{Config: digestA, Compose: digestB} + o := proxyObservation{ + Applied: proxy.Identity{Config: digestA, Compose: digestA}, AppliedKnown: true, + ProxyIDs: []string{"PX1"}, DiscoveryIDs: []string{"D1"}, OutputInode: "12", TraefikChanged: true, + } + if got := decideProxy(o, staged); got != proxyReplace { + t.Fatalf("decideProxy = %v, want %v", got, proxyReplace) + } + if got := proxyReplace.String(); got != "recreate Traefik (its container definition changed)" { + t.Fatalf("proxyReplace = %q", got) + } + o.AppliedKnown = false + if got := decideProxy(o, staged); got != proxyApply { + t.Fatalf("unknown config must still apply: %v", got) + } +} + +func TestPlanProxySaysStartWhenTraefikMissing(t *testing.T) { + for _, known := range []bool{true, false} { + t.Run(fmt.Sprint(known), func(t *testing.T) { + f := &transport.Fake{} + e, id, _ := proxyFixture(t, f) + rendered := testProxyCompose(t, e) + f.Dynamic = func(cmd string) (transport.Result, bool) { + switch { + case cmd == "cat '/var/lib/onebox/_host/proxy/config.hash'": + if known { + return transport.Result{Stdout: id.String()}, true + } + return transport.Result{ExitCode: 1}, true + case cmd == "cat '/var/lib/onebox/_host/proxy/compose.yaml'": + return transport.Result{Stdout: string(rendered)}, true + case strings.Contains(cmd, "service=discovery"): + return transport.Result{Stdout: "D1"}, true + case strings.Contains(cmd, "stat -c %i"): + return transport.Result{Stdout: "12"}, true + } + return transport.Result{}, false + } + got, err := e.PlanProxy(context.Background()) + if err != nil { + t.Fatal(err) + } + if got != "start Traefik" { + t.Fatalf("PlanProxy = %q, want start Traefik", got) + } + }) + } +} + +func TestObserveProxyTraefikDefinition(t *testing.T) { + for _, tt := range []struct { + name string + live transport.Result + want bool + }{ + {"same", transport.Result{Stdout: "services:\n proxy:\n image: traefik:v3.6\n"}, false}, + {"changed", transport.Result{Stdout: "services:\n proxy:\n image: traefik:v3.7\n"}, true}, + {"unreadable", transport.Result{ExitCode: 1}, true}, + {"failed read with content", transport.Result{ExitCode: 1, Stdout: "services:\n proxy:\n image: traefik:v3.6\n"}, true}, + } { + t.Run(tt.name, func(t *testing.T) { + f := &transport.Fake{Dynamic: func(cmd string) (transport.Result, bool) { + if cmd == "cat '/var/lib/onebox/_host/proxy/compose.yaml'" { + return tt.live, true + } + return transport.Result{}, false + }} + e, _, _ := proxyFixture(t, f) + o, err := e.observeProxy(context.Background(), []byte("services:\n proxy:\n image: traefik:v3.6\n")) + if err != nil { + t.Fatal(err) + } + if o.TraefikChanged != tt.want { + t.Fatalf("TraefikChanged = %v, want %v", o.TraefikChanged, tt.want) + } + }) + } +} + +func TestEnsureProxyReplacementUsesConvergePath(t *testing.T) { + f := &transport.Fake{} + e, staged, _ := proxyFixture(t, f) + rendered := testProxyCompose(t, e) + applied := staged + applied.Compose = "sha256:" + strings.Repeat("a", 64) + live := strings.Replace(string(rendered), "image: ", "image: old-", 1) + ps := proxyPS(f, true) + started := false + f.Dynamic = func(cmd string) (transport.Result, bool) { + switch { + case cmd == "cat '/var/lib/onebox/_host/proxy/config.hash'": + return transport.Result{Stdout: applied.String()}, true + case strings.HasPrefix(cmd, "cat '/var/lib/onebox/_host/proxy/compose.yaml'"): + return transport.Result{Stdout: live}, true + case strings.Contains(cmd, "up -d proxy"): + started = true + return transport.Result{}, true + case strings.Contains(cmd, "service=proxy") && started: + return transport.Result{Stdout: "PX2"}, true + case strings.Contains(cmd, "docker inspect") && strings.Contains(cmd, "PX2"): + return transport.Result{Stdout: "healthy"}, true + } + return ps(cmd) + } + inspected := false + e.T = &proxyUploadObserver{Fake: f, inspect: func(dir string) { + inspected = true + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatal(err) + } + if len(entries) != 1 || entries[0].Name() != "compose.yaml" { + t.Fatalf("replacement upload must hold only compose.yaml: %v", entries) + } + }} + if err := e.EnsureProxy(context.Background(), "replace-proxy", false); err != nil { + t.Fatal(err) + } + seq := strings.Join(f.Commands, "\n") + pending := proxy.Identity{Config: applied.Config, Compose: pendingReplaceDigest} + iPending := strings.Index(seq, testProxyIdentityWrite(pending)) + iStop := strings.Index(seq, " stop discovery") + iSample := strings.LastIndex(seq, "\n"+outputInodeCommand("/var/lib/onebox/_host/proxy/dynamic/onebox.yml")+"\n") + iUp := strings.Index(seq, " up -d discovery") + iWait := strings.Index(seq, "i=0;") + iProxy := strings.Index(seq, " up -d proxy") + iHealth := strings.Index(seq, "docker inspect -f '{{if .State.Health}}") + iFinal := strings.Index(seq, testProxyIdentityWrite(staged)) + if !inspected || iPending < 0 || iStop < iPending || iSample < iStop || iUp < iSample || iWait < iUp || iProxy < iWait || iHealth < iProxy || iFinal < iHealth { + t.Fatalf("replacement must converge discovery and wait for the new proxy before committing:\n%s", seq) + } + for _, forbidden := range []string{"rm -f '/var/lib/onebox/_host/proxy/config.hash'", "/retired/", "docker restart", "--force-recreate"} { + if strings.Contains(seq, forbidden) { + t.Fatalf("replacement must use the converge path, found %s:\n%s", forbidden, seq) + } + } +} + +func TestDecideProxy(t *testing.T) { + digestA := "sha256:" + strings.Repeat("a", 64) + digestB := "sha256:" + strings.Repeat("b", 64) + digestC := "sha256:" + strings.Repeat("c", 64) + staged := proxy.Identity{Config: digestA, Compose: digestB} + tests := []struct { + name string + mutate func(*proxyObservation) + want proxyAction + }{ + {"unknown marker", func(o *proxyObservation) { o.AppliedKnown = false }, proxyApply}, + {"config drift", func(o *proxyObservation) { o.Applied.Config = digestC }, proxyApply}, + {"compose drift", func(o *proxyObservation) { o.Applied.Compose = digestC }, proxyConverge}, + {"traefik missing", func(o *proxyObservation) { o.ProxyIDs = nil }, proxyConverge}, + {"discovery missing", func(o *proxyObservation) { o.DiscoveryIDs = nil }, proxyConverge}, + {"output missing", func(o *proxyObservation) { o.OutputInode = "" }, proxyConverge}, + {"all present and equal", func(o *proxyObservation) {}, proxyUnchanged}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + o := proxyObservation{Applied: staged, AppliedKnown: true, ProxyIDs: []string{"PX1"}, DiscoveryIDs: []string{"D1"}, OutputInode: "12"} + tt.mutate(&o) + if got := decideProxy(o, staged); got != tt.want { + t.Fatalf("decideProxy = %v, want %v", got, tt.want) + } + }) + } +} + +func TestObserveProxyIsReadOnly(t *testing.T) { + f := &transport.Fake{} + e, staged, _ := proxyFixture(t, f) + rendered := testProxyCompose(t, e) + f.Dynamic = func(cmd string) (transport.Result, bool) { + switch { + case strings.Contains(cmd, "{{.Image}}") || strings.Contains(cmd, "{{.Id}}"): + return transport.Result{Stdout: "sha256:proxy"}, true + case cmd == "cat '/var/lib/onebox/_host/proxy/compose.yaml'": + return transport.Result{Stdout: string(rendered)}, true + case strings.HasPrefix(cmd, "cat "): + return transport.Result{Stdout: staged.String() + "\n"}, true + case strings.Contains(cmd, "service=proxy"): + return transport.Result{Stdout: "PX1\n"}, true + case strings.Contains(cmd, "service=discovery"): + return transport.Result{Stdout: "D1\n"}, true + case strings.Contains(cmd, "stat -c %i"): + return transport.Result{Stdout: "12\n"}, true + } + return transport.Result{}, false + } + o, err := e.observeProxy(context.Background(), rendered) + if err != nil { + t.Fatal(err) + } + if !o.AppliedKnown || o.Applied != staged || strings.Join(o.ProxyIDs, ",") != "PX1" || strings.Join(o.DiscoveryIDs, ",") != "D1" || o.OutputInode != "12" { + t.Fatalf("observation = %+v", o) + } + if o.TraefikChanged { + t.Fatal("matching Traefik definition was reported changed") + } + for _, cmd := range f.Commands { + for _, mutation := range []string{"rm ", "mv ", "mkdir", "up -d", "docker restart", "> "} { + if strings.Contains(cmd, mutation) { + t.Fatalf("observe mutated host: %s", cmd) + } + } + } + if len(f.Commands) != 7 || len(f.Uploads) != 0 { + t.Fatalf("commands = %v; uploads = %v", f.Commands, f.Uploads) + } +} + +func TestObserveProxyMarker(t *testing.T) { + digest := "sha256:" + strings.Repeat("a", 64) + for _, marker := range []string{"", digest, digest + " sha256:abc", "unparseable"} { + t.Run(marker, func(t *testing.T) { + f := &transport.Fake{Dynamic: func(cmd string) (transport.Result, bool) { + if strings.HasPrefix(cmd, "cat ") { + return transport.Result{Stdout: marker}, true + } + return transport.Result{}, false + }} + e, _, _ := proxyFixture(t, f) + o, err := e.observeProxy(context.Background(), nil) + if err != nil { + t.Fatal(err) + } + if o.AppliedKnown { + t.Fatalf("invalid marker accepted: %+v", o) + } + if o.OutputInode != "" { + t.Fatalf("absent output inode = %q", o.OutputInode) + } + }) + } +} + +func TestProxyActionString(t *testing.T) { + for action, want := range map[proxyAction]string{ + proxyUnchanged: "unchanged", + proxyConverge: "update changed containers only, no restart", + proxyReplace: "recreate Traefik (its container definition changed)", + proxyApply: "restart Traefik (configuration changed)", + } { + if got := action.String(); got != want { + t.Fatalf("action %d = %q, want %q", action, got, want) + } + } +} + +func TestOutputInodeCommand(t *testing.T) { + const path = "/proxy with spaces/onebox.yml" + want := "if [ -s '/proxy with spaces/onebox.yml' ]; then stat -c %i '/proxy with spaces/onebox.yml'; fi" + if got := outputInodeCommand(path); got != want { + t.Fatalf("command = %q, want %q", got, want) + } +} + +func TestStageProxy(t *testing.T) { + e, want, _ := proxyFixture(t, &transport.Fake{}) + got, err := e.stageProxy(t.TempDir()) + if err != nil { + t.Fatal(err) + } + if got != want { + t.Fatalf("identity = %+v, want %+v", got, want) + } +} + +func TestPlanProxyReportsEachAction(t *testing.T) { + for _, tt := range []struct { + name string + want string + }{ + {"unchanged", "unchanged"}, + {"compose drift", "update changed containers only, no restart"}, + {"traefik drift", "recreate Traefik (its container definition changed)"}, + {"config drift", "restart Traefik (configuration changed)"}, + {"unmanaged", ""}, + } { + t.Run(tt.name, func(t *testing.T) { + f := &transport.Fake{} + e, applied, _ := proxyFixture(t, f) + live := string(testProxyCompose(t, e)) + switch tt.name { + case "compose drift": + applied.Compose = "sha256:" + strings.Repeat("c", 64) + live = strings.Replace(live, proxy.DiscoveryImage("dev"), "discovery:old", 1) + case "traefik drift": + applied.Compose = "sha256:" + strings.Repeat("c", 64) + live = strings.Replace(live, "image: ", "image: old-", 1) + case "config drift": + applied.Config = "sha256:" + strings.Repeat("c", 64) + case "unmanaged": + e.Spec.Proxy.Managed = false + } + f.Dynamic = func(cmd string) (transport.Result, bool) { + switch { + case strings.Contains(cmd, "{{.Image}}") || strings.Contains(cmd, "{{.Id}}"): + return transport.Result{Stdout: "sha256:proxy"}, true + case cmd == "cat '/var/lib/onebox/_host/proxy/compose.yaml'": + return transport.Result{Stdout: live}, true + case strings.HasPrefix(cmd, "cat "): + return transport.Result{Stdout: applied.String() + "\n"}, true + case strings.Contains(cmd, "service=proxy"): + return transport.Result{Stdout: "PX1\n"}, true + case strings.Contains(cmd, "service=discovery"): + return transport.Result{Stdout: "D1\n"}, true + case strings.Contains(cmd, "stat -c %i"): + return transport.Result{Stdout: "12\n"}, true + } + return transport.Result{}, false + } + got, err := e.PlanProxy(context.Background()) + if err != nil { + t.Fatal(err) + } + if got != tt.want { + t.Fatalf("PlanProxy = %q, want %q", got, tt.want) + } + for _, cmd := range f.Commands { + for _, mutation := range []string{"rm ", "mv ", "mkdir", "up -d", "docker restart", "> "} { + if strings.Contains(cmd, mutation) { + t.Fatalf("plan mutated host: %s", cmd) + } + } + } + wantCommands := 7 + if tt.name == "unmanaged" { + wantCommands = 0 + } + if len(f.Commands) != wantCommands || len(f.Uploads) != 0 { + t.Fatalf("commands = %v; uploads = %v", f.Commands, f.Uploads) + } + }) + } +} diff --git a/internal/engine/proxy_test.go b/internal/engine/proxy_test.go index 0f556002..2376e1b3 100644 --- a/internal/engine/proxy_test.go +++ b/internal/engine/proxy_test.go @@ -20,7 +20,7 @@ const testManagedProxyStatic = "ping: {}\nproviders:\n file:\n directory: /e // proxyFixture: a managed-proxy engine whose LocalDir holds traefik/{traefik.yml,.env}, // plus the staged payload hash the engine will compute for it. -func proxyFixture(t *testing.T, f *transport.Fake) (*Engine, string, *bytes.Buffer) { +func proxyFixture(t *testing.T, f *transport.Fake) (*Engine, proxy.Identity, *bytes.Buffer) { t.Helper() dir := t.TempDir() if err := os.Mkdir(filepath.Join(dir, "traefik"), 0o755); err != nil { @@ -84,8 +84,18 @@ func proxyPS(f *transport.Fake, preRunning bool) func(string) (transport.Result, return false } return func(cmd string) (transport.Result, bool) { + if strings.HasPrefix(cmd, "docker image inspect -f '{{.Id}}'") || + strings.HasPrefix(cmd, "docker inspect -f '{{.Image}}'") && strings.Contains(cmd, "PX1") { + return transport.Result{Stdout: "sha256:proxy\n"}, true + } + if cmd == outputInodeCommand("/var/lib/onebox/_host/proxy/dynamic/onebox.yml") { + return transport.Result{Stdout: "12\n"}, true + } if strings.Contains(cmd, "docker ps -q") && strings.Contains(cmd, "project='onebox-proxy'") { if preRunning || upRan() { + if strings.Contains(cmd, "service=discovery") { + return transport.Result{Stdout: "DISC1\n"}, true + } return transport.Result{Stdout: "PX1\n"}, true } return transport.Result{Stdout: ""}, true @@ -156,6 +166,26 @@ func TestEnsureProxyFreshHost(t *testing.T) { } } +func TestEnsureProxyLogsShortDigests(t *testing.T) { + f := &transport.Fake{} + f.Dynamic = proxyPS(f, false) + e, id, out := proxyFixture(t, f) + if err := e.EnsureProxy(context.Background(), "R1", false); err != nil { + t.Fatal(err) + } + want := "proxy: healthy at config " + strings.TrimPrefix(id.Config, "sha256:")[:8] + + " compose " + strings.TrimPrefix(id.Compose, "sha256:")[:8] + for _, line := range strings.Split(out.String(), "\n") { + if start := strings.Index(line, "proxy: healthy at config "); start >= 0 { + if line[start:] != want { + t.Fatalf("healthy log = %q, want %q", line, want) + } + return + } + } + t.Fatalf("healthy log missing from output:\n%s", out.String()) +} + func TestEnsureProxyStopsWhenJournalStartFails(t *testing.T) { f := &transport.Fake{} e, _, _ := proxyFixture(t, f) @@ -178,10 +208,17 @@ func TestEnsureProxyStopsWhenJournalStartFails(t *testing.T) { func TestEnsureProxyUnchangedIsNoOp(t *testing.T) { f := &transport.Fake{} e, hash, _ := proxyFixture(t, f) + live := string(testProxyCompose(t, e)) ps := proxyPS(f, true) f.Dynamic = func(cmd string) (transport.Result, bool) { + if strings.HasPrefix(cmd, "cat '/var/lib/onebox/_host/proxy/compose.yaml'") { + return transport.Result{Stdout: live}, true + } + if cmd == "rm -f '/var/lib/onebox/_host/proxy/config.hash'" && len(f.Uploads) != 0 { + t.Fatal("marker invalidated after upload") + } if strings.Contains(cmd, "cat '/var/lib/onebox/_host/proxy/config.hash'") { - return transport.Result{Stdout: hash + "\n"}, true + return transport.Result{Stdout: hash.String() + "\n"}, true } return ps(cmd) } @@ -207,7 +244,7 @@ func TestEnsureProxyRepairsMissingDiscoveryController(t *testing.T) { f.Dynamic = func(cmd string) (transport.Result, bool) { switch { case strings.Contains(cmd, "cat '/var/lib/onebox/_host/proxy/config.hash'"): - return transport.Result{Stdout: hash + "\n"}, true + return transport.Result{Stdout: hash.String() + "\n"}, true case strings.Contains(cmd, "com.docker.compose.service=discovery") && !strings.Contains(cmd, "up -d"): return transport.Result{Stdout: ""}, true } @@ -219,6 +256,10 @@ func TestEnsureProxyRepairsMissingDiscoveryController(t *testing.T) { if !strings.Contains(strings.Join(f.Commands, "\n"), "up -d --force-recreate discovery") { t.Fatalf("missing discovery controller was treated as an unchanged proxy:\n%s", strings.Join(f.Commands, "\n")) } + seq := strings.Join(f.Commands, "\n") + if strings.Contains(seq, "docker restart") || strings.Contains(seq, "/config' '/var/lib/onebox/_host/proxy/config'") { + t.Fatalf("discovery repair must preserve Traefik and its config:\n%s", seq) + } } func TestEnsureProxyRepairsMissingDiscoveryOutput(t *testing.T) { @@ -228,14 +269,14 @@ func TestEnsureProxyRepairsMissingDiscoveryOutput(t *testing.T) { f.Dynamic = func(cmd string) (transport.Result, bool) { switch { case strings.Contains(cmd, "cat '/var/lib/onebox/_host/proxy/config.hash'"): - return transport.Result{Stdout: hash + "\n"}, true - case strings.Contains(cmd, "test -s '/var/lib/onebox/_host/proxy/dynamic/onebox.yml'"): + return transport.Result{Stdout: hash.String() + "\n"}, true + case cmd == outputInodeCommand("/var/lib/onebox/_host/proxy/dynamic/onebox.yml"): for _, command := range f.Commands { if strings.Contains(command, "up -d --force-recreate discovery") { return transport.Result{}, true } } - return transport.Result{ExitCode: 1}, true + return transport.Result{}, true } return ps(cmd) } @@ -245,6 +286,10 @@ func TestEnsureProxyRepairsMissingDiscoveryOutput(t *testing.T) { if !strings.Contains(strings.Join(f.Commands, "\n"), "up -d --force-recreate discovery") { t.Fatalf("missing discovery output was treated as an unchanged proxy:\n%s", strings.Join(f.Commands, "\n")) } + seq := strings.Join(f.Commands, "\n") + if strings.Contains(seq, "docker restart") || strings.Contains(seq, "/config' '/var/lib/onebox/_host/proxy/config'") { + t.Fatalf("discovery repair must preserve Traefik and its config:\n%s", seq) + } } func TestEnsureProxyConfigOnlyChangeRestarts(t *testing.T) { @@ -254,6 +299,9 @@ func TestEnsureProxyConfigOnlyChangeRestarts(t *testing.T) { rendered := string(proxy.RenderComposeForApp("", proxy.DiscoveryImage("dev"), "sample", "", true, nil)) ps := proxyPS(f, true) f.Dynamic = func(cmd string) (transport.Result, bool) { + if cmd == "rm -f '/var/lib/onebox/_host/proxy/config.hash'" && len(f.Uploads) != 0 { + t.Fatal("marker invalidated after upload") + } if strings.Contains(cmd, "cat '/var/lib/onebox/_host/proxy/config.hash'") { return transport.Result{Stdout: "deadbeef\n"}, true } @@ -273,6 +321,9 @@ func TestEnsureProxyConfigOnlyChangeRestarts(t *testing.T) { if iUp < 0 || iRestart < 0 || iRestart < iUp { t.Fatalf("config-only change must up (no-op) then restart:\n%s", seq) } + if !strings.Contains(seq, "rm -f '/var/lib/onebox/_host/proxy/config.hash'") { + t.Fatalf("marker must be invalidated before uploading:\n%s", seq) + } if len(f.Uploads) != 1 { t.Fatalf("changed config must upload: %v", f.Uploads) } @@ -286,8 +337,8 @@ func TestEnsureProxyConfigOnlyChangeRestarts(t *testing.T) { // applied-state marker written ONLY after health confirms — an interrupted // converge must be retried, never mistaken for "unchanged" iHealth := strings.LastIndex(seq, "docker inspect") - iHash := strings.Index(seq, "> '/var/lib/onebox/_host/proxy/config.hash'") - if iHash < 0 || iHash < iHealth { + iHash := strings.Index(seq, "mv -f '/var/lib/onebox/_host/proxy/config.hash.tmp'") + if iHash < 0 || iHash < iHealth || !strings.Contains(seq, "printf '%s\\n'") { t.Fatalf("config.hash must be written after the health check:\n%s", seq) } } @@ -298,6 +349,9 @@ func TestEnsureProxyFailedConvergeLeavesHashUnwritten(t *testing.T) { rendered := string(proxy.RenderComposeForApp("", proxy.DiscoveryImage("dev"), "sample", "", true, nil)) ps := proxyPS(f, true) f.Dynamic = func(cmd string) (transport.Result, bool) { + if cmd == "rm -f '/var/lib/onebox/_host/proxy/config.hash'" && len(f.Uploads) != 0 { + t.Fatal("marker invalidated after upload") + } if strings.Contains(cmd, "cat '/var/lib/onebox/_host/proxy/config.hash'") { return transport.Result{Stdout: "deadbeef\n"}, true } @@ -313,7 +367,7 @@ func TestEnsureProxyFailedConvergeLeavesHashUnwritten(t *testing.T) { t.Fatal("failed restart must error") } seq := strings.Join(f.Commands, "\n") - if strings.Contains(seq, "> '/var/lib/onebox/_host/proxy/config.hash'") { + if !strings.Contains(seq, "rm -f '/var/lib/onebox/_host/proxy/config.hash'") || strings.Contains(seq, "config.hash.tmp") { t.Fatalf("failed converge must NOT record the applied hash (retry depends on it):\n%s", seq) } } @@ -382,3 +436,387 @@ func TestHostJournalPrunesOldestFiles(t *testing.T) { t.Fatalf("an absent journal is nothing to prune: %v\n%s", err, out) } } + +func TestEnsureProxyComposeOnlyChangeSkipsSwapAndRestart(t *testing.T) { + f := &transport.Fake{} + e, id, _ := proxyFixture(t, f) + id.Compose = "sha256:" + strings.Repeat("0", 64) + ps := proxyPS(f, true) + f.Dynamic = func(cmd string) (transport.Result, bool) { + if cmd == "cat '/var/lib/onebox/_host/proxy/config.hash'" { + return transport.Result{Stdout: id.String()}, true + } + if strings.Contains(cmd, "service=discovery") { + name := "D1" + if strings.Contains(strings.Join(f.Commands, "\n"), "up -d discovery") { + name = "D2" + } + return transport.Result{Stdout: name}, true + } + if cmd == outputInodeCommand("/var/lib/onebox/_host/proxy/dynamic/onebox.yml") { + return transport.Result{Stdout: "100"}, true + } + if strings.HasPrefix(cmd, "i=0;") { + if !strings.Contains(cmd, "100") || !strings.Contains(cmd, "stat -c %i") { + t.Fatalf("publication must require a fresh inode: %s", cmd) + } + return transport.Result{}, true // inode 101 was published + } + return ps(cmd) + } + if err := e.EnsureProxy(context.Background(), "compose-change", false); err != nil { + t.Fatal(err) + } + seq := strings.Join(f.Commands, "\n") + for _, forbidden := range []string{"/config' '/var/lib/onebox/_host/proxy/config'", "docker restart", "--force-recreate"} { + if strings.Contains(seq, forbidden) { + t.Fatalf("unexpected %s:\n%s", forbidden, seq) + } + } + if !strings.Contains(seq, "/compose.yaml' '/var/lib/onebox/_host/proxy/compose.yaml'") || !strings.Contains(seq, "i=0;") { + t.Fatalf("compose and publication must converge:\n%s", seq) + } + iMarker := strings.LastIndex(seq, "mv -f '/var/lib/onebox/_host/proxy/config.hash.tmp'") + if iMarker < 0 || iMarker < strings.LastIndex(seq, "docker inspect") { + t.Fatalf("marker precedes health:\n%s", seq) + } + if iMarker < strings.LastIndex(seq, "i=0;") { + t.Fatalf("marker precedes publication:\n%s", seq) + } +} + +func TestEnsureProxyMissingOutputForceRecreatesDiscoveryWithoutRestart(t *testing.T) { + f := &transport.Fake{} + e, id, _ := proxyFixture(t, f) + ps := proxyPS(f, true) + inode := "" + f.Dynamic = func(cmd string) (transport.Result, bool) { + if cmd == "cat '/var/lib/onebox/_host/proxy/config.hash'" { + return transport.Result{Stdout: id.String()}, true + } + if strings.Contains(cmd, "up -d --force-recreate discovery") { + inode = "200" + return transport.Result{}, true + } + if cmd == outputInodeCommand("/var/lib/onebox/_host/proxy/dynamic/onebox.yml") { + return transport.Result{Stdout: inode}, true + } + if strings.HasPrefix(cmd, "i=0;") { + if inode == "" { + return transport.Result{ExitCode: 1}, true + } + return transport.Result{}, true // inode 200 was published + } + return ps(cmd) + } + if err := e.EnsureProxy(context.Background(), "missing-output", false); err != nil { + t.Fatal(err) + } + seq := strings.Join(f.Commands, "\n") + if !strings.Contains(seq, "up -d --force-recreate discovery") || !strings.Contains(seq, "i=0;") { + t.Fatalf("missing output must force discovery and wait for publication:\n%s", seq) + } + if strings.Contains(seq, "docker restart") || strings.Contains(seq, "/config' '/var/lib/onebox/_host/proxy/config'") { + t.Fatalf("missing output must not swap config or restart Traefik:\n%s", seq) + } +} + +func TestEnsureProxyComposePathRejectsStaleOutput(t *testing.T) { + f := &transport.Fake{} + e, id, _ := proxyFixture(t, f) + live := string(testProxyCompose(t, e)) + staged := id + id.Compose = "sha256:" + strings.Repeat("0", 64) + ps := proxyPS(f, true) + f.Dynamic = func(cmd string) (transport.Result, bool) { + if cmd == "cat '/var/lib/onebox/_host/proxy/config.hash'" { + return transport.Result{Stdout: id.String()}, true + } + if strings.HasPrefix(cmd, "cat '/var/lib/onebox/_host/proxy/compose.yaml'") { + return transport.Result{Stdout: live}, true + } + if strings.Contains(cmd, "service=discovery") { + name := "D1" + if strings.Contains(strings.Join(f.Commands, "\n"), "up -d discovery") { + name = "D2" + } + return transport.Result{Stdout: name}, true + } + if cmd == outputInodeCommand("/var/lib/onebox/_host/proxy/dynamic/onebox.yml") { + return transport.Result{Stdout: "100"}, true + } + if strings.HasPrefix(cmd, "i=0;") { + return transport.Result{ExitCode: 1}, true + } + return ps(cmd) + } + err := e.EnsureProxy(context.Background(), "stale", false) + if err == nil || !strings.Contains(err.Error(), "did not publish") || !strings.Contains(err.Error(), "docker logs onebox-discovery") { + t.Fatalf("stale output error: %v", err) + } + seq := strings.Join(f.Commands, "\n") + iPending := strings.Index(seq, testProxyIdentityWrite(id)) + iMutation := strings.Index(seq, "rm -rf '/var/lib/onebox/_host/proxy/.staged-") + if iPending < 0 || iMutation < 0 || iPending > iMutation || + strings.Contains(seq, "rm -f '/var/lib/onebox/_host/proxy/config.hash'") || + strings.Contains(seq, testProxyIdentityWrite(staged)) { + t.Fatalf("failed converge must atomically mark pending before mutation without committing the staged identity:\n%s", seq) + } +} + +func testProxyIdentityWrite(id proxy.Identity) string { + path := "/var/lib/onebox/_host/proxy/config.hash" + return "printf '%s\\n' " + q(id.String()) + " > " + q(path+".tmp") + + " && mv -f " + q(path+".tmp") + " " + q(path) +} + +func TestEnsureProxyFailedConvergeRetriesAsConverge(t *testing.T) { + f := &transport.Fake{} + e, id, _ := proxyFixture(t, f) + live := string(testProxyCompose(t, e)) + applied := id + applied.Compose = "sha256:" + strings.Repeat("a", 64) + pending := proxy.Identity{Config: applied.Config, Compose: "sha256:" + strings.Repeat("0", 64)} + marker := applied.String() + fail := true + ps := proxyPS(f, true) + f.Dynamic = func(cmd string) (transport.Result, bool) { + switch { + case cmd == "cat '/var/lib/onebox/_host/proxy/config.hash'": + return transport.Result{Stdout: marker}, true + case strings.HasPrefix(cmd, "cat '/var/lib/onebox/_host/proxy/compose.yaml'"): + return transport.Result{Stdout: live}, true + case strings.Contains(cmd, testProxyIdentityWrite(pending)): + marker = pending.String() + return transport.Result{}, true + case strings.Contains(cmd, testProxyIdentityWrite(id)): + marker = id.String() + return transport.Result{}, true + case strings.Contains(cmd, "rm -f '/var/lib/onebox/_host/proxy/config.hash'"): + marker = "" + return transport.Result{}, true + } + if strings.Contains(cmd, "rm -rf '/var/lib/onebox/_host/proxy/.staged-") && marker != pending.String() { + t.Error("converge mutated staging before atomically recording the pending identity") + } + if strings.Contains(cmd, "up -d discovery") && fail { + return transport.Result{ExitCode: 1, Stderr: "cannot start discovery"}, true + } + return ps(cmd) + } + if err := e.EnsureProxy(context.Background(), "failed-converge", false); err == nil { + t.Fatal("failed discovery up must error") + } + if marker != pending.String() || strings.Contains(strings.Join(f.Commands, "\n"), testProxyIdentityWrite(id)) { + t.Fatalf("failed converge must keep only the pending identity: %q", marker) + } + beforeRetry := len(f.Commands) + fail = false + if err := e.EnsureProxy(context.Background(), "retry-converge", false); err != nil { + t.Fatal(err) + } + retry := strings.Join(f.Commands[beforeRetry:], "\n") + for _, forbidden := range []string{"docker restart", "/config' '/var/lib/onebox/_host/proxy/config'", "rm -f '/var/lib/onebox/_host/proxy/config.hash'"} { + if strings.Contains(retry, forbidden) { + t.Fatalf("retry must remain a converge, found %s:\n%s", forbidden, retry) + } + } + if marker != id.String() || !strings.Contains(retry, "i=0;") { + t.Fatalf("retry must publish before committing the final identity:\n%s", retry) + } +} + +func TestEnsureProxyConvergeStopsDiscoveryBeforeSampling(t *testing.T) { + f := &transport.Fake{} + e, id, _ := proxyFixture(t, f) + id.Compose = "sha256:" + strings.Repeat("a", 64) + ps := proxyPS(f, true) + stopped, started := false, false + inodeCommand := outputInodeCommand("/var/lib/onebox/_host/proxy/dynamic/onebox.yml") + f.Dynamic = func(cmd string) (transport.Result, bool) { + if cmd == "cat '/var/lib/onebox/_host/proxy/config.hash'" { + return transport.Result{Stdout: id.String()}, true + } + if strings.Contains(cmd, " stop discovery") { + stopped = true + return transport.Result{}, true + } + if cmd == inodeCommand { + if stopped { + return transport.Result{Stdout: "101"}, true + } + return transport.Result{Stdout: "100"}, true + } + if strings.Contains(cmd, "up -d discovery") { + started = true + return transport.Result{}, true + } + if strings.HasPrefix(cmd, "i=0;") { + if !stopped || !started || !strings.Contains(cmd, "!= '101'") { + t.Fatalf("publication must use the inode sampled with discovery stopped: %s", cmd) + } + return transport.Result{}, true + } + return ps(cmd) + } + if err := e.EnsureProxy(context.Background(), "fresh-publication", false); err != nil { + t.Fatal(err) + } + seq := strings.Join(f.Commands, "\n") + iStop, iSample, iUp := strings.Index(seq, " stop discovery"), strings.LastIndex(seq, "\n"+inodeCommand+"\n"), strings.Index(seq, " up -d discovery") + if iStop < 0 || iSample < iStop || iUp < iSample || !strings.Contains(seq, "i=0;") { + t.Fatalf("converge must stop, sample, start and await publication even when the discovery id stays the same:\n%s", seq) + } +} + +type proxyUploadObserver struct { + *transport.Fake + inspect func(string) +} + +func (f *proxyUploadObserver) Upload(ctx context.Context, localDir, remoteDir string) error { + f.inspect(localDir) + return f.Fake.Upload(ctx, localDir, remoteDir) +} + +func TestEnsureProxyConvergeUploadsOnlyCompose(t *testing.T) { + f := &transport.Fake{} + e, id, _ := proxyFixture(t, f) + id.Compose = "sha256:" + strings.Repeat("a", 64) + ps := proxyPS(f, true) + f.Dynamic = func(cmd string) (transport.Result, bool) { + if cmd == "cat '/var/lib/onebox/_host/proxy/config.hash'" { + return transport.Result{Stdout: id.String()}, true + } + return ps(cmd) + } + inspected := false + e.T = &proxyUploadObserver{Fake: f, inspect: func(dir string) { + inspected = true + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatal(err) + } + if len(entries) != 1 || entries[0].Name() != "compose.yaml" || entries[0].IsDir() { + t.Fatalf("converge upload must hold only compose.yaml: %v", entries) + } + }} + if err := e.EnsureProxy(context.Background(), "compose-upload", false); err != nil { + t.Fatal(err) + } + if !inspected { + t.Fatal("converge did not upload compose") + } +} + +func TestEnsureProxyConfigSwapRetiresLiveDirsInsteadOfDeleting(t *testing.T) { + f := &transport.Fake{} + e, _, _ := proxyFixture(t, f) + f.Dynamic = proxyPS(f, true) + if err := e.EnsureProxy(context.Background(), "retire-config", false); err != nil { + t.Fatal(err) + } + staged := strings.SplitN(f.Uploads[0], " -> ", 2)[1] + seq := strings.Join(f.Commands, "\n") + for _, dir := range []string{"config", "dynamic"} { + live := "/var/lib/onebox/_host/proxy/" + dir + if strings.Contains(seq, "rm -rf "+q(live)) || !strings.Contains(seq, "mv "+q(live)+" "+q(staged+"/retired/"+dir)) { + t.Fatalf("live %s must be retired without deleting its bind-mounted contents:\n%s", dir, seq) + } + } + iCleanup := strings.LastIndex(seq, "-name '.staged-*'") + if iCleanup < strings.LastIndex(seq, "docker inspect") || iCleanup < strings.LastIndex(seq, "config.hash.tmp") { + t.Fatalf("retired directories must survive until health and the final marker:\n%s", seq) + } +} + +func TestEnsureProxyFailedConfigApplyKeepsRetiredDirsUntilRetrySucceeds(t *testing.T) { + f := &transport.Fake{} + e, _, _ := proxyFixture(t, f) + ps := proxyPS(f, true) + fail := true + dir := t.TempDir() + retired := "" + filteredCleanups := 0 + f.Dynamic = func(cmd string) (transport.Result, bool) { + if strings.Contains(cmd, "-name '.staged-*'") { + if strings.Contains(cmd, "! -exec test -d ") { + runProxyStagingCleanup(t, cmd, dir) + filteredCleanups++ + if retired != "" { + if _, err := os.Stat(retired); err != nil { + t.Fatalf("filtered cleanup removed mounted retired directories: %v", err) + } + } + } + } + if strings.Contains(cmd, "/retired/config") && retired == "" { + retired = filepath.Join(dir, ".staged-"+e.hostLockToken, "retired") + if err := os.MkdirAll(retired, 0o755); err != nil { + t.Fatal(err) + } + } + if strings.Contains(cmd, "docker restart") && fail { + return transport.Result{ExitCode: 1, Stderr: "cannot restart"}, true + } + return ps(cmd) + } + if err := e.EnsureProxy(context.Background(), "failed-config", false); err == nil { + t.Fatal("failed restart must error") + } + staged := strings.SplitN(f.Uploads[0], " -> ", 2)[1] + first := strings.Join(f.Commands, "\n") + iSwap := strings.Index(first, "/config' '/var/lib/onebox/_host/proxy/config'") + if iSwap < 0 { + t.Fatal("config was not swapped") + } + for _, cmd := range f.Commands { + if strings.Contains(cmd, "-name '.staged-*'") && !strings.Contains(cmd, "! -exec test -d ") { + t.Fatalf("failed apply must not fully clean staging: %s", cmd) + } + } + if strings.Contains(first[iSwap:], "rm -rf "+q(staged)) { + t.Fatalf("failed apply must preserve the retired directories:\n%s", first) + } + beforeRetry := len(f.Commands) + fail = false + if err := e.EnsureProxy(context.Background(), "retry-config", false); err != nil { + t.Fatal(err) + } + retry := strings.Join(f.Commands[beforeRetry:], "\n") + iCleanup := -1 + for _, cmd := range f.Commands[beforeRetry:] { + if strings.Contains(cmd, "-name '.staged-*'") && !strings.Contains(cmd, "! -exec test -d ") { + iCleanup = strings.Index(retry, cmd) + break + } + } + if iCleanup < strings.LastIndex(retry, "docker inspect") || iCleanup < strings.LastIndex(retry, "config.hash.tmp") { + t.Fatalf("retry must keep retired directories until it succeeds:\n%s", retry) + } + if filteredCleanups != 2 { + t.Fatalf("both runs must clean failed uploads while preserving retired directories: %d", filteredCleanups) + } +} + +func TestEnsureProxyMissingMarkerTakesConfigPath(t *testing.T) { + for _, marker := range []string{"", "sha256:" + strings.Repeat("a", 64), "truncated marker"} { + t.Run(marker, func(t *testing.T) { + f := &transport.Fake{} + e, _, _ := proxyFixture(t, f) + ps := proxyPS(f, true) + f.Dynamic = func(cmd string) (transport.Result, bool) { + if cmd == "cat '/var/lib/onebox/_host/proxy/config.hash'" { + return transport.Result{Stdout: marker}, true + } + return ps(cmd) + } + if err := e.EnsureProxy(context.Background(), "unknown-marker", false); err != nil { + t.Fatal(err) + } + if !strings.Contains(strings.Join(f.Commands, "\n"), "/config' '/var/lib/onebox/_host/proxy/config'") { + t.Fatal("unknown marker did not apply config") + } + }) + } +} diff --git a/internal/engine/proxystatus.go b/internal/engine/proxystatus.go index 5d00f703..0a7ba88d 100644 --- a/internal/engine/proxystatus.go +++ b/internal/engine/proxystatus.go @@ -4,7 +4,6 @@ import ( "context" "fmt" "os" - "path/filepath" "strconv" "strings" @@ -23,11 +22,11 @@ type proxyRaw struct { ids []string health string // proxy container health, parsed from docker ps .Status discovery bool // isolated Docker discovery controller is running - applied string // config hash the host applied + applied string // config and Compose identity the host applied owner string // sole application identity from the host owner record ownerEnv string // environment identity from the owner record acme []string // raw ACME stores; parsed at render, and keys never leave - localHash string // hash of the locally staged config (computed offline) + localHash string // locally staged config and Compose identity (computed offline) // Why a read could not be trusted, when it could not. Recorded rather // than raised: gather returns on the first error and Status renders // nothing after it, so raising costs the operator every other fact about @@ -173,21 +172,13 @@ func (e *Engine) proxyReads(ctx context.Context, px *proxyRaw) []func() error { return nil }, func() error { - // Empty stays empty: joining it with the project directory would - // point at the repository root and ask it to be Traefik's config. - localCfg := e.Spec.Proxy.Config - if localCfg != "" && !filepath.IsAbs(localCfg) { - localCfg = filepath.Join(e.Opts.LocalDir, localCfg) - } staging, err := os.MkdirTemp("", "onebox-proxy-status") if err != nil { return err } defer os.RemoveAll(staging) - px.localHash, err = proxy.StageForAppManaged(localCfg, staging, e.Spec.Proxy.Image, - proxy.DiscoveryImage(e.Opts.Runner.Version), e.Spec.Name, - e.Spec.Proxy.Network, e.Spec.Proxy.Entrypoints, e.Spec.HasExactTerminatingTLS(), - e.Spec.HasWildcardTerminatingTLS(), e.Spec.Proxy.DNSChallenge) + id, err := e.stageProxy(staging) + px.localHash = id.String() return err }, } @@ -291,14 +282,15 @@ func (e *Engine) renderProxy(px proxyRaw) (bool, error) { diverged = true } - state := fmt.Sprintf("config %.8s %s", px.applied, e.ui.OK("(in sync)")) + local, _ := proxy.ParseIdentity(px.localHash) + state := fmt.Sprintf("config %s compose %s %s", shortDigest(local.Config), shortDigest(local.Compose), e.ui.OK("(in sync)")) if px.appliedIssue != "" { // Never DRIFTED on an unread file: that names a remedy (`ob proxy // apply`) for a problem it would not fix. state = e.ui.Warn("config UNKNOWN ⚠ — " + px.appliedIssue) diverged = true - } else if px.applied != px.localHash { - state = e.ui.Warn(fmt.Sprintf("config DRIFTED ⚠ (local %.8s ≠ applied %.8s) — `ob proxy apply`", px.localHash, px.applied)) + } else if parts := proxyDriftParts(px.applied, local); len(parts) > 0 { + state = e.ui.Warn(fmt.Sprintf("DRIFTED ⚠ (%s) — `ob proxy apply`", strings.Join(parts, ", "))) diverged = true } owner := px.owner @@ -350,3 +342,26 @@ func proxyCertExpiries(stores []string) ([]proxy.CertExpiry, error) { } return out, nil } + +func proxyDriftParts(applied string, local proxy.Identity) []string { + remote, ok := proxy.ParseIdentity(applied) + if _, valid := proxy.ParseIdentity(local.String()); !ok || !valid { + return []string{"config", "compose"} + } + var parts []string + if remote.Config != local.Config { + parts = append(parts, "config") + } + if remote.Compose != local.Compose { + parts = append(parts, "compose") + } + return parts +} + +func shortDigest(d string) string { + d = strings.TrimPrefix(d, "sha256:") + if len(d) > 8 { + return d[:8] + } + return d +} diff --git a/internal/engine/proxystatus_test.go b/internal/engine/proxystatus_test.go index a7c7b2c0..69d3d1c2 100644 --- a/internal/engine/proxystatus_test.go +++ b/internal/engine/proxystatus_test.go @@ -14,6 +14,7 @@ import ( "math/big" "os" "path/filepath" + "reflect" "strings" "testing" "time" @@ -61,7 +62,7 @@ func statusProxyEngine(t *testing.T, appliedHash *string, acme string, proxyHeal t.Fatal(err) } if *appliedHash == "" { - *appliedHash = localHash // default: in sync + *appliedHash = localHash.String() // default: in sync } f := &transport.Fake{} f.Dynamic = func(cmd string) (transport.Result, bool) { @@ -91,7 +92,7 @@ func statusProxyEngine(t *testing.T, appliedHash *string, acme string, proxyHeal var out bytes.Buffer now := func() time.Time { return time.Date(2026, 7, 4, 12, 0, 0, 0, time.UTC) } e := New(cfg, testProject(t), f, Options{Out: &out, Sleep: noSleep, Now: now, LocalDir: dir}) - return e, f, &out, localHash + return e, f, &out, localHash.String() } func TestStatusManagedProxyInSync(t *testing.T) { @@ -298,3 +299,64 @@ func TestStatusUnmanagedProxyUnchanged(t *testing.T) { t.Fatalf("unmanaged status must not touch _host:\n%s", strings.Join(f.Commands, "\n")) } } + +func TestProxyDriftParts(t *testing.T) { + local := proxy.Identity{Config: "sha256:" + strings.Repeat("a", 64), Compose: "sha256:" + strings.Repeat("b", 64)} + other := "sha256:" + strings.Repeat("c", 64) + for _, tt := range []struct { + name, applied string + want []string + }{ + {"equal", local.String(), nil}, + {"compose", (proxy.Identity{Config: local.Config, Compose: other}).String(), []string{"compose"}}, + {"config", (proxy.Identity{Config: other, Compose: local.Compose}).String(), []string{"config"}}, + {"both", (proxy.Identity{Config: other, Compose: other}).String(), []string{"config", "compose"}}, + {"legacy", local.Config, []string{"config", "compose"}}, + {"garbage", "deadbeef", []string{"config", "compose"}}, + {"empty", "", []string{"config", "compose"}}, + } { + t.Run(tt.name, func(t *testing.T) { + if got := proxyDriftParts(tt.applied, local); !reflect.DeepEqual(got, tt.want) { + t.Fatalf("got %v, want %v", got, tt.want) + } + }) + } + if got := proxyDriftParts(" ", proxy.Identity{}); !reflect.DeepEqual(got, []string{"config", "compose"}) { + t.Fatalf("invalid pair in sync: %v", got) + } +} + +func TestProxyStatusNamesComposeDrift(t *testing.T) { + applied := "" + e, _, out, localHash := statusProxyEngine(t, &applied, "", "healthy") + local, ok := proxy.ParseIdentity(localHash) + if !ok { + t.Fatal("invalid staged identity") + } + applied = (proxy.Identity{Config: local.Config, Compose: "sha256:" + strings.Repeat("c", 64)}).String() + if err := e.Status(context.Background()); err == nil { + t.Fatal("expected divergence") + } + if s := out.String(); !strings.Contains(s, "DRIFTED ⚠ (compose)") || strings.Contains(s, "(config") { + t.Fatalf("compose drift not named: %s", s) + } +} + +func TestProxyStatusShowsDigestPairInSync(t *testing.T) { + applied := "" + e, _, out, localHash := statusProxyEngine(t, &applied, "", "healthy") + local, _ := proxy.ParseIdentity(localHash) + if err := e.Status(context.Background()); err != nil { + t.Fatal(err) + } + want := "config " + strings.TrimPrefix(local.Config, "sha256:")[:8] + " compose " + strings.TrimPrefix(local.Compose, "sha256:")[:8] + " (in sync)" + if !strings.Contains(out.String(), want) { + t.Fatalf("missing %q: %s", want, out.String()) + } +} + +func TestShortDigest(t *testing.T) { + if got := shortDigest("sha256:0123456789abcdef"); got != "01234567" { + t.Fatalf("short digest = %q", got) + } +} diff --git a/internal/engine/recovery.go b/internal/engine/recovery.go index 5b919c02..806930c0 100644 --- a/internal/engine/recovery.go +++ b/internal/engine/recovery.go @@ -166,13 +166,21 @@ func (e *Engine) recoverInterrupted(ctx context.Context, request recoveryRequest } }() - if err := e.removeExactReleaseContainers(ctx, request.InterruptedID); err != nil { + if err := e.removeInterruptedNonServing(ctx, request.InterruptedID, previous); err != nil { return &RecoveryIncompleteError{Phase: "cleanup", Err: err} } if previous != nil { - if err := e.restoreReleaseRoles(ctx, previous, request.PreviousID); err != nil { + if err := e.undrainPredecessorReplicas(ctx, previous, request.PreviousID); err != nil { + return &RecoveryIncompleteError{Phase: "restore", Err: err} + } + if err := e.restoreReleaseRoles(ctx, previous, request.PreviousID, request.InterruptedID); err != nil { return &RecoveryIncompleteError{Phase: "restore", Err: err} } + } + if err := e.removeExactReleaseContainers(ctx, request.InterruptedID); err != nil { + return &RecoveryIncompleteError{Phase: "sweep", Err: err} + } + if previous != nil { if err := previous.Verify(ctx); err != nil { return &RecoveryIncompleteError{Phase: "verify", Err: err} } @@ -200,6 +208,101 @@ func (e *Engine) exactReleaseContainerIDs(ctx context.Context, releaseID string) return splitIDs(result.Stdout) } +func (e *Engine) removeInterruptedNonServing(ctx context.Context, interruptedID string, previous *Engine) error { + result, err := e.T.Run(ctx, "docker ps -a --filter label=onebox.app="+q(e.Spec.Name)+ + " --filter label=onebox.release="+q(interruptedID)+ + " --format "+q(`{{.ID}}|{{.Label "com.docker.compose.service"}}|{{.Names}}|{{.State}}`)) + if err != nil { + return err + } + if result.ExitCode != 0 { + return fmt.Errorf("list interrupted-release containers failed (exit %d): %s", result.ExitCode, strings.TrimSpace(result.Stderr)) + } + var remove []string + for _, line := range strings.Split(strings.TrimSpace(result.Stdout), "\n") { + if line == "" { + continue + } + fields := strings.Split(line, "|") + if len(fields) != 4 { + return fmt.Errorf("invalid interrupted-release container record %q", line) + } + if !validID.MatchString(fields[0]) { + return fmt.Errorf("suspicious container id %q from docker ps — refusing to reuse in a command", fields[0]) + } + if previous != nil && fields[3] == "running" { + if role, ok := previous.Spec.Workloads[fields[1]]; ok && role.Mode() == "rolling" { + if fields[2] == e.names().TransientContainer(fields[1]) { + occupied, err := e.T.Run(ctx, "docker ps -a --format '{{.Names}}'") + if err != nil { + return err + } + if occupied.ExitCode != 0 { + return fmt.Errorf("list occupied container names failed (exit %d): %s", occupied.ExitCode, strings.TrimSpace(occupied.Stderr)) + } + names := make(map[string]bool) + for _, name := range strings.FieldsFunc(occupied.Stdout, func(r rune) bool { return r == '\n' || r == ',' || r == ' ' || r == '\r' || r == '\t' }) { + names[name] = true + } + slot := 1 + for names[e.names().Container(fields[1], slot)] { + slot++ + } + if err := e.renameContainer(ctx, fields[0], e.names().Container(fields[1], slot)); err != nil { + return err + } + } + continue + } + } + remove = append(remove, fields[0]) + } + for _, id := range remove { + command := "if docker inspect " + id + " >/dev/null 2>&1; then docker rm -f " + id + "; fi" + if err := e.mutateChecked(ctx, "remove interrupted container "+id, command); err != nil { + return err + } + } + return nil +} + +// undrainPredecessorReplicas lets replicas drained by the interrupted roll +// become healthy before the predecessor roll admits them again. +func (e *Engine) undrainPredecessorReplicas(ctx context.Context, previous *Engine, previousID string) error { + result, err := e.T.Run(ctx, "docker ps --filter label=onebox.app="+q(e.Spec.Name)+ + " --filter label=onebox.release="+q(previousID)+ + " --filter status=running --format "+q(`{{.ID}}|{{.Label "com.docker.compose.service"}}`)) + if err != nil { + return err + } + if result.ExitCode != 0 { + return fmt.Errorf("list predecessor replicas failed (exit %d): %s", result.ExitCode, strings.TrimSpace(result.Stderr)) + } + var ids []string + for _, line := range strings.Split(strings.TrimSpace(result.Stdout), "\n") { + if line == "" { + continue + } + fields := strings.Split(line, "|") + if len(fields) != 2 || !validID.MatchString(fields[0]) { + return fmt.Errorf("invalid predecessor container record %q", line) + } + if role, ok := previous.Spec.Workloads[fields[1]]; ok && role.Mode() == "rolling" { + ids = append(ids, fields[0]) + } + } + for _, id := range ids { + result, err := e.mutate(ctx, "docker exec "+id+" rm -f "+app.DrainFile) + if err != nil { + return err + } + if result.ExitCode != 0 { + e.logf("undrain predecessor replica %s failed (exit %d): %s", id, result.ExitCode, strings.TrimSpace(result.Stderr)) + } + } + return nil +} + func (e *Engine) removeExactReleaseContainers(ctx context.Context, releaseID string) error { ids, err := e.exactReleaseContainerIDs(ctx, releaseID) if err != nil { @@ -225,7 +328,7 @@ func (e *Engine) assertNoReleaseContainers(ctx context.Context, releaseID string return nil } -func (e *Engine) restoreReleaseRoles(ctx context.Context, previous *Engine, previousID string) error { +func (e *Engine) restoreReleaseRoles(ctx context.Context, previous *Engine, previousID, interruptedID string) error { composePath := release.PathsFor(e.names()).Releases + "/" + previousID + "/compose.yaml" revisions, err := previous.releaseWorkloadRevisions(ctx, composePath) if err != nil { @@ -237,7 +340,14 @@ func (e *Engine) restoreReleaseRoles(ctx context.Context, previous *Engine, prev } for _, roleName := range previous.Spec.ReleaseOrder() { role := previous.Spec.Workloads[roleName] - if retainableWorkload(observed[roleName], revisions[roleName], role.Count()) { + interruptedOwned := false + for _, container := range observed[roleName] { + if container.release == interruptedID { + interruptedOwned = true + break + } + } + if !interruptedOwned && retainableWorkload(observed[roleName], revisions[roleName], role.Count()) { previous.logf("restore %s: already at target workload revision", roleName) continue } diff --git a/internal/engine/recovery_test.go b/internal/engine/recovery_test.go index ff2d62d6..63f2bfa9 100644 --- a/internal/engine/recovery_test.go +++ b/internal/engine/recovery_test.go @@ -4,10 +4,12 @@ import ( "bytes" "context" "errors" + "fmt" "strings" "testing" "time" + "github.com/labstack/onebox/internal/app" "github.com/labstack/onebox/internal/journal" "github.com/labstack/onebox/internal/release" "github.com/labstack/onebox/internal/transport" @@ -57,6 +59,8 @@ func TestRecoveryRetryKeepsCheckpointUntilHealthyAndSweepsStaleRoles(t *testing. switch { case strings.Contains(command, "readlink"): return transport.Result{Stdout: "releases/" + engineTestPreviousReleaseID + "\n"}, true + case strings.Contains(command, "docker ps -a --filter label=onebox.app="): + return transport.Result{Stdout: "NEW1|web|demo-web-1|running\nSTALE1|stale|demo-stale-1|running"}, true case strings.Contains(command, "docker ps -aq") && strings.Contains(command, "label=onebox.release='"+engineTestDeployReleaseID+"'"): var ids []string for _, pair := range []struct{ id, marker string }{{"NEW1", "docker rm -f NEW1"}, {"STALE1", "docker rm -f STALE1"}} { @@ -100,6 +104,11 @@ func TestRecoveryRetryKeepsCheckpointUntilHealthyAndSweepsStaleRoles(t *testing. if !errors.As(err, &incomplete) || incomplete.Code() != "recovery_incomplete" || incomplete.Phase != "verify" { t.Fatalf("first recovery error = %#v", err) } + commands := strings.Join(target.Commands, "\n") + if strings.Index(commands, "docker rm -f STALE1") > strings.Index(commands, "--scale web=") || + strings.Index(commands, "docker rm -f NEW1") < strings.Index(commands, "--scale web=") { + t.Fatalf("recovery cleanup order:\n%s", commands) + } if strings.Count(output.String(), "✓ web rolling") != 1 { t.Fatalf("restored workload must leave one completed step before verification: %s", output.String()) } @@ -338,7 +347,7 @@ func TestRestoreReleaseRolesRetainsMatchingTargetRevisions(t *testing.T) { return base(command) } engine := New(testConfig(), testProject(t), target, Options{Out: &bytes.Buffer{}, Sleep: noSleep}) - if err := engine.restoreReleaseRoles(context.Background(), engine, engineTestPreviousReleaseID); err != nil { + if err := engine.restoreReleaseRoles(context.Background(), engine, engineTestPreviousReleaseID, engineTestDeployReleaseID); err != nil { t.Fatal(err) } for _, command := range target.Commands { @@ -347,3 +356,666 @@ func TestRestoreReleaseRolesRetainsMatchingTargetRevisions(t *testing.T) { } } } + +func TestRecoverySingleReplicaKeepsInterruptedNewcomerServing(t *testing.T) { + for _, managed := range []bool{false, true} { + t.Run(fmt.Sprintf("managed=%v", managed), func(t *testing.T) { + target := happyFake() + base := target.Dynamic + clock := &virtualClock{} + engine := New(testConfig(), testProject(t), target, Options{Out: &bytes.Buffer{}, Sleep: clock.sleep, Now: clock.now, Wait: func(_ context.Context, d time.Duration) error { clock.sleep(d); return nil }}) + transient := engine.names().TransientContainer("web") + names := map[string]string{"OLD1": engine.names().Container("web", 1), "NEW1": transient} + undrained, drained, removed := false, false, false + healthyAt, publishedAt := -1, -1 + target.Dynamic = func(command string) (transport.Result, bool) { + switch { + case strings.Contains(command, "onebox.snapshot.yml"): + snapshot := engineProject + if managed { + snapshot = strings.Replace(snapshot, " workloads:", " proxy: {managed: true}\n workloads:", 1) + snapshot = strings.Replace(snapshot, " role: Application", " role: Application\n routes: [{hostname: example.com, port: 7500}]", 1) + } + return transport.Result{Stdout: snapshot}, true + case strings.Contains(command, "readlink"): + return transport.Result{Stdout: "releases/" + engineTestPreviousReleaseID + "\n"}, true + case command == "docker ps -a --format '{{.Names}}'": + // Slot 2 is held by an unrelated stopped container, not a running replica. + return transport.Result{Stdout: names["OLD1"] + "\n" + engine.names().Container("web", 2) + "\n" + names["NEW1"]}, true + case strings.Contains(command, "docker ps -a --filter label=onebox.app="): + if removed { + return transport.Result{}, true + } + return transport.Result{Stdout: "NEW1|web|" + names["NEW1"] + "|running"}, true + case strings.Contains(command, "docker ps --filter label=onebox.app=") && strings.Contains(command, "label=onebox.release="): + if !strings.Contains(command, "label=onebox.release="+q(engineTestPreviousReleaseID)) { + t.Fatalf("wrong predecessor release: %s", command) + } + return transport.Result{Stdout: "OLD1|web"}, true + case strings.Contains(command, "docker ps --filter label=onebox.app="): + health := "unhealthy" + if undrained { + health = "healthy" + } + listing := "OLD1|web|" + engineTestPreviousReleaseID + "||Up 1 hour (" + health + ")" + if !removed { + listing += "\nNEW1|web|" + engineTestDeployReleaseID + "||Up 1 hour (healthy)" + } + return transport.Result{Stdout: listing}, true + case strings.Contains(command, "docker ps -aq") && strings.Contains(command, "label=onebox.app="): + if strings.Contains(command, "label=onebox.release="+q(engineTestPreviousReleaseID)) { + return transport.Result{Stdout: "OLD1"}, true + } + if removed { + return transport.Result{}, true + } + return transport.Result{Stdout: "NEW1"}, true + case strings.Contains(command, "docker ps ") && strings.Contains(command, "compose.service='web'"): + if strings.Contains(command, "onebox.release=") { + return transport.Result{Stdout: "OLD1"}, true + } + ids := "OLD1" + if !removed { + ids += "\nNEW1" + } + return transport.Result{Stdout: ids}, true + case strings.Contains(command, "docker rename "): + fs := strings.Fields(command[strings.Index(command, "docker rename ")+len("docker rename "):]) + names[fs[0]] = fs[1] + return transport.Result{}, true + case strings.Contains(command, "{{.Name}}"): + return transport.Result{Stdout: "/" + names[strings.Fields(command)[len(strings.Fields(command))-1]]}, true + case strings.Contains(command, "docker exec OLD1 rm -f "+app.DrainFile): + undrained = true + return transport.Result{}, true + case strings.Contains(command, "Health.Status") && strings.HasSuffix(command, "OLD1"): + if undrained { + healthyAt = len(target.Commands) - 1 + return transport.Result{Stdout: "healthy"}, true + } + return transport.Result{Stdout: "unhealthy"}, true + case strings.Contains(command, "Health.Status") && strings.HasSuffix(command, "NEW1"): + health := "healthy" + if drained { + health = "unhealthy" + } + return transport.Result{Stdout: health}, true + case strings.Contains(command, "docker exec NEW1 touch "): + if healthyAt < 0 || managed && publishedAt < 0 { + t.Fatalf("NEW1 drained before OLD1 admitted: %s", command) + } + drained = true + return transport.Result{}, true + case strings.Contains(command, "docker rm NEW1") || strings.Contains(command, "docker rm -f NEW1"): + removed = true + return transport.Result{}, true + case strings.Contains(command, "IPAddress"): + address := "172.20.0.6 " + if strings.HasSuffix(command, "'OLD1'") || strings.HasSuffix(command, " OLD1") { + address = "172.20.0.5 " + } + return transport.Result{Stdout: address}, true + case strings.Contains(command, "/onebox.yml"): + if undrained { + publishedAt = len(target.Commands) - 1 + } + return transport.Result{Stdout: "http:\n services:\n " + engine.names().ProxyServiceFor("web", 0) + ":\n loadBalancer:\n servers:\n - url: http://172.20.0.5:7500\n"}, true + } + return base(command) + } + seedInterruptedRecoveryState(t, engine) + err := engine.recoverInterrupted(context.Background(), recoveryRequest{ + InterruptedID: engineTestDeployReleaseID, PreviousID: engineTestPreviousReleaseID, + TerminalState: release.StateAborted, GateCovered: true, Phase: "abort", Journal: recoveryWriter(engine), + }) + commands := strings.Join(target.Commands, "\n") + rename := strings.Index(commands, "docker rename NEW1 "+engine.names().Container("web", 3)) + undrain := strings.Index(commands, "docker exec OLD1 rm -f "+app.DrainFile) + touch := strings.Index(commands, "docker exec NEW1 touch ") + stop := strings.Index(commands, fmt.Sprintf("docker stop -t %d NEW1", engine.Spec.Workloads["web"].StopGraceSeconds())) + rm := strings.Index(commands, "docker rm NEW1") + if err != nil || rename < 0 || undrain < rename || touch < undrain || stop < touch || rm < stop || healthyAt < 0 || managed && publishedAt < 0 { + t.Fatalf("recovery must admit predecessor before retiring interrupted replica: %v\n%s", err, commands) + } + if strings.Contains(commands, "--scale web=") || strings.Contains(commands, "docker rm -f NEW1") || strings.Contains(commands, "docker exec OLD1 touch ") { + t.Fatalf("adoption must preserve OLD1 and retire NEW1 normally:\n%s", commands) + } + }) + } +} + +func TestRecoverySurgesWhenOnlyInterruptedNewcomerServes(t *testing.T) { + target := happyFake() + base := target.Dynamic + clock := &virtualClock{} + engine := New(testConfig(), testProject(t), target, Options{Out: &bytes.Buffer{}, Sleep: clock.sleep, Now: clock.now, Wait: func(_ context.Context, d time.Duration) error { clock.sleep(d); return nil }}) + transient := engine.names().TransientContainer("web") + names := map[string]string{"NEW1": transient} + surged, drained, removed := false, false, false + healthyAt, publishedAt := -1, -1 + target.Dynamic = func(command string) (transport.Result, bool) { + switch { + case strings.Contains(command, "onebox.snapshot.yml"): + snapshot := strings.Replace(engineProject, " workloads:", " proxy: {managed: true}\n workloads:", 1) + snapshot = strings.Replace(snapshot, " role: Application", " role: Application\n routes: [{hostname: example.com, port: 7500}]", 1) + return transport.Result{Stdout: snapshot}, true + case strings.Contains(command, "readlink"): + return transport.Result{Stdout: "releases/" + engineTestPreviousReleaseID + "\n"}, true + case command == "docker ps -a --format '{{.Names}}'": + var occupied []string + if !removed { + occupied = append(occupied, names["NEW1"]) + } + if surged { + occupied = append(occupied, names["RESTORED1"]) + } + return transport.Result{Stdout: strings.Join(occupied, "\n")}, true + case strings.Contains(command, "docker ps -a --filter label=onebox.app="): + if removed { + return transport.Result{}, true + } + return transport.Result{Stdout: "NEW1|web|" + names["NEW1"] + "|running"}, true + case strings.Contains(command, "docker ps --filter label=onebox.app=") && strings.Contains(command, "label=onebox.release="): + if surged { + return transport.Result{Stdout: "RESTORED1|web"}, true + } + return transport.Result{}, true // no predecessor replica to undrain or adopt + case strings.Contains(command, "docker ps --filter label=onebox.app="): + var listing []string + if !removed { + health := "healthy" + if drained { + health = "unhealthy" + } + listing = append(listing, "NEW1|web|"+engineTestDeployReleaseID+"||Up 1 hour ("+health+")") + } + if surged { + listing = append(listing, "RESTORED1|web|"+engineTestPreviousReleaseID+"||Up 1 minute (healthy)") + } + return transport.Result{Stdout: strings.Join(listing, "\n")}, true + case strings.Contains(command, "docker ps -aq") && strings.Contains(command, "label=onebox.app="): + if !removed && strings.Contains(command, "label=onebox.release="+q(engineTestDeployReleaseID)) { + return transport.Result{Stdout: "NEW1"}, true + } + if surged && strings.Contains(command, "label=onebox.release="+q(engineTestPreviousReleaseID)) { + return transport.Result{Stdout: "RESTORED1"}, true + } + return transport.Result{}, true + case strings.Contains(command, "status=exited"): + return transport.Result{}, true + case strings.Contains(command, "docker ps ") && strings.Contains(command, "compose.service='web'"): + if strings.Contains(command, "onebox.release=") { + if surged { + return transport.Result{Stdout: "RESTORED1"}, true + } + return transport.Result{}, true + } + var ids []string + if !removed { + ids = append(ids, "NEW1") + } + if surged { + ids = append(ids, "RESTORED1") + } + return transport.Result{Stdout: strings.Join(ids, "\n")}, true + case strings.Contains(command, "--scale web=2 web"): + if names["NEW1"] != engine.names().Container("web", 1) { + t.Fatal("surge must free the transient name without removing NEW1") + } + surged = true + names["RESTORED1"] = "sample-web-2" + return transport.Result{}, true + case strings.Contains(command, "docker rename "): + fields := strings.Fields(command[strings.Index(command, "docker rename ")+len("docker rename "):]) + for id, name := range names { + if id != fields[0] && name == fields[1] && !(id == "NEW1" && removed) { + t.Fatalf("rename collided with serving container %s: %s", id, command) + } + } + names[fields[0]] = fields[1] + return transport.Result{}, true + case strings.Contains(command, "{{.Name}}"): + fields := strings.Fields(command) + return transport.Result{Stdout: "/" + names[fields[len(fields)-1]]}, true + case strings.Contains(command, "Health.Status") && strings.HasSuffix(command, "RESTORED1"): + healthyAt = len(target.Commands) - 1 + return transport.Result{Stdout: "healthy"}, true + case strings.Contains(command, "Health.Status") && strings.HasSuffix(command, "NEW1"): + if drained { + return transport.Result{Stdout: "unhealthy"}, true + } + return transport.Result{Stdout: "healthy"}, true + case strings.Contains(command, "docker exec NEW1 touch "): + if healthyAt < 0 || publishedAt < 0 { + t.Fatal("NEW1 drained before the predecessor surge was admitted") + } + drained = true + return transport.Result{}, true + case strings.Contains(command, "docker rm -f NEW1"): + t.Fatal("serving interrupted replica must be retired by the roll") + case strings.Contains(command, "docker rm NEW1"): + removed = true + return transport.Result{}, true + case strings.Contains(command, "IPAddress"): + if strings.HasSuffix(command, "'RESTORED1'") || strings.HasSuffix(command, " RESTORED1") { + return transport.Result{Stdout: "172.20.0.5 "}, true + } + return transport.Result{Stdout: "172.20.0.6 "}, true + case strings.Contains(command, "/onebox.yml"): + body := "http:\n services:\n " + engine.names().ProxyServiceFor("web", 0) + ":\n loadBalancer:\n servers:\n" + if surged { + publishedAt = len(target.Commands) - 1 + body += " - url: http://172.20.0.5:7500\n" + } + if !drained { + body += " - url: http://172.20.0.6:7500\n" + } + return transport.Result{Stdout: body}, true + } + return base(command) + } + seedInterruptedRecoveryState(t, engine) + err := engine.recoverInterrupted(context.Background(), recoveryRequest{ + InterruptedID: engineTestDeployReleaseID, PreviousID: engineTestPreviousReleaseID, + TerminalState: release.StateAborted, GateCovered: true, Phase: "abort", Journal: recoveryWriter(engine), + }) + commands := strings.Join(target.Commands, "\n") + rename := strings.Index(commands, "docker rename NEW1 "+engine.names().Container("web", 1)) + surge := strings.Index(commands, "--scale web=2 web") + transientRename := strings.Index(commands, "docker rename RESTORED1 "+transient) + touch := strings.Index(commands, "docker exec NEW1 touch ") + stop := strings.Index(commands, fmt.Sprintf("docker stop -t %d NEW1", engine.Spec.Workloads["web"].StopGraceSeconds())) + rm := strings.Index(commands, "docker rm NEW1") + if err != nil || rename < 0 || surge < rename || transientRename < surge || touch < transientRename || stop < touch || rm < stop || healthyAt < 0 || publishedAt < 0 { + t.Fatalf("recovery must surge and admit the predecessor before retiring the only serving replica: %v\n%s", err, commands) + } + if strings.Count(commands, "--scale web=") != 1 || names["RESTORED1"] != engine.names().Container("web", 1) || !removed { + t.Fatalf("recovery must leave one restored replica in slot 1:\n%s", commands) + } +} + +func TestRecoveryRollsPredecessorBeforeRemovingServingContainer(t *testing.T) { + target := happyFake() + base := target.Dynamic + target.Dynamic = func(command string) (transport.Result, bool) { + switch { + case strings.Contains(command, "readlink"): + return transport.Result{Stdout: "releases/" + engineTestPreviousReleaseID + "\n"}, true + } + return base(command) + } + engine := New(testConfig(), testProject(t), target, Options{Out: &bytes.Buffer{}, Sleep: noSleep}) + seedInterruptedRecoveryState(t, engine) + err := engine.recoverInterrupted(context.Background(), recoveryRequest{ + InterruptedID: engineTestDeployReleaseID, PreviousID: engineTestPreviousReleaseID, + TerminalState: release.StateAborted, GateCovered: true, Phase: "abort", Journal: recoveryWriter(engine), + }) + if err != nil { + t.Fatal(err) + } + commands := strings.Join(target.Commands, "\n") + restore := strings.Index(commands, "--scale web=") + if restore < 0 { + t.Fatalf("no predecessor roll:\n%s", commands) + } + for _, command := range target.Commands { + if strings.Contains(command, "OLD1") && (strings.Contains(command, "docker stop") || strings.Contains(command, "docker rm")) { + if strings.Index(commands, command) < restore { + t.Fatalf("serving container removed before restore: %s", command) + } + } + } + touch := strings.Index(commands, "docker exec OLD1 touch ") + stop := strings.Index(commands, fmt.Sprintf("docker stop -t %d OLD1", engine.Spec.Workloads["web"].StopGraceSeconds())) + removed := strings.Index(commands, "docker rm OLD1") + if touch < restore || stop < touch || removed < stop || strings.Contains(commands, "docker rm -f NEW1") || strings.Contains(commands, "docker rm NEW1") || strings.Contains(commands, fmt.Sprintf("docker stop -t %d NEW1", engine.Spec.Workloads["web"].StopGraceSeconds())) { + t.Fatalf("predecessor must retire OLD1 after surge with NEW1 surviving:\n%s", commands) + } + +} + +func TestRecoveryWithoutPredecessorRemovesEverythingFirst(t *testing.T) { + target := happyFake() + base := target.Dynamic + target.Dynamic = func(command string) (transport.Result, bool) { + if strings.Contains(command, "docker ps -a --filter label=onebox.app=") { + return transport.Result{Stdout: "NEW1|web|demo-web-1|running\nSTALE1|stale|demo-stale-1|exited"}, true + } + if strings.Contains(command, "docker ps -aq") && strings.Contains(command, "label=onebox.release='"+engineTestDeployReleaseID+"'") { + return transport.Result{}, true + } + return base(command) + } + engine := New(testConfig(), testProject(t), target, Options{Out: &bytes.Buffer{}, Sleep: noSleep}) + seedInterruptedRecoveryState(t, engine) + // This is a first-release recovery, so its checkpoint has no predecessor. + checkpoint, err := release.NewActivationCheckpoint(engineTestDeployReleaseID, "", release.ActivationPrepared, time.Now()) + if err != nil { + t.Fatal(err) + } + if err := engine.writeActivationCheckpoint(context.Background(), checkpoint); err != nil { + t.Fatal(err) + } + if err := engine.recoverInterrupted(context.Background(), recoveryRequest{ + InterruptedID: engineTestDeployReleaseID, TerminalState: release.StateAborted, + GateCovered: true, Phase: "abort", Journal: recoveryWriter(engine), + }); err != nil { + t.Fatal(err) + } + commands := strings.Join(target.Commands, "\n") + for _, id := range []string{"NEW1", "STALE1"} { + removed := strings.Index(commands, "docker rm -f "+id) + if removed < 0 || removed > strings.Index(commands, "readlink") { + t.Fatalf("container %s not removed before finalization:\n%s", id, commands) + } + } +} + +func TestRecoveryNonServingSelection(t *testing.T) { + for _, tc := range []struct { + name, service, state string + transient, noPrevious, keep bool + }{ + {name: "running rolling", service: "web", state: "running", keep: true}, + {name: "stopped rolling", service: "web", state: "exited"}, + {name: "paused rolling", service: "web", state: "paused"}, + {name: "restarting rolling", service: "web", state: "restarting"}, + {name: "recreate", service: "worker", state: "running"}, + {name: "missing workload", service: "stale", state: "running"}, + {name: "unlabelled", state: "running"}, + {name: "transient", service: "web", state: "running", transient: true, keep: true}, + {name: "first release", service: "web", state: "running", noPrevious: true}, + } { + t.Run(tc.name, func(t *testing.T) { + target := &transport.Fake{} + engine := New(testConfig(), testProject(t), target, Options{Out: &bytes.Buffer{}, Sleep: noSleep}) + name := "sample-web-1" + if tc.transient { + name = engine.names().TransientContainer(tc.service) + } + target.Dynamic = func(command string) (transport.Result, bool) { + if strings.Contains(command, "docker ps -a --filter") { + return transport.Result{Stdout: "NEW1|" + tc.service + "|" + name + "|" + tc.state}, true + } + return transport.Result{}, false + } + previous := engine + if tc.noPrevious { + previous = nil + } + if err := engine.removeInterruptedNonServing(context.Background(), engineTestDeployReleaseID, previous); err != nil { + t.Fatal(err) + } + if tc.transient && tc.keep && !strings.Contains(strings.Join(target.Commands, "\n"), "docker rename NEW1 "+engine.names().Container(tc.service, 1)) { + t.Fatal("transient was not renamed to the free slot") + } + removed := strings.Contains(strings.Join(target.Commands, "\n"), "docker rm -f NEW1") + if removed == tc.keep { + t.Fatalf("removed = %v, want keep = %v", removed, tc.keep) + } + }) + } +} + +func TestRecoveryNonServingRejectsInvalidListingBeforeRemoval(t *testing.T) { + for _, listing := range []string{"NEW1|stale|name|exited\nBAD;id|web|name|running", "NEW1|stale|name|exited\n|web|name|running", "NEW1|stale|name|exited\nbroken", "NEW1|stale|name|exited\n BAD1|web|name|running"} { + t.Run(listing, func(t *testing.T) { + target := &transport.Fake{Dynamic: func(command string) (transport.Result, bool) { + if strings.Contains(command, "docker ps -a ") { + return transport.Result{Stdout: listing}, true + } + return transport.Result{}, false + }} + engine := New(testConfig(), testProject(t), target, Options{Out: &bytes.Buffer{}, Sleep: noSleep}) + if err := engine.removeInterruptedNonServing(context.Background(), engineTestDeployReleaseID, engine); err == nil { + t.Fatal("invalid listing accepted") + } + for _, command := range target.Commands { + if strings.Contains(command, "docker rm") { + t.Fatalf("mutated before validating listing: %s", command) + } + } + }) + } +} + +func TestRecoveryFakePreservesInterruptedOwnershipUntilRemoval(t *testing.T) { + for _, removal := range []string{"docker rm OLD1", "if docker inspect OLD1 >/dev/null 2>&1; then docker rm -f OLD1; fi"} { + t.Run(removal, func(t *testing.T) { + target := happyFake() + engine := New(testConfig(), testProject(t), target, Options{Out: &bytes.Buffer{}, Sleep: noSleep}) + ctx := context.Background() + listing := "docker ps -a --filter label=onebox.app=" + q(engine.Spec.Name) + + " --filter label=onebox.release=" + q(engineTestDeployReleaseID) + + " --format " + q(`{{.ID}}|{{.Label "com.docker.compose.service"}}|{{.Names}}|{{.State}}`) + if _, err := target.Run(ctx, listing); err != nil { + t.Fatal(err) + } + // Simulate predecessor creation without retiring the interrupted replica. + if _, err := target.Run(ctx, "docker compose up -d --scale web=2"); err != nil { + t.Fatal(err) + } + ids, err := engine.exactReleaseContainerIDs(ctx, engineTestDeployReleaseID) + if err != nil || strings.Join(ids, ",") != "OLD1" { + t.Fatalf("surviving interrupted ids = %v, %v", ids, err) + } + if err := engine.assertNoReleaseContainers(ctx, engineTestDeployReleaseID); err == nil { + t.Fatal("postcondition accepted surviving interrupted replica") + } + ids, err = engine.newcomerIDs(ctx, "web", engineTestPreviousReleaseID, "") + if err != nil || strings.Join(ids, ",") != "NEW1" { + t.Fatalf("predecessor ids = %v, %v", ids, err) + } + if _, err := target.Run(ctx, removal); err != nil { + t.Fatal(err) + } + if err := engine.assertNoReleaseContainers(ctx, engineTestDeployReleaseID); err != nil { + t.Fatal(err) + } + result, err := target.Run(ctx, listing) + if err != nil || strings.TrimSpace(result.Stdout) != "" { + t.Fatalf("removed interrupted replica reappeared: %+v, %v", result, err) + } + ids, err = engine.newcomerIDs(ctx, "web", engineTestPreviousReleaseID, "") + if err != nil || strings.Join(ids, ",") != "NEW1" { + t.Fatalf("removal changed predecessor ids = %v, %v", ids, err) + } + }) + } +} + +func TestRestoreReleaseRolesRollsInterruptedOwnedMatchingRevision(t *testing.T) { + target := happyFake() + revision := "sha256:" + strings.Repeat("a", 64) + base := target.Dynamic + target.Dynamic = func(command string) (transport.Result, bool) { + switch { + case strings.Contains(command, "/compose.yaml") && strings.HasPrefix(command, "cat "): + return transport.Result{Stdout: "services:\n web:\n labels:\n onebox.workload-revision: " + revision + "\n"}, true + case strings.Contains(command, "docker ps --filter label=onebox.app="): + return transport.Result{Stdout: "OLD1|web|" + engineTestDeployReleaseID + "|" + revision + "|Up 1 hour (healthy)\n"}, true + } + return base(command) + } + cfg := testConfig() + for name := range cfg.Workloads { + if name != "web" { + delete(cfg.Workloads, name) + } + } + cfg.Deployment.Order = []string{"web"} + engine := New(cfg, testProject(t), target, Options{Out: &bytes.Buffer{}, Sleep: noSleep}) + if err := engine.restoreReleaseRoles(context.Background(), engine, engineTestPreviousReleaseID, engineTestDeployReleaseID); err != nil { + t.Fatal(err) + } + commands := strings.Join(target.Commands, "\n") + last := -1 + for _, marker := range []string{"--scale web=", "docker exec OLD1 touch", "docker stop -t 30 OLD1", "docker rm OLD1"} { + at := strings.Index(commands, marker) + if at < 0 || at <= last { + t.Fatalf("missing or out-of-order %q:\n%s", marker, commands) + } + last = at + } +} + +func TestRecoveryTransientRenameFailuresKeepContainer(t *testing.T) { + for _, failure := range []string{"names read", "names exit", "rename"} { + t.Run(failure, func(t *testing.T) { + target := &transport.Fake{} + engine := New(testConfig(), testProject(t), target, Options{Out: &bytes.Buffer{}}) + target.Dynamic = func(command string) (transport.Result, bool) { + switch { + case strings.Contains(command, "docker ps -a --filter"): + return transport.Result{Stdout: "NEW1|web|" + engine.names().TransientContainer("web") + "|running"}, true + case command == "docker ps -a --format '{{.Names}}'": + if failure == "names exit" { + return transport.Result{ExitCode: 1}, true + } + // Docker can report multiple names on one line; both slots are occupied. + return transport.Result{Stdout: engine.names().Container("web", 1) + "," + engine.names().Container("web", 2)}, true + case strings.Contains(command, "docker rename"): + if !strings.Contains(command, "docker rename NEW1 "+engine.names().Container("web", 3)) { + t.Fatalf("did not skip occupied names: %s", command) + } + return transport.Result{ExitCode: 1}, true + } + return transport.Result{}, false + } + target.Err = func(command string) error { + if failure == "names read" && command == "docker ps -a --format '{{.Names}}'" { + return errors.New("read failed") + } + return nil + } + if err := engine.removeInterruptedNonServing(t.Context(), engineTestDeployReleaseID, engine); err == nil { + t.Fatal("rename failure accepted") + } + if strings.Contains(strings.Join(target.Commands, "\n"), "docker rm") { + t.Fatal("removed serving container on rename failure") + } + }) + } +} + +func TestRecoveryUndrainPredecessorSelectionAndBestEffort(t *testing.T) { + target := &transport.Fake{} + var output bytes.Buffer + engine := New(testConfig(), testProject(t), target, Options{Out: &output}) + target.Dynamic = func(command string) (transport.Result, bool) { + if strings.HasPrefix(command, "docker ps ") { + if !strings.Contains(command, "label=onebox.release="+q(engineTestPreviousReleaseID)) || !strings.Contains(command, "--filter status=running") || !strings.Contains(command, "label=onebox.app="+q(engine.Spec.Name)) { + t.Fatalf("unsafe predecessor selection: %s", command) + } + return transport.Result{Stdout: "OLD1|web\nOLD2|web\nWORK1|worker\nSTALE1|stale"}, true + } + if strings.Contains(command, "docker exec OLD1") { + return transport.Result{ExitCode: 1}, true + } + return transport.Result{}, false + } + if err := engine.undrainPredecessorReplicas(t.Context(), engine, engineTestPreviousReleaseID); err != nil { + t.Fatal(err) + } + commands := strings.Join(target.Commands, "\n") + for _, id := range []string{"OLD1", "OLD2"} { + if !strings.Contains(commands, "docker exec "+id+" rm -f "+app.DrainFile) { + t.Fatalf("best-effort missed %s: %s", id, commands) + } + } + if !strings.Contains(output.String(), "undrain predecessor replica OLD1") { + t.Fatalf("remote exit was not logged: %s", output.String()) + } + for _, id := range []string{"WORK1", "STALE1"} { + if strings.Contains(commands, "docker exec "+id) { + t.Fatalf("undrained non-rolling predecessor: %s", commands) + } + } +} + +func TestRecoveryUndrainRejectsFailedOrInvalidListing(t *testing.T) { + for _, result := range []transport.Result{{ExitCode: 1}, {Stdout: "OLD1|web\nBAD;id|web"}, {Stdout: "OLD1|web\nbroken"}} { + target := &transport.Fake{Dynamic: func(command string) (transport.Result, bool) { + if strings.HasPrefix(command, "docker ps ") { + return result, true + } + return transport.Result{}, false + }} + engine := New(testConfig(), testProject(t), target, Options{Out: &bytes.Buffer{}}) + if err := engine.undrainPredecessorReplicas(t.Context(), engine, engineTestPreviousReleaseID); err == nil { + t.Fatal("invalid predecessor listing accepted") + } + if strings.Contains(strings.Join(target.Commands, "\n"), "docker exec") { + t.Fatal("mutated before validating predecessor listing") + } + } +} + +func TestRecoveryUndrainAbortsOnMutationError(t *testing.T) { + transportErr := errors.New("undrain transport failed") + for _, fenced := range []bool{false, true} { + name := "transport" + want := transportErr + if fenced { + name, want = "fenced", ErrFenced + } + t.Run(name, func(t *testing.T) { + for _, recovery := range []bool{false, true} { + name := "helper" + if recovery { + name = "recovery" + } + t.Run(name, func(t *testing.T) { + target := happyFake() + base := target.Dynamic + target.Dynamic = func(command string) (transport.Result, bool) { + switch { + case strings.HasPrefix(command, "docker ps --filter label=onebox.app="): + return transport.Result{Stdout: "OLD1|web\nOLD2|web"}, true + case strings.Contains(command, "docker ps -a --filter label=onebox.app="): + return transport.Result{}, true + case strings.Contains(command, "docker exec OLD1 rm -f "+app.DrainFile) && fenced: + return transport.Result{ExitCode: 97, Stderr: "onebox-fenced"}, true + } + return base(command) + } + target.Err = func(command string) error { + if !fenced && strings.Contains(command, "docker exec OLD1 rm -f "+app.DrainFile) { + return transportErr + } + return nil + } + engine := New(testConfig(), testProject(t), target, Options{Out: &bytes.Buffer{}, Sleep: noSleep}) + seedInterruptedRecoveryState(t, engine) + engine.fenceVal = "current-owner" + target.Commands = nil + var err error + if recovery { + err = engine.recoverInterrupted(t.Context(), recoveryRequest{ + InterruptedID: engineTestDeployReleaseID, PreviousID: engineTestPreviousReleaseID, + TerminalState: release.StateAborted, GateCovered: true, Phase: "abort", Journal: recoveryWriter(engine), + }) + var incomplete *RecoveryIncompleteError + if !errors.As(err, &incomplete) || incomplete.Phase != "restore" || !errors.Is(incomplete.Err, want) { + t.Errorf("recovery must retain undrain error in restore phase: %v", err) + } + } else { + err = engine.undrainPredecessorReplicas(t.Context(), engine, engineTestPreviousReleaseID) + if !errors.Is(err, want) { + t.Errorf("undrain error = %v, want unchanged %v", err, want) + } + } + commands := strings.Join(target.Commands, "\n") + if !strings.Contains(commands, "docker exec OLD1 rm -f "+app.DrainFile) { + t.Fatalf("undrain error was not exercised: %s", commands) + } + for _, forbidden := range []string{"docker exec OLD2", "--scale web=", "docker exec OLD1 touch ", "docker stop ", "docker rm ", "docker ps -aq"} { + if strings.Contains(commands, forbidden) { + t.Errorf("continued recovery after undrain error (%s): %s", forbidden, commands) + } + } + }) + } + }) + } +} diff --git a/internal/engine/retired_workloads.go b/internal/engine/retired_workloads.go index 44cea10e..3a63de98 100644 --- a/internal/engine/retired_workloads.go +++ b/internal/engine/retired_workloads.go @@ -40,7 +40,7 @@ func (e *Engine) retireRemovedWorkloads(ctx context.Context, predecessor string) _, pollEvery := old.ReadyTiming() for _, id := range ids { e.logf("retire removed workload %s (%s)", name, id[:min(12, len(id))]) - if err := previous.retireContainer(ctx, old, id, pollEvery); err != nil { + if err := previous.retireContainer(ctx, name, old, id, pollEvery); err != nil { return fmt.Errorf("retire removed workload %s: %w", name, err) } } diff --git a/internal/engine/roll.go b/internal/engine/roll.go index 3b14351a..20b3b252 100644 --- a/internal/engine/roll.go +++ b/internal/engine/roll.go @@ -162,11 +162,17 @@ func (e *Engine) rollRoleForRelease(ctx context.Context, roleName, remoteCompose if len(news) >= desired && len(olds) == 0 { if len(news) == desired { + if err := e.admitNewcomers(ctx, roleName, role, news, within, pollEvery); err != nil { + return err + } break } + if err := e.admitNewcomers(ctx, roleName, role, news[:desired], within, pollEvery); err != nil { + return err + } // surplus new replicas (count reduced) — drain one down to desired update(completed, "retiring surplus replica") - if err := e.retireContainer(ctx, role, news[desired], pollEvery); err != nil { + if err := e.retireContainer(ctx, roleName, role, news[desired], pollEvery); err != nil { return err } continue @@ -234,12 +240,23 @@ func (e *Engine) rollRoleForRelease(ctx context.Context, roleName, remoteCompose cleanupErr := e.mutateChecked(ctx, "remove unhealthy newcomer "+newID, "docker rm -f "+newID) return errors.Join(fmt.Errorf("role %s: new container never became healthy: %w", roleName, err), cleanupErr) } + if e.routeGated(role) { + if err := e.awaitRouting(ctx, roleName, role, newID, true, pollEvery); err != nil { + e.logf("join failed for %s — removing new container, existing keep serving", roleName) + cleanupErr := e.mutateChecked(ctx, "remove unrouted newcomer "+newID, "docker rm -f "+newID) + return errors.Join(fmt.Errorf("role %s: %w", roleName, err), cleanupErr) + } + } + news = append(news, newID) } // retire one old, freeing its slot for the newcomer just added if len(olds) > 0 { + if err := e.admitNewcomers(ctx, roleName, role, news, within, pollEvery); err != nil { + return err + } update(completed, "retiring old replica") - if err := e.retireContainer(ctx, role, olds[0], pollEvery); err != nil { + if err := e.retireContainer(ctx, roleName, role, olds[0], pollEvery); err != nil { return err } } @@ -257,10 +274,25 @@ func (e *Engine) rollRoleForRelease(ctx context.Context, roleName, remoteCompose return nil } +// admitNewcomers checks replacements without removing adopted containers on failure. +func (e *Engine) admitNewcomers(ctx context.Context, workload string, role app.Workload, ids []string, within, pollEvery time.Duration) error { + for _, id := range ids { + if err := e.waitHealth(ctx, id, "healthy", within, pollEvery); err != nil { + return err + } + if e.routeGated(role) { + if err := e.awaitRouting(ctx, workload, role, id, true, pollEvery); err != nil { + return err + } + } + } + return nil +} + // retireContainer drains one container out of rotation (poison its health so the // proxy drops it BEFORE any signal), waits the drain, optionally bleeds long // connections, then stops and removes it. -func (e *Engine) retireContainer(ctx context.Context, role app.Workload, id string, pollEvery time.Duration) error { +func (e *Engine) retireContainer(ctx context.Context, workload string, role app.Workload, id string, pollEvery time.Duration) error { e.sleepBusy("converge (proxy observes the newcomer)", e.Opts.ConvergeBuffer) // Poisoning health is what takes the container out of rotation before any @@ -297,6 +329,11 @@ func (e *Engine) retireContainer(ctx context.Context, role app.Workload, id stri if err := e.waitHealth(ctx, id, "unhealthy", drainBudget, pollEvery); err != nil { e.warnf("container never reported unhealthy (%v); proceeding after buffer", err) } + if e.Spec.Proxy.Managed { + if err := e.awaitRouting(ctx, workload, role, id, false, pollEvery); err != nil { + return err + } + } e.sleepBusy("converge (proxy drops the drained container)", e.Opts.ConvergeBuffer) if wait := role.DrainWait(); role.Drain != nil && role.Drain.Wait != "" && wait > 0 { diff --git a/internal/engine/roll_test.go b/internal/engine/roll_test.go index 28879af4..83d1e1be 100644 --- a/internal/engine/roll_test.go +++ b/internal/engine/roll_test.go @@ -140,6 +140,76 @@ func rollFake() *transport.Fake { func noSleep(time.Duration) {} +func TestRollStopsWhenAdmittedNewcomerFailsLater(t *testing.T) { + f := replicaFake(3, []string{"OLD1", "OLD2", "OLD3"}, map[string]string{ + "OLD1": "sample-web-1", "OLD2": "sample-web-2", "OLD3": "sample-web-3", + }, false) + base := f.Dynamic + f.Dynamic = func(cmd string) (transport.Result, bool) { + if strings.Contains(cmd, "Health.Status") && strings.HasSuffix(cmd, "NEW1") && + strings.Contains(strings.Join(f.Commands, "\n"), "docker rm OLD1") { + return transport.Result{Stdout: "unhealthy"}, true + } + return base(cmd) + } + cfg := testConfig() + role := cfg.Workloads["web"] + role.Replicas = 3 + cfg.Workloads["web"] = role + clock := &virtualClock{} + e := New(cfg, testProject(t), f, Options{Out: &bytes.Buffer{}, Sleep: clock.sleep, Now: clock.now, Wait: func(ctx context.Context, d time.Duration) error { clock.sleep(d); return ctx.Err() }}) + err := e.RollRole(context.Background(), "web", "/var/lib/onebox/app/releases/R1/compose.yaml") + commands := strings.Join(f.Commands, "\n") + if err == nil || !strings.Contains(err.Error(), "NEW1") || !strings.Contains(commands, "docker rm OLD1") { + t.Fatalf("roll must reject a previously admitted newcomer that failed under load: %v\n%s", err, commands) + } + for _, cmd := range f.Commands { + for _, id := range []string{"OLD2", "OLD3"} { + if strings.Contains(cmd, id) && (strings.Contains(cmd, "docker exec") || strings.Contains(cmd, "docker stop") || strings.Contains(cmd, "docker rm")) { + t.Fatalf("unhealthy newcomer must leave old replicas serving: %s", cmd) + } + } + } + if strings.Contains(commands, "docker rm -f NEW1") { + t.Fatalf("failed re-admission must not remove an adopted newcomer:\n%s", commands) + } +} + +func TestRollKeepsLastHealthyOldWhenNewcomersFail(t *testing.T) { + f := replicaFake(2, []string{"OLD1", "OLD2", "OLD3"}, map[string]string{ + "OLD1": "sample-web-1", "OLD2": "sample-web-2", "OLD3": "sample-web-3", + }, false) + base := f.Dynamic + f.Dynamic = func(cmd string) (transport.Result, bool) { + if strings.Contains(cmd, "Health.Status") && (strings.HasSuffix(cmd, "NEW1") || strings.HasSuffix(cmd, "NEW2")) && + strings.Contains(strings.Join(f.Commands, "\n"), "docker rm OLD2") { + return transport.Result{Stdout: "unhealthy"}, true + } + return base(cmd) + } + cfg := testConfig() + role := cfg.Workloads["web"] + role.Replicas = 2 + cfg.Workloads["web"] = role + clock := &virtualClock{} + e := New(cfg, testProject(t), f, Options{Out: &bytes.Buffer{}, Sleep: clock.sleep, Now: clock.now, Wait: func(ctx context.Context, d time.Duration) error { clock.sleep(d); return ctx.Err() }}) + err := e.RollRole(context.Background(), "web", "/var/lib/onebox/app/releases/R1/compose.yaml") + commands := strings.Join(f.Commands, "\n") + if err == nil || !strings.Contains(err.Error(), "NEW1") || !strings.Contains(commands, "docker rm OLD2") { + t.Fatalf("reduced-count roll must reject failed newcomers before its last retirement: %v\n%s", err, commands) + } + for _, cmd := range f.Commands { + if strings.Contains(cmd, "OLD3") && (strings.Contains(cmd, "docker exec") || strings.Contains(cmd, "docker stop") || strings.Contains(cmd, "docker rm")) { + t.Fatalf("last healthy old replica must keep serving: %s", cmd) + } + } + for _, id := range []string{"NEW1", "NEW2"} { + if strings.Contains(commands, "docker rm -f "+id) { + t.Fatalf("failed re-admission must not remove adopted newcomers:\n%s", commands) + } + } +} + // A single-replica roll ends with the survivor in stable slot `sample-web-1`, renamed // only AFTER the old is gone (so the name is free), and never carries the // Compose-generated prefix (renamed to `sample-web-new` the instant it's created). @@ -430,3 +500,289 @@ func TestRollRoleReportsNewcomerThatExited(t *testing.T) { t.Fatalf("the old replica must keep serving:\n%s", strings.Join(f.Commands, "\n")) } } + +// routedRollFake publishes each endpoint from its requested document read onward. +func routedRollFake(base *transport.Fake, publishFrom map[string]int) *transport.Fake { + previous := base.Dynamic + reads := 0 + drained := false + base.Dynamic = func(cmd string) (transport.Result, bool) { + if strings.HasPrefix(cmd, "docker exec OLD1 touch ") { + drained = true + } + if strings.Contains(cmd, ".NetworkSettings.Networks") { + fields := strings.Fields(cmd) + id := strings.Trim(fields[len(fields)-1], "'") + if strings.HasPrefix(id, "OLD") { + return transport.Result{Stdout: "10.0.0.100 "}, true + } + return transport.Result{Stdout: "10.0.0." + strings.TrimPrefix(id, "NEW") + " "}, true + } + if strings.HasPrefix(cmd, "cat ") && strings.Contains(cmd, "onebox.yml") { + reads++ + body := "http:\n services:\n onebox_web:\n loadBalancer:\n servers:" + servers := "" + if !drained { + servers += "\n - url: http://10.0.0.100:3000" + } + for id, from := range publishFrom { + if from > 0 && reads >= from { + servers += "\n - url: http://10.0.0." + strings.TrimPrefix(id, "NEW") + ":3000" + } + } + if servers == "" { + servers = " []" + } + return transport.Result{Stdout: body + servers + "\n"}, true + } + return previous(cmd) + } + return base +} + +func routedRollConfig() *app.Resolved { + cfg := testConfig() + cfg.Proxy.Managed = true + role := cfg.Workloads["web"] + role.Routes = []app.Route{{Port: 3000}} + cfg.Workloads["web"] = role + return cfg +} + +func TestRollRetiresOldOnlyAfterNewcomerPublished(t *testing.T) { + f := routedRollFake(rollFake(), map[string]int{"NEW1": 3}) + clock := &virtualClock{} + e := New(routedRollConfig(), testProject(t), f, Options{Out: &bytes.Buffer{}, Sleep: clock.sleep, Now: clock.now, Wait: func(ctx context.Context, d time.Duration) error { clock.sleep(d); return ctx.Err() }}) + if err := e.RollRole(context.Background(), "web", "/var/lib/onebox/app/releases/R1/compose.yaml"); err != nil { + t.Fatal(err) + } + reads := 0 + for _, cmd := range f.Commands { + if strings.HasPrefix(cmd, "cat ") && strings.Contains(cmd, "onebox.yml") { + reads++ + } + if strings.HasPrefix(cmd, "docker exec OLD1") && reads < 3 { + t.Fatalf("retired after only %d route reads", reads) + } + } + if reads < 3 { + t.Fatalf("only %d route reads", reads) + } +} + +func TestRollRemovesNewcomerNeverPublished(t *testing.T) { + f := routedRollFake(rollFake(), map[string]int{"NEW1": 0}) + clock := &virtualClock{} + e := New(routedRollConfig(), testProject(t), f, Options{Out: &bytes.Buffer{}, Sleep: clock.sleep, Now: clock.now, Wait: func(ctx context.Context, d time.Duration) error { clock.sleep(d); return ctx.Err() }}) + err := e.RollRole(context.Background(), "web", "/var/lib/onebox/app/releases/R1/compose.yaml") + if err == nil || !strings.Contains(err.Error(), "did not publish") { + t.Fatalf("error = %v", err) + } + seq := strings.Join(f.Commands, "\n") + if !strings.Contains(seq, "docker rm -f NEW1") || strings.Contains(seq, "docker exec OLD1") { + t.Fatal(seq) + } +} + +func TestRollResumeAdmitsAdoptedNewcomerBeforeRetiring(t *testing.T) { + f := replicaFake(1, []string{"OLD1"}, map[string]string{"OLD1": "web"}, true) + base := f.Dynamic + f.Dynamic = func(cmd string) (transport.Result, bool) { + if strings.Contains(cmd, "State.Health") && strings.Contains(cmd, "NEW1") { + return transport.Result{Stdout: "starting\n"}, true + } + return base(cmd) + } + cfg := testConfig() + cfg.Workloads["web"] = withinMillis(cfg.Workloads["web"], 50) + clock := &virtualClock{} + e := New(cfg, testProject(t), f, Options{Out: &bytes.Buffer{}, Sleep: clock.sleep, Now: clock.now}) + if err := e.RollRole(context.Background(), "web", "/var/lib/onebox/app/releases/R1/compose.yaml"); err == nil { + t.Fatal("expected admission failure") + } + seq := strings.Join(f.Commands, "\n") + if strings.Contains(seq, "docker exec OLD1") || strings.Contains(seq, "docker rm -f NEW1") { + t.Fatal(seq) + } +} + +func TestRollUnmanagedProxyReadsNoRoutes(t *testing.T) { + f := rollFake() + e := New(testConfig(), testProject(t), f, Options{Out: &bytes.Buffer{}, Sleep: noSleep}) + if err := e.RollRole(context.Background(), "web", "/var/lib/onebox/app/releases/R1/compose.yaml"); err != nil { + t.Fatal(err) + } + for _, cmd := range f.Commands { + if strings.Contains(cmd, "onebox.yml") { + t.Fatal(cmd) + } + } +} + +func TestRetireWaitsForWithdrawalBeforeStop(t *testing.T) { + testRetireWithdrawal(t, "delayed") +} + +func TestRetireStopsWhenWithdrawalTimesOut(t *testing.T) { + testRetireWithdrawal(t, "listed") +} + +func TestRetireUnreadableRoutesAreNotWithdrawal(t *testing.T) { + testRetireWithdrawal(t, "unreadable") +} + +func TestRetireUnguardedHealthcheckSkipsWithdrawalGate(t *testing.T) { + testRetireWithdrawal(t, "unguarded") +} + +func testRetireWithdrawal(t *testing.T, mode string) { + t.Helper() + f := rollFake() + base := f.Dynamic + reads := 0 + stopped := false + f.Dynamic = func(cmd string) (transport.Result, bool) { + if mode == "unguarded" && strings.Contains(cmd, "Config.Healthcheck") { + return transport.Result{Stdout: `{"Test":["CMD","/check"]}`}, true + } + if strings.Contains(cmd, ".NetworkSettings.Networks") { + return transport.Result{Stdout: "10.0.0.9 "}, true + } + if strings.HasPrefix(cmd, "cat ") && strings.Contains(cmd, "onebox.yml") { + reads++ + if mode == "unreadable" { + return transport.Result{ExitCode: 1}, true + } + if mode == "listed" || reads <= 2 { + return transport.Result{Stdout: "http:\n services:\n onebox_web:\n loadBalancer:\n servers:\n - url: http://10.0.0.9:3000\n"}, true + } + return transport.Result{Stdout: "http: {}\n"}, true + } + if strings.HasPrefix(cmd, "docker stop ") { + stopped = true + if mode == "delayed" && reads != 3 { + t.Errorf("stop after %d route reads, want 3", reads) + } + } + return base(cmd) + } + clock := &virtualClock{} + cfg := routedRollConfig() + role := cfg.Workloads["web"] + role.Drain = &app.Drain{Wait: "1s", Signal: "USR1"} + e := New(cfg, testProject(t), f, Options{Out: &bytes.Buffer{}, Sleep: clock.sleep, Now: clock.now, Wait: func(ctx context.Context, d time.Duration) error { clock.sleep(d); return ctx.Err() }}) + err := e.retireContainer(context.Background(), "web", role, "OLD1", time.Second) + seq := strings.Join(f.Commands, "\n") + if mode == "listed" || mode == "unreadable" { + if err == nil || !strings.Contains(err.Error(), "still routes") { + t.Fatalf("error = %v, want still routes", err) + } + if stopped || strings.Contains(seq, "docker kill") || strings.Contains(seq, "docker rm") { + t.Fatal(seq) + } + if !strings.Contains(seq, "touch "+app.DrainFile) { + t.Fatal("container was not left draining") + } + } else { + if err != nil { + t.Fatal(err) + } + if !stopped { + t.Fatal("container was not stopped") + } + if mode == "unguarded" && (reads != 0 || clock.elapsed != 2*e.Opts.ConvergeBuffer) { + t.Fatalf("reads=%d elapsed=%s", reads, clock.elapsed) + } + } +} + +func TestRetireOldReplicaOffProxyNetworkIsWithdrawn(t *testing.T) { + testRetireChangedRoutes(t, "off-network") +} +func TestRetireWaitsWhileOldReplicaServesARemovedRoute(t *testing.T) { + testRetireChangedRoutes(t, "removed") +} +func TestRetireWaitsWhileOldReplicaServesAnotherProtocol(t *testing.T) { + testRetireChangedRoutes(t, "tcp") +} +func testRetireChangedRoutes(t *testing.T, mode string) { + t.Helper() + f := rollFake() + previous := f.Dynamic + reads, stops := 0, 0 + f.Dynamic = func(cmd string) (transport.Result, bool) { + if strings.Contains(cmd, ".NetworkSettings.Networks") { + if mode == "off-network" { + return transport.Result{Stdout: " "}, true + } + return transport.Result{Stdout: "10.0.0.100 "}, true + } + if strings.HasPrefix(cmd, "cat ") && strings.Contains(cmd, "onebox.yml") { + reads++ + if mode == "tcp" { + return transport.Result{Stdout: "tcp:\n services:\n removed:\n loadBalancer:\n servers:\n - address: 10.0.0.100:5432\n"}, true + } + if reads <= 2 { + return transport.Result{Stdout: "http:\n services:\n onebox_web:\n loadBalancer:\n servers:\n - url: http://10.0.0.100:3000\n"}, true + } + return transport.Result{Stdout: "{}"}, true + } + if strings.HasPrefix(cmd, "docker stop ") { + stops++ + if mode == "off-network" && reads != 0 || mode == "removed" && reads != 3 { + t.Errorf("stop after %d reads in %s", reads, mode) + } + } + return previous(cmd) + } + cfg := routedRollConfig() + role := cfg.Workloads["web"] + if mode == "removed" { + role.Routes = nil + } + clock := &virtualClock{} + e := New(cfg, testProject(t), f, Options{Out: &bytes.Buffer{}, Sleep: clock.sleep, Now: clock.now, Wait: func(ctx context.Context, d time.Duration) error { clock.sleep(d); return ctx.Err() }}) + err := e.retireContainer(context.Background(), "web", role, "OLD1", time.Second) + if mode == "tcp" { + if err == nil || !strings.Contains(err.Error(), "still routes") || stops != 0 || clock.elapsed < routePublishBudget { + t.Fatalf("err=%v stops=%d elapsed=%s", err, stops, clock.elapsed) + } + } else if err != nil || stops != 1 { + t.Fatalf("err=%v stops=%d", err, stops) + } +} +func TestRollResumeBlocksAdoptedHealthyUnpublishedNewcomer(t *testing.T) { + f := routedRollFake(replicaFake(1, []string{"OLD1"}, map[string]string{"OLD1": "web"}, true), map[string]int{"NEW1": 0}) + clock := &virtualClock{} + e := New(routedRollConfig(), testProject(t), f, Options{Out: &bytes.Buffer{}, Sleep: clock.sleep, Now: clock.now, Wait: func(ctx context.Context, d time.Duration) error { clock.sleep(d); return ctx.Err() }}) + err := e.RollRole(context.Background(), "web", "/var/lib/onebox/app/releases/R1/compose.yaml") + if err == nil || !strings.Contains(err.Error(), "did not publish") { + t.Fatalf("error=%v", err) + } + if strings.Contains(strings.Join(f.Commands, "\n"), "docker exec OLD1") { + t.Fatal("drained old before admission") + } +} +func TestRollWithdrawsOldReplicaInsideFullRoll(t *testing.T) { + f := routedRollFake(rollFake(), map[string]int{"NEW1": 1}) + previous := f.Dynamic + withdrawnRead := false + f.Dynamic = func(cmd string) (transport.Result, bool) { + result, ok := previous(cmd) + if strings.HasPrefix(cmd, "cat ") && strings.Contains(cmd, "onebox.yml") && !strings.Contains(result.Stdout, "10.0.0.100") { + withdrawnRead = true + } + if strings.HasPrefix(cmd, "docker stop ") && strings.Contains(cmd, "OLD1") && !withdrawnRead { + t.Error("stopped before a publication without OLD1") + } + return result, ok + } + clock := &virtualClock{} + e := New(routedRollConfig(), testProject(t), f, Options{Out: &bytes.Buffer{}, Sleep: clock.sleep, Now: clock.now, Wait: func(ctx context.Context, d time.Duration) error { clock.sleep(d); return ctx.Err() }}) + if err := e.RollRole(context.Background(), "web", "/var/lib/onebox/app/releases/R1/compose.yaml"); err != nil { + t.Fatal(err) + } + if !withdrawnRead || !strings.Contains(strings.Join(f.Commands, "\n"), "docker stop -t 30 OLD1") { + t.Fatal("missing withdrawal or stop") + } +} diff --git a/internal/engine/routing.go b/internal/engine/routing.go new file mode 100644 index 00000000..85f2a2bc --- /dev/null +++ b/internal/engine/routing.go @@ -0,0 +1,170 @@ +package engine + +import ( + "context" + "errors" + "fmt" + "io" + "net" + "net/url" + "strings" + "time" + + "github.com/labstack/onebox/internal/app" + "github.com/labstack/onebox/internal/discovery" + "github.com/labstack/onebox/internal/proxy" + "gopkg.in/yaml.v3" +) + +const routePublishBudget = 30 * time.Second + +func (e *Engine) routeGated(role app.Workload) bool { + return e.Spec.Proxy.Managed && len(role.NormalisedRoutes()) > 0 +} + +func routedIn(doc discovery.Dynamic, n app.Names, workload string, role app.Workload, address string) bool { + for i, route := range role.NormalisedRoutes() { + if !routeContains(doc, n.ProxyServiceFor(workload, i), route.Protocol, address) { + return false + } + } + return true +} + +// withdrawnFromAll checks every published service, including routes absent from the new role. +func withdrawnFromAll(doc discovery.Dynamic, address string) bool { + if doc.HTTP != nil { + for service := range doc.HTTP.Services { + if routeContains(doc, service, "http", address) { + return false + } + } + } + if doc.TCP != nil { + for service := range doc.TCP.Services { + if routeContains(doc, service, "tcp", address) { + return false + } + } + } + return true +} + +func routeContains(doc discovery.Dynamic, service, protocol, address string) bool { + ip := net.ParseIP(address) + if ip == nil { + return false + } + if protocol == "tcp" { + if doc.TCP == nil { + return false + } + for _, server := range doc.TCP.Services[service].LoadBalancer.Servers { + host, _, err := net.SplitHostPort(server.Address) + if err == nil && ip.Equal(net.ParseIP(host)) { + return true + } + } + } else { + if doc.HTTP == nil { + return false + } + for _, server := range doc.HTTP.Services[service].LoadBalancer.Servers { + endpoint, err := url.Parse(server.URL) + if err == nil && ip.Equal(net.ParseIP(endpoint.Hostname())) { + return true + } + } + } + return false +} + +func (e *Engine) routeAddress(ctx context.Context, id string) (string, error) { + template := fmt.Sprintf("{{with index .NetworkSettings.Networks %q}}{{.IPAddress}} {{.GlobalIPv6Address}}{{end}}", e.Spec.Proxy.Network) + result, err := e.T.Run(ctx, "docker inspect -f "+q(template)+" "+q(id)) + if err != nil { + return "", err + } + if err := ctx.Err(); err != nil { + return "", err + } + if result.ExitCode != 0 { + return "", fmt.Errorf("inspect route address: exit %d: %s", result.ExitCode, strings.TrimSpace(result.Stderr)) + } + ipv4, ipv6, _ := strings.Cut(strings.TrimRight(result.Stdout, "\r\n"), " ") + address := discovery.EndpointAddress(ipv4, strings.TrimSpace(ipv6)) + return address, nil +} + +func (e *Engine) readPublishedRoutes(ctx context.Context) (discovery.Dynamic, error) { + var doc discovery.Dynamic + result, err := e.T.Run(ctx, "cat "+q(proxy.HostPaths(e.names()).Dynamic+"/onebox.yml")) + if err != nil { + return doc, err + } + if err := ctx.Err(); err != nil { + return doc, err + } + if result.ExitCode != 0 { + return doc, fmt.Errorf("read published routes: exit %d: %s", result.ExitCode, strings.TrimSpace(result.Stderr)) + } + decoder := yaml.NewDecoder(strings.NewReader(result.Stdout)) + var document yaml.Node + if err := decoder.Decode(&document); err != nil { + return doc, fmt.Errorf("read published routes: %w", err) + } + if len(document.Content) != 1 || document.Content[0].Tag == "!!null" { + return doc, fmt.Errorf("read published routes: empty YAML document") + } + var extra yaml.Node + if err := decoder.Decode(&extra); !errors.Is(err, io.EOF) { + if err != nil { + return doc, fmt.Errorf("read published routes: %w", err) + } + return doc, fmt.Errorf("read published routes: expected one YAML document") + } + if err := document.Decode(&doc); err != nil { + return discovery.Dynamic{}, fmt.Errorf("read published routes: %w", err) + } + return doc, nil +} + +func (e *Engine) awaitRouting(ctx context.Context, workload string, role app.Workload, id string, routed bool, pollEvery time.Duration) error { + address, err := e.routeAddress(ctx, id) + if err != nil { + return err + } + if address == "" { + if routed { + return fmt.Errorf("container %s has no address on network %s", id, e.Spec.Proxy.Network) + } + return nil + } + deadline := e.Opts.Now().Add(routePublishBudget) + var lastErr error + for { + if err := ctx.Err(); err != nil { + return err + } + doc, err := e.readPublishedRoutes(ctx) + if err != nil { + lastErr = err + } else if routed && routedIn(doc, e.names(), workload, role, address) || !routed && withdrawnFromAll(doc, address) { + return nil + } + if !e.Opts.Now().Before(deadline) { + message := fmt.Sprintf("discovery did not publish container %s for workload %s; check docker logs %s", id, workload, proxy.DiscoveryContainerName) + if !routed { + message = fmt.Sprintf("discovery still routes to container %s for workload %s; fix discovery and run ob resume", id, workload) + } + if lastErr != nil { + return fmt.Errorf("%s: %w", message, lastErr) + } + return fmt.Errorf("%s", message) + } + interval := min(pollEvery, deadline.Sub(e.Opts.Now())) + if err := e.Opts.Wait(ctx, interval); err != nil { + return err + } + } +} diff --git a/internal/engine/routing_test.go b/internal/engine/routing_test.go new file mode 100644 index 00000000..213989ed --- /dev/null +++ b/internal/engine/routing_test.go @@ -0,0 +1,202 @@ +package engine + +import ( + "context" + "io" + "strings" + "testing" + "time" + + "github.com/labstack/onebox/internal/app" + "github.com/labstack/onebox/internal/discovery" + "github.com/labstack/onebox/internal/transport" + "gopkg.in/yaml.v3" +) + +func routingDocument(t *testing.T, body string) discovery.Dynamic { + t.Helper() + var doc discovery.Dynamic + if err := yaml.Unmarshal([]byte(body), &doc); err != nil { + t.Fatal(err) + } + return doc +} + +func TestRoutedInRequiresEveryExpectedService(t *testing.T) { + role := app.Workload{Routes: []app.Route{{Port: 3000}, {Protocol: "tcp", Port: 5432}}} + doc := routingDocument(t, "http:\n services:\n onebox_web:\n loadBalancer:\n servers:\n - url: http://10.0.0.5:3000\n") + if routedIn(doc, app.Names{}, "web", role, "10.0.0.5") { + t.Fatal("accepted missing TCP service") + } + doc.TCP = &discovery.TCP{Services: map[string]discovery.TCPService{"onebox_web_r1": {LoadBalancer: discovery.TCPLoadBalancer{Servers: []discovery.TCPServer{{Address: "10.0.0.5:5432"}}}}}} + if !routedIn(doc, app.Names{}, "web", role, "10.0.0.5") { + t.Fatal("rejected complete routing") + } + if routedIn(doc, app.Names{}, "other", role, "10.0.0.5") { + t.Fatal("matched unrelated workload") + } +} + +func TestRoutedInMatchesIPv6(t *testing.T) { + doc := routingDocument(t, "http:\n services:\n onebox_web:\n loadBalancer:\n servers:\n - url: http://[fd00::5]:3000\n") + role := app.Workload{Routes: []app.Route{{Port: 3000}}} + if !routedIn(doc, app.Names{}, "web", role, "fd00:0:0:0:0:0:0:5") { + t.Fatal("did not match IPv6") + } +} + +func TestRouteGatedOnlyForManagedProxyWithRoutes(t *testing.T) { + spec := &app.Resolved{Spec: &app.Spec{}} + e := New(spec, nil, &transport.Fake{}, Options{Out: io.Discard}) + role := app.Workload{Routes: []app.Route{{Port: 3000}}} + if e.routeGated(role) { + t.Fatal("gated unmanaged proxy") + } + spec.Proxy.Managed = true + if !e.routeGated(role) { + t.Fatal("did not gate managed routes") + } + if e.routeGated(app.Workload{}) { + t.Fatal("gated workload without routes") + } +} + +func TestAwaitRoutingTreatsReadFailureAsNeither(t *testing.T) { + for _, routed := range []bool{true, false} { + for _, body := range []string{"failed", "http: [", ""} { + clock := &virtualClock{} + f := &transport.Fake{Dynamic: func(cmd string) (transport.Result, bool) { + if strings.HasPrefix(cmd, "docker inspect") { + return transport.Result{Stdout: "10.0.0.5 fd00::5"}, true + } + if strings.HasPrefix(cmd, "cat ") { + if body == "failed" { + return transport.Result{ExitCode: 1, Stderr: "permission denied"}, true + } + return transport.Result{Stdout: body}, true + } + return transport.Result{}, false + }} + e := New(&app.Resolved{Spec: &app.Spec{}}, nil, f, Options{Out: io.Discard, Now: clock.now, Wait: func(ctx context.Context, d time.Duration) error { clock.sleep(d); return ctx.Err() }}) + err := e.awaitRouting(context.Background(), "web", app.Workload{Routes: []app.Route{{Port: 3000}}}, "container", routed, time.Second) + if err == nil { + t.Fatalf("routed=%v body=%q accepted read failure", routed, body) + } + for _, want := range map[bool][]string{true: {"did not publish", "docker logs onebox-discovery"}, false: {"still routes", "ob resume"}}[routed] { + if !strings.Contains(err.Error(), want) { + t.Fatalf("error %q missing %q", err, want) + } + } + if body == "failed" && !strings.Contains(err.Error(), "permission denied") { + t.Fatalf("lost last read error: %v", err) + } + } + } +} + +func TestRouteAddressUsesProxyNetwork(t *testing.T) { + for _, tc := range []struct{ output, want string }{{"10.0.0.5 fd00::5\n", "10.0.0.5"}, {" fd00::5\n", "fd00::5"}, {"", ""}} { + f := &transport.Fake{Dynamic: func(cmd string) (transport.Result, bool) { + if !strings.Contains(cmd, `.NetworkSettings.Networks "ingress"`) { + t.Fatalf("unexpected inspect: %s", cmd) + } + return transport.Result{Stdout: tc.output}, true + }} + spec := &app.Resolved{Spec: &app.Spec{}} + spec.Proxy.Network = "ingress" + e := New(spec, nil, f, Options{Out: io.Discard}) + got, err := e.routeAddress(context.Background(), "container") + if err != nil || got != tc.want { + t.Fatalf("address = %q, %v; want %q", got, err, tc.want) + } + if tc.want == "" { + err = e.awaitRouting(context.Background(), "web", app.Workload{Routes: []app.Route{{Port: 3000}}}, "container", true, time.Second) + if err == nil || !strings.Contains(err.Error(), "ingress") { + t.Fatalf("missing network error: %v", err) + } + } else if err != nil || got != tc.want { + t.Fatalf("address = %q, %v; want %q", got, err, tc.want) + } + } +} + +func TestAwaitRoutingWaitsForPublishedChange(t *testing.T) { + for _, routed := range []bool{true, false} { + clock := &virtualClock{} + reads, inspections := 0, 0 + f := &transport.Fake{Dynamic: func(cmd string) (transport.Result, bool) { + if strings.HasPrefix(cmd, "docker inspect") { + inspections++ + return transport.Result{Stdout: "10.0.0.5 "}, true + } + reads++ + present := reads == 1 + if routed { + present = !present + } + body := "{}" + if present { + body = "http:\n services:\n onebox_web:\n loadBalancer:\n servers:\n - url: http://10.0.0.5:3000\n" + } + return transport.Result{Stdout: body}, true + }} + e := New(&app.Resolved{Spec: &app.Spec{}}, nil, f, Options{Out: io.Discard, Now: clock.now, Wait: func(ctx context.Context, d time.Duration) error { clock.sleep(d); return ctx.Err() }}) + if err := e.awaitRouting(context.Background(), "web", app.Workload{Routes: []app.Route{{Port: 3000}}}, "container", routed, time.Second); err != nil { + t.Fatal(err) + } + if reads != 2 || inspections != 1 { + t.Fatalf("reads=%d inspections=%d", reads, inspections) + } + } +} + +func TestAwaitRoutingRejectsCancelledRead(t *testing.T) { + for _, routed := range []bool{true, false} { + ctx, cancel := context.WithCancel(context.Background()) + f := &transport.Fake{Dynamic: func(cmd string) (transport.Result, bool) { + if strings.HasPrefix(cmd, "docker inspect") { + return transport.Result{Stdout: "10.0.0.5 "}, true + } + cancel() + return transport.Result{Stdout: "{}"}, true + }} + e := New(&app.Resolved{Spec: &app.Spec{}}, nil, f, Options{Out: io.Discard}) + if err := e.awaitRouting(ctx, "web", app.Workload{Routes: []app.Route{{Port: 3000}}}, "container", routed, time.Second); err == nil { + t.Fatal("accepted cancelled read") + } + } +} + +func TestAwaitRoutingRejectsIncompleteYAMLStream(t *testing.T) { + present := "http:\n services:\n onebox_web:\n loadBalancer:\n servers:\n - url: http://10.0.0.5:3000\n" + for _, mode := range []struct { + name string + routed bool + }{{"publish", true}, {"withdraw", false}} { + for _, tc := range []struct{ name, body string }{ + {"explicit empty", "---\n"}, + {"malformed second document", "{}\n---\nhttp: ["}, + {"second document", "{}\n---\n{}\n"}, + {"published then malformed", present + "---\nhttp: ["}, + {"published then second document", present + "---\n{}\n"}, + } { + t.Run(mode.name+"/"+tc.name, func(t *testing.T) { + clock := &virtualClock{} + f := &transport.Fake{Dynamic: func(cmd string) (transport.Result, bool) { + if strings.HasPrefix(cmd, "docker inspect") { + return transport.Result{Stdout: "10.0.0.5 "}, true + } + return transport.Result{Stdout: tc.body}, true + }} + e := New(&app.Resolved{Spec: &app.Spec{}}, nil, f, Options{Out: io.Discard, Now: clock.now, Wait: func(ctx context.Context, d time.Duration) error { clock.sleep(d); return ctx.Err() }}) + err := e.awaitRouting(context.Background(), "web", app.Workload{Routes: []app.Route{{Port: 3000}}}, "container", mode.routed, time.Second) + if err == nil { + t.Fatal("accepted incomplete YAML stream") + } + if !strings.Contains(err.Error(), "read published routes") { + t.Fatalf("missing read error: %v", err) + } + }) + } + } +} diff --git a/internal/engine/services.go b/internal/engine/services.go index b7e5cf3c..d3015aee 100644 --- a/internal/engine/services.go +++ b/internal/engine/services.go @@ -138,8 +138,8 @@ func (e *Engine) applyServices(ctx context.Context, names []string, syncSchedule if err := e.writeServiceFile(ctx, n.ServiceFile(name), doc); err != nil { return fmt.Errorf("service %s: cannot place its runtime: %w", name, err) } - cmd := fmt.Sprintf("docker compose -p %s -f %s up -d --remove-orphans", - q(n.ServiceProject(name)), q(n.ServiceFile(name))) + cmd := fmt.Sprintf("docker compose -p %s -f %s up -d --remove-orphans --timeout %d", + q(n.ServiceProject(name)), q(n.ServiceFile(name)), int(app.ServiceStopGrace.Seconds())) st := e.ui.Step("service "+name, false) res, err := e.mutate(ctx, cmd) if err != nil { @@ -494,7 +494,10 @@ func (e *Engine) removeServices(ctx context.Context, removeVolumes bool) error { // Fail closed. Continuing would delete the schedules, the state // directory and the credential while the service is still live — // and then report a clean teardown. - if res, err := e.mutate(ctx, "docker rm -f "+id); err != nil { + // Stop is best effort; the removal result still decides success. + cmd := fmt.Sprintf("docker stop -t %d %s >/dev/null 2>&1; docker rm -f %s", + int(app.ServiceStopGrace.Seconds()), id, id) + if res, err := e.mutate(ctx, cmd); err != nil { return err } else if res.ExitCode != 0 { return fmt.Errorf("service %s: cannot remove container %s: %s — nothing further was destroyed", diff --git a/internal/engine/services_test.go b/internal/engine/services_test.go new file mode 100644 index 00000000..35a2953c --- /dev/null +++ b/internal/engine/services_test.go @@ -0,0 +1,45 @@ +package engine + +import ( + "bytes" + "context" + "strings" + "testing" + + "github.com/labstack/onebox/internal/transport" +) + +func TestApplyServicesPassesStopTimeout(t *testing.T) { + f := happyFake() + e := New(testConfig(), testProject(t), f, Options{Out: &bytes.Buffer{}, Sleep: noSleep, Environment: "production"}) + if err := e.ApplyServices(context.Background()); err != nil { + t.Fatal(err) + } + commands := strings.Join(f.Commands, "\n") + if !strings.Contains(commands, "up -d --remove-orphans --timeout 120") { + t.Fatalf("service apply has no stop timeout:\n%s", commands) + } +} + +func TestRemoveServicesStopsGracefullyBeforeRemoval(t *testing.T) { + for _, exitCode := range []int{0, 1} { + f := &transport.Fake{Dynamic: func(cmd string) (transport.Result, bool) { + if strings.Contains(cmd, "docker ps -aq") { + return transport.Result{Stdout: "SVC1\n"}, true + } + if strings.Contains(cmd, "docker rm -f SVC1") { + return transport.Result{ExitCode: exitCode, Stderr: "removal failed"}, true + } + return transport.Result{}, true + }} + e := New(testConfig(), testProject(t), f, Options{Out: &bytes.Buffer{}, Sleep: noSleep, Environment: "production"}) + err := e.removeServices(context.Background(), false) + if (err != nil) != (exitCode != 0) { + t.Fatalf("removal exit %d: %v", exitCode, err) + } + commands := strings.Join(f.Commands, "\n") + if !strings.Contains(commands, "docker stop -t 120 SVC1 >/dev/null 2>&1; docker rm -f SVC1") { + t.Fatalf("service was not stopped before removal:\n%s", commands) + } + } +} diff --git a/internal/engine/status_snapshot.go b/internal/engine/status_snapshot.go index 64df81f3..c213239e 100644 --- a/internal/engine/status_snapshot.go +++ b/internal/engine/status_snapshot.go @@ -11,6 +11,7 @@ import ( "github.com/labstack/onebox/internal/app" "github.com/labstack/onebox/internal/buildinfo" "github.com/labstack/onebox/internal/journal" + "github.com/labstack/onebox/internal/proxy" "github.com/labstack/onebox/internal/release" ) @@ -459,7 +460,8 @@ func makeStatusProxy(raw proxyRaw, readComplete []bool, now time.Time, applicati // file that was never read. renderProxy refuses the same comparison. if complete(statusProxyAppliedHashRead) && raw.appliedIssue == "" { status.AppliedConfigHash = raw.applied - if complete(statusProxyLocalHashRead) && raw.localHash != raw.applied { + local, _ := proxy.ParseIdentity(raw.localHash) + if complete(statusProxyLocalHashRead) && len(proxyDriftParts(raw.applied, local)) > 0 { status.ConfigDiverged = true status.Issues = append(status.Issues, "local and applied configuration hashes differ") } diff --git a/internal/engine/status_snapshot_test.go b/internal/engine/status_snapshot_test.go index 7faddba0..248b3bea 100644 --- a/internal/engine/status_snapshot_test.go +++ b/internal/engine/status_snapshot_test.go @@ -10,6 +10,7 @@ import ( "time" "github.com/labstack/onebox/internal/app" + "github.com/labstack/onebox/internal/proxy" "github.com/labstack/onebox/internal/transport" ) @@ -699,3 +700,34 @@ func TestStatusSnapshotCancellationIsHardError(t *testing.T) { t.Fatalf("pre-cancelled snapshot must not issue reads: %v", f.Commands) } } + +func TestStatusSnapshotProxyIdentityDrift(t *testing.T) { + for _, name := range []string{"equal", "compose", "legacy", "garbage"} { + t.Run(name, func(t *testing.T) { + applied := "" + e, _, _, localHash := statusProxyEngine(t, &applied, "", "healthy") + local, _ := proxy.ParseIdentity(localHash) + switch name { + case "equal": + applied = " " + local.Config + " " + local.Compose + " " + case "compose": + applied = (proxy.Identity{Config: local.Config, Compose: "sha256:" + strings.Repeat("c", 64)}).String() + case "legacy": + applied = local.Config + case "garbage": + applied = "deadbeef" + } + snapshot, err := e.StatusSnapshot(context.Background()) + if err != nil { + t.Fatal(err) + } + px := snapshot.Proxy + if px == nil || !px.Complete || px.ConfigDiverged != (name != "equal") || px.Diverged != (name != "equal") { + t.Fatalf("proxy identity status = %#v", px) + } + if px.LocalConfigHash != localHash || px.AppliedConfigHash != strings.TrimSpace(applied) { + t.Fatalf("identity fields = %#v", px) + } + }) + } +} diff --git a/internal/onebox/execute.go b/internal/onebox/execute.go index 6ca54b61..202694b3 100644 --- a/internal/onebox/execute.go +++ b/internal/onebox/execute.go @@ -445,6 +445,7 @@ func (s *Service) executeDeploy( options.MigrationBackupWasRequired = plan.MigrationBackup != nil options.MigrationBackup = migrationBackupAudit options.WorkloadPlans = engineWorkloadPlans(plan.Operation.Steps) + options.ExpectedProxyAction = plan.ProxyAction if request.Approval != nil { options.ApprovalDigest = request.Approval.ApprovalDigest options.ApprovedBy = request.Approval.ApprovedBy diff --git a/internal/onebox/execution_types.go b/internal/onebox/execution_types.go index 8dbac5b3..63d4cea3 100644 --- a/internal/onebox/execution_types.go +++ b/internal/onebox/execution_types.go @@ -57,6 +57,7 @@ type DeployPlan struct { Artifact engine.Artifact `json:"artifact"` Diff string `json:"diff,omitempty"` NoOp bool `json:"no_op"` + ProxyAction string `json:"proxy_action,omitempty"` MigrationBackup *MigrationBackupRequirement `json:"migration_backup,omitempty"` PlanDigest string `json:"plan_digest"` } diff --git a/internal/onebox/plan_deploy.go b/internal/onebox/plan_deploy.go index 3aab0375..1f1439bf 100644 --- a/internal/onebox/plan_deploy.go +++ b/internal/onebox/plan_deploy.go @@ -56,6 +56,10 @@ func (s *Service) PlanDeploy(ctx context.Context, _ PlanDeployRequest) (DeployPl if err != nil { return DeployPlan{}, fmt.Errorf("refresh: %w", err) } + proxyAction, err := e.PlanProxy(ctx) + if err != nil { + return DeployPlan{}, fmt.Errorf("plan proxy: %w", err) + } pins, err := e.PinImages(ctx) if err != nil { return DeployPlan{}, fmt.Errorf("pin images: %w", err) @@ -223,6 +227,7 @@ func (s *Service) PlanDeploy(ctx context.Context, _ PlanDeployRequest) (DeployPl Runner: s.runner, Operation: operation, Artifact: artifact, Diff: diff, NoOp: noOp, MigrationBackup: migrationBackup, + ProxyAction: proxyAction, } if err := plan.Seal(); err != nil { return DeployPlan{}, fmt.Errorf("seal executable deployment plan: %w", err) diff --git a/internal/onebox/proxy_execute_test.go b/internal/onebox/proxy_execute_test.go new file mode 100644 index 00000000..fba63b40 --- /dev/null +++ b/internal/onebox/proxy_execute_test.go @@ -0,0 +1,150 @@ +package onebox + +import ( + "context" + "errors" + "os" + "path/filepath" + "regexp" + "strings" + "testing" + "time" + + "github.com/labstack/onebox/internal/proxy" + "github.com/labstack/onebox/internal/transport" +) + +func TestExecuteBindsSealedProxyAction(t *testing.T) { + testExecuteSealedProxyAction(t, false) +} + +func TestExecuteProceedsWhenSealedProxyActionMatches(t *testing.T) { + testExecuteSealedProxyAction(t, true) +} + +type proxyTransferFailure struct { + *transport.Fake + err error +} + +func (f *proxyTransferFailure) Upload(ctx context.Context, localDir, remoteDir string) error { + if err := f.Fake.Upload(ctx, localDir, remoteDir); err != nil { + return err + } + return f.err +} + +func testExecuteSealedProxyAction(t *testing.T, matching bool) { + t.Helper() + for _, noOp := range []bool{false, true} { + t.Run(map[bool]string{false: "deploy", true: "no-op"}[noOp], func(t *testing.T) { + f := serviceFake() + svc := newTestService(t, f) + svc.now = func() time.Time { return time.Date(2026, 7, 12, 18, 0, 0, 0, time.UTC) } + body, err := os.ReadFile(svc.configPath) + if err != nil { + t.Fatal(err) + } + body = []byte(strings.Replace(string(body), " workloads:", " proxy: {managed: true}\n workloads:", 1)) + if err := os.WriteFile(svc.configPath, body, 0o600); err != nil { + t.Fatal(err) + } + lp, err := svc.loadProject(context.Background(), false) + if err != nil { + t.Fatal(err) + } + staging := t.TempDir() + spec := lp.resolved + id, err := proxy.StageForAppManaged("", staging, spec.Proxy.Image, proxy.DiscoveryImage(svc.runner.Version), + spec.Name, spec.Proxy.Network, spec.Proxy.Entrypoints, spec.HasExactTerminatingTLS(), spec.HasWildcardTerminatingTLS(), spec.Proxy.DNSChallenge) + if err != nil { + t.Fatal(err) + } + proxyCompose, err := os.ReadFile(filepath.Join(staging, "compose.yaml")) + if err != nil { + t.Fatal(err) + } + drift := false + liveCompose, payload := "", "" + base := f.Dynamic + f.Dynamic = func(cmd string) (transport.Result, bool) { + switch { + case strings.Contains(cmd, "readlink") && !noOp: + return transport.Result{}, true + case cmd == "cat '/var/lib/onebox/_host/proxy/config.hash'": + if drift { + return transport.Result{ExitCode: 1}, true + } + return transport.Result{Stdout: id.String()}, true + case strings.HasPrefix(cmd, "cat '/var/lib/onebox/_host/proxy/compose.yaml'"): + return transport.Result{Stdout: string(proxyCompose)}, true + case strings.Contains(cmd, "project='onebox-proxy'") && strings.Contains(cmd, "service=proxy"): + return transport.Result{Stdout: "PX1"}, true + case strings.Contains(cmd, "project='onebox-proxy'") && strings.Contains(cmd, "service=discovery"): + return transport.Result{Stdout: "D1"}, true + case strings.Contains(cmd, "stat -c %i"): + return transport.Result{Stdout: "12"}, true + case strings.HasPrefix(cmd, "docker image inspect -f '{{.Id}}'"): + return transport.Result{Stdout: "sha256:" + strings.Repeat("ef", 32)}, true + case strings.Contains(cmd, "cat ") && strings.Contains(cmd, "compose.yaml") && liveCompose != "": + return transport.Result{Stdout: liveCompose}, true + case strings.Contains(cmd, "find . -type f") && payload != "": + return transport.Result{Stdout: payload}, true + case strings.Contains(cmd, "docker version"): + return transport.Result{Stdout: "27.0.3"}, true + case strings.Contains(cmd, "compose version"): + return transport.Result{Stdout: "2.29.1"}, true + } + return base(cmd) + } + plan, err := svc.PlanDeploy(context.Background(), PlanDeployRequest{}) + if err != nil { + t.Fatal(err) + } + if noOp { + redacted := regexp.MustCompile("redacted:sha256:[0-9a-f]{12}") + liveCompose = redacted.ReplaceAllString(plan.Artifact.RenderedCompose, testSecret) + payload = plan.Operation.Binding.PayloadDigest + plan, err = svc.PlanDeploy(context.Background(), PlanDeployRequest{}) + if err != nil { + t.Fatal(err) + } + } + if plan.NoOp != noOp || plan.ProxyAction != "unchanged" { + t.Fatalf("fixture plan: no-op=%v proxy=%q", plan.NoOp, plan.ProxyAction) + } + approval := approvalForTestPlan(t, &plan) + drift = !matching + if !matching { + f.Err = func(cmd string) error { + if strings.Contains(cmd, "find '/var/lib/onebox/_host/proxy'") || strings.Contains(cmd, "mkdir -p '/var/lib/onebox/_host/proxy/acme'") { + return errors.New("proxy mutation reached without the bound action") + } + return nil + } + } + transferErr := errors.New("release transfer reached") + if matching && !noOp { + svc.connect = func(context.Context, transport.Route) (transport.Transport, error) { + return &proxyTransferFailure{Fake: f, err: transferErr}, nil + } + } + result, err := svc.Execute(context.Background(), ExecuteRequest{Kind: KindDeploy, Plan: &plan, Approval: &approval}) + if !matching { + if err == nil || !strings.Contains(err.Error(), "unchanged") || !strings.Contains(err.Error(), "restart Traefik (configuration changed)") || !strings.Contains(err.Error(), "re-plan") { + t.Fatalf("sealed proxy action must reach the engine on this execution path: %v", err) + } + if len(f.Uploads) != 0 { + t.Fatalf("proxy drift reached release transfer: %v", f.Uploads) + } + } else if noOp { + if err != nil || !result.NoOp || result.Status != OperationStatusNoOp || len(f.Uploads) != 0 { + t.Fatalf("matching no-op plan must succeed without transfer: result=%+v err=%v uploads=%v", result, err, f.Uploads) + } + } else if !errors.Is(err, transferErr) || len(f.Uploads) != 1 || + !strings.Contains(f.Uploads[0], "/releases/"+plan.Operation.ReleaseID) { + t.Fatalf("matching deploy plan must reach release transfer: %v uploads=%v", err, f.Uploads) + } + }) + } +} diff --git a/internal/proxy/identity.go b/internal/proxy/identity.go new file mode 100644 index 00000000..d8ac73c4 --- /dev/null +++ b/internal/proxy/identity.go @@ -0,0 +1,28 @@ +package proxy + +import "strings" + +// Identity records the staged configuration and Compose payload independently. +type Identity struct{ Config, Compose string } + +func (i Identity) String() string { + return i.Config + " " + i.Compose +} + +func ParseIdentity(s string) (Identity, bool) { + fields := strings.Fields(s) + if len(fields) != 2 { + return Identity{}, false + } + for _, digest := range fields { + if len(digest) != len("sha256:")+64 || !strings.HasPrefix(digest, "sha256:") { + return Identity{}, false + } + for _, c := range digest[len("sha256:"):] { + if !(c >= '0' && c <= '9' || c >= 'a' && c <= 'f') { + return Identity{}, false + } + } + } + return Identity{Config: fields[0], Compose: fields[1]}, true +} diff --git a/internal/proxy/identity_test.go b/internal/proxy/identity_test.go new file mode 100644 index 00000000..a929f084 --- /dev/null +++ b/internal/proxy/identity_test.go @@ -0,0 +1,68 @@ +package proxy + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +func TestParseIdentityRoundTrip(t *testing.T) { + id := Identity{Config: "sha256:" + strings.Repeat("a", 64), Compose: "sha256:" + strings.Repeat("b", 64)} + got, ok := ParseIdentity(id.String() + "\n") + if !ok || got != id { + t.Fatalf("got %+v %v", got, ok) + } +} + +func TestParseIdentityRejectsLegacyAndGarbage(t *testing.T) { + a := "sha256:" + strings.Repeat("a", 64) + for _, s := range []string{"", "deadbeef", a, a + " " + a + " " + a, a + " sha256:XYZ", strings.Repeat("a", 64) + " " + a} { + if _, ok := ParseIdentity(s); ok { + t.Errorf("accepted %q", s) + } + } +} + +func TestStageSeparatesComposeFromConfig(t *testing.T) { + for _, custom := range []bool{false, true} { + t.Run(map[bool]string{false: "managed", true: "custom"}[custom], func(t *testing.T) { + cfgDir := "" + if custom { + cfgDir = writeCfg(t, map[string]string{"traefik.yml": testSocketlessStatic}) + } + a, err := StageForApp(cfgDir, t.TempDir(), "", DiscoveryImage("v2026.10.4-alpha"), "onebox", "", nil, false) + if err != nil { + t.Fatal(err) + } + b, err := StageForApp(cfgDir, t.TempDir(), "", DiscoveryImage("v2026.10.5-alpha"), "onebox", "", nil, false) + if err != nil { + t.Fatal(err) + } + if a.Config != b.Config || a.Compose == b.Compose { + t.Fatalf("compose change: %+v -> %+v", a, b) + } + if got, ok := ParseIdentity(a.String()); !ok || got != a { + t.Fatalf("invalid staged identity: %+v", a) + } + }) + } +} + +func TestStageConfigChangeMovesOnlyConfig(t *testing.T) { + cfgDir := writeCfg(t, map[string]string{"traefik.yml": testSocketlessStatic}) + a, err := StageForApp(cfgDir, t.TempDir(), "", DiscoveryImage("v2026.10.4-alpha"), "onebox", "", nil, false) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(cfgDir, "traefik.yml"), []byte(testSocketlessStatic+"log:\n level: DEBUG\n"), 0o600); err != nil { + t.Fatal(err) + } + b, err := StageForApp(cfgDir, t.TempDir(), "", DiscoveryImage("v2026.10.4-alpha"), "onebox", "", nil, false) + if err != nil { + t.Fatal(err) + } + if a.Config == b.Config || a.Compose != b.Compose { + t.Fatalf("config change: %+v -> %+v", a, b) + } +} diff --git a/internal/proxy/proxy.go b/internal/proxy/proxy.go index 3010a784..002475bd 100644 --- a/internal/proxy/proxy.go +++ b/internal/proxy/proxy.go @@ -328,30 +328,30 @@ func renderStaticConfigWithDNS(entrypoints map[string]app.ProxyEntrypoint, dns * return []byte(out.String()) } -func Stage(localCfgDir, stagingDir, image, network string, entrypoints map[string]app.ProxyEntrypoint, requireCertificateResolver bool) (string, error) { +func Stage(localCfgDir, stagingDir, image, network string, entrypoints map[string]app.ProxyEntrypoint, requireCertificateResolver bool) (Identity, error) { return StageForApp(localCfgDir, stagingDir, image, DiscoveryImage(""), "onebox", network, entrypoints, requireCertificateResolver) } -func StageForApp(localCfgDir, stagingDir, image, discoveryImage, application, network string, entrypoints map[string]app.ProxyEntrypoint, requireCertificateResolver bool) (string, error) { +func StageForApp(localCfgDir, stagingDir, image, discoveryImage, application, network string, entrypoints map[string]app.ProxyEntrypoint, requireCertificateResolver bool) (Identity, error) { return StageForAppManaged(localCfgDir, stagingDir, image, discoveryImage, application, network, entrypoints, requireCertificateResolver, false, nil) } // StageForAppManaged stages a proxy with the managed ACME policy required by // the application's routes. Wildcard termination requires DNS-01 either from // dns or from a custom static configuration. -func StageForAppManaged(localCfgDir, stagingDir, image, discoveryImage, application, network string, entrypoints map[string]app.ProxyEntrypoint, requireExactCertificateResolver, requireWildcardTLS bool, dns *app.ProxyDNSChallenge) (string, error) { +func StageForAppManaged(localCfgDir, stagingDir, image, discoveryImage, application, network string, entrypoints map[string]app.ProxyEntrypoint, requireExactCertificateResolver, requireWildcardTLS bool, dns *app.ProxyDNSChallenge) (Identity, error) { if application == "" { application = "onebox" } if localCfgDir == "" { if requireWildcardTLS && dns == nil { - return "", errors.New("managed terminating wildcard routes require an ACME DNS challenge") + return Identity{}, errors.New("managed terminating wildcard routes require an ACME DNS challenge") } return stageDefaultManaged(stagingDir, image, discoveryImage, application, network, entrypoints, dns) } entries, err := os.ReadDir(localCfgDir) if err != nil { - return "", fmt.Errorf("proxy.config: %w", err) + return Identity{}, fmt.Errorf("proxy.config: %w", err) } names := make([]string, 0, len(entries)) staticConfigs := make([]string, 0, 2) @@ -359,20 +359,20 @@ func StageForAppManaged(localCfgDir, stagingDir, image, discoveryImage, applicat hasDynamic := false for _, e := range entries { if e.IsDir() { - return "", fmt.Errorf("proxy.config must be a flat dir of config files; %s/ is a directory", e.Name()) + return Identity{}, fmt.Errorf("proxy.config must be a flat dir of config files; %s/ is a directory", e.Name()) } if e.Name() == "dynamic" { - return "", fmt.Errorf("proxy.config reserves dynamic/ as the generated file-provider mountpoint; rename %s", e.Name()) + return Identity{}, fmt.Errorf("proxy.config reserves dynamic/ as the generated file-provider mountpoint; rename %s", e.Name()) } names = append(names, e.Name()) if e.Name() == "onebox.yml" || e.Name() == "onebox.yaml" { - return "", fmt.Errorf("proxy.config reserves onebox.yml and onebox.yaml for generated socketless discovery output; rename that file") + return Identity{}, fmt.Errorf("proxy.config reserves onebox.yml and onebox.yaml for generated socketless discovery output; rename that file") } if e.Name() == managedDynamicConfigName { - return "", fmt.Errorf("proxy.config reserves %s for Onebox-managed proxy defaults; rename that file", managedDynamicConfigName) + return Identity{}, fmt.Errorf("proxy.config reserves %s for Onebox-managed proxy defaults; rename that file", managedDynamicConfigName) } if e.Name() == "traefik.toml" { - return "", fmt.Errorf("proxy.config does not support traefik.toml as static configuration; use traefik.yml or traefik.yaml, or rename it as a dynamic extension") + return Identity{}, fmt.Errorf("proxy.config does not support traefik.toml as static configuration; use traefik.yml or traefik.yaml, or rename it as a dynamic extension") } if e.Name() == "traefik.yml" || e.Name() == "traefik.yaml" { staticConfigs = append(staticConfigs, e.Name()) @@ -384,27 +384,27 @@ func StageForAppManaged(localCfgDir, stagingDir, image, discoveryImage, applicat } } if len(staticConfigs) > 1 { - return "", fmt.Errorf("proxy.config: %s contains both traefik.yml and traefik.yaml; keep exactly one static configuration file", + return Identity{}, fmt.Errorf("proxy.config: %s contains both traefik.yml and traefik.yaml; keep exactly one static configuration file", localCfgDir) } customStatic := len(staticConfigs) == 1 if dns != nil && customStatic { - return "", errors.New("proxy.dns_challenge cannot be combined with project-owned traefik.yml or traefik.yaml; remove the static file so Onebox can render DNS-01") + return Identity{}, errors.New("proxy.dns_challenge cannot be combined with project-owned traefik.yml or traefik.yaml; remove the static file so Onebox can render DNS-01") } if requireWildcardTLS && !customStatic && dns == nil { - return "", errors.New("managed terminating wildcard routes require proxy.dns_challenge, or a project-owned traefik.yml or traefik.yaml defining ACME DNS-01") + return Identity{}, errors.New("managed terminating wildcard routes require proxy.dns_challenge, or a project-owned traefik.yml or traefik.yaml defining ACME DNS-01") } if !customStatic && !hasDynamic && !(dns != nil && hasEnv) { - return "", fmt.Errorf("proxy.config: %s contains no dynamic .yml, .yaml, or .toml files; "+ + return Identity{}, fmt.Errorf("proxy.config: %s contains no dynamic .yml, .yaml, or .toml files; "+ "remove proxy.config to use only Onebox's managed configuration, or add a dynamic extension", localCfgDir) } if !customStatic && hasEnv && dns == nil { - return "", fmt.Errorf("proxy.config .env requires traefik.yml or traefik.yaml; " + + return Identity{}, fmt.Errorf("proxy.config .env requires traefik.yml or traefik.yaml; " + "Onebox's managed static configuration does not consume custom proxy environment variables") } if dns != nil && !hasEnv { - return "", errors.New("proxy.dns_challenge requires proxy.config/.env containing the DNS provider credentials") + return Identity{}, errors.New("proxy.dns_challenge requires proxy.config/.env containing the DNS provider credentials") } staticName := "traefik.yml" @@ -414,10 +414,10 @@ func StageForAppManaged(localCfgDir, stagingDir, image, discoveryImage, applicat var err error staticBody, err = os.ReadFile(filepath.Join(localCfgDir, staticName)) if err != nil { - return "", err + return Identity{}, err } if err := validateSocketlessStaticConfig(staticBody, requireExactCertificateResolver, requireWildcardTLS); err != nil { - return "", fmt.Errorf("proxy.config %s: %w", staticName, err) + return Identity{}, fmt.Errorf("proxy.config %s: %w", staticName, err) } } else { // Hash and stage the effective configuration, not merely the authored @@ -429,17 +429,17 @@ func StageForAppManaged(localCfgDir, stagingDir, image, discoveryImage, applicat cfgOut := filepath.Join(stagingDir, "config") if err := os.MkdirAll(cfgOut, 0o755); err != nil { - return "", err + return Identity{}, err } // Docker cannot create a nested bind-mount target below the read-only // /etc/traefik mount. The parent source must therefore carry the empty // mountpoint before Compose creates the proxy container. if err := os.MkdirAll(filepath.Join(cfgOut, "dynamic"), 0o755); err != nil { - return "", err + return Identity{}, err } dynamicOut := filepath.Join(stagingDir, "dynamic") if err := os.MkdirAll(dynamicOut, 0o755); err != nil { - return "", err + return Identity{}, err } h := newProxyConfigHash() for _, name := range names { @@ -450,27 +450,27 @@ func StageForAppManaged(localCfgDir, stagingDir, image, discoveryImage, applicat var err error b, err = os.ReadFile(filepath.Join(localCfgDir, name)) if err != nil { - return "", err + return Identity{}, err } } if name != staticName && dynamicConfigExtension(name) { if err := validateDynamicOwnership(name, b); err != nil { - return "", fmt.Errorf("proxy.config %s: %w", name, err) + return Identity{}, fmt.Errorf("proxy.config %s: %w", name, err) } } if name == ".env" { if err := validateSocketlessEnv(b); err != nil { - return "", fmt.Errorf("proxy.config .env: %w", err) + return Identity{}, fmt.Errorf("proxy.config .env: %w", err) } } // .env may hold secrets (e.g. the DNS API token) — mode 600, same // trust boundary as the release payload's env files if err := os.WriteFile(filepath.Join(cfgOut, name), b, 0o600); err != nil { - return "", err + return Identity{}, err } if name != staticName && dynamicConfigExtension(name) { if err := os.WriteFile(filepath.Join(dynamicOut, name), b, 0o600); err != nil { - return "", err + return Identity{}, err } } fmt.Fprintf(h, "%s\x00%d\x00", name, len(b)) @@ -478,49 +478,48 @@ func StageForAppManaged(localCfgDir, stagingDir, image, discoveryImage, applicat } if !customStatic { if err := stageManagedDynamicConfig(dynamicOut, h); err != nil { - return "", err + return Identity{}, err } } compose := RenderComposeForApp(image, discoveryImage, application, network, hasEnv, entrypoints) if err := os.WriteFile(filepath.Join(stagingDir, "compose.yaml"), compose, 0o644); err != nil { - return "", err + return Identity{}, err } - fmt.Fprintf(h, "compose.yaml\x00%d\x00", len(compose)) - h.Write(compose) - return hex.EncodeToString(h.Sum(nil)), nil + composeHash := sha256.Sum256(compose) + return Identity{Config: "sha256:" + hex.EncodeToString(h.Sum(nil)), Compose: "sha256:" + hex.EncodeToString(composeHash[:])}, nil } // stageDefaultManaged writes the configuration Onebox owns, so a project that // declares a domain and nothing else can bootstrap. -func stageDefaultManaged(stagingDir, image, discoveryImage, application, network string, entrypoints map[string]app.ProxyEntrypoint, dns *app.ProxyDNSChallenge) (string, error) { +func stageDefaultManaged(stagingDir, image, discoveryImage, application, network string, entrypoints map[string]app.ProxyEntrypoint, dns *app.ProxyDNSChallenge) (Identity, error) { cfgOut := filepath.Join(stagingDir, "config") if err := os.MkdirAll(cfgOut, 0o755); err != nil { - return "", err + return Identity{}, err } if err := os.MkdirAll(filepath.Join(cfgOut, "dynamic"), 0o755); err != nil { - return "", err + return Identity{}, err } dynamicOut := filepath.Join(stagingDir, "dynamic") if err := os.MkdirAll(dynamicOut, 0o755); err != nil { - return "", err + return Identity{}, err } body := renderStaticConfigWithDNS(entrypoints, dns) if err := os.WriteFile(filepath.Join(cfgOut, "traefik.yml"), body, 0o600); err != nil { - return "", err + return Identity{}, err } h := newProxyConfigHash() fmt.Fprintf(h, "%s\x00%d\x00", "traefik.yml", len(body)) h.Write(body) if err := stageManagedDynamicConfig(dynamicOut, h); err != nil { - return "", err + return Identity{}, err } compose := RenderComposeForApp(image, discoveryImage, application, network, false, entrypoints) if err := os.WriteFile(filepath.Join(stagingDir, "compose.yaml"), compose, 0o644); err != nil { - return "", err + return Identity{}, err } - h.Write(compose) - return "sha256:" + hex.EncodeToString(h.Sum(nil)), nil + composeHash := sha256.Sum256(compose) + return Identity{Config: "sha256:" + hex.EncodeToString(h.Sum(nil)), Compose: "sha256:" + hex.EncodeToString(composeHash[:])}, nil } func stageManagedDynamicConfig(dynamicOut string, h hash.Hash) error { diff --git a/internal/proxy/proxy_test.go b/internal/proxy/proxy_test.go index 03f05b54..381401e5 100644 --- a/internal/proxy/proxy_test.go +++ b/internal/proxy/proxy_test.go @@ -246,7 +246,7 @@ func TestStage(t *testing.T) { if err != nil { t.Fatal(err) } - if hash == "" { + if hash == (Identity{}) { t.Fatal("empty hash") } // compose rendered with defaults; config copied @@ -306,10 +306,7 @@ func TestStageCustomStaticHashMigratesRequiredMountpoint(t *testing.T) { legacy := sha256.New() fmt.Fprintf(legacy, "%s\x00%d\x00", "traefik.yml", len(testSocketlessStatic)) _, _ = legacy.Write([]byte(testSocketlessStatic)) - compose := RenderCompose("", "", false, nil) - fmt.Fprintf(legacy, "compose.yaml\x00%d\x00", len(compose)) - _, _ = legacy.Write(compose) - if got == hex.EncodeToString(legacy.Sum(nil)) { + if got.Config == "sha256:"+hex.EncodeToString(legacy.Sum(nil)) { t.Fatal("the required config/dynamic mountpoint must migrate the applied proxy identity") } } @@ -672,7 +669,7 @@ func TestDefaultStaticConfigIsWrittenWhenNoneIsDeclared(t *testing.T) { if err != nil { t.Fatalf("a project without proxy.config must still bootstrap: %v", err) } - if hash == "" { + if hash == (Identity{}) { t.Fatal("the written configuration must have an identity to compare against") } body, err := os.ReadFile(filepath.Join(staging, "config", "traefik.yml")) diff --git a/site/src/content/docs/explanation/safety-envelope.mdx b/site/src/content/docs/explanation/safety-envelope.mdx index 72bab1cf..407f8a9e 100644 --- a/site/src/content/docs/explanation/safety-envelope.mdx +++ b/site/src/content/docs/explanation/safety-envelope.mdx @@ -80,6 +80,19 @@ connection is an inconvenience, not an incident. A retry first inspects the operation identity and uses resume or status behaviour; it never blindly starts a second deploy. +During a roll, Onebox retires a replica only after discovery has published its +replacement, and stops it only after the proxy has dropped it. This assumes +Traefik loads published routes within the 3s converge buffer. Recovery restores +the previous release before removing the interrupted release's serving containers. + +Traefik restarts when its configuration changes and is recreated when its own +image or ports change. Otherwise, `ob` upgrades and discovery repair do neither. +The deploy plan's `proxy:` line says which action it will take. + +After upgrading to this release, the first deploy or `ob proxy apply` restarts +Traefik once. The first `ob service apply` recreates each managed service once, +and the first deploy does so for PostgreSQL services with declared extensions. + ## What the envelope does not cover :::danger[Bounded claims] diff --git a/site/src/content/docs/guides/add-a-database.mdx b/site/src/content/docs/guides/add-a-database.mdx index bf91e5fe..97704b5c 100644 --- a/site/src/content/docs/guides/add-a-database.mdx +++ b/site/src/content/docs/guides/add-a-database.mdx @@ -29,6 +29,10 @@ between 256 MiB and 1 GiB, but never more than half the limit. A zero limit mean unlimited. This tmpfs uses memory only as PostgreSQL writes to it, and those pages count against the container's memory limit. +Managed services get up to 2 minutes to shut down cleanly. The first +`ob service apply` after upgrading recreates every service to install this +shutdown window; deploy does so for PostgreSQL services with declared extensions. + Declaring it does not start it. Services live outside the release lifecycle, so `ob deploy` wires up connections. For services with declared extensions, deploy also converges their configuration and recreates containers when it changes. diff --git a/site/src/content/docs/status/capabilities.mdx b/site/src/content/docs/status/capabilities.mdx index 34bc08f3..c97b35a0 100644 --- a/site/src/content/docs/status/capabilities.mdx +++ b/site/src/content/docs/status/capabilities.mdx @@ -34,7 +34,9 @@ This page is the reconciliation. Three states: (`ob job run`). - Per-workload reconciliation that retains proven-unchanged healthy workloads, health-gated rolling or recreate replacement for changed or uncertain ones, - traffic drain, verification, versioned releases, retention, rollback. + traffic drain, verification, versioned releases, retention, rollback. With + the managed proxy, a roll retires a replica only after its replacement is + published, and stops it only after the proxy has dropped it. - Locks, fencing, append-only journals, resume, abort, migration gates, status, audit, SOPS secrets, supporting services, proxy operations, notifications. - A canonical, versioned operation graph and one shared Go service for