[v14] Use dbcmd.WithPrintFormat only for preview of teleterm gateway command

integration/proxy/teleterm_test.go

@@ -372,5 +372,5 @@ func checkKubeconfigPathInCommandEnv(t *testing.T, gw gateway.Gateway, wantKubec
 
 	cmd, err := gw.CLICommand()
 	require.NoError(t, err)
-	require.Equal(t, cmd.Env, []string{"KUBECONFIG=" + wantKubeconfigPath})
+	require.Equal(t, []string{"KUBECONFIG=" + wantKubeconfigPath}, cmd.Env)
 }

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

@@ -570,6 +570,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/cmd/cmds/cmds.go

@@ -0,0 +1,33 @@
+// Copyright 2024 Gravitational, Inc
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package cmds
+
+import (
+	"os/exec"
+)
+
+// 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.
+//
+// Defined in a separate package to avoid cyclic imports. CLI commands got refactored in v15+ anyway.
+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
+}

lib/teleterm/cmd/db.go

@@ -15,14 +15,14 @@
 package cmd
 
 import (
-	"os/exec"
-
 	"github.com/gravitational/trace"
 
 	"github.com/gravitational/teleport/lib/client"
 	"github.com/gravitational/teleport/lib/client/db/dbcmd"
+	"github.com/gravitational/teleport/lib/defaults"
 	"github.com/gravitational/teleport/lib/teleterm/api/uri"
 	"github.com/gravitational/teleport/lib/teleterm/clusters"
+	"github.com/gravitational/teleport/lib/teleterm/cmd/cmds"
 	"github.com/gravitational/teleport/lib/teleterm/gateway"
 	"github.com/gravitational/teleport/lib/tlsca"
 )
@@ -45,10 +45,10 @@ func NewDBCLICommandProvider(storage StorageByResourceURI, execer dbcmd.Execer)
 	}
 }
 
-func (d DBCLICommandProvider) GetCommand(gateway gateway.Gateway) (*exec.Cmd, error) {
+func (d DBCLICommandProvider) GetCommand(gateway gateway.Gateway) (cmds.Cmds, error) {
 	cluster, _, err := d.storage.GetByResourceURI(gateway.TargetURI())
 	if err != nil {
-		return nil, trace.Wrap(err)
+		return cmds.Cmds{}, trace.Wrap(err)
 	}
 
 	routeToDb := tlsca.RouteToDatabase{
@@ -58,17 +58,35 @@ func (d DBCLICommandProvider) GetCommand(gateway gateway.Gateway) (*exec.Cmd, er
 		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(d.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 nil, trace.Wrap(err)
+		return cmds.Cmds{}, trace.Wrap(err)
+	}
+
+	previewCmd, err := clusters.NewDBCLICmdBuilder(cluster, routeToDb, previewOpts...).GetConnectCommand()
+	if err != nil {
+		return cmds.Cmds{}, trace.Wrap(err)
+	}
+
+	cmds := cmds.Cmds{
+		Exec:    execCmd,
+		Preview: previewCmd,
 	}
 
-	return cmd, nil
+	return cmds, nil
 }

lib/teleterm/cmd/db_test.go

@@ -15,6 +15,7 @@
 package cmd
 
 import (
+	"fmt"
 	"os/exec"
 	"path/filepath"
 	"strings"
@@ -28,6 +29,7 @@ import (
 	"github.com/gravitational/teleport/lib/defaults"
 	"github.com/gravitational/teleport/lib/teleterm/api/uri"
 	"github.com/gravitational/teleport/lib/teleterm/clusters"
+	"github.com/gravitational/teleport/lib/teleterm/cmd/cmds"
 	"github.com/gravitational/teleport/lib/teleterm/gateway"
 	"github.com/gravitational/teleport/lib/teleterm/gatewaytest"
 	"github.com/gravitational/teleport/lib/tlsca"
@@ -77,13 +79,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 }
@@ -93,14 +96,27 @@ func TestDbcmdCLICommandProviderGetCommand(t *testing.T) {
 	testCases := []struct {
 		name                  string
 		targetSubresourceName string
+		argsCount             int
+		protocol              string
+		checkCmds             func(*testing.T, fakeDatabaseGateway, cmds.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,
 		},
 	}
 
@@ -116,15 +132,14 @@ func TestDbcmdCLICommandProviderGetCommand(t *testing.T) {
 			mockGateway := fakeDatabaseGateway{
 				targetURI:       cluster.URI.AppendDB("foo"),
 				subresourceName: tc.targetSubresourceName,
+				protocol:        tc.protocol,
 			}
 
 			dbcmdCLICommandProvider := NewDBCLICommandProvider(fakeStorage, fakeExec{})
-			command, err := dbcmdCLICommandProvider.GetCommand(mockGateway)
-
+			cmds, err := dbcmdCLICommandProvider.GetCommand(mockGateway)
 			require.NoError(t, err)
-			require.NotEmpty(t, command.Args)
-			require.Contains(t, command.Args[1], tc.targetSubresourceName)
-			require.Contains(t, command.Args[1], mockGateway.LocalPort())
+
+			tc.checkCmds(t, mockGateway, cmds)
 		})
 	}
 }
@@ -168,3 +183,28 @@ func TestDbcmdCLICommandProviderGetCommand_ReturnsErrorIfClusterIsNotFound(t *te
 	require.Error(t, err)
 	require.True(t, trace.IsNotFound(err), "err is not trace.NotFound")
 }
+
+func checkMongoCmds(t *testing.T, gw fakeDatabaseGateway, cmds cmds.Cmds) {
+	t.Helper()
+	require.Len(t, cmds.Exec.Args, 2)
+	require.Len(t, cmds.Preview.Args, 2)
+
+	execConnString := cmds.Exec.Args[1]
+	previewConnString := cmds.Preview.Args[1]
+
+	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.Cmds) {
+	t.Helper()
+	require.NotEmpty(t, cmds.Exec.Args)
+	require.NotEmpty(t, cmds.Preview.Args)
+}

lib/teleterm/cmd/kube.go

@@ -23,6 +23,7 @@ import (
 	"github.com/gravitational/trace"
 
 	"github.com/gravitational/teleport"
+	"github.com/gravitational/teleport/lib/teleterm/cmd/cmds"
 	"github.com/gravitational/teleport/lib/teleterm/gateway"
 )
 
@@ -37,14 +38,14 @@ func NewKubeCLICommandProvider() KubeCLICommandProvider {
 
 // GetCommand returns a exec.Cmd with KUBECONFIG environment variable that can
 // be used by kube clients to connect to the kube gateway.
-func (p KubeCLICommandProvider) GetCommand(g gateway.Gateway) (*exec.Cmd, error) {
+func (p KubeCLICommandProvider) GetCommand(g gateway.Gateway) (cmds.Cmds, error) {
 	kube, err := gateway.AsKube(g)
 	if err != nil {
-		return nil, trace.Wrap(err)
+		return cmds.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.Cmds{Exec: cmd, Preview: cmd}, nil
 }

lib/teleterm/cmd/kube_test.go

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

lib/teleterm/gateway/base.go

@@ -21,7 +21,6 @@ import (
 	"crypto/tls"
 	"fmt"
 	"net"
-	"os/exec"
 	"strconv"
 	"strings"
 
@@ -32,6 +31,7 @@ import (
 	api "github.com/gravitational/teleport/gen/proto/go/teleport/lib/teleterm/v1"
 	alpn "github.com/gravitational/teleport/lib/srv/alpnproxy"
 	"github.com/gravitational/teleport/lib/teleterm/api/uri"
+	"github.com/gravitational/teleport/lib/teleterm/cmd/cmds"
 	"github.com/gravitational/teleport/lib/tlsca"
 	"github.com/gravitational/teleport/lib/utils"
 )
@@ -189,24 +189,24 @@ func (b *base) LocalPortInt() int {
 
 // CLICommand returns a command which launches a CLI client pointed at the gateway.
 func (b *base) CLICommand() (*api.GatewayCLICommand, error) {
-	cmd, err := b.cfg.CLICommandProvider.GetCommand(b)
+	cmds, err := b.cfg.CLICommandProvider.GetCommand(b)
 	if err != nil {
 		return nil, trace.Wrap(err)
 	}
-	return makeCLICommand(cmd), nil
+	return makeCLICommand(cmds), nil
 }
 
-func makeCLICommand(cmd *exec.Cmd) *api.GatewayCLICommand {
-	cmdString := strings.TrimSpace(
+func makeCLICommand(cmds cmds.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,
 	}
 }
 
@@ -277,7 +277,7 @@ type base struct {
 
 // CLICommandProvider provides a CLI command for gateways which support CLI clients.
 type CLICommandProvider interface {
-	GetCommand(gateway Gateway) (*exec.Cmd, error)
+	GetCommand(gateway Gateway) (cmds.Cmds, error)
 }
 
 type TCPPortAllocator interface {

lib/teleterm/gateway/base_test.go

@@ -29,6 +29,7 @@ import (
 
 	"github.com/gravitational/teleport/lib/defaults"
 	"github.com/gravitational/teleport/lib/teleterm/api/uri"
+	"github.com/gravitational/teleport/lib/teleterm/cmd/cmds"
 	"github.com/gravitational/teleport/lib/teleterm/gatewaytest"
 	"github.com/gravitational/teleport/lib/tlsca"
 )
@@ -148,10 +149,10 @@ func TestNewWithLocalPortReturnsErrorIfNewPortEqualsOldPort(t *testing.T) {
 
 type mockCLICommandProvider struct{}
 
-func (m mockCLICommandProvider) GetCommand(gateway Gateway) (*exec.Cmd, error) {
+func (m mockCLICommandProvider) GetCommand(gateway Gateway) (cmds.Cmds, error) {
 	absPath, err := filepath.Abs(gateway.Protocol())
 	if err != nil {
-		return nil, err
+		return cmds.Cmds{}, err
 	}
 	arg := fmt.Sprintf("%s/%s", gateway.TargetName(), gateway.TargetSubresourceName())
 	// Call exec.Command with a relative path so that cmd.Args[0] is a relative path.
@@ -164,7 +165,7 @@ func (m mockCLICommandProvider) GetCommand(gateway Gateway) (*exec.Cmd, error) {
 	cmd := exec.Command(gateway.Protocol(), arg)
 	cmd.Path = absPath
 	cmd.Env = []string{"FOO=bar"}
-	return cmd, nil
+	return cmds.Cmds{Exec: cmd, Preview: cmd}, nil
 }
 
 func createAndServeGateway(t *testing.T, tcpPortAllocator TCPPortAllocator) Gateway {