8000 Windows: Block pulling uplevel images by lowenna · Pull Request #36327 · moby/moby · GitHub
[go: up one dir, main page]
Skip to content

Windows: Block pulling uplevel images #36327

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 5, 2018
Merged

Windows: Block pulling uplevel images #36327

merged 1 commit into from
Mar 5, 2018

Conversation

lowenna
Copy link
Member
@lowenna lowenna commented Feb 15, 2018 •
edited
Loading

Signed-off-by: John Howard jhoward@microsoft.com

Fixes #36184. This change gives a decent error message about image incompatibility early, rather than blindly continuing to download the image, try to extract it, and then fail with an extremely misleading error:

Before this change, on an RS1 host, pulling a 1709/RS3 image:

PS E:\go\src\github.com\docker\docker> docker pull microsoft/nanoserver:1709
1709: Pulling from microsoft/nanoserver
407ada6e90de: Extracting [==================================================>]  81.04MB/81.04MB
3cb15ab58a2c: Download complete
failed to register layer: re-exec error: exit status 1: output: ProcessUtilityVMImage C:\control\windowsfilter\21c033dbff11f1084e2ef67c041c491fd04be6aea0eb6d622067f2136b6b48b4\UtilityVM: The system cannot find the path specified.
PS E:\go\src\github.com\docker\docker>

After this change:

PS E:\go\src\github.com\docker\docker> docker pull microsoft/nanoserver:1709
1709: Pulling from microsoft/nanoserver
a Windows version 10.0.16299-based image is incompatible with a 10.0.14393 host
PS E:\go\src\github.com\docker\docker>

@johnstep @darrenstahlmsft @thaJeztah PTAL

@lowenna lowenna requested a review from cpuguy83 as a code owner February 15, 2018 21:20
@lowenna lowenna mentioned this pull request Feb 15, 2018
Closed
darstahl
darstahl approved these changes Feb 15, 2018
View reviewed changes
Copy link
Contributor
@darstahl darstahl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with minor nit

distribution/pull_v2_windows.go Outdated
Copy link
Contributor
@darstahl darstahl Feb 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might want to remove this TODO, as we don't want to filter greater versions here anymore as the returned error would be just that there is no matching image in the manifest, rather than an incompatible version.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, update pushed.

@lowenna lowenna force-pushed the jjh/block-pulling-uplevel branch from 4b9e0ed to 1d67540 Compare February 15, 2018 21:42
@cpuguy83
Copy link
Member

Seems ok to do this, but I'm not familiar with this part of the codebase.

cpuguy83
cpuguy83 reviewed Feb 16, 2018
View reviewed changes
distribution/config.go Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naming is hard

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DecodeConfig and return a struct with said config?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, was also not too happy with four separate values being returned

@lowenna lowenna force-pushed the jjh/block-pulling-uplevel branch from 1d67540 to 8a9a94e Compare February 21, 2018 22:55
@lowenna
Copy link
Member Author
lowenna commented Feb 21, 2018

@thaJeztah @cpuguy83 Went back to the original implementation which was just RootFS and added a new method to get the platform pieces.

@lowenna
Copy link
Member Author
lowenna commented Feb 22, 2018

@johnstep as discussed offline. PTAL

@codecov
Copy link
codecov bot commented Feb 22, 2018 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@00c1c60). Click here to learn what that means.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #36327   +/-   ##
=========================================
  Coverage          ?   34.63%           
=========================================
  Files             ?      614           
  Lines             ?    45526           
  Branches          ?        0           
=========================================
  Hits              ?    15767           
  Misses            ?    27698           
  Partials          ?     2061
Impacted Files Coverage Δ
plugin/backend_linux.go 4.16% <0%> (ø)
distribution/push_v2.go 25.07% <0%> (ø)
plugin/blobstore.go 8.79% <0%> (ø)
distribution/pull_v2_unix.go 0% <0%> (ø)
distribution/pull_v2.go 10.53% <0%> (ø)
distribution/config.go 0% <0%> (ø)
plugin/manager.go 25.88% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 00c1c60...8390883. Read the comment docs.

johnstep
johnstep approved these changes Feb 26, 2018
View reviewed changes
Copy link
Member
@johnstep johnstep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

distribution/pull_v2_windows.go Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I like this name; there could be an updated older version that is technically newer. :) Also, I usually think of "is" functions returning a bool, but not sure. What about checkImageCompatibility or something?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, checkImageCompatibility. Update pushed.

Windows: Block pulling uplevel images
8390883 
Signed-off-by: John Howard <jhoward@microsoft.com>
@lowenna lowenna force-pushed the jjh/block-pulling-uplevel branch from 8a9a94e to 8390883 Compare February 26, 2018 20:36
This was referenced Feb 27, 2018
Closed
Merged
@johnstep
Copy link
Member
johnstep commented Feb 27, 2018 •
edited
Loading

@thaJeztah @cpuguy83 Will one of you take another look? It looks good to me.

thaJeztah
thaJeztah reviewed Feb 27, 2018
View reviewed changes
distribution/pull_v2_windows.go
if imageOSBuild, err := strconv.Atoi(splitImageOSVersion[2]); err == nil {
if imageOSBuild > int(hostOSV.Build) {
errMsg := fmt.Sprintf("a Windows version %s.%s.%s-based image is incompatible with a %s host", splitImageOSVersion[0], splitImageOSVersion[1], splitImageOSVersion[2], hostOSV.ToString())
logrus.Debugf(errMsg)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this already logged somewhere (as it's also returned as an error?)

thaJeztah
thaJeztah approved these changes Feb 27, 2018
View reviewed changes
Copy link
Member
@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

dmcgowan
dmcgowan approved these changes Mar 5, 2018
View reviewed changes
Copy link
Member
@dmcgowan dmcgowan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

< A3E2 /form>

LGTM

@johnstep johnstep merged commit 3e1505e into moby:master Mar 5, 2018
@lowenna lowenna deleted the jjh/block-pulling-uplevel branch March 8, 2018 17:45
@RajeetN RajeetN mentioned this pull request Mar 26, 2018
Closed
@JoshEbersol JoshEbersol mentioned this pull request Nov 14, 2018
Closed
@iskiselev iskiselev mentioned this pull request Mar 12, 2020
Closed
aznashwan added a commit to aznashwan/moby that referenced this pull request Nov 17, 2022
@aznashwan
windows: daemon: allow pulling/running of newer images under Hyper-V.
7108ffa 
Following the addition of Hyper-V isolation, it should be possible for
Windows hosts to run container images with older/newer OS versions given
a Windows host with the RS5 updates and above.

This patch includes the following changes:
- [distribution/pull_v2] enable pulling images newer than the host on RS5+
- [daemon/create] add version checks which prevent running newer/older
  unless using Hyper-V isolation

Previously, pulling a Windows image with a newer OS version than the
host was prevented by moby#36327, whose behavior is preserved on
pre-RS5 hosts.

Compatibility requirements were [refernced from Microsoft](https://learn.microsoft.com/en-us/virtualization/windowscontainers/deploy-containers/version-compatibility?tabs=windows-server-2022%2Cwindows-10#windows-server-host-os-compatibility).

Signed-off-by: Nashwan Azhari <nazhari@cloudbasesolutions.com>
aznashwan added a commit to aznashwan/moby that referenced this pull request Nov 17, 2022
@aznashwan
windows: daemon: allow pulling/running of newer images under Hyper-V.
98247be 
Following the addition of Hyper-V isolation, it should be possible for
Windows hosts to run container images with older/newer OS versions given
a Windows host with the RS5 updates and above.

This patch includes the following changes:
- [distribution/pull_v2] enable pulling images newer than the host on RS5+
- [daemon/create] add version checks which prevent running newer/older
  unless using Hyper-V isolation

Previously, pulling a Windows image with a newer OS version than the
host was prevented by moby#36327, whose behavior is preserved on
pre-RS5 hosts.

Signed-off-by: Nashwan Azhari <nazhari@cloudbasesolutions.com>
@aznashwan aznashwan mentioned this pull request Nov 17, 2022
Open
aznashwan added a commit to aznashwan/moby that referenced this pull request Nov 17, 2022
@aznashwan
windows: daemon: allow pulling/running of newer images under Hyper-V.
a6e46cc 
Following the addition of Hyper-V isolation, it should be possible for
Windows hosts to run container images with older/newer OS versions given
a Windows host with the RS5 updates and above.

This patch includes the following changes:
- [distribution/pull_v2] enable pulling images newer than the host on RS5+
- [daemon/create] add version checks which prevent running newer/older
  images unless using Hyper-V isolation

Previously, pulling a Windows image with a newer OS version than the
host was prevented by moby#36327, whose behavior is preserved on
pre-RS5 hosts.

Signed-off-by: Nashwan Azhari <nazhari@cloudbasesolutions.com>
aznashwan added a commit to aznashwan/moby that referenced this pull request Nov 17, 2022
@aznashwan
windows: daemon: allow pulling/running of newer images under Hyper-V.
806c6cf 
Following the addition of Hyper-V isolation, it should be possible for
Windows hosts to run container images with older/newer OS versions given
a Windows host with the RS5 updates and above.

This patch includes the following changes:
- [distribution/pull_v2] enable pulling images newer than the host on RS5+
- [daemon/create] add version checks which prevent running newer/older
  images unless using Hyper-V isolation

Previously, pulling a Windows image with a newer OS version than the
host was prevented by moby#36327, whose behavior is preserved on
pre-RS5 hosts.

Signed-off-by: Nashwan Azhari <nazhari@cloudbasesolutions.com>
aznashwan added a commit to aznashwan/moby that referenced this pull request Nov 17, 2022
@aznashwan
windows: daemon: allow pulling/running of newer images under Hyper-V.
211c8d2 
Following the addition of Hyper-V isolation, it should be possible for
Windows hosts to run container images with older/newer OS versions given
a Windows host with the RS5 updates and above.

This patch includes the following changes:
- [distribution/pull_v2] enable pulling images newer than the host on RS5+
- [daemon/create] add version checks which prevent running newer/older
  images unless using Hyper-V isolation

Previously, pulling a Windows image with a newer OS version than the
host was prevented by moby#36327, whose behavior is preserved on
pre-RS5 hosts.

Signed-off-by: Nashwan Azhari <nazhari@cloudbasesolutions.com>
aznashwan added a commit to aznashwan/moby that referenced this pull request Nov 22, 2022
@aznashwan
windows: daemon: allow pulling/running of newer images under Hyper-V.
00ca3e9 
Following the addition of Hyper-V isolation, it should be possible for
Windows hosts to run container images with older/newer OS versions given
a Windows host with the RS5 updates and above.

This patch includes the following changes:
- [distribution/pull_v2] enable pulling images newer than the host on RS5+
- [daemon/create] add version checks which prevent running newer/older
  images unless using Hyper-V isolation

Previously, pulling a Windows image with a newer OS version than the
host was prevented by moby#36327, whose behavior is preserved on
pre-RS5 hosts.

Signed-off-by: Nashwan Azhari <nazhari@cloudbasesolutions.com>
aznashwan added a commit to aznashwan/moby that referenced this pull request Nov 23, 2022
@aznashwan
windows: daemon: allow pulling/running of newer images under Hyper-V.
c50bbfd 
Following the addition of Hyper-V isolation, it should be possible for
Windows hosts to run container images with older/newer OS versions given
a Windows host with the RS5 updates and above.

This patch includes the following changes:
- [distribution/pull_v2] enable pulling images newer than the host on RS5+
- [daemon/create] add version checks which prevent running newer/older
  images unless using Hyper-V isolation

Previously, pulling a Windows image with a newer OS version than the
host was prevented by moby#36327, whose behavior is preserved on
pre-RS5 hosts.

Signed-off-by: Nashwan Azhari <nazhari@cloudbasesolutions.com>
aznashwan added a commit to aznashwan/moby that referenced this pull request Mar 28, 2023
@aznashwan
windows: daemon: allow pulling/running of newer images under Hyper-V.
0280642 
Following the addition of Hyper-V isolation, it should be possible for
Windows hosts to run container images with older/newer OS versions given
a Windows host with the RS5 updates and above.

This patch includes the following changes:
- [distribution/pull_v2] enable pulling images newer than the host on RS5+
- [daemon/create] add version checks which prevent running newer/older
  images unless using Hyper-V isolation

Previously, pulling a Windows image with a newer OS version than the
host was prevented by moby#36327, whose behavior is preserved on
pre-RS5 hosts.

Signed-off-by: Nashwan Azhari <nazhari@cloudbasesolutions.com>
940C
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cpuguy83 cpuguy83 cpuguy83 left review comments

@dmcgowan dmcgowan dmcgowan approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

@tonistiigi tonistiigi Awaiting requested review from tonistiigi

+2 more reviewers

@johnstep johnstep johnstep approved these changes

@darstahl darstahl darstahl approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@johnstep johnstep

Labels
status/2-code-review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@lowenna @cpuguy83 @johnstep @dmcgowan @thaJeztah @darstahl @vdemeester @GordonTheTurtle
0