Skip to content

DB multi-session MFA Part 1: implement basic "tsh db exec"#53296

Merged
greedy52 merged 26 commits intomasterfrom
STeve/51679_base_tsh_db_exec
Apr 24, 2025
Merged

DB multi-session MFA Part 1: implement basic "tsh db exec"#53296
greedy52 merged 26 commits intomasterfrom
STeve/51679_base_tsh_db_exec

Conversation

@greedy52
Copy link
Copy Markdown
Contributor

@greedy52 greedy52 commented Mar 21, 2025

related:

This PR implements basic tsh db exec without the MFA part

Usage examples:

Simple: selection by names and sequential
$ cat test.sql 
select @@hostname

$ tsh db exec "source test.sql"  --dbs mysql1,mysql2 --db-user teleport-admin
Fetching databases ...

Executing command for "mysql1".
@@hostname
64f979fb76c9

Executing command for "mysql2".
@@hostname
6bc83bfda8d1

Summary: 2 of 2 succeeded.
Search by labels, and run in parallel
$ tsh db exec "source ./test.sql" --db-user teleport-admin --labels env=dev --max-connections 4 --output-dir logs
Searching databases ...
Found 4 database(s):

Name   Description Protocol Labels  
------ ----------- -------- ------- 
mysql1             mysql    env=dev 
mysql2             mysql    env=dev 
mysql3             mysql    env=dev 
mysql4             mysql    env=dev 

Do you want to proceed with 4 databases? [y/N]: y
Executing command for "mysql2". Output will be saved at "logs/mysql2.output".
Executing command for "mysql1". Output will be saved at "logs/mysql1.output".
Executing command for "mysql4". Output will be saved at "logs/mysql4.output".
Executing command for "mysql3". Output will be saved at "logs/mysql3.output".

Summary: 0 of 4 succeeded.
Summary is saved at "logs/summary.json".

$ cat exec-logs/mysql1.output
@@hostname
64f979fb76c9
Search by keyword, and sample summary.json
$ tsh db exec "source test.sql"  --search mymy --db-user teleport-admin --output-dir logs --no-confirm
Searching databases ...
Found 2 database(s):

Name   Description Protocol Labels   
------ ----------- -------- -------- 
mymy-1             mysql    env=prod 
mymy-2             mysql    env=prod 

Skipping confirmation for "Do you want to proceed with 2 databases?" due to the --no-confirm flag.
Executing command for "mymy-1". Output will be saved at "logs/mymy-1.output".
Executing command for "mymy-2". Output will be saved at "logs/mymy-2.output".

Summary: 2 of 2 succeeded.
Summary is saved at "logs/summary.json".

$ cat logs/summary.json 
{
  "databases": [
    {
      "database": {
        "service_name": "mymy-1",
        "protocol": "mysql",
        "username": "teleport-admin"
      },
      "command": "source test.sql",
      "output_file": "/Users/stevehuang/workspace/teleport-data/multi-session/logs/mymy-1.output",
      "success": true,
      "exit_code": 0
    },
    {
      "database": {
        "service_name": "mymy-2",
        "protocol": "mysql",
        "username": "teleport-admin"
      },
      "command": "source test.sql",
      "output_file": "/Users/stevehuang/workspace/teleport-data/multi-session/logs/mymy-2.output",
      "success": true,
      "exit_code": 0
    }
  ],
  "success": 2,
  "failure": 0,
  "total": 2
}

