Docker implementation #1219
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,166 @@ | ||||||||
| """Logic for creating a Dockerfile and/or building portable Docker images from Constructor installers.""" | ||||||||
|
|
||||||||
| import logging | ||||||||
| import shutil | ||||||||
| import subprocess | ||||||||
| import tempfile | ||||||||
| from pathlib import Path | ||||||||
|
|
||||||||
| from jinja2 import Template | ||||||||
|
|
||||||||
| from . import __version__ | ||||||||
|
|
||||||||
| logger = logging.getLogger(__name__) | ||||||||
|
|
||||||||
| TEMPLATE_PATH = Path(__file__).parent / "dockerfile_template.tmpl" | ||||||||
|
|
||||||||
| DOCKER_PLATFORM_MAP = { | ||||||||
| "linux-64": "linux/amd64", | ||||||||
| "linux-aarch64": "linux/arm64", | ||||||||
| "linux-armv7l": "linux/arm/v7", | ||||||||
| "linux-32": "linux/386", | ||||||||
| "linux-ppc64le": "linux/ppc64le", | ||||||||
| "linux-s390x": "linux/s390x", | ||||||||
| } | ||||||||
|
|
||||||||
|
|
||||||||
| def generate_dockerfile(info: dict, docker_dir: Path) -> Path: | ||||||||
| """ | ||||||||
| Render the Dockerfile template and write it to the Docker build directory. | ||||||||
|
|
||||||||
| Parameters | ||||||||
| ---------- | ||||||||
| info: dict | ||||||||
| Constructor installer info dict. | ||||||||
| docker_dir: Path | ||||||||
| Path to the Docker build directory returned by prepare_docker_context(). | ||||||||
|
|
||||||||
| Returns | ||||||||
| ------- | ||||||||
| Path | ||||||||
| Path to the generated Dockerfile. | ||||||||
| """ | ||||||||
| from .conda_interface import MatchSpec | ||||||||
|
|
||||||||
| specs = {MatchSpec(spec).name for spec in info.get("specs", ())} | ||||||||
|
Contributor
There was a problem hiding this comment.
Suggested change
That will make things more clear later. Should probably also be done with |
||||||||
| has_mamba = "mamba" in specs | ||||||||
|
|
||||||||
| docker_template = Template(TEMPLATE_PATH.read_text()) | ||||||||
|
|
||||||||
| rendered_dockerfile = docker_template.render( | ||||||||
| constructor_version=__version__, | ||||||||
| base_image=info.get("docker_base_image"), | ||||||||
| default_prefix=info.get("default_prefix", f"/opt/{info['name'].lower()}"), | ||||||||
|
Jrice1317 marked this conversation as resolved.
|
||||||||
| installer_filename=Path(info["_outpath"]).name, | ||||||||
| name=info["name"], | ||||||||
| version=info["version"], | ||||||||
| labels=info.get("docker_labels", {}), | ||||||||
| init_cmd="$PREFIX/bin/mamba shell" if has_mamba else "$PREFIX/bin/python -m conda", | ||||||||
| register_envs=info.get("register_envs"), | ||||||||
| keep_pkgs=info.get("keep_pkgs"), | ||||||||
| ) | ||||||||
|
|
||||||||
| logger.info("Writing Dockerfile...") | ||||||||
| dockerfile_path = docker_dir / "Dockerfile" | ||||||||
| dockerfile_path.write_text(rendered_dockerfile) | ||||||||
| return dockerfile_path | ||||||||
|
|
||||||||
|
|
||||||||
| def build_image(info: dict, docker_dir: Path) -> Path: | ||||||||
| """Optionally build the docker image from the generated Dockerfile. | ||||||||
| Currently supported on linux and macOS platforms. | ||||||||
|
|
||||||||
| Parameters | ||||||||
| ---------- | ||||||||
| info: dict | ||||||||
| Constructor installer info dict. | ||||||||
| docker_dir: Path | ||||||||
| Path to the Docker directory containing the Docker outputs. | ||||||||
|
|
||||||||
| Returns | ||||||||
| ------- | ||||||||
| Path | ||||||||
| Path to the saved Docker image tarball. | ||||||||
|
|
||||||||
| """ | ||||||||
| if not (docker_platform := DOCKER_PLATFORM_MAP.get(info["_platform"])): | ||||||||
| raise RuntimeError( | ||||||||
| f"Unsupported platform for Docker build: {info['_platform']}. " | ||||||||
| f"Supported platforms are: {', '.join(DOCKER_PLATFORM_MAP.keys())}." | ||||||||
| ) | ||||||||
|
|
||||||||
| tag = info.get("docker_tag", f"{info['name'].lower()}:{info['version']}") | ||||||||
| tarball_dest = docker_dir / f"{Path(info['_outpath']).stem}-docker.tar" | ||||||||
|
|
||||||||
| cmd = [ | ||||||||
| "docker", | ||||||||
| "buildx", | ||||||||
| "build", | ||||||||
| str(docker_dir), | ||||||||
| "--platform", | ||||||||
| docker_platform, | ||||||||
| "-t", | ||||||||
| tag, | ||||||||
|
Comment on lines
+102
to
+103
Contributor
There was a problem hiding this comment. Here is where I'm wondering if we need the tag - can we also retrieve the Docker image based on the emitted SHA?
Contributor
Author
There was a problem hiding this comment. I'm sorry I'm not understanding the ask here. If we do not provide a tag, the image would show up like
Contributor
Author
There was a problem hiding this comment. Are you thinking we use the SHA to pass into docker save instead of the tag?
Contributor
There was a problem hiding this comment. The tag right now is mandatory
Contributor
Author
There was a problem hiding this comment. No, it is not required to work, but we most definitely should use a tag. When someone tries to run the image, one of the easiest ways to do that is to use the tag. If we don't provide a tag, there would be a
Contributor
Author
There was a problem hiding this comment. So, if you build an image with no tag and you look at your list of images using the Docker tags are not the same as GitHub tags. It is the name of the image and we should definitely have one. If no tag is provided, we would require using
Contributor
Author
There was a problem hiding this comment. To clarify intent: the local image is intentional — it's available for immediate use and testing after the build, while the tarball is for portability and sharing. I'd rather not cleanup by default since that would silently remove something the user may want, but I'm happy to add it if that's the preference. Either way, I just realized I need to update the docstring to clarify the artifacts and mention that only
Contributor
There was a problem hiding this comment. Here is what I see using version 29.4.3 on macOS: So, the output seems to be different for different versions and maybe platforms.
It's a temporary file, so that's not an issue unless there are reliability concerns.
I'm still not sure what the security concern is here. What is the exploit a tag prevents that an image ID or a SHA is vulnerable to? Tags are generally more vulnerable than a hash because tags are mutable (including Docker tags) whereas hashes are not - which is why the general recommendation is to add the hash to the base image.
So, if you want to impose that restriction, that's a design choice. I do not see the security angle here though, but it appears to be based on a specific (even if common) workflow. But then we need to be defensive and error out if a user writes
If a user wants the image immediately afterwards, they can use
Contributor
Author
There was a problem hiding this comment. Allowing untagged images adds complexity with no real benefit in this context. It would require To your point about I also want to clarify my use of "tag" throughout this thread. When I say tag, I mean the full I've removed the restriction requiring And on cleanup, I've added
Contributor
There was a problem hiding this comment. For reference also all of my images are tagged: Note here that |
||||||||
| "--load", | ||||||||
| ] | ||||||||
|
|
||||||||
| logger.info("Building Docker image: '%s'", tag) | ||||||||
| try: | ||||||||
| subprocess.run(cmd, check=True) | ||||||||
| except subprocess.CalledProcessError as e: | ||||||||
| # Gather diagnostics on failure | ||||||||
| docker_version = subprocess.run(["docker", "--version"], capture_output=True, text=True) | ||||||||
| buildx_version = subprocess.run( | ||||||||
| ["docker", "buildx", "version"], capture_output=True, text=True | ||||||||
| ) | ||||||||
| buildx_ls = subprocess.run(["docker", "buildx", "ls"], capture_output=True, text=True) | ||||||||
| raise RuntimeError( | ||||||||
| f"Docker build failed.\n" | ||||||||
| f"Command: {cmd}\n" | ||||||||
| f"Docker version: {docker_version.stdout.strip()}\n" | ||||||||
| f"Buildx version: {buildx_version.stdout.strip() or buildx_version.stderr.strip()}\n" | ||||||||
| f"Buildx builders: {buildx_ls.stdout.strip()}" | ||||||||
| ) from e | ||||||||
|
|
||||||||
| logger.info("Saving Docker image to tarball: '%s'", tarball_dest) | ||||||||
| subprocess.run(["docker", "save", tag, "-o", str(tarball_dest)], check=True) | ||||||||
| subprocess.run(["docker", "rmi", tag], check=False) | ||||||||
| return tarball_dest | ||||||||
|
|
||||||||
|
|
||||||||
| def create(info: dict, verbose: bool = False) -> None: | ||||||||
| """Build a Docker output | ||||||||
|
|
||||||||
| Parameters | ||||||||
| ---------- | ||||||||
| info: dict | ||||||||
| Constructor installer info dict. | ||||||||
| verbose: bool, optional | ||||||||
| If ``True``, enables verbose logging. | ||||||||
| Defaults to ``False``. | ||||||||
|
|
||||||||
| """ | ||||||||
| with tempfile.TemporaryDirectory() as temp_dir: | ||||||||
| docker_tmp_dir = Path(temp_dir) | ||||||||
|
|
||||||||
| installer_path = Path(info["_outpath"]) | ||||||||
| if not installer_path.exists(): | ||||||||
| raise RuntimeError(f"Expected .sh installer not found: {installer_path}") | ||||||||
| shutil.copy(installer_path, docker_tmp_dir / installer_path.name) | ||||||||
| logger.info("Copied installer to build directory.") | ||||||||
|
|
||||||||
| generate_dockerfile(info, docker_tmp_dir) | ||||||||
|
|
||||||||
| if info.get("docker_image") == "tar": | ||||||||
| tarball = build_image(info, docker_tmp_dir) | ||||||||
| shutil.copy(tarball, Path(info["_output_dir"]) / tarball.name) | ||||||||
| else: | ||||||||
| output_dir = Path(info["_output_dir"]) / installer_path.stem | ||||||||
| output_dir.mkdir(parents=True, exist_ok=True) | ||||||||
| shutil.copy(docker_tmp_dir / "Dockerfile", output_dir / "Dockerfile") | ||||||||
| shutil.copy( | ||||||||
| docker_tmp_dir / Path(info["_outpath"]).name, | ||||||||
| output_dir / Path(info["_outpath"]).name, | ||||||||
| ) | ||||||||
|
|
||||||||
| logger.info("Docker output complete. Docker directory: '%s'", info["_output_dir"]) | ||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of prefixing all fields with
docker_*, would it make more sense to makedockerits own key with all other properties being a subkey? I know that's not our current convention, so I'm not 100% sure about that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it could be cleaner, but unless we can do a broader refactor, I think docker_* keeps things consistent with how it's handled today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, please volunteer me if/when we can do a schema refactor. (:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sticking with the current schema is a fair choice. A schema refactor would constitute a major breaking change that I don't foresee ourselves doing unless we absolutely have to.