Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remaining issues pre v1.0.0-rc2 (Accept headers, etc.) #211

Open
jdolitsky opened this issue Nov 11, 2020 · 7 comments
Open

Remaining issues pre v1.0.0-rc2 (Accept headers, etc.) #211

jdolitsky opened this issue Nov 11, 2020 · 7 comments
Labels

Comments

@jdolitsky
Copy link
Member

Here a re a list of issues that came up in today's call (11/11/2020):

  • Accept headers
  • Show examples of types of manifests (OCI index, etc.)
    • need to see example of what this might look like, not as a requirement
  • unclear manifest vs. blob, what goes where
  • existence check of a blob (under Push section)
  • Show "Pushing a blob in chunks" first since they are more broadly supported
  • "A single POST request" should possibly be removed
  • Content-Type: application/vnd.oci.image.manifest.v1+json does not include what is required for indexes
  • "The uploaded manifest MUST reference any blobs that make up the artifact. However, the list of blobs MAY be empty."
    • For an image, this must reference one blob
    • Respect for "non-distributable" layers
  • " The is a pullable manifest URL."
    • what is pullable manifest URL?
    • do clients actually provide this header?

cc @jonjohnsonjr

@jdolitsky jdolitsky added this to the v1.0.0-rc2 milestone Nov 11, 2020
@jonjohnsonjr
Copy link
Contributor

Thanks for taking notes, I will circle back with more fleshed out details.

@jonjohnsonjr
Copy link
Contributor

jonjohnsonjr commented Nov 12, 2020

  1. Accept headers
    1. Covered by Add HEAD endpoints #207 (comment) and Proposal: add (optional) section on content-negotiation #212 in extraordinary detail. We need to include something about this in v1.0 because this is how registries currently behave, and clients need to understand what Accept headers to send if they want to successfully pull an image.
    2. We should probably link to https://docs.docker.com/registry/spec/manifest-v2-2/#backward-compatibility in a backwards compatibility section or something.
    3. If we're not going to have clients supply Accept headers, then that means every clients needs to support both manifest lists and images, which ties into my next point...
  2. Examples of types of manifests
    1. IMO, we should at least link to the manifest and image index sections of image-spec. These are what clients are likely to encounter, and they really do need to support both of these to support multi-platform images. @dmcgowan suggested just including these as examples.
    2. We should probably also link to media types in the image-spec somewhere, since registries and clients should both expect to see the equivalent docker media types.
  3. unclear manifest vs. blob
    1. Perhaps in some other section, include historical information about why /manifests/ and /blobs/ are different, and which media types are expected to be uploaded/downloaded to/from which endpoints. This has caused confusion in the past.
  4. existence check of a blob
    1. This should be the first thing most clients do before attempting to upload a blob, so we need to include it, and it should probably be the first section encountered under uploading blobs.
  5. Show "Pushing a blob in chunks" first since they are more broadly supported
    1. The POST/PATCH/PUT chunked uploading strategy is supported by every registry I've encountered (probably because that's what docker did), so we've modified our clients to only use this flow. Monolithic support may have caught up in the meantime, but as of a couple years ago, there were several smaller registries that did not support monolithic blob uploads.
    2. I believe containerd uses the POST/PUT monolithic upload strategy, so that is probably also widely supported (though not as widely as chunked).
  6. "A single POST request" should possibly be removed
    1. In the original spec, this method of blob upload was only barely mentioned as an optional thing for the blob upload initiation: Optionally, if the digest parameter is present, the request body will be used to complete the upload in a single request. I worry that not a lot of registries support this.
    2. This is going to be the most expensive blob upload method to implement for registries because there is no opportunity for the registry to offload the byte ingestion to a separate service (via the Location header, as in the other methods).
    3. I'd argue for removing it entirely, but would be happy if there were some caveats around its use. Derek mentioned maybe we could add something about "optimizations" like this, maybe in the FAQ from vsoch. There's no discussion in the spec about why you would choose one of these methods over another, which could go in that FAQ. No matter what we decide, this should definitely not be the first upload method that readers encounter.
  7. Content-Type: application/vnd.oci.image.manifest.v1+json does not include what is required for indexes
    1. Pretty self explanatory, but we should be consistent here. If we're going to include examples, that's fine. We should include both here. If we're just going to punt to the image-spec, that's fine, we should link to it and say something like "the media type of the manifest".
  8. "The uploaded manifest MUST reference any blobs that make up the artifact. However, the list of blobs MAY be empty."
    1. To elaborate here, the image-spec lists the config blob as REQUIRED for a manifest, so an image will absolutely reference at least one blob. It seems that layers can be empty, but it's really strange to actually call this out in the spec, given that it's in the image-spec and doesn't seem to really clarify anything to the reader.
    2. If we're going to mention this, we should at least explain why the manifest MUST reference blobs that make up an artifact -- to prevent registries from performing garbage collection on blobs that are unreferenced by manifests. This ties into the earlier blobs vs manifests discussion as well: there are different expectations around GC of content based on whether it's uploaded to /manifests/ or /blobs/. We can't be super prescriptive here, either, because some registries GC untagged images, and some don't. Some GC unreferenced blobs, and some don't. Perhaps this goes in some implementer's notes or FAQ with a link to it inline.
    3. Again, if we're going to mention this, we should at least link to non-distributable layers somewhere around here or in a section about GC. Both clients are registries need to know that they should handle them differently from other blobs.
  9. " The is a pullable manifest URL."
    1. This wasn't in the original spec, and I'm not sure what "pullable manifest URL" means.
    2. It seemed like this was something from HTTP around 201 response codes, looking at https://tools.ietf.org/html/rfc2616#section-10.2.2:

