-
Notifications
You must be signed in to change notification settings - Fork 426
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
POD-1277 | SSH sessions and proxy full support #1602
POD-1277 | SSH sessions and proxy full support #1602
Conversation
cmd/machine/ssh.go
Outdated
@@ -100,6 +101,76 @@ func (cmd *SSHCmd) Run(ctx context.Context, args []string) error { | |||
|
|||
type ExecFunc func(ctx context.Context, stdin io.Reader, stdout io.Writer, stderr io.Writer) error | |||
|
|||
func StartTailscaleSSHSession( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move this to pkg/tailscale
or even pkg/platform
? Afaik it's only used for pro anyway and machine
indicates it's used for everything
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On closer look, do we even need a separate function for this? 🤔 We don't seem to use the tsNet or I'm blind
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup correct this no longer seems to be needed. at some point during dev I was building ssh clients inside this func. Thx!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
merged terminal session handling with current machine.StartSSHSession
implementation so we don't duplicate code here anymore. This ssh session handling (machine.RunSSHSession
) can probably be now moved somewhere to pkg/ssh/
wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, makes sense
7bba28a
to
d19bdbe
Compare
1abe432
to
4648603
Compare
4648603
to
daad2dc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
6d0f6d3
into
loft-sh:feature/tailscale-network-rewrite
Fixes POD-1277
Example container state when running workspace