From ee25b00e87743ac26808161c185b97edbf4af856 Mon Sep 17 00:00:00 2001 From: Bryan Date: Wed, 23 Sep 2026 15:14:08 +0000 Subject: [PATCH] test(bind): cover bounded shutdown recovery --- .../test_bind_pull_request_workflow.py | 18 ++ .github/workflows/bind-tests.yml | 18 +- dns/bind/tests/bounded_shutdown_contract.py | 27 ++ dns/bind/tests/test_journal_lifecycle.py | 31 +- dns/bind/tests/test_log_visibility.py | 23 ++ .../tests/test_package_install_lifecycle.py | 2 +- dns/bind/tests/test_package_zone_backup.py | 10 +- dns/bind/tests/test_shutdown_timeout.py | 271 ++++++++++++++++++ 8 files changed, 390 insertions(+), 10 deletions(-) create mode 100644 dns/bind/tests/bounded_shutdown_contract.py create mode 100644 dns/bind/tests/test_log_visibility.py create mode 100644 dns/bind/tests/test_shutdown_timeout.py diff --git a/.github/ci/ci-tests/test_bind_pull_request_workflow.py b/.github/ci/ci-tests/test_bind_pull_request_workflow.py index 48a614850e..ae749aa6d7 100644 --- a/.github/ci/ci-tests/test_bind_pull_request_workflow.py +++ b/.github/ci/ci-tests/test_bind_pull_request_workflow.py @@ -60,6 +60,16 @@ def test_release_source_pull_requests_test_their_proposed_source(): assert 'if [[ "$PR_BASE" != "release/bind-rp/$SERIES" ]]' in workflow +def test_release_source_pull_requests_materialize_master_ci_helpers(): + workflow = workflow_text() + helper_job = workflow.split(' ci-helpers:', 1)[1].split(' discover:', 1)[0] + + assert 'PR_BASE: ${{ inputs.pull_request_base || github.event.pull_request.base.ref }}' in helper_job + assert 'if [[ "$PR_BASE" == release/bind-rp/* ]]' in helper_job + assert 'refs/heads/master:refs/remotes/origin/control-plane' in helper_job + assert '.github/ci .resolver-plugins/bind920.json' in helper_job + + def test_reusable_workflow_accepts_the_callers_pull_request_context(): workflow = workflow_text() @@ -69,6 +79,14 @@ def test_reusable_workflow_accepts_the_callers_pull_request_context(): assert 'PR_BASE: ${{ inputs.pull_request_base || github.event.pull_request.base.ref }}' in workflow +def test_release_source_pull_requests_always_use_master_canonical_tests(): + workflow = workflow_text() + test_job = workflow.split(' test:', 1)[1] + + assert 'if [[ "$PR_BASE" == release/bind-rp/* ]]' in test_job + assert 'refs/heads/master:refs/remotes/origin/canonical-tests' in test_job + + def test_workflow_has_read_only_permissions_and_pinned_actions(): workflow = workflow_text() references = action_references(workflow) diff --git a/.github/workflows/bind-tests.yml b/.github/workflows/bind-tests.yml index 7336bf8f11..216ece0729 100644 --- a/.github/workflows/bind-tests.yml +++ b/.github/workflows/bind-tests.yml @@ -69,6 +69,20 @@ jobs: with: ref: ${{ inputs.pull_request_sha || github.sha }} persist-credentials: false + - name: Materialize CI control plane + shell: bash + env: + PR_BASE: ${{ inputs.pull_request_base || github.event.pull_request.base.ref }} + run: | + set -euo pipefail + if [[ "$PR_BASE" == release/bind-rp/* ]]; then + git fetch --no-tags origin \ + 'refs/heads/master:refs/remotes/origin/control-plane' + git checkout refs/remotes/origin/control-plane -- \ + .github/ci .resolver-plugins/bind920.json + fi + test -d .github/ci + test -f .resolver-plugins/bind920.json - uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 with: python-version: '3.12.13' @@ -140,12 +154,14 @@ jobs: SERIES: ${{ matrix.series }} run: | set -euo pipefail - if [[ ! -d dns/bind/tests || ! -f .github/ci/metadata_profile.py ]]; then + if [[ "$PR_BASE" == release/bind-rp/* ]]; then git fetch --no-tags origin \ 'refs/heads/master:refs/remotes/origin/canonical-tests' git checkout refs/remotes/origin/canonical-tests -- \ .github/ci/metadata_profile.py dns/bind/tests fi + test -d dns/bind/tests + test -f .github/ci/metadata_profile.py source_commit=$(git rev-parse HEAD) if [[ "$PR_BASE" != "release/bind-rp/$SERIES" ]]; then source_ref="refs/heads/release/bind-rp/$SERIES" diff --git a/dns/bind/tests/bounded_shutdown_contract.py b/dns/bind/tests/bounded_shutdown_contract.py new file mode 100644 index 0000000000..3c0ab8a4eb --- /dev/null +++ b/dns/bind/tests/bounded_shutdown_contract.py @@ -0,0 +1,27 @@ +import json + + +BOUNDED_SHUTDOWN_THRESHOLDS = { + "26.1": None, + "26.7": ((26, 7), 6), +} + + +def current_release_requires_bounded_shutdown(bind_root): + metadata = bind_root.parents[1] / ".resolver-plugins/upstream.json" + series = json.loads(metadata.read_text())["series"] + try: + threshold = BOUNDED_SHUTDOWN_THRESHOLDS[series] + except KeyError as error: + raise ValueError(f"unsupported BIND release series: {series}") from error + if threshold is None: + return False + + values = {} + for line in (bind_root / "Makefile").read_text().splitlines(): + if line.startswith(("PLUGIN_VERSION=", "PLUGIN_REVISION=")): + key, value = line.split("=", 1) + values[key] = value.strip() + version = tuple(int(part) for part in values["PLUGIN_VERSION"].split(".")) + revision = int(values["PLUGIN_REVISION"]) + return (version, revision) >= threshold diff --git a/dns/bind/tests/test_journal_lifecycle.py b/dns/bind/tests/test_journal_lifecycle.py index 195343666c..53340b88e9 100644 --- a/dns/bind/tests/test_journal_lifecycle.py +++ b/dns/bind/tests/test_journal_lifecycle.py @@ -29,10 +29,16 @@ import tempfile import unittest +from .bounded_shutdown_contract import current_release_requires_bounded_shutdown + + +BIND_ROOT = pathlib.Path(__file__).resolve().parents[1] +BOUNDED_SHUTDOWN = current_release_requires_bounded_shutdown(BIND_ROOT) + class JournalLifecycleTest(unittest.TestCase): def test_stop_clears_journals_for_watcher_and_reverse_zones(self): - bind_root = pathlib.Path(__file__).resolve().parents[1] + bind_root = BIND_ROOT stop_script = bind_root / "src/opnsense/scripts/OPNsense/Bind/bindStop.py" with tempfile.TemporaryDirectory(dir=bind_root) as directory: @@ -62,7 +68,12 @@ def test_stop_clears_journals_for_watcher_and_reverse_zones(self): state.write_text("{}") events = temporary / "events" named = temporary / "named" - named.write_text("#!/bin/sh\nprintf '%s\\n' \"$*\" >> \"$TEST_EVENTS\"\n") + named.write_text( + "#!/bin/sh\n" + "printf '%s\\n' \"$*\" >> \"$TEST_EVENTS\"\n" + "[ \"$1\" = status ] && exit 1\n" + "exit 0\n" + ) named.chmod(0o755) result = subprocess.run( @@ -81,14 +92,14 @@ def test_stop_clears_journals_for_watcher_and_reverse_zones(self): ) self.assertEqual(result.returncode, 0, result.stderr) - self.assertEqual(events.read_text(), "stop\n") + self.assertEqual(events.read_text(), "status\n" if BOUNDED_SHUTDOWN else "stop\n") self.assertFalse(state.exists()) for zone in ("forward.example", "watcher.example", "1.168.192.in-addr.arpa"): for suffix in (".jnl", ".jnw", ".jbk"): self.assertFalse((zone_dir / f"{zone}.db{suffix}").exists()) def assert_stop_failure_preserves_journals_and_state(self, status_code): - bind_root = pathlib.Path(__file__).resolve().parents[1] + bind_root = BIND_ROOT stop_script = bind_root / "src/opnsense/scripts/OPNsense/Bind/bindStop.py" with tempfile.TemporaryDirectory(dir=bind_root) as directory: @@ -127,7 +138,10 @@ def assert_stop_failure_preserves_journals_and_state(self, status_code): ) self.assertEqual(result.returncode, 1, result.stderr) - self.assertEqual(events.read_text(), "stop\nstatus\n") + self.assertEqual( + events.read_text(), + "status\n" if BOUNDED_SHUTDOWN else "stop\nstatus\n", + ) self.assertTrue(state.exists()) self.assertTrue(journal.exists()) @@ -137,7 +151,7 @@ def test_stop_failure_preserves_journals_and_state(self): self.assert_stop_failure_preserves_journals_and_state(status_code) def test_already_stopped_clears_journals_and_state(self): - bind_root = pathlib.Path(__file__).resolve().parents[1] + bind_root = BIND_ROOT stop_script = bind_root / "src/opnsense/scripts/OPNsense/Bind/bindStop.py" with tempfile.TemporaryDirectory(dir=bind_root) as directory: @@ -174,7 +188,10 @@ def test_already_stopped_clears_journals_and_state(self): ) self.assertEqual(result.returncode, 0, result.stderr) - self.assertEqual(events.read_text(), "stop\nstatus\n") + self.assertEqual( + events.read_text(), + "status\n" if BOUNDED_SHUTDOWN else "stop\nstatus\n", + ) self.assertFalse(state.exists()) self.assertFalse(journal.exists()) diff --git a/dns/bind/tests/test_log_visibility.py b/dns/bind/tests/test_log_visibility.py new file mode 100644 index 0000000000..95e0070df9 --- /dev/null +++ b/dns/bind/tests/test_log_visibility.py @@ -0,0 +1,23 @@ +# Copyright (C) 2026 Bryan Wiegand +# All rights reserved. + +from pathlib import Path + +import pytest + +from .bounded_shutdown_contract import current_release_requires_bounded_shutdown + + +BIND_ROOT = Path(__file__).resolve().parents[1] +STOP_SCRIPT = BIND_ROOT / "src/opnsense/scripts/OPNsense/Bind/bindStop.py" + +if not current_release_requires_bounded_shutdown(BIND_ROOT): + pytestmark = pytest.mark.skip(reason="release predates bounded BIND shutdown") + + +def test_general_log_includes_informational_lifecycle_messages(): + view = (BIND_ROOT / "src/opnsense/mvc/app/views/OPNsense/Bind/logs.volt").read_text() + stop = STOP_SCRIPT.read_text() + + assert "'default_log_severity':'Informational'" in view + assert 'syslog.openlog("named")' in stop diff --git a/dns/bind/tests/test_package_install_lifecycle.py b/dns/bind/tests/test_package_install_lifecycle.py index a52604217b..324635bb85 100644 --- a/dns/bind/tests/test_package_install_lifecycle.py +++ b/dns/bind/tests/test_package_install_lifecycle.py @@ -267,7 +267,7 @@ def test_normal_stop_keeps_primary_journals_and_clears_reverse_scope(executable_ for zone in zones: (zone_dir / f"{zone}.db.jnl").write_text("") named = executable_tmp_path / "named-stop" - _write_executable(named, "#!/bin/sh\nexit 0\n") + _write_executable(named, '#!/bin/sh\n[ "$1" = status ] && exit 1\nexit 0\n') watcher = executable_tmp_path / "watcher.conf" watcher.write_text("") diff --git a/dns/bind/tests/test_package_zone_backup.py b/dns/bind/tests/test_package_zone_backup.py index ec83b22e02..ec945e4f11 100644 --- a/dns/bind/tests/test_package_zone_backup.py +++ b/dns/bind/tests/test_package_zone_backup.py @@ -8,6 +8,7 @@ import pytest +from .bounded_shutdown_contract import current_release_requires_bounded_shutdown from .package_lifecycle_contract import current_release_requires_lifecycle @@ -16,6 +17,7 @@ BIND_ROOT / "src/opnsense/scripts/OPNsense/Bind/bindPackageZones.py" ) MAKEFILE = BIND_ROOT / "Makefile" +BOUNDED_SHUTDOWN = current_release_requires_bounded_shutdown(BIND_ROOT) if MAKEFILE.is_file(): version_line = next( @@ -96,6 +98,7 @@ def _fixture(tmp_path, *, freeze_failure=""): named, """#!/bin/sh printf 'named %s\n' "$1" >> "$TEST_EVENTS" +[ "$1" = status ] && exit 1 exit 0 """, ) @@ -156,11 +159,16 @@ def test_package_backup_preserves_non_watcher_dynamic_record(executable_tmp_path ) assert discard.returncode == 0, discard.stderr assert not backup.exists() + stop_event = ( + "named status" + if BOUNDED_SHUTDOWN + else "named stop" + ) assert events.read_text().splitlines() == [ "rndc freeze 1.168.192.in-addr.arpa", "rndc freeze dynamic.example", "rndc freeze watcher.example", - "named stop", + stop_event, ] diff --git a/dns/bind/tests/test_shutdown_timeout.py b/dns/bind/tests/test_shutdown_timeout.py new file mode 100644 index 0000000000..c905852078 --- /dev/null +++ b/dns/bind/tests/test_shutdown_timeout.py @@ -0,0 +1,271 @@ +# Copyright (C) 2026 Bryan Wiegand +# All rights reserved. + +import os +import pathlib +import signal +import subprocess +import sys +import tempfile +import time + +import pytest + +from .bounded_shutdown_contract import current_release_requires_bounded_shutdown + + +BIND_ROOT = pathlib.Path(__file__).resolve().parents[1] +STOP_SCRIPT = BIND_ROOT / "src/opnsense/scripts/OPNsense/Bind/bindStop.py" + +if not current_release_requires_bounded_shutdown(BIND_ROOT): + pytestmark = pytest.mark.skip(reason="release predates bounded BIND shutdown") + + +@pytest.fixture +def executable_tmp_path(): + with tempfile.TemporaryDirectory(dir=BIND_ROOT) as directory: + yield pathlib.Path(directory) + + +def write_executable(path, source): + path.write_text(source) + path.chmod(0o755) + + +def start_process(tmp_path, name, *, events=None, ignore_term=False): + pidfile = tmp_path / f"{name}.pid" + events = events or tmp_path / "events" + if not events.exists(): + events.write_text("") + supervisor = tmp_path / f"{name}-supervisor.py" + supervisor.write_text( + """import os +import pathlib +import signal +import sys + +pidfile = pathlib.Path(sys.argv[1]) +events = pathlib.Path(sys.argv[2]) +name = sys.argv[3] +ignore_term = sys.argv[4] == "yes" +pid = os.fork() +if pid: + _, status = os.waitpid(pid, 0) + raise SystemExit(os.waitstatus_to_exitcode(status)) + +def stop(_signum, _frame): + with events.open("a") as output: + output.write(f"{name}-term\\n") + raise SystemExit(0) + +signal.signal(signal.SIGTERM, signal.SIG_IGN if ignore_term else stop) +pidfile.write_text(str(os.getpid())) +while True: + signal.pause() +""" + ) + process = subprocess.Popen( + [sys.executable, supervisor, pidfile, events, name, "yes" if ignore_term else "no"] + ) + for _ in range(100): + if pidfile.exists(): + return process, int(pidfile.read_text()), pidfile, events + time.sleep(0.01) + process.kill() + raise AssertionError("test named process did not start") + + +def stop_process(supervisor, child_pid, sig=signal.SIGKILL): + if supervisor.poll() is not None: + return + try: + os.kill(child_pid, sig) + except ProcessLookupError: + pass + try: + supervisor.wait(timeout=2) + except subprocess.TimeoutExpired: + supervisor.kill() + supervisor.wait() + + +def run_stop( + tmp_path, + pidfile, + events, + *, + watcher_pidfile=None, + named_rc_source=None, + rndc_source=None, + extra_env=None, +): + named_rc = tmp_path / "named-rc" + write_executable( + named_rc, + named_rc_source or """#!/bin/sh +if [ "$1" = status ]; then + kill -0 "$(cat "$TEST_PIDFILE")" 2>/dev/null + exit $? +fi +printf 'rc-stop\\n' >> "$TEST_EVENTS" +exit 0 +""", + ) + rndc = tmp_path / "rndc" + write_executable( + rndc, + rndc_source or "#!/bin/sh\nprintf 'rndc\\n' >> \"$TEST_EVENTS\"\n", + ) + config = tmp_path / "config.xml" + config.write_text("") + + environment = os.environ | { + "BIND_STOP_CONFIG": str(config), + "BIND_STOP_FORCE_TIMEOUT": "0.2", + "BIND_STOP_GRACE_TIMEOUT": "0.2", + "BIND_STOP_NAMED_PIDFILE": str(pidfile), + "BIND_STOP_NAMED_RC": str(named_rc), + "BIND_STOP_RNDC": str(rndc), + "BIND_STOP_STATE_FILE": str(tmp_path / "state.json"), + "BIND_STOP_WATCHER_CONFIG": str(tmp_path / "watcher.conf"), + "BIND_STOP_WATCHER_PIDFILE": str(watcher_pidfile or tmp_path / "watcher.pid"), + "BIND_STOP_ZONE_DIR": str(tmp_path), + "TEST_EVENTS": str(events), + "TEST_PIDFILE": str(pidfile), + } + environment.update(extra_env or {}) + return subprocess.run( + [sys.executable, STOP_SCRIPT], + env=environment, + capture_output=True, + text=True, + timeout=5, + check=False, + ) + + +def test_graceful_timeout_escalates_to_term(executable_tmp_path): + process, child_pid, pidfile, events = start_process(executable_tmp_path, "named") + try: + result = run_stop(executable_tmp_path, pidfile, events) + + assert result.returncode == 0, result.stderr + process.wait(timeout=2) + assert events.read_text().splitlines() == ["rndc", "named-term"] + finally: + stop_process(process, child_pid) + + +def test_term_timeout_escalates_to_kill(executable_tmp_path): + process, child_pid, pidfile, events = start_process( + executable_tmp_path, "named", ignore_term=True + ) + try: + result = run_stop(executable_tmp_path, pidfile, events) + + assert result.returncode == 0, result.stderr + process.wait(timeout=2) + assert events.read_text().splitlines() == ["rndc"] + finally: + stop_process(process, child_pid) + + +def test_rndc_timeout_is_bounded_and_escalates(executable_tmp_path): + process, child_pid, pidfile, events = start_process(executable_tmp_path, "named") + try: + started = time.monotonic() + result = run_stop( + executable_tmp_path, + pidfile, + events, + rndc_source=( + "#!/bin/sh\n" + "printf 'rndc\\n' >> \"$TEST_EVENTS\"\n" + "while :; do :; done\n" + ), + ) + + assert result.returncode == 0, result.stderr + assert time.monotonic() - started < 2 + process.wait(timeout=2) + assert events.read_text().splitlines() == ["rndc", "named-term"] + finally: + stop_process(process, child_pid) + + +def test_pid_change_prevents_signaling_replacement_process(executable_tmp_path): + named, named_pid, pidfile, events = start_process(executable_tmp_path, "named") + replacement, replacement_pid, replacement_pidfile, _ = start_process( + executable_tmp_path, + "replacement", + events=events, + ) + try: + result = run_stop( + executable_tmp_path, + pidfile, + events, + rndc_source=( + "#!/bin/sh\n" + "printf 'rndc\\n' >> \"$TEST_EVENTS\"\n" + "cp \"$TEST_REPLACEMENT_PIDFILE\" \"$TEST_PIDFILE\"\n" + ), + extra_env={"TEST_REPLACEMENT_PIDFILE": str(replacement_pidfile)}, + ) + + assert result.returncode == 1 + assert named.poll() is None + assert replacement.poll() is None + assert events.read_text().splitlines() == ["rndc"] + finally: + stop_process(named, named_pid, signal.SIGTERM) + stop_process(replacement, replacement_pid, signal.SIGTERM) + + +def test_watcher_stops_after_named_shutdown(executable_tmp_path): + named, named_pid, pidfile, events = start_process(executable_tmp_path, "named") + watcher, watcher_pid, watcher_pidfile, _ = start_process( + executable_tmp_path, "watcher", events=events + ) + try: + result = run_stop( + executable_tmp_path, + pidfile, + events, + watcher_pidfile=watcher_pidfile, + ) + + assert result.returncode == 0, result.stderr + named.wait(timeout=2) + watcher.wait(timeout=2) + assert events.read_text().splitlines() == [ + "rndc", + "named-term", + "watcher-term", + ] + finally: + stop_process(named, named_pid) + stop_process(watcher, watcher_pid) + + +def test_preflight_failure_leaves_watcher_running(executable_tmp_path): + named, named_pid, pidfile, events = start_process(executable_tmp_path, "named") + watcher, watcher_pid, watcher_pidfile, _ = start_process( + executable_tmp_path, "watcher", events=events + ) + try: + result = run_stop( + executable_tmp_path, + pidfile, + events, + watcher_pidfile=watcher_pidfile, + named_rc_source="#!/bin/sh\nexit 2\n", + ) + + assert result.returncode == 1 + assert named.poll() is None + assert watcher.poll() is None + assert events.read_text() == "" + finally: + stop_process(named, named_pid, signal.SIGTERM) + stop_process(watcher, watcher_pid, signal.SIGTERM)