Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,9 @@
FROM docker.io/rust:1.90.0-slim-bookworm AS cacher
ARG SCCACHE_BUCKET
ARG SCCACHE_REGION
ARG AWS_ACCESS_KEY_ID

Check warning on line 14 in Dockerfile

View workflow job for this annotation

GitHub Actions / Build arm64

Sensitive data should not be used in the ARG or ENV commands

SecretsUsedInArgOrEnv: Do not use ARG or ENV instructions for sensitive data (ARG "AWS_ACCESS_KEY_ID") More info: https://docs.docker.com/go/dockerfile/rule/secrets-used-in-arg-or-env/

Check warning on line 14 in Dockerfile

View workflow job for this annotation

GitHub Actions / Build amd64

Sensitive data should not be used in the ARG or ENV commands

SecretsUsedInArgOrEnv: Do not use ARG or ENV instructions for sensitive data (ARG "AWS_ACCESS_KEY_ID") More info: https://docs.docker.com/go/dockerfile/rule/secrets-used-in-arg-or-env/
ARG AWS_SECRET_ACCESS_KEY

Check warning on line 15 in Dockerfile

View workflow job for this annotation

GitHub Actions / Build arm64

Sensitive data should not be used in the ARG or ENV commands

SecretsUsedInArgOrEnv: Do not use ARG or ENV instructions for sensitive data (ARG "AWS_SECRET_ACCESS_KEY") More info: https://docs.docker.com/go/dockerfile/rule/secrets-used-in-arg-or-env/

Check warning on line 15 in Dockerfile

View workflow job for this annotation

GitHub Actions / Build amd64

Sensitive data should not be used in the ARG or ENV commands

SecretsUsedInArgOrEnv: Do not use ARG or ENV instructions for sensitive data (ARG "AWS_SECRET_ACCESS_KEY") More info: https://docs.docker.com/go/dockerfile/rule/secrets-used-in-arg-or-env/
ARG AWS_SESSION_TOKEN

Check warning on line 16 in Dockerfile

View workflow job for this annotation

GitHub Actions / Build arm64

Sensitive data should not be used in the ARG or ENV commands

SecretsUsedInArgOrEnv: Do not use ARG or ENV instructions for sensitive data (ARG "AWS_SESSION_TOKEN") More info: https://docs.docker.com/go/dockerfile/rule/secrets-used-in-arg-or-env/

Check warning on line 16 in Dockerfile

View workflow job for this annotation

GitHub Actions / Build amd64

Sensitive data should not be used in the ARG or ENV commands

SecretsUsedInArgOrEnv: Do not use ARG or ENV instructions for sensitive data (ARG "AWS_SESSION_TOKEN") More info: https://docs.docker.com/go/dockerfile/rule/secrets-used-in-arg-or-env/
ENV CARGO_INCREMENTAL=0
WORKDIR /app
RUN apt-get update && apt-get install -y \
Expand Down Expand Up @@ -43,7 +43,7 @@
export AWS_ACCESS_KEY_ID=$(cat /run/secrets/aws_access_key_id) && \
export AWS_SECRET_ACCESS_KEY=$(cat /run/secrets/aws_secret_access_key) && \
export AWS_SESSION_TOKEN=$(cat /run/secrets/aws_session_token) && \
export RUSTC_WRAPPER=sccache && \
if [ -n "${SCCACHE_BUCKET:-}" ]; then export RUSTC_WRAPPER=sccache; fi && \
cargo chef cook --release --locked --features logrotate_fs --recipe-path recipe.json

# Stage 2: Builder - Build source code
Expand Down Expand Up @@ -74,7 +74,7 @@
export AWS_ACCESS_KEY_ID=$(cat /run/secrets/aws_access_key_id) && \
export AWS_SECRET_ACCESS_KEY=$(cat /run/secrets/aws_secret_access_key) && \
export AWS_SESSION_TOKEN=$(cat /run/secrets/aws_session_token) && \
export RUSTC_WRAPPER=sccache && \
if [ -n "${SCCACHE_BUCKET:-}" ]; then export RUSTC_WRAPPER=sccache; fi && \
cargo build --release --locked --bin lading --features logrotate_fs

