Add Official Support for Windows Docker Images #2136
Conversation
|
I see something weird going on in the GitHub action when running the 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. |
|
Can you create a docs update PR for this? |
I have updated the README to include a section about the windowsservercore variant. |
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. |
|
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. |
|
I will let the other members review it but LGTM |
Dockerfile-windows.template
Outdated
| 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' } ; \ |
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
Aren't these two different checks though? One checks it wasn't modified, the other checks it came from the release team
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ah yeah, my bad. We might be good to just have the hardcoded checksum and no gpg
…KSUM Signed-off-by: Daniel Fiala <danfiala23@gmail.com>
Signed-off-by: Daniel Fiala <danfiala23@gmail.com>
|
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. |
|
@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. |
|
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. |
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
Checklist