Skip to content

update tsh db resource selection#28505

Merged
GavinFrazar merged 2 commits intomasterfrom
gavinfrazar/tsh-resource-selection-ux
Jul 14, 2023
Merged

update tsh db resource selection#28505
GavinFrazar merged 2 commits intomasterfrom
gavinfrazar/tsh-resource-selection-ux

Conversation

@GavinFrazar
Copy link
Copy Markdown
Contributor

@GavinFrazar GavinFrazar commented Jun 30, 2023

This PR implements the tsh db and tsh proxy db UX for RFD 129:

resource selection

Specifically, the following commands have been changed to support --labels and --query (predicate lang query) flags when selecting a database, and additionally selects a database by name prefix:

  • tsh db login
  • tsh db logout
  • tsh db connect
  • tsh db env
  • tsh db config
  • tsh proxy db

tsh db ls

tsh db ls (without --verbose flag) will now look for a label teleport.internal/discovered-name and print that instead of the full database name.

This label will be added by the discovery service (with future changes to discovery service); its value will be the name of the database in AWS/GCP/Azure.

If tsh db ls --verbose is used, then the listings will ignore that label and just print whatever the database name is. This is to avoid printing verbose database names generated by the discovery service.

predicate lang

The hasPrefix predicate has been added to resource predicate parsing, e.g. --query 'hasPrefix(name, "foo")' queries for resources that have "foo" as a prefix of their name. tsh db <subcommand> will query for databases by prefix, unless an active (already logged in) cert is selected. For backwards compatibility, tsh will detect predicate parser errors and fallback to matching database names by prefix on client side instead of using hasPrefix.

backporting

This PR will be backported to v13 to avoid poor UX when users have an older tsh than the discovery service when new discovery service version starts appending a uniquely identifying suffix to discovered resources.

TODO in subsequent PRs:

  1. update tsh kube and tsh app to support the same
  2. update Discovery Service to modify discovered resources names, appending a suffix, and adding a label to each discovered resource: teleport.internal/discovered-name containing the originally discovered resource name.

Example session:

$ tsh db ls
Name         Description               Allowed Users Labels                                                                                                     Connect 
------------ ------------------------- ------------- ---------------------------------------------------------------------------------------------------------- ------- 
bar          RDS instance in us-west-1 [alice]       account-id=123456789012,endpoint-type=instance,engine-version=13.10,engine=postgres,env=dev,region=us-w...
bar          RDS instance in us-west-2 [alice]       account-id=123456789012,endpoint-type=instance,engine-version=13.10,engine=postgres,env=dev,region=us-w...         
foo          RDS instance in us-west-1 [alice]       account-id=123456789012,endpoint-type=instance,engine-version=13.10,engine=postgres,env=prod,region=us-...         
postgres-rds                           [alice]                                                                                                                          

# login by prefix
$ tsh db login post
Connection information for database "postgres-rds" has been saved.

You can now connect to it using the following command:

  tsh db connect postgres-rds

You can view the connect command for the native database CLI client:

  tsh db config --format=cmd postgres-rds

# "foo" is the "discovered name", the actual name is foo-rds-us-west-1-123456789012
$ tsh db login --query 'name == "foo"'
ERROR: database with query (name == "foo") not found, use 'tsh db ls' to see registered databases

# using the hasPrefix query manually works
$ tsh db login --query 'hasPrefix(name, "foo")'
Connection information for database "foo-rds-us-west-1-123456789012" has been saved.

You can now connect to it using the following command:

  tsh db connect foo-rds-us-west-1-123456789012

You can view the connect command for the native database CLI client:

  tsh db config --format=cmd foo-rds-us-west-1-123456789012

# "bar" is a prefix that matches multiple databases, so we get an error message:
$ tsh db login bar
ERROR: database "bar" matches multiple databases:

Name                           Description               Protocol Type URI                                                   Allowed Users Labels                                                                                                                                                                    Connect 
------------------------------ ------------------------- -------- ---- ----------------------------------------------------- ------------- ------------------------------------------------------------------------------------------------------------------------------------------------------------------------- ------- 
bar-rds-us-west-1-123456789012 RDS instance in us-west-1 postgres rds  bar.abcdefghijklmnop.us-west-1.rds.amazonaws.com:5432 (unknown)     account-id=123456789012,endpoint-type=instance,engine-version=13.10,engine=postgres,env=dev,region=us-west-1,teleport.internal/discovered-name=bar,teleport.dev/origin=dynamic         
bar-rds-us-west-2-123456789012 RDS instance in us-west-2 postgres rds  bar.abcdefghijklmnop.us-west-1.rds.amazonaws.com:5432 (unknown)     account-id=123456789012,endpoint-type=instance,engine-version=13.10,engine=postgres,env=dev,region=us-west-2,teleport.internal/discovered-name=bar,teleport.dev/origin=dynamic

Hint: use 'tsh db ls -v' or 'tsh db ls --format=[json|yaml]' to list all databases with full details.
Hint: try selecting the database with a more specific name (ex: tsh db login full-database-name).
Hint: try selecting the database with additional --labels or --query predicate.

# We can resolve the error by being more specific.
# We don't need to use prefix + labels + query to select the database, but we can.
$ tsh db login bar --labels region=us-west-1 --query 'labels.env == "dev"'
Connection information for database "bar-rds-us-west-1-123456789012" has been saved.

You can now connect to it using the following command:

  tsh db connect bar-rds-us-west-1-123456789012

You can view the connect command for the native database CLI client:

  tsh db config --format=cmd bar-rds-us-west-1-123456789012

# env/config only consider active certs - so we don't need to be more specific than "bar" to use those commands:
$ tsh db env bar
export PGDATABASE=postgres
export PGHOST=postgres.mars.local.gd
export PGPORT=6543
export PGSSLMODE=verify-full
export PGSSLKEY=/Users/gavin/.tsh/keys/mars.local.gd/martian
export PGSSLROOTCERT=/Users/gavin/.tsh/keys/mars.local.gd/cas/mars.local.gd.pem
export PGSSLCERT=/Users/gavin/.tsh/keys/mars.local.gd/martian-db/mars.local.gd/bar-rds-us-west-1-123456789012-x509.pem
export PGUSER=alice
export PGGSSENCMODE=disable

$ tsh db config bar
Name:      bar-rds-us-west-1-123456789012
Host:      postgres.mars.local.gd
Port:      6543
User:      alice
Database:  postgres
CA:        /Users/gavin/.tsh/keys/mars.local.gd/cas/mars.local.gd.pem
Cert:      /Users/gavin/.tsh/keys/mars.local.gd/martian-db/mars.local.gd/bar-rds-us-west-1-123456789012-x509.pem
Key:       /Users/gavin/.tsh/keys/mars.local.gd/martian

# We can just be specific enough to login to the other "bar": one is in us-west-1, the other us-west-2.
$ tsh db login bar --labels region=us-west-2
Connection information for database "bar-rds-us-west-2-123456789012" has been saved.

You can now connect to it using the following command:

  tsh db connect bar-rds-us-west-2-123456789012

You can view the connect command for the native database CLI client:

  tsh db config --format=cmd bar-rds-us-west-2-123456789012

