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 5 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
51 changes: 49 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,55 @@ 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 RequestPayloadTooLarge = "request_payload_too_large";
// Envoy is doing non-streaming proxying, and the response payload exceeded
// configured limits.
const std::string ResponsePayloadTooLArge = "response_payload_too_large";
// The per-stream keepalive timeout was exceeded.
const std::string StreamIdleTimeout = "stream_idle_timeout";
// The per-stream total request timeout was exceeded
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";
// The HTTP/1.0 or HTTP/0.9 request was rejected due to HTTP/1.0 support not being configured.
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

@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 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 PathNormalizationFailed = "path_normalization_failed";
// The request was rejected because it attempted an unsupported upgrade.
const std::string UpgradeFailed = "upgrade_failed";
// The request was rejected by the router filter because there was no route found.
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.

@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 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";
// The request was rejected by the router filter because there was no healthy upstream found.
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";
// 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

@eziskind eziskind Apr 25, 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: before -> after

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

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
40 changes: 27 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().StreamIdleTimeout);
}
}

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().RequestOverallTimeout);
}

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().LowVersion);
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().MissingHost);
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().RequestHeadersTooLarge);
return;
}

Expand All @@ -664,17 +668,22 @@ 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);
is_head_request_, absl::nullopt,
has_path ? StreamInfo::ResponseCodeDetails::get().AbsolutePath
: StreamInfo::ResponseCodeDetails::get().MissingPath);
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().PathNormalizationFailed);
return;
}

Expand Down Expand Up @@ -704,7 +713,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().UpgradeFailed);
return;
}
// Allow non websocket requests to go through websocket enabled routes.
Expand Down Expand Up @@ -1114,13 +1124,15 @@ 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) {
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();
}
stream_info_.setResponseCodeDetails(details);
Utility::sendLocalReply(
is_grpc_request,
[this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void {
Expand Down Expand Up @@ -1875,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);
absl::nullopt, StreamInfo::ResponseCodeDetails::get().RequestPayloadTooLarge);
}
}

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

parent_.stream_info_.setResponseCodeDetails(
StreamInfo::ResponseCodeDetails::get().RequestHeadersTooLarge);
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
Loading