Skip to content

access_log: Add %REQ_WITHOUT_QUERY()% formatter #7847

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

Closed
dio wants to merge 1 commit into from
Closed

access_log: Add %REQ_WITHOUT_QUERY()% formatter #7847

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 13 additions & 0 deletions docs/root/configuration/access_log.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,19 @@ The following command operators are supported:
TCP
Not implemented ("-").

%REQ_WITHOUT_QUERY(X?Y):Z%
HTTP
Same as **%REQ(X?Y):Z%** but it removes query string from a header entry value.
For example, for the following header entry:

``":path" : "/?ok=true"``

* ``%REQ(:PATH)%`` will log: ``/?ok=true``
* ``%REQ_WITHOUT_QUERY(:PATH)%`` will log: ``/``
Copy link
Contributor

@snowp snowp Aug 8, 2019

Choose a reason for hiding this comment

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

will this ever be used with any other header? wondering if it makes more sense to go with the other suggestion in the issue with making this a top level format

Copy link
Member Author

@dio dio Aug 8, 2019

Choose a reason for hiding this comment

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

OH, I can also see: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Referer that's why I went ahead with this. But yeah, probably a suffix operator is better?

REQ(X?Y):<MAX_LENGTH>:<POST_PROCESSING_OPERATOR>, e.g. REQ(:PATH):2:REMOVE_QUERY, REQ(:PATH):-:REMOVE_QUERY. Need to go back to discuss this in the issue then.

Copy link
Contributor

@snowp snowp Aug 9, 2019

Choose a reason for hiding this comment

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

As long as there are more than one header this makes sense for I'm fine with this. Are suffix operators something we currently support? I don't feel super strongly about either, would probably go with whichever has precedent


TCP
Not implemented ("-").

