Skip to content
Open
Changes from 2 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
263 changes: 237 additions & 26 deletions .github/workflows/build-container.yml
Original file line number Diff line number Diff line change
Expand Up @@ -26,16 +26,17 @@ on:
outputs:
path:
description: "Path to built container"
value: ghcr.io/${{ jobs.build-amd64.outputs.repo }}/${{ inputs.name }}:${{ jobs.build-amd64.outputs.tag }}
value: ghcr.io/${{ jobs.check.outputs.repo }}/${{ inputs.name }}:${{ jobs.check.outputs.hash-tag }}

jobs:
build-amd64:
name: Build container (amd64)
runs-on: ${{ inputs.runs-on-amd64 }}
check:
name: Check for existing container
runs-on: ${{ inputs.runs-on-arm64 }}
outputs:
tag: ${{ steps.prepare.outputs.tag }}
repo: ${{ steps.prepare.outputs.repo }}
digest: ${{ steps.build.outputs.digest }}
hash-tag: ${{ steps.prepare.outputs.hash-tag }}
exists: ${{ steps.exists.outputs.exists }}
steps:
- name: Checkout code
uses: actions/checkout@v6
Expand All @@ -44,13 +45,221 @@ jobs:
allow-unsafe-pr-checkout: true
persist-credentials: false

# pull_request_target executes the base-branch workflow while the main
# checkout is the PR head. Grab the executing copy so the content key
# hashes what actually runs (see WORKFLOW_SUM below).
- name: Checkout executing workflow file
if: ${{ github.event_name == 'pull_request_target' }}
uses: actions/checkout@v6
with:
ref: ${{ github.sha }}
sparse-checkout: |
.github/workflows/build-container.yml
sparse-checkout-cone-mode: false
path: .executing-workflow
persist-credentials: false
Comment thread
coderabbitai[bot] marked this conversation as resolved.

# imagetools inspect is used below; ensure buildx is present on all
# runner images (stock GHA and custom labels).
- name: Set up Docker Buildx
uses: docker/setup-buildx-action@v4

# Must precede the digest lookups in "Prepare variables", which may need
# credentials to resolve a private image reference.
- name: Login to GitHub Container Registry
uses: docker/login-action@v4
with:
registry: ghcr.io
username: ${{ github.actor }}
password: ${{ secrets.GITHUB_TOKEN }}

- name: Prepare variables
id: prepare
env:
CONTEXT: ${{ inputs.context }}
DOCKERFILE: ${{ inputs.file }}
REPOSITORY: ${{ github.repository }}
run: |
# Without pipefail a failing stage mid-pipeline still yields a
# well-formed hash, which would silently pin us to a wrong image.
set -o pipefail
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 Blocking: Hash pipeline failures do not terminate the step

pipefail makes a failed find, sort, xargs, awk, or sha256sum pipeline return a failure status, but it does not stop the script without errexit. The script can therefore continue with partial command-substitution output, calculate HASH_TAG, and publish an incomplete content key. Enable errexit so hashing and discovery failures fail closed.

Suggested change
# Without pipefail a failing stage mid-pipeline still yields a
# well-formed hash, which would silently pin us to a wrong image.
set -o pipefail
# A failure in any discovery or hash pipeline must stop the step
# before an incomplete content key can be published.
set -euo pipefail

source: ['codex']

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Resolved in this update — Hash pipeline failures do not terminate the step no longer present.

Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.

BRANCH_NAME=$(echo "${GITHUB_REF##*/}" | tr '[:upper:]' '[:lower:]')
REPO_NAME=$(echo "${{ github.repository }}" | tr '[:upper:]' '[:lower:]')
echo "tag=${BRANCH_NAME}" >> "$GITHUB_OUTPUT"
echo "repo=${REPO_NAME}" >> "$GITHUB_OUTPUT"
REPO_NAME=$(echo "${REPOSITORY}" | tr '[:upper:]' '[:lower:]')

if [ -z "$(find "${CONTEXT}" -type f -print -quit)" ]; then
echo "::error::Build context '${CONTEXT}' contains no files"
exit 1
fi
# Anything the image is built from has to live inside the hashed
# context, or edits to it would not invalidate the tag.
CTX_ABS=$(cd "${CONTEXT}" && pwd -P)
DF_ABS="$(cd "$(dirname "${DOCKERFILE}")" && pwd -P)/$(basename "${DOCKERFILE}")"
case "${DF_ABS}" in
"${CTX_ABS}"/*) ;;
*) echo "::error::Dockerfile '${DOCKERFILE}' is outside the hashed context '${CONTEXT}'"
exit 1 ;;
esac

# Hash the whole context, not just the named Dockerfile: ci.Dockerfile
# pulls in ci-slim.Dockerfile via dockerfile-x, so hashing one file
# alone would let a sibling change reuse a stale image.
CONTEXT_SUM=$(find "${CONTEXT}" -type f -print0 | LC_ALL=C sort -z \
| xargs -0 sha256sum)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 Blocking: Symlinked Dockerfiles can introduce inputs outside the content key

DF_ABS resolves symlinks in the Dockerfile's parent directory but not in its final path component. A PR can therefore replace the selected in-context Dockerfile with a symlink to a repository file outside the context: the lexical boundary check passes, find -type f omits the symlink, and the build follows Dockerfile contents that are absent from the key. Subsequent changes to that target can incorrectly reuse the previously published image. The context stream also omits file modes and symlink targets, which can affect files copied into an image. Resolve and validate the complete Dockerfile path, and construct a deterministic context hash that includes entry type, path, mode, symlink target, and regular-file contents.

source: ['codex']

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Resolved in this update — Symlinked Dockerfiles can introduce inputs outside the content key no longer present.

Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.


# BuildKit re-resolves every external image on a real build, so an
# upstream push to any of them changes the result. They must be in the
# key or that push is silently ignored. Discovered by parsing rather
# than listed by hand, so a newly added FROM cannot be forgotten.
echo "Resolving external image references:"
# Scans every file in the context, not a *.Dockerfile glob, so a
# differently named Dockerfile cannot quietly lose coverage. Skipping
# leading flags handles "FROM --platform=x img" and
# "COPY --chown=u:g --from=img"; taking the first non-flag token after
# FROM ignores the "AS stage" alias. "# syntax = frontend" is collected
# too: BuildKit fetches that floating frontend on every build.
# Local dockerfile-x includes (./foo.Dockerfile) and stage names are
# then dropped, leaving only things resolvable from a registry.
# shellcheck disable=SC2016 # $1/$i are awk fields, not shell vars
EXTERNAL_REFS=$(find "${CONTEXT}" -type f -print0 | LC_ALL=C sort -z \
| xargs -0 awk '
/^#[[:space:]]*syntax[[:space:]]*=/ {
sub(/^#[[:space:]]*syntax[[:space:]]*=[[:space:]]*/, "")
print $1
next
}
toupper($1) == "FROM" {
for (i = 2; i <= NF; i++)
if ($i !~ /^--/) { print $i; break }
}
toupper($1) == "COPY" {
for (i = 2; i <= NF; i++)
if ($i ~ /^--from=/) { sub(/^--from=/, "", $i); print $i; break }
}' \
| grep -vE '^\.' | grep -E '[./:]' | LC_ALL=C sort -u)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Suggestion: Tagless official images are omitted from the digest key

The final grep -E '[./:]' drops valid tagless official references such as FROM ubuntu and FROM alpine, because they contain no dot, slash, or colon. The current Dockerfiles use tagged references, but this parser is intended to discover newly added FROM inputs automatically; after a tagless reference is introduced, movement of its implicit latest tag will not invalidate the key. Track declared stage aliases and explicitly exclude stages, scratch, local includes, and unresolved ARG forms instead of treating punctuation as proof that a reference is external.

source: ['codex']

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Resolved in this update — Tagless official images are omitted from the digest key no longer present.

Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve bare external image references

When a context adds a valid unqualified base such as FROM alpine, this filter drops it because the name contains none of ., /, or :. The other current references keep EXTERNAL_REFS nonempty, so the guard does not detect the omission; after the initial Dockerfile edit builds once, later movement of the implicit alpine:latest tag leaves the content key unchanged and causes CI to reuse a stale image.

Useful? React with 👍 / 👎.

if [ -z "${EXTERNAL_REFS}" ]; then
echo "::error::Found no external image references; the parser is broken and drift in base images would go undetected"
exit 1
fi
IMAGE_SUM=""
ANY_UNRESOLVED=false
# Here-string, not a pipe: a piped `while` runs in a subshell and
# would discard IMAGE_SUM / ANY_UNRESOLVED.
while IFS= read -r REF; do
[ -n "${REF}" ] || continue
RAW=$(docker buildx imagetools inspect --raw "${REF}" 2>/dev/null || true)
if [ -n "${RAW}" ]; then
REF_DIGEST=$(printf '%s' "${RAW}" | sha256sum | cut -d' ' -f1)
else
# Rate limit or outage. Mark the key and force a rebuild for this
# run: reusing a prior "unresolved" image can hide base-image
# drift that happened between outages. Once lookups recover the
# digest-keyed path is used again.
REF_DIGEST="unresolved"
ANY_UNRESOLVED=true
fi
echo " ${REF} -> ${REF_DIGEST}"
IMAGE_SUM="${IMAGE_SUM}${REF}=${REF_DIGEST}"$'\n'

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 Blocking: External digest observations are not bound to the published image

The check job records each floating reference here, but both architecture jobs subsequently resolve the original Dockerfiles and floating tags again. If a tag moves after inspection or between the two architecture builds, the workflow can publish an image—or even two architectures built from different revisions—under a key derived from the previously observed manifest. A later tag rollback can then make that incorrectly keyed image eligible for reuse. Failed lookups have a related collision: every outage run uses the same unresolved marker and forces a rebuild, but concurrent runs can still overwrite or combine the same architecture and manifest tags. Make the builds consume the inspected digests, or verify them again before publication and retry on drift; unresolved runs must fail or use a run-unique key.

source: ['codex']

done <<< "${EXTERNAL_REFS}"

# Note that unpinned apt packages and git refs that move under a fixed
# name (IWYU's clang_NN branch, dash_hash's tag) are deliberately not
# covered. Adding them would achieve nothing: the key only names the
# image, their RUN command strings are unchanged, and a rebuild would
# restore byte-identical layers from cache. Pin them in the Dockerfile
# if they need to move, the way CTCACHE_COMMIT already does.
#
# The Dockerfile we were told to build is part of the key too. Both
# images share this context, so hashing only the directory gives them
# the same key, and repointing one image's file: input would otherwise
# silently reuse the image built from the old one.
#
# This workflow is hashed as well: build-args, target and platforms
# all change the image without touching a Dockerfile, and they live
# in the build step below rather than in the context. Note this covers
# settings written here, not values a caller passes in. Anything added
# to workflow_call.inputs that reaches the build step -- a build-args
# or target passthrough, say -- has to be added to this key too, or
# changing it in build.yml will silently reuse the old image.
#
# Under pull_request_target the executing workflow is the base-branch
# copy (GITHUB_SHA), while the working tree is the PR head. Hash the
# version that actually runs so a PR cannot pre-seed a key for build
# settings it did not execute.
WORKFLOW_FILE=".github/workflows/build-container.yml"
if [ "${GITHUB_EVENT_NAME}" = "pull_request_target" ]; then
EXEC_WF=".executing-workflow/${WORKFLOW_FILE}"
if [ ! -f "${EXEC_WF}" ]; then
echo "::error::${EXEC_WF} not found; the key would silently stop covering build settings"
exit 1
fi
WORKFLOW_SUM=$(sha256sum "${EXEC_WF}")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Remove the checkout path from the workflow digest

In the inspected .github/workflows/build.yml, this reusable workflow runs for both pull_request_target and push. Because sha256sum emits the filename as well as the digest, this branch puts .executing-workflow/.github/workflows/build-container.yml into WORKFLOW_SUM, while the push branch puts .github/workflows/build-container.yml into it. Thus identical workflow contents produce different content keys across the PR and post-merge runs, preventing the push from reusing the image already built and tested for the PR and unnecessarily repeating both architecture builds; retain only the digest bytes before computing HASH_TAG.

Useful? React with 👍 / 👎.

else
if [ ! -f "${WORKFLOW_FILE}" ]; then
echo "::error::${WORKFLOW_FILE} not found; the key would silently stop covering build settings"
exit 1
fi
WORKFLOW_SUM=$(sha256sum "${WORKFLOW_FILE}")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Suggestion: Workflow filename prevents cache reuse between PR and push runs

