Add Official Support for Windows Docker Images #2136

zZHorizonZz · 2024-09-03T10:34:59Z

Description

This PR introduces official support for building windows based docker images for node. At the moment, these images don't include Yarn, I noticed that other PR(s) are pushing to remove it from the Linux images as well, so I followed suit. Although maybe we could enable corepack by default? As currently, based on the discussion in the PR to enable corepack by default in Node itself, it seems like they will be rewriting it because npm has some security concers, so that may take a couple of years.

Motivation and Context

I've seen a few attempts to add windows support over the years, but those PRs seem to have stalled. So, I thought I'd take a look at it. I've combined some useful bits from those previous efforts and added my own improvements to get things working smoothly on newer versions and new build system.

Testing Details

I've successfully built and tested these images locally. Plus, with the new Windows pipeline I added, they've been tested against both Windows Server 2019 and 2022 trough github actions.

Example Output (if appropriate)

None provided

Types of changes

Documentation
Version change (Update, remove or add more Node.js versions)
Variant change (Update, remove or add more variants, or versions of variants)
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Other (none of the above)

Checklist

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING.md document.
All new and existing tests passed.

Dockerfile-windows.template

zZHorizonZz · 2024-09-04T11:58:54Z

I see something weird going on in the GitHub action when running the gpg --keyserver command. If I run the build locally, it works, but if it runs in the GitHub action, it's throwing errors:

