Skip to content

reduce login latency#27029

Closed
rosstimothy wants to merge 1 commit intomasterfrom
tross/tsh_login_improvements
Closed

reduce login latency#27029
rosstimothy wants to merge 1 commit intomasterfrom
tross/tsh_login_improvements

Conversation

@rosstimothy
Copy link
Copy Markdown
Contributor

@rosstimothy rosstimothy commented May 26, 2023

Builds on #26903 and #26968 to reduce login latency by reusing auth clients.

@rosstimothy
Copy link
Copy Markdown
Contributor Author

image

@smallinsky smallinsky self-requested a review May 26, 2023 16:26
@rosstimothy rosstimothy force-pushed the tross/tsh_login_improvements branch 2 times, most recently from 5dfc1a5 to 3861780 Compare May 26, 2023 18:21
Comment thread lib/client/api.go
Comment on lines -3824 to -3834
errViaJumphost := err
// If JumpHosts was pointing at the leaf cluster (e.g. during 'tsh ssh
// -J leaf.example.com'), this could've caused the above error. Try to
// fetch CAs without JumpHosts to force it to use the root cluster.
if err := tc.WithoutJumpHosts(func(tc *TeleportClient) error {
return tc.UpdateTrustedCA(ctx, rootClusterName)
}); err != nil {
return trace.NewAggregate(errViaJumphost, err)
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This fallback flow always creates a new TeleportClient and in this PR this logic was removed.
Not sure if this flow is 100% covered by UT or integration test but I'm afraid that by removing this fallback we will break the flow.

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's now the responsibility of the caller to ensure that the provided services.AuthorityGetter is connected to the root Auth service so in theory we shouldn't need to attempt the fallback any more. I'll add a test to cover this flow if one doesn't already exist.

Comment thread tool/tsh/tsh.go
Comment on lines +1780 to +1795
if err := tc.ActivateKeyWithoutTrustedCerts(cf.Context, key); err != nil {
return trace.Wrap(err)
}

clusterClient, err := tc.ConnectToCluster(cf.Context)
if err != nil {
return trace.Wrap(err)
}
defer clusterClient.Close()

rootAuth, err := clusterClient.RootClient(cf.Context)
if err != nil {
return trace.Wrap(err)
}
defer rootAuth.Close()

if err := tc.UpdateTrustedCA(cf.Context, rootAuth); err != nil {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: Can we move this to a separate function that will return clusterClient and rootAuth ?

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.

I'm not sure if moving it all into one function would really make things clearer. This is also no different than the same api we have for the legacy ProxyClient.

teleport/lib/client/api.go

Lines 3390 to 3400 in 8da636b

proxyClient, err := tc.ConnectToProxy(ctx)
if err != nil {
return nil, trace.Wrap(err)
}
defer proxyClient.Close()
authClient, err := proxyClient.ConnectToRootCluster(ctx)
if err != nil {
return nil, trace.Wrap(err)
}
defer authClient.Close()

Comment thread lib/client/api.go Outdated
@rosstimothy rosstimothy force-pushed the tross/tsh_login_improvements branch 2 times, most recently from c36569f to d62335c Compare June 1, 2023 15:17
@rosstimothy rosstimothy force-pushed the tross/tsh_login_improvements branch from d62335c to 063e7e2 Compare June 1, 2023 18:27
@rosstimothy rosstimothy deleted the tross/tsh_login_improvements branch February 4, 2025 14:20
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.

2 participants