Skip to content
This repository has been archived by the owner on May 31, 2021. It is now read-only.

docker: use node lts and run the app as flood user #837

Closed
wants to merge 5 commits into from
Closed

docker: use node lts and run the app as flood user #837

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3ca47b9
:whale2: use node lts and run the app as flood user
mycodeself Sep 25, 2019
25409cc
Merge branch 'master' into master
jfurrow Sep 26, 2019
0434bf2
Add suggestions thanks @MacTwister + @jfurrow
Sep 28, 2019
4c754b6
Update Dockerfile
mycodeself Oct 4, 2019
d004b52
Merge branch 'master' into master
mycodeself Oct 25, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 21 additions & 9 deletions Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,23 @@
ARG NODE_IMAGE=node:12.2-alpine
ARG NODE_IMAGE=node:10.16-alpine
mycodeself marked this conversation as resolved.
Show resolved Hide resolved
ARG WORKDIR=/usr/src/app/
ARG FLOOD_BASE_URI=/
mycodeself marked this conversation as resolved.
Show resolved Hide resolved

FROM ${NODE_IMAGE} as nodebuild
ARG WORKDIR
ENV FLOOD_BASE_URI $FLOOD_BASE_URI

WORKDIR $WORKDIR

# Generate node_modules
COPY package.json \
package-lock.json \
.babelrc \
.eslintrc.js \
.eslintignore \
.prettierrc \
ABOUT.md \
$WORKDIR
package-lock.json \
Copy link
Member

Choose a reason for hiding this comment

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

nit: Can we restore the previous, unless there's a good reason to change it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The previous indentation has 5 spaces. The rest of file has 4 spaces, i think it should use the same. What do you think? If you want to mantain 5 spaces in these lines, i can revert it.

.babelrc \
.eslintrc.js \
.eslintignore \
.prettierrc \
ABOUT.md \
$WORKDIR

RUN apk add --no-cache --virtual=build-dependencies \
python build-base && \
npm install && \
Expand All @@ -39,10 +42,19 @@ WORKDIR $WORKDIR
RUN apk --no-cache add \
mediainfo

# Add user to run the application
RUN addgroup -S flood
RUN adduser -S flood -G flood
RUN mkdir /data
RUN chown flood:flood /data
Copy link

Choose a reason for hiding this comment

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

I am concerned if adding a user now will break existing setups, looking at existing docker volume for /data currently there are files owned by the root user which when the app runs under the flood user would in theory not have access the database file. Rebuilding the container will not change the files/owners of existing files on docker volumes.

I will have a quick test on my local setup to see what effects there are on my setup when a build new image with these changes, may require users to manually run chown after re-building the image.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have you been able to test how it affects current configurations?

Copy link
Contributor

Choose a reason for hiding this comment

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

@MacTwister maybe the file are own by root because the docker image was built and the default user was root but if you build it and add another user the file will be own by another user.

Copy link

Choose a reason for hiding this comment

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

Yes, so now if you build the docker image on your side using this Dockerfile (with chown changes) then the app will be owned and run as the flood user. 👍

For new setups using docker:

New container will run as flood user and create the database under /data owned by the flood user. All good here.

For existing setups using docker:

The scenario I am describing is for people already running with Docker and have setup the /data folder as a "volume" to persist the DB when restarting/upgrading the docker image. Below is simplified version to how I have my setup:

# ... rest of you docker compose file
  floodui:
    build: .
    ports:
      - 3000:3000
    volumes:
      - flood_data:/data

So when re-creating the container with new image, the app will crash with the following error:

[Error: EACCES: permission denied, open '/data/server/db/users.db'] {

This is because my volume /data is persisted over from the previous container, where it was first created (when container was root only) so my version of /data is owned by root:root.

julian$ docker-compose run --rm floodui ls -l /data
  total 4
  drwxr-xr-x    3 root     root          4096 Oct  3 08:41 server

What I am saying is that, if we go through with this change, we'd need to some how inform users what they would need to do to continue working after this upgrade. i.e. update the docs for docker setup to inform the to update the /data ownership manually.

What I needed to do using my docker compose setup to continue working:

julian$ docker-compose run --user=root --rm floodui chown -R flood:flood /data

RUN chown flood:flood ${WORKDIR}

USER flood

COPY --from=nodebuild $WORKDIR $WORKDIR
mycodeself marked this conversation as resolved.
Show resolved Hide resolved

# Hints for consumers of the container.
EXPOSE 3000
EXPOSE 3000
VOLUME ["/data"]

# Start application.
Expand Down