From 78e41f23d2ea03581c2d428dc2f1a84fd497630c Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Thu, 11 Apr 2019 13:50:34 -0400 Subject: [PATCH 1/7] WIP: rc details for main Envoy workflow Signed-off-by: Alyssa Wilk --- include/envoy/http/filter.h | 6 +++- include/envoy/stream_info/BUILD | 1 + include/envoy/stream_info/stream_info.h | 45 +++++++++++++++++++++++++ source/common/http/async_client_impl.h | 3 +- source/common/http/conn_manager_impl.cc | 36 +++++++++++++------- source/common/http/conn_manager_impl.h | 8 +++-- source/common/router/router.cc | 44 +++++++++++++++--------- source/common/router/router.h | 2 +- test/mocks/http/mocks.h | 3 +- 9 files changed, 112 insertions(+), 36 deletions(-) diff --git a/include/envoy/http/filter.h b/include/envoy/http/filter.h index eec28670591ed..218dbc7a73949 100644 --- a/include/envoy/http/filter.h +++ b/include/envoy/http/filter.h @@ -295,10 +295,14 @@ class StreamDecoderFilterCallbacks : public virtual StreamFilterCallbacks { * @param modify_headers supplies an optional callback function that can modify the * response headers. * @param grpc_status the gRPC status code to override the httpToGrpcStatus mapping with. + * @param details a string detailing why this local reply was sent. */ + // TODO(alyssawilk) send an email to envoy-dev for API change, add for all other filters, and + // make details required. virtual void sendLocalReply(Code response_code, absl::string_view body_text, std::function modify_headers, - const absl::optional grpc_status) PURE; + const absl::optional grpc_status, + absl::string_view details = "") PURE; /** * Called with 100-Continue headers to be encoded. diff --git a/include/envoy/stream_info/BUILD b/include/envoy/stream_info/BUILD index defe80c196a95..68a28a8ac6c61 100644 --- a/include/envoy/stream_info/BUILD +++ b/include/envoy/stream_info/BUILD @@ -20,6 +20,7 @@ envoy_cc_library( "//include/envoy/upstream:host_description_interface", "//source/common/common:assert_lib", "//source/common/protobuf", + "//source/common/singleton:const_singleton", ], ) diff --git a/include/envoy/stream_info/stream_info.h b/include/envoy/stream_info/stream_info.h index e30218e317554..f565f896dc29e 100644 --- a/include/envoy/stream_info/stream_info.h +++ b/include/envoy/stream_info/stream_info.h @@ -14,6 +14,7 @@ #include "common/common/assert.h" #include "common/protobuf/protobuf.h" +#include "common/singleton/const_singleton.h" #include "absl/types/optional.h" @@ -64,6 +65,50 @@ enum ResponseFlag { LastFlag = StreamIdleTimeout }; +struct ResponseCodeDetailValues { + const std::string SET_BY_UPSTREAM = "set_by_upstream"; + // Envoy is doing non-streaming proxying, and the request payload exceeded + // configured limits. + const std::string REQUEST_PAYLOAD_TOO_LARGE = "request_payload_too_large"; + // Envoy is doing non-streaming proxying, and the response payload exceeded + // configured limits. + const std::string RESPONSE_PAYLOAD_TOO_LARGE = "response_payload_too_large"; + // The per-stream keepalive timeout was exceeded. + const std::string STREAM_IDLE_TIMEOUT = "stream_idle_timeout"; + // The per-stream total request timeout was exceeded + const std::string REQUEST_OVERALL_TIMEOUT = "request_overall_timeout"; + // The request was rejected due to the Overload Manager reaching configured resouce limits. + const std::string OVERLOAD = "overload"; + // The HTTP/1.0 or HTTP/0.9 request was rejected due to HTTP/1.0 support not being configured. + const std::string LOW_VERSION = "low_version"; + // The request was rejected due to the Host: or :authority field missing + const std::string MISSING_HOST = "missing_host_header"; + // The request was rejected due to the Host: or :authority field missing + const std::string REQUEST_HEADERS_TOO_LARGE = "request_headers_too_large"; + // The request was rejected because path normalization was configured on and failed, probably due + // to an invalid path. + const std::string PATH_NORMALIZATION_FAILED = "path_normalization_failed"; + // The request was rejected because it attempted an unsupported upgrade. + + // TODO(reviewers) do we want a standard of filter etc. prefix or unique IDs enough? + // Alternately maybe this should be a proto. Ugh. + const std::string UPGRADE_FAILED = "upgrade_failed"; + // The request was rejected by the router filter because there was no route found. + const std::string ROUTE_NOT_FOUND = "route_not_found"; + // A direct response was generated by the router filter. + const std::string DIRECT_RESPONSE = "direct_response"; + // The request was rejected by the router filter because there was no route found. + const std::string CLUSTER_NOT_FOUND = "cluster_not_found"; + // The request was rejected by the router filter because the cluster was in maintainence mode. + const std::string MAINTENANCE_MODE = "maintenance_mode"; + // The request was rejected by the router filter because there was no healthy upstream found. + const std::string NO_HEALTHY_UPSTREAM = "no_healthy_upstream"; + // The upstream stream was reset + const std::string Upstream = "upstream_reset{%s}"; +}; + +typedef ConstSingleton ResponseCodeDetails; + struct UpstreamTiming { /** * Sets the time when the first byte of the request was sent upstream. diff --git a/source/common/http/async_client_impl.h b/source/common/http/async_client_impl.h index 35fffefc2c473..26c1f6595c9ae 100644 --- a/source/common/http/async_client_impl.h +++ b/source/common/http/async_client_impl.h @@ -320,7 +320,8 @@ class AsyncStreamImpl : public AsyncClient::Stream, } void sendLocalReply(Code code, absl::string_view body, std::function modify_headers, - const absl::optional grpc_status) override { + const absl::optional grpc_status, + absl::string_view) override { Utility::sendLocalReply( is_grpc_request_, [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 1fe100755b71d..39565e5c5c311 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -494,9 +494,10 @@ void ConnectionManagerImpl::ActiveStream::onIdleTimeout() { connection_manager_.doEndStream(*this); } else { stream_info_.setResponseFlag(StreamInfo::ResponseFlag::StreamIdleTimeout); - sendLocalReply( - request_headers_ != nullptr && Grpc::Common::hasGrpcContentType(*request_headers_), - Http::Code::RequestTimeout, "stream timeout", nullptr, is_head_request_, absl::nullopt); + sendLocalReply(request_headers_ != nullptr && + Grpc::Common::hasGrpcContentType(*request_headers_), + Http::Code::RequestTimeout, "stream timeout", nullptr, is_head_request_, + absl::nullopt, StreamInfo::ResponseCodeDetails::get().STREAM_IDLE_TIMEOUT); } } @@ -504,7 +505,7 @@ void ConnectionManagerImpl::ActiveStream::onRequestTimeout() { connection_manager_.stats_.named_.downstream_rq_timeout_.inc(); sendLocalReply(request_headers_ != nullptr && Grpc::Common::hasGrpcContentType(*request_headers_), Http::Code::RequestTimeout, "request timeout", nullptr, is_head_request_, - absl::nullopt); + absl::nullopt, StreamInfo::ResponseCodeDetails::get().REQUEST_OVERALL_TIMEOUT); } void ConnectionManagerImpl::ActiveStream::addStreamDecoderFilterWorker( @@ -593,7 +594,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, connection_manager_.stats_.named_.downstream_rq_overload_close_.inc(); sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Http::Code::ServiceUnavailable, "envoy overloaded", nullptr, is_head_request_, - absl::nullopt); + absl::nullopt, StreamInfo::ResponseCodeDetails::get().OVERLOAD); return; } @@ -621,7 +622,8 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, stream_info_.protocol(protocol); if (!connection_manager_.config_.http1Settings().accept_http_10_) { // Send "Upgrade Required" if HTTP/1.0 support is not explicitly configured on. - sendLocalReply(false, Code::UpgradeRequired, "", nullptr, is_head_request_, absl::nullopt); + sendLocalReply(false, Code::UpgradeRequired, "", nullptr, is_head_request_, absl::nullopt, + StreamInfo::ResponseCodeDetails::get().LOW_VERSION); return; } else { // HTTP/1.0 defaults to single-use connections. Make sure the connection @@ -644,7 +646,8 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, } else { // Require host header. For HTTP/1.1 Host has already been translated to :authority. sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Code::BadRequest, "", - nullptr, is_head_request_, absl::nullopt); + nullptr, is_head_request_, absl::nullopt, + StreamInfo::ResponseCodeDetails::get().MISSING_HOST); return; } } @@ -652,7 +655,8 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, ASSERT(connection_manager_.config_.maxRequestHeadersKb() > 0); if (request_headers_->byteSize() > (connection_manager_.config_.maxRequestHeadersKb() * 1024)) { sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), - Code::RequestHeaderFieldsTooLarge, "", nullptr, is_head_request_, absl::nullopt); + Code::RequestHeaderFieldsTooLarge, "", nullptr, is_head_request_, absl::nullopt, + StreamInfo::ResponseCodeDetails::get().REQUEST_HEADERS_TOO_LARGE); return; } @@ -664,7 +668,8 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, if (!request_headers_->Path() || request_headers_->Path()->value().c_str()[0] != '/') { connection_manager_.stats_.named_.downstream_rq_non_relative_path_.inc(); sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Code::NotFound, "", nullptr, - is_head_request_, absl::nullopt); + is_head_request_, absl::nullopt, + StreamInfo::ResponseCodeDetails::get().REQUEST_HEADERS_TOO_LARGE); return; } @@ -672,7 +677,8 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, if (!ConnectionManagerUtility::maybeNormalizePath(*request_headers_, connection_manager_.config_)) { sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Code::BadRequest, "", - nullptr, is_head_request_, absl::nullopt); + nullptr, is_head_request_, absl::nullopt, + StreamInfo::ResponseCodeDetails::get().PATH_NORMALIZATION_FAILED); return; } @@ -702,7 +708,8 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, // Do not allow upgrades if the route does not support it. connection_manager_.stats_.named_.downstream_rq_ws_on_non_ws_route_.inc(); sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Code::Forbidden, "", - nullptr, is_head_request_, absl::nullopt); + nullptr, is_head_request_, absl::nullopt, + StreamInfo::ResponseCodeDetails::get().UPGRADE_FAILED); return; } // Allow non websocket requests to go through websocket enabled routes. @@ -1108,10 +1115,11 @@ void ConnectionManagerImpl::ActiveStream::refreshCachedRoute() { } } +// TODO(alyssar, eziskind) put this in stream info when merging #6530 void ConnectionManagerImpl::ActiveStream::sendLocalReply( bool is_grpc_request, Code code, absl::string_view body, const std::function& modify_headers, bool is_head_request, - const absl::optional grpc_status) { + const absl::optional grpc_status, absl::string_view) { ASSERT(response_headers_ == nullptr); // For early error handling, do a best-effort attempt to create a filter chain // to ensure access logging. @@ -1872,7 +1880,7 @@ void ConnectionManagerImpl::ActiveStreamDecoderFilter::requestDataTooLarge() { } else { parent_.connection_manager_.stats_.named_.downstream_rq_too_large_.inc(); sendLocalReply(Code::PayloadTooLarge, CodeUtility::toString(Code::PayloadTooLarge), nullptr, - absl::nullopt); + absl::nullopt, StreamInfo::ResponseCodeDetails::get().REQUEST_PAYLOAD_TOO_LARGE); } } @@ -1976,6 +1984,8 @@ void ConnectionManagerImpl::ActiveStreamEncoderFilter::responseDataTooLarge() { parent_.state_.encoder_filters_streaming_ = true; allowIteration(); + // TODO(alyssar) as this doesn't use the HCM sendLocalReply set StreamInfo + // RESPONSE_PAYLOAD_TOO_LARGE here. Http::Utility::sendLocalReply( Grpc::Common::hasGrpcContentType(*parent_.request_headers_), [&](HeaderMapPtr&& response_headers, bool end_stream) -> void { diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index a968d1109f7a4..88f1348909aed 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -221,9 +221,10 @@ class ConnectionManagerImpl : Logger::Loggable, void sendLocalReply(Code code, absl::string_view body, std::function modify_headers, - const absl::optional grpc_status) override { + const absl::optional grpc_status, + absl::string_view details) override { parent_.sendLocalReply(is_grpc_request_, code, body, modify_headers, parent_.is_head_request_, - grpc_status); + grpc_status, details); } void encode100ContinueHeaders(HeaderMapPtr&& headers) override; void encodeHeaders(HeaderMapPtr&& headers, bool end_stream) override; @@ -357,7 +358,8 @@ class ConnectionManagerImpl : Logger::Loggable, void sendLocalReply(bool is_grpc_request, Code code, absl::string_view body, const std::function& modify_headers, bool is_head_request, - const absl::optional grpc_status); + const absl::optional grpc_status, + absl::string_view details); void encode100ContinueHeaders(ActiveStreamEncoderFilter* filter, HeaderMap& headers); void encodeHeaders(ActiveStreamEncoderFilter* filter, HeaderMap& headers, bool end_stream); // Sends data through encoding filter chains. filter_iteration_start_state indicates which diff --git a/source/common/router/router.cc b/source/common/router/router.cc index e2bd72dedd598..8a7b63c70b47d 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -277,7 +277,8 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e headers.Path()->value().c_str()); callbacks_->streamInfo().setResponseFlag(StreamInfo::ResponseFlag::NoRouteFound); - callbacks_->sendLocalReply(Http::Code::NotFound, "", nullptr, absl::nullopt); + callbacks_->sendLocalReply(Http::Code::NotFound, "", nullptr, absl::nullopt, + StreamInfo::ResponseCodeDetails::get().ROUTE_NOT_FOUND); return Http::FilterHeadersStatus::StopIteration; } @@ -296,7 +297,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e } direct_response->finalizeResponseHeaders(response_headers, callbacks_->streamInfo()); }, - absl::nullopt); + absl::nullopt, StreamInfo::ResponseCodeDetails::get().DIRECT_RESPONSE); return Http::FilterHeadersStatus::StopIteration; } @@ -309,7 +310,8 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e callbacks_->streamInfo().setResponseFlag(StreamInfo::ResponseFlag::NoRouteFound); callbacks_->sendLocalReply(route_entry_->clusterNotFoundResponseCode(), "", nullptr, - absl::nullopt); + absl::nullopt, + StreamInfo::ResponseCodeDetails::get().CLUSTER_NOT_FOUND); return Http::FilterHeadersStatus::StopIteration; } cluster_ = cluster->info(); @@ -329,14 +331,14 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e if (cluster_->maintenanceMode()) { callbacks_->streamInfo().setResponseFlag(StreamInfo::ResponseFlag::UpstreamOverflow); chargeUpstreamCode(Http::Code::ServiceUnavailable, nullptr, true); - callbacks_->sendLocalReply(Http::Code::ServiceUnavailable, "maintenance mode", - [this](Http::HeaderMap& headers) { - if (!config_.suppress_envoy_headers_) { - headers.insertEnvoyOverloaded().value( - Http::Headers::get().EnvoyOverloadedValues.True); - } - }, - absl::nullopt); + callbacks_->sendLocalReply( + Http::Code::ServiceUnavailable, "maintenance mode", + [this](Http::HeaderMap& headers) { + if (!config_.suppress_envoy_headers_) { + headers.insertEnvoyOverloaded().value(Http::Headers::get().EnvoyOverloadedValues.True); + } + }, + absl::nullopt, StreamInfo::ResponseCodeDetails::get().MAINTENANCE_MODE); cluster_->stats().upstream_rq_maintenance_mode_.inc(); return Http::FilterHeadersStatus::StopIteration; } @@ -409,7 +411,8 @@ void Filter::sendNoHealthyUpstreamResponse() { callbacks_->streamInfo().setResponseFlag(StreamInfo::ResponseFlag::NoHealthyUpstream); chargeUpstreamCode(Http::Code::ServiceUnavailable, nullptr, false); callbacks_->sendLocalReply(Http::Code::ServiceUnavailable, "no healthy upstream", nullptr, - absl::nullopt); + absl::nullopt, + StreamInfo::ResponseCodeDetails::get().NO_HEALTHY_UPSTREAM); } Http::FilterDataStatus Filter::decodeData(Buffer::Instance& data, bool end_stream) { @@ -557,14 +560,15 @@ void Filter::updateOutlierDetection(Http::Code code) { } } +// FIXME details void Filter::onUpstreamTimeoutAbort(StreamInfo::ResponseFlag response_flags) { const absl::string_view body = timeout_response_code_ == Http::Code::GatewayTimeout ? "upstream request timeout" : ""; - onUpstreamAbort(timeout_response_code_, response_flags, body, false); + onUpstreamAbort(timeout_response_code_, response_flags, body, false, ""); } void Filter::onUpstreamAbort(Http::Code code, StreamInfo::ResponseFlag response_flags, - absl::string_view body, bool dropped) { + absl::string_view body, bool dropped, absl::string_view details) { // If we have not yet sent anything downstream, send a response with an appropriate status code. // Otherwise just reset the ongoing response. if (downstream_response_started_) { @@ -601,7 +605,7 @@ void Filter::onUpstreamAbort(Http::Code code, StreamInfo::ResponseFlag response_ Http::Headers::get().EnvoyOverloadedValues.True); } }, - absl::nullopt); + absl::nullopt, details); } } @@ -656,7 +660,15 @@ void Filter::onUpstreamReset(Http::StreamResetReason reset_reason, const bool dropped = reset_reason == Http::StreamResetReason::Overflow; callbacks_->streamInfo().setUpstreamTransportFailureReason(transport_failure_reason); - onUpstreamAbort(Http::Code::ServiceUnavailable, response_flags, body, dropped); + // TODO(reviewer) do we want structure here rather than string? + // I think in practice if there's a transport failure it takes precedence, + // otherwise there was an application error. So maybe just converting both + // transport_failure_reasons and reset_reasons to the snake_case is + // sufficient? But maybe there's cases where we want mulitple levels to add + // their information. + std::string details = + absl::StrCat("upstream_reset{", reset_reason, ",", transport_failure_reason, "}"); + onUpstreamAbort(Http::Code::ServiceUnavailable, response_flags, body, dropped, details); } StreamInfo::ResponseFlag diff --git a/source/common/router/router.h b/source/common/router/router.h index 8bbd312974e90..f0b4947286a3a 100644 --- a/source/common/router/router.h +++ b/source/common/router/router.h @@ -382,7 +382,7 @@ class Filter : Logger::Loggable, // headers (e.g. due to a reset). Handles recording stats and responding // downstream if appropriate. void onUpstreamAbort(Http::Code code, StreamInfo::ResponseFlag response_flag, - absl::string_view body, bool dropped); + absl::string_view body, bool dropped, absl::string_view details); void onUpstreamHeaders(uint64_t response_code, Http::HeaderMapPtr&& headers, bool end_stream); void onUpstreamData(Buffer::Instance& data, bool end_stream); void onUpstreamTrailers(Http::HeaderMapPtr&& trailers); diff --git a/test/mocks/http/mocks.h b/test/mocks/http/mocks.h index a5f2971743c2b..c02a110fc3350 100644 --- a/test/mocks/http/mocks.h +++ b/test/mocks/http/mocks.h @@ -152,7 +152,8 @@ class MockStreamDecoderFilterCallbacks : public StreamDecoderFilterCallbacks, // Http::StreamDecoderFilterCallbacks void sendLocalReply(Code code, absl::string_view body, std::function modify_headers, - const absl::optional grpc_status) override { + const absl::optional grpc_status, + absl::string_view) override { Utility::sendLocalReply( is_grpc_request_, [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { From f5762580eb68b86248a98302107b3c96121333a7 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Thu, 11 Apr 2019 15:23:25 -0400 Subject: [PATCH 2/7] format Signed-off-by: Alyssa Wilk --- include/envoy/stream_info/stream_info.h | 4 ++-- source/common/http/conn_manager_impl.cc | 2 +- source/common/router/router.cc | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/include/envoy/stream_info/stream_info.h b/include/envoy/stream_info/stream_info.h index f565f896dc29e..b116ca7f91b31 100644 --- a/include/envoy/stream_info/stream_info.h +++ b/include/envoy/stream_info/stream_info.h @@ -77,7 +77,7 @@ struct ResponseCodeDetailValues { const std::string STREAM_IDLE_TIMEOUT = "stream_idle_timeout"; // The per-stream total request timeout was exceeded const std::string REQUEST_OVERALL_TIMEOUT = "request_overall_timeout"; - // The request was rejected due to the Overload Manager reaching configured resouce limits. + // The request was rejected due to the Overload Manager reaching configured resource limits. const std::string OVERLOAD = "overload"; // The HTTP/1.0 or HTTP/0.9 request was rejected due to HTTP/1.0 support not being configured. const std::string LOW_VERSION = "low_version"; @@ -99,7 +99,7 @@ struct ResponseCodeDetailValues { const std::string DIRECT_RESPONSE = "direct_response"; // The request was rejected by the router filter because there was no route found. const std::string CLUSTER_NOT_FOUND = "cluster_not_found"; - // The request was rejected by the router filter because the cluster was in maintainence mode. + // The request was rejected by the router filter because the cluster was in maintenance mode. const std::string MAINTENANCE_MODE = "maintenance_mode"; // The request was rejected by the router filter because there was no healthy upstream found. const std::string NO_HEALTHY_UPSTREAM = "no_healthy_upstream"; diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 39565e5c5c311..822f2da48d87a 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -1115,7 +1115,7 @@ void ConnectionManagerImpl::ActiveStream::refreshCachedRoute() { } } -// TODO(alyssar, eziskind) put this in stream info when merging #6530 +// TODO(alyssawilk) put this in stream info when merging #6530 void ConnectionManagerImpl::ActiveStream::sendLocalReply( bool is_grpc_request, Code code, absl::string_view body, const std::function& modify_headers, bool is_head_request, diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 8a7b63c70b47d..9e78838802183 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -560,7 +560,7 @@ void Filter::updateOutlierDetection(Http::Code code) { } } -// FIXME details +// TODO(alyssawilk) details void Filter::onUpstreamTimeoutAbort(StreamInfo::ResponseFlag response_flags) { const absl::string_view body = timeout_response_code_ == Http::Code::GatewayTimeout ? "upstream request timeout" : ""; @@ -664,7 +664,7 @@ void Filter::onUpstreamReset(Http::StreamResetReason reset_reason, // I think in practice if there's a transport failure it takes precedence, // otherwise there was an application error. So maybe just converting both // transport_failure_reasons and reset_reasons to the snake_case is - // sufficient? But maybe there's cases where we want mulitple levels to add + // sufficient? But maybe there's cases where we want multiple levels to add // their information. std::string details = absl::StrCat("upstream_reset{", reset_reason, ",", transport_failure_reason, "}"); From eee0b1b71cb4042f57da09b1782a40f1edf85e95 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Wed, 24 Apr 2019 11:19:29 -0400 Subject: [PATCH 3/7] cleanup, reviewer comments, tests Signed-off-by: Alyssa Wilk --- include/envoy/stream_info/stream_info.h | 43 ++++++++++--------- source/common/http/conn_manager_impl.cc | 28 ++++++------ source/common/router/router.cc | 35 ++++++++------- source/common/router/router.h | 2 +- test/common/http/conn_manager_impl_test.cc | 14 +++--- test/common/router/router_test.cc | 10 +++++ .../grpc_access_log_integration_test.cc | 3 ++ test/mocks/http/mocks.h | 4 +- 8 files changed, 82 insertions(+), 57 deletions(-) diff --git a/include/envoy/stream_info/stream_info.h b/include/envoy/stream_info/stream_info.h index 5b0855c31ba11..92d32fe4ada7f 100644 --- a/include/envoy/stream_info/stream_info.h +++ b/include/envoy/stream_info/stream_info.h @@ -77,42 +77,45 @@ struct ResponseCodeDetailValues { // Response code was set by the upstream. const std::string ViaUpstream = "via_upstream"; // configured limits. - const std::string REQUEST_PAYLOAD_TOO_LARGE = "request_payload_too_large"; + const std::string RequestPayloadTooLarge = "request_payload_too_large"; // Envoy is doing non-streaming proxying, and the response payload exceeded // configured limits. - const std::string RESPONSE_PAYLOAD_TOO_LARGE = "response_payload_too_large"; + const std::string ResponsePayloadTooLArge = "response_payload_too_large"; // The per-stream keepalive timeout was exceeded. - const std::string STREAM_IDLE_TIMEOUT = "stream_idle_timeout"; + const std::string StreamIdleTimeout = "stream_idle_timeout"; // The per-stream total request timeout was exceeded - const std::string REQUEST_OVERALL_TIMEOUT = "request_overall_timeout"; + const std::string RequestOverallTimeout = "request_overall_timeout"; // The request was rejected due to the Overload Manager reaching configured resource limits. - const std::string OVERLOAD = "overload"; + const std::string Overload = "overload"; // The HTTP/1.0 or HTTP/0.9 request was rejected due to HTTP/1.0 support not being configured. - const std::string LOW_VERSION = "low_version"; + const std::string LowVersion = "low_version"; // The request was rejected due to the Host: or :authority field missing - const std::string MISSING_HOST = "missing_host_header"; + const std::string MissingHost = "missing_host_header"; // The request was rejected due to the Host: or :authority field missing - const std::string REQUEST_HEADERS_TOO_LARGE = "request_headers_too_large"; + const std::string RequestHeadersTooLarge = "request_headers_too_large"; + // The request was rejected due to the Path or :path header field missing. + const std::string MissingPath = "missing_path_rejected"; + // The request was rejected due to using an absolute path on a route not supporting them. + const std::string AbsolutePath = "absolute_path_rejected"; // The request was rejected because path normalization was configured on and failed, probably due // to an invalid path. - const std::string PATH_NORMALIZATION_FAILED = "path_normalization_failed"; + const std::string PathNormalizationFailed = "path_normalization_failed"; // The request was rejected because it attempted an unsupported upgrade. - - // TODO(reviewers) do we want a standard of filter etc. prefix or unique IDs enough? - // Alternately maybe this should be a proto. Ugh. - const std::string UPGRADE_FAILED = "upgrade_failed"; + const std::string UpgradeFailed = "upgrade_failed"; // The request was rejected by the router filter because there was no route found. - const std::string ROUTE_NOT_FOUND = "route_not_found"; + const std::string RouteNotFound = "route_not_found"; // A direct response was generated by the router filter. - const std::string DIRECT_RESPONSE = "direct_response"; + const std::string DirectResponse = "direct_response"; // The request was rejected by the router filter because there was no route found. - const std::string CLUSTER_NOT_FOUND = "cluster_not_found"; + const std::string ClusterNotFound = "cluster_not_found"; // The request was rejected by the router filter because the cluster was in maintenance mode. - const std::string MAINTENANCE_MODE = "maintenance_mode"; + const std::string MaintenanceMode = "maintenance_mode"; // The request was rejected by the router filter because there was no healthy upstream found. - const std::string NO_HEALTHY_UPSTREAM = "no_healthy_upstream"; - // The upstream stream was reset - const std::string Upstream = "upstream_reset{%s}"; + const std::string NoHealthyUpstream = "no_healthy_upstream"; + // The upstream response timed out + const std::string UpstreamTimeout = "upstream_response_timeout"; + // The final upstream try timed out + const std::string UpstreamPerTryTimeout = "upstream_per_try_timeout"; // TODO(#6542): continue addding values for sendLocalReply use-cases }; diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 629bf43f50c98..e179668d169ce 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -497,7 +497,7 @@ void ConnectionManagerImpl::ActiveStream::onIdleTimeout() { sendLocalReply(request_headers_ != nullptr && Grpc::Common::hasGrpcContentType(*request_headers_), Http::Code::RequestTimeout, "stream timeout", nullptr, is_head_request_, - absl::nullopt, StreamInfo::ResponseCodeDetails::get().STREAM_IDLE_TIMEOUT); + absl::nullopt, StreamInfo::ResponseCodeDetails::get().StreamIdleTimeout); } } @@ -505,7 +505,7 @@ void ConnectionManagerImpl::ActiveStream::onRequestTimeout() { connection_manager_.stats_.named_.downstream_rq_timeout_.inc(); sendLocalReply(request_headers_ != nullptr && Grpc::Common::hasGrpcContentType(*request_headers_), Http::Code::RequestTimeout, "request timeout", nullptr, is_head_request_, - absl::nullopt, StreamInfo::ResponseCodeDetails::get().REQUEST_OVERALL_TIMEOUT); + absl::nullopt, StreamInfo::ResponseCodeDetails::get().RequestOverallTimeout); } void ConnectionManagerImpl::ActiveStream::addStreamDecoderFilterWorker( @@ -595,7 +595,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, connection_manager_.stats_.named_.downstream_rq_overload_close_.inc(); sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Http::Code::ServiceUnavailable, "envoy overloaded", nullptr, is_head_request_, - absl::nullopt, StreamInfo::ResponseCodeDetails::get().OVERLOAD); + absl::nullopt, StreamInfo::ResponseCodeDetails::get().Overload); return; } @@ -624,7 +624,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, if (!connection_manager_.config_.http1Settings().accept_http_10_) { // Send "Upgrade Required" if HTTP/1.0 support is not explicitly configured on. sendLocalReply(false, Code::UpgradeRequired, "", nullptr, is_head_request_, absl::nullopt, - StreamInfo::ResponseCodeDetails::get().LOW_VERSION); + StreamInfo::ResponseCodeDetails::get().LowVersion); return; } else { // HTTP/1.0 defaults to single-use connections. Make sure the connection @@ -648,7 +648,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, // Require host header. For HTTP/1.1 Host has already been translated to :authority. sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Code::BadRequest, "", nullptr, is_head_request_, absl::nullopt, - StreamInfo::ResponseCodeDetails::get().MISSING_HOST); + StreamInfo::ResponseCodeDetails::get().MissingHost); return; } } @@ -657,7 +657,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, if (request_headers_->byteSize() > (connection_manager_.config_.maxRequestHeadersKb() * 1024)) { sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Code::RequestHeaderFieldsTooLarge, "", nullptr, is_head_request_, absl::nullopt, - StreamInfo::ResponseCodeDetails::get().REQUEST_HEADERS_TOO_LARGE); + StreamInfo::ResponseCodeDetails::get().RequestHeadersTooLarge); return; } @@ -668,10 +668,13 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, // :path because CONNECT does not have a path, and we don't support that currently. if (!request_headers_->Path() || request_headers_->Path()->value().getStringView().empty() || request_headers_->Path()->value().getStringView()[0] != '/') { + const bool has_path = + request_headers_->Path() && !request_headers_->Path()->value().getStringView().empty(); connection_manager_.stats_.named_.downstream_rq_non_relative_path_.inc(); sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Code::NotFound, "", nullptr, is_head_request_, absl::nullopt, - StreamInfo::ResponseCodeDetails::get().REQUEST_HEADERS_TOO_LARGE); + has_path ? StreamInfo::ResponseCodeDetails::get().AbsolutePath + : StreamInfo::ResponseCodeDetails::get().MissingPath); return; } @@ -680,7 +683,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, connection_manager_.config_)) { sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Code::BadRequest, "", nullptr, is_head_request_, absl::nullopt, - StreamInfo::ResponseCodeDetails::get().PATH_NORMALIZATION_FAILED); + StreamInfo::ResponseCodeDetails::get().PathNormalizationFailed); return; } @@ -711,7 +714,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, connection_manager_.stats_.named_.downstream_rq_ws_on_non_ws_route_.inc(); sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Code::Forbidden, "", nullptr, is_head_request_, absl::nullopt, - StreamInfo::ResponseCodeDetails::get().UPGRADE_FAILED); + StreamInfo::ResponseCodeDetails::get().UpgradeFailed); return; } // Allow non websocket requests to go through websocket enabled routes. @@ -1122,13 +1125,14 @@ void ConnectionManagerImpl::ActiveStream::sendLocalReply( bool is_grpc_request, Code code, absl::string_view body, const std::function& modify_headers, bool is_head_request, const absl::optional grpc_status, absl::string_view details) { + ENVOY_STREAM_LOG(debug, "Sending local reply with details {}", *this, details); ASSERT(response_headers_ == nullptr); // For early error handling, do a best-effort attempt to create a filter chain // to ensure access logging. if (!state_.created_filter_chain_) { createFilterChain(); } - parent_.stream_info_.setResponseCodeDetails(details); + stream_info_.setResponseCodeDetails(details); Utility::sendLocalReply( is_grpc_request, [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { @@ -1883,7 +1887,7 @@ void ConnectionManagerImpl::ActiveStreamDecoderFilter::requestDataTooLarge() { } else { parent_.connection_manager_.stats_.named_.downstream_rq_too_large_.inc(); sendLocalReply(Code::PayloadTooLarge, CodeUtility::toString(Code::PayloadTooLarge), nullptr, - absl::nullopt, StreamInfo::ResponseCodeDetails::get().REQUEST_PAYLOAD_TOO_LARGE); + absl::nullopt, StreamInfo::ResponseCodeDetails::get().RequestPayloadTooLarge); } } @@ -1989,7 +1993,7 @@ void ConnectionManagerImpl::ActiveStreamEncoderFilter::responseDataTooLarge() { allowIteration(); parent_.stream_info_.setResponseCodeDetails( - StreamInfo::ResponseCodeDetails::get().REQUEST_HEADERS_TOO_LARGE); + StreamInfo::ResponseCodeDetails::get().RequestHeadersTooLarge); Http::Utility::sendLocalReply( Grpc::Common::hasGrpcContentType(*parent_.request_headers_), [&](HeaderMapPtr&& response_headers, bool end_stream) -> void { diff --git a/source/common/router/router.cc b/source/common/router/router.cc index e3d1376e45343..bed953009faa7 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -292,7 +292,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e callbacks_->streamInfo().setResponseFlag(StreamInfo::ResponseFlag::NoRouteFound); callbacks_->sendLocalReply(Http::Code::NotFound, "", nullptr, absl::nullopt, - StreamInfo::ResponseCodeDetails::get().ROUTE_NOT_FOUND); + StreamInfo::ResponseCodeDetails::get().RouteNotFound); return Http::FilterHeadersStatus::StopIteration; } @@ -311,7 +311,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e } direct_response->finalizeResponseHeaders(response_headers, callbacks_->streamInfo()); }, - absl::nullopt, StreamInfo::ResponseCodeDetails::get().DIRECT_RESPONSE); + absl::nullopt, StreamInfo::ResponseCodeDetails::get().DirectResponse); return Http::FilterHeadersStatus::StopIteration; } @@ -325,7 +325,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e callbacks_->streamInfo().setResponseFlag(StreamInfo::ResponseFlag::NoRouteFound); callbacks_->sendLocalReply(route_entry_->clusterNotFoundResponseCode(), "", nullptr, absl::nullopt, - StreamInfo::ResponseCodeDetails::get().CLUSTER_NOT_FOUND); + StreamInfo::ResponseCodeDetails::get().ClusterNotFound); return Http::FilterHeadersStatus::StopIteration; } cluster_ = cluster->info(); @@ -352,7 +352,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e headers.insertEnvoyOverloaded().value(Http::Headers::get().EnvoyOverloadedValues.True); } }, - absl::nullopt, StreamInfo::ResponseCodeDetails::get().MAINTENANCE_MODE); + absl::nullopt, StreamInfo::ResponseCodeDetails::get().MaintenanceMode); cluster_->stats().upstream_rq_maintenance_mode_.inc(); return Http::FilterHeadersStatus::StopIteration; } @@ -427,7 +427,7 @@ void Filter::sendNoHealthyUpstreamResponse() { chargeUpstreamCode(Http::Code::ServiceUnavailable, nullptr, false); callbacks_->sendLocalReply(Http::Code::ServiceUnavailable, "no healthy upstream", nullptr, absl::nullopt, - StreamInfo::ResponseCodeDetails::get().NO_HEALTHY_UPSTREAM); + StreamInfo::ResponseCodeDetails::get().NoHealthyUpstream); } Http::FilterDataStatus Filter::decodeData(Buffer::Instance& data, bool end_stream) { @@ -565,7 +565,8 @@ void Filter::onResponseTimeout() { upstream_requests_.front()->resetStream(); } - onUpstreamTimeoutAbort(StreamInfo::ResponseFlag::UpstreamRequestTimeout); + onUpstreamTimeoutAbort(StreamInfo::ResponseFlag::UpstreamRequestTimeout, + StreamInfo::ResponseCodeDetails::get().UpstreamTimeout); } void Filter::onPerTryTimeout(UpstreamRequest& upstream_request) { @@ -575,7 +576,8 @@ void Filter::onPerTryTimeout(UpstreamRequest& upstream_request) { return; } - onUpstreamTimeoutAbort(StreamInfo::ResponseFlag::UpstreamRequestTimeout); + onUpstreamTimeoutAbort(StreamInfo::ResponseFlag::UpstreamRequestTimeout, + StreamInfo::ResponseCodeDetails::get().UpstreamPerTryTimeout); } void Filter::updateOutlierDetection(Http::Code code, UpstreamRequest& upstream_request) { @@ -584,11 +586,11 @@ void Filter::updateOutlierDetection(Http::Code code, UpstreamRequest& upstream_r } } -// TODO(alyssawilk) details -void Filter::onUpstreamTimeoutAbort(StreamInfo::ResponseFlag response_flags) { +void Filter::onUpstreamTimeoutAbort(StreamInfo::ResponseFlag response_flags, + absl::string_view details) { const absl::string_view body = timeout_response_code_ == Http::Code::GatewayTimeout ? "upstream request timeout" : ""; - onUpstreamAbort(timeout_response_code_, response_flags, body, false, ""); + onUpstreamAbort(timeout_response_code_, response_flags, body, false, details); } void Filter::onUpstreamAbort(Http::Code code, StreamInfo::ResponseFlag response_flags, @@ -602,6 +604,7 @@ void Filter::onUpstreamAbort(Http::Code code, StreamInfo::ResponseFlag response_ config_.stats_.rq_reset_after_downstream_response_started_.inc(); } // This will destroy any created retry timers. + callbacks_->streamInfo().setResponseCodeDetails(details); cleanup(); callbacks_->resetStream(); } else { @@ -684,14 +687,10 @@ void Filter::onUpstreamReset(Http::StreamResetReason reset_reason, const bool dropped = reset_reason == Http::StreamResetReason::Overflow; callbacks_->streamInfo().setUpstreamTransportFailureReason(transport_failure_reason); - // TODO(reviewer) do we want structure here rather than string? - // I think in practice if there's a transport failure it takes precedence, - // otherwise there was an application error. So maybe just converting both - // transport_failure_reasons and reset_reasons to the snake_case is - // sufficient? But maybe there's cases where we want multiple levels to add - // their information. - std::string details = - absl::StrCat("upstream_reset{", reset_reason, ",", transport_failure_reason, "}"); + std::string details = absl::StrCat( + "upstream_reset_", downstream_response_started_ ? "after" : "before", "_response_started{", + Http::Utility::resetReasonToString(reset_reason), + transport_failure_reason.empty() ? "" : absl::StrCat(",", transport_failure_reason), "}"); onUpstreamAbort(Http::Code::ServiceUnavailable, response_flags, body, dropped, details); } diff --git a/source/common/router/router.h b/source/common/router/router.h index b25d14283800d..04cb299cab525 100644 --- a/source/common/router/router.h +++ b/source/common/router/router.h @@ -384,7 +384,7 @@ class Filter : Logger::Loggable, void onResponseTimeout(); void onUpstream100ContinueHeaders(Http::HeaderMapPtr&& headers); // Handle an upstream request aborted due to a local timeout. - void onUpstreamTimeoutAbort(StreamInfo::ResponseFlag response_flag); + void onUpstreamTimeoutAbort(StreamInfo::ResponseFlag response_flag, absl::string_view details); // Handle an "aborted" upstream request, meaning we didn't see response // headers (e.g. due to a reset). Handles recording stats and responding // downstream if appropriate. diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index dd6e61bcccf34..e94c147ae8650 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -551,8 +551,10 @@ TEST_F(HttpConnectionManagerImplTest, InvalidPathWithDualFilter) { EXPECT_CALL(*filter, encodeHeaders(_, true)); EXPECT_CALL(response_encoder_, encodeHeaders(_, true)) - .WillOnce(Invoke([](const HeaderMap& headers, bool) -> void { + .WillOnce(Invoke([&](const HeaderMap& headers, bool) -> void { EXPECT_EQ("404", headers.Status()->value().getStringView()); + EXPECT_EQ("absolute_path_rejected", + filter->decoder_callbacks_->streamInfo().responseCodeDetails().value()); })); EXPECT_CALL(*filter, onDestroy()); @@ -588,8 +590,10 @@ TEST_F(HttpConnectionManagerImplTest, PathFailedtoSanitize) { EXPECT_CALL(*filter, encodeHeaders(_, true)); EXPECT_CALL(response_encoder_, encodeHeaders(_, true)) - .WillOnce(Invoke([](const HeaderMap& headers, bool) -> void { + .WillOnce(Invoke([filter](const HeaderMap& headers, bool) -> void { EXPECT_EQ("400", headers.Status()->value().getStringView()); + EXPECT_EQ("path_normalization_failed", + filter->decoder_callbacks_->streamInfo().responseCodeDetails().value()); })); EXPECT_CALL(*filter, onDestroy()); @@ -1191,6 +1195,7 @@ TEST_F(HttpConnectionManagerImplTest, TestAccessLogWithInvalidRequest) { const StreamInfo::StreamInfo& stream_info) { EXPECT_TRUE(stream_info.responseCode()); EXPECT_EQ(stream_info.responseCode().value(), uint32_t(400)); + EXPECT_EQ("missing_host_header", stream_info.responseCodeDetails().value()); EXPECT_NE(nullptr, stream_info.downstreamLocalAddress()); EXPECT_NE(nullptr, stream_info.downstreamRemoteAddress()); EXPECT_NE(nullptr, stream_info.downstreamDirectRemoteAddress()); @@ -1364,7 +1369,7 @@ TEST_F(HttpConnectionManagerImplTest, PerStreamIdleTimeoutGlobal) { // 408 direct response after timeout. EXPECT_CALL(response_encoder_, encodeHeaders(_, false)) - .WillOnce(Invoke([](const HeaderMap& headers, bool) -> void { + .WillOnce(Invoke([&](const HeaderMap& headers, bool) -> void { EXPECT_EQ("408", headers.Status()->value().getStringView()); })); std::string response_body; @@ -3130,8 +3135,7 @@ TEST_F(HttpConnectionManagerImplTest, HitRequestBufferLimits) { Buffer::OwnedImpl data("A longer string"); decoder_filters_[0]->callbacks_->addDecodedData(data, false); const auto rc_details = encoder_filters_[1]->callbacks_->streamInfo().responseCodeDetails(); - EXPECT_TRUE(rc_details.has_value()); - EXPECT_EQ("", rc_details.value()); + EXPECT_EQ("request_payload_too_large", rc_details.value()); } // Return 413 from an intermediate filter and make sure we don't continue the filter chain. diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index e083c5e587ade..56949106c6131 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -259,6 +259,7 @@ TEST_F(RouterTest, RouteNotFound) { router_.decodeHeaders(headers, true); EXPECT_EQ(1UL, stats_store_.counter("test.no_route").value()); EXPECT_TRUE(verifyHostUpstreamStats(0, 0)); + EXPECT_EQ(callbacks_.details_, "route_not_found"); } TEST_F(RouterTest, ClusterNotFound) { @@ -271,6 +272,7 @@ TEST_F(RouterTest, ClusterNotFound) { router_.decodeHeaders(headers, true); EXPECT_EQ(1UL, stats_store_.counter("test.no_cluster").value()); EXPECT_TRUE(verifyHostUpstreamStats(0, 0)); + EXPECT_EQ(callbacks_.details_, "cluster_not_found"); } TEST_F(RouterTest, PoolFailureWithPriority) { @@ -301,6 +303,7 @@ TEST_F(RouterTest, PoolFailureWithPriority) { HttpTestUtility::addDefaultHeaders(headers); router_.decodeHeaders(headers, true); EXPECT_TRUE(verifyHostUpstreamStats(0, 1)); + EXPECT_EQ(callbacks_.details_, "upstream_reset_before_response_started{connection failure}"); } TEST_F(RouterTest, Http1Upstream) { @@ -500,6 +503,8 @@ TEST_F(RouterTest, AddCookie) { HttpTestUtility::addDefaultHeaders(headers); router_.decodeHeaders(headers, true); + absl::string_view rc_details2 = "via_upstream"; + EXPECT_CALL(callbacks_.stream_info_, setResponseCodeDetails(rc_details2)); Http::HeaderMapPtr response_headers(new Http::TestHeaderMapImpl{{":status", "200"}}); response_decoder->decodeHeaders(std::move(response_headers), true); // When the router filter gets reset we should cancel the pool request. @@ -696,6 +701,7 @@ TEST_F(RouterTest, NoHost) { .counter("upstream_rq_maintenance_mode") .value()); EXPECT_TRUE(verifyHostUpstreamStats(0, 0)); + EXPECT_EQ(callbacks_.details_, "no_healthy_upstream"); } TEST_F(RouterTest, MaintenanceMode) { @@ -720,6 +726,7 @@ TEST_F(RouterTest, MaintenanceMode) { EXPECT_EQ(1U, cm_.thread_local_cluster_.cluster_.info_->load_report_stats_store_ .counter("upstream_rq_dropped") .value()); + EXPECT_EQ(callbacks_.details_, "maintenance_mode"); } // Validate that we don't set x-envoy-overloaded when Envoy header suppression @@ -1555,6 +1562,8 @@ TEST_F(RouterTest, RetryUpstreamResetResponseStarted) { Http::HeaderMapPtr response_headers(new Http::TestHeaderMapImpl{{":status", "200"}}); EXPECT_CALL(cm_.conn_pool_.host_->outlier_detector_, putHttpResponseCode(200)); response_decoder->decodeHeaders(std::move(response_headers), false); + absl::string_view rc_details2 = "upstream_reset_after_response_started{remote reset}"; + EXPECT_CALL(callbacks_.stream_info_, setResponseCodeDetails(rc_details2)); EXPECT_CALL(cm_.conn_pool_.host_->outlier_detector_, putHttpResponseCode(503)); encoder1.stream_.resetStream(Http::StreamResetReason::RemoteReset); // For normal HTTP, once we have a 200 we consider this a success, even if a @@ -3108,6 +3117,7 @@ TEST_F(WatermarkTest, RetryRequestNotComplete) { // This should not trigger a retry as the retry state has been deleted. EXPECT_CALL(cm_.conn_pool_.host_->outlier_detector_, putHttpResponseCode(503)); encoder1.stream_.resetStream(Http::StreamResetReason::RemoteReset); + EXPECT_EQ(callbacks_.details_, "upstream_reset_before_response_started{remote reset}"); } class RouterTestChildSpan : public RouterTestBase { diff --git a/test/extensions/access_loggers/http_grpc/grpc_access_log_integration_test.cc b/test/extensions/access_loggers/http_grpc/grpc_access_log_integration_test.cc index 0e3188e776839..11ef6b03cf299 100644 --- a/test/extensions/access_loggers/http_grpc/grpc_access_log_integration_test.cc +++ b/test/extensions/access_loggers/http_grpc/grpc_access_log_integration_test.cc @@ -134,6 +134,7 @@ TEST_P(AccessLogIntegrationTest, BasicAccessLogFlow) { response: response_code: value: 404 + response_code_details: "route_not_found" response_headers_bytes: 54 )EOF", VersionInfo::version()))); @@ -157,6 +158,7 @@ TEST_P(AccessLogIntegrationTest, BasicAccessLogFlow) { response: response_code: value: 404 + response_code_details: "route_not_found" response_headers_bytes: 54 )EOF")); @@ -204,6 +206,7 @@ TEST_P(AccessLogIntegrationTest, BasicAccessLogFlow) { response: response_code: value: 404 + response_code_details: "route_not_found" response_headers_bytes: 54 )EOF", VersionInfo::version()))); diff --git a/test/mocks/http/mocks.h b/test/mocks/http/mocks.h index c02a110fc3350..774672196a95e 100644 --- a/test/mocks/http/mocks.h +++ b/test/mocks/http/mocks.h @@ -153,7 +153,8 @@ class MockStreamDecoderFilterCallbacks : public StreamDecoderFilterCallbacks, void sendLocalReply(Code code, absl::string_view body, std::function modify_headers, const absl::optional grpc_status, - absl::string_view) override { + absl::string_view details) override { + details_ = std::string(details); Utility::sendLocalReply( is_grpc_request_, [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { @@ -192,6 +193,7 @@ class MockStreamDecoderFilterCallbacks : public StreamDecoderFilterCallbacks, std::list callbacks_{}; testing::NiceMock active_span_; testing::NiceMock tracing_config_; + std::string details_; bool is_grpc_request_{}; bool is_head_request_{false}; bool stream_destroyed_{}; From effdd68239a9190d522316d76cd19881ddd6eb1f Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Wed, 24 Apr 2019 11:40:34 -0400 Subject: [PATCH 4/7] always using consts Signed-off-by: Alyssa Wilk --- include/envoy/stream_info/stream_info.h | 8 ++++++++ source/common/router/router.cc | 6 ++++-- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/include/envoy/stream_info/stream_info.h b/include/envoy/stream_info/stream_info.h index 92d32fe4ada7f..0c6ac0eeb27e8 100644 --- a/include/envoy/stream_info/stream_info.h +++ b/include/envoy/stream_info/stream_info.h @@ -116,6 +116,14 @@ struct ResponseCodeDetailValues { const std::string UpstreamTimeout = "upstream_response_timeout"; // The final upstream try timed out const std::string UpstreamPerTryTimeout = "upstream_per_try_timeout"; + // The upstream connection was reset before a response was started. This + // will generally be accompanied by details about why the reset occurred. + const std::string EarlyUpstreamReset = "upstream_reset_before_response_started"; + // The upstream connection was reset before a response was started. This + // will generally be accompanied by details about why the reset occurred but + // indicates that original "success" headers may have been sent downstream + // despite the subsequent failure. + const std::string LateUpstreamReset = "upstream_reset_after_response_started"; // TODO(#6542): continue addding values for sendLocalReply use-cases }; diff --git a/source/common/router/router.cc b/source/common/router/router.cc index bed953009faa7..db63532d3621e 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -687,9 +687,11 @@ void Filter::onUpstreamReset(Http::StreamResetReason reset_reason, const bool dropped = reset_reason == Http::StreamResetReason::Overflow; callbacks_->streamInfo().setUpstreamTransportFailureReason(transport_failure_reason); + const std::string& basic_details = + downstream_response_started_ ? StreamInfo::ResponseCodeDetails::get().LateUpstreamReset + : StreamInfo::ResponseCodeDetails::get().EarlyUpstreamReset; std::string details = absl::StrCat( - "upstream_reset_", downstream_response_started_ ? "after" : "before", "_response_started{", - Http::Utility::resetReasonToString(reset_reason), + basic_details, "{", Http::Utility::resetReasonToString(reset_reason), transport_failure_reason.empty() ? "" : absl::StrCat(",", transport_failure_reason), "}"); onUpstreamAbort(Http::Code::ServiceUnavailable, response_flags, body, dropped, details); } From 204db09c688c66fcd7a97c60f1a9500bb2dfabf9 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Mon, 29 Apr 2019 10:40:43 -0400 Subject: [PATCH 5/7] reviewer comments Signed-off-by: Alyssa Wilk --- include/envoy/http/filter.h | 16 +++++++++++++--- include/envoy/stream_info/stream_info.h | 3 ++- source/common/router/router.cc | 2 +- test/common/http/conn_manager_impl_test.cc | 4 ++-- 4 files changed, 18 insertions(+), 7 deletions(-) diff --git a/include/envoy/http/filter.h b/include/envoy/http/filter.h index 218dbc7a73949..44fa734bc3be9 100644 --- a/include/envoy/http/filter.h +++ b/include/envoy/http/filter.h @@ -283,6 +283,18 @@ class StreamDecoderFilterCallbacks : public virtual StreamFilterCallbacks { */ virtual HeaderMap& addDecodedTrailers() PURE; + /** + * A wrapper for legacy sendLocalReply replies without the details parameter. + * See sendLocalReply below for usage + */ + // TODO(alyssawilk) send an email to envoy-dev for API change, add for all other filters, and + // delete this placeholder. + void sendLocalReply(Code response_code, absl::string_view body_text, + std::function modify_headers, + const absl::optional grpc_status) { + sendLocalReply(response_code, body_text, modify_headers, grpc_status, ""); + } + /** * 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 @@ -297,12 +309,10 @@ class StreamDecoderFilterCallbacks : public virtual StreamFilterCallbacks { * @param grpc_status the gRPC status code to override the httpToGrpcStatus mapping with. * @param details a string detailing why this local reply was sent. */ - // TODO(alyssawilk) send an email to envoy-dev for API change, add for all other filters, and - // make details required. virtual void sendLocalReply(Code response_code, absl::string_view body_text, std::function modify_headers, const absl::optional grpc_status, - absl::string_view details = "") PURE; + absl::string_view details) PURE; /** * Called with 100-Continue headers to be encoded. diff --git a/include/envoy/stream_info/stream_info.h b/include/envoy/stream_info/stream_info.h index 0c6ac0eeb27e8..7c87bfc4c137a 100644 --- a/include/envoy/stream_info/stream_info.h +++ b/include/envoy/stream_info/stream_info.h @@ -106,7 +106,8 @@ struct ResponseCodeDetailValues { const std::string RouteNotFound = "route_not_found"; // A direct response was generated by the router filter. const std::string DirectResponse = "direct_response"; - // The request was rejected by the router filter because there was no route found. + // The request was rejected by the router filter because there was no cluster found for the + // selected route. const std::string ClusterNotFound = "cluster_not_found"; // The request was rejected by the router filter because the cluster was in maintenance mode. const std::string MaintenanceMode = "maintenance_mode"; diff --git a/source/common/router/router.cc b/source/common/router/router.cc index db63532d3621e..bdfcd87e0869a 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -690,7 +690,7 @@ void Filter::onUpstreamReset(Http::StreamResetReason reset_reason, const std::string& basic_details = downstream_response_started_ ? StreamInfo::ResponseCodeDetails::get().LateUpstreamReset : StreamInfo::ResponseCodeDetails::get().EarlyUpstreamReset; - std::string details = absl::StrCat( + const std::string details = absl::StrCat( basic_details, "{", Http::Utility::resetReasonToString(reset_reason), transport_failure_reason.empty() ? "" : absl::StrCat(",", transport_failure_reason), "}"); onUpstreamAbort(Http::Code::ServiceUnavailable, response_flags, body, dropped, details); diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index e94c147ae8650..201c8a20e2b41 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -590,7 +590,7 @@ TEST_F(HttpConnectionManagerImplTest, PathFailedtoSanitize) { EXPECT_CALL(*filter, encodeHeaders(_, true)); EXPECT_CALL(response_encoder_, encodeHeaders(_, true)) - .WillOnce(Invoke([filter](const HeaderMap& headers, bool) -> void { + .WillOnce(Invoke([&](const HeaderMap& headers, bool) -> void { EXPECT_EQ("400", headers.Status()->value().getStringView()); EXPECT_EQ("path_normalization_failed", filter->decoder_callbacks_->streamInfo().responseCodeDetails().value()); @@ -1369,7 +1369,7 @@ TEST_F(HttpConnectionManagerImplTest, PerStreamIdleTimeoutGlobal) { // 408 direct response after timeout. EXPECT_CALL(response_encoder_, encodeHeaders(_, false)) - .WillOnce(Invoke([&](const HeaderMap& headers, bool) -> void { + .WillOnce(Invoke([](const HeaderMap& headers, bool) -> void { EXPECT_EQ("408", headers.Status()->value().getStringView()); })); std::string response_body; From 5b504018e251e4ac87a7b4378b081ee8d6f979af Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Tue, 30 Apr 2019 09:22:40 -0400 Subject: [PATCH 6/7] one last comment Signed-off-by: Alyssa Wilk --- include/envoy/stream_info/stream_info.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/envoy/stream_info/stream_info.h b/include/envoy/stream_info/stream_info.h index 7c87bfc4c137a..e795781615979 100644 --- a/include/envoy/stream_info/stream_info.h +++ b/include/envoy/stream_info/stream_info.h @@ -120,7 +120,7 @@ struct ResponseCodeDetailValues { // The upstream connection was reset before a response was started. This // will generally be accompanied by details about why the reset occurred. const std::string EarlyUpstreamReset = "upstream_reset_before_response_started"; - // The upstream connection was reset before a response was started. This + // The upstream connection was reset after a response was started. This // will generally be accompanied by details about why the reset occurred but // indicates that original "success" headers may have been sent downstream // despite the subsequent failure. From 47b7db690b68218e7c3ea6265490766825e69945 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Tue, 30 Apr 2019 10:33:37 -0400 Subject: [PATCH 7/7] comments Signed-off-by: Alyssa Wilk --- include/envoy/stream_info/stream_info.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/include/envoy/stream_info/stream_info.h b/include/envoy/stream_info/stream_info.h index e795781615979..aed53c9506c4a 100644 --- a/include/envoy/stream_info/stream_info.h +++ b/include/envoy/stream_info/stream_info.h @@ -76,6 +76,7 @@ enum ResponseFlag { struct ResponseCodeDetailValues { // Response code was set by the upstream. const std::string ViaUpstream = "via_upstream"; + // Envoy is doing non-streaming proxying, and the request payload exceeded // configured limits. const std::string RequestPayloadTooLarge = "request_payload_too_large"; // Envoy is doing non-streaming proxying, and the response payload exceeded @@ -91,7 +92,7 @@ struct ResponseCodeDetailValues { const std::string LowVersion = "low_version"; // The request was rejected due to the Host: or :authority field missing const std::string MissingHost = "missing_host_header"; - // The request was rejected due to the Host: or :authority field missing + // The request was rejected due to the request headers being larger than the configured limit. const std::string RequestHeadersTooLarge = "request_headers_too_large"; // The request was rejected due to the Path or :path header field missing. const std::string MissingPath = "missing_path_rejected";