Can we have tests for index.json?

if you look at my comment above it says I need to rewrite tests because test API changed AND current integration tests are testing the old refs directory from image-spec. So yes, I'll do that by adapting the existing ones.

Do we need to check the platform here?

I'll add a check

we're already doing that when actually loading the image

https://github.com/runcom/docker/blob/ffd5a0e5a04ea3640ce7e3e8af669820ff0616fb/image/tarexport/load.go#L455

this seems to hold over into the next name's tags in the for loop below if more than one image is saved at once. e.g.

./build/docker-linux-amd64 -D save -f oci.v1 hello-world nginx -o multi-image-bundle.tar \
Error response from daemon: unable to include unique references "latest" in OCI image

both your nginx and hello-world image have indeed the latest tag, thus you can't save it uniquely

So you can't save two different images, like:
docker image save ubuntu:latest busybox:latest -o image-latest-save.tar
(which works today)?

nope, that's one of the assumption in the design #25779 - you need to use --ref

ah! i was using ref wrong. thanks @runcom

more clear would be "cannot load with both name and refs"

org.opencontainers.image.ref.name

Let's use the constant defined in https://github.com/opencontainers/image-spec/blob/master/specs-go/v1/annotations.go 😅

fixed

CLI tests -> API ones?

nit: remove this blank line

nit: remove this blank line

nit: remove this blank line

Looks like this adds name, refs options to POST /images/load; these should be documented in the swagger.yml, and in the api version history

The refs option looks like it needs some documentation (what's the format, and how is it used?); IIUC, it's a mapping from image:tag(s) to name(s) (OCI)?

I'll add doc around that

is there a swagger/api version doc for experimental as well? (this is experimental)

as discussed on Slack, no real need for swagger API for experimental

Same here for format and refs options for GET /images/get) to the API; these should be documented in the swagger.yml, and in the api version history

Be sure to document what accepted values are for format (and if there's a default / what's the default)

Same here for documenting the refs option as well; what's the format? what's the purpose?

will do

Thanks!

no swagger for experimental

The new options should be ignored on older API versions, and a default should be set (if needed) for older API versions

probably need to ignore new options then

the default is already set

Same here: the new options should be ignored on older API versions, and a default should be set (if needed) for older API versions

👍

already defaulted

@runcom shouldn't they still be ignored on older API versions (and if experimental is disabled?)

There's a != "" below this line which should make sure to ignore this

Oh, what I meant was that if I do;

/v1.25/images/load?refs=something&name=something

It should ignore those options, even though it's set, because API v1.25 doesn't support it

I don't think you need this alternative source for go4.org. vndr seems to work fine with just the canonical url.

tarexport shouldn't need to know about experimental. I think that needs to be handled in this Daemon method (same below for Load).

That looks like it's easy to do for save (just check format), but for load I guess it's a bit more involved. I think it can be handled by changing the tarexporter interface to return the Loader instead of Load(). Then you can check the type by adding tarexport.IsOCILoader(loader).

These options are being used as a dependency of tarexport, but they are specific to the operation, not re-usable dependencies.

They are also not used by all operations (save uses Format, load uses Name).
I think they should be split into SaveOptions and LoadOptions, and should be args to save/load (not the constructor).

This method really has nothing to do with tarexporter, and with my proposed change of removing Options from tarexporter this will need to move somewhere else.

I think this can be split into two functions:

func normalizeOCIRefs(refs map[string]string) error {
   ...
}

func refsToTaggedRefs(refs map[string]string) (map[string]reference.NamedTagged, error) {
   ...
}

parseOCINames can call normalizeOCIRefs since it doesn't need the tagged, and load can call refsToTaggedRefs.

Is this FIXME still relevant?

Instead of passing in both name and refs I think the validation at the top of this function and this if/else block could be extracted into a function for resolving refNameToTag(string) string, and passed in as a dependency to Load.

That helps keep the different responsibilities separate, and reduces the number of dependencies.

This probably makes more sense as a method on ociSaveSession

Discussing in the maintainers session with @tonistiigi @dmcgowan @cpuguy83 @tianon if we should make this customisable, so that a different annotation can be used as name instead of the standard one.

Basically thought is: if a custom annotation is specified (e.g. com.docker.image.refname, which could be used as a default), we take an annotation with that name, and use it as name/reference for the image.

Providing custom names, or loading an image without tagging it could then be done in a follow up pr (as extra feature)

why would we ever differ from what OCI states by introducing/reading a custom docker annotation for an OCI image?

What's current status?

I'm with @runcom -- this doesn't make any sense and would just lead to fragmentation among OCI image users. The main proponent of the org.opencontainers.ref.name setup was @stevvooe -- and I'm sure he had considered integration into Docker as part of the design.

@cyphar I have always been against having naming in OCI, as it causes these kinds of problems. I've only been around to contain the damage. Please stop harassing people with nonsense.

There are two considerations to make with ref.name:

1. It should express a reference to the granularity of a tag.
2. It is only valid on index.json to express tags with an archive.

The problem is that the contents of OCI ref.name may differ from what can be supported by docker. To avoid conflict between Docker and whatever OCI decided to put in ref.name, one can define a vendor specific field that has a more restrictive format that is compatible with docker.

If you want compatibility, you either need to lock ref.name down to what docker supports or support vendor-specific tags.

Please stop harassing people with nonsense.

Woah, simmer. I didn't see anything harassing.

I have always been against having naming in OCI, as it causes these kinds of problems. I've only been around to contain the damage. Please stop harassing people with nonsense.

It wasn't my intention to harass you at all, sorry if I made you feel that way. The reason why I mentioned you is because you did quite a bit of the work on ref.name as well as the main person I remember being a major voice in the ref.name discussions was you (hence as far as I'm aware you are the resident expert on it). I wanted to hear your opinion on it in this context.

For the record, I agree with you that naming is something that generally should've been ironed out a bit more (though I don't really agree it should've been excluded entirely, as that would have caused usability issues in my humble opinion) -- but we now have ref.name and I had always assumed the background of the design had considered how containerd/Docker would interact with it. If I'm mistaken in that assumption, please let me know.

If you want compatibility, you either need to lock ref.name down to what docker supports or support vendor-specific tags.

I don't mind supporting vendor specific tags, but if the idea of docker save --format=oci is to be compatible with other tools then I think compatibility of the "entrypoint" to the image with other tools needs to be considered. Most major OCI image tools (umoci, skopeo, and containerd as far as I know) use ref.name -- so if Docker starts outputting things not using ref.name then it will have compatibility issues (if you recall this was part of one of the longer discussions we had last year -- "how do we make sure name compatibility works among different implementations?").

I understand the concern with Docker putting stuff in ref.name that is incompatible -- maybe we should have some way of telling Docker to remap the image names for OCI images so that they are compatible with ref.name? Or maybe Docker should output both ref.name (in an encoded format to avoid the character set issue -- so that at least it's usable out-of-the-box with other tools) and also a "canonical" docker.ref.name which is vendor-specific and thus tools can grow support for it organically.

@cyphar The concern is really in the other direction. If other tools place stuff in ref.name that is incompatible, docker won't be able to import those without munging the names. The scope of what can be done in ref.name is greater than the scope of what docker can or will support.

While it would have been nice to specify OCI ref.name, there were too many requests for changes to make it work without changing docker. I'll also concede that the grammar is pretty bonkers, so specifying it formally would have been problematic.

I would say that as long as people adhere to what docker supports, there won't be a problem. If there are systems that want to use their own naming and be compatible with docker, the vendor specific ref.name would be an avenue of support.

As far as naming containers in the future goes, you can see some of my latest opinions in https://github.com/containerd/containerd/blob/master/reference/reference.go#L39. This creates a docker-compatible super-set based on schema-less URIs. The host becomes a namespace or authority and registry lookup is done out of band. This is what containerd implements but it defers interpretation of the url up to the actual subsystem that locates the image, rather than relying on microformat details.

@stevvooe

What if docker load --format=oci took a flag that told it what the in-Docker image name should be and what ref.name to source it from should be -- so that at the very least you didn't have to add Docker-specific tags in order to use the OCI import?

Speaking personally, I would prefer having to manually do docker load --format=oci --output cyphar/blah:foo --input some_oci_image:foo rather than having to modify every image I generate so that they can be imported into Docker.

And there is the obvious question of if we already have this issue with just docker load -- how will this problem shake out when (if?) Docker supports OCI images through docker pull? I will be honest that I'm not familiar how the OCI distribution-spec has changed since it was copied from the Docker one -- but I imagine that ref.name is something that we would want to have some sort of unity over? Or will the naming on the distribution-spec level be entirely separate to the naming on the image-spec level?

(As for naming, I think that there should be some even more drastic discussions about naming but I think those are way out of scope here.)