Do not re-tag non-distributable blob descriptors

[cpuguy83]

Before this change buildkit was changing the media type for
non-distributable layers to normal layers.
It was also clearing out the urls to get those blobs.

This keeps those layers as non-distributable and stores the urls so we can
keep those values when building the new manifest.

Fixes part of #2555

As a note, I don't have a full understanding of the internals here.
I got to this point pretty much by spraying some changes in and seeing what gave me the desired output.

[tonistiigi]

What if one blob has URLs and another access of same blob does not. I guess the same issue is with the mediatype as well. Current combinations avoid ambiguity by setting the correct mediatype from compression+oci/docker exporter options. Would you be ok with making this exporter option as well? Either just a bool or a more advanced variant would be to set the URL prefixes.

Is it correct that this does not affect the pusher? These blobs would still be pushed to the registry atm.

[cpuguy83]

Could you expand more on your concerns and what the exporter option is for?

Is it correct that this does not affect the pusher? These blobs would still be pushed to the registry atm.

Correct, this only changes the data we use to create new manifests, does not affect the behavior of pusher.
I would like to talk about what that could look like separately, though.
I do have a PR open to containerd to add an image handler to deal with the details of this: containerd/containerd#6424

[tonistiigi]

Could you expand more on your concerns and what the exporter option is for?

So for example there is one build that accesses blob abc and has URLs+foreign mediatype that are now saved on disk. And a second build has same blob abc but with regular mediatype and no URLs. Or in reverse order. On storage they are the same reference. Now when a new build creates a new manifest that contains this blob what mediatype should it use and should it include URLs. Currently, it would depend on the order the blobs were first accessed so 50% of the time it would be unexpected result for the user. It is also worse than current behavior as at least currently it does not push broken images what it may do now when URLs were unexpected.

So one way to solve it would be with a similar opt-in like I see in that containerd PR. There could of course be more complicated cases with 2 blobs with different sets of URLs or cases where user wants only some blobs to be non-distributable but at least it should not break the default case for anyone.

Another way would be to namespace these cases to separate refs on disk. That's somewhat tricky probably as current equality checks are digest-based.

[cpuguy83]

I see.
It seems like in this case we need to look at the "from" image's manifest to determine what should be done.
WDYT? Do we have this relationship available in the image writer currently?

[tonistiigi]

No, there is no such concept as "base/from image" in llb and there can't be. Buildkit only tracks chains of snapshot-blob pairs.

[cpuguy83]

@tonistiigi Thinking about this, this seems like an advanced use case which is already unsupported anyway and requires manually updating the manifest.

re: the containerd pr
The only opt-in there is whether or not to respect the non-distributable layer on push.
I'm not sure it makes sense to opt-in to not changing the the metadata around the layers especially since this is part of the spec explicitly so people don't redistribute content that they are not supposed to.

[TBBle]

So for example there is one build that accesses blob abc and has URLs+foreign mediatype that are now saved on disk. And a second build has same blob abc but with regular mediatype and no URLs.

This seems like "not immediately on fire" form of "What if we get two unrelated blobs with the same hash?". In the latter case, if we don't prevent this then you end up feeding a watermelon into unpigz and everything catches fire. In this specific case, the two things are file-format compatible so things don't catch fire immediately.

Perhaps they should catch fire immediately? When pulling a blob into the content store, is it possible to recognise a "same hash, different content type" conflict and fail there? (This might be a containerd-level problem to solve, on reflection).

For the use-cases of the non-distributable layers, I don't think there's any expectation that there will be the same objects floating around with different media types? If someone changes the mediatype and uploads it somewhere else (or unpacks it and repacks it via tooling), that's almost certainly in violation of whatever distribution restrictions are applicable.

[cpuguy83]

Yeah I don't really see a good way to deal with the problem of different metadata (mediatype, urls) for the same blob other than build it on a different machine or clear the cache.
I'm not sure if there is anything like this in the wild.

[tonistiigi]

When pulling a blob into the content store, is it possible to recognise a "same hash, different content type" conflict and fail there?

We look the contents when creating new blobs to see if they match the expected mediatype. Eg. if user exports image and requests zstd we look what is in the disk. If it is zstd then it is used, if it is uncompressed then it gets compressed. If it is gzip then we decompress and compress again. This was mainly added for estargz that uses the same mediatype as gzip but we need to understand if a blob is already stargz or not and we need to look the disk for that.

Theoretically, I'm not against doing some basic validation on pull as well. If the mediatype is gzip but blob isn't actually gzip I think it is fair to just error right away. For the other components of mediatype than compression, they don't stick with the blob. Eg. if you pulled with docker mediatypes it doesn't mean you export with them as well. You export with the types that user set on --output.

[cpuguy83]

Per our discussion, I've updated the PR:

This still always stores the original media type and urls (and always appends urls if new ones are found) in buildkit's cache.
There is a new exporter option propagate-nondist-layers=<bool> which I've added to all but the local exporter.
When false (default) it converts to the corresponding distributable media type for the output type defined in the exporter (e.g. oci vs docker).
When true it just keeps the non-distributable type already in the cache.

[tonistiigi]

I would prefer if GetRemotes() already returned the descriptor that is guaranteed to be either only distributable(default) or foreign if URLs exist. It means need to pass more arguments but atm none of the callers can make assumptions about the result, so they all need to try to convert mediatypes and clear the URLs. That is more duplicated code and eventually one of these callers will forget to run these cleanup routines.

In the future same should be done for the oci vs. docker mediatypes as well, but that is old code and can be fixed later.

[tonistiigi]

If you can, would be good to also get an integration test for it as well that tests that URLs are kept if opt-in and discarded otherwise. You can just push a small public image with a URL and add it to the mirrored image list in the beginning of integration tests. A more complicated way would be to make manifest manually and push it to the local registry but that probably gets too complex.

[TBBle]

When false (default) it converts to the corresponding distributable media type for the output type defined in the exporter (e.g. oci vs docker).

Shouldn't the propagate-nondist-layers option default to true? It seems like it'd be ecosystem-damaging if the default behaviour of BuildKit was silently putting people (both BuildKit users and third party) in unintentional violation of license agreements.

When true it just keeps the non-distributable type already in the cache.

Shouldn't it use the appropriate non-distributable layer type (application/vnd.oci.image.layer.nondistributable.v1.tar or application/vnd.docker.image.rootfs.foreign.diff.tar) for the chosen output type? Although I do note that this pair is missing from the OCI image spec's compatiblity matrix, I'm not sure if that's oversight or there's a specific reason for this.

[tonistiigi]

Shouldn't the propagate-nondist-layers option default to true? It seems like it'd be ecosystem-damaging if the default behaviour of BuildKit was silently putting people (both BuildKit users and third party) in unintentional violation of license agreements.

No buildkit user has accepted any license terms from the pulled image.

Shouldn't it use the appropriate non-distributable layer type (application/vnd.oci.image.layer.nondistributable.v1.tar or application/vnd.docker.image.rootfs.foreign.diff.tar) for the chosen output type?

I think this is already happening.

[tonistiigi]

Overall SGTM. Mostly comments about names and package structure. Still hoping for tests.

[tonistiigi]

Is there a use case that required a separate packages for this definition?

[cpuguy83]

Import cycle from cache->solver->cache

[tonistiigi]

Ok, let's figure this out later. It's more about what the dependency between solver and cache pkg should be.

[tonistiigi]

@ktock @sipsma Any ideas how to improve this? Still not very clean. And a bit of an issue with RefConfig is that keys can be uninitialized (eg. Compression doesn't have a clear zero value atm). We can do this refactoring later in a follow-up PR.

[cpuguy83]

Maybe functional options?

[cpuguy83]

Green CI, also added the new integration tests.

[TBBle]

No buildkit user has accepted any license terms from the pulled image.

You need a license to pull the image, as described at, e.g., https://hub.docker.com/_/microsoft-windows-nanoserver.

You could probably try and haggle over the meaning of "use" here, but if none of BuildKit's activities are part of "using the image", then there's no license grant for that, so it becomes less allowed.

At the very least, I have used RUN in a DockerFile via BuildKit to run a Windows Container as described in that license grant.

[tonistiigi]

You need a license to pull the image, as described at,

What you are describing here is basically equivalent of me pushing something to foobar.com and then going to the maintainers of curl and saying that they are invalidating my license because they allow users to run curl foobar.com. That they need to change their software, block my licensed files, add new options in curl to validate my license. Or I guess most comparable to this case would be that curl should never be allowed to save the output to disk as my license does not allow anyone to save my file. MSFT can of course test the validity of their claims against someone who uses that specific image but enforcing changes like this in tooling is quite ridiculous. Especially where atm it would break builds for users who don't care for any of these specific images or if an image has any urls.

[TBBle]

Your characterising is if if I was saying Buildkit is failing the license. I meant "you" in the generic sense, i.e. "one", not you, @tonistiigi, the author of BuildKit. Sorry if that wasn't clear.

If I download the thing you put on foobar.com, and I wasn't allowed to by the license, then by doing so I've broken the license. Not curl or the authors of curl. It's very possible I don't notice I've done that, as there's no indication within HTTP that this has happened.

If there was a flag or something built into HTTP that told me I wasn't licensed to download that file, then I think curl would be remiss as a HTTP client implementation if the default was "Ignore that flag", any more than it would be remiss if the default was "accept expired TLS certificates". Being able to override either is quite reasonable, as in both cases, I am saying "I know what I'm doing".

In this case, the only flag available for OCI registries that says "You are not allowed to download this" is authentication, and so MS have gone with a permissive approach "We assume you are licensed to download this" rather than a restrictive approach "You must log in and prove you are permitted to download this", which I think was the correct choice.

However, there is a specific flag in this ecosystem for "You're not allowed to upload this", so I would apply the same logic that tooling is remiss to ignore the flag.

So I would argue:

• the default should be "The user must explicitly indicate that this flag does not apply to them"; and
• silently removing this flag by silently changing media types is doing the user a disservice, as further processing within the tool may lead to them unintentionally doing a thing they should not do, and did not intend to do.

Anyway, the OCI spec is pretty clear, that irrespective of the specific legal details:

Implementations SHOULD NOT upload layers tagged with this media type

and automatically changing the media type to ignore this specification term doesn't really seem to fit the spirit of the spec.

[cpuguy83]

I tend to agree with @TBBle.
At the moment the default is catering to the the edge case where there could be multiple references to a given blob, one with URLs and one without.
The existing behavior is also causing needless pushing of typically large layers.

[TBBle]

I noticed the release note

Support exporting non-distributable blob descriptors. -o prefer-nondist-layers=true exports layer with a non-distributable mediatype and external URL if such URL was provided when blob was pulled from the remote registry.

suggests that presence or absence of URLs is being used to determine if a layer is non-distributable, and a quick check of the PR's changes to cache/refs.go confirms this. (And I note that the reason why is given there, and was also touched upon above).

That's inconsistent with the OCI Image spec directions on this topic:

Descriptors referencing non-distributable layers MAY include urls for downloading these layers directly; however, the presence of the urls field SHOULD NOT be used to determine whether or not a layer is non-distributable.

A use-case for this behaviour is that the non-distributable layer may not have a URL entry because it's not a publicly-usable base layer, so it's only expected to be referenced in images pushed to the same registry that already holds that layer. An example of this would be the layer in a CI image that contains an MS Visual Studio installation; last year a whole bunch of public images were noticed to be pushing such a layer to public Docker Hub images, where the license for MS VS Build Tools does not allow this, but allowed pushing to private images for use by other members of the same licensee, e.g., company-internal registries.

It would be reasonable (but not currently tool-supported, I think) to mark the layers containing the non-distributable content as non-distributable; and specify that internal developers building containers that need to use those tools must base it on the existing image, rather than rolling their own, to support/ensure license compliance.

(In the specific case of MS VS Build Tools, I've suggested that MS could make this less-painful, but have not had any useful response).

The mentioned edge case where some descriptor for a given layer includes URLs, and some other descriptor doesn't, and both are marked non-distributable also hits this point.

I've never seen this use-case in the wild, but it's not the sort of use-case I'd expect to see often in-the-wild anyway, as it'd be of more value in closed, controlled environments than on public container registries. And lack of tooling support (MS uses a custom tool to roll their Windows base container images) means it's not going to be a common sight either.

So I think future work on this area should address this situation. I still think the best solution is to not lose the content type of a layer at any point during the process, and fail pulls where a blob's content type does not match what's in the store already, with an explicit matrix of "known-compatible content types" to override this. But I recognise that's not necessarily a trivial change.

[tonistiigi]

At the moment the default is catering to the the edge case where

I've never seen this use-case in the wild, but it's not the sort of use-case I'd expect to see often in-the-wild anyway,

How common this case isn't really relevant for this case. Eg. we could say that it isn't very common that an image will try to attack the user system. 99.99% of images don't do that, so why implement a security sandbox at all? The possibility that someone builds an image and unexpectedly gets a broken image that can't be pulled or contains leaked information from other registries that were not part of this build at all is just something that can't happen. Even if it happens only one time or seems theoretical, it would be a bug that needs to be fixed.

(spec) Implementations SHOULD NOT upload layers tagged with this media type

iiuc this was true before and after this PR.

and automatically changing the media type to ignore this specification term doesn't really seem to fit the spirit of the spec.

One of the differences to me is that buildkit is a tool for building new images. It is not a tool for manipulating existing manifests or moving existing manifests between different storage. All the manifests it creates are 100% new. So when the user is putting together a manifest that points to abc, it could be that they got the bytes from a registry, or external URL, or got it from an external URL but now wish to just push it directly(assuming they are not breaking the license) etc.. These could all be true or in any order, and it depends on how the user thinks their new image will be used in the future. Saying that abc somehow has a fixed type doesn't really make sense. It only has a fixed type when an immutable image is already in the registry, not when a new image is being created from the data on the disk.

[TBBle]

FROM (the relevant part of image creation for this discussion) is by definition pulling already-defined layers from an already-defined image, and all of these pieces have an existing content type. They are immutable layers already in the registry, so I agree that their type should be treated as fixed.

Creating a new image that contains existing layers does not change the type or content of those layers, even when the resulting image is new. That's a pretty fundamental piece of the OCI ecosystem, since it allows docker push for example to not re-push layers the registry already knows about, and container runtimes to not pull layers they already have available locally when a new image references them.

As far as I know, BuildKit doesn't yet support tagging new layers as non-distributable, so for this discussion, we're only talking about pre-existing layers pulled from elsewhere.