Use slim node images to reduce download and running size

[spwoodcock]

Includes

• Built using node:18-slim instead of node:18.17.
  • I assumed we probably want patch versions of node that shouldn't be breaking.
  • Is the assumption correct, or should I change back to pinned 18.17?
• Merged RUN directives where possible
  • This was less significant than I thought, maybe 50 - 100 MB saving.
  • The dockerfile was missing a cleanup of the apt list here too:

    central/service.dockerfile

    Lines 21 to 22 in 774b491

    	apt-get install -y cron gettext postgresql-client-14

• Absolute paths for WORKDIR (as per dockerfile-utils linting).
• Add HEALTHCHECK. This is useful for docker-compose setups. It's ignored in Kubernetes.
• Remove EXPOSE command and replace with LABELs, as I think it's more useful.

Discussion

I image the reason for not using a slim image originally is because they don't have git installed for the
git describe --tags --dirty command. Solved with multi-stage.

This PR was originally discussed as part of #519.
Reducing the space required to build images may be useful on systems with very limited space.
However, the gains that can be made are rather small considering >1 GB of cached images shouldn't really be a problem for most.

There have been discussions around providing pre-built images and an open PR to facilitate this, so this discussion will be moot if that is merged.

There is always a tradeoff between cache image bloat and dockerfile readability.
However, in this example I personally think the original is much more readable:

FROM node:${node_version}-slim as intermediate
RUN apt-get update \
    && apt-get install -y --no-install-recommends \
        git \
    && rm -rf /var/lib/apt/lists/*
COPY . .
RUN mkdir /tmp/sentry-versions
RUN git describe --tags --dirty > /tmp/sentry-versions/central
WORKDIR /server
RUN git describe --tags --dirty > /tmp/sentry-versions/server
WORKDIR /client
RUN git describe --tags --dirty > /tmp/sentry-versions/client

compared to less readable & not best practice of cd in dockerfiles (for little gain):

FROM node:${node_version}-slim as intermediate
COPY . .
RUN apt-get update \
    && apt-get install -y --no-install-recommends \
        git \
    && rm -rf /var/lib/apt/lists/* \
    && mkdir /tmp/sentry-versions \
    && git describe --tags --dirty > /tmp/sentry-versions/central \
    && cd server && git describe --tags --dirty > /tmp/sentry-versions/server \
    && cd ../client && git describe --tags --dirty > /tmp/sentry-versions/client

Note that this is not the final build stage, so the discussion is about image cache bloat,
not intermediate image layer size.

[lognaturel]

Thanks for opening! I'm not entirely sure when we'll be able to take a focused look at this and #546, it may be after the current release goes out.

Built using node:18-slim instead of node:18.17

We do want to pin the minor version because there are more likely to be accidental breaking changes between minor versions and usually they're not critical to apply. We update minor versions when we release. We do want to leave the patch version floating so that a rebuild can get the latest patches (so we don't want e.g. node:18.17.1).

[spwoodcock]

Makes sense - thanks for clarifying!
Updated 😄

[lognaturel]

Couple of quick things to update. Other than that, this looks ready to me. Current plan is to merge it right after the release ~Dec 15. We may also end up publishing images in which case it's less important but let's still get it in first.

[lognaturel]

Thank you! Ready to merge after 2023.5 goes out. 🙏

[lognaturel]

Plot twist! In parallel I was trying to upgrade everything to use Node 20 but didn't have high hopes of it working because of some test failures I'd run into. But it's all resolved now and running well on staging. That means users will need to download new base images anyway, so let's go ahead and make this change for the upcoming release. We want all changes merged today so they're in for regression testing. I'll take over your branch since I know it's late on a Friday for you!

[lognaturel]

Before:

docker inspect -f "{{ .Size }}" central-service | numfmt --to=si
1.3G

After:

docker inspect -f "{{ .Size }}" central-service | numfmt --to=si
374M

Secrets goes from 1.1G to 201M and nginx only gets a change to the intermediate stage but that's still nice.

[lognaturel]

I image the reason for not using a slim image originally is because they don't have git installed

#249 (comment) has some context. The past attempt was to move to alpine. There used to be a lua script in the nginx container that didn't work well with that but it's been removed. We have historically been somewhat more concerned about download size than local storage size. The change to 20.10-slim will add ~60MB to the overall download because Enketo will pull the 20.10 image. I think that's ok and we can align Enketo in the next release.

I briefly considered changing the postgres image to alpine (there's no slim) but then I realized that this would result in more being downloaded because there's overlap in layers between the postgres14.10 image and node20.10.

[lognaturel]

The original PR description states that EXPOSE is deprecated. I see no evidence of this: https://docs.docker.com/engine/reference/builder/#expose

[yanokwa]

Expose is for documenting the port the builder of the image intends to publish. It doesn't actually publish it, so maybe that's the confusion? Either way, agreed that we can leave this.

[spwoodcock]

Sorry, my mistake!

I always leave out EXPOSE and prefer a LABEL, as expose doesn't actually do anything to the image.

Using a label allows a user to docker inspect to easily know which port they need to bind.

[yanokwa]

It's not a bad idea. Is it used anywhere else? Or documented somewhere as a best practice?

[spwoodcock]

Not as far as I can see!

For a long time LABEL had no standard set, so it's good to see opencontainers create one.

I added the port label from my own frustrations with using other images.
If the author doesn't have a label, then I had to hunt down either the dockerfile (if they use EXPOSE) or the application config to find the port.

Updates in my knowledge

• LABEL and ENV no longer require multiline definition. It's more readable to use individual lines.

• EXPOSE is now used by both docker and podman -P flag, to map all exposed ports to random ports on the host (does not seem useful to me, but maybe to some?).

[lognaturel]

Thanks again for getting these improvements in!

When doing docker inspect, doesn't Config/ExposedPorts show the ports that were specified with EXPOSE?

[spwoodcock]

You are correct!
That's an oversight from me.

I thought that EXPOSE was a useless command for years.
By the looks of it, it is actually a nice way to document via the metadata.

[lognaturel]

I think so! And downstream tools could use it programmatically, hopefully to do more useful things than random port binding. 😄 🤷🏻‍♀️ It's also very possible that Config/ExposedPorts is relatively new. There's a lot to learn and keep track of!

[lognaturel]

@yanokwa can you please do a sanity check? I'm satisfied that:

• it's a clean update
• there's user value
• it doesn't introduce anything non-standard
• it applies the improvement consistently and as broadly as it can

[lognaturel]

I just noticed that the Enketo image we produce automatically for GHCR gets some labels. I assume it's the Github action we use that does this:

  "labels": {
    "org.opencontainers.image.url": "https://github.com/enketo/enketo",
    "org.opencontainers.image.licenses": "Apache-2.0",
    "org.opencontainers.image.title": "enketo",
    "org.opencontainers.image.revision": "2f27f07b9d72eb27fb8576570501682b2f3ca5d2",
    "org.opencontainers.image.created": "2024-01-26T20:11:10.683Z",
    "org.opencontainers.image.version": "7.1.0",
    "org.opencontainers.image.description": "Enketo web forms monorepo",
    "org.opencontainers.image.source": "https://github.com/enketo/enketo"
  }

looks like version uses a tag if it exists or falls back to the branch name. I have not yet investigated whether this or what's specified in a Dockerfile takes precedence.

[spwoodcock]

Interesting - the part of the workflow that does it is here: https://github.com/enketo/enketo/blob/2f27f07b9d72eb27fb8576570501682b2f3ca5d2/.github/workflows/ghcr.yml#L50

I use docker/build-push-action a lot, but only ever set the tags from docker/metadata-action, not the labels 🤦

[lognaturel]

Looks like a good approach, and already at https://github.com/getodk/central/pull/546/files#diff-5b21991be47c2922383bdc0b6bf00b65af7db51b82d049dd8d6ad03e3d37ac98R68 🎉