Skip to content

Add cross-repository mounting section to spec/test#204

Merged
jdolitsky merged 3 commits intoopencontainers:masterfrom
bloodorangeio:cross-mount
Dec 1, 2020
Merged

Add cross-repository mounting section to spec/test#204
jdolitsky merged 3 commits intoopencontainers:masterfrom
bloodorangeio:cross-mount

Conversation

@pmengelbert
Copy link
Copy Markdown
Contributor

  • New test in 02_push_test.go
  • Section added to spec.md detailing cross-repository mounting
  • Section added to conformance/README.md on new configuration options
    for cross-repository mounting

@pmengelbert pmengelbert force-pushed the cross-mount branch 2 times, most recently from b910554 to ff20463 Compare October 27, 2020 21:43
Copy link
Copy Markdown
Contributor

@jdolitsky jdolitsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Tested against Docker distribution

In this case, `<name>` is the namespace to which the blob will be mounted. `<digest>` is the digest of the blob to mount,
and `<other_namespace>` is the namespace from which the blob should be mounted.

The response to a successful mount MUST be `201 Created`, and MUST contain the following header:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be worth mentioning that this is normally done in place of the normal call to .../uploads/ to start a blob upload. So if a registry does not support cross repository mount, it will return a normal 202 and the client will continue the upload (not reissue the request without the mount param).

Did we have 201 listed as valid return code for /uploads without mount for a reason? Or was that just listed because of this. We should be explicit about the difference between 201 and 202 here.

Copy link
Copy Markdown
Contributor Author

@pmengelbert pmengelbert Oct 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dmcgowan based on yesterday's call, we have decided to require cross-repository mount, making the distinction between 201 and 202 unnecessary here -- it will always be 201 unless the source blob is not found, in which case it will be 404.

I have pushed a new commit removing 405 as an acceptable status code, thus requiring cross-repository implementation.

Allowing both 201 and 202 on /uploads was a compromise solution based on what existing registries were already doing -- see #171 and @jdolitsky 's comment on #68

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've written several clients that rely on the fact that 201 means the mount succeeded and 202 means continue with the upload -- ideally registry behavior would be consistent here. I don't think requiring cross-repo mount invalidates the requirement of this distinction.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unless the source blob is not found, in which case it will be 404

This would also slow down uploads because you'd require an additional roundtrip to start the upload process if the mount fails.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jonjohnsonjr This is my proposed solution:
In the first test, allow either a 201 or 202. If 201, a fetch for the blob will be tested to ensure its existence. If 202, we will verify that a valid <session-id> is returned.

If I'm getting it right, this should satisfy the requirements -- please let me know if there's anything further I should know about before making changes.

@Wwwsylvia
Copy link
Copy Markdown

Wwwsylvia commented Oct 30, 2020

Tested against ACR. LGTM.

@pmengelbert
Copy link
Copy Markdown
Contributor Author

@jonjohnsonjr @dmcgowan I just pushed the requested changes. Please review, and feel free to approve if all looks good. Thanks!

* New test in 02_push_test.go
* Section added to spec.md detailing cross-repository mounting
* Section added to conformance/README.md on new configuration options
for cross-repository mounting

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>
Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>
Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>
@jdolitsky jdolitsky merged commit 55ebd32 into opencontainers:master Dec 1, 2020
@jdolitsky jdolitsky mentioned this pull request Mar 24, 2021
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

Successfully merging this pull request may close these issues.

5 participants