Use cached remote clients in Connect by gzdunek · Pull Request #38202 · gravitational/teleport

gzdunek · 2024-02-14T11:30:20Z

This PR replaces calls to clusterClient.ConnectToProxy() with daemon.GetRemoteClient().
Most of the callsites now receive auth.ClientI that they can use to make remote calls.
This also aligns with the idea of refactoring clusters.Cluster. Most of its methods now no longer need clusterClient and can be easily extracted to smaller packages like unifiedresources.

Changelog: Improve performance of remote calls in Teleport Connect.

gzdunek · 2024-02-14T11:33:05Z

If I didn't remove the "old" client, the user's access hasn't been changed.

gzdunek · 2024-02-21T09:11:15Z

I didn't test the case when disconnect_expired_cert is false properly, in that situation the connection is not closed, and a call to GenerateUserCerts with expired cert returns *interceptors.RemoteError access denied: client credentials have expired, please relogin error. The user just sees 'access denied' message instead of a login modal.

To fix this, I should add this error to the list of retryable errors

teleport/web/packages/teleterm/src/ui/utils/retryWithRelogin.ts

Lines 95 to 100 in 1b2f271

    
           export function isRetryable(error: unknown): boolean { 
        
             // TODO(ravicious): Replace this with actual check on metadata. 
        
             return ( 
        
               error instanceof Error && 
        
               (error.message.includes('ssh: handshake failed') || 
        
                 error.message.includes('ssh: cert has expired'))

but since it is an access denied error, the actual message is removed and I'm not able to inspect it #32550.

I think I have to fix the linked issue first and then come back here :/

ravicious

Fantastic work. I managed to get through the first commit and QA the app a little bit. I'll continue the review tomorrow.

Note to myself: test access requests and headless auth.

…mote-client

…ix when needed

… gateways code

rosstimothy

Nice work @gzdunek!

public-teleport-github-review-bot · 2024-02-28T07:28:02Z

@gzdunek See the table below for backport results.

Branch	Result
branch/v15	Failed

* Add remote client cache * Add an integration test * Close all clients when stopping the service * Move RemoteClientCache to the place where it is used * Do not check client cert in `Get` * Fix code style issues * Prevent potential race condition when removing a cached client * Test concurrent calls to `Get` * Add TODO * `remoteclientcache` -> `clientcache` * Reduce the `err` scope * Move `Config` closer to `New` and docs * Fix lint * Improve logging and error handling * Add missing comments * `Close` -> `Clear` * Improve the test * Remove mentions about "remote" client * Pass `cfg` directly to `Cache` * `InvalidateForRootCluster` -> `ClearForRootCluster` * Add docs for the interface * `ClearForRootCluster` -> `ClearForRoot` * Add config validation * Log multiple fields at once * Improve setting logger * Use cached remote clients in Connect (#38202) * Replace all simple `c.clusterClient.ConnectToProxy()` calls * Use cached proxy client to create gateways * Use cached proxy client to assume roles * Invalidate clients when logging in and out * Gracefully handle expired cert error returned by the server * Drop `GetRootClusterURI` in headless auth watcher since URIs are already root URIs * Simplify error check * Make `auth.ClientI` parameter naming more consistent, use `root` prefix when needed * Reduce error scope where possible * Clear cached clients before passwordless login * Use `fakeClientCache` without pointers * Move separate `proxyClient` parameter to `CreateGatewayParams` in the gateways code * Replace checking error string with `client.ErrClientCredentialsHaveExpired`

* Add remote client cache * Add an integration test * Close all clients when stopping the service * Move RemoteClientCache to the place where it is used * Do not check client cert in `Get` * Fix code style issues * Prevent potential race condition when removing a cached client * Test concurrent calls to `Get` * Add TODO * `remoteclientcache` -> `clientcache` * Reduce the `err` scope * Move `Config` closer to `New` and docs * Fix lint * Improve logging and error handling * Add missing comments * `Close` -> `Clear` * Improve the test * Remove mentions about "remote" client * Pass `cfg` directly to `Cache` * `InvalidateForRootCluster` -> `ClearForRootCluster` * Add docs for the interface * `ClearForRootCluster` -> `ClearForRoot` * Add config validation * Log multiple fields at once * Improve setting logger * Use cached remote clients in Connect (#38202) * Replace all simple `c.clusterClient.ConnectToProxy()` calls * Use cached proxy client to create gateways * Use cached proxy client to assume roles * Invalidate clients when logging in and out * Gracefully handle expired cert error returned by the server * Drop `GetRootClusterURI` in headless auth watcher since URIs are already root URIs * Simplify error check * Make `auth.ClientI` parameter naming more consistent, use `root` prefix when needed * Reduce error scope where possible * Clear cached clients before passwordless login * Use `fakeClientCache` without pointers * Move separate `proxyClient` parameter to `CreateGatewayParams` in the gateways code * Replace checking error string with `client.ErrClientCredentialsHaveExpired` (cherry picked from commit 39f9951)

* Cache remote clients in Connect (#38201) * Add remote client cache * Add an integration test * Close all clients when stopping the service * Move RemoteClientCache to the place where it is used * Do not check client cert in `Get` * Fix code style issues * Prevent potential race condition when removing a cached client * Test concurrent calls to `Get` * Add TODO * `remoteclientcache` -> `clientcache` * Reduce the `err` scope * Move `Config` closer to `New` and docs * Fix lint * Improve logging and error handling * Add missing comments * `Close` -> `Clear` * Improve the test * Remove mentions about "remote" client * Pass `cfg` directly to `Cache` * `InvalidateForRootCluster` -> `ClearForRootCluster` * Add docs for the interface * `ClearForRootCluster` -> `ClearForRoot` * Add config validation * Log multiple fields at once * Improve setting logger * Use cached remote clients in Connect (#38202) * Replace all simple `c.clusterClient.ConnectToProxy()` calls * Use cached proxy client to create gateways * Use cached proxy client to assume roles * Invalidate clients when logging in and out * Gracefully handle expired cert error returned by the server * Drop `GetRootClusterURI` in headless auth watcher since URIs are already root URIs * Simplify error check * Make `auth.ClientI` parameter naming more consistent, use `root` prefix when needed * Reduce error scope where possible * Clear cached clients before passwordless login * Use `fakeClientCache` without pointers * Move separate `proxyClient` parameter to `CreateGatewayParams` in the gateways code * Replace checking error string with `client.ErrClientCredentialsHaveExpired` (cherry picked from commit 39f9951) * Temporarily disable flaky part of `TestClientCache` (#38798) (cherry picked from commit 7067a88) * Make calls to the auth server concurrently (#38955) * Make calls to the auth server concurrently * Enhance the comment about preferences upsert * Summarize how `userpreferences.Update` works (cherry picked from commit ddc45a4)

gzdunek added backport/branch/v13 labels Feb 14, 2024

gzdunek commented Feb 14, 2024

View reviewed changes

gzdunek mentioned this pull request Feb 14, 2024

Cache remote clients in Connect #38201

Merged

gzdunek marked this pull request as ready for review February 14, 2024 11:54

gzdunek requested a review from ravicious February 14, 2024 11:54

github-actions Bot added the size/sm label Feb 14, 2024

github-actions Bot requested review from lxea and probakowski February 14, 2024 11:54

gzdunek requested review from espadolini and rosstimothy and removed request for lxea and probakowski February 14, 2024 11:55

gzdunek marked this pull request as draft February 15, 2024 14:14

gzdunek force-pushed the gzdunek/use-cached-remote-client branch from 9adb3f6 to 5536464 Compare February 15, 2024 18:16

gzdunek marked this pull request as ready for review February 15, 2024 18:17

github-actions Bot requested review from codingllama and flyinghermit February 15, 2024 18:18

gzdunek removed request for codingllama and flyinghermit February 15, 2024 18:18

Replace all simple c.clusterClient.ConnectToProxy() calls

0043749

gzdunek force-pushed the gzdunek/use-cached-remote-client branch from 5536464 to ad7405d Compare February 20, 2024 14:30

gzdunek added 3 commits February 20, 2024 15:37

Use cached proxy client to create gateways

41557f6

Use cached proxy client to assume roles

b8a1a87

Invalidate clients when logging in and out

75cc2a3

gzdunek force-pushed the gzdunek/use-cached-remote-client branch from ad7405d to 75cc2a3 Compare February 20, 2024 14:37

ravicious reviewed Feb 21, 2024

View reviewed changes

Comment thread lib/teleterm/daemon/daemon_headless.go Outdated

Comment thread lib/teleterm/daemon/daemon.go

ravicious self-requested a review February 21, 2024 09:34

espadolini reviewed Feb 21, 2024

View reviewed changes

Comment thread lib/teleterm/apiserver/middleware.go Outdated

ravicious reviewed Feb 22, 2024

View reviewed changes

Comment thread lib/teleterm/clusters/cluster_databases.go

Comment thread lib/teleterm/daemon/daemon.go

ravicious self-requested a review February 22, 2024 17:06

gzdunek added 2 commits February 23, 2024 11:13

Simplify error check

b83cab4

Merge branch 'gzdunek/cache-remote-client' into gzdunek/use-cached-re…

2345801

…mote-client

gzdunek removed backport/branch/v13 labels Feb 23, 2024

rosstimothy reviewed Feb 23, 2024

View reviewed changes

ravicious reviewed Feb 23, 2024

View reviewed changes

Comment thread lib/teleterm/clusters/cluster_gateways.go Outdated

Comment thread lib/teleterm/clusters/gateway_creator.go

Comment thread lib/teleterm/apiserver/handler/handler_auth.go Outdated

Comment thread lib/teleterm/apiserver/handler/handler_auth.go Outdated

gzdunek added 4 commits February 26, 2024 13:35

Merge branch 'gzdunek/cache-remote-client' into gzdunek/use-cached-re…

a5271d3

…mote-client

Make auth.ClientI parameter naming more consistent, use root pref…

c9e8db3

…ix when needed

Reduce error scope where possible

6a02959

Clear cached clients before passwordless login

a3df293

This was referenced Feb 27, 2024

Fix a regression issue that caused the re-login dialog reason and proxy errors to not be displayed #38664

Merged

Treat "expired certs" errors returned from RPCs as retryable on the client side #38668

Merged

gzdunek requested review from espadolini, ravicious and rosstimothy February 27, 2024 13:27

gzdunek added 2 commits February 27, 2024 14:58

Use fakeClientCache without pointers

5badebb

Move separate proxyClient parameter to CreateGatewayParams in the…

04c43e9

… gateways code

ravicious approved these changes Feb 27, 2024

View reviewed changes

rosstimothy approved these changes Feb 27, 2024

View reviewed changes

espadolini approved these changes Feb 27, 2024

View reviewed changes

gzdunek merged commit 97c48cb into gzdunek/cache-remote-client Feb 28, 2024

gzdunek deleted the gzdunek/use-cached-remote-client branch February 28, 2024 07:27

gzdunek mentioned this pull request Mar 6, 2024

[v15] Cache remote clients in Connect #39012

Merged

Conversation

gzdunek commented Feb 14, 2024

Uh oh!

gzdunek Feb 14, 2024

Choose a reason for hiding this comment

Uh oh!

gzdunek commented Feb 21, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ravicious left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rosstimothy left a comment

Choose a reason for hiding this comment

Uh oh!

public-teleport-github-review-bot Bot commented Feb 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants