From b7f2b3d3caab5aea45d76def79b8b84519b6d3f7 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 30 May 2022 17:39:13 +0300 Subject: [PATCH 01/38] feat: Refactor CLI config-ssh to improve UX - Magic block is replaced by Include statement - Writes are only done on changes - Inform user of changes via prompt - Allow displaying changes via `--diff` - Remove magic block if present TODO: - [ ] Parse previous `config-ssh` options and suggest re-using them - [ ] Auto-update `~/.ssh/coder` on `coder create` and `coder delete` - [ ] Tests for the new functionality Fixes #1326 --- cli/configssh.go | 362 +++++++++++++++++++++++++++++++++++------- cli/configssh_test.go | 7 +- 2 files changed, 310 insertions(+), 59 deletions(-) diff --git a/cli/configssh.go b/cli/configssh.go index 5ade88f70de6d..5c41da8d8e54e 100644 --- a/cli/configssh.go +++ b/cli/configssh.go @@ -1,15 +1,21 @@ package cli import ( + "bytes" + "errors" "fmt" + "io" + "io/fs" "os" + "os/exec" "path/filepath" "runtime" + "sort" "strings" - "sync" "github.com/cli/safeexec" "github.com/spf13/cobra" + "golang.org/x/exp/slices" "golang.org/x/sync/errgroup" "golang.org/x/xerrors" @@ -18,21 +24,33 @@ import ( "github.com/coder/coder/codersdk" ) -const sshStartToken = "# ------------START-CODER-----------" -const sshStartMessage = `# This was generated by "coder config-ssh". +const ( + sshDefaultConfigFileName = "~/.ssh/config" + // TODO(mafredri): Consider moving to coder config dir, i.e. ~/.config/coderv2/ssh_config? + sshDefaultCoderConfigFileName = "~/.ssh/coder" + sshCoderConfigHeader = `# This file is managed by coder. DO NOT EDIT. # -# To remove this blob, run: -# -# coder config-ssh --remove -# -# You should not hand-edit this section, unless you are deleting it.` -const sshEndToken = "# ------------END-CODER------------" +# You should not hand-edit this file, all changes will be lost upon workspace +# creation, deletion or when running "coder config-ssh". +` + sshCoderConfigOptionsHeader = `# +# Last config-ssh options: +` + // Relative paths are assumed to be in ~/.ssh, except when + // included in /etc/ssh. + sshConfigIncludeStatement = "Include coder" +) func configSSH() *cobra.Command { var ( sshConfigFile string + coderConfigFile string sshOptions []string + showDiff bool skipProxyCommand bool + + // Diff should exit with status 1 when files differ. + filesDiffer bool ) cmd := &cobra.Command{ Annotations: workspaceCommand, @@ -42,31 +60,80 @@ func configSSH() *cobra.Command { - You can use -o (or --ssh-option) so set SSH options to be used for all your workspaces. - ` + cliui.Styles.Code.Render("$ coder config-ssh -o ForwardAgent=yes"), + ` + cliui.Styles.Code.Render("$ coder config-ssh -o ForwardAgent=yes") + ` + + - You can use -D (or --diff) to display the changes that will be made. + + ` + cliui.Styles.Code.Render("$ coder config-ssh --diff"), + PostRun: func(cmd *cobra.Command, args []string) { + // TODO(mafredri): Should we refactor this.. e.g. sentinel error? + if showDiff && filesDiffer { + os.Exit(1) //nolint: revive + } + }, RunE: func(cmd *cobra.Command, args []string) error { client, err := createClient(cmd) if err != nil { return err } - if strings.HasPrefix(sshConfigFile, "~/") { - dirname, _ := os.UserHomeDir() - sshConfigFile = filepath.Join(dirname, sshConfigFile[2:]) - } - // Doesn't matter if this fails, because we write the file anyways. - sshConfigContentRaw, _ := os.ReadFile(sshConfigFile) - sshConfigContent := string(sshConfigContentRaw) - startIndex := strings.Index(sshConfigContent, sshStartToken) - endIndex := strings.Index(sshConfigContent, sshEndToken) - if startIndex != -1 && endIndex != -1 { - sshConfigContent = sshConfigContent[:startIndex-1] + sshConfigContent[endIndex+len(sshEndToken):] - } + // Early check for workspaces to ensure API key has not expired. workspaces, err := client.Workspaces(cmd.Context(), codersdk.WorkspaceFilter{ Owner: codersdk.Me, }) if err != nil { return err } + + dirname, err := os.UserHomeDir() + if err != nil { + return xerrors.Errorf("user home dir failed: %w", err) + } + + sshConfigFileOrig := sshConfigFile // Store the pre ~/ replacement name for serializing options. + if strings.HasPrefix(sshConfigFile, "~/") { + sshConfigFile = filepath.Join(dirname, sshConfigFile[2:]) + } + coderConfigFileOrig := coderConfigFile + coderConfigFile = filepath.Join(dirname, coderConfigFile[2:]) // Replace ~/ with home dir. + + // TODO(mafredri): Check coderConfigFile for previous options + // coderConfigFile. + + // Only allow not-exist errors to avoid trashing + // the users SSH config. + configRaw, err := os.ReadFile(sshConfigFile) + if err != nil && !errors.Is(err, fs.ErrNotExist) { + return xerrors.Errorf("read ssh config failed: %w", err) + } + + coderConfigRaw, err := os.ReadFile(coderConfigFile) + if err != nil && !errors.Is(err, fs.ErrNotExist) { + return xerrors.Errorf("read ssh config failed: %w", err) + } + + // Keep track of changes we are making. + var actions []string + + // Check for presence of old config format and + // remove if present. + configModified, ok := stripOldConfigBlock(configRaw) + if ok { + actions = append(actions, fmt.Sprintf("Remove old config block (START-CODER/END-CODER) from %s", sshConfigFileOrig)) + } + + if found := bytes.Index(configModified, []byte(sshConfigIncludeStatement)); found == -1 || (found > 0 && configModified[found-1] != '\n') { + actions = append(actions, fmt.Sprintf("Add 'Include coder' to %s", sshConfigFileOrig)) + // Separate Include statement from user content with an empty newline. + configModified = bytes.TrimRight(configModified, "\n") + sep := "\n\n" + if len(configModified) == 0 { + sep = "" + } + configModified = append(configModified, []byte(sep+sshConfigIncludeStatement+"\n")...) + } + + // TODO(mafredri): Where do we display this now...? if len(workspaces) == 0 { return xerrors.New("You don't have any workspaces!") } @@ -77,50 +144,36 @@ func configSSH() *cobra.Command { } root := createConfig(cmd) - sshConfigContent += "\n" + sshStartToken + "\n" + sshStartMessage + "\n\n" - sshConfigContentMutex := sync.Mutex{} var errGroup errgroup.Group - for _, workspace := range workspaces { + type workspaceConfig struct { + Name string + Hosts []string + } + workspaceConfigs := make([]workspaceConfig, len(workspaces)) + for i, workspace := range workspaces { + i := i workspace := workspace errGroup.Go(func() error { resources, err := client.TemplateVersionResources(cmd.Context(), workspace.LatestBuild.TemplateVersionID) if err != nil { return err } + + wc := workspaceConfig{Name: workspace.Name} for _, resource := range resources { if resource.Transition != codersdk.WorkspaceTransitionStart { continue } for _, agent := range resource.Agents { - sshConfigContentMutex.Lock() hostname := workspace.Name if len(resource.Agents) > 1 { hostname += "." + agent.Name } - configOptions := []string{ - "Host coder." + hostname, - } - for _, option := range sshOptions { - configOptions = append(configOptions, "\t"+option) - } - configOptions = append(configOptions, - "\tHostName coder."+hostname, - "\tConnectTimeout=0", - "\tStrictHostKeyChecking=no", - // Without this, the "REMOTE HOST IDENTITY CHANGED" - // message will appear. - "\tUserKnownHostsFile=/dev/null", - // This disables the "Warning: Permanently added 'hostname' (RSA) to the list of known hosts." - // message from appearing on every SSH. This happens because we ignore the known hosts. - "\tLogLevel ERROR", - ) - if !skipProxyCommand { - configOptions = append(configOptions, fmt.Sprintf("\tProxyCommand %q --global-config %q ssh --stdio %s", binaryFile, root, hostname)) - } - sshConfigContent += strings.Join(configOptions, "\n") + "\n" - sshConfigContentMutex.Unlock() + wc.Hosts = append(wc.Hosts, hostname) } } + workspaceConfigs[i] = wc + return nil }) } @@ -128,29 +181,165 @@ func configSSH() *cobra.Command { if err != nil { return err } - sshConfigContent += "\n" + sshEndToken - err = os.MkdirAll(filepath.Dir(sshConfigFile), os.ModePerm) - if err != nil { - return err + + buf := &bytes.Buffer{} + _, _ = buf.WriteString(sshCoderConfigHeader) + + // Store the provided flags as part of the + // config for future (re)use. + _, _ = buf.WriteString(sshCoderConfigOptionsHeader) + if sshConfigFileOrig != sshDefaultConfigFileName { + _, _ = fmt.Fprintf(buf, "# :%s=%s\n", "ssh-config-file", sshConfigFileOrig) } - err = os.WriteFile(sshConfigFile, []byte(sshConfigContent), os.ModePerm) - if err != nil { - return err + for _, opt := range sshOptions { + _, _ = fmt.Fprintf(buf, "# :%s=%s\n", "ssh-option", opt) + } + _, _ = buf.WriteString("#\n") + + // Ensure stable sorting of output. + slices.SortFunc(workspaceConfigs, func(a, b workspaceConfig) bool { + return a.Name < b.Name + }) + for _, wc := range workspaceConfigs { + sort.Strings(wc.Hosts) + // Write agent configuration. + for _, hostname := range wc.Hosts { + configOptions := []string{ + "Host coder." + hostname, + } + for _, option := range sshOptions { + configOptions = append(configOptions, "\t"+option) + } + configOptions = append(configOptions, + "\tHostName coder."+hostname, + "\tConnectTimeout=0", + "\tStrictHostKeyChecking=no", + // Without this, the "REMOTE HOST IDENTITY CHANGED" + // message will appear. + "\tUserKnownHostsFile=/dev/null", + // This disables the "Warning: Permanently added 'hostname' (RSA) to the list of known hosts." + // message from appearing on every SSH. This happens because we ignore the known hosts. + "\tLogLevel ERROR", + ) + if !skipProxyCommand { + configOptions = append(configOptions, fmt.Sprintf("\tProxyCommand %q --global-config %q ssh --stdio %s", binaryFile, root, hostname)) + } + + _, _ = buf.WriteString(strings.Join(configOptions, "\n")) + _ = buf.WriteByte('\n') + } + } + + modifyCoderConfig := !bytes.Equal(coderConfigRaw, buf.Bytes()) + if modifyCoderConfig { + if len(coderConfigRaw) == 0 { + actions = append(actions, fmt.Sprintf("Write auto-generated coder config file to %s", coderConfigFileOrig)) + } else { + actions = append(actions, fmt.Sprintf("Update auto-generated coder config file in %s", coderConfigFileOrig)) + } + } + + if showDiff { + if len(actions) > 0 { + // Write to stderr to avoid dirtying the diff output. + _, _ = fmt.Fprint(cmd.ErrOrStderr(), "Changes:\n\n") + for _, action := range actions { + _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "* %s\n", action) + } + } + + for _, diffFn := range []func() ([]byte, error){ + func() ([]byte, error) { return diffBytes(sshConfigFileOrig, configRaw, configModified) }, + func() ([]byte, error) { return diffBytes(coderConfigFileOrig, coderConfigRaw, buf.Bytes()) }, + } { + diff, err := diffFn() + if err != nil { + return xerrors.Errorf("diff failed: %w", err) + } + // TODO(mafredri): Colorize with github.com/sourcegraph/go-diff? + if len(diff) > 0 { + filesDiffer = true + _, _ = fmt.Fprintf(cmd.OutOrStdout(), "\n%s", diff) + } + } + + return nil + } + + if len(actions) > 0 { + _, err = cliui.Prompt(cmd, cliui.PromptOptions{ + Text: fmt.Sprintf("The following changes will be made to your SSH configuration (use --diff to see changes):\n\n * %s\n\n Continue?", strings.Join(actions, "\n * ")), + IsConfirm: true, + }) + if err != nil { + return err + } + _, _ = fmt.Fprint(cmd.OutOrStdout(), "\n") + + if !bytes.Equal(configRaw, configModified) { + err = writeWithTempFileAndMove(sshConfigFile, bytes.NewReader(configRaw)) + if err != nil { + return xerrors.Errorf("write ssh config failed: %w", err) + } + } + if modifyCoderConfig { + err := writeWithTempFileAndMove(coderConfigFile, buf) + if err != nil { + return xerrors.Errorf("write coder ssh config failed: %w", err) + } + } } - _, _ = fmt.Fprintf(cmd.OutOrStdout(), "An auto-generated ssh config was written to %q\n", sshConfigFile) + _, _ = fmt.Fprintln(cmd.OutOrStdout(), "You should now be able to ssh into your workspace") - _, _ = fmt.Fprintf(cmd.OutOrStdout(), "For example, try running\n\n\t$ ssh coder.%s\n\n", workspaces[0].Name) + _, _ = fmt.Fprintf(cmd.OutOrStdout(), "For example, try running:\n\n\t$ ssh coder.%s\n\n", workspaces[0].Name) return nil }, } - cliflag.StringVarP(cmd.Flags(), &sshConfigFile, "ssh-config-file", "", "CODER_SSH_CONFIG_FILE", "~/.ssh/config", "Specifies the path to an SSH config.") + cliflag.StringVarP(cmd.Flags(), &sshConfigFile, "ssh-config-file", "", "CODER_SSH_CONFIG_FILE", sshDefaultConfigFileName, "Specifies the path to an SSH config.") + cmd.Flags().StringVar(&coderConfigFile, "ssh-coder-config-file", sshDefaultCoderConfigFileName, "Specifies the path to an Coder SSH config file. Useful for testing.") + _ = cmd.Flags().MarkHidden("ssh-coder-config-file") cmd.Flags().StringArrayVarP(&sshOptions, "ssh-option", "o", []string{}, "Specifies additional SSH options to embed in each host stanza.") + cmd.Flags().BoolVarP(&showDiff, "diff", "D", false, "Show diff of changes that will be made.") cmd.Flags().BoolVarP(&skipProxyCommand, "skip-proxy-command", "", false, "Specifies whether the ProxyCommand option should be skipped. Useful for testing.") _ = cmd.Flags().MarkHidden("skip-proxy-command") return cmd } +// writeWithTempFileAndMove writes to a temporary file in the same +// directory as path and renames the temp file to the file provided in +// path. This ensure we avoid trashing the file we are writing due to +// unforeseen circumstance like filesystem full, command killed, etc. +func writeWithTempFileAndMove(path string, r io.Reader) error { + dir := filepath.Dir(path) + name := filepath.Base(path) + + // Create a tempfile in the same directory for ensuring write + // operation does not fail. + f, err := os.CreateTemp(dir, fmt.Sprintf(".%s.", name)) + if err != nil { + return xerrors.Errorf("create temp file failed: %w", err) + } + + _, err = io.Copy(f, r) + if err != nil { + _ = f.Close() + return err + } + + err = f.Close() + if err != nil { + return xerrors.Errorf("close temp file failed: %w", err) + } + + err = os.Rename(f.Name(), path) + if err != nil { + return xerrors.Errorf("rename temp file failed: %w", err) + } + + return nil +} + // currentBinPath returns the path to the coder binary suitable for use in ssh // ProxyCommand. func currentBinPath(cmd *cobra.Command) (string, error) { @@ -191,3 +380,60 @@ func currentBinPath(cmd *cobra.Command) (string, error) { return binName, nil } + +// diffBytes two byte slices as if they were in a file named name. +// Does best-effort cleanup ignoring non-critical errors. +func diffBytes(name string, b1, b2 []byte) (data []byte, err error) { + f1, err := os.CreateTemp("", "coder_config-ssh.") + if err != nil { + return nil, xerrors.Errorf("create temp 1 file failed: %w", err) + } + defer os.Remove(f1.Name()) + defer f1.Close() + + f2, err := os.CreateTemp("", "coder_config-ssh.") + if err != nil { + return nil, xerrors.Errorf("create temp 2 file failed: %w", err) + } + defer os.Remove(f2.Name()) + defer f2.Close() + + _, err = f1.Write(b1) + if err != nil { + return nil, xerrors.Errorf("write temp 1 file failed: %w", err) + } + _, err = f2.Write(b2) + if err != nil { + return nil, xerrors.Errorf("write temp 2 file failed: %w", err) + } + + // TODO(mafredri): Ensure diff binary exists, or return useful error when missing. + data, err = exec.Command("diff", "-u", f1.Name(), f2.Name()).Output() // #nosec + if len(data) == 0 && err != nil { // Ignore non-zero exit when files differ. + return nil, err + } + // Replace temp file names with friendly names. + data = bytes.Replace(data, []byte(f1.Name()), []byte(name), 1) + data = bytes.Replace(data, []byte(f2.Name()), []byte(name+".new"), 1) + + return data, err +} + +// stripOldConfigBlock is here to migrate users from old config block +// format to new include statement. +func stripOldConfigBlock(data []byte) ([]byte, bool) { + const ( + sshStartToken = "# ------------START-CODER-----------" + sshEndToken = "# ------------END-CODER------------" + ) + + startIndex := bytes.Index(data, []byte(sshStartToken)) + endIndex := bytes.Index(data, []byte(sshEndToken)) + if startIndex != -1 && endIndex != -1 { + newdata := append([]byte{}, data[:startIndex-1]...) + newdata = append(newdata, data[endIndex+len(sshEndToken):]...) + return newdata, true + } + + return data, false +} diff --git a/cli/configssh_test.go b/cli/configssh_test.go index e7aeb82fe11b9..86b892b123fc3 100644 --- a/cli/configssh_test.go +++ b/cli/configssh_test.go @@ -77,9 +77,13 @@ func TestConfigSSH(t *testing.T) { t.Cleanup(func() { _ = agentCloser.Close() }) - tempFile, err := os.CreateTemp(t.TempDir(), "") + tmpdir := t.TempDir() + tempFile, err := os.CreateTemp(tmpdir, "config") require.NoError(t, err) _ = tempFile.Close() + coderTempFile, err := os.CreateTemp(tmpdir, "coder") + require.NoError(t, err) + _ = coderTempFile.Close() resources := coderdtest.AwaitWorkspaceAgents(t, client, workspace.LatestBuild.ID) agentConn, err := client.DialWorkspaceAgent(context.Background(), resources[0].Agents[0].ID, nil) require.NoError(t, err) @@ -112,6 +116,7 @@ func TestConfigSSH(t *testing.T) { "--ssh-option", "HostName "+tcpAddr.IP.String(), "--ssh-option", "Port "+strconv.Itoa(tcpAddr.Port), "--ssh-config-file", tempFile.Name(), + "--ssh-coder-config-file", coderTempFile.Name(), "--skip-proxy-command") clitest.SetupConfig(t, client, root) doneChan := make(chan struct{}) From 4f03415e073e709697794e29bcdff27d91da1212 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 30 May 2022 18:23:30 +0300 Subject: [PATCH 02/38] chore: Switch to github.com/pkg/diff for diffing --- cli/configssh.go | 50 ++++++++++++++++++------------------------------ go.mod | 1 + go.sum | 1 + 3 files changed, 21 insertions(+), 31 deletions(-) diff --git a/cli/configssh.go b/cli/configssh.go index 5c41da8d8e54e..2515a95fe5ee2 100644 --- a/cli/configssh.go +++ b/cli/configssh.go @@ -7,13 +7,14 @@ import ( "io" "io/fs" "os" - "os/exec" "path/filepath" "runtime" "sort" "strings" "github.com/cli/safeexec" + "github.com/pkg/diff" + "github.com/pkg/diff/write" "github.com/spf13/cobra" "golang.org/x/exp/slices" "golang.org/x/sync/errgroup" @@ -383,40 +384,27 @@ func currentBinPath(cmd *cobra.Command) (string, error) { // diffBytes two byte slices as if they were in a file named name. // Does best-effort cleanup ignoring non-critical errors. -func diffBytes(name string, b1, b2 []byte) (data []byte, err error) { - f1, err := os.CreateTemp("", "coder_config-ssh.") - if err != nil { - return nil, xerrors.Errorf("create temp 1 file failed: %w", err) - } - defer os.Remove(f1.Name()) - defer f1.Close() - - f2, err := os.CreateTemp("", "coder_config-ssh.") - if err != nil { - return nil, xerrors.Errorf("create temp 2 file failed: %w", err) +func diffBytes(name string, b1, b2 []byte) ([]byte, error) { + var buf bytes.Buffer + var opts []write.Option + // TODO(mafredri): Toggle color on/off + if false { + opts = append(opts, write.TerminalColor()) } - defer os.Remove(f2.Name()) - defer f2.Close() - - _, err = f1.Write(b1) + err := diff.Text(name, name+".new", b1, b2, &buf, opts...) if err != nil { - return nil, xerrors.Errorf("write temp 1 file failed: %w", err) - } - _, err = f2.Write(b2) - if err != nil { - return nil, xerrors.Errorf("write temp 2 file failed: %w", err) - } - - // TODO(mafredri): Ensure diff binary exists, or return useful error when missing. - data, err = exec.Command("diff", "-u", f1.Name(), f2.Name()).Output() // #nosec - if len(data) == 0 && err != nil { // Ignore non-zero exit when files differ. return nil, err } - // Replace temp file names with friendly names. - data = bytes.Replace(data, []byte(f1.Name()), []byte(name), 1) - data = bytes.Replace(data, []byte(f2.Name()), []byte(name+".new"), 1) - - return data, err + b := buf.Bytes() + // Check if diff only output two lines, if yes, there's no diff. + // + // Example: + // --- ~/.ssh/config + // +++ ~/.ssh/config.new + if bytes.Count(b, []byte{'\n'}) == 2 { + b = nil + } + return b, nil } // stripOldConfigBlock is here to migrate users from old config block diff --git a/go.mod b/go.mod index eeb7afb5ac25c..93f821a3ca5e2 100644 --- a/go.mod +++ b/go.mod @@ -93,6 +93,7 @@ require ( github.com/pion/udp v0.1.1 github.com/pion/webrtc/v3 v3.1.41 github.com/pkg/browser v0.0.0-20210911075715-681adbf594b8 + github.com/pkg/diff v0.0.0-20210226163009-20ebb0f2a09e github.com/pkg/sftp v1.13.4 github.com/prometheus/client_golang v1.12.2 github.com/quasilyte/go-ruleguard/dsl v0.3.21 diff --git a/go.sum b/go.sum index 1e84b1a936a8c..3ba4ccc9472f3 100644 --- a/go.sum +++ b/go.sum @@ -1336,6 +1336,7 @@ github.com/pires/go-proxyproto v0.6.2/go.mod h1:Odh9VFOZJCf9G8cLW5o435Xf1J95Jw9G github.com/pkg/browser v0.0.0-20210706143420-7d21f8c997e2/go.mod h1:HKlIX3XHQyzLZPlr7++PzdhaXEj94dEiJgZDTsxEqUI= github.com/pkg/browser v0.0.0-20210911075715-681adbf594b8 h1:KoWmjvw+nsYOo29YJK9vDA65RGE3NrOnUtO7a+RF9HU= github.com/pkg/browser v0.0.0-20210911075715-681adbf594b8/go.mod h1:HKlIX3XHQyzLZPlr7++PzdhaXEj94dEiJgZDTsxEqUI= +github.com/pkg/diff v0.0.0-20210226163009-20ebb0f2a09e h1:aoZm08cpOy4WuID//EZDgcC4zIxODThtZNPirFr42+A= github.com/pkg/diff v0.0.0-20210226163009-20ebb0f2a09e/go.mod h1:pJLUxLENpZxwdsKMEsNbx1VGcRFpLqf3715MtcvvzbA= github.com/pkg/errors v0.8.0/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= github.com/pkg/errors v0.8.1-0.20171018195549-f15c970de5b7/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= From e196920474f92d62955d1e921e16c6b888cb405f Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 30 May 2022 18:26:22 +0300 Subject: [PATCH 03/38] fix: Output absolute paths for diffs --- cli/configssh.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cli/configssh.go b/cli/configssh.go index 2515a95fe5ee2..f83ee4de26df2 100644 --- a/cli/configssh.go +++ b/cli/configssh.go @@ -250,8 +250,8 @@ func configSSH() *cobra.Command { } for _, diffFn := range []func() ([]byte, error){ - func() ([]byte, error) { return diffBytes(sshConfigFileOrig, configRaw, configModified) }, - func() ([]byte, error) { return diffBytes(coderConfigFileOrig, coderConfigRaw, buf.Bytes()) }, + func() ([]byte, error) { return diffBytes(sshConfigFile, configRaw, configModified) }, + func() ([]byte, error) { return diffBytes(coderConfigFile, coderConfigRaw, buf.Bytes()) }, } { diff, err := diffFn() if err != nil { @@ -399,8 +399,8 @@ func diffBytes(name string, b1, b2 []byte) ([]byte, error) { // Check if diff only output two lines, if yes, there's no diff. // // Example: - // --- ~/.ssh/config - // +++ ~/.ssh/config.new + // --- /home/user/.ssh/config + // +++ /home/user/.ssh/config.new if bytes.Count(b, []byte{'\n'}) == 2 { b = nil } From cdcf6c35cdc326132ff8af7bfa0e1f99d3192db6 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 30 May 2022 18:26:45 +0300 Subject: [PATCH 04/38] chore: Remove todone --- cli/configssh.go | 1 - 1 file changed, 1 deletion(-) diff --git a/cli/configssh.go b/cli/configssh.go index f83ee4de26df2..2adf32b5840b6 100644 --- a/cli/configssh.go +++ b/cli/configssh.go @@ -257,7 +257,6 @@ func configSSH() *cobra.Command { if err != nil { return xerrors.Errorf("diff failed: %w", err) } - // TODO(mafredri): Colorize with github.com/sourcegraph/go-diff? if len(diff) > 0 { filesDiffer = true _, _ = fmt.Fprintf(cmd.OutOrStdout(), "\n%s", diff) From 263d8236ab3ed5eee66b85dc826c744c3275d244 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 30 May 2022 18:33:52 +0300 Subject: [PATCH 05/38] chore: Rename actions => changes for consistency --- cli/configssh.go | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/cli/configssh.go b/cli/configssh.go index 2adf32b5840b6..093553b90957b 100644 --- a/cli/configssh.go +++ b/cli/configssh.go @@ -114,17 +114,17 @@ func configSSH() *cobra.Command { } // Keep track of changes we are making. - var actions []string + var changes []string // Check for presence of old config format and // remove if present. configModified, ok := stripOldConfigBlock(configRaw) if ok { - actions = append(actions, fmt.Sprintf("Remove old config block (START-CODER/END-CODER) from %s", sshConfigFileOrig)) + changes = append(changes, fmt.Sprintf("Remove old config block (START-CODER/END-CODER) from %s", sshConfigFileOrig)) } if found := bytes.Index(configModified, []byte(sshConfigIncludeStatement)); found == -1 || (found > 0 && configModified[found-1] != '\n') { - actions = append(actions, fmt.Sprintf("Add 'Include coder' to %s", sshConfigFileOrig)) + changes = append(changes, fmt.Sprintf("Add 'Include coder' to %s", sshConfigFileOrig)) // Separate Include statement from user content with an empty newline. configModified = bytes.TrimRight(configModified, "\n") sep := "\n\n" @@ -234,19 +234,20 @@ func configSSH() *cobra.Command { modifyCoderConfig := !bytes.Equal(coderConfigRaw, buf.Bytes()) if modifyCoderConfig { if len(coderConfigRaw) == 0 { - actions = append(actions, fmt.Sprintf("Write auto-generated coder config file to %s", coderConfigFileOrig)) + changes = append(changes, fmt.Sprintf("Write auto-generated coder config file to %s", coderConfigFileOrig)) } else { - actions = append(actions, fmt.Sprintf("Update auto-generated coder config file in %s", coderConfigFileOrig)) + changes = append(changes, fmt.Sprintf("Update auto-generated coder config file in %s", coderConfigFileOrig)) } } if showDiff { - if len(actions) > 0 { + if len(changes) > 0 { // Write to stderr to avoid dirtying the diff output. _, _ = fmt.Fprint(cmd.ErrOrStderr(), "Changes:\n\n") - for _, action := range actions { - _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "* %s\n", action) + for _, change := range changes { + _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "* %s\n", change) } + } else { } for _, diffFn := range []func() ([]byte, error){ @@ -266,9 +267,9 @@ func configSSH() *cobra.Command { return nil } - if len(actions) > 0 { + if len(changes) > 0 { _, err = cliui.Prompt(cmd, cliui.PromptOptions{ - Text: fmt.Sprintf("The following changes will be made to your SSH configuration (use --diff to see changes):\n\n * %s\n\n Continue?", strings.Join(actions, "\n * ")), + Text: fmt.Sprintf("The following changes will be made to your SSH configuration (use --diff to see changes):\n\n * %s\n\n Continue?", strings.Join(changes, "\n * ")), IsConfirm: true, }) if err != nil { From 18797e71fdf931524413a1fd5e27b2b9233ab1f9 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 30 May 2022 18:36:02 +0300 Subject: [PATCH 06/38] fix: Remove workspaces check, removing is a legit use-case --- cli/configssh.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/cli/configssh.go b/cli/configssh.go index 093553b90957b..5f8b89f292617 100644 --- a/cli/configssh.go +++ b/cli/configssh.go @@ -134,11 +134,6 @@ func configSSH() *cobra.Command { configModified = append(configModified, []byte(sep+sshConfigIncludeStatement+"\n")...) } - // TODO(mafredri): Where do we display this now...? - if len(workspaces) == 0 { - return xerrors.New("You don't have any workspaces!") - } - binaryFile, err := currentBinPath(cmd) if err != nil { return err From 9680792aca1a2c20f1362e240f18d384afc4b63b Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 30 May 2022 18:37:09 +0300 Subject: [PATCH 07/38] fix: Remove unused else --- cli/configssh.go | 1 - 1 file changed, 1 deletion(-) diff --git a/cli/configssh.go b/cli/configssh.go index 5f8b89f292617..ef9b089a522c7 100644 --- a/cli/configssh.go +++ b/cli/configssh.go @@ -242,7 +242,6 @@ func configSSH() *cobra.Command { for _, change := range changes { _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "* %s\n", change) } - } else { } for _, diffFn := range []func() ([]byte, error){ From 7a4b8d69b3f34eed2b0d2908e92f4cd3068c7a61 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 30 May 2022 18:40:07 +0300 Subject: [PATCH 08/38] fix: Handle no workspaces case --- cli/configssh.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/cli/configssh.go b/cli/configssh.go index ef9b089a522c7..ac401223f3733 100644 --- a/cli/configssh.go +++ b/cli/configssh.go @@ -285,8 +285,12 @@ func configSSH() *cobra.Command { } } - _, _ = fmt.Fprintln(cmd.OutOrStdout(), "You should now be able to ssh into your workspace") - _, _ = fmt.Fprintf(cmd.OutOrStdout(), "For example, try running:\n\n\t$ ssh coder.%s\n\n", workspaces[0].Name) + if len(workspaces) > 0 { + _, _ = fmt.Fprintln(cmd.OutOrStdout(), "You should now be able to ssh into your workspace") + _, _ = fmt.Fprintf(cmd.OutOrStdout(), "For example, try running:\n\n\t$ ssh coder.%s\n\n", workspaces[0].Name) + } else { + _, _ = fmt.Fprint(cmd.OutOrStdout(), "You don't have any workspaces yet, try creating one with:\n\n\t$ coder create \n\n") + } return nil }, } From e552e846064319daf254b690cf60c6a58d7160fe Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 30 May 2022 18:58:17 +0300 Subject: [PATCH 09/38] chore: Update diffBytes comment --- cli/configssh.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cli/configssh.go b/cli/configssh.go index ac401223f3733..846d104d5a4ea 100644 --- a/cli/configssh.go +++ b/cli/configssh.go @@ -380,8 +380,8 @@ func currentBinPath(cmd *cobra.Command) (string, error) { return binName, nil } -// diffBytes two byte slices as if they were in a file named name. -// Does best-effort cleanup ignoring non-critical errors. +// diffBytes takes two byte slices and diffs them as if they were in a +// file named name. func diffBytes(name string, b1, b2 []byte) ([]byte, error) { var buf bytes.Buffer var opts []write.Option From 5250a8a25463c734742ef16cf8864eb520b51e56 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 30 May 2022 19:10:36 +0300 Subject: [PATCH 10/38] fix: Cleanup temp file on failure --- cli/configssh.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/cli/configssh.go b/cli/configssh.go index 846d104d5a4ea..23d0674b5ea99 100644 --- a/cli/configssh.go +++ b/cli/configssh.go @@ -309,7 +309,7 @@ func configSSH() *cobra.Command { // directory as path and renames the temp file to the file provided in // path. This ensure we avoid trashing the file we are writing due to // unforeseen circumstance like filesystem full, command killed, etc. -func writeWithTempFileAndMove(path string, r io.Reader) error { +func writeWithTempFileAndMove(path string, r io.Reader) (err error) { dir := filepath.Dir(path) name := filepath.Base(path) @@ -319,6 +319,11 @@ func writeWithTempFileAndMove(path string, r io.Reader) error { if err != nil { return xerrors.Errorf("create temp file failed: %w", err) } + defer func() { + if err != nil { + _ = os.Remove(f.Name()) // Cleanup in case a step failed. + } + }() _, err = io.Copy(f, r) if err != nil { From 0749e58875d358514be31129b9ecba49f2a0690b Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 30 May 2022 19:11:16 +0300 Subject: [PATCH 11/38] fix: Wrap error --- cli/configssh.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/configssh.go b/cli/configssh.go index 23d0674b5ea99..6f0599dc9dacb 100644 --- a/cli/configssh.go +++ b/cli/configssh.go @@ -328,7 +328,7 @@ func writeWithTempFileAndMove(path string, r io.Reader) (err error) { _, err = io.Copy(f, r) if err != nil { _ = f.Close() - return err + return xerrors.Errorf("write temp file failed: %w", err) } err = f.Close() From b3f448ae4284e658cad0cc427d3df49372aa2e85 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 30 May 2022 22:10:17 +0300 Subject: [PATCH 12/38] fix: Enable color when terminal is a tty --- cli/configssh.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/cli/configssh.go b/cli/configssh.go index 6f0599dc9dacb..7e900248bd845 100644 --- a/cli/configssh.go +++ b/cli/configssh.go @@ -244,9 +244,10 @@ func configSSH() *cobra.Command { } } + color := isTTY(cmd) for _, diffFn := range []func() ([]byte, error){ - func() ([]byte, error) { return diffBytes(sshConfigFile, configRaw, configModified) }, - func() ([]byte, error) { return diffBytes(coderConfigFile, coderConfigRaw, buf.Bytes()) }, + func() ([]byte, error) { return diffBytes(sshConfigFile, configRaw, configModified, color) }, + func() ([]byte, error) { return diffBytes(coderConfigFile, coderConfigRaw, buf.Bytes(), color) }, } { diff, err := diffFn() if err != nil { @@ -387,11 +388,11 @@ func currentBinPath(cmd *cobra.Command) (string, error) { // diffBytes takes two byte slices and diffs them as if they were in a // file named name. -func diffBytes(name string, b1, b2 []byte) ([]byte, error) { +//nolint: revive // Color is an option, not a control coupling. +func diffBytes(name string, b1, b2 []byte, color bool) ([]byte, error) { var buf bytes.Buffer var opts []write.Option - // TODO(mafredri): Toggle color on/off - if false { + if color { opts = append(opts, write.TerminalColor()) } err := diff.Text(name, name+".new", b1, b2, &buf, opts...) From c5f19836595281e7a037013e5a17df64038dad3b Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 31 May 2022 11:34:44 +0300 Subject: [PATCH 13/38] fix: Improve output, detect if stdout is tty --- cli/configssh.go | 39 +++++++++++++++++++++++---------------- cli/root.go | 18 ++++++++++++++++++ 2 files changed, 41 insertions(+), 16 deletions(-) diff --git a/cli/configssh.go b/cli/configssh.go index 7e900248bd845..41ec4461e018b 100644 --- a/cli/configssh.go +++ b/cli/configssh.go @@ -101,6 +101,15 @@ func configSSH() *cobra.Command { // TODO(mafredri): Check coderConfigFile for previous options // coderConfigFile. + out := cmd.OutOrStdout() + if showDiff { + out = cmd.OutOrStderr() + } + binaryFile, err := currentBinPath(out) + if err != nil { + return err + } + // Only allow not-exist errors to avoid trashing // the users SSH config. configRaw, err := os.ReadFile(sshConfigFile) @@ -134,11 +143,6 @@ func configSSH() *cobra.Command { configModified = append(configModified, []byte(sep+sshConfigIncludeStatement+"\n")...) } - binaryFile, err := currentBinPath(cmd) - if err != nil { - return err - } - root := createConfig(cmd) var errGroup errgroup.Group type workspaceConfig struct { @@ -238,13 +242,13 @@ func configSSH() *cobra.Command { if showDiff { if len(changes) > 0 { // Write to stderr to avoid dirtying the diff output. - _, _ = fmt.Fprint(cmd.ErrOrStderr(), "Changes:\n\n") + _, _ = fmt.Fprint(out, "Changes:\n\n") for _, change := range changes { - _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "* %s\n", change) + _, _ = fmt.Fprintf(out, "* %s\n", change) } } - color := isTTY(cmd) + color := isTTYOut(cmd) for _, diffFn := range []func() ([]byte, error){ func() ([]byte, error) { return diffBytes(sshConfigFile, configRaw, configModified, color) }, func() ([]byte, error) { return diffBytes(coderConfigFile, coderConfigRaw, buf.Bytes(), color) }, @@ -255,6 +259,7 @@ func configSSH() *cobra.Command { } if len(diff) > 0 { filesDiffer = true + // Always write to stdout. _, _ = fmt.Fprintf(cmd.OutOrStdout(), "\n%s", diff) } } @@ -268,9 +273,9 @@ func configSSH() *cobra.Command { IsConfirm: true, }) if err != nil { - return err + return nil } - _, _ = fmt.Fprint(cmd.OutOrStdout(), "\n") + _, _ = fmt.Fprint(out, "\n") if !bytes.Equal(configRaw, configModified) { err = writeWithTempFileAndMove(sshConfigFile, bytes.NewReader(configRaw)) @@ -287,10 +292,10 @@ func configSSH() *cobra.Command { } if len(workspaces) > 0 { - _, _ = fmt.Fprintln(cmd.OutOrStdout(), "You should now be able to ssh into your workspace") - _, _ = fmt.Fprintf(cmd.OutOrStdout(), "For example, try running:\n\n\t$ ssh coder.%s\n\n", workspaces[0].Name) + _, _ = fmt.Fprintln(out, "You should now be able to ssh into your workspace.") + _, _ = fmt.Fprintf(out, "For example, try running:\n\n\t$ ssh coder.%s\n\n", workspaces[0].Name) } else { - _, _ = fmt.Fprint(cmd.OutOrStdout(), "You don't have any workspaces yet, try creating one with:\n\n\t$ coder create \n\n") + _, _ = fmt.Fprint(out, "You don't have any workspaces yet, try creating one with:\n\n\t$ coder create \n\n") } return nil }, @@ -347,7 +352,7 @@ func writeWithTempFileAndMove(path string, r io.Reader) (err error) { // currentBinPath returns the path to the coder binary suitable for use in ssh // ProxyCommand. -func currentBinPath(cmd *cobra.Command) (string, error) { +func currentBinPath(w io.Writer) (string, error) { exePath, err := os.Executable() if err != nil { return "", xerrors.Errorf("get executable path: %w", err) @@ -363,11 +368,12 @@ func currentBinPath(cmd *cobra.Command) (string, error) { // correctly. Check if the current executable is in $PATH, and warn the user // if it isn't. if err != nil && runtime.GOOS == "windows" { - cliui.Warn(cmd.OutOrStdout(), + cliui.Warn(w, "The current executable is not in $PATH.", "This may lead to problems connecting to your workspace via SSH.", fmt.Sprintf("Please move %q to a location in your $PATH (such as System32) and run `%s config-ssh` again.", binName, binName), ) + _, _ = fmt.Fprint(w, "\n") // Return the exePath so SSH at least works outside of Msys2. return exePath, nil } @@ -375,12 +381,13 @@ func currentBinPath(cmd *cobra.Command) (string, error) { // Warn the user if the current executable is not the same as the one in // $PATH. if filepath.Clean(pathPath) != filepath.Clean(exePath) { - cliui.Warn(cmd.OutOrStdout(), + cliui.Warn(w, "The current executable path does not match the executable path found in $PATH.", "This may cause issues connecting to your workspace via SSH.", fmt.Sprintf("\tCurrent executable path: %q", exePath), fmt.Sprintf("\tExecutable path in $PATH: %q", pathPath), ) + _, _ = fmt.Fprint(w, "\n") } return binName, nil diff --git a/cli/root.go b/cli/root.go index a112de28f5b28..abf69c84f54cd 100644 --- a/cli/root.go +++ b/cli/root.go @@ -244,6 +244,24 @@ func isTTY(cmd *cobra.Command) bool { return isatty.IsTerminal(file.Fd()) } +// isTTYOut returns whether the passed reader is a TTY or not. +// This accepts a reader to work with Cobra's "OutOrStdout" +// function for simple testing. +func isTTYOut(cmd *cobra.Command) bool { + // If the `--force-tty` command is available, and set, + // assume we're in a tty. This is primarily for cases on Windows + // where we may not be able to reliably detect this automatically (ie, tests) + forceTty, err := cmd.Flags().GetBool(varForceTty) + if forceTty && err == nil { + return true + } + file, ok := cmd.OutOrStdout().(*os.File) + if !ok { + return false + } + return isatty.IsTerminal(file.Fd()) +} + func usageTemplate() string { // usageHeader is defined in init(). return `{{usageHeader "Usage:"}} From 8d0f72c819bd15a1135d920f09dc4629f3f6f7a1 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 31 May 2022 12:20:14 +0300 Subject: [PATCH 14/38] fix: Add Include to the top of config, verify semantically --- cli/configssh.go | 60 ++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 48 insertions(+), 12 deletions(-) diff --git a/cli/configssh.go b/cli/configssh.go index 41ec4461e018b..49af1232d2e30 100644 --- a/cli/configssh.go +++ b/cli/configssh.go @@ -8,6 +8,7 @@ import ( "io/fs" "os" "path/filepath" + "regexp" "runtime" "sort" "strings" @@ -26,8 +27,7 @@ import ( ) const ( - sshDefaultConfigFileName = "~/.ssh/config" - // TODO(mafredri): Consider moving to coder config dir, i.e. ~/.config/coderv2/ssh_config? + sshDefaultConfigFileName = "~/.ssh/config" sshDefaultCoderConfigFileName = "~/.ssh/coder" sshCoderConfigHeader = `# This file is managed by coder. DO NOT EDIT. # @@ -42,6 +42,16 @@ const ( sshConfigIncludeStatement = "Include coder" ) +// Regular expressions are used because SSH configs do not have +// meaningful indentation and keywords are case-insensitive. +var ( + // Find the first Host and Match statement as these restrict the + // following declarations to be used conditionally. + sshHostRe = regexp.MustCompile(`^\s*((?i)Host|Match)`) + // Find the semantically correct include statement. + sshCoderIncludedRe = regexp.MustCompile(`^\s*((?i)Include) coder(\s|$)`) +) + func configSSH() *cobra.Command { var ( sshConfigFile string @@ -132,15 +142,11 @@ func configSSH() *cobra.Command { changes = append(changes, fmt.Sprintf("Remove old config block (START-CODER/END-CODER) from %s", sshConfigFileOrig)) } - if found := bytes.Index(configModified, []byte(sshConfigIncludeStatement)); found == -1 || (found > 0 && configModified[found-1] != '\n') { - changes = append(changes, fmt.Sprintf("Add 'Include coder' to %s", sshConfigFileOrig)) - // Separate Include statement from user content with an empty newline. - configModified = bytes.TrimRight(configModified, "\n") - sep := "\n\n" - if len(configModified) == 0 { - sep = "" - } - configModified = append(configModified, []byte(sep+sshConfigIncludeStatement+"\n")...) + // Check for the presence of the coder Include + // statement is present and add if missing. + configModified, ok = sshConfigAddCoderInclude(configModified) + if ok { + changes = append(changes, fmt.Sprintf("Add %q to %s", "Include coder", sshConfigFileOrig)) } root := createConfig(cmd) @@ -278,7 +284,7 @@ func configSSH() *cobra.Command { _, _ = fmt.Fprint(out, "\n") if !bytes.Equal(configRaw, configModified) { - err = writeWithTempFileAndMove(sshConfigFile, bytes.NewReader(configRaw)) + err = writeWithTempFileAndMove(sshConfigFile, bytes.NewReader(configModified)) if err != nil { return xerrors.Errorf("write ssh config failed: %w", err) } @@ -311,6 +317,36 @@ func configSSH() *cobra.Command { return cmd } +// sshConfigAddCoderInclude checks for the coder Include statement and +// returns modified = true if it was added. +func sshConfigAddCoderInclude(data []byte) (modifiedData []byte, modified bool) { + found := false + firstHost := sshHostRe.FindIndex(data) + coderInclude := sshCoderIncludedRe.FindIndex(data) + if firstHost != nil && coderInclude != nil { + // If the Coder Include statement exists + // before a Host entry, we're good. + found = coderInclude[1] < firstHost[0] + } else if coderInclude != nil { + found = true + } + if found { + return data, false + } + + // Add Include statement to the top of SSH config. + // The user is allowed to move it as long as it + // stays above the first Host (or Match) statement. + sep := "\n\n" + if len(data) == 0 { + // If SSH config is empty, a single newline will suffice. + sep = "\n" + } + data = append([]byte(sshConfigIncludeStatement+sep), data...) + + return data, true +} + // writeWithTempFileAndMove writes to a temporary file in the same // directory as path and renames the temp file to the file provided in // path. This ensure we avoid trashing the file we are writing due to From d7838b5de40c93e7af0d3abdec8fcd838b35d199 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 31 May 2022 12:34:12 +0300 Subject: [PATCH 15/38] fix: Guard against overwriting unknown ~/.ssh/coder file --- cli/configssh.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/cli/configssh.go b/cli/configssh.go index 49af1232d2e30..878ff883bfa48 100644 --- a/cli/configssh.go +++ b/cli/configssh.go @@ -29,7 +29,8 @@ import ( const ( sshDefaultConfigFileName = "~/.ssh/config" sshDefaultCoderConfigFileName = "~/.ssh/coder" - sshCoderConfigHeader = `# This file is managed by coder. DO NOT EDIT. + sshCoderConfigHeader = "# This file is managed by coder. DO NOT EDIT." + sshCoderConfigDocsHeader = ` # # You should not hand-edit this file, all changes will be lost upon workspace # creation, deletion or when running "coder config-ssh". @@ -131,6 +132,11 @@ func configSSH() *cobra.Command { if err != nil && !errors.Is(err, fs.ErrNotExist) { return xerrors.Errorf("read ssh config failed: %w", err) } + if len(coderConfigRaw) > 0 { + if !bytes.HasPrefix(coderConfigRaw, []byte(sshCoderConfigHeader)) { + return xerrors.Errorf("unexpected content in %s: remove the file and rerun the command to continue", coderConfigFile) + } + } // Keep track of changes we are making. var changes []string @@ -190,6 +196,7 @@ func configSSH() *cobra.Command { buf := &bytes.Buffer{} _, _ = buf.WriteString(sshCoderConfigHeader) + _, _ = buf.WriteString(sshCoderConfigDocsHeader) // Store the provided flags as part of the // config for future (re)use. From f94bc3f8a73571aa90f2cf1929aed72dd257fc23 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 31 May 2022 15:38:52 +0300 Subject: [PATCH 16/38] feat: Parse and prompt to re-use previous configuration --- cli/configssh.go | 172 ++++++++++++++++++++++++++++++++++++----------- 1 file changed, 132 insertions(+), 40 deletions(-) diff --git a/cli/configssh.go b/cli/configssh.go index 878ff883bfa48..fe01ff629e9ba 100644 --- a/cli/configssh.go +++ b/cli/configssh.go @@ -1,6 +1,7 @@ package cli import ( + "bufio" "bytes" "errors" "fmt" @@ -33,9 +34,9 @@ const ( sshCoderConfigDocsHeader = ` # # You should not hand-edit this file, all changes will be lost upon workspace -# creation, deletion or when running "coder config-ssh". -` - sshCoderConfigOptionsHeader = `# +# creation, deletion or when running "coder config-ssh".` + sshCoderConfigOptionsHeader = ` +# # Last config-ssh options: ` // Relative paths are assumed to be in ~/.ssh, except when @@ -53,11 +54,50 @@ var ( sshCoderIncludedRe = regexp.MustCompile(`^\s*((?i)Include) coder(\s|$)`) ) +// sshCoderConfigOptions represents options that can be stored and read +// from the coder config in ~/.ssh/coder. +type sshCoderConfigOptions struct { + sshConfigFile string + sshOptions []string +} + +func (o sshCoderConfigOptions) isZero() bool { + return o.sshConfigFile == sshDefaultConfigFileName && len(o.sshOptions) == 0 +} + +func (o sshCoderConfigOptions) equal(other sshCoderConfigOptions) bool { + // Compare without side-effects or regard to order. + opt1 := slices.Clone(o.sshOptions) + sort.Strings(opt1) + opt2 := slices.Clone(other.sshOptions) + sort.Strings(opt2) + return o.sshConfigFile == other.sshConfigFile && slices.Equal(opt1, opt2) +} + +func (o sshCoderConfigOptions) asArgs() (args []string) { + if o.sshConfigFile != sshDefaultConfigFileName { + args = append(args, "--ssh-config-file", o.sshConfigFile) + } + for _, opt := range o.sshOptions { + args = append(args, "--ssh-option", fmt.Sprintf("%q", opt)) + } + return args +} + +func (o sshCoderConfigOptions) asList() (list []string) { + if o.sshConfigFile != sshDefaultConfigFileName { + list = append(list, fmt.Sprintf("ssh-config-file: %s", o.sshConfigFile)) + } + for _, opt := range o.sshOptions { + list = append(list, fmt.Sprintf("ssh-option: %s", opt)) + } + return list +} + func configSSH() *cobra.Command { var ( - sshConfigFile string + coderConfig sshCoderConfigOptions coderConfigFile string - sshOptions []string showDiff bool skipProxyCommand bool @@ -97,30 +137,26 @@ func configSSH() *cobra.Command { return err } + out := cmd.OutOrStdout() + if showDiff { + out = cmd.OutOrStderr() + } + binaryFile, err := currentBinPath(out) + if err != nil { + return err + } + dirname, err := os.UserHomeDir() if err != nil { return xerrors.Errorf("user home dir failed: %w", err) } - sshConfigFileOrig := sshConfigFile // Store the pre ~/ replacement name for serializing options. + sshConfigFile := coderConfig.sshConfigFile // Store the pre ~/ replacement name for serializing options. if strings.HasPrefix(sshConfigFile, "~/") { sshConfigFile = filepath.Join(dirname, sshConfigFile[2:]) } - coderConfigFileOrig := coderConfigFile coderConfigFile = filepath.Join(dirname, coderConfigFile[2:]) // Replace ~/ with home dir. - // TODO(mafredri): Check coderConfigFile for previous options - // coderConfigFile. - - out := cmd.OutOrStdout() - if showDiff { - out = cmd.OutOrStderr() - } - binaryFile, err := currentBinPath(out) - if err != nil { - return err - } - // Only allow not-exist errors to avoid trashing // the users SSH config. configRaw, err := os.ReadFile(sshConfigFile) @@ -137,6 +173,25 @@ func configSSH() *cobra.Command { return xerrors.Errorf("unexpected content in %s: remove the file and rerun the command to continue", coderConfigFile) } } + lastCoderConfig := sshCoderConfigParseLastOptions(bytes.NewReader(coderConfigRaw)) + + // Only prompt when no arguments are provided and avoid + // prompting in diff mode (unexpected behavior). + if !showDiff && coderConfig.isZero() && !lastCoderConfig.isZero() { + line, err := cliui.Prompt(cmd, cliui.PromptOptions{ + Text: fmt.Sprintf("Found previous configuration option(s):\n\n - %s\n\n Use previous option(s)?", strings.Join(lastCoderConfig.asList(), "\n - ")), + IsConfirm: true, + }) + if err != nil { + // TODO(mafredri): Better way to differ between "no" and Ctrl+C? + if line == "" && xerrors.Is(err, cliui.Canceled) { + return nil + } + } else { + coderConfig = lastCoderConfig + } + _, _ = fmt.Fprint(out, "\n") + } // Keep track of changes we are making. var changes []string @@ -145,14 +200,14 @@ func configSSH() *cobra.Command { // remove if present. configModified, ok := stripOldConfigBlock(configRaw) if ok { - changes = append(changes, fmt.Sprintf("Remove old config block (START-CODER/END-CODER) from %s", sshConfigFileOrig)) + changes = append(changes, fmt.Sprintf("Remove old config block (START-CODER/END-CODER) from %s", sshConfigFile)) } // Check for the presence of the coder Include // statement is present and add if missing. configModified, ok = sshConfigAddCoderInclude(configModified) if ok { - changes = append(changes, fmt.Sprintf("Add %q to %s", "Include coder", sshConfigFileOrig)) + changes = append(changes, fmt.Sprintf("Add %q to %s", "Include coder", sshConfigFile)) } root := createConfig(cmd) @@ -195,19 +250,13 @@ func configSSH() *cobra.Command { } buf := &bytes.Buffer{} - _, _ = buf.WriteString(sshCoderConfigHeader) - _, _ = buf.WriteString(sshCoderConfigDocsHeader) - - // Store the provided flags as part of the - // config for future (re)use. - _, _ = buf.WriteString(sshCoderConfigOptionsHeader) - if sshConfigFileOrig != sshDefaultConfigFileName { - _, _ = fmt.Fprintf(buf, "# :%s=%s\n", "ssh-config-file", sshConfigFileOrig) - } - for _, opt := range sshOptions { - _, _ = fmt.Fprintf(buf, "# :%s=%s\n", "ssh-option", opt) + + // Write header and store the provided options as part + // of the config for future (re)use. + err = sshCoderConfigWriteHeader(buf, coderConfig) + if err != nil { + return xerrors.Errorf("write coder config header failed: %w", err) } - _, _ = buf.WriteString("#\n") // Ensure stable sorting of output. slices.SortFunc(workspaceConfigs, func(a, b workspaceConfig) bool { @@ -220,7 +269,7 @@ func configSSH() *cobra.Command { configOptions := []string{ "Host coder." + hostname, } - for _, option := range sshOptions { + for _, option := range coderConfig.sshOptions { configOptions = append(configOptions, "\t"+option) } configOptions = append(configOptions, @@ -246,9 +295,9 @@ func configSSH() *cobra.Command { modifyCoderConfig := !bytes.Equal(coderConfigRaw, buf.Bytes()) if modifyCoderConfig { if len(coderConfigRaw) == 0 { - changes = append(changes, fmt.Sprintf("Write auto-generated coder config file to %s", coderConfigFileOrig)) + changes = append(changes, fmt.Sprintf("Write auto-generated coder config file to %s", coderConfigFile)) } else { - changes = append(changes, fmt.Sprintf("Update auto-generated coder config file in %s", coderConfigFileOrig)) + changes = append(changes, fmt.Sprintf("Update auto-generated coder config file in %s", coderConfigFile)) } } @@ -257,7 +306,7 @@ func configSSH() *cobra.Command { // Write to stderr to avoid dirtying the diff output. _, _ = fmt.Fprint(out, "Changes:\n\n") for _, change := range changes { - _, _ = fmt.Fprintf(out, "* %s\n", change) + _, _ = fmt.Fprintf(out, " * %s\n", change) } } @@ -281,8 +330,11 @@ func configSSH() *cobra.Command { } if len(changes) > 0 { + // In diff mode we don't prompt re-using the previous + // configuration, so we output the entire command. + diffCommand := fmt.Sprintf("$ %s %s", cmd.CommandPath(), strings.Join(append(coderConfig.asArgs(), "--diff"), " ")) _, err = cliui.Prompt(cmd, cliui.PromptOptions{ - Text: fmt.Sprintf("The following changes will be made to your SSH configuration (use --diff to see changes):\n\n * %s\n\n Continue?", strings.Join(changes, "\n * ")), + Text: fmt.Sprintf("The following changes will be made to your SSH configuration:\n\n * %s\n\n To see changes, run with --diff:\n\n %s\n\n Continue?", strings.Join(changes, "\n * "), diffCommand), IsConfirm: true, }) if err != nil { @@ -313,10 +365,10 @@ func configSSH() *cobra.Command { return nil }, } - cliflag.StringVarP(cmd.Flags(), &sshConfigFile, "ssh-config-file", "", "CODER_SSH_CONFIG_FILE", sshDefaultConfigFileName, "Specifies the path to an SSH config.") + cliflag.StringVarP(cmd.Flags(), &coderConfig.sshConfigFile, "ssh-config-file", "", "CODER_SSH_CONFIG_FILE", sshDefaultConfigFileName, "Specifies the path to an SSH config.") cmd.Flags().StringVar(&coderConfigFile, "ssh-coder-config-file", sshDefaultCoderConfigFileName, "Specifies the path to an Coder SSH config file. Useful for testing.") _ = cmd.Flags().MarkHidden("ssh-coder-config-file") - cmd.Flags().StringArrayVarP(&sshOptions, "ssh-option", "o", []string{}, "Specifies additional SSH options to embed in each host stanza.") + cmd.Flags().StringArrayVarP(&coderConfig.sshOptions, "ssh-option", "o", []string{}, "Specifies additional SSH options to embed in each host stanza.") cmd.Flags().BoolVarP(&showDiff, "diff", "D", false, "Show diff of changes that will be made.") cmd.Flags().BoolVarP(&skipProxyCommand, "skip-proxy-command", "", false, "Specifies whether the ProxyCommand option should be skipped. Useful for testing.") _ = cmd.Flags().MarkHidden("skip-proxy-command") @@ -354,6 +406,46 @@ func sshConfigAddCoderInclude(data []byte) (modifiedData []byte, modified bool) return data, true } +func sshCoderConfigWriteHeader(w io.Writer, o sshCoderConfigOptions) error { + _, _ = fmt.Fprint(w, sshCoderConfigHeader) + _, _ = fmt.Fprint(w, sshCoderConfigDocsHeader) + _, _ = fmt.Fprint(w, sshCoderConfigOptionsHeader) + if o.sshConfigFile != sshDefaultConfigFileName { + _, _ = fmt.Fprintf(w, "# :%s=%s\n", "ssh-config-file", o.sshConfigFile) + } + for _, opt := range o.sshOptions { + _, _ = fmt.Fprintf(w, "# :%s=%s\n", "ssh-option", opt) + } + _, _ = fmt.Fprint(w, "#\n") + return nil +} + +func sshCoderConfigParseLastOptions(r io.Reader) (o sshCoderConfigOptions) { + o.sshConfigFile = sshDefaultConfigFileName // Default value is not written. + + s := bufio.NewScanner(r) + for s.Scan() { + line := s.Text() + if strings.HasPrefix(line, "# :") { + line = strings.TrimPrefix(line, "# :") + parts := strings.SplitN(line, "=", 2) + switch parts[0] { + case "ssh-config-file": + o.sshConfigFile = parts[1] + case "ssh-option": + o.sshOptions = append(o.sshOptions, parts[1]) + default: + // Unknown option, ignore. + } + } + } + if err := s.Err(); err != nil { + panic(err) + } + + return o +} + // writeWithTempFileAndMove writes to a temporary file in the same // directory as path and renames the temp file to the file provided in // path. This ensure we avoid trashing the file we are writing due to From 00a8c129d53cd54e1ae03e0d127f124e50d794e0 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 31 May 2022 15:51:30 +0300 Subject: [PATCH 17/38] chore: Increase indentation --- cli/configssh.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/configssh.go b/cli/configssh.go index fe01ff629e9ba..51742c7f59e3c 100644 --- a/cli/configssh.go +++ b/cli/configssh.go @@ -179,7 +179,7 @@ func configSSH() *cobra.Command { // prompting in diff mode (unexpected behavior). if !showDiff && coderConfig.isZero() && !lastCoderConfig.isZero() { line, err := cliui.Prompt(cmd, cliui.PromptOptions{ - Text: fmt.Sprintf("Found previous configuration option(s):\n\n - %s\n\n Use previous option(s)?", strings.Join(lastCoderConfig.asList(), "\n - ")), + Text: fmt.Sprintf("Found previous configuration option(s):\n\n - %s\n\n Use previous option(s)?", strings.Join(lastCoderConfig.asList(), "\n - ")), IsConfirm: true, }) if err != nil { From e50c4c3bcad9f95062136d7dbe730f530fae9c04 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 31 May 2022 15:55:52 +0300 Subject: [PATCH 18/38] chore: Remove unused equality func --- cli/configssh.go | 9 --------- 1 file changed, 9 deletions(-) diff --git a/cli/configssh.go b/cli/configssh.go index 51742c7f59e3c..7714671e83c63 100644 --- a/cli/configssh.go +++ b/cli/configssh.go @@ -65,15 +65,6 @@ func (o sshCoderConfigOptions) isZero() bool { return o.sshConfigFile == sshDefaultConfigFileName && len(o.sshOptions) == 0 } -func (o sshCoderConfigOptions) equal(other sshCoderConfigOptions) bool { - // Compare without side-effects or regard to order. - opt1 := slices.Clone(o.sshOptions) - sort.Strings(opt1) - opt2 := slices.Clone(other.sshOptions) - sort.Strings(opt2) - return o.sshConfigFile == other.sshConfigFile && slices.Equal(opt1, opt2) -} - func (o sshCoderConfigOptions) asArgs() (args []string) { if o.sshConfigFile != sshDefaultConfigFileName { args = append(args, "--ssh-config-file", o.sshConfigFile) From 94c868144321f845bc28ef2cc0f90f4c2bbd24a3 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 1 Jun 2022 17:04:36 +0300 Subject: [PATCH 19/38] fix: Perform multi line regexp matches --- cli/configssh.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cli/configssh.go b/cli/configssh.go index 7714671e83c63..e25ff04d89398 100644 --- a/cli/configssh.go +++ b/cli/configssh.go @@ -49,9 +49,9 @@ const ( var ( // Find the first Host and Match statement as these restrict the // following declarations to be used conditionally. - sshHostRe = regexp.MustCompile(`^\s*((?i)Host|Match)`) + sshHostRe = regexp.MustCompile(`(?m)^\s*((?i)Host|Match)\s.*$`) // Find the semantically correct include statement. - sshCoderIncludedRe = regexp.MustCompile(`^\s*((?i)Include) coder(\s|$)`) + sshCoderIncludedRe = regexp.MustCompile(`(?m)^\s*((?i)Include) coder(\s.*)?$`) ) // sshCoderConfigOptions represents options that can be stored and read From 1e3388672c209e2110ee5b3b122f3ee3b21c34a0 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 1 Jun 2022 17:05:26 +0300 Subject: [PATCH 20/38] fix: Only rewrite coder file path if not ~/ --- cli/configssh.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/cli/configssh.go b/cli/configssh.go index e25ff04d89398..ec2ad4388d696 100644 --- a/cli/configssh.go +++ b/cli/configssh.go @@ -142,11 +142,13 @@ func configSSH() *cobra.Command { return xerrors.Errorf("user home dir failed: %w", err) } - sshConfigFile := coderConfig.sshConfigFile // Store the pre ~/ replacement name for serializing options. + sshConfigFile := coderConfig.sshConfigFile if strings.HasPrefix(sshConfigFile, "~/") { sshConfigFile = filepath.Join(dirname, sshConfigFile[2:]) } - coderConfigFile = filepath.Join(dirname, coderConfigFile[2:]) // Replace ~/ with home dir. + if strings.HasPrefix(coderConfigFile, "~/") { + coderConfigFile = filepath.Join(dirname, coderConfigFile[2:]) + } // Only allow not-exist errors to avoid trashing // the users SSH config. From 209c8ff7541ca6c4f9c88a151b0d3c58721377d6 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 1 Jun 2022 17:05:49 +0300 Subject: [PATCH 21/38] fix: Allow removing invalid Include coder entry --- cli/configssh.go | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/cli/configssh.go b/cli/configssh.go index ec2ad4388d696..b838dd299a4e5 100644 --- a/cli/configssh.go +++ b/cli/configssh.go @@ -372,17 +372,23 @@ func configSSH() *cobra.Command { // sshConfigAddCoderInclude checks for the coder Include statement and // returns modified = true if it was added. func sshConfigAddCoderInclude(data []byte) (modifiedData []byte, modified bool) { - found := false + valid := false firstHost := sshHostRe.FindIndex(data) coderInclude := sshCoderIncludedRe.FindIndex(data) if firstHost != nil && coderInclude != nil { // If the Coder Include statement exists // before a Host entry, we're good. - found = coderInclude[1] < firstHost[0] + valid = coderInclude[1] < firstHost[0] + if !valid { + // Remove invalid Include statement. + d := append([]byte{}, data[:coderInclude[0]]...) + d = append(d, data[coderInclude[1]:]...) + data = d + } } else if coderInclude != nil { - found = true + valid = true } - if found { + if valid { return data, false } From 5ed5e0ee2710971c39e90b28f76cdd4091a51aea Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 1 Jun 2022 17:06:21 +0300 Subject: [PATCH 22/38] Revert "chore: Remove unused equality func" This reverts commit f2f4a83369549ad390c59b281be41b68f9bd7006. --- cli/configssh.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/cli/configssh.go b/cli/configssh.go index b838dd299a4e5..80b94b7f3b93b 100644 --- a/cli/configssh.go +++ b/cli/configssh.go @@ -65,6 +65,15 @@ func (o sshCoderConfigOptions) isZero() bool { return o.sshConfigFile == sshDefaultConfigFileName && len(o.sshOptions) == 0 } +func (o sshCoderConfigOptions) equal(other sshCoderConfigOptions) bool { + // Compare without side-effects or regard to order. + opt1 := slices.Clone(o.sshOptions) + sort.Strings(opt1) + opt2 := slices.Clone(other.sshOptions) + sort.Strings(opt2) + return o.sshConfigFile == other.sshConfigFile && slices.Equal(opt1, opt2) +} + func (o sshCoderConfigOptions) asArgs() (args []string) { if o.sshConfigFile != sshDefaultConfigFileName { args = append(args, "--ssh-config-file", o.sshConfigFile) From 000c9b461ecc90a172a0ec28d66c0212644abcf9 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 1 Jun 2022 18:36:44 +0300 Subject: [PATCH 23/38] fix: Allow replacing default ssh config file in tests --- cli/configssh.go | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/cli/configssh.go b/cli/configssh.go index 80b94b7f3b93b..32ea9c3873a2e 100644 --- a/cli/configssh.go +++ b/cli/configssh.go @@ -57,12 +57,9 @@ var ( // sshCoderConfigOptions represents options that can be stored and read // from the coder config in ~/.ssh/coder. type sshCoderConfigOptions struct { - sshConfigFile string - sshOptions []string -} - -func (o sshCoderConfigOptions) isZero() bool { - return o.sshConfigFile == sshDefaultConfigFileName && len(o.sshOptions) == 0 + sshConfigDefaultFile string + sshConfigFile string + sshOptions []string } func (o sshCoderConfigOptions) equal(other sshCoderConfigOptions) bool { @@ -75,7 +72,7 @@ func (o sshCoderConfigOptions) equal(other sshCoderConfigOptions) bool { } func (o sshCoderConfigOptions) asArgs() (args []string) { - if o.sshConfigFile != sshDefaultConfigFileName { + if o.sshConfigFile != o.sshConfigDefaultFile { args = append(args, "--ssh-config-file", o.sshConfigFile) } for _, opt := range o.sshOptions { @@ -85,7 +82,7 @@ func (o sshCoderConfigOptions) asArgs() (args []string) { } func (o sshCoderConfigOptions) asList() (list []string) { - if o.sshConfigFile != sshDefaultConfigFileName { + if o.sshConfigFile != o.sshConfigDefaultFile { list = append(list, fmt.Sprintf("ssh-config-file: %s", o.sshConfigFile)) } for _, opt := range o.sshOptions { @@ -175,7 +172,7 @@ func configSSH() *cobra.Command { return xerrors.Errorf("unexpected content in %s: remove the file and rerun the command to continue", coderConfigFile) } } - lastCoderConfig := sshCoderConfigParseLastOptions(bytes.NewReader(coderConfigRaw)) + lastCoderConfig := sshCoderConfigParseLastOptions(bytes.NewReader(coderConfigRaw), coderConfig.sshConfigDefaultFile) // Only prompt when no arguments are provided and avoid // prompting in diff mode (unexpected behavior). @@ -368,8 +365,10 @@ func configSSH() *cobra.Command { }, } cliflag.StringVarP(cmd.Flags(), &coderConfig.sshConfigFile, "ssh-config-file", "", "CODER_SSH_CONFIG_FILE", sshDefaultConfigFileName, "Specifies the path to an SSH config.") - cmd.Flags().StringVar(&coderConfigFile, "ssh-coder-config-file", sshDefaultCoderConfigFileName, "Specifies the path to an Coder SSH config file. Useful for testing.") - _ = cmd.Flags().MarkHidden("ssh-coder-config-file") + cmd.Flags().StringVar(&coderConfig.sshConfigDefaultFile, "test.default-ssh-config-file", sshDefaultConfigFileName, "Specifies the default path to the SSH config file. Useful for testing.") + _ = cmd.Flags().MarkHidden("test.default-ssh-config-file") + cmd.Flags().StringVar(&coderConfigFile, "test.ssh-coder-config-file", sshDefaultCoderConfigFileName, "Specifies the path to an Coder SSH config file. Useful for testing.") + _ = cmd.Flags().MarkHidden("test.ssh-coder-config-file") cmd.Flags().StringArrayVarP(&coderConfig.sshOptions, "ssh-option", "o", []string{}, "Specifies additional SSH options to embed in each host stanza.") cmd.Flags().BoolVarP(&showDiff, "diff", "D", false, "Show diff of changes that will be made.") cmd.Flags().BoolVarP(&skipProxyCommand, "skip-proxy-command", "", false, "Specifies whether the ProxyCommand option should be skipped. Useful for testing.") @@ -418,7 +417,7 @@ func sshCoderConfigWriteHeader(w io.Writer, o sshCoderConfigOptions) error { _, _ = fmt.Fprint(w, sshCoderConfigHeader) _, _ = fmt.Fprint(w, sshCoderConfigDocsHeader) _, _ = fmt.Fprint(w, sshCoderConfigOptionsHeader) - if o.sshConfigFile != sshDefaultConfigFileName { + if o.sshConfigFile != o.sshConfigDefaultFile { _, _ = fmt.Fprintf(w, "# :%s=%s\n", "ssh-config-file", o.sshConfigFile) } for _, opt := range o.sshOptions { @@ -428,8 +427,9 @@ func sshCoderConfigWriteHeader(w io.Writer, o sshCoderConfigOptions) error { return nil } -func sshCoderConfigParseLastOptions(r io.Reader) (o sshCoderConfigOptions) { - o.sshConfigFile = sshDefaultConfigFileName // Default value is not written. +func sshCoderConfigParseLastOptions(r io.Reader, sshConfigDefaultFile string) (o sshCoderConfigOptions) { + o.sshConfigDefaultFile = sshConfigDefaultFile + o.sshConfigFile = sshConfigDefaultFile // Default value is not written. s := bufio.NewScanner(r) for s.Scan() { From 9d6468093751c040074ba905935d92f61e7c0600 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 1 Jun 2022 18:37:30 +0300 Subject: [PATCH 24/38] feat: Update options prompt, default to using new options --- cli/configssh.go | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/cli/configssh.go b/cli/configssh.go index 32ea9c3873a2e..fc32b5be39fc6 100644 --- a/cli/configssh.go +++ b/cli/configssh.go @@ -174,11 +174,21 @@ func configSSH() *cobra.Command { } lastCoderConfig := sshCoderConfigParseLastOptions(bytes.NewReader(coderConfigRaw), coderConfig.sshConfigDefaultFile) - // Only prompt when no arguments are provided and avoid - // prompting in diff mode (unexpected behavior). - if !showDiff && coderConfig.isZero() && !lastCoderConfig.isZero() { + // Avoid prompting in diff mode (unexpected behavior). + if !showDiff && !coderConfig.equal(lastCoderConfig) { + newOpts := coderConfig.asList() + newOptsMsg := "\n\n New options: none" + if len(newOpts) > 0 { + newOptsMsg = fmt.Sprintf("\n\n New options:\n * %s", strings.Join(newOpts, "\n * ")) + } + oldOpts := lastCoderConfig.asList() + oldOptsMsg := "\n\n Previouss options: none" + if len(oldOpts) > 0 { + oldOptsMsg = fmt.Sprintf("\n\n Previous options:\n * %s", strings.Join(oldOpts, "\n * ")) + } + line, err := cliui.Prompt(cmd, cliui.PromptOptions{ - Text: fmt.Sprintf("Found previous configuration option(s):\n\n - %s\n\n Use previous option(s)?", strings.Join(lastCoderConfig.asList(), "\n - ")), + Text: fmt.Sprintf("New options differ from previous options:%s%s\n\n Use new options?", newOptsMsg, oldOptsMsg), IsConfirm: true, }) if err != nil { @@ -186,7 +196,7 @@ func configSSH() *cobra.Command { if line == "" && xerrors.Is(err, cliui.Canceled) { return nil } - } else { + // Selecting "no" will use the last config. coderConfig = lastCoderConfig } _, _ = fmt.Fprint(out, "\n") From a7cb1494a2cfe3ea24a2dff3d7e09788d0a630d9 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 1 Jun 2022 19:06:19 +0300 Subject: [PATCH 25/38] fix: Improve regexes --- cli/configssh.go | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/cli/configssh.go b/cli/configssh.go index fc32b5be39fc6..13af0f1a41a4f 100644 --- a/cli/configssh.go +++ b/cli/configssh.go @@ -49,9 +49,15 @@ const ( var ( // Find the first Host and Match statement as these restrict the // following declarations to be used conditionally. - sshHostRe = regexp.MustCompile(`(?m)^\s*((?i)Host|Match)\s.*$`) - // Find the semantically correct include statement. - sshCoderIncludedRe = regexp.MustCompile(`(?m)^\s*((?i)Include) coder(\s.*)?$`) + sshHostRe = regexp.MustCompile(`(?m)^[\t ]*((?i)Host|Match)\s[^\n\r]*$`) + // Find the semantically correct include statement. Since the user can + // modify their configuration as they see fit, there could be: + // - Leading indentation (space, tab) + // - Trailing indentation (space, tab), followed by e.g. a comment or + // another file to Include (we don't want to support this, but + // explicitly blocking it adds complexity) + // - Select newline after Include statement for removal purposes + sshCoderIncludedRe = regexp.MustCompile(`(?m)^[\t ]*((?i)Include) coder([\t ].*)?[\r]?[\n]?$`) ) // sshCoderConfigOptions represents options that can be stored and read From a77c7913dccc41c1f72cc661795a478596bf1701 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 1 Jun 2022 19:07:59 +0300 Subject: [PATCH 26/38] feat: Add tests --- cli/configssh_test.go | 290 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 290 insertions(+) diff --git a/cli/configssh_test.go b/cli/configssh_test.go index 86b892b123fc3..f4e98a1026dca 100644 --- a/cli/configssh_test.go +++ b/cli/configssh_test.go @@ -6,6 +6,7 @@ import ( "net" "os" "os/exec" + "path/filepath" "strconv" "strings" "testing" @@ -138,3 +139,292 @@ func TestConfigSSH(t *testing.T) { require.NoError(t, err) require.Equal(t, "test", strings.TrimSpace(string(data))) } + +func sshConfigFileNames(t *testing.T) (sshConfig string, coderConfig string) { + t.Helper() + tmpdir := t.TempDir() + n1 := filepath.Join(tmpdir, "config") + n2 := filepath.Join(tmpdir, "coder") + return n1, n2 +} + +func sshConfigFileCreate(t *testing.T, name string, data io.Reader) { + t.Helper() + t.Logf("Writing %s", name) + f, err := os.Create(name) + require.NoError(t, err) + n, err := io.Copy(f, data) + t.Logf("Wrote %d", n) + require.NoError(t, err) + err = f.Close() + require.NoError(t, err) +} + +func sshConfigFileRead(t *testing.T, name string) string { + b, err := os.ReadFile(name) + require.NoError(t, err) + return string(b) +} + +func TestConfigSSH_FileWriteAndOptionsFlow(t *testing.T) { + t.Parallel() + + type writeConfig struct { + ssh string + coder string + } + type wantConfig struct { + ssh string + coder string + coderPartial bool + } + type match struct { + match, write string + } + tests := []struct { + name string + args []string + matches []match + writeConfig writeConfig + wantConfig wantConfig + wantErr bool + }{ + { + name: "Config files are created", + matches: []match{ + {match: "Continue?", write: "yes"}, + }, + wantConfig: wantConfig{ + ssh: strings.Join([]string{ + "Include coder", + "", + }, "\n"), + coder: "# This file is managed by coder. DO NOT EDIT.", + coderPartial: true, + }, + }, + { + name: "Include is written to top of ssh config", + writeConfig: writeConfig{ + ssh: strings.Join([]string{ + "# This is a host", + "Host test", + " HostName test", + }, "\n"), + }, + wantConfig: wantConfig{ + ssh: strings.Join([]string{ + "Include coder", + "", + "# This is a host", + "Host test", + " HostName test", + }, "\n"), + }, + matches: []match{ + {match: "Continue?", write: "yes"}, + }, + }, + { + name: "Include below Host is invalid, move it to the top", + writeConfig: writeConfig{ + ssh: strings.Join([]string{ + "Host test", + " HostName test", + "", + "Include coder", + "", + "", + }, "\n"), + }, + wantConfig: wantConfig{ + ssh: strings.Join([]string{ + "Include coder", + "", + "Host test", + " HostName test", + "", + // Only "Include coder" with accompanying + // newline is removed. + "", + "", + }, "\n"), + }, + matches: []match{ + {match: "Continue?", write: "yes"}, + }, + }, + { + name: "SSH Config does not need modification", + writeConfig: writeConfig{ + ssh: strings.Join([]string{ + "Include something/other", + "Include coder", + "", + "# This is a host", + "Host test", + " HostName test", + }, "\n"), + }, + wantConfig: wantConfig{ + ssh: strings.Join([]string{ + "Include something/other", + "Include coder", + "", + "# This is a host", + "Host test", + " HostName test", + }, "\n"), + }, + matches: []match{ + {match: "Continue?", write: "yes"}, + }, + }, + { + name: "When options differ, selecting yes overwrites previous options", + writeConfig: writeConfig{ + coder: strings.Join([]string{ + "# This file is managed by coder. DO NOT EDIT.", + "#", + "# You should not hand-edit this file, all changes will be lost upon workspace", + "# creation, deletion or when running \"coder config-ssh\".", + "#", + "# Last config-ssh options:", + "# :ssh-option=ForwardAgent=yes", + "#", + }, "\n"), + }, + wantConfig: wantConfig{ + coder: strings.Join([]string{ + "# This file is managed by coder. DO NOT EDIT.", + "#", + "# You should not hand-edit this file, all changes will be lost upon workspace", + "# creation, deletion or when running \"coder config-ssh\".", + "#", + "# Last config-ssh options:", + "#", + }, "\n"), + coderPartial: true, + }, + matches: []match{ + {match: "Use new options?", write: "yes"}, + {match: "Continue?", write: "yes"}, + }, + }, + { + name: "When options differ, selecting no preserves previous options", + writeConfig: writeConfig{ + coder: strings.Join([]string{ + "# This file is managed by coder. DO NOT EDIT.", + "#", + "# You should not hand-edit this file, all changes will be lost upon workspace", + "# creation, deletion or when running \"coder config-ssh\".", + "#", + "# Last config-ssh options:", + "# :ssh-option=ForwardAgent=yes", + "#", + }, "\n"), + }, + wantConfig: wantConfig{ + coder: strings.Join([]string{ + "# This file is managed by coder. DO NOT EDIT.", + "#", + "# You should not hand-edit this file, all changes will be lost upon workspace", + "# creation, deletion or when running \"coder config-ssh\".", + "#", + "# Last config-ssh options:", + "# :ssh-option=ForwardAgent=yes", + "#", + }, "\n"), + coderPartial: true, + }, + matches: []match{ + {match: "Use new options?", write: "no"}, + {match: "Continue?", write: "yes"}, + }, + }, + { + name: "Do not overwrite unknown coder config", + writeConfig: writeConfig{ + coder: strings.Join([]string{ + "We're no strangers to love", + "You know the rules and so do I (do I)", + }, "\n"), + }, + wantConfig: wantConfig{ + coder: strings.Join([]string{ + "We're no strangers to love", + "You know the rules and so do I (do I)", + }, "\n"), + }, + wantErr: true, + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + var ( + client = coderdtest.New(t, &coderdtest.Options{IncludeProvisionerD: true}) + user = coderdtest.CreateFirstUser(t, client) + version = coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) + _ = coderdtest.AwaitTemplateVersionJob(t, client, version.ID) + project = coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) + workspace = coderdtest.CreateWorkspace(t, client, user.OrganizationID, project.ID) + _ = coderdtest.AwaitWorkspaceBuildJob(t, client, workspace.LatestBuild.ID) + ) + + // Prepare ssh config files. + sshConfigName, coderConfigName := sshConfigFileNames(t) + if tt.writeConfig.ssh != "" { + sshConfigFileCreate(t, sshConfigName, strings.NewReader(tt.writeConfig.ssh)) + } + if tt.writeConfig.coder != "" { + sshConfigFileCreate(t, coderConfigName, strings.NewReader(tt.writeConfig.coder)) + } + + args := []string{ + "config-ssh", + "--ssh-config-file", sshConfigName, + "--test.default-ssh-config-file", sshConfigName, + "--test.ssh-coder-config-file", coderConfigName, + } + args = append(args, tt.args...) + cmd, root := clitest.New(t, args...) + clitest.SetupConfig(t, client, root) + + pty := ptytest.New(t) + cmd.SetIn(pty.Input()) + cmd.SetOut(pty.Output()) + done := tGo(t, func() { + err := cmd.Execute() + if !tt.wantErr { + assert.NoError(t, err) + } else { + assert.Error(t, err) + } + }) + + for _, m := range tt.matches { + pty.ExpectMatch(m.match) + pty.WriteLine(m.write) + } + + <-done + + if tt.wantConfig.ssh != "" { + got := sshConfigFileRead(t, sshConfigName) + assert.Equal(t, tt.wantConfig.ssh, got) + } + if tt.wantConfig.coder != "" { + got := sshConfigFileRead(t, coderConfigName) + if tt.wantConfig.coderPartial { + assert.Contains(t, got, tt.wantConfig.coder) + } else { + assert.Equal(t, tt.wantConfig.coder, got) + } + } + }) + } +} From 5bcd0383a851612df425fe786b5bd3cb3c33f41a Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 1 Jun 2022 19:35:19 +0300 Subject: [PATCH 27/38] fix: Do not prompt for previous opts on first run --- cli/configssh.go | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/cli/configssh.go b/cli/configssh.go index 13af0f1a41a4f..ab97039f67dee 100644 --- a/cli/configssh.go +++ b/cli/configssh.go @@ -169,9 +169,14 @@ func configSSH() *cobra.Command { return xerrors.Errorf("read ssh config failed: %w", err) } + coderConfigExists := true coderConfigRaw, err := os.ReadFile(coderConfigFile) - if err != nil && !errors.Is(err, fs.ErrNotExist) { - return xerrors.Errorf("read ssh config failed: %w", err) + if err != nil { + if errors.Is(err, fs.ErrNotExist) { + coderConfigExists = false + } else { + return xerrors.Errorf("read ssh config failed: %w", err) + } } if len(coderConfigRaw) > 0 { if !bytes.HasPrefix(coderConfigRaw, []byte(sshCoderConfigHeader)) { @@ -180,8 +185,9 @@ func configSSH() *cobra.Command { } lastCoderConfig := sshCoderConfigParseLastOptions(bytes.NewReader(coderConfigRaw), coderConfig.sshConfigDefaultFile) - // Avoid prompting in diff mode (unexpected behavior). - if !showDiff && !coderConfig.equal(lastCoderConfig) { + // Avoid prompting in diff mode (unexpected behavior) + // or when a previous config does not exist. + if !showDiff && !coderConfig.equal(lastCoderConfig) && coderConfigExists { newOpts := coderConfig.asList() newOptsMsg := "\n\n New options: none" if len(newOpts) > 0 { From 1632eb4bcecfade8360392f0d395b4321a7c02af Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 1 Jun 2022 19:35:57 +0300 Subject: [PATCH 28/38] fix: Fix broken TestConfigSSH test --- cli/configssh_test.go | 91 +++++++++++++++++++++++++------------------ 1 file changed, 53 insertions(+), 38 deletions(-) diff --git a/cli/configssh_test.go b/cli/configssh_test.go index f4e98a1026dca..7d3133e75164c 100644 --- a/cli/configssh_test.go +++ b/cli/configssh_test.go @@ -2,6 +2,7 @@ package cli_test import ( "context" + "fmt" "io" "net" "os" @@ -26,6 +27,36 @@ import ( "github.com/coder/coder/pty/ptytest" ) +func sshConfigFileNames(t *testing.T) (sshConfig string, coderConfig string) { + t.Helper() + tmpdir := t.TempDir() + dotssh := filepath.Join(tmpdir, ".ssh") + err := os.Mkdir(dotssh, 0o700) + require.NoError(t, err) + n1 := filepath.Join(dotssh, "config") + n2 := filepath.Join(dotssh, "coder") + return n1, n2 +} + +func sshConfigFileCreate(t *testing.T, name string, data io.Reader) { + t.Helper() + t.Logf("Writing %s", name) + f, err := os.Create(name) + require.NoError(t, err) + n, err := io.Copy(f, data) + t.Logf("Wrote %d", n) + require.NoError(t, err) + err = f.Close() + require.NoError(t, err) +} + +func sshConfigFileRead(t *testing.T, name string) string { + t.Helper() + b, err := os.ReadFile(name) + require.NoError(t, err) + return string(b) +} + func TestConfigSSH(t *testing.T) { t.Parallel() @@ -78,13 +109,6 @@ func TestConfigSSH(t *testing.T) { t.Cleanup(func() { _ = agentCloser.Close() }) - tmpdir := t.TempDir() - tempFile, err := os.CreateTemp(tmpdir, "config") - require.NoError(t, err) - _ = tempFile.Close() - coderTempFile, err := os.CreateTemp(tmpdir, "coder") - require.NoError(t, err) - _ = coderTempFile.Close() resources := coderdtest.AwaitWorkspaceAgents(t, client, workspace.LatestBuild.ID) agentConn, err := client.DialWorkspaceAgent(context.Background(), resources[0].Agents[0].ID, nil) require.NoError(t, err) @@ -111,61 +135,52 @@ func TestConfigSSH(t *testing.T) { _ = listener.Close() }) + sshConfigFile, coderConfigFile := sshConfigFileNames(t) + tcpAddr, valid := listener.Addr().(*net.TCPAddr) require.True(t, valid) cmd, root := clitest.New(t, "config-ssh", "--ssh-option", "HostName "+tcpAddr.IP.String(), "--ssh-option", "Port "+strconv.Itoa(tcpAddr.Port), - "--ssh-config-file", tempFile.Name(), - "--ssh-coder-config-file", coderTempFile.Name(), + "--ssh-config-file", sshConfigFile, + "--test.ssh-coder-config-file", coderConfigFile, "--skip-proxy-command") clitest.SetupConfig(t, client, root) doneChan := make(chan struct{}) pty := ptytest.New(t) cmd.SetIn(pty.Input()) - cmd.SetOut(pty.Output()) + cmd.SetOut(io.MultiWriter(pty.Output(), os.Stderr)) go func() { defer close(doneChan) err := cmd.Execute() assert.NoError(t, err) }() + + matches := []struct { + match, write string + }{ + {match: "Continue?", write: "yes"}, + } + for _, m := range matches { + pty.ExpectMatch(m.match) + pty.WriteLine(m.write) + } + <-doneChan - t.Log(tempFile.Name()) + t.Log(coderConfigFile) + t.Log(sshConfigFileRead(t, coderConfigFile)) + home := filepath.Dir(filepath.Dir(sshConfigFile)) // #nosec - sshCmd := exec.Command("ssh", "-F", tempFile.Name(), "coder."+workspace.Name, "echo", "test") + sshCmd := exec.Command("ssh", "-F", sshConfigFile, "coder."+workspace.Name, "echo", "test") + // Set HOME because coder config is included from ~/.ssh/coder. + sshCmd.Env = append(sshCmd.Env, fmt.Sprintf("HOME=%s", home)) sshCmd.Stderr = os.Stderr data, err := sshCmd.Output() require.NoError(t, err) require.Equal(t, "test", strings.TrimSpace(string(data))) } -func sshConfigFileNames(t *testing.T) (sshConfig string, coderConfig string) { - t.Helper() - tmpdir := t.TempDir() - n1 := filepath.Join(tmpdir, "config") - n2 := filepath.Join(tmpdir, "coder") - return n1, n2 -} - -func sshConfigFileCreate(t *testing.T, name string, data io.Reader) { - t.Helper() - t.Logf("Writing %s", name) - f, err := os.Create(name) - require.NoError(t, err) - n, err := io.Copy(f, data) - t.Logf("Wrote %d", n) - require.NoError(t, err) - err = f.Close() - require.NoError(t, err) -} - -func sshConfigFileRead(t *testing.T, name string) string { - b, err := os.ReadFile(name) - require.NoError(t, err) - return string(b) -} - func TestConfigSSH_FileWriteAndOptionsFlow(t *testing.T) { t.Parallel() From 99693ced45be0aca3c92d599d4dede05aa55095b Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 1 Jun 2022 19:38:04 +0300 Subject: [PATCH 29/38] fix: Revert multiwriter change usued while testing --- cli/configssh_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/configssh_test.go b/cli/configssh_test.go index 7d3133e75164c..1ab2e7e52dda9 100644 --- a/cli/configssh_test.go +++ b/cli/configssh_test.go @@ -149,7 +149,7 @@ func TestConfigSSH(t *testing.T) { doneChan := make(chan struct{}) pty := ptytest.New(t) cmd.SetIn(pty.Input()) - cmd.SetOut(io.MultiWriter(pty.Output(), os.Stderr)) + cmd.SetOut(pty.Output()) go func() { defer close(doneChan) err := cmd.Execute() From fb24742e5e6b8876be63c4bbdb9de5d26d1af39d Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 1 Jun 2022 20:07:41 +0300 Subject: [PATCH 30/38] fix: Change diff suggestions for more natural sentence --- cli/configssh.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/configssh.go b/cli/configssh.go index ab97039f67dee..4fbdebeaf22c1 100644 --- a/cli/configssh.go +++ b/cli/configssh.go @@ -355,7 +355,7 @@ func configSSH() *cobra.Command { // configuration, so we output the entire command. diffCommand := fmt.Sprintf("$ %s %s", cmd.CommandPath(), strings.Join(append(coderConfig.asArgs(), "--diff"), " ")) _, err = cliui.Prompt(cmd, cliui.PromptOptions{ - Text: fmt.Sprintf("The following changes will be made to your SSH configuration:\n\n * %s\n\n To see changes, run with --diff:\n\n %s\n\n Continue?", strings.Join(changes, "\n * "), diffCommand), + Text: fmt.Sprintf("The following changes will be made to your SSH configuration:\n\n * %s\n\n To see changes, run diff:\n\n %s\n\n Continue?", strings.Join(changes, "\n * "), diffCommand), IsConfirm: true, }) if err != nil { From 5c785dd26adb416b8cb182fac56b57a2112d11a6 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 1 Jun 2022 20:12:17 +0300 Subject: [PATCH 31/38] fix: Typo --- cli/configssh.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/configssh.go b/cli/configssh.go index 4fbdebeaf22c1..6e1aee30a2957 100644 --- a/cli/configssh.go +++ b/cli/configssh.go @@ -194,7 +194,7 @@ func configSSH() *cobra.Command { newOptsMsg = fmt.Sprintf("\n\n New options:\n * %s", strings.Join(newOpts, "\n * ")) } oldOpts := lastCoderConfig.asList() - oldOptsMsg := "\n\n Previouss options: none" + oldOptsMsg := "\n\n Previous options: none" if len(oldOpts) > 0 { oldOptsMsg = fmt.Sprintf("\n\n Previous options:\n * %s", strings.Join(oldOpts, "\n * ")) } From c60762f845cb9b89553ace85283b74b3059701a2 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 1 Jun 2022 20:34:47 +0300 Subject: [PATCH 32/38] fix: Fix linting (gocyclo, revive), improve responsiveness --- cli/configssh.go | 118 ++++++++++++++++++++++++++++------------------- 1 file changed, 71 insertions(+), 47 deletions(-) diff --git a/cli/configssh.go b/cli/configssh.go index 6e1aee30a2957..f7dbf1a462db8 100644 --- a/cli/configssh.go +++ b/cli/configssh.go @@ -3,6 +3,7 @@ package cli import ( "bufio" "bytes" + "context" "errors" "fmt" "io" @@ -97,6 +98,67 @@ func (o sshCoderConfigOptions) asList() (list []string) { return list } +type sshWorkspaceConfig struct { + Name string + Hosts []string +} + +func sshPrepareWorkspaceConfigs(ctx context.Context, client *codersdk.Client) (receive func() ([]sshWorkspaceConfig, error)) { + wcC := make(chan []sshWorkspaceConfig, 1) + errC := make(chan error, 1) + go func() { + wc, err := func() ([]sshWorkspaceConfig, error) { + workspaces, err := client.Workspaces(ctx, codersdk.WorkspaceFilter{ + Owner: codersdk.Me, + }) + if err != nil { + return nil, err + } + + var errGroup errgroup.Group + workspaceConfigs := make([]sshWorkspaceConfig, len(workspaces)) + for i, workspace := range workspaces { + i := i + workspace := workspace + errGroup.Go(func() error { + resources, err := client.TemplateVersionResources(ctx, workspace.LatestBuild.TemplateVersionID) + if err != nil { + return err + } + + wc := sshWorkspaceConfig{Name: workspace.Name} + for _, resource := range resources { + if resource.Transition != codersdk.WorkspaceTransitionStart { + continue + } + for _, agent := range resource.Agents { + hostname := workspace.Name + if len(resource.Agents) > 1 { + hostname += "." + agent.Name + } + wc.Hosts = append(wc.Hosts, hostname) + } + } + workspaceConfigs[i] = wc + + return nil + }) + } + err = errGroup.Wait() + if err != nil { + return nil, err + } + + return workspaceConfigs, nil + }() + wcC <- wc + errC <- err + }() + return func() ([]sshWorkspaceConfig, error) { + return <-wcC, <-errC + } +} + func configSSH() *cobra.Command { var ( coderConfig sshCoderConfigOptions @@ -132,13 +194,7 @@ func configSSH() *cobra.Command { return err } - // Early check for workspaces to ensure API key has not expired. - workspaces, err := client.Workspaces(cmd.Context(), codersdk.WorkspaceFilter{ - Owner: codersdk.Me, - }) - if err != nil { - return err - } + recvWorkspaceConfigs := sshPrepareWorkspaceConfigs(cmd.Context(), client) out := cmd.OutOrStdout() if showDiff { @@ -172,6 +228,7 @@ func configSSH() *cobra.Command { coderConfigExists := true coderConfigRaw, err := os.ReadFile(coderConfigFile) if err != nil { + //nolint: revive // Inverting this if statement doesn't improve readability. if errors.Is(err, fs.ErrNotExist) { coderConfigExists = false } else { @@ -232,43 +289,6 @@ func configSSH() *cobra.Command { } root := createConfig(cmd) - var errGroup errgroup.Group - type workspaceConfig struct { - Name string - Hosts []string - } - workspaceConfigs := make([]workspaceConfig, len(workspaces)) - for i, workspace := range workspaces { - i := i - workspace := workspace - errGroup.Go(func() error { - resources, err := client.TemplateVersionResources(cmd.Context(), workspace.LatestBuild.TemplateVersionID) - if err != nil { - return err - } - - wc := workspaceConfig{Name: workspace.Name} - for _, resource := range resources { - if resource.Transition != codersdk.WorkspaceTransitionStart { - continue - } - for _, agent := range resource.Agents { - hostname := workspace.Name - if len(resource.Agents) > 1 { - hostname += "." + agent.Name - } - wc.Hosts = append(wc.Hosts, hostname) - } - } - workspaceConfigs[i] = wc - - return nil - }) - } - err = errGroup.Wait() - if err != nil { - return err - } buf := &bytes.Buffer{} @@ -279,8 +299,12 @@ func configSSH() *cobra.Command { return xerrors.Errorf("write coder config header failed: %w", err) } + workspaceConfigs, err := recvWorkspaceConfigs() + if err != nil { + return xerrors.Errorf("fetch workspace configs failed: %w", err) + } // Ensure stable sorting of output. - slices.SortFunc(workspaceConfigs, func(a, b workspaceConfig) bool { + slices.SortFunc(workspaceConfigs, func(a, b sshWorkspaceConfig) bool { return a.Name < b.Name }) for _, wc := range workspaceConfigs { @@ -377,9 +401,9 @@ func configSSH() *cobra.Command { } } - if len(workspaces) > 0 { + if len(workspaceConfigs) > 0 { _, _ = fmt.Fprintln(out, "You should now be able to ssh into your workspace.") - _, _ = fmt.Fprintf(out, "For example, try running:\n\n\t$ ssh coder.%s\n\n", workspaces[0].Name) + _, _ = fmt.Fprintf(out, "For example, try running:\n\n\t$ ssh coder.%s\n\n", workspaceConfigs[0].Name) } else { _, _ = fmt.Fprint(out, "You don't have any workspaces yet, try creating one with:\n\n\t$ coder create \n\n") } From 0422e71c95d827f0210fee9b79d4767cf0ecf0dd Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 6 Jun 2022 19:08:26 +0300 Subject: [PATCH 33/38] chore: Update header comment --- cli/configssh.go | 4 ++-- cli/configssh_test.go | 16 ++++++++-------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/cli/configssh.go b/cli/configssh.go index f7dbf1a462db8..d813d58c15549 100644 --- a/cli/configssh.go +++ b/cli/configssh.go @@ -34,8 +34,8 @@ const ( sshCoderConfigHeader = "# This file is managed by coder. DO NOT EDIT." sshCoderConfigDocsHeader = ` # -# You should not hand-edit this file, all changes will be lost upon workspace -# creation, deletion or when running "coder config-ssh".` +# You should not hand-edit this file, all changes will be lost when running +# "coder config-ssh".` sshCoderConfigOptionsHeader = ` # # Last config-ssh options: diff --git a/cli/configssh_test.go b/cli/configssh_test.go index 1ab2e7e52dda9..c99830f5c0fae 100644 --- a/cli/configssh_test.go +++ b/cli/configssh_test.go @@ -301,8 +301,8 @@ func TestConfigSSH_FileWriteAndOptionsFlow(t *testing.T) { coder: strings.Join([]string{ "# This file is managed by coder. DO NOT EDIT.", "#", - "# You should not hand-edit this file, all changes will be lost upon workspace", - "# creation, deletion or when running \"coder config-ssh\".", + "# You should not hand-edit this file, all changes will be lost when running", + "# \"coder config-ssh\".", "#", "# Last config-ssh options:", "# :ssh-option=ForwardAgent=yes", @@ -313,8 +313,8 @@ func TestConfigSSH_FileWriteAndOptionsFlow(t *testing.T) { coder: strings.Join([]string{ "# This file is managed by coder. DO NOT EDIT.", "#", - "# You should not hand-edit this file, all changes will be lost upon workspace", - "# creation, deletion or when running \"coder config-ssh\".", + "# You should not hand-edit this file, all changes will be lost when running", + "# \"coder config-ssh\".", "#", "# Last config-ssh options:", "#", @@ -332,8 +332,8 @@ func TestConfigSSH_FileWriteAndOptionsFlow(t *testing.T) { coder: strings.Join([]string{ "# This file is managed by coder. DO NOT EDIT.", "#", - "# You should not hand-edit this file, all changes will be lost upon workspace", - "# creation, deletion or when running \"coder config-ssh\".", + "# You should not hand-edit this file, all changes will be lost when running", + "# \"coder config-ssh\".", "#", "# Last config-ssh options:", "# :ssh-option=ForwardAgent=yes", @@ -344,8 +344,8 @@ func TestConfigSSH_FileWriteAndOptionsFlow(t *testing.T) { coder: strings.Join([]string{ "# This file is managed by coder. DO NOT EDIT.", "#", - "# You should not hand-edit this file, all changes will be lost upon workspace", - "# creation, deletion or when running \"coder config-ssh\".", + "# You should not hand-edit this file, all changes will be lost when running", + "# \"coder config-ssh\".", "#", "# Last config-ssh options:", "# :ssh-option=ForwardAgent=yes", From 6e1e1a06a8f5fa31c17f53f5f15b06862c05d6aa Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 6 Jun 2022 19:11:30 +0300 Subject: [PATCH 34/38] chore: Extract function --- cli/configssh.go | 84 +++++++++++++++++++++++++----------------------- 1 file changed, 43 insertions(+), 41 deletions(-) diff --git a/cli/configssh.go b/cli/configssh.go index d813d58c15549..84b4dc619835a 100644 --- a/cli/configssh.go +++ b/cli/configssh.go @@ -103,54 +103,56 @@ type sshWorkspaceConfig struct { Hosts []string } -func sshPrepareWorkspaceConfigs(ctx context.Context, client *codersdk.Client) (receive func() ([]sshWorkspaceConfig, error)) { - wcC := make(chan []sshWorkspaceConfig, 1) - errC := make(chan error, 1) - go func() { - wc, err := func() ([]sshWorkspaceConfig, error) { - workspaces, err := client.Workspaces(ctx, codersdk.WorkspaceFilter{ - Owner: codersdk.Me, - }) +func fetchWorkspaceConfigs(ctx context.Context, client *codersdk.Client) ([]sshWorkspaceConfig, error) { + workspaces, err := client.Workspaces(ctx, codersdk.WorkspaceFilter{ + Owner: codersdk.Me, + }) + if err != nil { + return nil, err + } + + var errGroup errgroup.Group + workspaceConfigs := make([]sshWorkspaceConfig, len(workspaces)) + for i, workspace := range workspaces { + i := i + workspace := workspace + errGroup.Go(func() error { + resources, err := client.TemplateVersionResources(ctx, workspace.LatestBuild.TemplateVersionID) if err != nil { - return nil, err + return err } - var errGroup errgroup.Group - workspaceConfigs := make([]sshWorkspaceConfig, len(workspaces)) - for i, workspace := range workspaces { - i := i - workspace := workspace - errGroup.Go(func() error { - resources, err := client.TemplateVersionResources(ctx, workspace.LatestBuild.TemplateVersionID) - if err != nil { - return err + wc := sshWorkspaceConfig{Name: workspace.Name} + for _, resource := range resources { + if resource.Transition != codersdk.WorkspaceTransitionStart { + continue + } + for _, agent := range resource.Agents { + hostname := workspace.Name + if len(resource.Agents) > 1 { + hostname += "." + agent.Name } + wc.Hosts = append(wc.Hosts, hostname) + } + } + workspaceConfigs[i] = wc - wc := sshWorkspaceConfig{Name: workspace.Name} - for _, resource := range resources { - if resource.Transition != codersdk.WorkspaceTransitionStart { - continue - } - for _, agent := range resource.Agents { - hostname := workspace.Name - if len(resource.Agents) > 1 { - hostname += "." + agent.Name - } - wc.Hosts = append(wc.Hosts, hostname) - } - } - workspaceConfigs[i] = wc + return nil + }) + } + err = errGroup.Wait() + if err != nil { + return nil, err + } - return nil - }) - } - err = errGroup.Wait() - if err != nil { - return nil, err - } + return workspaceConfigs, nil +} - return workspaceConfigs, nil - }() +func sshPrepareWorkspaceConfigs(ctx context.Context, client *codersdk.Client) (receive func() ([]sshWorkspaceConfig, error)) { + wcC := make(chan []sshWorkspaceConfig, 1) + errC := make(chan error, 1) + go func() { + wc, err := fetchWorkspaceConfigs(ctx, client) wcC <- wc errC <- err }() From fda935fcb103889798454b62920dc83873ce861e Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 6 Jun 2022 19:13:59 +0300 Subject: [PATCH 35/38] chore: Remove test log statements --- cli/configssh_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/cli/configssh_test.go b/cli/configssh_test.go index c99830f5c0fae..a8c1239d889df 100644 --- a/cli/configssh_test.go +++ b/cli/configssh_test.go @@ -168,8 +168,6 @@ func TestConfigSSH(t *testing.T) { <-doneChan - t.Log(coderConfigFile) - t.Log(sshConfigFileRead(t, coderConfigFile)) home := filepath.Dir(filepath.Dir(sshConfigFile)) // #nosec sshCmd := exec.Command("ssh", "-F", sshConfigFile, "coder."+workspace.Name, "echo", "test") From f1be4c6634554c8797961e4ba3fe943d38270f6a Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 8 Jun 2022 11:06:15 +0300 Subject: [PATCH 36/38] Make Include regexp stricter to avoid deleting user data --- cli/configssh.go | 12 ++++--- cli/configssh_test.go | 80 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+), 5 deletions(-) diff --git a/cli/configssh.go b/cli/configssh.go index 84b4dc619835a..6ecd011491a80 100644 --- a/cli/configssh.go +++ b/cli/configssh.go @@ -54,11 +54,13 @@ var ( // Find the semantically correct include statement. Since the user can // modify their configuration as they see fit, there could be: // - Leading indentation (space, tab) - // - Trailing indentation (space, tab), followed by e.g. a comment or - // another file to Include (we don't want to support this, but - // explicitly blocking it adds complexity) - // - Select newline after Include statement for removal purposes - sshCoderIncludedRe = regexp.MustCompile(`(?m)^[\t ]*((?i)Include) coder([\t ].*)?[\r]?[\n]?$`) + // - Trailing indentation (space, tab) + // - Select newline after Include statement for cleaner removal + // In the following cases, we will not recognize the Include statement + // and leave as-is (i.e. they're not supported): + // - User adds another file to the Include statement + // - User adds a comment on the same line as the Include statement + sshCoderIncludedRe = regexp.MustCompile(`(?m)^[\t ]*((?i)Include) coder[\t ]*[\r]?[\n]?$`) ) // sshCoderConfigOptions represents options that can be stored and read diff --git a/cli/configssh_test.go b/cli/configssh_test.go index a8c1239d889df..7ee37964e9aa1 100644 --- a/cli/configssh_test.go +++ b/cli/configssh_test.go @@ -267,6 +267,86 @@ func TestConfigSSH_FileWriteAndOptionsFlow(t *testing.T) { {match: "Continue?", write: "yes"}, }, }, + { + name: "Included file must be named exactly coder, otherwise leave as-is", + writeConfig: writeConfig{ + ssh: strings.Join([]string{ + "Host test", + " HostName test", + "", + "Include coders", + "", + }, "\n"), + }, + wantConfig: wantConfig{ + ssh: strings.Join([]string{ + "Include coder", + "", + "Host test", + " HostName test", + "", + "Include coders", + "", + }, "\n"), + }, + matches: []match{ + {match: "Continue?", write: "yes"}, + }, + }, + { + name: "Second file added, Include(s) left as-is, new one on top", + writeConfig: writeConfig{ + ssh: strings.Join([]string{ + "Host test", + " HostName test", + "", + "Include coder other", + "Include other coder", + "", + }, "\n"), + }, + wantConfig: wantConfig{ + ssh: strings.Join([]string{ + "Include coder", + "", + "Host test", + " HostName test", + "", + "Include coder other", + "Include other coder", + "", + }, "\n"), + }, + matches: []match{ + {match: "Continue?", write: "yes"}, + }, + }, + { + name: "Comment added, Include left as-is, new one on top", + writeConfig: writeConfig{ + ssh: strings.Join([]string{ + "Host test", + " HostName test", + "", + "Include coder # comment", + "", + }, "\n"), + }, + wantConfig: wantConfig{ + ssh: strings.Join([]string{ + "Include coder", + "", + "Host test", + " HostName test", + "", + "Include coder # comment", + "", + }, "\n"), + }, + matches: []match{ + {match: "Continue?", write: "yes"}, + }, + }, { name: "SSH Config does not need modification", writeConfig: writeConfig{ From bf4596c2089829bf2ae5bf1f1b41fba5b792ab79 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 8 Jun 2022 11:20:05 +0300 Subject: [PATCH 37/38] chore: Update the "Changes" text for diff to match non-diff mode --- cli/configssh.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/configssh.go b/cli/configssh.go index 6ecd011491a80..17e8db73b539c 100644 --- a/cli/configssh.go +++ b/cli/configssh.go @@ -353,7 +353,7 @@ func configSSH() *cobra.Command { if showDiff { if len(changes) > 0 { // Write to stderr to avoid dirtying the diff output. - _, _ = fmt.Fprint(out, "Changes:\n\n") + _, _ = fmt.Fprint(out, "The following changes will be made to your SSH configuration:\n\n") for _, change := range changes { _, _ = fmt.Fprintf(out, " * %s\n", change) } From 093e2ce77309e5ea21170a4b58f3f67b4efdcd66 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 8 Jun 2022 11:23:13 +0300 Subject: [PATCH 38/38] chore: Use consistent naming of functions --- cli/configssh.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cli/configssh.go b/cli/configssh.go index 17e8db73b539c..f6d1af7666231 100644 --- a/cli/configssh.go +++ b/cli/configssh.go @@ -105,7 +105,7 @@ type sshWorkspaceConfig struct { Hosts []string } -func fetchWorkspaceConfigs(ctx context.Context, client *codersdk.Client) ([]sshWorkspaceConfig, error) { +func sshFetchWorkspaceConfigs(ctx context.Context, client *codersdk.Client) ([]sshWorkspaceConfig, error) { workspaces, err := client.Workspaces(ctx, codersdk.WorkspaceFilter{ Owner: codersdk.Me, }) @@ -154,7 +154,7 @@ func sshPrepareWorkspaceConfigs(ctx context.Context, client *codersdk.Client) (r wcC := make(chan []sshWorkspaceConfig, 1) errC := make(chan error, 1) go func() { - wc, err := fetchWorkspaceConfigs(ctx, client) + wc, err := sshFetchWorkspaceConfigs(ctx, client) wcC <- wc errC <- err }()