From ac0def4028ac9264f42e9c0eef571e8ec9711153 Mon Sep 17 00:00:00 2001 From: YuviPanda Date: Wed, 26 Feb 2025 11:04:19 -0800 Subject: [PATCH 01/35] Use CLI to push docker image Preserves existing authentication information handling behavior: 1. If registry_credentials are present, they are used but not leaked on to existing ~/.docker/config 2. If registry_credentials are not present, they are not used 3. Regardless of registry_credentials being present, we still will use existing authentication info in DOCKER_CONFIG --- repo2docker/docker.py | 52 ++++++++++++++++++++++++++++++++++--------- 1 file changed, 41 insertions(+), 11 deletions(-) diff --git a/repo2docker/docker.py b/repo2docker/docker.py index b92f807d..aaec2774 100644 --- a/repo2docker/docker.py +++ b/repo2docker/docker.py @@ -2,9 +2,13 @@ Docker container engine for repo2docker """ +import os import shutil +import subprocess import tarfile import tempfile +from contextlib import contextmanager +from pathlib import Path from iso8601 import parse_date from traitlets import Dict, List, Unicode @@ -58,7 +62,7 @@ class DockerEngine(ContainerEngine): https://docker-py.readthedocs.io/en/4.2.0/api.html#module-docker.api.build """ - string_output = False + string_output = True extra_init_args = Dict( {}, @@ -141,18 +145,12 @@ def build( args += [d] - for line in execute_cmd(args, True): - # Simulate structured JSON output from buildx build, since we - # do get structured json output from pushing and running - yield {"stream": line} + yield from execute_cmd(args, True) else: # Assume 'path' is passed in args += [path] - for line in execute_cmd(args, True): - # Simulate structured JSON output from buildx build, since we - # do get structured json output from pushing and running - yield {"stream": line} + yield from execute_cmd(args, True) def images(self): images = self._apiclient.images() @@ -162,10 +160,42 @@ def inspect_image(self, image): image = self._apiclient.inspect_image(image) return Image(tags=image["RepoTags"], config=image["Config"]) + @contextmanager + def docker_login(self, username, password, registry): + # Determine existing DOCKER_CONFIG + dc_path = Path( + os.environ.get("DOCKER_CONFIG", os.path.expanduser("~/.docker/config.json")) + ) + + with tempfile.TemporaryDirectory() as d: + new_dc_path = Path(d) / "config.json" + if dc_path.exists(): + # If there is an existing DOCKER_CONFIG, copy it to new location so we inherit + # whatever configuration the user has already set + shutil.copy2(dc_path, new_dc_path) + + env = os.environ.copy() + subprocess.check_call( + # FIXME: This should be using --password-stdin instead + [ + "docker", + "login", + "--username", + username, + "--password", + password, + registry, + ], + env=env, + ) + yield + def push(self, image_spec): if self.registry_credentials: - self._apiclient.login(**self.registry_credentials) - return self._apiclient.push(image_spec, stream=True) + with self.docker_login(**self.registry_credentials): + yield from execute_cmd(["docker", "push", image_spec], capture=True) + else: + yield from execute_cmd(["docker", "push", image_spec], capture=True) def run( self, From 268e18765b6e3760390690a8fbaf09f7ac7cc322 Mon Sep 17 00:00:00 2001 From: YuviPanda Date: Wed, 26 Feb 2025 12:58:29 -0800 Subject: [PATCH 02/35] Properly set DOCKER_CONFIG to be a dir, not a file --- repo2docker/docker.py | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/repo2docker/docker.py b/repo2docker/docker.py index aaec2774..b4361a69 100644 --- a/repo2docker/docker.py +++ b/repo2docker/docker.py @@ -163,9 +163,11 @@ def inspect_image(self, image): @contextmanager def docker_login(self, username, password, registry): # Determine existing DOCKER_CONFIG - dc_path = Path( - os.environ.get("DOCKER_CONFIG", os.path.expanduser("~/.docker/config.json")) - ) + old_dc_path = os.environ.get("DOCKER_CONFIG") + if old_dc_path is None: + dc_path = Path("~/.docker/config.json").expanduser() + else: + dc_path = Path(old_dc_path) with tempfile.TemporaryDirectory() as d: new_dc_path = Path(d) / "config.json" @@ -174,21 +176,25 @@ def docker_login(self, username, password, registry): # whatever configuration the user has already set shutil.copy2(dc_path, new_dc_path) - env = os.environ.copy() - subprocess.check_call( - # FIXME: This should be using --password-stdin instead - [ + os.environ['DOCKER_CONFIG'] = d + proc = subprocess.run([ "docker", "login", "--username", username, - "--password", - password, + "--password-stdin", registry, ], - env=env, + input=password.encode(), + check=True ) - yield + try: + yield + finally: + if old_dc_path: + os.environ['DOCKER_CONFIG'] = old_dc_path + else: + del os.environ['DOCKER_CONFIG'] def push(self, image_spec): if self.registry_credentials: From 3bdc004e461bbb204b1edef448d6fa24f3882158 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Wed, 26 Feb 2025 21:00:17 +0000 Subject: [PATCH 03/35] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- repo2docker/docker.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/repo2docker/docker.py b/repo2docker/docker.py index b4361a69..4222fadd 100644 --- a/repo2docker/docker.py +++ b/repo2docker/docker.py @@ -176,8 +176,9 @@ def docker_login(self, username, password, registry): # whatever configuration the user has already set shutil.copy2(dc_path, new_dc_path) - os.environ['DOCKER_CONFIG'] = d - proc = subprocess.run([ + os.environ["DOCKER_CONFIG"] = d + proc = subprocess.run( + [ "docker", "login", "--username", @@ -186,15 +187,15 @@ def docker_login(self, username, password, registry): registry, ], input=password.encode(), - check=True + check=True, ) try: yield finally: if old_dc_path: - os.environ['DOCKER_CONFIG'] = old_dc_path + os.environ["DOCKER_CONFIG"] = old_dc_path else: - del os.environ['DOCKER_CONFIG'] + del os.environ["DOCKER_CONFIG"] def push(self, image_spec): if self.registry_credentials: From 364a18932c0f41aa441e14a593eeffcaf08a8c48 Mon Sep 17 00:00:00 2001 From: YuviPanda Date: Thu, 27 Feb 2025 20:21:22 -0800 Subject: [PATCH 04/35] Add a local registry in github actions for us to test with --- .github/workflows/test.yml | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index de4fd3b9..924b2316 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -40,6 +40,7 @@ env: GIT_AUTHOR_EMAIL: ci-user@github.local GIT_AUTHOR_NAME: CI User + jobs: test: # Don't run scheduled tests on forks @@ -72,8 +73,22 @@ jobs: python_version: "3.9" repo_type: venv + services: + # So that we can test this in PRs/branches + local-registry: + image: registry:2 + ports: + - 5000:5000 + steps: - uses: actions/checkout@v4 + + - name: Set up Docker Buildx + uses: docker/setup-buildx-action@v3 + with: + # Allows pushing to registry on localhost:5000 + driver-opts: network=host + - uses: actions/setup-python@v5 with: python-version: "${{ matrix.python_version }}" From 82e049fcec44db30d3f7c90f05392e62454158b6 Mon Sep 17 00:00:00 2001 From: YuviPanda Date: Thu, 27 Feb 2025 20:49:09 -0800 Subject: [PATCH 05/35] Add a docker registry integration test with real registry --- .github/workflows/test.yml | 14 +-------- repo2docker/app.py | 15 ++-------- repo2docker/utils.py | 12 ++++++++ tests/registry/Dockerfile | 2 ++ tests/registry/test_registry.py | 50 +++++++++++++++++++++++++++++++++ tests/unit/test_docker.py | 40 -------------------------- 6 files changed, 67 insertions(+), 66 deletions(-) create mode 100644 tests/registry/Dockerfile create mode 100644 tests/registry/test_registry.py diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 924b2316..07bfc368 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -65,6 +65,7 @@ jobs: - unit - venv - contentproviders + - registry # Playwright test - ui include: @@ -73,22 +74,9 @@ jobs: python_version: "3.9" repo_type: venv - services: - # So that we can test this in PRs/branches - local-registry: - image: registry:2 - ports: - - 5000:5000 - steps: - uses: actions/checkout@v4 - - name: Set up Docker Buildx - uses: docker/setup-buildx-action@v3 - with: - # Allows pushing to registry on localhost:5000 - driver-opts: network=host - - uses: actions/setup-python@v5 with: python-version: "${{ matrix.python_version }}" diff --git a/repo2docker/app.py b/repo2docker/app.py index 0d817452..418ef12d 100755 --- a/repo2docker/app.py +++ b/repo2docker/app.py @@ -37,7 +37,7 @@ RBuildPack, ) from .engine import BuildError, ContainerEngineException, ImageLoadError -from .utils import ByteSpecification, R2dState, chdir, get_platform +from .utils import ByteSpecification, R2dState, chdir, get_platform, get_free_port class Repo2Docker(Application): @@ -660,7 +660,7 @@ def start_container(self): container_port = int(container_port_proto.split("/", 1)[0]) else: # no port specified, pick a random one - container_port = host_port = str(self._get_free_port()) + container_port = host_port = str(get_free_port()) self.ports = {f"{container_port}/tcp": host_port} self.port = host_port # To use the option --NotebookApp.custom_display_url @@ -744,17 +744,6 @@ def wait_for_container(self, container): if exit_code: sys.exit(exit_code) - def _get_free_port(self): - """ - Hacky method to get a free random port on local host - """ - import socket - - s = socket.socket(socket.AF_INET, socket.SOCK_STREAM) - s.bind(("", 0)) - port = s.getsockname()[1] - s.close() - return port def find_image(self): # if this is a dry run it is Ok for dockerd to be unreachable so we diff --git a/repo2docker/utils.py b/repo2docker/utils.py index 9c2769e1..ebae3da9 100644 --- a/repo2docker/utils.py +++ b/repo2docker/utils.py @@ -1,4 +1,5 @@ import os +import socket import platform import re import subprocess @@ -545,3 +546,14 @@ def get_platform(): else: warnings.warn(f"Unexpected platform '{m}', defaulting to linux/amd64") return "linux/amd64" + + +def get_free_port(): + """ + Hacky method to get a free random port on local host + """ + s = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + s.bind(("", 0)) + port = s.getsockname()[1] + s.close() + return port \ No newline at end of file diff --git a/tests/registry/Dockerfile b/tests/registry/Dockerfile new file mode 100644 index 00000000..bd083ea7 --- /dev/null +++ b/tests/registry/Dockerfile @@ -0,0 +1,2 @@ +# Smallest possible dockerfile, used only for building images to be tested +FROM scratch \ No newline at end of file diff --git a/tests/registry/test_registry.py b/tests/registry/test_registry.py new file mode 100644 index 00000000..03b00566 --- /dev/null +++ b/tests/registry/test_registry.py @@ -0,0 +1,50 @@ +from pathlib import Path +import subprocess +import pytest +from repo2docker.__main__ import make_r2d +from repo2docker.utils import get_free_port +import time +import requests +import secrets + +HERE = Path(__file__).parent + +@pytest.fixture +def registry(): + port = get_free_port() + cmd = [ + "docker", "run", "-it", "-p", f"{port}:5000", "registry:3.0.0-rc.3" + ] + proc = subprocess.Popen(cmd) + health_url = f'http://localhost:{port}/v2' + # Wait for the registry to actually come up + for i in range(10): + try: + resp = requests.get(health_url) + if resp.status_code in (401, 200): + break + except requests.ConnectionError: + # The service is not up yet + pass + time.sleep(i) + else: + raise TimeoutError("Test registry did not come up in time") + + try: + yield f"localhost:{port}" + finally: + proc.terminate() + proc.wait() + + +def test_registry(registry): + image_name = f"{registry}/{secrets.token_hex(8)}:latest" + r2d = make_r2d([ + "--image", image_name, + "--push", "--no-run", str(HERE) + ]) + + r2d.start() + + proc = subprocess.run(["docker", "manifest", "inspect", "--insecure", image_name]) + assert proc.returncode == 0 diff --git a/tests/unit/test_docker.py b/tests/unit/test_docker.py index c47aa4fa..a41bafa3 100644 --- a/tests/unit/test_docker.py +++ b/tests/unit/test_docker.py @@ -22,43 +22,3 @@ def test_git_credential_env(): .strip() ) assert out == credential_env - - -class MockDockerEngine(DockerEngine): - def __init__(self, *args, **kwargs): - self._apiclient = Mock() - - -def test_docker_push_no_credentials(): - engine = MockDockerEngine() - - engine.push("image") - - assert len(engine._apiclient.method_calls) == 1 - engine._apiclient.push.assert_called_once_with("image", stream=True) - - -def test_docker_push_dict_credentials(): - engine = MockDockerEngine() - engine.registry_credentials = {"username": "abc", "password": "def"} - - engine.push("image") - - assert len(engine._apiclient.method_calls) == 2 - engine._apiclient.login.assert_called_once_with(username="abc", password="def") - engine._apiclient.push.assert_called_once_with("image", stream=True) - - -def test_docker_push_env_credentials(): - engine = MockDockerEngine() - with patch.dict( - "os.environ", - { - "CONTAINER_ENGINE_REGISTRY_CREDENTIALS": '{"username": "abc", "password": "def"}' - }, - ): - engine.push("image") - - assert len(engine._apiclient.method_calls) == 2 - engine._apiclient.login.assert_called_once_with(username="abc", password="def") - engine._apiclient.push.assert_called_once_with("image", stream=True) From 8444a28d866189117b28b3ba4d5ed134b566c056 Mon Sep 17 00:00:00 2001 From: YuviPanda Date: Thu, 27 Feb 2025 21:26:42 -0800 Subject: [PATCH 06/35] Explicitly pull registry image before trying to run --- tests/registry/test_registry.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/registry/test_registry.py b/tests/registry/test_registry.py index 03b00566..7d9795c0 100644 --- a/tests/registry/test_registry.py +++ b/tests/registry/test_registry.py @@ -12,8 +12,11 @@ @pytest.fixture def registry(): port = get_free_port() + # Explicitly pull the image first so it runs on time + registry_image = "registry:3.0.0-rc.3" + subprocess.check_call(["docker", "pull", registry_image]) cmd = [ - "docker", "run", "-it", "-p", f"{port}:5000", "registry:3.0.0-rc.3" + "docker", "run", "-it", "-p", f"{port}:5000", registry_image ] proc = subprocess.Popen(cmd) health_url = f'http://localhost:{port}/v2' From 69e67a23bf22609f0f27a64800d4c720c28c4b52 Mon Sep 17 00:00:00 2001 From: YuviPanda Date: Thu, 27 Feb 2025 21:28:50 -0800 Subject: [PATCH 07/35] Don't need -it for docker run here --- tests/registry/test_registry.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/registry/test_registry.py b/tests/registry/test_registry.py index 7d9795c0..eed3f0f7 100644 --- a/tests/registry/test_registry.py +++ b/tests/registry/test_registry.py @@ -16,7 +16,7 @@ def registry(): registry_image = "registry:3.0.0-rc.3" subprocess.check_call(["docker", "pull", registry_image]) cmd = [ - "docker", "run", "-it", "-p", f"{port}:5000", registry_image + "docker", "run", "-p", f"{port}:5000", registry_image ] proc = subprocess.Popen(cmd) health_url = f'http://localhost:{port}/v2' From a8369beed4d4cec8bd842a3e676990057c82f437 Mon Sep 17 00:00:00 2001 From: YuviPanda Date: Fri, 28 Feb 2025 09:44:35 -0800 Subject: [PATCH 08/35] Use custom dind for talking to insecure registry --- tests/registry/test_registry.py | 60 +++++++++++++++++++++++++++++---- 1 file changed, 54 insertions(+), 6 deletions(-) diff --git a/tests/registry/test_registry.py b/tests/registry/test_registry.py index eed3f0f7..8c4ef441 100644 --- a/tests/registry/test_registry.py +++ b/tests/registry/test_registry.py @@ -1,3 +1,5 @@ +import os +import socket from pathlib import Path import subprocess import pytest @@ -9,17 +11,62 @@ HERE = Path(__file__).parent -@pytest.fixture -def registry(): +@pytest.fixture(scope="session") +def dind(registry, host_ip): + port = get_free_port() + dind_image = "docker:dind" + subprocess.check_call(["docker", "pull", dind_image]) + # This is insecure, because I'm disabling all kinds of authentication on the docker daemon. + # FIXME: Use TLS verification. + # but also docker this is your own fucking fault for making technical choices that force dockerhub + # to be the primary registry, so your registry handling sucks and forces these kinds of difficulties. + cmd = [ + "docker", "run", "-e", 'DOCKER_TLS_CERTDIR=', + "--privileged", "-p", f"{port}:2376", dind_image, + "--host", "0.0.0.0:2376", + "--insecure-registry", registry, + "--tls=false" + ] + proc = subprocess.Popen(cmd) + time.sleep(5) + + try: + yield f"tcp://{host_ip}:{port}" + finally: + proc.terminate() + proc.wait() + +@pytest.fixture(scope="session") +def host_ip(): + # Get the IP of the current machine, as we need to use the same IP + # for all our docker commands, *and* the dind we run needs to reach it + # in the same way. + # Thanks to https://stackoverflow.com/a/28950776 + s = socket.socket(socket.AF_INET, socket.SOCK_DGRAM) + s.settimeout(0) + try: + # doesn't even have to be reachable + s.connect(('10.254.254.254', 1)) + host_ip = s.getsockname()[0] + finally: + s.close() + + return host_ip + +@pytest.fixture(scope="session") +def registry(host_ip): port = get_free_port() # Explicitly pull the image first so it runs on time registry_image = "registry:3.0.0-rc.3" subprocess.check_call(["docker", "pull", registry_image]) + + cmd = [ - "docker", "run", "-p", f"{port}:5000", registry_image + "docker", "run", "--rm", + "-p", f"{port}:5000", registry_image ] proc = subprocess.Popen(cmd) - health_url = f'http://localhost:{port}/v2' + health_url = f'http://{host_ip}:{port}/v2' # Wait for the registry to actually come up for i in range(10): try: @@ -34,19 +81,20 @@ def registry(): raise TimeoutError("Test registry did not come up in time") try: - yield f"localhost:{port}" + yield f"{host_ip}:{port}" finally: proc.terminate() proc.wait() -def test_registry(registry): +def test_registry(registry, dind): image_name = f"{registry}/{secrets.token_hex(8)}:latest" r2d = make_r2d([ "--image", image_name, "--push", "--no-run", str(HERE) ]) + os.environ["DOCKER_HOST"] = dind r2d.start() proc = subprocess.run(["docker", "manifest", "inspect", "--insecure", image_name]) From 9f6fbf76784202873f8a29598d698e7770eb7eec Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Fri, 28 Feb 2025 17:44:57 +0000 Subject: [PATCH 09/35] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- repo2docker/app.py | 3 +-- repo2docker/utils.py | 4 +-- tests/registry/test_registry.py | 48 ++++++++++++++++++--------------- 3 files changed, 30 insertions(+), 25 deletions(-) diff --git a/repo2docker/app.py b/repo2docker/app.py index 418ef12d..d70ec761 100755 --- a/repo2docker/app.py +++ b/repo2docker/app.py @@ -37,7 +37,7 @@ RBuildPack, ) from .engine import BuildError, ContainerEngineException, ImageLoadError -from .utils import ByteSpecification, R2dState, chdir, get_platform, get_free_port +from .utils import ByteSpecification, R2dState, chdir, get_free_port, get_platform class Repo2Docker(Application): @@ -744,7 +744,6 @@ def wait_for_container(self, container): if exit_code: sys.exit(exit_code) - def find_image(self): # if this is a dry run it is Ok for dockerd to be unreachable so we # always return False for dry runs. diff --git a/repo2docker/utils.py b/repo2docker/utils.py index ebae3da9..e71dcf94 100644 --- a/repo2docker/utils.py +++ b/repo2docker/utils.py @@ -1,7 +1,7 @@ import os -import socket import platform import re +import socket import subprocess import warnings from contextlib import contextmanager @@ -556,4 +556,4 @@ def get_free_port(): s.bind(("", 0)) port = s.getsockname()[1] s.close() - return port \ No newline at end of file + return port diff --git a/tests/registry/test_registry.py b/tests/registry/test_registry.py index 8c4ef441..c641b43f 100644 --- a/tests/registry/test_registry.py +++ b/tests/registry/test_registry.py @@ -1,16 +1,19 @@ import os +import secrets import socket -from pathlib import Path import subprocess +import time +from pathlib import Path + import pytest +import requests + from repo2docker.__main__ import make_r2d from repo2docker.utils import get_free_port -import time -import requests -import secrets HERE = Path(__file__).parent + @pytest.fixture(scope="session") def dind(registry, host_ip): port = get_free_port() @@ -21,11 +24,19 @@ def dind(registry, host_ip): # but also docker this is your own fucking fault for making technical choices that force dockerhub # to be the primary registry, so your registry handling sucks and forces these kinds of difficulties. cmd = [ - "docker", "run", "-e", 'DOCKER_TLS_CERTDIR=', - "--privileged", "-p", f"{port}:2376", dind_image, - "--host", "0.0.0.0:2376", - "--insecure-registry", registry, - "--tls=false" + "docker", + "run", + "-e", + "DOCKER_TLS_CERTDIR=", + "--privileged", + "-p", + f"{port}:2376", + dind_image, + "--host", + "0.0.0.0:2376", + "--insecure-registry", + registry, + "--tls=false", ] proc = subprocess.Popen(cmd) time.sleep(5) @@ -36,6 +47,7 @@ def dind(registry, host_ip): proc.terminate() proc.wait() + @pytest.fixture(scope="session") def host_ip(): # Get the IP of the current machine, as we need to use the same IP @@ -46,27 +58,24 @@ def host_ip(): s.settimeout(0) try: # doesn't even have to be reachable - s.connect(('10.254.254.254', 1)) + s.connect(("10.254.254.254", 1)) host_ip = s.getsockname()[0] finally: s.close() return host_ip + @pytest.fixture(scope="session") def registry(host_ip): port = get_free_port() # Explicitly pull the image first so it runs on time - registry_image = "registry:3.0.0-rc.3" + registry_image = "registry:3.0.0-rc.3" subprocess.check_call(["docker", "pull", registry_image]) - - cmd = [ - "docker", "run", "--rm", - "-p", f"{port}:5000", registry_image - ] + cmd = ["docker", "run", "--rm", "-p", f"{port}:5000", registry_image] proc = subprocess.Popen(cmd) - health_url = f'http://{host_ip}:{port}/v2' + health_url = f"http://{host_ip}:{port}/v2" # Wait for the registry to actually come up for i in range(10): try: @@ -89,10 +98,7 @@ def registry(host_ip): def test_registry(registry, dind): image_name = f"{registry}/{secrets.token_hex(8)}:latest" - r2d = make_r2d([ - "--image", image_name, - "--push", "--no-run", str(HERE) - ]) + r2d = make_r2d(["--image", image_name, "--push", "--no-run", str(HERE)]) os.environ["DOCKER_HOST"] = dind r2d.start() From 576b8967bc1131793f9a7502b5c978aec065d6ff Mon Sep 17 00:00:00 2001 From: YuviPanda Date: Fri, 28 Feb 2025 11:17:10 -0800 Subject: [PATCH 10/35] Stop using docker-py for anything Also properly setup TLS for docker daemon to close security hole --- repo2docker/app.py | 8 ++------ repo2docker/buildpacks/docker.py | 2 -- repo2docker/docker.py | 32 ++++++++++++++------------------ repo2docker/engine.py | 12 +----------- setup.py | 1 - tests/registry/test_registry.py | 23 +++++++++++++++-------- tests/unit/test_docker.py | 3 --- tests/unit/test_ports.py | 4 ---- 8 files changed, 32 insertions(+), 53 deletions(-) diff --git a/repo2docker/app.py b/repo2docker/app.py index d70ec761..54482b7a 100755 --- a/repo2docker/app.py +++ b/repo2docker/app.py @@ -750,12 +750,8 @@ def find_image(self): if self.dry_run: return False # check if we already have an image for this content - client = self.get_engine() - for image in client.images(): - for tag in image.tags: - if tag == self.output_image_spec + ":latest": - return True - return False + engine = self.get_engine() + return engine.inspect_image(self.output_image_spec) is not None def build(self): """ diff --git a/repo2docker/buildpacks/docker.py b/repo2docker/buildpacks/docker.py index 9185307a..b0e58778 100644 --- a/repo2docker/buildpacks/docker.py +++ b/repo2docker/buildpacks/docker.py @@ -3,8 +3,6 @@ import os -import docker - from .base import BuildPack diff --git a/repo2docker/docker.py b/repo2docker/docker.py index 4222fadd..310a9082 100644 --- a/repo2docker/docker.py +++ b/repo2docker/docker.py @@ -2,6 +2,7 @@ Docker container engine for repo2docker """ +import json import os import shutil import subprocess @@ -13,9 +14,7 @@ from iso8601 import parse_date from traitlets import Dict, List, Unicode -import docker - -from .engine import Container, ContainerEngine, ContainerEngineException, Image +from .engine import Container, ContainerEngine, Image from .utils import execute_cmd @@ -86,16 +85,6 @@ class DockerEngine(ContainerEngine): config=True, ) - def __init__(self, *, parent): - super().__init__(parent=parent) - try: - kwargs = docker.utils.kwargs_from_env() - kwargs.update(self.extra_init_args) - kwargs.setdefault("version", "auto") - self._apiclient = docker.APIClient(**kwargs) - except docker.errors.DockerException as e: - raise ContainerEngineException("Check if docker is running on the host.", e) - def build( self, *, @@ -152,13 +141,20 @@ def build( yield from execute_cmd(args, True) - def images(self): - images = self._apiclient.images() - return [Image(tags=image["RepoTags"]) for image in images] def inspect_image(self, image): - image = self._apiclient.inspect_image(image) - return Image(tags=image["RepoTags"], config=image["Config"]) + """ + Return image configuration if it exists, otherwise None + """ + proc = subprocess.run([ + "docker", "image", "inspect", image + ], capture_output=True) + + if proc.returncode != 0: + return None + + config = json.loads(proc.stdout.decode()) + return Image(tags=config["RepoTags"], config=config["Config"]) @contextmanager def docker_login(self, username, password, registry): diff --git a/repo2docker/engine.py b/repo2docker/engine.py index a8897a64..a6932bd8 100644 --- a/repo2docker/engine.py +++ b/repo2docker/engine.py @@ -254,19 +254,9 @@ def build( """ raise NotImplementedError("build not implemented") - def images(self): - """ - List images - - Returns - ------- - list[Image] : List of Image objects. - """ - raise NotImplementedError("images not implemented") - def inspect_image(self, image): """ - Get information about an image + Get information about an image, or None if the image does not exist TODO: This is specific to the engine, can we convert it to a standard format? diff --git a/setup.py b/setup.py index c8fcbbd7..8c6fcedd 100644 --- a/setup.py +++ b/setup.py @@ -50,7 +50,6 @@ def get_identifier(json): version=versioneer.get_version(), install_requires=[ "chardet", - "docker!=5.0.0", "entrypoints", "escapism", "iso8601", diff --git a/tests/registry/test_registry.py b/tests/registry/test_registry.py index c641b43f..46ea5e2a 100644 --- a/tests/registry/test_registry.py +++ b/tests/registry/test_registry.py @@ -1,4 +1,5 @@ import os +import shutil import secrets import socket import subprocess @@ -17,18 +18,21 @@ @pytest.fixture(scope="session") def dind(registry, host_ip): port = get_free_port() + + # Generate CA certs here so we can securely connect to the docker daemon + cert_dir = HERE / f"tmp-certs-{secrets.token_hex(8)}" + cert_dir.mkdir() + dind_image = "docker:dind" subprocess.check_call(["docker", "pull", dind_image]) - # This is insecure, because I'm disabling all kinds of authentication on the docker daemon. - # FIXME: Use TLS verification. - # but also docker this is your own fucking fault for making technical choices that force dockerhub - # to be the primary registry, so your registry handling sucks and forces these kinds of difficulties. + cmd = [ "docker", "run", "-e", - "DOCKER_TLS_CERTDIR=", + "DOCKER_TLS_CERTDIR=/opt/certs", "--privileged", + "--mount", f"type=bind,src={cert_dir},dst=/opt/certs", "-p", f"{port}:2376", dind_image, @@ -36,14 +40,14 @@ def dind(registry, host_ip): "0.0.0.0:2376", "--insecure-registry", registry, - "--tls=false", ] proc = subprocess.Popen(cmd) time.sleep(5) try: - yield f"tcp://{host_ip}:{port}" + yield f"tcp://127.0.0.1:{port}", cert_dir finally: + shutil.rmtree(cert_dir) proc.terminate() proc.wait() @@ -100,7 +104,10 @@ def test_registry(registry, dind): image_name = f"{registry}/{secrets.token_hex(8)}:latest" r2d = make_r2d(["--image", image_name, "--push", "--no-run", str(HERE)]) - os.environ["DOCKER_HOST"] = dind + docker_host, cert_dir = dind + os.environ["DOCKER_HOST"] = docker_host + os.environ["DOCKER_CERT_PATH"] = str(cert_dir / "client") + os.environ["DOCKER_TLS_VERIFY"] = "1" r2d.start() proc = subprocess.run(["docker", "manifest", "inspect", "--insecure", image_name]) diff --git a/tests/unit/test_docker.py b/tests/unit/test_docker.py index a41bafa3..2fe0b6f5 100644 --- a/tests/unit/test_docker.py +++ b/tests/unit/test_docker.py @@ -2,9 +2,6 @@ import os from subprocess import check_output -from unittest.mock import Mock, patch - -from repo2docker.docker import DockerEngine repo_root = os.path.abspath( os.path.join(os.path.dirname(__file__), os.pardir, os.pardir) diff --git a/tests/unit/test_ports.py b/tests/unit/test_ports.py index aaebcdab..43fd990b 100644 --- a/tests/unit/test_ports.py +++ b/tests/unit/test_ports.py @@ -11,7 +11,6 @@ import pytest import requests -import docker from repo2docker.__main__ import make_r2d from repo2docker.app import Repo2Docker @@ -69,10 +68,7 @@ def _cleanup(): container.reload() if container.status == "running": container.kill() - try: container.remove() - except docker.errors.NotFound: - pass request.addfinalizer(_cleanup) From 48da2c1bf4dadcea5e9f0b697501c2e742128c07 Mon Sep 17 00:00:00 2001 From: YuviPanda Date: Fri, 28 Feb 2025 11:29:14 -0800 Subject: [PATCH 11/35] Re-add dockerpy for using exclude patterns --- setup.py | 1 + 1 file changed, 1 insertion(+) diff --git a/setup.py b/setup.py index 8c6fcedd..c8fcbbd7 100644 --- a/setup.py +++ b/setup.py @@ -50,6 +50,7 @@ def get_identifier(json): version=versioneer.get_version(), install_requires=[ "chardet", + "docker!=5.0.0", "entrypoints", "escapism", "iso8601", From a0f02ce2c6a16d3b44db0f6ef83a1c1c0e532a10 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Fri, 28 Feb 2025 19:31:50 +0000 Subject: [PATCH 12/35] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- repo2docker/docker.py | 7 +++---- tests/registry/test_registry.py | 5 +++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/repo2docker/docker.py b/repo2docker/docker.py index 310a9082..39670e3b 100644 --- a/repo2docker/docker.py +++ b/repo2docker/docker.py @@ -141,14 +141,13 @@ def build( yield from execute_cmd(args, True) - def inspect_image(self, image): """ Return image configuration if it exists, otherwise None """ - proc = subprocess.run([ - "docker", "image", "inspect", image - ], capture_output=True) + proc = subprocess.run( + ["docker", "image", "inspect", image], capture_output=True + ) if proc.returncode != 0: return None diff --git a/tests/registry/test_registry.py b/tests/registry/test_registry.py index 46ea5e2a..4960ad21 100644 --- a/tests/registry/test_registry.py +++ b/tests/registry/test_registry.py @@ -1,6 +1,6 @@ import os -import shutil import secrets +import shutil import socket import subprocess import time @@ -32,7 +32,8 @@ def dind(registry, host_ip): "-e", "DOCKER_TLS_CERTDIR=/opt/certs", "--privileged", - "--mount", f"type=bind,src={cert_dir},dst=/opt/certs", + "--mount", + f"type=bind,src={cert_dir},dst=/opt/certs", "-p", f"{port}:2376", dind_image, From 2d9a3507002abd4adabcfc41448fd6a7a860d2f0 Mon Sep 17 00:00:00 2001 From: YuviPanda Date: Fri, 28 Feb 2025 11:34:35 -0800 Subject: [PATCH 13/35] Inspect returns a list of dicts --- repo2docker/docker.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/repo2docker/docker.py b/repo2docker/docker.py index 39670e3b..858f793f 100644 --- a/repo2docker/docker.py +++ b/repo2docker/docker.py @@ -152,7 +152,7 @@ def inspect_image(self, image): if proc.returncode != 0: return None - config = json.loads(proc.stdout.decode()) + config = json.loads(proc.stdout.decode())[0] return Image(tags=config["RepoTags"], config=config["Config"]) @contextmanager From af1050f72bcab5c211ecdbb1491298df42425667 Mon Sep 17 00:00:00 2001 From: YuviPanda Date: Fri, 28 Feb 2025 11:45:26 -0800 Subject: [PATCH 14/35] Re-add docker import for container running --- repo2docker/docker.py | 1 + 1 file changed, 1 insertion(+) diff --git a/repo2docker/docker.py b/repo2docker/docker.py index 858f793f..d3dde576 100644 --- a/repo2docker/docker.py +++ b/repo2docker/docker.py @@ -10,6 +10,7 @@ import tempfile from contextlib import contextmanager from pathlib import Path +import docker from iso8601 import parse_date from traitlets import Dict, List, Unicode From 5aaf26aa9d73caff9636dde557a6829b94390f37 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Fri, 28 Feb 2025 19:45:45 +0000 Subject: [PATCH 15/35] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- repo2docker/docker.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/repo2docker/docker.py b/repo2docker/docker.py index d3dde576..1cb55fd0 100644 --- a/repo2docker/docker.py +++ b/repo2docker/docker.py @@ -10,11 +10,12 @@ import tempfile from contextlib import contextmanager from pathlib import Path -import docker from iso8601 import parse_date from traitlets import Dict, List, Unicode +import docker + from .engine import Container, ContainerEngine, Image from .utils import execute_cmd From 9753570370edd79c3f27fa0bf881e04d5c887bf4 Mon Sep 17 00:00:00 2001 From: YuviPanda Date: Fri, 28 Feb 2025 13:06:17 -0800 Subject: [PATCH 16/35] Pass cert_dir removal some of the time --- tests/registry/test_registry.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/registry/test_registry.py b/tests/registry/test_registry.py index 4960ad21..72526af3 100644 --- a/tests/registry/test_registry.py +++ b/tests/registry/test_registry.py @@ -48,7 +48,11 @@ def dind(registry, host_ip): try: yield f"tcp://127.0.0.1:{port}", cert_dir finally: - shutil.rmtree(cert_dir) + try: + shutil.rmtree(cert_dir) + except PermissionError: + # Sometimes this is owned by root in CI. is ok, let's let it go + pass proc.terminate() proc.wait() From c04652e706a6dfa96cf5308860c5dc99f91aa094 Mon Sep 17 00:00:00 2001 From: YuviPanda Date: Fri, 28 Feb 2025 13:11:34 -0800 Subject: [PATCH 17/35] Replace mocked find_image tests with real ones --- .github/workflows/test.yml | 2 +- tests/{registry => norun}/Dockerfile | 0 tests/norun/test_find.py | 23 +++++++++++++++++ tests/{registry => norun}/test_registry.py | 2 +- tests/unit/test_app.py | 29 ---------------------- 5 files changed, 25 insertions(+), 31 deletions(-) rename tests/{registry => norun}/Dockerfile (100%) create mode 100644 tests/norun/test_find.py rename tests/{registry => norun}/test_registry.py (99%) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 07bfc368..7e89cab5 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -65,7 +65,7 @@ jobs: - unit - venv - contentproviders - - registry + - norun # Playwright test - ui include: diff --git a/tests/registry/Dockerfile b/tests/norun/Dockerfile similarity index 100% rename from tests/registry/Dockerfile rename to tests/norun/Dockerfile diff --git a/tests/norun/test_find.py b/tests/norun/test_find.py new file mode 100644 index 00000000..24025b0f --- /dev/null +++ b/tests/norun/test_find.py @@ -0,0 +1,23 @@ +import secrets +from pathlib import Path + +from repo2docker.__main__ import make_r2d + +HERE = Path(__file__).parent + + +def test_find_image(): + image_name = f"{secrets.token_hex(8)}:latest" + r2d = make_r2d(["--image", image_name, "--no-run", str(HERE)]) + + r2d.start() + + assert r2d.find_image() + + +def test_dont_find_image(): + image_name = f"{secrets.token_hex(8)}:latest" + r2d = make_r2d(["--image", image_name, "--no-run", str(HERE)]) + + # Just don't actually start the build, so image won't be found + assert not r2d.find_image() \ No newline at end of file diff --git a/tests/registry/test_registry.py b/tests/norun/test_registry.py similarity index 99% rename from tests/registry/test_registry.py rename to tests/norun/test_registry.py index 72526af3..e65b2532 100644 --- a/tests/registry/test_registry.py +++ b/tests/norun/test_registry.py @@ -116,4 +116,4 @@ def test_registry(registry, dind): r2d.start() proc = subprocess.run(["docker", "manifest", "inspect", "--insecure", image_name]) - assert proc.returncode == 0 + assert proc.returncode == 0 \ No newline at end of file diff --git a/tests/unit/test_app.py b/tests/unit/test_app.py index 0ee4d0b3..02841ba4 100644 --- a/tests/unit/test_app.py +++ b/tests/unit/test_app.py @@ -10,35 +10,6 @@ from repo2docker.app import Repo2Docker from repo2docker.utils import chdir - -def test_find_image(): - images = [{"RepoTags": ["some-org/some-repo:latest"]}] - - with patch("repo2docker.docker.docker.APIClient") as FakeDockerClient: - instance = FakeDockerClient.return_value - instance.images.return_value = images - - r2d = Repo2Docker() - r2d.output_image_spec = "some-org/some-repo" - assert r2d.find_image() - - instance.images.assert_called_with() - - -def test_dont_find_image(): - images = [{"RepoTags": ["some-org/some-image-name:latest"]}] - - with patch("repo2docker.docker.docker.APIClient") as FakeDockerClient: - instance = FakeDockerClient.return_value - instance.images.return_value = images - - r2d = Repo2Docker() - r2d.output_image_spec = "some-org/some-other-image-name" - assert not r2d.find_image() - - instance.images.assert_called_with() - - def test_image_name_remains_unchanged(): # if we specify an image name, it should remain unmodified with TemporaryDirectory() as src: From c9b78be470a65c79d5526c23cafb5d650b705fad Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Fri, 28 Feb 2025 21:12:02 +0000 Subject: [PATCH 18/35] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- tests/norun/test_find.py | 2 +- tests/norun/test_registry.py | 2 +- tests/unit/test_app.py | 1 + 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/norun/test_find.py b/tests/norun/test_find.py index 24025b0f..6d02631d 100644 --- a/tests/norun/test_find.py +++ b/tests/norun/test_find.py @@ -20,4 +20,4 @@ def test_dont_find_image(): r2d = make_r2d(["--image", image_name, "--no-run", str(HERE)]) # Just don't actually start the build, so image won't be found - assert not r2d.find_image() \ No newline at end of file + assert not r2d.find_image() diff --git a/tests/norun/test_registry.py b/tests/norun/test_registry.py index e65b2532..72526af3 100644 --- a/tests/norun/test_registry.py +++ b/tests/norun/test_registry.py @@ -116,4 +116,4 @@ def test_registry(registry, dind): r2d.start() proc = subprocess.run(["docker", "manifest", "inspect", "--insecure", image_name]) - assert proc.returncode == 0 \ No newline at end of file + assert proc.returncode == 0 diff --git a/tests/unit/test_app.py b/tests/unit/test_app.py index 02841ba4..0b573716 100644 --- a/tests/unit/test_app.py +++ b/tests/unit/test_app.py @@ -10,6 +10,7 @@ from repo2docker.app import Repo2Docker from repo2docker.utils import chdir + def test_image_name_remains_unchanged(): # if we specify an image name, it should remain unmodified with TemporaryDirectory() as src: From 0bc550a49419a7679be0beae0d343a07d8a067cb Mon Sep 17 00:00:00 2001 From: YuviPanda Date: Fri, 28 Feb 2025 13:15:07 -0800 Subject: [PATCH 19/35] Add a .gitignore for any generated tmp certs --- tests/norun/.gitignore | 1 + 1 file changed, 1 insertion(+) create mode 100644 tests/norun/.gitignore diff --git a/tests/norun/.gitignore b/tests/norun/.gitignore new file mode 100644 index 00000000..6cbb595e --- /dev/null +++ b/tests/norun/.gitignore @@ -0,0 +1 @@ +tmp-certs-* From b854c6c30d32c238ce55f660fa4801c3825d6dbc Mon Sep 17 00:00:00 2001 From: YuviPanda Date: Fri, 28 Feb 2025 13:15:57 -0800 Subject: [PATCH 20/35] Document why we put certs in current dir --- tests/norun/test_registry.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/norun/test_registry.py b/tests/norun/test_registry.py index 72526af3..45bd7b9e 100644 --- a/tests/norun/test_registry.py +++ b/tests/norun/test_registry.py @@ -19,7 +19,9 @@ def dind(registry, host_ip): port = get_free_port() - # Generate CA certs here so we can securely connect to the docker daemon + # docker daemon will generate certs here, that we can then use to connect to it. + # put it in current dir than in /tmp because on macos, current dir is likely to + # shared with docker VM so it can be mounted, unlike /tmp cert_dir = HERE / f"tmp-certs-{secrets.token_hex(8)}" cert_dir.mkdir() From 3e458e3299cfc6cfe24d696f0368802e51768a5e Mon Sep 17 00:00:00 2001 From: YuviPanda Date: Fri, 28 Feb 2025 13:23:01 -0800 Subject: [PATCH 21/35] Remove a couple more tests --- tests/unit/test_app.py | 24 ------------------------ 1 file changed, 24 deletions(-) diff --git a/tests/unit/test_app.py b/tests/unit/test_app.py index 0b573716..04ec9086 100644 --- a/tests/unit/test_app.py +++ b/tests/unit/test_app.py @@ -1,9 +1,7 @@ -import errno from tempfile import TemporaryDirectory from unittest.mock import patch import escapism -import pytest import docker from repo2docker.__main__ import make_r2d @@ -76,25 +74,3 @@ def test_run_kwargs(repo_with_content): args, kwargs = containers.run.call_args assert "somekey" in kwargs assert kwargs["somekey"] == "somevalue" - - -def test_dryrun_works_without_docker(tmpdir, capsys): - with chdir(tmpdir): - with patch.object(docker, "APIClient") as client: - client.side_effect = docker.errors.DockerException("Error: no Docker") - app = Repo2Docker(dry_run=True) - app.build() - captured = capsys.readouterr() - assert "Error: no Docker" not in captured.err - - -def test_error_log_without_docker(tmpdir, capsys): - with chdir(tmpdir): - with patch.object(docker, "APIClient") as client: - client.side_effect = docker.errors.DockerException("Error: no Docker") - app = Repo2Docker() - - with pytest.raises(SystemExit): - app.build() - captured = capsys.readouterr() - assert "Error: no Docker" in captured.err From 1bd3d276f37de3e5a750ac1938cdbe32f6c9a44c Mon Sep 17 00:00:00 2001 From: YuviPanda Date: Fri, 28 Feb 2025 13:29:09 -0800 Subject: [PATCH 22/35] Restore env vars after test --- tests/norun/test_registry.py | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/tests/norun/test_registry.py b/tests/norun/test_registry.py index 45bd7b9e..7dcd43e3 100644 --- a/tests/norun/test_registry.py +++ b/tests/norun/test_registry.py @@ -112,10 +112,17 @@ def test_registry(registry, dind): r2d = make_r2d(["--image", image_name, "--push", "--no-run", str(HERE)]) docker_host, cert_dir = dind - os.environ["DOCKER_HOST"] = docker_host - os.environ["DOCKER_CERT_PATH"] = str(cert_dir / "client") - os.environ["DOCKER_TLS_VERIFY"] = "1" - r2d.start() - proc = subprocess.run(["docker", "manifest", "inspect", "--insecure", image_name]) - assert proc.returncode == 0 + old_environ = os.environ.copy() + + try: + os.environ["DOCKER_HOST"] = docker_host + os.environ["DOCKER_CERT_PATH"] = str(cert_dir / "client") + os.environ["DOCKER_TLS_VERIFY"] = "1" + r2d.start() + + proc = subprocess.run(["docker", "manifest", "inspect", "--insecure", image_name]) + assert proc.returncode == 0 + finally: + os.environ.clear() + os.environ.update(old_environ) \ No newline at end of file From c29423606336c6c7b4f94379c02fdbf07ef081d2 Mon Sep 17 00:00:00 2001 From: YuviPanda Date: Fri, 28 Feb 2025 13:29:15 -0800 Subject: [PATCH 23/35] Remove some more tests --- tests/unit/test_argumentvalidation.py | 27 --------------------------- 1 file changed, 27 deletions(-) diff --git a/tests/unit/test_argumentvalidation.py b/tests/unit/test_argumentvalidation.py index 78b96464..466a7d7d 100644 --- a/tests/unit/test_argumentvalidation.py +++ b/tests/unit/test_argumentvalidation.py @@ -212,33 +212,6 @@ def test_invalid_container_port_protocol_mapping_fail(temp_cwd): assert not validate_arguments(builddir, args_list, "Port specification") -def test_docker_handle_fail(temp_cwd): - """ - Test to check if r2d fails with minimal error message on not being able to connect to docker daemon - """ - args_list = [] - - assert not validate_arguments( - builddir, - args_list, - "Check if docker is running on the host.", - disable_dockerd=True, - ) - - -def test_docker_handle_debug_fail(temp_cwd): - """ - Test to check if r2d fails with helpful error message on not being able to connect to docker daemon and debug enabled - """ - args_list = ["--debug"] - - assert not validate_arguments( - builddir, - args_list, - "Check if docker is running on the host.", - disable_dockerd=True, - ) - def test_docker_no_build_success(temp_cwd): """ From aa948e08aff3c2515f6381e035a92f4ed17b7055 Mon Sep 17 00:00:00 2001 From: YuviPanda Date: Fri, 28 Feb 2025 13:44:18 -0800 Subject: [PATCH 24/35] Test login to registry as well --- dev-requirements.txt | 1 + tests/norun/test_registry.py | 82 +++++++++++++++++++++++++++++++++--- 2 files changed, 77 insertions(+), 6 deletions(-) diff --git a/dev-requirements.txt b/dev-requirements.txt index 22ba622e..1157007e 100644 --- a/dev-requirements.txt +++ b/dev-requirements.txt @@ -5,3 +5,4 @@ pytest-cov pytest>=7 pyyaml requests_mock +bcrypt \ No newline at end of file diff --git a/tests/norun/test_registry.py b/tests/norun/test_registry.py index 7dcd43e3..92aa1f6e 100644 --- a/tests/norun/test_registry.py +++ b/tests/norun/test_registry.py @@ -1,9 +1,13 @@ +import json import os import secrets import shutil import socket import subprocess +from tempfile import TemporaryDirectory +from base64 import b64encode import time +import bcrypt from pathlib import Path import pytest @@ -16,8 +20,9 @@ @pytest.fixture(scope="session") -def dind(registry, host_ip): +def dind(registry): port = get_free_port() + registry_host, _, _ = registry # docker daemon will generate certs here, that we can then use to connect to it. # put it in current dir than in /tmp because on macos, current dir is likely to @@ -42,7 +47,7 @@ def dind(registry, host_ip): "--host", "0.0.0.0:2376", "--insecure-registry", - registry, + registry_host, ] proc = subprocess.Popen(cmd) time.sleep(5) @@ -80,11 +85,27 @@ def host_ip(): @pytest.fixture(scope="session") def registry(host_ip): port = get_free_port() + username = "user" + password = secrets.token_hex(16) + bcrypted_pw = bcrypt.hashpw(password.encode("utf-8"), bcrypt.gensalt(rounds=12)).decode("utf-8") + + # We put our password here, and mount it into the container. + # put it in current dir than in /tmp because on macos, current dir is likely to + # shared with docker VM so it can be mounted, unlike /tmp + htpasswd_dir = HERE / f"tmp-certs-{secrets.token_hex(8)}" + htpasswd_dir.mkdir() + (htpasswd_dir / "htpasswd.conf").write_text(f"{username}:{bcrypted_pw}") + # Explicitly pull the image first so it runs on time registry_image = "registry:3.0.0-rc.3" subprocess.check_call(["docker", "pull", registry_image]) - cmd = ["docker", "run", "--rm", "-p", f"{port}:5000", registry_image] + cmd = ["docker", "run", "--rm", + "-e", "REGISTRY_AUTH=htpasswd", + "-e", "REGISTRY_AUTH_HTPASSWD_REALM=basic", + "-e", "REGISTRY_AUTH_HTPASSWD_PATH=/opt/htpasswd/htpasswd.conf", + "--mount", f"type=bind,src={htpasswd_dir},dst=/opt/htpasswd", + "-p", f"{port}:5000", registry_image] proc = subprocess.Popen(cmd) health_url = f"http://{host_ip}:{port}/v2" # Wait for the registry to actually come up @@ -101,14 +122,18 @@ def registry(host_ip): raise TimeoutError("Test registry did not come up in time") try: - yield f"{host_ip}:{port}" + yield f"{host_ip}:{port}", username, password finally: proc.terminate() proc.wait() -def test_registry(registry, dind): - image_name = f"{registry}/{secrets.token_hex(8)}:latest" +def test_registry_explicit_creds(registry, dind): + """ + Test that we can push to registry when given explicit credentials + """ + registry_host, username, password = registry + image_name = f"{registry_host}/{secrets.token_hex(8)}:latest" r2d = make_r2d(["--image", image_name, "--push", "--no-run", str(HERE)]) docker_host, cert_dir = dind @@ -119,10 +144,55 @@ def test_registry(registry, dind): os.environ["DOCKER_HOST"] = docker_host os.environ["DOCKER_CERT_PATH"] = str(cert_dir / "client") os.environ["DOCKER_TLS_VERIFY"] = "1" + os.environ["CONTAINER_ENGINE_REGISTRY_CREDENTIALS"] = json.dumps({ + "registry": f"http://{registry_host}", + "username": username, + "password": password + }) r2d.start() + proc = subprocess.run(["docker", "manifest", "inspect", "--insecure", image_name]) assert proc.returncode == 0 + + # Validate that we didn't leak our registry creds into existing docker config + docker_config_path = Path(os.environ.get("DOCKER_CONFIG", "~/.docker/config.json")).expanduser() + if docker_config_path.exists(): + # Just check that our randomly generated password is not in this file + # Can this cause a conflict? Sure, if there's a different randomly generated password in here + # that matches our own randomly generated password. But if you're that unlucky, take cover from the asteroid. + assert password not in docker_config_path.read_text() + finally: + os.environ.clear() + os.environ.update(old_environ) + + +def test_registry_no_explicit_creds(registry, dind): + """ + Test that we can push to registry *without* explicit credentials but reading from a DOCKER_CONFIG + """ + registry_host, username, password = registry + image_name = f"{registry_host}/{secrets.token_hex(8)}:latest" + r2d = make_r2d(["--image", image_name, "--push", "--no-run", str(HERE)]) + + docker_host, cert_dir = dind + + old_environ = os.environ.copy() + + try: + os.environ["DOCKER_HOST"] = docker_host + os.environ["DOCKER_CERT_PATH"] = str(cert_dir / "client") + os.environ["DOCKER_TLS_VERIFY"] = "1" + with TemporaryDirectory() as d: + (Path(d) / "config.json").write_text(json.dumps( + ({"auths":{f"http://{registry_host}":{"auth":b64encode(f"{username}:{password}".encode()).decode()}}}) + )) + os.environ["DOCKER_CONFIG"] = d + r2d.start() + + + proc = subprocess.run(["docker", "manifest", "inspect", "--insecure", image_name]) + assert proc.returncode == 0 finally: os.environ.clear() os.environ.update(old_environ) \ No newline at end of file From ad29ffd8e665eaae23eeffca8e329769594b72a2 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Fri, 28 Feb 2025 21:44:59 +0000 Subject: [PATCH 25/35] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- tests/norun/test_registry.py | 76 ++++++++++++++++++--------- tests/unit/test_argumentvalidation.py | 1 - 2 files changed, 52 insertions(+), 25 deletions(-) diff --git a/tests/norun/test_registry.py b/tests/norun/test_registry.py index 92aa1f6e..98a1d36c 100644 --- a/tests/norun/test_registry.py +++ b/tests/norun/test_registry.py @@ -4,12 +4,12 @@ import shutil import socket import subprocess -from tempfile import TemporaryDirectory -from base64 import b64encode import time -import bcrypt +from base64 import b64encode from pathlib import Path +from tempfile import TemporaryDirectory +import bcrypt import pytest import requests @@ -87,7 +87,9 @@ def registry(host_ip): port = get_free_port() username = "user" password = secrets.token_hex(16) - bcrypted_pw = bcrypt.hashpw(password.encode("utf-8"), bcrypt.gensalt(rounds=12)).decode("utf-8") + bcrypted_pw = bcrypt.hashpw( + password.encode("utf-8"), bcrypt.gensalt(rounds=12) + ).decode("utf-8") # We put our password here, and mount it into the container. # put it in current dir than in /tmp because on macos, current dir is likely to @@ -100,12 +102,22 @@ def registry(host_ip): registry_image = "registry:3.0.0-rc.3" subprocess.check_call(["docker", "pull", registry_image]) - cmd = ["docker", "run", "--rm", - "-e", "REGISTRY_AUTH=htpasswd", - "-e", "REGISTRY_AUTH_HTPASSWD_REALM=basic", - "-e", "REGISTRY_AUTH_HTPASSWD_PATH=/opt/htpasswd/htpasswd.conf", - "--mount", f"type=bind,src={htpasswd_dir},dst=/opt/htpasswd", - "-p", f"{port}:5000", registry_image] + cmd = [ + "docker", + "run", + "--rm", + "-e", + "REGISTRY_AUTH=htpasswd", + "-e", + "REGISTRY_AUTH_HTPASSWD_REALM=basic", + "-e", + "REGISTRY_AUTH_HTPASSWD_PATH=/opt/htpasswd/htpasswd.conf", + "--mount", + f"type=bind,src={htpasswd_dir},dst=/opt/htpasswd", + "-p", + f"{port}:5000", + registry_image, + ] proc = subprocess.Popen(cmd) health_url = f"http://{host_ip}:{port}/v2" # Wait for the registry to actually come up @@ -144,19 +156,24 @@ def test_registry_explicit_creds(registry, dind): os.environ["DOCKER_HOST"] = docker_host os.environ["DOCKER_CERT_PATH"] = str(cert_dir / "client") os.environ["DOCKER_TLS_VERIFY"] = "1" - os.environ["CONTAINER_ENGINE_REGISTRY_CREDENTIALS"] = json.dumps({ - "registry": f"http://{registry_host}", - "username": username, - "password": password - }) + os.environ["CONTAINER_ENGINE_REGISTRY_CREDENTIALS"] = json.dumps( + { + "registry": f"http://{registry_host}", + "username": username, + "password": password, + } + ) r2d.start() - - proc = subprocess.run(["docker", "manifest", "inspect", "--insecure", image_name]) + proc = subprocess.run( + ["docker", "manifest", "inspect", "--insecure", image_name] + ) assert proc.returncode == 0 # Validate that we didn't leak our registry creds into existing docker config - docker_config_path = Path(os.environ.get("DOCKER_CONFIG", "~/.docker/config.json")).expanduser() + docker_config_path = Path( + os.environ.get("DOCKER_CONFIG", "~/.docker/config.json") + ).expanduser() if docker_config_path.exists(): # Just check that our randomly generated password is not in this file # Can this cause a conflict? Sure, if there's a different randomly generated password in here @@ -184,15 +201,26 @@ def test_registry_no_explicit_creds(registry, dind): os.environ["DOCKER_CERT_PATH"] = str(cert_dir / "client") os.environ["DOCKER_TLS_VERIFY"] = "1" with TemporaryDirectory() as d: - (Path(d) / "config.json").write_text(json.dumps( - ({"auths":{f"http://{registry_host}":{"auth":b64encode(f"{username}:{password}".encode()).decode()}}}) - )) + (Path(d) / "config.json").write_text( + json.dumps( + { + "auths": { + f"http://{registry_host}": { + "auth": b64encode( + f"{username}:{password}".encode() + ).decode() + } + } + } + ) + ) os.environ["DOCKER_CONFIG"] = d r2d.start() - - proc = subprocess.run(["docker", "manifest", "inspect", "--insecure", image_name]) + proc = subprocess.run( + ["docker", "manifest", "inspect", "--insecure", image_name] + ) assert proc.returncode == 0 finally: os.environ.clear() - os.environ.update(old_environ) \ No newline at end of file + os.environ.update(old_environ) diff --git a/tests/unit/test_argumentvalidation.py b/tests/unit/test_argumentvalidation.py index 466a7d7d..1439a653 100644 --- a/tests/unit/test_argumentvalidation.py +++ b/tests/unit/test_argumentvalidation.py @@ -212,7 +212,6 @@ def test_invalid_container_port_protocol_mapping_fail(temp_cwd): assert not validate_arguments(builddir, args_list, "Port specification") - def test_docker_no_build_success(temp_cwd): """ Test to check if r2d succeeds with --no-build argument with not being able to connect to docker daemon From 2fece29210207b5fda055b5e3d8f91efb9c4c947 Mon Sep 17 00:00:00 2001 From: YuviPanda Date: Fri, 28 Feb 2025 13:53:51 -0800 Subject: [PATCH 26/35] Add validation for registry_credentials --- repo2docker/engine.py | 11 ++++++++++- tests/unit/test_engine.py | 13 +++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) create mode 100644 tests/unit/test_engine.py diff --git a/repo2docker/engine.py b/repo2docker/engine.py index a6932bd8..1117ed9d 100644 --- a/repo2docker/engine.py +++ b/repo2docker/engine.py @@ -6,7 +6,7 @@ import os from abc import ABC, abstractmethod -from traitlets import Dict, default +from traitlets import Dict, TraitError, default, validate from traitlets.config import LoggingConfigurable # Based on https://docker-py.readthedocs.io/en/4.2.0/containers.html @@ -176,6 +176,15 @@ def _registry_credentials_default(self): raise return {} + @validate("registry_credentials") + def _registry_credentials_validate(self, proposal): + """ + Validate form of registry credentials + """ + new = proposal["value"] + if len({"registry", "username", "password"} & new.keys()) != 3: + raise TraitError("registry_credentials must have keys 'registry', 'username' and 'password'") + string_output = True """ Whether progress events should be strings or an object. diff --git a/tests/unit/test_engine.py b/tests/unit/test_engine.py new file mode 100644 index 00000000..74e9a747 --- /dev/null +++ b/tests/unit/test_engine.py @@ -0,0 +1,13 @@ +import pytest +from traitlets import TraitError +from repo2docker.engine import ContainerEngine + + +def test_registry_credentials(): + e = ContainerEngine(parent=None) + + # This should be fine + e.registry_credentials = {"registry": "something", "username": "something", "password": "something"} + + with pytest.raises(TraitError): + e.registry_credentials = {"hi": "bye"} From 0f680569566cfd1153c67232e06ec5797ed15919 Mon Sep 17 00:00:00 2001 From: YuviPanda Date: Fri, 28 Feb 2025 14:01:30 -0800 Subject: [PATCH 27/35] Intentionally set auth when checking if image exists in registry --- tests/norun/test_registry.py | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/tests/norun/test_registry.py b/tests/norun/test_registry.py index 98a1d36c..2c6ef344 100644 --- a/tests/norun/test_registry.py +++ b/tests/norun/test_registry.py @@ -165,10 +165,29 @@ def test_registry_explicit_creds(registry, dind): ) r2d.start() - proc = subprocess.run( - ["docker", "manifest", "inspect", "--insecure", image_name] - ) - assert proc.returncode == 0 + # CONTAINER_ENGINE_REGISTRY_CREDENTIALS unfortunately doesn't propagate to docker manifest, so + # let's explicitly set up a docker_config here so we can check if the image exists + with TemporaryDirectory() as d: + (Path(d) / "config.json").write_text( + json.dumps( + { + "auths": { + f"http://{registry_host}": { + "auth": b64encode( + f"{username}:{password}".encode() + ).decode() + } + } + } + ) + ) + env = os.environ.copy() + env["DOCKER_CONFIG"] = d + proc = subprocess.run( + ["docker", "manifest", "inspect", "--insecure", image_name], + env=env + ) + assert proc.returncode == 0 # Validate that we didn't leak our registry creds into existing docker config docker_config_path = Path( From e066ca04a151a2a4322ac4fc260ca34d8d47d864 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Fri, 28 Feb 2025 22:02:01 +0000 Subject: [PATCH 28/35] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- repo2docker/engine.py | 4 +++- tests/norun/test_registry.py | 3 +-- tests/unit/test_engine.py | 7 ++++++- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/repo2docker/engine.py b/repo2docker/engine.py index 1117ed9d..d081a33c 100644 --- a/repo2docker/engine.py +++ b/repo2docker/engine.py @@ -183,7 +183,9 @@ def _registry_credentials_validate(self, proposal): """ new = proposal["value"] if len({"registry", "username", "password"} & new.keys()) != 3: - raise TraitError("registry_credentials must have keys 'registry', 'username' and 'password'") + raise TraitError( + "registry_credentials must have keys 'registry', 'username' and 'password'" + ) string_output = True """ diff --git a/tests/norun/test_registry.py b/tests/norun/test_registry.py index 2c6ef344..2b332a8e 100644 --- a/tests/norun/test_registry.py +++ b/tests/norun/test_registry.py @@ -184,8 +184,7 @@ def test_registry_explicit_creds(registry, dind): env = os.environ.copy() env["DOCKER_CONFIG"] = d proc = subprocess.run( - ["docker", "manifest", "inspect", "--insecure", image_name], - env=env + ["docker", "manifest", "inspect", "--insecure", image_name], env=env ) assert proc.returncode == 0 diff --git a/tests/unit/test_engine.py b/tests/unit/test_engine.py index 74e9a747..1ccf1dc2 100644 --- a/tests/unit/test_engine.py +++ b/tests/unit/test_engine.py @@ -1,5 +1,6 @@ import pytest from traitlets import TraitError + from repo2docker.engine import ContainerEngine @@ -7,7 +8,11 @@ def test_registry_credentials(): e = ContainerEngine(parent=None) # This should be fine - e.registry_credentials = {"registry": "something", "username": "something", "password": "something"} + e.registry_credentials = { + "registry": "something", + "username": "something", + "password": "something", + } with pytest.raises(TraitError): e.registry_credentials = {"hi": "bye"} From 07e97ee5ceff625236f8ad5e0b4b2ca3e89be9cc Mon Sep 17 00:00:00 2001 From: YuviPanda Date: Fri, 28 Feb 2025 14:16:28 -0800 Subject: [PATCH 29/35] Turn push and load into params for the build arg --- repo2docker/app.py | 60 +++++-------------------------------------- repo2docker/docker.py | 45 ++++++++++++++++++-------------- repo2docker/engine.py | 6 +++++ 3 files changed, 39 insertions(+), 72 deletions(-) diff --git a/repo2docker/app.py b/repo2docker/app.py index 54482b7a..816fc0bf 100755 --- a/repo2docker/app.py +++ b/repo2docker/app.py @@ -572,55 +572,6 @@ def initialize(self, *args, **kwargs): if self.volumes and not self.run: raise ValueError("Cannot mount volumes if container is not run") - def push_image(self): - """Push docker image to registry""" - client = self.get_engine() - # Build a progress setup for each layer, and only emit per-layer - # info every 1.5s - progress_layers = {} - layers = {} - last_emit_time = time.time() - for chunk in client.push(self.output_image_spec): - if client.string_output: - self.log.info(chunk, extra=dict(phase=R2dState.PUSHING)) - continue - # else this is Docker output - - # each chunk can be one or more lines of json events - # split lines here in case multiple are delivered at once - for line in chunk.splitlines(): - line = line.decode("utf-8", errors="replace") - try: - progress = json.loads(line) - except Exception as e: - self.log.warning("Not a JSON progress line: %r", line) - continue - if "error" in progress: - self.log.error(progress["error"], extra=dict(phase=R2dState.FAILED)) - raise ImageLoadError(progress["error"]) - if "id" not in progress: - continue - # deprecated truncated-progress data - if "progressDetail" in progress and progress["progressDetail"]: - progress_layers[progress["id"]] = progress["progressDetail"] - else: - progress_layers[progress["id"]] = progress["status"] - # include full progress data for each layer in 'layers' data - layers[progress["id"]] = progress - if time.time() - last_emit_time > 1.5: - self.log.info( - "Pushing image\n", - extra=dict( - progress=progress_layers, - layers=layers, - phase=R2dState.PUSHING, - ), - ) - last_emit_time = time.time() - self.log.info( - f"Successfully pushed {self.output_image_spec}", - extra=dict(phase=R2dState.PUSHING), - ) def run_image(self): """Run docker container from built image @@ -847,6 +798,12 @@ def build(self): extra=dict(phase=R2dState.BUILDING), ) + extra_build_kwargs = self.extra_build_kwargs.copy() + # Set "push" and "load" parameters in a backwards compat way, without + # having to change the signature of every buildpack + extra_build_kwargs["push"] = self.push + extra_build_kwargs["load"] = self.run + for l in picked_buildpack.build( docker_client, self.output_image_spec, @@ -854,7 +811,7 @@ def build(self): self.build_memory_limit, build_args, self.cache_from, - self.extra_build_kwargs, + extra_build_kwargs, platform=self.platform, ): if docker_client.string_output: @@ -886,8 +843,5 @@ def build(self): def start(self): self.build() - if self.push: - self.push_image() - if self.run: self.run_image() diff --git a/repo2docker/docker.py b/repo2docker/docker.py index 1cb55fd0..42001448 100644 --- a/repo2docker/docker.py +++ b/repo2docker/docker.py @@ -2,13 +2,14 @@ Docker container engine for repo2docker """ +from argparse import ArgumentError import json import os import shutil import subprocess import tarfile import tempfile -from contextlib import contextmanager +from contextlib import ExitStack, contextmanager from pathlib import Path from iso8601 import parse_date @@ -89,6 +90,8 @@ class DockerEngine(ContainerEngine): def build( self, + push=False, + load=False, *, buildargs=None, cache_from=None, @@ -104,7 +107,15 @@ def build( ): if not shutil.which("docker"): raise RuntimeError("The docker commandline client must be installed") - args = ["docker", "buildx", "build", "--progress", "plain", "--load"] + args = ["docker", "buildx", "build", "--progress", "plain"] + if load: + if push: + raise ValueError("Setting push=True and load=True is currently not supported") + args.append("--load") + + if push: + args.append("--push") + if buildargs: for k, v in buildargs.items(): args += ["--build-arg", f"{k}={v}"] @@ -129,19 +140,22 @@ def build( # place extra args right *before* the path args += self.extra_buildx_build_args - if fileobj: - with tempfile.TemporaryDirectory() as d: - tarf = tarfile.open(fileobj=fileobj) - tarf.extractall(d) + with ExitStack() as stack: + if self.registry_credentials: + stack.enter_context(self.docker_login(**self.registry_credentials)) + if fileobj: + with tempfile.TemporaryDirectory() as d: + tarf = tarfile.open(fileobj=fileobj) + tarf.extractall(d) - args += [d] + args += [d] - yield from execute_cmd(args, True) - else: - # Assume 'path' is passed in - args += [path] + yield from execute_cmd(args, True) + else: + # Assume 'path' is passed in + args += [path] - yield from execute_cmd(args, True) + yield from execute_cmd(args, True) def inspect_image(self, image): """ @@ -194,13 +208,6 @@ def docker_login(self, username, password, registry): else: del os.environ["DOCKER_CONFIG"] - def push(self, image_spec): - if self.registry_credentials: - with self.docker_login(**self.registry_credentials): - yield from execute_cmd(["docker", "push", image_spec], capture=True) - else: - yield from execute_cmd(["docker", "push", image_spec], capture=True) - def run( self, image_spec, diff --git a/repo2docker/engine.py b/repo2docker/engine.py index d081a33c..28580ebd 100644 --- a/repo2docker/engine.py +++ b/repo2docker/engine.py @@ -212,6 +212,8 @@ def __init__(self, *, parent): def build( self, + push=False, + load=False, *, buildargs={}, cache_from=[], @@ -230,6 +232,10 @@ def build( Parameters ---------- + push: bool + Push the resulting image to a registry + load: bool + Load the resulting image into the container store ready to be run buildargs : dict Dictionary of build arguments cache_from : list[str] From 2bd84781595da2edcf0c10861851b7f60ee8c578 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Fri, 28 Feb 2025 22:17:09 +0000 Subject: [PATCH 30/35] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- repo2docker/app.py | 1 - repo2docker/docker.py | 6 ++++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/repo2docker/app.py b/repo2docker/app.py index 816fc0bf..b9684c11 100755 --- a/repo2docker/app.py +++ b/repo2docker/app.py @@ -572,7 +572,6 @@ def initialize(self, *args, **kwargs): if self.volumes and not self.run: raise ValueError("Cannot mount volumes if container is not run") - def run_image(self): """Run docker container from built image diff --git a/repo2docker/docker.py b/repo2docker/docker.py index 42001448..76af6d04 100644 --- a/repo2docker/docker.py +++ b/repo2docker/docker.py @@ -2,13 +2,13 @@ Docker container engine for repo2docker """ -from argparse import ArgumentError import json import os import shutil import subprocess import tarfile import tempfile +from argparse import ArgumentError from contextlib import ExitStack, contextmanager from pathlib import Path @@ -110,7 +110,9 @@ def build( args = ["docker", "buildx", "build", "--progress", "plain"] if load: if push: - raise ValueError("Setting push=True and load=True is currently not supported") + raise ValueError( + "Setting push=True and load=True is currently not supported" + ) args.append("--load") if push: From 081c3c48cfc5c457527fc440fc80bd969e568e66 Mon Sep 17 00:00:00 2001 From: YuviPanda Date: Fri, 28 Feb 2025 14:19:06 -0800 Subject: [PATCH 31/35] Make build and push into explicit only args --- repo2docker/docker.py | 2 +- repo2docker/engine.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/repo2docker/docker.py b/repo2docker/docker.py index 76af6d04..483e7770 100644 --- a/repo2docker/docker.py +++ b/repo2docker/docker.py @@ -90,9 +90,9 @@ class DockerEngine(ContainerEngine): def build( self, + *, push=False, load=False, - *, buildargs=None, cache_from=None, container_limits=None, diff --git a/repo2docker/engine.py b/repo2docker/engine.py index 28580ebd..303b38e2 100644 --- a/repo2docker/engine.py +++ b/repo2docker/engine.py @@ -212,9 +212,9 @@ def __init__(self, *, parent): def build( self, + *, push=False, load=False, - *, buildargs={}, cache_from=[], container_limits={}, From 56cf063636c589475f3f19e39df5a6b9619f3c0f Mon Sep 17 00:00:00 2001 From: YuviPanda Date: Fri, 28 Feb 2025 14:28:21 -0800 Subject: [PATCH 32/35] Don't pass --no-run to find tests --- tests/norun/test_find.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/norun/test_find.py b/tests/norun/test_find.py index 6d02631d..4b8d2bd1 100644 --- a/tests/norun/test_find.py +++ b/tests/norun/test_find.py @@ -8,16 +8,16 @@ def test_find_image(): image_name = f"{secrets.token_hex(8)}:latest" - r2d = make_r2d(["--image", image_name, "--no-run", str(HERE)]) + r2d = make_r2d(["--image", image_name, str(HERE)]) - r2d.start() + r2d.build() assert r2d.find_image() def test_dont_find_image(): image_name = f"{secrets.token_hex(8)}:latest" - r2d = make_r2d(["--image", image_name, "--no-run", str(HERE)]) + r2d = make_r2d(["--image", image_name, str(HERE)]) # Just don't actually start the build, so image won't be found assert not r2d.find_image() From ce35f47f1e74467c1c47083be1f4feaf7676a77d Mon Sep 17 00:00:00 2001 From: YuviPanda Date: Fri, 28 Feb 2025 14:31:20 -0800 Subject: [PATCH 33/35] Actually remove push method --- repo2docker/engine.py | 21 --------------------- 1 file changed, 21 deletions(-) diff --git a/repo2docker/engine.py b/repo2docker/engine.py index 303b38e2..310bddbb 100644 --- a/repo2docker/engine.py +++ b/repo2docker/engine.py @@ -288,27 +288,6 @@ def inspect_image(self, image): """ raise NotImplementedError("inspect_image not implemented") - def push(self, image_spec): - """ - Push image to a registry - - If the registry_credentials traitlets is set it should be used to - authenticate with the registry before pushing. - - Parameters - ---------- - image_spec : str - The repository spec to push to - - Returns - ------- - A generator of strings. If an error occurs an exception must be thrown. - - If `string_output=True` this should instead be whatever Docker returns: - https://github.com/jupyter/repo2docker/blob/0.11.0/repo2docker/app.py#L469-L495 - """ - raise NotImplementedError("push not implemented") - # Note this is different from the Docker client which has Client.containers.run def run( self, From d3facba5fbfa7392660edd5fc3e339113ac72e7f Mon Sep 17 00:00:00 2001 From: YuviPanda Date: Fri, 28 Feb 2025 14:32:05 -0800 Subject: [PATCH 34/35] Fix documentation for `--push` --- docs/source/architecture.md | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/docs/source/architecture.md b/docs/source/architecture.md index c145010c..2d14daf3 100644 --- a/docs/source/architecture.md +++ b/docs/source/architecture.md @@ -98,10 +98,8 @@ At the end of the assemble step, the docker image is ready to be used in various ### Push -Optionally, repo2docker can **push** a built image to a [docker registry](https://docs.docker.com/registry/). -This is done as a convenience only (since you can do the same with a `docker push` after using repo2docker -only to build), and implemented in `Repo2Docker.push` method. It is only activated if using the -`--push` commandline flag. +Optionally, repo2docker can **push** a built image to a [docker registry](https://docs.docker.com/registry/), +if you specify the `--push` flag. ### Run From 342eea96a8ed5f7778d34a07ce0a3896aa87cfd6 Mon Sep 17 00:00:00 2001 From: YuviPanda Date: Fri, 28 Feb 2025 15:07:43 -0800 Subject: [PATCH 35/35] Move login command inside try so we clear env correctly --- repo2docker/docker.py | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/repo2docker/docker.py b/repo2docker/docker.py index 483e7770..e6d3958c 100644 --- a/repo2docker/docker.py +++ b/repo2docker/docker.py @@ -190,19 +190,19 @@ def docker_login(self, username, password, registry): shutil.copy2(dc_path, new_dc_path) os.environ["DOCKER_CONFIG"] = d - proc = subprocess.run( - [ - "docker", - "login", - "--username", - username, - "--password-stdin", - registry, - ], - input=password.encode(), - check=True, - ) try: + subprocess.run( + [ + "docker", + "login", + "--username", + username, + "--password-stdin", + registry, + ], + input=password.encode(), + check=True, + ) yield finally: if old_dc_path: