Skip to content

Connect: Fix premature proxyClient.Close() when getting db users#14230

Merged
ravicious merged 3 commits intomasterfrom
ravicious/db-users-proxy-client
Jul 12, 2022
Merged

Connect: Fix premature proxyClient.Close() when getting db users#14230
ravicious merged 3 commits intomasterfrom
ravicious/db-users-proxy-client

Conversation

@ravicious
Copy link
Copy Markdown
Member

In the previous version, the proxy client would be closed immediately
after addMetadataToRetryableError. This commit makes it so that the proxy
client is closed only after GetAllowedDatabaseUsers finishes.

When running Connect on Windows, Grzegorz ran into a problem where fetching
db users for MSSQL would fail but only on Windows and only for MSSQL:

Failed to fetch current user information: connection error:
desc = "transport: Error while dialing failed to dial: read tcp
10.211.55.4:55519->52.14.45.73:3023: use of closed network
connection". services\role.go:764

Other times the error would be

connection error: desc = "transport: Error while dialing failed
to dial: ssh: unexpected packet in response to channel open:
<nil>"] apiserver\middleware.go:39

Surprisingly, tsh db ls didn't have this problem. So when thinking about
what we're doing differently than tsh and how it might be related to
a closed connection, I noticed that I made a bug in the code that closes
the proxy client.


Full error log of the error that Grzegorz encountered on 8f477d4
08-07-22 13:47:58]: DEBU             Failed to fetch current user information: connection error: desc = "transport: Error while dialing failed to dial: read tcp 10.211.55.4:55519->52.14.45.73:3023: use of closed network connection". services\role.go:764
[08-07-22 13:47:58]: ERRO [CONN:APIS] Request failed. error:[
[08-07-22 13:47:58]: ERROR REPORT:
[08-07-22 13:47:58]: Original Error: *trace.ConnectionProblemError connection error: desc = "transport: Error while dialing failed to dial: read tcp 10.211.55.4:55519->52.14.45.73:3023: use of closed network connection"
[08-07-22 13:47:58]: Stack Trace:
[08-07-22 13:47:58]: 	C:/Users/grzegorz/Desktop/code/teleport/api/client/client.go:1470 github.com/gravitational/teleport/api/client.(*Client).GetRole
[08-07-22 13:47:58]: 	C:/Users/grzegorz/Desktop/code/teleport/lib/services/role.go:725 github.com/gravitational/teleport/lib/services.FetchRoleList
[08-07-22 13:47:58]: 	C:/Users/grzegorz/Desktop/code/teleport/lib/services/role.go:739 github.com/gravitational/teleport/lib/services.FetchRoles
[08-07-22 13:47:58]: 	C:/Users/grzegorz/Desktop/code/teleport/lib/services/role.go:774 github.com/gravitational/teleport/lib/services.FetchAllClusterRoles
[08-07-22 13:47:58]: 	C:/Users/grzegorz/Desktop/code/teleport/lib/teleterm/clusters/cluster_databases.go:176 github.com/gravitational/teleport/lib/teleterm/clusters.(*Cluster).GetAllowedDatabaseUsers
[08-07-22 13:47:58]: 	C:/Users/grzegorz/Desktop/code/teleport/lib/teleterm/apiserver/handler/handler_databases.go:58 github.com/gravitational/teleport/lib/teleterm/apiserver/handler.(*Handler).ListDatabaseUsers
[08-07-22 13:47:58]: 	C:/Users/grzegorz/Desktop/code/teleport/lib/teleterm/api/protogen/golang/v1/service.pb.go:2591 github.com/gravitational/teleport/lib/teleterm/api/protogen/golang/v1._TerminalService_ListDatabaseUsers_Handler.func1
[08-07-22 13:47:58]: 	C:/Users/grzegorz/Desktop/code/teleport/lib/teleterm/apiserver/middleware.go:37 github.com/gravitational/teleport/lib/teleterm/apiserver.withErrorHandling.func1
[08-07-22 13:47:58]: 	C:/Users/grzegorz/Desktop/code/teleport/lib/teleterm/api/protogen/golang/v1/service.pb.go:2593 github.com/gravitational/teleport/lib/teleterm/api/protogen/golang/v1._TerminalService_ListDatabaseUsers_Handler
[08-07-22 13:47:58]: 	C:/Users/grzegorz/go/pkg/mod/google.golang.org/grpc@v1.46.0/server.go:1283 google.golang.org/grpc.(*Server).processUnaryRPC
[08-07-22 13:47:58]: 	C:/Users/grzegorz/go/pkg/mod/google.golang.org/grpc@v1.46.0/server.go:1620 google.golang.org/grpc.(*Server).handleStream
[08-07-22 13:47:58]: 	C:/Users/grzegorz/go/pkg/mod/google.golang.org/grpc@v1.46.0/server.go:922 google.golang.org/grpc.(*Server).serveStreams.func1.2
[08-07-22 13:47:58]: 	C:/Program Files/Go/src/runtime/asm_amd64.s:1571 runtime.goexit
[08-07-22 13:47:58]: User Message: connection error: desc = "transport: Error while dialing failed to dial: read tcp 10.211.55.4:55519->52.14.45.73:3023: use of closed network connection"] apiserver\middleware.go:39

In the previous version, the proxy client would be closed immediately
after addMetadataToRetryableError. This commit makes it so that the proxy
client is closed only after GetAllowedDatabaseUsers finishes.

When running Connect on Windows, Grzegorz ran into a problem where fetching
db users for MSSQL would fail but only on Windows and only for MSSQL:

    Failed to fetch current user information: connection error:
    desc = "transport: Error while dialing failed to dial: read tcp
    10.211.55.4:55519->52.14.45.73:3023: use of closed network
    connection". services\role.go:764

Other times the error would be

    connection error: desc = "transport: Error while dialing failed
    to dial: ssh: unexpected packet in response to channel open:
    <nil>"] apiserver\middleware.go:39

Surprisingly, `tsh db ls` didn't have this problem. So when thinking about
what we're doing differently than tsh and how it might be related to
a closed connection, I noticed that I made a bug in the code that closes
the proxy client.
@ravicious
Copy link
Copy Markdown
Member Author

No idea how this managed to work fine on macOS all this time. 🤔

@ravicious
Copy link
Copy Markdown
Member Author

ravicious commented Jul 8, 2022

Well, it looks like it might not work for some users even on macOS after all: https://gravitational.slack.com/archives/C03FZRWDBR7/p1657294600272999

Edit: It seems that it has problems with Teleport 9.3.8 but not 9.3.6, see the Slack thread.

@ravicious ravicious requested a review from avatus July 12, 2022 09:59
@ravicious
Copy link
Copy Markdown
Member Author

It'd be great to include this one in the nearest patch release for v10.

@ravicious ravicious enabled auto-merge (squash) July 12, 2022 14:42
@ravicious ravicious merged commit 80a4237 into master Jul 12, 2022
@github-actions
Copy link
Copy Markdown
Contributor

@ravicious See the table below for backport results.

Branch Result
branch/v10 Create PR

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.

3 participants