fix: Clean up coder config-ssh dry-run behavior (#3660)

This commit also drops old deprecated code.

Fixes #2982

cli/configssh.go

@@ -137,7 +137,6 @@ func configSSH() *cobra.Command {
 		sshConfigFile    string
 		sshConfigOpts    sshConfigOptions
 		usePreviousOpts  bool
-		coderConfigFile  string
 		dryRun           bool
 		skipProxyCommand bool
 		wireguard        bool
@@ -198,15 +197,7 @@ func configSSH() *cobra.Command {
 			// Parse the previous configuration only if config-ssh
 			// has been run previously.
 			var lastConfig *sshConfigOptions
-			var ok bool
+			if section, ok := sshConfigGetCoderSection(configRaw); ok {
-			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.
-				c := sshConfigParseLastOptions(bytes.NewReader(coderConfigRaw))
-				lastConfig = &c
-			} else if section, ok := sshConfigGetCoderSection(configRaw); ok {
 				c := sshConfigParseLastOptions(bytes.NewReader(section))
 				lastConfig = &c
 			}
@@ -237,6 +228,8 @@ func configSSH() *cobra.Command {
 					}
 					// Selecting "no" will use the last config.
 					sshConfigOpts = *lastConfig
+				} else {
+					changes = append(changes, "Use new SSH options")
 				}
 				// Only print when prompts are shown.
 				if yes, _ := cmd.Flags().GetBool("yes"); !yes {
@@ -245,14 +238,6 @@ func configSSH() *cobra.Command {
 			}
 
 			configModified := configRaw
-
-			// Check for the presence of the coder Include
-			// statement is present and add if missing.
-			// 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{}
@@ -313,17 +298,34 @@ func configSSH() *cobra.Command {
 			_, _ = buf.Write(after)
 
 			if !bytes.Equal(configModified, buf.Bytes()) {
-				changes = append(changes, fmt.Sprintf("Update coder config section in %s", sshConfigFile))
+				changes = append(changes, fmt.Sprintf("Update the coder section in %s", sshConfigFile))
 				configModified = buf.Bytes()
 			}
 
-			if len(changes) > 0 {
+			if len(changes) == 0 {
-				dryRunDisclaimer := ""
+				_, _ = fmt.Fprintf(out, "No changes to make.\n")
-				if dryRun {
+				return nil
-					dryRunDisclaimer = " (dry-run, no changes will be made)"
 			}
+
+			if dryRun {
+				_, _ = fmt.Fprintf(out, "Dry run, the following changes would be made to your SSH configuration:\n\n  * %s\n\n", strings.Join(changes, "\n  * "))
+
+				color := isTTYOut(cmd)
+				diff, err := diffBytes(sshConfigFile, configRaw, configModified, color)
+				if err != nil {
+					return xerrors.Errorf("diff failed: %w", err)
+				}
+				if len(diff) > 0 {
+					// Write diff to stdout.
+					_, _ = fmt.Fprintf(cmd.OutOrStdout(), "%s", diff)
+				}
+
+				return nil
+			}
+
+			if len(changes) > 0 {
 				_, err = cliui.Prompt(cmd, cliui.PromptOptions{
-					Text:      fmt.Sprintf("The following changes will be made to your SSH configuration:\n\n    * %s\n\n  Continue?%s", strings.Join(changes, "\n    * "), dryRunDisclaimer),
+					Text:      fmt.Sprintf("The following changes will be made to your SSH configuration:\n\n    * %s\n\n  Continue?", strings.Join(changes, "\n    * ")),
 					IsConfirm: true,
 				})
 				if err != nil {
@@ -335,47 +337,18 @@ func configSSH() *cobra.Command {
 				}
 			}
 
-			if dryRun {
-				color := isTTYOut(cmd)
-				diffFns := []func() ([]byte, error){
-					func() ([]byte, error) { return diffBytes(sshConfigFile, configRaw, configModified, 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)
-					}
-					if len(diff) > 0 {
-						// Write diff to stdout.
-						_, _ = fmt.Fprintf(cmd.OutOrStdout(), "\n%s", diff)
-					}
-				}
-			} else {
 			if !bytes.Equal(configRaw, configModified) {
 				err = writeWithTempFileAndMove(sshConfigFile, bytes.NewReader(configModified))
 				if err != nil {
 					return xerrors.Errorf("write ssh config failed: %w", err)
 				}
 			}
-				// Deprecated: Remove after migration period.
-				if len(coderConfigRaw) > 0 {
-					err = os.Remove(coderConfigFile)
-					if err != nil {
-						return xerrors.Errorf("remove coder config failed: %w", err)
-					}
-				}
-			}
 
 			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", workspaceConfigs[0].Name)
+				_, _ = fmt.Fprintf(out, "For example, try running:\n\n\t$ ssh coder.%s\n", workspaceConfigs[0].Name)
 			} else {
-				_, _ = fmt.Fprint(out, "You don't have any workspaces yet, try creating one with:\n\n\t$ coder create <workspace>\n\n")
+				_, _ = fmt.Fprint(out, "You don't have any workspaces yet, try creating one with:\n\n\t$ coder create <workspace>\n")
 			}
 			return nil
 		},
@@ -389,10 +362,6 @@ func configSSH() *cobra.Command {
 	cliflag.BoolVarP(cmd.Flags(), &wireguard, "wireguard", "", "CODER_CONFIG_SSH_WIREGUARD", false, "Whether to use Wireguard for SSH tunneling.")
 	_ = cmd.Flags().MarkHidden("wireguard")
 
-	// 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")
-
 	cliui.AllowSkipPrompt(cmd)
 
 	return cmd

cli/configssh_old.go

@@ -1,66 +0,0 @@
-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
-}

cli/configssh_test.go

@@ -6,7 +6,6 @@ import (
 	"context"
 	"fmt"
 	"io"
-	"io/fs"
 	"net"
 	"os"
 	"os/exec"
@@ -30,15 +29,14 @@ import (
 	"github.com/coder/coder/pty/ptytest"
 )
 
-func sshConfigFileNames(t *testing.T) (sshConfig string, coderConfig string) {
+func sshConfigFileName(t *testing.T) (sshConfig 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")
+	n := filepath.Join(dotssh, "config")
-	n2 := filepath.Join(dotssh, "coder")
+	return n
-	return n1, n2
 }
 
 func sshConfigFileCreate(t *testing.T, name string, data io.Reader) {
@@ -135,7 +133,7 @@ func TestConfigSSH(t *testing.T) {
 		}
 	}()
 
-	sshConfigFile, _ := sshConfigFileNames(t)
+	sshConfigFile := sshConfigFileName(t)
 
 	tcpAddr, valid := listener.Addr().(*net.TCPAddr)
 	require.True(t, valid)
@@ -198,11 +196,9 @@ func TestConfigSSH_FileWriteAndOptionsFlow(t *testing.T) {
 
 	type writeConfig struct {
 		ssh string
-		coder string
 	}
 	type wantConfig struct {
 		ssh string
-		coderKept bool
 	}
 	type match struct {
 		match, write string
@@ -514,120 +510,6 @@ func TestConfigSSH_FileWriteAndOptionsFlow(t *testing.T) {
 				"--yes",
 			},
 		},
-
-		// Tests for deprecated split coder config.
-		{
-			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.",
-					"#",
-					"# 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{
-				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.",
-					"#",
-					"# 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{
-				ssh: strings.Join([]string{
-					baseHeader,
-					"",
-				}, "\n"),
-			},
-			matches: []match{
-				{match: "Use new options?", write: "yes"},
-				{match: "Continue?", write: "yes"},
-			},
-		},
-		{
-			name: "Allow overwriting previous options from coder config when they differ",
-			writeConfig: writeConfig{
-				ssh: strings.Join([]string{
-					"Include coder",
-					"",
-				}, "\n"),
-				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\".",
-					"#",
-					"# Last config-ssh options:",
-					"# :ssh-option=ForwardAgent=yes",
-					"#",
-				}, "\n"),
-			},
-			wantConfig: wantConfig{
-				ssh: strings.Join([]string{
-					headerStart,
-					"# Last config-ssh options:",
-					"# :ssh-option=ForwardAgent=no",
-					"#",
-					headerEnd,
-					"",
-				}, "\n"),
-			},
-			args: []string{"--ssh-option", "ForwardAgent=no"},
-			matches: []match{
-				{match: "Use new options?", write: "yes"},
-				{match: "Continue?", write: "yes"},
-			},
-		},
 	}
 	for _, tt := range tests {
 		tt := tt
@@ -645,18 +527,14 @@ func TestConfigSSH_FileWriteAndOptionsFlow(t *testing.T) {
 			)
 
 			// Prepare ssh config files.
-			sshConfigName, coderConfigName := sshConfigFileNames(t)
+			sshConfigName := sshConfigFileName(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.ssh-coder-config-file", coderConfigName,
 			}
 			args = append(args, tt.args...)
 			cmd, root := clitest.New(t, args...)
@@ -685,10 +563,6 @@ func TestConfigSSH_FileWriteAndOptionsFlow(t *testing.T) {
 				got := sshConfigFileRead(t, sshConfigName)
 				assert.Equal(t, tt.wantConfig.ssh, got)
 			}
-			if !tt.wantConfig.coderKept {
-				_, err := os.ReadFile(coderConfigName)
-				assert.ErrorIs(t, err, fs.ErrNotExist)
-			}
 		})
 	}
 }
@@ -778,7 +652,7 @@ func TestConfigSSH_Hostnames(t *testing.T) {
 			workspace := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID)
 			coderdtest.AwaitWorkspaceBuildJob(t, client, workspace.LatestBuild.ID)
 
-			sshConfigFile, _ := sshConfigFileNames(t)
+			sshConfigFile := sshConfigFileName(t)
 
 			cmd, root := clitest.New(t, "config-ssh", "--ssh-config-file", sshConfigFile)
 			clitest.SetupConfig(t, client, root)