Add non-root dev container user

[hunterjm]

Breaking change

Proposed change

Adds default non-root user to dev container with sudo permissions based on recommendation from Microsoft to fix file permission issue when developing on a linux machine.

Type of change

• Dependency upgrade
• Bugfix (non-breaking change which fixes an issue)
• New integration (thank you!)
• New feature (which adds functionality to an existing integration)
• Breaking change (fix/feature causing existing functionality to break)
• Code quality improvements to existing code or addition of tests

Example entry for configuration.yaml:

# Example configuration.yaml

Additional information

• This PR fixes or closes issue: fixes #
• This PR is related to issue:
• Link to documentation pull request:

Checklist

• The code change is tested and works locally.
• Local tests pass. Your PR cannot be merged unless tests pass
• There is no commented out code in this PR.
• I have followed the development checklist
• The code has been formatted using Black (black --fast homeassistant tests)
• Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

• Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

• The manifest file has all fields filled out correctly.
  Updated and included derived files by running: python3 -m script.hassfest.
• New or updated dependencies have been added to requirements_all.txt.
  Updated by running python3 -m script.gen_requirements_all.
• Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

• No score or internal
• 🥈 Silver
• 🥇 Gold
• 🏆 Platinum

[frenck]

Please note, that this will impact the running Home Assistant inside the container, as it will have fewer permissions as well.

[hunterjm]

Yeah, I've still got some bugs to work out as well regarding the install permissions. I thought everything was working when I opened this PR, but it appears there is still some things a little wonky.

Edit: What are known things that HA needs to run as root in order to be able to do? Previous dev was run in non-root venv anyway. The fact that this dev container uses appPort instead of on the host network breaks more integrations than I would imagine running as a non-root user would.

[hunterjm]

latest commit should fix homeassistant editable install issue.

[hunterjm]

@pvizeli - Would you care to provide any context around closing?

[balloob]

Probably a mistake, as otherwise we would have seen a comment. Re-opening.

[pvizeli]

While any images or Dockerfiles that come from the Remote - Containers extension will include a non-root user with a UID/GID of 1000 (typically either called vscode or node), many base images and Dockerfiles do not. Fortunately, you can update or create a Dockerfile that adds a non-root user into your container.

Repeat the last one:
Fortunately, you can update or create a Dockerfile that adds a non-root user into your container.

Indent, that is a an issue with container general but this is not the fix general, it just fix your use case. You will have this issue every time you use devcontainer in your setup. Let's hope docker fix the issue with a permission manager which can adapt to microsoft devcontainer and hope full also to our other images.

To fix this use case, I would say we remove devcontainer with git ignore and make templates to developer documentation. Everybody can select this template for his use case until the world have a fix for everybody.

[hunterjm]

I agree that it's a general docker issue with mounted volumes on Linux, but it's not an issue with every devcontainer. All out of the box dev containers provided by Microsoft have this addition, including the python dev container. If this Dockerfile were used for anything other than spinning up a VSCode devcontainer, I would agree that this change shoudn't be added. If Home Assistant is going to provide a devcontainer for users, it should match at least the recommendations made by VSCode.

The last sentence in that statement you quoted is in reference to OS containers or Application containers who do not add a non-root user. The context would be different if this were a PR against https://github.com/home-assistant/docker, but it's a PR here on Dockerfile.dev who's only purpose is to spin up a VSCode dev container using a method that has precedence in every Microsoft provided image for this use case.

If you'd like, I can remove the USER declaration so the default is still root but the image then at least provides the ability to run as a non-root user.

[Adminiuga]

I have to agree with Hunterjm here. IMO the sole purpose of adding Dockerfile.dev was to make setting up a dev environment for newcomers easier. Adding a non-root user to container is not such a big stretch.

[hunterjm]

I simplified this PR which achieves the same result when using this devcontainer.json:

{
  "name": "Home Assistant Dev",
  "context": "..",
  "dockerFile": "../Dockerfile.dev",
  "remoteUser": "vscode",
  "postCreateCommand": "mkdir -p config && pip3 install --no-use-pep517 --user -e .",
  "runArgs": ["-e", "GIT_EDITOR=code --wait", "--net=host"],
  "extensions": [
    "ms-python.python",
    "visualstudioexptteam.vscodeintellicode",
    "ms-azure-devops.azure-pipelines",
    "redhat.vscode-yaml",
    "esbenp.prettier-vscode"
  ],
  "settings": {
    "python.pythonPath": "/usr/local/bin/python",
    "python.linting.pylintEnabled": true,
    "python.linting.enabled": true,
    "python.formatting.provider": "black",
    "editor.formatOnPaste": false,
    "editor.formatOnSave": true,
    "editor.formatOnType": true,
    "files.trimTrailingWhitespace": true,
    "terminal.integrated.shell.linux": "/bin/bash",
    "yaml.customTags": [
      "!secret scalar",
      "!include_dir_named scalar",
      "!include_dir_list scalar",
      "!include_dir_merge_list scalar",
      "!include_dir_merge_named scalar"
    ]
  }
}

Note I also changed to --net=host instead of exposing appPort since on my linux machine that lets me work with discovered devices as well. The problem remains that devcontainer.json is tracked, and you can't ignore files that are already tracked. I can just be careful to not commit it, but what would you recommend as a different option? Should we just not track devcontainer.json and update the developer docs?

[balloob]

If it is recommended in the remote container docs, it sounds reasonable to add it.

@pvizeli what would be against adding this? It fixes Linux development, so that's good. Does it break Windows development?

net=host seems also like a good idea. I just saw this posted 20 min ago in devs_core.

[pvizeli]

In case of that, we can also use the base image from Microsoft:

I don't see a reason to maintain the own if there is a solution they include this part now.

net=host seems also like a good idea.

Please not. It's just a developer env to test the code and not run Home Assistant as @hunterjm described. It affects me not on windows because they run anyway in a virtual Network split from physical, but it breaks the local installation i.e. with zeroconf since we handle that wrong and an instance overwrites the exists one as an example. We should not mix up productive instances and references with developer ones. If someone needs to test something in the real network, he can modify the parameter for a test.

[hunterjm]

I agree with @pvizeli regarding using the host network. That only works on Linux, and would break Windows/Mac containers because port 8123 would no longer be forwarded.

I’m also Ok switching to use the Microsoft image as the base, but think it is helpful to install some of the other apt level dependencies for the project out of the box.

I still don’t know the best answer regarding devcontainer.json it’s helpful to have it tracked for the majority use case, but then when people modify it to meet their needs (ONVIF Discovery is a recent example for me), it shows up as changed and can be accidentally included in a PR if not careful.

Having it untracked would mean there is an extra step that would need to be documented in the setup docs for devs, but we could give more detailed recommendations based on the host OS...

[balloob]

We should have a default one that works out of the box for most people, and can offer a gitignore for a .devcontainer/devcontainer.custom.json which we can document.

I agree that the devcontainer doesn't need to support production and that it's for development, however there is a bunch of development done that also relies on discovery.

[pvizeli]

So we put our devcontainer on top of the vscode provided standard image, so we are sure that we are up to date for vscode solution to make it working on most as possible systems.

Now it's still the question about the host. Maybe we should today not until we fix our mdns mistake and avoid issues which would be comes if we change that. After that we can move to host network.

[balloob]

That sounds good. That way we also have to maintain less 👍

[stale]

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

[hunterjm]

I changed base to the vscode-remote image. We could also make it easier for users if we modify the devcontainer.json to default to a vscode user and include the following in the Dockerfile: https://github.com/microsoft/vscode-remote-try-python/blob/master/.devcontainer/Dockerfile#L18-L26