From 88f210c3dbe6fc36a1ee1c12899928e538657206 Mon Sep 17 00:00:00 2001 From: Artem Yushkovskiy Date: Thu, 1 Aug 2019 16:31:43 +0300 Subject: [PATCH 1/4] support remote images with host:port registry --- neuromation/api/parsing_utils.py | 24 +++++- tests/api/test_images.py | 132 ++++++++++++++++++++++++++++--- tests/api/test_parser.py | 22 ++++++ 3 files changed, 162 insertions(+), 16 deletions(-) diff --git a/neuromation/api/parsing_utils.py b/neuromation/api/parsing_utils.py index 7dfeed3f0..367a8eac3 100644 --- a/neuromation/api/parsing_utils.py +++ b/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: @@ -137,14 +136,23 @@ def _parse_as_neuro_image( url = URL(image) 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}'" + if url.scheme != "image": + # Workaround for https://github.com/aio-libs/yarl/issues/321 + parts = url.path.split("/") + assert parts[0].isdigit(), f"{parts[0]} is expected to be a digit" + url = URL.build( + scheme="image", + host=parts[1], + path="/".join([""] + parts[2:]), + query=url.query, + ) self._check_allowed_uri_elements(url) @@ -180,3 +188,11 @@ def _check_allowed_uri_elements(self, url: URL) -> None: raise ValueError(f"password is not allowed, found: '{url.password}'") if url.port: raise ValueError(f"port is not allowed, 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 diff --git a/tests/api/test_images.py b/tests/api/test_images.py index 2ac5f4118..42288bde5 100644 --- a/tests/api/test_images.py +++ b/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,104 @@ 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" + ) + + @pytest.mark.xfail( + reason="issue #938: `_ImageNameParser.parse_remote()`: " + "do not fall back to `parse_as_local_image()`" + ) + 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="scheme 'image://' is required"): + my_parser.parse_remote(image) + class TestRemoteImage: def test_as_str_in_neuro_registry_tag_none(self) -> None: @@ -725,9 +828,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 +852,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( diff --git a/tests/api/test_parser.py b/tests/api/test_parser.py index f26cd0713..99545dd63 100644 --- a/tests/api/test_parser.py +++ b/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 From f15442581505863533873bee57c1c360cfa3ad56 Mon Sep 17 00:00:00 2001 From: Artem Yushkovskiy Date: Thu, 1 Aug 2019 17:22:33 +0300 Subject: [PATCH 2/4] review --- neuromation/api/parsing_utils.py | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/neuromation/api/parsing_utils.py b/neuromation/api/parsing_utils.py index 367a8eac3..c70969433 100644 --- a/neuromation/api/parsing_utils.py +++ b/neuromation/api/parsing_utils.py @@ -133,7 +133,14 @@ def _parse_as_neuro_image( if not self.is_in_neuro_registry(image): raise ValueError("scheme 'image://' is required for remote images") + allow_port = False url = URL(image) + + if url.scheme and url.scheme != "image": + # image with port in registry: `localhost:5000/owner/ubuntu:latest` + url = URL(f"//{url}") + allow_port = True + if not url.scheme: parts = url.path.split("/") url = URL.build( @@ -142,19 +149,8 @@ def _parse_as_neuro_image( path="/".join([""] + parts[2:]), query=url.query, ) - else: - if url.scheme != "image": - # Workaround for https://github.com/aio-libs/yarl/issues/321 - parts = url.path.split("/") - assert parts[0].isdigit(), f"{parts[0]} is expected to be a digit" - url = URL.build( - scheme="image", - host=parts[1], - path="/".join([""] + parts[2:]), - query=url.query, - ) - - self._check_allowed_uri_elements(url) + + self._check_allowed_uri_elements(url, allow_port=allow_port) registry = self._registry owner = self._default_user if not url.host or url.host == "~" else url.host @@ -175,7 +171,7 @@ def _split_image_name( raise ValueError("too many tags") return image, tag - def _check_allowed_uri_elements(self, url: URL) -> None: + def _check_allowed_uri_elements(self, url: URL, allow_port: bool = False) -> None: if not url.path or url.path == "/": raise ValueError("no image name specified") if url.query: @@ -186,7 +182,7 @@ 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: + if url.port and not allow_port: raise ValueError(f"port is not allowed, found: '{url.port}'") From 15d4ad2908df2369e3f77500640fd7214aa9bc96 Mon Sep 17 00:00:00 2001 From: Artem Yushkovskiy Date: Thu, 1 Aug 2019 17:38:37 +0300 Subject: [PATCH 3/4] un-xfail test --- tests/api/test_images.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/tests/api/test_images.py b/tests/api/test_images.py index 42288bde5..75b152d09 100644 --- a/tests/api/test_images.py +++ b/tests/api/test_images.py @@ -592,16 +592,12 @@ def test_parse_remote__registry_has_port__image_in_good_repo(self) -> None: name="library/ubuntu", tag="v10.04", owner="bob", registry="localhost:5000" ) - @pytest.mark.xfail( - reason="issue #938: `_ImageNameParser.parse_remote()`: " - "do not fall back to `parse_as_local_image()`" - ) 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="scheme 'image://' is required"): + with pytest.raises(ValueError, match="too many tags"): my_parser.parse_remote(image) From 115d0092c2e2759f3dbbae4711b6ffc2a9dfe205 Mon Sep 17 00:00:00 2001 From: Artem Yushkovskiy Date: Thu, 1 Aug 2019 17:46:20 +0300 Subject: [PATCH 4/4] review: port is allowed --- neuromation/api/parsing_utils.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/neuromation/api/parsing_utils.py b/neuromation/api/parsing_utils.py index c70969433..0a4a6486c 100644 --- a/neuromation/api/parsing_utils.py +++ b/neuromation/api/parsing_utils.py @@ -133,13 +133,11 @@ def _parse_as_neuro_image( if not self.is_in_neuro_registry(image): raise ValueError("scheme 'image://' is required for remote images") - allow_port = False url = URL(image) if url.scheme and url.scheme != "image": # image with port in registry: `localhost:5000/owner/ubuntu:latest` url = URL(f"//{url}") - allow_port = True if not url.scheme: parts = url.path.split("/") @@ -150,7 +148,7 @@ def _parse_as_neuro_image( query=url.query, ) - self._check_allowed_uri_elements(url, allow_port=allow_port) + self._check_allowed_uri_elements(url) registry = self._registry owner = self._default_user if not url.host or url.host == "~" else url.host @@ -171,7 +169,7 @@ def _split_image_name( raise ValueError("too many tags") return image, tag - def _check_allowed_uri_elements(self, url: URL, allow_port: bool = False) -> None: + 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: @@ -182,8 +180,10 @@ def _check_allowed_uri_elements(self, url: URL, allow_port: bool = False) -> Non 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 and not allow_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]: