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

Allow injection of arbitrary Dockerfile content. #22

Merged
merged 1 commit into from
Nov 9, 2022

Conversation

freakboy3742
Copy link
Member

@freakboy3742 freakboy3742 commented Nov 5, 2022

The existing Dockerfile allows for very specific customisations, such as installation of additional system packages. However, sometimes that is insufficient.

For example, installing the Python cryptography package requires a Rust compiler, but it is not possible to install rustc
using apt-get; you need to run a rust-up.sh script.

This template update allows for an end-user to inject arbitrary Dockerfile content into their project configuration. See beeware/Python-support-testbed#5 for an example of this in use.

Fixes beeware/briefcase#886.
Refs beeware/Python-support-testbed#5

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

@mhsmith
Copy link
Member

mhsmith commented Nov 7, 2022

I got an email notification of a comment from @rmartin16, which I guess he's deleted now, but I think it's a good question:

Given the extra content will execute after USER brutus, the commands will have limited privileges.

Is this intentional?

Or should we allow users to run commands with root privileges?

I see that the use of this feature in beeware/Python-support-testbed#5 assumes (in the ENV PATH line) that the commands will be run as the brutus user. But isn't the existence of this user an internal implementation detail?

Running the commands as root might make the installation of Rust a bit less convenient, but it makes the overall feature easier to understand. It means one less thing for the user to learn (since anyone who knows about Docker knows it runs things as root by default), and it saves them from having to switch back to root if their commands require it.

@rmartin16
Copy link
Member

(ahhh....I did delete my comment so I could add it as part of a review; however, it seems since my review is "Pending", the comment apparently isn't viewable by others. TIL :)

@freakboy3742
Copy link
Member Author

@freakboy3742 freakboy3742 reopened this Nov 7, 2022
@freakboy3742
Copy link
Member Author

Dammit... wrong button :-)

Ok, so... there's 2 different pieces here:

  1. Making a modification that can support Rust (which is the immediate use case)
  2. Making a modification that is useful in the general case.

My original attempt at fixing (1) did put the injected block just before the USER brutus directive. However, that caused problems: Rust can't be installed as root (or at least, it can't be done reliably, so it's undocumented as an installation approach). The working assumption is that every developer will install and maintain their own Rust toolchain. So, from the prospective of the immediate problem of installing rustc, "user space commands" are actually what we need.

However, in the general case of (2), that won't always be the case; I'm sure there will always be examples where we need commands run as root, and examples where we need commands run as brutus.

I can see four possible solutions (five if you count "don't allow user customization at all").

  1. Use the patch as defined here, but lean on the fact that you can make as many calls to USER as you need. So, if you needed to install something after installing rust for some reason, the following will install rust as brutus, then switch back to the root user for a custom apt-get install.
dockerfile_extra_content = """
RUN curl ...
ENV PATH=...
USER root
RUN apt-get install -y foobar
USER brutus
"""

This is very much "messy, but it works", and only requires a single setting.

  1. Have 2 template inclusions: dockerfile_extra_root_content and dockerfile_extra_user_content - the former injected just before the final USER call, and the latter after it. This is explicit; however, it does open the door to having injection points at any other arbitrary point in the docker file - what if I need some content executed before the Python interpreter is installed? Where do we draw the line between arbitrary template inclusions and "fork your own template"?

  2. Replace dockerfile_extra_content with dockerfile_suffix; give the setting a default value that sets the USER; and if the end-user sets dockerfile_suffix, then they're responsible for ensuring that they include the directive to set the user. So, our Rust example would become something like:

dockerfile_suffix = """
USER brutus
RUN curl https://sh.rustup.rs | sh -s -- -y
ENV PATH=...
"""

This makes dockerfile_suffix a setting that is prone to the easy error of forgetting the USER directive; but the set of examples that will need this setting are fairly limited (e.g., installing rust), so the exposure to this bug isn't that bad.

  1. Require that the user provide a diff, rather than a literal template inclusion. This would be the most flexible option, allowing injection anywhere; but also the most fragile (and very difficult to write by hand).

My YAGNI instinct leans somewhere between 1 and 3. I like the simplicity of a single setting, inserting content at the end of the file. However, I'd be interested in any other opinions.

@rmartin16
Copy link
Member

I like option 2; I also think that's a fine line for saying "further modification requires a fork". However, maybe better is to just provide an option to use an arbitrary Dockerfile specified in pyproject.toml....then users wouldn't have to maintain an entire separate fork at least.

Regardless, given the apparent nature of rust installs and the absence of a need to do arbitrary things as root, I'm good with option 1 too.

@mhsmith
Copy link
Member

mhsmith commented Nov 8, 2022

I think YAGNI suggests we should do option 1, i.e. keep the current behavior, but document the USER issues in more detail.

@freakboy3742
Copy link
Member Author

I've added some clarification to the Briefcase docs in beeware/briefcase#963.

@mhsmith mhsmith merged commit 4c6316b into main Nov 9, 2022
@freakboy3742 freakboy3742 deleted the dockerfile-extra-content branch November 10, 2022 23:22
@mhsmith mhsmith mentioned this pull request Nov 21, 2022
4 tasks
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.

Provide a way to inject environment variables and/or scripts into the Docker container used for Linux builds
3 participants