Common way of parsing images: image:latest is forbidden

python/neuromation/cli/image.py

@@ -2,10 +2,8 @@
 import sys
 
 import click
-from yarl import URL
 
-# TODO(asvetlov): rename the class to avoid the namig conflict
-from neuromation.client.images import Image
+from neuromation.client import ImageParser
 
 from .command_spinner import SpinnerBase
 from .rc import Config
@@ -43,22 +41,25 @@ async def push(cfg: Config, image_name: str, remote_image_name: str) -> None:
 
     """
 
-    username = cfg.username
+    parser = ImageParser(cfg.username, cfg.registry_url)
+    local_img = parser.parse_as_docker_image(image_name)
+    remote_img = (
+        parser.parse_as_neuro_image(remote_image_name)
+        if remote_image_name
+        else parser.convert_to_neuro_image(local_img)
+    )
 
-    local_image = remote_image = Image.from_local(image_name, username)
-    if remote_image_name:
-        remote_image = Image.from_url(URL(remote_image_name), username)
-
-    log.info(f"Using remote image '{remote_image.url}'")
-    log.info(f"Using local image '{local_image.url}'")
+    # TODO: check out all todos by ajuszkowski in this PR!
+    log.info(f"Using local image '{local_img.as_local_str()}'")
+    log.info(f"Using remote image '{remote_img.as_url_str()}'")
+    log.debug(f"LOCAL: '{local_img}'")
+    log.debug(f"REMOTE: '{remote_img}'")
 
     spinner = SpinnerBase.create_spinner(sys.stdout.isatty(), "Pushing image {}  ")
 
     async with cfg.make_client() as client:
-        result_remote_image = await client.images.push(
-            local_image, remote_image, spinner
-        )
-        click.echo(result_remote_image.url)
+        result_remote_image = await client.images.push(local_img, remote_img, spinner)
+        click.echo(result_remote_image.as_url_str())
 
 
 @command()
@@ -81,21 +82,24 @@ async def pull(cfg: Config, image_name: str, local_image_name: str) -> None:
 
     """
 
-    username = cfg.username
+    parser = ImageParser(cfg.username, cfg.registry_url)
+    remote_img = parser.parse_as_neuro_image(image_name)
+    local_img = (
+        parser.parse_as_docker_image(local_image_name)
+        if local_image_name
+        else parser.convert_to_docker_image(remote_img)
+    )
 
-    remote_image = local_image = Image.from_url(URL(image_name), username)
-    if local_image_name:
-        local_image = Image.from_local(local_image_name, username)
-    log.info(f"Using remote image '{remote_image.url}'")
-    log.info(f"Using local image '{local_image.url}'")
+    log.info(f"Using remote image '{remote_img.as_url_str()}'")
+    log.info(f"Using local image '{local_img.as_local_str()}'")
+    log.debug(f"REMOTE: '{remote_img}'")
+    log.debug(f"LOCAL: '{local_img}'")
 
     spinner = SpinnerBase.create_spinner(sys.stdout.isatty(), "Pulling image {}  ")
 
     async with cfg.make_client() as client:
-        result_local_image = await client.images.pull(
-            remote_image, local_image, spinner
-        )
-        click.echo(result_local_image.local)
+        result_local_image = await client.images.pull(remote_img, local_img, spinner)
+        click.echo(result_local_image.as_local_str())
 
 
 @command()

python/neuromation/cli/job.py

