Repository: ansible/ansible-navigator (language: python)

Issue:
Ability to specify the podman --cert-dir option in config
##### ISSUE TYPE
 - Add ability to specify additional podman command-line options in ansible-navigator.yml

##### SUMMARY
Our corporate environment uses a 3rd party SSL inspection tool that re-signs all traffic with the vendor's root certificate.  This obviously breaks things when trying to pull from, say, quay.io and the cert is signed by someone else.  It appears there is no good way to put the vendor's cert in a standard location that podman will search (e.g., /etc/containers/certs.d) (see https://github.com/containers/podman/issues/2726).  As such, I have to resort to passing the `--certs-dir` option to podman on the command-line each time I need to login to a container registry and pull from it.

However, if you set the pull-policy in your ansible-navigator.yml config, it will fail to pull the image on launch of `ansible-navigator` because it is not passing that `--certs-dir` option.  If podman can search known directories and use the certificates therein, this is a moot point, but I cannot manage to get that working.



Discussion / hints:
Hey @watsonb ,

Thanks for this.  It's really great to get some real-world use cases.

I can think of 2 ways to handle this...

1) would be for a new setting to be supported, maybe 'pull-parameters', which would be just a string of anything that would be added to the pull command
2) turn the pull command into a setting that was templated with the image name, eg. `{container-engine} pull {image}` would be the default, but you could override it in the settings with `{container-engine} --certs-dir mydir pull {image}`

`container-engine` and `image` would be the only 2 variables available and pull from the other settings.

This would not be very different than they way we handle the `artifact-file`, where the default is `{playbook_dir}/{playbook_name}-artifact-{ts_utc}.json` and the dir/name and ts are variables

let me know if either or both would work, brad
Hello @cidrblock ,

I think either/both of those approaches would work just fine.  Let me know when you need a tester.

Fix patch:
diff --git a/docs/changelog-fragments.d/952.feature.md b/docs/changelog-fragments.d/952.feature.md
new file mode 100644
index 000000000..44f1b99a9
--- /dev/null
+++ b/docs/changelog-fragments.d/952.feature.md
@@ -0,0 +1,3 @@
+Added ability to pass additional arguments to the execution environment pull command.
+
+-- by {user}`cidrblock`
diff --git a/src/ansible_navigator/cli.py b/src/ansible_navigator/cli.py
index a3db842e2..57339715a 100644
--- a/src/ansible_navigator/cli.py
+++ b/src/ansible_navigator/cli.py
@@ -49,6 +49,7 @@ def pull_image(args):
     image_puller = ImagePuller(
         container_engine=args.container_engine,
         image=args.execution_environment_image,
+        arguments=args.pull_arguments,
         pull_policy=args.pull_policy,
     )
     image_puller.assess()
diff --git a/src/ansible_navigator/configuration_subsystem/navigator_configuration.py b/src/ansible_navigator/configuration_subsystem/navigator_configuration.py
index 4ebe9ec9a..f3f1ce038 100644
--- a/src/ansible_navigator/configuration_subsystem/navigator_configuration.py
+++ b/src/ansible_navigator/configuration_subsystem/navigator_configuration.py
@@ -473,6 +473,17 @@ class Internals(SimpleNamespace):
             subcommands=["doc"],
             value=SettingsEntryValue(default="module"),
         ),
+        SettingsEntry(
+            name="pull_arguments",
+            cli_parameters=CliParameters(action="append", nargs="+", short="--pa"),
+            settings_file_path_override="execution-environment.pull.arguments",
+            short_description=(
+                "Specify any additional parameters that should be added to the"
+                " pull command when pulling an execution environment from a container"
+                " registry. e.g. --pa='--tls-verify=false'"
+            ),
+            value=SettingsEntryValue(),
+        ),
         SettingsEntry(
             name="pull_policy",
             choices=["always", "missing", "never", "tag"],
diff --git a/src/ansible_navigator/configuration_subsystem/navigator_post_processor.py b/src/ansible_navigator/configuration_subsystem/navigator_post_processor.py
index 166451489..2640d1a41 100644
--- a/src/ansible_navigator/configuration_subsystem/navigator_post_processor.py
+++ b/src/ansible_navigator/configuration_subsystem/navigator_post_processor.py
@@ -843,6 +843,27 @@ def playbook_artifact_replay(
                 return messages, exit_messages
         return messages, exit_messages
 
+    @staticmethod
+    @_post_processor
+    def pull_arguments(
+        entry: SettingsEntry,
+        config: ApplicationConfiguration,
+    ) -> PostProcessorReturn:
+        # pylint: disable=unused-argument
+        """Post process ``pull_arguments``
+
+        :param entry: The current settings entry
+        :param config: The full application configuration
+        :return: An instance of the standard post process return object
+        """
+        messages: List[LogMessage] = []
+        exit_messages: List[ExitMessage] = []
+        if entry.value.source == C.ENVIRONMENT_VARIABLE:
+            entry.value.current = to_list(entry.value.current)
+        if entry.value.current is not C.NOT_SET:
+            entry.value.current = flatten_list(entry.value.current)
+        return messages, exit_messages
+
     @staticmethod
     @_post_processor
     def set_environment_variable(
diff --git a/src/ansible_navigator/image_manager/puller.py b/src/ansible_navigator/image_manager/puller.py
index 47ba46eb0..866fc2740 100644
--- a/src/ansible_navigator/image_manager/puller.py
+++ b/src/ansible_navigator/image_manager/puller.py
@@ -1,13 +1,17 @@
 """image puller"""
 import logging
+import shlex
 import subprocess
 
 from types import SimpleNamespace
 from typing import List
+from typing import Union
 
+from ..configuration_subsystem import Constants
 from ..utils import ExitMessage
 from ..utils import ExitPrefix
 from ..utils import LogMessage
+from ..utils import shlex_join
 
 
 class ImageAssessment(SimpleNamespace):
@@ -22,22 +26,34 @@ class ImagePuller:
     # pylint: disable=too-many-instance-attributes
     """Image puller"""
 
-    def __init__(self, container_engine: str, image: str, pull_policy: str):
+    def __init__(
+        self,
+        container_engine: str,
+        image: str,
+        arguments: Union[Constants, List[str]],
+        pull_policy: str,
+    ):
         """Initialize the container image puller.
 
         :param container_engine: The name of the container engine
         :param image: The name of the image to pull
+        :param arguments: Additional arguments to be appended to the pull policy
         :param pull_policy: The current pull policy from the settings
         """
+        if isinstance(arguments, list):
+            self._arguments = arguments
+        else:
+            self._arguments = []
+
         self._assessment = ImageAssessment
-        self._container_engine = container_engine
+        self._container_engine: str = container_engine
         self._exit_messages: List[ExitMessage] = []
-        self._image = image
+        self._image: str = image
         self._image_present: bool
         self._image_tag: str
         self._logger = logging.getLogger(__name__)
         self._messages: List[LogMessage] = []
-        self._pull_policy = pull_policy
+        self._pull_policy: str = pull_policy
         self._pull_required: bool = False
 
     def assess(self):
@@ -127,38 +143,53 @@ def prologue_stdout(self):
         """print a little value added information"""
         messages = [("Execution environment image name:", self._image)]
         messages.append(("Execution environment image tag:", self._image_tag))
+        arguments = shlex_join(self._arguments) or None
+        messages.append(("Execution environment pull arguments:", arguments))
         messages.append(("Execution environment pull policy:", self._pull_policy))
         messages.append(("Execution environment pull needed:", self._pull_required))
+
         width = max((len(m[0]) + len(str(m[1])) + 2 for m in messages))
         print("\u002d" * width)
         print("Execution environment image and pull policy overview")
         print("\u002d" * width)
         column_width = max((len(m[0]) for m in messages))
         for msg, value in messages:
-            print(f"{msg.ljust(column_width)} {value}")
-            self._log_message(message=f"{msg}: {value}", level=logging.INFO)
+            print(f"{msg.ljust(column_width)} {value!s}")
+            self._log_message(message=f"{msg!s}: {value!s}", level=logging.INFO)
         print("\u002d" * width)
         print("Updating the execution environment")
         print("\u002d" * width)
 
+    def _generate_pull_command(self) -> List[str]:
+        """Generate the pull command.
+
+        :returns: The list of command parts
+        """
+        command_line = [self._container_engine, "pull"]
+        # In case the settings file has an entry with a space
+        # e.g. ``--authfile file.txt``, split all of the entries
+        for argument in self._arguments:
+            command_line.extend(shlex.split(argument))
+        command_line.append(self._image)
+        return command_line
+
     def pull_stdout(self):
-        """pull the image, print to stdout
+        """Pull the image, print to stdout.
+
+        ``podman`` writes errors to stdout and ``docker`` writes to stderr.
 
-        podman writes to stderr
-        docker writes to stdout
+        This allows us to capture, log and color the error from ``docker`` but in the case
+        of ``podman`` the error is only printed on the screen.
+
+        In both cases, stdout is not captured so the user can see the progress on the screen
+        as the pull is happening.
         """
         try:
-            if self._container_engine == "podman":
-                subprocess.run([self._container_engine, "pull", self._image], check=True)
-            elif self._container_engine == "docker":
-                subprocess.run(
-                    [self._container_engine, "pull", self._image],
-                    check=True,
-                    stderr=subprocess.PIPE,
-                )
-            else:
-                raise ValueError("Unknown container engine")
-
+            command_line = self._generate_pull_command()
+            shlex_joined = shlex_join(command_line)
+            print(f"Running the command: {shlex_joined}")
+            stderr_pipe = subprocess.PIPE if self._container_engine == "docker" else None
+            subprocess.run(command_line, check=True, stderr=stderr_pipe)
             self._log_message(level=logging.INFO, message="Execution environment updated")
             self._pull_required = False
             self._assessment.pull_required = False
diff --git a/src/ansible_navigator/utils/functions.py b/src/ansible_navigator/utils/functions.py
index 33cd2d08d..3d7e18510 100644
--- a/src/ansible_navigator/utils/functions.py
+++ b/src/ansible_navigator/utils/functions.py
@@ -6,6 +6,7 @@
 import logging
 import os
 import re
+import shlex
 import shutil
 import sys
 import sysconfig
@@ -14,6 +15,7 @@
 from enum import Enum
 from pathlib import Path
 from typing import Any
+from typing import Iterable
 from typing import List
 from typing import Mapping
 from typing import NamedTuple
@@ -443,6 +445,19 @@ def round_half_up(number: Union[float, int]) -> int:
     return int(rounded)
 
 
+def shlex_join(tokens: Iterable[str]) -> str:
+    """Concatenate the tokens of a list and return a string.
+
+    ``shlex.join`` was new in version 3.8
+
+    :param tokens: The iterable of strings to join
+    :returns: The iterable joined with spaces
+    """
+    if sys.version_info >= (3, 8):
+        return shlex.join(split_command=tokens)
+    return " ".join(shlex.quote(token) for token in tokens)
+
+
 def str2bool(value: Any) -> bool:
     """Convert some commonly used values
     to a boolean


Test patch:
diff --git a/tests/fixtures/unit/configuration_subsystem/ansible-navigator.yml b/tests/fixtures/unit/configuration_subsystem/ansible-navigator.yml
index 33630fc0e..0449d0cd8 100644
--- a/tests/fixtures/unit/configuration_subsystem/ansible-navigator.yml
+++ b/tests/fixtures/unit/configuration_subsystem/ansible-navigator.yml
@@ -42,6 +42,9 @@ ansible-navigator:
         KEY3: VALUE3
     image: test_image:latest
     pull-policy: never
+    pull:
+      arguments:
+        - "--tls-verify=false"
     volume-mounts:
       - src: "/test1"
         dest: "/test1"
diff --git a/tests/unit/configuration_subsystem/data.py b/tests/unit/configuration_subsystem/data.py
index ebec778ee..e9688b310 100644
--- a/tests/unit/configuration_subsystem/data.py
+++ b/tests/unit/configuration_subsystem/data.py
@@ -243,6 +243,7 @@ def cli_data():
     ("playbook_artifact_save_as", "/tmp/save.json", "/tmp/save.json"),
     ("plugin_name", "shell", "shell"),
     ("plugin_type", "become", "become"),
+    ("pull_arguments", "--tls-verify=false", ["--tls-verify=false"]),
     ("pull_policy", "never", "never"),
     ("set_environment_variable", "T1=A,T2=B,T3=C", {"T1": "A", "T2": "B", "T3": "C"}),
     ("workdir", "/tmp/", "/tmp/"),
diff --git a/tests/unit/image_manager/test_image_puller.py b/tests/unit/image_manager/test_image_puller.py
index 7f8cf5337..940e08a2a 100644
--- a/tests/unit/image_manager/test_image_puller.py
+++ b/tests/unit/image_manager/test_image_puller.py
@@ -1,11 +1,13 @@
 """unit tests for image puller"""
 
+import shlex
 import uuid
 
 from typing import NamedTuple
 
 import pytest
 
+from ansible_navigator.configuration_subsystem import Constants
 from ansible_navigator.image_manager import ImagePuller
 from ...defaults import DEFAULT_CONTAINER_IMAGE
 from ...defaults import SMALL_TEST_IMAGE
@@ -38,6 +40,7 @@ def test_do_have(valid_container_engine, data):
     image_puller = ImagePuller(
         container_engine=valid_container_engine,
         image=DEFAULT_CONTAINER_IMAGE,
+        arguments=Constants.NOT_SET,
         pull_policy=data.pull_policy,
     )
     image_puller.assess()
@@ -59,6 +62,7 @@ def test_do_have_but_latest(valid_container_engine, data):
     image_puller = ImagePuller(
         container_engine=valid_container_engine,
         image=SMALL_TEST_IMAGE,
+        arguments=Constants.NOT_SET,
         pull_policy=data.pull_policy,
     )
     image_puller.assess()
@@ -80,6 +84,7 @@ def test_missing_locally(valid_container_engine, data):
     image_puller = ImagePuller(
         container_engine=valid_container_engine,
         image=uuid_str,
+        arguments=Constants.NOT_SET,
         pull_policy=data.pull_policy,
     )
     image_puller.assess()
@@ -102,6 +107,7 @@ def test_will_have(valid_container_engine, pullable_image, data):
     image_puller = ImagePuller(
         container_engine=valid_container_engine,
         image=pullable_image,
+        arguments=Constants.NOT_SET,
         pull_policy=data.pull_policy,
     )
     image_puller.assess()
@@ -130,6 +136,26 @@ def test_will_have(valid_container_engine, pullable_image, data):
 )
 def test_tag_parsing(image, expected_tag):
     """test that we parse image tags in a reasonable way"""
-    image_puller = ImagePuller("podman", image, "tag")
+    image_puller = ImagePuller(
+        container_engine="podman",
+        image=image,
+        arguments=Constants.NOT_SET,
+        pull_policy="tag",
+    )
     image_puller._extract_tag()  # pylint: disable=protected-access
     assert image_puller._image_tag == expected_tag  # pylint: disable=protected-access
+
+
+def test_pull_with_args():
+    """Ensure command is generated with additional arguments."""
+    image_puller = ImagePuller(
+        container_engine="podman",
+        image="my_image",
+        arguments=["--tls-verify false"],
+        pull_policy="tag",
+    )
+    result = image_puller._generate_pull_command()  # pylint: disable=protected-access
+    expected_list = ["podman", "pull", "--tls-verify", "false", "my_image"]
+    assert result == expected_list
+    expected_string = "podman pull --tls-verify false my_image"
+    assert result == shlex.split(expected_string)