sha256sum emits the filename alongside the digest. Under pull_request_target, WORKFLOW_SUM therefore contains .executing-workflow/.github/workflows/build-container.yml, while a push contains .github/workflows/build-container.yml. When the executing workflow contents are identical, the differing path text still creates different keys, so the post-merge push cannot reuse the image already built for the PR. Include only the digest bytes.

Suggested change
WORKFLOW_SUM=$(sha256sum "${EXEC_WF}")
else
if [ ! -f "${WORKFLOW_FILE}" ]; then
echo "::error::${WORKFLOW_FILE} not found; the key would silently stop covering build settings"
exit 1
fi
WORKFLOW_SUM=$(sha256sum "${WORKFLOW_FILE}")
WORKFLOW_SUM=$(sha256sum "${EXEC_WF}" | cut -d' ' -f1)
else
if [ ! -f "${WORKFLOW_FILE}" ]; then
echo "::error::${WORKFLOW_FILE} not found; the key would silently stop covering build settings"
exit 1
fi
WORKFLOW_SUM=$(sha256sum "${WORKFLOW_FILE}" | cut -d' ' -f1)

source: ['codex']

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Resolved in this update — Workflow filename prevents cache reuse between PR and push runs no longer present.

Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.

fi
HASH_TAG=$(printf '%s\n%sdockerfile=%s\n%s\n' \
"${CONTEXT_SUM}" "${IMAGE_SUM}" "${DF_ABS#"${CTX_ABS}/"}" \
"${WORKFLOW_SUM}" | sha256sum | cut -d' ' -f1)
echo "Content key: ${HASH_TAG}"
{
echo "tag=${BRANCH_NAME}"
echo "repo=${REPO_NAME}"
echo "hash-tag=${HASH_TAG}"
echo "unresolved=${ANY_UNRESOLVED}"
} >> "$GITHUB_OUTPUT"

