Skip to content

Add gRPC error interceptors to API client#30578

Merged
Joerger merged 9 commits intomasterfrom
joerger/grpc-error-interceptor
Aug 24, 2023
Merged

Add gRPC error interceptors to API client#30578
Joerger merged 9 commits intomasterfrom
joerger/grpc-error-interceptor

Conversation

@Joerger
Copy link
Copy Markdown
Contributor

@Joerger Joerger commented Aug 16, 2023

Moves the gRPC error interceptors to api/utils/grpc/interceptors and uses them in the API client.

In a follow up PR, after https://github.com/gravitational/teleport.e/pull/2007 is merged, the alias functions in lib/utils/grpc.go will be removed.

@Joerger Joerger requested a review from dboslee August 16, 2023 19:58
@Joerger Joerger removed the request for review from AntonAM August 16, 2023 19:58
@Joerger Joerger requested a review from AntonAM August 16, 2023 19:58
@Joerger Joerger changed the base branch from master to joerger/remove-insecure-api-client-testing August 16, 2023 20:28
@Joerger Joerger force-pushed the joerger/grpc-error-interceptor branch 2 times, most recently from 3efb8e3 to a7d10e2 Compare August 16, 2023 20:38
Copy link
Copy Markdown
Contributor

@codingllama codingllama left a comment

Choose a reason for hiding this comment

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

Many thanks!

Let's make sure we backport this, so other backported code won't work under the wrong assumptions.

Comment thread api/client/inventory.go Outdated
Comment thread api/client/keepaliver.go Outdated
Comment thread api/utils/grpc/interceptors/errors.go Outdated
Comment thread api/utils/grpc/interceptors/errors_test.go Outdated
Comment thread lib/utils/grpc.go Outdated
@Joerger Joerger force-pushed the joerger/grpc-error-interceptor branch from 4f5c4dc to 9a32e65 Compare August 17, 2023 00:24
@Joerger Joerger changed the base branch from joerger/remove-insecure-api-client-testing to master August 17, 2023 00:24
Copy link
Copy Markdown
Contributor

@rosstimothy rosstimothy left a comment

Choose a reason for hiding this comment

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

I added similar behavior in #25694 and then reverted it in #26169. It inadvertently broke tsh request ls and caused error traces to be rooted at the interceptor instead of the client. So instead of something which showed exactly where the error came from like the following

ERROR REPORT:
Original Error: *trace.TraceErr EOF
Stack Trace:
	github.com/gravitational/teleport/api/client/client.go:875 github.com/gravitational/teleport/api/client.(*Client).GetAccessRequests
	github.com/gravitational/teleport/tool/tsh/access_request.go:55 main.onRequestList.func1
	github.com/gravitational/teleport/lib/client/api.go:1395 github.com/gravitational/teleport/lib/client.(*TeleportClient).WithRootClusterClient
	github.com/gravitational/teleport/tool/tsh/access_request.go:54 main.onRequestList
	github.com/gravitational/teleport/tool/tsh/tsh.go:1215 main.Run
	github.com/gravitational/teleport/tool/tsh/tsh.go:506 main.main
	runtime/proc.go:250 runtime.main
	runtime/asm_arm64.s:1172 runtime.goexit
User Message: EOF

we would get something like this instead

ERROR REPORT:
Original Error: *trace.TraceErr EOF
Stack Trace:
	github.com/gravitational/teleport/api/utils/grpc/interceptors:55 github.com/gravitational/teleport/api/utils/grpc/interceptors.(* grpcClientStreamWrapper).RecvMsg
	github.com/gravitational/teleport/tool/tsh/access_request.go:55 main.onRequestList.func1
	github.com/gravitational/teleport/lib/client/api.go:1395 github.com/gravitational/teleport/lib/client.(*TeleportClient).WithRootClusterClient
	github.com/gravitational/teleport/tool/tsh/access_request.go:54 main.onRequestList
	github.com/gravitational/teleport/tool/tsh/tsh.go:1215 main.Run
	github.com/gravitational/teleport/tool/tsh/tsh.go:506 main.main
	runtime/proc.go:250 runtime.main
	runtime/asm_arm64.s:1172 runtime.goexit
User Message: EOF

@codingllama
Copy link
Copy Markdown
Contributor

@rosstimothy is not trace-wrapping in the interceptors enough to avoid it? Any tips to avoid making the same mistakes?

@rosstimothy
Copy link
Copy Markdown
Contributor

@rosstimothy is not trace-wrapping in the interceptors enough to avoid it? Any tips to avoid making the same mistakes?

If we don't do a trail.FromGRPC in the interceptors then what is the point of using them?

@Joerger
Copy link
Copy Markdown
Contributor Author

Joerger commented Aug 17, 2023

@rosstimothy How did your PR break tsh request ls? It is working fine with this PR.

Also, Alan is right, we just need to trace.Unwrap(trail.FromGRPC()). This way, we get a proper error from the error code without adding the middleware trace:

ERROR REPORT:
Original Error: *status.Error rpc error: code = Unknown desc = EOF
Stack Trace:
	github.com/gravitational/teleport/api/client/client.go:998 github.com/gravitational/teleport/api/client.(*Client).GetAccessRequests
	github.com/gravitational/teleport/tool/tsh/common/access_request.go:58 github.com/gravitational/teleport/tool/tsh/common.onRequestList.func1
	github.com/gravitational/teleport/lib/client/api.go:1468 github.com/gravitational/teleport/lib/client.(*TeleportClient).WithRootClusterClient
	github.com/gravitational/teleport/tool/tsh/common/access_request.go:57 github.com/gravitational/teleport/tool/tsh/common.onRequestList
	github.com/gravitational/teleport/tool/tsh/common/tsh.go:1403 github.com/gravitational/teleport/tool/tsh/common.Run
	github.com/gravitational/teleport/tool/tsh/common/tsh.go:545 github.com/gravitational/teleport/tool/tsh/common.Main
	github.com/gravitational/teleport/tool/tsh/main.go:24 main.main
	runtime/proc.go:267 runtime.main
	runtime/asm_amd64.s:1650 runtime.goexit
User Message: rpc error: code = Unknown desc = EOF

@Joerger Joerger requested a review from rosstimothy August 17, 2023 22:43
@Joerger Joerger requested a review from rosstimothy August 21, 2023 17:52
@Joerger Joerger force-pushed the joerger/grpc-error-interceptor branch from 7ac6a5a to 76a143d Compare August 22, 2023 19:47
@Joerger Joerger added this pull request to the merge queue Aug 24, 2023
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 24, 2023
@Joerger Joerger added this pull request to the merge queue Aug 24, 2023
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 24, 2023
@Joerger Joerger force-pushed the joerger/grpc-error-interceptor branch from e0e0490 to 5f869dc Compare August 24, 2023 23:10
@Joerger Joerger enabled auto-merge August 24, 2023 23:10
@Joerger Joerger added this pull request to the merge queue Aug 24, 2023
Merged via the queue into master with commit cf6473f Aug 24, 2023
@Joerger Joerger deleted the joerger/grpc-error-interceptor branch August 24, 2023 23:54
@codingllama
Copy link
Copy Markdown
Contributor

Adding this comment here to record the history.

Since the interceptors landed various server-side errors that used to be treated as status.Error are now trace.BadParameter errors, which triggers relogin in various situations where it shouldn't. See #36749 and #36866 (comment).

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.

3 participants