Support external url layers in other os also

[runcom]

This is based on #22866 (I'll drop @jstarks commits once merged)
I've been testing this and it works great on linux also. I still struggle to understand why we want to go windows only for this feature (sorry I haven't really understood).
The last patch just gets rid of the windows-only code and move it to the general pull_v2 file. The code seems to be working fine, I was able to use it with a registry from docker/distribution master code and I was successful in downloading a layer hosted on an external url.

Pushing is, however, pushing the layer..need to see what's happening

[0xmichalis]

cc: @miminar

[mrunalp]

👍 It makes sense to enable for all Operating Systems.

[miminar]

LGTM. But I'd prefer the original PR to look this way.

[runcom]

it's working fine now:

docker push localhost:5000/busybox:test
The push refers to a repository [localhost:5000/busybox]
5f70bf18a086: Skipped foreign layer 
1834950e52ce: Pushed 
test: digest: sha256:b096a5f35b3c138031ba4880007d95b2d380fa39befaf51fffbaefbb3a6c9117 size: 814

[runcom]

the uploaded manifest in the registry is also fine:

$ cat ./docker/registry/v2/blobs/sha256/b0/b096a5f35b3c138031ba4880007d95b2d380fa39befaf51fffbaefbb3a6c9117/data
{
   "schemaVersion": 2,
   "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
   "config": {
      "mediaType": "application/vnd.docker.container.image.v1+json",
      "size": 1370,
      "digest": "sha256:47bcc53f74dc94b1920f0b34f6036096526296767650f223433fe65c35f149eb"
   },
   "layers": [
      {
         "mediaType": "application/vnd.docker.image.rootfs.diff.tar.gzip",
         "size": 675992,
         "digest": "sha256:385e281300cc6d88bdd155e0931fbdfbb1801c2b0265340a40481ee2b733ae66"
      },
      {
         "mediaType": "application/vnd.docker.image.rootfs.foreign.diff.tar.gzip",
         "size": 32,
         "digest": "sha256:a3ed95caeb02ffe68cdd9fd84406680ae93d633cb16422d00e8a7c22955b46d4",
         "urls": [
            "http://localhost:5001/layer"
         ]
      }
   ]
}

[runcom]

I'm going to add some integration tests for this if I can (not sure how to bundle an image with a manifest in code, will see)

[thaJeztah]

We discussed this in the maintainers meeting, and there were no clear objections against allowing this. We do like to keep this as a separate PR, so that the original PR is not blocked by this decision.

We decided to keep this PR in design review for a couple of days, in case there are other maintainers that have information we overlooked, but should be able to move forward after that 👍

[stevvooe]

Rather than create a separate method, Register should just take a descriptor. If no packed digest is available, it should be omitted.

[runcom]

fine with me, I guess the linked PR should be updated as well not to clutter this interface

[runcom]

does @stevvooe comments apply to the original PR?

[jstarks]

My hope is that we can get the original PR merged as-is (very soon I am going to be unavailable for the next few weeks) and address these suggestions as part of the PR to remove the Windows-specific bits.

[runcom]

I don't think it makes sense to merge the windows one and then go on with this honestly, we could end up breaking windows either. I think this should be designed thoroughly to be now os-agnostic.

[dmcgowan]

For the layer/ interfaces I would prefer to stay away from higher level concepts like "Foreign sources". As Stephen suggested, we are just making read only layers https://godoc.org/github.com/docker/distribution#Describable. This should be reflected in the interface as RegisterWithDescriptor, SetDescriptor, and GetDescriptor. Support for this feature should be checked through interface checking rather than a package level option.

Created a patch to this change which illustrates what I mentioned. dmcgowan@7429a76. Further refactoring could probably be done to clean it up further but wanted to highlight what the change would look like.

[runcom]

reworked @dmcgowan patch to work on linux and push forced - push/pull works in my tests again

[stevvooe]

@runcom

does @stevvooe comments apply to the original PR?

Yes, but #22866 will be likely merged anyways, as Windows cannot be supported without it. If you look back at my comments on #22866, most of them are to make sure no issues arise when adding wider OS support. @jstarks did a great job in ensuring the PR complied with that commentary, even though it was completely orthogonal to his goal.

Remember, Docker support for Linux has been around since the beginning, whereas Windows Containers are very new. Considering this feature is limited to Windows, missteps in this regard are acceptable. A higher standard needs to be held for wider OS support to ensure we don't break backwards compatibility and, more importantly, that we don't open up a massive exploit.

A few things that need to be done before getting wider OS support:

1. We really need to centralize descriptor storage. Right now, there is digest storage, something stuffed into tags and foreign source. This is okay for Windows support, but this needs to be handled before adding wider support to ensure proper operation.
2. Since we need to store descriptors with the layers, we also need to store the compressed layer content to ensure that the descriptor stays valid, even after possibly non-deterministic compression.
3. The invalidation of a descriptors needs to be handled. This can happen on push, when the original content is no longer available. This is not a problem on windows, since users can't really build base layers.
4. The foreign source abstraction should not leak into the push/pull code. It should only look for URLs on the descriptors and use them, if present. The presence of a foreign source mediatype should only be used to make the determination whether or not to actually push. If done correctly, the only mention of foreign source should be when checking which layers to push.
5. Docker Hub Security Scanning needs to be updated to fetch these descriptor URLs and pull them in.

Let’s work together to get this out and make sure wider OS support is high quality.

[runcom]

@stevvooe thanks - is there anyone already working on some on the points above? if not, I guess point 4) is already handled here. Could you elaborate more on the other points?

[stevvooe]

@runcom This is something we've been discussing, in detail.

Layers have an identifier known as a DiffID. This is the hash of the uncompresssed tar file. The stability of this file is maintained by tar-split, ensuring that packing and unpacking from disk results in a stable hash. Descriptors describe the digests, size and mediatype of packed layer archives. Depending on the stability of the compression, layers may have differing digests and diffIDs. While this allows registries to implement content-agnostic content addresses, it means that a layer may have more than one remote digest. Put differently, the relationship between a layer and descriptors is 1:N, where N represents the variation in compression output. N is small, in practice, but the fact that it can be greater than 1 creates this need.

Currently, the management of Descriptor/Digest storage is fairly ad-hoc. We can see this in areas of the code that store just the digest then generate a descriptor from that. The correct cases are covered but we can use the properties described above to store them in a consistent way for both layers and other content, such as manifests.

On the other side of this, we need to ensure that the descriptors in that store are invalidated when a layer is regenerated and the new descriptor hash does not match. Furthermore, such that when a manifest is assembled, it represents the set of layers that are actually pushed into a registry.

Why does this matter for foreign layers? Up until this point, the mediatypes for layers has been fairly consistent. Using a default everywhere is sufficient. With this change, we know have a situation where the contents of the descriptor is not only unique and meaningful, but can affect the push behavior. In effect, we need to ensure that both the mediatype, used as a push policy, and the URLs are stored reliably, along with the correct digest.

Let me know if you need more to go on.

[runcom]

@stevvooe great, so, as part of what you highlighted - I'm going to move on with this starting from the first of your point above. IIUC, what you're saying if that we should make the descriptor.json (or whatever you name it) storable along with its layer. Is my understanding right? Right now the current approach is to store it only when its mediatype is a foreign source but this should be done for every layer to make this consistent.

[stevvooe]

Is my understanding right? Right now the current approach is to store it only when its mediatype is a foreign source but this should be done for every layer to make this consistent.

Yes. But we also have to take measures to ensure that remains valid. There are certain cases where two registries may have the same layer stored under different digests due to divergent compression. This can be done by invalidating the descriptor and regenerating the compressed layer or maintaining a list of known descriptors for a piece of content.

[thecloudtaylor]

Ping to @jhowardmsft while @jstarks is away on vacation.

[lowenna]

What's outstanding on this?

[aaronlehmann]

@jhowardmsft: First #22866 needs to be finished and merged. This PR is about extending that functionality for OS' beyond Windows, and there's still ongoing discussion about we get there.

The main item I see outstanding on that #22866 is #23014 (comment) (I believe that comment was meant to go on that PR, not this one).

[runcom]

@stevvooe @aaronlehmann as part of what said in earlier comments - we should store the compressed layer https://github.com/docker/docker/pull/23014/files#diff-0c4769e1568c0f439863abd30df1ceccL313 along with the descriptor - do we want to create a descriptorStore or it's layerStore's business to store descriptor and compressed layer? (for later invalidation possibly)

[runcom]

I've reworked the commit to extend support to other os - left a comment above to move on implementation for wider os support

[thaJeztah]

ping @stevvooe @aaronlehmann where are we on this one?

(@runcom this needs a rebase now)

[runcom]

(@runcom this needs a rebase now)

thx @thaJeztah - I'm waiting to get some guidance though

[thaJeztah]

I briefly discussed this, and the current situation is that we like to test
this feature before opening up this option for other platforms as well.

Opening up this feature for "general" use can have quite some implications. Also
see @stevvooe's comment #23014 (comment),
so they want to evaluate after the 1.12 release. Not all concerns are
addressed in terms of impact on registry operators SLAs, so that's
something to look into

[runcom]

Not all concerns are
addressed in terms of impact on registry operators SLAs, so that's
something to look into

I don't understand this :)

can you guys actually point out a list of TODO or TOTEST things in order to move on with this?

[stevvooe]

@runcom Mostly, we need to think about the UX and communication to users on these changes.

Do you mind demonstrating what happens on the client when a foreign layer isn't available?

[runcom]

Do you mind demonstrating what happens on the client when a foreign layer isn't available?

isn't available can mean many thing, I think if the registry doesn't have a layer it's going to tell the user that something went wrong and the layer cannot be downloaded (I have to test this out by removing a layer from a registry and docker pull an image with that layer). Likewise, if the layer isn't available at a 3rd party host, we can do the same and just tell the user the layer isn't available. What's the issue with telling ppl a layer cannot be downloaded?

[stevvooe]

isn't available can mean many thing

In this context, we specifically mean, availability for download. This can be due to connectivity, authorization or simple presence.

There are several cases where the urls field is present:

Registry Blob Available	Remote Blob Available	Foreign URL Supported	What Happens
No	No	No	Similar to today
Yes	No	No	Success, inconsequential
No	Yes	No	Fail, blob missing in registry
Yes	Yes	No	Works as it does today
No	No	Yes	Fail, blob missing in remove location (do we show it?)
No	Yes	Yes	Works, found in remote
Yes	No	Yes	Works, found in registry
Yes	Yes	Yes	Works, found in registry

What's the issue with telling ppl a layer cannot be downloaded?

There are many issues with this. What can a user do to resolve such a problem? Do they contact Docker? Do they contact the ISV?

[runcom]

So what people do when the base windows layer isn't available? I expect people to contact them directly, I don't see complication in this since windows already has this in place now. I doubt anyone would contact docker because a layer is missing from a Windows based image

[stevvooe]

So what people do when the base windows layer isn't available?

You're comparing a brand new community to one that is already established. Windows support can't work without foreign layers, whereas linux hasn't needed them.

I expect people to contact them directly, I don't see complication in this since windows already has this in place now. I doubt anyone would contact docker because a layer is missing from a Windows based image

You'd be surprised what we get contacted about. Unfortunately, this is not an area where speculation is going to work. We need to make sure the UI makes this clear.

Do you not see the value of testing this out on Windows community before introducing wider OS support?

[runcom]

You're comparing a brand new community to one that is already established. Windows support can't work without foreign layers, whereas linux hasn't needed them.

I'm talking about the same community, irrespective of windows or linux because Docker it's supposed to work the same on both platforms (we're also talking about images which, even in the foreign layer case, it's a pretty platform independent feature). Windows users aren't dealing with a different kind of docker to me wrt images.

Windows support can't work without foreign layers

they're required to host the base layers themselves. What if another ISV is required to do the same and the base layer happens to be on linux? I can't believe no other ISV is looking for this and I'm sure if we compare far more linux ISV need this. So it makes sense to perhaps put this feature in the experimental channel as a start - not just for Windows.

whereas linux hasn't needed them.

Linux does need foreign layers also. Just because no one came with this before, it doesn't mean ppl don't need this. I'm not sure where linux hasn't needed them is coming from.

Do you not see the value of testing this out on Windows community before introducing wider OS support?

No

first you say communities are different so what would be the point in windows testing this? I still think the community is one so linux or windows don't make any difference. Plus, the linux community should be wider and help us better test this whole thing out, maybe in experimental if you wish.

[aweiteka]

From pr#22866 we learned this feature...

...is specifically necessary to support downloading Windows base layers from Microsoft servers, since only Microsoft is allowed to distribute them.

This has always been the case for Red Hat base layers. In fact, we spelled out the use case nearly 2 years ago, well before Windows support.

One of the principles of docker from early on was to have a common user experience regardless of OS. How is it that now that the Windows OS has special treatment?

[dmp42]

Thanks for the PR @runcom

There are a number of blockers for this right now:

• backward compatibility: if this is a breaking change (and I believe it is), it's not something that can just "be merged and let's see". We need full blown backward compat solutions - maybe a patch in the open-source registry that would let it fetch the foreign layers and deliver them to older engines (+ rewrite the manifest) - but then, what with DCT? and wouldn't that defeat the purpose of this?
• such a change would have a major impact on registry operators (eg: breaking their SLA), so, another hard requirement for this is to have a whitelisting mechanism that let registry operators select which domains are allowed to host foreign layers
• consequently, there must be UI to provide feedback to the user when they try to push an image with disallowed origins for foreign layers
• also, what happens to the concept of private image? is an image still private with foreign layers?
• also, should we support authentication on the foreign origin? how would that work?
• finally, even if this would be merged now, you can't expect all registry operators to accept content with foreign layers (as I said, it will impact their SLA)

All in all:

• I believe we all agree that layer federation for ISVs is a desirable feature (though a different one from what the Docker Windows port does, even if technically close), and we should work together to make it happen.
• the work that already happened for use in the Windows Docker Port is definitely going to help layer federation for ISVs to land on every platform

We should look into this further and figure out solutions for the concerns listed above.

Hope that clarifies / helps.

[runcom]

Thanks for the PR @runcom

There are a number of blockers for this right now:

backward compatibility: if this is a breaking change (and I believe it is), it's not something that can just "be merged and let's see". We need full blown backward compat solutions - maybe a patch in the open-source registry that would let it fetch the foreign layers and deliver them to older engines (+ rewrite the manifest) - but then, what with DCT? and wouldn't that defeat the purpose of this?

I agree but such a compatibility hack can be made in place just for older client as you said. The registry can serve and forget (cache?) the foreign layer. It's just for compatibility though. I don't feel strong about this, it seems a clean way of ensuring backward compatibility to me even if layer federation is defeated in such a case somehow.

such a change would have a major impact on registry operators (eg: breaking their SLA), so, another hard requirement for this is to have a whitelisting mechanism that let registry operators select which domains are allowed to host foreign layers
consequently, there must be UI to provide feedback to the user when they try to push an image with disallowed origins for foreign layers

makes sense to both points above

also, what happens to the concept of private image? is an image still private with foreign layers?

as long as the manifest is private it should be or am I missing something? even if some layers are hosted elsewhere

also, should we support authentication on the foreign origin? how would that work?

This would be great, but I believe this can be discussed separately and it's not something I would do for an MVP, but definitely worth pursuing.

finally, even if this would be merged now, you can't expect all registry operators to accept content with foreign layers (as I said, it will impact their SLA)

we can let admins configure that in the registries.

All in all:

I believe we all agree that layer federation for ISVs is a desirable feature (though a different one from what the Docker Windows port does, even if technically close), and we should work together to make it happen.
the work that already happened for use in the Windows Docker Port is definitely going to help layer federation for ISVs to land on every platform
We should look into this further and figure out solutions for the concerns listed above.

Hope that clarifies / helps.

it certainly does, thanks @dmp42, I believe I can propose something in docker/distribution as a starting point so we can further discuss your points above. Or do you have other idea about how to go with this?

[dmp42]

Yes, a proposal in docker/distribution would certainly be a good starting point.

Agreed that authentication is not priority.

About private images: if the foreign layer is not access restricted, then that part of the image can not be guaranteed to be private even if the manifest is.

I would suggest you start it by collecting / listing / summarizing all (immediate, priority 0) concerns for a MVP (from all the feedback on this ticket).

Be sure to ping @RichardScothern @aaronlehmann @dmcgowan and @stevvooe for help and feedback on this.

Thanks again for your efforts on this @runcom