-
Notifications
You must be signed in to change notification settings - Fork 5.5k
http/filter: Add a callback for sendLocalReply() and encode gRPC message for local responses when the request is gRPC #3299
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
Changes from 18 commits
5d98848
7896b64
29ba376
2c264bf
c21e507
4c6b212
0903339
ae9c406
52f9baf
42327a3
04f23a6
a47b2a1
6bd512a
26aa335
dea2787
06b22a7
e125f63
2860297
74724fd
2b57fc4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -190,6 +190,21 @@ class StreamDecoderFilterCallbacks : public virtual StreamFilterCallbacks { | |
| */ | ||
| virtual void addDecodedData(Buffer::Instance& data, bool streaming_filter) PURE; | ||
|
|
||
| /** | ||
| * Create a locally generated response using the provided response_code and body_text paramewters. | ||
| * If the request was a gRPC request the local reply will be encoded as a gRPC response with a 200 | ||
| * HTTP response code and grpc-status and grpc-message headers mapped from the provided | ||
| * parameters. | ||
| * | ||
| * @param response_code supplies the HTTP response code. | ||
| * @param body_text supplies the optional body text which is sent using the text/plain content | ||
| * type. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: "... or encoded in the grpc-message header."
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added. |
||
| * @param modify_headers supplies an optional callback function that can modify the | ||
| * response headers. | ||
| */ | ||
| virtual void sendLocalReply(Code response_code, const std::string& body_text, | ||
| std::function<void(HeaderMap& headers)> modify_headers) PURE; | ||
|
|
||
| /** | ||
| * Called with 100-Continue headers to be encoded. | ||
| * | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,89 @@ | ||
| #include "common/grpc/status.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Grpc { | ||
|
|
||
| Status::GrpcStatus Utility::httpToGrpcStatus(uint64_t http_response_status) { | ||
| // From | ||
| // https://github.com/grpc/grpc/blob/master/doc/http-grpc-status-mapping.md. | ||
| switch (http_response_status) { | ||
| case 400: | ||
| return Status::GrpcStatus::Internal; | ||
| case 401: | ||
| return Status::GrpcStatus::Unauthenticated; | ||
| case 403: | ||
| return Status::GrpcStatus::PermissionDenied; | ||
| case 404: | ||
| return Status::GrpcStatus::Unimplemented; | ||
| case 429: | ||
| case 502: | ||
| case 503: | ||
| case 504: | ||
| return Status::GrpcStatus::Unavailable; | ||
| default: | ||
| return Status::GrpcStatus::Unknown; | ||
| } | ||
| } | ||
|
|
||
| uint64_t Utility::grpcToHttpStatus(Status::GrpcStatus grpc_status) { | ||
| // From https://cloud.google.com/apis/design/errors#handling_errors. | ||
| switch (grpc_status) { | ||
| case Status::GrpcStatus::Ok: | ||
| return 200; | ||
| case Status::GrpcStatus::Canceled: | ||
| // Client closed request. | ||
| return 499; | ||
| case Status::GrpcStatus::Unknown: | ||
| // Internal server error. | ||
| return 500; | ||
| case Status::GrpcStatus::InvalidArgument: | ||
| // Bad request. | ||
| return 400; | ||
| case Status::GrpcStatus::DeadlineExceeded: | ||
| // Gateway Time-out. | ||
| return 504; | ||
| case Status::GrpcStatus::NotFound: | ||
| // Not found. | ||
| return 404; | ||
| case Status::GrpcStatus::AlreadyExists: | ||
| // Conflict. | ||
| return 409; | ||
| case Status::GrpcStatus::PermissionDenied: | ||
| // Forbidden. | ||
| return 403; | ||
| case Status::GrpcStatus::ResourceExhausted: | ||
| // Too many requests. | ||
| return 429; | ||
| case Status::GrpcStatus::FailedPrecondition: | ||
| // Bad request. | ||
| return 400; | ||
| case Status::GrpcStatus::Aborted: | ||
| // Conflict. | ||
| return 409; | ||
| case Status::GrpcStatus::OutOfRange: | ||
| // Bad request. | ||
| return 400; | ||
| case Status::GrpcStatus::Unimplemented: | ||
| // Not implemented. | ||
| return 501; | ||
| case Status::GrpcStatus::Internal: | ||
| // Internal server error. | ||
| return 500; | ||
| case Status::GrpcStatus::Unavailable: | ||
| // Service unavailable. | ||
| return 503; | ||
| case Status::GrpcStatus::DataLoss: | ||
| // Internal server error. | ||
| return 500; | ||
| case Status::GrpcStatus::Unauthenticated: | ||
| // Unauthorized. | ||
| return 401; | ||
| case Status::GrpcStatus::InvalidCode: | ||
| default: | ||
| // Internal server error. | ||
| return 500; | ||
| } | ||
| } | ||
|
|
||
| } // namespace Grpc | ||
| } // namespace Envoy |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| #pragma once | ||
|
|
||
| #include <cstdint> | ||
|
|
||
| #include "envoy/grpc/status.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Grpc { | ||
|
|
||
| /** | ||
| * Grpc::Status utilities. | ||
| */ | ||
| class Utility { | ||
| public: | ||
| /** | ||
| * Returns the gRPC status code from a given HTTP response status code. Ordinarily, it is expected | ||
| * that a 200 response is provided, but gRPC defines a mapping for intermediaries that are not | ||
| * gRPC aware, see https://github.com/grpc/grpc/blob/master/doc/http-grpc-status-mapping.md. | ||
| * @param http_response_status HTTP status code. | ||
| * @return Status::GrpcStatus corresponding gRPC status code. | ||
| */ | ||
| static Status::GrpcStatus httpToGrpcStatus(uint64_t http_response_status); | ||
|
|
||
| /** | ||
| * @param grpc_status gRPC status from grpc-status header. | ||
| * @return uint64_t the canonical HTTP status code corresponding to a gRPC status code. | ||
| */ | ||
| static uint64_t grpcToHttpStatus(Status::GrpcStatus grpc_status); | ||
| }; | ||
|
|
||
| } // namespace Grpc | ||
| } // namespace Envoy |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -261,6 +261,19 @@ class AsyncStreamImpl : public AsyncClient::Stream, | |
| void continueDecoding() override { NOT_IMPLEMENTED; } | ||
| void addDecodedData(Buffer::Instance&, bool) override { NOT_IMPLEMENTED; } | ||
| const Buffer::Instance* decodingBuffer() override { return buffered_body_.get(); } | ||
| void sendLocalReply(Code code, const std::string& body, | ||
| std::function<void(HeaderMap& headers)> modify_headers) override { | ||
| Utility::sendLocalReply( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if this is correct for an HTTP client; do we ever hit this in the configured client filter stack? I.e. could it be
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I initially left this empty, and when tests failed put NOT_REACHED and it was reached. With this it works, and I did not dig deeper.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tested it again, these fail if this is just
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah we can definitely hit this for AsyncClient, since it goes through router and can return 503 for no healthy upstream and a bunch of other reasons.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, that makes sense. |
||
| is_grpc_request_, | ||
| [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { | ||
| if (headers != nullptr && modify_headers != nullptr) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can headers ever be nullptr here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct, removed test for non-null. |
||
| modify_headers(*headers); | ||
| } | ||
| encodeHeaders(std::move(headers), end_stream); | ||
| }, | ||
| [this](Buffer::Instance& data, bool end_stream) -> void { encodeData(data, end_stream); }, | ||
| remote_closed_, code, body); | ||
| } | ||
| // The async client won't pause if sending an Expect: 100-Continue so simply | ||
| // swallows any incoming encode100Continue. | ||
| void encode100ContinueHeaders(HeaderMapPtr&&) override {} | ||
|
|
@@ -284,6 +297,7 @@ class AsyncStreamImpl : public AsyncClient::Stream, | |
| bool local_closed_{}; | ||
| bool remote_closed_{}; | ||
| Buffer::InstancePtr buffered_body_; | ||
| bool is_grpc_request_{}; | ||
| friend class AsyncClientImpl; | ||
| }; | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo "paramewters"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixes, thanks.