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

If container running as non-root don't use gosu. #358

Closed
wants to merge 1 commit into from

Conversation

lkysow
Copy link
Member

@lkysow lkysow commented Nov 22, 2018

In OpenShift containers are run as a random user id. In this case, we
don't need to use gosu to step down from root since we're already not root.

This PR also makes the atlantis home directory owned by the root group and
puts atlantis in that group. This will allow OpenShift users to use /home/atlantis
as their data dir.

Todos:

  • Test that existing k8s users can still run with fsGroup: 01000

Notes:

@codecov
Copy link

codecov bot commented Nov 22, 2018

Codecov Report

Merging #358 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #358   +/-   ##
=======================================
  Coverage   70.72%   70.72%           
=======================================
  Files          61       61           
  Lines        3676     3676           
=======================================
  Hits         2600     2600           
  Misses        895      895           
  Partials      181      181

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f1cced0...9532e93. Read the comment docs.

@lkysow
Copy link
Member Author

lkysow commented Nov 22, 2018

@jocelynthode I think this is all that is needed for you to run Atlantis on OpenShift. I've pushed an image built using these changes to lkysow/atlantistest. You'll need to run with --data-dir=/home/atlantis.


ENV ATLANTIS_HOME_DIR=/home/atlantis
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer if we'd replace this variable by ENV HOME as I did in #346. Because when not running as the user atlantis, we still need to find the $HOME for atlantis

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it's a good idea to set the HOME env because that's set by the shell.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think in this case it is a good idea because atlantis expects it to be in /home/atlantis but when running with a random user this variable would not be set and thus failing when trying to create in / (the default when there is no $HOME set)

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't expect to be in /home/atlantis though. The code just defaults to using ~/.atlantis for the data directory.

If you're running as a user that doesn't have write access to your home directory then you need to specify a different directory to store the data in.

Copy link
Contributor

Choose a reason for hiding this comment

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

However the only two possible scenarios are either we run with the atlantis user which has the HOME=/home/atlantis or a random uid which won't have any home by default. Thus setting HOME=/home/atlantis ensures our user will always use this as its home which is a good guarantee for us as we have changed the permission on /home/atlantis to work with any random uid (setting the group permissions correctly on it)

# If we're running as root and we're trying to execute atlantis then we use
# gosu to step down from root and run as the atlantis user.
# In OpenShift, containers are run as a random users so we don't need to use gosu.
if [[ $(id -u) == 0 ]] && [[ "$1" = 'atlantis' ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

I also needed to add a user in /etc/passwd as you can see in #346. Otherwise terraform was not able to clone a module by git.

Copy link
Member Author

Choose a reason for hiding this comment

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

oh interesting. okay I've added that as well.

@jocelynthode
Copy link
Contributor

OpenShift users will have to run with --data-dir /home/atlantis since their random user won't have a home directory

If we specify the $HOME in the base image this won't be a problem. Furthermore I would like not to be forced to mount a data dir on the home directory

@lkysow
Copy link
Member Author

lkysow commented Nov 22, 2018

@jocelynthode why do you need to mount a data dir?

In OpenShift containers are run as a random user id. In this case, we
don't need to use gosu.

Fixes #345
@lkysow lkysow force-pushed the openshift-dockeruser branch from f07cd7e to 9532e93 Compare November 22, 2018 19:06
@jocelynthode
Copy link
Contributor

@lkysow Sorry I misunderstood and thought you were talking about a Persistent Volume

@jocelynthode
Copy link
Contributor

@lkysow As an aside I want it a bit disheartening that we're discontinuing my PR and working on a new one that basically implements what mine tried to achieve :(

@lkysow
Copy link
Member Author

lkysow commented Nov 23, 2018

Fair enough, I thought the changes were big enough to warrant a new PR but now the two are converging again. If you'd like to make the same changes in yours then I'm happy to close this one.

@lkysow
Copy link
Member Author

lkysow commented Nov 26, 2018

Closing in favour of #346

@lkysow lkysow closed this Nov 26, 2018
@lkysow lkysow deleted the openshift-dockeruser branch November 26, 2018 15:25
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.

2 participants