proxy: Add `GetConfig`, add manifest list support, add an integration test #1495

cgwalters · 2021-11-08T14:46:26Z

proxy: Add an API to fetch the config upconverted to OCI

While the caller could fetch this today as a blob, it'd be in
either docker or oci schema. In keeping with the model of having
this proxy only expose OCI, add an API which uses the c/image logic
to do the conversion.

This is necessary for callers to get the diffIDs, and in general
to implement something like an external skopeo copy.

proxy: Add a helper to return a byte array

Since this is shared between the manifest and config paths.

proxy: Use float → int helper for pipeid

Just noticed while scrolling past the code.

tests/integration/proxy_test: New test that exercises proxy.go

I debated adding "reverse dependency testing" using
https://crates.io/crates/containers-image-proxy
but I think it's easier to reuse the test infrastructure here.

This also starts fleshing out a Go client for the proxy (not
that this is going to be something most Go projects would want
versus vendoring c/image...but hey, maybe it'll be useful).

Now what I hit in trying to use the main test images is currently
the proxy fails on manifest lists, so I'll need to fix that.

proxy: Add support for manifest lists

We need to support manifest lists. I'm not sure how I missed this
originally. At least now we have integration tests that cover this.

The issue here is fairly subtle - the way c/image works right now,
image.FromUnparsedImage does pick a matching image from a list
by default. But it also overrides GetManifest() to return the
original manifest list, which defeats our goal here.

Handle this by adding explicit manifest list support code. We'll
want this anyways for future support for GetRawManifest or so
which exposes OCI manifest lists to the client.

proxy_test: Add a helper method to call without fd

To verify in one place.

proxy_test: Add helper to read all from a reply

Prep for testing GetConfig.

proxy_test: Test GetConfig

Now that we have a test suite, let's use it more.

cgwalters · 2021-11-08T15:16:32Z

vet: integration/proxy_test.go:186:3: unknown field Pdeathsig in struct literal

Blah. Annoying when Windows/MacOS come to rain on my Linux happy place.

cgwalters · 2021-11-08T17:43:21Z

integration/proxy_test.go

+)
+
+// This image is known to be x86_64 only right now
+const knownNotManifestListedImage_x8664 = "docker://quay.io/coreos/11bot"


I'm sure there's a better image for this, maybe something in quay.io/libpod?

vrothberg · 2021-11-09T15:39:45Z

Thanks, @cgwalters! I will have a look at this PR tomorrow morning.

vrothberg

Only a minor nit. Could you sign (git commit -s) your commits? I am not sure why the CI isn't barking on that but our contributing guidelines require that.

cmd/skopeo/proxy.go

cgwalters · 2021-11-10T13:52:40Z

Only a minor nit. Could you sign (git commit -s) your commits?

Done!

vrothberg

LGTM
@lsm5 @mtrmac @rhatdan PTAL

cgwalters · 2021-11-10T17:51:08Z

Of these I think 982aa51 could use specific review. I find the c/image APIs here a bit confusing.

mtrmac

Thanks! Looks good overall.

Do note the DiffID comment.

cmd/skopeo/proxy.go

integration/proxy_test.go

cmd/skopeo/proxy.go

integration/proxy_test.go

mtrmac · 2021-11-10T22:08:16Z

Of these I think 982aa51 could use specific review. I find the c/image APIs here a bit confusing.

Looks fine to me.

While the caller could fetch this today as a blob, it'd be in either docker or oci schema. In keeping with the model of having this proxy only expose OCI, add an API which uses the c/image logic to do the conversion. This is necessary for callers to get the diffIDs, and in general to implement something like an external `skopeo copy`. Signed-off-by: Colin Walters <walters@verbum.org>

Since this is shared between the manifest and config paths. Signed-off-by: Colin Walters <walters@verbum.org>

Just noticed while scrolling past the code. Signed-off-by: Colin Walters <walters@verbum.org>

I debated adding "reverse dependency testing" using https://crates.io/crates/containers-image-proxy but I think it's easier to reuse the test infrastructure here. This also starts fleshing out a Go client for the proxy (not that this is going to be something most Go projects would want versus vendoring c/image...but hey, maybe it'll be useful). Now what I hit in trying to use the main test images is currently the proxy fails on manifest lists, so I'll need to fix that. Signed-off-by: Colin Walters <walters@verbum.org>

We need to support manifest lists. I'm not sure how I missed this originally. At least now we have integration tests that cover this. The issue here is fairly subtle - the way c/image works right now, `image.FromUnparsedImage` does pick a matching image from a list by default. But it also overrides `GetManifest()` to return the original manifest list, which defeats our goal here. Handle this by adding explicit manifest list support code. We'll want this anyways for future support for `GetRawManifest` or so which exposes OCI manifest lists to the client. Signed-off-by: Colin Walters <walters@verbum.org>

To verify in one place. Signed-off-by: Colin Walters <walters@verbum.org>

Prep for testing `GetConfig`. Signed-off-by: Colin Walters <walters@verbum.org>

Now that we have a test suite, let's use it more. Signed-off-by: Colin Walters <walters@verbum.org>

To fix compilation on MacOS. I think actually we want to use this pervasively in our tests on Linux; it doesn't really matter when run inside a transient container, but `PDEATHSIG` is useful for persistent containers (e.g.) toolbox and when running outside of a pid namespace, e.g. on a host system shell directly or in systemd. Signed-off-by: Colin Walters <walters@verbum.org>

By Go convention. Signed-off-by: Colin Walters <walters@verbum.org>

cgwalters · 2021-11-16T12:45:36Z

CI failure looks unrelated, is there a way to retrigger or should I just repush?

vrothberg · 2021-11-16T12:49:55Z

I retriggered, let's see what's going on.

vrothberg · 2021-11-16T15:38:09Z

And one more time

cgwalters commented Nov 8, 2021

View reviewed changes

cgwalters mentioned this pull request Nov 8, 2021

enhancements: Propose CoreOS layering coreos/enhancements#7

Merged

8 tasks

cgwalters mentioned this pull request Nov 9, 2021

Use diffids - fix pulling from containers-storage: ostreedev/ostree-rs-ext#153

Closed

vrothberg reviewed Nov 10, 2021

View reviewed changes

cmd/skopeo/proxy.go Outdated Show resolved Hide resolved

cgwalters force-pushed the proxy-config branch 2 times, most recently from d4c90db to e02e864 Compare November 10, 2021 13:51

vrothberg reviewed Nov 10, 2021

View reviewed changes

mtrmac reviewed Nov 10, 2021

View reviewed changes

cgwalters force-pushed the proxy-config branch from e02e864 to 4870c1e Compare November 16, 2021 02:01

cgwalters added 10 commits November 15, 2021 21:02

proxy: Add a helper to return a byte array

6510f10

Since this is shared between the manifest and config paths. Signed-off-by: Colin Walters <walters@verbum.org>

proxy: Use float → int helper for pipeid

a3adf36

Just noticed while scrolling past the code. Signed-off-by: Colin Walters <walters@verbum.org>

proxy_test: Add a helper method to call without fd

f90725d

To verify in one place. Signed-off-by: Colin Walters <walters@verbum.org>

proxy_test: Add helper to read all from a reply

2bb6f27

Prep for testing `GetConfig`. Signed-off-by: Colin Walters <walters@verbum.org>

proxy_test: Test GetConfig

fa86297

Now that we have a test suite, let's use it more. Signed-off-by: Colin Walters <walters@verbum.org>

proxy: Uncapitalize all errors

05a2ed4

By Go convention. Signed-off-by: Colin Walters <walters@verbum.org>

cgwalters force-pushed the proxy-config branch from 4870c1e to 05a2ed4 Compare November 16, 2021 02:02

vrothberg merged commit 0029782 into containers:main Nov 16, 2021

cgwalters mentioned this pull request Nov 17, 2021

Expose GetConfig containers/containers-image-proxy-rs#14

Closed

cgwalters mentioned this pull request Nov 18, 2021

rollup issue for experimental-image-proxy #1482

Closed

github-actions bot added the locked - please file new issue/PR label Sep 21, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

proxy: Add `GetConfig`, add manifest list support, add an integration test #1495

proxy: Add `GetConfig`, add manifest list support, add an integration test #1495

cgwalters commented Nov 8, 2021

cgwalters commented Nov 8, 2021 •

edited

Loading

cgwalters Nov 8, 2021

vrothberg commented Nov 9, 2021

vrothberg left a comment

cgwalters commented Nov 10, 2021

vrothberg left a comment

cgwalters commented Nov 10, 2021

mtrmac left a comment

mtrmac commented Nov 10, 2021

cgwalters commented Nov 16, 2021

vrothberg commented Nov 16, 2021

vrothberg commented Nov 16, 2021

proxy: Add GetConfig, add manifest list support, add an integration test #1495

proxy: Add GetConfig, add manifest list support, add an integration test #1495

Conversation

cgwalters commented Nov 8, 2021

cgwalters commented Nov 8, 2021 • edited Loading

cgwalters Nov 8, 2021

Choose a reason for hiding this comment

vrothberg commented Nov 9, 2021

vrothberg left a comment

Choose a reason for hiding this comment

cgwalters commented Nov 10, 2021

vrothberg left a comment

Choose a reason for hiding this comment

cgwalters commented Nov 10, 2021

mtrmac left a comment

Choose a reason for hiding this comment

mtrmac commented Nov 10, 2021

cgwalters commented Nov 16, 2021

vrothberg commented Nov 16, 2021

vrothberg commented Nov 16, 2021

proxy: Add `GetConfig`, add manifest list support, add an integration test #1495

proxy: Add `GetConfig`, add manifest list support, add an integration test #1495

cgwalters commented Nov 8, 2021 •

edited

Loading