Add support for layers from foreign sources by jstarks · Pull Request #1725 · distribution/distribution

jstarks · 2016-05-14T22:01:43Z

This will be used to support downloading Windows base layers from Microsoft URLs.

You can see this change in full context at jstarks/docker@27b81d4.

codecov-io · 2016-05-15T02:14:26Z

Current coverage is 50.24%

Merging #1725 into master will decrease coverage by 11.56%

2 files in manifest/schema2 were modified. more
- Misses +8
- Partials +1
- Hits -9
2 files (not in diff) in registry/middleware were deleted. more
12 files (not in diff) in registry/handlers were deleted. more
4 files (not in diff) in ...istry/storage/driver were modified. more
- Misses +811
- Partials -93
- Hits -718
14 files (not in diff) in registry/storage were modified. more
- Misses +36
- Partials +50
- Hits -86
7 files (not in diff) in registry/proxy were modified. more
- Misses +120
- Partials -3
- Hits -117
2 files (not in diff) in registry/client/auth were modified. more
- Misses +1
- Partials +3
- Hits -4
2 files (not in diff) in registry/client were modified. more
- Misses +56
- Partials +5
- Hits -61
2 files (not in diff) in registry/api/errcode were modified. more
- Misses +8
- Partials +1
- Hits -9
1 files (not in diff) in registry/api were modified. more
- Misses +2
- Partials +3
- Hits -5

@@             master      #1725   diff @@
==========================================
  Files           121        107     -14   
  Lines         10579       9352   -1227   
  Methods           0          0           
  Messages          0          0           
  Branches          0          0           
==========================================
- Hits           6537       4698   -1839   
- Misses         3436       4173    +737   
+ Partials        606        481    -125

Powered by Codecov. Last updated by 4a915d6...a3db037

jstarks · 2016-05-17T00:52:21Z

I've updated the PR with changes to successfully validate manifests with foreign layers. With this update and the referenced Docker changes, I am able to push/pull foreign layers to a private registry.

jstarks · 2016-05-18T21:56:03Z

Any feedback?

stevvooe · 2016-05-18T22:15:00Z

manifest/schema2/manifest.go

+type LayerDescriptor struct {
+	distribution.Descriptor
+	// ForeignURL contains the foreign source URL of this layer.
+	ForeignURL string `json:"foreign_url,omitempty"`


URLs []string to allow more that one URL. I don't think there is any reason to call this "foreign".

In general, this works because foreign and URL doesn't need to be combined. More precisely, the presence of URL shouldn't imply the foreign media type and vice versa.

Would you expect a client to try to try each of the URLs in order until one succeeded?

stevvooe · 2016-05-18T22:21:24Z

@jstarks Looks like a good start. There may be some additions required in registry/client. Most importantly, the relevant specification updates need to happen with this field addition.

jstarks · 2016-05-18T23:46:57Z

@stevvooe Thanks. I'll look at registry/client and the specs.

jstarks · 2016-05-19T00:08:25Z

@stevvooe For registry/client, were you thinking that it would be helpful to have a function that returns a ReadSeekCloser that uses URLs if they are set in the descriptor? Currently I have put this code in the Docker engine, but I could move it to registry/client so that it would be available to any client.

The tricky part is that currently blobs.Open() takes a Digest and not a Descriptor, so this would have to be an extension to the BlobProvider interface...

Thoughts?

stevvooe · 2016-05-19T00:22:08Z

@jstarks I see. Since we aren't really communicating with the registry, it may not make sense to bake this into the client. Really, there needs to be some component that parses a manifest and figures out the set of operations. Currently, we really do this in the engine, so it's likely the proposed break down is fine.

jstarks · 2016-05-20T01:46:37Z

I've pushed an update that moves URLs into distributionDescriptor and updates the 2.2 spec.

stevvooe · 2016-05-20T02:14:16Z

docs/spec/manifest-v2-2.md


+    - **`urls`** *array*
+
+        For an ordinary layer, this is empty, and the layer contents can be


Do we want to go more generic here?

cc @RichardScothern @aaronlehmann

stevvooe · 2016-05-20T02:15:03Z

LGTM

aaronlehmann · 2016-05-20T18:39:15Z

registry/storage/schema2manifesthandler.go

+				for _, u := range fsLayer.URLs {
+					pu, err := url.ParseRequestURI(u)
+					if err != nil || (pu.Scheme != "http" && pu.Scheme != "https") {
+						err = errors.New("invalid URL on layer")


pu, err := url.ParseRequestURI(u) creates a new err variable in the current scope, so when you assign to err here, you are not assigning the one you declare with var err error, and this error won't be caught by the check below.

It might be worth adding a simple unit test to make sure that valid URLs are accepted and invalid URLs, or URLs in the presence of the wrong mediatype, are rejected.

Whoops, good catch. This is by far the most common mistake I make in Go...

I'll add a test.

Fixed and test added.

This will be used to support downloading Windows base layers from Microsoft URLs. Signed-off-by: John Starks <jostarks@microsoft.com>

aaronlehmann · 2016-05-20T19:55:53Z

LGTM

Add support for layers from foreign sources

GordonTheTurtle added status/0-triage group/windows labels May 14, 2016

jstarks force-pushed the foreign_layer branch 2 times, most recently from fb843bb to 8191731 Compare May 15, 2016 02:01

jstarks force-pushed the foreign_layer branch from 8191731 to 8b4d72f Compare May 17, 2016 00:51

stevvooe reviewed May 18, 2016
View reviewed changes

jstarks force-pushed the foreign_layer branch from 8b4d72f to 5c5aa72 Compare May 20, 2016 01:45

jstarks force-pushed the foreign_layer branch from 5c5aa72 to 6a5fbc1 Compare May 20, 2016 02:10

stevvooe reviewed May 20, 2016
View reviewed changes

aaronlehmann reviewed May 20, 2016
View reviewed changes

Add support for layers from foreign sources

f0052b8

This will be used to support downloading Windows base layers from Microsoft URLs. Signed-off-by: John Starks <jostarks@microsoft.com>

jstarks force-pushed the foreign_layer branch from 6a5fbc1 to f0052b8 Compare May 20, 2016 19:31

aaronlehmann merged commit 5bbf654 into distribution:master May 20, 2016

RichardScothern added this to the Registry/2.5 milestone Jun 13, 2016

wking mentioned this pull request Jul 6, 2016

manifest: layer federation opencontainers/image-spec#169

Merged

runcom mentioned this pull request Jul 8, 2016

[Proposal] layers federation #1825

Open

2 tasks

thaJeztah pushed a commit to thaJeztah/distribution that referenced this pull request Apr 22, 2021

Merge pull request distribution#1725 from jstarks/foreign_layer

4654e99

Add support for layers from foreign sources


		- `urls` array

		For an ordinary layer, this is empty, and the layer contents can be

Conversation

jstarks commented May 14, 2016

Uh oh!

codecov-io commented May 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 50.24%

Uh oh!

jstarks commented May 17, 2016

Uh oh!

jstarks commented May 18, 2016

Uh oh!

stevvooe May 18, 2016

Choose a reason for hiding this comment

Uh oh!

stevvooe May 18, 2016

Choose a reason for hiding this comment

Uh oh!

jstarks May 18, 2016

Choose a reason for hiding this comment

Uh oh!

stevvooe commented May 18, 2016

Uh oh!

jstarks commented May 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jstarks commented May 19, 2016

Uh oh!

stevvooe commented May 19, 2016

Uh oh!

jstarks commented May 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stevvooe May 20, 2016

Choose a reason for hiding this comment

Uh oh!

stevvooe commented May 20, 2016

Uh oh!

aaronlehmann May 20, 2016

Choose a reason for hiding this comment

Uh oh!

jstarks May 20, 2016

Choose a reason for hiding this comment

Uh oh!

jstarks May 20, 2016

Choose a reason for hiding this comment

Uh oh!

aaronlehmann commented May 20, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-io commented May 15, 2016 •

edited

Loading

jstarks commented May 18, 2016 •

edited

Loading

jstarks commented May 20, 2016 •

edited

Loading