Skip to content

Commit

Permalink
Add back validation
Browse files Browse the repository at this point in the history
  • Loading branch information
danielhollas committed Feb 4, 2024
1 parent dc2d221 commit f53cad5
Show file tree
Hide file tree
Showing 2 changed files with 41 additions and 6 deletions.
42 changes: 38 additions & 4 deletions aiidalab_launch/profile.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,17 @@ def _default_port() -> int: # explicit function required to enable test patchin
DEFAULT_IMAGE = "docker.io/aiidalab/full-stack:latest"


def _valid_volume_name(source: str) -> None:
# We do not allow relative paths so if the path is not absolute,
# we assume volume mount, whose name is restricted by Docker.
if not Path(source).is_absolute() and not re.fullmatch(
_REGEX_VALID_IMAGE_NAMES, source
):
raise docker.errors.InvalidArgument(
f"Invalid extra mount volume name '{source}'. Use absolute path for bind mounts."
)


def _get_configured_host_port(container: Container) -> int | None:
try:
host_config = container.attrs["HostConfig"]
Expand All @@ -52,18 +63,39 @@ def _get_aiidalab_default_apps(container: Container) -> list:


# We extend the Mount type from Docker API
# with some extra validation to fail early if user provide wrong argument.
# with some extra validation to fail early if user provides wrong argument.
# https://github.com/docker/docker-py/blob/bd164f928ab82e798e30db455903578d06ba2070/docker/types/services.py#L305
class ExtraMount(docker.types.Mount):
@classmethod
def parse_mount_string(cls, mount) -> docker.types.Mount:
mount = super().parse_mount_string(mount)
def parse_mount_string(cls, mount_str) -> docker.types.Mount:
mount = super().parse_mount_string(mount_str)
# For some reason, Docker API allows Source to be None??
# Not on our watch!
if mount["Source"] is None:
raise docker.errors.InvalidArgument(
f"Invalid extra mount specification '{mount}'"
)

# If the read-write mode is not "rw", docker assumes "ro"
# Let's be more strict here to avoid confusing errors later.
parts = mount_str.split(":")
if len(parts) == 3:
mode = parts[2]
if mode not in ("ro", "rw"):
raise docker.errors.InvalidArgument(
f"Invalid read-write mode in '{mount}'"
)

# Unlike for home_mount, we will not auto-create missing
# directories for extra mounts.
if mount["Type"] == "bind":
source_path = Path(mount["Source"])
if not source_path.exists():
raise ValueError(f"Directory '{source_path}' does not exist!")
raise docker.errors.InvalidArgument(
f"Directory '{source_path}' does not exist!"
)
else:
_valid_volume_name(mount["Source"])
return mount


Expand All @@ -90,6 +122,8 @@ def __post_init__(self):
if self.home_mount is None:
self.home_mount = f"{CONTAINER_PREFIX}{self.name}_home"

_valid_volume_name(self.home_mount)

# Normalize extra mount mode to be "rw" by default
# so that we match Docker default but are explicit.
for extra_mount in self.extra_mounts.copy():
Expand Down
5 changes: 3 additions & 2 deletions tests/test_profile.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
from dataclasses import replace
from pathlib import Path

import docker
import pytest

from aiidalab_launch.profile import Profile
Expand Down Expand Up @@ -57,7 +58,7 @@ def test_profile_init_valid_home_mounts(profile, extra_volume_name, home_mount):
@pytest.mark.parametrize("home_mount", INVALID_HOME_MOUNTS)
def test_profile_init_invalid_home_mounts(profile, extra_volume_name, home_mount):
home_mount = home_mount.format(path=Path.home(), vol=extra_volume_name)
with pytest.raises(ValueError):
with pytest.raises(docker.errors.InvalidArgument):
replace(profile, home_mount=home_mount)


Expand All @@ -70,7 +71,7 @@ def test_profile_init_valid_extra_mounts(profile, extra_volume_name, extra_mount
@pytest.mark.parametrize("extra_mount", INVALID_EXTRA_MOUNTS)
def test_profile_init_invalid_extra_mounts(profile, extra_volume_name, extra_mount):
extra_mounts = {extra_mount.format(path=Path.home(), vol=extra_volume_name)}
with pytest.raises(ValueError):
with pytest.raises(docker.errors.InvalidArgument):
replace(profile, extra_mounts=extra_mounts)


Expand Down

0 comments on commit f53cad5

Please sign in to comment.