Skip to content

http: rc details for main Envoy workflow #6560

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.

Sign up for GitHub

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

Merged
alyssawilk merged 8 commits into from
Apr 30, 2019
Merged

http: rc details for main Envoy workflow #6560

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
78e41f2
WIP: rc details for main Envoy workflow
alyssawilk Apr 11, 2019
f576258
format
alyssawilk Apr 11, 2019
84414ff
Merge branch 'master' into sendLocalReply
alyssawilk Apr 23, 2019
eee0b1b
cleanup, reviewer comments, tests
alyssawilk Apr 24, 2019
effdd68
always using consts
alyssawilk Apr 24, 2019
204db09
reviewer comments
alyssawilk Apr 29, 2019
5b50401
one last comment
alyssawilk Apr 30, 2019
47b7db6
comments
alyssawilk Apr 30, 2019
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
6 changes: 5 additions & 1 deletion include/envoy/http/filter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<void(HeaderMap& headers)> modify_headers,
const absl::optional<Grpc::Status::GrpcStatus> grpc_status) PURE;
const absl::optional<Grpc::Status::GrpcStatus> grpc_status,
absl::string_view details = "") PURE;

@lizan lizan Apr 24, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

default arguments is only allowed for non-virtual methods
https://google.github.io/styleguide/cppguide.html#Default_Arguments


/**
* Called with 100-Continue headers to be encoded.
Expand Down
40 changes: 38 additions & 2 deletions include/envoy/stream_info/stream_info.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,44 @@ enum ResponseFlag {
struct ResponseCodeDetailValues {

@PiotrSikora PiotrSikora Apr 22, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Those look great, thanks, but you're effectively rewriting even the existing Proxy-Status types, so what's your plan in regards to that header? Using corresponding standard types and then using those values in the details field or completely ignoring standard types and only using http_{request,response}_error with those values in details? In either case, I think you might need a std::pair (or perhaps even std::tuple) here, instead of only strings.

@alyssawilk alyssawilk Apr 22, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think when we implement proxy-status these could be extended. I could do a one off with today's proxy-status codes but I really don't want to sign up for keeping it up to date and I'd prefer to not add code which is going to bitrot as the IETF makes forward progress. I don't think creating this without the tuple creates more work for when we implement proxy-status, and it might reduce it (by filling in the details field)

// Response code was set by the upstream.
const std::string ViaUpstream = "via_upstream";

// TODO(#6542): add values for sendLocalReply use-cases
// configured limits.

@eziskind eziskind Apr 30, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks like this comment is cut off.

const std::string REQUEST_PAYLOAD_TOO_LARGE = "request_payload_too_large";

@eziskind eziskind Apr 23, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: I think the preference is to use PascalCase for these constants.

@alyssawilk alyssawilk Apr 24, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

PascalCase for enums for sure. For consts "In the Envoy codebase we use ConstantVar or CONSTANT_VAR."

That said I thought ALL_CAPS was strictly prefered and I prefer PascalCase so I'll switch nonetheless :-P

@jmarantz jmarantz Apr 24, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I had argued for a slight preference for PascalCase for constants to avoid collisions with macros in files we don't control. That's codified mildly in https://github.com/envoyproxy/envoy/blob/master/STYLE.md

The Google C++ style guide points out that constant vars should be named kConstantVar. In the Envoy codebase we use ConstantVar or CONSTANT_VAR. If you pick CONSTANT_VAR, please be certain the name is globally significant to avoid potential conflicts with #defines, which are not namespace-scoped, and may appear in externally controlled header files.

// 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 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";
// 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

@eziskind eziskind Apr 30, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

copy paste mistake for this comment

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?

@PiotrSikora PiotrSikora Apr 22, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we simply use filter's well know name? Unique ID might make debugging harder.

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

@snowp snowp Apr 24, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maybe "because the cluster in the selected route does not exist"?

const std::string CLUSTER_NOT_FOUND = "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";
// 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}";
// TODO(#6542): continue addding values for sendLocalReply use-cases
};

typedef ConstSingleton<ResponseCodeDetailValues> ResponseCodeDetails;
Expand Down
6 changes: 3 additions & 3 deletions source/common/http/async_client_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,9 +327,9 @@ class AsyncStreamImpl : public AsyncClient::Stream,
}
void sendLocalReply(Code code, absl::string_view body,
std::function<void(HeaderMap& headers)> modify_headers,
const absl::optional<Grpc::Status::GrpcStatus> grpc_status) override {
// TODO(#6542): add an extra parameter for setting rc details
stream_info_.setResponseCodeDetails("");
const absl::optional<Grpc::Status::GrpcStatus> grpc_status,
absl::string_view details) override {
stream_info_.setResponseCodeDetails(details);
Utility::sendLocalReply(
is_grpc_request_,
[this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void {
Expand Down
36 changes: 23 additions & 13 deletions source/common/http/conn_manager_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -494,17 +494,18 @@ 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);
}
}

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(
Expand Down Expand Up @@ -594,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);
absl::nullopt, StreamInfo::ResponseCodeDetails::get().OVERLOAD);
return;
}

Expand Down Expand Up @@ -622,7 +623,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
Expand All @@ -645,15 +647,17 @@ 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;
}
}

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;
}

Expand All @@ -666,15 +670,17 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers,
request_headers_->Path()->value().getStringView()[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;
}

// Path sanitization should happen before any path access other than the above sanity check.
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;
}

Expand Down Expand Up @@ -704,7 +710,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.
Expand Down Expand Up @@ -1114,13 +1121,14 @@ void ConnectionManagerImpl::ActiveStream::refreshCachedRoute() {
void ConnectionManagerImpl::ActiveStream::sendLocalReply(
bool is_grpc_request, Code code, absl::string_view body,
const std::function<void(HeaderMap& headers)>& modify_headers, bool is_head_request,
const absl::optional<Grpc::Status::GrpcStatus> grpc_status) {
const absl::optional<Grpc::Status::GrpcStatus> grpc_status, absl::string_view 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);
Utility::sendLocalReply(
is_grpc_request,
[this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void {
Expand Down Expand Up @@ -1875,7 +1883,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);
}
}

Expand Down Expand Up @@ -1980,6 +1988,8 @@ void ConnectionManagerImpl::ActiveStreamEncoderFilter::responseDataTooLarge() {
parent_.state_.encoder_filters_streaming_ = true;
allowIteration();

parent_.stream_info_.setResponseCodeDetails(
StreamInfo::ResponseCodeDetails::get().REQUEST_HEADERS_TOO_LARGE);
Http::Utility::sendLocalReply(
Grpc::Common::hasGrpcContentType(*parent_.request_headers_),
[&](HeaderMapPtr&& response_headers, bool end_stream) -> void {
Expand Down
11 changes: 6 additions & 5 deletions source/common/http/conn_manager_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,11 +221,11 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,

void sendLocalReply(Code code, absl::string_view body,
std::function<void(HeaderMap& headers)> modify_headers,
const absl::optional<Grpc::Status::GrpcStatus> grpc_status) override {
// TODO(#6542): add an extra parameter for setting rc details
parent_.stream_info_.setResponseCodeDetails("");
const absl::optional<Grpc::Status::GrpcStatus> grpc_status,
absl::string_view details) override {
parent_.stream_info_.setResponseCodeDetails(details);
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;
Expand Down Expand Up @@ -359,7 +359,8 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
void sendLocalReply(bool is_grpc_request, Code code, absl::string_view body,
const std::function<void(HeaderMap& headers)>& modify_headers,
bool is_head_request,
const absl::optional<Grpc::Status::GrpcStatus> grpc_status);
const absl::optional<Grpc::Status::GrpcStatus> 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
Expand Down
30 changes: 21 additions & 9 deletions source/common/router/router.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,8 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e
headers.Path()->value().getStringView());

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;
}

Expand All @@ -310,7 +311,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;
}

Expand All @@ -323,7 +324,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();
Expand All @@ -350,7 +352,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e
headers.insertEnvoyOverloaded().value(Http::Headers::get().EnvoyOverloadedValues.True);
}
},
absl::nullopt);
absl::nullopt, StreamInfo::ResponseCodeDetails::get().MAINTENANCE_MODE);
cluster_->stats().upstream_rq_maintenance_mode_.inc();
return Http::FilterHeadersStatus::StopIteration;
}
Expand Down Expand Up @@ -424,7 +426,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) {
Expand Down Expand Up @@ -581,14 +584,15 @@ void Filter::updateOutlierDetection(Http::Code code, UpstreamRequest& upstream_r
}
}

// TODO(alyssawilk) 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) {
ASSERT(upstream_requests_.size() <= 1);
// If we have not yet sent anything downstream, send a response with an appropriate status code.
// Otherwise just reset the ongoing response.
Expand Down Expand Up @@ -626,7 +630,7 @@ void Filter::onUpstreamAbort(Http::Code code, StreamInfo::ResponseFlag response_
headers.insertEnvoyOverloaded().value(Http::Headers::get().EnvoyOverloadedValues.True);
}
},
absl::nullopt);
absl::nullopt, details);
}
}

Expand Down Expand Up @@ -680,7 +684,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 multiple levels to add
// their information.
std::string details =
absl::StrCat("upstream_reset{", reset_reason, ",", transport_failure_reason, "}");

@eziskind eziskind Apr 12, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Both the reset reason and the transport failure reason are already available in the StreamInfo so why include them also in the rc details string? Seems simpler to just have the rc details by a fixed string like "upstream reset" which can be easily checked for and then access logs can look at the other fields if they want more specifics.

@alyssawilk alyssawilk Apr 22, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's totally doable. What I don't like about that is I feel like it encourages a paradigm of "look elsewhere for the underlying failure cause"

So today we have have "rc details" which might point us to "upstream failure" which would require us to know which of transport failure and/or reset reason to debug. But then we could have a generic "cache failure" which could tell you to look at the cache details (was the entry not present? expired? etc) and "WASM failure" with a bunch of reasons the code failed to execute.

I want all of the information to be communicated in access logs and headers and I'd mildly prefer to combine the data in the string as it's created rather than to encourage each complicated filter to split out layers of reasons.

@eziskind eziskind Apr 23, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That makes sense. It does make it a bit difficult to programmatically check if the response code details from a StreamInfo matches this "constant" but perhaps that is not a big deal - we could add some kind of regex matching if we really want that.

onUpstreamAbort(Http::Code::ServiceUnavailable, response_flags, body, dropped, details);
}

StreamInfo::ResponseFlag
Expand Down
2 changes: 1 addition & 1 deletion source/common/router/router.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,7 +389,7 @@ class Filter : Logger::Loggable<Logger::Id::router>,
// 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,
UpstreamRequest& upstream_request, bool end_stream);
void onUpstreamData(Buffer::Instance& data, UpstreamRequest& upstream_request, bool end_stream);
Expand Down
3 changes: 2 additions & 1 deletion test/mocks/http/mocks.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,8 @@ class MockStreamDecoderFilterCallbacks : public StreamDecoderFilterCallbacks,
// Http::StreamDecoderFilterCallbacks
void sendLocalReply(Code code, absl::string_view body,
std::function<void(HeaderMap& headers)> modify_headers,
const absl::optional<Grpc::Status::GrpcStatus> grpc_status) override {
const absl::optional<Grpc::Status::GrpcStatus> grpc_status,
absl::string_view) override {
Utility::sendLocalReply(
is_grpc_request_,
[this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void {
Expand Down