# Stage 3: Runtime
Expand Down
203 changes: 203 additions & 0 deletions ci/container-build-and-push
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This skips over the use of sccache (wanted to push off integrating with it as it adds some complexity) but even without it, this builds both architectures in ~8 min on my local laptop.

Good enough for v1. As long as we're <10 min for a step, the LLM loops handle that gracefully enough.

Original file line number Diff line number Diff line change
@@ -0,0 +1,203 @@
#!/usr/bin/env bash
set -o errexit
set -o nounset
set -o pipefail

ECR_REGISTRY="850406765696.dkr.ecr.us-west-2.amazonaws.com"
ECR_REPO="${ECR_REGISTRY}/lading-dev"
AWS_VAULT_PROFILE="smp"

usage() {
cat <<EOF
Usage: ci/container-build-and-push [OPTIONS]

Build and push lading Docker images to ECR.

Options:
--tag TAG Image tag (default: git commit SHA)
--arch ARCH Architecture(s): amd64, arm64, or amd64,arm64 (default: amd64,arm64)
--profile PROFILE aws-vault profile (default: smp)
--random-tag Use a random UUID as the image tag
--no-push Build only, don't push to ECR
-h, --help Show this help

Examples:
ci/container-build-and-push # build both amd64+arm64, tag from git SHA
ci/container-build-and-push --tag v0.32.0 # explicit tag
ci/container-build-and-push --arch amd64,arm64 # multi-arch build
ci/container-build-and-push --no-push # build only
EOF
}

# Parse arguments
TAG=""
RANDOM_TAG=false
ARCH=""
PUSH=true

while [[ $# -gt 0 ]]; do
case "$1" in
--tag) TAG="$2"; shift 2 ;;
--arch) ARCH="$2"; shift 2 ;;
--profile) AWS_VAULT_PROFILE="$2"; shift 2 ;;
Comment on lines +40 to +42

Choose a reason for hiding this comment

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

P2 Badge Validate option values before shifting past the next flag

--tag, --arch, and --profile unconditionally read $2, so a missing value silently consumes the following option instead of erroring. In ci/container-build-and-push --profile --no-push, for example, --no-push becomes the profile string and PUSH stays true, so the script can attempt an ECR push even though the caller explicitly asked for a local-only build. Rejecting missing values (or a next token that starts with -) avoids this misparse.

Useful? React with 👍 / 👎.

--random-tag) RANDOM_TAG=true; shift ;;
--no-push) PUSH=false; shift ;;
-h|--help) usage; exit 0 ;;
*) echo "Error: Unknown option: $1" >&2; usage >&2; exit 1 ;;
esac
done

if [[ -n "$TAG" && "$RANDOM_TAG" == "true" ]]; then
echo "Error: --tag and --random-tag are mutually exclusive" >&2
exit 1
fi

if [[ "$RANDOM_TAG" == "true" ]]; then
TAG="$(uuidgen | tr '[:upper:]' '[:lower:]')"

Choose a reason for hiding this comment

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

P2 Badge Avoid relying on uuidgen for --random-tag

In the repo's current workspace container, uuidgen is not installed, and this branch calls it unconditionally when --random-tag is passed. That makes the advertised flag exit with command not found before any Docker logic runs, so the script's local/agentic workflow cannot use random tags unless extra OS packages happen to be present. Please either gate this behind a prerequisite check or generate the tag with a tool that is already required here.

Useful? React with 👍 / 👎.

echo "Generated random tag: ${TAG}"
fi

# Check prerequisites
REQUIRED_CMDS=(docker)
if [[ "$PUSH" == "true" ]]; then
REQUIRED_CMDS+=(aws aws-vault jq)
fi
for cmd in "${REQUIRED_CMDS[@]}"; do
if ! command -v "$cmd" &>/dev/null; then
echo "Error: $cmd is not installed"
exit 1
fi
done

# Ensure Docker daemon is running
if ! docker info &>/dev/null; then
echo "Error: Docker daemon is not running. Start Docker Desktop and try again."
exit 1
fi

# Ensure buildx is available
if ! docker buildx version &>/dev/null; then
echo "Error: Docker Buildx is not available"
exit 1
fi

# Check Docker has enough memory for release builds with LTO
MIN_MEMORY_GB=16
DOCKER_MEM_BYTES=$(docker info --format '{{.MemTotal}}' 2>/dev/null)
DOCKER_MEM_GB=$(( DOCKER_MEM_BYTES / 1073741824 ))
if [[ "$DOCKER_MEM_GB" -lt "$MIN_MEMORY_GB" ]]; then
echo "Error: Docker has ${DOCKER_MEM_GB} GB of memory, but at least ${MIN_MEMORY_GB} GB is required"
echo "Increase memory in Docker Desktop -> Settings -> Resources"
exit 1
fi
echo "Docker memory: ${DOCKER_MEM_GB} GB"

# Default to both architectures if not specified
if [[ -z "$ARCH" ]]; then
ARCH="amd64,arm64"
echo "Defaulting to both architectures: ${ARCH}"
fi

# Determine tag
if [[ -z "$TAG" ]]; then
TAG="sha-$(git rev-parse HEAD)"
echo "Using git SHA tag: ${TAG}"
Comment on lines +102 to +104

Choose a reason for hiding this comment

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

P2 Badge Refuse dirty checkouts when deriving a sha tag

If the checkout has uncommitted changes, this still tags the image as sha-<HEAD>. In the default push flow that makes a dirty local build indistinguishable from an image built from the clean commit, and it can overwrite the tag other people expect to mean "exactly commit X". That breaks traceability/reproducibility whenever someone publishes local edits without also remembering to pass an explicit --tag.

Useful? React with 👍 / 👎.

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks worth addressing. The troublesome scenario I imagine here is two people try different changes on main at the same commit. What do you think about adding a unique value to this slug?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah less of a worry with immutable tags. It'll fail loudly if it happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly :)

imo, tempted to leave as is until we hit too much pain that it warrants a change.

Because I kind of like having a commit sha as the ultimate source of truth

fi

# Parse architectures
IFS=',' read -ra ARCHS <<< "$ARCH"
MULTI_ARCH=false
if [[ ${#ARCHS[@]} -gt 1 ]]; then
MULTI_ARCH=true
fi

# Build platform string
PLATFORMS=""
for a in "${ARCHS[@]}"; do
if [[ -n "$PLATFORMS" ]]; then
PLATFORMS="${PLATFORMS},"
fi
PLATFORMS="${PLATFORMS}linux/${a}"
done

# Set up QEMU for cross-arch builds
if [[ "$MULTI_ARCH" == "true" ]]; then
echo "Setting up QEMU for multi-arch builds..."
docker run --rm --privileged tonistiigi/binfmt --install all >/dev/null 2>&1 || true
Comment on lines +124 to +126

Choose a reason for hiding this comment

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

P2 Badge Install QEMU for any non-native target architecture

This only installs tonistiigi/binfmt when more than one platform is requested, but a single-platform foreign-arch build (--arch arm64 on x86, or --arch amd64 on Apple Silicon) still needs QEMU or a native builder. The Dockerfile runs target-arch apt-get/cargo commands instead of cross-compiling via BUILDPLATFORM/TARGETARCH, so the advertised single-arch cross-build mode fails on clean Linux hosts unless they were preconfigured out-of-band.

Useful? React with 👍 / 👎.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤷 not sure if this is really a concern

Choose a reason for hiding this comment

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

P2 Badge Don't swallow failures from binfmt setup

On hosts that actually depend on this emulation step for foreign-arch builds (for example the default amd64,arm64 path on a single-arch machine), docker run --privileged ... --install all can fail because privileged containers are disallowed or the helper image cannot be pulled. The trailing || true hides that prerequisite failure, so the script continues until docker buildx build dies later with a much less actionable platform/exec-format error instead of failing fast at the real cause.

Useful? React with 👍 / 👎.

fi

# Authenticate to ECR using a temporary docker config to avoid macOS credsStore issues.
# We copy the real docker config (minus credsStore) and symlink cli-plugins so that
# docker login can write credentials as plain JSON and buildx still works.
DOCKER_CONFIG_DIR=""
cleanup() {
if [[ -n "$DOCKER_CONFIG_DIR" ]]; then
rm -rf "$DOCKER_CONFIG_DIR"
fi
}
trap cleanup EXIT

if [[ "$PUSH" == "true" ]]; then
echo "Authenticating to ECR..."
REAL_DOCKER_CONFIG="${DOCKER_CONFIG:-${HOME}/.docker}"
DOCKER_CONFIG_DIR="$(mktemp -d)"
# Copy config without credsStore/credHelpers so docker login writes auth as plain JSON
jq 'del(.credsStore, .credHelpers)' "${REAL_DOCKER_CONFIG}/config.json" \
> "${DOCKER_CONFIG_DIR}/config.json"
Comment on lines +145 to +146

Choose a reason for hiding this comment

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

P2 Badge Fallback to empty Docker config when config.json is missing

During push, the script unconditionally runs jq ... "${REAL_DOCKER_CONFIG}/config.json"; on fresh Docker setups where ~/.docker/config.json has not been created yet, jq fails and set -o errexit aborts before docker login can create credentials, so pushes fail for first-time users. Adding a missing-file fallback (for example initializing with {}) avoids this hard failure.

Useful? React with 👍 / 👎.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

imo an edge case that I'm willing to forego for now. I'd prefer for this to fail in those cases and to debug it then.

Since I've been running this on my laptop (and that's what I expect outside of workspaces), I expect docker to be configured/setup already.

This build container script hasn't been tested in workspaces yet.

Comment on lines +145 to +146

Choose a reason for hiding this comment

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

P2 Badge Preserve registry helpers in the temporary Docker config

jq 'del(.credsStore, .credHelpers)' removes every non-ECR credential source from the temp config that line 143 later exports to docker buildx build. Docker's docker login docs say credsStore/credHelpers are how the CLI retrieves saved registry credentials, so the push path now pulls the Dockerfile's docker.io/rust and docker.io/debian bases anonymously. On hosts that rely on Docker Desktop's keychain-backed Docker Hub login or a helper for a private mirror, builds regress to 429/no basic auth credentials even though the user's normal Docker config works.

Useful? React with 👍 / 👎.

# Symlink subdirectories so buildx CLI plugin and Docker contexts are found
for subdir in cli-plugins contexts; do
if [[ -d "${REAL_DOCKER_CONFIG}/${subdir}" ]]; then
ln -s "${REAL_DOCKER_CONFIG}/${subdir}" "${DOCKER_CONFIG_DIR}/${subdir}"
fi
Comment on lines +148 to +151

Choose a reason for hiding this comment

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

P2 Badge Carry the caller's Buildx config into push mode

Push mode swaps in a fresh DOCKER_CONFIG, but this loop only preserves cli-plugins and contexts. Docker's Buildx variables docs state that builder state and buildkitd.default.toml live under $DOCKER_CONFIG/buildx, so any existing builder settings needed for custom certs, registry mirrors, or network access disappear only on --push. The later inspect/create/use logic then runs against an empty Buildx config and silently falls back to a plain docker-container builder instead of the one the user already configured.

Useful? React with 👍 / 👎.

done
aws-vault exec "$AWS_VAULT_PROFILE" -- \
aws ecr get-login-password --region us-west-2 \
| DOCKER_CONFIG="$DOCKER_CONFIG_DIR" docker login --username AWS --password-stdin "$ECR_REGISTRY"
export DOCKER_CONFIG="$DOCKER_CONFIG_DIR"
fi

# Create/use a buildx builder that supports multi-platform
BUILDER_NAME="lading-builder"
if ! docker buildx inspect "$BUILDER_NAME" &>/dev/null; then
echo "Creating buildx builder: ${BUILDER_NAME}"
docker buildx create --name "$BUILDER_NAME" --driver docker-container --use
else
docker buildx use "$BUILDER_NAME"
Comment on lines +161 to +165

Choose a reason for hiding this comment

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

P2 Badge Stop changing the caller's selected buildx builder

Docker's buildx docs say docker buildx use "switches the current builder instance" and that subsequent buildx build commands use the selected builder by default. Because this helper calls docker buildx create --use / docker buildx use but never restores the previous selection or scopes its own build with --builder, running it permanently rewrites the user's default buildx builder. Anyone who already relies on a different selected builder (for example a remote/cloud builder or one with custom networking/certs) will find later builds silently running against lading-builder instead.

Useful? React with 👍 / 👎.

fi

# Build image tags
BUILD_TAGS=("--tag" "${ECR_REPO}:${TAG}")

PUSH_FLAG=""
if [[ "$PUSH" == "true" ]]; then
PUSH_FLAG="--push"
elif [[ "$MULTI_ARCH" != "true" ]]; then
PUSH_FLAG="--load"
else
echo "Warning: multi-arch --no-push builds remain in cache only (use --arch amd64 or --arch arm64 for a loadable image)"
fi

# Build (multi-arch builds both platforms in parallel within a single buildx invocation)
echo "Building for platform(s): ${PLATFORMS}"
echo "Tag: ${TAG}"

docker buildx build \
--platform "$PLATFORMS" \
--build-arg SCCACHE_BUCKET="" \
--build-arg SCCACHE_REGION="" \
Comment on lines +186 to +187

Choose a reason for hiding this comment

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

P2 Badge Stop forcing SCCACHE off for every build

The new Dockerfile guards only enable RUSTC_WRAPPER=sccache when SCCACHE_BUCKET is non-empty, but this script hard-codes --build-arg SCCACHE_BUCKET="" on every invocation and mounts empty AWS secrets. As a result, even the --push path that already runs under aws-vault exec never uses the shared compiler cache, so cold builds regress to recompiling the dependency graph from scratch instead of benefiting from the sccache work added in the previous container-build changes.

Useful? React with 👍 / 👎.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not too worried about that. If build times become an issue, we may do well to try using cargo-chef.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. I think my last local build took <8 min for both release versions.

--build-arg AWS_ACCESS_KEY_ID="" \
--build-arg AWS_SECRET_ACCESS_KEY="" \
--build-arg AWS_SESSION_TOKEN="" \
--secret "id=aws_access_key_id,src=/dev/null" \
--secret "id=aws_secret_access_key,src=/dev/null" \
--secret "id=aws_session_token,src=/dev/null" \
"${BUILD_TAGS[@]}" \
${PUSH_FLAG} \
-f Dockerfile \
.
Comment on lines +196 to +197

Choose a reason for hiding this comment

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

P2 Badge Resolve the Dockerfile path from the repository root

This buildx invocation uses Dockerfile and . relative to the caller's working directory, so the script only works when it is launched from the repository root. Running ci/container-build-and-push from ci/, another workspace subdirectory, or via an absolute path from elsewhere makes Buildx read the wrong context (often omitting most of the repo) and typically fail with a missing Dockerfile or COPY source error. Anchoring both paths to git rev-parse --show-toplevel would avoid that brittle cwd dependency.

Useful? React with 👍 / 👎.

Comment on lines +196 to +197

Choose a reason for hiding this comment

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

P2 Badge Build from the repository root instead of the caller's cwd

I checked the new ci/container-build-and-push path by invoking it from /workspace/lading/docs, and the final docker buildx build -f Dockerfile . still uses the caller's working directory as both the Dockerfile lookup path and build context. That makes a common ../ci/container-build-and-push invocation fail even when you're inside this repo, because docs/ does not contain the root Dockerfile or the full source tree needed by the build.

Useful? React with 👍 / 👎.


if [[ "$PUSH" == "true" ]]; then
echo "Pushed to ${ECR_REPO}:${TAG}"
else
echo "Build complete (not pushed)"
fi
Loading