Refactor postgres service file handling, add db config command #5319

r0mant · 2021-01-15T22:20:12Z

This PR belongs to a series of smaller self-contained database access PRs that I will be submitting to land various QoL improvements and refactorings to help keep future MySQL integration PR smaller.

This PR includes:

Refactor and reorganize Postgres connection service file handling a bit to make adding MySQL option file easier.
While being at it, also added support for --insecure flag for Postgres service file. Closes Add insecure mode support for database access #5026.
tsh db ls command output now includes an example connect command to make it easier for users to learn how to connect to a database (can just copy-paste). Example:

➜  ~ tsh db ls
Name                               Description            Labels    Connect
---------------------------------- ---------------------- --------- -------------------------
> local (user: postgres, db: test) PostgreSQL 13.0: Local env=local psql "service=root-local"

Add tsh db config command that prints database connection information in text form. I realized the need for a command like this when configuring GUI clients - they often require entering paths for certificates for example, and regular users may not know where Teleport stores them on their machine. Example:

➜  ~ tsh db config
Name:      local
Host:      root.gravitational.io
Port:      3080
User:      postgres
Database:  test
CA:        /Users/r0mant/.tsh/keys/root.gravitational.io/certs.pem
Cert:      /Users/r0mant/.tsh/keys/root.gravitational.io/r0mant-db/root/local-x509.pem
Key:       /Users/r0mant/.tsh/keys/root.gravitational.io/r0mant

a-palchikov · 2021-01-18T10:34:43Z

lib/client/db/postgres/servicefile.go

