freedomofpress / securethenews

An automated scanner and web dashboard for tracking TLS deployment across news organizations
https://securethe.news
GNU Affero General Public License v3.0
102 stars 25 forks source link

Add USERID arg to prod Dockerfile, default to 1001 in dev #240

Closed maeve-fpf closed 4 years ago

maeve-fpf commented 4 years ago

In production, we want to explicitly specify a UID when running containers. We had USERID as a dockerfile arg for the dev Django and Node dockerfiles, but not for prod. So, this makes all of them consistent with a default UID for whichever user (gcorn or node).

While this is ready for review, I'd also like to bring up why the UID was an arg in dev -- the Makefile and docker-compose.yml build containers with it to the UID of the local user on the host running docker-compose(!?!!!????). This seems bad and I don't know what it was trying to solve (looks like Mike added it a long time ago). Unless someone has a reason not to I'd like to remove this and let the UID be the default.

My dev environment looks like it's running fine with this, but please test and let me know if it breaks anything.

harrislapiroff commented 4 years ago

I think this is so that containers can edit the same volume for creating and deleting a .node_complete file when node finishes building. However, if it's working locally for you that seems fine!

(I also personally think this .node_complete thing is a bit of a weird hack and it leaves the file stray when I do something like run a one off command on the django container—without booting the node one—so I think it might just be preferable for the containers not to wait for each other even if it means frontend files might not be available if I'm too hasty loading the site.)

maeve-fpf commented 4 years ago

Okay, if the requirement is just "The UIDs are the same" then I think passing any UID to docker-compose.yml is sufficient. But better to be explicit about it than rely on the default ARGs that I added, so I'll just change that one line in Makefile from $(id -u) to something static.

conorsch commented 4 years ago

All of the UID munging was an attempt to avoid root-owned volumes, particularly on those node_modules dirs. Definitely makes sense to be explicit in the build args as you suggest, @maeve-fpf. Unclear to me why we're setting 1000 in some places and 1001 in others—can't we just use 1000 everywhere, as the PR title suggests?

maeve-fpf commented 4 years ago

I recall having a conflict with my local UID being 1000 (default on Debian for the first user account) when testing this, although looking at it now I'm not sure why. I'll update either the code or the title when rebasing this in a sec.

maeve-fpf commented 4 years ago

OK, it's passed as 1000 now.

maeve-fpf commented 4 years ago

Aha! 1000 didn't fail for me, it failed on CI: https://circleci.com/gh/freedomofpress/securethenews/1847?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

This is because node already contains a 1000 user, which doesn't matter in prod because the prod Dockerfile only uses node as part of a multi-stage build, copying in files from it to the python-based image and tossing the rest, but the dev docker-compose setup (only dev) builds a separate Node container which does have node's root and thus predefined user accounts.