WIP RFC: internal/external interface separation/adapters

[mtrmac]

This is a RFC for a hopefully more manageable approach to evolving the internal interfaces, motivated by the progress discussion in #1438 .

I think this looks fairly clean, but boy it’s a lot of toil.

Major themes:

• Add imagedestination.FromPublic, which allows an internal caller to always use the fresh internal/types interface, instead of dealing with the compat versions. See how this benefits copy.
  • This might eventually turn into a next-generation public interface, now that types.ImageDestination is effectively frozen and deprecated: instead of having internal/imagedestination.FromPublic() returning an internal interface, we could have stable/imagedestination.Destination() returning a struct with newly-designed and newly-named public functions (e.g. using the "functional options" pattern, or anything else), while still letting us have an internal interface that changes over time.
  • ⚠️ As is, this is not really done, notably because it’s not really clear how this wrapping this way is compatible with interface checks like ImageDestinationPartial. I guess we’ll want to fold ImageDestinationPartial into ImageDestinationWithOptions and use a SupportsPutBlobPartial() bool, but it’s not obviously the only possible approach. Maybe just robust self tests would be enough.
• Add imagedestination/impl.AddCompat, which allows an internal transport to implement the new internal/types interface, and rely on provided stubs for to still implement the compat version. See how this benefits storage (although that required a fair bit of code movement).
  • (Also extend this by making internal/types use BlobInfoCache2 instead of the public frozen version; then docker can just assume that version, and delegate dealing with old callers to the generic infrastructure. Note that this has some run-time cost.)

@vrothberg RFC. There might be a much simpler design I’m missing.

I wanted to prototype this, using actually existing internal features and code, to see what it would take to have something similar for new features, e.g. have NewImageSource(…options{Progress:…}), or adding progress bars to those GetBlob/PutBlob implementations that create extra copies/compress/decompress and the like. I think this is broadly viable and correct — but it’s also a fair amount of somewhat tedious work I don’t think I want to commit to do in a rush before Podman 4.0.

[mtrmac]

On second thought, we will need basically all of this, both for ImageSource and ImageDestination, for Cosign, to add support for the new signature format.

So solving this is, one way or another, is not urgent but it’s going to have to happen soonish.

[mtrmac]

It makes no sense to have a separate imagedestination/impl package; a transport is a fully-connected graph of transport/reference/(source+destination), so importing any part of it is going to drag in all of that once we add new features to all most of these interfaces.

So we need something like internal/transport/compat, with compat.Implement{Source,Reference,Destination}(…) (many ways to name this…).

---

(OTOH the other adapter, imagedestination.FromPublic, does conceptually make more sense to have split granularly, one subpackage per interface, because not every caller is going to use every interface — especially if we ever made this public, for callers like skopeo inspect that only need one of the read/write directions.)

[mtrmac]

Because of the type checks, we need to have some mechanisms ensure the conformances hold, but it would be nice to be able to do that without relying on manually writing tests like this. (I did break ImageDestinationPartial in one version of this PR already.)

Hopefully a new internal Transport (with ParseInternalReference returning an internal Reference, and NewInternalSource returning an internal ImageSource, will do that automatically, or something like that.

[mtrmac]

It would be interesting to have the internal names clean, without the WithOptions baggage. We can’t quite do that directly in the returned ImageDestination objects, and doing it here would require wrapping the publicTypes.ImageDestination in another non-zero-cost layer of indirection. Or, alternatively, we could have all of the internal code clean and work directly with internal objects and internal names, but always proxy external callers though the external types in separate objects (so that we can have direct access to internal PutBlob(new) and external callers get a proxy object with PutBlob(old)).

A cheap compromise would be to just name the internal methods PutBlob2 or PutBlobX (with X reserved for all internal names), admittedly that’s also pretty ugly.

[mtrmac]

It almost seems clear that copyBlobFromStream should have a PutBlobOptions parameter, rather than individual ones; the large number of parameters to copyBlobFromStream are not nice at all.

Except for Cache, and for the fact that we might, in that case, want to pass PutBlobOptions by reference instead of by value.

Oh well, we can always introduce a single-purpose struct just for naming parameters to copyBlobFromStream. But that’s out of scope.

[vrothberg]

Very clever! I am cool with the changes but I also begin to wonder whether the added complexity of indirection outweighs the "benefits" of breaking the API and redesigning the API; just a thought/feeling.

[vrothberg]

I really like this idea; very elegant!

[mtrmac]

I also begin to wonder whether the added complexity of indirection outweighs the "benefits" of breaking the API and redesigning the API; just a thought/feeling.

One possible answer to that is to make the external API purely a utility/wrapper that we mostly don’t care about (everything internal uses an internal API), e.g. use internal/types everywhere, exclusively. (This PR leans in the direction of internal-API-primacy, so far without trying to avoid the public API.) OTOH that carries a fair bit of risk of breaking external users and not noticing.

E.g. if we, for SIF, add some kind of progress facility to NewImageSource, a transport that uses that would not actually need to return/implement publicTypes.ImageSource at all; we would just have an object that implements privateTypes.ImageSource, and callers that expect only that interface, in ideal theory nice and consistent and clean. (In practice actual backward/forward compatibility concerns and trade-offs would actually have to manifest somewhere.)

There just aren’t any great options.

• Just drop most of the interfaces, basically provide just c/image/copy. I don’t think a c/image that can’t implement skopeo inspect is very useful at all; also Podman needs various tools to expand/list/create multi-image archives and the like.
• Just don’t bother with API stability, work on features, and break the API. Because we have frozen on an API that isn’t easy to evolve, we would very likely keep bumping the major version quite frequently, the way we went from v1 to v5 in the course of just 7 months! That’s painful for consumers, especially anything that passes a types.ImageReference around (the one pretty stable concept that we I publicly strongly recommend using instead of strings) across repos like c/common would need to coordinate major version bumps.
  • The same, but don’t bump the API major version. Broadly the same kind of mess with worse tooling.
• Spend extra time to design a new API, with little feedback or experience using/evolving it, and announce a v6. Fair bit of risk that we’ll not get that v6 exactly right, and no-one has the time to just stop doing everything else on a rewrite.
• Freeze the problematic parts of the external API (most of it, arguably), add internal variants as we need them for more features, and eventually expose them publicly if there is a user (and only after we have some internal experience).
  • Have every consumer, and every transport, deal with the internal/external dichotomy individually.
  • Do something like this PR, a feature-less tedious work that hides that from most of the code.

Of all those options, it seems to me that something very broadly like this PR is the least bead way to keep moving forward. But I think all of them are sad, and I might very well be wrong about preferring this one.

Admittedly this PR does not feel productive, compared to actual features, and yes, those two interface wrappers are tedious and not elegant. OTOH it was a prerequisite for improving the call stack and naming in storageImageDestination, and I’m quite pleased about that part, if I say so myself.

[mtrmac]

A reminder from earlier #751 : buildah/pkg/blobcache is external to this package, and we need to handle it somehow.

Probably ideally to move that feature inside this repo.

Failing that, we would at least have a real-world test for all the compatibility infrastructure 😟

[mtrmac]

Superseded by #1589.