fix: uninstall download helpers from slim image #1182

SimenB · 2020-01-04T12:28:42Z

Should fix #1181.

/cc @tianon does this make sense?

SimenB · 2020-01-04T14:08:23Z

jessie-slim fails since it wants to delete apt. Any ideas on how to fix it?

LaurentGoderre · 2020-01-06T14:44:56Z

10/buster-slim/Dockerfile

@@ -17,7 +17,7 @@ RUN buildDeps='xz-utils' \
      *) echo "unsupported architecture"; exit 1 ;; \
    esac \
    && set -ex \
-    && apt-get update && apt-get install -y ca-certificates curl wget gnupg dirmngr $buildDeps --no-install-recommends \


I believe ca-certificates should not be moved to buildDeps.

if we don't uninstall ca-certificates we're back at square one since it's ca-certificates which pull in openssl

LaurentGoderre · 2020-01-06T14:45:53Z

How does it fix the issue @SimenB ?

SimenB · 2020-01-06T14:54:29Z

How does it fix the issue @SimenB ?

By not including openssl at all anymore - it's not part of the base image we extend from, it's installed as a side-effect of us downloading node

LaurentGoderre · 2020-01-06T20:38:27Z

Oooh, I see!

LaurentGoderre · 2020-01-06T20:39:55Z

This is a potentially breaking change for any image relying on that side-effect (Not that I am opposed ot it, just saying we might received a few bug reports afterwards).

nschonni · 2020-01-06T21:32:46Z

10/buster-slim/Dockerfile

-RUN set -ex \
+RUN buildDeps='ca-certificates curl wget gnupg dirmngr' \
+  && set -ex \
+  && apt-get update && apt-get install -y $buildDeps --no-install-recommends \


Does it need to install again for the second RUN?

Or should the apt-get purge just be moved down

It can't be moved down because of how layers work. It should be reinstalled again in the yarn step.

OK, I thought if the apt-get purge was removed from the previous RUN block, the packages would be available when running this layer too

the layers will be larger though. Ideally we'd have one large RUN which install both node and yarn in the same layer, but I didn't want to make such a big change.

Ah, thanks for clearing that up

It's the downside of the power of docker layers

In many repositories we've stopped using an explicit list of "things to install then remove later" exactly because of this problem of dependencies that we didn't install getting removed (or hand-maintaining a list of library dependencies after we've compiled something using them), instead opting to get "clever" with apt-mark to let APT itself do the heavy lifting (which then will only remove things we haven't explicitly said need to stay):

