spec: HEAD blob response should include content-length header#482
Conversation
ce02770 to
efb0e36
Compare
Signed-off-by: wayne warren <wayne.warren.s@gmail.com>
I think this should be treated as a bug in Docker since it knows the digest and size of the blob it just pushed. They shouldn't need to depend on the registry for this value.
I think that's also a bug in Docker, since we do not specify size as
It's been in the image-spec as a required field so I don't think
This looks like a good change to me. I suspect it's the current behavior in most, if not all well know registries (I did a spot check on distribution, zot, and quay). I think we also want it on the GET request, but there the HTTP RFCs may cover us, and clients have the content to check the actual length of the body. Checking for how this was handled for HEAD requests, it looks like the RFCs specifically call out that implementations may exclude that header: |
My argument was for anyone who is building a descriptor, The fundamental problem here is that golang just doesn't have a strong enough type system to cleanly express the difference between a default/zero value for a type and that type being entirely optional (my recollection of kubernetes api libraries was that they use references to numeric types to express set vs unset values, but that's gross in its own right).
Also, for what it's worth I think Docker is using this type at least in some places rather than defining their own.
I thought this was probably invalid/bad when I was looking at it yesterday morning: https://github.com/distribution/distribution/blob/c78d6f99ae5505e4328a323221ad012eacdf0754/registry/handlers/blob.go#L35-L36 🙃 |
After writing this I thought to myself there might be some hidden/implicit handling of content body going on behind the scenes and sure enough, there is: https://cs.opensource.google/go/go/+/refs/tags/go1.21.3:src/net/http/fs.go;l=352 ...so I stand corrected. |
https://explore.ggcr.dev/?image=tianon/scratch:zerobytes 👀 (It's technically valid and has a well known digest, but not a MUST in the spec so not totally supported across all registries, and has been the subject of many previous heated debates in the OCI.) |
|
Well shoot, I stand corrected again :). |
|
I was wondering to what extent the spec should describe Specifying that this header "SHOULD" be included on its own probably don't fix much, a that doesn't make it required, so clients wouldn't be able to depend on its being there (and still require alternatives). |
I'd be a strong proponent of that, and something I'm considering for a significant redesign of the spec (after a 1.1 release).
Clients shouldn't depend on it, but since the HTTP RFC's specifically call out this field as something that MAY be excluded, this felt like a hint to implementers to say it SHOULD be included. |
To the extent that in order for registries to be compatible with a major client implementation they really do need to include it.
The alternative to including this in the spec is that the next person to work on a registry implementation has to go through the same pain as I went through last week very tediously inferring the actual behavior of the client (and the client code itself is too disorganized, spread across multiple repos, to really understand what it's doing).
We could specify that clients SHOULD NOT depend on it being there and clarify that the reason for registries to implement it is that there are popular clients in the wild that do depend on it (even if we fix those clients moving forward, older versions will continue to persist pretty far into the future). |
My distribution implementation recently began passing all conformance tests, so I began testing my registry by pushing images using docker. My registry implementation happens to strictly adhere to the OCI image spec in terms of required fields, including on the OCI descriptor.
So when docker tried pushing image manifests without the
sizefield in the config descriptor, my registry returned aMANIFEST_INVALIDerror in its response.I submitted a couple issues to moby:
sizefield in manifest moby/buildkit#4328And after digging in for a while, I figured out what was going on.
Just before pushing a manifest docker issues a
HEAD PUT /v2/<repository>/blobs/<digest>request for the manifest config blob and seems to use thecontent-lengthheader in the response to that request to set the config descriptor size field.If there is no
content-lengthheader in the response, the field presumably gets set to 0. In combination with theomitemptytag on the Descriptor type's size field and the fact that whatever json marshalling library is being used considers 0 to be empty/unset, when docker serializes the Descriptor struct it omits the size field entirely.As weird as it is for me to say this given how much time I wasted on this, I think the
omitemptybehavior is right for theDescriptortype, otherwise my registry would have unknowingly accepted manifests with incorrectly 0-valued descriptor size fields. It wouldn't help registries implemented in weaker-typed languages to remove theomitemptybecause those languages lack the ability to effectively express the difference between optional and required values anyway (not impossible, just harder to do than in a strongly-typed language).So because there is a major distribution client with this behavior I am proposing that we update the spec to suggest that it's a good idea to set the
content-lengthfield. I might even go as far as to say it should be aMUSTrather than aSHOULD, but I suspect folks might balk at that.