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

[cgwalters]

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.

Ah sorry I misunderstood! So what I think will happen here is the c/common code will reuse the splitfdstream bits from c/storage right? That seems sane to me offhand but obviously it'd be helpful to prove that out.

[TomSweeneyRedHat]

For Gemini, I just opened up its web interface and asked it to review this PR. It asked me to provide the title, and once I did, off it went.

I think it saw the version. What I think it was harping about is a Version() function to ensure that whoever sent the request was on the same version.

[cgwalters]

Hmm, but I don't know that the web interface would actually go to the trouble of cloning the git repository, it might try to do some web requests but I wouldn't trust it to do that consistently.

Anything related to code is going to be most effective in a (sandboxed) agent that has the git repository directly.

[giuseppe]

I've addressed the initial comments I've got

[TomSweeneyRedHat]

Happy green test buttons and one LGTM. Can we get another and a merge?

[mtrmac]

The way I think about it, this sort of depends on #651 . Without that, this just makes the development of the proxy harder (all tests remain in the Skopeo repo, so any new feature will need to be 2 coordinated PRs).

[giuseppe]

The way I think about it, this sort of depends on #651 . Without that, this just makes the development of the proxy harder (all tests remain in the Skopeo repo, so any new feature will need to be 2 coordinated PRs).

This could help with moving the proxy to Podman so there is no need to also include Skopeo for bootc, so in a way this could be helpful on its own

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