Skip to content
9 changes: 9 additions & 0 deletions api/envoy/config/filter/accesslog/v2/accesslog.proto
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,9 @@ message AccessLogFilter {

// Header filter.
HeaderFilter header_filter = 8;

// Response flag filter.
ResponseFlagFilter response_flag_filter = 9;
}
}

Expand Down Expand Up @@ -150,3 +153,9 @@ message HeaderFilter {
// check.
envoy.api.v2.route.HeaderMatcher header = 1 [(validate.rules).message.required = true];
}

// Filters requests that received responses with an Envoy response flag set.
// A list of the response flags can be found
// in the access log formatter :ref:`documentation<config_access_log_format_response_flags>`.
message ResponseFlagFilter {
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

alternatively I thought about adding an option to specify the flags you care about. But I think that response flags are important, and infrequent enough to have the filter filter on all of them. Thoughts?

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 way to achieve both settings is have an array field that will specify which response flags to log on and if that field is empty default to all response flags? (or have an enum that says all)

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.

Yeah +1 to what @ccaraman said. I would do that.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yep, that is what I thought, will add.

2 changes: 2 additions & 0 deletions docs/root/configuration/access_log.rst
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,8 @@ The following command operators are supported:
TCP
Total duration in milliseconds of the downstream connection.

.. _config_access_log_format_response_flags:

%RESPONSE_FLAGS%
Additional details about the response or connection, if any. For TCP connections, the response codes mentioned in
the descriptions do not apply. Possible values are:
Expand Down
2 changes: 2 additions & 0 deletions docs/root/intro/version_history.rst
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ Version history
* access log: added ability to format START_TIME.
* access log: added DYNAMIC_METADATA :ref:`access log formatter <config_access_log_format>`.
* access log: added :ref:`HeaderFilter <envoy_api_msg_config.filter.accesslog.v2.HeaderFilter>`
to filter logs based on request headers

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.

Move to 1.8.0 section.

* access log: added :ref:`response flag filter <envoy_api_msg_config.filter.accesslog.v2.ResponseFlagFilter>` to filter based on the presence of Envoy response flags.
to filter logs based on request headers.
* access log: gRPC Access Log Service (ALS) support added for :ref:`HTTP access logs
<envoy_api_msg_config.accesslog.v2.HttpGrpcAccessLogConfig>`.
Expand Down
5 changes: 5 additions & 0 deletions include/envoy/request_info/request_info.h
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,11 @@ class RequestInfo {
*/
virtual bool getResponseFlag(ResponseFlag response_flag) const PURE;

/**
* @return whether any response flag is set or not.
*/
virtual bool getResponseFlag() const PURE;

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: should this be hasResponseFlag()?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yeah I think that is a better name, and also a better name for the other function getResponseFlag(ResponseFlag response_flag). I wanted to preserve naming. But now that you brought it up again I think it is probably better for clarity to change both names.


/**
* @return upstream host description.
*/
Expand Down
6 changes: 6 additions & 0 deletions source/common/access_log/access_log_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@ FilterFactory::fromProto(const envoy::config::filter::accesslog::v2::AccessLogFi
return FilterPtr{new OrFilter(config.or_filter(), runtime, random)};
case envoy::config::filter::accesslog::v2::AccessLogFilter::kHeaderFilter:
return FilterPtr{new HeaderFilter(config.header_filter())};
case envoy::config::filter::accesslog::v2::AccessLogFilter::kResponseFlagFilter:
return FilterPtr{new ResponseFlagFilter()};
default:
NOT_REACHED;
}
Expand Down Expand Up @@ -174,6 +176,10 @@ bool HeaderFilter::evaluate(const RequestInfo::RequestInfo&,
return Http::HeaderUtility::matchHeaders(request_headers, header_data_);
}

bool ResponseFlagFilter::evaluate(const RequestInfo::RequestInfo& info, const Http::HeaderMap&) {
return info.getResponseFlag();
}

InstanceSharedPtr
AccessLogFactory::fromProto(const envoy::config::filter::accesslog::v2::AccessLog& config,
Server::Configuration::FactoryContext& context) {
Expand Down
12 changes: 12 additions & 0 deletions source/common/access_log/access_log_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,18 @@ class HeaderFilter : public Filter {
std::vector<Http::HeaderUtility::HeaderData> header_data_;
};

/**
* Filter requests that had a response with an Envoy response flag set.
*/
class ResponseFlagFilter : public Filter {
public:
ResponseFlagFilter() {}

// AccessLog::Filter
bool evaluate(const RequestInfo::RequestInfo& info,
const Http::HeaderMap& request_headers) override;
};

/**
* Access log factory that reads the configuration from proto.
*/
Expand Down
2 changes: 2 additions & 0 deletions source/common/request_info/request_info_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,8 @@ struct RequestInfoImpl : public RequestInfo {

bool getResponseFlag(ResponseFlag flag) const override { return response_flags_ & flag; }

bool getResponseFlag() const override { return response_flags_ != 0; }

void onUpstreamHostSelected(Upstream::HostDescriptionConstSharedPtr host) override {
upstream_host_ = host;
}
Expand Down
19 changes: 19 additions & 0 deletions test/common/access_log/access_log_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -724,6 +724,25 @@ name: envoy.file_access_log
log->log(&request_headers_, &response_headers_, &response_trailers_, request_info_);
}

TEST_F(AccessLogImplTest, ResponseFlagFilter) {
const std::string yaml = R"EOF(
name: envoy.file_access_log
filter:
response_flag_filter: {}
config:
path: /dev/null
)EOF";

InstanceSharedPtr log = AccessLogFactory::fromProto(parseAccessLogFromV2Yaml(yaml), context_);

EXPECT_CALL(*file_, write(_)).Times(0);
log->log(&request_headers_, &response_headers_, &response_trailers_, request_info_);

request_info_.setResponseFlag(RequestInfo::ResponseFlag::NoRouteFound);
EXPECT_CALL(*file_, write(_));
log->log(&request_headers_, &response_headers_, &response_trailers_, request_info_);
}

} // namespace
} // namespace AccessLog
} // namespace Envoy
1 change: 1 addition & 0 deletions test/common/access_log/test_util.h
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ class TestRequestInfo : public RequestInfo::RequestInfo {
bool getResponseFlag(Envoy::RequestInfo::ResponseFlag response_flag) const override {
return response_flags_ & response_flag;
}
bool getResponseFlag() const override { return response_flags_ != 0; }
void setResponseFlag(Envoy::RequestInfo::ResponseFlag response_flag) override {
response_flags_ |= response_flag;
}
Expand Down
2 changes: 2 additions & 0 deletions test/common/request_info/request_info_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ TEST(RequestInfoImplTest, ResponseFlagTest) {
RateLimited};

RequestInfoImpl request_info(Http::Protocol::Http2);
EXPECT_FALSE(request_info.getResponseFlag());
for (ResponseFlag flag : responseFlags) {
// Test cumulative setting of response flags.
EXPECT_FALSE(request_info.getResponseFlag(flag))
Expand All @@ -104,6 +105,7 @@ TEST(RequestInfoImplTest, ResponseFlagTest) {
EXPECT_TRUE(request_info.getResponseFlag(flag))
<< fmt::format("Flag: {} was expected to be set", flag);
}
EXPECT_TRUE(request_info.getResponseFlag());
}

TEST(RequestInfoImplTest, MiscSettersAndGetters) {
Expand Down
1 change: 1 addition & 0 deletions test/mocks/request_info/mocks.h
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ class MockRequestInfo : public RequestInfo {
MOCK_CONST_METHOD0(responseCode, absl::optional<uint32_t>());
MOCK_CONST_METHOD0(bytesSent, uint64_t());
MOCK_CONST_METHOD1(getResponseFlag, bool(ResponseFlag));
MOCK_CONST_METHOD0(getResponseFlag, bool());
MOCK_CONST_METHOD0(upstreamHost, Upstream::HostDescriptionConstSharedPtr());
MOCK_CONST_METHOD0(upstreamLocalAddress, const Network::Address::InstanceConstSharedPtr&());
MOCK_CONST_METHOD0(healthCheck, bool());
Expand Down