# we're now logged into all these databases...
$ tsh db ls
Name                                      Description               Allowed Users Labels                                      Connect                                   
----------------------------------------- ------------------------- ------------- ------------------------------------------- ----------------------------------------- 
> bar (user: alice, db: postgres)         RDS instance in us-west-1 [alice]       account-id=123456789012,endpoint-type=in... tsh db connect bar-rds-us-west-1-12345... 
> bar (user: alice, db: postgres)         RDS instance in us-west-2 [alice]       account-id=123456789012,endpoint-type=in... tsh db connect bar-rds-us-west-2-12345... 
> foo (user: alice, db: postgres)         RDS instance in us-west-1 [alice]       account-id=123456789012,endpoint-type=in... tsh db connect foo-rds-us-west-1-12345... 
> postgres-rds (user: alice, db: postg...                           [alice]                                                   tsh db connect postgres-rds               

# we can logout of a subset of dbs by prefix
$ tsh db logout bar
Logged out of databases:
bar-rds-us-west-1-123456789012
bar-rds-us-west-2-123456789012

# or labels/query if we want:
$ tsh db logout --labels env=prod
Logged out of database foo-rds-us-west-1-123456789012

# with no selectors, logout of everything. It prints the name if we just logged out of one.
$ tsh db logout
Logged out of database postgres-rds

Related issue:

Changelog: tsh db commands can choose a database using a prefix of the database's name instead of requiring the full database name. Additionally, --labels and --query (predicate lang) can be used to narrow down the intended database selection when multiple databases match a given prefix name. Lastly, if the given "prefix" matches any database name fully, then that database is selected over any other databases that have a name starting with the prefix, e.g. tsh db login foo-db will choose database foo-db and ignore database foo-db-2.

@GavinFrazar GavinFrazar added backport/branch/v13 ux tsh tsh - Teleport's command line tool for logging into nodes running Teleport. database-access Database access related issues and PRs labels Jun 30, 2023
@github-actions github-actions Bot requested a review from alexfornuto June 30, 2023 00:43
@github-actions github-actions Bot requested a review from camscale June 30, 2023 00:43
@GavinFrazar GavinFrazar requested review from smallinsky and removed request for alexfornuto, camscale, lsgunn-teleport, ptgott and xinding33 June 30, 2023 00:52
Copy link
Copy Markdown
Contributor

@smallinsky smallinsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First quick pass looks reasonable. I have left some comment and I will take a second look on Monday.

Additionally I think that other cli should be also updated like tctl that allows to operate on db object by name and the Discovery Name fix will also impact Teleport web UI and Teleport Connect.

I would love to test this change with backend alignment to check backward compatibility.

Comment thread tool/tsh/common/db.go Outdated
Comment thread tool/tsh/common/tsh.go Outdated
Comment thread api/types/constants.go Outdated
@smallinsky smallinsky self-requested a review June 30, 2023 12:07
Copy link
Copy Markdown
Contributor

@smallinsky smallinsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR LGTM but I wonder about your thoughts about an alternative approach that will be less "aggressive" in terms of tsh,tctl web UI integrations.

Namely, right now we are changing the database.Name value and setting it to the full resource name and introducing the DiscoveredNameLabel that contains Short Database Name (User Readable db name prefix) that needs to be aligned on client sides.

but by switching this logic we can leave the database.Name unchanged and add a new filed like databse.FullName that will be used as the backend key for db resource objects and needs to be aligned on client sides.

So even if for old tsh clients like 13.0.0 without this alignment the legacy flow will be unchanged.

@GavinFrazar
Copy link
Copy Markdown
Contributor Author

The PR LGTM but I wonder about your thoughts about an alternative approach that will be less "aggressive" in terms of tsh,tctl web UI integrations.

Namely, right now we are changing the database.Name value and setting it to the full resource name and introducing the DiscoveredNameLabel that contains Short Database Name (User Readable db name prefix) that needs to be aligned on client sides.

but by switching this logic we can leave the database.Name unchanged and add a new filed like databse.FullName that will be used as the backend key for db resource objects and needs to be aligned on client sides.

So even if for old tsh clients like 13.0.0 without this alignment the legacy flow will be unchanged.

I worry that changing the backend key like this will create problems and make rollbacks difficult.

Here's why:

  1. if we change the backend key to use database.FullName, then we must also change the certs to use database.FullName in RouteToDatabase. If a rollback occurs, all user db certs become invalid because they have database route names that no longer correspond to proxied databases
  2. if an auth server rollback occurs, every discovery agent and database agent will have invalid cache, and special care will be needed to handle databases that no longer have a populated FullName.
  3. assuming the discovery and database services can handle the auth server rollback gracefully, all databases with the same cloud name will collide and we run into the same issues that the naming collision is intended to avoid. Customers will suddenly find that those databases are unavailable.
  4. In addition to all of that, it's possible to have multiple auth servers, and if they are on different versions then I think there will be problems if they write to different backend keyspaces for database objects

Consider instead the scenario where we don't change the backend key, and just add a label like I'm doing in this implementation:

  1. If a customer has to rollback the auth server, database and discovery agents will not have invalid cache, and can continue proxying databases normally.
  2. discovered databases will continue to be distinguished by the longer name, no information is lost and clients like tsh are unaffected. Even their logged in db certs will still be valid

| `exists(labels["env"])` | resources with a label key `env`; label value unchecked |
| `!exists(labels["env"])` | resources without a label key `env`; label value unchecked |
| `search("foo", "bar", "some phrase")` | fuzzy match against common resource fields |
| `hasPrefix(name, "foo")` | resources with a name that starts with the prefix `foo` |
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What kind of backwards compatibility consequences does adding this new function have?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the tsh version is newer than auth/proxy version, then tsh will get a predicate error when it tries to use it. For that reason, I have logic in tsh that detects this scenario and falls back to just filtering resources client side without using hasPrefix predicate to make the API list resources request.

@GavinFrazar GavinFrazar force-pushed the gavinfrazar/tsh-resource-selection-ux branch from 9934e02 to 88c5196 Compare July 14, 2023 22:21
* add --labels and --query to tsh db subcommands
* tsh db [login | logout | env | config | connect]
* tsh proxy db

* add hasPrefix to predicate lang

* add teleport.dev/discovered-name
* print "discovered name" of databases discovered by discovery service,
  which is the name of the database resource in the cloud, when using
  tsh db ls without --verbose flag. This avoids printing verbose
  uniquely identifying names when discovery service is updated to append
  a uniquely identify suffix to discovered databases in AWS/Azure/GCP.
* tsh db ls --verbose ignores the label
* fix db connect string in tsh db ls

* select database by prefix, labels, and/or query predicate.
* chooses active database by exact match if the "prefix" matches exactly
  and no labels/predicate is given.
* logout of a subset of databases with tsh db logout.
* print an "ambiguous match" error if prefix/labels/query matches
  multiple databases where one is required.
* move all --labels cli flags to cf.Labels from cf.UserHost

* update tsh db tests
    * speedup slow tsh db tests
    * postgres/mysql profile respect home dir
    * rename test cases for consistency
    * test database listing uses discovered-name
    * test login/env/config/logout with prefix/label/predicate selectors
    * test active db filtering logic
@GavinFrazar GavinFrazar force-pushed the gavinfrazar/tsh-resource-selection-ux branch from 88c5196 to 0abf7a0 Compare July 14, 2023 22:30
@GavinFrazar GavinFrazar enabled auto-merge July 14, 2023 22:30
@GavinFrazar GavinFrazar added this pull request to the merge queue Jul 14, 2023
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 14, 2023
@GavinFrazar GavinFrazar added this pull request to the merge queue Jul 14, 2023
Merged via the queue into master with commit 2b2de5c Jul 14, 2023
@GavinFrazar GavinFrazar deleted the gavinfrazar/tsh-resource-selection-ux branch July 14, 2023 23:59
@public-teleport-github-review-bot
Copy link
Copy Markdown

@GavinFrazar See the table below for backport results.

Branch Result
branch/v13 Failed

GavinFrazar added a commit that referenced this pull request Jul 15, 2023
github-merge-queue Bot pushed a commit that referenced this pull request Jul 25, 2023
* [v13] update tsh db resource selection

Backport #28505 to branch/v13

* [v13] Fix MySQL and Postgres opt file path override

Backports #29220 to branch/v13
@ravicious
Copy link
Copy Markdown
Member

Let me know if this was brought up during the review, but I think this PR introduced a regression.

I have two databases, postgres and postgres2, and I can no longer interact with postgres by just doing stuff like tsh db login postgres. I made an issue for this, #29673.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

database-access Database access related issues and PRs documentation size/md tsh tsh - Teleport's command line tool for logging into nodes running Teleport. ux

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants