Skip to content
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

SSH: Support executing commands and interactive shell with port forwarding #48

Conversation

GGonryun
Copy link
Contributor

@GGonryun GGonryun commented Feb 29, 2024

This PR enables

  • interactive shells & port forwarding: p0 ssh <instance> -L 56789:80
  • executing commands & port forwarding p0 ssh <instance> sleep 10 -L 56789:80

In order to support this behavior execution of the port forwarding command is done in a child process. The child process uses streams to intercept and merge the child processes (and it's descendent's) output into the parent processes stdout.

image

Copy link

linear bot commented Feb 29, 2024

ENG-1653 p0 cli SSH: Support executing commands with port forwarding

The -L option for SSH normally allows users to specify a command to run while port forwarding, but The implementation in ENG-1594 currently does not support executing an interactive command with port forwarding.

Possible Complications

  1. The current SSM command for opening a port requires a different session type (Port) and uses a different AWS managed document (StartPortForwardingSession). It might be necessary to execute two different commands to get port forwarding working with an interactive command.

Definition of Done

  1. Investigate both the behavior of ssh <destination> <command> -L 80:80 and the behavior of the -nL option and try to bring our p0 ssh command inline with the implementatoin.
  2. Allow users to execute a command and port forward at the same time. This would follow the syntax: `p0 ssh [command [args..]] -L port:port`

@GGonryun GGonryun self-assigned this Feb 29, 2024
@GGonryun GGonryun requested review from nbrahms and gergas3 February 29, 2024 21:55
@GGonryun GGonryun marked this pull request as ready for review February 29, 2024 22:40
Copy link
Contributor

@nbrahms nbrahms left a comment

Choose a reason for hiding this comment

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

Can you double-check the functionality of:

  • Typing exit in the SSH session
  • Typing CTRL-d in the SSH session
  • Resizing the SSH window (especially in a vi or nano window)

src/plugins/aws/ssm/index.ts Outdated Show resolved Hide resolved
src/plugins/aws/ssm/index.ts Outdated Show resolved Hide resolved
...process.env,
...credentials,
},
stdio: ["inherit", "pipe", "pipe"],
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this need to be ["inherit", "inherit", "pipe"] in order for the pty to be preserved?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs to be different for the ssh session and the port-forward subprocesses.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this is ["inherit", "inherit", "pipe"] then I get a subprocess with no readable output stream, this means that we can't suppress the "Starting Session" message we get when SSM commands run which results in this output:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we don't mind getting multiple "starting session" messages, then we can switch to ["inherit", "inherit", "pipe"]. What do you think?

});

// Ensures that content from the child process is printed to the terminal and the proxy stream
childProcess.stdout.pipe(proxyStream).pipe(process.stdout);
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this is dropping stderr?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it should be dropping stderr. From my understanding i'm piping the original output into a proxy stream so I can process the messages and then passing it back to process.stdout, which should leave stderr unaffected.

src/plugins/aws/ssm/index.ts Outdated Show resolved Hide resolved
@GGonryun
Copy link
Contributor Author

GGonryun commented Mar 1, 2024

  • Resizing the SSH window (especially in a vi or nano window)

Everything works fine except nano doesn't display properly:

Currently on Main

image

This PR

image

@GGonryun
Copy link
Contributor Author

GGonryun commented Mar 1, 2024

Closing this PR, an alternative approach is being taken in #50 which does not used detached processes and also doesn't intercept the primary process' output stream

@GGonryun GGonryun closed this Mar 1, 2024
@nbrahms nbrahms deleted the miguelcampos/eng-1653-p0-cli-ssh-support-executing-commands-with-port-forwarding branch April 2, 2024 16:36
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