The request has been fulfilled and resulted in a new resource being
created. The newly created resource can be referenced by the URI(s)
returned in the entity of the response, with the most specific URI
for the resource given by a Location header field. The response
SHOULD include an entity containing a list of resource
characteristics and location(s) from which the user or user agent can
choose the one most appropriate. The entity format is specified by
the media type given in the Content-Type header field. The origin
server MUST create the resource before returning the 201 status code.

Compare to our language:

Upon a successful upload, the registry MUST return response code 201 Created, and MUST have the following header:
Location: <location>
The <location> is a pullable manifest URL.

It seems pretty obvious to me that this header isn't actually necessary. There is a well-known URL for fetching a manifest from the registry API. If a registry wants to returns a separate location (to, say, object storage), that seems fine to me, but is this really a MUST?

Do any registries provide this?

Do any clients look for it?

@jonjohnsonjr
Copy link
Contributor

I will try to send a PR for some of these if I can find the time, but I understand that you want to cut 1.0 pretty soon, so I may move too slowly for that.

These are also just my own opinions, if other maintainers disagree, I'm happy to concede any or all of these points.

@jdolitsky
Copy link
Member Author

@jonjohnsonjr thanks for descriptions. PRs appreciated, but will attempt to cover all the relevant points prior to 1.0

@pmengelbert
Copy link
Contributor

pmengelbert commented Nov 20, 2020

@jonjohnsonjr Specifically, can you put together a PR for items 1 and 2 (Accept headers and links to image spec, etc). You have a great deal of experience with this and I imagine you can put together the language in about 20 minutes, which would save a lot of time vs. me doing my best first attempt and inevitably missing something and going back and forth on it.

If you aren't able to submit one, please let us know and we'll put something together early next week. In the meantime, we're working on items 3-5. Thanks!

Can anyone else volunteer to tackle one or two of the remaining items in a PR? We're looking to have a solid candidate for v1 in early December.

@jdolitsky jdolitsky changed the title Remaining issues pre v1.0.0-rc2 Remaining issues pre v1.0.0-rc2 (Accept headers, etc.) Dec 9, 2020
@jdolitsky
Copy link
Member Author

@jonjohnsonjr - please see https://github.com/opencontainers/distribution-spec/pull/218/files as a simple note to address #1. Searched through the old spec and did not find much detail on the use of Accept header. This was adapted from that

@jdolitsky
Copy link
Member Author

Going to pull from milestone as #218 addresses the Accept header (as a temporary solution)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants