From 7543f2b97e7e88038f60e31d041675c997793a27 Mon Sep 17 00:00:00 2001 From: Roger Luethi Date: Mon, 14 Sep 2026 17:57:02 +0200 Subject: [PATCH] tasks: report a missing vault password as such `osism apply` resolves the Ansible Vault password from one of two sources, and when neither is available it reports the wrong one. When Redis holds the password, VAULT points at the Redis-backed helper. When it does not, VAULT is left unset, and the run script in the worker container resolves VAULT=${VAULT:-$ENVIRONMENTS_DIRECTORY/.vault_pass} Every one of those scripts passes --vault-password-file unconditionally, so if that file is absent -- as it is in a configuration repository generated by a recent cfg-cookiecutter -- the play dies with ERROR! The vault password file /opt/configuration/environments/.vault_pass was not found That sends the operator looking for a missing file in the configuration repository. The state that actually holds is "the worker has no vault password", and the remedy is one command, `osism set vault password`, which is neither named by the message nor suggested by the file name. This matters more than a one-off install-time mistake would, because the condition recurs by design: the password lives only in Redis, which the manager compose file runs with no volume, so it has to be re-supplied after every manager update, upgrade or Redis image bump. That also makes environments/.vault_pass the only unattended path, which is why the new message names it alongside the interactive one. This module is the only layer that sees both sources -- it has already read Redis, and the configuration directory is mounted here -- so it is the only place the complete condition can be detected and named. The alternative sites (20 run scripts across two container-image repos) see only the file and could not mention `osism set vault password` at all. Detect the condition before dispatch and fail with a message naming the state and both remedies. Behaviour is otherwise unchanged: the run already failed in this state, and failing is correct, since there is no TTY in the worker to prompt on. The message is pushed to the task output stream and the rc published before raising, so the CLI paths that read both back out of Redis are unaffected. The check has to track both inputs of the expansion above, not just the file. The subprocess inherits this process' environment, so an operator can point the script at a custom password file or helper by setting VAULT -- manager_environment_extra is rendered verbatim into ansible.env, which is an env_file on every ansible worker container. The script then prefers that value and never looks at the fallback file, so a set VAULT suppresses the check; its value is not validated here, since it may name an executable helper rather than a file. The test is on the value rather than on the key, because ${:-} substitutes on null as well as on unset: an empty VAULT is the fallback case. Tests: the existing test_run_ansible_vault_env_absent_without_password covered "no password in Redis" without saying anything about the file, which is now two distinct outcomes. It is replaced by test_run_ansible_vault_env_absent_when_only_the_file_exists, which pins the unchanged fallback, plus new tests for the failure path and for the set, empty and inherited spellings of VAULT. Assisted-by: Claude:claude-opus-5 Signed-off-by: Roger Luethi --- osism/tasks/__init__.py | 33 ++++++++++ tests/unit/tasks/test_init.py | 121 +++++++++++++++++++++++++++++++++- 2 files changed, 153 insertions(+), 1 deletion(-) diff --git a/osism/tasks/__init__.py b/osism/tasks/__init__.py index c0c464a19..adb9f0b2f 100644 --- a/osism/tasks/__init__.py +++ b/osism/tasks/__init__.py @@ -50,6 +50,10 @@ ISOLATED_ENV_PREFIXES = ("LC_",) +# The path the run scripts in the worker containers fall back to when VAULT is +# unset: CONFIGURATION_DIRECTORY/environments/.vault_pass, both hardcoded there. +ANSIBLE_VAULT_PASSWORD_FILE = "/opt/configuration/environments/.vault_pass" + class AnsibleFailure(Exception): """Raised when an Ansible run in a worker container exits non-zero. @@ -300,6 +304,35 @@ def run_ansible_in_environment( ansible_vault_password = utils.redis.get("ansible_vault_password") if ansible_vault_password: env["VAULT"] = "/ansible-vault.py" + elif not env.get("VAULT") and not os.path.exists(ANSIBLE_VAULT_PASSWORD_FILE): + # The run script resolves VAULT=${VAULT:-}, so both of its + # inputs have to be checked here. A non-empty inherited VAULT is an + # operator pointing at a custom password file or helper + # (manager_environment_extra reaches the worker containers as plain + # environment variables); the script prefers it and never looks at + # the file below. ${:-} substitutes on null as well as on unset, so + # an empty VAULT is the fallback case, not the operator one. + # + # With neither, the script falls back to the file, and every one of + # them passes --vault-password-file unconditionally, so the play + # dies with "The vault password file ... was not found". That + # message sends the operator looking for a file in the configuration + # repository, while the state that actually holds is "the worker has + # no vault password" -- and it recurs by design, because the password + # lives only in Redis, which the manager runs without a volume. This + # is the only layer that sees both sources, so it is the only one + # that can say so. Report it here and do not dispatch. + message = ( + "No Ansible Vault password is available. Run " + "'osism set vault password' on the manager, or add " + "environments/.vault_pass to the configuration repository " + "for unattended operation." + ) + logger.error(message) + if publish: + utils.push_task_output(request_id, f"{message}\n") + utils.finish_task_output(request_id, rc=1) + raise AnsibleFailure(message) # Log play execution start log_play_execution( diff --git a/tests/unit/tasks/test_init.py b/tests/unit/tasks/test_init.py index bfabb1055..9c420abbf 100644 --- a/tests/unit/tasks/test_init.py +++ b/tests/unit/tasks/test_init.py @@ -327,8 +327,16 @@ def runner_mocks(mocker, tmp_path, mock_redis, monkeypatch): popen = mocker.patch("osism.tasks.subprocess.Popen") popen.return_value = make_process(["ok: [node-1]\n"]) + # The default is a deployment that can run: no password in Redis, but the + # configuration repository carries the file the run scripts fall back to. + # Tests for the empty-Redis case unlink it. + vault_file = tmp_path / ".vault_pass" + vault_file.write_text("secret\n") + mocker.patch.object(tasks, "ANSIBLE_VAULT_PASSWORD_FILE", str(vault_file)) + return SimpleNamespace( popen=popen, + vault_file=vault_file, create_redlock=mocker.patch("osism.tasks.utils.create_redlock"), push=mocker.patch("osism.tasks.utils.push_task_output"), finish=mocker.patch("osism.tasks.utils.finish_task_output"), @@ -514,13 +522,124 @@ def test_run_ansible_vault_env_set_when_password_present(runner_mocks): assert env["VAULT"] == "/ansible-vault.py" -def test_run_ansible_vault_env_absent_without_password(runner_mocks): +def test_run_ansible_vault_env_absent_when_only_the_file_exists(runner_mocks): + """No password in Redis but a file in the configuration repository is a + working deployment: VAULT stays unset so the run script falls back to + ``$ENVIRONMENTS_DIRECTORY/.vault_pass`` itself.""" runner_mocks.redis.get.return_value = None run_ansible() env = runner_mocks.popen.call_args.kwargs["env"] assert "VAULT" not in env +def test_run_ansible_vault_file_not_consulted_when_redis_has_password(runner_mocks): + """The Redis password wins, so a configuration repository without the file + -- the state a freshly generated one is in -- still runs.""" + runner_mocks.redis.get.return_value = b"secret" + runner_mocks.vault_file.unlink() + run_ansible() + assert runner_mocks.popen.call_args.kwargs["env"]["VAULT"] == "/ansible-vault.py" + + +def test_run_ansible_no_vault_password_anywhere_raises_before_dispatch(runner_mocks): + """With neither source available the run script would abort with "The vault + password file ... was not found", pointing at the configuration repository + instead of at the worker state that actually holds. Both sources are only + visible here, so the play is not started at all.""" + runner_mocks.redis.get.return_value = None + runner_mocks.vault_file.unlink() + + with pytest.raises(tasks.AnsibleFailure): + run_ansible() + + runner_mocks.popen.assert_not_called() + + +def test_run_ansible_no_vault_password_names_both_remedies(runner_mocks): + """The interactive remedy is not discoverable from the state, and the file + is the only unattended one -- so the message has to name both.""" + runner_mocks.redis.get.return_value = None + runner_mocks.vault_file.unlink() + + with pytest.raises(tasks.AnsibleFailure) as excinfo: + run_ansible() + + message = str(excinfo.value) + assert "osism set vault password" in message + assert "environments/.vault_pass" in message + + +def test_run_ansible_inherited_vault_env_suppresses_the_check( + runner_mocks, monkeypatch +): + """An operator can point the run script at a custom password file or helper + by putting VAULT in manager_environment_extra, which reaches this process as + an inherited environment variable. The script prefers it over its own + fallback, so the fallback file being absent is not a failure then.""" + monkeypatch.setenv("VAULT", "/opt/configuration/environments/custom.vault_pass") + runner_mocks.redis.get.return_value = None + runner_mocks.vault_file.unlink() + + run_ansible() + + env = runner_mocks.popen.call_args.kwargs["env"] + assert env["VAULT"] == "/opt/configuration/environments/custom.vault_pass" + + +def test_run_ansible_empty_vault_env_does_not_suppress_the_check( + runner_mocks, monkeypatch +): + """``${VAULT:-}`` substitutes on null as well as on unset, so an + empty VAULT leaves the run script on the fallback file and the check has to + treat it exactly like an absent one.""" + monkeypatch.setenv("VAULT", "") + runner_mocks.redis.get.return_value = None + runner_mocks.vault_file.unlink() + + with pytest.raises(tasks.AnsibleFailure): + run_ansible() + + runner_mocks.popen.assert_not_called() + + +def test_run_ansible_no_vault_password_streams_message_and_rc(runner_mocks): + """``osism apply`` reads both the message and the rc back out of the Redis + output stream, so raising alone would leave it waiting for output that + never arrives.""" + runner_mocks.redis.get.return_value = None + runner_mocks.vault_file.unlink() + + with pytest.raises(tasks.AnsibleFailure): + run_ansible() + + pushed = "".join(call.args[1] for call in runner_mocks.push.call_args_list) + assert "osism set vault password" in pushed + runner_mocks.finish.assert_called_once_with("req-1", rc=1) + + +def test_run_ansible_no_vault_password_publish_false_does_not_stream(runner_mocks): + runner_mocks.redis.get.return_value = None + runner_mocks.vault_file.unlink() + + with pytest.raises(tasks.AnsibleFailure): + run_ansible(publish=False) + + runner_mocks.push.assert_not_called() + runner_mocks.finish.assert_not_called() + + +def test_run_ansible_no_vault_password_cleans_ssh_dir(runner_mocks): + """The raise happens inside the try whose ``finally`` removes the per-task + ControlPath directory.""" + runner_mocks.redis.get.return_value = None + runner_mocks.vault_file.unlink() + + with pytest.raises(tasks.AnsibleFailure): + run_ansible() + + runner_mocks.rmtree.assert_called_once() + + # -- worker dispatch --