microsoft / vscode-remote-release

Visual Studio Code Remote Development: Open any folder in WSL, in a Docker container, or on a remote machine using SSH and take advantage of VS Code's full feature set.
https://aka.ms/vscode-remote
Other
3.62k stars 279 forks source link

Support to define additional attributes for ports #4046

Closed egamma closed 3 years ago

egamma commented 3 years ago

There is a desire to associate a port with a name and to define that a range of ports is ignored.

The proposal is to do this by introducing a remote setting:

    "settings": {
        "terminal.integrated.shell.linux": "/bin/bash",
        "remote.ports": [
            {
                "port": "8000",
                "label": "gatsbyserver"
            },
            {
                "port": "30000-50000",
                "ignored": true
            },
            {
                "port": "3000",
                "notification": false
            },
            {
                "port": "3001",
                "openBrowser": true
            },      
               ]
    },

update it should also be possible to supress the Open Browser notification for a port. Similary, it would be useful to auto open a browser when a port is forwarded. I've updated the example above.

//fyi @Chuxel

PavelSosin-320 commented 3 years ago

CNI solves it in a very easy way: it never mixes networks! On every computer and VM where it exists, you can find the "local" bridge network but a user can create additional networks, and Configurations of every network can be stored in a separate file. I suppose that everybody who uses more than one computer or VM wants to configure them separately. Every Docker or Podman command related to networking has the option --network 'Network-name' plus command create network 'Network-name' which adds the corresponding configuration file templated on the base of driver's type. The command rm network removes the defined network. Only the "bridge" is created automatically and can't be removed. The absolutely platform-specific CNI instance running on every node makes all necessary manipulations using the "local" utilities. Users can define that on the local machine SSH port is 22 and in some other place 222, 220, in the range 220-240, in the range 2020-2040 for non-root user etc. On every machine, CNI uses different configuration utilities (iptables, nftables, Powershell, bridgectl ) to configure different infrastructure but the configuration language is common. Everything is pluggable.

PavelSosin-320 commented 3 years ago

I think that today developers can use several machines for different purposes simultaneously and networking parameters are unique for every machine. It would better to separate network-related configuration once and forever and create a small network configuration file for the local machine in the %USERPROFILE%/Appdata/Local/ for the local machine and one small configuration file per Machine/Configuration, Cloud Instance or VM in %USERPROFILE%/Appdata/Roaming/ and let the user play with these files and copy them to every remote machine using scp whenever the new machine is added to development landscape. It is easier manageable than a single one hundred parameter configuration. Small config files can be replicated using copy&paste when a new machine is added. If the port is randomized it can be written into the same file, i.e. persisted.

roblourens commented 3 years ago

Would also be useful if extensions can dynamically declare that a port should not be forwarded so the debug adapter can continue to pick a random free port. Otherwise, we need to hardcode a different port for each launch config, and list all those ports in this setting.

lostintangent commented 3 years ago

Random thought: now that we've introduced a rich experience for auto-discovering ports, is there still significant value in declaring the ports up-front in the devcontainer.json file? I only ask, because in some ways, the metadata described by this issue may actually become the most valuable port-related settings, and I wonder if it would feel more first-class if they were top-level devcontainer.json properties, as opposed to "remote" settings.

Exposing this metadata via remote.* settings seems to optimize for SSH interop (which makes total sense), whereas making them a top-level devcontainer.json property might optimize it a bit for Codespaces? (similar to how we're trying to remove the reliance on the Remote Explorer)

alexr00 commented 3 years ago

I lean towards using settings instead of devcontainer.json because setting are already familiar to many more users and for use with SSH (as you mentioned). Setting are already first-class within VS Code, so we aren't losing anything there. Instead of requiring users to learn another file to configure how ports work within VS Code, they just need to use their familiar settings file. Bonus: we can also make sure that it has nice UI in the Settings UI if we want.

But you're right, I don't see significant value in declaring ports upfront in the devcontainer.json file now. I think we're actually going to switch over our samples to not include this now.

Chuxel commented 3 years ago

@alexr00 @lostintangent It would be great to discuss this once we are all back in the office. Here's the things we need to keep in mind going forward:

  1. There are ports that need to be forwarded that are started up before VS Code is running - anything from the Docker Compose scenario (e.g. a database port) along with anything that happens from an entrypoint - e.g. VNC.

  2. Not everyone will enable automatic port forwarding due to security concerns - We need to be able to turn this off. I know from big enterprise conversations that this concerned people when it was mentioned (particularly low ports like 22)

  3. The current property is extremely discoverable because its in the devcontainer.json schema. The settings property covers the entirety of VS Code, so this is a bit of a needle in a haystack.

Ergo, it is still an important setting - particularly as we add in the ability to specify an alternate host in addition to a label. This will better enable things like database tools to connect to things spun up with the containers themselves.

I am personally concerned if port forwarding only ends up in settings. The settings property in devcontainer.json is VS Code only and is intentionally "schemaless" since its designed for use with any extensions. We've discussed wanting to enable connecting to a dev container via the command line (e.g. via SSH) and have discussed opening up the devcontainer.json spec in the future to try to get broader appeal for the concept amongst the developer community as a whole outside of our own specific ecosystem - much like LSP. We're not ready for that yet, but we should factor it in. That means that any core properties should not be in properties that we believe are VS Code specific. Right now there's two: settings and extensions. For Codespaces, I guarantee it will need to support some level of non VS Code tools over time and it uses this same file. So, if we do put this in settings, we should also support it separate from settings as well.

If the implementation ends up that the property is just put into settings, ok, but I don't want to make a call on the property location for a customer facing setting purely based on implementation sharing. We could essentially support the same structure either in settings or the forwardPorts property with forwardPorts taking precedence.

Assuming we decide to make devcontainer.json an open spec ala LSP in the long run, the alternative is to be open to the idea the settings property can be used in non-VS Code editor scenarios. That I think is pretty risky to do given its nature.

I think we're actually going to switch over our samples to not include this now.

This is actually to raise visibility to the dynamic support, not to deprecate the property. We still have it mentioned as something people can go and add as a next step. (See step number four here.)

PavelSosin-320 commented 3 years ago

I'm wondering how Project-scope port forwarding, DNS configuration can help developers to create modern cloud-ready applications where the network is governed by the broadband home routers and Cloud infrastructure. They are IT configurations. For example, the correct Docker documentation says cleary: "By default, a container inherits the DNS settings of the host, as defined in the /etc/resolv.conf" In the home networks connected via broadband routers, the DNS list and DNS forwarding are configured in the routers based on nslookup performance measurement. It is not a single record but an array that can be optimized to achieve the best performance of certain services like DockerHub. If somebody uses a local Docker registry, git service it is better to configure it in /etc/hosts. Ports can be forwarded by the locally installed proxies from very basic Nodejs packages to Nginx or Traefic egresses with convenient WebUI configuration. Port forwarding can be blocked by the installed firewall if it is not configured properly. The application must run in any network to be portable. I think that all things related to networking should be configured separately, in a separate place, maybe done by IT instead of a developer, and deployed as a separate file that is not a part of the project. At least, separate section of dev-container.json file that can have multiple "profiles".

chrmarti commented 3 years ago

@Chuxel

  1. There are ports that need to be forwarded that are started up before VS Code is running - anything from the Docker Compose scenario (e.g. a database port) along with anything that happens from an entrypoint - e.g. VNC.

The implementation is currently such that both the VS Code Server and VS Code need to be running for port forwarding to be available.

  1. Not everyone will enable automatic port forwarding due to security concerns - We need to be able to turn this off. I know from big enterprise conversations that this concerned people when it was mentioned (particularly low ports like 22)

I expect the automatic forwarding can be turned off while explicitly configured forwardings in the settings will still be set up.

  1. The current property is extremely discoverable because its in the devcontainer.json schema. The settings property covers the entirety of VS Code, so this is a bit of a needle in a haystack.

Having the options in a single place (and share it with the other Remote extensions) also has some advantages for discoverability.

I agree with the rest, but would defer it to the future as it is currently unclear at what point the devcontainer.json's forwardPorts property would become useful beyond VS Code.

Chuxel commented 3 years ago

@chrmarti

The implementation is currently such that both the VS Code Server and VS Code need to be running for port forwarding to be available.

Yep! Which is why it's still important to have some way to forward ports that are already up when VS Code Server starts. We don't want to forward every system port, but there are some the user will want to specify once things are up. This is addressing that these properties still have a role long term.

I expect the automatic forwarding can be turned off while explicitly configured forwardings in the settings will still be set up.

Great, yeah @lostintangent was asking about the importance of the property - in this scenario its even more important, so it was addressing that.

I agree with the rest, but would defer it to the future as it is currently unclear at what point the devcontainer.json's forwardPorts property would become useful beyond VS Code.

The property already exists, so what I'm advocating for is not to deprecate it. We mention it all over the place and existing devcontainer.json files will already have it. Taking it back and pushing towards another property given that fact is only something we should do if we really want it to go away long term, which I don't think is the case.

alexr00 commented 3 years ago

I have no problem with keeping the devcontainer.json forwarded ports. I just think that the additional properties that this issue tracks adding do not belong in devcontainer.json. Here's what I'm thinking for the new properties:

"settings": {
    // Ports listed below will NOT be forwarded immediately upon connection.
    // Instead, this specifies properties for these port when they are forwarded.
    // They could be forwarded by: 
    //    - auto forwarding
    //    - user forwarding
    //    - extension forwarding (such as from devcontainer.json)
    "remote.portsAttributes": [
        {
            "port": "8000", // Required property, can be a number or a range
            "host": "127.0.0.1", // Optional, defaults to localhost
            "label": "gatsbyserver", // Display label for the Ports view (optional)
            "autoForwardAction": "browser", // Enum indicating the action to be taken when the port is auto forwarded (optional)
            // Other options for autoForwardAction: ignore (don't auto forward),
            // notify (default, current behavior), silent (auto forward but no notification).
            "elevateIfPrivileged": false // The UX around elevating will require some interaction in addition to 
            // the elevation prompt.  When true, elevateIfPrivileged will skip this UX and just show the prompt.
        }   
         ]
},

Update: added elevateIfPrivileged

PavelSosin-320 commented 3 years ago

After hitting the Firewall CLI differences in the different Linux distro and releases issue I found that the most popular firewall CLI in the contemporary Linux Distros: Ubuntu, CentoOS, Suse is UFW - uncomplicated Firewall. Please, take into account its port designation syntax. Introduced in Ubuntu the Installation of UFW in the existing distro is also not a problem. UFW is accomplished with GUI in the Desktop distros.

alexr00 commented 3 years ago

This is now implemented. The current options:

"settings": {
    // Ports listed below will NOT be forwarded immediately upon connection.
    // Instead, this specifies properties for these port when they are forwarded.
    // They could be forwarded by: 
    //    - auto forwarding
    //    - user forwarding
    //    - extension forwarding (such as from devcontainer.json)
    "remote.portsAttributes": [
        {
            "port": "8000", // Required property, can be a number or a range
            "label": "gatsbyserver", // Display label for the Ports view (optional)
            "onAutoForward": "open", // Enum indicating the action to be taken when the port is auto forwarded (optional)
            // Other options for onAutoForward: ignore (don't auto forward),
            // notify (default, current behavior), silent (auto forward but no notification).
            "elevateIfNeeded": false // The UX around elevating will require some interaction in addition to 
            // the elevation prompt.  When true, elevateIfNeededwill skip this UX and just show the prompt.
        }   
         ]
},
roblourens commented 3 years ago

Is there another plan to address the issue with debug adapters, or do we expect to hardcode debug ports and use this setting? Also could port accept a number too?

egamma commented 3 years ago

Is there another plan to address the issue with debug adapters, or do we expect to hardcode debug ports and use this setting?

@roblourens pls see https://github.com/microsoft/vscode-internalbacklog/issues/1717