RUN set -eux; \ savedAptMark="$(apt-mark showmanual)"; \ apt-get update; \ apt-get install -y --no-install-recommends ca-certificates curl dirmngr gnupg wget; \ rm -rf /var/lib/apt/lists/*; \ \ # ... do stuff here ... \ apt-mark auto '.*' > /dev/null; \ apt-mark manual $savedAptMark > /dev/null; \ apt-get purge -y --auto-remove -o APT::AutoRemove::RecommendsImportant=false

See https://github.com/docker-library/php/blob/ae3f372b446b6457326504d9b9984ce708f20c3d/Dockerfile-debian.template#L65-L97 for one example of this pattern being used, and https://github.com/docker-library/php/blob/ae3f372b446b6457326504d9b9984ce708f20c3d/Dockerfile-debian.template#L196-L207 for a more complicated example of keeping appropriate lib* packages automatically.

Ah, nice! I've never even heard of apt-mark 😅 I'll change to use that, seems ideal

Pushed that now. apt-mark showmanual was empty, so I dropped that part

Starefossen · 2020-01-09T11:45:22Z

@SimenB this needs to be reverted, we can simply not "break" all existing slim images that relies on curl being there. Inadvertent or not. Going forward these types of changes needs to be reserved for major version (ref. SemVer)

As a minimum potentially and unintentionally breaking changes needs to be communicated or noted somewhere as opposed to a single comment in a pull request (ref. @LaurentGoderre).

Tapppi · 2020-01-09T14:50:38Z

@Starefossen Just to note, you already did. I am guessing that the docker images are released automatically and at least our build pipeline already broke on this and we had to switch to full images.

LaurentGoderre · 2020-01-09T15:00:30Z

@Tapppi you could install it as part of your image (or were you using the stock image as is?)

willdurand · 2020-01-10T13:11:00Z

We also noticed a regression in our build system because of this patch... In our case, the image cannot be built anymore because the gnupg has been removed.

Although I have no problem with this patch per se, I find it not very nice that such patches can break everything so easily and finding the root cause took more time that it should have taken (some sort of versioning/changelog would be appreciated).

Tapppi · 2020-01-10T15:43:26Z

@LaurentGoderre We could've, but we already swapped to the full image instead at least while this situation plays out. We do very few modifications to the image at that point, adding maintenance burden feels like an anti-goal in this case :)

cosmith · 2020-01-10T15:54:45Z

To add a data point, this broke our deployment with the following error:

Step 7/10 : RUN yarn install --frozen-lockfile --no-cache
---> Running in ba0b9a8ebe73
yarn install v1.21.1
[1/5] Validating package.json...
[2/5] Resolving packages...
[3/5] Fetching packages...
error Command failed.
Exit code: 128
Command: git
Arguments: ls-remote --tags --heads https://<user>:<key>@github.com/<org>/<repo>.git
Directory: /
Output:
fatal: unable to access 'https://<user>:<key>@github.com/<org>/<repo>.git/': Problem with the SSL CA cert (path? access rights?)

We reverted to node:lts from node:lts-slim but I'm still not sure what was removed that prevents yarn from accessing github (gnupg? openssl?). Adding curl only didn't solve the issue.

Update: installing gnupg and ca-certificates seems to do the trick

LaurentGoderre · 2020-01-10T20:07:10Z

@cosmith @Tapppi if you only need curl for npm or yarn install, you could use multi-stage builds that use full debian for the build stage and slim for the final stage.

LaurentGoderre · 2020-01-10T20:11:24Z

@cosmith I believe you only need ca-certificates

SimenB requested a review from a team January 4, 2020 12:28

LaurentGoderre reviewed Jan 6, 2020

View reviewed changes

nschonni reviewed Jan 6, 2020

View reviewed changes

fix: uninstall download helpers from slim image

f99660d

SimenB force-pushed the uninstall-slim-helpers branch from 96ebc38 to f99660d Compare January 8, 2020 08:30

SimenB requested a review from a team January 8, 2020 08:30

LaurentGoderre approved these changes Jan 8, 2020

View reviewed changes

SimenB merged commit add2e3f into nodejs:master Jan 8, 2020

SimenB deleted the uninstall-slim-helpers branch January 8, 2020 14:38

SimenB mentioned this pull request Jan 9, 2020

curl no longer shipped with node:slim #1185

Closed

willdurand mentioned this pull request Jan 10, 2020

Update Dockerfile to install gnupg mozilla/addons#13660

Closed

nschonni mentioned this pull request Jan 12, 2020

image arm32v7/node:13.6-slim not working #1195

Closed

amatissart mentioned this pull request Jan 13, 2020

Dockerfile: install ca-certificates explicitly Qwant/erdapfel#512

Merged

This was referenced Jan 15, 2020

openssl not found in node:12.14.1-slim #1200

Closed

Create CHANGELOG.md #1201

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: uninstall download helpers from slim image #1182

fix: uninstall download helpers from slim image #1182

SimenB commented Jan 4, 2020

SimenB commented Jan 4, 2020

LaurentGoderre Jan 6, 2020 •

edited

Loading

SimenB Jan 6, 2020

SimenB Jan 7, 2020

LaurentGoderre commented Jan 6, 2020

SimenB commented Jan 6, 2020 •

edited

Loading

LaurentGoderre commented Jan 6, 2020

LaurentGoderre commented Jan 6, 2020

nschonni Jan 6, 2020

nschonni Jan 6, 2020

LaurentGoderre Jan 6, 2020

nschonni Jan 6, 2020

SimenB Jan 7, 2020

nschonni Jan 7, 2020

LaurentGoderre Jan 7, 2020

tianon Jan 8, 2020

SimenB Jan 8, 2020

SimenB Jan 8, 2020

Starefossen commented Jan 9, 2020 •

edited

Loading

Tapppi commented Jan 9, 2020

LaurentGoderre commented Jan 9, 2020

willdurand commented Jan 10, 2020

Tapppi commented Jan 10, 2020

cosmith commented Jan 10, 2020 •

edited

Loading

LaurentGoderre commented Jan 10, 2020

LaurentGoderre commented Jan 10, 2020

fix: uninstall download helpers from slim image #1182

fix: uninstall download helpers from slim image #1182

Conversation

SimenB commented Jan 4, 2020

SimenB commented Jan 4, 2020

LaurentGoderre Jan 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LaurentGoderre commented Jan 6, 2020

SimenB commented Jan 6, 2020 • edited Loading

LaurentGoderre commented Jan 6, 2020

LaurentGoderre commented Jan 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Starefossen commented Jan 9, 2020 • edited Loading

Tapppi commented Jan 9, 2020

LaurentGoderre commented Jan 9, 2020

willdurand commented Jan 10, 2020

Tapppi commented Jan 10, 2020

cosmith commented Jan 10, 2020 • edited Loading

LaurentGoderre commented Jan 10, 2020

LaurentGoderre commented Jan 10, 2020

LaurentGoderre Jan 6, 2020 •

edited

Loading

SimenB commented Jan 6, 2020 •

edited

Loading

Starefossen commented Jan 9, 2020 •

edited

Loading

cosmith commented Jan 10, 2020 •

edited

Loading