Skip to content
This repository was archived by the owner on Jan 19, 2018. It is now read-only.

Conversation

@cdrage
Copy link
Member

@cdrage cdrage commented Mar 31, 2016

This call finds out what user is running the Atomic App.

Whether they are:
sudo
root
user

If it is running under sudo, this util will find the user running the
sudo command.

Once the user is found, os.path.expanduser(user) is utilized to find the
home directory. If a home directory is not found atomic app will error
out.

Fixes #656

This util will be used to implement default config locations for OpenShift and k8s providers.

@cdrage
Copy link
Member Author

cdrage commented Mar 31, 2016

@dustymabe @kadel :)

@dustymabe
Copy link
Contributor

@cdrage - this doesn't work when running inside a container. I modified it to print out the results of a call to this function and here is what you see when running in a container:

[vagrant@f23 wordpress-centos7-atomicapp]$ sudo docker run -it --rm  --privileged -v $(pwd):/atomicapp -v /run:/run -v /:/host --net=host --name atomicapp projectatomic/atomicapp:local run ./      
[INFO] - main.py - Action/Mode Selected is: run
XXX /root

here is what you get when not running inside a container:

[vagrant@f23 wordpress-centos7-atomicapp]$ sudo PYTHONPATH=/sharedfolder/atomicapp /usr/bin/python -B /sharedfolder/atomicapp/atomicapp/cli/main.py run ./                                           
[INFO] - main.py - Action/Mode Selected is: run
XXX /home/vagrant

@cdrage
Copy link
Member Author

cdrage commented Apr 1, 2016

@dustymabe
One more thing to add then! Now it's determining whether we are in a container or not.

@cdrage
Copy link
Member Author

cdrage commented Apr 1, 2016

http://stackoverflow.com/questions/20010199/determining-if-a-process-runs-inside-lxc-docker seems like the best way, i'll modify this a bit.

@dustymabe
So what should it output when it's in a container? How do you think we should be able to detect the host dir of the user?

@cdrage
Copy link
Member Author

cdrage commented Apr 4, 2016

@dustymabe

Okay, so essentially it's two things that we push for AtomicApp. Running it via Atomic CLI or AtomicApp CLI.

Thus the only change we need to add is -e SUDO and -e SUDO_USER to the RUN label within our Dockerfile.

P.S. Although I'll still need to implement whether or not we are in a container or not in order to get the the config from inside the /host dir instead of / dir inside the container.

@cdrage cdrage force-pushed the add-finding-root-user branch from 6b8b64f to 27aadba Compare April 4, 2016 16:51
@cdrage
Copy link
Member Author

cdrage commented Apr 4, 2016

@dustymabe

Updated this in order to detect if we are running within a container. Thus when we use atomic CLI we can still detect what user is running it as from a usage perspective the only two options we give is to run via atomicapp or atomic.

Tested this with atomic CLI. By building this container (tagging it as atomicapp) then building the helloapache example with FROM atomicapp and running it via the Atomic CLI with the Utils.getHomeDir() somewhere in the code in order to see the output. Everything works correctly, woo!

Remember: We'll only use this function when need be, not putting this randomly in the base.py code. Thus when we run Atomic App as a container within OpenShift (that doesn't need to find the configdata) it won't break anything.

This function is intended to:

  • Fix the permission issues when using sudo fetch, run, etc. so it doesn't extract as a root user
  • Be able to find configdata by default for k8s (when the api version is implemented) and openshift / marathon providers.

# This ONLY checks to see if we are running in a Docker container
# Atomic App does not support any other container platform (yet).
# Within a Docker container, the .dockerenv and .dockerinit files ALWAYS exist.
@staticmethod
Copy link
Contributor

Choose a reason for hiding this comment

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

what about the inContainer() function that we have in this same file?

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 yeah, I see that now! Adding .dockerenv and .docketinit to it would be viable too.

@cdrage cdrage force-pushed the add-finding-root-user branch from 27aadba to b2e198d Compare April 4, 2016 19:22
@dustymabe
Copy link
Contributor

Also it would be cool to have the part that figures out what "user" we are as part of a separate function that is called from getUserHome(). I can see that being used in other places.

@cdrage cdrage force-pushed the add-finding-root-user branch from b2e198d to 339a1bc Compare April 4, 2016 19:24
@cdrage
Copy link
Member Author

cdrage commented Apr 4, 2016

@dustymabe The part of the function that gets sudo + sudo_user env variables?

Otherwise, I've updated with your patch as well as removed the container function I had to inContainer!

# the /host path for finding the user directory. We are unable to use
# os.path.expanduser due to the passed volume
if Utils.inContainer():
logger.debug("Atomic App is running in a container")
Copy link
Contributor

Choose a reason for hiding this comment

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

since inContainer checks this I don't think we need it anymore.

@dustymabe
Copy link
Contributor

@dustymabe The part of the function that gets sudo + sudo_user env variables?

yes

(bool): True == we are in a container
"""
if os.path.isdir(HOST_DIR):
if os.path.isfile('/.dockerenv') and os.path.isfile('/.dockerinit') and os.path.isdir(HOST_DIR):
Copy link
Contributor

Choose a reason for hiding this comment

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

are we sure we want to make this change here? if we do this should probably be in a separate commit

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'll do this a separate commit then )

@dustymabe
Copy link
Contributor

Also we need to update all dockerfiles.. don't want them getting out of sync

@cdrage cdrage force-pushed the add-finding-root-user branch from 339a1bc to d869888 Compare April 4, 2016 20:48
@cdrage
Copy link
Member Author

cdrage commented Apr 4, 2016

@dustymabe
Updated as well as the Dockerfiles. Put the inContainer() edit in a separate commit.

@cdrage
Copy link
Member Author

cdrage commented Apr 5, 2016

#dotests

@dustymabe
Copy link
Contributor

Also it would be cool to have the part that figures out what "user" we are as part of a separate function that is called from getUserHome(). I can see that being used in other places.

So I refactored the code a bit.. there is a new getUserName() function as well as refactored the getUserHome() function to not repeat code. What do you think?

    @staticmethod
    def getUserName():
        """
        Finds the username of the user running the application. Uses the 
        SUDO_USER and USER environment variables. If runnning within a 
        container, SUDO_USER and USER varibles must be passed for proper
        detection.
        Ex. docker run -v /:/host -e SUDO_USER -e USER foobar
        """
        sudo_user = os.environ.get('SUDO_USER')

        if os.getegid() == 0 and sudo_user is None:
            user = 'root'
        elif sudo_user is not None:
            user = sudo_user
        else:
            user = os.environ.get('USER')
        return user

    @staticmethod
    def getUserHome():
        """
        Finds the home directory of the user running the application.
        If runnning within a container, the root dir must be passed as
        a volume.
        Ex. docker run -v /:/host -e SUDO_USER -e USER foobar
        """
        logger.debug("Finding the users home directory")
        user = Utils.getUserName()
        incontainer = Utils.inContainer()

        # Check to see if we are running in a container. If we are we
        # will chroot into the /host path before calling os.path.expanduser
        if incontainer: os.chroot(HOST_DIR)

        # Call os.path.expanduser to determine the user's home dir.
        # See https://docs.python.org/2/library/os.path.html#os.path.expanduser
        # Warn if none is detected, don't error as not having a home
        # dir doesn't mean we fail.
        home = os.path.expanduser("~%s" % user)
        if home == ("~%s" % user):
            logger.error("No home directory exists for user %s" % user)

        # Back out of chroot if necessary
        if incontainer: os.chroot("../..")

        logger.debug("Running as user %s. Using home directory %s for configuration data"
                     % (user, home))
        return home

@cdrage cdrage force-pushed the add-finding-root-user branch from d869888 to 99bc6aa Compare April 7, 2016 13:09
cdrage added 2 commits April 7, 2016 09:10
…should use

This call finds out what user is running the Atomic App.

Whether they are:
  sudo
  root
  user

If it is running under sudo, this util will find the user running the
sudo command.

Once the user is found, os.path.expanduser(user) is utilized to find the
home directory. If a home directory is not found atomic app will error
out.

If Atomic App is running within a Docker container it will utilize the
passed /host dir (from the RUN/STOP label of Atomic CLI) in order to
find the directory of the corresponding user
As we don't support any other container platform (yet), check to see if
this is a Docker container.
@cdrage cdrage force-pushed the add-finding-root-user branch from 99bc6aa to 303a548 Compare April 7, 2016 13:10
@cdrage
Copy link
Member Author

cdrage commented Apr 7, 2016

@dustymabe
WOAH! Thanks 👯 That's wicked.

I've updated the PR with your changes as well as re-based against master.

@dustymabe
Copy link
Contributor

#dotests

@dustymabe
Copy link
Contributor

That darn pesky kubernetes test needs some love :(

cdrage most all looks good to me. as part of this do you mind bringing the Dockerfiles.pkgs/ Dockerfiles up to date. Let's keep the version #, run labels, etc.. synced across all Dockerfiles. Do you agree?

@cdrage
Copy link
Member Author

cdrage commented Apr 7, 2016

@dustymabe

Agreed. Let's bring them back up-to-date and let's get this merged. WOOHOO!

@cdrage
Copy link
Member Author

cdrage commented Apr 7, 2016

@dustymabe
Updated the Dockerfile.pkgs folder to an up-to-date-version.

@dustymabe
Copy link
Contributor

LGTM.

@dustymabe
Copy link
Contributor

#dotests

@cdrage
Copy link
Member Author

cdrage commented Apr 7, 2016

Woo! 🎊

@cdrage cdrage merged commit 48dab7d into projectatomic:master Apr 7, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants