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

Investigate: Reducing reliance on runArgs for common scenarios: mount points, environment variables #1201

Closed
chrmarti opened this issue Aug 19, 2019 · 10 comments
Assignees
Labels
containers Issue in vscode-remote containers plan-item A plan item
Milestone

Comments

@chrmarti
Copy link
Contributor

No description provided.

@chrmarti chrmarti added the plan-item A plan item label Aug 19, 2019
@chrmarti chrmarti added this to the August 2019 milestone Aug 19, 2019
@chrmarti chrmarti self-assigned this Aug 19, 2019
@egamma egamma added the containers Issue in vscode-remote containers label Aug 20, 2019
@chrmarti chrmarti modified the milestones: August 2019, September 2019 Aug 26, 2019
@chrmarti chrmarti modified the milestones: September 2019, October 2019 Oct 2, 2019
@chrmarti chrmarti modified the milestones: October 2019, November 2019 Oct 28, 2019
@Chuxel
Copy link
Member

Chuxel commented Nov 8, 2019

@chrmarti FYI - After looking into the best way to enable HTTPS support for .NET Core applications, I've updated the vscode-remote-try-dotnetcore sample. It is now a pretty textbook example of where env and mounts would be useful properties. The readme explains why.

"runArgs": [
    "-u", "vscode",
    "-e", "ASPNETCORE_Kestrel__Endpoints__Http__Url=http://*:5000",
    "-e", "ASPNETCORE_Kestrel__Endpoints__Https__Url=https://*:5001",
    "-v", "${env:HOME}${env:USERPROFILE}/.aspnet/https:/home/vscode/.aspnet/https",
    "-e", "ASPNETCORE_Kestrel__Certificates__Default__Password=SecurePwdGoesHere",
    "-e", "ASPNETCORE_Kestrel__Certificates__Default__Path=/home/vscode/.aspnet/https/aspnetapp.pfx"
]

The https://github.com/microsoft/vscode-remote-try-dotnetcore README explains what these things do. Theoretically you could bake the env vars into the Docker image, but leaving them in devcontainer.json allows you to enable/disable HTTPS and change port mappings as appropriate.

I'd expect this to be something people will pretty commonly want to do given .NET Core's out of box self-signed HTTPS certs.

@chrmarti
Copy link
Contributor Author

To pick up another discussion: We could have two properties: "env" to set variables on the container and "serverEnv" to set variables on the VS Code Server environment. The latter would update when the server is restarted, the former only when the container is rebuilt.

@Chuxel
Copy link
Member

Chuxel commented Nov 11, 2019

@chrmarti That's an excellent idea. In some ways, I almost wonder if it should be flipped. env is what you mention as serverEnv and then there is a containerEnv for run.

env then could also be applied to the attach scenario which would be fantastic.

@chrmarti
Copy link
Contributor Author

Or devEnv for the server and env for the container. 🤔

Is there a reason to prefer env for the container?

One limitation for the server's environment is that all connected windows will need to reload because we need to restart the agent. (We could look into keeping the old agent and extension hosts around for windows that aren't reloaded yet, but I'm not sure that simply works.)

@Chuxel
Copy link
Member

Chuxel commented Nov 12, 2019

@chrmarti Hmmm. Effectively wouldn't a rebuild would have the same effect, but would be slower?
There's two things driving the thought - one is optimizing the speed that minor changes like this take effect in Remote - Containers. Adding a simple env var ideally wouldn't drive a full rebuild of the entire container. I can see having other changes follow this path over time as an optimization (e.g. adding another extension). The second is I am also thinking a bit about VSO's use of devcontainer.json. In that environment, I'd expect you'd want to do anything you can to prevent having to do a container rebuild, so the restart path would be primary.

@chrmarti
Copy link
Contributor Author

@Chuxel Correct, with 'agent' I mean the VS Code server process, the container can keep running. The limitation might be that if you have multiple windows open on the same container, you need to reload all of them - not just one - for the change to apply.

@Chuxel
Copy link
Member

Chuxel commented Nov 14, 2019

@chrmarti Yep, but effectively a rebuild would get it to kick in as well, which is why it seems like a better default. It would work in both cases rather than just a full rebuild.

On a related note, on the VSO side, I know they're looking at adding a "restart server" option to allow this kind of thing to kick in. Perhaps we should consider something similar for the remote extensions for the reason you mention. SSH has "Kill" which effectively gets you along the same lines, but containers and WSL don't have a similar concept.

@Chuxel
Copy link
Member

Chuxel commented Nov 14, 2019

@chrmarti Okay, talking with the VSO folks, here's a proposal:

  1. env is used on startup
  2. containerEnv is used on container create

env is the one we want most people to use for compatibility with VSO anyway. The other thing I am noticing is we do see isssues with people wanting to modfiy the environment based on another environment variable. This is coming up quite a bit for VSO and the PATH. Since env applies after the container is created, we could also have it support a syntax like the following:

"env": [
    "PATH": "${containerEnv:PATH}:/some/other/location
]

Which would take the container's PATH env var and add something to it. This wouldn't work for "-e", but would in this case.

What do you think? Even if we don't add the second part right away, the VSO folks might to solve some issues.

@chrmarti
Copy link
Contributor Author

Copying @Chuxel 's notes from a wider discussion here:

  • containerEnv - Establishes env vars for the container itself focused particularly on situations where the image being used requires specific variables be set. These cannot be changed without recreating the container. containerEnv can refer to ${localEnv:...} in the Remote - Container case. Long term, this could apply to VSO environments created via VS Code, but I'd assume not initially.

  • remoteEnv - Establishes env vars that are set when the remote host / container is spun up. They're tied to the start of VS Code server. These can refer to ${localEnv:...} like the above and can also refer to containerEnv to do things like update the path. For example "PATH": "${containerEnv:PATH}:/some/other/location/in/container

User then has the same pattern.

  • containerUser is the user that starts the container.
  • remoteUser is the user operates in the remote environment - specifically the one that actually starts VS Code server.

@chrmarti
Copy link
Contributor Author

Note that the resolver API allows the resolver to return environment variables in the ResolverResult. These will apply to the remote extension host even if there is another remote extension host with older environment variables still running on that same container. This is contrary (i.e., better) to what I previously thought where my assumption was that all windows (and thereby remote extension hosts) need to be restarted for a change to "remoteEnv" to apply.

chrmarti added a commit to microsoft/vscode that referenced this issue Nov 20, 2019
chrmarti added a commit to microsoft/vscode that referenced this issue Nov 20, 2019
@vscodebot vscodebot bot locked and limited conversation to collaborators Jan 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
containers Issue in vscode-remote containers plan-item A plan item
Projects
None yet
Development

No branches or pull requests

3 participants