feat: support `forwardPorts` config and `--skip-forward-ports` flag

[agilgur5]

Summary

Fixes #186 by adding -p args for all forwardPorts, not just appPort Allows disabling this via the CLI flag --skip-forward-ports

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 https://github.com/devcontainers/cli/issues/186#issuecomment-1455910147: this commit: https://github.com/MunsMan/devcontainercli/commit/6909283588fc02695f48ca9572515acdeff1d4a8

• I simplified it and fixed some styling etc

Newer bits

EDIT: After review comments:

• added a CLI flag, --skip-forward-ports, to allow disabling this behavior for alternate implementations that use the CLI under-the-hood
  • summarized below

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 https://github.com/microsoft/vscode-remote-release/issues/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.)

[almathie]

Is there any progress on this ? On our side we are trying to leverage devcontainers for a pretty standard Rails app. The required ports are exposed through the forwardPorts directive as recommended here: https://containers.dev/implementors/json_reference/

Everything works fine for VSCode / Codespace (aka:Microsoft lead products) users but developers using vim/emcas or similar can't properly spin up a working devcontainer from their terminal.

It feels weird to me that the documentation recommends an approach that is currently unsupported by the reference implementation. Additionally, some tools and services are apparently leveraging the cli behind the scene, and they are thus subject to the same limitations (we tried).

At the moment it feels like using VSCode as the "devcontainer manager" is required to have forwardPorts working at all which goes a bit against again the idea of having devcontainers as an open specification.

[agilgur5]

reference implementation

It feels weird to me that the documentation recommends an approach that is currently unsupported by the reference implementation

At the moment it feels like using VSCode as the "devcontainer manager" is required to have forwardPorts working at all which goes a bit against again the idea of having devcontainers as an open specification.

Yea, I generally agree with both these statements per my above comments

summary

Is there any progress on this ?

Let me summarize the comments to address this (and refresh my memory and make sure everyone's on the same page):

The Dev Containers extension for VS Code and GitHub Codespaces both use the Dev Containers CLI to create the dev container.

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

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.)

As I understand, this basically means that the forwardPorts implementations in GH Codespaces and the VS Code extension are not compatible with the Docker publishing of this PR. And as they do use this CLI under-the-hood, that is why it was recommended to put these behind a flag or separate command.

A separate command would be incompatible with Docker publishing and so requires re-implementing forwarding (which is a bit of a rabbit hole and may require various dependencies on the host and container).

Both a separate command or behind a flag would make the reference implementation not quite correspond to the spec by default though.

next steps

As such, I would go back to my earlier proposal of an env var (or flag) to disable forwardPorts. The default then follows the spec and other implementations, like GH Codespaces and the VS Code extension, can disable this if they simultaneously use the CLI and also have alternate implementations.

That might seem like a breaking change from the GH Codespaces / VS Code extension perspective, but it is a feature for the reference since it did not previously exist in the reference. I.e. those implementations relied on an out-of-tree addition to the spec that would now conflict with the reference without an explicit disable / opt-out

@chrmarti would that proposal work?

At this point, I'm also willing to re-write it as an opt-in flag just to get the behavior in the CLI, but that would still be odd as a reference implementation and so would not be my preference. And of course, you can decide to release as a breaking change either way if desired to require downstream consumers to be aware of the change.

[agilgur5]

I added a --skip-forward-ports flag in https://github.com/devcontainers/cli/pull/859/commits/ae893d9bd934b39019af17eae98e8f6440d68d03.

The function arg drilling is not the most ideal, but changing that would require a lot of refactoring that I don't think I'm best suited to handle at this time. At this time, I followed the existing practices in the codebase for flags, naming, arg passing, etc without modifications.

[apriendeau]

@chrmarti is there anyway that we can review this? This blocks all editors that are not VS Code from developing.

[agilgur5]

following up @chrmarti

[chrmarti]

Publishing the ports by default would break existing setups. Could you invert the logic and make it --publish-forward-ports or something?

[agilgur5]

Could you invert the logic and make it --publish-forward-ports or something?

As I wrote above, this would not be spec compliant and therefore especially confusing for a reference implementation. If you really want me to, I can, as it's a simple change and this currently blocks lots of users and tools as others wrote above, but I am quite strongly against that option due to its lack of spec compliance.

Publishing the ports by default would break existing setups.

I think it's important to qualify this statement -- as I and others wrote above and in the issue, many existing setups are already broken because this feature doesn't exist in the reference implementation. Afaik, it would only break VS Code and GH Codespaces, and that's only if they were to update and not add --skip-forward-ports. It would only break those two because they have their own custom, out-of-tree implementation of forwardPorts that differs from the reference. Nominally and conventionally speaking, it would be the responsibility of those tools to disable reference features if they implement them differently out-of-tree.

As I wrote above, if you really want to, you can certainly release this feature as a breaking change with a major version bump, thereby requiring downstream users to acknowledge the breakage and adjust appropriately. As I wrote, this isn't strictly breaking in the sense that it's adding a missing feature, but could be considered so in the sense that it changes default behavior (to be spec compliant).