Skip to content

Unify errors returned from ProxyClient when targets are ambiguous#25004

Merged
rosstimothy merged 1 commit intomasterfrom
tross/fix_tsh_ambiguous_hosts
Apr 21, 2023
Merged

Unify errors returned from ProxyClient when targets are ambiguous#25004
rosstimothy merged 1 commit intomasterfrom
tross/fix_tsh_ambiguous_hosts

Conversation

@rosstimothy
Copy link
Copy Markdown
Contributor

Ensures that SSH and gRPC connections via tsh return the same error when dialing a host fails.

Closes #24943

Ensures that SSH and gRPC connections via `tsh` return the same
error when dialing a host fails.

Closes #24943
conn, details, err := c.transport.DialHost(ctx, target, cluster, nil, keyring)
if err != nil {
return nil, ClusterDetails{}, trace.Wrap(err)
return nil, ClusterDetails{}, trace.ConnectionProblem(err, "failed connecting to host %s: %v", target, err)
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.

This now matches the error that was returned from the SSH equivalent:

if err := session.RequestSubsystem(ctx, "proxy:"+targetAddress); err != nil {
// read the stderr output from the failed SSH session and append
// it to the end of our own message:
serverErrorMsg, _ := io.ReadAll(sessionError)
return nil, trace.ConnectionProblem(err, "failed connecting to host %s: %s. %v", targetAddress, serverErrorMsg, err)
}

@rosstimothy rosstimothy marked this pull request as ready for review April 21, 2023 19:47
@rosstimothy rosstimothy requested review from nklaassen and r0mant April 21, 2023 19:47
@github-actions github-actions Bot requested review from AntonAM and ravicious April 21, 2023 19:47
Copy link
Copy Markdown
Contributor

@nklaassen nklaassen left a comment

Choose a reason for hiding this comment

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

this seems good, but regarding the linked issue I really don't think we should be attempting relogin for all NotFound errors, we should probably only relogin for specific known errors

@rosstimothy
Copy link
Copy Markdown
Contributor Author

this seems good, but regarding the linked issue I really don't think we should be attempting relogin for all NotFound errors, we should probably only relogin for specific known errors

The linked issue seems to be more like two issues: 1 being that the gRPC connections broke ambiguous host handling and 2 NotFound triggers a re-login attempt. I'm a bit hesitant to address 2 as I don't have any idea if there is something else relying on that to work.

@rosstimothy rosstimothy added this pull request to the merge queue Apr 21, 2023
Merged via the queue into master with commit b6b57bc Apr 21, 2023
@rosstimothy rosstimothy deleted the tross/fix_tsh_ambiguous_hosts branch April 21, 2023 20:50
@public-teleport-github-review-bot
Copy link
Copy Markdown

@rosstimothy See the table below for backport results.

Branch Result
branch/v13 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

tsh attempts relogin for "ambiguous host" errors

3 participants