Merge pull request #1421 from yuvipanda/docker-login

Switch to using CLI for everything except running the container

Author: yuvipanda

.github/workflows/test.yml

@@ -40,6 +40,7 @@ env:
   GIT_AUTHOR_EMAIL: [email protected]
   GIT_AUTHOR_NAME: CI User
 
+
 jobs:
   test:
     # Don't run scheduled tests on forks
@@ -64,6 +65,7 @@ jobs:
           - unit
           - venv
           - contentproviders
+          - norun
           # Playwright test
           - ui
         include:
@@ -74,6 +76,7 @@ jobs:
 
     steps:
       - uses: actions/checkout@v4
+
       - uses: actions/setup-python@v5
         with:
           python-version: "${{ matrix.python_version }}"

dev-requirements.txt

@@ -5,3 +5,4 @@ pytest-cov
 pytest>=7
 pyyaml
 requests_mock
+bcrypt

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
 

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_free_port, get_platform
 
 
 class Repo2Docker(Application):
@@ -572,56 +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
 
@@ -660,7 +610,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,30 +694,14 @@ 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
         # always return False for dry runs.
         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):
         """
@@ -863,14 +797,20 @@ 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,
                         # This is deprecated, but passing it anyway to not break backwards compatibility
                         self.build_memory_limit,
                         build_args,
                         self.cache_from,
-                        self.extra_build_kwargs,
+                        extra_build_kwargs,
                         platform=self.platform,
                     ):
                         if docker_client.string_output:
@@ -902,8 +842,5 @@ def build(self):
     def start(self):
         self.build()
 
-        if self.push:
-            self.push_image()
-
         if self.run:
             self.run_image()

repo2docker/buildpacks/docker.py

@@ -3,8 +3,6 @@
 
 import os
 
-import docker
-
 from .base import BuildPack
 
 

repo2docker/docker.py

@@ -2,16 +2,22 @@
 Docker container engine for repo2docker
 """
 
+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
 
 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
 
 
@@ -58,7 +64,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(
         {},
@@ -82,19 +88,11 @@ 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,
         *,
+        push=False,
+        load=False,
         buildargs=None,
         cache_from=None,
         container_limits=None,
@@ -109,7 +107,17 @@ 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}"]
@@ -134,38 +142,73 @@ 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]
-
-                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}
-        else:
-            # Assume 'path' is passed in
-            args += [path]
+                    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]
 
-    def images(self):
-        images = self._apiclient.images()
-        return [Image(tags=image["RepoTags"]) for image in images]
+                yield from execute_cmd(args, True)
 
     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
 
-    def push(self, image_spec):
-        if self.registry_credentials:
-            self._apiclient.login(**self.registry_credentials)
-        return self._apiclient.push(image_spec, stream=True)
+        config = json.loads(proc.stdout.decode())[0]
+        return Image(tags=config["RepoTags"], config=config["Config"])
+
+    @contextmanager
+    def docker_login(self, username, password, registry):
+        # Determine existing DOCKER_CONFIG
+        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"
+            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)
+
+            os.environ["DOCKER_CONFIG"] = d
+            try:
+                subprocess.run(
+                    [
+                        "docker",
+                        "login",
+                        "--username",
+                        username,
+                        "--password-stdin",
+                        registry,
+                    ],
+                    input=password.encode(),
+                    check=True,
+                )
+                yield
+            finally:
+                if old_dc_path:
+                    os.environ["DOCKER_CONFIG"] = old_dc_path
+                else:
+                    del os.environ["DOCKER_CONFIG"]
 
     def run(
         self,