@greedy52 greedy52 force-pushed the STeve/51679_base_tsh_db_exec branch from 5398325 to a39bdbc Compare March 21, 2025 17:53
@greedy52 greedy52 changed the title Implement basic "tsh db exec" DB multi-session MFA Part 1: implement basic "tsh db exec" Mar 21, 2025
@greedy52 greedy52 self-assigned this Mar 21, 2025
@greedy52 greedy52 added the no-changelog Indicates that a PR does not require a changelog entry label Mar 21, 2025
@greedy52 greedy52 force-pushed the STeve/51679_base_tsh_db_exec branch from 2481051 to c7cbb50 Compare March 22, 2025 02:02
Comment thread api/types/resource.go Outdated
@greedy52 greedy52 force-pushed the STeve/51679_base_tsh_db_exec branch from 0d40eed to f4e7900 Compare March 24, 2025 15:44
@greedy52 greedy52 requested a review from zmb3 March 24, 2025 16:00
@greedy52 greedy52 marked this pull request as ready for review March 24, 2025 16:00
@github-actions github-actions Bot added size/lg tsh tsh - Teleport's command line tool for logging into nodes running Teleport. labels Mar 24, 2025
@github-actions github-actions Bot requested review from gzdunek and kimlisa March 24, 2025 16:01
@kimlisa kimlisa removed their request for review March 24, 2025 21:00
Comment thread tool/tsh/common/tsh.go Outdated
Comment thread tool/tsh/common/tsh.go Outdated
Comment thread tool/tsh/common/tsh.go Outdated
Comment thread tool/tsh/common/db_exec.go Outdated
Comment thread tool/tsh/common/db_exec.go Outdated
Comment thread tool/tsh/common/db_exec.go Outdated
Comment thread tool/tsh/common/db_exec.go Outdated
Comment thread tool/tsh/common/db_print.go Outdated
Copy link
Copy Markdown
Contributor

@Tener Tener left a comment

Choose a reason for hiding this comment

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

Really cool feature, I can see using it myself in the future 👍

I gave it some thought though and I think we should optimise for multi-database, scriptable experience. To me, that means a few features:

  1. Fault tolerance. It shouldn't be a fatal error if a subset of databases is unreachable.
  2. Machine readable output. As a user, I want predictable way of processing the script results further. More on that below.
  3. Some sort of summary once execution finishes. For each accessed database information about command executed, output files (if any), time, exit code etc.
  4. (Nice to have) Configurable timeouts for individual commands.
  5. (Nice to have) Configurable command to run.
  6. (Nice to have) A way to see which database names were provided, but couldn't be found; I guess a line in execution summary?

Two main modes for output, to me, are:

  1. Split output
  • one file per executed command.
  • based in output dir
  • execution summary file, if requested, json format
  1. Combined output
  • default to stdout, but may be redirected to file using flag. MFA prompts etc. would still use stdout!
  • each line prefixed with the unique database name
  • may use JSON/CSV(?)
  • also optional summary

Comment thread tool/tsh/common/tsh.go Outdated
Comment thread tool/tsh/common/tsh.go Outdated
Comment thread tool/tsh/common/tsh.go Outdated
Comment thread tool/tsh/common/tsh.go Outdated
Comment thread tool/tsh/common/db_exec.go Outdated
Comment thread tool/tsh/common/db_exec.go Outdated
Comment thread tool/tsh/common/db_exec.go Outdated
Comment thread tool/tsh/common/db_exec.go Outdated
Comment thread tool/tsh/common/db_exec.go
Comment thread tool/tsh/common/tsh.go Outdated
Comment thread tool/tsh/common/db_exec.go
Comment thread tool/tsh/common/db_exec.go Outdated
@greedy52
Copy link
Copy Markdown
Contributor Author

Friendly ping @gzdunek @r0mant

@greedy52 greedy52 force-pushed the STeve/51679_base_tsh_db_exec branch 2 times, most recently from 28087c8 to 1398bc7 Compare April 17, 2025 18:37
@greedy52 greedy52 removed the size/lg label Apr 17, 2025
@greedy52 greedy52 force-pushed the STeve/51679_base_tsh_db_exec branch from 1398bc7 to 64e6c76 Compare April 17, 2025 18:42
Copy link
Copy Markdown
Collaborator

@r0mant r0mant left a comment

Choose a reason for hiding this comment

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

lgtm with a few comments

Comment thread lib/client/db/dbcmd/exec.go Outdated
Comment thread lib/client/db/dbcmd/exec.go Outdated
case defaults.ProtocolMySQL:
return c.getMySQLExecCommand(query)
default:
return nil, trace.BadParameter("unsupported database protocol: %v", c.db)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's try to make errors less generic.

Suggested change
return nil, trace.BadParameter("unsupported database protocol: %v", c.db)
return nil, trace.NotImplemented("%q databases do not support exec commands yet", c.db)

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.

it was pointed out earlier that tsh will catch NotImplemented error and print an error stating server needs to be updated:

if trace.IsNotImplemented(err) {
return handleUnimplementedError(ctx, err, cf)
}

I've updated the error message but kept trace.BadParameter for now. we should improve the way tsh handles NotImplemented in the future.

Comment thread tool/tsh/common/db.go Outdated
Comment thread tool/tsh/common/db_print.go Outdated
Comment thread tool/tsh/common/tsh.go
}
}()

return trace.Wrap(group.Wait())
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is there any sort of timeout on the execution?

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.

no. the timeout will vary depending on the command to be executed. we can add a new flag in the future if a user asks for it. for now, user can always ctrl-c to kill it.

Comment thread tool/tsh/common/db_exec.go Outdated
Comment thread tool/tsh/common/db_exec.go Outdated
Comment thread tool/tsh/common/db_exec.go Outdated
Comment thread tool/tsh/common/db_exec.go Outdated
@greedy52 greedy52 force-pushed the STeve/51679_base_tsh_db_exec branch from 0325a54 to 1300f3d Compare April 24, 2025 19:04
@greedy52 greedy52 added this pull request to the merge queue Apr 24, 2025
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Apr 24, 2025
@greedy52 greedy52 added this pull request to the merge queue Apr 24, 2025
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Apr 24, 2025
@greedy52 greedy52 enabled auto-merge April 24, 2025 20:01
@greedy52 greedy52 added this pull request to the merge queue Apr 24, 2025
Merged via the queue into master with commit 238ff22 Apr 24, 2025
40 checks passed
@greedy52 greedy52 deleted the STeve/51679_base_tsh_db_exec branch April 24, 2025 20:48
greedy52 added a commit that referenced this pull request Jun 13, 2025
* Implement basic "tsh db exec"

* adding ut

* minor refactor, fix race, rename iter func

* add help

* always use service name

* overwrite max connections with env var

* single get databases call

* remove prefix output

* fix some flags

* iterutils

* ensure each database

* add summery

* refactor, tests

* revert auto rename change by editor

* revert migrate

* remove unused var

* review comments

* renaming --max-connections to --parallel

* make exec return result instead of error

* hint TELEPORT_PARALLEL_JOBS

* fix golint

* address PR comments
github-merge-queue Bot pushed a commit that referenced this pull request Jun 25, 2025
* DB multi-session MFA Part 1: implement basic "tsh db exec" (#53296)

* Implement basic "tsh db exec"

* adding ut

* minor refactor, fix race, rename iter func

* add help

* always use service name

* overwrite max connections with env var

* single get databases call

* remove prefix output

* fix some flags

* iterutils

* ensure each database

* add summery

* refactor, tests

* revert auto rename change by editor

* revert migrate

* remove unused var

* review comments

* renaming --max-connections to --parallel

* make exec return result instead of error

* hint TELEPORT_PARALLEL_JOBS

* fix golint

* address PR comments

* DB multi-session MFA Part 2: MFA reuse for GenerateUserCerts (#54069)

* enable multi-session mfa

* add ut

* add lib/client ut

* fix ut and proofread

* result.MFAResponse --> result.ReusableMFAResponse

* use errors.Is

* address comments

* fix logger

* fix test

* remove new tests from master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport/branch/v17 no-changelog Indicates that a PR does not require a changelog entry tsh tsh - Teleport's command line tool for logging into nodes running Teleport.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants