Common way of parsing images: image:latest is forbidden

python/CHANGELOG.D/539.bugfix

@@ -0,0 +1,2 @@
+Fix parsing image URIs.
+

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 ImageNameParser
 
 from .command_spinner import SpinnerBase
 from .rc import Config
@@ -42,22 +40,23 @@ async def push(cfg: Config, image_name: str, remote_image_name: str) -> None:
 
     """
 
-    username = cfg.username
-
-    local_image = remote_image = Image.from_local(image_name, username)
+    parser = ImageNameParser(cfg.username, cfg.registry_url)
+    local_img = parser.parse_as_docker_image(image_name)
     if remote_image_name:
-        remote_image = Image.from_url(URL(remote_image_name), username)
+        remote_img = parser.parse_as_neuro_image(remote_image_name)
+    else:
+        remote_img = parser.convert_to_neuro_image(local_img)
 
-    log.info(f"Using remote image '{remote_image.url}'")
-    log.info(f"Using local image '{local_image.url}'")
+    click.echo(f"Using local image '{local_img.as_local_str()}'")
+    click.echo(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()
@@ -79,21 +78,23 @@ async def pull(cfg: Config, image_name: str, local_image_name: str) -> None:
 
     """
 
-    username = cfg.username
-
-    remote_image = local_image = Image.from_url(URL(image_name), username)
+    parser = ImageNameParser(cfg.username, cfg.registry_url)
+    remote_img = parser.parse_as_neuro_image(image_name)
     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}'")
+        local_img = parser.parse_as_docker_image(local_image_name)
+    else:
+        local_img = parser.convert_to_docker_image(remote_img)
+
+    click.echo(f"Using remote image '{remote_img.as_url_str()}'")
+    click.echo(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,
+    ImageNameParser,
     JobStatus,
     NetworkPortForwarding,
     Resources,
@@ -198,11 +199,18 @@ async def submit(
     log.debug(f'cmd="{cmd}"')
 
     memory = to_megabytes_str(memory)
-    image_obj = Image(image=image, command=cmd)
+
+    image_parser = ImageNameParser(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/cli/model.py

@@ -136,7 +136,6 @@ async def train(
         log.debug(f'cmdline="{cmdline}"')
 
         if not quiet:
-            # TODO (ajuszkowski 01-Feb-19) normalize image name to URI (issue 452)
             log.info(f"Using image '{image}'")
             log.info(f"Using dataset '{dataset_url}'")
             log.info(f"Using weights '{resultset_url}'")

python/neuromation/client/__init__.py

@@ -32,12 +32,14 @@
     Volume,
 )
 from .models import Models, TrainResult
+from .parsing_utils import ImageNameParser
 from .storage import FileStatus, FileStatusType, Storage
 from .users import Action, Permission, Users
 
 
 __all__ = (
     "Image",
+    "ImageNameParser",
     "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.as_local_str()
 
-        return cls(url=URL(f"image://{username}/{name}"), local=name)
+    def as_local_str(self) -> str:
+        return f"{self.name}:{self.tag}"
 
 
 class Images:
@@ -94,22 +86,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()
@@ -122,7 +113,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()
@@ -134,9 +127,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(
@@ -147,11 +143,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()
 
@@ -163,7 +161,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