Support remote images with registry host:port

neuromation/api/parsing_utils.py

@@ -22,7 +22,6 @@ def _is_in_neuro_registry(image: RemoteImage) -> bool:
 
 
 def _as_repo_str(image: RemoteImage) -> str:
-    # TODO (ajuszkowski, 11-Feb-2019) should be host:port (see URL.explicit_port)
     pre = f"{image.registry}/{image.owner}/" if _is_in_neuro_registry(image) else ""
     post = f":{image.tag}" if image.tag else ""
     return pre + image.name + post
@@ -48,7 +47,7 @@ def __init__(self, default_user: str, registry_url: URL):
                 f"Empty hostname in registry URL '{registry_url}': "
                 "please consider updating configuration"
             )
-        self._registry = registry_url.host
+        self._registry = _get_url_authority(registry_url)
 
     def parse_as_local_image(self, image: str) -> LocalImage:
         try:
@@ -135,16 +134,19 @@ def _parse_as_neuro_image(
             raise ValueError("scheme 'image://' is required for remote images")
 
         url = URL(image)
+
+        if url.scheme and url.scheme != "image":
+            # image with port in registry: `localhost:5000/owner/ubuntu:latest`
+            url = URL(f"//{url}")
+
         if not url.scheme:
             parts = url.path.split("/")
-            url = url.build(
+            url = URL.build(
                 scheme="image",
                 host=parts[1],
                 path="/".join([""] + parts[2:]),
                 query=url.query,
             )
-        else:
-            assert url.scheme == "image", f"invalid image scheme: '{url.scheme}'"
 
         self._check_allowed_uri_elements(url)
 
@@ -178,5 +180,15 @@ def _check_allowed_uri_elements(self, url: URL) -> None:
             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}'")
+        if url.port and url.scheme == "image":
+            raise ValueError(
+                f"port is not allowed with 'image://' scheme, found: '{url.port}'"
+            )
+
+
+def _get_url_authority(url: URL) -> Optional[str]:
+    if url.host is None:
+        return None
+    port = url.explicit_port  # type: ignore
+    suffix = f":{port}" if port is not None else ""
+    return url.host + suffix

tests/api/test_images.py

@@ -10,7 +10,11 @@
 
 from neuromation.api import AuthorizationError, Client
 from neuromation.api.images import LocalImage, RemoteImage
-from neuromation.api.parsing_utils import _as_repo_str, _ImageNameParser
+from neuromation.api.parsing_utils import (
+    _as_repo_str,
+    _get_url_authority,
+    _ImageNameParser,
+)
 from tests import _TestServerFactory
 
 
@@ -59,18 +63,19 @@ def test_has_tag_too_many_tags(self) -> None:
 
     @pytest.mark.parametrize(
         "registry_url",
-        [
-            "http://reg.neu.ro",
-            "https://reg.neu.ro",
-            "http://reg.neu.ro:5000",
-            "https://reg.neu.ro/bla/bla",
-            "http://reg.neu.ro:5000/bla/bla",
-        ],
+        ["http://reg.neu.ro", "https://reg.neu.ro", "https://reg.neu.ro/bla/bla"],
     )
     def test_get_registry_hostname(self, registry_url: str) -> None:
         parser = _ImageNameParser(default_user="alice", registry_url=URL(registry_url))
         assert parser._registry == "reg.neu.ro"
 
+    @pytest.mark.parametrize(
+        "registry_url", ["http://reg.neu.ro:5000", "http://reg.neu.ro:5000/bla/bla"]
+    )
+    def test_get_registry_hostname_with_port(self, registry_url: str) -> None:
+        parser = _ImageNameParser(default_user="alice", registry_url=URL(registry_url))
+        assert parser._registry == "reg.neu.ro:5000"
+
     @pytest.mark.parametrize(
         "registry_url",
         ["", "reg.neu.ro", "reg.neu.ro:5000", "https://", "https:///bla/bla"],
@@ -501,6 +506,100 @@ def test_parse_as_local_image__ambiguous_case__fail(self) -> None:
         with pytest.raises(ValueError, match="ambiguous value"):
             self.parser.parse_as_local_image(url)
 
+    # other corner cases
+
+    def test_is_in_neuro_registry__registry_has_port__neuro_image(self) -> None:
+        my_parser = _ImageNameParser(
+            default_user="alice", registry_url=URL("http://localhost:5000")
+        )
+        image = "image://bob/library/ubuntu:v10.04"
+        assert my_parser.is_in_neuro_registry(image) is True
+
+    def test_is_in_neuro_registry__registry_has_port__image_in_good_repo(self) -> None:
+        my_parser = _ImageNameParser(
+            default_user="alice", registry_url=URL("http://localhost:5000")
+        )
+        image = "localhost:5000/bob/library/ubuntu:v10.04"
+        assert my_parser.is_in_neuro_registry(image) is True
+
+    def test_is_in_neuro_registry__registry_has_port__image_in_bad_repo(self) -> None:
+        my_parser = _ImageNameParser(
+            default_user="alice", registry_url=URL("http://localhost:5000")
+        )
+        image = "localhost:9999/bob/library/ubuntu:v10.04"
+        assert my_parser.is_in_neuro_registry(image) is False
+
+    def test_is_in_neuro_registry__registry_has_port__local_image(self) -> None:
+        my_parser = _ImageNameParser(
+            default_user="alice", registry_url=URL("http://localhost:5000")
+        )
+        image = "ubuntu:v10.04"
+        assert my_parser.is_in_neuro_registry(image) is False
+
+    def test_is_in_neuro_registry__registry_has_port(self) -> None:
+        my_parser = _ImageNameParser(
+            default_user="alice", registry_url=URL("http://localhost:5000")
+        )
+        image = "ubuntu:v10.04"
+        parsed = my_parser.parse_as_local_image(image)
+        assert parsed == LocalImage(name="ubuntu", tag="v10.04")
+
+    def test_parse_as_neuro_image__registry_has_port__neuro_image(self) -> None:
+        my_parser = _ImageNameParser(
+            default_user="alice", registry_url=URL("http://localhost:5000")
+        )
+        image = "image://bob/library/ubuntu:v10.04"
+        parsed = my_parser.parse_as_neuro_image(image)
+        assert parsed == RemoteImage(
+            name="library/ubuntu", tag="v10.04", owner="bob", registry="localhost:5000"
+        )
+
+    def test_parse_as_neuro_image__registry_has_port__image_in_good_repo(self) -> None:
+        my_parser = _ImageNameParser(
+            default_user="alice", registry_url=URL("http://localhost:5000")
+        )
+        image = "localhost:5000/bob/library/ubuntu:v10.04"
+        parsed = my_parser.parse_as_neuro_image(image)
+        assert parsed == RemoteImage(
+            name="library/ubuntu", tag="v10.04", owner="bob", registry="localhost:5000"
+        )
+
+    def test_parse_as_neuro_image__registry_has_port__image_in_bad_repo(self) -> None:
+        my_parser = _ImageNameParser(
+            default_user="alice", registry_url=URL("http://localhost:5000")
+        )
+        image = "localhost:9999/bob/library/ubuntu:v10.04"
+        with pytest.raises(ValueError, match="scheme 'image://' is required"):
+            my_parser.parse_as_neuro_image(image)
+
+    def test_parse_remote__registry_has_port__neuro_image(self) -> None:
+        my_parser = _ImageNameParser(
+            default_user="alice", registry_url=URL("http://localhost:5000")
+        )
+        image = "image://bob/library/ubuntu:v10.04"
+        parsed = my_parser.parse_remote(image)
+        assert parsed == RemoteImage(
+            name="library/ubuntu", tag="v10.04", owner="bob", registry="localhost:5000"
+        )
+
+    def test_parse_remote__registry_has_port__image_in_good_repo(self) -> None:
+        my_parser = _ImageNameParser(
+            default_user="alice", registry_url=URL("http://localhost:5000")
+        )
+        image = "localhost:5000/bob/library/ubuntu:v10.04"
+        parsed = my_parser.parse_remote(image)
+        assert parsed == RemoteImage(
+            name="library/ubuntu", tag="v10.04", owner="bob", registry="localhost:5000"
+        )
+
+    def test_parse_remote__registry_has_port__image_in_bad_repo(self) -> None:
+        my_parser = _ImageNameParser(
+            default_user="alice", registry_url=URL("http://localhost:5000")
+        )
+        image = "localhost:9999/bob/library/ubuntu:v10.04"
+        with pytest.raises(ValueError, match="too many tags"):
+            my_parser.parse_remote(image)
+
 
 class TestRemoteImage:
     def test_as_str_in_neuro_registry_tag_none(self) -> None:
@@ -725,9 +824,11 @@ async def handler(request: web.Request) -> web.Response:
         registry_url = srv.make_url("/v2/")
         async with make_client(url, registry_url=registry_url) as client:
             ret = await client.images.ls()
+
+        registry = _get_url_authority(registry_url)
         assert set(ret) == {
-            RemoteImage("alpine", tag=None, owner="bob", registry="127.0.0.1"),
-            RemoteImage("bananas", tag=None, owner="jill", registry="127.0.0.1"),
+            RemoteImage("alpine", tag=None, owner="bob", registry=registry),
+            RemoteImage("bananas", tag=None, owner="jill", registry=registry),
         }
 
     @pytest.mark.skipif(
@@ -747,11 +848,14 @@ async def handler(request: web.Request) -> web.Response:
         srv = await aiohttp_server(app)
         url = "http://platform"
         registry_url = srv.make_url("/v2/")
+
         async with make_client(url, registry_url=registry_url) as client:
             ret = await client.images.ls()
+
+        registry = _get_url_authority(registry_url)
         assert set(ret) == {
-            RemoteImage("alpine", tag=None, owner="bob", registry="127.0.0.1"),
-            RemoteImage("bananas", tag=None, owner="jill", registry="127.0.0.1"),
+            RemoteImage("alpine", tag=None, owner="bob", registry=registry),
+            RemoteImage("bananas", tag=None, owner="jill", registry=registry),
         }
 
     @pytest.mark.skipif(

tests/api/test_parser.py

@@ -1,8 +1,10 @@
 from typing import Callable
 
 import pytest
+from yarl import URL
 
 from neuromation.api import Client, LocalImage, RemoteImage
+from neuromation.api.parsing_utils import _get_url_authority
 
 
 _MakeClient = Callable[..., Client]
@@ -29,3 +31,23 @@ async def test_parse_remote(make_client: _MakeClient) -> None:
     assert result == RemoteImage(
         "bananas", "latest", owner="bob", registry="registry-dev.neu.ro"
     )
+
+
+def test_get_url_authority_with_explicit_port() -> None:
+    url = URL("http://example.com:8080/")
+    assert _get_url_authority(url) == "example.com:8080"
+
+
+def test_get_url_authority_with_implicit_port() -> None:
+    url = URL("http://example.com/")  # here `url.port == 80`
+    assert _get_url_authority(url) == "example.com"
+
+
+def test_get_url_authority_without_port() -> None:
+    url = URL("scheme://example.com/")  # here `url.port is None`
+    assert _get_url_authority(url) == "example.com"
+
+
+def test_get_url_authority_without_host() -> None:
+    url = URL("scheme://")
+    assert _get_url_authority(url) is None