fix(secrets): read a secret without asking for a terminal

alakazam.py

@@ -79,6 +79,8 @@ CONNECTION_ERRORS = (
 MISSING_SECRET_ERROR = "doesn't exist on server"
 # abra subcommands that take a secret as an argument or return one
 SECRET_COMMANDS = ("insert", "generate")
+# marker in the abra output for an app that is not running, which is not an error to its callers
+NOT_DEPLOYED_ERROR = "is not deployed?"
 # markers in the abra output for a secret run that had nothing left to do, which abra exits 1 on
 NO_SECRETS_GENERATED = "no secrets generated"
 NO_SECRETS_TO_REMOVE = "no secrets to remove"
@@ -782,11 +784,30 @@ def abra(*args: str, machine_output: bool = False, ignore_error: bool = False, s
             f'{" ".join(command)} \n STDOUT: \n {process.stdout.decode()} \n STDERR: {process.stderr.decode()}')
     if machine_output:
         if output:= process.stdout.decode():
-            return json.loads(output)
+            return parse_machine_output(output)
         else:
             return {}
     return process.stdout.decode()
 
+
+def parse_machine_output(output: str) -> Any:
+    """
+    Parses the json abra writes with '-m', which some commands spread over one document per line.
+
+    'recipe versions' prints a document per recipe version instead of one for the whole answer, so
+    reading the output as a single document stops after the first line with "Extra data".
+
+    Args:
+        output (str): The stdout of an abra call that asked for machine output
+
+    Returns:
+        Any: The document abra wrote, or the list of them when it wrote more than one
+    """
+    try:
+        return json.loads(output)
+    except json.JSONDecodeError:
+        return [json.loads(line) for line in output.splitlines() if line.strip()]
+
 def sync_repo(path: Path) -> None:
     """
     Fetches a git repository and fast-forwards it, mirroring what abra does when it is online.
@@ -1190,18 +1211,8 @@ def share_secrets(app1_domain: str, app2_domain: str, secrets: Dict[str, str], s
     app1_stored_secrets = {x['name']: str2bool(x['created on server']) for x in app1_stored_secrets}
     app2_stored_secrets = abra("app", "secret", "ls", app2_domain, machine_output=True)
     app2_stored_secrets = {x['name']: str2bool(x['created on server']) for x in app2_stored_secrets}
-    try:
-        app1_container = abra("app" ,"ps", "-m", app1_domain, machine_output=True)
-        app1_container = list(app1_container)
-    except RuntimeError:
-        # App1 is not deployed
-        app1_container = []
-    try:
-        app2_container = abra("app" ,"ps", "-m", app2_domain, machine_output=True)
-        app2_container = list(app2_container)
-    except RuntimeError:
-        # App2 is not deployed
-        app2_container = []
+    app1_container = get_running_containers(app1_domain)
+    app2_container = get_running_containers(app2_domain)
     for app2_secret in secrets:
         app1_secret = secrets[app2_secret]
         try:
@@ -1273,6 +1284,31 @@ def share_secrets(app1_domain: str, app2_domain: str, secrets: Dict[str, str], s
                 )
 
 
+def get_running_containers(domain: str) -> List:
+    """
+    Lists the containers of an app, treating an app that is not deployed as having none.
+
+    Every other failure is raised: reading it as "not running" would turn a server that cannot be
+    reached into a silently skipped secret exchange.
+
+    Args:
+        domain (str): The app domain to read the containers of
+
+    Returns:
+        list: The containers abra reports, empty when the app is not deployed
+
+    Raises:
+        RuntimeError: If abra fails for any other reason
+    """
+    try:
+        return list(abra("app", "ps", "-m", domain, machine_output=True))
+    except RuntimeError as error:
+        if NOT_DEPLOYED_ERROR not in str(error):
+            raise
+        logging.info(f"{domain} is not deployed, it holds no containers to read a secret from")
+        return []
+
+
 def get_secret(domain: str, secret_name: str, containers: List) -> str:
     """
     Retrieves a specified secret from the application specified by its domain using the 'abra' command.
@@ -1287,7 +1323,9 @@ def get_secret(domain: str, secret_name: str, containers: List) -> str:
     """
     for container in containers:
         try:
-            secret = abra("app", "run", domain, container, "cat", f"/var/run/secrets/{secret_name}")
+            # -t is --no-tty: a run that asks for a terminal has none in ci, and a terminal would
+            # translate the line endings of the value on its way out
+            secret = abra("app", "run", "-t", domain, container, "cat", f"/var/run/secrets/{secret_name}")
             return str(secret)
         except RuntimeError:
             continue
@@ -1297,6 +1335,30 @@ def get_secret(domain: str, secret_name: str, containers: List) -> str:
     return get_secret_from_host(domain, secret_name)
 
 
+def run_over_ssh(host: str, command: str, purpose: str) -> str:
+    """
+    Runs a command on a host over ssh and returns its output, refusing to read a failure as empty.
+
+    Args:
+        host (str): The host to run the command on
+        command (str): The shell command to run there
+        purpose (str): What the command is for, used in the error message
+
+    Returns:
+        str: The stdout of the command, unchanged
+
+    Raises:
+        RuntimeError: If ssh or the command itself fails
+    """
+    result = subprocess.run(["ssh", host, command], capture_output=True, text=True)
+    if result.returncode:
+        raise RuntimeError(f"{purpose} on {host} exited with code {result.returncode}: "
+                           f"{result.stderr.strip() or 'no output'}")
+    if error := result.stderr.strip():
+        logging.warning(f"{purpose} on {host} wrote to stderr: {error}")
+    return result.stdout
+
+
 def get_secret_from_host(domain: str, secret_name: str) -> str:
     """
     Read a Docker Swarm secret value by SSHing to the host and reading from the
@@ -1324,8 +1386,7 @@ def get_secret_from_host(domain: str, secret_name: str) -> str:
         f"    '.[]? | .SecretID + \" \" + $cid + \" \" + .SecretName'; "
         f"done | grep ' {stack_name}_{secret_name}_' | head -1"
     )
-    result = subprocess.run(["ssh", host, find_cmd], capture_output=True, text=True)
-    match_line = result.stdout.strip()
+    match_line = run_over_ssh(host, find_cmd, f"the lookup of secret '{secret_name}'").strip()
     if not match_line:
         raise RuntimeError(f"Secret '{secret_name}' not found in stack '{stack_name}' on {host}")
 
@@ -1337,8 +1398,8 @@ def get_secret_from_host(domain: str, secret_name: str) -> str:
         f"cat /var/lib/docker/containers/{container_id}/mounts/secrets/{secret_id} 2>/dev/null "
         f"|| sudo cat /var/lib/docker/containers/{container_id}/mounts/secrets/{secret_id}"
     )
-    read_result = subprocess.run(["ssh", host, read_cmd], capture_output=True, text=True)
-    value = read_result.stdout  # no strip — trailing newline could be part of the secret
+    # no strip — a trailing newline could be part of the secret
+    value = run_over_ssh(host, read_cmd, f"the read of secret '{secret_name}'")
     if not value:
         raise RuntimeError(f"Could not read value for secret '{secret_name}' on {host}")
     return value
@@ -2071,31 +2132,50 @@ def upgrade(recipes: Tuple[str], execute_hooks: bool, dry_run: bool, redeploy: b
                 execute_cmds(app_config, upgrade=True, dry_run=True, instance_domain=instance_domain)
         return
     if noninteractive or input("Do you really want to upgrade these apps? Type YES: ") == "YES":
+        upgraded, failed = [], []
         for app, app_config, upgrade_cmd, instance_domain in upgrade_cmds:
             app_domain = app_config.get('app_domain')
-            # a recipe can bring a new secret along, which abra refuses to deploy without. The app
-            # is running, so nothing else can be missing: it had all of them when it was deployed
-            if missing := get_missing_secrets(app_domain):
-                print(f"{app_domain} is missing {len(missing)} secret(s): {', '.join(sorted(missing))}")
-                create_secrets((app,), instances=(instance_domain,))
-            if redeploy:
-                upgrade_cmd.pop(0)
-                upgrade_cmd.insert(0, 'deploy')
-                print(f'undeploy {app_domain}')
-                print(abra("app", "undeploy", '--no-input', app_domain))
-                print(f'waiting {redeploy_wait}s …')
-                sleep(redeploy_wait)
-            print(f'deploy {app_domain}')
-            print(abra("app", *upgrade_cmd))
-        if not upgrade_cmds:
-            return
-        # the deployments were started without waiting for each other, so the services are still
-        # settling here and a hook would reach a container that is not up again yet
-        wait_for_healthy_apps(tuple({app for app, *_ in upgrade_cmds}), timeout, settle=settle)
-        if execute_hooks:
-            for app, app_config, _, instance_domain in upgrade_cmds:
-                logging.info(f"execute commands for {app_config.get('app_domain')}")
-                execute_cmds(app_config, upgrade=True, instance_domain=instance_domain)
+            try:
+                # a recipe can bring a new secret along, which abra refuses to deploy without. The
+                # app is running, so nothing else can be missing: it had all of them when deployed
+                if missing := get_missing_secrets(app_domain):
+                    print(f"{app_domain} is missing {len(missing)} secret(s): {', '.join(sorted(missing))}")
+                    create_secrets((app,), instances=(instance_domain,))
+                if redeploy:
+                    upgrade_cmd.pop(0)
+                    upgrade_cmd.insert(0, 'deploy')
+                    print(f'undeploy {app_domain}')
+                    print(abra("app", "undeploy", '--no-input', app_domain))
+                    print(f'waiting {redeploy_wait}s …')
+                    sleep(redeploy_wait)
+                print(f'deploy {app_domain}')
+                print(abra("app", *upgrade_cmd))
+            except RuntimeError as error:
+                # the apps behind this one do not depend on it, so they are still upgraded and the
+                # run ends on the failure instead of dropping everything where it stands
+                logging.error(f"upgrade of {app_domain} failed: {error}")
+                failed.append(app_domain)
+                continue
+            upgraded.append((app, app_config, instance_domain))
+        problems = []
+        if failed:
+            problems.append(
+                f"{len(failed)} of {len(upgrade_cmds)} app(s) failed to upgrade: {', '.join(failed)}")
+        if upgraded:
+            try:
+                # the deployments were started without waiting for each other, so the services are
+                # still settling here and a hook would reach a container that is not up again yet
+                wait_for_healthy_apps(tuple({app for app, *_ in upgraded}), timeout, settle=settle)
+            except click.ClickException as error:
+                # collected rather than raised, so that it cannot hide the apps that failed before
+                problems.append(error.message)
+            else:
+                if execute_hooks:
+                    for app, app_config, instance_domain in upgraded:
+                        logging.info(f"execute commands for {app_config.get('app_domain')}")
+                        execute_cmds(app_config, upgrade=True, instance_domain=instance_domain)
+        if problems:
+            raise click.ClickException("\n".join(problems))
 
 
 @cli.command()
@@ -2412,8 +2492,13 @@ def get_latest_version(app: str) -> str:
     Returns:
         str: The latest version of the application as a string.
     """
-    versions = abra('recipe', 'versions', app, machine_output=True)
-    latest = sorted([(version.parse(v['version']), v['version']) for v in versions])[-1][1]
+    releases = abra('recipe', 'versions', app, machine_output=True)
+    # a release is the list of images of one recipe version, and each of them carries that version
+    images = [image for release in releases
+              for image in (release if isinstance(release, list) else [release])]
+    if not images:
+        raise click.ClickException(f"abra reports no versions for {app}, pin one in the configuration")
+    latest = sorted([(version.parse(i['version']), i['version']) for i in images])[-1][1]
     logging.debug(f'latest version of {app}: {latest}')
     return latest
 

tests/test_latest_version.py

@@ -0,0 +1,68 @@
+"""Tests for reading the newest recipe version out of what 'abra recipe versions -m' writes."""
+
+import json
+import os
+import sys
+
+import click
+import pytest
+
+sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
+
+import alakazam
+from alakazam import get_latest_version, parse_machine_output
+
+# one document per recipe version, each listing the images of that version, as abra writes it
+GRIST_VERSIONS = (
+    '[{"name":"gristlabs/grist-oss","service":"app","tag":"1.7.19","version":"3.6.4+1.7.19"},'
+    '{"name":"redis","service":"redis","tag":"8.10-alpine","version":"3.6.4+1.7.19"}]\n'
+    '[{"name":"gristlabs/grist-oss","service":"app","tag":"1.7.18","version":"3.6.3+1.7.18"},'
+    '{"name":"redis","service":"redis","tag":"8.10-alpine","version":"3.6.3+1.7.18"}]\n'
+    '[{"name":"gristlabs/grist","service":"app","tag":"1.1.12","version":"1.0.0+1.1.12"},'
+    '{"name":"postgres","service":"db","tag":"13","version":"1.0.0+1.1.12"}]\n'
+)
+
+
+class TestParseMachineOutput:
+    def test_several_documents_are_read_as_a_list(self):
+        """A plain json.loads() stops after the first line with "Extra data"."""
+        assert len(parse_machine_output(GRIST_VERSIONS)) == 3
+
+    def test_a_single_document_stays_one_document(self):
+        assert parse_machine_output('{"a.org": {"apps": []}}') == {"a.org": {"apps": []}}
+
+    def test_a_single_list_stays_one_list(self):
+        assert parse_machine_output('[{"name": "db_password"}]') == [{"name": "db_password"}]
+
+    def test_blank_lines_are_skipped(self):
+        assert parse_machine_output('{"a": 1}\n\n{"b": 2}\n') == [{"a": 1}, {"b": 2}]
+
+    def test_output_that_is_no_json_at_all_still_raises(self):
+        """Swallowing it would turn a broken abra call into an empty but plausible answer."""
+        with pytest.raises(json.JSONDecodeError):
+            parse_machine_output("FATA something went wrong")
+
+
+class TestGetLatestVersion:
+    def install(self, monkeypatch, output):
+        monkeypatch.setattr(alakazam, "abra", lambda *a, **k: parse_machine_output(output))
+
+    def test_the_newest_version_is_taken(self, monkeypatch):
+        self.install(monkeypatch, GRIST_VERSIONS)
+        assert get_latest_version("grist") == "3.6.4+1.7.19"
+
+    def test_the_order_of_the_documents_does_not_matter(self, monkeypatch):
+        self.install(monkeypatch, "\n".join(reversed(GRIST_VERSIONS.strip().splitlines())))
+        assert get_latest_version("grist") == "3.6.4+1.7.19"
+
+    def test_a_single_flat_document_is_read_too(self, monkeypatch):
+        """Which is what abra writes for a recipe that has exactly one version."""
+        self.install(monkeypatch, '[{"name":"redis","tag":"8.10","version":"1.0.0+1.1.12"}]')
+        assert get_latest_version("grist") == "1.0.0+1.1.12"
+
+    def test_a_recipe_without_versions_is_reported(self, monkeypatch):
+        """Reaching for the newest of nothing would end in an IndexError with no hint in it."""
+        monkeypatch.setattr(alakazam, "abra", lambda *a, **k: {})
+        with pytest.raises(click.ClickException) as excinfo:
+            get_latest_version("grist")
+        assert "no versions for grist" in excinfo.value.message

tests/test_secret_exchange.py

@@ -0,0 +1,85 @@
+"""Tests for reading a secret out of a running app, and for what happens when that fails."""
+
+import logging
+import os
+import subprocess
+import sys
+
+import pytest
+
+sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
+
+import alakazam
+from alakazam import get_running_containers, get_secret, run_over_ssh
+
+
+class TestGetRunningContainers:
+    def test_the_containers_are_returned(self, monkeypatch):
+        monkeypatch.setattr(alakazam, "abra", lambda *a, **k: [{"service": "app"}])
+        assert get_running_containers("cloud.a.org") == [{"service": "app"}]
+
+    def test_an_app_that_is_not_deployed_has_none(self, monkeypatch):
+        def abra(*args, **kwargs):
+            raise RuntimeError("abra -o app ps -m cloud.a.org\n STDERR: FATA cloud.a.org is not deployed?")
+
+        monkeypatch.setattr(alakazam, "abra", abra)
+        assert get_running_containers("cloud.a.org") == []
+
+    def test_any_other_failure_is_raised(self, monkeypatch):
+        """Reading an unreachable server as "not running" would skip the exchange without a word."""
+        def abra(*args, **kwargs):
+            raise RuntimeError("abra -o app ps -m cloud.a.org\n STDERR: ssh: connect to host")
+
+        monkeypatch.setattr(alakazam, "abra", abra)
+        with pytest.raises(RuntimeError, match="connect to host"):
+            get_running_containers("cloud.a.org")
+
+
+class TestGetSecret:
+    def test_the_run_asks_for_no_terminal(self, monkeypatch):
+        """A ci run has no terminal to give, and one would rewrite the line endings of the value."""
+        calls = []
+        monkeypatch.setattr(alakazam, "abra", lambda *a, **k: calls.append(a) or "s3cret")
+        assert get_secret("cloud.a.org", "db_password", ["app"]) == "s3cret"
+        assert calls[0][:4] == ("app", "run", "-t", "cloud.a.org")
+
+    def test_the_next_container_is_tried(self, monkeypatch):
+        def abra(*args, **kwargs):
+            if args[4] == "db":
+                raise RuntimeError("FATA no such container")
+            return "s3cret"
+
+        monkeypatch.setattr(alakazam, "abra", abra)
+        assert get_secret("cloud.a.org", "db_password", ["db", "app"]) == "s3cret"
+
+
+class TestRunOverSsh:
+    def result(self, returncode=0, stdout="", stderr=""):
+        return subprocess.CompletedProcess(["ssh"], returncode, stdout, stderr)
+
+    def test_the_output_is_returned_unchanged(self, monkeypatch):
+        """A trailing newline can be part of a secret, so nothing is stripped."""
+        monkeypatch.setattr(subprocess, "run", lambda *a, **k: self.result(stdout="s3cret\n"))
+        assert run_over_ssh("a.org", "cat x", "the read") == "s3cret\n"
+
+    def test_a_failing_command_is_raised(self, monkeypatch):
+        """It exited 1 and wrote nothing, which read as an empty secret before."""
+        monkeypatch.setattr(subprocess, "run",
+                            lambda *a, **k: self.result(returncode=1, stderr="Permission denied"))
+        with pytest.raises(RuntimeError) as excinfo:
+            run_over_ssh("a.org", "cat x", "the read")
+        assert "exited with code 1" in str(excinfo.value)
+        assert "Permission denied" in str(excinfo.value)
+
+    def test_a_silent_failure_is_still_raised(self, monkeypatch):
+        monkeypatch.setattr(subprocess, "run", lambda *a, **k: self.result(returncode=255))
+        with pytest.raises(RuntimeError, match="no output"):
+            run_over_ssh("a.org", "cat x", "the read")
+
+    def test_stderr_on_a_successful_command_is_reported(self, monkeypatch, caplog):
+        """ssh writes its own warnings there, and they explain an answer that looks wrong."""
+        monkeypatch.setattr(subprocess, "run",
+                            lambda *a, **k: self.result(stdout="s3cret", stderr="Pseudo-terminal will not be allocated"))
+        with caplog.at_level(logging.WARNING):
+            assert run_over_ssh("a.org", "cat x", "the read") == "s3cret"
+        assert "Pseudo-terminal" in caplog.text

tests/test_upgrade_hooks.py

@@ -1,8 +1,10 @@
 """Tests for what an upgrade run does: deploy everything, wait for all of it, then hook."""
 
+import logging
 import os
 import sys
 
+import click
 import pytest
 from click.testing import CliRunner
 
@@ -177,3 +179,71 @@ class TestSettleOption:
         run(["-n", "--settle", "0"])
         assert selected(steps, "sleep:") == []
         assert selected(steps, "ps:") == ["ps:cloud.a.org", "ps:boards.a.org"]
+
+
+class TestAFailingApp:
+    """A server that refuses one app must not take the apps behind it down with it."""
+
+    @pytest.fixture
+    def failing(self, monkeypatch, steps):
+        """Lets the upgrade of cloud.a.org fail the way abra reports a rejected deployment."""
+        original = alakazam.abra
+
+        def abra(*args, **kwargs):
+            # the planning phase asks for the release notes with the same command plus -r
+            if args[:2] == ("app", "upgrade") and "-r" not in args and "cloud.a.org" in args:
+                raise RuntimeError("abra -o app upgrade -n cloud.a.org 2.0.0 \n STDERR: FATA "
+                                   "failed to update cloud_a_org_db: context deadline exceeded")
+            return original(*args, **kwargs)
+
+        monkeypatch.setattr(alakazam, "abra", abra)
+        return steps
+
+    def invoke(self, args):
+        return CliRunner().invoke(alakazam.upgrade, ["-n"] + args, standalone_mode=False)
+
+    def test_the_other_apps_are_still_upgraded(self, failing):
+        self.invoke(["-e"])
+        assert "app upgrade -n --no-converge-checks boards.a.org 2.0.0" in failing
+
+    def test_the_run_ends_on_the_failure(self, failing):
+        result = self.invoke(["-e"])
+        assert isinstance(result.exception, click.ClickException)
+        assert "1 of 2 app(s) failed to upgrade: cloud.a.org" in result.exception.message
+
+    def test_the_failing_app_is_neither_waited_for_nor_hooked(self, failing):
+        """Its state is unknown, so a hook against it would run on whatever is left there."""
+        self.invoke(["-e"])
+        assert selected(failing, "ps:") == ["ps:boards.a.org"]
+        assert selected(failing, "hook:") == ["hook:boards.a.org"]
+
+    def test_the_reason_is_reported(self, failing, caplog):
+        with caplog.at_level(logging.ERROR):
+            self.invoke(["-e"])
+        assert "context deadline exceeded" in caplog.text
+
+    def test_every_app_failing_skips_the_wait(self, steps, monkeypatch):
+        def abra(*args, **kwargs):
+            if args[:2] == ("app", "upgrade") and "-r" not in args:
+                raise RuntimeError("FATA failed to update: context deadline exceeded")
+            return ""
+
+        monkeypatch.setattr(alakazam, "abra", abra)
+        result = self.invoke(["-e"])
+        assert isinstance(result.exception, click.ClickException)
+        assert "2 of 2 app(s) failed" in result.exception.message
+        assert selected(steps, "ps:") == []
+
+    def test_a_failing_wait_does_not_hide_the_failing_app(self, failing, monkeypatch):
+        """One failure covering up another is how a broken app slips through a green pipeline."""
+        monkeypatch.setattr(alakazam, "wait_for_healthy_apps", lambda *a, **k: (_ for _ in ()).throw(
+            click.ClickException("1 service(s) still not healthy after 900s")))
+        message = self.invoke(["-e"]).exception.message
+        assert "cloud.a.org" in message
+        assert "still not healthy" in message
+
+    def test_the_hooks_stay_out_when_the_wait_failed(self, failing, monkeypatch):
+        monkeypatch.setattr(alakazam, "wait_for_healthy_apps", lambda *a, **k: (_ for _ in ()).throw(
+            click.ClickException("1 service(s) still not healthy after 900s")))
+        self.invoke(["-e"])
+        assert selected(failing, "hook:") == []