Automate conformance testing on PR using zot

[jdolitsky]

Looks like its not quite passing yet, so made it still pass the test if conformance fails

Ran 63 of 68 Specs in 0.111 seconds
FAIL! -- 60 Passed | 3 Failed | 0 Pending | 5 Skipped

[jdolitsky]

cc @rchincha

[rchincha]

@jdolitsky Taking a look at these failures.

[eusebiu-constantin-petu-dbk]

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

[rchincha]

@peusebiu pls test with https://github.com/project-zot/zot/blob/main/examples/config-conformance.json. Your observations still valid?

[eusebiu-constantin-petu-dbk]

@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

[rchincha]

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.

[eusebiu-constantin-petu-dbk]

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.

[jdolitsky]

added this change to allow for missing "bytes=" prefix

[jdolitsky]

chatting with @rchincha - looks like this is required: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Range

working on zot version with this fix

[rchincha]

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Range

<unit> doesn't look optional, so this appears to be a zot bug and addressed in project-zot/zot#1341

[rchincha]

That said, #203 (comment)

[sudo-bmitch]

To follow the robustness principle, I believe we need to be strict in what registries output here, only allowing the output without bytes=. Registries and clients can be liberal in what they accept from each other, but for conformance, their output should follow the distribution-spec.

[jdolitsky]

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.

[jdolitsky]

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 with 0.
It MUST match the following regular expression:

^[0-9]+-[0-9]+$

[jonjohnsonjr]

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 :/

[jdolitsky]

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?

[rchincha]

Do we want to switch to the following which would allow both?

Would do only one, even if doesn't follow the rfc :(

lgtm

[jdolitsky]

As discussed on the call, switched to allow either format for now

[rchincha]

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.

[rchincha]

lgtm