Use dbcmd.WithPrintFormat only for preview of teleterm gateway command

integration/proxy/teleterm_test.go

@@ -516,9 +516,10 @@ func testKubeGatewayCertRenewal(ctx context.Context, t *testing.T, params kubeGa
 func checkKubeconfigPathInCommandEnv(t *testing.T, daemonService *daemon.Service, gw gateway.Gateway, wantKubeconfigPath string) {
 	t.Helper()
 
-	cmd, err := daemonService.GetGatewayCLICommand(gw)
+	cmds, err := daemonService.GetGatewayCLICommand(gw)
 	require.NoError(t, err)
-	require.Equal(t, []string{"KUBECONFIG=" + wantKubeconfigPath}, cmd.Env)
+	require.Equal(t, []string{"KUBECONFIG=" + wantKubeconfigPath}, cmds.Exec.Env)
+	require.Equal(t, []string{"KUBECONFIG=" + wantKubeconfigPath}, cmds.Preview.Env)
 }
 
 // setupUserMFA upserts role so that it requires per-session MFA and configures the user account to

lib/client/db/dbcmd/dbcmd.go

@@ -861,7 +861,7 @@ func WithPassword(pass string) ConnectCommandFunc {
 // WithPrintFormat is known to be used for the following situations:
 // - tsh db config --format cmd <database>
 // - tsh proxy db --tunnel <database>
-// - Teleport Connect where the command is put into a terminal.
+// - Teleport Connect where the gateway command is shown in the UI.
 //
 // WithPrintFormat should NOT be used when the exec.Cmd gets executed by the
 // client application.

lib/client/db/dbcmd/dbcmd_test.go

@@ -574,6 +574,8 @@ func TestCLICommandBuilderGetConnectCommand(t *testing.T) {
 			cmd:          nil,
 			wantErr:      true,
 		},
+		// If you find yourself changing this test so that generating a command for DynamoDB _doesn't_
+		// fail if WithPrintFormat() is not provided, please remember to update lib/teleterm/cmd/db.go.
 		{
 			name:         "dynamodb for exec is an error",
 			dbProtocol:   defaults.ProtocolDynamoDB,

lib/teleterm/apiserver/handler/handler_gateways.go

@@ -21,12 +21,12 @@ package handler
 import (
 	"context"
 	"fmt"
-	"os/exec"
 	"strings"
 
 	"github.com/gravitational/trace"
 
 	api "github.com/gravitational/teleport/gen/proto/go/teleport/lib/teleterm/v1"
+	"github.com/gravitational/teleport/lib/teleterm/cmd"
 	"github.com/gravitational/teleport/lib/teleterm/daemon"
 	"github.com/gravitational/teleport/lib/teleterm/gateway"
 )
@@ -82,7 +82,7 @@ func (s *Handler) RemoveGateway(ctx context.Context, req *api.RemoveGatewayReque
 }
 
 func (s *Handler) newAPIGateway(gateway gateway.Gateway) (*api.Gateway, error) {
-	command, err := s.DaemonService.GetGatewayCLICommand(gateway)
+	cmds, err := s.DaemonService.GetGatewayCLICommand(gateway)
 	if err != nil {
 		return nil, trace.Wrap(err)
 	}
@@ -96,21 +96,21 @@ func (s *Handler) newAPIGateway(gateway gateway.Gateway) (*api.Gateway, error) {
 		Protocol:              gateway.Protocol(),
 		LocalAddress:          gateway.LocalAddress(),
 		LocalPort:             gateway.LocalPort(),
-		GatewayCliCommand:     makeGatewayCLICommand(command),
+		GatewayCliCommand:     makeGatewayCLICommand(cmds),
 	}, nil
 }
 
-func makeGatewayCLICommand(cmd *exec.Cmd) *api.GatewayCLICommand {
-	cmdString := strings.TrimSpace(
+func makeGatewayCLICommand(cmds cmd.Cmds) *api.GatewayCLICommand {
+	preview := strings.TrimSpace(
 		fmt.Sprintf("%s %s",
-			strings.Join(cmd.Env, " "),
-			strings.Join(cmd.Args, " ")))
+			strings.Join(cmds.Preview.Env, " "),
+			strings.Join(cmds.Preview.Args, " ")))
 
 	return &api.GatewayCLICommand{
-		Path:    cmd.Path,
-		Args:    cmd.Args,
-		Env:     cmd.Env,
-		Preview: cmdString,
+		Path:    cmds.Exec.Path,
+		Args:    cmds.Exec.Args,
+		Env:     cmds.Exec.Env,
+		Preview: preview,
 	}
 }
 

lib/teleterm/apiserver/handler/handler_gateways_test.go

@@ -26,29 +26,30 @@ import (
 	"github.com/stretchr/testify/require"
 
 	api "github.com/gravitational/teleport/gen/proto/go/teleport/lib/teleterm/v1"
+	"github.com/gravitational/teleport/lib/teleterm/cmd"
 )
 
 func Test_makeGatewayCLICommand(t *testing.T) {
 	absPath, err := filepath.Abs("test-binary")
 	require.NoError(t, err)
 
-	// Call exec.Command with a relative path so that cmd.Args[0] is a relative path.
-	// Then replace cmd.Path with an absolute path to simulate binary being resolved to
+	// Call exec.Command with a relative path so that command.Args[0] is a relative path.
+	// Then replace command.Path with an absolute path to simulate binary being resolved to
 	// an absolute path. This way we can later verify that gateway.CLICommand doesn't use the absolute
 	// path.
 	//
 	// This also ensures that exec.Command behaves the same way on different devices, no matter
 	// whether a command like postgres is installed on the system or not.
-	cmd := exec.Command("test-binary", "arg1", "arg2")
-	cmd.Path = absPath
-	cmd.Env = []string{"FOO=bar"}
+	command := exec.Command("test-binary", "arg1", "arg2")
+	command.Path = absPath
+	command.Env = []string{"FOO=bar"}
 
-	command := makeGatewayCLICommand(cmd)
+	gatewayCmd := makeGatewayCLICommand(cmd.Cmds{Exec: command, Preview: command})
 
 	require.Equal(t, &api.GatewayCLICommand{
 		Path:    absPath,
 		Args:    []string{"test-binary", "arg1", "arg2"},
 		Env:     []string{"FOO=bar"},
 		Preview: "FOO=bar test-binary arg1 arg2",
-	}, command)
+	}, gatewayCmd)
 }

lib/teleterm/cmd/db.go

@@ -24,36 +24,67 @@ import (
 	"github.com/gravitational/trace"
 
 	"github.com/gravitational/teleport/lib/client/db/dbcmd"
+	"github.com/gravitational/teleport/lib/defaults"
 	"github.com/gravitational/teleport/lib/teleterm/clusters"
 	"github.com/gravitational/teleport/lib/teleterm/gateway"
 	"github.com/gravitational/teleport/lib/tlsca"
 )
 
+// Cmds represents a single command in two variants – one that can be used to spawn a process and
+// one that can be copied and pasted into a terminal.
+type Cmds struct {
+	// Exec is the command that should be used when directly executing a command for the given
+	// gateway.
+	Exec *exec.Cmd
+	// Preview is the command that should be used to display the command in the UI. Typically this
+	// means that Preview includes quotes around special characters, so that the command gets executed
+	// properly when the user copies and then pastes it into a terminal.
+	Preview *exec.Cmd
+}
+
 // NewDBCLICommand creates CLI commands for database gateway.
-func NewDBCLICommand(cluster *clusters.Cluster, gateway gateway.Gateway) (*exec.Cmd, error) {
-	cmd, err := newDBCLICommandWithExecer(cluster, gateway, dbcmd.SystemExecer{})
-	return cmd, trace.Wrap(err)
+func NewDBCLICommand(cluster *clusters.Cluster, gateway gateway.Gateway) (Cmds, error) {
+	cmds, err := newDBCLICommandWithExecer(cluster, gateway, dbcmd.SystemExecer{})
+	return cmds, trace.Wrap(err)
 }
 
-func newDBCLICommandWithExecer(cluster *clusters.Cluster, gateway gateway.Gateway, execer dbcmd.Execer) (*exec.Cmd, error) {
+func newDBCLICommandWithExecer(cluster *clusters.Cluster, gateway gateway.Gateway, execer dbcmd.Execer) (Cmds, error) {
 	routeToDb := tlsca.RouteToDatabase{
 		ServiceName: gateway.TargetName(),
 		Protocol:    gateway.Protocol(),
 		Username:    gateway.TargetUser(),
 		Database:    gateway.TargetSubresourceName(),
 	}
 
-	cmd, err := clusters.NewDBCLICmdBuilder(cluster, routeToDb,
+	opts := []dbcmd.ConnectCommandFunc{
 		dbcmd.WithLogger(gateway.Log()),
 		dbcmd.WithLocalProxy(gateway.LocalAddress(), gateway.LocalPortInt(), ""),
 		dbcmd.WithNoTLS(),
-		dbcmd.WithPrintFormat(),
 		dbcmd.WithTolerateMissingCLIClient(),
 		dbcmd.WithExecer(execer),
-	).GetConnectCommand()
+	}
+
+	// DynamoDB doesn't support non-print-format use.
+	if gateway.Protocol() == defaults.ProtocolDynamoDB {
+		opts = append(opts, dbcmd.WithPrintFormat())
+	}
+
+	previewOpts := append(opts, dbcmd.WithPrintFormat())
+
+	execCmd, err := clusters.NewDBCLICmdBuilder(cluster, routeToDb, opts...).GetConnectCommand()
+	if err != nil {
+		return Cmds{}, trace.Wrap(err)
+	}
+
+	previewCmd, err := clusters.NewDBCLICmdBuilder(cluster, routeToDb, previewOpts...).GetConnectCommand()
 	if err != nil {
-		return nil, trace.Wrap(err)
+		return Cmds{}, trace.Wrap(err)
+	}
+
+	cmds := Cmds{
+		Exec:    execCmd,
+		Preview: previewCmd,
 	}
 
-	return cmd, nil
+	return cmds, nil
 }

lib/teleterm/cmd/db_test.go

@@ -19,6 +19,7 @@
 package cmd
 
 import (
+	"fmt"
 	"os/exec"
 	"path/filepath"
 	"testing"
@@ -52,13 +53,14 @@ type fakeDatabaseGateway struct {
 	gateway.Database
 	targetURI       uri.ResourceURI
 	subresourceName string
+	protocol        string
 }
 
 func (m fakeDatabaseGateway) TargetURI() uri.ResourceURI    { return m.targetURI }
 func (m fakeDatabaseGateway) TargetName() string            { return m.targetURI.GetDbName() }
 func (m fakeDatabaseGateway) TargetUser() string            { return "alice" }
 func (m fakeDatabaseGateway) TargetSubresourceName() string { return m.subresourceName }
-func (m fakeDatabaseGateway) Protocol() string              { return defaults.ProtocolMongoDB }
+func (m fakeDatabaseGateway) Protocol() string              { return m.protocol }
 func (m fakeDatabaseGateway) Log() *logrus.Entry            { return nil }
 func (m fakeDatabaseGateway) LocalAddress() string          { return "localhost" }
 func (m fakeDatabaseGateway) LocalPortInt() int             { return 8888 }
@@ -68,14 +70,27 @@ func TestNewDBCLICommand(t *testing.T) {
 	testCases := []struct {
 		name                  string
 		targetSubresourceName string
+		argsCount             int
+		protocol              string
+		checkCmds             func(*testing.T, fakeDatabaseGateway, Cmds)
 	}{
 		{
 			name:                  "empty name",
+			protocol:              defaults.ProtocolMongoDB,
 			targetSubresourceName: "",
+			checkCmds:             checkMongoCmds,
 		},
 		{
 			name:                  "with name",
+			protocol:              defaults.ProtocolMongoDB,
 			targetSubresourceName: "bar",
+			checkCmds:             checkMongoCmds,
+		},
+		{
+			name:                  "custom handling of DynamoDB does not blow up",
+			targetSubresourceName: "bar",
+			protocol:              defaults.ProtocolDynamoDB,
+			checkCmds:             checkArgsNotEmpty,
 		},
 	}
 
@@ -88,14 +103,34 @@ func TestNewDBCLICommand(t *testing.T) {
 			mockGateway := fakeDatabaseGateway{
 				targetURI:       cluster.URI.AppendDB("foo"),
 				subresourceName: tc.targetSubresourceName,
+				protocol:        tc.protocol,
 			}
 
-			command, err := newDBCLICommandWithExecer(&cluster, mockGateway, fakeExec{})
-
+			cmds, err := newDBCLICommandWithExecer(&cluster, mockGateway, fakeExec{})
 			require.NoError(t, err)
-			require.Len(t, command.Args, 2)
-			require.Contains(t, command.Args[1], tc.targetSubresourceName)
-			require.Contains(t, command.Args[1], mockGateway.LocalPort())
+
+			tc.checkCmds(t, mockGateway, cmds)
 		})
 	}
 }
+
+func checkMongoCmds(t *testing.T, gw fakeDatabaseGateway, cmds Cmds) {
+	execConnString := cmds.Exec.Args[1]
+	previewConnString := cmds.Preview.Args[1]
+	require.Len(t, cmds.Exec.Args, 2)
+	require.Len(t, cmds.Preview.Args, 2)
+	require.Contains(t, execConnString, gw.TargetSubresourceName())
+	require.Contains(t, previewConnString, gw.TargetSubresourceName())
+	require.Contains(t, execConnString, gw.LocalPort())
+	require.Contains(t, previewConnString, gw.LocalPort())
+
+	// Verify that the preview cmd has exec cmd conn string wrapped in quotes.
+	require.NotContains(t, execConnString, "\"")
+	expectedPreviewConnString := fmt.Sprintf("%q", execConnString)
+	require.Equal(t, expectedPreviewConnString, previewConnString)
+}
+
+func checkArgsNotEmpty(t *testing.T, gw fakeDatabaseGateway, cmds Cmds) {
+	require.NotEmpty(t, cmds.Exec.Args)
+	require.NotEmpty(t, cmds.Preview.Args)
+}

lib/teleterm/cmd/kube.go

@@ -29,14 +29,14 @@ import (
 )
 
 // NewKubeCLICommand creates CLI commands for kube gateways.
-func NewKubeCLICommand(g gateway.Gateway) (*exec.Cmd, error) {
+func NewKubeCLICommand(g gateway.Gateway) (Cmds, error) {
 	kube, err := gateway.AsKube(g)
 	if err != nil {
-		return nil, trace.Wrap(err)
+		return Cmds{}, trace.Wrap(err)
 	}
 
 	// Use kubectl version as placeholders. Only env should be used.
 	cmd := exec.Command("kubectl", "version")
 	cmd.Env = []string{fmt.Sprintf("%v=%v", teleport.EnvKubeConfig, kube.KubeconfigPath())}
-	return cmd, nil
+	return Cmds{Exec: cmd, Preview: cmd}, nil
 }

lib/teleterm/cmd/kube_test.go

@@ -35,5 +35,5 @@ func (m fakeKubeGateway) KubeconfigPath() string { return "test.kubeconfig" }
 func TestNewKubeCLICommand(t *testing.T) {
 	cmd, err := NewKubeCLICommand(fakeKubeGateway{})
 	require.NoError(t, err)
-	require.Equal(t, []string{"KUBECONFIG=test.kubeconfig"}, cmd.Env)
+	require.Equal(t, []string{"KUBECONFIG=test.kubeconfig"}, cmd.Exec.Env)
 }