- name: Check whether the image was already built
id: exists
env:
REF: ghcr.io/${{ steps.prepare.outputs.repo }}/${{ inputs.name }}:${{ steps.prepare.outputs.hash-tag }}
UNRESOLVED: ${{ steps.prepare.outputs.unresolved }}
run: |
# If any external digest lookup failed, force a rebuild. A prior
# image published under the same "unresolved" marker may predate a
# base-image change that we could not observe during the outage.
if [ "${UNRESOLVED}" = "true" ]; then
echo "External image lookup was unresolved; rebuilding rather than reusing ${REF}"
echo "exists=false" >> "$GITHUB_OUTPUT"
exit 0
fi
# The multi-arch manifest is pushed last, so its presence means both
# arch-specific builds completed. Any failure here falls through to a
# rebuild, which is correct (just slower).
RAW=$(docker buildx imagetools inspect --raw "${REF}" 2>/dev/null || true)
COMPLETE=$(jq -r '
[(.manifests // [])[]
| select(.platform.os == "linux")
| .platform.architecture] as $arch
| (($arch | index("amd64")) != null) and (($arch | index("arm64")) != null)
' <<<"${RAW}" 2>/dev/null || true)
if [ "${COMPLETE}" = "true" ]; then
echo "Reusing existing image ${REF}"
echo "exists=true" >> "$GITHUB_OUTPUT"
else
echo "No complete multi-arch image at ${REF}, building"
echo "exists=false" >> "$GITHUB_OUTPUT"
fi

build-amd64:
name: Build container (amd64)
needs: [check]
# success() is implicit for an `if` with no status function, so this is
# explicit rather than load-bearing: a failed check skips the build either
# way. Only a status function (always(), !cancelled()) would change that.
if: ${{ success() && needs.check.outputs.exists != 'true' }}
runs-on: ${{ inputs.runs-on-amd64 }}
outputs:
digest: ${{ steps.build.outputs.digest }}
steps:
- name: Checkout code
uses: actions/checkout@v6
with:
ref: ${{ github.event.pull_request.head.sha }}
allow-unsafe-pr-checkout: true
persist-credentials: false

- name: Set up Docker Buildx
uses: docker/setup-buildx-action@v4
Expand All @@ -71,14 +280,19 @@ jobs:
push: true
platforms: linux/amd64
tags: |
ghcr.io/${{ steps.prepare.outputs.repo }}/${{ inputs.name }}:${{ hashFiles(inputs.file) }}-amd64
ghcr.io/${{ needs.check.outputs.repo }}/${{ inputs.name }}:${{ needs.check.outputs.hash-tag }}-amd64
cache-from: |
type=registry,ref=ghcr.io/${{ steps.prepare.outputs.repo }}/${{ inputs.name }}:${{ hashFiles(inputs.file) }}-amd64
type=registry,ref=ghcr.io/${{ steps.prepare.outputs.repo }}/${{ inputs.name }}:${{ steps.prepare.outputs.tag }}
type=registry,ref=ghcr.io/${{ needs.check.outputs.repo }}/${{ inputs.name }}:${{ needs.check.outputs.hash-tag }}-amd64
type=registry,ref=ghcr.io/${{ needs.check.outputs.repo }}/${{ inputs.name }}:${{ needs.check.outputs.tag }}
cache-to: type=inline

build-arm64:
name: Build container (arm64)
needs: [check]
# success() is implicit for an `if` with no status function, so this is
# explicit rather than load-bearing: a failed check skips the build either
# way. Only a status function (always(), !cancelled()) would change that.
if: ${{ success() && needs.check.outputs.exists != 'true' }}
runs-on: ${{ inputs.runs-on-arm64 }}
outputs:
digest: ${{ steps.build.outputs.digest }}
Expand All @@ -90,14 +304,6 @@ jobs:
allow-unsafe-pr-checkout: true
persist-credentials: false

- name: Prepare variables
id: prepare
run: |
BRANCH_NAME=$(echo "${GITHUB_REF##*/}" | tr '[:upper:]' '[:lower:]')
REPO_NAME=$(echo "${{ github.repository }}" | tr '[:upper:]' '[:lower:]')
echo "tag=${BRANCH_NAME}" >> "$GITHUB_OUTPUT"
echo "repo=${REPO_NAME}" >> "$GITHUB_OUTPUT"

- name: Set up Docker Buildx
uses: docker/setup-buildx-action@v4

Expand All @@ -117,16 +323,16 @@ jobs:
push: true
platforms: linux/arm64
tags: |
ghcr.io/${{ steps.prepare.outputs.repo }}/${{ inputs.name }}:${{ hashFiles(inputs.file) }}-arm64
ghcr.io/${{ needs.check.outputs.repo }}/${{ inputs.name }}:${{ needs.check.outputs.hash-tag }}-arm64
cache-from: |
type=registry,ref=ghcr.io/${{ steps.prepare.outputs.repo }}/${{ inputs.name }}:${{ hashFiles(inputs.file) }}-arm64
type=registry,ref=ghcr.io/${{ steps.prepare.outputs.repo }}/${{ inputs.name }}:${{ steps.prepare.outputs.tag }}
type=registry,ref=ghcr.io/${{ needs.check.outputs.repo }}/${{ inputs.name }}:${{ needs.check.outputs.hash-tag }}-arm64
type=registry,ref=ghcr.io/${{ needs.check.outputs.repo }}/${{ inputs.name }}:${{ needs.check.outputs.tag }}
cache-to: type=inline

create-manifest:
name: Create multi-arch manifest
runs-on: ${{ inputs.runs-on-arm64 }}
needs: [build-amd64, build-arm64]
needs: [check, build-amd64, build-arm64]
Comment thread
coderabbitai[bot] marked this conversation as resolved.
steps:
- name: Checkout code
uses: actions/checkout@v6
Expand All @@ -146,10 +352,15 @@ jobs:
password: ${{ secrets.GITHUB_TOKEN }}

- name: Create and push multi-arch manifest
env:
CHECK_REPO: ${{ needs.check.outputs.repo }}
IMAGE_NAME: ${{ inputs.name }}
CHECK_TAG: ${{ needs.check.outputs.tag }}
CHECK_HASH_TAG: ${{ needs.check.outputs.hash-tag }}
run: |
REPO="ghcr.io/${{ needs.build-amd64.outputs.repo }}/${{ inputs.name }}"
TAG="${{ needs.build-amd64.outputs.tag }}"
HASH_TAG="${{ hashFiles(inputs.file) }}"
REPO="ghcr.io/${CHECK_REPO}/${IMAGE_NAME}"
TAG="${CHECK_TAG}"
HASH_TAG="${CHECK_HASH_TAG}"

# Create manifest from arch-specific images
docker buildx imagetools create -t "${REPO}:${HASH_TAG}" \
Expand Down