Compute an uncompressed digest for chunked layers

[mtrmac]

… 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.

[cgwalters]

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.

[mtrmac]

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.

[mtrmac]

Comment added, discussing more of the goals, costs and tradeoffs.

[cgwalters]

This doesn't have O_CLOEXEC set

[mtrmac]

This Dup is no longer necessary.

[cgwalters]

(A little surprising securejoin doesn't have a high level securejoin.OpenReader or something)

[mtrmac]

(We have an userns.secureOpen internally with two other callers — centralizing that, and proposing that upstream, does sound attractive.)

[cgwalters]

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?

[mtrmac]

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).

[mtrmac]

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.

[cgwalters]

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.

[mtrmac]

… 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.

[mtrmac]

Cc: @giuseppe

[cgwalters]

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.

[mtrmac]

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.

[mtrmac]

@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.

[mtrmac]

estargz and old zstd:chunked layers now fall back to full pulls by default, unless the user opts out of layer integrity enforcement.

[giuseppe]

just a nit, LGTM

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, 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:

• OWNERS [giuseppe,mtrmac]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[mtrmac]

BTW note the phrasing: without guaranteeing consistency. I.e. this disables the computation of uncompressed digest on partial pulls.

But images are still required to set correct DiffIDs: If we know the uncompressed digest for some reason (e.g. from a non-partial pull of the same blob, when the annotations were missing or the registry did not support partial pulls), and we see that it does not match DiffIDs, we are going to fail any future partial pull of that layer

[giuseppe]

@rhatdan @cgwalters PTAL

[rhatdan]

/lgtm