feat: support forwardPorts config

[agilgur5]

Summary

Fixes #186 by adding -p args for all forwardPorts, not just appPort

Details

• split out a normalizePorts function used on both appPort and forwardPorts
• then concat and flatMap those two together into args

Credits to @MunsMan for the original variant listed in this comment #186 (comment): this commit: MunsMan@6909283

• I simplified it and fixed some styling etc

Notes for Reviewers

There didn't seem to be any existing tests for appPort that I could extend for forwardPorts, afaict. Let me know if there's a proper place to add tests for this

[chrmarti]

Thanks for the PR! A few considerations:

The Dev Containers extension for VS Code forwards the listed ports to a free port on the local machine (which might be different from where Docker runs). GitHub Codespaces also has its own port forwarding. Additionally using -p might conflict with other containers running on the same Docker host.

Could this be added behind a CLI option, so the above scenarios don't break? forwardPorts is also available for dev containers using Docker Compose (unlike appPort).

[agilgur5]

I would think that all of those scenarios should be handled already downstream, as the behavior is identical to appPort and so they'd need to handle it already, no?

Could this be added behind a CLI option, so the above scenarios don't break?

For a reference implementation, that seems pretty confusing to me: the spec is the source of truth, yet you also need a flag? That behavior would be "ignore the spec field by default unless I tell you otherwise", which is pretty confusing, esp for a reference implementation -- why would the reference ignore the spec?
An env var to disable might make more sense, although, per above, I'm not sure that would really be necessary.

[chrmarti]

The extension uses VS Code's port forwarding which is temporary while VS Code is connected and also works when the VS Code window runs on a different machine than the Docker daemon. (It is not use Docker's port forwarding.)

I think another approach would be to replicate the temporary forwarding as part of an additional CLI command which would keep running until the port forwarding is no longer required and only then would be terminated.

[agilgur5]

The extension uses VS Code's port forwarding

Then it should already be ignoring this, no? As it already ignores appPort for the same reason

I think another approach would be to replicate the temporary forwarding as part of an additional CLI command

While this makes sense to me, this doesn't match the existing behavior of appPort. As far as I could tell, forwardPorts is partly an array replacement for it, since the spec says:

In most cases, we recommend using the new forwardPorts property.

The property is most useful for forwarding ports that cannot be auto-forwarded

But I don't think I realized appPort could be an array as well, that workaround might suffice for my case at least. I feel like I tried that and it didn't work for some reason though 🤔 (I do use the same spec for the extension and Codespaces when necessary, but tend to prefer the more minimal CLI, unsure if related)

I had also looked at microsoft/vscode-remote-release#1809, where you did say (almost 5 years ago):

I agree, we can't make published ports disappear, they are part of the basic functionality of Docker.

and someone else said a bit further up in the thread:

and the fact that it's done with docker publishing the port seems like an implementation detail. Instead of surfacing a confusing implementation detail (publish vs. forward)

So I assumed they're equivalent in a Docker-based implementation. I'm not sure if that's changed since then or that's specific to the VS Code extension, but since the devcontainer CLI is similarly Docker-based, I would think that applies the same?

(It is not use Docker's port forwarding.)

This makes it sound like the extension is different these days (also is the extension not open source? I tried finding its implementation to compare), but I wonder if the same makes sense to apply to the CLI. At least in my usage, my devcontainer is only running (up) when I'm using it and so the ports are only forwarded then. Auto forwarding would be nice over a constant forward when nothing is available, but that's just a nice-to-have; I still stop the container when I'm not using it

[chrmarti]

Just to clarify: The Dev Containers extension for VS Code and GitHub Codespaces both use the Dev Containers CLI to create the dev container. We moved away (appPort > forwardPorts) from Docker-based forwarding ("publishing") because these can't be added to an already running container and they only work locally, not when the dev container runs on a remote machine connected via SSH.

[agilgur5]

A few questions for clarity:

both use the Dev Containers CLI to create the dev container.

Gotcha. And then they have their own forwarding implementation, right?

How do they handle the existing appPort publishing right now though? That is throwing me for a loop, since I just reused that existing behavior in the PR and you're saying it's problematic

because these can't be added to an already running container

Yea that was my other concern -- with a separate command for this, it would be done while already running, meaning we can't use the Docker implementation and have to reimplement forwarding (which is a bit of a rabbit hole). Does it make sense to move/share the downstream implementations here?

[chrmarti]

We left appPort as-is at the time to not break anyone and started to recommend users use forwardPorts instead.

There is some basic forwarding code in the extension that is used to establish an initial connection for VS Code. VS Code then tunnels everything through that initial connection. That code assumes Node.js is available in the container - which is true when using VS Code and the VS Code Server. (The extension is not OSS.)