Skip to content

Strip the user prefix when forwarding the local registry port - #1957

Open
talha7k wants to merge 1 commit into
basecamp:mainfrom
talha7k:port-forwarding-user-host-and-existing-tunnel
Open

talha7k wants to merge 1 commit into
basecamp:mainfrom
talha7k:port-forwarding-user-host-and-existing-tunnel

Conversation

@talha7k

@talha7k talha7k commented Sep 30, 2026

Copy link
Copy Markdown

Servers configured in the canonical user@host form broke deploys that
use the local registry on 2.12.0. Net::SSH.start received the raw host
from the config, so deploy@203.0.113.10 went to getaddrinfo verbatim
and resolution failed with getaddrinfo: Name or service not known before
a single image was built. SSHKit splits the user out when it connects for
the deploy commands themselves; the port forwarding setup skipped that
step. The prefix is now stripped before connecting, and authentication
still uses ssh_options[:user].

A rejected tcpip-forward also aborted the deploy. That is exactly what
happens when the remote port is already bound — for example by an
operator-managed persistent ssh -R tunnel serving the local registry.
The rejection is now logged ("Port forward rejected on ... tunnel may
already exist") and the deploy continues; a connection failure during
setup passes the same way instead of raising. If the registry really is
unreachable, the deploy still fails later with the registry error.

Repro on 2.12.0: servers: [ "deploy@<ip>" ] with a remote builder and a
local registry, then kamal deploy (or kamal app pull) — it dies with
getaddrinfo during "Setting up local registry port forwarding". With the
patch the same config forwards to the bare host and deploys.

Tests added for the PortForwarding class: the user prefix is stripped
when connecting, and both a rejected forward and a failed connection are
logged without aborting the deploy.

Copilot AI balanced review requested due to automatic review settings September 30, 2026 01:19

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The host prefix’s username is discarded, causing canonical deploy@host configurations to authenticate as the configured default user instead.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
What changed in this PR

Updates local-registry SSH port forwarding to handle user@host server addresses and tolerate tunnel setup failures.

Changes:

  • Strips the user prefix before hostname resolution.
  • Logs rejected forwards and connection failures without aborting immediately.
  • Adds focused port-forwarding tests.

[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.

File Description
lib/​kamal/​cli/​build/​port_forwarding.rb Updates SSH connection and failure handling.
test/​cli/​build/​port_forwarding_test.rb Tests hostname parsing and recoverable failures.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread lib/kamal/cli/build/port_forwarding.rb Outdated
Thread.new do
begin
Net::SSH.start(host, ssh_options[:user], **ssh_options.except(:user)) do |ssh|
Net::SSH.start(host.to_s.split("@").last, ssh_options[:user], **ssh_options.except(:user)) do |ssh|
@talha7k
talha7k force-pushed the port-forwarding-user-host-and-existing-tunnel branch 2 times, most recently from 324ed4f to 2334211 Compare October 27, 2026 12:14
@talha7k
talha7k force-pushed the port-forwarding-user-host-and-existing-tunnel branch from 2334211 to d28a3f3 Compare November 7, 2026 03:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants