Skip to content

Conversation

@runcom
Copy link
Member

@runcom runcom commented Jun 28, 2016

This is in preparation to support manifests list:

  • add the possibility to select manifest by reference (tag or digest) when getting a manifest from an ImageSource

The interface does not seems that bad to me but I'd like to know if you're ok with it. This is basically needed because after getting a manifest list, we need to select a manifest digest from the manifest list for a given platform (we'll start with supporting the current runtime the library is used on at first and then we can expand to let users specify platform specs) and fetch it (by digest) and then convert it to either v2s1 or v2s2 to be used with the genericManifest interface.

This is basically the same as GetBlob which lets you specify a digest - because the image repository is the smallest unit we work on - not the actual digest or tag - when dealing with registries.

/cc @mtrmac

Signed-off-by: Antonio Murdaca [email protected]

@mtrmac
Copy link
Collaborator

mtrmac commented Jun 29, 2016

I don’t think the ability to specify a tag here is useful or desirable; the tag, if it exists at all (dir: doesn’t have it), is a part of the identity of the ImageSource. For similarity with GetBlob, I think the parameter should be called a digest and support digests only. (Though I realize that implementation-wise “digest or tag” and “digest” may amount to be the same code.)

Also, if I specify a something:specifictag ImageSource, and then call GetManifest(…, digestNotMatchingThatTag), what is supposed to happen? I would hope for “explicitly unsupported”, right now the API does not say.

(I guess what I am suggesting in all of the above by s/reference/digest/ and having the types.go comment say that the digest, if non-empty, must originate from a manifest list received from the same ImageSource.)


A philosophical/bikeshedding question: “whether a manifest list is a manifest”: do I get a manifest list through GetManifest and then call GetManifest for a specific manifest out of that list, or would there be a GetManifestList without that digest parameter? Perhaps we can start with the GetManifest until/unless there is a clear need to distinguish the two, but having the thinking and design clear would be nice.


Implementation-wise, I find it really surprising that GetManifest, if given a specific digest, would in some of the implementations silently and happily ignore the digest and return a non-matching manifest. I mean, technically we can define the API that way, and force the verification to happen in the callers (if they care) — and there might be fewer callers (genericImage and copy?) than backend sources (we already have 3–4) — but it feels awkward and error-prone. Either outright rejecting non-empty digests in backends which do not support them (like dir: currently, which always gives you the same manifest), or reading the manifest and doing a manifest.MatchesDigest check before returning it to the caller, would make more sense to me.

OTOH, admittedly, we do not validate contents of a GetBlob inside GetBlob, and leave signature consumers who care etc. to do that themselves. I don’t see that docker: needs to do the manifest.MatchesDigest check at this level; it’s just the dir: behavior which is particularly awkward.

[Actually, having dir: truly support manifests lists and this API would be nice, and somewhat make this concern go away — but I’d rather have the API definition clear now; the dir: support can be easily added later.]

@runcom
Copy link
Member Author

runcom commented Jun 29, 2016

ok to s/reference/digest in GetManifest

BTW, getting a manifest by digest - given an image repo - is ok and it's this works in Docker as well, you setup a repo and then call get_manifest on a repo given a digest (which can be a tag)

A philosophical/bikeshedding question: “whether a manifest list is a manifest”: do I get a manifest list through GetManifest and then call GetManifest for a specific manifest out of that list, or would there be a GetManifestList without that digest parameter? Perhaps we can start with the GetManifest until/unless there is a clear need to distinguish the two, but having the thinking and design clear would be nice.

I say GetManifest and then GetManifest for a specific manifest in the list (what Docker does also)

Not sure about dir, but yeah - I'd really love to have manifest list there as well since there's only one tool which started to support manifest list [1] [which was forked from skopeo FWIW] and I would them supported here as well

[1] https://github.com/estesp/manifest-tool

@runcom runcom changed the title types: ImageSource: support getting a specific manifest [RFE] types: ImageSource: support getting a specific manifest Jul 1, 2016
@runcom runcom changed the title [RFE] types: ImageSource: support getting a specific manifest [RFC] types: ImageSource: support getting a specific manifest Jul 1, 2016
@mtrmac
Copy link
Collaborator

mtrmac commented Aug 17, 2016

FWIW https://github.com/mtrmac/image/tree/docker-lookaside really needs the identity of the manifest to be constant across all uses of an ImageSource; in that branch, that means that requestedManifestMIMETypes must be set at ImageSource initialization, not later. Similar concerns would apply here.

Perhaps we could do this with an explicit GetManifestList, which either returns a real list or indicates that only a single manifest is actually available. Callers then would have to choose one manifest they want to use from the source, and commit to it, before being able to download manifests or signatures.

Or we could have a ManifestListSource and ImageSource as separate concepts.

@mtrmac
Copy link
Collaborator

mtrmac commented Oct 31, 2016

Isn’t this made unnecessary by #115 ?

@runcom
Copy link
Member Author

runcom commented Oct 31, 2016

Yep

@runcom runcom closed this Oct 31, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants