From 56e97c46f39d193a22bedce7615d030c3950edad Mon Sep 17 00:00:00 2001 From: DanielRMErskine Date: Mon, 13 Jul 2026 11:48:13 -0700 Subject: [PATCH 1/2] v0.31.7: don't write unusable DNS servers into Docker's daemon.json The install step that points Docker's container DNS at the box's upstream resolvers excluded only the 127.0.0.53 stub and trusted every other value systemd-resolved reported. On a network that advertises DNS over IPv6 router advertisement that includes a link-local resolver carrying a zone id (fe80::1%3), which Docker cannot parse -- and Docker refuses to start at all rather than skipping the entry or falling back to the valid resolvers beside it. Because daemon.json is persistent, the daemon then stayed down across reboots, and re-running the installer rewrote the file, undoing any manual repair. Validate resolvers before writing them, roll the change back if Docker will not start with it, and stop the install when the daemon is down instead of running six more steps against it and failing with a misleading error. --- CHANGELOG.md | 41 +++++ cli/__init__.py | 2 +- .../scripts/configure_docker_dns.py | 122 ++++++++++++++ .../scripts/configure_docker_dns.sh | 72 +++++++++ .../scripts/setup_and_deploy_box.sh | 69 ++++---- docs/docs.json | 1 + docs/source/release-notes/v0.31.7.mdx | 33 ++++ test/unit/cli/test_configure_docker_dns.py | 150 ++++++++++++++++++ .../cli/test_configure_docker_dns_rollback.py | 115 ++++++++++++++ 9 files changed, 562 insertions(+), 43 deletions(-) create mode 100644 cli/deployment/scripts/configure_docker_dns.py create mode 100755 cli/deployment/scripts/configure_docker_dns.sh create mode 100644 docs/source/release-notes/v0.31.7.mdx create mode 100644 test/unit/cli/test_configure_docker_dns.py create mode 100644 test/unit/cli/test_configure_docker_dns_rollback.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 76e4246d..0e537acc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,47 @@ All notable changes to the Lager platform are documented here. For detailed release notes, see [docs.lagerdata.com](https://docs.lagerdata.com). +## [0.31.7] - 2026-07-13 + +`lager install` can no longer leave a box with a Docker daemon that will not start, +and it stops rather than deploying into one that isn't running. + +### Fixed +- **`lager install` no longer writes a DNS server Docker cannot parse into + `/etc/docker/daemon.json`.** The "Configuring Docker DNS" step reads the box's + upstream resolvers from systemd-resolved and merges them into `daemon.json`, + but excluded only the `127.0.0.53` stub — every other value was trusted to be a + usable IP. On a network that advertises DNS over IPv6 router advertisement, + systemd-resolved records a link-local resolver carrying a zone id + (`fe80::1%3`). Docker validates each `dns` entry as a bare IP address and + **refuses to start** when one does not parse — it does not skip the entry or fall + back to the valid resolvers beside it. Because `daemon.json` is persistent, the + daemon then stayed down across reboots, and re-running the installer rewrote the + file each time, undoing any manual repair. Resolvers are now validated before + they are written: link-local, loopback, unspecified, multicast and unparseable + values are dropped (a link-local address is unreachable from a container's + network namespace regardless), and anything dropped is named in the install log. +- **A failed Docker DNS change is rolled back instead of left in place.** Pointing + Docker at the box's resolvers is an optimization; it must not be able to leave the + box worse off. `daemon.json` is now backed up before the change, and if Docker + will not come back with the new config, the previous file is restored and Docker + restarted on it. A box that cannot be improved is no longer a box that gets broken. +- **The installer stops when the box's Docker daemon is down, instead of running six + more steps against it.** Every Docker command in the container step is + best-effort (`|| true`), so a dead daemon left no trace until `start_box.sh` failed + on `docker network create` with a bare "Cannot connect to the Docker daemon" — + far from whatever actually stopped it, and pointing at the wrong thing. The step + now checks the daemon first and fails with the commands needed to diagnose it. + Likewise, a DNS step that fails now reports that the box kept its previous Docker + configuration and that the install is continuing, rather than a bare warning. + +### Changed +- The deployment script's Docker DNS logic moved out of an inline heredoc into + `cli/deployment/scripts/configure_docker_dns.{sh,py}`, shipped to the box like + `secure_box_firewall.sh` already is, and is now covered by unit tests. +- The install step counter reports `[N/8]`; there were eight steps and a total of + seven, so the last one printed as `[8/7]`. + ## [0.31.6] - 2026-07-13 Help pages now share one usage grammar across every net-style command, and the box diff --git a/cli/__init__.py b/cli/__init__.py index 938ce6c9..2e9c38b1 100644 --- a/cli/__init__.py +++ b/cli/__init__.py @@ -7,4 +7,4 @@ A Command Line Interface for Lager Data """ -__version__ = '0.31.6' +__version__ = '0.31.7' diff --git a/cli/deployment/scripts/configure_docker_dns.py b/cli/deployment/scripts/configure_docker_dns.py new file mode 100644 index 00000000..4ec362ac --- /dev/null +++ b/cli/deployment/scripts/configure_docker_dns.py @@ -0,0 +1,122 @@ +#!/usr/bin/env python3 +# Copyright 2024-2026 Lager Data +# SPDX-License-Identifier: Apache-2.0 + +"""Choose usable upstream resolvers for Docker's container DNS. + +Runs on the box under the system python3, so stdlib only. + +Docker validates every entry of daemon.json's "dns" list as a bare IP address and +refuses to start -- not warn, not skip the entry -- if one of them does not parse. +A resolver learned over IPv6 router advertisement is a link-local address carrying +a zone id (``fe80::1%3``), which does not parse. Writing one out therefore takes +the daemon down, and it stays down across reboots, because daemon.json is +persistent. + +Link-local resolvers are dropped rather than stripped of their zone: a container's +network namespace cannot reach the host's link-local scope, so such an address is +useless to the containers this list exists to serve. +""" + +import argparse +import ipaddress +import json +import os + +FALLBACKS = ("1.1.1.1", "8.8.8.8") +MAX_UPSTREAM = 3 +DEFAULT_RESOLV_CONF = "/run/systemd/resolve/resolv.conf" +DEFAULT_DAEMON_JSON = "/etc/docker/daemon.json" + + +def rejection_reason(value): + """Why `value` is unusable as a Docker DNS server, or None if it is usable.""" + address = value.split("%", 1)[0] + try: + ip = ipaddress.ip_address(address) + except ValueError: + return "not an IP address" + if ip.is_loopback: + return "loopback" + if ip.is_link_local: + return "link-local" + if ip.is_unspecified: + return "unspecified" + if ip.is_multicast: + return "multicast" + return None + + +def _nameservers(text): + for line in text.splitlines(): + fields = line.split() + if len(fields) >= 2 and fields[0] == "nameserver": + yield fields[1] + + +def usable_resolvers(text, max_upstream=MAX_UPSTREAM): + """Usable resolvers from resolv.conf `text`, in order, deduplicated.""" + servers = [] + for value in _nameservers(text): + if rejection_reason(value) or value in servers: + continue + servers.append(value) + if len(servers) == max_upstream: + break + return servers + + +def rejected_resolvers(text): + """(value, reason) pairs for every nameserver we refuse to hand to Docker.""" + return [(v, rejection_reason(v)) for v in _nameservers(text) if rejection_reason(v)] + + +def merge_dns(cfg, servers): + """Return `cfg` with "dns" set, preserving every operator-set key.""" + dns = list(servers) + for fallback in FALLBACKS: + if fallback not in dns: + dns.append(fallback) + merged = dict(cfg) + merged["dns"] = dns + return merged + + +def load_daemon_json(path): + """Existing daemon.json, or {} if it is absent or unreadable.""" + if not os.path.exists(path): + return {} + try: + with open(path) as fh: + return json.load(fh) or {} + except (OSError, ValueError): + # Docker cannot parse it either, so there is nothing worth preserving. + return {} + + +def main(): + parser = argparse.ArgumentParser(description="Build Docker's daemon.json DNS config") + parser.add_argument("--resolv-conf", default=DEFAULT_RESOLV_CONF) + parser.add_argument("--daemon-json", default=DEFAULT_DAEMON_JSON) + parser.add_argument("--out", required=True, help="where to write the merged daemon.json") + args = parser.parse_args() + + try: + with open(args.resolv_conf) as fh: + resolv = fh.read() + except OSError: + resolv = "" + + cfg = merge_dns(load_daemon_json(args.daemon_json), usable_resolvers(resolv)) + + with open(args.out, "w") as fh: + json.dump(cfg, fh, indent=2) + fh.write("\n") + + for value, reason in rejected_resolvers(resolv): + print("Ignoring unusable resolver {} ({})".format(value, reason)) + print("Docker container DNS ->", ", ".join(cfg["dns"])) + + +if __name__ == "__main__": + main() diff --git a/cli/deployment/scripts/configure_docker_dns.sh b/cli/deployment/scripts/configure_docker_dns.sh new file mode 100755 index 00000000..150ec97e --- /dev/null +++ b/cli/deployment/scripts/configure_docker_dns.sh @@ -0,0 +1,72 @@ +#!/bin/bash +# Copyright 2024-2026 Lager Data +# SPDX-License-Identifier: Apache-2.0 +# +# Point Docker's container DNS at the box's real uplink resolvers. Runs on the box. +# +# On a systemd-resolved box, /etc/resolv.conf lists only the 127.0.0.53 stub, which +# a container cannot use, so Docker falls back to 8.8.8.8; where that resolver is +# unreachable, `docker build` cannot resolve github.com/pypi and the image build +# dies partway through. systemd-resolved's own resolv.conf names the real upstream +# servers, so we merge those into daemon.json. configure_docker_dns.py decides +# which of them Docker can actually use. +# +# This is an optimization, so it must never leave the box worse off than it found +# it: if Docker will not come back with the new config, the previous daemon.json is +# restored and Docker is restarted on it before we exit non-zero. +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +DAEMON_JSON="${DAEMON_JSON:-/etc/docker/daemon.json}" +RESOLV_CONF="${RESOLV_CONF:-/run/systemd/resolve/resolv.conf}" +BACKUP="${DAEMON_JSON}.lager-bak" + +STAGED=$(mktemp) +trap 'rm -f "$STAGED"' EXIT + +python3 "${SCRIPT_DIR}/configure_docker_dns.py" \ + --resolv-conf "$RESOLV_CONF" --daemon-json "$DAEMON_JSON" --out "$STAGED" + +# Snapshot the current config so a failed restart can be undone. Track whether +# there was one at all, so we restore the old file or remove ours accordingly. +if [ -f "$DAEMON_JSON" ]; then + HAD_CONFIG=1 + sudo cp -f "$DAEMON_JSON" "$BACKUP" +else + HAD_CONFIG=0 + sudo rm -f "$BACKUP" +fi + +daemon_is_up() { + # `systemctl is-active` reads the unit's own state: unlike `docker info` it + # needs neither root nor docker-group membership, which a freshly added user + # may not have picked up in this SSH session yet. + local _ + for _ in 1 2 3 4 5; do + if systemctl is-active --quiet docker; then + return 0 + fi + sleep 1 + done + return 1 +} + +restore_previous() { + if [ "$HAD_CONFIG" -eq 1 ]; then + sudo install -m 0644 "$BACKUP" "$DAEMON_JSON" + else + sudo rm -f "$DAEMON_JSON" + fi + sudo systemctl restart docker || true +} + +sudo install -m 0644 "$STAGED" "$DAEMON_JSON" + +if ! sudo systemctl restart docker || ! daemon_is_up; then + restore_previous + echo "ERROR: Docker would not start with the new DNS configuration." >&2 + echo " Restored the previous ${DAEMON_JSON} and restarted Docker." >&2 + exit 1 +fi + +echo "Docker restarted with the new DNS configuration" diff --git a/cli/deployment/scripts/setup_and_deploy_box.sh b/cli/deployment/scripts/setup_and_deploy_box.sh index 9502d6c0..dfbfaf3d 100755 --- a/cli/deployment/scripts/setup_and_deploy_box.sh +++ b/cli/deployment/scripts/setup_and_deploy_box.sh @@ -202,7 +202,7 @@ echo -e "${BLUE}Time:${NC} $(date '+%Y-%m-%d %H:%M:%S')" echo "" # Step counter -TOTAL_STEPS=7 +TOTAL_STEPS=8 CURRENT_STEP=0 print_step() { @@ -722,54 +722,20 @@ fi # resolver is blocked or flaky, `docker build` can't resolve github.com/pypi # and the image build dies on its git/pip clone steps. Point Docker at the # box's real uplink resolvers (with public fallbacks) so builds resolve -# reliably. Requires a docker restart to take effect. +# reliably. Requires a docker restart to take effect; configure_docker_dns.sh +# rolls the change back if Docker won't come up with it. print_step "Configuring Docker DNS" print_info "Pointing Docker's container DNS at the box's real uplink resolvers (you may be prompted for a password)..." echo "" -TEMP_DNS_SCRIPT=$(mktemp) -cat > "$TEMP_DNS_SCRIPT" << SCRIPT_EOF -#!/bin/bash -set -e -# Real upstream resolvers systemd-resolved talks to (its own resolv.conf lists -# the actual servers, not the 127.0.0.53 stub). Drop the loopback stub if it -# appears, and cap at three. -UPSTREAM=\$(awk '/^nameserver/ && \$2 != "127.0.0.53" { print \$2 }' /run/systemd/resolve/resolv.conf 2>/dev/null | head -3 | tr '\n' ' ') -# Merge a "dns" key into any existing daemon.json (python3 is always present on -# a lager box) rather than clobbering operator-set options. -python3 - "\$UPSTREAM" <<'PYEOF' -import json, os, sys -servers = [s for s in sys.argv[1].split() if s] -for fallback in ("1.1.1.1", "8.8.8.8"): - if fallback not in servers: - servers.append(fallback) -path = "/etc/docker/daemon.json" -cfg = {} -if os.path.exists(path): - try: - with open(path) as fh: - cfg = json.load(fh) or {} - except Exception: - cfg = {} -cfg["dns"] = servers -with open("/tmp/lager_daemon.json", "w") as fh: - json.dump(cfg, fh, indent=2) - fh.write("\n") -print("Docker container DNS ->", ", ".join(servers)) -PYEOF -sudo install -m 0644 /tmp/lager_daemon.json /etc/docker/daemon.json -rm -f /tmp/lager_daemon.json -sudo systemctl restart docker -SCRIPT_EOF - -scp $SCP_OPTS "$TEMP_DNS_SCRIPT" "${BOX_USER}@${BOX_IP}:/tmp/setup_docker_dns.sh" >/dev/null -if ssh_t "${BOX_USER}@${BOX_IP}" "chmod +x /tmp/setup_docker_dns.sh && /tmp/setup_docker_dns.sh && rm /tmp/setup_docker_dns.sh"; then +scp $SCP_OPTS "${SCRIPT_DIR}/configure_docker_dns.sh" "${SCRIPT_DIR}/configure_docker_dns.py" "${BOX_USER}@${BOX_IP}:/tmp/" >/dev/null +if ssh_t "${BOX_USER}@${BOX_IP}" "chmod +x /tmp/configure_docker_dns.sh && /tmp/configure_docker_dns.sh; rc=\$?; rm -f /tmp/configure_docker_dns.sh /tmp/configure_docker_dns.py; exit \$rc"; then print_success "Docker DNS configured" else - print_warning "Could not configure Docker DNS automatically" - print_info "If image builds fail with 'Could not resolve host', set \"dns\" in /etc/docker/daemon.json on the box and restart docker." + print_warning "Could not configure Docker DNS - the box kept its previous Docker configuration" + print_info "Docker is still running and the install will continue. If image builds later fail" + print_info "with 'Could not resolve host', set \"dns\" in /etc/docker/daemon.json on the box." fi -rm "$TEMP_DNS_SCRIPT" echo "" # Ensure /etc/lager directory exists (always check, even if sudo was already configured) @@ -1402,6 +1368,25 @@ if [ -n "$VPN_INTERFACE" ]; then echo "" fi +# Every docker command in this step is best-effort (`|| true`), so a daemon that is +# down leaves no trace here and start_box.sh is the first thing to notice -- failing +# on `docker network create` with a bare "Cannot connect to the Docker daemon", well +# after whatever actually stopped it. Check explicitly, while the cause is still near. +print_info "Verifying the Docker daemon is running..." +if ! ssh $SSH_OPTS "${BOX_USER}@${BOX_IP}" "docker info >/dev/null 2>&1"; then + print_error "The Docker daemon is not running on the box" + echo "" + echo " Nothing can be deployed until it starts. On the box:" + echo " sudo systemctl status docker" + echo " sudo journalctl -u docker -n 50 --no-pager" + echo "" + echo " A daemon that refuses to start usually has a bad /etc/docker/daemon.json." + echo "" + exit 1 +fi +print_success "Docker daemon is running" +echo "" + print_info "Building and starting containers..." echo "" ssh $SSH_OPTS "${BOX_USER}@${BOX_IP}" "cd ~/box && chmod +x start_box.sh && ./start_box.sh" diff --git a/docs/docs.json b/docs/docs.json index 664fe766..23c07205 100644 --- a/docs/docs.json +++ b/docs/docs.json @@ -211,6 +211,7 @@ { "group": "Version History", "pages": [ + "source/release-notes/v0.31.7", "source/release-notes/v0.31.6", "source/release-notes/v0.31.5", "source/release-notes/v0.31.4", diff --git a/docs/source/release-notes/v0.31.7.mdx b/docs/source/release-notes/v0.31.7.mdx new file mode 100644 index 00000000..ce6baf81 --- /dev/null +++ b/docs/source/release-notes/v0.31.7.mdx @@ -0,0 +1,33 @@ +--- +title: "Version 0.31.7" +description: "July 13, 2026" +--- + +## Bug Fixes + +- **`lager install` no longer writes a DNS server Docker cannot parse.** The installer copies the box's upstream resolvers into `/etc/docker/daemon.json` so image builds resolve reliably, but it previously trusted every value systemd-resolved reported. On a network that advertises DNS over IPv6 router advertisement, that includes a link-local resolver with a zone id (`fe80::1%3`) — and Docker **refuses to start** when any `dns` entry is not a bare IP address, rather than skipping it. Because `daemon.json` persists, the daemon stayed down across reboots, and re-running the installer undid any manual repair. Resolvers are now validated before they are written; link-local, loopback and unparseable values are dropped and named in the install log. +- **A Docker DNS change that doesn't take is rolled back.** `daemon.json` is backed up first, and if Docker will not start with the new configuration, the previous file is restored and Docker is restarted on it. Pointing Docker at the box's resolvers is an optimization, and it can no longer leave a box worse off than it found it. +- **The installer stops when the box's Docker daemon is down.** Previously it continued for six more steps and failed with a bare "Cannot connect to the Docker daemon" from `start_box.sh`, far from the actual cause. It now checks the daemon before deploying and reports the commands needed to diagnose it. + +## Improvements + +- The Docker DNS logic moved into `configure_docker_dns.sh` / `configure_docker_dns.py` and is covered by unit tests. +- The install step counter no longer prints `[8/7]`. + +## Installation + +To install this version: + +```bash +pip install lager-cli==0.31.7 +``` + +To upgrade from a previous version: + +```bash +pip install --upgrade lager-cli +``` + +## Resources + +[View Release on PyPI](https://pypi.org/project/lager-cli/0.31.7/) diff --git a/test/unit/cli/test_configure_docker_dns.py b/test/unit/cli/test_configure_docker_dns.py new file mode 100644 index 00000000..7e7ca964 --- /dev/null +++ b/test/unit/cli/test_configure_docker_dns.py @@ -0,0 +1,150 @@ +#!/usr/bin/env python3 + +# Copyright 2024-2026 Lager Data +# SPDX-License-Identifier: Apache-2.0 +"""Unit tests for ``cli.deployment.scripts.configure_docker_dns``. + +Docker validates every entry of daemon.json's ``dns`` list as a bare IP address and +refuses to start if one does not parse. The install step that populates that list +reads the box's resolvers straight out of systemd-resolved's resolv.conf, so +whatever the box's network hands it ends up in front of the daemon. + +On a network using IPv6 router advertisement, that includes a link-local resolver +carrying a zone id (``fe80::1%3``). Writing one out stopped Docker from starting at +all -- and, because daemon.json is persistent, kept it from starting on every +subsequent boot. These tests pin what the filter accepts and, above all, that +everything it emits is something Docker can parse. +""" + +import ipaddress +import json + +import pytest + +from cli.deployment.scripts.configure_docker_dns import ( + FALLBACKS, + load_daemon_json, + merge_dns, + rejected_resolvers, + rejection_reason, + usable_resolvers, +) + + +def test_link_local_resolver_is_dropped_and_real_one_kept(): + """The exact resolv.conf shape that took a box's Docker daemon down.""" + resolv = "nameserver 192.168.100.1\nnameserver fe80::1%3\n" + + assert usable_resolvers(resolv) == ["192.168.100.1"] + assert merge_dns({}, usable_resolvers(resolv))["dns"] == [ + "192.168.100.1", + "1.1.1.1", + "8.8.8.8", + ] + + +@pytest.mark.parametrize( + "value, reason", + [ + ("fe80::1%3", "link-local"), # zone id as an interface index + ("fe80::1%enp3s0", "link-local"), # zone id as an interface name + ("fe80::1", "link-local"), # parses, but unreachable from a container + ("169.254.1.1", "link-local"), # IPv4 link-local + ("127.0.0.53", "loopback"), # the systemd-resolved stub + ("127.0.0.1", "loopback"), + ("::1", "loopback"), + ("0.0.0.0", "unspecified"), + ("224.0.0.1", "multicast"), + ("not-an-ip", "not an IP address"), + ("", "not an IP address"), + ], +) +def test_unusable_resolvers_are_rejected_with_a_reason(value, reason): + assert rejection_reason(value) == reason + + +@pytest.mark.parametrize("value", ["192.168.100.1", "8.8.4.4", "2606:4700:4700::1111"]) +def test_routable_resolvers_are_kept(value): + assert rejection_reason(value) is None + assert usable_resolvers("nameserver {}\n".format(value)) == [value] + + +def test_every_emitted_server_is_parseable_by_docker(): + """The invariant that matters: nothing we emit can stop the daemon starting. + + Docker parses each ``dns`` entry with the equivalent of ``ip_address()`` and + refuses to start if any one of them fails. Feed the filter a resolv.conf full + of things that break it and assert the output is still entirely clean. + """ + resolv = "\n".join( + [ + "nameserver fe80::1%3", + "nameserver 127.0.0.53", + "nameserver 169.254.1.1", + "nameserver garbage", + "nameserver 192.168.1.1", + "search example.com", + "options edns0", + ] + ) + + dns = merge_dns({}, usable_resolvers(resolv))["dns"] + + assert dns == ["192.168.1.1", "1.1.1.1", "8.8.8.8"] + for server in dns: + ipaddress.ip_address(server) # raises ValueError if Docker would reject it + + +def test_resolvers_are_capped_and_deduplicated(): + resolv = "".join( + "nameserver {}\n".format(ip) + for ip in ["10.0.0.1", "10.0.0.1", "10.0.0.2", "10.0.0.3", "10.0.0.4"] + ) + + assert usable_resolvers(resolv) == ["10.0.0.1", "10.0.0.2", "10.0.0.3"] + + +def test_no_usable_resolvers_falls_back_to_public_ones(): + """A box whose resolv.conf yields nothing still gets a working container DNS.""" + resolv = "nameserver 127.0.0.53\nnameserver fe80::1%3\n" + + assert merge_dns({}, usable_resolvers(resolv))["dns"] == list(FALLBACKS) + assert merge_dns({}, usable_resolvers(""))["dns"] == list(FALLBACKS) + + +def test_fallbacks_are_not_duplicated_when_already_upstream(): + resolv = "nameserver 1.1.1.1\n" + + assert merge_dns({}, usable_resolvers(resolv))["dns"] == ["1.1.1.1", "8.8.8.8"] + + +def test_operator_set_keys_are_preserved(): + """We own the "dns" key and nothing else on the box's daemon.json.""" + cfg = {"log-driver": "json-file", "dns": ["fe80::1%3"]} + + merged = merge_dns(cfg, ["192.168.1.1"]) + + assert merged["log-driver"] == "json-file" + assert merged["dns"] == ["192.168.1.1", "1.1.1.1", "8.8.8.8"] + assert cfg["dns"] == ["fe80::1%3"], "input config should not be mutated" + + +def test_rejected_resolvers_are_reported_for_the_install_log(): + resolv = "nameserver 192.168.1.1\nnameserver fe80::1%3\nnameserver 127.0.0.53\n" + + assert rejected_resolvers(resolv) == [ + ("fe80::1%3", "link-local"), + ("127.0.0.53", "loopback"), + ] + + +def test_load_daemon_json_tolerates_missing_and_corrupt_files(tmp_path): + assert load_daemon_json(str(tmp_path / "absent.json")) == {} + + corrupt = tmp_path / "daemon.json" + corrupt.write_text("{not json") + assert load_daemon_json(str(corrupt)) == {} + + valid = tmp_path / "valid.json" + valid.write_text(json.dumps({"log-driver": "journald"})) + assert load_daemon_json(str(valid)) == {"log-driver": "journald"} diff --git a/test/unit/cli/test_configure_docker_dns_rollback.py b/test/unit/cli/test_configure_docker_dns_rollback.py new file mode 100644 index 00000000..d1a02455 --- /dev/null +++ b/test/unit/cli/test_configure_docker_dns_rollback.py @@ -0,0 +1,115 @@ +#!/usr/bin/env python3 + +# Copyright 2024-2026 Lager Data +# SPDX-License-Identifier: Apache-2.0 +"""Rollback behaviour of ``cli/deployment/scripts/configure_docker_dns.sh``. + +Pointing Docker at the box's uplink resolvers is an optimization: it makes image +builds resolve reliably, and nothing more. It therefore must never be able to leave +the box worse off than it found it. The failure this guards against is not +hypothetical -- a config Docker refused to parse left a box's daemon dead across +reboots, and every re-run of the installer rewrote it. + +So: if Docker will not come back with the new daemon.json, the previous one must be +restored and Docker restarted on it. `sudo`, `systemctl` and `docker` are stubbed on +PATH, and the daemon.json / resolv.conf paths are pointed at a temp dir, so this +runs without root and without a Docker daemon. +""" + +import json +import os +import shutil +import subprocess +from pathlib import Path + +import pytest + +SCRIPT = ( + Path(__file__).resolve().parents[3] + / "cli" + / "deployment" + / "scripts" + / "configure_docker_dns.sh" +) + +ORIGINAL_CONFIG = {"log-driver": "journald", "dns": ["10.9.9.9"]} + + +def _write_stub(directory, name, body): + path = directory / name + path.write_text("#!/bin/bash\n" + body + "\n") + path.chmod(0o755) + + +@pytest.fixture +def box(tmp_path): + """A fake box: stubbed sudo/systemctl/docker, and a resolv.conf worth reading.""" + bin_dir = tmp_path / "bin" + bin_dir.mkdir() + + # `sudo` just runs the command -- the test owns the files it touches. + _write_stub(bin_dir, "sudo", 'exec "$@"') + # `systemctl restart` fails when the test asks it to; is-active mirrors that. + _write_stub( + bin_dir, + "systemctl", + 'if [ "$1" = "restart" ]; then exit "${RESTART_RC:-0}"; fi\n' + 'if [ "$1" = "is-active" ]; then exit "${RESTART_RC:-0}"; fi\n' + "exit 0", + ) + _write_stub(bin_dir, "docker", "exit 0") + + (tmp_path / "resolv.conf").write_text("nameserver 192.168.100.1\nnameserver fe80::1%3\n") + + return tmp_path, bin_dir + + +def _run(box, restart_rc, daemon_json_exists=True): + tmp_path, bin_dir = box + daemon_json = tmp_path / "daemon.json" + if daemon_json_exists: + daemon_json.write_text(json.dumps(ORIGINAL_CONFIG, indent=2) + "\n") + + env = dict(os.environ) + env.update( + PATH="{}:{}".format(bin_dir, env["PATH"]), + DAEMON_JSON=str(daemon_json), + RESOLV_CONF=str(tmp_path / "resolv.conf"), + RESTART_RC=str(restart_rc), + ) + + result = subprocess.run( + ["bash", str(SCRIPT)], env=env, capture_output=True, text=True, timeout=60 + ) + return result, daemon_json + + +@pytest.mark.skipif(shutil.which("bash") is None, reason="requires bash") +def test_successful_restart_leaves_the_new_config_in_place(box): + result, daemon_json = _run(box, restart_rc=0) + + assert result.returncode == 0, result.stderr + written = json.loads(daemon_json.read_text()) + # The link-local resolver is gone; the real one and the fallbacks remain. + assert written["dns"] == ["192.168.100.1", "1.1.1.1", "8.8.8.8"] + # An operator's unrelated settings survive. + assert written["log-driver"] == "journald" + + +@pytest.mark.skipif(shutil.which("bash") is None, reason="requires bash") +def test_failed_restart_restores_the_previous_config(box): + """The property that matters: a box we could not improve is a box we did not break.""" + result, daemon_json = _run(box, restart_rc=1) + + assert result.returncode == 1 + assert json.loads(daemon_json.read_text()) == ORIGINAL_CONFIG + assert "Restored the previous" in result.stderr + + +@pytest.mark.skipif(shutil.which("bash") is None, reason="requires bash") +def test_failed_restart_removes_config_that_did_not_exist_before(box): + """Rolling back to "no daemon.json" means removing ours, not restoring an empty one.""" + result, daemon_json = _run(box, restart_rc=1, daemon_json_exists=False) + + assert result.returncode == 1 + assert not daemon_json.exists() From 0efb4e02a7bf9794d5e514349fe7ef9b77422049 Mon Sep 17 00:00:00 2001 From: DanielRMErskine Date: Mon, 13 Jul 2026 12:08:22 -0700 Subject: [PATCH 2/2] Keep the Docker DNS script inside the box's passwordless sudo grant Review caught that staging the merged config via mktemp put `sudo install` outside the box's NOPASSWD rule, which names /tmp/lager_daemon.json exactly, and that `sudo cp` / `sudo rm` on /etc/docker were never granted at all. On a box that does not hand its login user blanket sudo, that means a password prompt on every run -- including on the rollback path, where a prompt means the recovery does not happen. Stage at the granted path, snapshot into /tmp with no privileges at all (daemon.json is world-readable), and roll back by installing an empty object rather than removing the file. Pin every sudo in the script to a grant the installer writes: the hardware box used for validation carries a blanket NOPASSWD: ALL rule, so a live install could not have surfaced this. Also treat a daemon.json that is valid JSON but not an object as absent, rather than letting it break the merge. --- .../scripts/configure_docker_dns.py | 7 ++- .../scripts/configure_docker_dns.sh | 31 ++++++++---- test/unit/cli/test_configure_docker_dns.py | 17 +++++++ .../cli/test_configure_docker_dns_rollback.py | 49 +++++++++++++++---- 4 files changed, 83 insertions(+), 21 deletions(-) diff --git a/cli/deployment/scripts/configure_docker_dns.py b/cli/deployment/scripts/configure_docker_dns.py index 4ec362ac..417f900d 100644 --- a/cli/deployment/scripts/configure_docker_dns.py +++ b/cli/deployment/scripts/configure_docker_dns.py @@ -83,15 +83,18 @@ def merge_dns(cfg, servers): def load_daemon_json(path): - """Existing daemon.json, or {} if it is absent or unreadable.""" + """Existing daemon.json, or {} if it is absent, unreadable, or not an object.""" if not os.path.exists(path): return {} try: with open(path) as fh: - return json.load(fh) or {} + cfg = json.load(fh) except (OSError, ValueError): # Docker cannot parse it either, so there is nothing worth preserving. return {} + # Valid JSON that isn't an object (a list, say) is not a config Docker can use, + # and would break the merge. Same treatment as unparseable. + return cfg if isinstance(cfg, dict) else {} def main(): diff --git a/cli/deployment/scripts/configure_docker_dns.sh b/cli/deployment/scripts/configure_docker_dns.sh index 150ec97e..6521e347 100755 --- a/cli/deployment/scripts/configure_docker_dns.sh +++ b/cli/deployment/scripts/configure_docker_dns.sh @@ -14,27 +14,34 @@ # This is an optimization, so it must never leave the box worse off than it found # it: if Docker will not come back with the new config, the previous daemon.json is # restored and Docker is restarted on it before we exit non-zero. +# +# Every privileged action here is one the box's sudoers file already grants +# NOPASSWD (see setup_and_deploy_box.sh): `install` from the fixed path +# /tmp/lager_daemon.json, and `systemctl restart docker`. Staging anywhere else -- +# a mktemp path, say -- silently falls outside the grant and makes every run prompt +# for a password. Snapshot and restore therefore route through that same path, and +# the backup lives in /tmp, where no privileges are needed to write it. set -euo pipefail SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" DAEMON_JSON="${DAEMON_JSON:-/etc/docker/daemon.json}" RESOLV_CONF="${RESOLV_CONF:-/run/systemd/resolve/resolv.conf}" -BACKUP="${DAEMON_JSON}.lager-bak" +STAGED="${STAGED:-/tmp/lager_daemon.json}" +BACKUP="${BACKUP:-/tmp/lager_daemon.json.bak}" -STAGED=$(mktemp) -trap 'rm -f "$STAGED"' EXIT +rm -f "$STAGED" "$BACKUP" 2>/dev/null || true +trap 'rm -f "$STAGED" "$BACKUP" 2>/dev/null || true' EXIT python3 "${SCRIPT_DIR}/configure_docker_dns.py" \ --resolv-conf "$RESOLV_CONF" --daemon-json "$DAEMON_JSON" --out "$STAGED" -# Snapshot the current config so a failed restart can be undone. Track whether -# there was one at all, so we restore the old file or remove ours accordingly. +# Snapshot the current config so a failed restart can be undone. daemon.json is +# world-readable, so this needs no sudo. Track whether there was one at all. if [ -f "$DAEMON_JSON" ]; then HAD_CONFIG=1 - sudo cp -f "$DAEMON_JSON" "$BACKUP" + cp -f "$DAEMON_JSON" "$BACKUP" else HAD_CONFIG=0 - sudo rm -f "$BACKUP" fi daemon_is_up() { @@ -52,11 +59,17 @@ daemon_is_up() { } restore_previous() { + # Where there was no daemon.json, an empty object restores Docker's defaults -- + # which is what the absent file meant. We write that rather than removing the + # file because `rm` on /etc/docker is not in the sudoers grant, and a recovery + # path that stops to ask for a password is a recovery path that does not run. if [ "$HAD_CONFIG" -eq 1 ]; then - sudo install -m 0644 "$BACKUP" "$DAEMON_JSON" + cp -f "$BACKUP" "$STAGED" else - sudo rm -f "$DAEMON_JSON" + echo '{}' > "$STAGED" fi + sudo install -m 0644 "$STAGED" "$DAEMON_JSON" \ + || echo "WARNING: could not restore ${DAEMON_JSON}" >&2 sudo systemctl restart docker || true } diff --git a/test/unit/cli/test_configure_docker_dns.py b/test/unit/cli/test_configure_docker_dns.py index 7e7ca964..b8203b84 100644 --- a/test/unit/cli/test_configure_docker_dns.py +++ b/test/unit/cli/test_configure_docker_dns.py @@ -148,3 +148,20 @@ def test_load_daemon_json_tolerates_missing_and_corrupt_files(tmp_path): valid = tmp_path / "valid.json" valid.write_text(json.dumps({"log-driver": "journald"})) assert load_daemon_json(str(valid)) == {"log-driver": "journald"} + + +@pytest.mark.parametrize("content", ["[1, 2]", '"a string"', "42", "null"]) +def test_load_daemon_json_rejects_valid_json_that_is_not_an_object(tmp_path, content): + """A daemon.json that isn't an object is no more usable to us than to Docker. + + Returning it would blow up the merge rather than fall back cleanly. + """ + path = tmp_path / "daemon.json" + path.write_text(content) + + assert load_daemon_json(str(path)) == {} + assert merge_dns(load_daemon_json(str(path)), ["192.168.1.1"])["dns"] == [ + "192.168.1.1", + "1.1.1.1", + "8.8.8.8", + ] diff --git a/test/unit/cli/test_configure_docker_dns_rollback.py b/test/unit/cli/test_configure_docker_dns_rollback.py index d1a02455..e48e548b 100644 --- a/test/unit/cli/test_configure_docker_dns_rollback.py +++ b/test/unit/cli/test_configure_docker_dns_rollback.py @@ -18,19 +18,16 @@ import json import os +import re import shutil import subprocess from pathlib import Path import pytest -SCRIPT = ( - Path(__file__).resolve().parents[3] - / "cli" - / "deployment" - / "scripts" - / "configure_docker_dns.sh" -) +SCRIPTS = Path(__file__).resolve().parents[3] / "cli" / "deployment" / "scripts" +SCRIPT = SCRIPTS / "configure_docker_dns.sh" +DEPLOY_SCRIPT = SCRIPTS / "setup_and_deploy_box.sh" ORIGINAL_CONFIG = {"log-driver": "journald", "dns": ["10.9.9.9"]} @@ -75,6 +72,8 @@ def _run(box, restart_rc, daemon_json_exists=True): PATH="{}:{}".format(bin_dir, env["PATH"]), DAEMON_JSON=str(daemon_json), RESOLV_CONF=str(tmp_path / "resolv.conf"), + STAGED=str(tmp_path / "lager_daemon.json"), + BACKUP=str(tmp_path / "lager_daemon.json.bak"), RESTART_RC=str(restart_rc), ) @@ -106,10 +105,40 @@ def test_failed_restart_restores_the_previous_config(box): assert "Restored the previous" in result.stderr +def test_privileged_commands_stay_within_the_sudoers_grant(): + """Every `sudo` here must match a NOPASSWD rule the installer writes. + + The box's sudoers file grants `install` from one fixed path, + /tmp/lager_daemon.json, and nothing else under /etc/docker. Staging elsewhere + (a mktemp path, say) or reaching for `sudo cp`/`sudo rm` still *works* -- it + just silently falls outside the grant and starts prompting for a password on + every run, including on the rollback path, where a prompt means the recovery + never happens. That failure is invisible wherever sudo credentials happen to be + cached, so pin it here rather than hope a hardware run catches it. + """ + code = "\n".join( + line for line in SCRIPT.read_text().splitlines() if not line.strip().startswith("#") + ) + grants = DEPLOY_SCRIPT.read_text() + + assert set(re.findall(r"\bsudo\s+(\S+)", code)) == {"install", "systemctl"} + assert 'STAGED="${STAGED:-/tmp/lager_daemon.json}"' in SCRIPT.read_text() + assert ( + "NOPASSWD: /usr/bin/install -m 0644 /tmp/lager_daemon.json /etc/docker/daemon.json" + in grants + ) + assert "NOPASSWD: /bin/systemctl restart docker" in grants + + @pytest.mark.skipif(shutil.which("bash") is None, reason="requires bash") -def test_failed_restart_removes_config_that_did_not_exist_before(box): - """Rolling back to "no daemon.json" means removing ours, not restoring an empty one.""" +def test_failed_restart_restores_defaults_when_there_was_no_config(box): + """With no daemon.json to go back to, roll back to Docker's defaults. + + An empty object is what the absent file meant. We write it rather than deleting + the file because `rm` on /etc/docker is not in the box's sudoers grant, and a + recovery path that stops to ask for a password is one that does not run. + """ result, daemon_json = _run(box, restart_rc=1, daemon_json_exists=False) assert result.returncode == 1 - assert not daemon_json.exists() + assert json.loads(daemon_json.read_text()) == {}