Add --reuse-sock flag so browser IDEs can reuse another SSH connections SSH_AUTH_SOCK #1471
Conversation
| @@ -430,6 +433,10 @@ func (cmd *SSHCmd) startTunnel(ctx context.Context, devPodConfig *config.Config, | |||
|
|
|||
| log.Debugf("Run outer container tunnel") | |||
| command := fmt.Sprintf("'%s' helper ssh-server --track-activity --stdio --workdir '%s'", agent.ContainerDevPodHelperLocation, workdir) | |||
| if cmd.ReuseSSHAuthSock != "" { | |||
| log.Info("Reusing SSH_AUTH_SOCK") | |||
There was a problem hiding this comment.
nit: This will always be printing when we connect, can you make this log level Debug please?
| @@ -484,7 +491,7 @@ func (cmd *SSHCmd) startServices( | |||
| log log.Logger, | |||
| ) { | |||
| if cmd.User != "" { | |||
| err := tunnel.RunInContainer( | |||
| err := tunnel.RunServices( | |||
There was a problem hiding this comment.
Could you revert this naming change please? Makes it harder to backmerge and it's not really required in this PR.
If you absolutely want to change it, please open a new one
There was a problem hiding this comment.
The reason I changed this function's name was because it runs a server locally, so the name is misleading IMO
| cmd.SSHAuthSockID = util.RandStringBytes(10) | ||
| log.Debug("Reusing SSH_AUTH_SOCK", cmd.SSHAuthSockID) | ||
| } else if cmd.Proxy && ide.ReusesAuthSock(targetIDE) { | ||
| log.Info("Reusing SSH_AUTH_SOCK is not supported with proxy mode, consider launching the IDE from the platform UI") |
There was a problem hiding this comment.
This can potentially be confusing if you don't know what SSH_AUTH_SOCK is and how it relates to the ssh agent. Could you make this a debug log please
| @@ -994,6 +1077,8 @@ func createSSHCommand( | |||
| } | |||
| args = append(args, extraArgs...) | |||
|
|
|||
| logger.Debug("Connecting with SSH command ", execPath, args) | |||
There was a problem hiding this comment.
Do we still need this?
There was a problem hiding this comment.
I thought it would be a useful debug log so we have the flags the SSH connection made
| @@ -174,7 +183,8 @@ func (c *ContainerHandler) updateConfig(ctx context.Context, sshClient *ssh.Clie | |||
| } | |||
| } | |||
|
|
|||
| func (c *ContainerHandler) runRunInContainer(ctx context.Context, sshClient *ssh.Client, runInContainer Handler, envVars map[string]string) error { | |||
| // runInContainer uses the connected SSH client to execute runInContainer on the remote | |||
| func (c *ContainerTunnel) runInContainer(ctx context.Context, sshClient *ssh.Client, handler Handler, envVars map[string]string) error { | |||
There was a problem hiding this comment.
Let's add the doc string but please revert the naming changes 😅
There was a problem hiding this comment.
again I feel like this function name is confusing, runRunInContainer reads poorly especially if we pass a function as a parameter named runInContainer. runInContainer is named handler in the call above and not sure why we rename it, IMO it makes the code harder to follow
This PR introduces the hidden flag
--reuse-sockto the SSH and helper ssh-server commands so that browser IDE's can first set up a SSH connection and allow the browser tunnel to reuse this.Fixes #1307, #1456 and #1464
Why do we need this? "normal" IDE's like vscode use an SSH connection to connect to the remote, this connection requersts agent forwarding so the user's SSH_AUTH_SOCK is available in the workspace.
For browser IDEs this SSH connection is not made, the only long lived connection we have is used for the credential server and runs as root, which it must.
Therefore this fix is to emulate the behaviour of the normal IDE and allow the vscode session to highjack the SSH_AUTH_SOCK of the unused "backhaul" connection.
This PR was tested using local docker provider and kubernetes with openvscode and vscode. I tried pro with vscode and it works but using openvscode does not because the devpod CLI on the platform does not have the build changes.
Additionally we perform the clean up of the tmp dir if used