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

http/filter: Add a callback for sendLocalReply() and encode gRPC message for local responses when the request is gRPC #3299

Merged
merged 20 commits into from
May 15, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
5d98848
router: Use gRPC message for local responses when the request is gRPC
jrajahalme May 10, 2018
7896b64
grpc: Move utility functions to http/utility.
jrajahalme May 10, 2018
29ba376
http: Make Utility::sendLocalResponse() aware of gRPC.
jrajahalme May 10, 2018
2c264bf
http: add StreamDecoderFilter callback sendLocalReply().
jrajahalme May 9, 2018
c21e507
http: Add a missing include.
jrajahalme May 10, 2018
4c6b212
http: Mark local complete also when sending local gRPC response.
jrajahalme May 11, 2018
0903339
http: Add comment for clarification (needed)
jrajahalme May 11, 2018
ae9c406
http: Move gRPC utilities back to grpc/common
jrajahalme May 11, 2018
52f9baf
test/grpc: Revert unnecessary namespace prefix changes.
jrajahalme May 11, 2018
42327a3
Merge branch 'master' into grpc-local-responses
jrajahalme May 11, 2018
04f23a6
http: Use sendLocalReply() for all local replies in ConnectionManager…
jrajahalme May 11, 2018
a47b2a1
filters: Use sendLocalReply() decoder callback for local replies.
jrajahalme May 11, 2018
6bd512a
conn_manager_impl: Fix format.
jrajahalme May 11, 2018
26aa335
filters: Remove unnecessary tracking of stream status.
jrajahalme May 14, 2018
dea2787
http: Clarify comments on sendLocalReply()
jrajahalme May 14, 2018
06b22a7
conn_manager_impl: Remove nullptr parameter and add TODOs.
jrajahalme May 14, 2018
e125f63
filter: Add comment about gRPC encoding of local responses.
jrajahalme May 14, 2018
2860297
common/grpc: Add status_lib
jrajahalme May 14, 2018
74724fd
test: http mock cleanup.
jrajahalme May 15, 2018
2b57fc4
http: Final review comment fixes.
jrajahalme May 15, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions docs/root/intro/version_history.rst
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ Version history
local configuration.
* http: added the ability to pass DNS type Subject Alternative Names of the client certificate in the
:ref:`config_http_conn_man_headers_x-forwarded-client-cert` header.
* http: local responses to gRPC requests are now sent as trailers-only gRPC responses instead of plain HTTP responses.
Notably the HTTP response code is always "200" in this case, and the gRPC error code is carried in "grpc-status"
header, optionally accompanied with a text message in "grpc-message" header.
* listeners: added :ref:`tcp_fast_open_queue_length <envoy_api_field_Listener.tcp_fast_open_queue_length>` option.
* load balancing: added :ref:`weighted round robin
<arch_overview_load_balancing_types_round_robin>` support. The round robin
Expand Down
15 changes: 15 additions & 0 deletions include/envoy/http/filter.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 parameters.
* 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, or encoded in the grpc-message header.
* @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.
*
Expand Down
11 changes: 10 additions & 1 deletion source/common/grpc/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -46,13 +46,21 @@ envoy_cc_library(
],
)

envoy_cc_library(
name = "status_lib",
srcs = ["status.cc"],
hdrs = ["status.h"],
deps = [
"//include/envoy/grpc:status",
],
)

envoy_cc_library(
name = "common_lib",
srcs = ["common.cc"],
hdrs = ["common.h"],
external_deps = ["abseil_optional"],
deps = [
"//include/envoy/grpc:status",
"//include/envoy/http:header_map_interface",
"//include/envoy/http:message_interface",
"//include/envoy/stats:stats_interface",
Expand All @@ -64,6 +72,7 @@ envoy_cc_library(
"//source/common/common:enum_to_int",
"//source/common/common:macros",
"//source/common/common:utility_lib",
"//source/common/grpc:status_lib",
"//source/common/http:filter_utility_lib",
"//source/common/http:headers_lib",
"//source/common/http:message_lib",
Expand Down
2 changes: 1 addition & 1 deletion source/common/grpc/async_client_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ void AsyncStreamImpl::onHeaders(Http::HeaderMapPtr&& headers, bool end_stream) {
}
// Technically this should be
// https://github.com/grpc/grpc/blob/master/doc/http-grpc-status-mapping.md
// as given by Common::httpToGrpcStatus(), but the Google gRPC client treats
// as given by Grpc::Utility::httpToGrpcStatus(), but the Google gRPC client treats
// this as GrpcStatus::Canceled.
streamError(Status::GrpcStatus::Canceled);
return;
Expand Down
82 changes: 0 additions & 82 deletions source/common/grpc/common.cc
Original file line number Diff line number Diff line change
Expand Up @@ -124,88 +124,6 @@ bool Common::resolveServiceAndMethod(const Http::HeaderEntry* path, std::string*
return true;
}

Status::GrpcStatus Common::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 Common::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;
}
}

Buffer::InstancePtr Common::serializeBody(const Protobuf::Message& message) {
// http://www.grpc.io/docs/guides/wire.html
// Reserve enough space for the entire message and the 5 byte header.
Expand Down
16 changes: 1 addition & 15 deletions source/common/grpc/common.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include "envoy/http/message.h"
#include "envoy/stats/stats.h"

#include "common/grpc/status.h"
#include "common/protobuf/protobuf.h"

#include "absl/types/optional.h"
Expand Down Expand Up @@ -56,21 +57,6 @@ class Common {
*/
static std::string getGrpcMessage(const Http::HeaderMap& trailers);

/**
* 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);

/**
* Charge a success/failure stat to a cluster/service/method.
* @param cluster supplies the target cluster.
Expand Down
89 changes: 89 additions & 0 deletions source/common/grpc/status.cc
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
32 changes: 32 additions & 0 deletions source/common/grpc/status.h
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
1 change: 1 addition & 0 deletions source/common/http/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,7 @@ envoy_cc_library(
"//source/common/common:empty_string",
"//source/common/common:enum_to_int",
"//source/common/common:utility_lib",
"//source/common/grpc:status_lib",
"//source/common/json:json_loader_lib",
"//source/common/network:utility_lib",
"//source/common/protobuf:utility_lib",
Expand Down
2 changes: 2 additions & 0 deletions source/common/http/async_client_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include <string>
#include <vector>

#include "common/grpc/common.h"
#include "common/http/utility.h"

namespace Envoy {
Expand Down Expand Up @@ -110,6 +111,7 @@ void AsyncStreamImpl::encodeTrailers(HeaderMapPtr&& trailers) {
}

void AsyncStreamImpl::sendHeaders(HeaderMap& headers, bool end_stream) {
is_grpc_request_ = Grpc::Common::hasGrpcContentType(headers);
headers.insertEnvoyInternalRequest().value().setReference(
Headers::get().EnvoyInternalRequestValues.True);
Utility::appendXff(headers, *parent_.config_.local_info_.address());
Expand Down
14 changes: 14 additions & 0 deletions source/common/http/async_client_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Copy link
Member

Choose a reason for hiding this comment

The 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 NOT_IMPLEMENTED?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tested it again, these fail if this is just {}:

//test/integration:legacy_json_integration_test                         TIMEOUT in 315.1s
  /home/vagrant/.cache/bazel/_bazel_vagrant/c7b138b989b4d12d83f51d45caa4ff0e/execroot/envoy/bazel-out/k8-fastbuild/testlogs/test/integration/legacy_json_integration_test/test.log
//test/integration:ratelimit_integration_test                           TIMEOUT in 315.1s
  /home/vagrant/.cache/bazel/_bazel_vagrant/c7b138b989b4d12d83f51d45caa4ff0e/execroot/envoy/bazel-out/k8-fastbuild/testlogs/test/integration/ratelimit_integration_test/test.log
//test/common/http:async_client_impl_test                                FAILED in 0.3s
  /home/vagrant/.cache/bazel/_bazel_vagrant/c7b138b989b4d12d83f51d45caa4ff0e/execroot/envoy/bazel-out/k8-fastbuild/testlogs/test/common/http/async_client_impl_test/test.log
//test/integration:hotrestart_test                                       FAILED in 35.0s
  /home/vagrant/.cache/bazel/_bazel_vagrant/c7b138b989b4d12d83f51d45caa4ff0e/execroot/envoy/bazel-out/k8-fastbuild/testlogs/test/integration/hotrestart_test/test.log

Executed 52 out of 239 tests: 235 tests pass and 4 fail locally.

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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 (modify_headers != nullptr) {
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 {}
Expand All @@ -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;
};

Expand Down
Loading