@@ -10,6 +10,7 @@
 
 from neuromation.client import (
     Image,
+    ImageParser,
     JobStatus,
     NetworkPortForwarding,
     Resources,
@@ -193,11 +194,18 @@ async def submit(
     log.debug(f'cmd="{cmd}"')
 
     memory = to_megabytes_str(memory)
-    image_obj = Image(image=image, command=cmd)
+
+    image_parser = ImageParser(cfg.username, cfg.registry_url)
+    if image_parser.is_in_neuro_registry(image):
+        parsed_image = image_parser.parse_as_neuro_image(image)
+    else:
+        parsed_image = image_parser.parse_as_docker_image(image)
     # TODO (ajuszkowski 01-Feb-19) process --quiet globally to set up logger+click
     if not quiet:
-        # TODO (ajuszkowski 01-Feb-19) normalize image name to URI (issue 452)
-        log.info(f"Using image '{image_obj.image}'")
+        log.info(f"Using image '{parsed_image.as_url_str()}'")
+        log.debug(f"IMAGE: {parsed_image}")
+    image_obj = Image(image=parsed_image.as_repo_str(), command=cmd)
+
     network = NetworkPortForwarding.from_cli(http, ssh)
     resources = Resources.create(cpu, gpu, gpu_model, memory, extshm)
     volumes = Volume.from_cli_list(username, volume)

python/neuromation/client/__init__.py

@@ -32,9 +32,11 @@
 from .users import Action, Permission, Users
 from .images import Images
 from .config import Config
+from .parsing_utils import ImageParser
 
 __all__ = (
     "Image",
+    "ImageParser",
     "JobDescription",
     "JobStatus",
     "JobStatusHistory",

python/neuromation/client/images.py

@@ -1,7 +1,8 @@
+import logging
 import re
 from contextlib import suppress
 from dataclasses import dataclass
-from typing import Dict, List
+from typing import Dict, List, Optional
 
 import aiodocker
 import aiohttp
@@ -17,44 +18,35 @@
 STATUS_FORBIDDEN = 403
 STATUS_NOT_FOUND = 404
 STATUS_CUSTOM_ERROR = 900
-DEFAULT_TAG = "latest"
 
+log = logging.getLogger(__name__)
 
-@dataclass(frozen=True)
-class Image:
-    url: URL
-    local: str
-
-    @classmethod
-    def from_url(cls, url: URL, username: str) -> "Image":
-        if not url:
-            raise ValueError(f"Image URL cannot be empty")
-        if url.scheme != "image":
-            raise ValueError(f"Invalid scheme, for image URL: {url}")
-        if url.path == "/" or url.query or url.fragment or url.user or url.port:
-            raise ValueError(f"Invalid image URL: {url}")
-        colon_count = url.path.count(":")
-        if colon_count > 1:
-            raise ValueError(f"Invalid image URL, only one colon allowed: {url}")
+IMAGE_SCHEME = "image"
 
-        if not colon_count:
-            url = url.with_path(f"{url.path}:{DEFAULT_TAG}")
 
-        if not url.host:
-            url = URL(f"image://{username}/{url.path.lstrip('/')}")
+@dataclass(frozen=True)
+# TODO (ajuszkowski, 20-feb-2019): rename this class: docker-images refer to both local
+# images and images in docker hub, and neuro-images refer to an image in neuro registry
+class DockerImage:
+    name: str
+    tag: str = "latest"
+    owner: Optional[str] = None
+    registry: Optional[str] = None
 
-        return cls(url=url, local=url.path.lstrip("/"))
+    def is_in_neuro_registry(self) -> bool:
+        return bool(self.registry and self.owner)
 
-    @classmethod
-    def from_local(cls, name: str, username: str) -> "Image":
-        colon_count = name.count(":")
-        if colon_count > 1:
-            raise ValueError(f"Invalid image name, only one colon allowed: {name}")
+    def as_url_str(self) -> str:
+        pre = f"{IMAGE_SCHEME}://{self.owner}/" if self.is_in_neuro_registry() else ""
+        return f"{pre}{self.name}:{self.tag}"
 
-        if not colon_count:
-            name = f"{name}:{DEFAULT_TAG}"
+    def as_repo_str(self) -> str:
+        # TODO (ajuszkowski, 11-Feb-2019) should be host:port (see URL.explicit_port)
+        pre = f"{self.registry}/{self.owner}/" if self.is_in_neuro_registry() else ""
+        return pre + self.name
 
-        return cls(url=URL(f"image://{username}/{name}"), local=name)
+    def as_local_str(self) -> str:
+        return f"{self.name}:{self.tag}"
 
 
 class Images:
@@ -93,22 +85,21 @@ async def close(self) -> None:
     def _auth(self) -> Dict[str, str]:
         return {"username": "token", "password": self._config.token}
 
-    def _repo(self, image: Image) -> str:
-        # TODO (ajuszkowski, 11-Feb-2019) here we use only registry host, not host:port
-        return f"{self._config.registry_url.host}/{image.url.host}{image.url.path}"
-
     async def push(
-        self, local_image: Image, remote_image: Image, spinner: AbstractSpinner
-    ) -> Image:
-        repo = self._repo(local_image)
+        self,
+        local_image: DockerImage,
+        remote_image: DockerImage,
+        spinner: AbstractSpinner,
+    ) -> DockerImage:
+        repo = remote_image.as_repo_str()
         spinner.start("Pushing image ...")
         try:
-            await self._docker.images.tag(local_image.local, repo)
+            await self._docker.images.tag(local_image.as_local_str(), repo)
         except DockerError as error:
             spinner.complete()
             if error.status == STATUS_NOT_FOUND:
                 raise ValueError(
-                    f"Image {local_image.local} was not found "
+                    f"Image {local_image.as_local_str()} was not found "
                     "in your local docker images"
                 ) from error
         spinner.tick()
@@ -121,7 +112,9 @@ async def push(
             spinner.complete()
             # TODO check this part when registry fixed
             if error.status == STATUS_FORBIDDEN:
-                raise AuthorizationError(f"Access denied {remote_image.url}") from error
+                raise AuthorizationError(
+                    f"Access denied {remote_image.as_url_str()}"
+                ) from error
             raise  # pragma: no cover
         async for obj in stream:
             spinner.tick()
@@ -133,9 +126,12 @@ async def push(
         return remote_image
 
     async def pull(
-        self, remote_image: Image, local_image: Image, spinner: AbstractSpinner
-    ) -> Image:
-        repo = self._repo(remote_image)
+        self,
+        remote_image: DockerImage,
+        local_image: DockerImage,
+        spinner: AbstractSpinner,
+    ) -> DockerImage:
+        repo = remote_image.as_repo_str()
         spinner.start("Pulling image ...")
         try:
             stream = await self._docker.pull(
@@ -146,11 +142,13 @@ async def pull(
             spinner.complete()
             if error.status == STATUS_NOT_FOUND:
                 raise ValueError(
-                    f"Image {remote_image.url} was not found " "in registry"
+                    f"Image {remote_image.as_url_str()} was not found " "in registry"
                 ) from error
             # TODO check this part when registry fixed
             elif error.status == STATUS_FORBIDDEN:
-                raise AuthorizationError(f"Access denied {remote_image.url}") from error
+                raise AuthorizationError(
+                    f"Access denied {remote_image.as_url_str()}"
+                ) from error
             raise  # pragma: no cover
         spinner.tick()
 
@@ -162,7 +160,7 @@ async def pull(
                 raise DockerError(STATUS_CUSTOM_ERROR, error_details)
         spinner.tick()
 
-        await self._docker.images.tag(repo, local_image.local)
+        await self._docker.images.tag(repo, local_image.as_local_str())
         spinner.complete()
 
         return local_image

python/neuromation/client/parsing_utils.py

@@ -0,0 +1,101 @@
+from typing import Tuple
+
+from yarl import URL
+
+from .images import IMAGE_SCHEME, DockerImage
+
+
+class ImageParser:
+    default_tag = "latest"
+
+    def __init__(self, default_user: str, registry_url: str):
+        self._default_user = default_user
+        self._registry = self._get_registry_hostname(registry_url)
+
+    def parse_as_docker_image(self, image: str) -> DockerImage:
+        try:
+            return self._parse_as_docker_image(image)
+        except ValueError as e:
+            raise ValueError(f"Invalid docker image '{image}': {e}") from e
+
+    def parse_as_neuro_image(self, image: str) -> DockerImage:
+        try:
+            return self._parse_as_neuro_image(image)
+        except ValueError as e:
+            raise ValueError(f"Invalid remote image '{image}': {e}") from e
+
+    def is_in_neuro_registry(self, image: str) -> bool:
+        # not use URL here because URL("ubuntu:v1") is parsed as scheme=ubuntu path=v1
+        return image.startswith(f"{IMAGE_SCHEME}:")
+
+    def convert_to_neuro_image(self, image: DockerImage) -> DockerImage:
+        return DockerImage(
+            name=image.name,
+            tag=image.tag,
+            owner=self._default_user,
+            registry=self._registry,
+        )
+
+    def convert_to_docker_image(self, image: DockerImage) -> DockerImage:
+        return DockerImage(name=image.name, tag=image.tag)
+
+    def _parse_as_docker_image(self, image: str) -> DockerImage:
+        if not image:
+            raise ValueError("empty image name")
+        if self.is_in_neuro_registry(image):
+            raise ValueError(
+                f"scheme '{IMAGE_SCHEME}://' is not allowed for docker images"
+            )
+        name, tag = self._split_image_name(image)
+        return DockerImage(name=name, tag=tag)
+
+    def _parse_as_neuro_image(self, image: str) -> DockerImage:
+        if not image:
+            raise ValueError("empty image name")
+        if not self.is_in_neuro_registry(image):
+            raise ValueError(f"scheme '{IMAGE_SCHEME}://' is required")
+
+        url = URL(image)
+        assert url.scheme == IMAGE_SCHEME
+        self._check_allowed_uri_elements(url)
+
+        registry = self._registry
+        owner = self._default_user if not url.host or url.host == "~" else url.host
+        name, tag = self._split_image_name(url.path.lstrip("/"))
+        return DockerImage(name=name, tag=tag, registry=registry, owner=owner)
+
+    def _split_image_name(self, image: str) -> Tuple[str, str]:
+        colon_count = image.count(":")
+        if colon_count == 0:
+            image, tag = image, self.default_tag
+        elif colon_count == 1:
+            image, tag = image.split(":")
+        else:
+            raise ValueError(f"cannot parse image name '{image}': too many tags")
+        return image, tag
+
+    def _get_registry_hostname(self, registry_url: str) -> str:
+        try:
+            url = URL(registry_url)
+        except ValueError as e:
+            raise ValueError(f"Could not parse registry URL: {e}")
+        if not url.host:
+            raise ValueError(
+                f"Empty hostname in registry URL '{registry_url}': "
+                "please consider updating configuration"
+            )
+        return url.host
+
+    def _check_allowed_uri_elements(self, url: URL) -> None:
+        if not url.path or url.path == "/":
+            raise ValueError("no image name specified")
+        if url.query:
+            raise ValueError(f"query is not allowed, found: '{url.query}'")
+        if url.fragment:
+            raise ValueError(f"fragment is not allowed, found: '{url.fragment}'")
+        if url.user:
+            raise ValueError(f"user is not allowed, found: '{url.user}'")
+        if url.password:
+            raise ValueError(f"password is not allowed, found: '{url.password}'")
+        if url.port:
+            raise ValueError(f"port is not allowed, found: '{url.port}'")

python/neuromation/client/url_utils.py

@@ -1,12 +1,8 @@
-import logging
 from pathlib import Path
 
 from yarl import URL
 
 
-log = logging.getLogger(__name__)
-
-
 def normalize_storage_path_uri(uri: URL, username: str) -> URL:
     """Normalize storage url."""
     if uri.scheme != "storage":