Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Handle interceptor errors as responses (#840) #842

Merged
merged 1 commit into from
Feb 15, 2022

Conversation

NAlexPear
Copy link
Contributor

@NAlexPear NAlexPear commented Nov 19, 2021

Summary

This PR forwards Interceptor errors/Statuses as valid HTTP responses, just like any other gRPC service generated by tonic.

Motivation

In the very specific case of missing Content-Type headers when requests are intercepted, this PR fixes the bugs reported in #840 and #759.

In the more general case, this class of bug arose because interceptors and their statuses are treated differently from the rest of the gRPC service stack: their errors/Status are bubbled up as errors to be handled at the tower service layer, but not as a gRPC response. That means that a basic HTTP response is returned, sans trailers or headers of any kind (including the missing Content-Type header). See this repository and this comment to compare the responses generated by returning a Status from a method versus an interceptor.

This behavior means that clients like grpcurl that encounter the responses from interceptors don't know how to handle them, and it's likely that this affects any consumer that depends on the Content-Type header (among others).

Solution

The solution in this PR is to handle Status errors the same way in both interceptors and regular ol' gRPC services: by converting those statuses to valid responses in the Interceptor service itself, matching the behavior in the existing grpc module.

The downside to this approach is a few additional constraints on the response body type. These constraints seem reasonable given that they match up with the current uses of interceptors in tonic, but they do make wrapping non-tonic services trickier.

Next Steps

EDIT: looks like those constraints are too restrictive for tonic-health 🤦. I'll take a look at either relaxing those constraints or updating tonic-health accordingly.

EDIT 2: I've both relaxed the constraints and added a bit of restriction to clients in tonic-build. I'm not sure what the right mix between the two should be for a final implementation, but this does solve the issue for my use-case.

Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

LGTM!

@LucioFranco LucioFranco added this to the 0.7 milestone Dec 8, 2021
@okkero
Copy link

okkero commented Jan 17, 2022

Looking forward to seeing this merged

@olliebatch
Copy link

Looking forward to seeing this merged, Prevents the use of grpcCurl as it errors so will be useful!

@LucioFranco LucioFranco changed the title fix(tonic): Handle interceptor errors as responses (#840) fix: Handle interceptor errors as responses (#840) Feb 15, 2022
@LucioFranco LucioFranco merged commit bf44940 into hyperium:master Feb 15, 2022
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.

5 participants