diff --git a/docs/ARCANE-GIT-SYNC.md b/docs/ARCANE-GIT-SYNC.md index 7ba46f1ec..43fc3c151 100644 --- a/docs/ARCANE-GIT-SYNC.md +++ b/docs/ARCANE-GIT-SYNC.md @@ -518,8 +518,12 @@ carry the git revision they were built from, in three places, and - **Backend** — compiled in. `backend-service/build.rs` reads the `GIT_SHA` build arg and re-exports it as `APIARY_GIT_SHA`; `/healthz` returns it as - the `revision` field (`{"ok":true,"es":true,"revision":"3dca445…"}`) and - the boot log line carries it. `build.rs` also emits + the `revision` field of the liveness body + (`{"live":true,"built":"2026-09-27T01:46:34+00:00","revision":"3dca445…"}`) + and the boot log line carries it. Note the `ok`/`es` pair that used to sit + beside it is gone: #3317 split the endpoint into `/livez` (no Elasticsearch) + and `/readyz` (503 + `reason`), so `revision` now rides on the + liveness side. `build.rs` also emits `cargo:rerun-if-env-changed=GIT_SHA`, which is load-bearing: without it cargo reuses a cached build and the second build of an identical tree keeps the *first* revision, producing the exact failure this fixes. diff --git a/docs/OPERATIONS.md b/docs/OPERATIONS.md index c85a43380..8f8803dff 100644 --- a/docs/OPERATIONS.md +++ b/docs/OPERATIONS.md @@ -249,7 +249,7 @@ appeared there. Split into: ```console $ curl -s http://backend-service:8081/livez -{"live":true,"built":"2026-09-27T01:46:34+00:00"} +{"live":true,"built":"2026-09-27T01:46:34+00:00","revision":"3dca4457f1b2c0d4e5a69788796a5b4c3d2e1f0ab"} $ curl -s http://backend-service:8081/readyz {"ready":true,"cluster":"green","write_blocked":[]} @@ -258,6 +258,12 @@ $ curl -s -o /dev/null -w '%{http_code}\n' http://backend-service:8081/readyz 503 ``` +`/readyz` deliberately carries **no** `revision`: readiness is about whether +this process can do its job, which is a property of the running container and +not of a build, and the two questions have different answers during a rollback. +The revision belongs to the liveness body, which is the one every probe and +every ops script already reads. + **`/livez` is what the container `HEALTHCHECK` curls, and it must stay that way.** A probe that can block on Elasticsearch converts that dependency's outage into a restart loop of a container that was never the problem. The @@ -302,6 +308,102 @@ that blocks for 30s is indistinguishable from the outage it exists to report. - **"Why is the dashboard empty?"** → `/api/v1/source-health` (the per-sensor freshness page). `/readyz` says the backend cannot write; only source-health says whether events are arriving. +- **"Which commit is actually deployed?"** → `revision` on `/livez`, plus + the image label. See below. + +### Which revision is deployed (#3315) + +The failure this exists for is the one `docs/ARCANE-GIT-SYNC.md` names +directly: a content-change redeploy leaves the dashboard **"green, healthy, +running the old code."** A fresh container id, a passing healthcheck and a +recent image build all say the machinery ran — none of them say this code is +what is running. So both dashboard images carry the git revision they were +built from, and `scripts/verify-deploy.sh` compares it against what you +expect. + +**The contract, in one table.** Four surfaces, one value: + +| Surface | Where | Read it with | +|---|---|---| +| Backend health body | `{"live":…,"built":…,"revision":…}` on `/livez` and `/healthz` | `curl -s …/healthz` | +| Backend boot log | `revision=…` on the `apiary-backend listening` line | `docker logs … \| grep listening` | +| Frontend static file | `/build.json` → `{"built":…,"revision":…,"source":…}` | `curl -s …/build.json` | +| Both images | `org.opencontainers.image.revision` (with `.source`, `.created`) | `docker image inspect --format '{{ index .Config.Labels "org.opencontainers.image.revision" }}' apiary-backend:latest` | + +The health body and `build.json` are small enough to read unfiltered, so +there is nothing to parse here and nothing that needs `jq` — which is +deliberately absent from these hosts (`verify-deploy.sh` uses `python3` for +the same reason). + +**Where the value comes from, and what `"unknown"` means.** `GIT_SHA` is a +Docker **build arg**, declared in both Dockerfiles and passed by the stacks' +compose files as `build.args.GIT_SHA: ${GIT_SHA:-}`. The backend compiles it +in (`build.rs` re-exports `GIT_SHA` as `APIARY_GIT_SHA`); the frontend writes +it into `public/build.json` before `npm run build`. Unset is normal and not +an error — you get the literal string `unknown`, never a fabricated revision, +because a plausible-looking wrong answer is worse than no answer. Both tiers +normalize to a bare lowercase hex object name (optional `sha256:` prefix +stripped, 7–64 hex chars) or `unknown`, and both are held to one shared table, +`backend-service/src/revision-corpus.json`, so the two normalizers — different +languages, different CI lanes — cannot drift. + +`build.rs` also emits `cargo:rerun-if-env-changed=GIT_SHA`. That line is +load-bearing, not hygiene: cargo caches a build script's output by its inputs +and an environment variable is not one of them unless declared, so without it +the *second* build of an identical tree silently reports the *first* build's +revision — the exact failure this section exists to end, reproduced by the +stamp itself. + +**Turning it on.** Nothing repo-tracked writes `GIT_SHA`; the stacks' `.env` +files are root-owned and provisioned outside this repository, and there is no +`.git` on the host to ask (`deploy.yml` rsyncs with `--exclude .git/`). So +the first run after this landed correctly reports **not stamped**. From a +clone you are deploying, one line per stack: + +```console +echo "GIT_SHA=$(git rev-parse HEAD)" >> /var/dockge/stacks/honeypot-dashboard-backend/.env +echo "GIT_SHA=$(git rev-parse HEAD)" >> /var/dockge/stacks/honeypot-dashboard/.env +``` + +followed by the `POST /projects/{id}/build` that a content-change sync does +*not* do. CI (`.github/workflows/containers.yml`) passes `github.sha` for the +two Dockerfiles that declare the arg. + +**Running the check.** `scripts/verify-deploy.sh` compares the live revision, +the image label, and an expected revision — which it takes from `origin/main` +in a real clone, or from an argument: + +```console +# On a host with a clone of apiary, the full check: +scripts/verify-deploy.sh --healthz-exec \ + 'docker exec hp-apiary-backend curl -sf http://127.0.0.1:8081/healthz' \ + --image apiary-backend:latest + +# Without one, name the expected revision yourself: +scripts/verify-deploy.sh --behind-days 0 "$(git rev-parse origin/main)" \ + --healthz-exec 'docker exec hp-apiary-backend curl -sf http://127.0.0.1:8081/healthz' + +# The frontend's form: +scripts/verify-deploy.sh --healthz-url http://host:19090/build.json +``` + +Exit codes are the point, and are deliberately three-valued: **0** the +deployed revision matches; **1** a finding (missing, unstamped, malformed, or +stale past `--behind-days`); **2** *could not tell* — an unreadable body or an +image that is not on this host. An unreadable answer must never read as a +stale deploy, which is why 2 is not 1. Add `--image` to check a label against +the running container, which catches the commoner variant where a +`compose up` recreated from an older image than the one whose label you just +read. The lag check measures the age of the *commit*, not of the container, so +it survives a container that has been up since before the merge it is missing +— but it needs a clone to measure against, hence `--behind-days 0` for the +clone-less form. + +`diagnostics.yml` runs it in `--warn-only` mode in its own "Deployed +revision" section, against `$GITHUB_SHA`: the home runner has no clone to +measure against, so the section degrades to "is the running revision the +current tip of `main`" and says so in the report. Its exit 2 is still +reported as **could not tell**, not as a pass. ## Disk space monitoring diff --git a/tests/docs/test_3283_fix.py b/tests/docs/test_3283_fix.py index d7dcf2fd2..2882fb4b2 100644 --- a/tests/docs/test_3283_fix.py +++ b/tests/docs/test_3283_fix.py @@ -51,6 +51,7 @@ import re import shutil import subprocess +import tempfile import threading import urllib.parse from contextlib import contextmanager @@ -230,16 +231,50 @@ def get(self, key, default=None): return computed if computed is not None else default -def run_script(routes: dict, *, env: dict | None = None, expect_ok: bool = True): - """Run the real script against a stub cluster; return (calls, result).""" - with stub_cluster(routes) as server: +def run_script(routes: dict, *, env: dict | None = None, args: list[str] | None = None, + expect_ok: bool = True): + """Run the real script against a stub cluster; return (calls, result). + + `args` is the subcommand, and the default (no subcommand) is the script's + own `generate`. The two are separate processes in production too -- the + stash is written by `shadow` either side of db.pl and read by `generate` + after it -- so a test that wants to see stash state travel has to drive + both. + + The stash gets a fresh private path per call, and that is load-bearing + rather than tidiness. `SHADOW_FILE` unset means the script falls back to + its default, a *fixed* name under `os.tmpdir()` -- one name shared by + every run of this suite, by test_3343_fix.py, and by any other job + running on the same host. composable-templates.js opens that path 0700 + and the file 0600, so the moment anything else owns the directory first, + every run here dies with + + reading /tmp/arkime-composable-shadow/shadow.json: EACCES + + before it asserts anything. That is not hypothetical: this suite goes + green in a fresh container and red on the homeserver runner, where + /tmp/arkime-composable-shadow is long-lived and shared. Which is also + why the fix is SHADOW_FILE and not a chmod -- a suite must not depend on + the permissions of a machine-wide directory it does not own, and it must + not need to be able to write outside its own tmpdir to be run at all. + + The env stays otherwise minimal on purpose (it is a closed set, not + os.environ), so adding SHADOW_FILE here rather than inheriting the + parent environment keeps that property. + """ + with stub_cluster(routes) as server, tempfile.TemporaryDirectory() as stash: # The handler records into server.calls; the computed-route hook needs # the same object, so it is wired here rather than in __init__. handler_calls = server.calls - environ = {"PATH": "/usr/bin:/bin", "ARKIME__elasticsearch": f"http://127.0.0.1:{server.server_address[1]}"} + shadow_file = pathlib.Path(stash) / "shadow.json" + environ = { + "PATH": "/usr/bin:/bin", + "ARKIME__elasticsearch": f"http://127.0.0.1:{server.server_address[1]}", + "SHADOW_FILE": str(shadow_file), + } environ.update(env or {}) result = subprocess.run( - [NODE, str(SCRIPT)], capture_output=True, text=True, env=environ, timeout=60 + [NODE, str(SCRIPT), *(args or [])], capture_output=True, text=True, env=environ, timeout=60 ) calls = list(handler_calls) if expect_ok: @@ -487,6 +522,109 @@ def test_a_cluster_with_no_sessions_indices_still_succeeds(): assert "0 index(es) adopted, 0 already on" in result.stdout, result.stdout +# -------------------------------------------------------------------------- +# The stash this suite uses +# -------------------------------------------------------------------------- + + +@needs_node +def test_the_stash_is_ours_alone_and_never_the_default_tmp_path(tmp_path): + """A run must not consult composable-templates.js's default stash path. + + The regression this pins: run_script passed no SHADOW_FILE, so the script + fell back to its default -- one fixed name under os.tmpdir(), opened 0700 + with the file 0600. On the homeserver runner that directory already + belongs to another user, and all 20-odd tests in this file died on + `EACCES` before reaching an assert, while the same commit was green in a + clean container. A suite whose pass/fail depends on who owns a shared + directory reports the machine, not the code. + + TMPDIR is pointed at a private directory and that default path is + poisoned, which reproduces the runner's condition exactly -- the script + resolves os.tmpdir() to this dir, computes exactly the path that is + unreadable, and would fail on it if it consulted the default at all. + Reproducing the condition rather than fixing the machine is the point: + planting a decoy in the real /tmp would make this test depend on, and + disturb, the same shared state it is asserting independence from. + """ + hostile_root = tmp_path / "hostile-tmp" + decoy_dir = hostile_root / "arkime-composable-shadow" + decoy_dir.mkdir(parents=True) + decoy = decoy_dir / "shadow.json" + decoy.write_text("{}", encoding="utf-8") + # Unreadable rather than merely foreign-owned, so the check does not + # depend on which uid the suite runs as. The directory stays traversable + # so the failure under test is the file open -- which is the one the + # homeserver runner produced. + decoy.chmod(0o000) + + calls, result = run_script(_default_responses(), env={"TMPDIR": str(hostile_root)}) + + assert result.returncode == 0, ( + "run_script consulted the default stash path under TMPDIR:\n" + f"stdout:\n{result.stdout}\nstderr:\n{result.stderr}" + ) + # And the real work still happened -- isolation is not a way to make the + # script a no-op. + assert body_for(calls, "PUT", f"/_ilm/policy/{POLICY}") is not None, ( + "the run did nothing, so a green result here would prove nothing" + ) + + +@needs_node +def test_two_runs_do_not_see_each_others_stash(tmp_path): + """Each run gets its own stash, so no test can inherit another's state. + + The same fixed-default-path bug as the test above, seen from the other + side: even with nothing else on the host, two runs into one path let the + second see what the first stashed. A suite that depends on run order to be + correct is a suite that breaks when pytest-xdist or a reordering lands. + + The cluster here has a composable template covering an Arkime family, so + the `shadow` pass really does stash and delete it -- without that the + stash stays empty and a leak would be invisible to any assertion. The + two runs are `shadow` then `generate`, which is the pairing production + uses: one stashes, a later process restores. + """ + routes = _default_responses() + routes[("GET", "/_index_template")] = (200, {"index_templates": [ + { + "name": "operator-catch-all", + "index_template": {"index_patterns": ["arkime_sessions3-*"]}, + }, + ]}) + routes[("DELETE", "/_index_template/operator-catch-all")] = (200, {"acknowledged": True}) + shared_root = tmp_path / "shared-tmp" + shared_root.mkdir() + env = {"TMPDIR": str(shared_root)} + + shadowed, shadow_result = run_script(routes, env=env, args=["shadow"]) + generated, generate_result = run_script(routes, env=env, args=["generate"]) + + for result in (shadow_result, generate_result): + assert result.returncode == 0, ( + f"a run failed:\nstdout:\n{result.stdout}\nstderr:\n{result.stderr}" + ) + # The precondition, so a green result below cannot be vacuous: the first + # run must actually have stashed and deleted something. Membership rather + # than body_for: a DELETE carries no body, so a body check would read + # "was called" as "was not called". + def called(calls, method, path): + return (method, path) in [(m, p) for m, p, _ in calls] + + assert called(shadowed, "DELETE", "/_index_template/operator-catch-all"), ( + f"nothing was shadowed, so a leak would be invisible here: {mutations(shadowed)}" + ) + # The second run is the tell. A shared stash means it inherits the + # operator-catch-all body and puts it back -- a write the first run's own + # state does not account for, and the clearest possible statement that + # one run's state reached another's. + assert not called(generated, "PUT", "/_index_template/operator-catch-all"), ( + "the generate pass restored a template this run never shadowed, so it " + f"inherited the previous run's stash: {mutations(generated)}" + ) + + # -------------------------------------------------------------------------- # DRY_RUN # --------------------------------------------------------------------------