From 636e605871e1e30a68d899302ea67c64dc578466 Mon Sep 17 00:00:00 2001 From: Chris Roche Date: Fri, 6 Apr 2018 16:57:13 -0700 Subject: [PATCH 01/33] Router: Route/VHost/W.Cluster local filter configuration Signed-off-by: Chris Roche --- bazel/repository_locations.bzl | 40 +++--- include/envoy/router/router.h | 22 ++++ include/envoy/server/filter_config.h | 9 ++ source/common/config/utility.h | 14 +++ source/common/router/BUILD | 2 + source/common/router/config_impl.cc | 50 +++++++- source/common/router/config_impl.h | 22 ++++ test/common/router/BUILD | 2 + test/common/router/config_impl_test.cc | 163 +++++++++++++++++++++++++ test/mocks/router/mocks.h | 3 + 10 files changed, 304 insertions(+), 23 deletions(-) diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index d3f974e1bff2..3320cc0b8ad3 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -4,10 +4,6 @@ REPOSITORY_LOCATIONS = dict( commit = "9df0c47bc034d60d73d216cd0e090707b3fbea58", # chromium-65.0.3325.146 remote = "https://github.com/google/boringssl", ), - com_google_absl = dict( - commit = "787891a3882795cee0364e8a0f0dda315578d155", - remote = "https://github.com/abseil/abseil-cpp", - ), com_github_bombela_backward = dict( commit = "44ae9609e860e3428cd057f7052e505b4819eb84", # 2018-02-06 remote = "https://github.com/bombela/backward-cpp", @@ -35,23 +31,11 @@ REPOSITORY_LOCATIONS = dict( remote = "https://github.com/gcovr/gcovr", ), com_github_grpc_grpc = dict( - commit = "66b9770a8ad326c1ee0dbedc5a8f32a52a604567", # v1.10.1 + commit = "474c5950686e3962bd339c93d27e369bf64f568f", # v1.10.0 remote = "https://github.com/grpc/grpc.git", ), - io_opentracing_cpp = dict( - commit = "f6be24043e00baa2a25e0c1bb8793433d44ecc8b", - remote = "https://github.com/opentracing/opentracing-cpp", - ), - com_lightstep_tracer_cpp = dict( - commit = "6a198acd328f976984699f7272bbec7c8b220f65", - remote = "https://github.com/lightstep/lightstep-tracer-cpp", # v0.6.1 - ), - lightstep_vendored_googleapis = dict( - commit = "d6f78d948c53f3b400bb46996eb3084359914f9b", - remote = "https://github.com/google/googleapis", - ), com_github_nodejs_http_parser = dict( - commit = "54f55a2f02a823e5f5c87abe853bb76d1170718d", # v2.8.1 + commit = "dd74753cf5cf8944438d6f49ddf46f9659993dfb", # v2.8.0 remote = "https://github.com/nodejs/http-parser", ), com_github_pallets_jinja = dict( @@ -66,6 +50,10 @@ REPOSITORY_LOCATIONS = dict( commit = "f54b0e47a08782a6131cc3d60f94d038fa6e0a51", # v1.1.0 remote = "https://github.com/tencent/rapidjson", ), + com_google_absl = dict( + commit = "787891a3882795cee0364e8a0f0dda315578d155", + remote = "https://github.com/abseil/abseil-cpp", + ), com_google_googletest = dict( commit = "43863938377a9ea1399c0596269e0890b5c5515a", remote = "https://github.com/google/googletest", @@ -75,8 +63,12 @@ REPOSITORY_LOCATIONS = dict( strip_prefix = "protobuf-3.5.0", urls = ["https://github.com/google/protobuf/archive/v3.5.0.tar.gz"], ), + com_lightstep_tracer_cpp = dict( + commit = "6a198acd328f976984699f7272bbec7c8b220f65", + remote = "https://github.com/lightstep/lightstep-tracer-cpp", # v0.6.1 + ), envoy_api = dict( - commit = "2dcc435e8ae1d35f8c3a4fa9f132778482fb1a78", + commit = "c15104fce713856203fa4ad948d752e9a1f657c1", remote = "https://github.com/envoyproxy/data-plane-api", ), grpc_httpjson_transcoding = dict( @@ -84,9 +76,17 @@ REPOSITORY_LOCATIONS = dict( remote = "https://github.com/grpc-ecosystem/grpc-httpjson-transcoding", ), io_bazel_rules_go = dict( - commit = "0.10.3", + commit = "0.10.1", remote = "https://github.com/bazelbuild/rules_go", ), + io_opentracing_cpp = dict( + commit = "f6be24043e00baa2a25e0c1bb8793433d44ecc8b", + remote = "https://github.com/opentracing/opentracing-cpp", + ), + lightstep_vendored_googleapis = dict( + commit = "d6f78d948c53f3b400bb46996eb3084359914f9b", + remote = "https://github.com/google/googleapis", + ), # I'd love to name this `com_github_google_subpar`, but something in the Subpar # code assumes its repository name is just `subpar`. subpar = dict( diff --git a/include/envoy/router/router.h b/include/envoy/router/router.h index 1e1ead94333a..0ccc7eaa0c92 100644 --- a/include/envoy/router/router.h +++ b/include/envoy/router/router.h @@ -258,6 +258,13 @@ class VirtualHost { * @return const Config& the RouteConfiguration that owns this virtual host. */ virtual const Config& routeConfig() const PURE; + + /** + * @return const Protobuf::Message* the per-filter config for the given + * filter name configured for this virtual host. If none is present, the + * nullptr is returned. + */ + virtual const Protobuf::Message* perFilterConfig(const std::string& name) const PURE; }; /** @@ -463,6 +470,14 @@ class RouteEntry : public ResponseEntry { * @return const PathMatchCriterion& the match criterion for this route. */ virtual const PathMatchCriterion& pathMatchCriterion() const PURE; + + /** + * @return const Protobuf::Message* the per-filter config for the given + * filter name configured for this route entry. Only weighted cluster entries + * will potentially have these values available. If none is present, the + * nullptr is returned. + */ + virtual const Protobuf::Message* perFilterConfig(const std::string& name) const PURE; }; /** @@ -508,6 +523,13 @@ class Route { * @return the decorator or nullptr if not defined for the request. */ virtual const Decorator* decorator() const PURE; + + /** + * @return const Protobuf::Message* the per-filter config for the given + * filter name configured for this route. If none is present, the nullptr is + * returned. + */ + virtual const Protobuf::Message* perFilterConfig(const std::string& name) const PURE; }; typedef std::shared_ptr RouteConstSharedPtr; diff --git a/include/envoy/server/filter_config.h b/include/envoy/server/filter_config.h index 14333fa8f5bf..afdbeb27ab52 100644 --- a/include/envoy/server/filter_config.h +++ b/include/envoy/server/filter_config.h @@ -295,6 +295,15 @@ class NamedHttpFilterConfigFactory { */ virtual ProtobufTypes::MessagePtr createEmptyConfigProto() { return nullptr; } + /** + * @return ProtobufTypes::MessagePtr create an empty virtual host, route, or weight cluster-local + * config proto message for v2. The filter config, which arrives in an opaque + * google.protobuf.Struct message, will be converted to JSON and then parsed into this + * empty proto. By default, this method returns the same value as createEmptyConfigProto, + * and can be optionally overridden in implementations. + */ + virtual ProtobufTypes::MessagePtr createEmptyRDSConfigProto() { return createEmptyConfigProto(); } + /** * @return std::string the identifying name for a particular implementation of an http filter * produced by the factory. diff --git a/source/common/config/utility.h b/source/common/config/utility.h index 331ad9c48b94..654085d92795 100644 --- a/source/common/config/utility.h +++ b/source/common/config/utility.h @@ -216,6 +216,20 @@ class Utility { return config; } + template + static ProtobufTypes::MessagePtr translateToFactoryRDSConfig(const ProtoMessage& source, + Factory& factory) { + ProtobufTypes::MessagePtr config = factory.createEmptyRDSConfigProto(); + + if (config == nullptr) { + throw EnvoyException(fmt::format( + "{} factory returned nullptr instead of empty RDS config message.", factory.name())); + } + + MessageUtil::jsonConvert(source, *config); + return config; + } + /** * Create TagProducer instance. Check all tag names for conflicts to avoid * unexpected tag name overwriting. diff --git a/source/common/router/BUILD b/source/common/router/BUILD index 7954f21f2a8a..649fec5b4b13 100644 --- a/source/common/router/BUILD +++ b/source/common/router/BUILD @@ -22,6 +22,7 @@ envoy_cc_library( "//include/envoy/http:header_map_interface", "//include/envoy/router:router_interface", "//include/envoy/runtime:runtime_interface", + "//include/envoy/server:filter_config_interface", "//include/envoy/upstream:cluster_manager_interface", "//include/envoy/upstream:upstream_interface", "//source/common/common:assert_lib", @@ -30,6 +31,7 @@ envoy_cc_library( "//source/common/common:utility_lib", "//source/common/config:metadata_lib", "//source/common/config:rds_json_lib", + "//source/common/config:utility_lib", "//source/common/config:well_known_names", "//source/common/http:headers_lib", "//source/common/http:utility_lib", diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index bb43210610da..d471b4769130 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -11,6 +11,7 @@ #include "envoy/http/header_map.h" #include "envoy/runtime/runtime.h" +#include "envoy/server/filter_config.h" #include "envoy/upstream/cluster_manager.h" #include "envoy/upstream/upstream.h" @@ -21,6 +22,7 @@ #include "common/common/utility.h" #include "common/config/metadata.h" #include "common/config/rds_json.h" +#include "common/config/utility.h" #include "common/config/well_known_names.h" #include "common/http/headers.h" #include "common/http/utility.h" @@ -243,7 +245,8 @@ RouteEntryImplBase::RouteEntryImplBase(const VirtualHostImpl& vhost, route.route().response_headers_to_remove())), opaque_config_(parseOpaqueConfig(route)), decorator_(parseDecorator(route)), direct_response_code_(ConfigUtility::parseDirectResponseCode(route)), - direct_response_body_(ConfigUtility::parseDirectResponseBody(route)) { + direct_response_body_(ConfigUtility::parseDirectResponseBody(route)), + per_filter_configs_(route.per_filter_config()) { if (route.route().has_metadata_match()) { const auto filter_it = route.route().metadata_match().filter_metadata().find( Envoy::Config::MetadataFilters::get().ENVOY_LB); @@ -519,6 +522,10 @@ void RouteEntryImplBase::validateClusters(Upstream::ClusterManager& cm) const { } } +const Protobuf::Message* RouteEntryImplBase::perFilterConfig(const std::string& name) const { + return per_filter_configs_.get(name); +} + RouteEntryImplBase::WeightedClusterEntry::WeightedClusterEntry( const RouteEntryImplBase* parent, const std::string runtime_key, Runtime::Loader& loader, const envoy::api::v2::route::WeightedCluster_ClusterWeight& cluster) @@ -526,7 +533,8 @@ RouteEntryImplBase::WeightedClusterEntry::WeightedClusterEntry( cluster_weight_(PROTOBUF_GET_WRAPPED_REQUIRED(cluster, weight)), request_headers_parser_(HeaderParser::configure(cluster.request_headers_to_add())), response_headers_parser_(HeaderParser::configure(cluster.response_headers_to_add(), - cluster.response_headers_to_remove())) { + cluster.response_headers_to_remove())), + per_filter_configs_(cluster.per_filter_config()) { if (cluster.has_metadata_match()) { const auto filter_it = cluster.metadata_match().filter_metadata().find( Envoy::Config::MetadataFilters::get().ENVOY_LB); @@ -541,6 +549,15 @@ RouteEntryImplBase::WeightedClusterEntry::WeightedClusterEntry( } } +const Protobuf::Message* +RouteEntryImplBase::WeightedClusterEntry::perFilterConfig(const std::string& name) const { + const Protobuf::Message* cfg = per_filter_configs_.get(name); + if (cfg != nullptr) { + return cfg; + } + return DynamicRouteEntry::perFilterConfig(name); +} + PrefixRouteEntryImpl::PrefixRouteEntryImpl(const VirtualHostImpl& vhost, const envoy::api::v2::route::Route& route, Runtime::Loader& loader) @@ -651,7 +668,8 @@ VirtualHostImpl::VirtualHostImpl(const envoy::api::v2::route::VirtualHost& virtu global_route_config_(global_route_config), request_headers_parser_(HeaderParser::configure(virtual_host.request_headers_to_add())), response_headers_parser_(HeaderParser::configure(virtual_host.response_headers_to_add(), - virtual_host.response_headers_to_remove())) { + virtual_host.response_headers_to_remove())), + per_filter_configs_(virtual_host.per_filter_config()) { switch (virtual_host.require_tls()) { case envoy::api::v2::route::VirtualHost::NONE: ssl_requirements_ = SslRequirements::NONE; @@ -715,6 +733,10 @@ VirtualHostImpl::VirtualClusterEntry::VirtualClusterEntry( const Config& VirtualHostImpl::routeConfig() const { return global_route_config_; } +const Protobuf::Message* VirtualHostImpl::perFilterConfig(const std::string& name) const { + return per_filter_configs_.get(name); +} + const VirtualHostImpl* RouteMatcher::findWildcardVirtualHost(const std::string& host) const { // We do a longest wildcard suffix match against the host that's passed in. // (e.g. foo-bar.baz.com should match *-bar.baz.com before matching *.baz.com) @@ -854,5 +876,27 @@ ConfigImpl::ConfigImpl(const envoy::api::v2::RouteConfiguration& config, Runtime config.response_headers_to_remove()); } +PerFilterConfigs::PerFilterConfigs(const Protobuf::Map& configs) { + for (const auto& cfg : configs) { + const std::string& name = cfg.first; + const Protobuf::Struct& struct_config = cfg.second; + + auto& factory = Envoy::Config::Utility::getAndCheckFactory< + Server::Configuration::NamedHttpFilterConfigFactory>(name); + + configs_[name] = Envoy::Config::Utility::translateToFactoryRDSConfig(struct_config, factory); + } +} + +const Protobuf::Message* PerFilterConfigs::get(const std::string& name) const { + auto cfg = configs_.find(name); + + if (cfg == configs_.end()) { + return nullptr; + } + + return cfg->second.get(); +} + } // namespace Router } // namespace Envoy diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h index 359df2ce2f6f..4b974ac874cd 100644 --- a/source/common/router/config_impl.h +++ b/source/common/router/config_impl.h @@ -44,6 +44,16 @@ class Matchable { uint64_t random_value) const PURE; }; +class PerFilterConfigs { +public: + PerFilterConfigs(const Protobuf::Map& configs); + + const Protobuf::Message* get(const std::string& name) const; + +private: + std::unordered_map configs_; +}; + class RouteEntryImplBase; typedef std::shared_ptr RouteEntryImplBaseConstSharedPtr; @@ -66,6 +76,7 @@ class SslRedirectRoute : public Route { const DirectResponseEntry* directResponseEntry() const override { return &SSL_REDIRECTOR; } const RouteEntry* routeEntry() const override { return nullptr; } const Decorator* decorator() const override { return nullptr; } + const Protobuf::Message* perFilterConfig(const std::string&) const override { return nullptr; } private: static const SslRedirector SSL_REDIRECTOR; @@ -119,6 +130,7 @@ class VirtualHostImpl : public VirtualHost { const std::string& name() const override { return name_; } const RateLimitPolicy& rateLimitPolicy() const override { return rate_limit_policy_; } const Config& routeConfig() const override; + const Protobuf::Message* perFilterConfig(const std::string& name) const override; private: enum class SslRequirements { NONE, EXTERNAL_ONLY, ALL }; @@ -154,6 +166,7 @@ class VirtualHostImpl : public VirtualHost { // raw ref to the top level config is currently safe. HeaderParserPtr request_headers_parser_; HeaderParserPtr response_headers_parser_; + PerFilterConfigs per_filter_configs_; }; typedef std::shared_ptr VirtualHostSharedPtr; @@ -357,6 +370,7 @@ class RouteEntryImplBase : public RouteEntry, const DirectResponseEntry* directResponseEntry() const override; const RouteEntry* routeEntry() const override; const Decorator* decorator() const override { return decorator_.get(); } + const Protobuf::Message* perFilterConfig(const std::string& name) const override; protected: const bool case_sensitive_; @@ -430,6 +444,10 @@ class RouteEntryImplBase : public RouteEntry, const RouteEntry* routeEntry() const override { return this; } const Decorator* decorator() const override { return parent_->decorator(); } + const Protobuf::Message* perFilterConfig(const std::string& name) const override { + return parent_->perFilterConfig(name); + }; + private: const RouteEntryImplBase* parent_; const std::string cluster_name_; @@ -469,6 +487,8 @@ class RouteEntryImplBase : public RouteEntry, DynamicRouteEntry::finalizeResponseHeaders(headers, request_info); } + const Protobuf::Message* perFilterConfig(const std::string& name) const override; + private: const std::string runtime_key_; Runtime::Loader& loader_; @@ -476,6 +496,7 @@ class RouteEntryImplBase : public RouteEntry, MetadataMatchCriteriaImplConstPtr cluster_metadata_match_criteria_; HeaderParserPtr request_headers_parser_; HeaderParserPtr response_headers_parser_; + PerFilterConfigs per_filter_configs_; }; typedef std::shared_ptr WeightedClusterEntrySharedPtr; @@ -527,6 +548,7 @@ class RouteEntryImplBase : public RouteEntry, const DecoratorConstPtr decorator_; const absl::optional direct_response_code_; std::string direct_response_body_; + PerFilterConfigs per_filter_configs_; }; /** diff --git a/test/common/router/BUILD b/test/common/router/BUILD index f713221dcc51..842f9787e5a5 100644 --- a/test/common/router/BUILD +++ b/test/common/router/BUILD @@ -18,9 +18,11 @@ envoy_cc_test( "//source/common/http:headers_lib", "//source/common/json:json_loader_lib", "//source/common/router:config_lib", + "//source/server/config/http:buffer_lib", "//test/mocks/runtime:runtime_mocks", "//test/mocks/upstream:upstream_mocks", "//test/test_common:environment_lib", + "//test/test_common:registry_lib", "//test/test_common:utility_lib", ], ) diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index f520ba501f16..a79c4e66a996 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -4,6 +4,9 @@ #include #include +#include "envoy/config/filter/http/buffer/v2/buffer.pb.h" +#include "envoy/server/filter_config.h" + #include "common/config/metadata.h" #include "common/config/rds_json.h" #include "common/config/well_known_names.h" @@ -13,10 +16,13 @@ #include "common/network/address_impl.h" #include "common/router/config_impl.h" +#include "server/config/http/buffer.h" + #include "test/mocks/runtime/mocks.h" #include "test/mocks/upstream/mocks.h" #include "test/test_common/environment.h" #include "test/test_common/printers.h" +#include "test/test_common/registry.h" #include "test/test_common/utility.h" #include "gmock/gmock.h" @@ -4204,6 +4210,163 @@ name: foo config.route(headers, 0)->routeEntry()->clusterName()); } } + +TEST(PerFilterConfigsTest, PerFilterConfigs) { + Server::Configuration::BufferFilterConfig factory; + Registry::InjectFactory register_buffer( + factory); + + std::string yaml = R"EOF( +max_request_bytes: 123 +max_request_time: + seconds: 456 + nanos: 789 +)EOF"; + + Protobuf::Struct buffer_cfg; + MessageUtil::loadFromYaml(yaml, buffer_cfg); + Protobuf::Map proto_cfgs; + proto_cfgs[factory.name()] = buffer_cfg; + PerFilterConfigs configs{proto_cfgs}; + + ASSERT_EQ(nullptr, configs.get("unknown.filter")) + << "filter configs that aren't present should return nullptr"; + + const Protobuf::Message* processed = configs.get(factory.name()); + ASSERT_NE(nullptr, processed) << "filter configs present should return a concrete Message"; + + ASSERT_NO_THROW({ + const envoy::config::filter::http::buffer::v2::Buffer* buf = + dynamic_cast(processed); + + ASSERT_EQ(123, buf->max_request_bytes().value()); + ASSERT_EQ(456, buf->max_request_time().seconds()); + ASSERT_EQ(789, buf->max_request_time().nanos()); + }) << "returned message should be castable to known type"; +} + +TEST(PerFilterConfigsTest, UnknownFilter) { + std::string yaml = R"EOF( +foo: "bar" +)EOF"; + + Protobuf::Struct some_cfg; + MessageUtil::loadFromYaml(yaml, some_cfg); + Protobuf::Map proto_cfgs; + proto_cfgs["unknown.filter"] = some_cfg; + + ASSERT_THROW(PerFilterConfigs{proto_cfgs}, EnvoyException) + << "should throw on unrecognized filter"; +} + +void checkPerFilterConfig(const Protobuf::Message* cfg, uint32_t expected_max_bytes, + std::string source) { + ASSERT_NE(nullptr, cfg) << "config should not be null for source: " << source; + ASSERT_NO_THROW({ + const auto buffer = dynamic_cast(cfg); + ASSERT_EQ(expected_max_bytes, buffer->max_request_bytes().value()) + << "config value does not match expected for source: " << source; + }) << "config should properly dynamic_cast to the appropriate type for source: " + << source; +} + +TEST(PerFilterConfigsTest, RouteLocalConfig) { + Server::Configuration::BufferFilterConfig factory; + Registry::InjectFactory register_buffer( + factory); + + std::string yaml = R"EOF( +name: foo +virtual_hosts: + - name: bar + domains: ["*"] + routes: + - match: { prefix: "/" } + route: { cluster: baz } + per_filter_config: { envoy.buffer: { max_request_bytes: 123 } } + per_filter_config: { envoy.buffer: { max_request_bytes: 456 } } +)EOF"; + + NiceMock runtime; + NiceMock cm; + const ConfigImpl config(parseRouteConfigurationFromV2Yaml(yaml), runtime, cm, true); + + const auto route = config.route(genHeaders("www.foo.com", "/", "GET"), 0); + const auto* route_entry = route->routeEntry(); + const auto& vhost = route_entry->virtualHost(); + + checkPerFilterConfig(route_entry->perFilterConfig(factory.name()), 123, "route entry"); + checkPerFilterConfig(route->perFilterConfig(factory.name()), 123, "route"); + checkPerFilterConfig(vhost.perFilterConfig(factory.name()), 456, "virtual host"); +} + +TEST(PerFilterConfigTest, WeightedClusterConfig) { + Server::Configuration::BufferFilterConfig factory; + Registry::InjectFactory register_buffer( + factory); + + std::string yaml = R"EOF( +name: foo +virtual_hosts: + - name: bar + domains: ["*"] + routes: + - match: { prefix: "/" } + route: + weighted_clusters: + clusters: + - name: baz + weight: 100 + per_filter_config: { envoy.buffer: { max_request_bytes: 789 } } + per_filter_config: { envoy.buffer: { max_request_bytes: 1011 } } +)EOF"; + + NiceMock runtime; + NiceMock cm; + const ConfigImpl config(parseRouteConfigurationFromV2Yaml(yaml), runtime, cm, true); + + const auto route = config.route(genHeaders("www.foo.com", "/", "GET"), 0); + const auto* route_entry = route->routeEntry(); + const auto& vhost = route_entry->virtualHost(); + + checkPerFilterConfig(route_entry->perFilterConfig(factory.name()), 789, "route entry"); + checkPerFilterConfig(route->perFilterConfig(factory.name()), 789, "route"); + checkPerFilterConfig(vhost.perFilterConfig(factory.name()), 1011, "virtual host"); +} + +TEST(PerFilterConfigTest, WeightedClusterFallthroughConfig) { + Server::Configuration::BufferFilterConfig factory; + Registry::InjectFactory register_buffer( + factory); + + std::string yaml = R"EOF( +name: foo +virtual_hosts: + - name: bar + domains: ["*"] + routes: + - match: { prefix: "/" } + route: + weighted_clusters: + clusters: + - name: baz + weight: 100 + per_filter_config: { envoy.buffer: { max_request_bytes: 1213 } } + per_filter_config: { envoy.buffer: { max_request_bytes: 1415 } } +)EOF"; + + NiceMock runtime; + NiceMock cm; + const ConfigImpl config(parseRouteConfigurationFromV2Yaml(yaml), runtime, cm, true); + + const auto route = config.route(genHeaders("www.foo.com", "/", "GET"), 0); + const auto* route_entry = route->routeEntry(); + const auto& vhost = route_entry->virtualHost(); + + checkPerFilterConfig(route_entry->perFilterConfig(factory.name()), 1213, "route entry"); + checkPerFilterConfig(route->perFilterConfig(factory.name()), 1213, "route"); + checkPerFilterConfig(vhost.perFilterConfig(factory.name()), 1415, "virtual host"); +} } // namespace } // namespace Router } // namespace Envoy diff --git a/test/mocks/router/mocks.h b/test/mocks/router/mocks.h index 4dd6b2a1e453..8d3992f5f1c4 100644 --- a/test/mocks/router/mocks.h +++ b/test/mocks/router/mocks.h @@ -163,6 +163,7 @@ class MockVirtualHost : public VirtualHost { MOCK_CONST_METHOD0(rateLimitPolicy, const RateLimitPolicy&()); MOCK_CONST_METHOD0(corsPolicy, const CorsPolicy*()); MOCK_CONST_METHOD0(routeConfig, const Config&()); + MOCK_CONST_METHOD1(perFilterConfig, const Protobuf::Message*(const std::string&)); std::string name_{"fake_vhost"}; testing::NiceMock rate_limit_policy_; @@ -232,6 +233,7 @@ class MockRouteEntry : public RouteEntry { MOCK_CONST_METHOD0(corsPolicy, const CorsPolicy*()); MOCK_CONST_METHOD0(metadata, const envoy::api::v2::core::Metadata&()); MOCK_CONST_METHOD0(pathMatchCriterion, const PathMatchCriterion&()); + MOCK_CONST_METHOD1(perFilterConfig, const Protobuf::Message*(const std::string&)); std::string cluster_name_{"fake_cluster"}; std::multimap opaque_config_; @@ -268,6 +270,7 @@ class MockRoute : public Route { MOCK_CONST_METHOD0(directResponseEntry, const DirectResponseEntry*()); MOCK_CONST_METHOD0(routeEntry, const RouteEntry*()); MOCK_CONST_METHOD0(decorator, const Decorator*()); + MOCK_CONST_METHOD1(perFilterConfig, const Protobuf::Message*(const std::string&)); testing::NiceMock route_entry_; testing::NiceMock decorator_; From b2d026331654b92f63c26b1da29a5f7721061348 Mon Sep 17 00:00:00 2001 From: Chris Roche Date: Tue, 10 Apr 2018 12:24:01 -0700 Subject: [PATCH 02/33] format Signed-off-by: Chris Roche --- source/common/router/config_impl.cc | 4 ++-- source/common/router/config_impl.h | 2 +- test/common/router/config_impl_test.cc | 8 ++++---- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index d471b4769130..ee3f8a4efbec 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -876,10 +876,10 @@ ConfigImpl::ConfigImpl(const envoy::api::v2::RouteConfiguration& config, Runtime config.response_headers_to_remove()); } -PerFilterConfigs::PerFilterConfigs(const Protobuf::Map& configs) { +PerFilterConfigs::PerFilterConfigs(const Protobuf::Map& configs) { for (const auto& cfg : configs) { const std::string& name = cfg.first; - const Protobuf::Struct& struct_config = cfg.second; + const ProtobufWkt::Struct& struct_config = cfg.second; auto& factory = Envoy::Config::Utility::getAndCheckFactory< Server::Configuration::NamedHttpFilterConfigFactory>(name); diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h index 4b974ac874cd..25fc83b4ee2a 100644 --- a/source/common/router/config_impl.h +++ b/source/common/router/config_impl.h @@ -46,7 +46,7 @@ class Matchable { class PerFilterConfigs { public: - PerFilterConfigs(const Protobuf::Map& configs); + PerFilterConfigs(const Protobuf::Map& configs); const Protobuf::Message* get(const std::string& name) const; diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index a79c4e66a996..ca40d6944827 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -4223,9 +4223,9 @@ max_request_bytes: 123 nanos: 789 )EOF"; - Protobuf::Struct buffer_cfg; + ProtobufWkt::Struct buffer_cfg; MessageUtil::loadFromYaml(yaml, buffer_cfg); - Protobuf::Map proto_cfgs; + Protobuf::Map proto_cfgs; proto_cfgs[factory.name()] = buffer_cfg; PerFilterConfigs configs{proto_cfgs}; @@ -4250,9 +4250,9 @@ TEST(PerFilterConfigsTest, UnknownFilter) { foo: "bar" )EOF"; - Protobuf::Struct some_cfg; + ProtobufWkt::Struct some_cfg; MessageUtil::loadFromYaml(yaml, some_cfg); - Protobuf::Map proto_cfgs; + Protobuf::Map proto_cfgs; proto_cfgs["unknown.filter"] = some_cfg; ASSERT_THROW(PerFilterConfigs{proto_cfgs}, EnvoyException) From 134734b69f735297bf3b8bc521fe10e3f2de84d9 Mon Sep 17 00:00:00 2001 From: Chris Roche Date: Tue, 10 Apr 2018 13:04:50 -0700 Subject: [PATCH 03/33] missed async client types Signed-off-by: Chris Roche --- source/common/http/async_client_impl.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/source/common/http/async_client_impl.h b/source/common/http/async_client_impl.h index 66a976d39bed..d5771506e297 100644 --- a/source/common/http/async_client_impl.h +++ b/source/common/http/async_client_impl.h @@ -148,6 +148,7 @@ class AsyncStreamImpl : public AsyncClient::Stream, const Router::RateLimitPolicy& rateLimitPolicy() const override { return rate_limit_policy_; } const Router::CorsPolicy* corsPolicy() const override { return nullptr; } const Router::Config& routeConfig() const override { return route_configuration_; } + const Protobuf::Message* perFilterConfig(const std::string&) const override { return nullptr; } static const NullRateLimitPolicy rate_limit_policy_; static const NullConfig route_configuration_; @@ -202,6 +203,8 @@ class AsyncStreamImpl : public AsyncClient::Stream, return path_match_criterion_; } + const Protobuf::Message* perFilterConfig(const std::string&) const override { return nullptr; } + static const NullRateLimitPolicy rate_limit_policy_; static const NullRetryPolicy retry_policy_; static const NullShadowPolicy shadow_policy_; @@ -223,6 +226,7 @@ class AsyncStreamImpl : public AsyncClient::Stream, const Router::DirectResponseEntry* directResponseEntry() const override { return nullptr; } const Router::RouteEntry* routeEntry() const override { return &route_entry_; } const Router::Decorator* decorator() const override { return nullptr; } + const Protobuf::Message* perFilterConfig(const std::string&) const override { return nullptr; } RouteEntryImpl route_entry_; }; From 0cc834f308697f1fe6d06f95db7cee22bba3f8c4 Mon Sep 17 00:00:00 2001 From: Chris Roche Date: Wed, 11 Apr 2018 15:09:09 -0700 Subject: [PATCH 04/33] fix repo locations Signed-off-by: Chris Roche --- bazel/repository_locations.bzl | 40 +++++++++++++++++----------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index 3320cc0b8ad3..5bb724f671d2 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -4,6 +4,10 @@ REPOSITORY_LOCATIONS = dict( commit = "9df0c47bc034d60d73d216cd0e090707b3fbea58", # chromium-65.0.3325.146 remote = "https://github.com/google/boringssl", ), + com_google_absl = dict( + commit = "787891a3882795cee0364e8a0f0dda315578d155", + remote = "https://github.com/abseil/abseil-cpp", + ), com_github_bombela_backward = dict( commit = "44ae9609e860e3428cd057f7052e505b4819eb84", # 2018-02-06 remote = "https://github.com/bombela/backward-cpp", @@ -31,11 +35,23 @@ REPOSITORY_LOCATIONS = dict( remote = "https://github.com/gcovr/gcovr", ), com_github_grpc_grpc = dict( - commit = "474c5950686e3962bd339c93d27e369bf64f568f", # v1.10.0 + commit = "66b9770a8ad326c1ee0dbedc5a8f32a52a604567", # v1.10.1 remote = "https://github.com/grpc/grpc.git", ), + io_opentracing_cpp = dict( + commit = "f6be24043e00baa2a25e0c1bb8793433d44ecc8b", + remote = "https://github.com/opentracing/opentracing-cpp", + ), + com_lightstep_tracer_cpp = dict( + commit = "6a198acd328f976984699f7272bbec7c8b220f65", + remote = "https://github.com/lightstep/lightstep-tracer-cpp", # v0.6.1 + ), + lightstep_vendored_googleapis = dict( + commit = "d6f78d948c53f3b400bb46996eb3084359914f9b", + remote = "https://github.com/google/googleapis", + ), com_github_nodejs_http_parser = dict( - commit = "dd74753cf5cf8944438d6f49ddf46f9659993dfb", # v2.8.0 + commit = "54f55a2f02a823e5f5c87abe853bb76d1170718d", # v2.8.1 remote = "https://github.com/nodejs/http-parser", ), com_github_pallets_jinja = dict( @@ -50,10 +66,6 @@ REPOSITORY_LOCATIONS = dict( commit = "f54b0e47a08782a6131cc3d60f94d038fa6e0a51", # v1.1.0 remote = "https://github.com/tencent/rapidjson", ), - com_google_absl = dict( - commit = "787891a3882795cee0364e8a0f0dda315578d155", - remote = "https://github.com/abseil/abseil-cpp", - ), com_google_googletest = dict( commit = "43863938377a9ea1399c0596269e0890b5c5515a", remote = "https://github.com/google/googletest", @@ -63,12 +75,8 @@ REPOSITORY_LOCATIONS = dict( strip_prefix = "protobuf-3.5.0", urls = ["https://github.com/google/protobuf/archive/v3.5.0.tar.gz"], ), - com_lightstep_tracer_cpp = dict( - commit = "6a198acd328f976984699f7272bbec7c8b220f65", - remote = "https://github.com/lightstep/lightstep-tracer-cpp", # v0.6.1 - ), envoy_api = dict( - commit = "c15104fce713856203fa4ad948d752e9a1f657c1", + commit = "c40deb34c7a68506dc629c2c786ee155fd8bab1e", remote = "https://github.com/envoyproxy/data-plane-api", ), grpc_httpjson_transcoding = dict( @@ -76,17 +84,9 @@ REPOSITORY_LOCATIONS = dict( remote = "https://github.com/grpc-ecosystem/grpc-httpjson-transcoding", ), io_bazel_rules_go = dict( - commit = "0.10.1", + commit = "0.10.3", remote = "https://github.com/bazelbuild/rules_go", ), - io_opentracing_cpp = dict( - commit = "f6be24043e00baa2a25e0c1bb8793433d44ecc8b", - remote = "https://github.com/opentracing/opentracing-cpp", - ), - lightstep_vendored_googleapis = dict( - commit = "d6f78d948c53f3b400bb46996eb3084359914f9b", - remote = "https://github.com/google/googleapis", - ), # I'd love to name this `com_github_google_subpar`, but something in the Subpar # code assumes its repository name is just `subpar`. subpar = dict( From 35f37f74315a0bab9439aa664339e67411898082 Mon Sep 17 00:00:00 2001 From: Chris Roche Date: Wed, 11 Apr 2018 15:13:03 -0700 Subject: [PATCH 05/33] s/RDS/Route/ Signed-off-by: Chris Roche --- include/envoy/server/filter_config.h | 2 +- source/common/config/utility.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/include/envoy/server/filter_config.h b/include/envoy/server/filter_config.h index afdbeb27ab52..c842e8c05648 100644 --- a/include/envoy/server/filter_config.h +++ b/include/envoy/server/filter_config.h @@ -302,7 +302,7 @@ class NamedHttpFilterConfigFactory { * empty proto. By default, this method returns the same value as createEmptyConfigProto, * and can be optionally overridden in implementations. */ - virtual ProtobufTypes::MessagePtr createEmptyRDSConfigProto() { return createEmptyConfigProto(); } + virtual ProtobufTypes::MessagePtr createEmptyRouteConfigProto() { return createEmptyConfigProto(); } /** * @return std::string the identifying name for a particular implementation of an http filter diff --git a/source/common/config/utility.h b/source/common/config/utility.h index 654085d92795..31314d241a23 100644 --- a/source/common/config/utility.h +++ b/source/common/config/utility.h @@ -219,7 +219,7 @@ class Utility { template static ProtobufTypes::MessagePtr translateToFactoryRDSConfig(const ProtoMessage& source, Factory& factory) { - ProtobufTypes::MessagePtr config = factory.createEmptyRDSConfigProto(); + ProtobufTypes::MessagePtr config = factory.createEmptyRouteConfigProto(); if (config == nullptr) { throw EnvoyException(fmt::format( From 4820c486eeb6dcd0954653d4f272923d7309fdcb Mon Sep 17 00:00:00 2001 From: Chris Roche Date: Wed, 11 Apr 2018 15:21:57 -0700 Subject: [PATCH 06/33] remove unneccessary template var Signed-off-by: Chris Roche --- source/common/config/utility.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/config/utility.h b/source/common/config/utility.h index 31314d241a23..7ec00dac1b6f 100644 --- a/source/common/config/utility.h +++ b/source/common/config/utility.h @@ -216,8 +216,8 @@ class Utility { return config; } - template - static ProtobufTypes::MessagePtr translateToFactoryRDSConfig(const ProtoMessage& source, + template + static ProtobufTypes::MessagePtr translateToFactoryRDSConfig(const Protobuf::Message& source, Factory& factory) { ProtobufTypes::MessagePtr config = factory.createEmptyRouteConfigProto(); From d76ad4d83490b0b2a50d033072a8508528026946 Mon Sep 17 00:00:00 2001 From: Chris Roche Date: Wed, 11 Apr 2018 15:23:20 -0700 Subject: [PATCH 07/33] the nullptr likes to be called just nullptr Signed-off-by: Chris Roche --- include/envoy/router/router.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/include/envoy/router/router.h b/include/envoy/router/router.h index 0ccc7eaa0c92..aedb8fdd7c87 100644 --- a/include/envoy/router/router.h +++ b/include/envoy/router/router.h @@ -261,7 +261,7 @@ class VirtualHost { /** * @return const Protobuf::Message* the per-filter config for the given - * filter name configured for this virtual host. If none is present, the + * filter name configured for this virtual host. If none is present, * nullptr is returned. */ virtual const Protobuf::Message* perFilterConfig(const std::string& name) const PURE; @@ -474,7 +474,7 @@ class RouteEntry : public ResponseEntry { /** * @return const Protobuf::Message* the per-filter config for the given * filter name configured for this route entry. Only weighted cluster entries - * will potentially have these values available. If none is present, the + * will potentially have these values available. If none is present, * nullptr is returned. */ virtual const Protobuf::Message* perFilterConfig(const std::string& name) const PURE; @@ -526,7 +526,7 @@ class Route { /** * @return const Protobuf::Message* the per-filter config for the given - * filter name configured for this route. If none is present, the nullptr is + * filter name configured for this route. If none is present, nullptr is * returned. */ virtual const Protobuf::Message* perFilterConfig(const std::string& name) const PURE; From 6309d41960ab4362baa88c4e14af3b8985cb2c17 Mon Sep 17 00:00:00 2001 From: Chris Roche Date: Wed, 11 Apr 2018 17:07:19 -0700 Subject: [PATCH 08/33] checkpoint Signed-off-by: Chris Roche --- source/common/config/utility.h | 11 +++++------ source/common/router/config_impl.cc | 2 +- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/source/common/config/utility.h b/source/common/config/utility.h index 7ec00dac1b6f..e743300c82da 100644 --- a/source/common/config/utility.h +++ b/source/common/config/utility.h @@ -206,6 +206,7 @@ class Utility { static ProtobufTypes::MessagePtr translateToFactoryConfig(const ProtoMessage& enclosing_message, Factory& factory) { ProtobufTypes::MessagePtr config = factory.createEmptyConfigProto(); + // Fail in an obvious way if a plugin does not return a proto. RELEASE_ASSERT(config != nullptr); @@ -217,14 +218,12 @@ class Utility { } template - static ProtobufTypes::MessagePtr translateToFactoryRDSConfig(const Protobuf::Message& source, - Factory& factory) { + static ProtobufTypes::MessagePtr translateToFactoryRouteConfig(const Protobuf::Message& source, + Factory& factory) { ProtobufTypes::MessagePtr config = factory.createEmptyRouteConfigProto(); - if (config == nullptr) { - throw EnvoyException(fmt::format( - "{} factory returned nullptr instead of empty RDS config message.", factory.name())); - } + // Fail in an obvious way if a plugin does not return a proto. + RELEASE_ASSERT(config != nullptr); MessageUtil::jsonConvert(source, *config); return config; diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index ee3f8a4efbec..f4357da5bc30 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -884,7 +884,7 @@ PerFilterConfigs::PerFilterConfigs(const Protobuf::Map(name); - configs_[name] = Envoy::Config::Utility::translateToFactoryRDSConfig(struct_config, factory); + configs_[name] = Envoy::Config::Utility::translateToFactoryRouteConfig(struct_config, factory); } } From 98971382b9e49b0aa7904b9b3a5aa3d7aee176a7 Mon Sep 17 00:00:00 2001 From: Chris Roche Date: Wed, 11 Apr 2018 17:51:59 -0700 Subject: [PATCH 09/33] s/ASSERT/EXPECT/ Signed-off-by: Chris Roche --- source/common/config/utility.h | 10 ++++++++++ test/common/router/config_impl_test.cc | 20 ++++++++++---------- 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/source/common/config/utility.h b/source/common/config/utility.h index e743300c82da..e3b2ca893f11 100644 --- a/source/common/config/utility.h +++ b/source/common/config/utility.h @@ -217,6 +217,16 @@ class Utility { return config; } + /** + * Translate a nested config into a route-specific proto message provided by + * the implementation factory. + * @param source a message (typically Struct) that contains the config for + * the given factory's route-local configuration (as returned by + * createEmptyRouteConfigProto). + * @param factory implementation factory with the method + * 'createEmptyRouteConfigProto' to produce a proto to be filled with + * the translated configuration. + */ template static ProtobufTypes::MessagePtr translateToFactoryRouteConfig(const Protobuf::Message& source, Factory& factory) { diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index ca40d6944827..d0f3504ab2f2 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -4229,19 +4229,19 @@ max_request_bytes: 123 proto_cfgs[factory.name()] = buffer_cfg; PerFilterConfigs configs{proto_cfgs}; - ASSERT_EQ(nullptr, configs.get("unknown.filter")) + EXPECT_EQ(nullptr, configs.get("unknown.filter")) << "filter configs that aren't present should return nullptr"; const Protobuf::Message* processed = configs.get(factory.name()); - ASSERT_NE(nullptr, processed) << "filter configs present should return a concrete Message"; + EXPECT_NE(nullptr, processed) << "filter configs present should return a concrete Message"; - ASSERT_NO_THROW({ + EXPECT_NO_THROW({ const envoy::config::filter::http::buffer::v2::Buffer* buf = dynamic_cast(processed); - ASSERT_EQ(123, buf->max_request_bytes().value()); - ASSERT_EQ(456, buf->max_request_time().seconds()); - ASSERT_EQ(789, buf->max_request_time().nanos()); + EXPECT_EQ(123, buf->max_request_bytes().value()); + EXPECT_EQ(456, buf->max_request_time().seconds()); + EXPECT_EQ(789, buf->max_request_time().nanos()); }) << "returned message should be castable to known type"; } @@ -4255,16 +4255,16 @@ foo: "bar" Protobuf::Map proto_cfgs; proto_cfgs["unknown.filter"] = some_cfg; - ASSERT_THROW(PerFilterConfigs{proto_cfgs}, EnvoyException) + EXPECT_THROW(PerFilterConfigs{proto_cfgs}, EnvoyException) << "should throw on unrecognized filter"; } void checkPerFilterConfig(const Protobuf::Message* cfg, uint32_t expected_max_bytes, std::string source) { - ASSERT_NE(nullptr, cfg) << "config should not be null for source: " << source; - ASSERT_NO_THROW({ + EXPECT_NE(nullptr, cfg) << "config should not be null for source: " << source; + EXPECT_NO_THROW({ const auto buffer = dynamic_cast(cfg); - ASSERT_EQ(expected_max_bytes, buffer->max_request_bytes().value()) + EXPECT_EQ(expected_max_bytes, buffer->max_request_bytes().value()) << "config value does not match expected for source: " << source; }) << "config should properly dynamic_cast to the appropriate type for source: " << source; From 118c728627f41c864571756c3a0d5566ee22a83d Mon Sep 17 00:00:00 2001 From: Chris Roche Date: Wed, 11 Apr 2018 18:02:06 -0700 Subject: [PATCH 10/33] dependency shuffle Signed-off-by: Chris Roche --- source/common/router/BUILD | 2 +- source/common/router/config_impl.cc | 2 +- test/common/router/BUILD | 2 +- test/common/router/config_impl_test.cc | 10 +++++----- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/source/common/router/BUILD b/source/common/router/BUILD index 649fec5b4b13..0e3baddd773d 100644 --- a/source/common/router/BUILD +++ b/source/common/router/BUILD @@ -22,7 +22,7 @@ envoy_cc_library( "//include/envoy/http:header_map_interface", "//include/envoy/router:router_interface", "//include/envoy/runtime:runtime_interface", - "//include/envoy/server:filter_config_interface", + "//include/envoy/server:filter_config_interface", # TODO(rodaine): break dependency on server "//include/envoy/upstream:cluster_manager_interface", "//include/envoy/upstream:upstream_interface", "//source/common/common:assert_lib", diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index f4357da5bc30..1c7aaea29aa8 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -11,7 +11,7 @@ #include "envoy/http/header_map.h" #include "envoy/runtime/runtime.h" -#include "envoy/server/filter_config.h" +#include "envoy/server/filter_config.h" // TODO(rodaine): break dependency on server #include "envoy/upstream/cluster_manager.h" #include "envoy/upstream/upstream.h" diff --git a/test/common/router/BUILD b/test/common/router/BUILD index 842f9787e5a5..b938f3c6c1d4 100644 --- a/test/common/router/BUILD +++ b/test/common/router/BUILD @@ -18,7 +18,7 @@ envoy_cc_test( "//source/common/http:headers_lib", "//source/common/json:json_loader_lib", "//source/common/router:config_lib", - "//source/server/config/http:buffer_lib", + "//source/extensions/filters/http/buffer:config", "//test/mocks/runtime:runtime_mocks", "//test/mocks/upstream:upstream_mocks", "//test/test_common:environment_lib", diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index d0f3504ab2f2..9c8ac96f2883 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -16,7 +16,7 @@ #include "common/network/address_impl.h" #include "common/router/config_impl.h" -#include "server/config/http/buffer.h" +#include "extensions/filters/http/buffer/config.h" #include "test/mocks/runtime/mocks.h" #include "test/mocks/upstream/mocks.h" @@ -4212,7 +4212,7 @@ name: foo } TEST(PerFilterConfigsTest, PerFilterConfigs) { - Server::Configuration::BufferFilterConfig factory; + Extensions::HttpFilters::BufferFilter::BufferFilterConfigFactory factory; Registry::InjectFactory register_buffer( factory); @@ -4271,7 +4271,7 @@ void checkPerFilterConfig(const Protobuf::Message* cfg, uint32_t expected_max_by } TEST(PerFilterConfigsTest, RouteLocalConfig) { - Server::Configuration::BufferFilterConfig factory; + Extensions::HttpFilters::BufferFilter::BufferFilterConfigFactory factory; Registry::InjectFactory register_buffer( factory); @@ -4301,7 +4301,7 @@ name: foo } TEST(PerFilterConfigTest, WeightedClusterConfig) { - Server::Configuration::BufferFilterConfig factory; + Extensions::HttpFilters::BufferFilter::BufferFilterConfigFactory factory; Registry::InjectFactory register_buffer( factory); @@ -4335,7 +4335,7 @@ name: foo } TEST(PerFilterConfigTest, WeightedClusterFallthroughConfig) { - Server::Configuration::BufferFilterConfig factory; + Extensions::HttpFilters::BufferFilter::BufferFilterConfigFactory factory; Registry::InjectFactory register_buffer( factory); From 711f59fcab6aefe35f25fc86c63d03822fc6d072 Mon Sep 17 00:00:00 2001 From: Chris Roche Date: Thu, 12 Apr 2018 14:22:24 -0700 Subject: [PATCH 11/33] format + convert test to use fixtures with dummy filter Signed-off-by: Chris Roche --- include/envoy/server/filter_config.h | 4 +- test/common/router/BUILD | 2 +- test/common/router/config_impl_test.cc | 156 +++++++++++-------------- 3 files changed, 74 insertions(+), 88 deletions(-) diff --git a/include/envoy/server/filter_config.h b/include/envoy/server/filter_config.h index c842e8c05648..ae27fc0aa67f 100644 --- a/include/envoy/server/filter_config.h +++ b/include/envoy/server/filter_config.h @@ -302,7 +302,9 @@ class NamedHttpFilterConfigFactory { * empty proto. By default, this method returns the same value as createEmptyConfigProto, * and can be optionally overridden in implementations. */ - virtual ProtobufTypes::MessagePtr createEmptyRouteConfigProto() { return createEmptyConfigProto(); } + virtual ProtobufTypes::MessagePtr createEmptyRouteConfigProto() { + return createEmptyConfigProto(); + } /** * @return std::string the identifying name for a particular implementation of an http filter diff --git a/test/common/router/BUILD b/test/common/router/BUILD index b938f3c6c1d4..3ceb8a55188d 100644 --- a/test/common/router/BUILD +++ b/test/common/router/BUILD @@ -18,7 +18,7 @@ envoy_cc_test( "//source/common/http:headers_lib", "//source/common/json:json_loader_lib", "//source/common/router:config_lib", - "//source/extensions/filters/http/buffer:config", + "//source/extensions/filters/http/common:empty_http_filter_config_lib", "//test/mocks/runtime:runtime_mocks", "//test/mocks/upstream:upstream_mocks", "//test/test_common:environment_lib", diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index 9c8ac96f2883..d6eab60c1783 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -4,7 +4,6 @@ #include #include -#include "envoy/config/filter/http/buffer/v2/buffer.pb.h" #include "envoy/server/filter_config.h" #include "common/config/metadata.h" @@ -16,7 +15,7 @@ #include "common/network/address_impl.h" #include "common/router/config_impl.h" -#include "extensions/filters/http/buffer/config.h" +#include "extensions/filters/http/common/empty_http_filter_config.h" #include "test/mocks/runtime/mocks.h" #include "test/mocks/upstream/mocks.h" @@ -4211,41 +4210,79 @@ name: foo } } -TEST(PerFilterConfigsTest, PerFilterConfigs) { - Extensions::HttpFilters::BufferFilter::BufferFilterConfigFactory factory; - Registry::InjectFactory register_buffer( - factory); +class PerFilterConfigsTest : public testing::Test, + public Extensions::HttpFilters::Common::EmptyHttpFilterConfig { +public: + void SetUp() override { + registered_factory_.reset( + new Registry::InjectFactory(*this)); + } + + // EmptyHttpFilterConfig + Server::Configuration::HttpFilterFactoryCb + createFilter(const std::string&, Server::Configuration::FactoryContext&) override { + NOT_IMPLEMENTED; + } + ProtobufTypes::MessagePtr createEmptyRouteConfigProto() override { + return ProtobufTypes::MessagePtr{new ProtobufWkt::Timestamp()}; + } + std::string name() override { return "test.filter"; } + + void checkEach(const std::string& yaml, uint32_t expected_entry, uint32_t expected_route, + uint32_t expected_vhost) { + const ConfigImpl config(parseRouteConfigurationFromV2Yaml(yaml), runtime_, cm_, true); + + const auto route = config.route(genHeaders("www.foo.com", "/", "GET"), 0); + const auto* route_entry = route->routeEntry(); + const auto& vhost = route_entry->virtualHost(); + + check(route_entry->perFilterConfig(name()), expected_entry, "route entry"); + check(route->perFilterConfig(name()), expected_route, "route"); + check(vhost.perFilterConfig(name()), expected_vhost, "virtual host"); + } + + void check(const Protobuf::Message* cfg, uint32_t expected_seconds, std::string source) { + EXPECT_NE(nullptr, cfg) << "config should not be null for source: " << source; + EXPECT_NO_THROW({ + const auto ts = dynamic_cast(cfg); + EXPECT_EQ(expected_seconds, ts->seconds()) + << "config value does not match expected for source: " << source; + }) << "config should properly dynamic_cast to the appropriate type for source: " + << source; + } + + std::unique_ptr> + registered_factory_; + NiceMock runtime_; + NiceMock cm_; +}; +TEST_F(PerFilterConfigsTest, PerFilterConfigs) { std::string yaml = R"EOF( -max_request_bytes: 123 -max_request_time: - seconds: 456 - nanos: 789 +seconds: 123 +nanos: 456 )EOF"; - ProtobufWkt::Struct buffer_cfg; - MessageUtil::loadFromYaml(yaml, buffer_cfg); + ProtobufWkt::Struct struct_cfg; + MessageUtil::loadFromYaml(yaml, struct_cfg); Protobuf::Map proto_cfgs; - proto_cfgs[factory.name()] = buffer_cfg; + proto_cfgs[name()] = struct_cfg; PerFilterConfigs configs{proto_cfgs}; EXPECT_EQ(nullptr, configs.get("unknown.filter")) << "filter configs that aren't present should return nullptr"; - const Protobuf::Message* processed = configs.get(factory.name()); + const Protobuf::Message* processed = configs.get(name()); EXPECT_NE(nullptr, processed) << "filter configs present should return a concrete Message"; EXPECT_NO_THROW({ - const envoy::config::filter::http::buffer::v2::Buffer* buf = - dynamic_cast(processed); - - EXPECT_EQ(123, buf->max_request_bytes().value()); - EXPECT_EQ(456, buf->max_request_time().seconds()); - EXPECT_EQ(789, buf->max_request_time().nanos()); + const ProtobufWkt::Timestamp* cfg = dynamic_cast(processed); + EXPECT_EQ(123, cfg->seconds()); + EXPECT_EQ(456, cfg->nanos()); }) << "returned message should be castable to known type"; } -TEST(PerFilterConfigsTest, UnknownFilter) { +TEST_F(PerFilterConfigsTest, UnknownFilter) { std::string yaml = R"EOF( foo: "bar" )EOF"; @@ -4259,22 +4296,7 @@ foo: "bar" << "should throw on unrecognized filter"; } -void checkPerFilterConfig(const Protobuf::Message* cfg, uint32_t expected_max_bytes, - std::string source) { - EXPECT_NE(nullptr, cfg) << "config should not be null for source: " << source; - EXPECT_NO_THROW({ - const auto buffer = dynamic_cast(cfg); - EXPECT_EQ(expected_max_bytes, buffer->max_request_bytes().value()) - << "config value does not match expected for source: " << source; - }) << "config should properly dynamic_cast to the appropriate type for source: " - << source; -} - -TEST(PerFilterConfigsTest, RouteLocalConfig) { - Extensions::HttpFilters::BufferFilter::BufferFilterConfigFactory factory; - Registry::InjectFactory register_buffer( - factory); - +TEST_F(PerFilterConfigsTest, RouteLocalConfig) { std::string yaml = R"EOF( name: foo virtual_hosts: @@ -4283,28 +4305,14 @@ name: foo routes: - match: { prefix: "/" } route: { cluster: baz } - per_filter_config: { envoy.buffer: { max_request_bytes: 123 } } - per_filter_config: { envoy.buffer: { max_request_bytes: 456 } } + per_filter_config: { test.filter: { seconds: 123 } } + per_filter_config: { test.filter: { seconds: 456 } } )EOF"; - NiceMock runtime; - NiceMock cm; - const ConfigImpl config(parseRouteConfigurationFromV2Yaml(yaml), runtime, cm, true); - - const auto route = config.route(genHeaders("www.foo.com", "/", "GET"), 0); - const auto* route_entry = route->routeEntry(); - const auto& vhost = route_entry->virtualHost(); - - checkPerFilterConfig(route_entry->perFilterConfig(factory.name()), 123, "route entry"); - checkPerFilterConfig(route->perFilterConfig(factory.name()), 123, "route"); - checkPerFilterConfig(vhost.perFilterConfig(factory.name()), 456, "virtual host"); + checkEach(yaml, 123, 123, 456); } -TEST(PerFilterConfigTest, WeightedClusterConfig) { - Extensions::HttpFilters::BufferFilter::BufferFilterConfigFactory factory; - Registry::InjectFactory register_buffer( - factory); - +TEST_F(PerFilterConfigsTest, WeightedClusterConfig) { std::string yaml = R"EOF( name: foo virtual_hosts: @@ -4317,28 +4325,14 @@ name: foo clusters: - name: baz weight: 100 - per_filter_config: { envoy.buffer: { max_request_bytes: 789 } } - per_filter_config: { envoy.buffer: { max_request_bytes: 1011 } } + per_filter_config: { test.filter: { seconds: 789 } } + per_filter_config: { test.filter: { seconds: 1011 } } )EOF"; - NiceMock runtime; - NiceMock cm; - const ConfigImpl config(parseRouteConfigurationFromV2Yaml(yaml), runtime, cm, true); - - const auto route = config.route(genHeaders("www.foo.com", "/", "GET"), 0); - const auto* route_entry = route->routeEntry(); - const auto& vhost = route_entry->virtualHost(); - - checkPerFilterConfig(route_entry->perFilterConfig(factory.name()), 789, "route entry"); - checkPerFilterConfig(route->perFilterConfig(factory.name()), 789, "route"); - checkPerFilterConfig(vhost.perFilterConfig(factory.name()), 1011, "virtual host"); + checkEach(yaml, 789, 789, 1011); } -TEST(PerFilterConfigTest, WeightedClusterFallthroughConfig) { - Extensions::HttpFilters::BufferFilter::BufferFilterConfigFactory factory; - Registry::InjectFactory register_buffer( - factory); - +TEST_F(PerFilterConfigsTest, WeightedClusterFallthroughConfig) { std::string yaml = R"EOF( name: foo virtual_hosts: @@ -4351,21 +4345,11 @@ name: foo clusters: - name: baz weight: 100 - per_filter_config: { envoy.buffer: { max_request_bytes: 1213 } } - per_filter_config: { envoy.buffer: { max_request_bytes: 1415 } } + per_filter_config: { test.filter: { seconds: 1213 } } + per_filter_config: { test.filter: { seconds: 1415 } } )EOF"; - NiceMock runtime; - NiceMock cm; - const ConfigImpl config(parseRouteConfigurationFromV2Yaml(yaml), runtime, cm, true); - - const auto route = config.route(genHeaders("www.foo.com", "/", "GET"), 0); - const auto* route_entry = route->routeEntry(); - const auto& vhost = route_entry->virtualHost(); - - checkPerFilterConfig(route_entry->perFilterConfig(factory.name()), 1213, "route entry"); - checkPerFilterConfig(route->perFilterConfig(factory.name()), 1213, "route"); - checkPerFilterConfig(vhost.perFilterConfig(factory.name()), 1415, "virtual host"); + checkEach(yaml, 1213, 1213, 1415); } } // namespace } // namespace Router From aad91ea99b5914802a0aebf62a3e56e59bce89f1 Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Mon, 16 Apr 2018 11:35:58 -0400 Subject: [PATCH 12/33] compilation fixes post repo reorg Signed-off-by: Shriram Rajagopalan --- source/extensions/filters/http/fault/BUILD | 1 + .../filters/http/fault/fault_filter.cc | 22 +++++- test/extensions/filters/http/fault/BUILD | 1 + .../filters/http/fault/config_test.cc | 15 ++-- .../filters/http/fault/fault_filter_test.cc | 74 ++++++++++++++++++- 5 files changed, 98 insertions(+), 15 deletions(-) diff --git a/source/extensions/filters/http/fault/BUILD b/source/extensions/filters/http/fault/BUILD index effc8c246335..8f6956c329a4 100644 --- a/source/extensions/filters/http/fault/BUILD +++ b/source/extensions/filters/http/fault/BUILD @@ -24,6 +24,7 @@ envoy_cc_library( "//include/envoy/stats:stats_macros", "//source/common/common:assert_lib", "//source/common/common:empty_string", + "//source/common/config:well_known_names", "//source/common/http:codes_lib", "//source/common/http:header_map_lib", "//source/common/http:headers_lib", diff --git a/source/extensions/filters/http/fault/fault_filter.cc b/source/extensions/filters/http/fault/fault_filter.cc index af1a17b2db9e..82c2d9f2d234 100644 --- a/source/extensions/filters/http/fault/fault_filter.cc +++ b/source/extensions/filters/http/fault/fault_filter.cc @@ -13,6 +13,7 @@ #include "common/common/assert.h" #include "common/common/empty_string.h" #include "common/common/fmt.h" +#include "common/config/well_known_names.h" #include "common/http/codes.h" #include "common/http/header_map_impl.h" #include "common/http/headers.h" @@ -36,10 +37,6 @@ FaultFilterConfig::FaultFilterConfig(const envoy::config::filter::http::fault::v : runtime_(runtime), stats_(generateStats(stats_prefix, scope)), stats_prefix_(stats_prefix), scope_(scope) { - if (!fault.has_abort() && !fault.has_delay()) { - throw EnvoyException("fault filter must have at least abort or delay specified in the config."); - } - if (fault.has_abort()) { abort_percent_ = fault.abort().percent(); http_status_ = fault.abort().http_status(); @@ -71,6 +68,23 @@ FaultFilter::~FaultFilter() { ASSERT(!delay_timer_); } // if we inject a delay, then we will inject the abort in the delay timer // callback. Http::FilterHeadersStatus FaultFilter::decodeHeaders(Http::HeaderMap& headers, bool) { + // Route-level configuration overrides filter-level configuration + // TODO (rshriram): We should not be using Runtimes when reading from route-level + // faults until we parameterize the runtime keys for faults based on the route name. + // Its okay for the moment, since the only use case of faults with runtimes is + // by folks using filter level configuration in the fault filter (mainly Lyft folks). + if (callbacks_->route() && callbacks_->route()->routeEntry()) { + const auto metadata = callbacks_->route()->routeEntry()->metadata(); + const auto filter_it = + metadata.filter_metadata().find(Envoy::Config::HttpFilterNames::get().FAULT); + if (filter_it != metadata.filter_metadata().end()) { + envoy::config::filter::http::fault::v2::HTTPFault proto_config; + MessageUtil::jsonConvert(filter_it->second, proto_config); + config_.reset(new FaultFilterConfig(proto_config, config_->runtime(), config_->statsPrefix(), + config_->scope())); + } + } + if (!matchesTargetUpstreamCluster()) { return Http::FilterHeadersStatus::Continue; } diff --git a/test/extensions/filters/http/fault/BUILD b/test/extensions/filters/http/fault/BUILD index 432c8ffada28..0024f0b534de 100644 --- a/test/extensions/filters/http/fault/BUILD +++ b/test/extensions/filters/http/fault/BUILD @@ -20,6 +20,7 @@ envoy_extension_cc_test( "//source/common/buffer:buffer_lib", "//source/common/common:empty_string", "//source/common/config:filter_json_lib", + "//source/common/config:well_known_names", "//source/common/http:header_map_lib", "//source/common/http:headers_lib", "//source/common/stats:stats_lib", diff --git a/test/extensions/filters/http/fault/config_test.cc b/test/extensions/filters/http/fault/config_test.cc index ebe7df44d114..18bd50babb3e 100644 --- a/test/extensions/filters/http/fault/config_test.cc +++ b/test/extensions/filters/http/fault/config_test.cc @@ -56,19 +56,14 @@ TEST(FaultFilterConfigTest, FaultFilterCorrectProto) { cb(filter_callback); } -TEST(FaultFilterConfigTest, InvalidFaultFilterInProto) { - envoy::config::filter::http::fault::v2::HTTPFault config{}; - NiceMock context; - FaultFilterFactory factory; - EXPECT_THROW(factory.createFilterFactoryFromProto(config, "stats", context), EnvoyException); -} - TEST(FaultFilterConfigTest, FaultFilterEmptyProto) { NiceMock context; FaultFilterFactory factory; - EXPECT_THROW( - factory.createFilterFactoryFromProto(*factory.createEmptyConfigProto(), "stats", context), - EnvoyException); + HttpFilterFactoryCb cb = + factory.createFilterFactoryFromProto(*factory.createEmptyConfigProto(), "stats", context); + Http::MockFilterChainFactoryCallbacks filter_callback; + EXPECT_CALL(filter_callback, addStreamDecoderFilter(_)); + cb(filter_callback); } } // namespace Fault diff --git a/test/extensions/filters/http/fault/fault_filter_test.cc b/test/extensions/filters/http/fault/fault_filter_test.cc index 7cb17ab49ef1..99434f67ee45 100644 --- a/test/extensions/filters/http/fault/fault_filter_test.cc +++ b/test/extensions/filters/http/fault/fault_filter_test.cc @@ -8,6 +8,7 @@ #include "common/buffer/buffer_impl.h" #include "common/common/empty_string.h" #include "common/config/filter_json.h" +#include "common/config/well_known_names.h" #include "common/http/header_map_impl.h" #include "common/http/headers.h" #include "common/stats/stats_impl.h" @@ -115,6 +116,22 @@ class FaultFilterTest : public testing::Test { } )EOF"; + const std::string v2_fault_in_route_metadata_json = R"EOF( + { + "delay" : { + "type" : "FIXED", + "percent" : 100, + "fixedDelay" : "5s" + }, + "upstreamCluster" : "www1" + } + )EOF"; + + const std::string empty_listener_level_fault_json = R"EOF( + { + } + )EOF"; + void SetUpTest(const std::string json) { Json::ObjectSharedPtr config = Json::Factory::loadFromString(json); envoy::config::filter::http::fault::v2::HTTPFault fault; @@ -131,6 +148,8 @@ class FaultFilterTest : public testing::Test { EXPECT_CALL(*timer_, disableTimer()); } + void TestRouteLevelFault(); // utility function + FaultFilterConfigSharedPtr config_; std::unique_ptr filter_; NiceMock filter_callbacks_; @@ -719,7 +738,7 @@ TEST_F(FaultFilterTest, FaultWithTargetClusterNullRoute) { SetUpTest(fault_with_target_cluster_json); const std::string upstream_cluster("www1"); - EXPECT_CALL(*filter_callbacks_.route_, routeEntry()).WillOnce(Return(nullptr)); + EXPECT_CALL(*filter_callbacks_.route_, routeEntry()).WillRepeatedly(Return(nullptr)); EXPECT_CALL(runtime_.snapshot_, featureEnabled("fault.http.delay.fixed_delay_percent", _)) .Times(0); EXPECT_CALL(runtime_.snapshot_, getInteger("fault.http.delay.fixed_duration_ms", _)).Times(0); @@ -737,6 +756,59 @@ TEST_F(FaultFilterTest, FaultWithTargetClusterNullRoute) { EXPECT_EQ(0UL, config_->stats().aborts_injected_.value()); } +void FaultFilterTest::TestRouteLevelFault() { + Envoy::ProtobufWkt::Struct route_level_fault; + MessageUtil::loadFromJson(v2_fault_in_route_metadata_json, route_level_fault); + envoy::api::v2::core::Metadata metadata; + (*metadata.mutable_filter_metadata())[Envoy::Config::HttpFilterNames::get().FAULT] = + route_level_fault; + + EXPECT_CALL(filter_callbacks_.route_->route_entry_, metadata()).WillOnce(ReturnRef(metadata)); + + const std::string upstream_cluster("www1"); + + EXPECT_CALL(filter_callbacks_.route_->route_entry_, clusterName()) + .WillOnce(ReturnRef(upstream_cluster)); + + // Delay related calls + EXPECT_CALL(runtime_.snapshot_, featureEnabled("fault.http.delay.fixed_delay_percent", 100)) + .WillOnce(Return(true)); + + EXPECT_CALL(runtime_.snapshot_, getInteger("fault.http.delay.fixed_duration_ms", 5000)) + .WillOnce(Return(5000UL)); + + SCOPED_TRACE("RouteLevelFault"); + expectDelayTimer(5000UL); + + EXPECT_CALL(filter_callbacks_.request_info_, + setResponseFlag(RequestInfo::ResponseFlag::DelayInjected)); + EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, + filter_->decodeHeaders(request_headers_, false)); + + // Abort related calls + EXPECT_CALL(runtime_.snapshot_, featureEnabled("fault.http.abort.abort_percent", 0)) + .WillOnce(Return(false)); + + EXPECT_CALL(filter_callbacks_, continueDecoding()); + timer_->callback_(); + + EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(data_, false)); + EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->decodeTrailers(request_headers_)); + + EXPECT_EQ(1UL, config_->stats().delays_injected_.value()); + EXPECT_EQ(0UL, config_->stats().aborts_injected_.value()); +} + +TEST_F(FaultFilterTest, OnlyRouteLevelFault) { + SetUpTest(empty_listener_level_fault_json); + TestRouteLevelFault(); +} + +TEST_F(FaultFilterTest, RouteLevelFaultOverridesListenerLevelFault) { + SetUpTest(abort_only_json); // This is a valid listener level fault + TestRouteLevelFault(); +} + } // namespace Fault } // namespace HttpFilters } // namespace Extensions From 99039ab6605e58f71454aafc2f4d56028cf8f77a Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Mon, 16 Apr 2018 13:09:03 -0400 Subject: [PATCH 13/33] converting Fault Filter to perfilterconfig Signed-off-by: Shriram Rajagopalan --- .../filters/http/fault/fault_filter.cc | 22 +++-- .../filters/http/fault/fault_filter_test.cc | 85 +++++++++++-------- 2 files changed, 65 insertions(+), 42 deletions(-) diff --git a/source/extensions/filters/http/fault/fault_filter.cc b/source/extensions/filters/http/fault/fault_filter.cc index 82c2d9f2d234..137593e172b8 100644 --- a/source/extensions/filters/http/fault/fault_filter.cc +++ b/source/extensions/filters/http/fault/fault_filter.cc @@ -74,14 +74,20 @@ Http::FilterHeadersStatus FaultFilter::decodeHeaders(Http::HeaderMap& headers, b // Its okay for the moment, since the only use case of faults with runtimes is // by folks using filter level configuration in the fault filter (mainly Lyft folks). if (callbacks_->route() && callbacks_->route()->routeEntry()) { - const auto metadata = callbacks_->route()->routeEntry()->metadata(); - const auto filter_it = - metadata.filter_metadata().find(Envoy::Config::HttpFilterNames::get().FAULT); - if (filter_it != metadata.filter_metadata().end()) { - envoy::config::filter::http::fault::v2::HTTPFault proto_config; - MessageUtil::jsonConvert(filter_it->second, proto_config); - config_.reset(new FaultFilterConfig(proto_config, config_->runtime(), config_->statsPrefix(), - config_->scope())); + auto proto_config = callbacks_->route()->routeEntry()->perFilterConfig( + Envoy::Config::HttpFilterNames::get().FAULT); + if (!proto_config) { + // See if the virtual host has any fault filter config + proto_config = callbacks_->route()->routeEntry()->virtualHost().perFilterConfig( + Envoy::Config::HttpFilterNames::get().FAULT); + } + + if (proto_config) { + const envoy::config::filter::http::fault::v2::HTTPFault per_filter_config = + dynamic_cast(*proto_config); + + config_.reset(new FaultFilterConfig(per_filter_config, config_->runtime(), + config_->statsPrefix(), config_->scope())); } } diff --git a/test/extensions/filters/http/fault/fault_filter_test.cc b/test/extensions/filters/http/fault/fault_filter_test.cc index 99434f67ee45..d8f063da4042 100644 --- a/test/extensions/filters/http/fault/fault_filter_test.cc +++ b/test/extensions/filters/http/fault/fault_filter_test.cc @@ -105,7 +105,7 @@ class FaultFilterTest : public testing::Test { } )EOF"; - const std::string fault_with_target_cluster_json = R"EOF( + const std::string delay_with_upstream_cluster_json = R"EOF( { "delay" : { "type" : "fixed", @@ -116,27 +116,21 @@ class FaultFilterTest : public testing::Test { } )EOF"; - const std::string v2_fault_in_route_metadata_json = R"EOF( + const std::string v2_empty_fault_config_json = R"EOF( { - "delay" : { - "type" : "FIXED", - "percent" : 100, - "fixedDelay" : "5s" - }, - "upstreamCluster" : "www1" } )EOF"; - const std::string empty_listener_level_fault_json = R"EOF( - { - } - )EOF"; - - void SetUpTest(const std::string json) { + envoy::config::filter::http::fault::v2::HTTPFault + convertJsonStrToProtoConfig(const std::string json) { Json::ObjectSharedPtr config = Json::Factory::loadFromString(json); envoy::config::filter::http::fault::v2::HTTPFault fault; - Config::FilterJson::translateFaultFilter(*config, fault); + return fault; + } + + void SetUpTest(const std::string json) { + envoy::config::filter::http::fault::v2::HTTPFault fault = convertJsonStrToProtoConfig(json); config_.reset(new FaultFilterConfig(fault, runtime_, "prefix.", stats_)); filter_.reset(new FaultFilter(config_)); filter_->setDecoderFilterCallbacks(filter_callbacks_); @@ -148,7 +142,8 @@ class FaultFilterTest : public testing::Test { EXPECT_CALL(*timer_, disableTimer()); } - void TestRouteLevelFault(); // utility function + void TestPerFilterConfigFault(envoy::config::filter::http::fault::v2::HTTPFault* route_fault, + envoy::config::filter::http::fault::v2::HTTPFault* vhost_fault); FaultFilterConfigSharedPtr config_; std::unique_ptr filter_; @@ -670,7 +665,7 @@ TEST_F(FaultFilterTest, TimerResetAfterStreamReset) { } TEST_F(FaultFilterTest, FaultWithTargetClusterMatchSuccess) { - SetUpTest(fault_with_target_cluster_json); + SetUpTest(delay_with_upstream_cluster_json); const std::string upstream_cluster("www1"); EXPECT_CALL(filter_callbacks_.route_->route_entry_, clusterName()) @@ -712,7 +707,7 @@ TEST_F(FaultFilterTest, FaultWithTargetClusterMatchSuccess) { } TEST_F(FaultFilterTest, FaultWithTargetClusterMatchFail) { - SetUpTest(fault_with_target_cluster_json); + SetUpTest(delay_with_upstream_cluster_json); const std::string upstream_cluster("mismatch"); EXPECT_CALL(filter_callbacks_.route_->route_entry_, clusterName()) @@ -735,7 +730,7 @@ TEST_F(FaultFilterTest, FaultWithTargetClusterMatchFail) { } TEST_F(FaultFilterTest, FaultWithTargetClusterNullRoute) { - SetUpTest(fault_with_target_cluster_json); + SetUpTest(delay_with_upstream_cluster_json); const std::string upstream_cluster("www1"); EXPECT_CALL(*filter_callbacks_.route_, routeEntry()).WillRepeatedly(Return(nullptr)); @@ -756,14 +751,16 @@ TEST_F(FaultFilterTest, FaultWithTargetClusterNullRoute) { EXPECT_EQ(0UL, config_->stats().aborts_injected_.value()); } -void FaultFilterTest::TestRouteLevelFault() { - Envoy::ProtobufWkt::Struct route_level_fault; - MessageUtil::loadFromJson(v2_fault_in_route_metadata_json, route_level_fault); - envoy::api::v2::core::Metadata metadata; - (*metadata.mutable_filter_metadata())[Envoy::Config::HttpFilterNames::get().FAULT] = - route_level_fault; +void FaultFilterTest::TestPerFilterConfigFault( + envoy::config::filter::http::fault::v2::HTTPFault* route_fault, + envoy::config::filter::http::fault::v2::HTTPFault* vhost_fault) { - EXPECT_CALL(filter_callbacks_.route_->route_entry_, metadata()).WillOnce(ReturnRef(metadata)); + ON_CALL(filter_callbacks_.route_->route_entry_, + perFilterConfig(Envoy::Config::HttpFilterNames::get().FAULT)) + .WillByDefault(Return(route_fault)); + ON_CALL(filter_callbacks_.route_->route_entry_.virtual_host_, + perFilterConfig(Envoy::Config::HttpFilterNames::get().FAULT)) + .WillByDefault(Return(vhost_fault)); const std::string upstream_cluster("www1"); @@ -777,7 +774,7 @@ void FaultFilterTest::TestRouteLevelFault() { EXPECT_CALL(runtime_.snapshot_, getInteger("fault.http.delay.fixed_duration_ms", 5000)) .WillOnce(Return(5000UL)); - SCOPED_TRACE("RouteLevelFault"); + SCOPED_TRACE("PerFilterConfigFault"); expectDelayTimer(5000UL); EXPECT_CALL(filter_callbacks_.request_info_, @@ -799,14 +796,34 @@ void FaultFilterTest::TestRouteLevelFault() { EXPECT_EQ(0UL, config_->stats().aborts_injected_.value()); } -TEST_F(FaultFilterTest, OnlyRouteLevelFault) { - SetUpTest(empty_listener_level_fault_json); - TestRouteLevelFault(); -} +TEST_F(FaultFilterTest, RouteFaultOverridesListenerFault) { -TEST_F(FaultFilterTest, RouteLevelFaultOverridesListenerLevelFault) { - SetUpTest(abort_only_json); // This is a valid listener level fault - TestRouteLevelFault(); + envoy::config::filter::http::fault::v2::HTTPFault abort_fault = + convertJsonStrToProtoConfig(abort_only_json); + envoy::config::filter::http::fault::v2::HTTPFault delay_fault = + convertJsonStrToProtoConfig(delay_with_upstream_cluster_json); + + // route-level fault overrides listener-level fault + { + SetUpTest(v2_empty_fault_config_json); // This is a valid listener level fault + TestPerFilterConfigFault(&delay_fault, nullptr); + } + + // virtual-host-level fault overrides listener-level fault + { + config_->stats().aborts_injected_.reset(); + config_->stats().delays_injected_.reset(); + SetUpTest(v2_empty_fault_config_json); + TestPerFilterConfigFault(nullptr, &delay_fault); + } + + // route-level fault overrides virtual-host-level fault + { + config_->stats().aborts_injected_.reset(); + config_->stats().delays_injected_.reset(); + SetUpTest(v2_empty_fault_config_json); + TestPerFilterConfigFault(&delay_fault, &abort_fault); + } } } // namespace Fault From c2f915bbb8accdcad26b2ae51767c271f707aa8e Mon Sep 17 00:00:00 2001 From: Chris Roche Date: Mon, 16 Apr 2018 11:34:39 -0700 Subject: [PATCH 14/33] comment nits Signed-off-by: Chris Roche --- source/common/router/config_impl.cc | 7 +-- test/common/router/config_impl_test.cc | 82 +++++++++----------------- 2 files changed, 30 insertions(+), 59 deletions(-) diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index 68f4f3f9ba63..da0bc7a54cb9 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -892,12 +892,7 @@ PerFilterConfigs::PerFilterConfigs(const Protobuf::Mapsecond.get(); + return cfg == configs_.end() ? nullptr : cfg->second.get(); } } // namespace Router diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index d6eab60c1783..73002e0f546f 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -4210,23 +4210,21 @@ name: foo } } -class PerFilterConfigsTest : public testing::Test, - public Extensions::HttpFilters::Common::EmptyHttpFilterConfig { +class PerFilterConfigsTest : public testing::Test { public: - void SetUp() override { - registered_factory_.reset( - new Registry::InjectFactory(*this)); - } + PerFilterConfigsTest() : factory_(), registered_factory_(factory_) {} - // EmptyHttpFilterConfig - Server::Configuration::HttpFilterFactoryCb - createFilter(const std::string&, Server::Configuration::FactoryContext&) override { - NOT_IMPLEMENTED; - } - ProtobufTypes::MessagePtr createEmptyRouteConfigProto() override { - return ProtobufTypes::MessagePtr{new ProtobufWkt::Timestamp()}; - } - std::string name() override { return "test.filter"; } + class TestFilterConfig : public Extensions::HttpFilters::Common::EmptyHttpFilterConfig { + public: + Server::Configuration::HttpFilterFactoryCb + createFilter(const std::string&, Server::Configuration::FactoryContext&) override { + NOT_IMPLEMENTED; + } + ProtobufTypes::MessagePtr createEmptyRouteConfigProto() override { + return ProtobufTypes::MessagePtr{new ProtobufWkt::Timestamp()}; + } + std::string name() override { return "test.filter"; } + }; void checkEach(const std::string& yaml, uint32_t expected_entry, uint32_t expected_route, uint32_t expected_vhost) { @@ -4236,9 +4234,9 @@ class PerFilterConfigsTest : public testing::Test, const auto* route_entry = route->routeEntry(); const auto& vhost = route_entry->virtualHost(); - check(route_entry->perFilterConfig(name()), expected_entry, "route entry"); - check(route->perFilterConfig(name()), expected_route, "route"); - check(vhost.perFilterConfig(name()), expected_vhost, "virtual host"); + check(route_entry->perFilterConfig(factory_.name()), expected_entry, "route entry"); + check(route->perFilterConfig(factory_.name()), expected_route, "route"); + check(vhost.perFilterConfig(factory_.name()), expected_vhost, "virtual host"); } void check(const Protobuf::Message* cfg, uint32_t expected_seconds, std::string source) { @@ -4251,49 +4249,27 @@ class PerFilterConfigsTest : public testing::Test, << source; } - std::unique_ptr> - registered_factory_; + TestFilterConfig factory_; + Registry::InjectFactory registered_factory_; NiceMock runtime_; NiceMock cm_; }; -TEST_F(PerFilterConfigsTest, PerFilterConfigs) { - std::string yaml = R"EOF( -seconds: 123 -nanos: 456 -)EOF"; - - ProtobufWkt::Struct struct_cfg; - MessageUtil::loadFromYaml(yaml, struct_cfg); - Protobuf::Map proto_cfgs; - proto_cfgs[name()] = struct_cfg; - PerFilterConfigs configs{proto_cfgs}; - - EXPECT_EQ(nullptr, configs.get("unknown.filter")) - << "filter configs that aren't present should return nullptr"; - - const Protobuf::Message* processed = configs.get(name()); - EXPECT_NE(nullptr, processed) << "filter configs present should return a concrete Message"; - - EXPECT_NO_THROW({ - const ProtobufWkt::Timestamp* cfg = dynamic_cast(processed); - EXPECT_EQ(123, cfg->seconds()); - EXPECT_EQ(456, cfg->nanos()); - }) << "returned message should be castable to known type"; -} - TEST_F(PerFilterConfigsTest, UnknownFilter) { std::string yaml = R"EOF( -foo: "bar" +name: foo +virtual_hosts: + - name: bar + domains: ["*"] + routes: + - match: { prefix: "/" } + route: { cluster: baz } + per_filter_config: { unknown.filter: {} } )EOF"; - ProtobufWkt::Struct some_cfg; - MessageUtil::loadFromYaml(yaml, some_cfg); - Protobuf::Map proto_cfgs; - proto_cfgs["unknown.filter"] = some_cfg; - - EXPECT_THROW(PerFilterConfigs{proto_cfgs}, EnvoyException) - << "should throw on unrecognized filter"; + EXPECT_THROW_WITH_MESSAGE( + ConfigImpl(parseRouteConfigurationFromV2Yaml(yaml), runtime_, cm_, true), EnvoyException, + "Didn't find a registered implementation for name: 'unknown.filter'"); } TEST_F(PerFilterConfigsTest, RouteLocalConfig) { From 298e22626cda4fd7bb6cc880b5065fdd18330269 Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Mon, 16 Apr 2018 17:15:48 -0400 Subject: [PATCH 15/33] well known names Signed-off-by: Shriram Rajagopalan --- source/extensions/filters/http/fault/fault_filter.cc | 4 ++-- test/extensions/filters/http/fault/fault_filter_test.cc | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/source/extensions/filters/http/fault/fault_filter.cc b/source/extensions/filters/http/fault/fault_filter.cc index 137593e172b8..7e63fcb5a443 100644 --- a/source/extensions/filters/http/fault/fault_filter.cc +++ b/source/extensions/filters/http/fault/fault_filter.cc @@ -75,11 +75,11 @@ Http::FilterHeadersStatus FaultFilter::decodeHeaders(Http::HeaderMap& headers, b // by folks using filter level configuration in the fault filter (mainly Lyft folks). if (callbacks_->route() && callbacks_->route()->routeEntry()) { auto proto_config = callbacks_->route()->routeEntry()->perFilterConfig( - Envoy::Config::HttpFilterNames::get().FAULT); + Extensions::HttpFilters::HttpFilterNames::get().FAULT); if (!proto_config) { // See if the virtual host has any fault filter config proto_config = callbacks_->route()->routeEntry()->virtualHost().perFilterConfig( - Envoy::Config::HttpFilterNames::get().FAULT); + Extensions::HttpFilters::HttpFilterNames::get().FAULT); } if (proto_config) { diff --git a/test/extensions/filters/http/fault/fault_filter_test.cc b/test/extensions/filters/http/fault/fault_filter_test.cc index d8f063da4042..fe47b865f3ac 100644 --- a/test/extensions/filters/http/fault/fault_filter_test.cc +++ b/test/extensions/filters/http/fault/fault_filter_test.cc @@ -756,10 +756,10 @@ void FaultFilterTest::TestPerFilterConfigFault( envoy::config::filter::http::fault::v2::HTTPFault* vhost_fault) { ON_CALL(filter_callbacks_.route_->route_entry_, - perFilterConfig(Envoy::Config::HttpFilterNames::get().FAULT)) + perFilterConfig(Extensions::HttpFilters::HttpFilterNames::get().ROUTER)) .WillByDefault(Return(route_fault)); ON_CALL(filter_callbacks_.route_->route_entry_.virtual_host_, - perFilterConfig(Envoy::Config::HttpFilterNames::get().FAULT)) + perFilterConfig(Extensions::HttpFilters::HttpFilterNames::get().ROUTER)) .WillByDefault(Return(vhost_fault)); const std::string upstream_cluster("www1"); From 73f2f66fc225fc19cc46ce73e76f9f8761b6077c Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Mon, 16 Apr 2018 18:13:02 -0400 Subject: [PATCH 16/33] remove unnecessary includes Signed-off-by: Shriram Rajagopalan --- source/extensions/filters/http/fault/BUILD | 1 - source/extensions/filters/http/fault/fault_filter.cc | 1 - test/extensions/filters/http/fault/BUILD | 1 - test/extensions/filters/http/fault/fault_filter_test.cc | 5 ++--- 4 files changed, 2 insertions(+), 6 deletions(-) diff --git a/source/extensions/filters/http/fault/BUILD b/source/extensions/filters/http/fault/BUILD index 1b7031ae4c5f..f0207cbd3a99 100644 --- a/source/extensions/filters/http/fault/BUILD +++ b/source/extensions/filters/http/fault/BUILD @@ -24,7 +24,6 @@ envoy_cc_library( "//include/envoy/stats:stats_macros", "//source/common/common:assert_lib", "//source/common/common:empty_string", - "//source/common/config:well_known_names", "//source/common/http:codes_lib", "//source/common/http:header_map_lib", "//source/common/http:headers_lib", diff --git a/source/extensions/filters/http/fault/fault_filter.cc b/source/extensions/filters/http/fault/fault_filter.cc index 7e63fcb5a443..62d2257f9bd2 100644 --- a/source/extensions/filters/http/fault/fault_filter.cc +++ b/source/extensions/filters/http/fault/fault_filter.cc @@ -13,7 +13,6 @@ #include "common/common/assert.h" #include "common/common/empty_string.h" #include "common/common/fmt.h" -#include "common/config/well_known_names.h" #include "common/http/codes.h" #include "common/http/header_map_impl.h" #include "common/http/headers.h" diff --git a/test/extensions/filters/http/fault/BUILD b/test/extensions/filters/http/fault/BUILD index 0024f0b534de..432c8ffada28 100644 --- a/test/extensions/filters/http/fault/BUILD +++ b/test/extensions/filters/http/fault/BUILD @@ -20,7 +20,6 @@ envoy_extension_cc_test( "//source/common/buffer:buffer_lib", "//source/common/common:empty_string", "//source/common/config:filter_json_lib", - "//source/common/config:well_known_names", "//source/common/http:header_map_lib", "//source/common/http:headers_lib", "//source/common/stats:stats_lib", diff --git a/test/extensions/filters/http/fault/fault_filter_test.cc b/test/extensions/filters/http/fault/fault_filter_test.cc index fe47b865f3ac..e4329e58029b 100644 --- a/test/extensions/filters/http/fault/fault_filter_test.cc +++ b/test/extensions/filters/http/fault/fault_filter_test.cc @@ -8,7 +8,6 @@ #include "common/buffer/buffer_impl.h" #include "common/common/empty_string.h" #include "common/config/filter_json.h" -#include "common/config/well_known_names.h" #include "common/http/header_map_impl.h" #include "common/http/headers.h" #include "common/stats/stats_impl.h" @@ -756,10 +755,10 @@ void FaultFilterTest::TestPerFilterConfigFault( envoy::config::filter::http::fault::v2::HTTPFault* vhost_fault) { ON_CALL(filter_callbacks_.route_->route_entry_, - perFilterConfig(Extensions::HttpFilters::HttpFilterNames::get().ROUTER)) + perFilterConfig(Extensions::HttpFilters::HttpFilterNames::get().FAULT)) .WillByDefault(Return(route_fault)); ON_CALL(filter_callbacks_.route_->route_entry_.virtual_host_, - perFilterConfig(Extensions::HttpFilters::HttpFilterNames::get().ROUTER)) + perFilterConfig(Extensions::HttpFilters::HttpFilterNames::get().FAULT)) .WillByDefault(Return(vhost_fault)); const std::string upstream_cluster("www1"); From 295f82af2d5a9448e4e3838af8acd5b0250df6f4 Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Mon, 16 Apr 2018 22:48:48 -0400 Subject: [PATCH 17/33] missing includes Signed-off-by: Shriram Rajagopalan --- source/extensions/filters/http/fault/fault_filter.cc | 2 ++ test/extensions/filters/http/fault/fault_filter_test.cc | 1 + 2 files changed, 3 insertions(+) diff --git a/source/extensions/filters/http/fault/fault_filter.cc b/source/extensions/filters/http/fault/fault_filter.cc index 62d2257f9bd2..fdbe99a9fd69 100644 --- a/source/extensions/filters/http/fault/fault_filter.cc +++ b/source/extensions/filters/http/fault/fault_filter.cc @@ -20,6 +20,8 @@ #include "common/protobuf/utility.h" #include "common/router/config_impl.h" +#include "extensions/filters/http/well_known_names.h" + namespace Envoy { namespace Extensions { namespace HttpFilters { diff --git a/test/extensions/filters/http/fault/fault_filter_test.cc b/test/extensions/filters/http/fault/fault_filter_test.cc index e4329e58029b..40f3378df6a1 100644 --- a/test/extensions/filters/http/fault/fault_filter_test.cc +++ b/test/extensions/filters/http/fault/fault_filter_test.cc @@ -13,6 +13,7 @@ #include "common/stats/stats_impl.h" #include "extensions/filters/http/fault/fault_filter.h" +#include "extensions/filters/http/well_known_names.h" #include "test/common/http/common.h" #include "test/mocks/http/mocks.h" From 0d5fe62fbd46fd2f1fc862354ef4eea371ae83b4 Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Mon, 16 Apr 2018 22:54:07 -0400 Subject: [PATCH 18/33] format Signed-off-by: Shriram Rajagopalan --- source/extensions/filters/http/fault/fault_filter.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/extensions/filters/http/fault/fault_filter.cc b/source/extensions/filters/http/fault/fault_filter.cc index fdbe99a9fd69..855441adad48 100644 --- a/source/extensions/filters/http/fault/fault_filter.cc +++ b/source/extensions/filters/http/fault/fault_filter.cc @@ -80,7 +80,7 @@ Http::FilterHeadersStatus FaultFilter::decodeHeaders(Http::HeaderMap& headers, b if (!proto_config) { // See if the virtual host has any fault filter config proto_config = callbacks_->route()->routeEntry()->virtualHost().perFilterConfig( - Extensions::HttpFilters::HttpFilterNames::get().FAULT); + Extensions::HttpFilters::HttpFilterNames::get().FAULT); } if (proto_config) { From c0c6f3aa079bcd7fe399767fa72f256b66967a44 Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Tue, 17 Apr 2018 11:23:54 -0400 Subject: [PATCH 19/33] moar compile fixes Signed-off-by: Shriram Rajagopalan --- test/extensions/filters/http/fault/config_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/extensions/filters/http/fault/config_test.cc b/test/extensions/filters/http/fault/config_test.cc index 18bd50babb3e..3e1df4ca02bb 100644 --- a/test/extensions/filters/http/fault/config_test.cc +++ b/test/extensions/filters/http/fault/config_test.cc @@ -59,7 +59,7 @@ TEST(FaultFilterConfigTest, FaultFilterCorrectProto) { TEST(FaultFilterConfigTest, FaultFilterEmptyProto) { NiceMock context; FaultFilterFactory factory; - HttpFilterFactoryCb cb = + Server::Configuration::HttpFilterFactoryCb cb = factory.createFilterFactoryFromProto(*factory.createEmptyConfigProto(), "stats", context); Http::MockFilterChainFactoryCallbacks filter_callback; EXPECT_CALL(filter_callback, addStreamDecoderFilter(_)); From 80221f7b2957e726779ca785dd684a99748b90de Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Wed, 18 Apr 2018 12:30:14 -0400 Subject: [PATCH 20/33] ternary Signed-off-by: Shriram Rajagopalan --- source/extensions/filters/http/fault/fault_filter.cc | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/source/extensions/filters/http/fault/fault_filter.cc b/source/extensions/filters/http/fault/fault_filter.cc index 855441adad48..944458a84abe 100644 --- a/source/extensions/filters/http/fault/fault_filter.cc +++ b/source/extensions/filters/http/fault/fault_filter.cc @@ -75,13 +75,11 @@ Http::FilterHeadersStatus FaultFilter::decodeHeaders(Http::HeaderMap& headers, b // Its okay for the moment, since the only use case of faults with runtimes is // by folks using filter level configuration in the fault filter (mainly Lyft folks). if (callbacks_->route() && callbacks_->route()->routeEntry()) { - auto proto_config = callbacks_->route()->routeEntry()->perFilterConfig( - Extensions::HttpFilters::HttpFilterNames::get().FAULT); - if (!proto_config) { - // See if the virtual host has any fault filter config - proto_config = callbacks_->route()->routeEntry()->virtualHost().perFilterConfig( - Extensions::HttpFilters::HttpFilterNames::get().FAULT); - } + const std::string name = Extensions::HttpFilters::HttpFilterNames::get().FAULT; + const auto* route_entry = callbacks_->route()->routeEntry(); + + const auto* proto_config = route_entry->perFilterConfig(name) + ?: route_entry->virtualHost().perFilterConfig(name); if (proto_config) { const envoy::config::filter::http::fault::v2::HTTPFault per_filter_config = From 8870ac33ece426242ff57d53908ccb519c2f6cee Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Wed, 18 Apr 2018 13:50:58 -0400 Subject: [PATCH 21/33] format Signed-off-by: Shriram Rajagopalan --- source/extensions/filters/http/fault/fault_filter.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/extensions/filters/http/fault/fault_filter.cc b/source/extensions/filters/http/fault/fault_filter.cc index 944458a84abe..380a9dbae2b2 100644 --- a/source/extensions/filters/http/fault/fault_filter.cc +++ b/source/extensions/filters/http/fault/fault_filter.cc @@ -78,8 +78,8 @@ Http::FilterHeadersStatus FaultFilter::decodeHeaders(Http::HeaderMap& headers, b const std::string name = Extensions::HttpFilters::HttpFilterNames::get().FAULT; const auto* route_entry = callbacks_->route()->routeEntry(); - const auto* proto_config = route_entry->perFilterConfig(name) - ?: route_entry->virtualHost().perFilterConfig(name); + const auto* proto_config = + route_entry->perFilterConfig(name) ?: route_entry->virtualHost().perFilterConfig(name); if (proto_config) { const envoy::config::filter::http::fault::v2::HTTPFault per_filter_config = From 28356e83f113665e1d8b178fcef2fd9eb25e81e7 Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Wed, 18 Apr 2018 16:12:52 -0400 Subject: [PATCH 22/33] nits Signed-off-by: Shriram Rajagopalan --- source/extensions/filters/http/fault/fault_filter.cc | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/source/extensions/filters/http/fault/fault_filter.cc b/source/extensions/filters/http/fault/fault_filter.cc index 380a9dbae2b2..fe10634bde83 100644 --- a/source/extensions/filters/http/fault/fault_filter.cc +++ b/source/extensions/filters/http/fault/fault_filter.cc @@ -70,10 +70,9 @@ FaultFilter::~FaultFilter() { ASSERT(!delay_timer_); } // callback. Http::FilterHeadersStatus FaultFilter::decodeHeaders(Http::HeaderMap& headers, bool) { // Route-level configuration overrides filter-level configuration - // TODO (rshriram): We should not be using Runtimes when reading from route-level - // faults until we parameterize the runtime keys for faults based on the route name. - // Its okay for the moment, since the only use case of faults with runtimes is - // by folks using filter level configuration in the fault filter (mainly Lyft folks). + // NOTE: We should not use runtime when reading from route-level + // faults. In other words, runtime is supported only when faults are + // configured at the filter level. if (callbacks_->route() && callbacks_->route()->routeEntry()) { const std::string name = Extensions::HttpFilters::HttpFilterNames::get().FAULT; const auto* route_entry = callbacks_->route()->routeEntry(); @@ -85,6 +84,8 @@ Http::FilterHeadersStatus FaultFilter::decodeHeaders(Http::HeaderMap& headers, b const envoy::config::filter::http::fault::v2::HTTPFault per_filter_config = dynamic_cast(*proto_config); + // TODO (qiwzhang): Optimize this such that its updated only by the + // route update callback config_.reset(new FaultFilterConfig(per_filter_config, config_->runtime(), config_->statsPrefix(), config_->scope())); } From 739e572812192148e859869ebe38d5a29d620ba9 Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Thu, 19 Apr 2018 21:02:24 -0400 Subject: [PATCH 23/33] rework based on new interface Signed-off-by: Shriram Rajagopalan --- .../extensions/filters/http/fault/config.cc | 9 ++++ source/extensions/filters/http/fault/config.h | 3 ++ .../filters/http/fault/fault_filter.cc | 47 +++++++++++------- .../filters/http/fault/fault_filter.h | 48 +++++++++++++------ .../filters/http/fault/fault_filter_test.cc | 17 ++++--- 5 files changed, 84 insertions(+), 40 deletions(-) diff --git a/source/extensions/filters/http/fault/config.cc b/source/extensions/filters/http/fault/config.cc index 1ee045228db1..db976cf45b9b 100644 --- a/source/extensions/filters/http/fault/config.cc +++ b/source/extensions/filters/http/fault/config.cc @@ -42,6 +42,15 @@ FaultFilterFactory::createFilterFactoryFromProto(const Protobuf::Message& proto_ stats_prefix, context); } +Router::RouteSpecificFilterConfigConstSharedPtr +FaultFilterFactory::createRouteSpecificFilterConfig(const ProtobufWkt::Struct& struct_config) { + envoy::config::filter::http::fault::v2::HTTPFault proto_config; + MessageUtil::jsonConvert(struct_config, proto_config); + Router::RouteSpecificFilterConfigConstSharedPtr rconfig; + rconfig.reset(new Fault::PerRouteFaultFilterConfig(proto_config)); + return rconfig; +} + /** * Static registration for the fault filter. @see RegisterFactory. */ diff --git a/source/extensions/filters/http/fault/config.h b/source/extensions/filters/http/fault/config.h index 29253bacae19..bd5a93fdf618 100644 --- a/source/extensions/filters/http/fault/config.h +++ b/source/extensions/filters/http/fault/config.h @@ -27,6 +27,9 @@ class FaultFilterFactory : public Server::Configuration::NamedHttpFilterConfigFa return ProtobufTypes::MessagePtr{new envoy::config::filter::http::fault::v2::HTTPFault()}; } + Router::RouteSpecificFilterConfigConstSharedPtr + createRouteSpecificFilterConfig(const ProtobufWkt::Struct&) override; + std::string name() override { return HttpFilterNames::get().FAULT; } private: diff --git a/source/extensions/filters/http/fault/fault_filter.cc b/source/extensions/filters/http/fault/fault_filter.cc index fe10634bde83..b3a5d9ebe8ea 100644 --- a/source/extensions/filters/http/fault/fault_filter.cc +++ b/source/extensions/filters/http/fault/fault_filter.cc @@ -32,11 +32,12 @@ const std::string FaultFilter::ABORT_PERCENT_KEY = "fault.http.abort.abort_perce const std::string FaultFilter::DELAY_DURATION_KEY = "fault.http.delay.fixed_duration_ms"; const std::string FaultFilter::ABORT_HTTP_STATUS_KEY = "fault.http.abort.http_status"; -FaultFilterConfig::FaultFilterConfig(const envoy::config::filter::http::fault::v2::HTTPFault& fault, - Runtime::Loader& runtime, const std::string& stats_prefix, - Stats::Scope& scope) - : runtime_(runtime), stats_(generateStats(stats_prefix, scope)), stats_prefix_(stats_prefix), - scope_(scope) { +PerRouteFaultFilterConfig::PerRouteFaultFilterConfig( + const envoy::config::filter::http::fault::v2::HTTPFault& fault) { + + if (!fault.has_abort() && !fault.has_delay()) { + throw EnvoyException("fault filter must have at least abort or delay specified in the config."); + } if (fault.has_abort()) { abort_percent_ = fault.abort().percent(); @@ -60,6 +61,23 @@ FaultFilterConfig::FaultFilterConfig(const envoy::config::filter::http::fault::v } } +FaultFilterConfig::FaultFilterConfig(const envoy::config::filter::http::fault::v2::HTTPFault& fault, + Runtime::Loader& runtime, const std::string& stats_prefix, + Stats::Scope& scope) + : runtime_(runtime), stats_(generateStats(stats_prefix, scope)), stats_prefix_(stats_prefix), + scope_(scope) { + + // A small optimization to eliminate unnecessary processing in the + // fault filter when an empty config is provided + if (!fault.has_delay() && !fault.has_abort()) { + return; + } + + local_config_.reset(new PerRouteFaultFilterConfig(fault)); + // This feels a bit ugly, but we don't want to allocate anything here. + filter_config_ = local_config_.get(); +} + FaultFilter::FaultFilter(FaultFilterConfigSharedPtr config) : config_(config) {} FaultFilter::~FaultFilter() { ASSERT(!delay_timer_); } @@ -77,18 +95,15 @@ Http::FilterHeadersStatus FaultFilter::decodeHeaders(Http::HeaderMap& headers, b const std::string name = Extensions::HttpFilters::HttpFilterNames::get().FAULT; const auto* route_entry = callbacks_->route()->routeEntry(); - const auto* proto_config = - route_entry->perFilterConfig(name) ?: route_entry->virtualHost().perFilterConfig(name); - - if (proto_config) { - const envoy::config::filter::http::fault::v2::HTTPFault per_filter_config = - dynamic_cast(*proto_config); + const PerRouteFaultFilterConfig* per_filter_config = + route_entry->perFilterConfigTyped(name) + ?: route_entry->virtualHost().perFilterConfigTyped(name); + config_->updateFilterConfig(per_filter_config); + } - // TODO (qiwzhang): Optimize this such that its updated only by the - // route update callback - config_.reset(new FaultFilterConfig(per_filter_config, config_->runtime(), - config_->statsPrefix(), config_->scope())); - } + // ignore if filter config is empty + if (config_->emptyFilterConfig()) { + return Http::FilterHeadersStatus::Continue; } if (!matchesTargetUpstreamCluster()) { diff --git a/source/extensions/filters/http/fault/fault_filter.h b/source/extensions/filters/http/fault/fault_filter.h index cd64aff71b42..9f54d55838fe 100644 --- a/source/extensions/filters/http/fault/fault_filter.h +++ b/source/extensions/filters/http/fault/fault_filter.h @@ -35,6 +35,23 @@ struct FaultFilterStats { ALL_FAULT_FILTER_STATS(GENERATE_COUNTER_STRUCT) }; +/** + * Configuration for the fault filter that can be specified per route + */ +struct PerRouteFaultFilterConfig : public Router::RouteSpecificFilterConfig { + PerRouteFaultFilterConfig(const envoy::config::filter::http::fault::v2::HTTPFault& fault); + + uint64_t abort_percent_{}; // 0-100 + uint64_t http_status_{}; // HTTP or gRPC return codes + uint64_t fixed_delay_percent_{}; // 0-100 + uint64_t fixed_duration_ms_{}; // in milliseconds + std::string upstream_cluster_; // restrict faults to specific upstream cluster + std::vector fault_filter_headers_; + std::unordered_set downstream_nodes_{}; // Inject failures for specific downstream +}; + +typedef std::shared_ptr PerRouteFaultFilterConfigConstSharedPtr; + /** * Configuration for the fault filter. */ @@ -44,30 +61,31 @@ class FaultFilterConfig { Runtime::Loader& runtime, const std::string& stats_prefix, Stats::Scope& scope); const std::vector& filterHeaders() { - return fault_filter_headers_; + return filter_config_->fault_filter_headers_; } - uint64_t abortPercent() { return abort_percent_; } - uint64_t delayPercent() { return fixed_delay_percent_; } - uint64_t delayDuration() { return fixed_duration_ms_; } - uint64_t abortCode() { return http_status_; } - const std::string& upstreamCluster() { return upstream_cluster_; } + uint64_t abortPercent() { return filter_config_->abort_percent_; } + uint64_t delayPercent() { return filter_config_->fixed_delay_percent_; } + uint64_t delayDuration() { return filter_config_->fixed_duration_ms_; } + uint64_t abortCode() { return filter_config_->http_status_; } + const std::string& upstreamCluster() { return filter_config_->upstream_cluster_; } Runtime::Loader& runtime() { return runtime_; } FaultFilterStats& stats() { return stats_; } - const std::unordered_set& downstreamNodes() { return downstream_nodes_; } + const std::unordered_set& downstreamNodes() { + return filter_config_->downstream_nodes_; + } const std::string& statsPrefix() { return stats_prefix_; } Stats::Scope& scope() { return scope_; } + bool emptyFilterConfig() { return !!filter_config_; } + + void updateFilterConfig(const PerRouteFaultFilterConfig* config) { + filter_config_ = config != nullptr ? config : filter_config_; + } private: static FaultFilterStats generateStats(const std::string& prefix, Stats::Scope& scope); - uint64_t abort_percent_{}; // 0-100 - uint64_t http_status_{}; // HTTP or gRPC return codes - uint64_t fixed_delay_percent_{}; // 0-100 - uint64_t fixed_duration_ms_{}; // in milliseconds - std::string upstream_cluster_; // restrict faults to specific upstream cluster - std::vector fault_filter_headers_; - std::unordered_set downstream_nodes_{}; // Inject failures for specific downstream - // nodes. If not set then inject for all. + const PerRouteFaultFilterConfig* filter_config_; + PerRouteFaultFilterConfigConstSharedPtr local_config_; Runtime::Loader& runtime_; FaultFilterStats stats_; const std::string stats_prefix_; diff --git a/test/extensions/filters/http/fault/fault_filter_test.cc b/test/extensions/filters/http/fault/fault_filter_test.cc index 40f3378df6a1..150a4c568c18 100644 --- a/test/extensions/filters/http/fault/fault_filter_test.cc +++ b/test/extensions/filters/http/fault/fault_filter_test.cc @@ -24,13 +24,13 @@ #include "gmock/gmock.h" #include "gtest/gtest.h" +using testing::_; using testing::DoAll; using testing::Invoke; using testing::NiceMock; using testing::Return; using testing::ReturnRef; using testing::WithArgs; -using testing::_; namespace Envoy { namespace Extensions { @@ -142,8 +142,8 @@ class FaultFilterTest : public testing::Test { EXPECT_CALL(*timer_, disableTimer()); } - void TestPerFilterConfigFault(envoy::config::filter::http::fault::v2::HTTPFault* route_fault, - envoy::config::filter::http::fault::v2::HTTPFault* vhost_fault); + void TestPerFilterConfigFault(const Router::RouteSpecificFilterConfig* route_fault, + const Router::RouteSpecificFilterConfig* vhost_fault); FaultFilterConfigSharedPtr config_; std::unique_ptr filter_; @@ -752,8 +752,8 @@ TEST_F(FaultFilterTest, FaultWithTargetClusterNullRoute) { } void FaultFilterTest::TestPerFilterConfigFault( - envoy::config::filter::http::fault::v2::HTTPFault* route_fault, - envoy::config::filter::http::fault::v2::HTTPFault* vhost_fault) { + const Router::RouteSpecificFilterConfig* route_fault, + const Router::RouteSpecificFilterConfig* vhost_fault) { ON_CALL(filter_callbacks_.route_->route_entry_, perFilterConfig(Extensions::HttpFilters::HttpFilterNames::get().FAULT)) @@ -798,10 +798,9 @@ void FaultFilterTest::TestPerFilterConfigFault( TEST_F(FaultFilterTest, RouteFaultOverridesListenerFault) { - envoy::config::filter::http::fault::v2::HTTPFault abort_fault = - convertJsonStrToProtoConfig(abort_only_json); - envoy::config::filter::http::fault::v2::HTTPFault delay_fault = - convertJsonStrToProtoConfig(delay_with_upstream_cluster_json); + Fault::PerRouteFaultFilterConfig abort_fault(convertJsonStrToProtoConfig(abort_only_json)); + Fault::PerRouteFaultFilterConfig delay_fault( + convertJsonStrToProtoConfig(delay_with_upstream_cluster_json)); // route-level fault overrides listener-level fault { From 70f13d08cdf3d7a98eb972657c38a321e0aee1c2 Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Thu, 19 Apr 2018 21:22:38 -0400 Subject: [PATCH 24/33] format Signed-off-by: Shriram Rajagopalan --- test/extensions/filters/http/fault/fault_filter_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/extensions/filters/http/fault/fault_filter_test.cc b/test/extensions/filters/http/fault/fault_filter_test.cc index 150a4c568c18..9b1a2d3a18f0 100644 --- a/test/extensions/filters/http/fault/fault_filter_test.cc +++ b/test/extensions/filters/http/fault/fault_filter_test.cc @@ -24,13 +24,13 @@ #include "gmock/gmock.h" #include "gtest/gtest.h" -using testing::_; using testing::DoAll; using testing::Invoke; using testing::NiceMock; using testing::Return; using testing::ReturnRef; using testing::WithArgs; +using testing::_; namespace Envoy { namespace Extensions { From ce73907534edf46d96399d0f987d65d7c12a677f Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Thu, 19 Apr 2018 22:15:04 -0400 Subject: [PATCH 25/33] fixes Signed-off-by: Shriram Rajagopalan --- .../filters/http/fault/fault_filter.cc | 17 ++++------------- .../filters/http/fault/fault_filter.h | 4 ++-- 2 files changed, 6 insertions(+), 15 deletions(-) diff --git a/source/extensions/filters/http/fault/fault_filter.cc b/source/extensions/filters/http/fault/fault_filter.cc index b3a5d9ebe8ea..46a82da231b0 100644 --- a/source/extensions/filters/http/fault/fault_filter.cc +++ b/source/extensions/filters/http/fault/fault_filter.cc @@ -35,10 +35,6 @@ const std::string FaultFilter::ABORT_HTTP_STATUS_KEY = "fault.http.abort.http_st PerRouteFaultFilterConfig::PerRouteFaultFilterConfig( const envoy::config::filter::http::fault::v2::HTTPFault& fault) { - if (!fault.has_abort() && !fault.has_delay()) { - throw EnvoyException("fault filter must have at least abort or delay specified in the config."); - } - if (fault.has_abort()) { abort_percent_ = fault.abort().percent(); http_status_ = fault.abort().http_status(); @@ -65,17 +61,12 @@ FaultFilterConfig::FaultFilterConfig(const envoy::config::filter::http::fault::v Runtime::Loader& runtime, const std::string& stats_prefix, Stats::Scope& scope) : runtime_(runtime), stats_(generateStats(stats_prefix, scope)), stats_prefix_(stats_prefix), - scope_(scope) { + scope_(scope), local_config_(PerRouteFaultFilterConfig(fault)), + filter_config_(&local_config_) { - // A small optimization to eliminate unnecessary processing in the - // fault filter when an empty config is provided - if (!fault.has_delay() && !fault.has_abort()) { - return; + if (!fault.has_abort() && !fault.has_delay()) { + filter_config_ = nullptr; } - - local_config_.reset(new PerRouteFaultFilterConfig(fault)); - // This feels a bit ugly, but we don't want to allocate anything here. - filter_config_ = local_config_.get(); } FaultFilter::FaultFilter(FaultFilterConfigSharedPtr config) : config_(config) {} diff --git a/source/extensions/filters/http/fault/fault_filter.h b/source/extensions/filters/http/fault/fault_filter.h index 9f54d55838fe..34a1a7be67d3 100644 --- a/source/extensions/filters/http/fault/fault_filter.h +++ b/source/extensions/filters/http/fault/fault_filter.h @@ -75,7 +75,7 @@ class FaultFilterConfig { } const std::string& statsPrefix() { return stats_prefix_; } Stats::Scope& scope() { return scope_; } - bool emptyFilterConfig() { return !!filter_config_; } + bool emptyFilterConfig() { return filter_config_ == nullptr; } void updateFilterConfig(const PerRouteFaultFilterConfig* config) { filter_config_ = config != nullptr ? config : filter_config_; @@ -85,7 +85,7 @@ class FaultFilterConfig { static FaultFilterStats generateStats(const std::string& prefix, Stats::Scope& scope); const PerRouteFaultFilterConfig* filter_config_; - PerRouteFaultFilterConfigConstSharedPtr local_config_; + PerRouteFaultFilterConfig local_config_; Runtime::Loader& runtime_; FaultFilterStats stats_; const std::string stats_prefix_; From 897f78f420fd83cc73fbf8512363eb1c0905c1a0 Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Fri, 20 Apr 2018 06:43:40 -0400 Subject: [PATCH 26/33] cleanup Signed-off-by: Shriram Rajagopalan --- .../extensions/filters/http/fault/config.cc | 2 +- .../filters/http/fault/fault_filter.cc | 24 ++++---------- .../filters/http/fault/fault_filter.h | 31 +++++++++---------- .../filters/http/fault/fault_filter_test.cc | 5 ++- 4 files changed, 23 insertions(+), 39 deletions(-) diff --git a/source/extensions/filters/http/fault/config.cc b/source/extensions/filters/http/fault/config.cc index db976cf45b9b..08fd5772a865 100644 --- a/source/extensions/filters/http/fault/config.cc +++ b/source/extensions/filters/http/fault/config.cc @@ -47,7 +47,7 @@ FaultFilterFactory::createRouteSpecificFilterConfig(const ProtobufWkt::Struct& s envoy::config::filter::http::fault::v2::HTTPFault proto_config; MessageUtil::jsonConvert(struct_config, proto_config); Router::RouteSpecificFilterConfigConstSharedPtr rconfig; - rconfig.reset(new Fault::PerRouteFaultFilterConfig(proto_config)); + rconfig.reset(new Fault::FaultSettings(proto_config)); return rconfig; } diff --git a/source/extensions/filters/http/fault/fault_filter.cc b/source/extensions/filters/http/fault/fault_filter.cc index 46a82da231b0..64c3f9d0ca14 100644 --- a/source/extensions/filters/http/fault/fault_filter.cc +++ b/source/extensions/filters/http/fault/fault_filter.cc @@ -32,8 +32,7 @@ const std::string FaultFilter::ABORT_PERCENT_KEY = "fault.http.abort.abort_perce const std::string FaultFilter::DELAY_DURATION_KEY = "fault.http.delay.fixed_duration_ms"; const std::string FaultFilter::ABORT_HTTP_STATUS_KEY = "fault.http.abort.http_status"; -PerRouteFaultFilterConfig::PerRouteFaultFilterConfig( - const envoy::config::filter::http::fault::v2::HTTPFault& fault) { +FaultSettings::FaultSettings(const envoy::config::filter::http::fault::v2::HTTPFault& fault) { if (fault.has_abort()) { abort_percent_ = fault.abort().percent(); @@ -61,13 +60,7 @@ FaultFilterConfig::FaultFilterConfig(const envoy::config::filter::http::fault::v Runtime::Loader& runtime, const std::string& stats_prefix, Stats::Scope& scope) : runtime_(runtime), stats_(generateStats(stats_prefix, scope)), stats_prefix_(stats_prefix), - scope_(scope), local_config_(PerRouteFaultFilterConfig(fault)), - filter_config_(&local_config_) { - - if (!fault.has_abort() && !fault.has_delay()) { - filter_config_ = nullptr; - } -} + scope_(scope), filter_config_(fault), current_config_(&filter_config_) {} FaultFilter::FaultFilter(FaultFilterConfigSharedPtr config) : config_(config) {} @@ -86,15 +79,10 @@ Http::FilterHeadersStatus FaultFilter::decodeHeaders(Http::HeaderMap& headers, b const std::string name = Extensions::HttpFilters::HttpFilterNames::get().FAULT; const auto* route_entry = callbacks_->route()->routeEntry(); - const PerRouteFaultFilterConfig* per_filter_config = - route_entry->perFilterConfigTyped(name) - ?: route_entry->virtualHost().perFilterConfigTyped(name); - config_->updateFilterConfig(per_filter_config); - } - - // ignore if filter config is empty - if (config_->emptyFilterConfig()) { - return Http::FilterHeadersStatus::Continue; + const FaultSettings* per_route_settings = + route_entry->perFilterConfigTyped(name) + ?: route_entry->virtualHost().perFilterConfigTyped(name); + config_->updateFaultSettings(per_route_settings); } if (!matchesTargetUpstreamCluster()) { diff --git a/source/extensions/filters/http/fault/fault_filter.h b/source/extensions/filters/http/fault/fault_filter.h index 34a1a7be67d3..0586fbb05c96 100644 --- a/source/extensions/filters/http/fault/fault_filter.h +++ b/source/extensions/filters/http/fault/fault_filter.h @@ -36,10 +36,10 @@ struct FaultFilterStats { }; /** - * Configuration for the fault filter that can be specified per route + * Configuration for fault injection. */ -struct PerRouteFaultFilterConfig : public Router::RouteSpecificFilterConfig { - PerRouteFaultFilterConfig(const envoy::config::filter::http::fault::v2::HTTPFault& fault); +struct FaultSettings : public Router::RouteSpecificFilterConfig { + FaultSettings(const envoy::config::filter::http::fault::v2::HTTPFault& fault); uint64_t abort_percent_{}; // 0-100 uint64_t http_status_{}; // HTTP or gRPC return codes @@ -50,8 +50,6 @@ struct PerRouteFaultFilterConfig : public Router::RouteSpecificFilterConfig { std::unordered_set downstream_nodes_{}; // Inject failures for specific downstream }; -typedef std::shared_ptr PerRouteFaultFilterConfigConstSharedPtr; - /** * Configuration for the fault filter. */ @@ -61,35 +59,34 @@ class FaultFilterConfig { Runtime::Loader& runtime, const std::string& stats_prefix, Stats::Scope& scope); const std::vector& filterHeaders() { - return filter_config_->fault_filter_headers_; + return current_config_->fault_filter_headers_; } - uint64_t abortPercent() { return filter_config_->abort_percent_; } - uint64_t delayPercent() { return filter_config_->fixed_delay_percent_; } - uint64_t delayDuration() { return filter_config_->fixed_duration_ms_; } - uint64_t abortCode() { return filter_config_->http_status_; } - const std::string& upstreamCluster() { return filter_config_->upstream_cluster_; } + uint64_t abortPercent() { return current_config_->abort_percent_; } + uint64_t delayPercent() { return current_config_->fixed_delay_percent_; } + uint64_t delayDuration() { return current_config_->fixed_duration_ms_; } + uint64_t abortCode() { return current_config_->http_status_; } + const std::string& upstreamCluster() { return current_config_->upstream_cluster_; } Runtime::Loader& runtime() { return runtime_; } FaultFilterStats& stats() { return stats_; } const std::unordered_set& downstreamNodes() { - return filter_config_->downstream_nodes_; + return current_config_->downstream_nodes_; } const std::string& statsPrefix() { return stats_prefix_; } Stats::Scope& scope() { return scope_; } - bool emptyFilterConfig() { return filter_config_ == nullptr; } - void updateFilterConfig(const PerRouteFaultFilterConfig* config) { - filter_config_ = config != nullptr ? config : filter_config_; + void updateFaultSettings(const FaultSettings* config) { + current_config_ = config != nullptr ? config : current_config_; } private: static FaultFilterStats generateStats(const std::string& prefix, Stats::Scope& scope); - const PerRouteFaultFilterConfig* filter_config_; - PerRouteFaultFilterConfig local_config_; Runtime::Loader& runtime_; FaultFilterStats stats_; const std::string stats_prefix_; Stats::Scope& scope_; + const FaultSettings filter_config_; + const FaultSettings* current_config_; }; typedef std::shared_ptr FaultFilterConfigSharedPtr; diff --git a/test/extensions/filters/http/fault/fault_filter_test.cc b/test/extensions/filters/http/fault/fault_filter_test.cc index 9b1a2d3a18f0..c231a6796bd1 100644 --- a/test/extensions/filters/http/fault/fault_filter_test.cc +++ b/test/extensions/filters/http/fault/fault_filter_test.cc @@ -798,9 +798,8 @@ void FaultFilterTest::TestPerFilterConfigFault( TEST_F(FaultFilterTest, RouteFaultOverridesListenerFault) { - Fault::PerRouteFaultFilterConfig abort_fault(convertJsonStrToProtoConfig(abort_only_json)); - Fault::PerRouteFaultFilterConfig delay_fault( - convertJsonStrToProtoConfig(delay_with_upstream_cluster_json)); + Fault::FaultSettings abort_fault(convertJsonStrToProtoConfig(abort_only_json)); + Fault::FaultSettings delay_fault(convertJsonStrToProtoConfig(delay_with_upstream_cluster_json)); // route-level fault overrides listener-level fault { From e58e9587b091f3a00c480dbf4c14e9af4150373c Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Fri, 20 Apr 2018 13:15:51 -0400 Subject: [PATCH 27/33] fixes Signed-off-by: Shriram Rajagopalan --- .../extensions/filters/http/fault/config.cc | 4 +- .../filters/http/fault/fault_filter.cc | 43 ++++++++++--------- .../filters/http/fault/fault_filter.h | 34 +++++++-------- 3 files changed, 40 insertions(+), 41 deletions(-) diff --git a/source/extensions/filters/http/fault/config.cc b/source/extensions/filters/http/fault/config.cc index 08fd5772a865..149f066ba937 100644 --- a/source/extensions/filters/http/fault/config.cc +++ b/source/extensions/filters/http/fault/config.cc @@ -46,9 +46,7 @@ Router::RouteSpecificFilterConfigConstSharedPtr FaultFilterFactory::createRouteSpecificFilterConfig(const ProtobufWkt::Struct& struct_config) { envoy::config::filter::http::fault::v2::HTTPFault proto_config; MessageUtil::jsonConvert(struct_config, proto_config); - Router::RouteSpecificFilterConfigConstSharedPtr rconfig; - rconfig.reset(new Fault::FaultSettings(proto_config)); - return rconfig; + return std::make_shared(Fault::FaultSettings(proto_config)); } /** diff --git a/source/extensions/filters/http/fault/fault_filter.cc b/source/extensions/filters/http/fault/fault_filter.cc index 64c3f9d0ca14..2fa43ba23c7a 100644 --- a/source/extensions/filters/http/fault/fault_filter.cc +++ b/source/extensions/filters/http/fault/fault_filter.cc @@ -60,7 +60,7 @@ FaultFilterConfig::FaultFilterConfig(const envoy::config::filter::http::fault::v Runtime::Loader& runtime, const std::string& stats_prefix, Stats::Scope& scope) : runtime_(runtime), stats_(generateStats(stats_prefix, scope)), stats_prefix_(stats_prefix), - scope_(scope), filter_config_(fault), current_config_(&filter_config_) {} + scope_(scope), settings_(fault) {} FaultFilter::FaultFilter(FaultFilterConfigSharedPtr config) : config_(config) {} @@ -76,13 +76,15 @@ Http::FilterHeadersStatus FaultFilter::decodeHeaders(Http::HeaderMap& headers, b // faults. In other words, runtime is supported only when faults are // configured at the filter level. if (callbacks_->route() && callbacks_->route()->routeEntry()) { - const std::string name = Extensions::HttpFilters::HttpFilterNames::get().FAULT; + const std::string& name = Extensions::HttpFilters::HttpFilterNames::get().FAULT; const auto* route_entry = callbacks_->route()->routeEntry(); - const FaultSettings* per_route_settings = - route_entry->perFilterConfigTyped(name) - ?: route_entry->virtualHost().perFilterConfigTyped(name); - config_->updateFaultSettings(per_route_settings); + fault_settings_ = route_entry->perFilterConfigTyped(name) + ?: route_entry->virtualHost().perFilterConfigTyped(name); + if (!fault_settings_) { + // If there is no route_specific_setting, use the filter-level setting. + fault_settings_ = config_->settings(); + } } if (!matchesTargetUpstreamCluster()) { @@ -94,7 +96,7 @@ Http::FilterHeadersStatus FaultFilter::decodeHeaders(Http::HeaderMap& headers, b } // Check for header matches - if (!Router::ConfigUtility::matchHeaders(headers, config_->filterHeaders())) { + if (!Router::ConfigUtility::matchHeaders(headers, fault_settings_->filterHeaders())) { return Http::FilterHeadersStatus::Continue; } @@ -129,24 +131,24 @@ Http::FilterHeadersStatus FaultFilter::decodeHeaders(Http::HeaderMap& headers, b } bool FaultFilter::isDelayEnabled() { - bool enabled = - config_->runtime().snapshot().featureEnabled(DELAY_PERCENT_KEY, config_->delayPercent()); + bool enabled = config_->runtime().snapshot().featureEnabled(DELAY_PERCENT_KEY, + fault_settings_->delayPercent()); if (!downstream_cluster_delay_percent_key_.empty()) { enabled |= config_->runtime().snapshot().featureEnabled(downstream_cluster_delay_percent_key_, - config_->delayPercent()); + fault_settings_->delayPercent()); } return enabled; } bool FaultFilter::isAbortEnabled() { - bool enabled = - config_->runtime().snapshot().featureEnabled(ABORT_PERCENT_KEY, config_->abortPercent()); + bool enabled = config_->runtime().snapshot().featureEnabled(ABORT_PERCENT_KEY, + fault_settings_->abortPercent()); if (!downstream_cluster_abort_percent_key_.empty()) { enabled |= config_->runtime().snapshot().featureEnabled(downstream_cluster_abort_percent_key_, - config_->abortPercent()); + fault_settings_->abortPercent()); } return enabled; @@ -159,8 +161,8 @@ absl::optional FaultFilter::delayDuration() { return ret; } - uint64_t duration = - config_->runtime().snapshot().getInteger(DELAY_DURATION_KEY, config_->delayDuration()); + uint64_t duration = config_->runtime().snapshot().getInteger(DELAY_DURATION_KEY, + fault_settings_->delayDuration()); if (!downstream_cluster_delay_duration_key_.empty()) { duration = config_->runtime().snapshot().getInteger(downstream_cluster_delay_duration_key_, duration); @@ -177,7 +179,7 @@ absl::optional FaultFilter::delayDuration() { uint64_t FaultFilter::abortHttpStatus() { // TODO(mattklein123): check http status codes obtained from runtime. uint64_t http_status = - config_->runtime().snapshot().getInteger(ABORT_HTTP_STATUS_KEY, config_->abortCode()); + config_->runtime().snapshot().getInteger(ABORT_HTTP_STATUS_KEY, fault_settings_->abortCode()); if (!downstream_cluster_abort_http_status_key_.empty()) { http_status = config_->runtime().snapshot().getInteger( @@ -258,17 +260,17 @@ void FaultFilter::abortWithHTTPStatus() { bool FaultFilter::matchesTargetUpstreamCluster() { bool matches = true; - if (!config_->upstreamCluster().empty()) { + if (!fault_settings_->upstreamCluster().empty()) { Router::RouteConstSharedPtr route = callbacks_->route(); matches = route && route->routeEntry() && - (route->routeEntry()->clusterName() == config_->upstreamCluster()); + (route->routeEntry()->clusterName() == fault_settings_->upstreamCluster()); } return matches; } bool FaultFilter::matchesDownstreamNodes(const Http::HeaderMap& headers) { - if (config_->downstreamNodes().empty()) { + if (fault_settings_->downstreamNodes().empty()) { return true; } @@ -277,7 +279,8 @@ bool FaultFilter::matchesDownstreamNodes(const Http::HeaderMap& headers) { } const std::string downstream_node = headers.EnvoyDownstreamServiceNode()->value().c_str(); - return config_->downstreamNodes().find(downstream_node) != config_->downstreamNodes().end(); + return fault_settings_->downstreamNodes().find(downstream_node) != + fault_settings_->downstreamNodes().end(); } void FaultFilter::resetTimerState() { diff --git a/source/extensions/filters/http/fault/fault_filter.h b/source/extensions/filters/http/fault/fault_filter.h index 0586fbb05c96..fd6d7d98d731 100644 --- a/source/extensions/filters/http/fault/fault_filter.h +++ b/source/extensions/filters/http/fault/fault_filter.h @@ -38,9 +38,21 @@ struct FaultFilterStats { /** * Configuration for fault injection. */ -struct FaultSettings : public Router::RouteSpecificFilterConfig { +class FaultSettings : public Router::RouteSpecificFilterConfig { +public: FaultSettings(const envoy::config::filter::http::fault::v2::HTTPFault& fault); + const std::vector& filterHeaders() const { + return fault_filter_headers_; + } + uint64_t abortPercent() const { return abort_percent_; } + uint64_t delayPercent() const { return fixed_delay_percent_; } + uint64_t delayDuration() const { return fixed_duration_ms_; } + uint64_t abortCode() const { return http_status_; } + const std::string& upstreamCluster() const { return upstream_cluster_; } + const std::unordered_set& downstreamNodes() const { return downstream_nodes_; } + +private: uint64_t abort_percent_{}; // 0-100 uint64_t http_status_{}; // HTTP or gRPC return codes uint64_t fixed_delay_percent_{}; // 0-100 @@ -58,25 +70,11 @@ class FaultFilterConfig { FaultFilterConfig(const envoy::config::filter::http::fault::v2::HTTPFault& fault, Runtime::Loader& runtime, const std::string& stats_prefix, Stats::Scope& scope); - const std::vector& filterHeaders() { - return current_config_->fault_filter_headers_; - } - uint64_t abortPercent() { return current_config_->abort_percent_; } - uint64_t delayPercent() { return current_config_->fixed_delay_percent_; } - uint64_t delayDuration() { return current_config_->fixed_duration_ms_; } - uint64_t abortCode() { return current_config_->http_status_; } - const std::string& upstreamCluster() { return current_config_->upstream_cluster_; } Runtime::Loader& runtime() { return runtime_; } FaultFilterStats& stats() { return stats_; } - const std::unordered_set& downstreamNodes() { - return current_config_->downstream_nodes_; - } const std::string& statsPrefix() { return stats_prefix_; } Stats::Scope& scope() { return scope_; } - - void updateFaultSettings(const FaultSettings* config) { - current_config_ = config != nullptr ? config : current_config_; - } + const FaultSettings* settings() { return &settings_; } private: static FaultFilterStats generateStats(const std::string& prefix, Stats::Scope& scope); @@ -85,8 +83,7 @@ class FaultFilterConfig { FaultFilterStats stats_; const std::string stats_prefix_; Stats::Scope& scope_; - const FaultSettings filter_config_; - const FaultSettings* current_config_; + const FaultSettings settings_; }; typedef std::shared_ptr FaultFilterConfigSharedPtr; @@ -127,6 +124,7 @@ class FaultFilter : public Http::StreamDecoderFilter { Event::TimerPtr delay_timer_; std::string downstream_cluster_{}; bool stream_destroyed_{}; + const FaultSettings* fault_settings_; std::string downstream_cluster_delay_percent_key_{}; std::string downstream_cluster_abort_percent_key_{}; From 46dbf9fcd4374bf6b51bd010fd00a67a6da3cd18 Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Fri, 20 Apr 2018 13:20:36 -0400 Subject: [PATCH 28/33] format Signed-off-by: Shriram Rajagopalan --- source/extensions/filters/http/fault/config.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source/extensions/filters/http/fault/config.cc b/source/extensions/filters/http/fault/config.cc index 149f066ba937..1cd8e3a9c6a2 100644 --- a/source/extensions/filters/http/fault/config.cc +++ b/source/extensions/filters/http/fault/config.cc @@ -46,7 +46,8 @@ Router::RouteSpecificFilterConfigConstSharedPtr FaultFilterFactory::createRouteSpecificFilterConfig(const ProtobufWkt::Struct& struct_config) { envoy::config::filter::http::fault::v2::HTTPFault proto_config; MessageUtil::jsonConvert(struct_config, proto_config); - return std::make_shared(Fault::FaultSettings(proto_config)); + return std::make_shared( + Fault::FaultSettings(proto_config)); } /** From 80a3aaa3a9ad2df44f8a45ee9309260a0e5d4456 Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Fri, 20 Apr 2018 13:39:55 -0400 Subject: [PATCH 29/33] const faultfilterconfig Signed-off-by: Shriram Rajagopalan --- source/extensions/filters/http/fault/fault_filter.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/extensions/filters/http/fault/fault_filter.h b/source/extensions/filters/http/fault/fault_filter.h index fd6d7d98d731..e99a59e07142 100644 --- a/source/extensions/filters/http/fault/fault_filter.h +++ b/source/extensions/filters/http/fault/fault_filter.h @@ -86,7 +86,7 @@ class FaultFilterConfig { const FaultSettings settings_; }; -typedef std::shared_ptr FaultFilterConfigSharedPtr; +typedef std::shared_ptr FaultFilterConfigSharedPtr; /** * A filter that is capable of faulting an entire request before dispatching it upstream. From 9a15b9fa770570e533a2af90f7d3d4b5fce18f5f Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Fri, 20 Apr 2018 14:26:14 -0400 Subject: [PATCH 30/33] const everything Signed-off-by: Shriram Rajagopalan --- source/extensions/filters/http/fault/fault_filter.h | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/source/extensions/filters/http/fault/fault_filter.h b/source/extensions/filters/http/fault/fault_filter.h index e99a59e07142..84690d91505b 100644 --- a/source/extensions/filters/http/fault/fault_filter.h +++ b/source/extensions/filters/http/fault/fault_filter.h @@ -70,11 +70,11 @@ class FaultFilterConfig { FaultFilterConfig(const envoy::config::filter::http::fault::v2::HTTPFault& fault, Runtime::Loader& runtime, const std::string& stats_prefix, Stats::Scope& scope); - Runtime::Loader& runtime() { return runtime_; } - FaultFilterStats& stats() { return stats_; } - const std::string& statsPrefix() { return stats_prefix_; } - Stats::Scope& scope() { return scope_; } - const FaultSettings* settings() { return &settings_; } + Runtime::Loader& runtime() const { return runtime_; } + FaultFilterStats& stats() const { return stats_; } + const std::string& statsPrefix() const { return stats_prefix_; } + Stats::Scope& scope() const { return scope_; } + const FaultSettings* settings() const { return &settings_; } private: static FaultFilterStats generateStats(const std::string& prefix, Stats::Scope& scope); From 5982b5f41ba2d5ba3f3edb0b8bdbd202d25b9033 Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Fri, 20 Apr 2018 14:43:41 -0400 Subject: [PATCH 31/33] unconst Signed-off-by: Shriram Rajagopalan --- source/extensions/filters/http/fault/fault_filter.h | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/source/extensions/filters/http/fault/fault_filter.h b/source/extensions/filters/http/fault/fault_filter.h index 84690d91505b..fd6d7d98d731 100644 --- a/source/extensions/filters/http/fault/fault_filter.h +++ b/source/extensions/filters/http/fault/fault_filter.h @@ -70,11 +70,11 @@ class FaultFilterConfig { FaultFilterConfig(const envoy::config::filter::http::fault::v2::HTTPFault& fault, Runtime::Loader& runtime, const std::string& stats_prefix, Stats::Scope& scope); - Runtime::Loader& runtime() const { return runtime_; } - FaultFilterStats& stats() const { return stats_; } - const std::string& statsPrefix() const { return stats_prefix_; } - Stats::Scope& scope() const { return scope_; } - const FaultSettings* settings() const { return &settings_; } + Runtime::Loader& runtime() { return runtime_; } + FaultFilterStats& stats() { return stats_; } + const std::string& statsPrefix() { return stats_prefix_; } + Stats::Scope& scope() { return scope_; } + const FaultSettings* settings() { return &settings_; } private: static FaultFilterStats generateStats(const std::string& prefix, Stats::Scope& scope); @@ -86,7 +86,7 @@ class FaultFilterConfig { const FaultSettings settings_; }; -typedef std::shared_ptr FaultFilterConfigSharedPtr; +typedef std::shared_ptr FaultFilterConfigSharedPtr; /** * A filter that is capable of faulting an entire request before dispatching it upstream. From 1e585b6ef74154fe2e98e25dba85ba2a966c1b51 Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Fri, 20 Apr 2018 15:55:33 -0400 Subject: [PATCH 32/33] alignment fix Signed-off-by: Shriram Rajagopalan --- source/extensions/filters/http/fault/fault_filter.cc | 4 ++-- source/extensions/filters/http/fault/fault_filter.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/source/extensions/filters/http/fault/fault_filter.cc b/source/extensions/filters/http/fault/fault_filter.cc index 2fa43ba23c7a..4c94a30dbe9c 100644 --- a/source/extensions/filters/http/fault/fault_filter.cc +++ b/source/extensions/filters/http/fault/fault_filter.cc @@ -59,8 +59,8 @@ FaultSettings::FaultSettings(const envoy::config::filter::http::fault::v2::HTTPF FaultFilterConfig::FaultFilterConfig(const envoy::config::filter::http::fault::v2::HTTPFault& fault, Runtime::Loader& runtime, const std::string& stats_prefix, Stats::Scope& scope) - : runtime_(runtime), stats_(generateStats(stats_prefix, scope)), stats_prefix_(stats_prefix), - scope_(scope), settings_(fault) {} + : settings_(fault), runtime_(runtime), stats_(generateStats(stats_prefix, scope)), + stats_prefix_(stats_prefix), scope_(scope) {} FaultFilter::FaultFilter(FaultFilterConfigSharedPtr config) : config_(config) {} diff --git a/source/extensions/filters/http/fault/fault_filter.h b/source/extensions/filters/http/fault/fault_filter.h index fd6d7d98d731..295e3e292964 100644 --- a/source/extensions/filters/http/fault/fault_filter.h +++ b/source/extensions/filters/http/fault/fault_filter.h @@ -79,11 +79,11 @@ class FaultFilterConfig { private: static FaultFilterStats generateStats(const std::string& prefix, Stats::Scope& scope); + const FaultSettings settings_; Runtime::Loader& runtime_; FaultFilterStats stats_; const std::string stats_prefix_; Stats::Scope& scope_; - const FaultSettings settings_; }; typedef std::shared_ptr FaultFilterConfigSharedPtr; From 5e893aa5363e08927274ddce0d5992ca2fc5fe7c Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Fri, 20 Apr 2018 17:59:32 -0400 Subject: [PATCH 33/33] fix conditional assignment Signed-off-by: Shriram Rajagopalan --- source/extensions/filters/http/fault/fault_filter.cc | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/source/extensions/filters/http/fault/fault_filter.cc b/source/extensions/filters/http/fault/fault_filter.cc index 4c94a30dbe9c..a3280c0eaef6 100644 --- a/source/extensions/filters/http/fault/fault_filter.cc +++ b/source/extensions/filters/http/fault/fault_filter.cc @@ -75,16 +75,15 @@ Http::FilterHeadersStatus FaultFilter::decodeHeaders(Http::HeaderMap& headers, b // NOTE: We should not use runtime when reading from route-level // faults. In other words, runtime is supported only when faults are // configured at the filter level. + fault_settings_ = config_->settings(); if (callbacks_->route() && callbacks_->route()->routeEntry()) { const std::string& name = Extensions::HttpFilters::HttpFilterNames::get().FAULT; const auto* route_entry = callbacks_->route()->routeEntry(); - fault_settings_ = route_entry->perFilterConfigTyped(name) - ?: route_entry->virtualHost().perFilterConfigTyped(name); - if (!fault_settings_) { - // If there is no route_specific_setting, use the filter-level setting. - fault_settings_ = config_->settings(); - } + const FaultSettings* per_route_settings_ = + route_entry->perFilterConfigTyped(name) + ?: route_entry->virtualHost().perFilterConfigTyped(name); + fault_settings_ = per_route_settings_ ?: fault_settings_; } if (!matchesTargetUpstreamCluster()) {