feat: allow remote hostnames in ssh forward ports#742
Open
davzucky wants to merge 6 commits intoskevetter:mainfrom
Open
feat: allow remote hostnames in ssh forward ports#742davzucky wants to merge 6 commits intoskevetter:mainfrom
davzucky wants to merge 6 commits intoskevetter:mainfrom
Conversation
This keeps reverse forwarding unchanged while letting forward-only port mappings target service names and other remote hostnames that resolve inside the workspace runtime.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds remote service-name/hostname support to SSH forward-port specs, refactors port-spec parsing and port-forward orchestration into a shared runner, updates CLI help and docs, and adds unit and e2e tests covering hostname-based forwarding and orchestration behaviors. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(20,116,240,0.5)
participant LocalCLI as Local devpod CLI
end
rect rgba(34,197,94,0.5)
participant SSHMgr as runPortForwards / SSH manager
end
rect rgba(236,72,153,0.5)
participant RemoteWS as Remote Workspace / Container
end
rect rgba(250,204,21,0.5)
participant TargetSvc as Target Service (e.g., nginx)
end
LocalCLI->>SSHMgr: start with `--forward-ports` spec (e.g., 18080:nginx:8080)
SSHMgr->>RemoteWS: establish SSH tunnel / request remote connect to "nginx:8080"
RemoteWS->>TargetSvc: resolve "nginx" and dial :8080
TargetSvc-->>RemoteWS: respond (HTTP)
RemoteWS-->>SSHMgr: forward traffic back through tunnel
SSHMgr-->>LocalCLI: expose local port (18080) with proxied traffic
Note over LocalCLI,TargetSvc: Client requests to localhost:18080 reach TargetSvc via SSH + remote DNS resolution
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
cmd/ssh_forward_ports.go (3)
35-50: Error wrapper says "container address" in an SSH-forwarding context.
newForwardPortMappingwraps the remote-side error as"parse container address: ...". That terminology is carried over fromport.ParsePortSpec, but the whole point of this new parser is that the remote side is a host/service, not necessarily a container. Consider"parse remote address"to match the rest of this file's naming (parseForwardRemoteAddress).Proposed diff
- containerAddress, err := parseForwardRemoteAddress(remoteHost, remotePort) - if err != nil { - return port.Mapping{}, fmt.Errorf("parse container address: %w", err) - } + remoteAddress, err := parseForwardRemoteAddress(remoteHost, remotePort) + if err != nil { + return port.Mapping{}, fmt.Errorf("parse remote address: %w", err) + } @@ - Container: containerAddress, + Container: remoteAddress,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/ssh_forward_ports.go` around lines 35 - 50, newForwardPortMapping currently wraps errors from parseForwardRemoteAddress using the phrase "parse container address"; update the error message to reflect SSH-forwarding terminology by returning fmt.Errorf("parse remote address: %w", err) instead of "parse container address: %w". Locate this change in the newForwardPortMapping function where containerAddress is set from parseForwardRemoteAddress and adjust the error wrapper to "parse remote address" to match parseForwardRemoteAddress and the file's naming.
83-86:isPortNumberdoesn't validate range (0–65535).
strconv.Atoiaccepts"0", negatives, and values > 65535. Downstreamnet.JoinHostPortwill happily accept any string, and SSH forwarding will fail later with a less clear error. This matches the pre-existing behavior ofpkg/port.toAddress, so not a regression — but since you're already building a dedicated parser, a range check here would give users better diagnostics. Optional.Proposed tightening
func isPortNumber(raw string) bool { - _, err := strconv.Atoi(raw) - return err == nil + n, err := strconv.Atoi(raw) + return err == nil && n >= 0 && n <= 65535 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/ssh_forward_ports.go` around lines 83 - 86, The isPortNumber helper currently only checks that strconv.Atoi(raw) succeeds but does not validate the port range; update isPortNumber to parse the string as an integer and ensure the value is within 0..65535 (e.g., use strconv.Atoi or strconv.ParseUint and then verify 0 <= port && port <= 65535) so callers like net.JoinHostPort/SSH forwarding get an immediate, clear validation error; keep the function name isPortNumber and return true only when the numeric range check passes.
24-32: 4-part branch: consider treating empty local host the same as the 3-part numeric case.In the 3-part numeric path,
newForwardPortMapping("", ...)happily defaults the local bind tolocalhost(viaparseForwardTCPAddress). In the 4-part path, an explicitly empty first segment (:8080:nginx:80) is rejected with "local host is empty". Not wrong, but slightly asymmetric. If the intent is to always require explicitness in 4-part form, a brief comment on line 25 would help; otherwise defaulting tolocalhostwould be more consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/ssh_forward_ports.go` around lines 24 - 32, The 4-part branch currently rejects an empty local host but the 3-part numeric path allows an empty host and defaults to localhost via parseForwardTCPAddress called by newForwardPortMapping; remove the explicit error check in the case 4 branch and instead pass parts[0], parts[1], parts[2], parts[3] to newForwardPortMapping (so an empty parts[0] will be handled consistently by newForwardPortMapping/parseForwardTCPAddress), or if you prefer to keep the rejection, add a short comment explaining the intentional asymmetry next to the check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/tests/up-docker-compose/up_docker_compose.go`:
- Around line 183-186: The current pattern uses net.Listen to get a free port
then closes the listener (listener.Close()) and reuses localPort, which
introduces a TOCTOU race; to fix it eliminate the race by keeping the listener
open and handing the active listener (or its file descriptor) directly to the
code that will serve SSH rather than closing and re-binding — modify the code
around net.Listen, listener, localPort and the devpod ssh invocation so the
produced listener is passed through to the server routine (or use the listener's
FD transfer) instead of closing and re-opening the port.
---
Nitpick comments:
In `@cmd/ssh_forward_ports.go`:
- Around line 35-50: newForwardPortMapping currently wraps errors from
parseForwardRemoteAddress using the phrase "parse container address"; update the
error message to reflect SSH-forwarding terminology by returning
fmt.Errorf("parse remote address: %w", err) instead of "parse container address:
%w". Locate this change in the newForwardPortMapping function where
containerAddress is set from parseForwardRemoteAddress and adjust the error
wrapper to "parse remote address" to match parseForwardRemoteAddress and the
file's naming.
- Around line 83-86: The isPortNumber helper currently only checks that
strconv.Atoi(raw) succeeds but does not validate the port range; update
isPortNumber to parse the string as an integer and ensure the value is within
0..65535 (e.g., use strconv.Atoi or strconv.ParseUint and then verify 0 <= port
&& port <= 65535) so callers like net.JoinHostPort/SSH forwarding get an
immediate, clear validation error; keep the function name isPortNumber and
return true only when the numeric range check passes.
- Around line 24-32: The 4-part branch currently rejects an empty local host but
the 3-part numeric path allows an empty host and defaults to localhost via
parseForwardTCPAddress called by newForwardPortMapping; remove the explicit
error check in the case 4 branch and instead pass parts[0], parts[1], parts[2],
parts[3] to newForwardPortMapping (so an empty parts[0] will be handled
consistently by newForwardPortMapping/parseForwardTCPAddress), or if you prefer
to keep the rejection, add a short comment explaining the intentional asymmetry
next to the check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9ef32169-ad95-4a2c-ad62-c1612708f883
📒 Files selected for processing (4)
cmd/ssh.gocmd/ssh_forward_ports.gocmd/ssh_forward_ports_test.goe2e/tests/up-docker-compose/up_docker_compose.go
Comment on lines
+183
to
+186
| listener, err := net.Listen("tcp", "127.0.0.1:0") | ||
| framework.ExpectNoError(err) | ||
| localPort := listener.Addr().(*net.TCPAddr).Port | ||
| framework.ExpectNoError(listener.Close()) |
There was a problem hiding this comment.
Dynamic port allocation has a minor TOCTOU window, but acceptable for e2e.
Closing the listener and reusing the port has a brief race where another process could grab it before devpod ssh binds. In practice this is fine for e2e and matches common Go test patterns, so no change required — just flagging it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/tests/up-docker-compose/up_docker_compose.go` around lines 183 - 186, The
current pattern uses net.Listen to get a free port then closes the listener
(listener.Close()) and reuses localPort, which introduces a TOCTOU race; to fix
it eliminate the race by keeping the listener open and handing the active
listener (or its file descriptor) directly to the code that will serve SSH
rather than closing and re-binding — modify the code around net.Listen,
listener, localPort and the devpod ssh invocation so the produced listener is
passed through to the server routine (or use the listener's FD transfer) instead
of closing and re-opening the port.
Format the new forward-port parser as expected by golangci-lint-fmt and satisfy lint in the compose e2e by handling the HTTP response close and documenting the controlled exec arguments.
skevetter
reviewed
Apr 19, 2026
| "github.com/skevetter/devpod/pkg/port" | ||
| ) | ||
|
|
||
| func parseForwardPortSpec(raw string) (port.Mapping, error) { |
Owner
There was a problem hiding this comment.
cmd has historically been abused as a dumping ground. Let's move this over pkg if needed.
Also, is this logic a duplicate of
Line 70 in f6c3ab2
Author
There was a problem hiding this comment.
Agreed. I moved the parsing into pkg/port and removed the cmd-local parser. The final shape now uses a single shared parser for both --forward-ports and --reverse-forward-ports: the listen side stays strict, while the dial side accepts hostnames and service names.
Move the parser into pkg/port so both ssh forwarding directions share the same listen-versus-target rules while allowing service names on the dial side and documenting the new behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/port/parse.go (1)
91-103:⚠️ Potential issue | 🟡 MinorReject empty Unix socket paths during parsing.
toUnixAddresscurrently accepts an emptyport/socket path, so malformed specs like"",":8080", or"8080:"can parse as Unix socket mappings and fail later in forwarding.🛡️ Proposed validation
func toUnixAddress(ip, port string, opts addressOptions) (Address, error) { + if port == "" { + return Address{}, fmt.Errorf("unix socket path is empty") + } + if opts.requireHost && ip == "" { return Address{}, fmt.Errorf("%s is empty", opts.emptyHostLabel) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/port/parse.go` around lines 91 - 103, toUnixAddress currently permits an empty port/socket path which lets malformed specs parse as a unix Address; update to validate that port is non-empty and return an error when it is empty (use the existing opts.emptyHostLabel for the error message so behavior is consistent with the requireHost check). Specifically, inside toUnixAddress (and still honoring opts.requireHost/opts.emptyHostLabel), add a guard that if port == "" return Address{} with fmt.Errorf("%s is empty", opts.emptyHostLabel) instead of constructing a unix Address with an empty path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/ssh.go`:
- Around line 465-482: The goroutine that calls config.forwardFn sometimes
returns without sending anything to errChan on clean exits (nil or io.EOF),
causing the receive at "return <-errChan" to hang; modify the goroutine (the
anonymous func invoked with portMapping, mapping) so that whenever
config.forwardFn returns nil or io.EOF it still signals completion to errChan
(for example send nil or a sentinel error like nil) and only send a wrapped
error when err != nil && !errors.Is(err, io.EOF); alternatively implement a
WaitGroup and close errChan after all goroutines finish and change the receiver
to read from the closed channel—reference config.forwardFn, errChan, the
anonymous goroutine and portMapping/mapping to locate where to apply the change.
---
Outside diff comments:
In `@pkg/port/parse.go`:
- Around line 91-103: toUnixAddress currently permits an empty port/socket path
which lets malformed specs parse as a unix Address; update to validate that port
is non-empty and return an error when it is empty (use the existing
opts.emptyHostLabel for the error message so behavior is consistent with the
requireHost check). Specifically, inside toUnixAddress (and still honoring
opts.requireHost/opts.emptyHostLabel), add a guard that if port == "" return
Address{} with fmt.Errorf("%s is empty", opts.emptyHostLabel) instead of
constructing a unix Address with an empty path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d66353ea-efd6-4745-b67a-be2e16a4eaed
📒 Files selected for processing (5)
cmd/ssh.godocs/pages/developing-in-workspaces/connect-to-a-workspace.mdxe2e/tests/up-docker-compose/up_docker_compose.gopkg/port/parse.gopkg/port/parse_test.go
✅ Files skipped from review due to trivial changes (1)
- docs/pages/developing-in-workspaces/connect-to-a-workspace.mdx
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/tests/up-docker-compose/up_docker_compose.go
Wait for all forwarding goroutines to finish so clean exits cannot hang the ssh command, and reject empty unix socket paths during port spec parsing with regression tests for both cases.
Author
|
Addressed the latest review feedback in 6b88d4a:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cmd/ssh_test.go (1)
65-88: Consider extending coverage to error and EOF paths.The new test only validates the happy path. Given the recent hang fix (WaitGroup +
close(errChan)) and the intentionalio.EOFtolerance inrunPortForwards, it would be valuable to add table-driven cases that:
- return
io.EOFfromforwardFnand assertNoError(regression guard for the clean-shutdown fix referenced in past reviews),- return a real error and assert it is wrapped with
"error forwarding ..."and propagated,- pass an invalid mapping (e.g.,
"") and assert a parse error wrapped with"parse port mapping",- use multiple mappings with mixed outcomes to exercise the concurrency and the first-error return.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/ssh_test.go` around lines 65 - 88, Add table-driven tests for SSHCmd.runPortForwards to cover EOF and error paths: create cases that (1) have forwardFn return io.EOF and assert no error (to guard the EOF-tolerance behavior), (2) have forwardFn return a non-nil error and assert the returned error is wrapped with "error forwarding ..." and propagated, (3) provide an invalid mapping like "" and assert a parse error wrapped with "parse port mapping", and (4) use multiple mappings where one succeeds and another returns an error to exercise concurrency and first-error return; reference SSHCmd, runPortForwards, portForwardConfig, mappings, forwardFn and ensure tests assert expected wrapping/propagation semantics and that the errChan/WaitGroup shutdown path behaves correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/ssh.go`:
- Around line 446-449: forwardTimeout currently shadows the parsed timeout
variable so it always returns zero; change the inner declaration in function
forwardTimeout (replace "timeout, err :=
time.ParseDuration(cmd.ForwardPortsTimeout)" with an assignment that sets the
outer timeout variable or simply return the parsed duration) so the successfully
parsed duration is actually returned; ensure runPortForwards (and its calls into
devssh.PortForward and devssh.ReversePortForward) receives and uses the non-zero
timeout from forwardTimeout.
- Around line 451-485: runPortForwards currently launches forwarder goroutines
as it parses each port spec using port.ParsePortSpec, which can cause orphaned
in-flight forwarders if a later parse fails; to fix, parse and validate all
entries from config.mappings up front (call port.ParsePortSpec for each into a
slice of port.Mapping) before spawning any goroutines, or alternatively create a
cancellable child context (ctxChild, cancel := context.WithCancel(ctx)) and if
any parse error occurs call cancel(), then ensure you still wait on waitGroup
and spawn the close-on-wait goroutine before returning; update uses of
mapping/portMapping, waitGroup, errChan and forwardFn in runPortForwards
accordingly so no listener is left running on early return.
---
Nitpick comments:
In `@cmd/ssh_test.go`:
- Around line 65-88: Add table-driven tests for SSHCmd.runPortForwards to cover
EOF and error paths: create cases that (1) have forwardFn return io.EOF and
assert no error (to guard the EOF-tolerance behavior), (2) have forwardFn return
a non-nil error and assert the returned error is wrapped with "error forwarding
..." and propagated, (3) provide an invalid mapping like "" and assert a parse
error wrapped with "parse port mapping", and (4) use multiple mappings where one
succeeds and another returns an error to exercise concurrency and first-error
return; reference SSHCmd, runPortForwards, portForwardConfig, mappings,
forwardFn and ensure tests assert expected wrapping/propagation semantics and
that the errChan/WaitGroup shutdown path behaves correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 174c244c-099f-4f09-be70-09b41d854785
📒 Files selected for processing (4)
cmd/ssh.gocmd/ssh_test.gopkg/port/parse.gopkg/port/parse_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/port/parse_test.go
- pkg/port/parse.go
Return the parsed forward timeout value, validate all port mappings before any forwarders start, and add focused regression coverage for timeout parsing, EOF handling, parse failures, and mixed multi-mapping outcomes.
Extract port mapping parsing to reduce command complexity, split the forwarding regression coverage into smaller focused tests, and make the multi-mapping error case deterministic under concurrency.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cmd/ssh.go (1)
446-451: Include the failing mapping in parse errors.With multiple
-L/-Rvalues,parse port mapping: ...does not identify which spec failed. IncludeportMappingin the wrapper.Small diagnostic improvement
mapping, err := port.ParsePortSpec(portMapping) if err != nil { - return nil, fmt.Errorf("parse port mapping: %w", err) + return nil, fmt.Errorf("parse port mapping %q: %w", portMapping, err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/ssh.go` around lines 446 - 451, The parsePortForwards function currently wraps port.ParsePortSpec errors without the failing spec; change the error wrapper to include the portMapping value so callers can see which -L/-R string failed (e.g., replace fmt.Errorf("parse port mapping: %w", err) with a wrapper that includes portMapping). Update the error message produced in parsePortForwards (referencing the portMapping variable and the port.ParsePortSpec call) to include the spec text and the original error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/ssh.go`:
- Around line 119-121: The reverse-forward flag registration currently exposes
expanded port.ParsePortSpec behavior and mentions service names/hostnames;
change cmd.ReverseForwardPorts handling to use the legacy/direction-aware parser
used for reverse bindings (route through the strict reverse parser instead of
port.ParsePortSpec) and update the flag registration (StringArrayVarP for
cmd.ReverseForwardPorts) help text to match the original legacy semantics
(remove service-name/hostname wording and limit to the legacy reverse-forward
syntax). Ensure the code that parses cmd.ReverseForwardPorts calls the
reverse-specific parsing function (e.g., the legacy ParseReversePortSpec or
equivalent direction-aware parser) and not the generic port.ParsePortSpec so
reverse forwarding remains restricted to the original behavior.
- Around line 463-524: Wrap the incoming ctx in a cancellable child (ctx, cancel
:= context.WithCancel(ctx)) at the top of runPortForwards and pass that child
ctx into config.forwardFn so all forwarder goroutines see cancellation; in the
per-goroutine error handling, when you detect a real error (err != nil &&
!errors.Is(err, io.EOF)) send the formatted error to errChan and call cancel()
to cancel sibling forwarders; in the receiving loop, capture the first error but
continue draining errChan until it is closed by the waitGroup goroutine, then
return the first non-nil error (ensuring cancel is deferred/called to release
resources when leaving).
---
Nitpick comments:
In `@cmd/ssh.go`:
- Around line 446-451: The parsePortForwards function currently wraps
port.ParsePortSpec errors without the failing spec; change the error wrapper to
include the portMapping value so callers can see which -L/-R string failed
(e.g., replace fmt.Errorf("parse port mapping: %w", err) with a wrapper that
includes portMapping). Update the error message produced in parsePortForwards
(referencing the portMapping variable and the port.ParsePortSpec call) to
include the spec text and the original error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4d6db67f-d3b1-44f2-bbc7-8fa7eee898cf
📒 Files selected for processing (2)
cmd/ssh.gocmd/ssh_test.go
✅ Files skipped from review due to trivial changes (1)
- cmd/ssh_test.go
Comment on lines
119
to
+121
| StringArrayVarP(&cmd.ReverseForwardPorts, "reverse-forward-ports", "R", []string{}, | ||
| "Specifies that connections to the given TCP port or Unix socket on the local (client) "+ | ||
| "host are to be reverse forwarded to the given host and port, or Unix socket, on the remote side.") | ||
| "Specifies that connections to the given TCP port or Unix socket on the remote side "+ | ||
| "are to be reverse forwarded to the given local host, service name, and port, or Unix socket.") |
There was a problem hiding this comment.
Keep --reverse-forward-ports on the legacy parser/contract.
reverseForwardPorts now shares the expanded port.ParsePortSpec path, and the help text advertises service names for -R. That appears to broaden reverse forwarding even though the PR scope says hostname/service-name support is forward-only. Please route -R through a strict/direction-aware parser and keep the reverse help text aligned with the legacy behavior.
Possible structure
type portForwardConfig struct {
mappings []string
logTemplate string
forwardFn portForwardFunc
+ parseFn func(string) (port.Mapping, error)
}
func (cmd *SSHCmd) reverseForwardPorts(
ctx context.Context,
containerClient *ssh.Client,
log log.Logger,
) error {
return cmd.runPortForwards(ctx, containerClient, portForwardConfig{
mappings: cmd.ReverseForwardPorts,
logTemplate: "Reverse forwarding remote %s/%s to local %s/%s",
forwardFn: devssh.ReversePortForward,
+ parseFn: port.ParseReversePortSpec, // strict parser preserving legacy -R behavior
}, log)
}
func (cmd *SSHCmd) forwardPorts(
ctx context.Context,
containerClient *ssh.Client,
log log.Logger,
) error {
return cmd.runPortForwards(ctx, containerClient, portForwardConfig{
mappings: cmd.ForwardPorts,
logTemplate: "Forwarding local %s/%s to remote %s/%s",
forwardFn: devssh.PortForward,
+ parseFn: port.ParsePortSpec,
}, log)
}Also applies to: 405-409
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/ssh.go` around lines 119 - 121, The reverse-forward flag registration
currently exposes expanded port.ParsePortSpec behavior and mentions service
names/hostnames; change cmd.ReverseForwardPorts handling to use the
legacy/direction-aware parser used for reverse bindings (route through the
strict reverse parser instead of port.ParsePortSpec) and update the flag
registration (StringArrayVarP for cmd.ReverseForwardPorts) help text to match
the original legacy semantics (remove service-name/hostname wording and limit to
the legacy reverse-forward syntax). Ensure the code that parses
cmd.ReverseForwardPorts calls the reverse-specific parsing function (e.g., the
legacy ParseReversePortSpec or equivalent direction-aware parser) and not the
generic port.ParsePortSpec so reverse forwarding remains restricted to the
original behavior.
Comment on lines
+463
to
524
| func (cmd *SSHCmd) runPortForwards( | ||
| ctx context.Context, | ||
| containerClient *ssh.Client, | ||
| log log.Logger, | ||
| config portForwardConfig, | ||
| logger log.Logger, | ||
| ) error { | ||
| timeout, err := cmd.forwardTimeout(log) | ||
| timeout, err := cmd.forwardTimeout(logger) | ||
| if err != nil { | ||
| return fmt.Errorf("parse forward ports timeout: %w", err) | ||
| } | ||
|
|
||
| errChan := make(chan error, len(cmd.ForwardPorts)) | ||
| for _, portMapping := range cmd.ForwardPorts { | ||
| mapping, err := port.ParsePortSpec(portMapping) | ||
| if err != nil { | ||
| return fmt.Errorf("parse port mapping: %w", err) | ||
| } | ||
| parsedMappings, err := parsePortForwards(config.mappings) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| errChan := make(chan error, len(parsedMappings)) | ||
| var waitGroup sync.WaitGroup | ||
| for _, parsedMapping := range parsedMappings { | ||
| portMapping, mapping := parsedMapping.spec, parsedMapping.mapping | ||
|
|
||
| // start the forwarding | ||
| log.Infof( | ||
| "Forwarding local %s/%s to remote %s/%s", | ||
| logger.Infof( | ||
| config.logTemplate, | ||
| mapping.Host.Protocol, | ||
| mapping.Host.Address, | ||
| mapping.Container.Protocol, | ||
| mapping.Container.Address, | ||
| ) | ||
| go func(portMapping string) { | ||
| err := devssh.PortForward( | ||
| waitGroup.Add(1) | ||
| go func(portMapping string, mapping port.Mapping) { | ||
| defer waitGroup.Done() | ||
|
|
||
| err := config.forwardFn( | ||
| ctx, | ||
| containerClient, | ||
| mapping.Host.Protocol, | ||
| mapping.Host.Address, | ||
| mapping.Container.Protocol, | ||
| mapping.Container.Address, | ||
| timeout, | ||
| log, | ||
| logger, | ||
| ) | ||
| if !errors.Is(io.EOF, err) { | ||
| if err != nil && !errors.Is(err, io.EOF) { | ||
| errChan <- fmt.Errorf("error forwarding %s: %w", portMapping, err) | ||
| } | ||
| }(portMapping) | ||
| }(portMapping, mapping) | ||
| } | ||
|
|
||
| go func() { | ||
| waitGroup.Wait() | ||
| close(errChan) | ||
| }() | ||
|
|
||
| for err := range errChan { | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| return <-errChan | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Cancel sibling forwarders before returning the first runtime error.
If one forwarding goroutine fails, Line 519 returns immediately while the other forwarders keep using the caller’s ctx and may keep listeners open. Use a child context, cancel it on the first real error, then drain the channel until the WaitGroup closes it.
Proposed cleanup on first error
func (cmd *SSHCmd) runPortForwards(
ctx context.Context,
containerClient *ssh.Client,
config portForwardConfig,
logger log.Logger,
) error {
timeout, err := cmd.forwardTimeout(logger)
if err != nil {
return fmt.Errorf("parse forward ports timeout: %w", err)
}
parsedMappings, err := parsePortForwards(config.mappings)
if err != nil {
return err
}
+
+ forwardCtx, cancel := context.WithCancel(ctx)
+ defer cancel()
errChan := make(chan error, len(parsedMappings))
var waitGroup sync.WaitGroup
for _, parsedMapping := range parsedMappings {
portMapping, mapping := parsedMapping.spec, parsedMapping.mapping
@@
defer waitGroup.Done()
err := config.forwardFn(
- ctx,
+ forwardCtx,
containerClient,
mapping.Host.Protocol,
mapping.Host.Address,
mapping.Container.Protocol,
@@
}(portMapping, mapping)
}
go func() {
waitGroup.Wait()
close(errChan)
}()
+ var firstErr error
for err := range errChan {
- if err != nil {
- return err
+ if err != nil && firstErr == nil {
+ firstErr = err
+ cancel()
}
}
- return nil
+ return firstErr
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (cmd *SSHCmd) runPortForwards( | |
| ctx context.Context, | |
| containerClient *ssh.Client, | |
| log log.Logger, | |
| config portForwardConfig, | |
| logger log.Logger, | |
| ) error { | |
| timeout, err := cmd.forwardTimeout(log) | |
| timeout, err := cmd.forwardTimeout(logger) | |
| if err != nil { | |
| return fmt.Errorf("parse forward ports timeout: %w", err) | |
| } | |
| errChan := make(chan error, len(cmd.ForwardPorts)) | |
| for _, portMapping := range cmd.ForwardPorts { | |
| mapping, err := port.ParsePortSpec(portMapping) | |
| if err != nil { | |
| return fmt.Errorf("parse port mapping: %w", err) | |
| } | |
| parsedMappings, err := parsePortForwards(config.mappings) | |
| if err != nil { | |
| return err | |
| } | |
| errChan := make(chan error, len(parsedMappings)) | |
| var waitGroup sync.WaitGroup | |
| for _, parsedMapping := range parsedMappings { | |
| portMapping, mapping := parsedMapping.spec, parsedMapping.mapping | |
| // start the forwarding | |
| log.Infof( | |
| "Forwarding local %s/%s to remote %s/%s", | |
| logger.Infof( | |
| config.logTemplate, | |
| mapping.Host.Protocol, | |
| mapping.Host.Address, | |
| mapping.Container.Protocol, | |
| mapping.Container.Address, | |
| ) | |
| go func(portMapping string) { | |
| err := devssh.PortForward( | |
| waitGroup.Add(1) | |
| go func(portMapping string, mapping port.Mapping) { | |
| defer waitGroup.Done() | |
| err := config.forwardFn( | |
| ctx, | |
| containerClient, | |
| mapping.Host.Protocol, | |
| mapping.Host.Address, | |
| mapping.Container.Protocol, | |
| mapping.Container.Address, | |
| timeout, | |
| log, | |
| logger, | |
| ) | |
| if !errors.Is(io.EOF, err) { | |
| if err != nil && !errors.Is(err, io.EOF) { | |
| errChan <- fmt.Errorf("error forwarding %s: %w", portMapping, err) | |
| } | |
| }(portMapping) | |
| }(portMapping, mapping) | |
| } | |
| go func() { | |
| waitGroup.Wait() | |
| close(errChan) | |
| }() | |
| for err := range errChan { | |
| if err != nil { | |
| return err | |
| } | |
| } | |
| return <-errChan | |
| return nil | |
| } | |
| func (cmd *SSHCmd) runPortForwards( | |
| ctx context.Context, | |
| containerClient *ssh.Client, | |
| config portForwardConfig, | |
| logger log.Logger, | |
| ) error { | |
| timeout, err := cmd.forwardTimeout(logger) | |
| if err != nil { | |
| return fmt.Errorf("parse forward ports timeout: %w", err) | |
| } | |
| parsedMappings, err := parsePortForwards(config.mappings) | |
| if err != nil { | |
| return err | |
| } | |
| forwardCtx, cancel := context.WithCancel(ctx) | |
| defer cancel() | |
| errChan := make(chan error, len(parsedMappings)) | |
| var waitGroup sync.WaitGroup | |
| for _, parsedMapping := range parsedMappings { | |
| portMapping, mapping := parsedMapping.spec, parsedMapping.mapping | |
| // start the forwarding | |
| logger.Infof( | |
| config.logTemplate, | |
| mapping.Host.Protocol, | |
| mapping.Host.Address, | |
| mapping.Container.Protocol, | |
| mapping.Container.Address, | |
| ) | |
| waitGroup.Add(1) | |
| go func(portMapping string, mapping port.Mapping) { | |
| defer waitGroup.Done() | |
| err := config.forwardFn( | |
| forwardCtx, | |
| containerClient, | |
| mapping.Host.Protocol, | |
| mapping.Host.Address, | |
| mapping.Container.Protocol, | |
| mapping.Container.Address, | |
| timeout, | |
| logger, | |
| ) | |
| if err != nil && !errors.Is(err, io.EOF) { | |
| errChan <- fmt.Errorf("error forwarding %s: %w", portMapping, err) | |
| } | |
| }(portMapping, mapping) | |
| } | |
| go func() { | |
| waitGroup.Wait() | |
| close(errChan) | |
| }() | |
| var firstErr error | |
| for err := range errChan { | |
| if err != nil && firstErr == nil { | |
| firstErr = err | |
| cancel() | |
| } | |
| } | |
| return firstErr | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/ssh.go` around lines 463 - 524, Wrap the incoming ctx in a cancellable
child (ctx, cancel := context.WithCancel(ctx)) at the top of runPortForwards and
pass that child ctx into config.forwardFn so all forwarder goroutines see
cancellation; in the per-goroutine error handling, when you detect a real error
(err != nil && !errors.Is(err, io.EOF)) send the formatted error to errChan and
call cancel() to cancel sibling forwarders; in the receiving loop, capture the
first error but continue draining errChan until it is closed by the waitGroup
goroutine, then return the first non-nil error (ensuring cancel is
deferred/called to release resources when leaving).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
pkg/portso--forward-portsand--reverse-forward-portsshare the same parserlocalhostor literal IPspkg/port, keep compose coverage for forwarding tonginx:8080, and document hostname examples fordevpod sshTesting
PATH=\"/home/david/.local/share/mise/installs/go/1.24.2/bin:$PATH\" /home/david/.local/share/mise/installs/go/1.24.2/bin/go test ./pkg/port ./cmdPATH=\"/home/david/.local/share/mise/installs/go/1.24.2/bin:$PATH\" /home/david/.local/share/mise/installs/go/1.24.2/bin/go test ./e2e/tests/up-docker-compose -run TestDoesNotExistCloses #741
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests