Adds support for Docker Desktop and rootless Docker #1331

rmartin16 · 2023-06-23T20:34:35Z

Changes

Fixes Add Support for Docker Desktop and Rootless Docker on Linux #1083
Determines how Docker is running and passes a flag to Dockerfile creation to use a root or a dedicated user

Dependencies

Add support to conditionally step down from root briefcase-linux-system-template#6
Add support to conditionally step down from root briefcase-linux-appimage-template#33

PR Checklist:

All new features have been tested
All new features have been documented
I have read the CONTRIBUTING.md file
I will abide by the code of conduct

rmartin16 · 2023-06-26T21:43:23Z

After thinking about this for a bit, I've moved some things around. Would appreciate any feedback, @freakboy3742, before I finish tests, etc.

tldr

Combined Docker().prepare() into Docker().check_output()
Docker's operating mode is determined at Docker verification

Removed `Docker().prepare()`

The previous implementation of prepare() was basically a preemption of the docker run command to need to download an image to create the container....and that was only necessary to prevent the downloading of the image from polluting the command output from docker run.

So, I implemented a (relatively) fast verification in check_output() to confirm the image is available before trying to spin up a container with the image.

This makes more sense to me because check_output() takes an arbitrary image:tag. So, it isn't especially safe to assume that prepare() has been called for any image.....and therefore, the safest practice would be calling prepare() before any use of check_output().....but at that point, why aren't you just doing it as part of check_output(). However, if a Docker instance was bound to a specific image:tag, then it would probably be a lot safer to assume any "prepare" steps had already been done.

Assessing Docker's operating mode

Without considering implementation details, if you asked me where an assessment of Docker's operating mode should be, I think the most logical place is where Docker is instantiated; that is, when Docker is verified. So, that's where I've put it...for right now, anyway.

To still allow using an image:tag relevant to the Briefcase command, one can be passed to Docker verification. However, this means console output for downloading a Docker image may be the first thing a user sees. This is less than ideal....but not the end of the world. For most users, they will only see this download happen once.

As an aside....I've been wanting to add a [briefcase] Starting up... output section when Briefcase starts. This would encapsulate Wait Bars for the tool verification and other tasks that take place before the Command actually starts doing its thing. For instance, on my macOS VM, it takes a good second or two (or three) to get any output; this happens on slower machines in general.

Additionally, assessing Docker here means Linux AppImage Commands cannot use whatever base image is described in the Dockerfile (or derived from pyproject.toml).....especially since the most general case would require parsing such image from the FROM command in the Dockerfile. Therefore, this assessment falls back to using alpine in this situation. This is a very widely used image, is <10MB, and supports all the relevant architectures.

rmartin16 · 2023-06-26T21:54:38Z

Also...a failure mode I was thinking about...but probably not too important...

Since it is only reasonable for Briefcase to assess Docker's operating mode when it's going to be using Docker, it would still be possible to roll out a template with an incompatible Dockerfile.

This would happen in three situations I've considered:

User runs briefcase create linux appimage --no-docker with Docker Desktop
- If the user doesn't pass --no-docker in subsequent runs, those will fail
User creates Linux System package using Docker to target their host platform with Docker Desktop
- If a user already created a System package natively and then tries in Docker using an image that matches their host platform, it will fail
User switches their Docker operating mode
- This is a bit unavoidable; user should roll out the template again

#472 might be able to help with these but I'm planning to leave them as "known issues" in the implementation....although, maybe something should be done to detect this and alert the user.....

freakboy3742

This looks like it's on the right track to me - a couple of minor notes inline, but the general approach to the refactor makes sense.

freakboy3742 · 2023-06-27T01:44:36Z

src/briefcase/integrations/docker.py

+        containers. If the owning user is the host user, root should be used.
+        """
+        write_test_filename = "container_write_test"
+        host_write_test_dir_path = Path.cwd()


Using temp filename in a temp location would be preferable here - or, at the very least, using the build directory so there's no chance of dirtying the project root.

Using the build directory probably won't be possible here since this can run before it exists.

I considered using a temporary file....but it presents a few of its own questions I think.

For starters, we can't use Python's tempfile to create the file....because we need the file to be created inside the container.

However, we can use tempfile.gettempdir() as the host directory to mount in to the container perform the write test. First, this doesn't confirm a Docker container will have write permissions inside the project....but that'll be problematic no matter what. Second, this still leaves the possibility that we leave a root-owned file in the user's tmp directory....and to avoid issues with the file persisting from a previous Briefcase run, we'll need to effectively recreate a method to manufacture a random and unique filename.

These aren't insurmountable....but is why I went with this obvious approach. I can consider the temporary directory more.

FWIW, I think creating the build directory if it doesn't exist would be reasonable. I can't think of any edge case where we'd end up with a build directory in a weird location - worst case is we get a build directory that is empty because it's only used to create a temporary permission check file. All the other locations that use the build directory are set up as "create if doesn't exist"; this would just be one more usage of that pattern.

freakboy3742 · 2023-06-27T01:59:02Z

src/briefcase/integrations/docker.py

+            ) from e
+
+        # if the file is not owned by `root`, then Docker is mapping usernames
+        self.is_userns_remap = 0 != self.tools.os.stat(host_write_test_file_path).st_uid


Two notes here:

the naming here is unclear to me - userns is... user namespace?

Where we're setting a stateful variable on the Docker instance, but there's no fallback - we're just assuming that _determine_docker_mode() has been invoked. Which it will be, because it's invoked right after the Docker() constructor... but something doesn't quite sit right about this.

I get the reason why this happens - the base Docker instance isn't bound to a particular image, but we need an image to evaluate whether Docker is running in rootless mode. I wonder if perhaps the Docker constructor should take a the image as a "hint", which would mean that we can guarantee that every Docker instance will have is_userns_remap set.

Regardless - the docs for verify_install should probably also highlight that the image is a hint, not anything that will result in the base Docker tool being bound.

the naming here is unclear to me - userns is... user namespace?

Yeah, sorry; I've renamed this variable 10 times. userns_remap is the name of the Docker daemon setting for this. I may yet consider a different name.

Where we're setting a stateful variable on the Docker instance, but there's no fallback - we're just assuming that _determine_docker_mode() has been invoked.

I waffled on where to call _determine_docker_mode(). Putting it in Docker's constructor works as well for me. I'll clarify the docs as well.

freakboy3742 · 2023-06-27T02:11:50Z

As an aside....I've been wanting to add a [briefcase] Starting up... output section when Briefcase starts. This would encapsulate Wait Bars for the tool verification and other tasks that take place before the Command actually starts doing its thing. For instance, on my macOS VM, it takes a good second or two (or three) to get any output; this happens on slower machines in general.

Agreed that radio silence during startup isn't great, especially when startup takes time. I had mentally filed this lag as "CPython interpreter sometimes takes time to start up, especially when there's new byte code to compile", but if you've got evidence that there's a significant lag caused by the tool checks themselves, then I'm not opposed to addressing this.

My preferred solution would be "make it faster" :-) - but given that some of the required checks Just Take Time, I think a tasteful "Checking tools..." waitbar might be called for.

Therefore, this assessment falls back to using alpine in this situation. This is a very widely used image, is <10MB, and supports all the relevant architectures.

I agree that using Alpine as a fallback is a reasonable approach. However, as I flagged in my review, I think we might be able to address this by treating the image as a "hint" - explicitly flagging it as something that isn't required, and falls back to Alpine, but if provided, any checks will use that image, as an optimization avoiding unnecessary images.

AFAICT, In practice, all uses of Docker will know the image they're going to use at the point they're instantiating Docker - an AppImage will be using the configured manylinux (or ubuntu 18.04 fallback); System packages will be using an explicitly provide image. I can't think of a situation where we need to configure Docker for use, but we don't have an in-content image that will be used as soon as Docker has been configured.

Also...a failure mode I was thinking about...but probably not too important...

Since it is only reasonable for Briefcase to assess Docker's operating mode when it's going to be using Docker, it would still be possible to roll out a template with an incompatible Dockerfile.

This would happen in three situations I've considered:

Agreed that these edge cases all exist; however, as you say, short of addressing #472, I'm not sure there's much we can do about them. I'm comfortable putting these into "future work/#472" territory.

rmartin16 · 2023-06-27T02:46:11Z

AFAICT, In practice, all uses of Docker will know the image they're going to use at the point they're instantiating Docker

Unfortunately, this is only true for runs for Linux System since it determines the image:tag in parse_options()....which will be before verify_tools().

For Linux AppImage, it only ever actually derives the image:tag for the Dockerfile during briefcase create linux appimage command....and this is after Docker is instantiated. This could be mitigated, though, by refactoring that derivation to happen in time for verify_tools().

Furthermore, though, we can't completely assume that ubuntu:18.04 is the base image if manylinux doesn't exist in pyproject.toml.....because arbitrary user templates (🙃) could be using any ol' image. So, defaulting to ubuntu:18.04 would likely use the actual base image in the Dockerfile....but no guarantees. This also interlocks the defaults in the template to what's in Briefcase....but I don't think that's a huge deal.

This is why I was defaulting to alpine....although, this refactoring and acceptance of compromises would avoid that.

freakboy3742 · 2023-06-29T02:02:37Z

AFAICT, In practice, all uses of Docker will know the image they're going to use at the point they're instantiating Docker

Unfortunately, this is only true for runs for Linux System ...
... we can't completely assume that ubuntu:18.04 is the base image if manylinux doesn't exist in pyproject.toml.....because arbitrary user templates (🙃)

Darn users... always messing up perfectly good plans... 😝

This is why I was defaulting to alpine....although, this refactoring and acceptance of compromises would avoid that.

Given the alpine image is 10MB, I don't think this is worth extraordinary gymnastics. Unless using the "right" image falls out reliably and easily, I'm completely OK with an alpine fallback. Falling back to ubuntu:18.04 when the user actually has a custom base image would be much worse.

rmartin16 · 2023-07-01T22:31:14Z

a fun twist....arch's makepkg is not permitted to be run as root...even inside a container...

http://allanmcrae.com/2015/01/replacing-makepkg-asroot/

- Perform check in constructor so the setting is always available - Perform write test in `build` directory to help prevent project pollution

rmartin16 · 2023-07-07T18:45:09Z

This is in its final form. However, I still want to do testing on the different platforms to make sure I didn't miss anything.

still not sure what to do about the inability to run makepkg as root...

rmartin16 · 2023-07-09T19:02:08Z

src/briefcase/integrations/docker.py

+            ) from e
+
+        try:
+            self.tools.subprocess.run(


An optimization that I've considered by didn't implement is leveraging docker exec.

Here, as in other places, we're spinning up an entire container for one quick command...just to immediately destroy the entire thing. Alternatively, we could start the container at the beginning and just call docker exec to run commands ad hoc and then destroy the container when the command is finishing.

It may even be possible to allow multiple commands to use the same container.

I haven't done much assessment on what this would take or consequences from reusing the same container....but in my virtualized environments and my Pis, all this docker setup and teardown definitely slows things down.

Agreed this might be an optimisation worth exploring; but it seems like it's opening up a whole "Docker session" worldview that we'd want to get right. docker run isn't prohibitively slow, I'm happy to live with it for now at least.

- Arch's `makepkg` cannot be run as root

rmartin16 · 2023-07-10T00:39:16Z

src/briefcase/platforms/linux/system.py

+            if app.target_vendor_base == ARCH and self.tools.docker.is_users_mapped:
+                raise BriefcaseCommandError(


This'll provide feedback when Arch package builds aren't feasible....but it also better illuminates that macOS can no longer build Arch packages...

Well that's annoying... if I'm reading this right, this branch will also catch Linux users whose Docker config doesn't require user mapping.

Would a better approach be to always turn on the step-down user for arch builds, and only raise an error if the local configuration will break with a step down user?

(Mostly asking for self-serving purposes - I can't easily build Arch packages without this capability)

(chatted a lot off line but answering here for posterity)

Well that's annoying... if I'm reading this right, this branch will also catch Linux users whose Docker config doesn't require user mapping.

This will catch Linux users using Docker Desktop or rootless Docker since they can't use the step-down user and would be running as root in the container now. Existing users with Docker Engine wouldn't be affected since user mapping isn't (normally) enabled.

Would a better approach be to always turn on the step-down user for arch builds, and only raise an error if the local configuration will break with a step down user?

(Mostly asking for self-serving purposes - I can't easily build Arch packages without this capability)

We can only use the step-down user in containers when Docker is not performing user mapping. This is because of how user mapping works. If Docker is performing user mapping and we use a user in the container with ID 1000, then the owner of the files in the host file system end up with an owner ID that doesn't even exist; however, this user ID that shows up is tied to how Linux user namespaces work.

Nonetheless, this also reveals another strategy that could allow us to accommodate this. We could set the GID of all the files to a GID the host user is also a part of. In that way, while the files are owned by a "non-existent" user, the host user would still have permission to interact with them. I ultimately didn't pursue this, though, because it presents a lot of opportunity for issues one way or another. In general, a lot of the strategies to mitigate these file permissions problems required strong cooperation between the host environment and containers that I thought would be onerous for users.

freakboy3742

This all looks good; I've tweaked the pluralisation of is_user_mapped, and snuck in a cookiecutter version pin bump.

The only real issue I can see is the problem with macOS Arch builds.

freakboy3742 · 2023-07-10T02:37:27Z

src/briefcase/integrations/docker.py

+            ) from e
+
+        try:
+            self.tools.subprocess.run(


Agreed this might be an optimisation worth exploring; but it seems like it's opening up a whole "Docker session" worldview that we'd want to get right. docker run isn't prohibitively slow, I'm happy to live with it for now at least.

freakboy3742 · 2023-07-10T02:38:10Z

src/briefcase/integrations/docker.py

+            at all bound to the instance.
+        """
+        super().__init__(tools=tools)
+        self.is_users_mapped = self._is_user_mapping_enabled(image_tag)


The pluralisation on this grates me the wrong way.... I'll likely tweak this before landing.

freakboy3742 · 2023-07-10T03:16:51Z

src/briefcase/platforms/linux/system.py

+            if app.target_vendor_base == ARCH and self.tools.docker.is_users_mapped:
+                raise BriefcaseCommandError(


Well that's annoying... if I'm reading this right, this branch will also catch Linux users whose Docker config doesn't require user mapping.

Would a better approach be to always turn on the step-down user for arch builds, and only raise an error if the local configuration will break with a step down user?

(Mostly asking for self-serving purposes - I can't easily build Arch packages without this capability)

freakboy3742

I've just pushed an update to enable the step-down user on macOS; with that, I'm happy with this PR.

rmartin16 · 2023-07-10T04:47:33Z

src/briefcase/platforms/linux/system.py

        try:
-            context["use_non_root_user"] = not self.tools.docker.is_user_mapped
+            context["use_non_root_user"] = self.use_docker and (
+                self.tools.host_os == "Darwin" or not self.tools.docker.is_user_mapped
+            )
        except AttributeError:
            pass  # ignore if not using Docker


If we check self.use_docker here, then use_non_root_user will be False if rolling out a template without using Docker. While I suppose this setting is arbitrary in this case, this will reverse the current default of True from the templates.

It also effectively mitigates the need for the try/except AttributeError since self.tools.docker will exist if Docker is being used.

I'm not sure this setting is especially important....but throwing this out there.

That's a good point - the template default is "use root user", so it's safer to fall back to that than set it to false because there's no Docker. I'll tweak the logic.

(Of course the real issue here is that briefcase create is sensitive to whether you used Docker, but that's a much bigger can of worms)

rmartin16 · 2023-07-10T05:05:40Z

GitHub won't let me approve my own PR...but I'm on board with your changes 👍🏼

rmartin16 force-pushed the docker-desktop branch 3 times, most recently from 2973ffa to f06f299 Compare June 26, 2023 20:54

Add support for Docker Desktop and rootless Docker

215068b

rmartin16 force-pushed the docker-desktop branch from f06f299 to 215068b Compare June 26, 2023 21:07

freakboy3742 requested changes Jun 27, 2023

View reviewed changes

rmartin16 mentioned this pull request Jul 1, 2023

Ensure repo checkout is unshallow in RTD builds for SCM versioning #1340

Merged

4 tasks

Revise user mapping inspection implementation

d219035

- Perform check in constructor so the setting is always available - Perform write test in `build` directory to help prevent project pollution

rmartin16 force-pushed the docker-desktop branch 2 times, most recently from 6ee5205 to 5ed3010 Compare July 5, 2023 17:36

rmartin16 mentioned this pull request Jul 7, 2023

Rename template info key names in cookiecutter context beeware/briefcase-template#72

Merged

4 tasks

Add tests for Docker user mapping inspection

24cdf4c

rmartin16 force-pushed the docker-desktop branch from 5ed3010 to 24cdf4c Compare July 7, 2023 17:41

rmartin16 commented Jul 9, 2023

View reviewed changes

Disable building Arch packages when Docker is mapping users

b31e51f

- Arch's `makepkg` cannot be run as root

rmartin16 force-pushed the docker-desktop branch from 3a331b2 to b31e51f Compare July 10, 2023 00:00

rmartin16 commented Jul 10, 2023

View reviewed changes

rmartin16 marked this pull request as ready for review July 10, 2023 02:44

freakboy3742 added 2 commits July 10, 2023 11:25

Tweak the pluralization of a variable.

6ab39d1

Bump the required cookiecutter version to v2.2; Refs beeware#1347.

76ceb1b

freakboy3742 requested changes Jul 10, 2023

View reviewed changes

Allow the step-down user on macOS.

3677230

freakboy3742 approved these changes Jul 10, 2023

View reviewed changes

rmartin16 commented Jul 10, 2023

View reviewed changes

Fall back to template default if we don't know anything about Docker

38ab799

freakboy3742 merged commit 878af41 into beeware:main Jul 10, 2023

rmartin16 deleted the docker-desktop branch July 10, 2023 05:35

rmartin16 mentioned this pull request Jul 22, 2023

Briefcase data directory is still being mounted in to containers for brutus user when root is used #1374

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds support for Docker Desktop and rootless Docker #1331

Adds support for Docker Desktop and rootless Docker #1331

rmartin16 commented Jun 23, 2023 •

edited

Loading

rmartin16 commented Jun 26, 2023

rmartin16 commented Jun 26, 2023 •

edited

Loading

freakboy3742 left a comment

freakboy3742 Jun 27, 2023

rmartin16 Jun 27, 2023

freakboy3742 Jun 29, 2023

freakboy3742 Jun 27, 2023

rmartin16 Jun 27, 2023

freakboy3742 commented Jun 27, 2023

rmartin16 commented Jun 27, 2023

freakboy3742 commented Jun 29, 2023

rmartin16 commented Jul 1, 2023

rmartin16 commented Jul 7, 2023

rmartin16 Jul 9, 2023

freakboy3742 Jul 10, 2023

rmartin16 Jul 10, 2023

freakboy3742 Jul 10, 2023

rmartin16 Jul 10, 2023

freakboy3742 left a comment

freakboy3742 Jul 10, 2023

freakboy3742 Jul 10, 2023

freakboy3742 Jul 10, 2023

freakboy3742 left a comment

rmartin16 Jul 10, 2023

freakboy3742 Jul 10, 2023

rmartin16 commented Jul 10, 2023

		if app.target_vendor_base == ARCH and self.tools.docker.is_users_mapped:
		raise BriefcaseCommandError(

Adds support for Docker Desktop and rootless Docker #1331

Adds support for Docker Desktop and rootless Docker #1331

Conversation

rmartin16 commented Jun 23, 2023 • edited Loading

Changes

Dependencies

PR Checklist:

rmartin16 commented Jun 26, 2023

tldr

Removed Docker().prepare()

Assessing Docker's operating mode

rmartin16 commented Jun 26, 2023 • edited Loading

freakboy3742 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freakboy3742 commented Jun 27, 2023

rmartin16 commented Jun 27, 2023

freakboy3742 commented Jun 29, 2023

rmartin16 commented Jul 1, 2023

rmartin16 commented Jul 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freakboy3742 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freakboy3742 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmartin16 commented Jul 10, 2023

rmartin16 commented Jun 23, 2023 •

edited

Loading

Removed `Docker().prepare()`

rmartin16 commented Jun 26, 2023 •

edited

Loading