Docker-node: curl no longer shipped with node:slim

[scooper91]

Hi,

curl used to be available on node:slim. This morning our build failed as curl is now not installed.

Old node:12-slim image:

root@4d4a38196442:/# curl --version
curl 7.52.1 (x86_64-pc-linux-gnu) libcurl/7.52.1 OpenSSL/1.0.2t zlib/1.2.8 libidn2/0.16 libpsl/0.17.0 (+libidn2/0.16) libssh2/1.7.0 nghttp2/1.18.1 librtmp/2.3
Protocols: dict file ftp ftps gopher http https imap imaps ldap ldaps pop3 pop3s rtmp rtsp scp sftp smb smbs smtp smtps telnet tftp 
Features: AsynchDNS IDN IPv6 Largefile GSS-API Kerberos SPNEGO NTLM NTLM_WB SSL libz TLS-SRP HTTP2 UnixSockets HTTPS-proxy PSL

New node:12-slim image:

root@250a454b1ac1:/# curl --version
bash: curl: command not found

I've tried node:10-slim and node:12-slim and curl is also not installed.

Is this intentional?

Thanks!

[SimenB]

Hey! Yes, this was intentional as it was just installed as a side effect of us downloading node into the base debian images.

See #1182 (also https://github.com/nodejs/docker-node/pull/1182#issuecomment-571305071)

The reason is that we don't wanna add any packages beyond node, npm and yarn into the base images. It also makes the image more than 10% smaller

node                12.14.1-slim         918c7b4d1cc5        11 hours ago        137MB
node                12.14.0-slim         bc30bd223212        11 days ago         157MB

[Starefossen]

IMHO this should have waited to the next major version. Even through this was unintended this is still a breaking change for many users.

[LaurentGoderre]

@Starefossen maybe but this changes might prevent creating security issues in the future

[SimenB]

🙁

I'd rather not revert. For people wanting curl (or whatever else was removed) it's as simple as apt-get update && apt-get install curl -y. By choosing to use the slim image you're opting out of having a bunch of convenience packages installed, and I think we should try to respect that choice.

On the other hand, I very much understand it's a breaking change, but juggling multiple versions of the templates just so people don't have to install curl doesn't seem worth the maintenance burden?

That said, I don't feel strongly about this, so feel free to open up a PR with a revert if you want - I won't oppose it at all. However, I wonder if a pinned issue and clear messaging ("Run apt…" etc) is enough - it's trivial for people who relied on the previous behavior to fix, and for everybody else node:slim no longer implicitly lies about what the image is by installing a bunch of extra packages.

[lukebatchelor]

Hiya 👋

I'm 100% never a fan of useless +1 messages or pileons, but is there any chance this might be reconsidered? This is causing a ton of build's to fail around the world now.

Completely understand that it might have been installed unintentionally, but curl is super widely used and no image based on node-slim is going to have reinstalled curl if it was already there previously.

[jjcm]

+1 as well. I totally get not wanting package bloat, but this is curl we're talking about here, not some obscure npm package. Curl is something that I just straight up just expect to be on a system, like ls. I get wanting to reduce the size of this because it's the slim version of the image, but I'll definitely echo what @Starefossen mentioned - a change that breaks tons of build scripts is something that shouldn't be pushed out in a minor version update.

[SimenB]

Curl is something that I just straight up just expect to be on a system, like ls.

There's not a single slim image that ships with curl, at least from any one I tested (I tried to pick out programming languages from the list of all official images, but you're welcome to try others if I missed any: https://github.com/docker-library/official-images/tree/master/library).

$ docker run --rm ruby:slim curl --version
docker: Error response from daemon: OCI runtime create failed: container_linux.go:346: starting container process caused "exec: \"curl\": executable file not found in $PATH": unknown.
$ docker run --rm python:slim curl --version
docker: Error response from daemon: OCI runtime create failed: container_linux.go:346: starting container process caused "exec: \"curl\": executable file not found in $PATH": unknown.
$ docker run --rm erlang:slim curl --version
docker: Error response from daemon: OCI runtime create failed: container_linux.go:346: starting container process caused "exec: \"curl\": executable file not found in $PATH": unknown.
$ docker run --rm swift:slim curl --version
docker: Error response from daemon: OCI runtime create failed: container_linux.go:346: starting container process caused "exec: \"curl\": executable file not found in $PATH": unknown.

# None of these have a `slim` variant, so I tested with `alpine` which is way smaller than `slim`
$ docker run --rm openjdk:alpine curl --version
docker: Error response from daemon: OCI runtime create failed: container_linux.go:346: starting container process caused "exec: \"curl\": executable file not found in $PATH": unknown.
$ docker run --rm golang:alpine curl --version
docker: Error response from daemon: OCI runtime create failed: container_linux.go:346: starting container process caused "exec: \"curl\": executable file not found in $PATH": unknown.

# This one actually has it
$ docker run --rm php:alpine curl --version
curl 7.67.0 (x86_64-alpine-linux-musl) libcurl/7.67.0 OpenSSL/1.1.1d zlib/1.2.11 nghttp2/1.40.0
Release-Date: 2019-11-06
Protocols: dict file ftp ftps gopher http https imap imaps pop3 pop3s rtsp smb smbs smtp smtps telnet tftp
Features: AsynchDNS HTTP2 HTTPS-proxy IPv6 Largefile libz NTLM NTLM_WB SSL TLS-SRP UnixSockets

So only php:alpine ships with curl, while none other do.

---

Our docs on the slim variant states (from https://github.com/nodejs/docker-node/blob/ba024f05bc22fb68d3245af06980753dfbff20fc/README.md#nodeslim):

This image does not contain the common packages contained in the default tag and only contains the minimal packages needed to run node.

curl (or wget, openssl etc. that was also removed) is not needed at all to run node, and thus it's a clear bug that they were added - going both against what all other official images do and what our own docs state.

---

With that I'll unsubscribe from this thread. Again, if any other maintainers (/cc @nodejs/docker) thinks this is too breaking and wanna revert it, go ahead

[PeterDaveHello]

Hey @scooper91 @lukebatchelor @jjcm, sorry for the breaking changes here, I agree with @Starefossen that the breaking changes should be delivered with major version, no matter it's intended or not, it's not a good idea to ship breaking changes without users' expectation.

As what's done it's done, maybe it's not a must to revert #1128, IMO it's more important to prevent similar situation happened again (but I'm open to have a vote or discussion about revert it or not, if someone strongly suggest to).

So, instead of revert #1128, I'll suggest that known breaking changes need more approvals before being merged in the future, in this case, the PR received only one approval from the @nodejs/docker team members, which is a little bit not enough, we could ping the team for more discussion and approval next time, for breaking changes, I suggest we have approvals from half of the team members, I know I didn't show up in time in this PR, and because we all volunteered our spare time to maintain the repository, sometimes the process won't be very agile, we'll try our best to make it agile.

Anyone who's interested in helping us and the project, feel free to join the discussion of issues and pull requests, send and help review patches, I'd like to invite those contributors who're active in the project to join @nodejs/docker!
(We currently have only 6 members, and only half of the members are very active)

[scooper91]

Hey @PeterDaveHello !

Aside from the issue of it being a breaking change, I think one of the main things from my side was that without digging through commits/PRs, I couldn't tell whether this was a desired change. It would be useful to have a change log, so people can quickly tell what has changed (apologies if there is one which I've missed!).

I agree that the process should be as agile and as lean as possible. I think the team members should be trusted to make the right decisions (without any worry of blame!), but be empowered to require more team members to approve a change if necessary. For simple changes, it seems unnecessary to enforce that multiple people have to approve a PR.

[PeterDaveHello]

Hi @scooper91,

Thanks for the feedback, yes, I think we should at least have a place to describe breaking changes, but I'm not sure if we have enough resource to have a full changelog, would you like to open an issue for everyone who's interested in to discuss about the details? (I know that I can open it, but as a maintainer, I'd always like to encourage users to engage, contribute back, to make the project better and better!)

And yes, for simple changes, it's not required to have additional approvals, only breaking changes or potential breaking changes must have it.

[LaurentGoderre]

@jjcm the problem is that by including curl as a side effect with this package, curl or any of its dependency would get security updates like we normally would from base images. When the debian image we use as base are updated, we inherit this update. However, for any package we install ourselves, we do not get these automatic update.

Perhaps the merge was a bit hasty but I believe we had to do it at one point or another to fix a security problem.

[theogravity]

For those who have been searching (like me!) on how to get wget back in, add this to your dockerfile:

RUN apt-get update && apt-get install -y ca-certificates wget