Make Docker-Content-Digest non-optional #595
Conversation
syed
requested review from
dmcgowan,
jdolitsky,
jonjohnsonjr,
jzelinskie,
mikebrow,
stevvooe and
sudo-bmitch
as code owners
syed
force-pushed
the
head-requests-digest
branch
5 times, most recently
from
7253851 to
198bc28
Compare
sudo-bmitch
left a comment
sudo-bmitch
left a comment
There was a problem hiding this comment.
Given the goal from our conversation is to make it possible to verify the digest of a tag to attach referrers and pin references, I'm also looking at other parts of the spec. Specifically, in a few places we have:
The
Docker-Content-Digestheader, if present on the response, returns the canonical digest ...
Our usage of "canonical digest" references sha256, so I think the language there should be adjusted to reference the pushed digest when available. Though perhaps we want to save that change for once we have a way to push a tagged manifest with a different digest algorithm.
spec.md
Outdated
| A successful response SHOULD contain the digest of the uploaded blob or manifest in the header `Docker-Content-Digest`. | ||
| A successful response SHOULD contain the size in bytes of the uploaded blob or manifest in the header `Content-Length`. |
There was a problem hiding this comment.
Would the community consider changing this to a MUST? It would only apply to newer versions of the spec, so if we did that, I'd also want an implementers note for clients warning that older versions of the spec did not require the header.
There was a problem hiding this comment.
I'm not opposed to changing it to a MUST
There was a problem hiding this comment.
what do you think about the language around the GET. I believe we should also change this to a MUST to be consistent
A GET request to an existing manifest URL MUST provide the expected manifest, with a response code that MUST be `200 OK`.
A successful response SHOULD contain the digest of the uploaded blob in the header `Docker-Content-Digest`.
The `Docker-Content-Digest` header, if present on the response, returns the canonical digest of the uploaded blob which MAY differ from the provided digest.
There was a problem hiding this comment.
I agree that it would need to be changed throughout to be consistent. Lets get some other maintainers to weigh in before updating the PR.
There was a problem hiding this comment.
I think SHOULD is strong enough for the case where the client only expects sha256 default digests, but we need to explain the digest issue in the specification and how/why it MUST be there in the case where the digest is not, for any reason, already known by the client.
mikebrow
left a comment
mikebrow
left a comment
There was a problem hiding this comment.
for the purposes of documenting the issue outside the call..
The originally added text "These headers are OPTIONAL and clients SHOULD NOT depend on them." Appears to have been written in error and only meant to cover the version header and was not meant for the digest.
see suggested changes.
spec.md
Outdated
| A successful response SHOULD contain the digest of the uploaded blob or manifest in the header `Docker-Content-Digest`. | ||
| A successful response SHOULD contain the size in bytes of the uploaded blob or manifest in the header `Content-Length`. |
There was a problem hiding this comment.
I think SHOULD is strong enough for the case where the client only expects sha256 default digests, but we need to explain the digest issue in the specification and how/why it MUST be there in the case where the digest is not, for any reason, already known by the client.
sudo-bmitch
left a comment
sudo-bmitch
left a comment
There was a problem hiding this comment.
Overall, the SHOULD -> MUST changes LGTM. I don't think we ever define "canonical digest" anywhere, perhaps we should have an entry in the definitions.
I am thinking of removing the word |
sudo-bmitch
left a comment
sudo-bmitch
left a comment
There was a problem hiding this comment.
One line needs to get added back. And we need to squash the commits. After that, it LGTM.
|
Thanks. Please squash the commits and I'll give it my 👍🏼 . |
- Change Docker-Content-Digest from SHOULD to MUST for GET requests on manifests and blobs - Change Docker-Content-Digest and Content-Length from SHOULD to MUST for HEAD requests on manifests and blobs - Add implementers note for clients encountering older registries that may not include the header, suggesting sha256 as a reasonable default - Remove "canonical" qualifier from digest language throughout spec - Update legacy Docker support section to reference spec documentation
syed
force-pushed
the
head-requests-digest
branch
from
08b1331 to
8056120
Compare
|
squashed |
Looks like the DCO signoff got lost (or maybe wasn't there). |
5 participants
Based on the discussion on the call, we want to have
Docker-Content-Digestas a required header when doing a HEAD on a reference. This fixes an issue where given a tag for a manifest, there is no way currently for the client to know which hashing algorithm was used when pushing