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

Best practice proposal: do not recommend --system for a Docker context #2762

Merged

Conversation

giorgiosironi
Copy link
Contributor

@giorgiosironi giorgiosironi commented Aug 20, 2018

The issue

Despite an application's Docker container not usually running other Python processes than the application itself, it still has a system Python whose packages should not necessarily be overwritten or upgraded by an application's choices.

For example, python3-software-properties in Ubuntu contains utilities
written in Python like add-apt-repository whose use is widespread in
Dockerfiles
.

By inadvertently applying an application's dependency onto the the container's Python, it is possible to:

  • subtly break system-level software like these that is still present in the container image
  • run into errors where that software is executed while extending the image with another Dockerfile
  • run into errors when docker run|exec ... COMMAND is used to run another process inside the same container for debugging purposes

I realize this is not necessarily a likely use case, but we have seen
enough projects/tools vendoring Requests in fear of a conflict.

The fix

The best practice in my opinion is to favor virtual environments inside a Docker container as the standard use case, as it is for deployment onto a full-fledged OS.

Very open on the wording and whether system Python is the correct term
to designate the global Python for that OS/container image.

The checklist
  • The issue itself is captured in this description, as this was a small change to propose (but complex to describe)
  • A news fragment in the news/ directory to describe this fix with the extension .bugfix, .feature, .behavior, .doc. .vendor. or .trivial (this will appear in the release changelog). Use semantic line breaks and name the file after the issue number or the PR #.

Despite an application's Docker container not usually running other Python processes than the application itself, it still has a system Python whose packages should not necessarily be overwritten or upgraded by an application's choices.

For example, `python3-software-properties` in Ubuntu contains utilities
written in Python like `add-apt-repository` whose [use is widespread in
Dockerfiles](https://github.com/search?l=Dockerfile&q=%22add-apt-repository%22&type=Code).

By inadvertently applying an application's dependency onto the the container's Python, it is possible to:

- subtly break system-level software like these that is still present in the container image
- run into errors where that software is executed while extending the image with another Dockerfile
- run into errors when `docker run|exec ... COMMAND` is used to run another process inside the same container for debugging purposes

I realize this is not necessarily a likely use case, but we have seen
enough projects/tools vendoring `Requests` in fear of a conflict.

Very open on the wording and whether `system Python` is the correct term
to designate the global Python for that OS/container image.
@uranusjr
Copy link
Member

I think this is a reasonable change. Personally I am against using Python without virtual environments, even inside a container. Most people are simply doing it wrong.

@uranusjr uranusjr added Type: Documentation 📖 This issue relates to documentation of pipenv. PR: awaiting-merge The PR related to this issue has been reviewed and is awaiting merge. labels Aug 29, 2018
@uranusjr uranusjr merged commit b5490b4 into pypa:master Aug 29, 2018
@techalchemy
Copy link
Member

I did mean to look at this -- I have no real qualms with it per se but I want to caveat that heroku does this not to manage infrastructure, but they do it on their python buildpack -- most containers are deployed without virtualenvs as I believe you both note, I believe the purpose is to stay slim and reduce attack surface area by installing as little as possible

That said, still no issues with the change, just wanted to caveat a bit

@NdubisiOnuora
Copy link

Any reasons then why you are using "--system" at https://github.com/pypa/pipenv/blob/master/Dockerfile#L28?

@uranusjr
Copy link
Member

uranusjr commented May 8, 2019

Because no one bothered to fix it yet :p Pull requests are welcomed!

ferndot added a commit to flexy/cookiecutter-drf that referenced this pull request May 20, 2019
ferndot added a commit to flexy/cookiecutter-drf that referenced this pull request May 20, 2019
ferndot added a commit to flexy/cookiecutter-drf that referenced this pull request May 20, 2019
ferndot added a commit to flexy/cookiecutter-drf that referenced this pull request May 20, 2019
ferndot added a commit to flexy/cookiecutter-drf that referenced this pull request May 20, 2019
ferndot added a commit to flexy/cookiecutter-drf that referenced this pull request May 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: awaiting-merge The PR related to this issue has been reviewed and is awaiting merge. Type: Documentation 📖 This issue relates to documentation of pipenv.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants