mirror of
https://github.com/coder/coder.git
synced 2025-07-06 15:41:45 +00:00
feat: Refactor CLI config-ssh to improve UX (#1900)
- 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 - Safer config writing via tmp-file + rename - Parse previous `config-ssh` options, compare to new options and ask to use new (otherwise old ones are used) - Tests the new functionality Fixes #1326
This commit is contained in:
Mathias Fredriksson
committed by
GitHub
GitHub
parent
945fa9dacf
commit
b65259f95e
@ -2,10 +2,12 @@ package cli_test
|
||||
|
||||
import (
|
||||
"context"
|
||||
"fmt"
|
||||
"io"
|
||||
"net"
|
||||
"os"
|
||||
"os/exec"
|
||||
"path/filepath"
|
||||
"strconv"
|
||||
"strings"
|
||||
"testing"
|
||||
@ -25,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()
|
||||
|
||||
@ -77,9 +109,6 @@ func TestConfigSSH(t *testing.T) {
|
||||
t.Cleanup(func() {
|
||||
_ = agentCloser.Close()
|
||||
})
|
||||
tempFile, err := os.CreateTemp(t.TempDir(), "")
|
||||
require.NoError(t, err)
|
||||
_ = tempFile.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)
|
||||
@ -106,12 +135,15 @@ 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-config-file", sshConfigFile,
|
||||
"--test.ssh-coder-config-file", coderConfigFile,
|
||||
"--skip-proxy-command")
|
||||
clitest.SetupConfig(t, client, root)
|
||||
doneChan := make(chan struct{})
|
||||
@ -123,13 +155,369 @@ func TestConfigSSH(t *testing.T) {
|
||||
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())
|
||||
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 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: "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{
|
||||
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 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 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 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 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)
|
||||
}
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user