Support OCI artifacts #1574

mtrmac · 2022-06-11T00:34:22Z

This broadly follows the plan from #1408 (comment) :

Reject image-specific operations (like inspect, image ID) on OCI artifacts
Don’t change compression of artifact layers
FIXME: Should we also reject encryption/decryption operations?

See the individual commits for details.

FIXME: At this point I’m rather unhappy about the CanChangeLayerCompression API. It’s good enough for internal use for now, exposing it in c/image/manifest is not great. OTOH I’d rather not spend time investigating the more general which-image-formats-can-support-which-compression-and-what-to-do-if-they-don’t problem space.

At this point absolutely untested apart from unit tests (which don’t cover most of the copy code); this will be the first run against Skopeo’s integration test and a real artifact.

See individual commit messages for details. Quite a lot of the refactoring could be split into a separate PR… if an existing OCI artifact didn’t break CI right now. Something to consider later.

mtrmac · 2022-06-11T00:35:22Z

Note to self: See whether this resolves all of #1150, #1370, #1408 .

mtrmac · 2022-06-11T13:11:50Z

Parts of this have now been split into #1576 and #1577.

vrothberg

LGTM, nice work

vrothberg · 2022-06-13T09:05:45Z

internal/manifest/errors.go

+// This is publicly visible as c/image/manifest.NonImageArtifactError (but we don’t provide a public constructor)
+type NonImageArtifactError struct {
+	// Callers should not be blindly calling image-specific operations and only checking MIME types
+	// on failure; if they care about the artifact type, they should check before using it.


Note that it's non-trivial to figure out whether an image is an artifact or not. An artifact may have a custom config and/or custom layers.

Maybe we can help callers in figuring that out.

https://github.com/opencontainers/artifacts/blob/main/artifact-authors.md#visualizing-artifacts explicitly says that the config.mediaType is how they are supposed to be differentiated.

But then both the primary motivations for this work are not compliant: #1408 wants to use images with custom layers, and the https://github.com/sigstore/cosign/blob/main/specs/SIGNATURE_SPEC.md explicitly defines the signature to use an image MIME type.

In the cases where this PR adds this failure mode, we really care about the thing being an image (e.g. we inspect the config or even convert the config’s format). In cases of abuse of that MIME type, it seems … technically correct… to complain about an “invalid image” more than about an “unexpected artifact”.

But you’re completely right, in practice it might be much more practical to detect the major violators and provide good error messages for them. That’s actually a pretty good reason not to expose the mimeType value of the error (which I was quite on the fence about) until we can provide something useful to callers.

... primarily so that it accepts OCI-formatted images. Proof of concept, as well as other updates necessary to work with that registry version, are in containers/image#1574 . Signed-off-by: Miloslav Trmač <[email protected]>

... primarily so that it accepts OCI-formatted images. Proof of concept, as well as other updates necessary to work with that registry version, are in containers/image#1574 . See See containers/skopeo#1673 for more context. Signed-off-by: Miloslav Trmač <[email protected]>

... primarily so that it accepts OCI-formatted images. Proof of concept, as well as other updates necessary to work with that registry version, are in containers/image#1574 . See containers/skopeo#1673 for more context. Signed-off-by: Miloslav Trmač <[email protected]>

... from containers/image#1574 . > go mod edit -replace github.com/containers/image/v5=github.com/mtrmac/image/v5@oci-artifacts > make vendor Signed-off-by: Miloslav Trmač <[email protected]>

mtrmac · 2022-06-16T00:34:11Z

Fixes squashfs: passthrough squashfs compressed layers #1408 : Layers in non-image OCI artifacts, and layers in images with unknown MIME types, are now never compressed or decompressed, and we don’t update their MIME types even if we detect a specific compression format.
Fixes Generic OCI artifact transport? #1150: (Parts of that report are an artifact-like object with v2s2 schema, which is invalid.) Same as the above, the copy was failing on trying to compress the layers, and that’s now handled correctly.
Support other OCI artifact types #1370 is unclear. (WRT the conversation in that issue, this PR correctly refuses conversions from OCI artifacts to other non-artifact image types, and fails on attempts to query image data on non-image artifacts.) But the original request was for oci/layout to somehow support directories where the top level is nether an OCI index nor an OCI manifest — those are not OCI artifacts and this PR doesn’t add support.

... from containers/image#1574 . > go mod edit -replace github.com/containers/image/v5=github.com/mtrmac/image/v5@oci-artifacts > make vendor Signed-off-by: Miloslav Trmač <[email protected]>

mtrmac · 2022-06-16T18:26:32Z

OK, I’m done with this.

One of the intermediate versions (motivating #1579 ) added a SourcedImage method to check whether the current manifest format supports the chosen compression algorithm. That’s incorrect, because we can convert v2s2 to OCI and compress using Zstd (not supported in v2s2) in one step, i.e. that depends on the destination manifest format, not on the source one. Extending the logic for that is certainly possible in principle (ideally already in determineManifestConversion?), but that’s really a different feature from “don’t break when syncing repos with OCI artifacts”, and a rather lower-priority one.

So, this PR does not handle that any more — and now we need only a single new SourcedImage method instead of three in the last prototype, which simpler and less baggage to carry around if we ended up needing a completely different design.

This also updates the CI image to a newer registry that can accept OCI artifacts, although we don’t currently test with any.

Then, containers/skopeo#1680 re-enables tests of syncing the repo with a Cosign signature; see that for at least a smoke test of this PR.

mtrmac · 2022-06-16T18:38:06Z

This also updates the CI image to a newer registry that can accept OCI artifacts, although we don’t currently test with any.

Oh, actually, that requires containers/skopeo#1679 . Dropping that commit again.

So I think the merging order should be:

This PR, and Use an updated CI image with OCI-capable registry skopeo#1679 , in either order.
After both are in, the revert part of Add OCI artifact support, test syncing Cosign signatures again skopeo#1680 .

One of the is fine, but better placed closer to the reference. The other doesn't relate to that code path at all, and is already documented in types.BlobInfo. Signed-off-by: Miloslav Trmač <[email protected]>

Having each test case streched across four lines doesn't make it any more readable; seeing the test cases on the same screen as the test body is more valuable. Should not change (test) behavior. Signed-off-by: Miloslav Trmač <[email protected]>

Add manifestSchema2FromFixture and manifestOCI1FromFixture instead of open-coding the read+decode steps over and over. The tests also fail immediately if they can't use the fixture, instead of somehow trying to continue (and probably crashing). Signed-off-by: Miloslav Trmač <[email protected]>

…ly caller ... to make the code a tiny bit easier to follow. Only moves unchanged code, should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>

... and add tests for the ImageID implementations. Signed-off-by: Miloslav Trmač <[email protected]>

Signed-off-by: Miloslav Trmač <[email protected]>

This will allow us to refer to ic.c.cannotModifyManifestReason and image-specific properties; it just replaces the objects without changing the logic. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>

... to eliminate at least one of the many parameters. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>

Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>

We will use it for debug logs. Signed-off-by: Miloslav Trmač <[email protected]>

We will add another user of the lookup code. Erorr messages now use mimeType instead of mt, which were required required to be equal on that code path, now that mt is not visible. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>

- Don't compress/decompress layers with unknown MIME types, and layers in OCI artifacts. - Don't even change manifest MIME types in these situations, whatever happens. - Don't substitute compressed/uncompressed variants (via TryReusingBlobWithOptions) for OCI artifacts, if we discover the same variants when copying images that refer to the same blobs. Note that this does _not_ restrict compression to algorithms supported by the SourcedImage, because that would prohibit a single-pass conversion from v2s2 to OCI while compressing to zstd [1], and that's a feature we currently exercise in tests. So, this prevents us from failing to copy OCI artifacts, but users of zstd still need to be careful about choosing OCI manually. [1] We would need to ask the _destination_ format handler about zstd, not the source-format SourcedImage, and we don't currently have that infrastructure. It's also not immediately clear how to combine this with the sequence of alternative manifest formats returned by determineManifestConversion. Signed-off-by: Miloslav Trmač <[email protected]>

vrothberg

LGTM

mtrmac · 2022-06-17T19:12:23Z

So I think the merging order should be:

This PR, and Use an updated CI image with OCI-capable registry skopeo#1679 , in either order.

Here, c/image needs to switch to the updated CI image here as well, or c/image CI is going to break: #1581 .

After both are in, the revert part of DO NOT MERGE: Add OCI artifact support, test syncing Cosign signatures again skopeo#1680 .

including containers/image#1574 . > go get github.com/containers/image/v5@main > make vendor Signed-off-by: Miloslav Trmač <[email protected]>

mtrmac mentioned this pull request Jun 11, 2022

Implement OCI artifacts and revert #1672 containers/skopeo#1673

Closed

mtrmac force-pushed the oci-artifacts branch from 4775d1e to 65ae448 Compare June 11, 2022 01:37

This was referenced Jun 11, 2022

Make c/image/image available as c/image/internal/image #1576

Merged

Consolidate interface mocks in a testing subpackage #1577

Merged

mtrmac force-pushed the oci-artifacts branch from 65ae448 to 77bc502 Compare June 11, 2022 13:11

vrothberg reviewed Jun 13, 2022

View reviewed changes

mtrmac force-pushed the oci-artifacts branch from 77bc502 to ff8971d Compare June 13, 2022 14:40

mtrmac mentioned this pull request Jun 13, 2022

Consider adding a SystemContext override for use-sigstore-attachments containers/container-libs#236

Open

mtrmac mentioned this pull request Jun 13, 2022

Update the registry used for Skopeo CI to 2.8.1 containers/automation_images#137

Merged

mtrmac mentioned this pull request Jun 14, 2022

Use a SourcedImage struct in c/image/copy #1578

Merged

mtrmac force-pushed the oci-artifacts branch from ff8971d to f3ba58d Compare June 14, 2022 16:39

mtrmac mentioned this pull request Jun 15, 2022

Refactor blob copying and compression #1579

Merged

mtrmac force-pushed the oci-artifacts branch 2 times, most recently from b9cc5a9 to a8b2234 Compare June 15, 2022 23:39

mtrmac mentioned this pull request Jun 16, 2022

Add OCI artifact support, test syncing Cosign signatures again containers/skopeo#1680

Merged

mtrmac linked an issue Jun 16, 2022 that may be closed by this pull request

Generic OCI artifact transport? #1150

Closed

mtrmac force-pushed the oci-artifacts branch 2 times, most recently from 4bead21 to ed0ab8c Compare June 16, 2022 18:13

mtrmac force-pushed the oci-artifacts branch from ed0ab8c to e414261 Compare June 16, 2022 18:24

mtrmac marked this pull request as ready for review June 16, 2022 18:26

mtrmac added 14 commits June 16, 2022 20:38

Clean up PreserveOriginal comments

5e86eb8

One of the is fine, but better placed closer to the reference. The other doesn't relate to that code path at all, and is already documented in types.BlobInfo. Signed-off-by: Miloslav Trmač <[email protected]>

Reformat TestSupported*MediaType

ef4d0e2

Having each test case streched across four lines doesn't make it any more readable; seeing the test cases on the same screen as the test body is more valuable. Should not change (test) behavior. Signed-off-by: Miloslav Trmač <[email protected]>

Move getEncryptedMediaType and getDecryptedMediaType closer to the on…

bf597f6

…ly caller ... to make the code a tiny bit easier to follow. Only moves unchanged code, should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>

Reject OCI artifacts in manifest.OCI1.ImageID

83aee8f

... and add tests for the ImageID implementations. Signed-off-by: Miloslav Trmač <[email protected]>

Reject OCI artifacts in manifest.OCI1.Inspect

ec3d50b

Signed-off-by: Miloslav Trmač <[email protected]>

Refuse to convert non-image OCI artifacts to Docker formats

e6e1a4f

Signed-off-by: Miloslav Trmač <[email protected]>

Reject OCI artifacts in image.manifestOCI1.OCIConfig

d24d259

Signed-off-by: Miloslav Trmač <[email protected]>

Move the canModifyBlob decision inside copyBlobFromStream

59bc35e

... to eliminate at least one of the many parameters. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>

Reorganize copyBlobFromStream a bit

00e7d0f

Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>

Add a srcInfo parameter to blobPipelineCompressionStep

83034b5

We will use it for debug logs. Signed-off-by: Miloslav Trmač <[email protected]>

mtrmac force-pushed the oci-artifacts branch from e414261 to 6d458ba Compare June 16, 2022 18:38

vrothberg approved these changes Jun 17, 2022

View reviewed changes

vrothberg merged commit 929f14a into containers:main Jun 17, 2022

mtrmac deleted the oci-artifacts branch June 17, 2022 18:38

This was referenced Jun 17, 2022

squashfs: passthrough squashfs compressed layers #1408

Closed

Generic OCI artifact transport? #1150

Closed

Support other OCI artifact types #1370

Closed

DrDaveD mentioned this pull request Jan 31, 2023

Problems with today's ubuntu containers on dockerhub docker-library/official-images#13983

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Support OCI artifacts #1574

Support OCI artifacts #1574

Uh oh!

mtrmac commented Jun 11, 2022

Uh oh!

mtrmac commented Jun 11, 2022

Uh oh!

mtrmac commented Jun 11, 2022

Uh oh!

vrothberg left a comment

Uh oh!

vrothberg Jun 13, 2022

Uh oh!

vrothberg Jun 13, 2022

Uh oh!

mtrmac Jun 13, 2022

Uh oh!

mtrmac commented Jun 16, 2022

Uh oh!

mtrmac commented Jun 16, 2022

Uh oh!

mtrmac commented Jun 16, 2022

Uh oh!

vrothberg left a comment

Uh oh!

mtrmac commented Jun 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Support OCI artifacts #1574

Support OCI artifacts #1574

Uh oh!

Conversation

mtrmac commented Jun 11, 2022

Uh oh!

mtrmac commented Jun 11, 2022

Uh oh!

mtrmac commented Jun 11, 2022

Uh oh!

vrothberg left a comment

Choose a reason for hiding this comment

Uh oh!

vrothberg Jun 13, 2022

Choose a reason for hiding this comment

Uh oh!

vrothberg Jun 13, 2022

Choose a reason for hiding this comment

Uh oh!

mtrmac Jun 13, 2022

Choose a reason for hiding this comment

Uh oh!

mtrmac commented Jun 16, 2022

Uh oh!

mtrmac commented Jun 16, 2022

Uh oh!

mtrmac commented Jun 16, 2022

Uh oh!

vrothberg left a comment

Choose a reason for hiding this comment

Uh oh!

mtrmac commented Jun 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants