Conversation
Summary of ChangesHello @casibbald, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the reliability and performance of Skaffold's buildpacks builder when pushing images, particularly for users on macOS with containerd-backed Docker daemons. By instructing the underlying Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request aims to fix an issue with buildpacks on containerd-based systems by having pack publish images directly to the registry. The changes correctly delegate the push operation to pack and adjust Skaffold's logic to avoid tagging and pushing the image itself. However, a critical issue has been identified: while pack is instructed to publish the image, it's consistently using the :latest tag instead of the unique tag generated by Skaffold. This logic flaw means the intended image tag is not pushed to the registry, leading to deployments of stale or non-existent images. This poses a high security risk as it can result in the deployment of vulnerable code versions and will cause deployments to fail with "image not found" errors. The build process should be updated to ensure that the intended tag is correctly published to the registry when direct publishing is enabled. A detailed comment with a suggested fix has been left in pkg/skaffold/build/buildpacks/lifecycle.go.
| ClearCache: clearCache, | ||
| ContainerConfig: cc, | ||
| ProjectDescriptor: projectDescriptor, | ||
| Publish: b.pushImages, |
There was a problem hiding this comment.
When Publish: true is set, pack will publish the image specified in the Image option. Currently, this is always ...:latest (from line 68 and 108). This means the uniquely tagged image (e.g., with a git commit SHA) is never pushed to the registry. Instead, only the :latest tag is pushed.
The Build function in pkg/skaffold/build/buildpacks/build.go then returns the unique tag, assuming it has been pushed. This will lead to "image not found" errors downstream when Kubernetes tries to pull the uniquely tagged image.
When publishing, pack should be instructed to build and push the final, unique tag. The :latest tag should only be used for local, non-pushed builds to leverage caching.
I suggest changing the image passed to pack based on whether we are pushing or not. Since I cannot suggest changes outside the diff, here is an example of how to fix this in pkg/skaffold/build/buildpacks/lifecycle.go:
// ...
parsed, err := docker.ParseReference(tag)
if err != nil {
return "", fmt.Errorf("parsing tag %q: %w", tag, err)
}
latest := parsed.BaseName + ":latest"
imageToBuild := latest
if b.pushImages {
imageToBuild = tag
}
// ...
if err := runPackBuildFunc(ctx, out, b.localDocker, pack.BuildOptions{
// ...
Image: imageToBuild,
// ...
Publish: b.pushImages,
}); err != nil {
// ...The key is to conditionally set the Image option for pack.BuildOptions.
| if b.pushImages { | ||
| return tag, nil | ||
| } |
There was a problem hiding this comment.
The logic introduced here when b.pushImages is true creates a discrepancy between the image pushed to the registry and the tag returned by Skaffold.
In pkg/skaffold/build/buildpacks/lifecycle.go, the build process is configured to publish the image using the :latest tag (e.g., myrepo/myimage:latest). However, this Build function returns the original tag (e.g., myrepo/myimage:v1) and skips the local tagging and pushing steps.
As a result, the intended tag is never pushed to the registry. Subsequent steps in the Skaffold pipeline (like deployment) will attempt to use the intended tag, which will either fail or, more critically, pull a stale version of the image if it already existed in the registry. This can lead to the deployment of incorrect or outdated code, potentially bypassing security fixes included in the new build.
120289a to
7fe1309
Compare
f805770 to
1bb8f91
Compare
When pushImages is true, pass Publish: true to pack and set imageToBuild to the requested tag so pack builds and pushes that tag directly (no daemon write on Mac/containerd). Return the tag from build without local tag/push. Fixes buildpacks build failure on Docker Desktop/Colima.
1bb8f91 to
0d0bf02
Compare
… assembly When docker build --push runs with BuildKit for cross-platform builds, BuildKit generates a provenance/attestation manifest and wraps the pushed image in an OCI Index (manifest list) even for a single-platform build. This caused CreateManifestList to fail with: "no child with platform linux/amd64 in index <arm64-tag>@<digest>" because remote.Image() resolves the index using the host platform (amd64) as the selector, which is not present in the arm64-only index. Two-part fix: 1. manifest_list.go: fetchSinglePlatformImage() first tries remote.Image() (the fast path for plain images). If that fails, it falls back to remote.Index() and extracts the correct platform image from the index, skipping attestation manifests (os=unknown or annotated as such). 2. build/docker/docker.go: set BUILDX_NO_DEFAULT_ATTESTATIONS=1 when building a specific platform with --push so BuildKit does not add the attestation manifest in the first place (defense-in-depth).
Buildpacks: set Publish when pushing to avoid daemon export on containerd (e.g. Mac)
Related: buildpacks/pack#2272 (pack build 27× slower / digest errors with containerd image store), buildpacks/pack#2270 (build fails with containerd + untrusted builder – digest not found)
Description
When users run
skaffold build --push(or equivalent) with the buildpacks builder, Skaffold invokes the pack library withpack.BuildOptions. Previously, Skaffold never setPublish: true, so pack always wrote the built app image to the Docker daemon first; Skaffold would then tag and push. On daemons that use containerd storage (Docker Desktop and Colima on Mac, default for new Docker Desktop installs), that export path can:unable to create manifests file: NotFound: content digest sha256:...: not found– see pack#2270)Setting
Publish: b.pushImagesinpack.BuildOptionswhen building ensures that when Skaffold is doing a push build, pack publishes the image directly to the registry and does not write it to the daemon. That avoids the containerd digest path entirely.When
pushImagesis true, we also return the requested tag immediately after the build and do not calllocalDocker.TagorlocalDocker.Push, because pack has already pushed the image.Changes
pkg/skaffold/build/buildpacks/lifecycle.goIn the
pack.BuildOptionspassed torunPackBuildFunc, setPublish: b.pushImages.pkg/skaffold/build/buildpacks/build.goIn
Build(), whenb.pushImagesis true, returntag, niland skip the usuallocalDocker.Tag/localDocker.Pushpath.User facing changes
skaffold build --pushwith buildpacks on Mac (containerd-backed daemon) could fail with a digest error or hang/slow during export; users had to use workarounds (e.g. run pack with--publishoutside Skaffold).skaffold build --pushwith buildpacks instructs pack to publish directly to the registry. Builds complete successfully on containerd-backed daemons without writing the app image to the daemon.For local registries (e.g.
localhost:5001), the pack lifecycle runs inside a container wherelocalhostis the container itself. Users should set the default repo to a host-reachable address (e.g.--default-repo host.docker.internal:5001orSKAFFOLD_DEFAULT_REPO=host.docker.internal:5001) so the lifecycle can push.Follow-up Work
--default-repo host.docker.internal:5001may be required for buildpacks push.