Skip to content

Pin image mount digests in WithImagesResolved - #884

Closed
ssam18 wants to merge 3 commits into
compose-spec:mainfrom
ssam18:fix/resolve-image-mount-digests
Closed

Pin image mount digests in WithImagesResolved#884
ssam18 wants to merge 3 commits into
compose-spec:mainfrom
ssam18:fix/resolve-image-mount-digests

Conversation

@ssam18

@ssam18 ssam18 commented Jun 8, 2026

Copy link
Copy Markdown

docker compose config --resolve-image-digests pins service image tags to digests but leaves image mounts untouched, so a volume of type image keeps a floating tag. This extends WithImagesResolved so it also pins the source of image mounts the same way it pins the service image. The per image resolution moved into a small helper that is reused for both cases, with a test covering an image mount alongside a bind mount that stays untouched. Reported in docker/compose#13827.

@ssam18
ssam18 requested a review from ndeloof as a code owner June 8, 2026 21:26
@ssam18

ssam18 commented Jun 8, 2026

Copy link
Copy Markdown
Author

This is the compose-go side of the fix for docker/compose#13827. Once it lands and gets released I will follow up with a compose PR that bumps compose-go so the config and publish commands pick it up. Happy to tweak the helper naming or add more cases if you would prefer.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Extends image digest resolution in the project model so WithImagesResolved pins not only service.Image but also image-type volume mount sources, matching docker compose config --resolve-image-digests expectations (per docker/compose#13827).

Changes:

  • Refactors per-image digest pinning into a small helper (resolveImageDigest) reused across service images and image-mount volumes.
  • Updates WithImagesResolved to also resolve service.Volumes[*].Source when volume.Type == VolumeTypeImage.
  • Adds a unit test covering an image mount being pinned while a bind mount remains unchanged.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
types/project.go Reuses a helper to pin digests and now resolves image-mount volume sources in addition to service.Image.
types/project_test.go Adds coverage for resolving an image mount source while leaving bind mounts untouched.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread types/project.go Outdated
Comment thread types/project.go

@glours glours left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The compose-go change itself looks reasonable to me: WithImagesResolved is the right place to extend digest resolution to image mount sources, and the behavior is consistent with how service images are handled today.

That said, I don’t think this is enough to fix the Docker Compose behavior end-to-end. In docker/compose, --lock-image-digests and the publish digest override paths currently build an override that only keeps services.*.image, so any resolved image mount source would still be dropped there. The --no-interpolate --resolve-image-digests path also builds a pseudo-project containing only service images before calling WithImagesResolved.

So a compose-go bump alone won’t fully address docker/compose#13827, and if the follow-up Compose PR only bumps compose-go, Dependabot would cover that anyway. We’ll need corresponding docker/compose changes/tests for the lock/publish/no-interpolate paths, or the scope of this PR should be narrowed accordingly.

I’d also like to see a bit more compose-go coverage here:

  • service without image but with a type: image volume;
  • already canonical image mount source, ensuring the resolver is not called;
  • invalid/unresolvable image mount source.

The WithImagesResolved comment should also be updated since it no longer resolves only service images.

@ssam18
ssam18 force-pushed the fix/resolve-image-mount-digests branch 2 times, most recently from 7180d6a to 0243a83 Compare June 9, 2026 12:21

@glours glours left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the iteration — memoization, the four additional test cases, and the updated doc comment all address my earlier feedback, and the core change is sound (deep-copy isolation holds, concurrency is safe, single-image behavior is preserved).

A few targeted suggestions left inline:

  • one test that conflates canonical detection with the cache and could mask a future regression,
  • the per-service cache scope (a project-wide cache would dedupe across services too),
  • a doc-comment phrasing that overstates the dedup guarantee (could be fix by the per-project cache above ☝️ )
  • plus two small nits on a parameter name and a brittle error assertion.

Comment thread types/project_test.go
Comment thread types/project.go Outdated
Comment thread types/project.go Outdated
Comment thread types/project.go Outdated
Comment thread types/project_test.go Outdated
@ssam18
ssam18 force-pushed the fix/resolve-image-mount-digests branch from 0243a83 to dc979e1 Compare July 5, 2026 15:41
@ssam18

ssam18 commented Jul 5, 2026

Copy link
Copy Markdown
Author

Thanks @glours! All points addressed in the latest push, and the branch is rebased on current main:

  • Project-wide cache: hoisted to WithImagesResolved as a sync.Map as suggested. Since WithServicesTransform runs services concurrently, I also wrapped lookups in golang.org/x/sync/singleflight (already a dependency via errgroup) a bare sync.Map would still let two services racing on a cache miss each hit the resolver, so singleflight is what actually guarantees one resolver call per distinct reference. It also makes the new cross-service dedup test deterministic.
  • Doc comment: reworded to describe the project-wide dedup guarantee.
  • alreadyCanonical test: dropped service.Image so only the image-mount path is exercised and resolverCalls == 0 can only come from canonical detection.
  • New test: Test_ResolveImages_imageMount_sharedAcrossServices three services sharing one tag, asserting a single resolver call (race-clean under -race -count=20).
  • Nits: dropped the named parameter name in the resolver type literal, and switched to assert.ErrorContains.

ssam18 added 3 commits July 5, 2026 10:43
docker compose config --resolve-image-digests pins service image tags to digests but leaves image mounts untouched, so a volume of type image keeps a floating tag. This extends WithImagesResolved so it also pins the source of image mounts the same way it pins the service image. The per image resolution moved into a small helper that is reused for both cases, with a test covering an image mount alongside a bind mount that stays untouched.
Reported in docker/compose#13827
Signed-off-by: Samaresh Kumar Singh <ssam3003@gmail.com>
…y doc comment

Signed-off-by: Samaresh Kumar Singh <ssam3003@gmail.com>
- Hoist the resolution cache from per-service to WithImagesResolved scope (sync.Map) so a reference shared by several services is resolved once, and wrap lookups in singleflight so services transformed concurrently cannot race past the cache and trigger duplicate resolver calls.
- Update the WithImagesResolved doc comment to describe the project-wide dedup guarantee.
- Drop the named parameter name in resolveImageDigest's resolver type literal to avoid visual collision with the local variable.
- alreadyCanonical test: drop service.Image so resolverCalls == 0 can only be explained by canonical detection, not a cache hit.
- Use assert.ErrorContains for the unresolvable-image error check.
- Add a test covering a reference shared across services (single resolver call project-wide).

Signed-off-by: Samaresh Kumar Singh <ssam3003@gmail.com>
@ssam18
ssam18 force-pushed the fix/resolve-image-mount-digests branch from dc979e1 to 54673f6 Compare July 5, 2026 15:44
@ndeloof

ndeloof commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

fixed by #894

@ndeloof ndeloof closed this Jul 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants