Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
1 change: 1 addition & 0 deletions include/envoy/stream_info/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -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",
],
)

Expand Down
26 changes: 26 additions & 0 deletions include/envoy/stream_info/stream_info.h
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -64,6 +65,20 @@ enum ResponseFlag {
LastFlag = StreamIdleTimeout
};

/**
* Constants for the response code details field of StreamInfo.
*
* These provide details about the stream state such as whether the

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.

One other minor nit - do you think it's worth clarifying what's done now vs what this will do?
These will provide such details, but right now they only do rc_set_by_upstream

I'd be fine with a tracking issue assigned my way for actually setting values in sendLocalReply and referencing the issue here?

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.

Filed #6542 to track this. I'll add a reference to this in the code.

* response is from the upstream or from envoy (in case of a local reply).
* Custom extensions can define additional values provided they are appropriately
* scoped to avoid collisions.
*/
struct ResponseCodeDetailValues {
const std::string RC_SET_BY_UPSTREAM = "response_code_set_by_upstream";

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.

nit: I wonder if we have to prefix RC here given the structs name?

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.

Removed the RC_ prefix

};

typedef ConstSingleton<ResponseCodeDetailValues> ResponseCodeDetails;

struct UpstreamTiming {
/**
* Sets the time when the first byte of the request was sent upstream.
Expand Down Expand Up @@ -116,6 +131,12 @@ class StreamInfo {
*/
virtual void setResponseFlag(ResponseFlag response_flag) PURE;

/**
* @param rc_details the response code details string to set for this request.

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 some details here about what response code details are?

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.

Done

* See ResponseCodeDetailValues above for well-known constants.
*/
virtual void setResponseCodeDetails(absl::string_view rc_details) PURE;

/**
* @param response_flags the response_flags to intersect with.
* @return true if the intersection of the response_flags argument and the currently set response
Expand Down Expand Up @@ -153,6 +174,11 @@ class StreamInfo {
*/
virtual absl::optional<uint32_t> responseCode() const PURE;

/**
* @return the response code details.
*/
virtual const absl::optional<std::string>& responseCodeDetails() const PURE;

/**
* @return the time that the first byte of the request was received.
*/
Expand Down
2 changes: 2 additions & 0 deletions source/common/http/async_client_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -321,6 +321,8 @@ 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(eziskind): add an extra parameter for setting rc details
stream_info_.setResponseCodeDetails("");
Utility::sendLocalReply(
is_grpc_request_,
[this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void {
Expand Down
2 changes: 2 additions & 0 deletions source/common/http/conn_manager_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,8 @@ 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(eziskind): add an extra parameter for setting rc details
parent_.stream_info_.setResponseCodeDetails("");
parent_.sendLocalReply(is_grpc_request_, code, body, modify_headers, parent_.is_head_request_,
grpc_status);
}
Expand Down
2 changes: 2 additions & 0 deletions source/common/router/router.cc
Original file line number Diff line number Diff line change
Expand Up @@ -792,6 +792,8 @@ void Filter::onUpstreamHeaders(uint64_t response_code, Http::HeaderMapPtr&& head
onUpstreamComplete();
}

callbacks_->streamInfo().setResponseCodeDetails(
StreamInfo::ResponseCodeDetails::get().RC_SET_BY_UPSTREAM);

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 think this PR would be a bit more correct if you also did the work to set the string to nothing in sendLocalReply.

I don't think we have any response filters that pause on encodeHeaders, but if we did it could be the case that while that encoder filter were doing its RPC, it took too long and for example triggered an idle timeout where the HCM would sendLocalReply. Because that could transform, say, a 200 from upstream to a 408 where the details would eventually be "stream timeout" but for today would be "" rather than "response_code_set_by_upstream"

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.

Done

callbacks_->encodeHeaders(std::move(headers), end_stream);
}

Expand Down
9 changes: 9 additions & 0 deletions source/common/stream_info/stream_info_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,14 @@ struct StreamInfoImpl : public StreamInfo {

absl::optional<uint32_t> responseCode() const override { return response_code_; }

const absl::optional<std::string>& responseCodeDetails() const override {
return response_code_details_;
}

void setResponseCodeDetails(absl::string_view rc_details) override {

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.

Is it up to the caller on how to enrich this information? Or do you imagine codifying this somehow, i.e a comma delimited list of details?

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.

My thinking was that this will be just one of the string constants defined in ResponseCodeDetailValues. That currently only has 1 value but that will change soon (#6542). Also, since this is a string, 3rd party extensions can add additional values if it doesn't make sense to add it to OSS envoy. I don't see a need for making this an array (or comma-separated list). @alyssawilk wdyt?

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.

Just to add another point - I think the idea is that there should be enough distinct strings available to uniquely specify why envoy is returning an error code (more or less one for each time sendLocalReply is called). So a list of strings should not be necessary.

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'm fine with it as-is.

For single-hop responses we only need one rc-details. It might be useful to have a full trace of what happened somewhere (500-retried, timeout-retries, gave up and sent 500) but I think that's better off via some other trace mechanism where we can put arbitrary information about how much time was spent in each filter etc. etc.

For multi-hop responses while we might want both layers captured in access logs (e.g. layer-2 envoy had timeout, layer-1 envoy proxied response code sent by upstream) I think that the information from prior hops would be transmitted via headers, and could be sent to access logs via logging the proxy-status header. SG?

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.

Sounds good, thanks!

response_code_details_.emplace(rc_details);
}

void addBytesSent(uint64_t bytes_sent) override { bytes_sent_ += bytes_sent; }

uint64_t bytesSent() const override { return bytes_sent_; }
Expand Down Expand Up @@ -205,6 +213,7 @@ struct StreamInfoImpl : public StreamInfo {

absl::optional<Http::Protocol> protocol_;
absl::optional<uint32_t> response_code_;
absl::optional<std::string> response_code_details_;
uint64_t response_flags_{};
Upstream::HostDescriptionConstSharedPtr upstream_host_{};
bool health_check_request_{};
Expand Down
3 changes: 3 additions & 0 deletions test/common/http/conn_manager_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3119,6 +3119,9 @@ TEST_F(HttpConnectionManagerImplTest, HitRequestBufferLimits) {
EXPECT_CALL(*encoder_filters_[1], encodeComplete());
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());
}

// Return 413 from an intermediate filter and make sure we don't continue the filter chain.
Expand Down
23 changes: 23 additions & 0 deletions test/common/router/router_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -738,6 +738,29 @@ TEST_F(RouterTestSuppressEnvoyHeaders, MaintenanceMode) {
router_.decodeHeaders(headers, true);
}

TEST_F(RouterTest, ResponseCodeDetailsSetByUpstream) {
NiceMock<Http::MockStreamEncoder> encoder1;
Http::StreamDecoder* response_decoder = nullptr;
EXPECT_CALL(cm_.conn_pool_, newStream(_, _))
.WillOnce(Invoke([&](Http::StreamDecoder& decoder, Http::ConnectionPool::Callbacks& callbacks)
-> Http::ConnectionPool::Cancellable* {
response_decoder = &decoder;
callbacks.onPoolReady(encoder1, cm_.conn_pool_.host_);
return nullptr;
}));
expectResponseTimerCreate();

Http::TestHeaderMapImpl headers;
HttpTestUtility::addDefaultHeaders(headers);
router_.decodeHeaders(headers, true);

Http::HeaderMapPtr response_headers(new Http::TestHeaderMapImpl{{":status", "200"}});
absl::string_view rc_details = StreamInfo::ResponseCodeDetails::get().RC_SET_BY_UPSTREAM;
EXPECT_CALL(callbacks_.stream_info_, setResponseCodeDetails(rc_details));
response_decoder->decodeHeaders(std::move(response_headers), true);
EXPECT_TRUE(verifyHostUpstreamStats(1, 0));
}

// Validate that x-envoy-upstream-service-time is added on a regular
// request/response path.
TEST_F(RouterTest, EnvoyUpstreamServiceTime) {
Expand Down
6 changes: 6 additions & 0 deletions test/common/stream_info/stream_info_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,12 @@ TEST_F(StreamInfoImplTest, MiscSettersAndGetters) {
ASSERT_TRUE(stream_info.responseCode());
EXPECT_EQ(200, stream_info.responseCode().value());

EXPECT_FALSE(stream_info.responseCodeDetails().has_value());
stream_info.setResponseCodeDetails(ResponseCodeDetails::get().RC_SET_BY_UPSTREAM);
ASSERT_TRUE(stream_info.responseCodeDetails().has_value());
EXPECT_EQ(ResponseCodeDetails::get().RC_SET_BY_UPSTREAM,
stream_info.responseCodeDetails().value());

EXPECT_EQ(nullptr, stream_info.upstreamHost());
Upstream::HostDescriptionConstSharedPtr host(new NiceMock<Upstream::MockHostDescription>());
stream_info.onUpstreamHostSelected(host);
Expand Down
7 changes: 7 additions & 0 deletions test/common/stream_info/test_util.h
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,12 @@ class TestStreamInfo : public StreamInfo::StreamInfo {
absl::optional<Http::Protocol> protocol() const override { return protocol_; }
void protocol(Http::Protocol protocol) override { protocol_ = protocol; }
absl::optional<uint32_t> responseCode() const override { return response_code_; }
const absl::optional<std::string>& responseCodeDetails() const override {
return response_code_details_;
}
void setResponseCodeDetails(absl::string_view rc_details) override {
response_code_details_.emplace(rc_details);
}
void addBytesSent(uint64_t) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; }
uint64_t bytesSent() const override { return 2; }
bool intersectResponseFlags(uint64_t response_flags) const override {
Expand Down Expand Up @@ -188,6 +194,7 @@ class TestStreamInfo : public StreamInfo::StreamInfo {

absl::optional<Http::Protocol> protocol_{Http::Protocol::Http11};
absl::optional<uint32_t> response_code_;
absl::optional<std::string> response_code_details_;
uint64_t response_flags_{};
Upstream::HostDescriptionConstSharedPtr upstream_host_{};
bool health_check_request_{};
Expand Down
1 change: 1 addition & 0 deletions test/mocks/stream_info/mocks.cc
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ MockStreamInfo::MockStreamInfo()
}));
ON_CALL(*this, protocol()).WillByDefault(ReturnPointee(&protocol_));
ON_CALL(*this, responseCode()).WillByDefault(ReturnPointee(&response_code_));
ON_CALL(*this, responseCodeDetails()).WillByDefault(ReturnPointee(&response_code_details_));
ON_CALL(*this, addBytesReceived(_)).WillByDefault(Invoke([this](uint64_t bytes_received) {
bytes_received_ += bytes_received;
}));
Expand Down
3 changes: 3 additions & 0 deletions test/mocks/stream_info/mocks.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ class MockStreamInfo : public StreamInfo {

// StreamInfo::StreamInfo
MOCK_METHOD1(setResponseFlag, void(ResponseFlag response_flag));
MOCK_METHOD1(setResponseCodeDetails, void(absl::string_view));
MOCK_CONST_METHOD1(intersectResponseFlags, bool(uint64_t));
MOCK_METHOD1(onUpstreamHostSelected, void(Upstream::HostDescriptionConstSharedPtr host));
MOCK_CONST_METHOD0(startTime, SystemTime());
Expand All @@ -44,6 +45,7 @@ class MockStreamInfo : public StreamInfo {
MOCK_CONST_METHOD0(protocol, absl::optional<Http::Protocol>());
MOCK_METHOD1(protocol, void(Http::Protocol protocol));
MOCK_CONST_METHOD0(responseCode, absl::optional<uint32_t>());
MOCK_CONST_METHOD0(responseCodeDetails, const absl::optional<std::string>&());
MOCK_METHOD1(addBytesSent, void(uint64_t));
MOCK_CONST_METHOD0(bytesSent, uint64_t());
MOCK_CONST_METHOD1(hasResponseFlag, bool(ResponseFlag));
Expand Down Expand Up @@ -90,6 +92,7 @@ class MockStreamInfo : public StreamInfo {
absl::optional<std::chrono::nanoseconds> end_time_;
absl::optional<Http::Protocol> protocol_;
absl::optional<uint32_t> response_code_;
absl::optional<std::string> response_code_details_;
envoy::api::v2::core::Metadata metadata_;
FilterStateImpl filter_state_;
uint64_t bytes_received_{};
Expand Down