Generate DB CLI commands for Teleterm from tsh daemon #11835

ravicious · 2022-04-08T16:35:20Z

Best reviewed commit-by-commit.
Files under lib/teleterm/api/protogen are autogenerated from proto files.

To test this change in Teleterm, you need a dev version of Teleterm with gravitational/webapps#726.

Why

When you connect to a cluster with Teleterm and click "Connect" next to a database, it sets up the equivalent of tsh proxy db --tunnel (--tunnel was added in #11720).

However, up until this point, Teleterm was constructing the DB CLI commands by hand, meaning that each time the Database Access team adds support for a new protocol, we'd need to update Teleterm. In general, we'd need to keep both places in sync.

What

@smallinsky suggested to generate those inside the tsh daemon and this is what this PR achieves.

On top of that, it adds GetRelativeConnectCommand so that Teleterm doesn't have to show absolute paths to the binaries.

ravicious · 2022-04-08T16:42:27Z

I'll fix the lint problems on Monday.

ravicious · 2022-04-08T16:45:38Z

lib/client/db/dbcmd/dbcmd.go

-	// Check for mysql binary. Return with error as mysql and mariadb are missing. There is nothing else we can do here.
-	if !c.isMySQLBinAvailable() {
-		return nil, trace.NotFound("neither %q nor %q CLI clients were found, please make sure an appropriate CLI client is available in $PATH", mysqlBin, mariadbBin)
-	}
-


This was removed so that dbcmd doesn't fail for Teleterm if the user doesn't have the binaries installed.

For the most part, the DB CLI command in Teleterm has a more "decorative" function – unlike tsh db connect, it doesn't have to actually work on the user's machine.

r0mant

Left a few nits but mostly lgtm.

lib/client/db/dbcmd/dbcmd.go

lib/teleterm/gateway/gateway.go

smallinsky

I took quick look and left some comments.

lib/teleterm/clusters/cluster_gateways.go

smallinsky · 2022-04-08T18:50:29Z

lib/teleterm/clusters/cluster_gateways.go

+	routeToDb := tlsca.RouteToDatabase{
+		ServiceName: gw.TargetName,
+		Protocol:    gw.Protocol,
+		Username:    gw.TargetUser,
+	}


Seems that database name is missing.

Right, for now there's no way for the user to specify the db name anyway.

smallinsky · 2022-04-08T19:09:21Z

lib/teleterm/clusters/cluster_gateways.go

+	}
+
+	cmd, err := dbcmd.NewCmdBuilder(c.clusterClient, &c.status, &routeToDb, c.URI.GetRootClusterName(),
+		dbcmd.WithLocalProxy(gw.LocalAddress, gw.LocalPortInt(), ""),


Not sure if it the last argument - caPath should be empty. For example mongo, and redis commands are using this arg to overwrite CA

teleport/tool/tsh/dbcmd.go

Line 333 in 97c0b5c

if c.options.caPath != "" {

They do this only in the absence of the noTLS option and this callsite here will always use dbcmd.WithNoTLS().

Perhaps in the future it'd be worthwhile to refactor connectionCommandOpts so that it's not possible to pass mutually exclusive options.

lib/client/db/dbcmd/dbcmd.go

jakule · 2022-04-11T15:51:59Z

lib/client/db/dbcmd/dbcmd.go

-	}
-
-	// Check for mysql binary. Return with error as mysql and mariadb are missing. There is nothing else we can do here.
-	if !c.isMySQLBinAvailable() {


Is there a way for Teleterm to deal with that error?

Currently if none of the supported binaries are installed tsh will print mysql not found not mentioning that mariadb` is also a supported client

If Teleterm fails on every error returned from CLIBuildder then another person working on that code can easily return an error and break the Teleterm. If this is the case I'd suggest removing the error from returned values.

If Teleterm fails on every error returned from CLIBuildder then another person working on that code can easily return an error and break the Teleterm.

Good point.

The fundamental issue is that tsh db connect expects dbcmd to return a command that will work and if it can't provide one, it should either return an appropriate error or attempt to execute the command which will result in the <binary> not found error.

tsh db connect also executes dbcmd only when the command is actually needed. So if someone has a cluster with a Postgres db but they don't have psql installed it and they never try to access it, nothing bad ever happens.

Teleterm on the other hand doesn't expect this command to work. It's more of a "if you have the CLI client installed on your system, this command should connect you to that db". It also needs this command any time it creates a local proxy for the db, but the user might not even want to use that command. They might want to use a GUI app. So again, in this case we don't really care if the binary is actually on the system.

@jakule What if we added a tolerateMissingCLIClient flag to connectionCommandOpts? If it's present, dbcmd would not return an error here, but rather c.getMySQLOracleCommand().

#11843 added the printFormat flag so that tsh db config can wrap Postgres connection strings in quotes. With tolerateMissingCLIClient, the option usage would look like this:

use case printFormat tolerateMissingCLIClient

tsh db config ✅ ❌

tsh db connect ❌ ❌

Teleterm ✅ ✅

@jakule I added a comment which adds said option, please let me know what you think.

Fine by me. I'd just change the name of that flag to ignoreMissingBin or skipBinCheck maybe?

Please also add a test with this flag set.

Heh, I thought about a name like this. However, for all the other protocols, dbcmd doesn't actually check if the binary is there or not – attempting to run the resulting command is just going to fail with a regular shell error about a missing binary.

At the moment it really is just a special case for MySQL where instead of telling the user that mysql doesn't exist we also want to point out that mariadb is a valid option too. So I didn't want to use a field name that would suggest that if the field is not present then the check will be done.

In that sense it's more of a caller telling dbcmd "I'm okay with the CLI missing on user's system".

Please also add a test with this flag set.

Yep, I already added one that makes mysql and mariadb not available and expects that the method will not return an error.

lib/client/db/dbcmd/dbcmd_test.go

lib/teleterm/gateway/gateway.go

Teleterm wasn't launch yet, so we don't have to worry too much about backwards compatibility in the proto files and we can just repurpose old field numbers.

* Extract dbcmd.go into a new package under lib/client/db/dbcmd * Use dbcmd to generate CliCommand for gateways * Return relative db command from tsh daemon * Add WithTolarateMissingCLIClient func to dbcmd

…commands for Teleport Connect (#12206) * Quote postgres connection string for printing to terminals (#11843) * Generate DB CLI commands for Teleterm from tsh daemon (#11835) * Extract dbcmd.go into a new package under lib/client/db/dbcmd * Use dbcmd to generate CliCommand for gateways * Return relative db command from tsh daemon * Add WithTolarateMissingCLIClient func to dbcmd Co-authored-by: STeve (Xin) Huang <xin.huang@goteleport.com>

ravicious mentioned this pull request Apr 8, 2022

Use DB CLI commands provided by tsh daemon gravitational/webapps#726

Merged

ravicious marked this pull request as ready for review April 8, 2022 16:40

github-actions bot requested review from ibeckermayer and zmb3 April 8, 2022 16:40

github-actions bot added the tsh tsh - Teleport's command line tool for logging into nodes running Teleport. label Apr 8, 2022

ravicious requested review from r0mant and jakule and removed request for zmb3 and ibeckermayer April 8, 2022 16:41

ravicious commented Apr 8, 2022

View reviewed changes

ravicious added the teleterm-backport label Apr 8, 2022

r0mant reviewed Apr 8, 2022

View reviewed changes

smallinsky reviewed Apr 8, 2022

View reviewed changes

ravicious requested review from r0mant and smallinsky April 11, 2022 14:26

r0mant approved these changes Apr 11, 2022

View reviewed changes

lib/client/db/dbcmd/dbcmd.go Outdated Show resolved Hide resolved

lib/client/db/dbcmd/dbcmd.go Show resolved Hide resolved

jakule reviewed Apr 11, 2022

View reviewed changes

lib/client/db/dbcmd/dbcmd_test.go Outdated Show resolved Hide resolved

jakule reviewed Apr 11, 2022

View reviewed changes

lib/teleterm/gateway/gateway.go Outdated Show resolved Hide resolved

ravicious added 10 commits April 12, 2022 14:35

Extract dbcmd.go into a new package under lib/client/db/dbcmd

b59ea3a

Add cli_command to gateway proto, remove unused fields

aca1d79

Teleterm wasn't launch yet, so we don't have to worry too much about backwards compatibility in the proto files and we can just repurpose old field numbers.

Use dbcmd to generate CliCommand for gateways

dea26cd

Return relative db command from tsh daemon

b2bdf71

Export dbcmd.CliCommandBuilder to satisfy linter

f085de1

Rename gateway.CliCommand to gateway.CLICommand

5d3017c

Rename buildCliCommand to buildCLICommand

ef014a4

Rename GetRelativeConnectCommand to GetConnectCommandNoAbsPath

3f83a00

Add missing godocs

3a77875

Rename CliCommandBuilder to CLICommandBuilder

be1b615

Refactor how opts are passed in dbcmd tests

b7f2759

ravicious force-pushed the ravicious/teleterm-db-cli-command branch from 1e650a8 to dd1d11c Compare April 12, 2022 14:49

ravicious requested a review from jakule April 12, 2022 14:52

ravicious added 2 commits April 12, 2022 16:56

Add WithTolarateMissingCLIClient func to dbcmd

0437882

Gateway.LocalPortInt: Use strconv.Atoi

d60bf57

ravicious force-pushed the ravicious/teleterm-db-cli-command branch from b73446b to d60bf57 Compare April 12, 2022 14:56

Fix license

fc76d32

smallinsky approved these changes Apr 13, 2022

View reviewed changes

Merge branch 'master' into ravicious/teleterm-db-cli-command

d0dbc65

ravicious enabled auto-merge (squash) April 13, 2022 11:11

jakule approved these changes Apr 13, 2022

View reviewed changes

ravicious disabled auto-merge April 13, 2022 14:41

Merge branch 'master' into ravicious/teleterm-db-cli-command

a6d434b

ravicious enabled auto-merge (squash) April 13, 2022 15:26

ravicious disabled auto-merge April 13, 2022 16:02

ravicious enabled auto-merge (squash) April 14, 2022 08:08

Merge branch 'master' into ravicious/teleterm-db-cli-command

916ec58

ravicious merged commit 60f5972 into master Apr 14, 2022

ravicious deleted the ravicious/teleterm-db-cli-command branch April 14, 2022 08:29

ravicious mentioned this pull request Apr 25, 2022

[v9] Backport quoting Postgres connection string & generating DB CLI commands for Teleport Connect #12206

Merged

ravicious removed the teleconnect-backport label Apr 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate DB CLI commands for Teleterm from tsh daemon #11835

Generate DB CLI commands for Teleterm from tsh daemon #11835

ravicious commented Apr 8, 2022 •

edited

Loading

ravicious commented Apr 8, 2022

ravicious Apr 8, 2022

r0mant left a comment

smallinsky left a comment

smallinsky Apr 8, 2022

ravicious Apr 11, 2022

smallinsky Apr 8, 2022

ravicious Apr 11, 2022

jakule Apr 11, 2022

ravicious Apr 12, 2022

ravicious Apr 12, 2022

jakule Apr 13, 2022

jakule Apr 13, 2022

ravicious Apr 13, 2022

ravicious Apr 13, 2022

Generate DB CLI commands for Teleterm from tsh daemon #11835

Generate DB CLI commands for Teleterm from tsh daemon #11835

Conversation

ravicious commented Apr 8, 2022 • edited Loading

Why

What

ravicious commented Apr 8, 2022

Choose a reason for hiding this comment

r0mant left a comment

Choose a reason for hiding this comment

smallinsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravicious commented Apr 8, 2022 •

edited

Loading