From aefa26b7c6706be2bd9f867d386d1af5b85bd5f4 Mon Sep 17 00:00:00 2001 From: Jonathon Reinhart Date: Fri, 22 Mar 2024 00:32:03 -0400 Subject: [PATCH 1/2] scuba: Split up scuba.config._expand_path() This splits scuba.config._expand_path() into two separate methods: - _expand_env_vars() - _absoluteify_path() --- scuba/config.py | 63 +++++++++++++++++++++++++++++-------------------- 1 file changed, 38 insertions(+), 25 deletions(-) diff --git a/scuba/config.py b/scuba/config.py index 4e1ecb4..23409d5 100644 --- a/scuba/config.py +++ b/scuba/config.py @@ -11,7 +11,7 @@ import yaml.nodes from .constants import DEFAULT_SHELL, SCUBA_YML -from .utils import expand_env_vars, parse_env_var +from . import utils from .dockerutil import make_vol_opt CfgNode = Any @@ -190,6 +190,32 @@ def find_config() -> Tuple[Path, Path, ScubaConfig]: ) +def _expand_env_vars(in_str: str) -> str: + """Wraps utils.expand_env_vars() to convert errors + + Args: + in_str: Input string. + + Returns: + The input string with environment variables expanded. + + Raises: + ConfigError: If a referenced environment variable is not set. + ConfigError: An environment variable reference could not be parsed. + """ + try: + return utils.expand_env_vars(in_str) + except KeyError as err: + # pylint: disable=raise-missing-from + raise ConfigError( + f"Unset environment variable {err.args[0]!r} used in {in_str!r}" + ) + except ValueError as ve: + raise ConfigError( + f"Unable to expand string '{in_str}' due to parsing errors" + ) from ve + + def _process_script_node(node: CfgNode, name: str) -> List[str]: """Process a script-type node @@ -236,7 +262,7 @@ def _process_environment(node: CfgNode, name: str) -> Environment: result[k] = str(v) elif isinstance(node, list): for e in node: - k, v = parse_env_var(e) + k, v = utils.parse_env_var(e) result[k] = v else: raise ConfigError( @@ -332,18 +358,16 @@ def _get_volumes( vols = {} for cpath_str, v in voldata.items(): - cpath = _expand_path(cpath_str) # no base_dir; container path must be absolute. + cpath_str = _expand_env_vars(cpath_str) + cpath = _absoluteify_path(cpath_str) # container path must be absolute. vols[cpath] = ScubaVolume.from_dict(cpath, v, scuba_root) return vols -def _expand_path(in_str: str, base_dir: Optional[Path] = None) -> Path: - """Expand variables in a path string and make it absolute. - - Environment variable references (e.g. $FOO and ${FOO}) are expanded using - the host environment. +def _absoluteify_path(in_str: str, base_dir: Optional[Path] = None) -> Path: + """Take a path string and make it absolute. - After environment variable expansion, absolute paths are returned as-is. + Absolute paths are returned as-is. Relative paths must start with ./ or ../ and are joined to base_dir, if provided. @@ -356,26 +380,13 @@ def _expand_path(in_str: str, base_dir: Optional[Path] = None) -> Path: Raises: ValueError: If base_dir is provided but not absolute. - ConfigError: If a referenced environment variable is not set. - ConfigError: An environment variable reference could not be parsed. ConfigError: A relative path does not start with "./" or "../". ConfigError: A relative path is given when base_dir is not provided. """ if base_dir is not None and not base_dir.is_absolute(): raise ValueError(f"base_dir is not absolute: {base_dir}") - try: - path_str = expand_env_vars(in_str) - except KeyError as ke: - # pylint: disable=raise-missing-from - raise ConfigError( - f"Unset environment variable {ke.args[0]!r} used in {in_str!r}" - ) - except ValueError as ve: - raise ConfigError( - f"Unable to expand string '{in_str}' due to parsing errors" - ) from ve - + path_str = _expand_env_vars(in_str) path = Path(path_str) if not path.is_absolute(): @@ -414,9 +425,10 @@ def from_dict( # volumes: # /foo: /host/foo if isinstance(node, str): + node = _expand_env_vars(node) return cls( container_path=cpath, - host_path=_expand_path(node, scuba_root), + host_path=_absoluteify_path(node, scuba_root), ) # Complex form @@ -428,9 +440,10 @@ def from_dict( hpath = node.get("hostpath") if hpath is None: raise ConfigError(f"Volume {cpath} must have a 'hostpath' subkey") + hpath = _expand_env_vars(hpath) return cls( container_path=cpath, - host_path=_expand_path(hpath, scuba_root), + host_path=_absoluteify_path(hpath, scuba_root), options=_get_delimited_str_list(node, "options", ","), ) From 7a20042ca9416b59c937a043fdc461e5f9fa730e Mon Sep 17 00:00:00 2001 From: Jonathon Reinhart Date: Fri, 22 Mar 2024 00:50:15 -0400 Subject: [PATCH 2/2] scuba: Add support for mounting named volumes Named volumes were unintentionally supported prior to #227. This restores the prior behavior while remaining unambiguiously compatible with relative bind-mounts added in #227. This also adds support for explicit named volumes in complex form configuration. Fixes #248 --- docs/configuration.rst | 53 +++++++++++--- scuba/config.py | 66 ++++++++++++++--- scuba/dockerutil.py | 13 +++- scuba/scuba.py | 2 +- tests/test_config.py | 161 +++++++++++++++++++++++++++++++++++++---- tests/test_main.py | 48 +++++++++++- tests/utils.py | 2 + 7 files changed, 299 insertions(+), 46 deletions(-) diff --git a/docs/configuration.rst b/docs/configuration.rst index 841626a..4569a86 100644 --- a/docs/configuration.rst +++ b/docs/configuration.rst @@ -147,25 +147,40 @@ style `_: The optional ``volumes`` node *(added in v2.9.0)* allows additional `bind-mounts `_ to be specified. +As of v2.13.0, `named volumes `_ +are also supported. + ``volumes`` is a mapping (dictionary) where each key is the container-path. -In the simple form, the value is a string, the host-path to be bind-mounted: +In the simple form, the value is a string, which can be: + +* An absolute or relative path which results in a bind-mount. +* A Docker volume name. .. code-block:: yaml + :caption: Example of simple-form volumes volumes: - /var/lib/foo: /host/foo - /var/lib/bar: ./bar + /var/lib/foo: /host/foo # bind-mount: absolute path + /var/lib/bar: ./bar # bind-mount: path relative to .scuba.yml dir + /var/log: persist-logs # named volume + +In the complex form, the value is a mapping with the following supported keys: + +* ``hostpath``: An absolute or relative path specifying a host bind-mount. +* ``name``: The name of a named Docker volume. +* ``options``: A comma-separated list of volume options. -In the complex form, the value is a mapping which must contain a ``hostpath`` -subkey. It can also contain an ``options`` subkey with a comma-separated list -of volume options: +``hostpath`` and ``name`` are mutually-exclusive and one must be specified. .. code-block:: yaml + :caption: Example of complex-form volumes volumes: /var/lib/foo: - hostpath: /host/foo + hostpath: /host/foo # bind-mount options: ro,cached + /var/log: + name: persist-logs # named volume The paths (host or container) used in volume mappings can contain environment variables **which are expanded in the host environment**. For example, this @@ -182,18 +197,32 @@ configuration error. Volume container paths must be absolute. -Volume host paths can be absolute or relative. If a relative path is used, it -is interpreted as relative to the directory in which ``.scuba.yml`` is found. -To avoid ambiguity, relative paths must start with ``./`` or ``../``. +Bind-mount host paths can be absolute or relative. If a relative path is used, +it is interpreted as relative to the directory in which ``.scuba.yml`` is +found. To avoid ambiguity with a named volume, relative paths must start with +``./`` or ``../``. -Volume host directories which do not already exist are created as the current -user before creating the container. +Bind-mount host directories which do not already exist are created as the +current user before creating the container. .. note:: Because variable expansion is now applied to all volume paths, if one desires to use a literal ``$`` character in a path, it must be written as ``$$``. +.. note:: + Docker named volumes are created with ``drwxr-xr-x`` (0755) permissions. + If scuba is not run with ``--root``, the scuba user will be unable to write + to this directory. As a workaround, one can use a :ref:`root hook + ` to change permissions on the directory. + + .. code-block:: yaml + + volumes: + /foo: foo-volume + hooks: + root: chmod 777 /foo + .. _conf_aliases: diff --git a/scuba/config.py b/scuba/config.py index 23409d5..ba4a507 100644 --- a/scuba/config.py +++ b/scuba/config.py @@ -19,6 +19,8 @@ Environment = Dict[str, str] _T = TypeVar("_T") +VOLUME_NAME_PATTERN = re.compile(r"^[a-zA-Z0-9][a-zA-Z0-9_.-]+$") + class ConfigError(Exception): pass @@ -410,9 +412,14 @@ def _absoluteify_path(in_str: str, base_dir: Optional[Path] = None) -> Path: @dataclasses.dataclass(frozen=True) class ScubaVolume: container_path: Path - host_path: Path # TODO: Optional for anonymous volume + host_path: Optional[Path] = None + volume_name: Optional[str] = None options: List[str] = dataclasses.field(default_factory=list) + def __post_init__(self) -> None: + if sum(bool(x) for x in (self.host_path, self.volume_name)) != 1: + raise ValueError("Exactly one of host_path, volume_name must be set") + @classmethod def from_dict( cls, cpath: Path, node: CfgNode, scuba_root: Optional[Path] @@ -423,12 +430,26 @@ def from_dict( # Simple form: # volumes: - # /foo: /host/foo + # /foo: foo-volume # volume name + # /bar: /host/bar # absolute path + # /snap: ./snap # relative path if isinstance(node, str): node = _expand_env_vars(node) + + # Absolute or relative path + valid_prefixes = ("/", "./", "../") + if any(node.startswith(pfx) for pfx in valid_prefixes): + return cls( + container_path=cpath, + host_path=_absoluteify_path(node, scuba_root), + ) + + # Volume name + if not VOLUME_NAME_PATTERN.match(node): + raise ConfigError(f"Invalid volume name: {node!r}") return cls( container_path=cpath, - host_path=_absoluteify_path(node, scuba_root), + volume_name=node, ) # Complex form @@ -436,21 +457,42 @@ def from_dict( # /foo: # hostpath: /host/foo # options: ro,z + # /bar: + # name: bar-volume if isinstance(node, dict): hpath = node.get("hostpath") - if hpath is None: - raise ConfigError(f"Volume {cpath} must have a 'hostpath' subkey") - hpath = _expand_env_vars(hpath) - return cls( - container_path=cpath, - host_path=_absoluteify_path(hpath, scuba_root), - options=_get_delimited_str_list(node, "options", ","), - ) + name = node.get("name") + options = _get_delimited_str_list(node, "options", ",") + + if sum(bool(x) for x in (hpath, name)) != 1: + raise ConfigError( + f"Volume {cpath} must have exactly one of" + " 'hostpath' or 'name' subkey" + ) + + if hpath is not None: + hpath = _expand_env_vars(hpath) + return cls( + container_path=cpath, + host_path=_absoluteify_path(hpath, scuba_root), + options=options, + ) + + if name is not None: + return cls( + container_path=cpath, + volume_name=_expand_env_vars(name), + options=options, + ) raise ConfigError(f"{cpath}: must be string or dict") def get_vol_opt(self) -> str: - return make_vol_opt(self.host_path, self.container_path, self.options) + if self.host_path: + return make_vol_opt(self.host_path, self.container_path, self.options) + if self.volume_name: + return make_vol_opt(self.volume_name, self.container_path, self.options) + raise Exception("host_path or volume_name must be set") @dataclasses.dataclass(frozen=True) diff --git a/scuba/dockerutil.py b/scuba/dockerutil.py index a2b1ac4..6ecd30d 100644 --- a/scuba/dockerutil.py +++ b/scuba/dockerutil.py @@ -132,17 +132,22 @@ def get_image_entrypoint(image: str) -> Optional[Sequence[str]]: def make_vol_opt( - hostdir: Path, + hostdir_or_volname: Union[Path, str], contdir: Path, options: Optional[Sequence[str]] = None, ) -> str: """Generate a docker volume option""" - if not hostdir.is_absolute(): - raise ValueError(f"hostdir not absolute: {hostdir}") + if isinstance(hostdir_or_volname, Path): + hostdir: Path = hostdir_or_volname + if not hostdir.is_absolute(): + # NOTE: As of Docker Engine version 23, you can use relative paths + # on the host. But we have no minimum Docker version, so we don't + # rely on this. + raise ValueError(f"hostdir not absolute: {hostdir}") if not contdir.is_absolute(): raise ValueError(f"contdir not absolute: {contdir}") - vol = f"--volume={hostdir}:{contdir}" + vol = f"--volume={hostdir_or_volname}:{contdir}" if options: assert not isinstance(options, str) vol += ":" + ",".join(options) diff --git a/scuba/scuba.py b/scuba/scuba.py index 0642388..6b14961 100644 --- a/scuba/scuba.py +++ b/scuba/scuba.py @@ -165,7 +165,7 @@ def try_create_volumes(self) -> None: return for vol in self.context.volumes.values(): - if vol.host_path.exists(): + if vol.host_path is None or vol.host_path.exists(): continue try: diff --git a/tests/test_config.py b/tests/test_config.py index d07b481..65aa924 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -10,6 +10,7 @@ from .utils import assert_paths_equal, assert_vol import scuba.config +from scuba.config import ScubaVolume def load_config() -> scuba.config.ScubaConfig: @@ -878,7 +879,7 @@ def test_volume_as_dict_missing_hostpath(self) -> None: self._invalid_config("hostpath") - def test_volumes_simple_volume(self) -> None: + def test_volumes_simple_bind(self) -> None: """volumes can be set using the simple form""" with open(".scuba.yml", "w") as f: f.write( @@ -895,7 +896,7 @@ def test_volumes_simple_volume(self) -> None: assert_vol(config.volumes, "/cpath", "/hpath") - def test_volumes_complex(self) -> None: + def test_volumes_complex_bind(self) -> None: """volumes can be set using the complex form""" with open(".scuba.yml", "w") as f: f.write( @@ -920,6 +921,27 @@ def test_volumes_complex(self) -> None: assert_vol(vols, "/bar", "/host/bar") assert_vol(vols, "/snap", "/host/snap", ["z", "ro"]) + def test_volumes_complex_named_volume(self) -> None: + """volumes complex form can specify a named volume""" + with open(".scuba.yml", "w") as f: + f.write( + r""" + image: na + volumes: + /foo: + name: foo-volume + """ + ) + + config = load_config() + assert config.volumes is not None + assert len(config.volumes) == 1 + vol = config.volumes[Path("/foo")] + + assert isinstance(vol, ScubaVolume) + assert_paths_equal(vol.container_path, "/foo") + assert vol.volume_name == "foo-volume" + def test_alias_volumes_set(self) -> None: """docker_args can be set via alias""" with open(".scuba.yml", "w") as f: @@ -1076,20 +1098,6 @@ def test_volumes_hostpath_rel_above(self, monkeypatch, in_tmp_path) -> None: assert config.volumes is not None assert_vol(config.volumes, "/foo", in_tmp_path / "foo_up_here") - def test_volumes_hostpath_rel_req_dot_simple( - self, monkeypatch, in_tmp_path - ) -> None: - """relaitve volume hostpath (simple form) must start with ./ or ../""" - with open(".scuba.yml", "w") as f: - f.write( - r""" - image: na - volumes: - /one: foo # Forbidden - """ - ) - self._invalid_config("Relative path must start with ./ or ../") - def test_volumes_hostpath_rel_requires_dot_complex( self, monkeypatch, in_tmp_path ) -> None: @@ -1134,3 +1142,124 @@ def test_volumes_contpath_rel(self, monkeypatch, in_tmp_path) -> None: """ ) self._invalid_config("Relative path not allowed: foo") + + def test_volumes_simple_named_volume(self) -> None: + """volumes simple form can specify a named volume""" + with open(".scuba.yml", "w") as f: + f.write( + r""" + image: na + volumes: + /foo: foo-volume + """ + ) + + config = load_config() + assert config.volumes is not None + assert len(config.volumes) == 1 + vol = config.volumes[Path("/foo")] + + assert isinstance(vol, ScubaVolume) + assert_paths_equal(vol.container_path, "/foo") + assert vol.volume_name == "foo-volume" + + def test_volumes_simple_named_volume_env(self, monkeypatch) -> None: + """volumes simple form can specify a named volume via env var""" + with open(".scuba.yml", "w") as f: + f.write( + r""" + image: na + volumes: + /foo: $FOO_VOLUME + """ + ) + + monkeypatch.setenv("FOO_VOLUME", "foo-volume") + + config = load_config() + assert config.volumes is not None + assert len(config.volumes) == 1 + vol = config.volumes[Path("/foo")] + + assert isinstance(vol, ScubaVolume) + assert_paths_equal(vol.container_path, "/foo") + assert vol.volume_name == "foo-volume" + + def test_volumes_complex_named_volume_env(self, monkeypatch) -> None: + """volumes complex form can specify a named volume via env var""" + with open(".scuba.yml", "w") as f: + f.write( + r""" + image: na + volumes: + /foo: + name: $FOO_VOLUME + """ + ) + + monkeypatch.setenv("FOO_VOLUME", "foo-volume") + + config = load_config() + assert config.volumes is not None + assert len(config.volumes) == 1 + vol = config.volumes[Path("/foo")] + + assert isinstance(vol, ScubaVolume) + assert_paths_equal(vol.container_path, "/foo") + assert vol.volume_name == "foo-volume" + + def test_volumes_complex_named_volume_env_unset(self) -> None: + """volumes complex form fails on unset env var""" + with open(".scuba.yml", "w") as f: + f.write( + r""" + image: na + volumes: + /foo: + name: $FOO_VOLUME + """ + ) + self._invalid_config("Unset environment variable") + + def test_volumes_complex_invalid_hostpath(self) -> None: + """volumes complex form cannot specify an invalid hostpath""" + with open(".scuba.yml", "w") as f: + f.write( + r""" + image: na + volumes: + /foo: + hostpath: foo-volume + """ + ) + self._invalid_config("Relative path must start with ./ or ../") + + def test_volumes_complex_hostpath_and_name(self) -> None: + """volumes complex form cannot specify a named volume and hostpath""" + with open(".scuba.yml", "w") as f: + f.write( + r""" + image: na + volumes: + /foo: + hostpath: /bar + name: foo-volume + """ + ) + self._invalid_config( + "Volume /foo must have exactly one of 'hostpath' or 'name' subkey" + ) + + def test_volumes_complex_empty(self) -> None: + """volumes complex form cannot be empty""" + with open(".scuba.yml", "w") as f: + f.write( + r""" + image: na + volumes: + /foo: + """ + ) + self._invalid_config( + "Volume /foo must have exactly one of 'hostpath' or 'name' subkey" + ) diff --git a/tests/test_main.py b/tests/test_main.py index 5e77b32..7b242fb 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -29,7 +29,7 @@ @pytest.mark.usefixtures("in_tmp_path") -class TestMain: +class TestMainBase: def run_scuba(self, args, exp_retval=0, mock_isatty=False, stdin=None): """Run scuba, checking its return value @@ -87,6 +87,8 @@ def run_scuba(self, args, exp_retval=0, mock_isatty=False, stdin=None): sys.stdout = old_stdout sys.stderr = old_stderr + +class TestMain(TestMainBase): def test_basic(self) -> None: """Verify basic scuba functionality""" @@ -1133,3 +1135,47 @@ def test_volumes_hostpath_rel_above(self) -> None: out, _ = self.run_scuba(["cat", "/userdir/test.txt"]) assert out == test_message + + +class TestMainNamedVolumes(TestMainBase): + VOLUME_NAME = "foo-volume" + + def _rm_volume(self) -> None: + result = subprocess.run( + ["docker", "volume", "rm", self.VOLUME_NAME], + capture_output=True, + text=True, + ) + if result.returncode == 1 and re.match(r".*no such volume.*", result.stderr): + return + result.check_returncode() + + def setup_method(self, method) -> None: + self._rm_volume() + + def teardown_method(self, method) -> None: + self._rm_volume() + + def test_volumes_named(self) -> None: + """Verify named volumes can be used""" + VOL_PATH = Path("/foo") + TEST_PATH = VOL_PATH / "test.txt" + TEST_STR = "it works!" + + with open(".scuba.yml", "w") as f: + f.write( + f""" + image: {DOCKER_IMAGE} + hooks: + root: chmod 777 {VOL_PATH} + volumes: + {VOL_PATH}: {self.VOLUME_NAME} + """ + ) + + # Inoke scuba once: Write a file to the named volume + self.run_scuba(["/bin/sh", "-c", f"echo {TEST_STR} > {TEST_PATH}"]) + + # Invoke scuba again: Verify the file is still there + out, _ = self.run_scuba(["/bin/sh", "-c", f"cat {TEST_PATH}"]) + assert_str_equalish(out, TEST_STR) diff --git a/tests/utils.py b/tests/utils.py index efe7036..8e78fea 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -48,6 +48,8 @@ def assert_vol( v = vols[cpath] assert isinstance(v, ScubaVolume) assert_paths_equal(v.container_path, cpath) + assert v.volume_name is None + assert v.host_path is not None assert_paths_equal(v.host_path, hpath) assert v.options == options