From 0ee2b656c10ea9ef7cb54d0c61220d6b9353ff63 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 15 Jun 2022 16:10:52 +0300 Subject: [PATCH 1/2] fix: Revert to old SSH config section management in config-ssh This commit partially reverts #1900 and removes the separate `~/.ssh/coder` config file whilst keeping most other features. This will allow us to remain more compatible with different IDEs. Fixes #2317 --- cli/configssh.go | 301 +++++++++++++++++------------------------- cli/configssh_old.go | 66 +++++++++ cli/configssh_test.go | 294 ++++++++++++++++++++++++++--------------- 3 files changed, 376 insertions(+), 285 deletions(-) create mode 100644 cli/configssh_old.go diff --git a/cli/configssh.go b/cli/configssh.go index f6d1af7666231..32333936b65d3 100644 --- a/cli/configssh.go +++ b/cli/configssh.go @@ -10,7 +10,6 @@ import ( "io/fs" "os" "path/filepath" - "regexp" "runtime" "sort" "strings" @@ -29,71 +28,43 @@ import ( ) const ( - sshDefaultConfigFileName = "~/.ssh/config" - sshDefaultCoderConfigFileName = "~/.ssh/coder" - sshCoderConfigHeader = "# This file is managed by coder. DO NOT EDIT." - sshCoderConfigDocsHeader = ` -# -# You should not hand-edit this file, all changes will be lost when running -# "coder config-ssh".` - sshCoderConfigOptionsHeader = ` + sshDefaultConfigFileName = "~/.ssh/config" + sshStartToken = "# ------------START-CODER-----------" + sshEndToken = "# ------------END-CODER------------" + sshConfigSectionHeader = "# This section is managed by coder. DO NOT EDIT." + sshConfigDocsHeader = ` # +# You should not hand-edit this section unless you are removing it, all +# changes will be lost when running "coder config-ssh". +` + sshConfigOptionsHeader = `# # Last config-ssh options: ` - // Relative paths are assumed to be in ~/.ssh, except when - // included in /etc/ssh. - 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(`(?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) - // - 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 +// sshConfigOptions represents options that can be stored and read // from the coder config in ~/.ssh/coder. -type sshCoderConfigOptions struct { - sshConfigDefaultFile string - sshConfigFile string - sshOptions []string +type sshConfigOptions struct { + sshOptions []string } -func (o sshCoderConfigOptions) equal(other sshCoderConfigOptions) bool { +func (o sshConfigOptions) equal(other sshConfigOptions) 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) + return slices.Equal(opt1, opt2) } -func (o sshCoderConfigOptions) asArgs() (args []string) { - if o.sshConfigFile != o.sshConfigDefaultFile { - args = append(args, "--ssh-config-file", o.sshConfigFile) - } +func (o sshConfigOptions) asArgs() (args []string) { 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 != o.sshConfigDefaultFile { - list = append(list, fmt.Sprintf("ssh-config-file: %s", o.sshConfigFile)) - } +func (o sshConfigOptions) asList() (list []string) { for _, opt := range o.sshOptions { list = append(list, fmt.Sprintf("ssh-option: %s", opt)) } @@ -165,7 +136,8 @@ func sshPrepareWorkspaceConfigs(ctx context.Context, client *codersdk.Client) (r func configSSH() *cobra.Command { var ( - coderConfig sshCoderConfigOptions + sshConfigFile string + sshConfigOpts sshConfigOptions coderConfigFile string showDiff bool skipProxyCommand bool @@ -187,7 +159,6 @@ func configSSH() *cobra.Command { ` + 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 } @@ -209,17 +180,14 @@ func configSSH() *cobra.Command { return err } - dirname, err := os.UserHomeDir() + homedir, err := os.UserHomeDir() if err != nil { return xerrors.Errorf("user home dir failed: %w", err) } - sshConfigFile := coderConfig.sshConfigFile + sshConfigFileOrig := sshConfigFile if strings.HasPrefix(sshConfigFile, "~/") { - sshConfigFile = filepath.Join(dirname, sshConfigFile[2:]) - } - if strings.HasPrefix(coderConfigFile, "~/") { - coderConfigFile = filepath.Join(dirname, coderConfigFile[2:]) + sshConfigFile = filepath.Join(homedir, sshConfigFile[2:]) } // Only allow not-exist errors to avoid trashing @@ -229,32 +197,29 @@ func configSSH() *cobra.Command { return xerrors.Errorf("read ssh config failed: %w", err) } - 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 { - 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 + + lastConfig := sshConfigParseLastOptions(bytes.NewReader(configRaw)) + + // Deprecated: Remove after migration period. + var ok bool + var coderConfigRaw []byte + if coderConfigFile, coderConfigRaw, ok = readDeprecatedCoderConfigFile(homedir, coderConfigFile); ok { + changes = append(changes, fmt.Sprintf("Remove old auto-generated coder config file at %s", coderConfigFile)) + // Backwards compate, restore old options. + lastConfig = sshConfigParseLastOptions(bytes.NewReader(coderConfigRaw)) } - lastCoderConfig := sshCoderConfigParseLastOptions(bytes.NewReader(coderConfigRaw), coderConfig.sshConfigDefaultFile) // Avoid prompting in diff mode (unexpected behavior) // or when a previous config does not exist. - if !showDiff && !coderConfig.equal(lastCoderConfig) && coderConfigExists { - newOpts := coderConfig.asList() + if !showDiff && !sshConfigOpts.equal(lastConfig) { + newOpts := sshConfigOpts.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() + oldOpts := lastConfig.asList() oldOptsMsg := "\n\n Previous options: none" if len(oldOpts) > 0 { oldOptsMsg = fmt.Sprintf("\n\n Previous options:\n * %s", strings.Join(oldOpts, "\n * ")) @@ -265,43 +230,35 @@ func configSSH() *cobra.Command { 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 } // Selecting "no" will use the last config. - coderConfig = lastCoderConfig + sshConfigOpts = lastConfig } _, _ = fmt.Fprint(out, "\n") } - // Keep track of changes we are making. - var changes []string - - // Check for presence of old config format and - // remove if present. - configModified, ok := stripOldConfigBlock(configRaw) - if ok { - changes = append(changes, fmt.Sprintf("Remove old config block (START-CODER/END-CODER) from %s", sshConfigFile)) - } + configModified := configRaw // 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", sshConfigFile)) + // Deprecated: Remove after migration period. + if configModified, ok = removeDeprecatedSSHIncludeStatement(configModified); ok { + changes = append(changes, fmt.Sprintf("Remove %q from %s", "Include coder", sshConfigFile)) } root := createConfig(cmd) buf := &bytes.Buffer{} + before, after := sshConfigSplitOnCoderSection(configModified) + // Write the first half of the users config file to buf. + _, _ = buf.Write(before) - // Write header and store the provided options as part + // Write comment 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) - } + newline := len(before) > 0 + sshConfigWriteSectionHeader(buf, newline, sshConfigOpts) workspaceConfigs, err := recvWorkspaceConfigs() if err != nil { @@ -318,7 +275,7 @@ func configSSH() *cobra.Command { configOptions := []string{ "Host coder." + hostname, } - for _, option := range coderConfig.sshOptions { + for _, option := range sshConfigOpts.sshOptions { configOptions = append(configOptions, "\t"+option) } configOptions = append(configOptions, @@ -341,13 +298,14 @@ 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", coderConfigFile)) - } else { - changes = append(changes, fmt.Sprintf("Update auto-generated coder config file in %s", coderConfigFile)) - } + sshConfigWriteSectionEnd(buf) + + // Write the remainder of the users config file to buf. + _, _ = buf.Write(after) + + if !bytes.Equal(configModified, buf.Bytes()) { + changes = append(changes, fmt.Sprintf("Update coder config section in %s", sshConfigFile)) + configModified = buf.Bytes() } if showDiff { @@ -360,10 +318,15 @@ func configSSH() *cobra.Command { } color := isTTYOut(cmd) - for _, diffFn := range []func() ([]byte, error){ + diffFns := []func() ([]byte, error){ func() ([]byte, error) { return diffBytes(sshConfigFile, configRaw, configModified, color) }, - func() ([]byte, error) { return diffBytes(coderConfigFile, coderConfigRaw, buf.Bytes(), color) }, - } { + } + if len(coderConfigRaw) > 0 { + // Deprecated: Remove after migration period. + diffFns = append(diffFns, func() ([]byte, error) { return diffBytes(coderConfigFile, coderConfigRaw, nil, color) }) + } + + for _, diffFn := range diffFns { diff, err := diffFn() if err != nil { return xerrors.Errorf("diff failed: %w", err) @@ -381,7 +344,13 @@ 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"), " ")) + var args []string + if sshConfigFileOrig != sshDefaultConfigFileName { + args = append(args, "--ssh-config-file", sshConfigFileOrig) + } + args = append(args, sshConfigOpts.asArgs()...) + args = append(args, "--diff") + diffCommand := fmt.Sprintf("$ %s %s", cmd.CommandPath(), strings.Join(args, " ")) _, 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 diff:\n\n %s\n\n Continue?", strings.Join(changes, "\n * "), diffCommand), IsConfirm: true, @@ -397,10 +366,11 @@ func configSSH() *cobra.Command { return xerrors.Errorf("write ssh config failed: %w", err) } } - if modifyCoderConfig { - err := writeWithTempFileAndMove(coderConfigFile, buf) + // Deprecated: Remove after migration period. + if len(coderConfigRaw) > 0 { + err = os.Remove(coderConfigFile) if err != nil { - return xerrors.Errorf("write coder ssh config failed: %w", err) + return xerrors.Errorf("remove coder config failed: %w", err) } } } @@ -414,73 +384,42 @@ func configSSH() *cobra.Command { return nil }, } - cliflag.StringVarP(cmd.Flags(), &coderConfig.sshConfigFile, "ssh-config-file", "", "CODER_SSH_CONFIG_FILE", sshDefaultConfigFileName, "Specifies the path to an SSH config.") - 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.") + cliflag.StringVarP(cmd.Flags(), &sshConfigFile, "ssh-config-file", "", "CODER_SSH_CONFIG_FILE", sshDefaultConfigFileName, "Specifies the path to an SSH config.") + cmd.Flags().StringArrayVarP(&sshConfigOpts.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 -} - -// sshConfigAddCoderInclude checks for the coder Include statement and -// returns modified = true if it was added. -func sshConfigAddCoderInclude(data []byte) (modifiedData []byte, modified bool) { - 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. - 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 { - valid = true - } - if valid { - 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...) + // Deprecated: Remove after migration period. + 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") - return data, true + return cmd } -func sshCoderConfigWriteHeader(w io.Writer, o sshCoderConfigOptions) error { - _, _ = fmt.Fprint(w, sshCoderConfigHeader) - _, _ = fmt.Fprint(w, sshCoderConfigDocsHeader) - _, _ = fmt.Fprint(w, sshCoderConfigOptionsHeader) - if o.sshConfigFile != o.sshConfigDefaultFile { - _, _ = fmt.Fprintf(w, "# :%s=%s\n", "ssh-config-file", o.sshConfigFile) +//nolint:revive +func sshConfigWriteSectionHeader(w io.Writer, addNewline bool, o sshConfigOptions) { + nl := "\n" + if !addNewline { + nl = "" } - for _, opt := range o.sshOptions { - _, _ = fmt.Fprintf(w, "# :%s=%s\n", "ssh-option", opt) + _, _ = fmt.Fprint(w, nl+sshStartToken+"\n") + _, _ = fmt.Fprint(w, sshConfigSectionHeader) + _, _ = fmt.Fprint(w, sshConfigDocsHeader) + if len(o.sshOptions) > 0 { + _, _ = fmt.Fprint(w, sshConfigOptionsHeader) + 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, sshConfigDefaultFile string) (o sshCoderConfigOptions) { - o.sshConfigDefaultFile = sshConfigDefaultFile - o.sshConfigFile = sshConfigDefaultFile // Default value is not written. +func sshConfigWriteSectionEnd(w io.Writer) { + _, _ = fmt.Fprint(w, sshEndToken+"\n") +} +func sshConfigParseLastOptions(r io.Reader) (o sshConfigOptions) { s := bufio.NewScanner(r) for s.Scan() { line := s.Text() @@ -488,8 +427,6 @@ func sshCoderConfigParseLastOptions(r io.Reader, sshConfigDefaultFile string) (o 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: @@ -504,6 +441,29 @@ func sshCoderConfigParseLastOptions(r io.Reader, sshConfigDefaultFile string) (o return o } +// sshConfigSplitOnCoderSection splits the SSH config into two sections, +// before contains the lines before sshStartToken and after contains the +// lines after sshEndToken. +func sshConfigSplitOnCoderSection(data []byte) (before, after []byte) { + startIndex := bytes.Index(data, []byte(sshStartToken)) + endIndex := bytes.Index(data, []byte(sshEndToken)) + if startIndex != -1 && endIndex != -1 { + // We use -1 and +1 here to also include the preceding + // and trailing newline, where applicable. + start := startIndex + if start > 0 { + start-- + } + end := endIndex + len(sshEndToken) + if end < len(data) { + end++ + } + return data[:start], data[end:] + } + + return data, nil +} + // 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 @@ -610,22 +570,3 @@ func diffBytes(name string, b1, b2 []byte, color bool) ([]byte, error) { } return b, nil } - -// 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_old.go b/cli/configssh_old.go new file mode 100644 index 0000000000000..1bb9ed11be6b3 --- /dev/null +++ b/cli/configssh_old.go @@ -0,0 +1,66 @@ +package cli + +import ( + "bytes" + "os" + "path/filepath" + "regexp" + "strings" +) + +// This file contains config-ssh definitions that are deprecated, they +// will be removed after a migratory period. + +const ( + sshDefaultCoderConfigFileName = "~/.ssh/coder" + sshCoderConfigHeader = "# This file is managed by coder. DO NOT EDIT." +) + +// Regular expressions are used because SSH configs do not have +// meaningful indentation and keywords are case-insensitive. +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) + // - 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]?$`) +) + +// removeDeprecatedSSHIncludeStatement checks for the Include coder statement +// and returns modified = true if it was removed. +func removeDeprecatedSSHIncludeStatement(data []byte) (modifiedData []byte, modified bool) { + coderInclude := sshCoderIncludedRe.FindIndex(data) + if coderInclude == nil { + return data, false + } + + // Remove Include statement. + d := append([]byte{}, data[:coderInclude[0]]...) + d = append(d, data[coderInclude[1]:]...) + data = d + + return data, true +} + +// readDeprecatedCoderConfigFile reads the deprecated split config file. +func readDeprecatedCoderConfigFile(homedir, coderConfigFile string) (name string, data []byte, ok bool) { + if strings.HasPrefix(coderConfigFile, "~/") { + coderConfigFile = filepath.Join(homedir, coderConfigFile[2:]) + } + + b, err := os.ReadFile(coderConfigFile) + if err != nil { + return coderConfigFile, nil, false + } + if len(b) > 0 { + if !bytes.HasPrefix(b, []byte(sshCoderConfigHeader)) { + return coderConfigFile, nil, false + } + } + return coderConfigFile, b, true +} diff --git a/cli/configssh_test.go b/cli/configssh_test.go index 7ee37964e9aa1..1e20be5b1447b 100644 --- a/cli/configssh_test.go +++ b/cli/configssh_test.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "io" + "io/fs" "net" "os" "os/exec" @@ -182,14 +183,27 @@ func TestConfigSSH(t *testing.T) { func TestConfigSSH_FileWriteAndOptionsFlow(t *testing.T) { t.Parallel() + headerStart := strings.Join([]string{ + "# ------------START-CODER-----------", + "# This section is managed by coder. DO NOT EDIT.", + "#", + "# You should not hand-edit this section unless you are removing it, all", + "# changes will be lost when running \"coder config-ssh\".", + "#", + }, "\n") + headerEnd := "# ------------END-CODER------------" + baseHeader := strings.Join([]string{ + headerStart, + headerEnd, + }, "\n") + type writeConfig struct { ssh string coder string } type wantConfig struct { - ssh string - coder string - coderPartial bool + ssh string + coderKept bool } type match struct { match, write string @@ -203,35 +217,31 @@ func TestConfigSSH_FileWriteAndOptionsFlow(t *testing.T) { wantErr bool }{ { - name: "Config files are created", + name: "Config file is created", matches: []match{ {match: "Continue?", write: "yes"}, }, wantConfig: wantConfig{ ssh: strings.Join([]string{ - "Include coder", + baseHeader, "", }, "\n"), - coder: "# This file is managed by coder. DO NOT EDIT.", - coderPartial: true, }, }, { - name: "Include is written to top of ssh config", + name: "Section is written after user content", writeConfig: writeConfig{ ssh: strings.Join([]string{ - "# This is a host", - "Host test", - " HostName test", + "Host myhost", + " HostName myhost", }, "\n"), }, wantConfig: wantConfig{ ssh: strings.Join([]string{ - "Include coder", + "Host myhost", + " HostName myhost", + baseHeader, "", - "# This is a host", - "Host test", - " HostName test", }, "\n"), }, matches: []match{ @@ -239,81 +249,80 @@ func TestConfigSSH_FileWriteAndOptionsFlow(t *testing.T) { }, }, { - name: "Include below Host is invalid, move it to the top", + name: "Section is not moved on re-run", writeConfig: writeConfig{ ssh: strings.Join([]string{ - "Host test", - " HostName test", + "Host myhost", + " HostName myhost", "", - "Include coder", + baseHeader, "", + "Host otherhost", + " HostName otherhost", "", }, "\n"), }, wantConfig: wantConfig{ ssh: strings.Join([]string{ - "Include coder", + "Host myhost", + " HostName myhost", "", - "Host test", - " HostName test", - "", - // Only "Include coder" with accompanying - // newline is removed. + baseHeader, "", + "Host otherhost", + " HostName otherhost", "", }, "\n"), }, - matches: []match{ - {match: "Continue?", write: "yes"}, - }, }, { - name: "Included file must be named exactly coder, otherwise leave as-is", + name: "Section is not moved on re-run with new options", writeConfig: writeConfig{ ssh: strings.Join([]string{ - "Host test", - " HostName test", + "Host myhost", + " HostName myhost", "", - "Include coders", + baseHeader, + "", + "Host otherhost", + " HostName otherhost", "", }, "\n"), }, wantConfig: wantConfig{ ssh: strings.Join([]string{ - "Include coder", + "Host myhost", + " HostName myhost", "", - "Host test", - " HostName test", + headerStart, + "# Last config-ssh options:", + "# :ssh-option=ForwardAgent=yes", + "#", + headerEnd, "", - "Include coders", + "Host otherhost", + " HostName otherhost", "", }, "\n"), }, + args: []string{ + "--ssh-option", "ForwardAgent=yes", + }, matches: []match{ + {match: "Use new options?", write: "yes"}, {match: "Continue?", write: "yes"}, }, }, { - name: "Second file added, Include(s) left as-is, new one on top", + name: "Adds newline at EOF", writeConfig: writeConfig{ ssh: strings.Join([]string{ - "Host test", - " HostName test", - "", - "Include coder other", - "Include other coder", - "", + baseHeader, }, "\n"), }, wantConfig: wantConfig{ ssh: strings.Join([]string{ - "Include coder", - "", - "Host test", - " HostName test", - "", - "Include coder other", - "Include other coder", + baseHeader, "", }, "\n"), }, @@ -322,91 +331,127 @@ func TestConfigSSH_FileWriteAndOptionsFlow(t *testing.T) { }, }, { - name: "Comment added, Include left as-is, new one on top", + name: "Prompt for new options when there are no previous options", writeConfig: writeConfig{ ssh: strings.Join([]string{ - "Host test", - " HostName test", - "", - "Include coder # comment", - "", + baseHeader, }, "\n"), }, wantConfig: wantConfig{ ssh: strings.Join([]string{ - "Include coder", - "", - "Host test", - " HostName test", - "", - "Include coder # comment", + headerStart, + "# Last config-ssh options:", + "# :ssh-option=ForwardAgent=yes", + "#", + headerEnd, "", }, "\n"), }, + args: []string{"--ssh-option", "ForwardAgent=yes"}, matches: []match{ + {match: "Use new options?", write: "yes"}, {match: "Continue?", write: "yes"}, }, }, { - name: "SSH Config does not need modification", + name: "Prompt for new options when there are previous options", writeConfig: writeConfig{ ssh: strings.Join([]string{ - "Include something/other", - "Include coder", - "", - "# This is a host", - "Host test", - " HostName test", + headerStart, + "# Last config-ssh options:", + "# :ssh-option=ForwardAgent=yes", + "#", + headerEnd, }, "\n"), }, wantConfig: wantConfig{ ssh: strings.Join([]string{ - "Include something/other", - "Include coder", + baseHeader, "", - "# This is a host", - "Host test", - " HostName test", }, "\n"), }, matches: []match{ + {match: "Use new options?", write: "yes"}, {match: "Continue?", write: "yes"}, }, }, { - name: "When options differ, selecting yes overwrites previous options", + name: "No prompt on no changes", 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 when running", - "# \"coder config-ssh\".", - "#", + ssh: strings.Join([]string{ + headerStart, "# Last config-ssh options:", "# :ssh-option=ForwardAgent=yes", "#", + headerEnd, + "", }, "\n"), }, wantConfig: wantConfig{ - coder: strings.Join([]string{ - "# This file is managed by coder. DO NOT EDIT.", + ssh: strings.Join([]string{ + headerStart, + "# Last config-ssh options:", + "# :ssh-option=ForwardAgent=yes", "#", - "# You should not hand-edit this file, all changes will be lost when running", - "# \"coder config-ssh\".", + headerEnd, + "", + }, "\n"), + }, + args: []string{"--ssh-option", "ForwardAgent=yes"}, + }, + { + name: "No changes when continue = no", + writeConfig: writeConfig{ + ssh: strings.Join([]string{ + headerStart, + "# Last config-ssh options:", + "# :ssh-option=ForwardAgent=yes", "#", + headerEnd, + "", + }, "\n"), + }, + wantConfig: wantConfig{ + ssh: strings.Join([]string{ + headerStart, "# Last config-ssh options:", + "# :ssh-option=ForwardAgent=yes", "#", + headerEnd, + "", }, "\n"), - coderPartial: true, }, + args: []string{"--ssh-option", "ForwardAgent=no"}, matches: []match{ {match: "Use new options?", write: "yes"}, - {match: "Continue?", write: "yes"}, + {match: "Continue?", write: "no"}, }, }, + + // Tests for deprecated split coder config. { - name: "When options differ, selecting no preserves previous options", + name: "Do not overwrite unknown coder config", writeConfig: writeConfig{ + ssh: strings.Join([]string{ + baseHeader, + "", + }, "\n"), + coder: strings.Join([]string{ + "We're no strangers to love", + "You know the rules and so do I (do I)", + }, "\n"), + }, + wantConfig: wantConfig{ + coderKept: true, + }, + }, + { + name: "Transfer options from coder to ssh config", + writeConfig: writeConfig{ + ssh: strings.Join([]string{ + "Include coder", + "", + }, "\n"), coder: strings.Join([]string{ "# This file is managed by coder. DO NOT EDIT.", "#", @@ -419,6 +464,27 @@ func TestConfigSSH_FileWriteAndOptionsFlow(t *testing.T) { }, "\n"), }, wantConfig: wantConfig{ + ssh: strings.Join([]string{ + headerStart, + "# Last config-ssh options:", + "# :ssh-option=ForwardAgent=yes", + "#", + headerEnd, + "", + }, "\n"), + }, + matches: []match{ + {match: "Use new options?", write: "no"}, + {match: "Continue?", write: "yes"}, + }, + }, + { + name: "Allow overwriting previous options from coder config", + writeConfig: writeConfig{ + ssh: strings.Join([]string{ + "Include coder", + "", + }, "\n"), coder: strings.Join([]string{ "# This file is managed by coder. DO NOT EDIT.", "#", @@ -429,28 +495,51 @@ func TestConfigSSH_FileWriteAndOptionsFlow(t *testing.T) { "# :ssh-option=ForwardAgent=yes", "#", }, "\n"), - coderPartial: true, + }, + wantConfig: wantConfig{ + ssh: strings.Join([]string{ + baseHeader, + "", + }, "\n"), }, matches: []match{ - {match: "Use new options?", write: "no"}, + {match: "Use new options?", write: "yes"}, {match: "Continue?", write: "yes"}, }, }, { - name: "Do not overwrite unknown coder config", + name: "Allow overwriting previous options from coder config when they differ", writeConfig: writeConfig{ + ssh: strings.Join([]string{ + "Include coder", + "", + }, "\n"), coder: strings.Join([]string{ - "We're no strangers to love", - "You know the rules and so do I (do I)", + "# This file is managed by coder. DO NOT EDIT.", + "#", + "# 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", + "#", }, "\n"), }, wantConfig: wantConfig{ - coder: strings.Join([]string{ - "We're no strangers to love", - "You know the rules and so do I (do I)", + ssh: strings.Join([]string{ + headerStart, + "# Last config-ssh options:", + "# :ssh-option=ForwardAgent=no", + "#", + headerEnd, + "", }, "\n"), }, - wantErr: true, + args: []string{"--ssh-option", "ForwardAgent=no"}, + matches: []match{ + {match: "Use new options?", write: "yes"}, + {match: "Continue?", write: "yes"}, + }, }, } for _, tt := range tests { @@ -480,7 +569,6 @@ func TestConfigSSH_FileWriteAndOptionsFlow(t *testing.T) { 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...) @@ -510,13 +598,9 @@ func TestConfigSSH_FileWriteAndOptionsFlow(t *testing.T) { 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) - } + if !tt.wantConfig.coderKept { + _, err := os.ReadFile(coderConfigName) + assert.ErrorIs(t, err, fs.ErrNotExist) } }) } From c13ff0463113bde53e2c65272ced5d69e1c319c8 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 15 Jun 2022 17:09:01 +0300 Subject: [PATCH 2/2] fix: Only prompt for previous options after first config-ssh --- cli/configssh.go | 26 ++++++++++++++++++++------ cli/configssh_test.go | 23 +++++++++++++++++++++-- 2 files changed, 41 insertions(+), 8 deletions(-) diff --git a/cli/configssh.go b/cli/configssh.go index 32333936b65d3..f908efc2e7056 100644 --- a/cli/configssh.go +++ b/cli/configssh.go @@ -200,20 +200,25 @@ func configSSH() *cobra.Command { // Keep track of changes we are making. var changes []string - lastConfig := sshConfigParseLastOptions(bytes.NewReader(configRaw)) - - // Deprecated: Remove after migration period. + // Parse the previous configuration only if config-ssh + // has been run previously. + var lastConfig *sshConfigOptions var ok bool var coderConfigRaw []byte if coderConfigFile, coderConfigRaw, ok = readDeprecatedCoderConfigFile(homedir, coderConfigFile); ok { + // Deprecated: Remove after migration period. changes = append(changes, fmt.Sprintf("Remove old auto-generated coder config file at %s", coderConfigFile)) // Backwards compate, restore old options. - lastConfig = sshConfigParseLastOptions(bytes.NewReader(coderConfigRaw)) + c := sshConfigParseLastOptions(bytes.NewReader(coderConfigRaw)) + lastConfig = &c + } else if section, ok := sshConfigGetCoderSection(configRaw); ok { + c := sshConfigParseLastOptions(bytes.NewReader(section)) + lastConfig = &c } // Avoid prompting in diff mode (unexpected behavior) // or when a previous config does not exist. - if !showDiff && !sshConfigOpts.equal(lastConfig) { + if !showDiff && lastConfig != nil && !sshConfigOpts.equal(*lastConfig) { newOpts := sshConfigOpts.asList() newOptsMsg := "\n\n New options: none" if len(newOpts) > 0 { @@ -234,7 +239,7 @@ func configSSH() *cobra.Command { return nil } // Selecting "no" will use the last config. - sshConfigOpts = lastConfig + sshConfigOpts = *lastConfig } _, _ = fmt.Fprint(out, "\n") } @@ -441,6 +446,15 @@ func sshConfigParseLastOptions(r io.Reader) (o sshConfigOptions) { return o } +func sshConfigGetCoderSection(data []byte) (section []byte, ok bool) { + startIndex := bytes.Index(data, []byte(sshStartToken)) + endIndex := bytes.Index(data, []byte(sshEndToken)) + if startIndex != -1 && endIndex != -1 { + return data[startIndex : endIndex+len(sshEndToken)], true + } + return nil, false +} + // sshConfigSplitOnCoderSection splits the SSH config into two sections, // before contains the lines before sshStartToken and after contains the // lines after sshEndToken. diff --git a/cli/configssh_test.go b/cli/configssh_test.go index 1e20be5b1447b..72c9c71029082 100644 --- a/cli/configssh_test.go +++ b/cli/configssh_test.go @@ -136,7 +136,7 @@ func TestConfigSSH(t *testing.T) { _ = listener.Close() }) - sshConfigFile, coderConfigFile := sshConfigFileNames(t) + sshConfigFile, _ := sshConfigFileNames(t) tcpAddr, valid := listener.Addr().(*net.TCPAddr) require.True(t, valid) @@ -144,7 +144,6 @@ func TestConfigSSH(t *testing.T) { "--ssh-option", "HostName "+tcpAddr.IP.String(), "--ssh-option", "Port "+strconv.Itoa(tcpAddr.Port), "--ssh-config-file", sshConfigFile, - "--test.ssh-coder-config-file", coderConfigFile, "--skip-proxy-command") clitest.SetupConfig(t, client, root) doneChan := make(chan struct{}) @@ -330,6 +329,26 @@ func TestConfigSSH_FileWriteAndOptionsFlow(t *testing.T) { {match: "Continue?", write: "yes"}, }, }, + { + name: "Do not prompt for new options on first run", + writeConfig: writeConfig{ + ssh: "", + }, + wantConfig: wantConfig{ + ssh: strings.Join([]string{ + headerStart, + "# Last config-ssh options:", + "# :ssh-option=ForwardAgent=yes", + "#", + headerEnd, + "", + }, "\n"), + }, + args: []string{"--ssh-option", "ForwardAgent=yes"}, + matches: []match{ + {match: "Continue?", write: "yes"}, + }, + }, { name: "Prompt for new options when there are no previous options", writeConfig: writeConfig{