WIP: Compute an uncompressed digest for chunked layers #2155
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mtrmac The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
| } | ||
| defer fg.Close() | ||
| digester := digest.Canonical.Digester() | ||
| if err := asm.WriteOutputTarStream(fg, metadata, digester.Hash()); err != nil { |
There was a problem hiding this comment.
This is the expensive part where we're basically re-synthesizing the tarball from the unpacked files and the tar split-data just to get the checksum, right?
I know this is just a PoC but it seems very much worth a comment at the top of this conditional, something like:
// If we don't yet have the uncompressed digest, compute it now. This is needed
// for zstd:chunked for example to ensure callers identifying images by this
// have the data.
There was a problem hiding this comment.
This is the expensive part where we're basically re-synthesizing the tarball from the unpacked files and the tar split-data just to get the checksum, right?
Yes.
There was a problem hiding this comment.
Comment added, discussing more of the goals, costs and tradeoffs.
pkg/chunked/storage_linux.go
Outdated
| } | ||
| } | ||
|
|
||
| dirFD2, err := unix.Dup(dirFD) |
There was a problem hiding this comment.
This doesn't have O_CLOEXEC set
There was a problem hiding this comment.
This Dup is no longer necessary.
| } | ||
| filename = path | ||
| } | ||
| pathFD, err := securejoin.OpenatInRoot(fg.rootDir, filename) |
There was a problem hiding this comment.
(A little surprising securejoin doesn't have a high level securejoin.OpenReader or something)
There was a problem hiding this comment.
(We have an userns.secureOpen internally with two other callers — centralizing that, and proposing that upstream, does sound attractive.)
pkg/chunked/storage_linux.go
Outdated
| @@ -1734,3 +1754,63 @@ func validateChunkChecksum(chunk *internal.FileMetadata, root, path string, offs | |||
|
|
|||
| return digester.Digest() == digest | |||
| } | |||
|
|
|||
| func newStagedFileGetter(dirFD int, differOpts *graphdriver.DifferOptions, entriesWithOriginalNames []fileMetadata) (*stagedFileGetter, error) { | |||
There was a problem hiding this comment.
I don't know this codebase well but it seems surprising to me that we wouldn't have an equivalent of this function somewhere else, there has to already be code that's serializing zstd:chunked to tar in another place which has to do the same thing?
There was a problem hiding this comment.
There is a getter in layers.go which doesn’t support the composefs digest mapping (in that case we fall back to a different expensive way to generate the diff, IIRC).
There was a problem hiding this comment.
I do agree that having those FileGetter implementations scattered around the codebase suggest that a better design might be possible. OTOH the path/digest mapping is expensive to maintain in a ready-to-use form.
There was a problem hiding this comment.
OTOH the path/digest mapping is expensive to maintain in a ready-to-use form.
On the composefs side that's basically what the composefs blob is (plus all inline metadata) and it's pretty cheap to read back a deserialized abstract tree from a composefs blob, and a big bonus is that because fsverity is enabled on that file, validation is automatic.
There was a problem hiding this comment.
… yes, that’s a great point. There already is a special overlayFileGetter with composefs support, but it uses a temporary mount instead of reading the data directly.
It seems likely that reading the data in user-space would be better, and that could be reused here.
But that’s, also, not the focus of this PR.
There was a problem hiding this comment.
It's also possible to deserialize a composefs in userspace via composefs-info dump; for the use case of turning "composefs into tar" that can be done without mounting it at a kernel level if desired. We have Rust code wrapping that in our crates. I suspect what would be helpful is to try to create a more extensive Go library for composefs separate from this project that fleshes things out like this.
Basically a cool thing about composefs is that it can be natively kernel mounted and provide full verity and things like that, but also because it's just metadata you can think of it much like a tar-split that can be handled in userspace too and efficiently merged etc.
We will start validating them. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
- Use a map of struct{} to save on storage
- Track the items by path, not by the digest; that's one byte more
per entry, but it will allow us to abstract the path logic.
Should not change behavior.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
…igest ... to centralize the composefs backing file layout design. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
mtrmac
force-pushed
the
wip-authentic
branch
from
6c95f6e
to
043e2cd
Compare
…sed digest This will allow c/image to validate the uncompressed digest against the config's RootFS.DiffID value (ensuring that the layer's contents are the same when pulled via TOC and traditionally); and the uncompressed digest will be used as a layer ID, ensuring users see the traditional layer and image IDs they are used to. This doesn't work for chunked layers without a tar-split. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
mtrmac
force-pushed
the
wip-authentic
branch
from
043e2cd
to
4c9e6d8
Compare
There was a problem hiding this comment.
Updated:
- Introduced
composefs.RegularFilePathForValidatedDigest(d)to centralize the path mapping logic - Avoided the
Dup()of the top-level directory file handle - Added more comments.
@giuseppe RFC. Still mostly untested, but closer to the final form, I think.
| // Also, layers without a tar-split (estargz layers and old zstd:chunked layers) can't produce an UncompressedDigest that | ||
| // matches the expected RootFS.DiffID; they will need to use a special TOC-based layer ID, and we need to somehow (??) | ||
| // ensure unambiguity - either by always falling back to full pulls, or by never falling back. |
There was a problem hiding this comment.
@giuseppe @cgwalters note this.
zstd:chunked layers have tar-split in the current format since b5413c2 , i.e. c/storage 1.54 == Podman 5.1.0.
I’m not 100% sure what to do here. I’m leaning towards outright refusing to partially pull estargz and old chunked layers (falling back to full pull), that is “clearly correct” and always possible. (Users who insist on always using partial pulls, e.g. for composefs, can rebuild their images.) Alternatively, we could always do a partial pull and refuse a fallback, but then we would need to break pulling those layers on other graph drivers (or, eventually, implement partial pulls for those graph drivers) — and we still would have the signing ambiguity.
… to ensure we can always use traditional layer IDs and image IDs, and use the uncompressed digests to avoid zstd:chunked view ambiguity, in c/image.
Absolutely untested.