%RESP(X?Y):Z%
HTTP
Same as **%REQ(X?Y):Z%** but taken from HTTP response headers.
Expand Down
1 change: 1 addition & 0 deletions docs/root/intro/version_history.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ Version history
1.12.0 (pending)
================
* access log: added :ref:`buffering <envoy_api_field_config.accesslog.v2.CommonGrpcAccessLogConfig.buffer_size_bytes>` and :ref:`periodical flushing <envoy_api_field_config.accesslog.v2.CommonGrpcAccessLogConfig.buffer_flush_interval>` support to gRPC access logger. Defaults to 16KB buffer and flushing every 1 second.
* access log: added %REQ_WITHOUT_QUERY(X?Y):Z formatter to remove query string from a header entry value.
* admin: added ability to configure listener :ref:`socket options <envoy_api_field_config.bootstrap.v2.Admin.socket_options>`.
* admin: added config dump support for Secret Discovery Service :ref:`SecretConfigDump <envoy_api_msg_admin.v2alpha.SecretsConfigDump>`.
* config: added access log :ref:`extension filter<envoy_api_field_config.filter.accesslog.v2.AccessLogFilter.extension_filter>`.
Expand Down
39 changes: 24 additions & 15 deletions source/common/access_log/access_log_formatter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,6 @@ void AccessLogFormatParser::parseCommand(const std::string& token, const size_t
std::vector<FormatterProviderPtr> AccessLogFormatParser::parse(const std::string& format) {
std::string current_token;
std::vector<FormatterProviderPtr> formatters;
const std::string DYNAMIC_META_TOKEN = "DYNAMIC_METADATA(";
const std::regex command_w_args_regex(R"EOF(%([A-Z]|_)+(\([^\)]*\))?(:[0-9]+)?(%))EOF");

for (size_t pos = 0; pos < format.length(); ++pos) {
Expand All @@ -245,14 +244,16 @@ std::vector<FormatterProviderPtr> AccessLogFormatParser::parse(const std::string
pos += 1;
int command_end_position = pos + token.length();

if (absl::StartsWith(token, "REQ(")) {
const bool req_without_query = absl::StartsWith(token, "REQ_WITHOUT_QUERY(");
Copy link
Contributor

@snowp snowp Aug 13, 2019

Choose a reason for hiding this comment

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

hmm looking at this code would it make more sense to have REQ_WITHOUT_QUERY just take the header key as an input instead of a string to transform? right now it has to be used with REQ so it doesn't seem like its providing any additional flexibility

if (absl::StartsWith(token, "REQ(") || req_without_query) {
std::string main_header, alternative_header;
absl::optional<size_t> max_length;

parseCommandHeader(token, ReqParamStart, main_header, alternative_header, max_length);
parseCommandHeader(token, req_without_query ? ReqWithoutQueryParamStart : ReqParamStart,
main_header, alternative_header, max_length);

formatters.emplace_back(FormatterProviderPtr{
new RequestHeaderFormatter(main_header, alternative_header, max_length)});
formatters.emplace_back(FormatterProviderPtr{new RequestHeaderFormatter(
main_header, alternative_header, max_length, req_without_query)});
} else if (absl::StartsWith(token, "RESP(")) {
std::string main_header, alternative_header;
absl::optional<size_t> max_length;
Expand All @@ -269,13 +270,13 @@ std::vector<FormatterProviderPtr> AccessLogFormatParser::parse(const std::string

formatters.emplace_back(FormatterProviderPtr{
new ResponseTrailerFormatter(main_header, alternative_header, max_length)});
} else if (absl::StartsWith(token, DYNAMIC_META_TOKEN)) {
} else if (absl::StartsWith(token, "DYNAMIC_METADATA(")) {
std::string filter_namespace;
absl::optional<size_t> max_length;
std::vector<std::string> path;
const size_t start = DYNAMIC_META_TOKEN.size();

parseCommand(token, start, ":", filter_namespace, path, max_length);
parseCommand(token, DynamicMetadataParamStart, ":", filter_namespace, path, max_length);

formatters.emplace_back(
FormatterProviderPtr{new DynamicMetadataFormatter(filter_namespace, path, max_length)});
} else if (absl::StartsWith(token, "START_TIME")) {
Expand Down Expand Up @@ -508,8 +509,9 @@ std::string PlainStringFormatter::format(const Http::HeaderMap&, const Http::Hea

HeaderFormatter::HeaderFormatter(const std::string& main_header,
const std::string& alternative_header,
absl::optional<size_t> max_length)
: main_header_(main_header), alternative_header_(alternative_header), max_length_(max_length) {}
absl::optional<size_t> max_length, bool remove_query)
: main_header_(main_header), alternative_header_(alternative_header), max_length_(max_length),
remove_query_(remove_query) {}

std::string HeaderFormatter::format(const Http::HeaderMap& headers) const {
const Http::HeaderEntry* header = headers.get(main_header_);
Expand All @@ -522,7 +524,14 @@ std::string HeaderFormatter::format(const Http::HeaderMap& headers) const {
if (!header) {
header_value_string = UnspecifiedValueString;
} else {
header_value_string = std::string(header->value().getStringView());
absl::string_view header_value = header->value().getStringView();
if (remove_query_) {
const size_t query_pos = header_value.find('?');
header_value_string = std::string(
header_value.data(), query_pos != header_value.npos ? query_pos : header_value.size());
} else {
header_value_string = std::string(header_value);
}
}

if (max_length_ && header_value_string.length() > max_length_.value()) {
Expand All @@ -535,7 +544,7 @@ std::string HeaderFormatter::format(const Http::HeaderMap& headers) const {
ResponseHeaderFormatter::ResponseHeaderFormatter(const std::string& main_header,
const std::string& alternative_header,
absl::optional<size_t> max_length)
: HeaderFormatter(main_header, alternative_header, max_length) {}
: HeaderFormatter(main_header, alternative_header, max_length, false) {}

std::string ResponseHeaderFormatter::format(const Http::HeaderMap&,
const Http::HeaderMap& response_headers,
Expand All @@ -546,8 +555,8 @@ std::string ResponseHeaderFormatter::format(const Http::HeaderMap&,

RequestHeaderFormatter::RequestHeaderFormatter(const std::string& main_header,
const std::string& alternative_header,
absl::optional<size_t> max_length)
: HeaderFormatter(main_header, alternative_header, max_length) {}
absl::optional<size_t> max_length, bool remove_query)
: HeaderFormatter(main_header, alternative_header, max_length, remove_query) {}

std::string RequestHeaderFormatter::format(const Http::HeaderMap& request_headers,
const Http::HeaderMap&, const Http::HeaderMap&,
Expand All @@ -558,7 +567,7 @@ std::string RequestHeaderFormatter::format(const Http::HeaderMap& request_header
ResponseTrailerFormatter::ResponseTrailerFormatter(const std::string& main_header,
const std::string& alternative_header,
absl::optional<size_t> max_length)
: HeaderFormatter(main_header, alternative_header, max_length) {}
: HeaderFormatter(main_header, alternative_header, max_length, false) {}

std::string ResponseTrailerFormatter::format(const Http::HeaderMap&, const Http::HeaderMap&,
const Http::HeaderMap& response_trailers,
Expand Down
7 changes: 5 additions & 2 deletions source/common/access_log/access_log_formatter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,10 @@ class AccessLogFormatParser {

// the indexes of where the parameters for each directive is expected to begin
static const size_t ReqParamStart{sizeof("REQ(") - 1};
static const size_t ReqWithoutQueryParamStart{sizeof("REQ_WITHOUT_QUERY(") - 1};
static const size_t RespParamStart{sizeof("RESP(") - 1};
static const size_t TrailParamStart{sizeof("TRAILER(") - 1};
static const size_t DynamicMetadataParamStart{sizeof("DYNAMIC_METADATA(") - 1};
static const size_t StartTimeParamStart{sizeof("START_TIME(") - 1};
};

Expand Down Expand Up @@ -137,14 +139,15 @@ class PlainStringFormatter : public FormatterProvider {
class HeaderFormatter {
public:
HeaderFormatter(const std::string& main_header, const std::string& alternative_header,
absl::optional<size_t> max_length);
absl::optional<size_t> max_length, bool remove_query);

std::string format(const Http::HeaderMap& headers) const;

private:
Http::LowerCaseString main_header_;
Http::LowerCaseString alternative_header_;
absl::optional<size_t> max_length_;
const bool remove_query_;
};

/**
Expand All @@ -153,7 +156,7 @@ class HeaderFormatter {
class RequestHeaderFormatter : public FormatterProvider, HeaderFormatter {
public:
RequestHeaderFormatter(const std::string& main_header, const std::string& alternative_header,
absl::optional<size_t> max_length);
absl::optional<size_t> max_length, bool remove_query);

// Formatter::format
std::string format(const Http::HeaderMap& request_headers, const Http::HeaderMap&,
Expand Down
33 changes: 27 additions & 6 deletions test/common/access_log/access_log_formatter_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -546,30 +546,37 @@ TEST(AccessLogFormatterTest, streamInfoFormatter) {

TEST(AccessLogFormatterTest, requestHeaderFormatter) {
StreamInfo::MockStreamInfo stream_info;
Http::TestHeaderMapImpl request_header{{":method", "GET"}, {":path", "/"}};
Http::TestHeaderMapImpl request_header{{":method", "GET"}, {":path", "/?ok=true"}};
Http::TestHeaderMapImpl response_header{{":method", "PUT"}};
Http::TestHeaderMapImpl response_trailer{{":method", "POST"}, {"test-2", "test-2"}};

{
RequestHeaderFormatter formatter(":Method", "", absl::optional<size_t>());
RequestHeaderFormatter formatter(":Method", "", absl::optional<size_t>(), false);
EXPECT_EQ("GET",
formatter.format(request_header, response_header, response_trailer, stream_info));
}

{
RequestHeaderFormatter formatter(":path", ":method", absl::optional<size_t>());
RequestHeaderFormatter formatter(":path", ":method", absl::optional<size_t>(), false);
EXPECT_EQ("/?ok=true",
formatter.format(request_header, response_header, response_trailer, stream_info));
}

{
RequestHeaderFormatter formatter(":path", ":method", absl::optional<size_t>(),
true /* to remove query string from :path */);
EXPECT_EQ("/",
formatter.format(request_header, response_header, response_trailer, stream_info));
}

{
RequestHeaderFormatter formatter(":TEST", ":METHOD", absl::optional<size_t>());
RequestHeaderFormatter formatter(":TEST", ":METHOD", absl::optional<size_t>(), false);
EXPECT_EQ("GET",
formatter.format(request_header, response_header, response_trailer, stream_info));
}

{
RequestHeaderFormatter formatter("does_not_exist", "", absl::optional<size_t>());
RequestHeaderFormatter formatter("does_not_exist", "", absl::optional<size_t>(), false);
EXPECT_EQ("-",
formatter.format(request_header, response_header, response_trailer, stream_info));
}
Expand Down Expand Up @@ -920,7 +927,7 @@ TEST(AccessLogFormatterTest, JsonFormatterMultiTokenTest) {

TEST(AccessLogFormatterTest, CompositeFormatterSuccess) {
StreamInfo::MockStreamInfo stream_info;
Http::TestHeaderMapImpl request_header{{"first", "GET"}, {":path", "/"}};
Http::TestHeaderMapImpl request_header{{"first", "GET"}, {":path", "/?ok=true"}};
Http::TestHeaderMapImpl response_header{{"second", "PUT"}, {"test", "test"}};
Http::TestHeaderMapImpl response_trailer{{"third", "POST"}, {"test-2", "test-2"}};

Expand Down Expand Up @@ -1028,6 +1035,20 @@ TEST(AccessLogFormatterTest, CompositeFormatterSuccess) {
EXPECT_EQ("%%|%%123456000|1522796769%%123|1%%1522796769",
formatter.format(request_header, response_header, response_trailer, stream_info));
}

{
const std::string format = "%REQ(:PATH)%";
FormatterImpl formatter(format);
EXPECT_EQ("/?ok=true",
formatter.format(request_header, response_header, response_trailer, stream_info));
}

{
const std::string format = "%REQ_WITHOUT_QUERY(:PATH)%";
FormatterImpl formatter(format);
EXPECT_EQ("/",
formatter.format(request_header, response_header, response_trailer, stream_info));
}
}

TEST(AccessLogFormatterTest, ParserFailures) {
Expand Down