common/pkg/json-proxy: extract proxy library from skopeo

[giuseppe]

Closes: #674

Skopeo PR: containers/skopeo#2816

[packit-as-a-service]

Packit jobs failed. @containers/packit-build please check.

[packit-as-a-service]

Packit jobs failed. @containers/packit-build please check.

[packit-as-a-service]

Packit jobs failed. @containers/packit-build please check.

[giuseppe]

@mtrmac @cgwalters PTAL

[giuseppe]

changes for Skopeo: containers/skopeo#2816

[cgwalters]

I was curious about the diff between this and the original code. Recently I got burned by an LLM rewriting some of code I just expected to be moved (see composefs/composefs-rs#240 ). On this one I had Opus take a look and it said:

---

• One real bug fix was included: the err/writeErr variable shadowing in GetRawBlob where skopeo was checking the wrong error variable for EPIPE.

(This looks right!)

• Context propagation is the most significant behavioral change — all 10 hardcoded context.Background()/context.TODO() calls were replaced with a passed-in context, enabling proper cancellation.

• Platform support was narrowed from !windows (Linux + macOS + BSDs) to linux only.

• Three panic paths were eliminated (panic in marshal error, unsafe type assertion, nil-unsafe log message in close), all appropriate for library code.

The core protocol and all RPC method implementations are functionally identical to the skopeo originals.

---

Is this accurate? It seems well worth noting if so any intentional changes from the original...

I guess we probably should discuss at least the BSD support, AFAIK this would all work just fine there and would be useful for all the same reasons. MacOS...more nuanced but this does relate to e.g. containers/skopeo#658 in that if we offer a stable RPC API, having progress notifications as part of it would be clearly useful.

(Though solving the skopeo-on-mac-progress-and-RPC problem domain really quickly gets into jsonrpc bindings for the Copy operation, not just raw blob fetches)

[giuseppe]

• One real bug fix was included: the err/writeErr variable shadowing in GetRawBlob where skopeo was checking the wrong error variable for EPIPE.

(This looks right!)

• Context propagation is the most significant behavioral change — all 10 hardcoded context.Background()/context.TODO() calls were replaced with a passed-in context, enabling proper cancellation.
• Platform support was narrowed from !windows (Linux + macOS + BSDs) to linux only.
• Three panic paths were eliminated (panic in marshal error, unsafe type assertion, nil-unsafe log message in close), all appropriate for library code.

The core protocol and all RPC method implementations are functionally identical to the skopeo originals.

the context propagation was forced by the linter we have here, in fact it was not present in the first iteration.

The panic was an explicit change I've done to avoid having it in the library code.

I wrongly assumed this was Linux-only, I'll take another look at that right now.

[giuseppe]

I wrongly assumed this was Linux-only, I'll take another look at that right now.

fixed now

[cgwalters]

Do you know if Go link time dead code will ensure this is omitted from things not using it today? That relates to a related topic in that I think we might just call this "proxy v0" or so and create a "proxy v1" which uses jsonrpc-fdpass from the start or so?

[giuseppe]

Do you know if Go link time dead code will ensure this is omitted from things not using it today?

I've tried vendoring this version into podman, that doesn't use the package, and there is no difference in the resulting binary size

[mtrmac]

Just a drive-by looking at the external API shape only. This is not an implicit ACK to the idea.

---

I really have not followed any progress on the "tar with file descriptor passing for data" PR space since originally complaining about complexity vs. benefit.

I continue to be worried around #674 (comment) , this []any parameter passing style is not really helpful. But I have no firm opinion on long-term direction, nor on possible short-term/long-term trade-offs.

---

How does this help with #651 at all? c/storage can’t depend on c/common (and this can’t go into c/storage because it depends on c/image).

[giuseppe]

How does this help with #651 at all? c/storage can’t depend on c/common (and this can’t go into c/storage because it depends on c/image).

there are two reasons for moving the proxy code here: 1) possibly exposing it in podman, so that bootc doesn't need both podman and skopeo, 2) for #651 we will still need a way to expose the splitfdstream JSON API somewhere, and since we already have something similar, we can just add it as a functionality here

[TomSweeneyRedHat]

This LGTM overall. I did run this through a Gemini review. It pointed out the thought of a Version() function to check the proxies match across versions of Podman say. I didn't see it in the code, is it necessary? Heres Gemini's take:

JSON-RPC Protocol Stability
Since this library will be used across different binaries (e.g., Podman v5.x talking to a Proxy built from a newer container-libs), versioning is key:

Check: Does the handshaking logic include a protocol version?

Recommendation: If it doesn't, now is the time to add a Version() method to the proxy interface to prevent mismatched JSON schemas from crashing the caller.

[cgwalters]

How does this help with #651 at all? c/storage can’t depend on c/common (and this can’t go into c/storage because it depends on c/image).

I am not deep into this so I may be wrong here but: do we need to invent a new pkg/container-libs that acts as a super-lib that depends on the others and might have this? (effectively starting the ball rolling on making the monorepo more into a monolibrary)

[mtrmac]

c/common already is "the top one". The point is that we must not have a dependency circle (it doesn’t matter whether it is between Go modules or just subpackages), so c/storage can’t directly depend on anything higher in the stack.

[cgwalters]

Version() function

How did you give Gemini the context here? Seems hard to miss https://github.com/containers/container-libs/pull/677/changes#diff-b737bb5db7429044b5422fe3a693f965cecd5e9c6011364c9ce7e488f901dda8R20

[mtrmac]

This could help with moving the proxy to Podman so there is no need to also include Skopeo for bootc

IIRC there was historically resistance to this, but I can’t remember the details (there is containers/podman#10075 but that probably doesn’t explain much). @containers/podman-maintainers ?

[Luap99]

I agree, if the goal is to add this to podman then this should be discussed with all podman maintainers first if we even want this.
So the obvious question is how much bloat does this add to podman? I am not convinced that this is a general purpose tool worth shipping as part of Podman overall so if it adds a bunch of bloat for people who do not want this then I would object.
And if this proxy is really worth it for all kind of consumers then why require all of them to install it as part of podman which is much bigger than skopeo? Just because bootc happens to use podman seems little justification for me to move these things around.
And since I guess we have to ship this in podman and skopeo for the near future no matter what we will increase the maintenance load for us.

And I echo @mtrmac comments about testing, we have enough fragile parts of the stack that we cannot test properly here and always require tests in downstream repos, adding yet another such case is not good.

[giuseppe]

We are not going to add it to Podman (yet), this is just to prepare this possibility. It will be exposed only in Skopeo for now.

Now this feature is hidden in Skopeo, but at some point we'd need to stabilize that as well, unless we are planning to keep using experimental-image-proxy for the time being and be fine with it. For this reason, I do not expect the testing effort to be significantly different.

[giuseppe]

also, this change will make #651 easier as we will reuse the same proxy endpoint to expose the fd-pass interface

[Luap99]

I don't have strong feeling about the location of the code. I don't work on it so if you believe having this here is less maintenance effort sure no objection.

I cannot commit to reviewing this here though so I am not going to LGTM it and let someone else do the review/merge.

[cgwalters]

So the obvious question is how much bloat does this add to podman? I

Hi @Luap99 we already discussed this in containers/skopeo#2576 (comment)

Bigger picture your reply (kind of as before) felt more like a "no" and not a "yes and" - you were not giving any alternative suggestions for paths forward. I'm not just showing up here to slightly increase the binary size of podman - my employer is shipping a product for which we are trying to tightly integrate podman and bootc among other tools to have a more container-native operating system, and so I hope we can come to rough consensus on a path to better integration!

To be clear, there is an alternative to this whole codebase, which is in the short term on the bootc (composefs really) side we grow a readonly implmentation of the overlay driver, that's in composefs/composefs-rs#218

But in that path, we'd need to ensure that any nontrivial (esp breaking) changes are coordinated obviously when shipping a product.

[baude]

I'm generally OK with this and think this is probably the right initial location. Couple of questions: are there consumers of this now or soon in the future that will complain if it is moved again ?

secondly, is it possible to include some tests to help address Paul's maintainer concerns?

[giuseppe]

I'm generally OK with this and think this is probably the right initial location. Couple of questions: are there consumers of this now or soon in the future that will complain if it is moved again ?

secondly, is it possible to include some tests to help address Paul's maintainer concerns?

the only consumer is Skopeo, there are no other consumers planned for now.

Are you OK if we add tests later so in the meanwhile we unblock other work that depends on this? The only goal was to move verbatim (as much as possible) the code from Skopeo here without affecting the functionalities

[cgwalters]

It should be pretty easy to wire up a new test context here for this. I did one manually before in containers/skopeo#1781 but I think that basically got moved to cirrus in containers/skopeo#1808 and then became a black hole to me... (Edit nevermind I see it now on recent PRs)

Edit 2: That said we can do WAY WAY better here nowadays. The way this should work I think is:

• Build a binary that exposes a compatible CLI interface
• Follow the same "reverse dependency" style testing that was done there, and is also done in e.g. composefs-rs to test bootc (ref https://github.com/composefs/composefs-rs/blob/main/.github/workflows/bootc-revdep.yml - look how clean and simple that is (IMO))
• This would be lowest friction/easiest in GHA, not Cirrus because especially post CNCF donation we have been leaning into using the CNCF donated resources

[baude]

I'm am fine with a promissory test of course.

[mtrmac]

… oh. We have “integration” tests in Skopeo, and we do run the integration test suite for Skopeo here, but only for the c/image repo. We don’t currently have infrastructure for running Skopeo tests on updates to c/common.

So we would notice breakage here only after vendoring updated c/common into Skopeo, with an arbitrarily-long delay.

[giuseppe]

would be acceptable to run these tests also for this package?

… oh. We have “integration” tests in Skopeo, and we do run the integration test suite for Skopeo here, but only for the c/image repo. We don’t currently have infrastructure for running Skopeo tests on updates to c/common.

So we would notice breakage here only after vendoring updated c/common into Skopeo, with an arbitrarily-long delay.

I've added a new commit that enables Skopeo tests also for changes to this package

[Luap99]

… oh. We have “integration” tests in Skopeo, and we do run the integration test suite for Skopeo here, but only for the c/image repo. We don’t currently have infrastructure for running Skopeo tests on updates to c/common.

But what does this test? The skopeo CI task is named ostree-rs-ext for this thing no and not running as part of integration tests as such the integration tests running here do not cover anything of the proxy code I assume?

[Luap99]

well arguably this is not right, running the cross task or the in repo image tests when the proxy code changes is not useful

at most we should run it for image_test_skopeo_task but even then see my other comment I do not see how this tests the actual proxy code as this is a different cirrus task in skopeo (ostree-rs-ext)

[mtrmac]

… oh. We have “integration” tests in Skopeo, and we do run the integration test suite for Skopeo here, but only for the c/image repo. We don’t currently have infrastructure for running Skopeo tests on updates to c/common.

(I was mostly wrong here, the Skopeo tests do vendor an updated c/common, they just don’t trigger on c/common changes.)

But what does this test? The skopeo CI task is named ostree-rs-ext for this thing no and not running as part of integration tests as such the integration tests running here do not cover anything of the proxy code I assume?

There actually is https://github.com/containers/skopeo/blob/main/integration/proxy_test.go .

(ostree-rs-ext does exist… and points at an archived repo, it turns out.)

A LLM tells me the two test suites have broadly comparable coverage, testing most of the workflow, with some gaps (different in each one).

[giuseppe]

@Luap99 thanks for checking that.

So it seems we have only two alternatives, accept the delay or duplicate the ostree-rs-ext test here.

I'd pick the delay and use a vendoring PR to validate changes, that is no different than what we do with Podman/Buildah

[Luap99]

There actually is https://github.com/containers/skopeo/blob/main/integration/proxy_test.go .

Ah yeah I guess we could update the cirrus condition then to only run the skopeo int task

I'd pick the delay and use a vendoring PR to validate changes, that is no different than what we do with Podman/Buildah

I can certainly live with that.

[Luap99]

I have not looked at any of the code changes to verify they are right so I wont LGTM but I do not object to having the code here so someone else can merge it anytime.

[TomSweeneyRedHat]

I'd like to get at least another LGTM here before merging.

[cgwalters]

I agree with the concerns about tests, we had a live chat about this, I think there's a few options; but leaving aside the full reverse dependency testing (which I think is quite doable and I might have an agent try to draft), but for now:

• copy proxy_test.go here
• build a binary (like is done in c/storage) that is a CLI exposing the core apis ("pretend-skopeo"), in this case that binary is quite trivial as its role is jsut to launch one api
• Run integration tests (like proxy_test) against that binary

[mheon]

Reverse dependency is a good idea, but I think a separate conversation; just getting some testing running locally is IMO sufficient to get this merged.

[giuseppe]

added some local tests

[mtrmac]

Just a late note to keep in mind for the future, no need to immediately change anything (this is an explicitly unstable API anyway).

[mtrmac]

logrus is “in maintenance mode”, so adding more explicit dependencies to it is no longer ideal.

I don’t know what to do instead (we have no other logger set up in Skopeo/Podman now). context.Context is sort of non-committal, and sufficient for log/slog to do priority filtering.