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

Add non-root user for running uwsgi in Dockerfile #388

Merged
merged 14 commits into from Oct 9, 2020
Merged

Add non-root user for running uwsgi in Dockerfile #388

merged 14 commits into from Oct 9, 2020

Conversation

platipo
Copy link
Contributor

@platipo platipo commented Oct 4, 2020

Fixes #383

@aaron-junot
Copy link
Member

The error in CI is this

E   PermissionError: [Errno 13] Permission denied: 'log'

There was one point in time where we were writing logs to a file on the filesystem. But then we later realized we were deploying to kubernetes and that logging to stdout would be better for other tools to consume it. There's some dead code around logging to a file. If you want, you can open a separate PR to remove that code, and rebase this after we merge that which should fix the permissions issue

@aaron-junot
Copy link
Member

I merged #391 so you should be able to rebase and fix this

@Kandeel4411
Copy link
Collaborator

The problem that's popping up right now is due to docker-compose.yml volume. When running using docker the permissions of the directories persist and works great but since we have:

 volumes:
    - .:/src

In the docker compose file, it overwrites the permissions of the docker container with the current permissions of the host files which leads to the current errors

@Kandeel4411
Copy link
Collaborator

Kandeel4411 commented Oct 7, 2020

@aaron-suarez I think in order for us to fix this is to either add a separate docker-compose file for development and a separate one for production (which we could test with) Or just simply add an additional action in the CI config to change the ownership of the current directory to the new user (uwsgi). It shouldn't throw an error even if the user doesn't exist yet so it should work, but I am not sure. What do you think?

@platipo
Copy link
Contributor Author

platipo commented Oct 7, 2020

Thank you @Kandeel4411 for your expert advice.

I tried following many paths down the non-root rabbithole but none seems to go anywhere:

@aaron-junot
Copy link
Member

fwiw, I pulled down your branch and couldn't reproduce the current error locally. If you also can't figure it out, it might be something to do with the CircleCI user, and maybe we can make a special make command like make ci-tests that doesn't require outputting coverage information or something... I'm not sure if that will work, but it might be worth a try?

@Kandeel4411
Copy link
Collaborator

Kandeel4411 commented Oct 7, 2020

@platipo thanks :) thought I am by no means an expert! You did really great - I encountered this error before and I think its mostly related to permissions being overridden which sadly can't be helped ( as far as I know ) with docker-compose since it always uses the host permissions when mounting it as a volume. I think our main issue with the permissions was because of the config.yml of CircleCI having this step in its job:

run: sudo chown -R circleci:circleci /usr/local/bin

Which made the host files permissions be only to the circleci user. I created a separate branch (Just to test) with an edit to the config to simply add this extra step before running the tests:

run: sudo chown -R 5000 .

and in the Dockerfile when creating the user we assign the above uid

RUN useradd -ms /bin/bash --uid 5000 uwsgi

Since chown doesn't check if a uid exists yet or not, it was able to work and the checks are passing

@Kandeel4411
Copy link
Collaborator

You could cherry pick or make the same edit here and it should be able to work. We have to test it on staging though to check if there is no issues running there (hence it being a CircleCI problem only)

@aaron-junot
Copy link
Member

Sounds good to me. I also should have branch permissions so if you need me to cherry pick the commit and push it to this branch, I can. Just let me know @platipo

@platipo
Copy link
Contributor Author

platipo commented Oct 8, 2020

Thanks @Kandeel4411 and @aaron-suarez for your kind support!
I pulled directly from your branch and applied the commits onto my previous work.

I have been thinking for a bit on a cleaner solution to the permission problem and came up with two alternatives:

  • Changing the entry-point to a bash script that fixes at run time the non-root user permissions (like in fixuid)
  • Adding a UID environment variable to the .env file and propagate it from the docker-compose.yml to the Dockerfile (I don't know if it could work for the CircleCi config it seems there is an orb for passing varibles from .env files)

@Kandeel4411
Copy link
Collaborator

I think we could go with the current solution for this and open a new issue with those alternatives to decide later as they do seem quite cleaner, or we could just implement one here. What do you think @aaron-suarez?

@aaron-junot
Copy link
Member

@platipo I'm good with this solution. Please rebase and we'll merge it. Thanks for this!!

If you want to follow up with any clean ups like the orb and all that, feel free to

@platipo
Copy link
Contributor Author

platipo commented Oct 9, 2020

@aaron-suarez all done!

Do you think it's better to open another issue for the cleanup to keep things clean?

@aaron-junot aaron-junot merged commit 96233c6 into OperationCode:main Oct 9, 2020
@aaron-junot
Copy link
Member

@platipo either open an issue or just open a pull request. If you're going to do it yourself, no need for an issue. If you think there's a chance you'll never come back to it, opening an issue allows someone else to pick it up

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dockerfile should not be root
3 participants