Closed jdolitsky closed 1 year ago
cc @rchincha
@jdolitsky Taking a look at these failures.
Hello, I've been debugging this, here is the summary: 1) test failure
Get on stale blob upload should return 204 with a range and location [It]
/home/runner/work/distribution-spec/distribution-spec/conformance/02_push_test.go:216
Expected
<string>: 0-21
to equal
<string>: bytes=0-21
This one is pretty straightforward, it happens because our GetBlobUpload doesn't return "bytes=" part in Range header.
2) test failure
POST request to mount another repository's blob should return 201 or 202 [It]
/home/runner/work/distribution-spec/distribution-spec/conformance/02_push_test.go:268
Expected
<string>: "...lobs/upload..."
to equal |
<string>: "...lobs/sha256..."
This one assumes that the registry will do a cross-repo mount. In this case, zot will not mount the blob because it has dedupe disabled. (zot doesn't support cross-repo mounts but it uses its dedupe cache to work around it)
Looking at the test https://github.com/opencontainers/distribution-spec/blob/main/conformance/02_push_test.go#L276 it expects either a 201 or a 202 response (cross mount happened or not), but afterwards it expects the Location to point to the cross mounted blob, but in our case(zot didn't mount the blob) it will return a new sessionID instead with a 202 status code, which is acceptable from the spec point of view, but the test fails.
From my understanding the test should be fixed(more below), but I may be wrong.
3) test failure
Cross-mounting of nonexistent blob should yield session id [It]
/home/runner/work/distribution-spec/distribution-spec/conformance/02_push_test.go:295
Expected
<string>: /v2/myorg/myrepo/blobs/uploads/910c4c1b-2917-4a9c-8cde-5e05f34766f3
to have prefix
<string>: /v2/conformance-ca3bc013-07bb-4e9e-8a0b-9f3ff724d59e/blobs/uploads/
This one is using the http response from test 2) https://github.com/opencontainers/distribution-spec/blob/main/conformance/02_push_test.go#L297
But in this case, because 2) failed, I think that lastResponse doesn't get updated, and it checks a previous response. So this one fails because 2) fails.
As a side effect, both 2) and 3) passes if we use dedupe true(because then cross mount will work), just a a side note.
From my point of view this would be a fix, moving the last assertion to the next test:
diff --git a/conformance/02_push_test.go b/conformance/02_push_test.go
index fa3eca3..12d9cdf 100644
--- a/conformance/02_push_test.go
+++ b/conformance/02_push_test.go
@@ -277,7 +277,6 @@ var test02Push = func() {
Equal(http.StatusCreated),
Equal(http.StatusAccepted),
))
- Expect(resp.GetRelativeLocation()).To(Equal(fmt.Sprintf("/v2/%s/blobs/%s", crossmountNamespace, testBlobADigest)))
lastResponse = resp
})
@@ -290,6 +289,8 @@ var test02Push = func() {
resp, err := client.Do(req)
Expect(err).To(BeNil())
Expect(resp.StatusCode()).To(Equal(http.StatusOK))
+
+ Expect(resp.GetRelativeLocation()).To(Equal(fmt.Sprintf("/v2/%s/blobs/%s", crossmountNamespace, testBlobADigest)))
})
g.Specify("Cross-mounting of nonexistent blob should yield session id", func() {
Basically the test SHOULD:
Expect a blob session id /v2/<name>/blobs/uploads/<session-id>
when registry returns 202
Expect blob location /v2/<name>/blobs/<digest>
when registry returns 201
according to this: https://github.com/opencontainers/distribution-spec/blob/main/spec.md?plain=1#L437
Currently, in the case of 202 response, the test expects both a sessionID Location and a blob Location which doesn't make sense: 1) one here: https://github.com/opencontainers/distribution-spec/blob/main/conformance/02_push_test.go#L280 2) another one here: https://github.com/opencontainers/distribution-spec/blob/main/conformance/02_push_test.go#L299
@peusebiu pls test with https://github.com/project-zot/zot/blob/main/examples/config-conformance.json. Your observations still valid?
@peusebiu pls test with https://github.com/project-zot/zot/blob/main/examples/config-conformance.json. Your observations still valid?
yes, I used that without any modification
The registry MAY treat the from parameter as optional, and it MAY cross-mount the blob if it can be found.
Alternatively, if a registry does not support cross-repository mounting or is unable to mount the requested blob, it SHOULD return a 202. This indicates that the upload session has begun and that the client MAY proceed with the upload.
Hello,
In the cross mount blob test you expect one of two responses:
Expect(resp.StatusCode()).To(SatisfyAny(
Equal(http.StatusCreated),
Equal(http.StatusAccepted),
))
so far so good, this meets the dist-spec standard.
But then, on the next line, you expect a blob to be mounted:
Expect(resp.GetRelativeLocation()).To(Equal(fmt.Sprintf("/v2/%s/blobs/%s", crossmountNamespace, testBlobADigest)))
This line ^ should be run only if the blob was mounted, if registry couldn't mount the blob, or doesn't support cross-mounts, then it should start an upload session and return its location (according to dist-spec).
Move the above line here: https://github.com/opencontainers/distribution-spec/blob/main/conformance/02_push_test.go#L288
to correctly run it only if the blob was mount.
As it is now, this test can not pass on any dist-spec implementation.
A location can not be both equal with:
fmt.Sprintf("/v2/%s/blobs/%s", crossmountNamespace, testBlobADigest)
and
fmt.Sprintf("/v2/%s/blobs/uploads/", crossmountNamespace)
as the test implies.
After opening up all the Test Reports > Blob Upload Chunked here: https://conformance.opencontainers.org/
it looks like every registry provides the Range
header missing the bytes=
prefix. Looks like this was added in #366 cc @sudo-bmitch. Reverting that change in this PR.
It looks like the spec actually covers this:
The
<range>
refers to the byte range of the chunk, and MUST be inclusive on both ends. The first chunk's range MUST begin with0
. It MUST match the following regular expression:^[0-9]+-[0-9]+$
it looks like every registry provides the Range header missing the bytes= prefix.
It would be great if we didn't flout the HTTP RFCs like this -- ideally we fix this, but I'm ok if we allow both for legacy reasons. I'm not sure how clients would handle it :/
It would be great if we didn't flout the HTTP RFCs like this -- ideally we fix this, but I'm ok if we allow both for legacy reasons. I'm not sure how clients would handle it :/
Yes, I'm not sure how clients would handle it either.. do not know where/how to start testing that.
Do we want to switch to the following which would allow both?
Expect(resp.Header().Get("Range")).To(SatisfyAny(
Equal(testBlobBChunk1Range), // Allow missing "bytes=" prefix
Equal(fmt.Sprintf("bytes=%s", testBlobBChunk1Range)),
))
Does this risk a registry claiming conformance, providing the prefix and some client breaking somewhere?
Do we want to switch to the following which would allow both?
Would do only one, even if doesn't follow the rfc :(
lgtm
As discussed on the call, switched to allow either format for now
If popular clients don't break (most likely they are not using chunked uploads at all), considering to make the change in zot to do the right thing per HTTP rfc. We have had non-cloud embedded clients requiring the HTTP rfc behavior.
Your "SatisfyAny()" clause should still work.
Looks like its not quite passing yet, so made it still pass the test if conformance fails