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

vendor-specific strings in HTTP headers #26

Closed
dongsupark opened this issue Sep 14, 2018 · 30 comments
Closed

vendor-specific strings in HTTP headers #26

dongsupark opened this issue Sep 14, 2018 · 30 comments
Milestone

Comments

@dongsupark
Copy link
Contributor

The current distribution-spec contains vendor-specific strings like Docker in HTTP headers. Though it's not clear to me what we should do about it, because simple string substitutions would be incompatible changes. So I'm simply listing them below, without creating a PR.

@mikebrow
Copy link
Member

mikebrow commented Sep 14, 2018

I suggest one of two paths:

  1. Indicate in the specification that to support legacy implementations of the distribution spec the following http headers MUST be supported as alias names for the corresponding OCI header names:
    Docker-Content-Digest == OCI-Content-Digest
    ...

  2. State somewhere in the specification that in order to support current implementations of docker registries headers named Docker-* are considered herein to be OCI-* headers.

@bsatlas
Copy link
Contributor

bsatlas commented Nov 24, 2018

@mikebrow I like option 1.

What about the Docker-Distribution-Api-Version header? Currently it SHOULD be set to registry/2.0 but it'd personally like to see something with semantic versioning.

@bsatlas
Copy link
Contributor

bsatlas commented Nov 24, 2018

Below is a table of the Docker headers mapped to OCI:

Docker OCI
Docker-Distribution-Api-Version OCI-Distribution-Version
Docker-Content-Digest OCI-Content-Digest
Docker-Upload-UUID OCI-Upload-ID
OCI-Implementation

I think it would be a good idea to split Docker-Distribution-Api-Version into two separate headers. OCI-Distribution-Version contains the semantic version of the distribution spec being used and OCI-Implementation contains the name and semantic version of the server implementation. Server implementation info in a separate header allows a standardized way for clients to know how to interact with the server's endpoints that are out of scope.

Examples

  • OCI-Distribution-Version: v0.1.0
  • OCI-Content-Digest: <digest>
  • OCI-Upload-ID: <unique id> (Issue #13)
  • OCI-Implementation: aws-ecr/v2.3.5

@dongsupark
Copy link
Contributor Author

@atlaskerr It sounds like a good idea. 👍
I also prefer the option 1, supporting legacy headers as alias, as @mikebrow mentioned.

Question. Would it be possible to change so before the distribution-spec release v1.0?

@bsatlas
Copy link
Contributor

bsatlas commented Nov 26, 2018

@dongsupark That's a question for the maintainers. I would love to see this change made before the v1.0 though.

@bsatlas
Copy link
Contributor

bsatlas commented Nov 30, 2018

After thinking about it, if v1.0 is all about standardizing the current spec, we should keep the headers and switch to OCI headers in v1.1.

The OpenAPI spec did the same thing. The first version of the spec allowed the top object name to be swagger to maintain backwards-compatibility with the OG swagger spec and then changed it to openapi for every release after that.

@dmcgowan dmcgowan added this to the v1.0 RC1 milestone Jan 16, 2019
@dmcgowan
Copy link
Member

Docker-Content-Digest must remain as required, the others we can make optional as well as add an OCI optional value. A PR for the upload ID is already done, we should figure out something for OCI-Distribution-Version or something though. Adding this to the first RC milestone.

@stevvooe
Copy link
Contributor

personally like to see something with semantic versioning.

This isn't really the place to have semantic versioning. To actual enforce semantic version, you drop the patch number so that people don't break semver by using the patch version. Basically, you only expose as much of the version as you want clients to switch and you get actual semantic versioning. Really, you should only expose the major.

But let me be certainly clear: changing names to satisfy some sense of "Vendor Neutral-ness" isn't going to help with the adoption of the specification. 🤗

Ultimately, we should drop OCI-Distribution-Version and it's forbearer.

@bsatlas
Copy link
Contributor

bsatlas commented Jan 17, 2019

But let me be certainly clear: changing names to satisfy some sense of "Vendor Neutral-ness" isn't going to help with the adoption of the specification.

@stevvooe I agree. After working with the spec for a bit I'm beginning to see the error in my ways haha

@mikebrow
Copy link
Member

A PR for the upload ID is already done

That PR was closed.. so let's consider this still unresolved.

@mikebrow
Copy link
Member

Have to agree with @stevvooe "changing names to satisfy some sense of "Vendor Neutral-ness" isn't going to help with the adoption of the specification."

So to modify option 1 from above:

  1. Indicate in the specification (via table?) that to support legacy implementations of the distribution spec the following http headers MUST be supported. The alias names for the corresponding OCI header names, as listed, are OPTIONAL for this release. In a future release of this specification the OCI header names MAY become required.

@mikebrow
Copy link
Member

mikebrow commented Jan 25, 2019

Ultimately, we should drop OCI-Distribution-Version and it's forbearer.

Agreed, let's discuss this on the call and see if there is any desire to replace it or just drop for now. Maybe leaving some optional wording (legacy version MAY be implemented) or even a comment regarding why it was left out and what our recommendations are regarding versioning for this and future releases.

somewhat related discussion regarding version prefixes here: #28

@bsatlas
Copy link
Contributor

bsatlas commented Jan 25, 2019

Ultimately, we should drop OCI-Distribution-Version and it's forbearer.

I made a thread on the mailing list about that last week but it didn't get much traction:
https://groups.google.com/a/opencontainers.org/forum/#!topic/dev/I2P75y_j9hU

@mikebrow
Copy link
Member

didn't get much traction

@atlaskerr, might have been related to the advertising for your "image registry that aims to be the successor to the Docker registry." Pretty high aims there :-)

