Introduce OCI image unpack feature - #381
henrybear327 wants to merge 5 commits into
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 16 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cmd/oci/helpers_test.go">
<violation number="1" location="cmd/oci/helpers_test.go:59">
P3: When a tarEntry has an empty Name with Type==0 and Link=="", `e.Name[len(e.Name)-1]` indexes index -1 and panics the test. Guard for the empty Name before indexing, or produce a clear t.Fatal.</violation>
</file>
<file name="go.mod">
<violation number="1" location="go.mod:8">
P3: Removing gotest.tools/v3 from go.mod while its go.sum hashes remain means go.sum was not tidied. Run `go mod tidy` so the go.sum entry for the removed module is dropped, keeping go.mod/go.sum consistent.</violation>
</file>
<file name="cmd/oci/store.go">
<violation number="1" location="cmd/oci/store.go:78">
P2: When `oci-layout` is missing or malformed, `openStoreForRead` still accepts the store and `unpack` proceeds. Validate the required OCI layout metadata read-only before returning the store.</violation>
</file>
<file name="cmd/oci/unpack_test.go">
<violation number="1" location="cmd/oci/unpack_test.go:85">
P2: The dangling-symlink subtest always fails because it bypasses `unpackImage` and calls `unpackImageFresh`, whose atomic rename replaces the symlink successfully. Call `unpackImage` here so the test exercises the destination validation it intends to verify.</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
| return nil, fmt.Errorf("store: %s is not a directory", root) | ||
| } | ||
| s := &store{root: root} | ||
| if err := s.checkLayout(); err != nil { |
There was a problem hiding this comment.
P2: When oci-layout is missing or malformed, openStoreForRead still accepts the store and unpack proceeds. Validate the required OCI layout metadata read-only before returning the store.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmd/oci/store.go, line 78:
<comment>When `oci-layout` is missing or malformed, `openStoreForRead` still accepts the store and `unpack` proceeds. Validate the required OCI layout metadata read-only before returning the store.</comment>
<file context>
@@ -53,6 +59,184 @@ func openStore(root string) (*store, error) {
+ return nil, fmt.Errorf("store: %s is not a directory", root)
+ }
+ s := &store{root: root}
+ if err := s.checkLayout(); err != nil {
+ if errors.Is(err, errNoMarker) {
+ return nil, fmt.Errorf("store: %s is not an elfuse OCI store", root)
</file context>
There was a problem hiding this comment.
Is the issue still valid? Why?
There was a problem hiding this comment.
@henrybear327 Partially. The malformed half is fixed in the current code, but the missing-file half is not.
The latest openStoreForRead now reads oci-layout and runs checkJSON on it when the file exists, so a corrupt layout is refused (store: corrupt oci-layout), and TestOpenStoreForReadRefusals covers exactly that. That part of the comment is resolved.
What's still open: the missing case. Look at this branch:
if b, err := os.ReadFile(filepath.Join(root, "oci-layout")); err == nil {
if err := checkJSON("oci-layout", b); err != nil {
return nil, err
}
} else if !os.IsNotExist(err) {
return nil, err
}A missing oci-layout falls through and the store is accepted, so unpack still proceeds on a directory that isn't a valid OCI layout. The justification for the comment holds: the read path claims to apply pull's format checks without repairing, and pull treats a missing oci-layout as a file to create — i.e. part of the format. A marker-holding store without oci-layout only arises from tampering, but there's no test for it either.
So: worth narrowing the comment to the missing-file case rather than retracting it. Severity is low since it only triggers on a hand-edited store.
Tip: get faster answers by chatting with cubic’s review copilot in the review UI
| } | ||
| var err error | ||
| captureOutput(t, func() { | ||
| err = unpackImageFresh(context.Background(), s, manifestOf(t, s, d), dest) |
There was a problem hiding this comment.
P2: The dangling-symlink subtest always fails because it bypasses unpackImage and calls unpackImageFresh, whose atomic rename replaces the symlink successfully. Call unpackImage here so the test exercises the destination validation it intends to verify.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmd/oci/unpack_test.go, line 85:
<comment>The dangling-symlink subtest always fails because it bypasses `unpackImage` and calls `unpackImageFresh`, whose atomic rename replaces the symlink successfully. Call `unpackImage` here so the test exercises the destination validation it intends to verify.</comment>
<file context>
@@ -0,0 +1,648 @@
+ }
+ var err error
+ captureOutput(t, func() {
+ err = unpackImageFresh(context.Background(), s, manifestOf(t, s, d), dest)
+ })
+ if err != nil {
</file context>
| err = unpackImageFresh(context.Background(), s, manifestOf(t, s, d), dest) | |
| err = unpackImage(context.Background(), s, "bad:1", manifestOf(t, s, d), dest) |
There was a problem hiding this comment.
Is the issue still valid? Why?
There was a problem hiding this comment.
@henrybear327 Yes, the issue is still valid.
The dangling-symlink case still calls unpackImageFresh through unpackFreshTo:
err = unpackImageFresh(context.Background(), s, manifestOf(t, s, d), dest, false)unpackImageFresh stages into a sibling directory and then renames it over dest. On Unix, that rename replaces a dangling symlink, so the test does not exercise the intended “destination must be a directory” validation and may succeed unexpectedly.
Use unpackImage for this subtest instead:
err = unpackImage(
context.Background(), s, "bad:1",
manifestOf(t, s, d), dest,
)unpackImage calls existingDirectory first, which uses Lstat and correctly rejects the symlink without modifying it.
Tip: get faster answers by chatting with cubic’s review copilot in the review UI
| hdr.Typeflag = tar.TypeSymlink | ||
| hdr.Linkname = e.Link | ||
| hdr.Size = 0 | ||
| case e.Name[len(e.Name)-1] == '/': |
There was a problem hiding this comment.
P3: When a tarEntry has an empty Name with Type==0 and Link=="", e.Name[len(e.Name)-1] indexes index -1 and panics the test. Guard for the empty Name before indexing, or produce a clear t.Fatal.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmd/oci/helpers_test.go, line 59:
<comment>When a tarEntry has an empty Name with Type==0 and Link=="", `e.Name[len(e.Name)-1]` indexes index -1 and panics the test. Guard for the empty Name before indexing, or produce a clear t.Fatal.</comment>
<file context>
@@ -10,27 +10,59 @@ import (
+ hdr.Typeflag = tar.TypeSymlink
+ hdr.Linkname = e.Link
+ hdr.Size = 0
+ case e.Name[len(e.Name)-1] == '/':
+ hdr.Typeflag = tar.TypeDir
+ if e.Mode == 0 {
</file context>
There was a problem hiding this comment.
Is the issue still valid? Why?
There was a problem hiding this comment.
@henrybear327 Yes, it still stands as a code-level hazard, though it's defensive rather than something the current tests hit.
The switch is unchanged in the current file: when e.Type == 0 and e.Link == "" (both the zero values), control falls to case e.Name[len(e.Name)-1] == '/', and tarEntry{Name: ""} (a plain body with no type/link) indexes [-1] and panics. Every call site in the current suite passes a non-empty Name, so it won't fire today — but the helper's contract doesn't enforce that, and a future test author adding an entry with just a Body (the struct's most natural bare use) would get a confusing index-out-of-range panic instead of a clear message.
Cheapest fix is to make the empty case explicit before the indexing, e.g.:
case e.Name == "":
// clear t.Fatal, or fall through to a regular file with a generated nameThat preserves the existing behavior for real entries and turns the footgun into a readable failure. I'd keep it as P3 either way.
Tip: get faster answers by chatting with cubic’s review copilot in the review UI
| require ( | ||
| github.com/alecthomas/kong v1.16.1 | ||
| github.com/google/go-containerregistry v0.21.7 | ||
| github.com/moby/go-archive v0.3.3 |
There was a problem hiding this comment.
P3: Removing gotest.tools/v3 from go.mod while its go.sum hashes remain means go.sum was not tidied. Run go mod tidy so the go.sum entry for the removed module is dropped, keeping go.mod/go.sum consistent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At go.mod, line 8:
<comment>Removing gotest.tools/v3 from go.mod while its go.sum hashes remain means go.sum was not tidied. Run `go mod tidy` so the go.sum entry for the removed module is dropped, keeping go.mod/go.sum consistent.</comment>
<file context>
@@ -5,16 +5,21 @@ go 1.25.0
require (
github.com/alecthomas/kong v1.16.1
github.com/google/go-containerregistry v0.21.7
+ github.com/moby/go-archive v0.3.3
github.com/opencontainers/image-spec v1.1.1
)
</file context>
There was a problem hiding this comment.
Is the issue still valid? Why?
There was a problem hiding this comment.
@henrybear327 Still valid, with one correction to the original wording.
The substance holds: go.mod drops gotest.tools/v3 from the indirect requires while go.sum still carries its hashes, and that is the shape of an untidied go.sum. The fix—run go mod tidy and commit the result—is still the right action.
But the claim "the go.sum entry for the removed module is dropped" was too strong. gotest.tools/v3 v3.5.2 is still in the module graph: moby/go-archive v0.3.3's go.mod requires it (it's a test dependency of that module), so a Go 1.17+ tidy keeps the gotest.tools/v3 v3.5.2/go.mod hash for graph verification. What tidy actually prunes is the full-module h1: hash, since nothing in elfuse's build imports gotest.tools packages. Expect the outcome to be one line removed, not two.
Worth noting while you're there: davecgh/go-spew also lingers in go.sum with both hashes and no apparent importer or graph requirement, which is more stale-cruft evidence that go mod tidy wasn't the last step. The issue is cosmetic though—stale go.sum entries don't fail builds or verification, so this stays a P3-level nit.
Tip: get faster answers by chatting with cubic’s review copilot in the review UI
Defer the temporary file's close and removal in place of the per-path cleanup calls, and sync the directory through syncDirectory after the rename.
ensureLayoutLocked, ensureJSONFile, and pinLocked keep their behavior. The unpack read path needs the same marker, legacy-layout, JSON, and per-reference index checks without the repair steps around them.
Resolve a reference and platform to a manifest digest through digestFor, load that manifest through manifestFor, and derive the directory its rootfs unpacks to through cacheDir. Cache directories are keyed by manifest digest under a per-kind sha256 directory, and a symlink at either level is refused. openStoreForRead applies pull's format checks, oci-layout included, and creates or repairs nothing, so a mistyped --store leaves nothing behind. refuseRootfsInStore compares directory identity up the ancestor chain, so --rootfs can neither name a path inside the store, case aliases included, nor contain the store.
dd87cca to
b7f5670
Compare
Without --rootfs the rootfs is published under the store by manifest digest through a rename, and only that content-addressed entry may reuse another unpack's tree when the rename is lost; a caller-named rootfs is merged in place or staged and renamed, and a lost rename is an error. ensurePrivateDir sets the kind directory's mode too, so the blob and rootfs caches share it. moby/go-archive owns layer application, whiteouts, decompression, containment, and metadata. Each header is rewritten after the previous entry has been applied, so parent symlinks already on disk resolve. Devices, FIFOs, and hardlinks to them become whiteouts. Absolute symlink targets are rebased relative to the link, and a hardlink to such a symlink is rebased at its own location. A .. above the root clamps there, in parent paths and in rebased targets. Special mode bits are cleared because ownership is never applied. Directories stay accessible until all layers finish, then their modes are restored, on failure too, skipping a record whose path a case alias in a later layer replaced. Staging trees are removed without following symlinks or failing on restrictive directory modes. Cancellation is checked on the decompressed stream, so an unpigz child's exit status cannot replace it.
Describe the rootfs cache, --rootfs, what moby/go-archive handles and what elfuse rewrites first, and the case-sensitive volume a real rootfs needs.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
b7f5670 to
2a39064
Compare
| if err := refuseRootfsInStore(s.root, c.Rootfs); err != nil { | ||
| return err | ||
| } | ||
| ctx := context.Background() |
There was a problem hiding this comment.
context.Background() has no deadline and no signal source, so the cancellation work in applyLayer and contextReader never runs outside tests. Ctrl-C kills the process before any deferred cleanup. For an existing --rootfs, the image is left half applied, and every directory newLayerPolicy relaxed keeps its added u+rwx. For an absent --rootfs, the .NAME.tmp-* staging tree stays next to it, and nothing sweeps that location. Wrapping the context with signal.NotifyContext(ctx, os.Interrupt, syscall.SIGTERM) lets the existing error path restore the modes and remove the staging tree.
| ctx := context.Background() | |
| ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) | |
| defer stop() |
| fmt.Fprintf(os.Stderr, "Already unpacked %s -> %s\n", ref, dest) | ||
| return nil | ||
| } | ||
| fmt.Fprintf(os.Stderr, "Unpacking %s -> %s\n", ref, dest) |
There was a problem hiding this comment.
The managed cache lives under the store, which by default is on the case-folding boot volume. Without --rootfs, the case-sensitive sparsebundle that docs/usage.md recommends has nowhere to go. go-archive removes whatever the next entry's name matches, so two names that differ only in case silently overwrite each other (for example, Alpine linux-headers ships xt_DSCP.h and xt_dscp.h). The unpack still reports success. At minimum, check whether the destination volume is case-sensitive and warn when it isn't. Better: fail when an entry would replace a path whose on-disk spelling differs and which an earlier entry of the same unpack created. The unpack can then say which file it lost.
2 participants
Summary by cubic
Adds the
unpackcommand toelfuse-ociso stored images can be extracted into a rootfs for use withelfuse --sysroot; previously images could only be pulled and stored.--rootfs, the rootfs is cached in the store by manifest digest, staged in a sibling temp directory, and published by rename; completed entries are reused and abandoned staging trees are swept after a day.--rootfs DIR, an existing directory is merged in place, an absent one is staged and renamed, and destinations inside the store, or containing it, are refused, including case aliases.moby/go-archivefor whiteouts, hardlinks, decompression, and metadata; ownership is never applied.pull's store format checks without creating or repairing the store.Written for commit 2a39064. Summary will update on new commits.