From 1176b61635edf2a5ea215f8a150009d012e84be3 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Fri, 1 Jun 2018 16:42:49 -0700 Subject: [PATCH 01/15] access log: add response flag filter Signed-off-by: Jose Nino --- .../filter/accesslog/v2/accesslog.proto | 9 +++++++++ docs/root/configuration/access_log.rst | 2 ++ include/envoy/request_info/request_info.h | 5 +++++ source/common/access_log/access_log_impl.cc | 7 +++++++ source/common/access_log/access_log_impl.h | 12 ++++++++++++ .../common/request_info/request_info_impl.h | 2 ++ .../common/access_log/access_log_impl_test.cc | 19 +++++++++++++++++++ test/common/access_log/test_util.h | 3 +++ .../request_info/request_info_impl_test.cc | 2 ++ test/mocks/request_info/mocks.h | 1 + 10 files changed, 62 insertions(+) diff --git a/api/envoy/config/filter/accesslog/v2/accesslog.proto b/api/envoy/config/filter/accesslog/v2/accesslog.proto index 378fb5728ae3a..e107325b2478e 100644 --- a/api/envoy/config/filter/accesslog/v2/accesslog.proto +++ b/api/envoy/config/filter/accesslog/v2/accesslog.proto @@ -338,6 +338,9 @@ message AccessLogFilter { // Header filter. HeaderFilter header_filter = 8; + + // Response flag filter. + ResponseFlagFilter response_flag_filter = 9; } } @@ -428,6 +431,12 @@ message HeaderFilter { envoy.api.v2.route.HeaderMatcher header = 1 [(validate.rules).message.required = true]; } +// Filters requests that received responses with an Envoy response flag set. +// All the response flags are listed +// in the access log formatter :ref:`documentation`. +message ResponseFlagFilter{ +} + // Custom configuration for an AccessLog that writes log entries directly to a file. // Configures the built-in *envoy.file_access_log* AccessLog. message FileAccessLog { diff --git a/docs/root/configuration/access_log.rst b/docs/root/configuration/access_log.rst index 66ceb0760f6d1..ee11c79e860f2 100644 --- a/docs/root/configuration/access_log.rst +++ b/docs/root/configuration/access_log.rst @@ -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: diff --git a/include/envoy/request_info/request_info.h b/include/envoy/request_info/request_info.h index 420b57b2d29dc..6c2c03c368c14 100644 --- a/include/envoy/request_info/request_info.h +++ b/include/envoy/request_info/request_info.h @@ -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; + /** * @return upstream host description. */ diff --git a/source/common/access_log/access_log_impl.cc b/source/common/access_log/access_log_impl.cc index 74f0f5b677fdb..2286248eac0a1 100644 --- a/source/common/access_log/access_log_impl.cc +++ b/source/common/access_log/access_log_impl.cc @@ -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; } @@ -174,6 +176,11 @@ 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) { diff --git a/source/common/access_log/access_log_impl.h b/source/common/access_log/access_log_impl.h index 63c22735f996a..1b5c7a6fc97e3 100644 --- a/source/common/access_log/access_log_impl.h +++ b/source/common/access_log/access_log_impl.h @@ -166,6 +166,18 @@ class HeaderFilter : public Filter { std::vector 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. */ diff --git a/source/common/request_info/request_info_impl.h b/source/common/request_info/request_info_impl.h index f883a0453463a..3f14f1091d0fb 100644 --- a/source/common/request_info/request_info_impl.h +++ b/source/common/request_info/request_info_impl.h @@ -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; } diff --git a/test/common/access_log/access_log_impl_test.cc b/test/common/access_log/access_log_impl_test.cc index 8b51a6b3d0a41..0d26d5f72b005 100644 --- a/test/common/access_log/access_log_impl_test.cc +++ b/test/common/access_log/access_log_impl_test.cc @@ -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 diff --git a/test/common/access_log/test_util.h b/test/common/access_log/test_util.h index 1438b668f6f5f..c734ace9ae8de 100644 --- a/test/common/access_log/test_util.h +++ b/test/common/access_log/test_util.h @@ -30,6 +30,9 @@ 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; } diff --git a/test/common/request_info/request_info_impl_test.cc b/test/common/request_info/request_info_impl_test.cc index 16d435bca44db..bad1933864497 100644 --- a/test/common/request_info/request_info_impl_test.cc +++ b/test/common/request_info/request_info_impl_test.cc @@ -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)) @@ -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) { diff --git a/test/mocks/request_info/mocks.h b/test/mocks/request_info/mocks.h index 47d06faf57b3e..47bf009569bf4 100644 --- a/test/mocks/request_info/mocks.h +++ b/test/mocks/request_info/mocks.h @@ -42,6 +42,7 @@ class MockRequestInfo : public RequestInfo { MOCK_CONST_METHOD0(responseCode, absl::optional()); 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()); From d379e0434211a3374ac592e8c2ec2d2b9cecf7ee Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Fri, 1 Jun 2018 16:55:15 -0700 Subject: [PATCH 02/15] fmt Signed-off-by: Jose Nino --- api/envoy/config/filter/accesslog/v2/accesslog.proto | 2 +- source/common/access_log/access_log_impl.cc | 3 +-- source/common/access_log/access_log_impl.h | 2 +- test/common/access_log/test_util.h | 4 +--- 4 files changed, 4 insertions(+), 7 deletions(-) diff --git a/api/envoy/config/filter/accesslog/v2/accesslog.proto b/api/envoy/config/filter/accesslog/v2/accesslog.proto index e107325b2478e..1f4ac4c98d196 100644 --- a/api/envoy/config/filter/accesslog/v2/accesslog.proto +++ b/api/envoy/config/filter/accesslog/v2/accesslog.proto @@ -434,7 +434,7 @@ message HeaderFilter { // Filters requests that received responses with an Envoy response flag set. // All the response flags are listed // in the access log formatter :ref:`documentation`. -message ResponseFlagFilter{ +message ResponseFlagFilter { } // Custom configuration for an AccessLog that writes log entries directly to a file. diff --git a/source/common/access_log/access_log_impl.cc b/source/common/access_log/access_log_impl.cc index 2286248eac0a1..52c351c852459 100644 --- a/source/common/access_log/access_log_impl.cc +++ b/source/common/access_log/access_log_impl.cc @@ -176,8 +176,7 @@ bool HeaderFilter::evaluate(const RequestInfo::RequestInfo&, return Http::HeaderUtility::matchHeaders(request_headers, header_data_); } -bool ResponseFlagFilter::evaluate(const RequestInfo::RequestInfo& info, - const Http::HeaderMap&) { +bool ResponseFlagFilter::evaluate(const RequestInfo::RequestInfo& info, const Http::HeaderMap&) { return info.getResponseFlag(); } diff --git a/source/common/access_log/access_log_impl.h b/source/common/access_log/access_log_impl.h index 1b5c7a6fc97e3..7a654cc5ae8a0 100644 --- a/source/common/access_log/access_log_impl.h +++ b/source/common/access_log/access_log_impl.h @@ -167,7 +167,7 @@ class HeaderFilter : public Filter { }; /** - * Filter requests that had a response with an Envoy response flag set. + * Filter requests that had a response with an Envoy response flag set. */ class ResponseFlagFilter : public Filter { public: diff --git a/test/common/access_log/test_util.h b/test/common/access_log/test_util.h index c734ace9ae8de..f22c9db71cae7 100644 --- a/test/common/access_log/test_util.h +++ b/test/common/access_log/test_util.h @@ -30,9 +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; - } + bool getResponseFlag() const override { return response_flags_ != 0; } void setResponseFlag(Envoy::RequestInfo::ResponseFlag response_flag) override { response_flags_ |= response_flag; } From 05eb710ea59bf24f7708f0224612dd49ac613599 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Mon, 4 Jun 2018 11:09:19 -0700 Subject: [PATCH 03/15] docs Signed-off-by: Jose Nino --- api/envoy/config/filter/accesslog/v2/accesslog.proto | 2 +- docs/root/intro/version_history.rst | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/api/envoy/config/filter/accesslog/v2/accesslog.proto b/api/envoy/config/filter/accesslog/v2/accesslog.proto index 1f4ac4c98d196..9f461bd920378 100644 --- a/api/envoy/config/filter/accesslog/v2/accesslog.proto +++ b/api/envoy/config/filter/accesslog/v2/accesslog.proto @@ -432,7 +432,7 @@ message HeaderFilter { } // Filters requests that received responses with an Envoy response flag set. -// All the response flags are listed +// A list of the response flags can be found // in the access log formatter :ref:`documentation`. message ResponseFlagFilter { } diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 57d20d6740207..91f7ecae27154 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -9,6 +9,7 @@ Version history * access log: added DYNAMIC_METADATA :ref:`access log formatter `. * access log: added :ref:`HeaderFilter ` to filter logs based on request headers +* access log: added :ref:`response flag filter ` to filter based on the presence of Envoy response flags. * admin: added :http:get:`/config_dump` for dumping the current configuration and associated xDS version information (if applicable). * admin: added :http:get:`/stats/prometheus` as an alternative endpoint for getting stats in prometheus format. From 0b4b6316f4e878f0113f0b6f01aaf432db2fed39 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Mon, 4 Jun 2018 11:16:07 -0700 Subject: [PATCH 04/15] fmt Signed-off-by: Jose Nino --- docs/root/intro/version_history.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 8ca8cf010c0f8..59a2859fd5dad 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -9,7 +9,7 @@ Version history * access log: added DYNAMIC_METADATA :ref:`access log formatter `. * access log: added :ref:`HeaderFilter ` to filter logs based on request headers -* access log: added :ref:`response flag filter ` to filter based on the presence of Envoy response flags. +* access log: added :ref:`response flag filter ` 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 `. From d02b862188cf907cd51f8d6522f2b7a81687ea01 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Mon, 4 Jun 2018 11:54:07 -0700 Subject: [PATCH 05/15] merge conflict Signed-off-by: Jose Nino --- api/envoy/config/filter/accesslog/v2/accesslog.proto | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/api/envoy/config/filter/accesslog/v2/accesslog.proto b/api/envoy/config/filter/accesslog/v2/accesslog.proto index 33c5cb8f30d02..3906529cd6d7e 100644 --- a/api/envoy/config/filter/accesslog/v2/accesslog.proto +++ b/api/envoy/config/filter/accesslog/v2/accesslog.proto @@ -159,15 +159,3 @@ message HeaderFilter { // in the access log formatter :ref:`documentation`. message ResponseFlagFilter { } - -// Custom configuration for an AccessLog that writes log entries directly to a file. -// Configures the built-in *envoy.file_access_log* AccessLog. -message FileAccessLog { - // A path to a local file to which to write the access log entries. - string path = 1 [(validate.rules).string.min_bytes = 1]; - - // Access log format. Envoy supports :ref:`custom access log formats - // ` as well as a :ref:`default format - // `. - string format = 2; -} From 53697d532f4408d20da9ae5f8c75ded12caadeb0 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Wed, 13 Jun 2018 17:13:52 -0700 Subject: [PATCH 06/15] wip Signed-off-by: Jose Nino --- api/bazel/repositories.bzl | 2 +- .../config/filter/accesslog/v2/accesslog.proto | 3 +++ source/common/access_log/BUILD | 4 ++++ source/common/access_log/access_log_impl.cc | 13 ++++++++++++- source/common/access_log/access_log_impl.h | 7 +++++-- source/common/request_info/utility.cc | 6 ++++++ source/common/request_info/utility.h | 1 + 7 files changed, 32 insertions(+), 4 deletions(-) diff --git a/api/bazel/repositories.bzl b/api/bazel/repositories.bzl index 97320951b2c40..52db6f2b90026 100644 --- a/api/bazel/repositories.bzl +++ b/api/bazel/repositories.bzl @@ -3,7 +3,7 @@ GOGOPROTO_SHA = "1adfc126b41513cc696b209667c8656ea7aac67c" # v1.0.0 PROMETHEUS_SHA = "99fa1f4be8e564e8a6b613da7fa6f46c9edafc6c" # Nov 17, 2017 OPENCENSUS_SHA = "ab82e5fdec8267dc2a726544b10af97675970847" # May 23, 2018 -PGV_GIT_SHA = "9f600c2cd2d7031fdc8e25e1c9f5ad81c8cab4fe" +PGV_GIT_SHA = "ce08cec08c77144ab530bb82b33ff267fd906529" load("@bazel_tools//tools/build_defs/repo:git.bzl", "git_repository") diff --git a/api/envoy/config/filter/accesslog/v2/accesslog.proto b/api/envoy/config/filter/accesslog/v2/accesslog.proto index 3906529cd6d7e..518aa2661bcf1 100644 --- a/api/envoy/config/filter/accesslog/v2/accesslog.proto +++ b/api/envoy/config/filter/accesslog/v2/accesslog.proto @@ -158,4 +158,7 @@ message HeaderFilter { // A list of the response flags can be found // in the access log formatter :ref:`documentation`. message ResponseFlagFilter { + repeated string flags = 1 [(validate.rules).repeated .items.string = { + in: ["LH", "UH", "UT", "LR", "UR", "UF", "UC", "UO", "NR", "DI", "FI", "RL", "UAEX"] + }]; } diff --git a/source/common/access_log/BUILD b/source/common/access_log/BUILD index 40896979eae9d..d2e90f6abfd64 100644 --- a/source/common/access_log/BUILD +++ b/source/common/access_log/BUILD @@ -51,7 +51,11 @@ envoy_cc_library( "//source/common/http:header_utility_lib", "//source/common/http:headers_lib", "//source/common/http:utility_lib", + "//source/common/protobuf:utility_lib", + "//source/common/request_info:request_info_lib", + "//source/common/request_info:utility_lib", "//source/common/runtime:uuid_util_lib", "//source/common/tracing:http_tracer_lib", + "@envoy_api//envoy/config/filter/accesslog/v2:accesslog_cc", ], ) diff --git a/source/common/access_log/access_log_impl.cc b/source/common/access_log/access_log_impl.cc index 52c351c852459..810e89d21f1c9 100644 --- a/source/common/access_log/access_log_impl.cc +++ b/source/common/access_log/access_log_impl.cc @@ -4,6 +4,7 @@ #include #include "envoy/common/time.h" +#include "envoy/config/filter/accesslog/v2/accesslog.pb.validate.h" #include "envoy/filesystem/filesystem.h" #include "envoy/http/header_map.h" #include "envoy/runtime/runtime.h" @@ -17,6 +18,8 @@ #include "common/http/header_utility.h" #include "common/http/headers.h" #include "common/http/utility.h" +#include "common/protobuf/utility.h" +#include "common/request_info/utility.h" #include "common/runtime/uuid_util.h" #include "common/tracing/http_tracer_impl.h" @@ -69,7 +72,8 @@ FilterFactory::fromProto(const envoy::config::filter::accesslog::v2::AccessLogFi 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()}; + MessageUtil::validate(config); + return FilterPtr{new ResponseFlagFilter(config.response_flag_filter())}; default: NOT_REACHED; } @@ -176,6 +180,13 @@ bool HeaderFilter::evaluate(const RequestInfo::RequestInfo&, return Http::HeaderUtility::matchHeaders(request_headers, header_data_); } +ResponseFlagFilter::ResponseFlagFilter( + const envoy::config::filter::accesslog::v2::ResponseFlagFilter& config) { + if (config.flags_size() > 0) { + info_.setResponseFlag(RequestInfo::ResponseFlagUtils::toResponseFlag("LH")); + } +} + bool ResponseFlagFilter::evaluate(const RequestInfo::RequestInfo& info, const Http::HeaderMap&) { return info.getResponseFlag(); } diff --git a/source/common/access_log/access_log_impl.h b/source/common/access_log/access_log_impl.h index 7a654cc5ae8a0..dc7f42db21039 100644 --- a/source/common/access_log/access_log_impl.h +++ b/source/common/access_log/access_log_impl.h @@ -6,12 +6,12 @@ #include "envoy/access_log/access_log.h" #include "envoy/config/filter/accesslog/v2/accesslog.pb.h" -#include "envoy/request_info/request_info.h" #include "envoy/runtime/runtime.h" #include "envoy/server/access_log_config.h" #include "common/http/header_utility.h" #include "common/protobuf/protobuf.h" +#include "common/request_info/request_info_impl.h" namespace Envoy { namespace AccessLog { @@ -171,11 +171,14 @@ class HeaderFilter : public Filter { */ class ResponseFlagFilter : public Filter { public: - ResponseFlagFilter() {} + ResponseFlagFilter(const envoy::config::filter::accesslog::v2::ResponseFlagFilter& config); // AccessLog::Filter bool evaluate(const RequestInfo::RequestInfo& info, const Http::HeaderMap& request_headers) override; + +private: + RequestInfo::RequestInfoImpl info_; }; /** diff --git a/source/common/request_info/utility.cc b/source/common/request_info/utility.cc index b8138833068b2..a42cdbd359ce8 100644 --- a/source/common/request_info/utility.cc +++ b/source/common/request_info/utility.cc @@ -88,6 +88,12 @@ const std::string ResponseFlagUtils::toShortString(const RequestInfo& request_in return result.empty() ? NONE : result; } +ResponseFlag ResponseFlagUtils::toResponseFlag(const std::string&) { + static const std::map map = { + {ResponseFlagUtils::FAILED_LOCAL_HEALTH_CHECK, ResponseFlag::FailedLocalHealthCheck}}; + return map.find(ResponseFlagUtils::FAILED_LOCAL_HEALTH_CHECK)->second; +} + const std::string& Utility::formatDownstreamAddressNoPort(const Network::Address::Instance& address) { if (address.type() == Network::Address::Type::Ip) { diff --git a/source/common/request_info/utility.h b/source/common/request_info/utility.h index 15b34fb83c42a..91a247cf22def 100644 --- a/source/common/request_info/utility.h +++ b/source/common/request_info/utility.h @@ -14,6 +14,7 @@ namespace RequestInfo { class ResponseFlagUtils { public: static const std::string toShortString(const RequestInfo& request_info); + static ResponseFlag toResponseFlag(const std::string& response_flag); private: ResponseFlagUtils(); From e5ad2c8b62754e54fc96f494f3cf61cb5a564ee1 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Wed, 20 Jun 2018 14:34:51 -0700 Subject: [PATCH 07/15] wip Signed-off-by: Jose Nino --- include/envoy/request_info/request_info.h | 11 +++++++++++ source/common/access_log/access_log_impl.cc | 9 ++++++--- source/common/request_info/request_info_impl.h | 4 ++++ test/common/access_log/test_util.h | 3 ++- test/common/request_info/request_info_impl_test.cc | 8 ++++++++ test/mocks/request_info/mocks.h | 2 ++ 6 files changed, 33 insertions(+), 4 deletions(-) diff --git a/include/envoy/request_info/request_info.h b/include/envoy/request_info/request_info.h index 6c2c03c368c14..fd4f964e439a5 100644 --- a/include/envoy/request_info/request_info.h +++ b/include/envoy/request_info/request_info.h @@ -58,12 +58,23 @@ class RequestInfo { public: virtual ~RequestInfo() {} + /** + * @return the current state of the response flags. + */ + virtual uint64_t currentResponseFlags() const PURE; + /** * @param response_flag the response flag. Each filter can set independent response flags. The * flags are accumulated. */ virtual void setResponseFlag(ResponseFlag response_flag) 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 flags is non-empty. + */ + virtual bool intersectResponseFlags(uint64_t response_flags) const PURE; + /** * @param host the selected upstream host for the request. */ diff --git a/source/common/access_log/access_log_impl.cc b/source/common/access_log/access_log_impl.cc index 810e89d21f1c9..501425204acba 100644 --- a/source/common/access_log/access_log_impl.cc +++ b/source/common/access_log/access_log_impl.cc @@ -72,7 +72,7 @@ FilterFactory::fromProto(const envoy::config::filter::accesslog::v2::AccessLogFi case envoy::config::filter::accesslog::v2::AccessLogFilter::kHeaderFilter: return FilterPtr{new HeaderFilter(config.header_filter())}; case envoy::config::filter::accesslog::v2::AccessLogFilter::kResponseFlagFilter: - MessageUtil::validate(config); + //MessageUtil::validate(config); return FilterPtr{new ResponseFlagFilter(config.response_flag_filter())}; default: NOT_REACHED; @@ -182,12 +182,15 @@ bool HeaderFilter::evaluate(const RequestInfo::RequestInfo&, ResponseFlagFilter::ResponseFlagFilter( const envoy::config::filter::accesslog::v2::ResponseFlagFilter& config) { - if (config.flags_size() > 0) { - info_.setResponseFlag(RequestInfo::ResponseFlagUtils::toResponseFlag("LH")); + for (int i = 0; i < config.flags_size(); i++) { + info_.setResponseFlag(RequestInfo::ResponseFlagUtils::toResponseFlag(config.flags(i))); } } bool ResponseFlagFilter::evaluate(const RequestInfo::RequestInfo& info, const Http::HeaderMap&) { + if (info_.getResponseFlag()) { + return info_.intersectResponseFlags(info.currentResponseFlags()); + } return info.getResponseFlag(); } diff --git a/source/common/request_info/request_info_impl.h b/source/common/request_info/request_info_impl.h index 3f14f1091d0fb..4f7b009d08287 100644 --- a/source/common/request_info/request_info_impl.h +++ b/source/common/request_info/request_info_impl.h @@ -119,8 +119,12 @@ struct RequestInfoImpl : public RequestInfo { uint64_t bytesSent() const override { return bytes_sent_; } + uint64_t currentResponseFlags() const override { return response_flags_; } + void setResponseFlag(ResponseFlag response_flag) override { response_flags_ |= response_flag; } + bool intersectResponseFlags(uint64_t response_flags) const override { return (response_flags_ & response_flags) != 0; } + bool getResponseFlag(ResponseFlag flag) const override { return response_flags_ & flag; } bool getResponseFlag() const override { return response_flags_ != 0; } diff --git a/test/common/access_log/test_util.h b/test/common/access_log/test_util.h index f22c9db71cae7..4b8c95a882004 100644 --- a/test/common/access_log/test_util.h +++ b/test/common/access_log/test_util.h @@ -26,7 +26,8 @@ class TestRequestInfo : public RequestInfo::RequestInfo { void protocol(Http::Protocol protocol) override { protocol_ = protocol; } absl::optional responseCode() const override { return response_code_; } uint64_t bytesSent() const override { return 2; } - + uint64_t currentResponseFlags() const override { return response_flags_; } + bool intersectResponseFlags(uint64_t response_flags) const override { return (response_flags_ & response_flags) != 0; } bool getResponseFlag(Envoy::RequestInfo::ResponseFlag response_flag) const override { return response_flags_ & response_flag; } diff --git a/test/common/request_info/request_info_impl_test.cc b/test/common/request_info/request_info_impl_test.cc index bad1933864497..e384e4312b185 100644 --- a/test/common/request_info/request_info_impl_test.cc +++ b/test/common/request_info/request_info_impl_test.cc @@ -97,6 +97,8 @@ TEST(RequestInfoImplTest, ResponseFlagTest) { RequestInfoImpl request_info(Http::Protocol::Http2); EXPECT_FALSE(request_info.getResponseFlag()); + EXPECT_EQ(0, request_info.currentResponseFlags()); + EXPECT_FALSE(request_info.intersectResponseFlags(0)); for (ResponseFlag flag : responseFlags) { // Test cumulative setting of response flags. EXPECT_FALSE(request_info.getResponseFlag(flag)) @@ -106,6 +108,12 @@ TEST(RequestInfoImplTest, ResponseFlagTest) { << fmt::format("Flag: {} was expected to be set", flag); } EXPECT_TRUE(request_info.getResponseFlag()); + EXPECT_EQ(4095, request_info.currentResponseFlags()); + + RequestInfoImpl request_info2(Http::Protocol::Http2); + request_info2.setResponseFlag(LocalReset); + + EXPECT_TRUE(request_info.intersectResponseFlags(request_info2.currentResponseFlags())); } TEST(RequestInfoImplTest, MiscSettersAndGetters) { diff --git a/test/mocks/request_info/mocks.h b/test/mocks/request_info/mocks.h index 47bf009569bf4..67bf2a368174c 100644 --- a/test/mocks/request_info/mocks.h +++ b/test/mocks/request_info/mocks.h @@ -15,7 +15,9 @@ class MockRequestInfo : public RequestInfo { ~MockRequestInfo(); // RequestInfo::RequestInfo + MOCK_CONST_METHOD0(currentResponseFlags, uint64_t()); MOCK_METHOD1(setResponseFlag, void(ResponseFlag response_flag)); + MOCK_CONST_METHOD1(intersectResponseFlags, bool(uint64_t)); MOCK_METHOD1(onUpstreamHostSelected, void(Upstream::HostDescriptionConstSharedPtr host)); MOCK_CONST_METHOD0(startTime, SystemTime()); MOCK_CONST_METHOD0(startTimeMonotonic, MonotonicTime()); From c421d95de8fb286d44ccb94a56659114576a1cc7 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Wed, 20 Jun 2018 19:26:56 -0700 Subject: [PATCH 08/15] finish up Signed-off-by: Jose Nino --- include/envoy/request_info/request_info.h | 3 +- source/common/access_log/access_log_impl.cc | 8 +- .../common/request_info/request_info_impl.h | 4 +- source/common/request_info/utility.cc | 24 +++++- source/common/request_info/utility.h | 2 +- .../common/access_log/access_log_impl_test.cc | 78 ++++++++++++++++++- test/common/access_log/test_util.h | 4 +- 7 files changed, 111 insertions(+), 12 deletions(-) diff --git a/include/envoy/request_info/request_info.h b/include/envoy/request_info/request_info.h index d40d661c6f148..508efdf696449 100644 --- a/include/envoy/request_info/request_info.h +++ b/include/envoy/request_info/request_info.h @@ -71,7 +71,8 @@ class RequestInfo { /** * @param response_flags the response_flags to intersect with. - * @return true if the intersection of the response_flags argument and the currently set response flags is non-empty. + * @return true if the intersection of the response_flags argument and the currently set response + * flags is non-empty. */ virtual bool intersectResponseFlags(uint64_t response_flags) const PURE; diff --git a/source/common/access_log/access_log_impl.cc b/source/common/access_log/access_log_impl.cc index 501425204acba..74e8452d7d6ef 100644 --- a/source/common/access_log/access_log_impl.cc +++ b/source/common/access_log/access_log_impl.cc @@ -72,7 +72,7 @@ FilterFactory::fromProto(const envoy::config::filter::accesslog::v2::AccessLogFi case envoy::config::filter::accesslog::v2::AccessLogFilter::kHeaderFilter: return FilterPtr{new HeaderFilter(config.header_filter())}; case envoy::config::filter::accesslog::v2::AccessLogFilter::kResponseFlagFilter: - //MessageUtil::validate(config); + MessageUtil::validate(config); return FilterPtr{new ResponseFlagFilter(config.response_flag_filter())}; default: NOT_REACHED; @@ -183,7 +183,11 @@ bool HeaderFilter::evaluate(const RequestInfo::RequestInfo&, ResponseFlagFilter::ResponseFlagFilter( const envoy::config::filter::accesslog::v2::ResponseFlagFilter& config) { for (int i = 0; i < config.flags_size(); i++) { - info_.setResponseFlag(RequestInfo::ResponseFlagUtils::toResponseFlag(config.flags(i))); + absl::optional response_flag = + RequestInfo::ResponseFlagUtils::toResponseFlag(config.flags(i)); + // The config has been validated. Therefore, every flag in the config will have a mapping. + RELEASE_ASSERT(response_flag.has_value()); + info_.setResponseFlag(response_flag.value()); } } diff --git a/source/common/request_info/request_info_impl.h b/source/common/request_info/request_info_impl.h index af0152cc7824f..b449e2466df1b 100644 --- a/source/common/request_info/request_info_impl.h +++ b/source/common/request_info/request_info_impl.h @@ -127,7 +127,9 @@ struct RequestInfoImpl : public RequestInfo { void setResponseFlag(ResponseFlag response_flag) override { response_flags_ |= response_flag; } - bool intersectResponseFlags(uint64_t response_flags) const override { return (response_flags_ & response_flags) != 0; } + bool intersectResponseFlags(uint64_t response_flags) const override { + return (response_flags_ & response_flags) != 0; + } bool getResponseFlag(ResponseFlag flag) const override { return response_flags_ & flag; } diff --git a/source/common/request_info/utility.cc b/source/common/request_info/utility.cc index a42cdbd359ce8..d0d98746d8ca7 100644 --- a/source/common/request_info/utility.cc +++ b/source/common/request_info/utility.cc @@ -88,10 +88,28 @@ const std::string ResponseFlagUtils::toShortString(const RequestInfo& request_in return result.empty() ? NONE : result; } -ResponseFlag ResponseFlagUtils::toResponseFlag(const std::string&) { +absl::optional ResponseFlagUtils::toResponseFlag(const std::string& flag) { static const std::map map = { - {ResponseFlagUtils::FAILED_LOCAL_HEALTH_CHECK, ResponseFlag::FailedLocalHealthCheck}}; - return map.find(ResponseFlagUtils::FAILED_LOCAL_HEALTH_CHECK)->second; + {ResponseFlagUtils::FAILED_LOCAL_HEALTH_CHECK, ResponseFlag::FailedLocalHealthCheck}, + {ResponseFlagUtils::NO_HEALTHY_UPSTREAM, ResponseFlag::NoHealthyUpstream}, + {ResponseFlagUtils::UPSTREAM_REQUEST_TIMEOUT, ResponseFlag::UpstreamRequestTimeout}, + {ResponseFlagUtils::LOCAL_RESET, ResponseFlag::LocalReset}, + {ResponseFlagUtils::UPSTREAM_REMOTE_RESET, ResponseFlag::UpstreamRemoteReset}, + {ResponseFlagUtils::UPSTREAM_CONNECTION_FAILURE, ResponseFlag::UpstreamConnectionFailure}, + {ResponseFlagUtils::UPSTREAM_CONNECTION_TERMINATION, + ResponseFlag::UpstreamConnectionTermination}, + {ResponseFlagUtils::UPSTREAM_OVERFLOW, ResponseFlag::UpstreamOverflow}, + {ResponseFlagUtils::NO_ROUTE_FOUND, ResponseFlag::NoRouteFound}, + {ResponseFlagUtils::DELAY_INJECTED, ResponseFlag::DelayInjected}, + {ResponseFlagUtils::FAULT_INJECTED, ResponseFlag::FaultInjected}, + {ResponseFlagUtils::RATE_LIMITED, ResponseFlag::RateLimited}, + {ResponseFlagUtils::UNAUTHORIZED_EXTERNAL_SERVICE, ResponseFlag::UnauthorizedExternalService}, + }; + const auto& it = map.find(flag); + if (it != map.end()) { + return absl::make_optional(it->second); + } + return absl::nullopt; } const std::string& diff --git a/source/common/request_info/utility.h b/source/common/request_info/utility.h index 91a247cf22def..879bccd196cff 100644 --- a/source/common/request_info/utility.h +++ b/source/common/request_info/utility.h @@ -14,7 +14,7 @@ namespace RequestInfo { class ResponseFlagUtils { public: static const std::string toShortString(const RequestInfo& request_info); - static ResponseFlag toResponseFlag(const std::string& response_flag); + static absl::optional toResponseFlag(const std::string& response_flag); private: ResponseFlagUtils(); diff --git a/test/common/access_log/access_log_impl_test.cc b/test/common/access_log/access_log_impl_test.cc index 0d26d5f72b005..6759a4450d513 100644 --- a/test/common/access_log/access_log_impl_test.cc +++ b/test/common/access_log/access_log_impl_test.cc @@ -52,8 +52,8 @@ class AccessLogImplTest : public testing::Test { public: AccessLogImplTest() : file_(new Filesystem::MockFile()) { ON_CALL(context_, runtime()).WillByDefault(ReturnRef(runtime_)); - EXPECT_CALL(context_, accessLogManager()).WillOnce(ReturnRef(log_manager_)); - EXPECT_CALL(log_manager_, createAccessLog(_)).WillOnce(Return(file_)); + ON_CALL(context_, accessLogManager()).WillByDefault(ReturnRef(log_manager_)); + ON_CALL(log_manager_, createAccessLog(_)).WillByDefault(Return(file_)); ON_CALL(*file_, write(_)).WillByDefault(SaveArg<0>(&output_)); } @@ -724,7 +724,7 @@ name: envoy.file_access_log log->log(&request_headers_, &response_headers_, &response_trailers_, request_info_); } -TEST_F(AccessLogImplTest, ResponseFlagFilter) { +TEST_F(AccessLogImplTest, ResponseFlagFilterAnyFlag) { const std::string yaml = R"EOF( name: envoy.file_access_log filter: @@ -743,6 +743,78 @@ name: envoy.file_access_log log->log(&request_headers_, &response_headers_, &response_trailers_, request_info_); } +TEST_F(AccessLogImplTest, ResponseFlagFilterSpecificFlag) { + const std::string yaml = R"EOF( +name: envoy.file_access_log +filter: + response_flag_filter: + flags: + - UO +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(_)).Times(0); + log->log(&request_headers_, &response_headers_, &response_trailers_, request_info_); + + request_info_.setResponseFlag(RequestInfo::ResponseFlag::UpstreamOverflow); + EXPECT_CALL(*file_, write(_)); + log->log(&request_headers_, &response_headers_, &response_trailers_, request_info_); +} + +TEST_F(AccessLogImplTest, ResponseFlagFilterSeveralFlags) { + const std::string yaml = R"EOF( +name: envoy.file_access_log +filter: + response_flag_filter: + flags: + - UO + - RL +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(_)).Times(0); + log->log(&request_headers_, &response_headers_, &response_trailers_, request_info_); + + request_info_.setResponseFlag(RequestInfo::ResponseFlag::UpstreamOverflow); + EXPECT_CALL(*file_, write(_)); + log->log(&request_headers_, &response_headers_, &response_trailers_, request_info_); +} + +TEST_F(AccessLogImplTest, ResponseFlagFilterUnsupportedFlag) { + const std::string yaml = R"EOF( +name: envoy.file_access_log +filter: + response_flag_filter: + flags: + - UnsupportedFlag +config: + path: /dev/null + )EOF"; + + EXPECT_THROW_WITH_MESSAGE( + AccessLogFactory::fromProto(parseAccessLogFromV2Yaml(yaml), context_), + ProtoValidationException, + "Proto constraint validation failed (AccessLogFilterValidationError.ResponseFlagFilter: " + "[\"embedded message failed validation\"] | caused by " + "ResponseFlagFilterValidationError.Flags[i]: [\"value must be in list \" [\"LH\" \"UH\" " + "\"UT\" \"LR\" \"UR\" \"UF\" \"UC\" \"UO\" \"NR\" \"DI\" \"FI\" \"RL\" \"UAEX\"]]): " + "response_flag_filter {\n flags: \"UnsupportedFlag\"\n}\n"); +} + } // namespace } // namespace AccessLog } // namespace Envoy diff --git a/test/common/access_log/test_util.h b/test/common/access_log/test_util.h index 5e45e61b99946..74ed684937f6a 100644 --- a/test/common/access_log/test_util.h +++ b/test/common/access_log/test_util.h @@ -29,7 +29,9 @@ class TestRequestInfo : public RequestInfo::RequestInfo { void addBytesSent(uint64_t) override { NOT_IMPLEMENTED; } uint64_t bytesSent() const override { return 2; } uint64_t currentResponseFlags() const override { return response_flags_; } - bool intersectResponseFlags(uint64_t response_flags) const override { return (response_flags_ & response_flags) != 0; } + bool intersectResponseFlags(uint64_t response_flags) const override { + return (response_flags_ & response_flags) != 0; + } bool getResponseFlag(Envoy::RequestInfo::ResponseFlag response_flag) const override { return response_flags_ & response_flag; } From aa5b20d051dcc68a168f21a7e535879f8a10f621 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Wed, 20 Jun 2018 19:33:34 -0700 Subject: [PATCH 09/15] docs Signed-off-by: Jose Nino --- api/envoy/config/filter/accesslog/v2/accesslog.proto | 3 +++ 1 file changed, 3 insertions(+) diff --git a/api/envoy/config/filter/accesslog/v2/accesslog.proto b/api/envoy/config/filter/accesslog/v2/accesslog.proto index 518aa2661bcf1..e6ed12dcaf60d 100644 --- a/api/envoy/config/filter/accesslog/v2/accesslog.proto +++ b/api/envoy/config/filter/accesslog/v2/accesslog.proto @@ -158,6 +158,9 @@ message HeaderFilter { // A list of the response flags can be found // in the access log formatter :ref:`documentation`. message ResponseFlagFilter { + // Only responses with the any of the flags listed in this field will be logged. + // This field is optional if it is not specified, then any response flag will pass + // the filter check. repeated string flags = 1 [(validate.rules).repeated .items.string = { in: ["LH", "UH", "UT", "LR", "UR", "UF", "UC", "UO", "NR", "DI", "FI", "RL", "UAEX"] }]; From 1c99b8a9a8d74d139108c09beac2826a035af32c Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Fri, 22 Jun 2018 15:43:20 -0700 Subject: [PATCH 10/15] wip test Signed-off-by: Jose Nino --- include/envoy/request_info/request_info.h | 4 +- source/common/access_log/access_log_impl.cc | 4 +- .../common/request_info/request_info_impl.h | 4 +- source/common/request_info/utility.cc | 26 +++++----- .../http_grpc/grpc_access_log_impl.cc | 26 +++++----- .../access_log/access_log_formatter_test.cc | 2 +- .../common/access_log/access_log_impl_test.cc | 52 +++++++++++++++++++ test/common/access_log/test_util.h | 4 +- .../request_info/request_info_impl_test.cc | 8 +-- test/common/request_info/utility_test.cc | 38 ++++++++++++-- test/common/tracing/http_tracer_impl_test.cc | 2 +- .../http_grpc/grpc_access_log_impl_test.cc | 4 +- test/mocks/request_info/mocks.h | 4 +- 13 files changed, 129 insertions(+), 49 deletions(-) diff --git a/include/envoy/request_info/request_info.h b/include/envoy/request_info/request_info.h index 508efdf696449..cd8781b54c574 100644 --- a/include/envoy/request_info/request_info.h +++ b/include/envoy/request_info/request_info.h @@ -228,12 +228,12 @@ class RequestInfo { /** * @return whether response flag is set or not. */ - virtual bool getResponseFlag(ResponseFlag response_flag) const PURE; + virtual bool hasResponseFlag(ResponseFlag response_flag) const PURE; /** * @return whether any response flag is set or not. */ - virtual bool getResponseFlag() const PURE; + virtual bool hasResponseFlag() const PURE; /** * @return upstream host description. diff --git a/source/common/access_log/access_log_impl.cc b/source/common/access_log/access_log_impl.cc index 74e8452d7d6ef..eafa6fbf8b0db 100644 --- a/source/common/access_log/access_log_impl.cc +++ b/source/common/access_log/access_log_impl.cc @@ -192,10 +192,10 @@ ResponseFlagFilter::ResponseFlagFilter( } bool ResponseFlagFilter::evaluate(const RequestInfo::RequestInfo& info, const Http::HeaderMap&) { - if (info_.getResponseFlag()) { + if (info_.hasResponseFlag()) { return info_.intersectResponseFlags(info.currentResponseFlags()); } - return info.getResponseFlag(); + return info.hasResponseFlag(); } InstanceSharedPtr diff --git a/source/common/request_info/request_info_impl.h b/source/common/request_info/request_info_impl.h index b449e2466df1b..8b45b7c403799 100644 --- a/source/common/request_info/request_info_impl.h +++ b/source/common/request_info/request_info_impl.h @@ -131,9 +131,9 @@ struct RequestInfoImpl : public RequestInfo { return (response_flags_ & response_flags) != 0; } - bool getResponseFlag(ResponseFlag flag) const override { return response_flags_ & flag; } + bool hasResponseFlag(ResponseFlag flag) const override { return response_flags_ & flag; } - bool getResponseFlag() const override { return response_flags_ != 0; } + bool hasResponseFlag() const override { return response_flags_ != 0; } void onUpstreamHostSelected(Upstream::HostDescriptionConstSharedPtr host) override { upstream_host_ = host; diff --git a/source/common/request_info/utility.cc b/source/common/request_info/utility.cc index d0d98746d8ca7..27a01a6b24453 100644 --- a/source/common/request_info/utility.cc +++ b/source/common/request_info/utility.cc @@ -33,55 +33,55 @@ const std::string ResponseFlagUtils::toShortString(const RequestInfo& request_in static_assert(ResponseFlag::LastFlag == 0x1000, "A flag has been added. Fix this code."); - if (request_info.getResponseFlag(ResponseFlag::FailedLocalHealthCheck)) { + if (request_info.hasResponseFlag(ResponseFlag::FailedLocalHealthCheck)) { appendString(result, FAILED_LOCAL_HEALTH_CHECK); } - if (request_info.getResponseFlag(ResponseFlag::NoHealthyUpstream)) { + if (request_info.hasResponseFlag(ResponseFlag::NoHealthyUpstream)) { appendString(result, NO_HEALTHY_UPSTREAM); } - if (request_info.getResponseFlag(ResponseFlag::UpstreamRequestTimeout)) { + if (request_info.hasResponseFlag(ResponseFlag::UpstreamRequestTimeout)) { appendString(result, UPSTREAM_REQUEST_TIMEOUT); } - if (request_info.getResponseFlag(ResponseFlag::LocalReset)) { + if (request_info.hasResponseFlag(ResponseFlag::LocalReset)) { appendString(result, LOCAL_RESET); } - if (request_info.getResponseFlag(ResponseFlag::UpstreamRemoteReset)) { + if (request_info.hasResponseFlag(ResponseFlag::UpstreamRemoteReset)) { appendString(result, UPSTREAM_REMOTE_RESET); } - if (request_info.getResponseFlag(ResponseFlag::UpstreamConnectionFailure)) { + if (request_info.hasResponseFlag(ResponseFlag::UpstreamConnectionFailure)) { appendString(result, UPSTREAM_CONNECTION_FAILURE); } - if (request_info.getResponseFlag(ResponseFlag::UpstreamConnectionTermination)) { + if (request_info.hasResponseFlag(ResponseFlag::UpstreamConnectionTermination)) { appendString(result, UPSTREAM_CONNECTION_TERMINATION); } - if (request_info.getResponseFlag(ResponseFlag::UpstreamOverflow)) { + if (request_info.hasResponseFlag(ResponseFlag::UpstreamOverflow)) { appendString(result, UPSTREAM_OVERFLOW); } - if (request_info.getResponseFlag(ResponseFlag::NoRouteFound)) { + if (request_info.hasResponseFlag(ResponseFlag::NoRouteFound)) { appendString(result, NO_ROUTE_FOUND); } - if (request_info.getResponseFlag(ResponseFlag::DelayInjected)) { + if (request_info.hasResponseFlag(ResponseFlag::DelayInjected)) { appendString(result, DELAY_INJECTED); } - if (request_info.getResponseFlag(ResponseFlag::FaultInjected)) { + if (request_info.hasResponseFlag(ResponseFlag::FaultInjected)) { appendString(result, FAULT_INJECTED); } - if (request_info.getResponseFlag(ResponseFlag::RateLimited)) { + if (request_info.hasResponseFlag(ResponseFlag::RateLimited)) { appendString(result, RATE_LIMITED); } - if (request_info.getResponseFlag(ResponseFlag::UnauthorizedExternalService)) { + if (request_info.hasResponseFlag(ResponseFlag::UnauthorizedExternalService)) { appendString(result, UNAUTHORIZED_EXTERNAL_SERVICE); } diff --git a/source/extensions/access_loggers/http_grpc/grpc_access_log_impl.cc b/source/extensions/access_loggers/http_grpc/grpc_access_log_impl.cc index b90bbfc352dbf..6804cdd9bd617 100644 --- a/source/extensions/access_loggers/http_grpc/grpc_access_log_impl.cc +++ b/source/extensions/access_loggers/http_grpc/grpc_access_log_impl.cc @@ -88,55 +88,55 @@ void HttpGrpcAccessLog::responseFlagsToAccessLogResponseFlags( static_assert(RequestInfo::ResponseFlag::LastFlag == 0x1000, "A flag has been added. Fix this code."); - if (request_info.getResponseFlag(RequestInfo::ResponseFlag::FailedLocalHealthCheck)) { + if (request_info.hasResponseFlag(RequestInfo::ResponseFlag::FailedLocalHealthCheck)) { common_access_log.mutable_response_flags()->set_failed_local_healthcheck(true); } - if (request_info.getResponseFlag(RequestInfo::ResponseFlag::NoHealthyUpstream)) { + if (request_info.hasResponseFlag(RequestInfo::ResponseFlag::NoHealthyUpstream)) { common_access_log.mutable_response_flags()->set_no_healthy_upstream(true); } - if (request_info.getResponseFlag(RequestInfo::ResponseFlag::UpstreamRequestTimeout)) { + if (request_info.hasResponseFlag(RequestInfo::ResponseFlag::UpstreamRequestTimeout)) { common_access_log.mutable_response_flags()->set_upstream_request_timeout(true); } - if (request_info.getResponseFlag(RequestInfo::ResponseFlag::LocalReset)) { + if (request_info.hasResponseFlag(RequestInfo::ResponseFlag::LocalReset)) { common_access_log.mutable_response_flags()->set_local_reset(true); } - if (request_info.getResponseFlag(RequestInfo::ResponseFlag::UpstreamRemoteReset)) { + if (request_info.hasResponseFlag(RequestInfo::ResponseFlag::UpstreamRemoteReset)) { common_access_log.mutable_response_flags()->set_upstream_remote_reset(true); } - if (request_info.getResponseFlag(RequestInfo::ResponseFlag::UpstreamConnectionFailure)) { + if (request_info.hasResponseFlag(RequestInfo::ResponseFlag::UpstreamConnectionFailure)) { common_access_log.mutable_response_flags()->set_upstream_connection_failure(true); } - if (request_info.getResponseFlag(RequestInfo::ResponseFlag::UpstreamConnectionTermination)) { + if (request_info.hasResponseFlag(RequestInfo::ResponseFlag::UpstreamConnectionTermination)) { common_access_log.mutable_response_flags()->set_upstream_connection_termination(true); } - if (request_info.getResponseFlag(RequestInfo::ResponseFlag::UpstreamOverflow)) { + if (request_info.hasResponseFlag(RequestInfo::ResponseFlag::UpstreamOverflow)) { common_access_log.mutable_response_flags()->set_upstream_overflow(true); } - if (request_info.getResponseFlag(RequestInfo::ResponseFlag::NoRouteFound)) { + if (request_info.hasResponseFlag(RequestInfo::ResponseFlag::NoRouteFound)) { common_access_log.mutable_response_flags()->set_no_route_found(true); } - if (request_info.getResponseFlag(RequestInfo::ResponseFlag::DelayInjected)) { + if (request_info.hasResponseFlag(RequestInfo::ResponseFlag::DelayInjected)) { common_access_log.mutable_response_flags()->set_delay_injected(true); } - if (request_info.getResponseFlag(RequestInfo::ResponseFlag::FaultInjected)) { + if (request_info.hasResponseFlag(RequestInfo::ResponseFlag::FaultInjected)) { common_access_log.mutable_response_flags()->set_fault_injected(true); } - if (request_info.getResponseFlag(RequestInfo::ResponseFlag::RateLimited)) { + if (request_info.hasResponseFlag(RequestInfo::ResponseFlag::RateLimited)) { common_access_log.mutable_response_flags()->set_rate_limited(true); } - if (request_info.getResponseFlag(RequestInfo::ResponseFlag::UnauthorizedExternalService)) { + if (request_info.hasResponseFlag(RequestInfo::ResponseFlag::UnauthorizedExternalService)) { common_access_log.mutable_response_flags()->mutable_unauthorized_details()->set_reason( envoy::data::accesslog::v2::ResponseFlags_Unauthorized_Reason:: ResponseFlags_Unauthorized_Reason_EXTERNAL_SERVICE); diff --git a/test/common/access_log/access_log_formatter_test.cc b/test/common/access_log/access_log_formatter_test.cc index 6cc11f9234643..fe711bf4f2b1e 100644 --- a/test/common/access_log/access_log_formatter_test.cc +++ b/test/common/access_log/access_log_formatter_test.cc @@ -115,7 +115,7 @@ TEST(AccessLogFormatterTest, requestInfoFormatter) { { RequestInfoFormatter response_flags_format("RESPONSE_FLAGS"); - ON_CALL(request_info, getResponseFlag(RequestInfo::ResponseFlag::LocalReset)) + ON_CALL(request_info, hasResponseFlag(RequestInfo::ResponseFlag::LocalReset)) .WillByDefault(Return(true)); EXPECT_EQ("LR", response_flags_format.format(header, header, header, request_info)); } diff --git a/test/common/access_log/access_log_impl_test.cc b/test/common/access_log/access_log_impl_test.cc index 6759a4450d513..cf9b939d825cd 100644 --- a/test/common/access_log/access_log_impl_test.cc +++ b/test/common/access_log/access_log_impl_test.cc @@ -794,6 +794,58 @@ name: envoy.file_access_log log->log(&request_headers_, &response_headers_, &response_trailers_, request_info_); } +TEST_F(AccessLogImplTest, ResponseFlagFilterAllFlagsInPGV) { + const std::string yaml = R"EOF( +name: envoy.file_access_log +filter: + response_flag_filter: + flags: + - LH + - UH + - UT + - LR + - UR + - UF + - UC + - UO + - NR + - DI + - FI + - RL + - UAEX +config: + path: /dev/null + )EOF"; + + static_assert(RequestInfo::ResponseFlag::LastFlag == 0x1000, + "A flag has been added. Fix this code."); + + std::vector all_response_flags = { + RequestInfo::ResponseFlag::FailedLocalHealthCheck, + RequestInfo::ResponseFlag::NoHealthyUpstream, + RequestInfo::ResponseFlag::UpstreamRequestTimeout, + RequestInfo::ResponseFlag::LocalReset, + RequestInfo::ResponseFlag::UpstreamRemoteReset, + RequestInfo::ResponseFlag::UpstreamConnectionFailure, + RequestInfo::ResponseFlag::UpstreamConnectionTermination, + RequestInfo::ResponseFlag::UpstreamOverflow, + RequestInfo::ResponseFlag::NoRouteFound, + RequestInfo::ResponseFlag::DelayInjected, + RequestInfo::ResponseFlag::FaultInjected, + RequestInfo::ResponseFlag::RateLimited, + RequestInfo::ResponseFlag::UnauthorizedExternalService, + }; + + InstanceSharedPtr log = AccessLogFactory::fromProto(parseAccessLogFromV2Yaml(yaml), context_); + + for (const auto response_flag : all_response_flags) { + TestRequestInfo request_info; + request_info.setResponseFlag(response_flag); + EXPECT_CALL(*file_, write(_)); + log->log(&request_headers_, &response_headers_, &response_trailers_, request_info); + } +} + TEST_F(AccessLogImplTest, ResponseFlagFilterUnsupportedFlag) { const std::string yaml = R"EOF( name: envoy.file_access_log diff --git a/test/common/access_log/test_util.h b/test/common/access_log/test_util.h index 74ed684937f6a..4c1ee4e2fe101 100644 --- a/test/common/access_log/test_util.h +++ b/test/common/access_log/test_util.h @@ -32,10 +32,10 @@ class TestRequestInfo : public RequestInfo::RequestInfo { bool intersectResponseFlags(uint64_t response_flags) const override { return (response_flags_ & response_flags) != 0; } - bool getResponseFlag(Envoy::RequestInfo::ResponseFlag response_flag) const override { + bool hasResponseFlag(Envoy::RequestInfo::ResponseFlag response_flag) const override { return response_flags_ & response_flag; } - bool getResponseFlag() const override { return response_flags_ != 0; } + bool hasResponseFlag() const override { return response_flags_ != 0; } void setResponseFlag(Envoy::RequestInfo::ResponseFlag response_flag) override { response_flags_ |= response_flag; } diff --git a/test/common/request_info/request_info_impl_test.cc b/test/common/request_info/request_info_impl_test.cc index 8e3566544de6e..e2eb3a33268ef 100644 --- a/test/common/request_info/request_info_impl_test.cc +++ b/test/common/request_info/request_info_impl_test.cc @@ -96,18 +96,18 @@ TEST(RequestInfoImplTest, ResponseFlagTest) { RateLimited}; RequestInfoImpl request_info(Http::Protocol::Http2); - EXPECT_FALSE(request_info.getResponseFlag()); + EXPECT_FALSE(request_info.hasResponseFlag()); EXPECT_EQ(0, request_info.currentResponseFlags()); EXPECT_FALSE(request_info.intersectResponseFlags(0)); for (ResponseFlag flag : responseFlags) { // Test cumulative setting of response flags. - EXPECT_FALSE(request_info.getResponseFlag(flag)) + EXPECT_FALSE(request_info.hasResponseFlag(flag)) << fmt::format("Flag: {} was already set", flag); request_info.setResponseFlag(flag); - EXPECT_TRUE(request_info.getResponseFlag(flag)) + EXPECT_TRUE(request_info.hasResponseFlag(flag)) << fmt::format("Flag: {} was expected to be set", flag); } - EXPECT_TRUE(request_info.getResponseFlag()); + EXPECT_TRUE(request_info.hasResponseFlag()); EXPECT_EQ(4095, request_info.currentResponseFlags()); RequestInfoImpl request_info2(Http::Protocol::Http2); diff --git a/test/common/request_info/utility_test.cc b/test/common/request_info/utility_test.cc index 47dba21103991..e9058a1f1ed5a 100644 --- a/test/common/request_info/utility_test.cc +++ b/test/common/request_info/utility_test.cc @@ -34,14 +34,14 @@ TEST(ResponseFlagUtilsTest, toShortStringConversion) { for (const auto& test_case : expected) { NiceMock request_info; - ON_CALL(request_info, getResponseFlag(test_case.first)).WillByDefault(Return(true)); + ON_CALL(request_info, hasResponseFlag(test_case.first)).WillByDefault(Return(true)); EXPECT_EQ(test_case.second, ResponseFlagUtils::toShortString(request_info)); } // No flag is set. { NiceMock request_info; - ON_CALL(request_info, getResponseFlag(_)).WillByDefault(Return(false)); + ON_CALL(request_info, hasResponseFlag(_)).WillByDefault(Return(false)); EXPECT_EQ("-", ResponseFlagUtils::toShortString(request_info)); } @@ -49,14 +49,42 @@ TEST(ResponseFlagUtilsTest, toShortStringConversion) { // These are not real use cases, but are used to cover multiple response flags case. { NiceMock request_info; - ON_CALL(request_info, getResponseFlag(ResponseFlag::DelayInjected)).WillByDefault(Return(true)); - ON_CALL(request_info, getResponseFlag(ResponseFlag::FaultInjected)).WillByDefault(Return(true)); - ON_CALL(request_info, getResponseFlag(ResponseFlag::UpstreamRequestTimeout)) + ON_CALL(request_info, hasResponseFlag(ResponseFlag::DelayInjected)).WillByDefault(Return(true)); + ON_CALL(request_info, hasResponseFlag(ResponseFlag::FaultInjected)).WillByDefault(Return(true)); + ON_CALL(request_info, hasResponseFlag(ResponseFlag::UpstreamRequestTimeout)) .WillByDefault(Return(true)); EXPECT_EQ("UT,DI,FI", ResponseFlagUtils::toShortString(request_info)); } } +TEST(ResponseFlagsUtilsTest, toResponseFlagConversion) { + static_assert(ResponseFlag::LastFlag == 0x1000, "A flag has been added. Fix this code."); + + std::vector> expected = { + std::make_pair("LH", ResponseFlag::FailedLocalHealthCheck), + std::make_pair("UH", ResponseFlag::NoHealthyUpstream), + std::make_pair("UT", ResponseFlag::UpstreamRequestTimeout), + std::make_pair("LR", ResponseFlag::LocalReset), + std::make_pair("UR", ResponseFlag::UpstreamRemoteReset), + std::make_pair("UF", ResponseFlag::UpstreamConnectionFailure), + std::make_pair("UC", ResponseFlag::UpstreamConnectionTermination), + std::make_pair("UO", ResponseFlag::UpstreamOverflow), + std::make_pair("NR", ResponseFlag::NoRouteFound), + std::make_pair("DI", ResponseFlag::DelayInjected), + std::make_pair("FI", ResponseFlag::FaultInjected), + std::make_pair("RL", ResponseFlag::RateLimited), + std::make_pair("UAEX", ResponseFlag::UnauthorizedExternalService), + }; + + EXPECT_FALSE(ResponseFlagUtils::toResponseFlag("NonExistentFlag").has_value()); + + for (const auto& test_case : expected) { + absl::optional response_flag = ResponseFlagUtils::toResponseFlag(test_case.first); + EXPECT_TRUE(response_flag.has_value()); + EXPECT_EQ(test_case.second, response_flag.value()); + } +} + TEST(UtilityTest, formatDownstreamAddressNoPort) { EXPECT_EQ("1.2.3.4", Utility::formatDownstreamAddressNoPort(Network::Address::Ipv4Instance("1.2.3.4"))); diff --git a/test/common/tracing/http_tracer_impl_test.cc b/test/common/tracing/http_tracer_impl_test.cc index b1f0707ef4423..0f8d432a543db 100644 --- a/test/common/tracing/http_tracer_impl_test.cc +++ b/test/common/tracing/http_tracer_impl_test.cc @@ -387,7 +387,7 @@ TEST(HttpConnManFinalizerImpl, SpanPopulatedFailureResponse) { absl::optional response_code(503); EXPECT_CALL(request_info, responseCode()).WillRepeatedly(ReturnPointee(&response_code)); EXPECT_CALL(request_info, bytesSent()).WillOnce(Return(100)); - ON_CALL(request_info, getResponseFlag(RequestInfo::ResponseFlag::UpstreamRequestTimeout)) + ON_CALL(request_info, hasResponseFlag(RequestInfo::ResponseFlag::UpstreamRequestTimeout)) .WillByDefault(Return(true)); EXPECT_CALL(request_info, upstreamHost()).WillOnce(Return(nullptr)); diff --git a/test/extensions/access_loggers/http_grpc/grpc_access_log_impl_test.cc b/test/extensions/access_loggers/http_grpc/grpc_access_log_impl_test.cc index e365cff61b448..bfbb62bc7eeb6 100644 --- a/test/extensions/access_loggers/http_grpc/grpc_access_log_impl_test.cc +++ b/test/extensions/access_loggers/http_grpc/grpc_access_log_impl_test.cc @@ -219,7 +219,7 @@ TEST_F(HttpGrpcAccessLogTest, Marshalling) { request_info.addBytesReceived(10); request_info.addBytesSent(20); request_info.response_code_ = 200; - ON_CALL(request_info, getResponseFlag(RequestInfo::ResponseFlag::FaultInjected)) + ON_CALL(request_info, hasResponseFlag(RequestInfo::ResponseFlag::FaultInjected)) .WillByDefault(Return(true)); Http::TestHeaderMapImpl request_headers{ @@ -414,7 +414,7 @@ TEST_F(HttpGrpcAccessLogTest, MarshallingAdditionalHeaders) { TEST(responseFlagsToAccessLogResponseFlagsTest, All) { NiceMock request_info; - ON_CALL(request_info, getResponseFlag(_)).WillByDefault(Return(true)); + ON_CALL(request_info, hasResponseFlag(_)).WillByDefault(Return(true)); envoy::data::accesslog::v2::AccessLogCommon common_access_log; HttpGrpcAccessLog::responseFlagsToAccessLogResponseFlags(common_access_log, request_info); diff --git a/test/mocks/request_info/mocks.h b/test/mocks/request_info/mocks.h index 420b1a811d0b2..24119e0c05681 100644 --- a/test/mocks/request_info/mocks.h +++ b/test/mocks/request_info/mocks.h @@ -45,8 +45,8 @@ class MockRequestInfo : public RequestInfo { MOCK_CONST_METHOD0(responseCode, absl::optional()); MOCK_METHOD1(addBytesSent, void(uint64_t)); MOCK_CONST_METHOD0(bytesSent, uint64_t()); - MOCK_CONST_METHOD1(getResponseFlag, bool(ResponseFlag)); - MOCK_CONST_METHOD0(getResponseFlag, bool()); + MOCK_CONST_METHOD1(hasResponseFlag, bool(ResponseFlag)); + MOCK_CONST_METHOD0(hasResponseFlag, bool()); MOCK_CONST_METHOD0(upstreamHost, Upstream::HostDescriptionConstSharedPtr()); MOCK_METHOD1(setUpstreamLocalAddress, void(const Network::Address::InstanceConstSharedPtr&)); MOCK_CONST_METHOD0(upstreamLocalAddress, const Network::Address::InstanceConstSharedPtr&()); From 53662e87af4099ba8559ae95099395845f771e74 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Tue, 26 Jun 2018 13:41:01 -0700 Subject: [PATCH 11/15] update pgv sha Signed-off-by: Jose Nino --- api/bazel/repositories.bzl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/bazel/repositories.bzl b/api/bazel/repositories.bzl index 840b2c6625c42..b1550bf7851ac 100644 --- a/api/bazel/repositories.bzl +++ b/api/bazel/repositories.bzl @@ -3,7 +3,7 @@ GOGOPROTO_SHA = "1adfc126b41513cc696b209667c8656ea7aac67c" # v1.0.0 PROMETHEUS_SHA = "99fa1f4be8e564e8a6b613da7fa6f46c9edafc6c" # Nov 17, 2017 OPENCENSUS_SHA = "ab82e5fdec8267dc2a726544b10af97675970847" # May 23, 2018 -PGV_GIT_SHA = "345b6b478ef955ad31382955d21fb504e95f38c7" +PGV_GIT_SHA = "f9d2b11e44149635b23a002693b76512b01ae515" load("@bazel_tools//tools/build_defs/repo:git.bzl", "git_repository") From 342185519670e8eaf023efbf3cb8df93997208f5 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Thu, 28 Jun 2018 11:00:35 -0700 Subject: [PATCH 12/15] comments Signed-off-by: Jose Nino --- include/envoy/request_info/request_info.h | 5 ----- source/common/access_log/access_log_impl.cc | 8 ++++---- source/common/access_log/access_log_impl.h | 2 +- source/common/request_info/request_info_impl.h | 2 -- test/common/access_log/test_util.h | 1 - test/common/request_info/request_info_impl_test.cc | 6 ++---- test/mocks/request_info/mocks.h | 1 - 7 files changed, 7 insertions(+), 18 deletions(-) diff --git a/include/envoy/request_info/request_info.h b/include/envoy/request_info/request_info.h index cd8781b54c574..2d37544963c07 100644 --- a/include/envoy/request_info/request_info.h +++ b/include/envoy/request_info/request_info.h @@ -58,11 +58,6 @@ class RequestInfo { public: virtual ~RequestInfo() {} - /** - * @return the current state of the response flags. - */ - virtual uint64_t currentResponseFlags() const PURE; - /** * @param response_flag the response flag. Each filter can set independent response flags. The * flags are accumulated. diff --git a/source/common/access_log/access_log_impl.cc b/source/common/access_log/access_log_impl.cc index eafa6fbf8b0db..ca92094f08440 100644 --- a/source/common/access_log/access_log_impl.cc +++ b/source/common/access_log/access_log_impl.cc @@ -186,14 +186,14 @@ ResponseFlagFilter::ResponseFlagFilter( absl::optional response_flag = RequestInfo::ResponseFlagUtils::toResponseFlag(config.flags(i)); // The config has been validated. Therefore, every flag in the config will have a mapping. - RELEASE_ASSERT(response_flag.has_value()); - info_.setResponseFlag(response_flag.value()); + ASSERT(response_flag.has_value()); + configured_flags_ |= response_flag.value(); } } bool ResponseFlagFilter::evaluate(const RequestInfo::RequestInfo& info, const Http::HeaderMap&) { - if (info_.hasResponseFlag()) { - return info_.intersectResponseFlags(info.currentResponseFlags()); + if (configured_flags_ != 0) { + return info.intersectResponseFlags(configured_flags_); } return info.hasResponseFlag(); } diff --git a/source/common/access_log/access_log_impl.h b/source/common/access_log/access_log_impl.h index dc7f42db21039..04b10addf7dea 100644 --- a/source/common/access_log/access_log_impl.h +++ b/source/common/access_log/access_log_impl.h @@ -178,7 +178,7 @@ class ResponseFlagFilter : public Filter { const Http::HeaderMap& request_headers) override; private: - RequestInfo::RequestInfoImpl info_; + uint64_t configured_flags_{}; }; /** diff --git a/source/common/request_info/request_info_impl.h b/source/common/request_info/request_info_impl.h index 8b45b7c403799..429febc0d7e16 100644 --- a/source/common/request_info/request_info_impl.h +++ b/source/common/request_info/request_info_impl.h @@ -123,8 +123,6 @@ struct RequestInfoImpl : public RequestInfo { uint64_t bytesSent() const override { return bytes_sent_; } - uint64_t currentResponseFlags() const override { return response_flags_; } - void setResponseFlag(ResponseFlag response_flag) override { response_flags_ |= response_flag; } bool intersectResponseFlags(uint64_t response_flags) const override { diff --git a/test/common/access_log/test_util.h b/test/common/access_log/test_util.h index 4c1ee4e2fe101..9080810e38144 100644 --- a/test/common/access_log/test_util.h +++ b/test/common/access_log/test_util.h @@ -28,7 +28,6 @@ class TestRequestInfo : public RequestInfo::RequestInfo { absl::optional responseCode() const override { return response_code_; } void addBytesSent(uint64_t) override { NOT_IMPLEMENTED; } uint64_t bytesSent() const override { return 2; } - uint64_t currentResponseFlags() const override { return response_flags_; } bool intersectResponseFlags(uint64_t response_flags) const override { return (response_flags_ & response_flags) != 0; } diff --git a/test/common/request_info/request_info_impl_test.cc b/test/common/request_info/request_info_impl_test.cc index e2eb3a33268ef..7b1e3b1807173 100644 --- a/test/common/request_info/request_info_impl_test.cc +++ b/test/common/request_info/request_info_impl_test.cc @@ -97,7 +97,6 @@ TEST(RequestInfoImplTest, ResponseFlagTest) { RequestInfoImpl request_info(Http::Protocol::Http2); EXPECT_FALSE(request_info.hasResponseFlag()); - EXPECT_EQ(0, request_info.currentResponseFlags()); EXPECT_FALSE(request_info.intersectResponseFlags(0)); for (ResponseFlag flag : responseFlags) { // Test cumulative setting of response flags. @@ -108,12 +107,11 @@ TEST(RequestInfoImplTest, ResponseFlagTest) { << fmt::format("Flag: {} was expected to be set", flag); } EXPECT_TRUE(request_info.hasResponseFlag()); - EXPECT_EQ(4095, request_info.currentResponseFlags()); RequestInfoImpl request_info2(Http::Protocol::Http2); - request_info2.setResponseFlag(LocalReset); + request_info2.setResponseFlag(FailedLocalHealthCheck); - EXPECT_TRUE(request_info.intersectResponseFlags(request_info2.currentResponseFlags())); + EXPECT_TRUE(request_info2.intersectResponseFlags(FailedLocalHealthCheck)); } TEST(RequestInfoImplTest, MiscSettersAndGetters) { diff --git a/test/mocks/request_info/mocks.h b/test/mocks/request_info/mocks.h index 24119e0c05681..04159bd263637 100644 --- a/test/mocks/request_info/mocks.h +++ b/test/mocks/request_info/mocks.h @@ -15,7 +15,6 @@ class MockRequestInfo : public RequestInfo { ~MockRequestInfo(); // RequestInfo::RequestInfo - MOCK_CONST_METHOD0(currentResponseFlags, uint64_t()); MOCK_METHOD1(setResponseFlag, void(ResponseFlag response_flag)); MOCK_CONST_METHOD1(intersectResponseFlags, bool(uint64_t)); MOCK_METHOD1(onUpstreamHostSelected, void(Upstream::HostDescriptionConstSharedPtr host)); From 9ebe1104b1ed76a21ccd3671eb54b683669fc611 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Thu, 28 Jun 2018 12:37:47 -0700 Subject: [PATCH 13/15] docs Signed-off-by: Jose Nino --- api/envoy/config/filter/accesslog/v2/accesslog.proto | 2 +- docs/root/intro/version_history.rst | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/api/envoy/config/filter/accesslog/v2/accesslog.proto b/api/envoy/config/filter/accesslog/v2/accesslog.proto index e6ed12dcaf60d..6642560694eaf 100644 --- a/api/envoy/config/filter/accesslog/v2/accesslog.proto +++ b/api/envoy/config/filter/accesslog/v2/accesslog.proto @@ -159,7 +159,7 @@ message HeaderFilter { // in the access log formatter :ref:`documentation`. message ResponseFlagFilter { // Only responses with the any of the flags listed in this field will be logged. - // This field is optional if it is not specified, then any response flag will pass + // This field is optional. If it is not specified, then any response flag will pass // the filter check. repeated string flags = 1 [(validate.rules).repeated .items.string = { in: ["LH", "UH", "UT", "LR", "UR", "UF", "UC", "UO", "NR", "DI", "FI", "RL", "UAEX"] diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index cd78218a0c824..ed3f04dbe0860 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -3,6 +3,8 @@ Version history 1.8.0 (Pending) =============== +* access log: added :ref:`response flag filter ` to filter based on the presence of Envoy response flags. + to filter logs based on request headers. * admin: added :http:get:`/hystrix_event_stream` as an endpoint for monitoring envoy's statistics through `Hystrix dashboard `_. * http: response filters not applied to early error paths such as http_parser generated 400s. @@ -22,8 +24,6 @@ Version history * access log: added DYNAMIC_METADATA :ref:`access log formatter `. * access log: added :ref:`HeaderFilter ` to filter logs based on request headers -* access log: added :ref:`response flag filter ` to filter based on the presence of Envoy response flags. - to filter logs based on request headers. * access log: added `%([1-9])?f` as one of START_TIME specifiers to render subseconds. * access log: gRPC Access Log Service (ALS) support added for :ref:`HTTP access logs `. From 08bb66ece63b7b947c8b5d8e85c23ac75b351006 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Thu, 28 Jun 2018 13:47:06 -0700 Subject: [PATCH 14/15] period Signed-off-by: Jose Nino --- docs/root/intro/version_history.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index ed3f04dbe0860..3a07eed00e3bc 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -23,7 +23,7 @@ Version history * access log: added ability to format START_TIME. * access log: added DYNAMIC_METADATA :ref:`access log formatter `. * access log: added :ref:`HeaderFilter ` - to filter logs based on request headers + to filter logs based on request headers. * access log: added `%([1-9])?f` as one of START_TIME specifiers to render subseconds. * access log: gRPC Access Log Service (ALS) support added for :ref:`HTTP access logs `. From 779af0815b5b05699fe075937380bc544fa2a541 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Thu, 28 Jun 2018 17:07:54 -0700 Subject: [PATCH 15/15] comments Signed-off-by: Jose Nino --- docs/root/intro/version_history.rst | 4 ++-- include/envoy/request_info/request_info.h | 2 +- source/common/access_log/BUILD | 1 - source/common/access_log/access_log_impl.cc | 2 +- source/common/access_log/access_log_impl.h | 1 - source/common/request_info/request_info_impl.h | 2 +- test/common/access_log/test_util.h | 2 +- test/common/request_info/request_info_impl_test.cc | 4 ++-- test/mocks/request_info/mocks.h | 2 +- 9 files changed, 9 insertions(+), 11 deletions(-) diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 3a07eed00e3bc..ebc3c78fdfa51 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -3,8 +3,8 @@ Version history 1.8.0 (Pending) =============== -* access log: added :ref:`response flag filter ` to filter based on the presence of Envoy response flags. - to filter logs based on request headers. +* access log: added :ref:`response flag filter ` + to filter based on the presence of Envoy response flags. * admin: added :http:get:`/hystrix_event_stream` as an endpoint for monitoring envoy's statistics through `Hystrix dashboard `_. * http: response filters not applied to early error paths such as http_parser generated 400s. diff --git a/include/envoy/request_info/request_info.h b/include/envoy/request_info/request_info.h index 2d37544963c07..ee21a3ae7e8b1 100644 --- a/include/envoy/request_info/request_info.h +++ b/include/envoy/request_info/request_info.h @@ -228,7 +228,7 @@ class RequestInfo { /** * @return whether any response flag is set or not. */ - virtual bool hasResponseFlag() const PURE; + virtual bool hasAnyResponseFlag() const PURE; /** * @return upstream host description. diff --git a/source/common/access_log/BUILD b/source/common/access_log/BUILD index d2e90f6abfd64..afbca0e51e2cf 100644 --- a/source/common/access_log/BUILD +++ b/source/common/access_log/BUILD @@ -53,7 +53,6 @@ envoy_cc_library( "//source/common/http:utility_lib", "//source/common/protobuf:utility_lib", "//source/common/request_info:request_info_lib", - "//source/common/request_info:utility_lib", "//source/common/runtime:uuid_util_lib", "//source/common/tracing:http_tracer_lib", "@envoy_api//envoy/config/filter/accesslog/v2:accesslog_cc", diff --git a/source/common/access_log/access_log_impl.cc b/source/common/access_log/access_log_impl.cc index ca92094f08440..3fa7fbbc061f5 100644 --- a/source/common/access_log/access_log_impl.cc +++ b/source/common/access_log/access_log_impl.cc @@ -195,7 +195,7 @@ bool ResponseFlagFilter::evaluate(const RequestInfo::RequestInfo& info, const Ht if (configured_flags_ != 0) { return info.intersectResponseFlags(configured_flags_); } - return info.hasResponseFlag(); + return info.hasAnyResponseFlag(); } InstanceSharedPtr diff --git a/source/common/access_log/access_log_impl.h b/source/common/access_log/access_log_impl.h index 04b10addf7dea..cad54fd633012 100644 --- a/source/common/access_log/access_log_impl.h +++ b/source/common/access_log/access_log_impl.h @@ -11,7 +11,6 @@ #include "common/http/header_utility.h" #include "common/protobuf/protobuf.h" -#include "common/request_info/request_info_impl.h" namespace Envoy { namespace AccessLog { diff --git a/source/common/request_info/request_info_impl.h b/source/common/request_info/request_info_impl.h index 429febc0d7e16..f56f34ebc4e52 100644 --- a/source/common/request_info/request_info_impl.h +++ b/source/common/request_info/request_info_impl.h @@ -131,7 +131,7 @@ struct RequestInfoImpl : public RequestInfo { bool hasResponseFlag(ResponseFlag flag) const override { return response_flags_ & flag; } - bool hasResponseFlag() const override { return response_flags_ != 0; } + bool hasAnyResponseFlag() const override { return response_flags_ != 0; } void onUpstreamHostSelected(Upstream::HostDescriptionConstSharedPtr host) override { upstream_host_ = host; diff --git a/test/common/access_log/test_util.h b/test/common/access_log/test_util.h index 9080810e38144..42bc015aebd6f 100644 --- a/test/common/access_log/test_util.h +++ b/test/common/access_log/test_util.h @@ -34,7 +34,7 @@ class TestRequestInfo : public RequestInfo::RequestInfo { bool hasResponseFlag(Envoy::RequestInfo::ResponseFlag response_flag) const override { return response_flags_ & response_flag; } - bool hasResponseFlag() const override { return response_flags_ != 0; } + bool hasAnyResponseFlag() const override { return response_flags_ != 0; } void setResponseFlag(Envoy::RequestInfo::ResponseFlag response_flag) override { response_flags_ |= response_flag; } diff --git a/test/common/request_info/request_info_impl_test.cc b/test/common/request_info/request_info_impl_test.cc index 7b1e3b1807173..687a6264a3c86 100644 --- a/test/common/request_info/request_info_impl_test.cc +++ b/test/common/request_info/request_info_impl_test.cc @@ -96,7 +96,7 @@ TEST(RequestInfoImplTest, ResponseFlagTest) { RateLimited}; RequestInfoImpl request_info(Http::Protocol::Http2); - EXPECT_FALSE(request_info.hasResponseFlag()); + EXPECT_FALSE(request_info.hasAnyResponseFlag()); EXPECT_FALSE(request_info.intersectResponseFlags(0)); for (ResponseFlag flag : responseFlags) { // Test cumulative setting of response flags. @@ -106,7 +106,7 @@ TEST(RequestInfoImplTest, ResponseFlagTest) { EXPECT_TRUE(request_info.hasResponseFlag(flag)) << fmt::format("Flag: {} was expected to be set", flag); } - EXPECT_TRUE(request_info.hasResponseFlag()); + EXPECT_TRUE(request_info.hasAnyResponseFlag()); RequestInfoImpl request_info2(Http::Protocol::Http2); request_info2.setResponseFlag(FailedLocalHealthCheck); diff --git a/test/mocks/request_info/mocks.h b/test/mocks/request_info/mocks.h index 04159bd263637..cafe6815e5bcc 100644 --- a/test/mocks/request_info/mocks.h +++ b/test/mocks/request_info/mocks.h @@ -45,7 +45,7 @@ class MockRequestInfo : public RequestInfo { MOCK_METHOD1(addBytesSent, void(uint64_t)); MOCK_CONST_METHOD0(bytesSent, uint64_t()); MOCK_CONST_METHOD1(hasResponseFlag, bool(ResponseFlag)); - MOCK_CONST_METHOD0(hasResponseFlag, bool()); + MOCK_CONST_METHOD0(hasAnyResponseFlag, bool()); MOCK_CONST_METHOD0(upstreamHost, Upstream::HostDescriptionConstSharedPtr()); MOCK_METHOD1(setUpstreamLocalAddress, void(const Network::Address::InstanceConstSharedPtr&)); MOCK_CONST_METHOD0(upstreamLocalAddress, const Network::Address::InstanceConstSharedPtr&());