Skip to content

Create Dockerfile #133

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 16 commits into from
Jun 11, 2025
Merged

Create Dockerfile #133

merged 16 commits into from
Jun 11, 2025

Conversation

y4ssi
Copy link
Contributor

@y4ssi y4ssi commented May 6, 2025

This PR adds a multi-stage Docker build for the zallet binary, resulting in a minimal and secure container image.

Build Stage (builder)

  • Based on rust:1-slim (amd64).
  • Installs only the necessary packages for building (clang, libclang-dev, pkg-config, git).
  • Compiles the project in --release mode.
  • Uses strip to reduce binary size.

Runtime Stage (distroless)

  • Uses gcr.io/distroless/cc for a minimal and secure runtime environment.
  • Copies only the final zallet binary.
  • Runs as a non-root user by default.
  • No shell and no package manager, which minimizes the attack surface.

Additional Changes

  • Adds a GitHub Action to build the image and verify that the binary runs with at least one argument (more tests can be added in the future).
  • Adds a GitHub Action to release the Docker image to Docker Hub.

This setup ensures small image size, strong security practices, and a clear separation between build and runtime environments.

Copy link
Collaborator

@str4d str4d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed b44296a. In addition to my comments, the zizmor failures need to be fixed.

echo "zallet_version=$(echo ${{ github.ref_name }} | sed 's/v//g')" >> $GITHUB_OUTPUT

build_push:
uses: zcash/.github/.github/workflows/build-and-push-docker-hub.yaml@main
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this doing? It looks recursive to me which makes no sense. Is this actually pulling from the zcash/zcash repo?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is the template we're using in Zcash and across several of our other repositories. - Just realized I missed updating a path — just fixed it now.

@y4ssi y4ssi requested a review from str4d May 13, 2025 18:41
@str4d str4d added the A-packaging Area: Packaging label May 14, 2025
Copy link
Contributor

@gustavovalverde gustavovalverde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some comments for better UX and reproducibility.

Dockerfile Outdated

COPY . .

RUN cargo build --release && strip target/release/zallet
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using strip makes debugging harder. If the stripped zallet binary crashes inside the distroless container, the stack traces will be much less informative because the symbol information is not available. This is a good example: https://stackoverflow.com/a/73628573

Adding the --locked flag is recommended for reproducibility, as it forces Cargo to use only the versions of dependencies specified in the Cargo.lock file. This is also aligned with the use of cargo vet --locked in the /.github/workflows/audits.yml workflow

Optional: Adding --package zallet --bin zallet for visibility and explicitness. Also, if in the future another package is added to the [workspace.members] cargo build without --package might try to build all members or a different default. Similar with --bin

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@str4d any thoughts on this?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the zcashd release process we strip the binaries and save the symbols in a separate file so we have them for reference later. I'm not particlarly fussed either way here, and am fine with the current approach of not stripping (if binary sizes become a problem where stripping would help, it can be considered later).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commit introduces some optimizations to the Docker build process:

1.  **Build Caching**:
    *   Leverages Docker BuildKit cache mounts for `cargo` dependencies (`/app/.cargo`) and compiled artifacts (`/app/target/`). This speeds up subsequent builds by reusing downloaded and compiled dependencies.
    *   Uses bind mounts for `zallet` source, `Cargo.toml`, and `Cargo.lock` to ensure the build step has access to the necessary files during the cached `RUN` instruction.
    *   The `--link` flag is now used with `COPY` for potentially faster copies when supported.

2.  **Optimized Build Command**:
    *   The `cargo build` command is now more explicit, specifying `--locked --package zallet --bin zallet`.
    *   The `strip` command has been removed to allow runtime debugging information.

3.  **Explicit Naming and Structure**:
    *   The runtime stage is now explicitly named `AS runtime`.
    *   The `ENTRYPOINT` now uses the simple binary name `zallet`, relying on it being in the `PATH`.

4.  **`.dockerignore` File**:
    *   A `.dockerignore` file has been added to control the build context sent to the Docker daemon.
    *   It follows an "exclude all, then include specific" pattern (`*` followed by `!pattern`) to ensure only necessary files (`.cargo`, `*.toml`, `*.lock`, and the `zallet` source directory) are included in the build context. This reduces the context size and can prevent unnecessary cache invalidations.
@gustavovalverde
Copy link
Contributor

I opened #150 targeting this PR, with minimal design changes, but adding some of the optimizations mentioned in the comments

Copy link
Collaborator

@str4d str4d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 0347f86. We can iterate on this in-repo as necessary in the lead-up to the first alpha release.

set -Eeuo pipefail
IFS=$'\n\t'

if [[ "$VERSION" =~ ^v[0-9]+\.[0-9]+\.[0-9]+(-rc-[0-9]+)?$ ]]; then
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will need to be updated to support whatever pre-release scheme we end up using for the early Zallet alpha/beta/whatever, but it is sufficient for now.

@str4d str4d merged commit c5f09c9 into main Jun 11, 2025
17 of 20 checks passed
@str4d str4d deleted the dockerfile branch June 11, 2025 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-packaging Area: Packaging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants