-
Notifications
You must be signed in to change notification settings - Fork 18.6k
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
Support layers from external URLs #22866
Conversation
Signed-off-by: John Starks <jostarks@microsoft.com>
if err != nil { | ||
return nil, err | ||
} | ||
return ref, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the content of foreign-source
somehow got set to null
, this could return a nil pointer. I think it would be a little safer to have var ref distribution.Descriptor
and unmarshal into that. Then this function could return distribution.Descriptor
instead of *distribution.Descriptor
so there's not even any ambiguity about whether the result can be nil.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We return nil to mean that there is no foreign source. I think this is the right way to handle it, rather than requiring callers to check for a non-empty digest or return an additional bool or something.
You're right, though; if foreign-source
exists, we should return a non-nil value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it could be confusing for the function to return a nil
error but also a nil
result. The general convention is that err == nil
means there's a valid value being returned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you suggest indicating to the caller that there is no foreign source?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would probably return a sentinel error value like ErrNoForeignSource
to return in this case.
@jstarks is there anything preventing right now to enable this on linux for instance? other than the != windows check and error out |
We definitely want something like this for supporting RHEL base images. |
@runcom No, this could work on any OS. I had received some feedback that we might want to limit it to Windows initially. |
7cf45d4
to
b34f56a
Compare
I've pushed an update that fixes a few issues, renames Note, I haven't validated these changes yet. |
This is used to support downloading Windows base images from Microsoft servers. Signed-off-by: John Starks <jostarks@microsoft.com>
LGTM pending your validation |
:/ |
+1 for this. +2 for making this OS generic. It would in fact simplify the PR. |
This is something we really need on Linux as much as Windows. |
@aaronlehmann I've validated these changes locally. |
I don't think the CI failures are related to this change. |
This comment applies to this PR as well #23014 (comment). The change is only intended as a code interface and not a change to what is stored on disk. I could support getting this in as in with the understanding the code will be refactored. |
LGTM |
Can we merge this? Tagging John/Stefan @jhowardmsft @swernli while john is out of the office. |
I trust @aaronlehmann and @stevvooe on the review, I only gave it a read. LGTM 👍 |
I don't think there's any documentation involved on docker side, but we might need to update specs in the distribution repository maybe? |
RepoTags []string | ||
Layers []string | ||
Parent image.ID `json:",omitempty"` | ||
LayerSources map[layer.DiffID]*distribution.Descriptor `json:",omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's unfortunate this map is indexed by layer.DiffID
- this basically prevents having common code when integrating OCI save/load (#26369).
You can't get foreign sources by diffID in OCI since all hashes are just from compressed layers digests and at the time we build this manifestItem
for an OCI tarball we don't have info about diffIDs and cannot populate this map.
@aaronlehmann @stevvooe @tonistiigi any idea on how to convert this to something more useful to be used in the OCI PR also?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose you could use digest.Digest
, since both DiffID
s and OCI foreign sources are digests of some sort?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mmm nope, it's a matter of the different digest actually. the index in LayerSources are non-compressed digests, OCI has compressed digests
This change adds support for layers that are distributed from external URLs. This is specifically necessary to support downloading Windows base layers from Microsoft servers, since only Microsoft is allowed to distribute them.
Essentially this just changes pull to know how to download these layers, push to know not to upload these layers, and save and load to maintain the appropriate metadata.
Currently this is only enabled on Windows builds.
In the future we may want to extend this to support treating these foreign layers as normal layers for air-gapped scenarios. This is not supported yet.
cc @stevvooe, @aaronlehmann