Control automatic port forwarding with a devcontainer.json file #49

aaronlehmann · 2025-02-21T22:42:45Z

devcontainer.json has a system for specifying default behavior for forwarding ports, and also behavior for specific ports and ranges of ports. Its schema is essentially identical to the settings VS Code uses to control port forwarding, so supporting this format for port settings keeps things consistent between VS Code and JetBrains.

See https://containers.dev/implementors/json_reference/ for the spec. As an example, this will turn off automatic port forwarding except for ports 7123 and 8100-8150:

{
  "otherPortsAttributes": {
    "onAutoForward": "ignore"
  },
  "portsAttributes": {
    "7123": {
      "onAutoForward": "notify"
    },
    "8100-8150": {
      "onAutoForward": "notify"
    }
  }
}

Fixes: #38

aaronlehmann · 2025-02-21T22:44:11Z

src/main/kotlin/com/coder/jetbrains/services/CoderPortForwardService.kt

@@ -48,12 +56,44 @@ class CoderPortForwardService(
    }

    private fun start() {
-        logger.info("starting port scanner")
+        // TODO: make path configurable?
+        val devcontainerFile = File(System.getProperty("user.home"), ".cache/JetBrains/devcontainer.json")


I originally wanted to make this a setting, but this turns out to be a bit complicated because this plugin doesn't have any settings yet, so there's a bit of infrastructure involved. I can come back to this if needed, or we can go with this fixed path for now (or some other path). It's pretty straightforward for other software on the workspace to put the file in the expected place or create a symlink.

I think a fixed path for now is fine, and then we'll see if a need for a setting arises.

Nit: Might be worth extracting to a function in some kind of config module just to have a natural place to start moving things that could become settings, but definitely doesn't need to happen in this PR.

Moved to a settings class, please let me know if the location/naming makes sense.

bcpeinhardt

Approving as all my comments are actually nits. I like the approach here, especially that it provides consistency with VSCode.

Side note: Everybody has different preferences on the level on "nit" comments they want to see in reviews, so let me know if these bug you. I mainly don't want you to see a blanket approval on a PR and think "Ben hardly read this", but I also don't want to hold up progress unless there's actually a good reason too :)

bcpeinhardt · 2025-02-25T20:29:16Z

src/main/kotlin/com/coder/jetbrains/matcher/PortMatcher.kt

+
+        return when {
+            // Try parsing as single port
+            portPart.all { it.isDigit() } -> {


Non Blocking Nit/Question: Should we validate the port is between 0 and 65535 for the SinglePort and PortRange parsing?

Added this validation

bcpeinhardt · 2025-02-25T20:41:29Z

src/main/kotlin/com/coder/jetbrains/services/CoderPortForwardService.kt

@@ -48,12 +56,44 @@ class CoderPortForwardService(
    }

    private fun start() {
-        logger.info("starting port scanner")
+        // TODO: make path configurable?
+        val devcontainerFile = File(System.getProperty("user.home"), ".cache/JetBrains/devcontainer.json")


I think a fixed path for now is fine, and then we'll see if a need for a setting arises.

Nit: Might be worth extracting to a function in some kind of config module just to have a natural place to start moving things that could become settings, but definitely doesn't need to happen in this PR.

bcpeinhardt · 2025-02-25T21:02:21Z

src/main/kotlin/com/coder/jetbrains/services/CoderPortForwardService.kt

+                val portsAttributes = obj.optJSONObject("portsAttributes") ?: JSONObject()
+                portsAttributes.keys().forEach { spec ->
+                    portsAttributes.optJSONObject(spec)?.let { attrs ->
+                        val onAutoForward = attrs.optString("onAutoForward")


Nit/Not even sure I want a change here: spec says "onAutoForward" is an an enum of [ "notify", "openBrowser", "openBrowserOnce", "openPreview", "silent", "ignore" ]. Might be worth checking we don't set a PortMatcher('[object Object]') or something to save us a debugging headache in the future, but also see the value of leaving it flexible so we don't have to come back and make an update in the event the spec changes

I added validation that the onAutoForward value is a string. I think it probably makes sense to accept any non-empty value here except ignore as equivalent to notify, but happy to tweak this as you see fit.

codecov-commenter · 2025-02-25T21:15:47Z

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

src/main/kotlin/com/coder/jetbrains/matcher/PortMatcher.kt

src/main/kotlin/com/coder/jetbrains/services/CoderPortForwardService.kt

src/test/kotlin/com/coder/jetbrains/matcher/PortMatcherTest.kt

devcontainer.json has a system for specifying default behavior for forwarding ports, and also behavior for specific ports and ranges of ports. Its schema is essentially identical to the settings VS Code uses to control port forwarding, so supporting this format for port settings keeps things consistent between VS Code and JetBrains. See https://containers.dev/implementors/json_reference/ for the spec. As an example, this will turn off automatic port forwarding except for ports 7123 and 8100-8150: { "otherPortsAttributes": { "onAutoForward": "ignore" }, "portsAttributes": { "7123": { "onAutoForward": "notify" }, "8100-8150": { "onAutoForward": "notify" } } } Fixes: coder#38

fioan89 · 2025-02-26T09:29:12Z

build.gradle.kts

@@ -24,6 +25,7 @@ repositories {
 // Dependencies are managed with Gradle version catalog - read more: https://docs.gradle.org/current/userguide/platforms.html#sub:version-catalog
 dependencies {
 //    implementation(libs.annotations)
+    implementation("org.jetbrains.kotlinx:kotlinx-serialization-json:1.8.0")


Thank you for changing this.

Since IntelliJ provides it at runtime as well this should be compileOnly. Another important thing is that here we need to declare the version that is packaged with minimum supported IntelliJ.
We support IntelliJ 2022.3 which uses kotlinx.serialization:1.4.1 so that's the minimum version we should use, and it should only be changed when the minimum supported version of IntelliJ changes.

That's unfortunate because this version is too old to support allowComments, and devcontainer.json files often include comments (since the reference implementation and VS Code allow them). It's not an issue in my particular case because I'll be stripping comments before injecting this file, but for maximum compatibility it seems like supporting comments would be a good thing.

Could we depend on this newer version and add a comment that the dependency can be changed to compileOnly once the minimum supported IDE version provides kotlinx.serialization >= 1.7.0? It looks like that minimum IDE version will be idea/251.14649.49 (which includes JetBrains/intellij-community@548a3c9).

as far as I know it should be possible to shadow the dependencies but please test it. As a rule of thumb is best not to shadow intellij dependencies to avoid any classloader/memory leak issue.

Some context: any jars packaged with the plugin are not exposed to the rest of the IDE or other plugins, the plugin has its own classloader, so it's a good way to make plugin stable in its sandbox

So if we need this serialization library and there's a compatibility issue, it's better to bundle it.

I added the comment about switching to compileOnly in the future. I've been testing with IntelliJ 242.23339.11 and it seems to work fine, but just tried with EAP 251.22821.72 and it works with that version as well.

Let me know if you think it's better or safer to downgrade the dependency and remove allowComments. I'm also fine with that approach.

the plugin has its own classloader, so it's a good way to make plugin stable in its sandbox

Correct , and intellij platform sdk also allows the plugins to reference classes provided by other plugins in which the classloader for those plugin dependencies will be used. Not really the case here, so I agree with your statement. It should be safe to pack the serialization library.

Let me know if you think it's better or safer to downgrade the dependency and remove allowComments

I agree with @kirillk there is no need for a downgrade for now.

aaronlehmann · 2025-02-27T15:46:31Z

@bcpeinhardt @fioan89: Thanks for the reviews! Are there any next steps to get this merged?

fioan89 · 2025-02-27T16:09:43Z

I don't have any other concern @aaronlehmann

aaronlehmann commented Feb 21, 2025

View reviewed changes

aaronlehmann mentioned this pull request Feb 21, 2025

Feature: Dynamic port forwarding allow+deny list #38

Closed

aaronlehmann force-pushed the ports-from-devcontainer-json branch from ad75308 to ede5a60 Compare February 22, 2025 16:48

bcpeinhardt approved these changes Feb 25, 2025

View reviewed changes

fioan89 reviewed Feb 25, 2025

View reviewed changes

aaronlehmann added 6 commits February 25, 2025 17:17

Validate single port or range endpoint is between 0 and 65535

ae1df2f

Split devcontainer.json file path out to a settings module

933e1d4

Add validation that onAutoForward is a string

cf25cbe

PortMatcher code review feedback

69432d7

Use kotlinx.serialization for JSON deserialization

3433d28

aaronlehmann force-pushed the ports-from-devcontainer-json branch from 8886009 to 3433d28 Compare February 26, 2025 01:17

fioan89 reviewed Feb 26, 2025

View reviewed changes

Add comment about switching kotlinx-serialization-json to compileOnly

2da0908

fioan89 approved these changes Feb 26, 2025

View reviewed changes

bcpeinhardt and others added 2 commits February 27, 2025 14:20

Merge branch 'main' into ports-from-devcontainer-json

8d86ee7

changelog

fa0df63

bcpeinhardt merged commit f6783a2 into coder:main Feb 27, 2025

ggjulio mentioned this pull request May 8, 2025

Devcontainer Integration (coderd, agent, ui and IDEs) coder/coder#15143

Open

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Control automatic port forwarding with a devcontainer.json file #49

Control automatic port forwarding with a devcontainer.json file #49

aaronlehmann commented Feb 21, 2025

aaronlehmann Feb 21, 2025

bcpeinhardt Feb 25, 2025

aaronlehmann Feb 25, 2025

bcpeinhardt left a comment

bcpeinhardt Feb 25, 2025

aaronlehmann Feb 25, 2025

bcpeinhardt Feb 25, 2025

bcpeinhardt Feb 25, 2025

aaronlehmann Feb 25, 2025

codecov-commenter commented Feb 25, 2025

fioan89 Feb 26, 2025 •

edited

Loading

aaronlehmann Feb 26, 2025

fioan89 Feb 26, 2025

kirillk Feb 26, 2025

aaronlehmann Feb 26, 2025 •

edited

Loading

fioan89 Feb 26, 2025

aaronlehmann commented Feb 27, 2025

fioan89 commented Feb 27, 2025

Control automatic port forwarding with a devcontainer.json file #49

Control automatic port forwarding with a devcontainer.json file #49

Conversation

aaronlehmann commented Feb 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcpeinhardt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Feb 25, 2025

Welcome to Codecov 🎉

fioan89 Feb 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronlehmann Feb 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronlehmann commented Feb 27, 2025

fioan89 commented Feb 27, 2025

fioan89 Feb 26, 2025 •

edited

Loading

aaronlehmann Feb 26, 2025 •

edited

Loading