Skip to content

make tsh match against discovered db name#31689

Merged
GavinFrazar merged 2 commits intomasterfrom
gavinfrazar/tsh-match-against-discovered-name
Sep 13, 2023
Merged

make tsh match against discovered db name#31689
GavinFrazar merged 2 commits intomasterfrom
gavinfrazar/tsh-match-against-discovered-name

Conversation

@GavinFrazar
Copy link
Copy Markdown
Contributor

@GavinFrazar GavinFrazar commented Sep 11, 2023 •
edited
Loading

Fixes #31286

changelog: tsh commands can select a database by the original database name instead of the more detailed name generated by the v14+ Teleport Discovery service.

What

This PR changes tsh db commands to check for an exact match of the "discovered name" label that is added to discovered databases.

Why

The purpose of this change, is to avoid breaking any existing scripts after v14 discovery renaming goes live.

The logic here was getting quite complicated, and test coverage was insufficient, so I refactored to simplify the code and make it easier to unit test functions.

@GavinFrazar GavinFrazar added ux tsh tsh - Teleport's command line tool for logging into nodes running Teleport. database-access Database access related issues and PRs backport/branch/v13 labels Sep 11, 2023
@GavinFrazar GavinFrazar changed the title match against discovered name match against discovered db name Sep 11, 2023
@GavinFrazar GavinFrazar changed the title match against discovered db name make tsh match against discovered db name Sep 11, 2023
@GavinFrazar GavinFrazar requested review from gabrielcorado and removed request for fspmarshall and ibeckermayer September 11, 2023 03:47
smallinsky
smallinsky reviewed Sep 11, 2023
View reviewed changes
Comment thread tool/tsh/common/db.go Outdated
Comment thread tool/tsh/common/db.go Outdated
Comment thread tool/tsh/common/db.go
Comment thread tool/tsh/common/db.go Outdated
Comment thread tool/tsh/common/db.go Outdated
Comment thread tool/tsh/common/db.go Outdated
greedy52
greedy52 reviewed Sep 11, 2023
View reviewed changes
Comment thread tool/tsh/common/db.go Outdated
@GavinFrazar GavinFrazar force-pushed the gavinfrazar/tsh-match-against-discovered-name branch from 6cdc5eb to faed6ca Compare September 13, 2023 05:40
smallinsky
smallinsky approved these changes Sep 13, 2023
View reviewed changes
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.

Thanks for the fix. LGMT if tested.
I will do an additional testing later.

greedy52
greedy52 approved these changes Sep 13, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@greedy52 greedy52 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the update. Love it 👍

One small nit: maybe we can add debug log in important branches just in case we have to debug this in the field. Maybe the predicate used for the API too.

Comment thread tool/tsh/common/db.go Outdated
Comment thread tool/tsh/common/db_test.go Outdated
@GavinFrazar GavinFrazar force-pushed the gavinfrazar/tsh-match-against-discovered-name branch from e73a2b1 to d5131bb Compare September 13, 2023 17:01
@GavinFrazar
rework tsh database selection logic
98d9128 
* disallow prefix matching
* select by exact name match first,
* otherwise look for unambiguous discovered name label match.
* look for an active db to resolve discovered name match ambiguity.
* add more predicate builder helpers
* check for db name in not found error for stale cert hint
* no error status on tsh db logout with no logged in dbs
* remove dead code
* refactor helper funcs to simplify code and make it easier to test
* test complex database selection
* test findActiveDatabase
* test choosing one db by discovered name
* add more resource selectors tests
* test formatDatabaseLoginCommand
* add debug logging for db selection
@GavinFrazar GavinFrazar added this pull request to the merge queue Sep 13, 2023
@GavinFrazar GavinFrazar force-pushed the gavinfrazar/tsh-match-against-discovered-name branch from d5131bb to 98d9128 Compare September 13, 2023 17:36
@GavinFrazar GavinFrazar removed this pull request from the merge queue due to a manual request Sep 13, 2023
@GavinFrazar GavinFrazar added this pull request to the merge queue Sep 13, 2023
Merged via the queue into master with commit d433f96 Sep 13, 2023
@GavinFrazar GavinFrazar deleted the gavinfrazar/tsh-match-against-discovered-name branch September 13, 2023 18:13
@public-teleport-github-review-bot
Copy link
Copy Markdown

public-teleport-github-review-bot Bot commented Sep 13, 2023

@GavinFrazar See the table below for backport results.

Branch Result
branch/v13 Failed
branch/v14 Create PR

@GavinFrazar GavinFrazar mentioned this pull request Sep 13, 2023
Merged
GavinFrazar added a commit that referenced this pull request Sep 16, 2023
@GavinFrazar
[v13] rework tsh database selection logic
3af5902 
backports #31689 to branch/v13.

