diff --git a/CHANGELOG.md b/CHANGELOG.md index 39422efc..45577114 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,62 @@ All notable changes to the Lager platform are documented here. For detailed release notes, see [docs.lagerdata.com](https://docs.lagerdata.com). +## [0.31.11] - 2026-07-13 + +### Fixed + +- **`lager box config apply` no longer reports success while applying nothing.** + `apply` runs `start_box.sh` on the box as the login user, and its four + box-config renderers *create* files in `/etc/lager` (`box_config.docker.sh`, + `user_requirements.txt`, `cargo_packages.txt`, `npm_packages.txt`). Creating a + file needs write permission on the **directory**, and `lager install` left + `/etc/lager` owned by the container user only (`33:33`, mode `755`) — so every + render failed with `EACCES`. Renders are soft-failed by design (the container + must always come up), which turned this into a silent no-op: the install steps + read files that were never written, so `apply` skipped them, stamped the + applied-hash, and printed "Applied box config". Every `pip`/`cargo`/`npm` + package, mount, volume, and env var added through `lager box config` was + quietly dropped on any box whose last provisioning step was an install. + `/etc/lager` is now `33:` mode `2775` (setgid), so the + container (owner) and `start_box.sh` (group) can both write it. + +- **`/etc/lager` is no longer world-writable.** `lager update` granted the box + user write access by running `chmod 777` on the directory, which also gave it + to every other local account — enough to replace `box_config.json`, + `saved_nets.json`, or the org secrets. It now gets the same owner/group/setgid + treatment as above, which is what the two writers actually need and nothing + more. This is also why the breakage above was invisible for so long: an + updated box ended up world-writable and rendered fine, while a freshly + *installed* box was owner-only and silently applied nothing — the two paths + disagreed. + +- **A box-config render failure is now loud, and no longer poisons the retry.** + A failed render printed a one-line warning and a raw Python traceback. It now + reports which file could not be written, why, and how to repair it. + `start_box.sh` exits `3` — a new code meaning "the container is up, but the + config on disk was not applied to it" — and `apply` neither stamps the + applied-hash nor rolls back on it. Not stamping the hash is the load-bearing + half: it previously did, so the retry after a fix short-circuited on "Config + unchanged since last apply; skipping restart" and the config could never be + applied. Rolling back is wrong here because the container is healthy and the + fault is environmental — restoring the previous config and re-bouncing would + fail identically. A genuine bounce failure (container possibly down) still + rolls back exactly as before. A failed in-container `pip`/`cargo`/`npm` + install now exits `3` for the same reason — those steps run *after* + `docker run`, so they already reported "container is up but ... may be + incomplete" while exiting `1` and triggering a rollback of a healthy + container. + +- **An uncaught exception in a script run inside the box container no longer + buries the real error.** `lager_excepthook` read `LAGER_HOST_MODULE_FOLDER` + with `os.environ[...]` in order to rewrite host paths out of tracebacks. That + variable is set by `lager python`; anything else that execs a script into the + container leaves it unset, so the exception *handler* raised `KeyError` — + Python then printed "Error in sys.excepthook:" followed by a traceback from + inside lager, and only after that the user's actual exception. A one-line + `ModuleNotFoundError` arrived buried under a stack trace pointing at lager + internals. With no host paths to rewrite, the traceback is now printed + unchanged. ## [0.31.10] - 2026-07-13 ### Added diff --git a/box/lager/box_config/config.py b/box/lager/box_config/config.py index 38cfd61d..4a152294 100644 --- a/box/lager/box_config/config.py +++ b/box/lager/box_config/config.py @@ -884,3 +884,49 @@ def _atomic_write_json(path: str, payload: Any) -> None: if os.path.exists(tmp): os.unlink(tmp) raise + + +class RenderWriteError(Exception): + """A rendered box-config artifact could not be written to disk.""" + + +def _current_user() -> str: + try: + import getpass + + return getpass.getuser() + except Exception: + return f"uid {os.getuid()}" + + +def write_atomic(path: str, body: str) -> None: + """Atomically write `body` to `path`, raising RenderWriteError on failure. + + Shared by the four render_*.py scripts, which start_box.sh runs on the box + HOST as the box's login user. Creating the tmp file needs write permission + on the *directory*, so a /etc/lager that only the container user (uid 33) + can write makes every render fail — silently, because start_box.sh soft-fails + renders to guarantee the container still comes up. The result was a box whose + box_config.json said one thing and whose container ran another, with the CLI + reporting a successful apply. Turn the raw OSError into something an operator + can act on. + """ + _ensure_dir(path) + tmp = f"{path}.tmp" + try: + with open(tmp, "w", encoding="utf-8") as f: + f.write(body) + os.replace(tmp, path) + except OSError as e: + if os.path.exists(tmp): + try: + os.unlink(tmp) + except OSError: + pass + d = os.path.dirname(path) or "." + raise RenderWriteError( + f"cannot write {path}: {e.strerror or e}\n" + f" {d} must be writable by the user start_box.sh runs as " + f"({_current_user()}). Run `lager update --box ` to repair " + f"the permissions on {d}." + ) from e diff --git a/box/lager/box_config/render_cargo_packages.py b/box/lager/box_config/render_cargo_packages.py index ec984f97..0258d435 100644 --- a/box/lager/box_config/render_cargo_packages.py +++ b/box/lager/box_config/render_cargo_packages.py @@ -33,17 +33,10 @@ ) -def _write(path: str, body: str) -> None: - tmp = f"{path}.tmp" - d = os.path.dirname(path) - if d and not os.path.isdir(d): - os.makedirs(d, exist_ok=True) - with open(tmp, "w", encoding="utf-8") as f: - f.write(body) - os.replace(tmp, path) +_write = cfg.write_atomic -def main(argv: list) -> int: +def _render(argv: list) -> int: if len(argv) < 3: print("usage: render_cargo_packages.py ", file=sys.stderr) return 2 @@ -71,5 +64,16 @@ def main(argv: list) -> int: return 0 +def main(argv: list) -> int: + # A write failure here used to surface as a raw traceback that start_box.sh + # swallowed into a one-line warning, leaving the container running the old + # package set while the CLI reported a successful apply. + try: + return _render(argv) + except cfg.RenderWriteError as e: + print(f"[ERROR] {e}", file=sys.stderr) + return 1 + + if __name__ == "__main__": sys.exit(main(sys.argv)) diff --git a/box/lager/box_config/render_docker_args.py b/box/lager/box_config/render_docker_args.py index cc0b4f44..d658409b 100644 --- a/box/lager/box_config/render_docker_args.py +++ b/box/lager/box_config/render_docker_args.py @@ -81,17 +81,10 @@ def _empty_body() -> str: ) -def _atomic_write(path: str, body: str) -> None: - d = os.path.dirname(path) - if d and not os.path.isdir(d): - os.makedirs(d, exist_ok=True) - tmp = f"{path}.tmp" - with open(tmp, "w", encoding="utf-8") as f: - f.write(body) - os.replace(tmp, path) +_atomic_write = cfg.write_atomic -def main(argv: list) -> int: +def _render(argv: list) -> int: if len(argv) < 3: print("usage: render_docker_args.py ", file=sys.stderr) return 2 @@ -120,5 +113,17 @@ def main(argv: list) -> int: return 0 +def main(argv: list) -> int: + # A write failure here used to surface as a raw traceback that start_box.sh + # swallowed into a one-line warning, so the container came up with none of + # the box_config mounts, volumes, or env vars while the CLI reported a + # successful apply. + try: + return _render(argv) + except cfg.RenderWriteError as e: + print(f"[ERROR] {e}", file=sys.stderr) + return 1 + + if __name__ == "__main__": sys.exit(main(sys.argv)) diff --git a/box/lager/box_config/render_npm_packages.py b/box/lager/box_config/render_npm_packages.py index 4a5efcd6..363b2917 100644 --- a/box/lager/box_config/render_npm_packages.py +++ b/box/lager/box_config/render_npm_packages.py @@ -33,17 +33,10 @@ ) -def _write(path: str, body: str) -> None: - tmp = f"{path}.tmp" - d = os.path.dirname(path) - if d and not os.path.isdir(d): - os.makedirs(d, exist_ok=True) - with open(tmp, "w", encoding="utf-8") as f: - f.write(body) - os.replace(tmp, path) +_write = cfg.write_atomic -def main(argv: list) -> int: +def _render(argv: list) -> int: if len(argv) < 3: print("usage: render_npm_packages.py ", file=sys.stderr) return 2 @@ -71,5 +64,16 @@ def main(argv: list) -> int: return 0 +def main(argv: list) -> int: + # A write failure here used to surface as a raw traceback that start_box.sh + # swallowed into a one-line warning, leaving the container running the old + # package set while the CLI reported a successful apply. + try: + return _render(argv) + except cfg.RenderWriteError as e: + print(f"[ERROR] {e}", file=sys.stderr) + return 1 + + if __name__ == "__main__": sys.exit(main(sys.argv)) diff --git a/box/lager/box_config/render_pip_requirements.py b/box/lager/box_config/render_pip_requirements.py index fa6b5bbd..89df3ddc 100644 --- a/box/lager/box_config/render_pip_requirements.py +++ b/box/lager/box_config/render_pip_requirements.py @@ -32,17 +32,10 @@ ) -def _write(path: str, body: str) -> None: - tmp = f"{path}.tmp" - d = os.path.dirname(path) - if d and not os.path.isdir(d): - os.makedirs(d, exist_ok=True) - with open(tmp, "w", encoding="utf-8") as f: - f.write(body) - os.replace(tmp, path) +_write = cfg.write_atomic -def main(argv: list[str]) -> int: +def _render(argv: list[str]) -> int: if len(argv) < 3: print("usage: render_pip_requirements.py ", file=sys.stderr) return 2 @@ -70,5 +63,16 @@ def main(argv: list[str]) -> int: return 0 +def main(argv: list[str]) -> int: + # A write failure here used to surface as a raw traceback that start_box.sh + # swallowed into a one-line warning, leaving the container running the old + # package set while the CLI reported a successful apply. + try: + return _render(argv) + except cfg.RenderWriteError as e: + print(f"[ERROR] {e}", file=sys.stderr) + return 1 + + if __name__ == "__main__": sys.exit(main(sys.argv)) diff --git a/box/lager/core.py b/box/lager/core.py index 17202522..88d36685 100644 --- a/box/lager/core.py +++ b/box/lager/core.py @@ -66,8 +66,22 @@ def lager_excepthook(etype, value, tb): Custom exception printer so users get nice tracebacks without weird temporary filenames and paths """ - module_folder = os.environ['LAGER_HOST_MODULE_FOLDER'] error_lines = traceback.format_exception(etype, value, tb) + + # LAGER_HOST_MODULE_FOLDER is set by `lager python`, which is what the path + # rewriting below is for. Anything else that execs a script into the + # container (an external test runner, `docker exec python3 ...`) leaves it + # unset — and reading it with [] made the excepthook itself raise KeyError. + # Python then prints "Error in sys.excepthook:" plus a traceback from inside + # lager, and only then the user's actual exception, so a one-line + # ModuleNotFoundError arrived buried under a stack trace pointing at lager + # internals. With no host paths to rewrite, print the traceback as-is. + module_folder = os.environ.get('LAGER_HOST_MODULE_FOLDER') + if module_folder is None: + for line in error_lines: + print(line, end='') + return + if module_folder == '/tmp': # We are just running a standalone script regex = re.compile(r'\A(\s*File \")tmp[a-zA-Z0-9_-]+.py(\",.*)') diff --git a/box/start_box.sh b/box/start_box.sh index ddc499ab..1bf0c619 100644 --- a/box/start_box.sh +++ b/box/start_box.sh @@ -127,7 +127,11 @@ if [ ! -d /etc/lager ]; then exit 1 fi -# Initialize saved_nets.json if it doesn't exist (no sudo needed since we own /etc/lager) +# Initialize saved_nets.json if it doesn't exist. No sudo needed: /etc/lager is +# owned by www-data (the container's uid) and group-writable by this user, which +# `lager install` / `lager update` set up. On a box whose /etc/lager predates +# that (owner-only 33:33 755), this and every box_config render below fail — +# run `lager update --box ` to repair the permissions. if [ ! -f /etc/lager/saved_nets.json ]; then echo "Initializing /etc/lager/saved_nets.json..." echo "[]" > /etc/lager/saved_nets.json @@ -296,11 +300,19 @@ NPM_PKGS_FILE="/etc/lager/npm_packages.txt" BOX_CONFIG_MOUNTS=() BOX_CONFIG_ENV=() BOX_CONFIG_HOST_PATHS=() +# Set by any renderer that fails below. The container still comes up (that is a +# hard requirement of this script), but the script exits 3 at the end so the +# caller can tell "box is up AND config applied" from "box is up but the config +# did NOT apply". See the exit at the bottom of this file. +BOX_CONFIG_RENDER_FAILED=0 if [ -f "$BOX_CONFIG_FILE" ]; then echo "Lager Box config detected at $BOX_CONFIG_FILE" if ! python3 "${SCRIPT_DIR}/lager/box_config/render_docker_args.py" \ "$BOX_CONFIG_FILE" "$BOX_CONFIG_ARGS_FILE"; then - echo "[ERROR] box_config.json failed validation; container will start without applying it. Fix and rerun." + echo "[ERROR] Could not render docker args from box_config.json (see above);" + echo " the container will start from the PREVIOUS render if one exists" + echo " (stale mounts/volumes/env), or with none at all if it does not." + BOX_CONFIG_RENDER_FAILED=1 fi # Source whether the renderer succeeded or not — on failure it still # writes empty arrays, which is the right "no box-config" state. Skipping @@ -333,18 +345,25 @@ if [ -f "$BOX_CONFIG_FILE" ]; then unset _p # Render pip_packages from box_config.json into the requirements file that - # the in-container `pip install -r` step (below) reads. Soft-fail: if render - # blows up we leave the existing file alone so the bounce still proceeds. + # the in-container `pip install -r` step (below) reads. A render failure is + # NOT fatal to the container — it always comes up — but it is fatal to the + # apply: the install steps below key off these files, so a failed render + # means the box silently runs the previous package set (or none at all). + # BOX_CONFIG_RENDER_FAILED makes the script exit non-zero at the end so + # `lager box config apply` reports failure instead of stamping the + # applied-hash on a config it never actually applied. if ! python3 "${SCRIPT_DIR}/lager/box_config/render_pip_requirements.py" \ "$BOX_CONFIG_FILE" "$PIP_REQS_FILE" 2>&1; then - echo "[WARNING] Failed to render pip_packages; using existing $PIP_REQS_FILE." + echo "[ERROR] Failed to render pip_packages; using existing $PIP_REQS_FILE." + BOX_CONFIG_RENDER_FAILED=1 fi # Same idea for cargo_packages: render to a flat file the post-run loop # below reads, one crate spec per non-comment line. if ! python3 "${SCRIPT_DIR}/lager/box_config/render_cargo_packages.py" \ "$BOX_CONFIG_FILE" "$CARGO_PKGS_FILE" 2>&1; then - echo "[WARNING] Failed to render cargo_packages; using existing $CARGO_PKGS_FILE." + echo "[ERROR] Failed to render cargo_packages; using existing $CARGO_PKGS_FILE." + BOX_CONFIG_RENDER_FAILED=1 fi # Same for npm_packages — flat file the post-run loop reads. Container must @@ -352,7 +371,8 @@ if [ -f "$BOX_CONFIG_FILE" ]; then # `npm` binary fails the install loop with a clear error. if ! python3 "${SCRIPT_DIR}/lager/box_config/render_npm_packages.py" \ "$BOX_CONFIG_FILE" "$NPM_PKGS_FILE" 2>&1; then - echo "[WARNING] Failed to render npm_packages; using existing $NPM_PKGS_FILE." + echo "[ERROR] Failed to render npm_packages; using existing $NPM_PKGS_FILE." + BOX_CONFIG_RENDER_FAILED=1 fi echo "" fi @@ -422,11 +442,11 @@ fi # # The two `lager-cargo` / `lager-npm-global` named volumes persist # user-installed cargo crates and global npm packages across container -# recreation. Without them, every `lager box update` (or `box config apply`) +# recreation. Without them, every `lager update` (or `box config apply`) # rebuilds the container from scratch and the post-run loops below recompile # `cargo install` packages from source — minutes per update. With them, the # second-and-onward run sees "already installed" and finishes in seconds. -# `lager box update` wipes both volumes alongside `docker rmi lager` whenever +# `lager update` wipes both volumes alongside `docker rmi lager` whenever # the build-hash (Dockerfile + requirements.txt) changes, so a Dockerfile # rustup/node bump can't leave a stale toolchain in the volume. # @@ -531,7 +551,10 @@ if [ -s "$PIP_REQS_FILE" ] && grep -qvE '^[[:space:]]*(#|$)' "$PIP_REQS_FILE"; t echo "[ERROR] User pip install failed (rc=$_rc); container is up but pip_packages may be incomplete." fi unset _rc - exit 1 + # Exit 3, not 1: the container is UP (these installs run after `docker run`), + # the config just didn't fully apply. `lager box config apply` must not roll + # back a healthy container, and must not stamp the applied-hash. + exit 3 fi echo "" fi @@ -583,7 +606,10 @@ if [ -s "$CARGO_PKGS_FILE" ] && grep -qvE '^[[:space:]]*(#|$)' "$CARGO_PKGS_FILE unset _crate_spec _name _ver _args _rc if [ "$_cargo_failed" -ne 0 ]; then echo "[ERROR] One or more cargo crates failed to install; container is up but cargo_packages may be incomplete." - exit 1 + # Exit 3, not 1: the container is UP (these installs run after `docker run`), + # the config just didn't fully apply. `lager box config apply` must not roll + # back a healthy container, and must not stamp the applied-hash. + exit 3 fi echo "" fi @@ -619,7 +645,10 @@ if [ -s "$NPM_PKGS_FILE" ] && grep -qvE '^[[:space:]]*(#|$)' "$NPM_PKGS_FILE"; t unset _npm_spec _rc if [ "$_npm_failed" -ne 0 ]; then echo "[ERROR] One or more npm packages failed to install; container is up but npm_packages may be incomplete." - exit 1 + # Exit 3, not 1: the container is UP (these installs run after `docker run`), + # the config just didn't fully apply. `lager box config apply` must not roll + # back a healthy container, and must not stamp the applied-hash. + exit 3 fi echo "" fi @@ -643,3 +672,22 @@ echo "IMPORTANT: The controller container is NO LONGER NEEDED!" echo " All functionality has been moved to the lager container." echo "" docker ps --filter "name=lager" + +# The container is up either way — that is deliberate, a box must never be left +# down by a bad config. But if any box_config renderer failed above, the config +# on disk is NOT what this container is running, so exit non-zero to say so. +# +# Exit 3 specifically, not 1: a failed `docker run` (exit 1) means the container +# is GONE and `lager box config apply` must roll back to the last good config. +# Here the container is healthy and the fault is environmental (typically +# /etc/lager not writable by this user), so rolling back and re-bouncing would +# fail in exactly the same way. Exit 3 tells apply: report the failure, do not +# stamp the applied-hash, do not roll back. +if [ "$BOX_CONFIG_RENDER_FAILED" = "1" ]; then + echo "" + echo "[ERROR] The container is running, but box_config.json was NOT applied" + echo " (one or more renderers failed above)." + echo " If this is a permissions error on /etc/lager, repair it with:" + echo " lager update --box " + exit 3 +fi diff --git a/cli/__init__.py b/cli/__init__.py index 39c7cfe4..b898dbd2 100644 --- a/cli/__init__.py +++ b/cli/__init__.py @@ -7,4 +7,4 @@ A Command Line Interface for Lager Data """ -__version__ = '0.31.10' +__version__ = '0.31.11' diff --git a/cli/commands/box/config.py b/cli/commands/box/config.py index b68ed3f9..025d0963 100644 --- a/cli/commands/box/config.py +++ b/cli/commands/box/config.py @@ -1104,7 +1104,18 @@ def restart_cmd(ctx: click.Context, box: Optional[str], yes: bool) -> None: ): click.secho("Aborted.", fg="yellow") return - if not _bounce_container(ctx, resolved): + bounce_rc = _bounce_container_rc(ctx, resolved) + if bounce_rc == _BOUNCE_CONFIG_NOT_APPLIED: + # The container did restart, so this is not a restart failure — but it + # came up without the box config, which the operator needs to know. + click.secho( + f"The container restarted on {resolved}, but box_config.json was not " + "applied to it (see the errors above). Repair the box with " + "`lager update --box `, then re-run.", + fg="red", err=True, + ) + ctx.exit(1) + if bounce_rc != _BOUNCE_OK: click.secho( "Container restart failed. SSH into the box and run " "`~/box/start_box.sh` manually to inspect.", @@ -1229,7 +1240,28 @@ def _apply_one( if not _preflight_mounts(ctx, resolved, recursive=recursive_chown): return False - if not _bounce_container(ctx, resolved): + bounce_rc = _bounce_container_rc(ctx, resolved) + + if bounce_rc == _BOUNCE_CONFIG_NOT_APPLIED: + # The container is up, but the renderers couldn't write their output, so + # it is running the OLD config. Don't roll back: the fault is in the + # environment (almost always /etc/lager not writable by the box user), + # not in the new config, so restoring the previous config and bouncing + # again would fail in exactly the same way and buy nothing. Returning + # False here is what matters — it skips SET_APPLIED_HASH below, so a + # retry after the repair actually re-applies instead of reporting + # "Config unchanged since last apply; skipping restart." + click.secho( + f"Box config was NOT applied on {resolved} (the container is up and " + "running the previous config). The applied-hash was left unchanged, " + "so `lager box config apply` will retry this config once the box is " + "repaired — typically with `lager update --box `.", + fg="red", + err=True, + ) + return False + + if bounce_rc != _BOUNCE_OK: # Bounce of the new config failed. The container may be down (start_box.sh # exits between `docker stop` and a successful `docker run` when, e.g., # a mount entry is malformed). Try to restore the last applied snapshot @@ -1627,11 +1659,23 @@ def _wait_for_box_api( _BOUNCE_TIMEOUT_SECONDS = 900 +# start_box.sh exit codes we distinguish. +_BOUNCE_OK = 0 +# The container is UP, but one or more box_config renderers failed, so the +# container is not running the config on disk. Typically /etc/lager is not +# writable by the box's login user (an older `lager install` left it owner-only), +# which is environmental: rolling back and re-bouncing would fail identically. +# Callers must not stamp the applied-hash, and must not roll back. +_BOUNCE_CONFIG_NOT_APPLIED = 3 +# Anything else: the bounce failed and the container may be DOWN. +_BOUNCE_FAILED = 1 -def _bounce_container(ctx: click.Context, resolved_box: str) -> bool: + +def _bounce_container_rc(ctx: click.Context, resolved_box: str) -> int: + """Run start_box.sh on the box. Returns one of the _BOUNCE_* codes.""" click.echo(f"Restarting lager container on {resolved_box} via SSH...") try: - rc, _stdout, _stderr = default_ssh_runner( + rc, stdout, stderr = default_ssh_runner( resolved_box, "cd ~/box && ./start_box.sh", timeout=_BOUNCE_TIMEOUT_SECONDS, @@ -1643,11 +1687,77 @@ def _bounce_container(ctx: click.Context, resolved_box: str) -> bool: "re-run `lager box config apply` once it finishes.", fg="red", err=True, ) - return False + return _BOUNCE_FAILED except FileNotFoundError: click.secho("ssh not found on local machine.", fg="red", err=True) - return False - return rc == 0 + return _BOUNCE_FAILED + + if rc == _BOUNCE_CONFIG_NOT_APPLIED: + # start_box.sh already printed why; relay its diagnosis rather than + # inventing a vaguer one. Its output is captured, so without this the + # operator sees nothing at all. + for line in _render_error_lines(stdout, stderr): + click.secho(f" {line}", fg="red", err=True) + return _BOUNCE_CONFIG_NOT_APPLIED + + return _BOUNCE_OK if rc == 0 else _BOUNCE_FAILED + + +_MAX_ERROR_CONTINUATION_LINES = 2 + + +def _render_error_lines(stdout: Optional[str], stderr: Optional[str]) -> list: + """The [ERROR] blocks start_box.sh emitted, for relaying to the operator. + + An [ERROR] line plus its indented continuation lines — that's the shape both + start_box.sh and the renderers use, and the continuations carry the + actionable part (which directory, which repair command). + + Bounded and de-duplicated on purpose. The renderers write to stderr, and so + does pip, whose indented `WARNING: The script X is installed in ...` lines + would otherwise be swept up as continuations of an unrelated earlier error. + And all four renderers emit the same repair sentence, which is worth saying + once, not four times. + """ + lines = [] + for stream in (stdout, stderr): + continuation = 0 + for raw in (stream or "").splitlines(): + stripped = raw.strip() + if stripped.startswith("[ERROR]"): + lines.append(stripped) + continuation = _MAX_ERROR_CONTINUATION_LINES + elif ( + continuation + and stripped + and raw.startswith((" ", "\t")) + and not stripped.startswith("WARNING:") + ): + lines.append(stripped) + continuation -= 1 + else: + continuation = 0 + + seen = set() + unique = [] + for line in lines: + if line not in seen: + seen.add(line) + unique.append(line) + return unique + + +def _bounce_container(ctx: click.Context, resolved_box: str) -> bool: + """True iff the container came up. + + A config-render failure still leaves the container running, so it counts as + up here. Only `apply` needs the finer distinction — it uses + _bounce_container_rc directly so it can refuse to stamp the applied-hash. + """ + return _bounce_container_rc(ctx, resolved_box) in ( + _BOUNCE_OK, + _BOUNCE_CONFIG_NOT_APPLIED, + ) @box_config.group("mount", help="Manage host-to-container bind mounts.") diff --git a/cli/commands/utility/update.py b/cli/commands/utility/update.py index 9f885699..f2c4d3a3 100644 --- a/cli/commands/utility/update.py +++ b/cli/commands/utility/update.py @@ -789,9 +789,10 @@ def store_build_hash(hash_value): (lagerdata) is not in that group, so a plain `echo > /etc/lager/build-hash` fails with EACCES and — when the write was unchecked — left a stale hash that made every subsequent `lager update` rebuild needlessly. Step 10 has - already `chmod 777`'d /etc/lager, so we write a temp file in that dir and + already made /etc/lager group-writable by this user (owner www-data, + group , setgid), so we write a temp file in that dir and `mv -f` it over the target: unlinking the old file needs only - directory-write (granted by 777), and the replacement is owned by + directory-write, and the replacement is owned by lagerdata, so future runs can overwrite it directly. Same-filesystem mv is an atomic rename. The mktemp template's trailing X's must stay at the end of the name. @@ -1856,10 +1857,28 @@ def _docker_supports_buildkit(): progress.update("Setting up directories...") log('Setting up directories...', nl=False) - # Full paths match the sudoers whitelist in the deployment script; mkdir - # and chmod are idempotent and passwordless via sudoers. + # Full paths match the sudoers whitelist in the deployment script; mkdir, + # chown and chmod are idempotent and passwordless via sudoers. + # + # /etc/lager is written by two users and needs to be writable by both: + # - the container, which runs as www-data (uid 33) -> owner + # - start_box.sh + this command, which run on the host + # as the box's login user -> group + # It used to be `chmod 777`, which granted that by making the directory + # WORLD-writable — any local account could replace box_config.json, + # saved_nets.json or the org secrets. Owner www-data + group + + # setgid gives both writers what they need and nobody else. `chown` before + # `chmod` so the setgid bit isn't applied to a group we're about to change. + # + # Note the 2-argument forms: the sudoers spec is `chown * /etc/lager`, which + # matches exactly one argument before the path, so `chown -R ...` would not + # match it. Only the directory's own group and mode matter here — the + # renderers and store_build_hash replace files via tmp+rename, which needs + # permission on the directory, not on the file being replaced. dirs_result = run_ssh_command_with_output( - 'sudo /bin/mkdir -p /etc/lager && sudo /bin/chmod 777 /etc/lager && ' + 'sudo /bin/mkdir -p /etc/lager && ' + 'sudo /bin/chown 33:"$(id -g)" /etc/lager && ' + 'sudo /bin/chmod 2775 /etc/lager && ' '{ mkdir -p ~/third_party/customer-binaries && ' 'chmod 777 ~/third_party/customer-binaries; } || true', timeout_secs=30 @@ -1871,13 +1890,14 @@ def _docker_supports_buildkit(): log_error('Error: Failed to create /etc/lager on the box') click.echo('This is usually a sudo permission issue. SSH into the box and run:', err=True) click.echo(f' ssh {ssh_host}', err=True) - click.echo(' sudo mkdir -p /etc/lager && sudo chmod 777 /etc/lager', err=True) + click.echo(' sudo mkdir -p /etc/lager && sudo chown 33:"$(id -g)" /etc/lager ' + '&& sudo chmod 2775 /etc/lager', err=True) click.echo('Then run `lager box update` again.', err=True) ctx.exit(1) log_status('OK', 'green') - # Persist the build-inputs hash now that /etc/lager is world-writable (Step - # 10). This is what lets the next run's cache-validity check short-circuit to + # Persist the build-inputs hash now that /etc/lager is group-writable by this + # user (Step 10). This is what lets the next run's cache-validity check short-circuit to # the no-op fast path instead of rebuilding; a stale hash defeats it. Unlike # the old unchecked write, store_build_hash verifies success and we surface a # visible warning on failure rather than silently leaving a stale hash. diff --git a/cli/deployment/scripts/setup_and_deploy_box.sh b/cli/deployment/scripts/setup_and_deploy_box.sh index dfbfaf3d..c84f1ccf 100755 --- a/cli/deployment/scripts/setup_and_deploy_box.sh +++ b/cli/deployment/scripts/setup_and_deploy_box.sh @@ -747,13 +747,21 @@ if ! ssh $SSH_OPTS "${BOX_USER}@${BOX_IP}" "test -d /etc/lager" 2>/dev/null; the TEMP_SCRIPT=$(mktemp) cat > "$TEMP_SCRIPT" << 'SCRIPT_EOF' #!/bin/bash -# Create /etc/lager directory for box configuration -# Docker containers run as www-data (UID 33), so set ownership accordingly +# Create /etc/lager directory for box configuration. +# +# Ownership is shared between two writers, and both need it: +# - the container, which runs as www-data (UID 33) -> owner +# - start_box.sh, which runs on the host as the box's login user, and whose +# box_config renderers create files here -> group +# Owner-only 33:33 755 (the old value) silently broke every renderer: creating +# a file needs write permission on the DIRECTORY, so `lager box config apply` +# reported success while none of the pip/cargo/npm/mount config was applied. +# setgid keeps files created here in the box user's group. if [ ! -d /etc/lager ]; then sudo mkdir -p /etc/lager - sudo chown -R 33:33 /etc/lager - sudo chmod 755 /etc/lager - echo "[OK] /etc/lager directory created (owned by www-data UID 33)" + sudo chown -R 33:"$(id -g)" /etc/lager + sudo chmod 2775 /etc/lager + echo "[OK] /etc/lager directory created (www-data UID 33, group $(id -gn), group-writable)" fi # Initialize saved_nets.json if it doesn't exist @@ -773,10 +781,13 @@ else print_success "/etc/lager directory exists" fi -# Always ensure correct permissions (even if directory existed before) +# Always ensure correct permissions (even if directory existed before). This +# also repairs boxes provisioned by an older CLI, which left /etc/lager +# owner-only and therefore unwritable by start_box.sh's box_config renderers. +# Single-quoted so $(id -g) is evaluated ON THE BOX, not on the operator's host. print_info "Ensuring correct permissions on /etc/lager..." -ssh_t "${BOX_USER}@${BOX_IP}" "sudo chown -R 33:33 /etc/lager && sudo chmod 755 /etc/lager" -print_success "Permissions set correctly (owned by www-data UID 33)" +ssh_t "${BOX_USER}@${BOX_IP}" 'sudo chown -R 33:"$(id -g)" /etc/lager && sudo chmod 2775 /etc/lager' +print_success "Permissions set correctly (www-data UID 33, group-writable by ${BOX_USER})" # Ensure saved_nets.json exists and is writable if ! ssh $SSH_OPTS "${BOX_USER}@${BOX_IP}" "test -f /etc/lager/saved_nets.json" 2>/dev/null; then diff --git a/test/unit/box/test_box_config_cli.py b/test/unit/box/test_box_config_cli.py index d99cf50d..c6b162b4 100644 --- a/test/unit/box/test_box_config_cli.py +++ b/test/unit/box/test_box_config_cli.py @@ -165,7 +165,7 @@ def test_set_applied_hash_called_only_after_api_ready(self): backend = self._backend_for_apply() with _patch_resolve(), \ patch.object(box_config_cli, "_run_box_config_py", side_effect=backend), \ - patch.object(box_config_cli, "_bounce_container", return_value=True), \ + patch.object(box_config_cli, "_bounce_container_rc", return_value=0), \ patch.object(box_config_cli, "_wait_for_box_api", return_value=True) as wait_mock: result = self.runner.invoke( box_config_cli.box_config, @@ -181,7 +181,7 @@ def test_api_timeout_skips_set_applied_hash(self): backend = self._backend_for_apply() with _patch_resolve(), \ patch.object(box_config_cli, "_run_box_config_py", side_effect=backend), \ - patch.object(box_config_cli, "_bounce_container", return_value=True), \ + patch.object(box_config_cli, "_bounce_container_rc", return_value=0), \ patch.object(box_config_cli, "_wait_for_box_api", return_value=False): result = self.runner.invoke( box_config_cli.box_config, @@ -235,6 +235,95 @@ def test_returns_false_at_deadline(self): # Issue 3: rollback on bounce failure # --------------------------------------------------------------------------- +class BounceExitCodeClassification(unittest.TestCase): + """start_box.sh's exit code decides whether the container is up. + + 3 is the "container is up, but box_config did NOT apply" code. It must not + be confused with a failed bounce (container possibly down), because the two + call for opposite recoveries: roll back vs. don't. + """ + + def _bounce(self, rc, stdout="", stderr=""): + with patch.object(box_config_cli, "default_ssh_runner", + return_value=(rc, stdout, stderr)): + return box_config_cli._bounce_container_rc(None, "test-box") + + def test_zero_is_ok(self): + self.assertEqual(self._bounce(0), box_config_cli._BOUNCE_OK) + + def test_three_is_config_not_applied(self): + self.assertEqual( + self._bounce(3), box_config_cli._BOUNCE_CONFIG_NOT_APPLIED + ) + + def test_other_nonzero_is_failure(self): + self.assertEqual(self._bounce(1), box_config_cli._BOUNCE_FAILED) + self.assertEqual(self._bounce(2), box_config_cli._BOUNCE_FAILED) + + def test_container_counts_as_up_when_only_the_config_failed(self): + # The bool wrapper is what `restart`/`repair`/rollback use to ask "is the + # container up?" — a render failure leaves it running, so: yes. + with patch.object(box_config_cli, "_bounce_container_rc", + return_value=box_config_cli._BOUNCE_CONFIG_NOT_APPLIED): + self.assertTrue(box_config_cli._bounce_container(None, "test-box")) + with patch.object(box_config_cli, "_bounce_container_rc", + return_value=box_config_cli._BOUNCE_FAILED): + self.assertFalse(box_config_cli._bounce_container(None, "test-box")) + + def test_relays_the_box_side_error_block(self): + # start_box.sh's output is captured, so unless we relay it the operator + # sees nothing at all about why the config didn't apply. + stdout = ( + "Lager Box container started\n" + "[ERROR] cannot write /etc/lager/user_requirements.txt: Permission denied\n" + " /etc/lager must be writable by the user start_box.sh runs as (lagerdata).\n" + "Box started successfully!\n" + ) + lines = box_config_cli._render_error_lines(stdout, "") + self.assertEqual(len(lines), 2) + self.assertIn("Permission denied", lines[0]) + self.assertIn("/etc/lager must be writable", lines[1]) + self.assertNotIn("Box started successfully!", lines) + + def test_relay_does_not_swallow_unrelated_pip_warnings(self): + """Found on hardware: pip writes to stderr too, and its warnings are + indented, so a naive "indented line after an [ERROR]" rule relayed + pip's script-not-on-PATH noise as if it were part of the failure.""" + stderr = ( + "[ERROR] cannot write /etc/lager/user_requirements.txt: Permission denied\n" + " /etc/lager must be writable by the user start_box.sh runs as (lagerdata).\n" + "Installing collected packages: qrcode, Pillow\n" + " WARNING: The script qr is installed in '/home/www-data/.local/bin' " + "which is not on PATH.\n" + " Consider adding this directory to PATH.\n" + ) + lines = box_config_cli._render_error_lines("", stderr) + self.assertEqual(len(lines), 2) + self.assertFalse( + [ln for ln in lines if "WARNING" in ln or "PATH" in ln], + f"pip noise leaked into the relayed error: {lines}", + ) + + def test_relay_says_the_repair_once_not_once_per_renderer(self): + # All four renderers emit the same repair sentence; the operator needs + # to read it once. + stderr = "".join( + f"[ERROR] cannot write /etc/lager/{name}: Permission denied\n" + " /etc/lager must be writable by the user start_box.sh runs as (lagerdata).\n" + for name in ( + "box_config.docker.sh", + "user_requirements.txt", + "cargo_packages.txt", + "npm_packages.txt", + ) + ) + lines = box_config_cli._render_error_lines("", stderr) + repair = [ln for ln in lines if "must be writable" in ln] + self.assertEqual(len(repair), 1, f"repair line repeated: {lines}") + # But each distinct file still gets named. + self.assertEqual(len([ln for ln in lines if "cannot write" in ln]), 4) + + class ApplyRollback(unittest.TestCase): """Rollback uses direct SSH file ops (not the shim) because the container is dead by the time rollback fires — start_box.sh stopped @@ -272,8 +361,8 @@ def test_rollback_succeeds_when_snapshot_exists(self): patch.object(box_config_cli, "_run_box_config_py", side_effect=backend), \ patch.object(box_config_cli, "default_ssh_runner", side_effect=self._ssh_runner(snapshot_exists=True)), \ - patch.object(box_config_cli, "_bounce_container", - side_effect=[False, True]) as bounce_mock: + patch.object(box_config_cli, "_bounce_container_rc", + side_effect=[1, 0]) as bounce_mock: result = self.runner.invoke( box_config_cli.box_config, ["apply", "--box", "test-box", "--yes"], @@ -293,7 +382,7 @@ def test_no_snapshot_no_rollback(self): patch.object(box_config_cli, "_run_box_config_py", side_effect=backend), \ patch.object(box_config_cli, "default_ssh_runner", side_effect=self._ssh_runner(snapshot_exists=False)), \ - patch.object(box_config_cli, "_bounce_container", return_value=False) as bounce_mock: + patch.object(box_config_cli, "_bounce_container_rc", return_value=1) as bounce_mock: result = self.runner.invoke( box_config_cli.box_config, ["apply", "--box", "test-box", "--yes"], @@ -312,7 +401,7 @@ def test_rollback_bounce_also_fails(self): patch.object(box_config_cli, "_run_box_config_py", side_effect=backend), \ patch.object(box_config_cli, "default_ssh_runner", side_effect=self._ssh_runner(snapshot_exists=True)), \ - patch.object(box_config_cli, "_bounce_container", return_value=False) as bounce_mock: + patch.object(box_config_cli, "_bounce_container_rc", return_value=1) as bounce_mock: result = self.runner.invoke( box_config_cli.box_config, ["apply", "--box", "test-box", "--yes"], @@ -321,6 +410,37 @@ def test_rollback_bounce_also_fails(self): self.assertIn("rollback was not possible", result.output) self.assertEqual(bounce_mock.call_count, 2) + def test_render_failure_does_not_stamp_hash_and_does_not_roll_back(self): + """start_box.sh rc 3: container is UP but the config did not apply. + + This is the shape of the /etc/lager-not-writable bug. Two things must + hold, and neither did before: + - applied-hash is NOT stamped, so the retry after repairing the box + actually re-applies instead of short-circuiting on + "Config unchanged since last apply; skipping restart". + - we do NOT roll back. The container is healthy and the fault is + environmental, so restoring the previous config and re-bouncing + would fail identically and buy nothing. + """ + backend = self._backend() + with _patch_resolve(), \ + patch.object(box_config_cli, "_run_box_config_py", side_effect=backend), \ + patch.object(box_config_cli, "default_ssh_runner", + side_effect=self._ssh_runner(snapshot_exists=True)), \ + patch.object(box_config_cli, "_bounce_container_rc", + return_value=box_config_cli._BOUNCE_CONFIG_NOT_APPLIED) as bounce_mock: + result = self.runner.invoke( + box_config_cli.box_config, + ["apply", "--box", "test-box", "--yes"], + ) + self.assertNotEqual(result.exit_code, 0) + self.assertIn("NOT applied", result.output) + # Bounced exactly once: no rollback re-bounce. + self.assertEqual(bounce_mock.call_count, 1) + verbs = [c[0] for c in backend.calls] + self.assertNotIn("set-applied-hash", verbs) + self.assertNotIn("restore-applied", verbs) + # --------------------------------------------------------------------------- # apt / sysctl / cargo CLI verbs @@ -669,7 +789,7 @@ def test_apt_skipped_when_unchanged(self): backend = self._backend(current=current, applied=current) with _patch_resolve(), \ patch.object(box_config_cli, "_run_box_config_py", side_effect=backend), \ - patch.object(box_config_cli, "_bounce_container", return_value=True), \ + patch.object(box_config_cli, "_bounce_container_rc", return_value=0), \ patch.object(box_config_cli, "_wait_for_box_api", return_value=True), \ patch("cli.commands.box.config.apt_install") as apt_mock, \ patch("cli.commands.box.config.sysctl_apply") as sysctl_mock: @@ -695,7 +815,7 @@ def test_apt_runs_when_changed(self): apt_result = HostOpResult(ok=True, action="installed", message="Installed/verified 2 apt package(s).") with _patch_resolve(), \ patch.object(box_config_cli, "_run_box_config_py", side_effect=backend), \ - patch.object(box_config_cli, "_bounce_container", return_value=True), \ + patch.object(box_config_cli, "_bounce_container_rc", return_value=0), \ patch.object(box_config_cli, "_wait_for_box_api", return_value=True), \ patch("cli.commands.box.config.apt_install", return_value=apt_result) as apt_mock, \ patch("cli.commands.box.config.sysctl_apply") as sysctl_mock: @@ -716,7 +836,7 @@ def test_apt_failure_aborts_before_bounce(self): apt_result = HostOpResult(ok=False, action="failed", message="sudo not configured") with _patch_resolve(), \ patch.object(box_config_cli, "_run_box_config_py", side_effect=backend), \ - patch.object(box_config_cli, "_bounce_container") as bounce_mock, \ + patch.object(box_config_cli, "_bounce_container_rc", return_value=0) as bounce_mock, \ patch("cli.commands.box.config.apt_install", return_value=apt_result): result = self.runner.invoke( box_config_cli.box_config, @@ -741,7 +861,7 @@ def test_sysctl_runs_when_changed(self): sysctl_result = HostOpResult(ok=True, action="applied", message="Wrote 1 sysctl key.") with _patch_resolve(), \ patch.object(box_config_cli, "_run_box_config_py", side_effect=backend), \ - patch.object(box_config_cli, "_bounce_container", return_value=True), \ + patch.object(box_config_cli, "_bounce_container_rc", return_value=0), \ patch.object(box_config_cli, "_wait_for_box_api", return_value=True), \ patch("cli.commands.box.config.sysctl_apply", return_value=sysctl_result) as sysctl_mock: result = self.runner.invoke( @@ -792,7 +912,7 @@ def test_consistent_state_updates_applied_hash(self): ) with _patch_resolve(), \ patch.object(box_config_cli, "_run_box_config_py", side_effect=backend), \ - patch.object(box_config_cli, "_bounce_container", return_value=True), \ + patch.object(box_config_cli, "_bounce_container_rc", return_value=0), \ patch.object(box_config_cli, "_wait_for_box_api", return_value=True): result = self.runner.invoke( box_config_cli.box_config, @@ -813,7 +933,7 @@ def test_post_bounce_validation_failure_skips_applied_hash(self): ) with _patch_resolve(), \ patch.object(box_config_cli, "_run_box_config_py", side_effect=backend), \ - patch.object(box_config_cli, "_bounce_container", return_value=True), \ + patch.object(box_config_cli, "_bounce_container_rc", return_value=0), \ patch.object(box_config_cli, "_wait_for_box_api", return_value=True): result = self.runner.invoke( box_config_cli.box_config, @@ -835,7 +955,7 @@ def test_post_bounce_show_drift_skips_applied_hash(self): ) with _patch_resolve(), \ patch.object(box_config_cli, "_run_box_config_py", side_effect=backend), \ - patch.object(box_config_cli, "_bounce_container", return_value=True), \ + patch.object(box_config_cli, "_bounce_container_rc", return_value=0), \ patch.object(box_config_cli, "_wait_for_box_api", return_value=True), \ patch("cli.commands.box.config.apt_install", return_value=__import__("cli.commands.box._host_ops", fromlist=["HostOpResult"]).HostOpResult( @@ -965,7 +1085,7 @@ def test_diff_printed_before_confirm(self): # Decline at the confirm prompt — we just want to see the diff appear. with _patch_resolve(), \ patch.object(box_config_cli, "_run_box_config_py", side_effect=backend), \ - patch.object(box_config_cli, "_bounce_container") as bounce_mock: + patch.object(box_config_cli, "_bounce_container_rc", return_value=0) as bounce_mock: result = self.runner.invoke( box_config_cli.box_config, ["apply", "--box", "test-box"], @@ -1122,7 +1242,7 @@ def test_dry_run_skips_all_writes(self): backend = self._backend(current=current, applied=applied) with _patch_resolve(), \ patch.object(box_config_cli, "_run_box_config_py", side_effect=backend), \ - patch.object(box_config_cli, "_bounce_container") as bounce_mock, \ + patch.object(box_config_cli, "_bounce_container_rc", return_value=0) as bounce_mock, \ patch.object(box_config_cli, "_preflight_mounts") as prep_mock, \ patch("cli.commands.box.config.apt_install") as apt_mock, \ patch("cli.commands.box.config.sysctl_apply") as sysctl_mock: @@ -1146,7 +1266,7 @@ def test_dry_run_reports_clean_when_unchanged(self): backend = self._backend(current=snap, applied=snap, cur_hash="x", applied_hash="x") with _patch_resolve(), \ patch.object(box_config_cli, "_run_box_config_py", side_effect=backend), \ - patch.object(box_config_cli, "_bounce_container") as bounce_mock: + patch.object(box_config_cli, "_bounce_container_rc", return_value=0) as bounce_mock: result = self.runner.invoke( box_config_cli.box_config, ["apply", "--box", "test-box", "--yes", "--dry-run"], @@ -1702,7 +1822,7 @@ def test_apply_continues_past_first_box_failure(self): }) with patch.object(box_config_cli, "_resolve_box", side_effect=["1.2.3.4", "5.6.7.8"]), \ patch.object(box_config_cli, "_run_box_config_py", side_effect=backend), \ - patch.object(box_config_cli, "_bounce_container", return_value=True), \ + patch.object(box_config_cli, "_bounce_container_rc", return_value=0), \ patch.object(box_config_cli, "_wait_for_box_api", return_value=True): result = self.runner.invoke( box_config_cli.box_config, @@ -1735,7 +1855,7 @@ def test_repair_succeeds_end_to_end(self): with _patch_resolve(), \ patch.object(box_config_cli, "default_ssh_runner", side_effect=self._ssh_runner(snapshot_exists=True)), \ - patch.object(box_config_cli, "_bounce_container", return_value=True), \ + patch.object(box_config_cli, "_bounce_container_rc", return_value=0), \ patch.object(box_config_cli, "_wait_for_box_api", return_value=True): result = self.runner.invoke( box_config_cli.box_config, @@ -1776,7 +1896,7 @@ def test_repair_bounce_failure(self): with _patch_resolve(), \ patch.object(box_config_cli, "default_ssh_runner", side_effect=self._ssh_runner(snapshot_exists=True)), \ - patch.object(box_config_cli, "_bounce_container", return_value=False): + patch.object(box_config_cli, "_bounce_container_rc", return_value=1): result = self.runner.invoke( box_config_cli.box_config, ["repair", "--box", "test-box", "--yes"], @@ -1927,7 +2047,7 @@ def test_ssh_failed_preflight_warns_and_continues(self): backend = self._backend() with _patch_resolve(), \ patch.object(box_config_cli, "_run_box_config_py", side_effect=backend), \ - patch.object(box_config_cli, "_bounce_container", return_value=True) as bounce_mock, \ + patch.object(box_config_cli, "_bounce_container_rc", return_value=0) as bounce_mock, \ patch.object(box_config_cli, "_wait_for_box_api", return_value=True), \ patch("cli.commands.box.config.ensure_host_path_owned", return_value=_SSH_FAILED_PREP): @@ -1951,7 +2071,7 @@ def test_ssh_failed_checks_only_first_mount(self): ) with _patch_resolve(), \ patch.object(box_config_cli, "_run_box_config_py", side_effect=backend), \ - patch.object(box_config_cli, "_bounce_container", return_value=True), \ + patch.object(box_config_cli, "_bounce_container_rc", return_value=0), \ patch.object(box_config_cli, "_wait_for_box_api", return_value=True), \ patch("cli.commands.box.config.ensure_host_path_owned", return_value=_SSH_FAILED_PREP) as prep_mock: @@ -1966,7 +2086,7 @@ def test_refused_populated_still_aborts(self): backend = self._backend() with _patch_resolve(), \ patch.object(box_config_cli, "_run_box_config_py", side_effect=backend), \ - patch.object(box_config_cli, "_bounce_container") as bounce_mock, \ + patch.object(box_config_cli, "_bounce_container_rc", return_value=0) as bounce_mock, \ patch("cli.commands.box.config.ensure_host_path_owned", return_value=_prep( False, "refused_populated", @@ -1986,7 +2106,7 @@ def test_sudo_failed_still_aborts(self): backend = self._backend() with _patch_resolve(), \ patch.object(box_config_cli, "_run_box_config_py", side_effect=backend), \ - patch.object(box_config_cli, "_bounce_container") as bounce_mock, \ + patch.object(box_config_cli, "_bounce_container_rc", return_value=0) as bounce_mock, \ patch("cli.commands.box.config.ensure_host_path_owned", return_value=_prep( False, "sudo_failed", @@ -2041,12 +2161,12 @@ def fake_prep(*args, **kwargs): def fake_bounce(*args, **kwargs): order.append("bounce") - return True + return 0 backend = self._backend() with _patch_resolve(), \ patch.object(box_config_cli, "_run_box_config_py", side_effect=backend), \ - patch.object(box_config_cli, "_bounce_container", side_effect=fake_bounce), \ + patch.object(box_config_cli, "_bounce_container_rc", side_effect=fake_bounce), \ patch.object(box_config_cli, "_wait_for_box_api", return_value=True), \ patch("cli.commands.box.config.apt_install", side_effect=fake_apt), \ patch("cli.commands.box.config.ensure_host_path_owned", side_effect=fake_prep): @@ -2061,7 +2181,7 @@ def test_preflight_not_run_when_confirm_declined(self): backend = self._backend() with _patch_resolve(), \ patch.object(box_config_cli, "_run_box_config_py", side_effect=backend), \ - patch.object(box_config_cli, "_bounce_container") as bounce_mock, \ + patch.object(box_config_cli, "_bounce_container_rc", return_value=0) as bounce_mock, \ patch("cli.commands.box.config.ensure_host_path_owned") as prep_mock: result = self.runner.invoke( box_config_cli.box_config, @@ -2117,12 +2237,12 @@ def fake_prep(*args, **kwargs): def fake_bounce(*args, **kwargs): order.append("bounce") - return True + return 0 backend = self._backend(cur_hash="aaa", applied_hash="aaa") with _patch_resolve(), \ patch.object(box_config_cli, "_run_box_config_py", side_effect=backend), \ - patch.object(box_config_cli, "_bounce_container", side_effect=fake_bounce), \ + patch.object(box_config_cli, "_bounce_container_rc", side_effect=fake_bounce), \ patch.object(box_config_cli, "_wait_for_box_api", return_value=True), \ patch("cli.commands.box.config.ensure_host_path_owned", side_effect=fake_prep): result = self.runner.invoke( @@ -2136,7 +2256,7 @@ def test_dry_run_with_mounts_never_touches_ssh(self): backend = self._backend() with _patch_resolve(), \ patch.object(box_config_cli, "_run_box_config_py", side_effect=backend), \ - patch.object(box_config_cli, "_bounce_container") as bounce_mock, \ + patch.object(box_config_cli, "_bounce_container_rc", return_value=0) as bounce_mock, \ patch("cli.commands.box.config.ensure_host_path_owned") as prep_mock: result = self.runner.invoke( box_config_cli.box_config, @@ -2150,7 +2270,7 @@ def test_no_auto_prep_skips_preflight_but_bounces(self): backend = self._backend() with _patch_resolve(), \ patch.object(box_config_cli, "_run_box_config_py", side_effect=backend), \ - patch.object(box_config_cli, "_bounce_container", return_value=True) as bounce_mock, \ + patch.object(box_config_cli, "_bounce_container_rc", return_value=0) as bounce_mock, \ patch.object(box_config_cli, "_wait_for_box_api", return_value=True), \ patch("cli.commands.box.config.ensure_host_path_owned") as prep_mock: result = self.runner.invoke( @@ -2189,7 +2309,7 @@ def test_ssh_warn_on_one_box_does_not_fail_fanout(self): preps = iter([_SSH_FAILED_PREP, _prep(True, "ok_readonly", message="/Hyphen exists.")]) with patch.object(box_config_cli, "_resolve_boxes", return_value=["1.1.1.1", "2.2.2.2"]), \ patch.object(box_config_cli, "_run_box_config_py", side_effect=backend), \ - patch.object(box_config_cli, "_bounce_container", return_value=True), \ + patch.object(box_config_cli, "_bounce_container_rc", return_value=0), \ patch.object(box_config_cli, "_wait_for_box_api", return_value=True), \ patch("cli.commands.box.config.ensure_host_path_owned", side_effect=lambda *a, **k: next(preps)): @@ -2212,7 +2332,7 @@ def test_hard_failure_on_one_box_continues_and_aggregates(self): ]) with patch.object(box_config_cli, "_resolve_boxes", return_value=["1.1.1.1", "2.2.2.2"]), \ patch.object(box_config_cli, "_run_box_config_py", side_effect=backend), \ - patch.object(box_config_cli, "_bounce_container", return_value=True) as bounce_mock, \ + patch.object(box_config_cli, "_bounce_container_rc", return_value=0) as bounce_mock, \ patch.object(box_config_cli, "_wait_for_box_api", return_value=True), \ patch("cli.commands.box.config.ensure_host_path_owned", side_effect=lambda *a, **k: next(preps)): @@ -2262,7 +2382,7 @@ def test_real_failure_then_ssh_failed_aborts(self): ]) with _patch_resolve(), \ patch.object(box_config_cli, "_run_box_config_py", side_effect=backend), \ - patch.object(box_config_cli, "_bounce_container") as bounce_mock, \ + patch.object(box_config_cli, "_bounce_container_rc", return_value=0) as bounce_mock, \ patch("cli.commands.box.config.ensure_host_path_owned", side_effect=lambda *a, **k: next(preps)): result = self.runner.invoke( @@ -2288,7 +2408,7 @@ def test_non_string_host_skipped_without_crash(self): }) with _patch_resolve(), \ patch.object(box_config_cli, "_run_box_config_py", side_effect=backend), \ - patch.object(box_config_cli, "_bounce_container", return_value=True), \ + patch.object(box_config_cli, "_bounce_container_rc", return_value=0), \ patch.object(box_config_cli, "_wait_for_box_api", return_value=True), \ patch("cli.commands.box.config.ensure_host_path_owned") as prep_mock: result = self.runner.invoke( @@ -2302,7 +2422,7 @@ def test_bounce_failure_after_ssh_warn_engages_rollback(self): backend = self._backend_two_mounts() with _patch_resolve(), \ patch.object(box_config_cli, "_run_box_config_py", side_effect=backend), \ - patch.object(box_config_cli, "_bounce_container", return_value=False), \ + patch.object(box_config_cli, "_bounce_container_rc", return_value=1), \ patch.object(box_config_cli, "_attempt_rollback", return_value=True) as rb_mock, \ patch("cli.commands.box.config.ensure_host_path_owned", return_value=_SSH_FAILED_PREP): diff --git a/test/unit/box/test_render_packages.py b/test/unit/box/test_render_packages.py index 51e407ff..3494bf37 100644 --- a/test/unit/box/test_render_packages.py +++ b/test/unit/box/test_render_packages.py @@ -106,5 +106,56 @@ def test_udev_rules_do_not_change_output(self): self.assertNotIn("1209", body_b) +class UnwritableOutputDir(unittest.TestCase): + """A renderer that cannot write its output must say so, loudly. + + This is the failure that made `lager box config` a silent no-op fleet-wide: + /etc/lager is owned by the container user (uid 33), start_box.sh runs as the + box's login user, and creating a file needs write permission on the + DIRECTORY. Every renderer died with a bare PermissionError traceback that + start_box.sh folded into a one-line warning, so `apply` reported success + while installing nothing. Each renderer must now exit non-zero with an + actionable message naming the directory. + """ + + def _render_into_readonly_dir(self, renderer, config_dict): + d = tempfile.mkdtemp(prefix="lager-render-ro-") + ro_dir = os.path.join(d, "etc-lager") + os.mkdir(ro_dir) + try: + config_path = os.path.join(d, "box_config.json") + with open(config_path, "w") as f: + json.dump(config_dict, f) + out_path = os.path.join(ro_dir, "out.txt") + os.chmod(ro_dir, 0o555) # r-x: cannot create the tmp file + proc = subprocess.run( + ["python3", os.path.join(_RENDER_DIR, renderer), config_path, out_path], + capture_output=True, + text=True, + ) + return proc.returncode, proc.stdout, proc.stderr, out_path + finally: + os.chmod(ro_dir, 0o755) + shutil.rmtree(d, ignore_errors=True) + + def test_every_renderer_fails_loudly_on_unwritable_dir(self): + for renderer, field, specs in _RENDERERS + [ + ("render_docker_args.py", "pip_packages", []), + ]: + with self.subTest(renderer=renderer): + rc, stdout, stderr, out_path = self._render_into_readonly_dir( + renderer, {"version": 1, field: specs} + ) + out = stdout + stderr + self.assertNotEqual(rc, 0, "must not report success") + self.assertIn("[ERROR]", out) + self.assertIn("cannot write", out) + # Actionable: names the directory at fault and the repair. + self.assertIn(os.path.dirname(out_path), out) + self.assertIn("lager update", out) + # A traceback is what we are replacing; it must not resurface. + self.assertNotIn("Traceback", out) + + if __name__ == "__main__": unittest.main()