Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add --cluster flag to all tsh db subcommands, Add "--diag_addr" flag to teleport db/app start #9220

Merged
merged 11 commits into from
Dec 9, 2021

Conversation

greedy52
Copy link
Contributor

@greedy52 greedy52 commented Dec 3, 2021

closes #8653 #8943

Changes

  • Added --diag_addr to teleport db/app start
  • Added --cluster flag to all tsh db subcommands
  • Updated tsh db login/ls" to show --cluster ` in corresponding section, if cluster flag is defined
  • Fixed a few places cf.HomePath should be used instead of "", for StatusCurrent and SaveProfile calls

Testing

Setup

  • two local clusters, one joins another
  • local mongodb, and local postgres

Tested

  • all tsh db subcommands works as expected
    • with or without --cluster flag
    • without logging into second/leaf cluster
  • tsh ssh works as its is

lib/client/api.go Outdated Show resolved Hide resolved
lib/client/api.go Show resolved Hide resolved
Comment on lines +477 to +479
if clusterName == "" || clusterName == p.Cluster {
return p.Databases, nil
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this special case needed? Maybe we can just get rid of p.Databases field completely since profile now has a method for returning databases for the specified cluster?

Copy link
Contributor Author

@greedy52 greedy52 Dec 6, 2021

Choose a reason for hiding this comment

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

I just found that tsh logut is using calling dbprofile.Delete on each of profile.Databases

I can certainly change this to use the new function. But this is a problem if we have logined db from another cluster, then we are not calling dbprofile.Delete on dbs in other clusters.

i think this is an existing problem though

Copy link
Contributor Author

@greedy52 greedy52 Dec 6, 2021

Choose a reason for hiding this comment

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

Added issue #9250 for tracking the dbprofile.Delete problem

I found a few places where p.Databases are still used. Mainly when we looping through all profiles.

For example, in tsh logout, we are trying to get databases in each profile

for _, db := range profile.Databases {

Similarly, we printStatus are all profiles and their databases (default cluster)

for _, p := range profiles {

I am debating to keep this special case for these scenarios for now. I think we can remove it once we decide what to do for the issue added above

lib/client/api.go Outdated Show resolved Hide resolved
lib/client/keyagent.go Outdated Show resolved Hide resolved
}

// addKey activates a new signed session key by adding it into the keystore.
func (a *LocalKeyAgent) addKey(key *Key) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should pick a more explicative name?

Copy link
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.

Could you test the tsh db --cluster connect command for MySQL. It seems user my.cnf mysql profile name is generated for root cluster and needs to be aligned to work correctly.

@greedy52
Copy link
Contributor Author

greedy52 commented Dec 7, 2021

Could you test the tsh db --cluster connect command for MySQL. It seems user my.cnf mysql profile name is generated for root cluster and needs to be aligned to work correctly.

Good catch @smallinsky! it's using profile's cluster. Let me fix and will test locally

@greedy52 greedy52 requested review from smallinsky and r0mant December 7, 2021 21:14
@greedy52 greedy52 merged commit 4e3f795 into master Dec 9, 2021
@greedy52 greedy52 deleted the STeve/8653-8943-more-flags branch December 9, 2021 16:24
greedy52 added a commit that referenced this pull request Dec 21, 2021
* Use t.Setenv in tests (#9154)

This new feature in Go 1.17 automatically restores the environment
variable to its previous value when a test ends, making it simpler
to set up the environment for tests and less likely that we accidentally
leave behind global state.

Also convert some of the remaining uses of check to standard Go tests.

* use shorter temp dir for socket path

Co-authored-by: Zac Bergquist <[email protected]>

Also ported a fix from #9220 to use short path for unix path in key_agent.test
greedy52 added a commit that referenced this pull request Dec 21, 2021
…lag to `teleport db/app start` (#9220)

* add diag to teleport db/app start

* db --cluster flag supports

* add some ut and fix issue ~/.tsh get removed during test

* working mongodb

* fix logout

* fix ut

* code review comment

* fix mysql
@greedy52 greedy52 mentioned this pull request Dec 21, 2021
greedy52 added a commit that referenced this pull request Dec 22, 2021
@webvictim webvictim mentioned this pull request Mar 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

diag_addr parameter not available for teleport db start or teleport app start
4 participants