Skip to content
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

Use slim node images to reduce download and running size #549

Merged
merged 9 commits into from
Dec 9, 2023
9 changes: 8 additions & 1 deletion nginx.dockerfile
Original file line number Diff line number Diff line change
@@ -1,10 +1,17 @@
FROM node:20.10 as intermediate
FROM node:20.10-slim as intermediate

RUN apt-get update \
&& apt-get install -y --no-install-recommends \
git \
&& rm -rf /var/lib/apt/lists/*

COPY ./ ./
RUN files/prebuild/write-version.sh
ARG OIDC_ENABLED
RUN OIDC_ENABLED="$OIDC_ENABLED" files/prebuild/build-frontend.sh



# when upgrading, look for upstream changes to redirector.conf
# also, confirm setup-odk.sh strips out HTTP-01 ACME challenge location
FROM jonasal/nginx-certbot:4.2.0
Expand Down
3 changes: 2 additions & 1 deletion secrets.dockerfile
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
FROM node:20.10
FROM node:20.10-slim

COPY files/enketo/generate-secrets.sh ./
59 changes: 45 additions & 14 deletions service.dockerfile
Original file line number Diff line number Diff line change
@@ -1,38 +1,69 @@
ARG node_version=20.10
FROM node:${node_version} as intermediate



FROM node:${node_version}-slim as pgdg
RUN apt-get update \
&& apt-get install -y --no-install-recommends \
ca-certificates \
curl \
gpg \
&& rm -rf /var/lib/apt/lists/* \
&& update-ca-certificates
RUN echo "deb http://apt.postgresql.org/pub/repos/apt/ $(grep -oP 'VERSION_CODENAME=\K\w+' /etc/os-release)-pgdg main" \
| tee /etc/apt/sources.list.d/pgdg.list \
&& curl https://www.postgresql.org/media/keys/ACCC4CF8.asc \
| gpg --dearmor > /etc/apt/trusted.gpg.d/apt.postgresql.org.gpg



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
WORKDIR /server
RUN git describe --tags --dirty > /tmp/sentry-versions/server
WORKDIR ../client
WORKDIR /client
RUN git describe --tags --dirty > /tmp/sentry-versions/client

FROM node:${node_version}

WORKDIR /usr/odk

RUN apt-get update && apt-get install wait-for-it && rm -rf /var/lib/apt/lists/*
FROM node:${node_version}-slim

RUN echo "deb http://apt.postgresql.org/pub/repos/apt/ $(grep -oP 'VERSION_CODENAME=\K\w+' /etc/os-release)-pgdg main" | tee /etc/apt/sources.list.d/pgdg.list && \
curl https://www.postgresql.org/media/keys/ACCC4CF8.asc | gpg --dearmor > /etc/apt/trusted.gpg.d/apt.postgresql.org.gpg && \
apt-get update && \
apt-get install -y cron gettext postgresql-client-14
ARG node_version
LABEL org.opencontainers.image.source="https://github.com/getodk/central"

COPY files/service/crontab /etc/cron.d/odk
WORKDIR /usr/odk

COPY server/package*.json ./

RUN npm clean-install --omit=dev --legacy-peer-deps --no-audit --fund=false --update-notifier=false
COPY --from=pgdg /etc/apt/sources.list.d/pgdg.list \
/etc/apt/sources.list.d/pgdg.list
COPY --from=pgdg /etc/apt/trusted.gpg.d/apt.postgresql.org.gpg \
/etc/apt/trusted.gpg.d/apt.postgresql.org.gpg
RUN apt-get update \
&& apt-get install -y --no-install-recommends \
gpg \
cron \
wait-for-it \
gettext \
procps \
postgresql-client-14 \
netcat-traditional \
&& rm -rf /var/lib/apt/lists/* \
&& npm clean-install --omit=dev --legacy-peer-deps --no-audit \
--fund=false --update-notifier=false

COPY server/ ./
COPY files/service/scripts/ ./

COPY files/service/config.json.template /usr/share/odk/
COPY files/service/crontab /etc/cron.d/odk
COPY files/service/odk-cmd /usr/bin/

COPY --from=intermediate /tmp/sentry-versions/ ./sentry-versions

EXPOSE 8383
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@yanokwa yanokwa Dec 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for getting these improvements in!

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!