Repository: JonathonReinhart/scuba

Issue:
Restore ability to use named volumes
Prior to #227, you could, as a not-officially-support feature map a named volume at a container path:
```yaml
volumes:
  /foo: foo-volume
```

```console
$ scuba --root --image debian:8.2 /bin/bash -c "echo yes > /foo/does-it-persist"
$ scuba --root --image debian:8.2 /bin/bash -c "cat /foo/does-it-persist"
yes
$ docker volume ls
DRIVER    VOLUME NAME
local     foo-volume
```

Since #227, this no longer works.
```
scuba: Config error: Relative path must start with ./ or ../: foo-volume
```

*Reported https://github.com/JonathonReinhart/scuba/pull/227#issuecomment-2005067223 by @haboustak.*

Discussion:

Patch:
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 <https://yaml.org/spec/1.2/spec.html#id2788097>`_:
 
 The optional ``volumes`` node *(added in v2.9.0)* allows additional
 `bind-mounts <https://docs.docker.com/storage/bind-mounts/>`_ to be specified.
+As of v2.13.0, `named volumes <https://docs.docker.com/storage/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
+   <conf_hooks>` 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 4e1ecb4..ba4a507 100644
--- a/scuba/config.py
+++ b/scuba/config.py
@@ -11,7 +11,7 @@ import yaml
 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
@@ -19,6 +19,8 @@ CfgData = Dict[str, CfgNode]
 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
@@ -190,6 +192,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 +264,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 +360,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 +382,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():
@@ -399,9 +412,14 @@ def _expand_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]
@@ -412,11 +430,26 @@ class ScubaVolume:
 
         # 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=_expand_path(node, scuba_root),
+                volume_name=node,
             )
 
         # Complex form
@@ -424,20 +457,42 @@ class ScubaVolume:
         #   /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")
-            return cls(
-                container_path=cpath,
-                host_path=_expand_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 @@ class ScubaDive:
             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:


Test patch:
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 unittest import mock
 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 @@ class TestConfig:
 
         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 @@ class TestConfig:
 
         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 @@ class TestConfig:
         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 @@ class TestConfig:
         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 @@ class TestConfig:
                 """
             )
         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 @@ SCUBAINIT_EXIT_FAIL = 99
 
 
 @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 @@ class TestMain:
                     sys.stdout = old_stdout
                     sys.stderr = old_stderr
 
+
+class TestMain(TestMainBase):
     def test_basic(self) -> None:
         """Verify basic scuba functionality"""
 
@@ -1133,3 +1135,47 @@ class TestMain:
 
         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