goldbergyoni / nodebestpractices

:white_check_mark: The Node.js best practices list (July 2024)
https://twitter.com/nodepractices/
Creative Commons Attribution Share Alike 4.0 International
99.4k stars 10.11k forks source link

Change bootstrap with node in Docker to run with dumb-init / tini #1122

Open rluvaton opened 2 years ago

rluvaton commented 2 years ago

Right now, we advise people to use the CMD ["node", "server.js"] while as stated in 10 best practices to containerize Node.js web applications with Docker by @lirantal and @goldbergyoni (and in a great talk given by Liran in NodeTLV 2021) we should use dumb-init or tini to start our application

To quote the Node.js Docker working group recommendation on this: “Node.js was not designed to run as PID 1 which leads to unexpected behavior when running inside of Docker. For example, a Node.js process running as PID 1 will not respond to SIGINT (CTRL-C) and similar signals”.

The way to go about it then is to use a tool that will act like an init process, in that it is invoked with PID 1, then spawns our Node.js application as another process whilst ensuring that all signals are proxied to that Node.js process. If possible, we’d like a small as possible tooling footprint for doing so to not risk having security vulnerabilities added to our container image.

From (the same article mentioned above) Why you should use a tool that will act like an init process

While we mention tini we don't recommend it as the best way to go and explain the danger of not using it

cc @goldbergyoni

goldbergyoni commented 2 years ago

@rluvaton When we wrote the Docker section this was evaluated and there was a reason that we did not recommend this. What is the reason? I forgot:)

One argument for using tini was about closing child-processes when the main thread dies, but very few Node app do use child processes. The other reason that you mentioned that Node won't react to signals, I'd like to verify this. If it's true, let's update the recommendation. Can you verify?

stale[bot] commented 2 years ago

Hello there! 👋 This issue has gone silent. Eerily silent. ⏳ We currently close issues after 100 days of inactivity. It has been 90 days since the last update here. If needed, you can keep it open by replying here. Thanks for being a part of the Node.js Best Practices community! 💚

rluvaton commented 2 years ago

Yeah, just tested it and it doesn't get the SIGHUP and possibly other signals as well

Repro ## `index.js`: ```js function handle(signal) { console.log(`*^!@4=> Received event: ${signal}`) } let seconds = 0; setInterval(() => { console.log(++seconds) }, 1000); process.on('SIGHUP', handle) ``` ## `Dockerfile`: ```dockerfile # Uncomment to use the fixed version FROM node:alpine # RUN apk add dumb-init WORKDIR /usr/src/app COPY --chown=node:node . /usr/src/app RUN npm i USER node CMD "npm" "start" #CMD ["dumb-init", "node", "index.js"] ``` ## `package.json`: ```json { "name": "vanilla-js", "version": "1.0.0", "scripts": { "start": "node index.js" } } ``` And run: ```console # Terminal 1: $ docker build -t repro-1122 . && docker run --name repro-1122 --rm repro-1122 # Terminal 2: $ docker kill --signal=SIGHUP repro-1122 ```
lirantal commented 2 years ago

Had a doubt? I tested this before writing the article :-)

rluvaton commented 2 years ago

Had a doubt?

I tested this before writing the article :-)

Thanks for joining in!

Although there are some signals that do pass 🤔

codespearhead commented 1 year ago

@rluvaton When we wrote the Docker section this was evaluated and there was a reason that we did not recommend this. What is the reason? I forgot:)

One argument for using tini was about closing child-processes when the main thread dies, but very few Node app do use child processes. The other reason that you mentioned that Node won't react to signals, I'd like to verify this. If it's true, let's update the recommendation. Can you verify?

If you run the following code it'll print out positive integers on the console until you press CRTL+C, which will stop the Node process.

(function counter() {
  let count = 0;
  setInterval(() => {
    count++;
    console.log(count);
  }, 1000);
})();

However, SIGINIT (CRTL+C) will be ignored if you run it in a container, which is expected behavior as described here.

Minimal Reproducible Example

  1. Save the content below in a Dockerfile
FROM node:18.13-bullseye

# Uncomment the last three lines of one of the code blocks below
# to see that once you do, you'll be able to stop the Node process
# by sending SIGINT (CRTL+C).

# For Tini
# ENV TINI_VERSION v0.19.0
# ADD https://github.com/krallin/tini/releases/download/${TINI_VERSION}/tini /tini
# RUN chmod +x /tini
# ENTRYPOINT ["/tini", "--"]

# For dumb-init
# RUN wget -O /usr/bin/dumb-init https://github.com/Yelp/dumb-init/releases/download/v1.2.5/dumb-init_1.2.5_x86_64
# RUN chmod +x /usr/bin/dumb-init
# ENTRYPOINT ["/usr/bin/dumb-init", "--"]

RUN echo "(function counter() { let count = 0; setInterval(() => { count++; console.log(count); }, 1000); })();" >> app.js

CMD ["node", "app.js"]
  1. Build and run the container using the following commands in the same folder as the Dockerfile
docker build -t "init-mre" .
docker run --rm "init-mre"
saard commented 4 months ago

Hi, I know this is a very old repo / issue, however I tested my application with node 22 inside of a docker container and I was able to receive the SIGINT successfully.

{"level":30,"time":1715033483998,"pid":1,"hostname":"31a5ac8ebd91","msg":"Server listening at http://0.0.0.0:3000"} {"level":50,"time":1715033504502,"pid":1,"hostname":"31a5ac8ebd91","msg":"SIGINT received"}