* disallow prefix matching
* select by exact name match first,
* otherwise look for unambiguous discovered name label match.
* look for an active db to resolve discovered name match ambiguity.
* add more predicate builder helpers
* check for db name in not found error for stale cert hint
* no error status on tsh db logout with no logged in dbs
* remove dead code
* refactor helper funcs to simplify code and make it easier to test
* test complex database selection
* test findActiveDatabase
* test choosing one db by discovered name
* add more resource selectors tests
* test formatDatabaseLoginCommand
* add debug logging for db selection
@GavinFrazar GavinFrazar mentioned this pull request Sep 16, 2023
Closed
@GavinFrazar GavinFrazar mentioned this pull request Sep 18, 2023
Merged
GavinFrazar added a commit that referenced this pull request Sep 19, 2023
@GavinFrazar
[v13] rework tsh database selection logic
0e5619a 
backports #31689 to branch/v13.

* disallow prefix matching
* select by exact name match first,
* otherwise look for unambiguous discovered name label match.
* look for an active db to resolve discovered name match ambiguity.
* add more predicate builder helpers
* check for db name in not found error for stale cert hint
* no error status on tsh db logout with no logged in dbs
* remove dead code
* refactor helper funcs to simplify code and make it easier to test
* test complex database selection
* test findActiveDatabase
* test choosing one db by discovered name
* add more resource selectors tests
* test formatDatabaseLoginCommand
* add debug logging for db selection
GavinFrazar added a commit that referenced this pull request Sep 19, 2023
@GavinFrazar
[v13] rework tsh database selection logic (#32092)
8b3a123 
backports #31689 to branch/v13.

* disallow prefix matching
* select by exact name match first,
* otherwise look for unambiguous discovered name label match.
* look for an active db to resolve discovered name match ambiguity.
* add more predicate builder helpers
* check for db name in not found error for stale cert hint
* no error status on tsh db logout with no logged in dbs
* remove dead code
* refactor helper funcs to simplify code and make it easier to test
* test complex database selection
* test findActiveDatabase
* test choosing one db by discovered name
* add more resource selectors tests
* test formatDatabaseLoginCommand
* add debug logging for db selection
github-merge-queue Bot pushed a commit that referenced this pull request Sep 19, 2023
@GavinFrazar @tigrato
[v13] Fix Kubernetes selected cluster (#32087)
04d8985 
* [v13] Fix Kubernetes selected cluster

backports #30167 to branch/v13.

* Fix Kubernetes selected cluster

Kubeconfig context name can be customized using `--set-context-name`
flag. When using it, the selected Kubernetes cluster fails to correctly
identify the cluster name.

* [v13] tsh kube login by prefix, query, labels (#32088)

backports #30252 to branch/v13.

* fetch kube clusters once for login

* [v13] update `tsh proxy kube` cluster selection ux (#32089)

backports #30478 to branch/v13.

* select by labels, query predicate, name, and/or prefix of name.
* fix --cluster flag not being propagated

* [v13] simplify tsh db prefix predicate logic (#32090)

backports #30531 to branch/v13.

* [v13] check for discovered kube cluster name exact match (#32091)

backports #31744 to branch/v13.

* [v13] rework tsh database selection logic (#32092)

backports #31689 to branch/v13.

* disallow prefix matching
* select by exact name match first,
* otherwise look for unambiguous discovered name label match.
* look for an active db to resolve discovered name match ambiguity.
* add more predicate builder helpers
* check for db name in not found error for stale cert hint
* no error status on tsh db logout with no logged in dbs
* remove dead code
* refactor helper funcs to simplify code and make it easier to test
* test complex database selection
* test findActiveDatabase
* test choosing one db by discovered name
* add more resource selectors tests
* test formatDatabaseLoginCommand
* add debug logging for db selection

* [v13] Fix `tsh kube login` when creds are expired or doesn't exist (#32095)

Backports #31418 to branch/v13.

This PR uses `client.RetryWithRelogin` helper to deal with cases where user's
credentials don't exist or are already expired.

Co-authored-by: Tiago Silva <tiago.silva@goteleport.com>

* remove tsh kube prefix matching (#31852) (#32097)

* fix retry with relogin for ambiguous clusters
* consolidate test setup for login/proxy kube selection tests
* add more test cases for kube selection
* remove prefix testing
* add origin cloud label in tests
* refactor the check for multiple cluster login into a func

* [v13] remove prefix matching in tctl (#32104)

Backports #31916 to branch/v13.

* remove prefix matching in tctl
* replace prefix matching with exact discovered name match as a fallback
  when no resource full name matches the name given by a user
* refactor test helpers
* avoid decoding yaml/json into already initialized var

---------

Co-authored-by: Tiago Silva <tiago.silva@goteleport.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@greedy52 greedy52 greedy52 approved these changes

@smallinsky smallinsky smallinsky approved these changes

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

tsh db connect <rds-primary-endpoint> throws matches multiple databases error

3 participants

@GavinFrazar @greedy52 @smallinsky