fix(lambda): stop rejecting a foreign-platform image the daemon cannot describe - #3357
Conversation
|
| Filename | Overview |
|---|---|
| src/main/java/io/github/hectorvent/floci/services/lambda/launcher/ImageCacheService.java | Stops rejecting successfully pulled images based on unreliable Docker 29 index metadata while retaining image-presence and ID validation. |
| src/test/java/io/github/hectorvent/floci/core/common/docker/ContainerPlatformDockerIntegrationTest.java | Verifies the selected foreign architecture through uname and safely falls back when emulation or container waiting fails. |
| src/test/java/io/github/hectorvent/floci/services/lambda/launcher/ImageCacheServiceTest.java | Covers Docker 29 inspection responses that report either the host platform or no platform after a foreign-platform pull. |
Reviews (5): Last reviewed commit: "test(docker): fall back to inspect when ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟡 Changes recommended
The updated ContainerPlatformDockerIntegrationTest introduces a compilation issue (return Assumptions.abort(...)) and a fallback path that can still fail due to non-best-effort log reading.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes Lambda’s Docker image resolution on Docker 29 (containerd image store) by stopping Floci from rejecting a successfully pulled foreign-platform image when docker image inspect reports empty or host-variant Os/Architecture. This preserves the intended behavior where the Docker daemon enforces the requested platform at pull and container create time.
Changes:
- Remove the post-pull platform/arch verification from
ImageCacheServiceand only require that Docker reports an image ID. - Add regression tests covering Docker 29’s inspect behaviors (empty platform, or host variant after a foreign pull).
- Update
ContainerPlatformDockerIntegrationTestto verify the selected image variant primarily by runninguname -minside the container, with a fallback to daemon inspection and skip behavior when inspection cannot confirm.
File summaries
| File | Description |
|---|---|
src/main/java/io/github/hectorvent/floci/services/lambda/launcher/ImageCacheService.java |
Stops rejecting pulled images based on unreliable inspect-reported platform, and instead resolves by image ID only. |
src/test/java/io/github/hectorvent/floci/services/lambda/launcher/ImageCacheServiceTest.java |
Adds unit regression coverage for Docker 29 containerd index-inspect behaviors. |
src/test/java/io/github/hectorvent/floci/core/common/docker/ContainerPlatformDockerIntegrationTest.java |
Switches verification to runtime uname -m with fallback inspect-based assertion or skip when daemon reporting is ambiguous. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This comment was marked as outdated.
This comment was marked as outdated.
c06a9d4 to
4c2a7e2
Compare
|
Upstream context for this, including why an inspect cannot confirm the platform and why it only surfaced on Docker 29, is collected on the issue: #3304 (comment) The short version for review: Docker 29 can answer precisely through |
This comment was marked as outdated.
This comment was marked as outdated.
4c2a7e2 to
edc239d
Compare
…t describe Docker 29 makes the containerd image store its default, and there an image inspect answers for the index rather than the variant that will run: an empty Os and Architecture while no other variant of the tag is local, and the host's variant once one is. ImageCacheService compared that answer with the requested platform and threw, so every container started for another architecture failed with "Docker image does not match requested platform" even though the pull had selected the right variant. Drop that comparison. The daemon enforces the platform where it counts, choosing the variant to pull and the variant to create the container from, so resolving the reference only has to hand back an id. The integration test verified the platform through the same inspect, so it now runs the container and reads uname -m, falling back to inspect where the host cannot run another architecture.
…r never reports awaitStatusCode throws when its timeout expires, so a container that starts but hangs failed the test instead of reaching the inspect fallback the method documents. The wait now treats a failure the same way it treats a nonzero exit. Reading the logs is diagnostic on that path, so it reports a failed read in the text it returns rather than throwing, as the sibling Docker tests already do.
edc239d to
15090da
Compare
pgermosen
left a comment
pgermosen
left a comment
There was a problem hiding this comment.
Traced through why removing the check rather than patching it is right: --platform enforcement at pull and create is daemon-authoritative on every Docker version, so the old inspect-based re-check was always redundant with that, not the actual correctness mechanism, and it's actively wrong on Docker 29's containerd store. A genuinely unavailable platform still fails at the pull step itself before reaching this code, so nothing slips through. The two new unit tests mirror the exact broken readings your own measurement table documents, and the integration test's fallback to running the container degrades sensibly on a host without cross-arch emulation. Approving.
Summary
Fixes #3304.
Docker 29 makes the containerd image store its default, and there an image
inspect answers for the index rather than for the variant that will run. For an
image pulled for another architecture it reports an empty Os and Architecture
while no other variant of that tag is local, and the host's variant once one is.
ImageCacheServicecompared that answer with the requested platform and threw,so every container started for a non-host architecture failed even though the
pull had already selected the right variant:
decide anything,
pullImageCmd.withPlatformchoosing the variant to fetch andcreateContainerCmd.withPlatforminContainerLifecycleManagerchoosing thevariant to run, so resolving the reference only has to hand back an id and say
so plainly when the daemon reports no image at all.
still means "pull it", which is the safe direction and costs only a pull.
ContainerPlatformDockerIntegrationTestverified the platform through the sameinspect, so it would have compared
amd64against an empty string. It now runsthe container, whose command is
uname -m, and reads what the selected variantreports. Running a container built for another architecture needs emulation on
the host, so where the start fails the test falls back to the inspect
assertion, and skips when that reading is the empty or host-variant answer that
cannot confirm anything.
Type of change
fix:)feat:)feat!:orfix!:)AWS Compatibility
No wire behavior changes. This is the container launcher that every
Docker-backed service shares, and the fix restores
Architectures: [arm64]onan amd64 host, and an amd64 image on Apple Silicon, on Docker 29.
Measured on Docker 29.7.2 (macOS, Apple Silicon, containerd image store):
docker image inspect <tag>right afterpull --platform linux/amd64linux/arm64, the host's variantdocker inspect <container>created with--platform linux/amd64Platform: linux, no architectureuname -minside that containerx86_64So the only reading that describes the variant the daemon selected is the
running container, which is what the integration test now uses.
ImageCacheServiceTestgains a regression test for each of the two readingsabove, both of which the old implementation rejects: the pull that comes back
described as the host's variant, and the one that comes back described as
nothing. Docker-backed classes are green together:
ImageCache*,ContainerLauncher*,ContainerPlatform*,ContainerCaBundle*,LambdaExecutionRoleDocker*,ElbV2LambdaTargetDataPlane*,SwfLambda*andRdsContainerManager*, 161 tests.A full
./mvnw teston this host, with #3348 applied for the unrelated Maciefailure it fixes, runs 19150 tests with no failures. Before this change that run
ends on the exception above, since
ContainerPlatformDockerIntegrationTestisthe test that exercises it.
Checklist
./mvnw testpasses locally