gpg: keyserver receive failed: Server indicated a failure
gpg: keyserver receive failed: Server indicated a failure
gpg: keyserver receive failed: Server indicated a failure
gpg: keyserver receive failed: Server indicated a failure
gpg: keyserver receive failed: Server indicated a failure
gpg: keyserver receive failed: Server indicated a failure
gpg: keyserver receive failed: Server indicated a failure
gpg: keyserver receive failed: Server indicated a failure
gpg: keyserver receive failed: Server indicated a failure
gpg: keyserver receive failed: Server indicated a failure
gpg: keyserver receive failed: Server indicated a failure
gpg: keyserver receive failed: Server indicated a failure
gpg: Signature made 08/22/24 14:16:19 Coordinated Universal Time
gpg:                using RSA key 890C08DBEAB4DFCF[55](https://github.com/zZHorizonZz/docker-node-windows/actions/runs/10681419694/job/29605009626#step:4:56)5EF4
gpg: Can't check signature: No public key

It almost seems like it's not able to call the servers outside.

Dockerfile-windows.template

zZHorizonZz · 2024-09-05T08:03:25Z

I see something weird going on in the GitHub action when running the gpg --keyserver command. If I run the build locally, it works, but if it runs in the GitHub action, it's throwing errors:

gpg: keyserver receive failed: Server indicated a failure
gpg: keyserver receive failed: Server indicated a failure
gpg: keyserver receive failed: Server indicated a failure
gpg: keyserver receive failed: Server indicated a failure
gpg: keyserver receive failed: Server indicated a failure
gpg: keyserver receive failed: Server indicated a failure
gpg: keyserver receive failed: Server indicated a failure
gpg: keyserver receive failed: Server indicated a failure
gpg: keyserver receive failed: Server indicated a failure
gpg: keyserver receive failed: Server indicated a failure
gpg: keyserver receive failed: Server indicated a failure
gpg: keyserver receive failed: Server indicated a failure
gpg: Signature made 08/22/24 14:16:19 Coordinated Universal Time
gpg:                using RSA key 890C08DBEAB4DFCF[55](https://github.com/zZHorizonZz/docker-node-windows/actions/runs/10681419694/job/29605009626#step:4:56)5EF4
gpg: Can't check signature: No public key

It almost seems like it's not able to call the servers outside.

The only thing I can think of is that it's being rate limited on the keyserver side, as the requests are coming from GitHub where I imagine a lot of people are sending requests. But in that case, I don't understand why the Linux build would work.

Dockerfile-windows.template

LaurentGoderre · 2024-09-09T13:44:55Z

Can you create a docs update PR for this?

zZHorizonZz · 2024-09-09T15:47:04Z

Can you create a docs update PR for this?

I have updated the README to include a section about the windowsservercore variant.

zZHorizonZz · 2024-09-10T04:46:39Z

@LaurentGoderre

Can you create a docs update PR for this?

Maybe I understand it wrong, but did you mean to update the README or is there some separate documentation I should update? I didn't find any other doc files in this project.

LaurentGoderre · 2024-09-10T13:40:10Z

There's a separate repo for the documentation on Hub but I realise the Node one is out of sync so I will fix that seperatly.

LaurentGoderre · 2024-09-10T13:43:40Z

I will let the other members review it but LGTM

Dockerfile-windows.template

LaurentGoderre · 2024-09-19T20:35:28Z

Dockerfile-windows.template

+    gpg --batch --decrypt --output SHASUMS256.txt SHASUMS256.txt.asc ; \
+    Invoke-WebRequest $('https://nodejs.org/dist/v{0}/node-v{0}-win-x64.zip' -f $env:NODE_VERSION) -OutFile 'node.zip' -UseBasicParsing ; \
+    $sum = $(cat SHASUMS256.txt.asc | sls $('  node-v{0}-win-x64.zip' -f $env:NODE_VERSION)) -Split ' ' ; \
+    if ((Get-FileHash node.zip -Algorithm sha256).Hash -ne $sum[0]) { Write-Error 'SHA256 mismatch' } ; \


@zZHorizonZz to meet what @tianon said, I think you could change these 2 line to be

if ((Get-FileHash node.zip -Algorithm sha256).Hash -ne $NODE_CHECKSUM) { Write-Error 'SHA256 mismatch' } ; \

The reason why this is better is that the images get rebuilt when the base image changes so if the server was compromised, the key would be changed at the same time as the archive. This means that the arcgive can't change or rebuilds would fail.

@LaurentGoderre It's not really just about changing these two lines. If we check the checksum, we don't really need GPG or multistage, which makes it simpler. So, what I'll do is remove the GPG installation and SHA256 download and replace it with verification through the NODE_CHECKSUM variable. I'll also remove the multistage as that's not necessary if we're going this way.

Aren't these two different checks though? One checks it wasn't modified, the other checks it came from the release team

I'm starting to get a little bit confused here.

If we change these two lines:

$sum = $(cat SHASUMS256.txt.asc | sls $(' node-v{0}-win-x64.zip' -f $env:NODE_VERSION)) -Split ' ' ; \ if ((Get-FileHash node.zip -Algorithm sha256).Hash -ne $sum[0]) { Write-Error 'SHA256 mismatch' } ; \

To this:

if ((Get-FileHash node.zip -Algorithm sha256).Hash -ne $NODE_CHECKSUM) { Write-Error 'SHA256 mismatch' } ;

We validate the checksum against the supplied checksum NODE_CHECKSUM. In the first version, we used the sum extracted from the SHASUMS256.txt.asc file. Maybe I misunderstood you. Did you mean to replace these lines or add a new check but keep the verification from SHASUMS as well? I got confused because @tianon is also discussing whether we should use GPG at all. I looked through the Golang Windows images, and they are also using only one check with a static SHA256.

Ah yeah, my bad. We might be good to just have the hardcoded checksum and no gpg

…KSUM Signed-off-by: Daniel Fiala <[email protected]>

Signed-off-by: Daniel Fiala <[email protected]>

zZHorizonZz · 2024-09-22T15:09:10Z

So since this has been merged, it means that users won't be able to use corepack without installing it (In the future). So do we want to install yarn or not? I personally think the decision on what to use for a package manager should be up to the user. So I would personally not install yarn at all and keep it the way it is, just plain Node.js, or install corepack so that users can then enable the package manager of their choice themselves.

LaurentGoderre · 2024-09-23T14:13:49Z

@zZHorizonZz I personally think it would be good to not include yarn in this variant by default since it's new and wouldn't break anything but I could see people wanting the variants to be consistent. I will let others weigh in on this.

zZHorizonZz · 2024-09-23T14:46:15Z

Yeah, the reason why I'm even discussing this is because of this issue you created, where I see an inclination to not just include yarn even in linux images, or create an image variant with yarn pre-installed.

iancward · 2024-10-25T22:03:54Z

Looks like this will address #2063.

Copilot reviewed 3 out of 9 changed files in this pull request and generated 2 suggestions.

Files not reviewed (6)

22/windowsservercore-ltsc2019/Dockerfile: Language not supported
22/windowsservercore-ltsc2022/Dockerfile: Language not supported
Dockerfile-windows.template: Language not supported
architectures: Language not supported
functions.sh: Language not supported
update.sh: Language not supported

Comments skipped due to low confidence (1)

genMatrix.js:22

The function name 'getNodeVerionDirs' should be 'getNodeVersionDirs'.

const getNodeVerionDirs = (base) => getChildDirectories(base)

2024-12-09T18:30:31Z

genMatrix.js

@@ -8,23 +8,25 @@ const testFiles = [
 ];

 const nodeDirRegex = /^\d+$/;
+// Directories starting with 'windowsservercore-' are excluded from the matrix windows-2019 are excluded for example


[nitpick] The phrase 'windows-2019 are excluded for example' seems to be a typo and should be rephrased for clarity.

Suggested change

// Directories starting with 'windowsservercore-' are excluded from the matrix windows-2019 are excluded for example

// Directories starting with 'windowsservercore-' are excluded; 'windows-2019' is an example

2024-12-09T18:30:31Z

genMatrix.js


 const areTestFilesChanged = (changedFiles) => changedFiles
  .some((file) => testFiles.includes(file));

-// Returns a list of the child directories in the given path
+// Returns a list of the child directories in the given path, excluding those starting with 'windows-'


[nitpick] The comment should be 'excluding those starting with 'windowsservercore-'.

Suggested change

// Returns a list of the child directories in the given path, excluding those starting with 'windows-'

// Returns a list of the child directories in the given path, excluding those starting with 'windowsservercore-'

zZHorizonZz · 2024-12-10T06:53:32Z

Should I fix the nitpicks that Copilot made? I haven't really used this on GitHub, but there are commit suggestion actions available for both of these.

Squash commits

d5e9215

zZHorizonZz force-pushed the main branch from cb0fb52 to d5e9215 Compare September 3, 2024 10:44

LaurentGoderre reviewed Sep 3, 2024

View reviewed changes

Dockerfile-windows.template Outdated Show resolved Hide resolved

zZHorizonZz added 2 commits September 4, 2024 10:54

Add Official Support for Windows Docker Images

c77f154

Add fallback to gpg

880fca8

LaurentGoderre reviewed Sep 4, 2024

View reviewed changes

Dockerfile-windows.template Outdated Show resolved Hide resolved

LaurentGoderre reviewed Sep 4, 2024

View reviewed changes

Dockerfile-windows.template Outdated Show resolved Hide resolved

zZHorizonZz added 3 commits September 5, 2024 07:03

Added verbose logging and sha256 checksum for gpg

788df2f

Added link to gpg key and remove verbose logging

47843c9

Updated update.sh to allow skiping everything besides windows

537bc56

LaurentGoderre reviewed Sep 5, 2024

View reviewed changes

Dockerfile-windows.template Outdated Show resolved Hide resolved

zZHorizonZz added 3 commits September 6, 2024 06:58

Remove empty smoke test layer

0547704

Remove empty smoke test layer

b98f859

Fix linting errors

48e25c2

zZHorizonZz added 3 commits September 9, 2024 17:29

Renamed windows variant to windowsservercore

d5532b8

Fixed github action after rename

a8724fb

Fixed github action after rename

dbe54a6

Updated README.md to include information about windoservercore images.

e3485a7

zZHorizonZz force-pushed the main branch from 2aa62eb to e3485a7 Compare September 9, 2024 16:02

LaurentGoderre requested review from nschonni, SimenB and PeterDaveHello September 10, 2024 13:42

tianon reviewed Sep 12, 2024

View reviewed changes

Dockerfile-windows.template Show resolved Hide resolved

tianon reviewed Sep 12, 2024

View reviewed changes

Dockerfile-windows.template Outdated Show resolved Hide resolved

tianon reviewed Sep 12, 2024

View reviewed changes

Dockerfile-windows.template Outdated Show resolved Hide resolved

LaurentGoderre reviewed Sep 19, 2024

View reviewed changes

zZHorizonZz added 2 commits September 20, 2024 07:26

Remove gpg, multistage from windowservercore images and use NODE_CHEC…

79bf2fe

…KSUM Signed-off-by: Daniel Fiala <[email protected]>

Fixed build-test-windows.yml

41d4d9f

Signed-off-by: Daniel Fiala <[email protected]>

zZHorizonZz force-pushed the main branch from a88fb68 to 41d4d9f Compare September 20, 2024 06:24

zZHorizonZz requested review from tianon and LaurentGoderre September 20, 2024 06:29

PeterDaveHello requested a review from Copilot December 9, 2024 18:29

Copilot AI reviewed Dec 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Official Support for Windows Docker Images #2136

Add Official Support for Windows Docker Images #2136

zZHorizonZz commented Sep 3, 2024 •

edited

Loading

zZHorizonZz commented Sep 4, 2024 •

edited

Loading

zZHorizonZz commented Sep 5, 2024

LaurentGoderre commented Sep 9, 2024

zZHorizonZz commented Sep 9, 2024

zZHorizonZz commented Sep 10, 2024 •

edited

Loading

LaurentGoderre commented Sep 10, 2024

LaurentGoderre commented Sep 10, 2024

LaurentGoderre Sep 19, 2024 •

edited

Loading

zZHorizonZz Sep 20, 2024

LaurentGoderre Sep 20, 2024

zZHorizonZz Sep 20, 2024

LaurentGoderre Sep 20, 2024

zZHorizonZz commented Sep 22, 2024 •

edited

Loading

LaurentGoderre commented Sep 23, 2024

zZHorizonZz commented Sep 23, 2024

iancward commented Oct 25, 2024

Copilot AI left a comment

Copilot AI Dec 9, 2024

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Copilot AI Dec 9, 2024

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

zZHorizonZz commented Dec 10, 2024 •

edited

Loading

	// Directories starting with 'windowsservercore-' are excluded from the matrix windows-2019 are excluded for example
	// Directories starting with 'windowsservercore-' are excluded; 'windows-2019' is an example

	// Returns a list of the child directories in the given path, excluding those starting with 'windows-'
	// Returns a list of the child directories in the given path, excluding those starting with 'windowsservercore-'

Add Official Support for Windows Docker Images #2136

Are you sure you want to change the base?

Add Official Support for Windows Docker Images #2136

Conversation

zZHorizonZz commented Sep 3, 2024 • edited Loading

Description

Motivation and Context

Testing Details

Example Output (if appropriate)

Types of changes

Checklist

zZHorizonZz commented Sep 4, 2024 • edited Loading

zZHorizonZz commented Sep 5, 2024

LaurentGoderre commented Sep 9, 2024

zZHorizonZz commented Sep 9, 2024

zZHorizonZz commented Sep 10, 2024 • edited Loading

LaurentGoderre commented Sep 10, 2024

LaurentGoderre commented Sep 10, 2024

LaurentGoderre Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

zZHorizonZz Sep 20, 2024

Choose a reason for hiding this comment

LaurentGoderre Sep 20, 2024

Choose a reason for hiding this comment

zZHorizonZz Sep 20, 2024

Choose a reason for hiding this comment

LaurentGoderre Sep 20, 2024

Choose a reason for hiding this comment

zZHorizonZz commented Sep 22, 2024 • edited Loading

LaurentGoderre commented Sep 23, 2024

zZHorizonZz commented Sep 23, 2024

iancward commented Oct 25, 2024

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot AI Dec 9, 2024

Choose a reason for hiding this comment

Copilot AI Dec 9, 2024

Choose a reason for hiding this comment

zZHorizonZz commented Dec 10, 2024 • edited Loading

zZHorizonZz commented Sep 3, 2024 •

edited

Loading

zZHorizonZz commented Sep 4, 2024 •

edited

Loading

zZHorizonZz commented Sep 10, 2024 •

edited

Loading

LaurentGoderre Sep 19, 2024 •

edited

Loading

zZHorizonZz commented Sep 22, 2024 •

edited

Loading

zZHorizonZz commented Dec 10, 2024 •

edited

Loading