Cheers, Mike

@bsatlas
Copy link
Contributor

bsatlas commented Jan 25, 2019

@mikebrow I didn't mean for it to sound offensive. I'll take it down.

@mikebrow
Copy link
Member

No offense taken.. no need to take it down, or withdraw prs, or close issues,...

Give it some time, the maintainers tend to get busy with other projects, as we all do, and thus the distribution spec project has taken a little longer than expected to get going, when compared to other OCI projects.

It was important to disclose why this is important to you, helps us get to know you, ... all good :-)

@bsatlas
Copy link
Contributor

bsatlas commented Jan 28, 2019

I've reopened #29.

Let's plan for merging this PR this week after the Wednesday call once we hammer out the details.

@vbatts
Copy link
Member

vbatts commented Jan 30, 2019

per today's discussion,
for the /v2/_ping version header, this should be optional. and MAY have structured version information like version and potentially feature flags.

@jonjohnsonjr
Copy link
Contributor

@vbatts just /v2/, _ping was v1 😄

@dmcgowan
Copy link
Member

We also discussed with the Docker-Content-Digest header of not replacing it, but just adding it as SHOULD or MAY. The problem with this header today is that you cannot rely on it for security, it may only be used as a content checksum. Additionally, many registries have contain some sort of bug where there are cases this is not returned to the correct value. With that in mind, it would be best to encourage clients to start with a digest they expect to received (whether that is directly from a configuration, notary, or a future tags endpoint) and use that to do digest verification. When the client does not have a digest and the registry does not provide one, the client can must trust the registry reponse anyway, so it can just compute the digest and use that.

I guess all that is to say is we should just keep the vendor strings as legacy, but don't attempt to replace any of the ones we haven't already.

@bsatlas
Copy link
Contributor

bsatlas commented Jan 31, 2019

So adding the Docker-Content-Digest header in the response would be optional for registry implementations moving forward?

@mikebrow
Copy link
Member

hate doing legacy text in a new spec for something we don't want to be used :) But it is what it is. Maybe a "Legacy" highlight..

@dmcgowan
Copy link
Member

Looking through the client code there is a case where this header is very valuable. Having this header will allow clients to get the content digest via a HEAD request. That said, yes, it is optional, but we should set it.

@mikebrow
Copy link
Member

mikebrow commented Jan 31, 2019

Sure.. no way to generate a digest without the body, and if you get the digest in the head request you can check it against some "expected" digest that may have been specified in a client pull request for example. Save some download time.

@dmcgowan
Copy link
Member

@mikebrow a common use case would be to HEAD the registry and compare the digest with what you already have. We also do this in containerd to create a descriptor then always doing a GET with the digest.

@dmcgowan
Copy link
Member

For the 1.1 release there is a plan/idea of adding a tags endpoint which should provide clients a better way to do tag<-->digest checking (ping @stevvooe ). With that in mind, I don't think it makes sense to replace Docker-Content-Digest with another header. We can keep that as optional legacy and provide a single better way to do that check in the future. In the end, we want to have only one suggested way to do something.

@vbatts vbatts modified the milestones: v1.0.0-rc.0, v1.0.0-rc1 Feb 14, 2019
@vbatts
Copy link
Member

vbatts commented Mar 6, 2019

@dongsupark i think this have been worked at, but could you review again to see how far it is from being closed?

@bsatlas
Copy link
Contributor

bsatlas commented Mar 6, 2019

At meeting, we concluded Docker-Content-Digest is the last thing that needs to be worked out in order to close this issue. This discussion has been centered around making that specific header optional.

@jzelinskie
Copy link
Member

As discussed on the call today, we'd like to include every Docker prefixed header as optional, and not replace any with OCI prefixed headers for 1.0.0.

The follow steps need to be done to close this issue:
Docker-Distribution-API-Version - needs to be documented that it is OPTIONAL
Docker-Content-Digest - needs to be made clear in the tables that it is OPTIONAL

@jdolitsky
Copy link
Member

Resolved by #206

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

No branches or pull requests

9 participants