Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Dec 1, 2020

Conversation

pmengelbert
Copy link
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
Member

@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
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
Contributor Author

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

Wwwsylvia commented Oct 30, 2020

Tested against ACR. LGTM.

@pmengelbert
Copy link
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.

None yet

5 participants