Skip to content

Support for benchmarking web sessions#24864

Merged
rosstimothy merged 1 commit intomasterfrom
tross/bench_web
Jun 23, 2023
Merged

Support for benchmarking web sessions#24864
rosstimothy merged 1 commit intomasterfrom
tross/bench_web

Conversation

@rosstimothy
Copy link
Copy Markdown
Contributor

@rosstimothy rosstimothy commented Apr 19, 2023

Adds tsh bench web ssh to allow benchmarking ssh sessions that are created via the web api. To prevent import cycles between lib/web and lib/client the cookie implementation in lib/web/cookie.go was moved into its own package lib/web/session. There is curerntly no support for SSO users - adding a local server to handle the login was out of scope and can be added in the future.

@rosstimothy rosstimothy force-pushed the tross/bench_web branch 4 times, most recently from 722e8d2 to 715c66a Compare May 1, 2023 23:08
@rosstimothy rosstimothy force-pushed the tross/bench_web branch 5 times, most recently from 00632c5 to a0e6715 Compare June 12, 2023 18:58
@rosstimothy rosstimothy marked this pull request as ready for review June 12, 2023 19:13
@github-actions github-actions Bot added size/lg tsh tsh - Teleport's command line tool for logging into nodes running Teleport. labels Jun 12, 2023
@github-actions github-actions Bot requested review from lxea and ravicious June 12, 2023 19:14
Comment thread lib/client/weblogin.go Outdated
Comment thread lib/client/api.go
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This seems to be a lot of repeated auth code from regular tsh versions of the same APIs, but what can I say – we did the same thing in lib/teleterm. I wish I was more proficient in Go to suggest a better alternative off the top of my head.

Do you think any non-benchmark code will ever want to use those web api methods? My assumption is that this will only ever be used in specialized cases such as benchmarking.

From the perspective of someone who regularly dives into the code to answer the question "How does tsh do X?", I think adding those methods directly in lib/client/api.go will make it harder to understand which variant of auth methods should be used for certain scenarios.

What do you think of moving those methods to lib/client/webapi.go with a comment at the top explaining how these methods are different from their regular counterparts and in which scenarios they should be used?

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.

Do you think any non-benchmark code will ever want to use those web api methods? My assumption is that this will only ever be used in specialized cases such as benchmarking.

I don't anticipate that anything outside of the benchmarks will likely ever need to create web sessions from tsh.

What do you think of moving those methods to lib/client/webapi.go with a comment at the top explaining how these methods are different from their regular counterparts and in which scenarios they should be used?

Our authentication story is unfortunately not very uniform, see #20343 for more detail. There isn't much fundamentally different between WebLoginFunc implementations and SSHLoginFunc implementations. They both interact with the web api to authenticate the user, the only difference being that the former results in a web session being created and returned via a cookie. I don't know if lib/client/webapi.go would be a better place than lib/client/api.go for either of these implementations to exist, it would probably make sense for them both to exist in a web api client and not something implemented on TeleportClient. We do have api/client/webclient which exists for that purpose but its quite limited in functionality.

@rosstimothy rosstimothy force-pushed the tross/bench_web branch 4 times, most recently from 795deec to 060b3dd Compare June 23, 2023 14:34
@rosstimothy rosstimothy enabled auto-merge June 23, 2023 14:34
Adds `tsh bench web ssh` to allow benchmarking ssh sessions that are
created via the web api. To prevent import cycles between `lib/web`
and `lib/client` the cookie implementation in `lib/web/cookie.go`
was moved into its own package `lib/web/session`. There is curerntly
no support for SSO users - adding a local server to handle the login
was out of scope and can be added in the future.
@rosstimothy rosstimothy added this pull request to the merge queue Jun 23, 2023
Merged via the queue into master with commit 2593843 Jun 23, 2023
@rosstimothy rosstimothy deleted the tross/bench_web branch June 23, 2023 15:26
rosstimothy added a commit that referenced this pull request Jun 30, 2023
#24864 added a dependency of lib/web into tsh which broke windows
tsh builds because lib/web transiently depends on lib/srv which
has linux specific code. This shuffles around a few things so
that lib/web is no longer importing lib/srv at all by:

- Indirectly using the srv.SessionController to apply session
control for web ssh sessions

- Moving the common reversetunnel interfaces into
reversetunnelclient since lib/reversetunnel imports
lib/srv/forward which imports lib/srv.

- Directly converting mysql client errors in the connection
tester instead of calling a common function.
github-merge-queue Bot pushed a commit that referenced this pull request Jun 30, 2023
#24864 added a dependency of lib/web into tsh which broke windows
tsh builds because lib/web transiently depends on lib/srv which
has linux specific code. This shuffles around a few things so
that lib/web is no longer importing lib/srv at all by:

- Indirectly using the srv.SessionController to apply session
control for web ssh sessions

- Moving the common reversetunnel interfaces into
reversetunnelclient since lib/reversetunnel imports
lib/srv/forward which imports lib/srv.

- Directly converting mysql client errors in the connection
tester instead of calling a common function.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/lg 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.

3 participants