move libraries to pkgs

[runcom]

@mtrmac PTAL (I've also added some TODO)

This is just another small steps in defining a good API for sub pkgs (which could be later used by other pkgs, i.e. k8s)
Eventually I'd love to remove docker/docker_image.go and integrate relevand code either in img src or img dest. (@mtrmac please confirm this is your view also 😄 )
The major change (other then moving files around) is that I've strongly types GetManifest() to return an interface (with some methods just added) so it will work across implementation.

Again, this is still a WIP but I would like to have this merged and tested as we going forward with this (and also avoid having to review 10k LOC PRs :) :) :))

This code compiles right but I'd love if you could test it out (specifically for openshift related stuff and signing)

Signed-off-by: Antonio Murdaca runcom@redhat.com

[runcom]

This will be eventually just an anonymous struct with methods from the interface to be complain with the return interface arg (or at least I'd love to do so)

[mtrmac]

The ImageManifest returned by dirImageSource needs to understand the manifest contents just as well as the dockerImageSource implementation.

I.e. I think the “download a manifest” and “parse and understand a manifest” code should be separate, with the ImageSource abstraction serving as the border.

[runcom]

I don't remember why this is here (@mtrmac ?)

[mtrmac]

"breaking the abstraction” refers to dockerImage directly using members of i.src = dockerImageSource. That would not be applicable any longer if the method were moved to dockerImageSource.

[runcom]

thx, I've removed it

[mtrmac]

I am not sure about the direction of this; I was thinking of ImageSource/ImageDestination as fairly dumb storage layers, with manifest really being a (blob, MIME content type) pair only. You can get a manifest blob from any source you like, they are all parsed in the same way.

Basically we need to very carefully think about which attributes are common to all storage mechanisms, and which are specific to Docker Registry / OCI bundles / ostree subtrees / … .

(It might well turn out that the dumb storage abstraction is simply unworkable. But if so, we should not be using the “manifest” term referring to v2s[12] config+layer lists to mean “any container image in any format whatsoever”.)

[runcom]

@mtrmac I don't still know if Image[Source|Destination] will be good. However this PR is simply moving files around (and just adapting some code).
I believe after we have moved everything to pkgs we can start concentrating at defining common things across the stuff we need to support.

skopeo was born to just fetch and display manifest from registries at the beginning. However, I think it would be good trying to abstract a common template for manifests (as originally thought upstream also).

(It might well turn out that the dumb storage abstraction is simply unworkable. But if so, we should not be using the “manifest” term referring to v2s[12] config+layer lists to mean “any container image in any format whatsoever”.)

when referring to manifest I'm referring to types.ImageManifest which is an interface though (code probably doesn't refer to this everywhere)

[runcom]

I'm thinking for instance, that the storage layers you added (source|destination) can be very well suited inside a top level object exposed in the API (thinking out loud)

[mtrmac]

(Strictly speaking it is printed to stdout, so not unused…)

The digest is there only because dockerImage.retrieveRawManifest was using it to eventually store it into a DockerImageManifest, and skopeo inspect is printing it to stdout. I did not look further into whether that should happen at all (though why print the unverified digest instead computing a fresh one, now that I look at it?) and simply preserved the original data flow.

[mtrmac]

Actually I don’t think there is a good reason for copy to write this to stdout at all. #53 .

[runcom]

I am not sure about the direction of this; I was thinking of ImageSource/ImageDestination as fairly dumb storage layers, with manifest really being a (blob, MIME content type) pair only. You can get a manifest blob from any source you like, they are all parsed in the same way.

I understand your concerns here though, let me rework this again so that those 2 interfaces just give out a blob

[mtrmac]

However this PR is simply moving files around (and just adapting some code).

I would agree with a simple move in a jiffy. The “adapting some code” here is changing the philosopy, and that will require more thought. You may well be right but I’d like to think about it a bit more.

[mtrmac]

when referring to manifest I'm referring to types.ImageManifest which is an interface though (code probably doesn't refer to this everywhere

This is probably the root of the disconnect; I am thinking about the Docker Registry manifest concept, which does not know that much about containers or tags or anything.

[mtrmac]

I'm thinking for instance, that the storage layers you added (source|destination) can be very well suited inside a top level object exposed in the API (thinking out loud)

Yes, I am thinking along the same lines. If we separate the distribution aspect (the ImageSource and ImageDestination are really DockerV2ImageSource and DockerV2ImageDestination now) and the contents aspect, we could have a

func ParseDockerImage(types.ImageSource) (types.Image, error)

which would internally download the manifest blob, and then convert it into something much more useful to actual end users (e.g. list of layers, labels, configuration extracted from the manifest into nice fields, a method to convert that into a runc setup, whatever).

(Yes, that is quite similar to the existing docker_image top layer; but the distribution of code between dockerImage and dockerImageSource, and especially between dockerImage and its CLI callers, might be different than it is now — the ImageDestination use in dockerImage really should not be there.)

(DockerV2)Image{Source,Destination} would not be used by callers who want to understand the image; it would be only for those who want to copy it and make sure that they don’t modify it (in particular that they don’t break signatures). Though it is unclear that the various transport methods have enough in common that they can be abstracted like this. (e.g. IIRC some versions of the docker save tarballs don’t preserve manifests?)

[rhatdan]

One other thing while you are doing this rework could you remove "docker" wherever possible and use oci, if applicable. Since we will be pulling and pushing oci images as well as docker images.

[mtrmac]

BTW (independent of the rest):

If all retrieveRawManifest does is store the values of i.src.GetManifest, wouldn’t it be simpler to call it directly here, and remove the retrieveRawManifest function entirely?

I suppose we might want caching… but if so, perhaps it should be somehow universal. Make it a contract of ImageSource that data is cached, or add a CachingImageSource?

[runcom]

you're totally right - I just overlooked thinking about this but it does smell a lot - thanks for the suggestion

[mtrmac]

One other thing while you are doing this rework could you remove "docker" wherever possible and use oci, if applicable. Since we will be pulling and pushing oci images as well as docker images.

At the moment, this implements Docker Registry (for which there is no corresponding OCI specification) and Registry V2 “Schema 1” manifests (likewise; OCI standardizes “Schema 2”).

The top-level types are named generically, e.g. ImageSource (though they do assume a lot about a Docker-like structure at the moment).

Other than that, throughout the code we refer to

• “Docker manifest digest”, the $algorithm:$value hash of a Docker manifest. That one might be made more generic, OTOH OCI defines it by referring to the Docker Registry API doc…

• “Docker reference”, the [hostname/]namespace/repo string format. Many parties may use strings with a similar form, so saying “Docker reference” as opposed to generic “URL-like string” makes sense to me. OTOH I am uncertain about this, especially when using the “Docker reference” term in signatures to refer to OpenShift images (which can't be pulled using a plain docker pull with the string being signed). I would welcome independent review whether we should be using the “docker reference” format and naming for OpenShift.

  (Note: We do separate Docker and OpenShift in the UI, docker://host/namespace/repo:tag vs atomic:host/namespace/imagestream:tag; but both look the same, without the docker:// / atomic: prefix, in signatures.)

[mtrmac]

Other than that, throughout the code we refer to

… and then there is the dockerutils subpackage, which is almost, but not completely, not specific to Docker.

[runcom]

closing this in favor of what @mtrmac is doing (though the conversation was interesting)