From c5bd8c41279a91711be7ce5a33cfcba4c98208bd Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Wed, 15 Sep 2021 15:37:37 -0400 Subject: [PATCH 01/12] initial spike to accept args --- cmd/ghcs/ssh.go | 6 +++--- internal/codespaces/ssh.go | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/cmd/ghcs/ssh.go b/cmd/ghcs/ssh.go index e5289e205..3bc2110a0 100644 --- a/cmd/ghcs/ssh.go +++ b/cmd/ghcs/ssh.go @@ -21,7 +21,7 @@ func newSSHCmd() *cobra.Command { Use: "ssh", Short: "SSH into a codespace", RunE: func(cmd *cobra.Command, args []string) error { - return ssh(context.Background(), sshProfile, codespaceName, sshServerPort) + return ssh(context.Background(), args, sshProfile, codespaceName, sshServerPort) }, } @@ -36,7 +36,7 @@ func init() { rootCmd.AddCommand(newSSHCmd()) } -func ssh(ctx context.Context, sshProfile, codespaceName string, localSSHServerPort int) error { +func ssh(ctx context.Context, sshArgs []string, sshProfile, codespaceName string, localSSHServerPort int) error { // Ensure all child tasks (e.g. port forwarding) terminate before return. ctx, cancel := context.WithCancel(ctx) defer cancel() @@ -88,7 +88,7 @@ func ssh(ctx context.Context, sshProfile, codespaceName string, localSSHServerPo shellClosed := make(chan error, 1) go func() { - shellClosed <- codespaces.Shell(ctx, log, localSSHServerPort, connectDestination, usingCustomPort) + shellClosed <- codespaces.Shell(ctx, log, sshArgs, localSSHServerPort, connectDestination, usingCustomPort) }() select { diff --git a/internal/codespaces/ssh.go b/internal/codespaces/ssh.go index 14dbfbb88..717371286 100644 --- a/internal/codespaces/ssh.go +++ b/internal/codespaces/ssh.go @@ -39,7 +39,7 @@ func StartSSHServer(ctx context.Context, session *liveshare.Session, log logger) // Shell runs an interactive secure shell over an existing // port-forwarding session. It runs until the shell is terminated // (including by cancellation of the context). -func Shell(ctx context.Context, log logger, port int, destination string, usingCustomPort bool) error { +func Shell(ctx context.Context, log logger, sshArgs []string, port int, destination string, usingCustomPort bool) error { cmd, connArgs := newSSHCommand(ctx, port, destination, "") if usingCustomPort { From 8a0f8b6d1c1834186ca4fcb6da650d34df89b1eb Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Thu, 16 Sep 2021 10:32:27 -0400 Subject: [PATCH 02/12] parse ssh args and command --- internal/codespaces/ssh.go | 63 ++++++++++++++++++++++++++------ internal/codespaces/ssh_test.go | 64 +++++++++++++++++++++++++++++++++ 2 files changed, 117 insertions(+), 10 deletions(-) create mode 100644 internal/codespaces/ssh_test.go diff --git a/internal/codespaces/ssh.go b/internal/codespaces/ssh.go index 661caecdc..0eee21286 100644 --- a/internal/codespaces/ssh.go +++ b/internal/codespaces/ssh.go @@ -12,7 +12,7 @@ import ( // port-forwarding session. It runs until the shell is terminated // (including by cancellation of the context). func Shell(ctx context.Context, log logger, sshArgs []string, port int, destination string, usingCustomPort bool) error { - cmd, connArgs := newSSHCommand(ctx, port, destination, "") + cmd, connArgs := newSSHCommand(ctx, port, destination, sshArgs) if usingCustomPort { log.Println("Connection Details: ssh " + destination + " " + strings.Join(connArgs, " ")) @@ -23,23 +23,21 @@ func Shell(ctx context.Context, log logger, sshArgs []string, port int, destinat // NewRemoteCommand returns an exec.Cmd that will securely run a shell // command on the remote machine. -func NewRemoteCommand(ctx context.Context, tunnelPort int, destination, command string) *exec.Cmd { - cmd, _ := newSSHCommand(ctx, tunnelPort, destination, command) +func NewRemoteCommand(ctx context.Context, tunnelPort int, destination string, sshArgs ...string) *exec.Cmd { + cmd, _ := newSSHCommand(ctx, tunnelPort, destination, sshArgs) return cmd } // newSSHCommand populates an exec.Cmd to run a command (or if blank, // an interactive shell) over ssh. -func newSSHCommand(ctx context.Context, port int, dst, command string) (*exec.Cmd, []string) { +func newSSHCommand(ctx context.Context, port int, dst string, cmdArgs []string) (*exec.Cmd, []string) { connArgs := []string{"-p", strconv.Itoa(port), "-o", "NoHostAuthenticationForLocalhost=yes"} - cmdArgs := []string{dst, "-C"} // Always use Compression - if command == "" { - // if we are in a shell send X11 and X11Trust - cmdArgs = append(cmdArgs, "-X", "-Y") - } - + cmdArgs, command := parseSSHArgs(cmdArgs) cmdArgs = append(cmdArgs, connArgs...) + cmdArgs = append(cmdArgs, "-C") // Compression + cmdArgs = append(cmdArgs, dst) // user@host + if command != "" { cmdArgs = append(cmdArgs, command) } @@ -51,3 +49,48 @@ func newSSHCommand(ctx context.Context, port int, dst, command string) (*exec.Cm return cmd, connArgs } + +var sshArgumentFlags = map[string]bool{ + "-b": true, + "-c": true, + "-D": true, + "-e": true, + "-F": true, + "-I": true, + "-i": true, + "-L": true, + "-l": true, + "-m": true, + "-O": true, + "-o": true, + "-p": true, + "-R": true, + "-S": true, + "-W": true, + "-w": true, +} + +func parseSSHArgs(sshArgs []string) ([]string, string) { + var ( + cmdArgs []string + command []string + flagArgument bool + ) + + for _, arg := range sshArgs { + switch { + case strings.HasPrefix(arg, "-"): + cmdArgs = append(cmdArgs, arg) + if _, ok := sshArgumentFlags[arg]; ok { + flagArgument = true + } + case flagArgument: + cmdArgs = append(cmdArgs, arg) + flagArgument = false + default: + command = append(command, arg) + } + } + + return cmdArgs, strings.Join(command, " ") +} diff --git a/internal/codespaces/ssh_test.go b/internal/codespaces/ssh_test.go new file mode 100644 index 000000000..cd92b39a6 --- /dev/null +++ b/internal/codespaces/ssh_test.go @@ -0,0 +1,64 @@ +package codespaces + +import "testing" + +func TestParseSSHArgs(t *testing.T) { + type testCase struct { + Args []string + ParsedArgs []string + Command string + } + + testCases := []testCase{ + { + Args: []string{"-X", "-Y"}, + ParsedArgs: []string{"-X", "-Y"}, + Command: "", + }, + { + Args: []string{"-X", "-Y", "-o", "someoption=test"}, + ParsedArgs: []string{"-X", "-Y", "-o", "someoption=test"}, + Command: "", + }, + { + Args: []string{"-X", "-Y", "-o", "someoption=test", "somecommand"}, + ParsedArgs: []string{"-X", "-Y", "-o", "someoption=test"}, + Command: "somecommand", + }, + { + Args: []string{"-X", "-Y", "-o", "someoption=test", "echo", "test"}, + ParsedArgs: []string{"-X", "-Y", "-o", "someoption=test"}, + Command: "echo test", + }, + { + Args: []string{"somecommand"}, + ParsedArgs: []string{}, + Command: "somecommand", + }, + { + Args: []string{"echo", "test"}, + ParsedArgs: []string{}, + Command: "echo test", + }, + { + Args: []string{"-v", "echo", "hello", "world"}, + ParsedArgs: []string{"-v"}, + Command: "echo hello world", + }, + } + + for _, tcase := range testCases { + args, command := parseSSHArgs(tcase.Args) + if len(args) != len(tcase.ParsedArgs) { + t.Fatalf("args do not match length of expected args. %#v, got '%d', expected: '%d'", tcase, len(args), len(tcase.ParsedArgs)) + } + for i, arg := range args { + if arg != tcase.ParsedArgs[i] { + t.Fatalf("arg does not match expected parsed arg. %v, got '%s', expected: '%s'", tcase, arg, tcase.ParsedArgs[i]) + } + } + if command != tcase.Command { + t.Fatalf("command does not match expected command. %v, got: '%s', expected: '%s'", tcase, command, tcase.Command) + } + } +} From 42e47a98d7b51d0ea70ee1bcc5a09392a49080fc Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Thu, 16 Sep 2021 15:22:47 -0400 Subject: [PATCH 03/12] add docs, simplify map, error on invalid args --- cmd/ghcs/logs.go | 5 ++- internal/codespaces/ssh.go | 58 ++++++++++++++++----------------- internal/codespaces/ssh_test.go | 13 +++++++- internal/codespaces/states.go | 6 +++- 4 files changed, 50 insertions(+), 32 deletions(-) diff --git a/cmd/ghcs/logs.go b/cmd/ghcs/logs.go index ccfb46236..725a243b0 100644 --- a/cmd/ghcs/logs.go +++ b/cmd/ghcs/logs.go @@ -82,9 +82,12 @@ func logs(ctx context.Context, log *output.Logger, codespaceName string, follow } dst := fmt.Sprintf("%s@localhost", sshUser) - cmd := codespaces.NewRemoteCommand( + cmd, err := codespaces.NewRemoteCommand( ctx, localPort, dst, fmt.Sprintf("%s /workspaces/.codespaces/.persistedshare/creation.log", cmdType), ) + if err != nil { + return fmt.Errorf("remote command: %w", err) + } tunnelClosed := make(chan error, 1) go func() { diff --git a/internal/codespaces/ssh.go b/internal/codespaces/ssh.go index 0eee21286..b58741e34 100644 --- a/internal/codespaces/ssh.go +++ b/internal/codespaces/ssh.go @@ -2,6 +2,7 @@ package codespaces import ( "context" + "fmt" "os" "os/exec" "strconv" @@ -12,7 +13,10 @@ import ( // port-forwarding session. It runs until the shell is terminated // (including by cancellation of the context). func Shell(ctx context.Context, log logger, sshArgs []string, port int, destination string, usingCustomPort bool) error { - cmd, connArgs := newSSHCommand(ctx, port, destination, sshArgs) + cmd, connArgs, err := newSSHCommand(ctx, port, destination, sshArgs) + if err != nil { + return fmt.Errorf("failed to create ssh command: %w", err) + } if usingCustomPort { log.Println("Connection Details: ssh " + destination + " " + strings.Join(connArgs, " ")) @@ -23,17 +27,27 @@ func Shell(ctx context.Context, log logger, sshArgs []string, port int, destinat // NewRemoteCommand returns an exec.Cmd that will securely run a shell // command on the remote machine. -func NewRemoteCommand(ctx context.Context, tunnelPort int, destination string, sshArgs ...string) *exec.Cmd { - cmd, _ := newSSHCommand(ctx, tunnelPort, destination, sshArgs) - return cmd +func NewRemoteCommand(ctx context.Context, tunnelPort int, destination string, sshArgs ...string) (*exec.Cmd, error) { + cmd, _, err := newSSHCommand(ctx, tunnelPort, destination, sshArgs) + return cmd, err } // newSSHCommand populates an exec.Cmd to run a command (or if blank, // an interactive shell) over ssh. -func newSSHCommand(ctx context.Context, port int, dst string, cmdArgs []string) (*exec.Cmd, []string) { +func newSSHCommand(ctx context.Context, port int, dst string, cmdArgs []string) (*exec.Cmd, []string, error) { connArgs := []string{"-p", strconv.Itoa(port), "-o", "NoHostAuthenticationForLocalhost=yes"} - cmdArgs, command := parseSSHArgs(cmdArgs) + // The ssh command syntax is: ssh [flags] user@host command [args...] + // There is no way to specify the user@host destination as a flag. + // Unfortunately, that means we need to know which user-provided words are + // SSH flags and which are command arguments so that we can place + // them before or after the destination, and that means we need to know all + // the flags and their arities. + cmdArgs, command, err := parseSSHArgs(cmdArgs) + if err != nil { + return nil, []string{}, err + } + cmdArgs = append(cmdArgs, connArgs...) cmdArgs = append(cmdArgs, "-C") // Compression cmdArgs = append(cmdArgs, dst) // user@host @@ -47,30 +61,12 @@ func newSSHCommand(ctx context.Context, port int, dst string, cmdArgs []string) cmd.Stdin = os.Stdin cmd.Stderr = os.Stderr - return cmd, connArgs + return cmd, connArgs, nil } -var sshArgumentFlags = map[string]bool{ - "-b": true, - "-c": true, - "-D": true, - "-e": true, - "-F": true, - "-I": true, - "-i": true, - "-L": true, - "-l": true, - "-m": true, - "-O": true, - "-o": true, - "-p": true, - "-R": true, - "-S": true, - "-W": true, - "-w": true, -} +var sshArgumentFlags = "-b-c-D-e-F-I-i-L-l-m-O-o-p-R-S-W-w" -func parseSSHArgs(sshArgs []string) ([]string, string) { +func parseSSHArgs(sshArgs []string) ([]string, string, error) { var ( cmdArgs []string command []string @@ -80,8 +76,12 @@ func parseSSHArgs(sshArgs []string) ([]string, string) { for _, arg := range sshArgs { switch { case strings.HasPrefix(arg, "-"): + if len(command) > 0 { + return []string{}, "", fmt.Errorf("invalid flag after command: %s", arg) + } + cmdArgs = append(cmdArgs, arg) - if _, ok := sshArgumentFlags[arg]; ok { + if strings.Contains(sshArgumentFlags, arg) { flagArgument = true } case flagArgument: @@ -92,5 +92,5 @@ func parseSSHArgs(sshArgs []string) ([]string, string) { } } - return cmdArgs, strings.Join(command, " ") + return cmdArgs, strings.Join(command, " "), nil } diff --git a/internal/codespaces/ssh_test.go b/internal/codespaces/ssh_test.go index cd92b39a6..2847ffc9f 100644 --- a/internal/codespaces/ssh_test.go +++ b/internal/codespaces/ssh_test.go @@ -48,7 +48,11 @@ func TestParseSSHArgs(t *testing.T) { } for _, tcase := range testCases { - args, command := parseSSHArgs(tcase.Args) + args, command, err := parseSSHArgs(tcase.Args) + if err != nil { + t.Errorf("received unexpected error: %w", err) + } + if len(args) != len(tcase.ParsedArgs) { t.Fatalf("args do not match length of expected args. %#v, got '%d', expected: '%d'", tcase, len(args), len(tcase.ParsedArgs)) } @@ -62,3 +66,10 @@ func TestParseSSHArgs(t *testing.T) { } } } + +func TestParseSSHArgsError(t *testing.T) { + _, _, err := parseSSHArgs([]string{"-X", "test", "-Y"}) + if err == nil { + t.Error("expected an error for invalid args") + } +} diff --git a/internal/codespaces/states.go b/internal/codespaces/states.go index 99683b51d..408f11941 100644 --- a/internal/codespaces/states.go +++ b/internal/codespaces/states.go @@ -89,10 +89,14 @@ func PollPostCreateStates(ctx context.Context, log logger, apiClient *api.API, u } func getPostCreateOutput(ctx context.Context, tunnelPort int, codespace *api.Codespace, user string) ([]PostCreateState, error) { - cmd := NewRemoteCommand( + cmd, err := NewRemoteCommand( ctx, tunnelPort, fmt.Sprintf("%s@localhost", user), "cat /workspaces/.codespaces/shared/postCreateOutput.json", ) + if err != nil { + return nil, fmt.Errorf("remote command: %w", err) + } + stdout := new(bytes.Buffer) cmd.Stdout = stdout if err := cmd.Run(); err != nil { From 60d066f0a69ad86e6a09053284bc4beef924dfa0 Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Fri, 17 Sep 2021 11:51:37 -0400 Subject: [PATCH 04/12] PR Feedback - return nil for slices - handle `-L -l` case - document `parseSSHArgs` --- internal/codespaces/ssh.go | 43 +++++++++++++++++---------------- internal/codespaces/ssh_test.go | 35 ++++++++++++++++++--------- 2 files changed, 45 insertions(+), 33 deletions(-) diff --git a/internal/codespaces/ssh.go b/internal/codespaces/ssh.go index b58741e34..33fbd092a 100644 --- a/internal/codespaces/ssh.go +++ b/internal/codespaces/ssh.go @@ -45,15 +45,15 @@ func newSSHCommand(ctx context.Context, port int, dst string, cmdArgs []string) // the flags and their arities. cmdArgs, command, err := parseSSHArgs(cmdArgs) if err != nil { - return nil, []string{}, err + return nil, nil, err } cmdArgs = append(cmdArgs, connArgs...) cmdArgs = append(cmdArgs, "-C") // Compression cmdArgs = append(cmdArgs, dst) // user@host - if command != "" { - cmdArgs = append(cmdArgs, command) + if command != nil { + cmdArgs = append(cmdArgs, command...) } cmd := exec.CommandContext(ctx, "ssh", cmdArgs...) @@ -64,33 +64,34 @@ func newSSHCommand(ctx context.Context, port int, dst string, cmdArgs []string) return cmd, connArgs, nil } -var sshArgumentFlags = "-b-c-D-e-F-I-i-L-l-m-O-o-p-R-S-W-w" - -func parseSSHArgs(sshArgs []string) ([]string, string, error) { +// parseSSHArgs parses SSH arguments into two distinct slices of flags +// and command. It returns an error if flags are found after a command +// or if a unary flag is provided without an argument. +func parseSSHArgs(args []string) ([]string, []string, error) { var ( - cmdArgs []string - command []string - flagArgument bool + cmdArgs []string + command []string ) - for _, arg := range sshArgs { - switch { - case strings.HasPrefix(arg, "-"): - if len(command) > 0 { - return []string{}, "", fmt.Errorf("invalid flag after command: %s", arg) + for i := 0; i < len(args); i++ { + arg := args[i] + if strings.HasPrefix(arg, "-") { + if command != nil { + return nil, nil, fmt.Errorf("invalid flag after command: %s", arg) } cmdArgs = append(cmdArgs, arg) - if strings.Contains(sshArgumentFlags, arg) { - flagArgument = true + if strings.Contains("bcDeFIiLlmOopRSWw", arg[1:2]) { + if i++; i == len(args) { + return nil, nil, fmt.Errorf("invalid unary flag without argument: %s", arg) + } + + cmdArgs = append(cmdArgs, args[i]) } - case flagArgument: - cmdArgs = append(cmdArgs, arg) - flagArgument = false - default: + } else { command = append(command, arg) } } - return cmdArgs, strings.Join(command, " "), nil + return cmdArgs, command, nil } diff --git a/internal/codespaces/ssh_test.go b/internal/codespaces/ssh_test.go index 2847ffc9f..04d52b090 100644 --- a/internal/codespaces/ssh_test.go +++ b/internal/codespaces/ssh_test.go @@ -6,44 +6,49 @@ func TestParseSSHArgs(t *testing.T) { type testCase struct { Args []string ParsedArgs []string - Command string + Command []string } testCases := []testCase{ { Args: []string{"-X", "-Y"}, ParsedArgs: []string{"-X", "-Y"}, - Command: "", + Command: nil, }, { Args: []string{"-X", "-Y", "-o", "someoption=test"}, ParsedArgs: []string{"-X", "-Y", "-o", "someoption=test"}, - Command: "", + Command: nil, }, { Args: []string{"-X", "-Y", "-o", "someoption=test", "somecommand"}, ParsedArgs: []string{"-X", "-Y", "-o", "someoption=test"}, - Command: "somecommand", + Command: []string{"somecommand"}, }, { Args: []string{"-X", "-Y", "-o", "someoption=test", "echo", "test"}, ParsedArgs: []string{"-X", "-Y", "-o", "someoption=test"}, - Command: "echo test", + Command: []string{"echo", "test"}, }, { Args: []string{"somecommand"}, ParsedArgs: []string{}, - Command: "somecommand", + Command: []string{"somecommand"}, }, { Args: []string{"echo", "test"}, ParsedArgs: []string{}, - Command: "echo test", + Command: []string{"echo", "test"}, }, { Args: []string{"-v", "echo", "hello", "world"}, ParsedArgs: []string{"-v"}, - Command: "echo hello world", + Command: []string{"echo", "hello", "world"}, + }, + { + Args: []string{"-L", "-l"}, + ParsedArgs: []string{"-L", "-l"}, + Command: nil, }, } @@ -54,15 +59,21 @@ func TestParseSSHArgs(t *testing.T) { } if len(args) != len(tcase.ParsedArgs) { - t.Fatalf("args do not match length of expected args. %#v, got '%d', expected: '%d'", tcase, len(args), len(tcase.ParsedArgs)) + t.Fatalf("args do not match length of expected args. %#v, got '%d'", tcase, len(args)) } + if len(command) != len(tcase.Command) { + t.Fatalf("command dooes not match length of expected command. %#v, got '%d'", tcase, len(command)) + } + for i, arg := range args { if arg != tcase.ParsedArgs[i] { - t.Fatalf("arg does not match expected parsed arg. %v, got '%s', expected: '%s'", tcase, arg, tcase.ParsedArgs[i]) + t.Fatalf("arg does not match expected parsed arg. %v, got '%s'", tcase, arg) } } - if command != tcase.Command { - t.Fatalf("command does not match expected command. %v, got: '%s', expected: '%s'", tcase, command, tcase.Command) + for i, c := range command { + if c != tcase.Command[i] { + t.Fatalf("command does not match expected command. %v, got: '%v'", tcase, command) + } } } } From 54265afda00db981d030cffa6c9564161e096ca9 Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Fri, 17 Sep 2021 13:43:23 -0400 Subject: [PATCH 05/12] PR Feedback - use named returns - handle command flags + test case - simplify tests --- internal/codespaces/ssh.go | 20 +++++-------- internal/codespaces/ssh_test.go | 52 ++++++++++++++++++--------------- 2 files changed, 37 insertions(+), 35 deletions(-) diff --git a/internal/codespaces/ssh.go b/internal/codespaces/ssh.go index 33fbd092a..e99f8971d 100644 --- a/internal/codespaces/ssh.go +++ b/internal/codespaces/ssh.go @@ -67,23 +67,19 @@ func newSSHCommand(ctx context.Context, port int, dst string, cmdArgs []string) // parseSSHArgs parses SSH arguments into two distinct slices of flags // and command. It returns an error if flags are found after a command // or if a unary flag is provided without an argument. -func parseSSHArgs(args []string) ([]string, []string, error) { - var ( - cmdArgs []string - command []string - ) - +func parseSSHArgs(args []string) (cmdArgs []string, command []string, err error) { for i := 0; i < len(args); i++ { arg := args[i] - if strings.HasPrefix(arg, "-") { - if command != nil { - return nil, nil, fmt.Errorf("invalid flag after command: %s", arg) - } + if command != nil { + command = append(command, arg) + continue + } + if strings.HasPrefix(arg, "-") { cmdArgs = append(cmdArgs, arg) - if strings.Contains("bcDeFIiLlmOopRSWw", arg[1:2]) { + if len(arg) == 2 && strings.Contains("bcDeFIiLlmOopRSWw", arg[1:2]) { if i++; i == len(args) { - return nil, nil, fmt.Errorf("invalid unary flag without argument: %s", arg) + return nil, nil, fmt.Errorf("ssh flag: %s requires an argument", arg) } cmdArgs = append(cmdArgs, args[i]) diff --git a/internal/codespaces/ssh_test.go b/internal/codespaces/ssh_test.go index 04d52b090..5450adf1a 100644 --- a/internal/codespaces/ssh_test.go +++ b/internal/codespaces/ssh_test.go @@ -1,12 +1,16 @@ package codespaces -import "testing" +import ( + "fmt" + "testing" +) func TestParseSSHArgs(t *testing.T) { type testCase struct { Args []string ParsedArgs []string Command []string + Error bool } testCases := []testCase{ @@ -50,37 +54,39 @@ func TestParseSSHArgs(t *testing.T) { ParsedArgs: []string{"-L", "-l"}, Command: nil, }, + { + Args: []string{"-v", "echo", "-n", "test"}, + ParsedArgs: []string{"-v"}, + Command: []string{"echo", "-n", "test"}, + }, + { + Args: []string{"-b"}, + ParsedArgs: nil, + Command: nil, + Error: true, + }, } for _, tcase := range testCases { args, command, err := parseSSHArgs(tcase.Args) - if err != nil { - t.Errorf("received unexpected error: %w", err) + if err != nil && !tcase.Error { + t.Errorf("unexpected error: %v on test case: %#v", err, tcase) + continue } - if len(args) != len(tcase.ParsedArgs) { - t.Fatalf("args do not match length of expected args. %#v, got '%d'", tcase, len(args)) - } - if len(command) != len(tcase.Command) { - t.Fatalf("command dooes not match length of expected command. %#v, got '%d'", tcase, len(command)) + if tcase.Error && err == nil { + t.Errorf("expected error and got nil: %#v", tcase) + continue } - for i, arg := range args { - if arg != tcase.ParsedArgs[i] { - t.Fatalf("arg does not match expected parsed arg. %v, got '%s'", tcase, arg) - } + argsStr, parsedArgsStr := fmt.Sprintf("%s", args), fmt.Sprintf("%s", tcase.ParsedArgs) + if argsStr != parsedArgsStr { + t.Errorf("args do not match parsed args. got: '%s', expected: '%s'", argsStr, parsedArgsStr) } - for i, c := range command { - if c != tcase.Command[i] { - t.Fatalf("command does not match expected command. %v, got: '%v'", tcase, command) - } + + commandStr, parsedCommandStr := fmt.Sprintf("%s", command), fmt.Sprintf("%s", tcase.Command) + if commandStr != parsedCommandStr { + t.Errorf("command does not match parsed command. got: '%s', expected: '%s'", commandStr, parsedCommandStr) } } } - -func TestParseSSHArgsError(t *testing.T) { - _, _, err := parseSSHArgs([]string{"-X", "test", "-Y"}) - if err == nil { - t.Error("expected an error for invalid args") - } -} From 76037ee75367125bdea702aa92885947cff3973c Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Fri, 17 Sep 2021 13:54:00 -0400 Subject: [PATCH 06/12] Update docs, simplify loop to append to command --- internal/codespaces/ssh.go | 16 +++++++--------- internal/codespaces/ssh_test.go | 2 +- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/internal/codespaces/ssh.go b/internal/codespaces/ssh.go index e99f8971d..4cd0a4c92 100644 --- a/internal/codespaces/ssh.go +++ b/internal/codespaces/ssh.go @@ -64,16 +64,11 @@ func newSSHCommand(ctx context.Context, port int, dst string, cmdArgs []string) return cmd, connArgs, nil } -// parseSSHArgs parses SSH arguments into two distinct slices of flags -// and command. It returns an error if flags are found after a command -// or if a unary flag is provided without an argument. +// parseSSHArgs parses SSH arguments into two distinct slices of flags and command. +// It returns an error if a unary flag is provided without an argument. func parseSSHArgs(args []string) (cmdArgs []string, command []string, err error) { for i := 0; i < len(args); i++ { arg := args[i] - if command != nil { - command = append(command, arg) - continue - } if strings.HasPrefix(arg, "-") { cmdArgs = append(cmdArgs, arg) @@ -84,9 +79,12 @@ func parseSSHArgs(args []string) (cmdArgs []string, command []string, err error) cmdArgs = append(cmdArgs, args[i]) } - } else { - command = append(command, arg) + continue } + + // if we've started parsing the command, append all further args to it + command = append(command, args[i:]...) + break } return cmdArgs, command, nil diff --git a/internal/codespaces/ssh_test.go b/internal/codespaces/ssh_test.go index 5450adf1a..ed6922762 100644 --- a/internal/codespaces/ssh_test.go +++ b/internal/codespaces/ssh_test.go @@ -69,7 +69,7 @@ func TestParseSSHArgs(t *testing.T) { for _, tcase := range testCases { args, command, err := parseSSHArgs(tcase.Args) - if err != nil && !tcase.Error { + if !tcase.Error && err != nil { t.Errorf("unexpected error: %v on test case: %#v", err, tcase) continue } From 65e1c6f789fb52415b74b6a3b5d33787732b2ab4 Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Fri, 17 Sep 2021 13:56:38 -0400 Subject: [PATCH 07/12] More test cases --- internal/codespaces/ssh_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/internal/codespaces/ssh_test.go b/internal/codespaces/ssh_test.go index ed6922762..c3e1b4c0a 100644 --- a/internal/codespaces/ssh_test.go +++ b/internal/codespaces/ssh_test.go @@ -14,6 +14,7 @@ func TestParseSSHArgs(t *testing.T) { } testCases := []testCase{ + {}, // empty test case { Args: []string{"-X", "-Y"}, ParsedArgs: []string{"-X", "-Y"}, @@ -59,6 +60,11 @@ func TestParseSSHArgs(t *testing.T) { ParsedArgs: []string{"-v"}, Command: []string{"echo", "-n", "test"}, }, + { + Args: []string{"-v", "echo", "-b", "test"}, + ParsedArgs: []string{"-v"}, + Command: []string{"echo", "-b", "test"}, + }, { Args: []string{"-b"}, ParsedArgs: nil, From 9f84015bd010818416442935717ae174c1072098 Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Fri, 17 Sep 2021 14:00:16 -0400 Subject: [PATCH 08/12] Avoid append --- internal/codespaces/ssh.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/codespaces/ssh.go b/internal/codespaces/ssh.go index 4cd0a4c92..6563db91a 100644 --- a/internal/codespaces/ssh.go +++ b/internal/codespaces/ssh.go @@ -82,8 +82,8 @@ func parseSSHArgs(args []string) (cmdArgs []string, command []string, err error) continue } - // if we've started parsing the command, append all further args to it - command = append(command, args[i:]...) + // if we've started parsing the command, set it to the rest of the args + command = args[i:] break } From da58313358f62a478794fa1337841dcc00aab065 Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Fri, 17 Sep 2021 14:03:31 -0400 Subject: [PATCH 09/12] Remove redudant type def --- internal/codespaces/ssh.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/codespaces/ssh.go b/internal/codespaces/ssh.go index 6563db91a..4c6ccb6d7 100644 --- a/internal/codespaces/ssh.go +++ b/internal/codespaces/ssh.go @@ -66,7 +66,7 @@ func newSSHCommand(ctx context.Context, port int, dst string, cmdArgs []string) // parseSSHArgs parses SSH arguments into two distinct slices of flags and command. // It returns an error if a unary flag is provided without an argument. -func parseSSHArgs(args []string) (cmdArgs []string, command []string, err error) { +func parseSSHArgs(args []string) (cmdArgs, command []string, err error) { for i := 0; i < len(args); i++ { arg := args[i] From 5890d6ad66ee56899ad497fe503d987adebfe744 Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Fri, 17 Sep 2021 15:04:55 -0400 Subject: [PATCH 10/12] Switch if block logic, assert err string --- internal/codespaces/ssh.go | 25 ++++++++++++------------- internal/codespaces/ssh_test.go | 19 +++++++++++++------ 2 files changed, 25 insertions(+), 19 deletions(-) diff --git a/internal/codespaces/ssh.go b/internal/codespaces/ssh.go index 4c6ccb6d7..36c8bf5b2 100644 --- a/internal/codespaces/ssh.go +++ b/internal/codespaces/ssh.go @@ -70,21 +70,20 @@ func parseSSHArgs(args []string) (cmdArgs, command []string, err error) { for i := 0; i < len(args); i++ { arg := args[i] - if strings.HasPrefix(arg, "-") { - cmdArgs = append(cmdArgs, arg) - if len(arg) == 2 && strings.Contains("bcDeFIiLlmOopRSWw", arg[1:2]) { - if i++; i == len(args) { - return nil, nil, fmt.Errorf("ssh flag: %s requires an argument", arg) - } - - cmdArgs = append(cmdArgs, args[i]) - } - continue + // if we've started parsing the command, set it to the rest of the args + if !strings.HasPrefix(arg, "-") { + command = args[i:] + break } - // if we've started parsing the command, set it to the rest of the args - command = args[i:] - break + cmdArgs = append(cmdArgs, arg) + if len(arg) == 2 && strings.Contains("bcDeFIiLlmOopRSWw", arg[1:2]) { + if i++; i == len(args) { + return nil, nil, fmt.Errorf("ssh flag: %s requires an argument", arg) + } + + cmdArgs = append(cmdArgs, args[i]) + } } return cmdArgs, command, nil diff --git a/internal/codespaces/ssh_test.go b/internal/codespaces/ssh_test.go index c3e1b4c0a..c804f6000 100644 --- a/internal/codespaces/ssh_test.go +++ b/internal/codespaces/ssh_test.go @@ -10,7 +10,7 @@ func TestParseSSHArgs(t *testing.T) { Args []string ParsedArgs []string Command []string - Error bool + Error string } testCases := []testCase{ @@ -69,19 +69,26 @@ func TestParseSSHArgs(t *testing.T) { Args: []string{"-b"}, ParsedArgs: nil, Command: nil, - Error: true, + Error: "ssh flag: -b requires an argument", }, } for _, tcase := range testCases { args, command, err := parseSSHArgs(tcase.Args) - if !tcase.Error && err != nil { - t.Errorf("unexpected error: %v on test case: %#v", err, tcase) + if tcase.Error != "" { + if err == nil { + t.Errorf("expected error and got nil: %#v", tcase) + } + + if err.Error() != tcase.Error { + t.Errorf("error does not match expected error, got: '%s', expected: '%s'", err.Error(), tcase.Error) + } + continue } - if tcase.Error && err == nil { - t.Errorf("expected error and got nil: %#v", tcase) + if err != nil { + t.Errorf("unexpected error: %v on test case: %#v", err, tcase) continue } From 47c6a5fce818b6681edec6a39d292f9387c0d008 Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Fri, 17 Sep 2021 15:13:09 -0400 Subject: [PATCH 11/12] Update usage --- cmd/ghcs/ssh.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/ghcs/ssh.go b/cmd/ghcs/ssh.go index cdfcdbcbe..390fc25c8 100644 --- a/cmd/ghcs/ssh.go +++ b/cmd/ghcs/ssh.go @@ -18,7 +18,7 @@ func newSSHCmd() *cobra.Command { var sshServerPort int sshCmd := &cobra.Command{ - Use: "ssh", + Use: "ssh [flags] -- [ssh-flags] [command]", Short: "SSH into a codespace", RunE: func(cmd *cobra.Command, args []string) error { return ssh(context.Background(), args, sshProfile, codespaceName, sshServerPort) From 82c19729d3ce9fcc0c604c811ddd609ed6190f5e Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Fri, 17 Sep 2021 15:17:38 -0400 Subject: [PATCH 12/12] Wrap -- with optional argument brackets --- cmd/ghcs/ssh.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/ghcs/ssh.go b/cmd/ghcs/ssh.go index 390fc25c8..e459f265a 100644 --- a/cmd/ghcs/ssh.go +++ b/cmd/ghcs/ssh.go @@ -18,7 +18,7 @@ func newSSHCmd() *cobra.Command { var sshServerPort int sshCmd := &cobra.Command{ - Use: "ssh [flags] -- [ssh-flags] [command]", + Use: "ssh [flags] [--] [ssh-flags] [command]", Short: "SSH into a codespace", RunE: func(cmd *cobra.Command, args []string) error { return ssh(context.Background(), args, sshProfile, codespaceName, sshServerPort)