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

content-negotiation: bring over the table from Sebastiaan #226

Merged
merged 1 commit into from
Jun 24, 2021

Conversation

vbatts
Copy link
Member

@vbatts vbatts commented Jan 28, 2021

From #212
Tagging @jonjohnsonjr and @thaJeztah, to wrap up that thread

I think this is missing expected status codes.

@vbatts
Copy link
Member Author

vbatts commented Jan 28, 2021

and

We should probably link to https://docs.docker.com/registry/spec/manifest-v2-2/#backward-compatibility in a backwards compatibility section or something.

@vbatts vbatts force-pushed the vbatts/content-table branch from 1b53d7a to b6c971b Compare January 28, 2021 19:41
Copy link
Member

@jdolitsky jdolitsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@vbatts
Copy link
Member Author

vbatts commented Jan 28, 2021

k. Updated a little. And with the reference to the docker docs, I wonder if it may be good to also copy-paste the paragraph here for posterity sake? @thaJeztah

@jonjohnsonjr
Copy link
Contributor

jonjohnsonjr commented Jan 30, 2021

I think we need something like this, but I also think this is something incomplete.

At one point I put something together to decide what kind of down-conversion GCR would do, copying over from a spreadsheet, I think this is what we settled on:

Schema requested Pushed as S1 Pushed as S2 Pushed as Manifest List Pushed as OCI Pushed as Image Index
S1 S2 -> S1 ML (-> S2) -> S1 x x
S2 x ML -> S2 x x
Manifest List x x x x
OCI x x x II -> OCI
Image Index x x x x

Diagonal is reading what you wrote, so it works fine (blank space).

An x represents an error -- GCR returns a 404 here, which is misaligned with the HTTP spec, I think.

Anything else describes how we down-convert for backward compatibility stuff.

We don't consider the ordering of accept headers, just "Do they accept what we have? If not do they accept anything we can convert to?"

There were other variants of what is possible, but we decided to be conservative, because mutating content-addressable stuff is... not a great idea. I think we require you to put OCI accept headers, for example, even though most clients would probably do just fine if we returned an OCI manifest when they accept schema 2 manifests. Alas.

I'd like to be explicit about what status codes should be returned by a registry in these situations.

ECR has a similar diagram: https://docs.aws.amazon.com/AmazonECR/latest/userguide/image-manifest-formats.html

Note that ECR will freely translate between OCI and Docker media types, which is different from what GCR does, mostly because this isn't in the spec 😄

Base automatically changed from master to main March 9, 2021 17:38
Copy link
Contributor

@stevvooe stevvooe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I think there are some other comments to address but this documents an interesting history of datatypes.

Copy link
Member

@dmcgowan dmcgowan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

sorry, not sure why I didn't see this one.

Would be good if someone could re-run the curl command for these combinations that I mentioned in #212

(to be sure I didn't mess up any of the combinations .. it's been a while 😂)

@vbatts
Copy link
Member Author

vbatts commented Jun 24, 2021

(not sure why pullapprove is only seeing 1 of the approvals, but that won't matter as we've moved away from pullapprove)

@vbatts vbatts merged commit ad6b078 into main Jun 24, 2021
@vbatts vbatts deleted the vbatts/content-table branch June 24, 2021 17:13
@jdolitsky jdolitsky mentioned this pull request Sep 15, 2022
@sudo-bmitch sudo-bmitch mentioned this pull request Feb 1, 2024
8 tasks
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.

6 participants