-
Notifications
You must be signed in to change notification settings - Fork 68
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
Make the Dockerfile cross-compile without qemu #801
base: main
Are you sure you want to change the base?
Conversation
Dockerfile
Outdated
|
||
|
||
# Stage 3: Build the final runtime image | ||
FROM --platform=$TARGETPLATFORM gcr.io/distroless/nodejs${NODEJS_VERSION}-debian${DEBIAN_VERSION}:nonroot AS runtime |
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.
Please please note that this switches the end image to a distroless-based one, with none of the base tools. It should be lighter and more secure, but it also makes it that you can't pop a basic shell in the container anymore. I can roll back this change as needed, or consider using the nonroot-debug
image, which includes busybox
(so a light shell and common utilities)
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 tend to prefer at least a base shell for inspection over lightness, how bad would it be to have the debug one?
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.
So considering it's node anyway, so if you have an RCE, not having a shell will probably not stop you form doing anything anyway.
I see three options:
- provide both a debug and non-debug variant (which we basically get for free at this point) with two build targets. This is what I did on MAS: https://github.com/matrix-org/matrix-authentication-service/blob/d6130176b526f9e490ba6bfd2949694ddd6cc665/Dockerfile#L166-L186
- flip to the debug variant if there is no value of having a non-debug one
- go back to using
library/node
directly if we're not really concerned about lightness. It would also let us build in a bookworm-based environment, since the distroless images don't have Debian 12-based images yet
I really don't have strong opinions on any of those options 🤷
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 ended up going back to library/node, which allowed me to:
- pin the nodejs version better
- make it based on bookworm
- have a proper shell, avoid breaking anything in the image
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.
This looks great wow! Thanks so much for spending the time on it. I've added a few questions or thoughts where I had them, but I'd be happy to go with this.
Dockerfile
Outdated
# syntax = docker/dockerfile:1.4 | ||
|
||
# The Debian version and version name must be in sync | ||
ARG DEBIAN_VERSION=11 |
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.
should we be using bookworm as latest stable or is it better not to?
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.
See the other comment below, because distroless images don't support Debian 12 yet: GoogleContainerTools/distroless#1337
Dockerfile
Outdated
ARG NODEJS_VERSION=18 | ||
ARG CARGO_ZIGBUILD_VERSION=0.16.12 | ||
# This needs to be kept in sync with the version in the package.json | ||
ARG MATRIX_SDK_VERSION=0.1.0-beta.6 |
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.
No way for us to extract this? we're totally going to forget!
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 mean, I expect that the next release will have the PR that avoids having to do that in the first place merged: matrix-org/matrix-rust-sdk-crypto-nodejs#8
|
||
# We need rustup so we have a sensible rust version, the version packed with bullsye is too old | ||
RUN curl https://sh.rustup.rs -sSf | sh -s -- -y --profile minimal | ||
RUN curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y --default-toolchain "${RUSTC_VERSION}" --profile minimal |
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.
what's the explicit tls / proto for?
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 to ensure that there is no TLS downgrade attack 🤷
It's the recommended flags you have on https://rustup.rs
Dockerfile
Outdated
|
||
|
||
# Stage 3: Build the final runtime image | ||
FROM --platform=$TARGETPLATFORM gcr.io/distroless/nodejs${NODEJS_VERSION}-debian${DEBIAN_VERSION}:nonroot AS runtime |
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 tend to prefer at least a base shell for inspection over lightness, how bad would it be to have the debug one?
1a8151b
to
957a92e
Compare
bd552b3
to
071266a
Compare
Thanks @AndrewFerr for the crypto SDK bindings release! I was able to remove the manual SDK download workaround 🎉 I had to upgrade via the package.json's resolutions field to override the sub-dependency, and it would require a new release of @vector-im/matrix-bot-sdk. Also note that the upgrade does deprecate the sled store, and I have not updated the docs accordingly. What I would suggest we do is:
|
This changes the Dockerfile so that it can be fully cross-compiled to arm64 & x86_64 without any qemu emulation.
This should mean way less time spent building the arm64 image in CI.
This also means better caching between the two architectures.
I had to bump the NAPI CLI to a 3.0 alpha version, because the new version has a handy
--cross-compile
flag, which automatically invokescargo-zigbuild
if needed.