-	section.NewKey("sslcert", profile.SSLCert)
-	section.NewKey("sslkey", profile.SSLKey)
+	if profile.Insecure {
+		section.NewKey("sslmode", SSLModeVerifyCA)


According to documentation, there are 6 SSL modes and the 3 require, verify-ca and verify-full will error if Postgres was not compiled with SSL support - is this a concern in this case? Also looks like disable or even allow would make a better candidate for insecure mode than verify-ca.

It's not a concern because we require TLS for the database access. I'm using verify-ca for the same reason, all our routing/authentication it based on client certificates. Also, this is configuring client to talk to Teleport proxy, not Postgres.

lib/client/db/postgres/servicefile.go

a-palchikov · 2021-01-18T10:37:14Z

lib/client/db/profile.go

+limitations under the License.
+*/
+
+// Package db contains methods for working with database connection profiles


Sounds like lib/client/db/profile would be a more apt package location for working with profiles

Can't move it there due to circular imports.

I'm not sure which imports you mean here, but my suggestion was to pull the interfaces (ConnectProfileFile and ConnectProfile) to lib/client/db and implement support for profile handling for a specific database flavor in the corresponding lib/client/db/profile/<profile> or lib/client/db/<flavor>/.

lib/client/db/profile.go

a-palchikov · 2021-01-18T10:44:29Z

lib/client/db/profile/profile.go

+limitations under the License.
+*/
+
+package profile


This is probably meant to land in lib/client/db instead - to be implemented by more specific packages in lib/client/db/...?

Not really, this is intentional so particular implementations can reuse this struct from here. Can't move it level up due to circular imports.

With the structs in lib/client/db, they'd do exactly that. The specific provider implementation would use the struct from lib/client/db. Of course this means lib/client/db/<provider> cannot be used from lib/client/db which does cause a circular dependency. But it should not have to - the providers should just register themselves somewhere in the root and the root would use them via an interface - does that make sense?

Yeah, I know what you're saying, I just feel that introducing this sort of registry when I only need to share one struct between a couple of packages may be a bit of an overkill.

tool/tsh/db.go

a-palchikov · 2021-01-18T10:50:27Z

tool/tsh/db.go

+	}
+	host, port := tc.WebProxyHostPort()
+	fmt.Printf(`Name:      %v
+Host:      %v


If would consider adding a format option for this to make this machine-parsable - this way it can be converted to any other format on the fly without much trouble.

This is intended to be human-readable first. I agree it might make sense to add other format in future, I'll leave it as a potential improvements if anyone actually needs it.

tool/tsh/tsh.go

r0mant · 2021-01-20T19:36:00Z

@awly @a-palchikov @russjones @fspmarshall Can I please get some codeowners reviews when you get a chance? I'd like to get this merged so I can continue with MySQL integration. Thanks!

a-palchikov

Feel free to merge if the suggested provider package layout is not warranted.

a-palchikov · 2021-01-20T20:48:59Z

lib/client/db/profile.go

+limitations under the License.
+*/
+
+// Package db contains methods for working with database connection profiles


I'm not sure which imports you mean here, but my suggestion was to pull the interfaces (ConnectProfileFile and ConnectProfile) to lib/client/db and implement support for profile handling for a specific database flavor in the corresponding lib/client/db/profile/<profile> or lib/client/db/<flavor>/.

a-palchikov · 2021-01-20T20:53:10Z

lib/client/db/profile/profile.go

+limitations under the License.
+*/
+
+package profile


With the structs in lib/client/db, they'd do exactly that. The specific provider implementation would use the struct from lib/client/db. Of course this means lib/client/db/<provider> cannot be used from lib/client/db which does cause a circular dependency. But it should not have to - the providers should just register themselves somewhere in the root and the root would use them via an interface - does that make sense?

tool/tsh/tsh.go

lib/client/db/profile/profile.go

russjones · 2021-01-21T19:12:49Z

tool/tsh/db.go

+	tc, err := makeClient(cf, false)
+	if err != nil {
+		utils.FatalError(err)
+	}
+	database, err := pickActiveDatabase(cf)
+	if err != nil {
+		utils.FatalError(err)
+	}
+	env, err := dbprofile.Env(tc, *database)
+	if err != nil {
+		utils.FatalError(err)
+	}
+	for k, v := range env {
+		fmt.Printf("export %v=%v\n", k, v)
+	}


You may not need this, but if you want to print some instructions as well, you can add something like the following:

fmt.Printf(`echo 'Environment loaded, run the following command to connect...\n'`)

I'm doing something similar for tsh login leaf.example.com --env.

The purpose of this command is for its output to be fed to shell's eval (like, $(tsh db env) - similar to what minikube for example does as well). For easier discovery of the connect commands I added those to tsh db ls table.

…e/dynamodb-audit-fix * 'master' of github.com:gravitational/teleport: Refactor db proxy/engine for easier reuse (#5325) Refactor postgres service file handling, add db config command (#5319) Fix tsh login issue when --proxy differs from actual proxy public addr (#5380)

r0mant added the database-access Database access related issues and PRs label Jan 15, 2021

r0mant added this to the 6.0 "San Diego" milestone Jan 15, 2021

r0mant self-assigned this Jan 15, 2021

r0mant requested review from a-palchikov, awly, fspmarshall, klizhentas, russjones and webvictim as code owners January 15, 2021 22:20

a-palchikov reviewed Jan 18, 2021

View reviewed changes

r0mant force-pushed the roman/dbprofile branch from fbb6ec8 to 335190f Compare January 19, 2021 18:01

r0mant requested a review from a-palchikov January 20, 2021 18:58

a-palchikov approved these changes Jan 20, 2021

View reviewed changes

webvictim approved these changes Jan 21, 2021

View reviewed changes

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

russjones approved these changes Jan 21, 2021

View reviewed changes

r0mant added 3 commits January 21, 2021 15:06

Refactor postgres service file handling, add db config command

9851e41

Address review comments

4be159b

Fix

6acc78f

r0mant force-pushed the roman/dbprofile branch from 335190f to 6acc78f Compare January 21, 2021 23:08

r0mant merged commit f7e6838 into master Jan 21, 2021

r0mant deleted the roman/dbprofile branch January 21, 2021 23:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor postgres service file handling, add db config command #5319

Refactor postgres service file handling, add db config command #5319

r0mant commented Jan 15, 2021 •

edited

a-palchikov Jan 18, 2021

r0mant Jan 19, 2021

a-palchikov Jan 18, 2021

r0mant Jan 19, 2021

a-palchikov Jan 20, 2021

a-palchikov Jan 18, 2021

r0mant Jan 19, 2021

a-palchikov Jan 20, 2021

r0mant Jan 20, 2021

a-palchikov Jan 18, 2021

r0mant Jan 19, 2021

r0mant commented Jan 20, 2021

a-palchikov left a comment

a-palchikov Jan 20, 2021

a-palchikov Jan 20, 2021

russjones Jan 21, 2021

r0mant Jan 21, 2021

Refactor postgres service file handling, add db config command #5319

Refactor postgres service file handling, add db config command #5319

Conversation

r0mant commented Jan 15, 2021 • edited

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

r0mant commented Jan 20, 2021

a-palchikov 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

r0mant commented Jan 15, 2021 •

edited