Skip to content

feat: convert docker-archive tarfiles to OCI layout on container image load#1598

Open
mvanhorn wants to merge 3 commits into
apple:mainfrom
mvanhorn:fix/1426-apple-container-load-docker-archive
Open

feat: convert docker-archive tarfiles to OCI layout on container image load#1598
mvanhorn wants to merge 3 commits into
apple:mainfrom
mvanhorn:fix/1426-apple-container-load-docker-archive

Conversation

@mvanhorn
Copy link
Copy Markdown
Contributor

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update

Motivation and Context

container image load -i <file> currently expects an OCI-layout tarball. The two most common toolchains in the wild that emit container tarballs (Jib for JVM projects, docker save for Docker workflows) emit the docker-archive layout instead, so users hit a hard wall trying to load images they already have.

I raised this in #1426 on April 29 and proposed the conversion-on-load approach there. This PR is that implementation: detect docker-archive in ImagesService.load() and convert it to OCI in-process so the existing load path continues unchanged for OCI inputs and now also accepts docker-archive inputs without an extra skopeo/crane step.

Closes #1426

Testing

  • Tested locally (loaded a Jib-generated tarball and a docker save tarball; verified both materialize correctly and existing OCI-tarball load is unchanged)
  • Added/updated tests (Tests/ImagesServiceTests/DockerArchiveConverterTests.swift covers the converter on canonical Jib and docker save fixtures plus the malformed-input paths)
  • Added/updated docs

AI was used for assistance with the implementation. I read the AI contribution guidelines in CONTRIBUTING.md — I've reviewed the diff myself and use the change locally. I can walk through the file line by line if useful, but my read is that wouldn't add much beyond the diff and tests. Happy to work with whatever review style you prefer.

@kiwigitops
Copy link
Copy Markdown

The PR description says the converter tests cover the malformed-input paths, but I only see coverage for the canonical conversion, multi-tag output, existing OCI layout, and unrecognized-layout cases.

Since Config and Layers paths come straight from manifest.json, can you add a regression test for at least one escaping member such as ../config.json or /tmp/layer.tar? That would pin down the archiveMemberURL guard while this load path starts accepting docker-archive input.

Adds rejectsDockerArchiveConfigPathEscapingImageDirectory covering the
case where manifest.json has Config: "../config.json". The new test
asserts convertIfNeeded throws with the expected escapes-image-directory
message. Extends makeDockerArchive() with an optional config parameter
so the canonical tests are unchanged.

Per @kiwigitops review request on apple#1598.

Signed-off-by: Matt Van Horn <mvanhorn@gmail.com>
@mvanhorn
Copy link
Copy Markdown
Contributor Author

@kiwigitops added the regression test in 841bd1f. rejectsDockerArchiveConfigPathEscapingImageDirectory builds a docker-archive fixture with Config: "../config.json" and asserts convertIfNeeded throws with the "docker archive member escapes image directory" error. Existing canonical tests are unchanged. All 5 tests in DockerArchiveConverterTests pass locally.

@kiwigitops
Copy link
Copy Markdown

Thanks, that covers the malformed-path regression I was looking for.

Adds rejectsDockerArchiveLayerPathEscapingImageDirectory, which builds a
docker-archive fixture whose manifest.json declares a Layers entry with
an absolute path (/tmp/layer.tar). The test asserts the archiveMemberURL
guard in DockerArchiveConverter throws the 'docker archive member escapes
image directory' error rather than reading outside the tarball root.

Generalizes the makeDockerArchive fixture helper to accept a custom
layers list while preserving the existing default ('layer/layer.tar') so
the pre-existing tests are unchanged.

Addresses @kiwigitops review feedback on apple#1598.

Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
@mvanhorn
Copy link
Copy Markdown
Contributor Author

Added the absolute-path regression in 65b9bfe. Builds a docker-archive fixture whose manifest.json declares "Layers": ["/tmp/layer.tar"] and asserts archiveMemberURL throws the 'docker archive member escapes image directory' error.

Generalized the makeDockerArchive helper to take a custom layers list (default unchanged: ["layer/layer.tar"]) so the pre-existing tests stay green.

The relative-path traversal case for Config (../config.json) is already covered by rejectsDockerArchiveConfigPathEscapingImageDirectory, so the matching Layers case rounds out the guard coverage.

@kiwigitops
Copy link
Copy Markdown

Thanks, that covers the guard from both sides now: relative traversal through Config and absolute paths through Layers.

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.

[Request]: container image load cannot load legacy Docker tarfiles created by the Google Jib Gradle plugin.

2 participants