From 69b7383c15edb7195960b1be11255ce0866230f4 Mon Sep 17 00:00:00 2001 From: Bill Gallagher Date: Thu, 16 Feb 2017 20:09:57 -0500 Subject: [PATCH 01/13] opaque config --- include/envoy/router/router.h | 6 ++++ source/common/http/async_client_impl.cc | 1 + source/common/http/async_client_impl.h | 4 +++ source/common/json/config_schemas.cc | 3 +- source/common/router/config_impl.cc | 3 +- source/common/router/config_impl.h | 6 ++++ test/common/router/config_impl_test.cc | 37 +++++++++++++++++++++++++ test/mocks/router/mocks.h | 1 + 8 files changed, 59 insertions(+), 2 deletions(-) diff --git a/include/envoy/router/router.h b/include/envoy/router/router.h index 5f1d74adb05ce..ffe8ac570bc3c 100644 --- a/include/envoy/router/router.h +++ b/include/envoy/router/router.h @@ -3,6 +3,7 @@ #include "envoy/common/optional.h" #include "envoy/http/codec.h" #include "envoy/http/header_map.h" +#include "envoy/json/json_object.h" #include "envoy/upstream/resource_manager.h" namespace Router { @@ -225,6 +226,11 @@ class RouteEntry { * @return bool true if the :authority header should be overwritten with the upstream hostname. */ virtual bool autoHostRewrite() const PURE; + + /** + * @return const Json::ObjectPtr& the opaque configuration associated with the route + */ + virtual const Json::ObjectPtr& opaqueConfig() const PURE; }; /** diff --git a/source/common/http/async_client_impl.cc b/source/common/http/async_client_impl.cc index 19470d94823cd..6290f2a649759 100644 --- a/source/common/http/async_client_impl.cc +++ b/source/common/http/async_client_impl.cc @@ -10,6 +10,7 @@ const AsyncStreamImpl::NullRetryPolicy AsyncStreamImpl::RouteEntryImpl::retry_po const AsyncStreamImpl::NullShadowPolicy AsyncStreamImpl::RouteEntryImpl::shadow_policy_; const AsyncStreamImpl::NullVirtualHost AsyncStreamImpl::RouteEntryImpl::virtual_host_; const AsyncStreamImpl::NullRateLimitPolicy AsyncStreamImpl::NullVirtualHost::rate_limit_policy_; +const Json::ObjectPtr AsyncStreamImpl::RouteEntryImpl::opaque_config_; AsyncClientImpl::AsyncClientImpl(const Upstream::ClusterInfo& cluster, Stats::Store& stats_store, Event::Dispatcher& dispatcher, diff --git a/source/common/http/async_client_impl.h b/source/common/http/async_client_impl.h index eab69bcaf4899..7c07902500cfd 100644 --- a/source/common/http/async_client_impl.h +++ b/source/common/http/async_client_impl.h @@ -128,6 +128,9 @@ class AsyncStreamImpl : public AsyncClient::Stream, const Router::VirtualCluster* virtualCluster(const Http::HeaderMap&) const override { return nullptr; } + const Json::ObjectPtr& opaqueConfig() const override { + return opaque_config_; + } const Router::VirtualHost& virtualHost() const override { return virtual_host_; } bool autoHostRewrite() const override { return false; } @@ -135,6 +138,7 @@ class AsyncStreamImpl : public AsyncClient::Stream, static const NullRetryPolicy retry_policy_; static const NullShadowPolicy shadow_policy_; static const NullVirtualHost virtual_host_; + static const Json::ObjectPtr opaque_config_; const std::string& cluster_name_; Optional timeout_; diff --git a/source/common/json/config_schemas.cc b/source/common/json/config_schemas.cc index 85bd0ff050c91..5b5fa418f16b2 100644 --- a/source/common/json/config_schemas.cc +++ b/source/common/json/config_schemas.cc @@ -544,7 +544,8 @@ const std::string Json::Schema::ROUTE_ENTRY_CONFIGURATION_SCHEMA(R"EOF( }, "required" : ["header_name"], "additionalProperties" : false - } + }, + "opaque_config" : {"type" : "any"} }, "additionalProperties" : false } diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index 8504c83256575..0682f4e906940 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -102,7 +102,8 @@ RouteEntryImplBase::RouteEntryImplBase(const VirtualHostImpl& vhost, const Json: host_redirect_(route.getString("host_redirect", "")), path_redirect_(route.getString("path_redirect", "")), retry_policy_(route), rate_limit_policy_(route), shadow_policy_(route), - priority_(ConfigUtility::parsePriority(route)) { + priority_(ConfigUtility::parsePriority(route)), + opaque_config_(route.getObject("opaque_config")) { route.validateSchema(Json::Schema::ROUTE_ENTRY_CONFIGURATION_SCHEMA); diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h index 2e51dfb007586..177551b47f240 100644 --- a/source/common/router/config_impl.h +++ b/source/common/router/config_impl.h @@ -213,6 +213,7 @@ class RouteEntryImplBase : public RouteEntry, std::chrono::milliseconds timeout() const override { return timeout_; } const VirtualHost& virtualHost() const override { return vhost_; } bool autoHostRewrite() const override { return auto_host_rewrite_; } + const Json::ObjectPtr& opaqueConfig() const override { return opaque_config_; } // Router::RedirectEntry std::string newPath(const Http::HeaderMap& headers) const override; @@ -258,6 +259,10 @@ class RouteEntryImplBase : public RouteEntry, return parent_->virtualCluster(headers); } + const Json::ObjectPtr& opaqueConfig() const override { + return parent_->opaqueConfig(); + } + const VirtualHost& virtualHost() const override { return parent_->virtualHost(); } bool autoHostRewrite() const override { return parent_->autoHostRewrite(); } @@ -318,6 +323,7 @@ class RouteEntryImplBase : public RouteEntry, std::vector config_headers_; std::vector weighted_clusters_; std::unique_ptr hash_policy_; + const Json::ObjectPtr opaque_config_; }; /** diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index 4fc5cfa6d41a4..1bb153bff2f77 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -1628,4 +1628,41 @@ TEST(BadHttpRouteConfigurationsTest, BadRouteEntryConfigPrefixAndPath) { EXPECT_THROW(ConfigImpl(*loader, runtime, cm, true), EnvoyException); } +TEST(RouteMatcherTest, TestOpaqueOptions) { + std::string json = R"EOF( +{ + "virtual_hosts": [ + { + "name": "default", + "domains": ["*"], + "routes": [ + { + "prefix": "/api/application_data", + "cluster": "ats", + "opaque_config": { + "teststring": "testvalue", + "testbool": true, + "testnumber": 1234 + } + }] + } + ] + +} + )EOF"; + + Json::ObjectPtr loader = Json::Factory::LoadFromString(json); + NiceMock runtime; + NiceMock cm; + ConfigImpl config(*loader, runtime, cm, true); + + EXPECT_EQ("testvalue", + config.route(genHeaders("api.lyft.com", "/", "GET"), 0)->routeEntry()->opaqueConfig()->getString("teststring")); + EXPECT_EQ(true, + config.route(genHeaders("api.lyft.com", "/", "GET"), 0)->routeEntry()->opaqueConfig()->getBoolean("testbool")); + EXPECT_EQ(1234, + config.route(genHeaders("api.lyft.com", "/", "GET"), 0)->routeEntry()->opaqueConfig()->getInteger("testnumber")); +} + + } // Router diff --git a/test/mocks/router/mocks.h b/test/mocks/router/mocks.h index 24c61d2b441f4..4a610dfc641ff 100644 --- a/test/mocks/router/mocks.h +++ b/test/mocks/router/mocks.h @@ -148,6 +148,7 @@ class MockRouteEntry : public RouteEntry { MOCK_CONST_METHOD0(virtualHostName, const std::string&()); MOCK_CONST_METHOD0(virtualHost, const VirtualHost&()); MOCK_CONST_METHOD0(autoHostRewrite, bool()); + MOCK_CONST_METHOD0(opaqueConfig, const Json::ObjectPtr&()); std::string cluster_name_{"fake_cluster"}; TestVirtualCluster virtual_cluster_; From 35a261307f0330bef6337ec38c6d04c11c4e4310 Mon Sep 17 00:00:00 2001 From: Bill Gallagher Date: Fri, 17 Feb 2017 11:58:58 -0500 Subject: [PATCH 02/13] fix format --- source/common/http/async_client_impl.h | 6 ++---- source/common/router/config_impl.h | 4 +--- test/common/router/config_impl_test.cc | 19 ++++++++++++------- 3 files changed, 15 insertions(+), 14 deletions(-) diff --git a/source/common/http/async_client_impl.h b/source/common/http/async_client_impl.h index 7c07902500cfd..13e84c3177ac7 100644 --- a/source/common/http/async_client_impl.h +++ b/source/common/http/async_client_impl.h @@ -128,9 +128,7 @@ class AsyncStreamImpl : public AsyncClient::Stream, const Router::VirtualCluster* virtualCluster(const Http::HeaderMap&) const override { return nullptr; } - const Json::ObjectPtr& opaqueConfig() const override { - return opaque_config_; - } + const Json::ObjectPtr& opaqueConfig() const override { return opaque_config_; } const Router::VirtualHost& virtualHost() const override { return virtual_host_; } bool autoHostRewrite() const override { return false; } @@ -138,7 +136,7 @@ class AsyncStreamImpl : public AsyncClient::Stream, static const NullRetryPolicy retry_policy_; static const NullShadowPolicy shadow_policy_; static const NullVirtualHost virtual_host_; - static const Json::ObjectPtr opaque_config_; + static const Json::ObjectPtr opaque_config_; const std::string& cluster_name_; Optional timeout_; diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h index 177551b47f240..78b030935a20b 100644 --- a/source/common/router/config_impl.h +++ b/source/common/router/config_impl.h @@ -259,9 +259,7 @@ class RouteEntryImplBase : public RouteEntry, return parent_->virtualCluster(headers); } - const Json::ObjectPtr& opaqueConfig() const override { - return parent_->opaqueConfig(); - } + const Json::ObjectPtr& opaqueConfig() const override { return parent_->opaqueConfig(); } const VirtualHost& virtualHost() const override { return parent_->virtualHost(); } bool autoHostRewrite() const override { return parent_->autoHostRewrite(); } diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index 1bb153bff2f77..8501e58847f12 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -1656,13 +1656,18 @@ TEST(RouteMatcherTest, TestOpaqueOptions) { NiceMock cm; ConfigImpl config(*loader, runtime, cm, true); - EXPECT_EQ("testvalue", - config.route(genHeaders("api.lyft.com", "/", "GET"), 0)->routeEntry()->opaqueConfig()->getString("teststring")); - EXPECT_EQ(true, - config.route(genHeaders("api.lyft.com", "/", "GET"), 0)->routeEntry()->opaqueConfig()->getBoolean("testbool")); - EXPECT_EQ(1234, - config.route(genHeaders("api.lyft.com", "/", "GET"), 0)->routeEntry()->opaqueConfig()->getInteger("testnumber")); + EXPECT_EQ("testvalue", config.route(genHeaders("api.lyft.com", "/", "GET"), 0) + ->routeEntry() + ->opaqueConfig() + ->getString("teststring")); + EXPECT_EQ(true, config.route(genHeaders("api.lyft.com", "/", "GET"), 0) + ->routeEntry() + ->opaqueConfig() + ->getBoolean("testbool")); + EXPECT_EQ(1234, config.route(genHeaders("api.lyft.com", "/", "GET"), 0) + ->routeEntry() + ->opaqueConfig() + ->getInteger("testnumber")); } - } // Router From 2ce53823ab7955be6eeecef1fdc28b0e39a8b9a6 Mon Sep 17 00:00:00 2001 From: Bill Gallagher Date: Tue, 21 Feb 2017 11:35:57 -0500 Subject: [PATCH 03/13] review comments --- .../http_conn_man/route_config/route.rst | 27 +++++++++++++- include/envoy/router/router.h | 6 ++-- source/common/http/async_client_impl.cc | 2 +- source/common/http/async_client_impl.h | 6 ++-- source/common/json/config_schemas.cc | 14 +++++++- source/common/router/config_impl.cc | 15 ++++++-- source/common/router/config_impl.h | 12 +++++-- test/common/router/config_impl_test.cc | 36 ++++++++----------- test/mocks/router/mocks.h | 2 +- 9 files changed, 84 insertions(+), 36 deletions(-) diff --git a/docs/configuration/http_conn_man/route_config/route.rst b/docs/configuration/http_conn_man/route_config/route.rst index 161f418f7c0fd..d44d58f25bc30 100644 --- a/docs/configuration/http_conn_man/route_config/route.rst +++ b/docs/configuration/http_conn_man/route_config/route.rst @@ -27,7 +27,8 @@ next (e.g., redirect, forward, rewrite, etc.). "priority": "...", "headers": [], "rate_limits": [], - "hash_policy": "{...}" + "hash_policy": "{...}", + "opaque_config": [] } prefix @@ -139,6 +140,9 @@ priority :ref:`headers ` *(optional, array)* Specifies a set of headers that the route should match on. +:ref:`opaque_config ` + *(optional, array)* Specifies a set of optional route configuration values that can be accessed by filters. + .. _config_http_conn_man_route_table_route_rate_limits: :ref:`rate_limits ` @@ -334,3 +338,24 @@ header_name *(required, string)* The name of the request header that will be used to obtain the hash key. If the request header is not present, the load balancer will use a random number as the hash, effectively making the load balancing policy random. + +.. _config_http_conn_man_route_table_opaque_config: + +Opaque Config +------------- + +Additional configuration can be provided to filters through the "Opaque Config" mechanism. A +list of Key/Value pairs are specified in the route config. The configuration is uninterpreted +by envoy and can be accessed within a user-defined filter. + +.. code-block:: json + + [ + {"name": "...", "value": "..."} + ] + +name + *(required, string)* A user defined configuration name + +value + *(required, string)* A user defined configuration value diff --git a/include/envoy/router/router.h b/include/envoy/router/router.h index ffe8ac570bc3c..a56d84474df2e 100644 --- a/include/envoy/router/router.h +++ b/include/envoy/router/router.h @@ -3,7 +3,6 @@ #include "envoy/common/optional.h" #include "envoy/http/codec.h" #include "envoy/http/header_map.h" -#include "envoy/json/json_object.h" #include "envoy/upstream/resource_manager.h" namespace Router { @@ -228,9 +227,10 @@ class RouteEntry { virtual bool autoHostRewrite() const PURE; /** - * @return const Json::ObjectPtr& the opaque configuration associated with the route + * @return const std::unordered_map the opaque configuration associated + * with the route */ - virtual const Json::ObjectPtr& opaqueConfig() const PURE; + virtual const std::unordered_map& opaqueConfig() const PURE; }; /** diff --git a/source/common/http/async_client_impl.cc b/source/common/http/async_client_impl.cc index 6290f2a649759..88061fef32931 100644 --- a/source/common/http/async_client_impl.cc +++ b/source/common/http/async_client_impl.cc @@ -10,7 +10,7 @@ const AsyncStreamImpl::NullRetryPolicy AsyncStreamImpl::RouteEntryImpl::retry_po const AsyncStreamImpl::NullShadowPolicy AsyncStreamImpl::RouteEntryImpl::shadow_policy_; const AsyncStreamImpl::NullVirtualHost AsyncStreamImpl::RouteEntryImpl::virtual_host_; const AsyncStreamImpl::NullRateLimitPolicy AsyncStreamImpl::NullVirtualHost::rate_limit_policy_; -const Json::ObjectPtr AsyncStreamImpl::RouteEntryImpl::opaque_config_; +const std::unordered_map AsyncStreamImpl::RouteEntryImpl::opaque_config_; AsyncClientImpl::AsyncClientImpl(const Upstream::ClusterInfo& cluster, Stats::Store& stats_store, Event::Dispatcher& dispatcher, diff --git a/source/common/http/async_client_impl.h b/source/common/http/async_client_impl.h index 13e84c3177ac7..0af0148bbf4cb 100644 --- a/source/common/http/async_client_impl.h +++ b/source/common/http/async_client_impl.h @@ -128,7 +128,9 @@ class AsyncStreamImpl : public AsyncClient::Stream, const Router::VirtualCluster* virtualCluster(const Http::HeaderMap&) const override { return nullptr; } - const Json::ObjectPtr& opaqueConfig() const override { return opaque_config_; } + const std::unordered_map& opaqueConfig() const override { + return opaque_config_; + } const Router::VirtualHost& virtualHost() const override { return virtual_host_; } bool autoHostRewrite() const override { return false; } @@ -136,7 +138,7 @@ class AsyncStreamImpl : public AsyncClient::Stream, static const NullRetryPolicy retry_policy_; static const NullShadowPolicy shadow_policy_; static const NullVirtualHost virtual_host_; - static const Json::ObjectPtr opaque_config_; + static const std::unordered_map opaque_config_; const std::string& cluster_name_; Optional timeout_; diff --git a/source/common/json/config_schemas.cc b/source/common/json/config_schemas.cc index 5b5fa418f16b2..74e97d537e0f8 100644 --- a/source/common/json/config_schemas.cc +++ b/source/common/json/config_schemas.cc @@ -545,7 +545,19 @@ const std::string Json::Schema::ROUTE_ENTRY_CONFIGURATION_SCHEMA(R"EOF( "required" : ["header_name"], "additionalProperties" : false }, - "opaque_config" : {"type" : "any"} + "opaque_config" : { + "type" : "array", + "minItems": 1, + "items" : { + "type" : "object", + "properties" : { + "name" : {"type" : "string"}, + "value" : {"type" : "string"} + }, + "required" : ["name", "value"], + "additionalProperties": false + } + } }, "additionalProperties" : false } diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index 0682f4e906940..9b2821a5a9e9a 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -102,8 +102,7 @@ RouteEntryImplBase::RouteEntryImplBase(const VirtualHostImpl& vhost, const Json: host_redirect_(route.getString("host_redirect", "")), path_redirect_(route.getString("path_redirect", "")), retry_policy_(route), rate_limit_policy_(route), shadow_policy_(route), - priority_(ConfigUtility::parsePriority(route)), - opaque_config_(route.getObject("opaque_config")) { + priority_(ConfigUtility::parsePriority(route)), opaque_config_(parseOpaqueConfig(route)) { route.validateSchema(Json::Schema::ROUTE_ENTRY_CONFIGURATION_SCHEMA); @@ -251,6 +250,18 @@ std::string RouteEntryImplBase::newPath(const Http::HeaderMap& headers) const { final_path); } +std::unordered_map +RouteEntryImplBase::parseOpaqueConfig(const Json::Object& route) { + std::unordered_map ret; + if (route.hasObject("opaque_config")) { + std::vector kvs = route.getObjectArray("opaque_config"); + for (const Json::ObjectPtr& kv : kvs) { + ret[kv->getString("name")] = kv->getString("value"); + } + } + return ret; +} + const RedirectEntry* RouteEntryImplBase::redirectEntry() const { // A route for a request can exclusively be a route entry or a redirect entry. if (isRedirect()) { diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h index 78b030935a20b..89fb2902fe620 100644 --- a/source/common/router/config_impl.h +++ b/source/common/router/config_impl.h @@ -213,7 +213,9 @@ class RouteEntryImplBase : public RouteEntry, std::chrono::milliseconds timeout() const override { return timeout_; } const VirtualHost& virtualHost() const override { return vhost_; } bool autoHostRewrite() const override { return auto_host_rewrite_; } - const Json::ObjectPtr& opaqueConfig() const override { return opaque_config_; } + const std::unordered_map& opaqueConfig() const override { + return opaque_config_; + } // Router::RedirectEntry std::string newPath(const Http::HeaderMap& headers) const override; @@ -259,7 +261,9 @@ class RouteEntryImplBase : public RouteEntry, return parent_->virtualCluster(headers); } - const Json::ObjectPtr& opaqueConfig() const override { return parent_->opaqueConfig(); } + const std::unordered_map& opaqueConfig() const override { + return parent_->opaqueConfig(); + } const VirtualHost& virtualHost() const override { return parent_->virtualHost(); } bool autoHostRewrite() const override { return parent_->autoHostRewrite(); } @@ -302,6 +306,8 @@ class RouteEntryImplBase : public RouteEntry, static Optional loadRuntimeData(const Json::Object& route); + static std::unordered_map parseOpaqueConfig(const Json::Object& route); + // Default timeout is 15s if nothing is specified in the route config. static const uint64_t DEFAULT_ROUTE_TIMEOUT_MS = 15000; @@ -321,7 +327,7 @@ class RouteEntryImplBase : public RouteEntry, std::vector config_headers_; std::vector weighted_clusters_; std::unique_ptr hash_policy_; - const Json::ObjectPtr opaque_config_; + const std::unordered_map opaque_config_; }; /** diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index 8501e58847f12..7c78f367cfa3f 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -1628,7 +1628,7 @@ TEST(BadHttpRouteConfigurationsTest, BadRouteEntryConfigPrefixAndPath) { EXPECT_THROW(ConfigImpl(*loader, runtime, cm, true), EnvoyException); } -TEST(RouteMatcherTest, TestOpaqueOptions) { +TEST(RouteMatcherTest, TestOpaqueConfig) { std::string json = R"EOF( { "virtual_hosts": [ @@ -1637,37 +1637,29 @@ TEST(RouteMatcherTest, TestOpaqueOptions) { "domains": ["*"], "routes": [ { - "prefix": "/api/application_data", + "prefix": "/api", "cluster": "ats", - "opaque_config": { - "teststring": "testvalue", - "testbool": true, - "testnumber": 1234 - } - }] + "opaque_config" : [ + { "name": "name1", "value": "value1" }, + { "name": "name2", "value": "value2" } + ] + } + ] } ] - } - )EOF"; +)EOF"; Json::ObjectPtr loader = Json::Factory::LoadFromString(json); NiceMock runtime; NiceMock cm; ConfigImpl config(*loader, runtime, cm, true); - EXPECT_EQ("testvalue", config.route(genHeaders("api.lyft.com", "/", "GET"), 0) - ->routeEntry() - ->opaqueConfig() - ->getString("teststring")); - EXPECT_EQ(true, config.route(genHeaders("api.lyft.com", "/", "GET"), 0) - ->routeEntry() - ->opaqueConfig() - ->getBoolean("testbool")); - EXPECT_EQ(1234, config.route(genHeaders("api.lyft.com", "/", "GET"), 0) - ->routeEntry() - ->opaqueConfig() - ->getInteger("testnumber")); + const std::unordered_map& opaque_config = + config.route(genHeaders("api.lyft.com", "/api", "GET"), 0)->routeEntry()->opaqueConfig(); + + EXPECT_EQ("value1", opaque_config.find("name1")->second); + EXPECT_EQ("value2", opaque_config.find("name2")->second); } } // Router diff --git a/test/mocks/router/mocks.h b/test/mocks/router/mocks.h index 4a610dfc641ff..0c75965b384fa 100644 --- a/test/mocks/router/mocks.h +++ b/test/mocks/router/mocks.h @@ -148,7 +148,7 @@ class MockRouteEntry : public RouteEntry { MOCK_CONST_METHOD0(virtualHostName, const std::string&()); MOCK_CONST_METHOD0(virtualHost, const VirtualHost&()); MOCK_CONST_METHOD0(autoHostRewrite, bool()); - MOCK_CONST_METHOD0(opaqueConfig, const Json::ObjectPtr&()); + MOCK_CONST_METHOD0(opaqueConfig, const std::unordered_map&()); std::string cluster_name_{"fake_cluster"}; TestVirtualCluster virtual_cluster_; From f9d4c63085694c7d58fc98f6c85e5d1ec3aa12f6 Mon Sep 17 00:00:00 2001 From: Bill Gallagher Date: Thu, 23 Feb 2017 08:10:49 -0500 Subject: [PATCH 04/13] fix format --- test/common/router/config_impl_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index 7c78f367cfa3f..e18d0724f8cca 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -1655,7 +1655,7 @@ TEST(RouteMatcherTest, TestOpaqueConfig) { NiceMock cm; ConfigImpl config(*loader, runtime, cm, true); - const std::unordered_map& opaque_config = + const std::unordered_map& opaque_config = config.route(genHeaders("api.lyft.com", "/api", "GET"), 0)->routeEntry()->opaqueConfig(); EXPECT_EQ("value1", opaque_config.find("name1")->second); From 49f40b78da30da1658a75c96f92cac4d997d866d Mon Sep 17 00:00:00 2001 From: Bill Gallagher Date: Mon, 27 Feb 2017 06:26:41 -0500 Subject: [PATCH 05/13] review comments --- .../http_conn_man/route_config/route.rst | 4 ++-- include/envoy/router/router.h | 4 ++-- source/common/http/async_client_impl.cc | 2 +- source/common/http/async_client_impl.h | 4 ++-- source/common/json/config_schemas.cc | 15 +++------------ source/common/router/config_impl.cc | 13 +++++++------ source/common/router/config_impl.h | 8 ++++---- test/common/router/config_impl_test.cc | 10 +++++----- test/mocks/router/mocks.h | 2 +- 9 files changed, 27 insertions(+), 35 deletions(-) diff --git a/docs/configuration/http_conn_man/route_config/route.rst b/docs/configuration/http_conn_man/route_config/route.rst index d44d58f25bc30..da751c02b6d57 100644 --- a/docs/configuration/http_conn_man/route_config/route.rst +++ b/docs/configuration/http_conn_man/route_config/route.rst @@ -345,13 +345,13 @@ Opaque Config ------------- Additional configuration can be provided to filters through the "Opaque Config" mechanism. A -list of Key/Value pairs are specified in the route config. The configuration is uninterpreted +list of properties are specified in the route config. The configuration is uninterpreted by envoy and can be accessed within a user-defined filter. .. code-block:: json [ - {"name": "...", "value": "..."} + {"name": "value", ...} ] name diff --git a/include/envoy/router/router.h b/include/envoy/router/router.h index a56d84474df2e..05ad97306e119 100644 --- a/include/envoy/router/router.h +++ b/include/envoy/router/router.h @@ -227,10 +227,10 @@ class RouteEntry { virtual bool autoHostRewrite() const PURE; /** - * @return const std::unordered_map the opaque configuration associated + * @return const std::multimap the opaque configuration associated * with the route */ - virtual const std::unordered_map& opaqueConfig() const PURE; + virtual const std::multimap& opaqueConfig() const PURE; }; /** diff --git a/source/common/http/async_client_impl.cc b/source/common/http/async_client_impl.cc index 88061fef32931..8b3fb28f5476b 100644 --- a/source/common/http/async_client_impl.cc +++ b/source/common/http/async_client_impl.cc @@ -10,7 +10,7 @@ const AsyncStreamImpl::NullRetryPolicy AsyncStreamImpl::RouteEntryImpl::retry_po const AsyncStreamImpl::NullShadowPolicy AsyncStreamImpl::RouteEntryImpl::shadow_policy_; const AsyncStreamImpl::NullVirtualHost AsyncStreamImpl::RouteEntryImpl::virtual_host_; const AsyncStreamImpl::NullRateLimitPolicy AsyncStreamImpl::NullVirtualHost::rate_limit_policy_; -const std::unordered_map AsyncStreamImpl::RouteEntryImpl::opaque_config_; +const std::multimap AsyncStreamImpl::RouteEntryImpl::opaque_config_; AsyncClientImpl::AsyncClientImpl(const Upstream::ClusterInfo& cluster, Stats::Store& stats_store, Event::Dispatcher& dispatcher, diff --git a/source/common/http/async_client_impl.h b/source/common/http/async_client_impl.h index 0af0148bbf4cb..99a1976c38251 100644 --- a/source/common/http/async_client_impl.h +++ b/source/common/http/async_client_impl.h @@ -128,7 +128,7 @@ class AsyncStreamImpl : public AsyncClient::Stream, const Router::VirtualCluster* virtualCluster(const Http::HeaderMap&) const override { return nullptr; } - const std::unordered_map& opaqueConfig() const override { + const std::multimap& opaqueConfig() const override { return opaque_config_; } const Router::VirtualHost& virtualHost() const override { return virtual_host_; } @@ -138,7 +138,7 @@ class AsyncStreamImpl : public AsyncClient::Stream, static const NullRetryPolicy retry_policy_; static const NullShadowPolicy shadow_policy_; static const NullVirtualHost virtual_host_; - static const std::unordered_map opaque_config_; + static const std::multimap opaque_config_; const std::string& cluster_name_; Optional timeout_; diff --git a/source/common/json/config_schemas.cc b/source/common/json/config_schemas.cc index 74e97d537e0f8..2e96e4ea0f92a 100644 --- a/source/common/json/config_schemas.cc +++ b/source/common/json/config_schemas.cc @@ -545,18 +545,9 @@ const std::string Json::Schema::ROUTE_ENTRY_CONFIGURATION_SCHEMA(R"EOF( "required" : ["header_name"], "additionalProperties" : false }, - "opaque_config" : { - "type" : "array", - "minItems": 1, - "items" : { - "type" : "object", - "properties" : { - "name" : {"type" : "string"}, - "value" : {"type" : "string"} - }, - "required" : ["name", "value"], - "additionalProperties": false - } + "opaque_config" : { + "type" : "object", + "additionalProperties" : true } }, "additionalProperties" : false diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index 9b2821a5a9e9a..b6579b6871f37 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -250,14 +250,15 @@ std::string RouteEntryImplBase::newPath(const Http::HeaderMap& headers) const { final_path); } -std::unordered_map +std::multimap RouteEntryImplBase::parseOpaqueConfig(const Json::Object& route) { - std::unordered_map ret; + std::multimap ret; if (route.hasObject("opaque_config")) { - std::vector kvs = route.getObjectArray("opaque_config"); - for (const Json::ObjectPtr& kv : kvs) { - ret[kv->getString("name")] = kv->getString("value"); - } + Json::ObjectPtr obj = route.getObject("opaque_config"); + obj->iterate([&ret,&obj](const std::string& name, const Json::Object&) { + ret.emplace(name, obj->getString(name)); + return true; + }); } return ret; } diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h index 89fb2902fe620..d64a0769f9b9a 100644 --- a/source/common/router/config_impl.h +++ b/source/common/router/config_impl.h @@ -213,7 +213,7 @@ class RouteEntryImplBase : public RouteEntry, std::chrono::milliseconds timeout() const override { return timeout_; } const VirtualHost& virtualHost() const override { return vhost_; } bool autoHostRewrite() const override { return auto_host_rewrite_; } - const std::unordered_map& opaqueConfig() const override { + const std::multimap& opaqueConfig() const override { return opaque_config_; } @@ -261,7 +261,7 @@ class RouteEntryImplBase : public RouteEntry, return parent_->virtualCluster(headers); } - const std::unordered_map& opaqueConfig() const override { + const std::multimap& opaqueConfig() const override { return parent_->opaqueConfig(); } @@ -306,7 +306,7 @@ class RouteEntryImplBase : public RouteEntry, static Optional loadRuntimeData(const Json::Object& route); - static std::unordered_map parseOpaqueConfig(const Json::Object& route); + static std::multimap parseOpaqueConfig(const Json::Object& route); // Default timeout is 15s if nothing is specified in the route config. static const uint64_t DEFAULT_ROUTE_TIMEOUT_MS = 15000; @@ -327,7 +327,7 @@ class RouteEntryImplBase : public RouteEntry, std::vector config_headers_; std::vector weighted_clusters_; std::unique_ptr hash_policy_; - const std::unordered_map opaque_config_; + const std::multimap opaque_config_; }; /** diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index e18d0724f8cca..4dfb3381955ce 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -1639,10 +1639,10 @@ TEST(RouteMatcherTest, TestOpaqueConfig) { { "prefix": "/api", "cluster": "ats", - "opaque_config" : [ - { "name": "name1", "value": "value1" }, - { "name": "name2", "value": "value2" } - ] + "opaque_config" : { + "name1": "value1", + "name2": "value2" + } } ] } @@ -1655,7 +1655,7 @@ TEST(RouteMatcherTest, TestOpaqueConfig) { NiceMock cm; ConfigImpl config(*loader, runtime, cm, true); - const std::unordered_map& opaque_config = + const std::multimap& opaque_config = config.route(genHeaders("api.lyft.com", "/api", "GET"), 0)->routeEntry()->opaqueConfig(); EXPECT_EQ("value1", opaque_config.find("name1")->second); diff --git a/test/mocks/router/mocks.h b/test/mocks/router/mocks.h index 0c75965b384fa..3a35c5508ef87 100644 --- a/test/mocks/router/mocks.h +++ b/test/mocks/router/mocks.h @@ -148,7 +148,7 @@ class MockRouteEntry : public RouteEntry { MOCK_CONST_METHOD0(virtualHostName, const std::string&()); MOCK_CONST_METHOD0(virtualHost, const VirtualHost&()); MOCK_CONST_METHOD0(autoHostRewrite, bool()); - MOCK_CONST_METHOD0(opaqueConfig, const std::unordered_map&()); + MOCK_CONST_METHOD0(opaqueConfig, const std::multimap&()); std::string cluster_name_{"fake_cluster"}; TestVirtualCluster virtual_cluster_; From 05da56466ef67a56eec12b6583bf6567f2dcc0f7 Mon Sep 17 00:00:00 2001 From: Bill Gallagher Date: Mon, 27 Feb 2017 06:46:07 -0500 Subject: [PATCH 06/13] fix format --- source/common/router/config_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index b6579b6871f37..c89c9f95823ef 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -255,7 +255,7 @@ RouteEntryImplBase::parseOpaqueConfig(const Json::Object& route) { std::multimap ret; if (route.hasObject("opaque_config")) { Json::ObjectPtr obj = route.getObject("opaque_config"); - obj->iterate([&ret,&obj](const std::string& name, const Json::Object&) { + obj->iterate([&ret, &obj](const std::string& name, const Json::Object&) { ret.emplace(name, obj->getString(name)); return true; }); From 7cae4e1e80194bdeee6334befae3d1e2d97a167f Mon Sep 17 00:00:00 2001 From: Bill Gallagher Date: Mon, 27 Feb 2017 11:24:00 -0500 Subject: [PATCH 07/13] review comments --- docs/configuration/http_conn_man/route_config/route.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/configuration/http_conn_man/route_config/route.rst b/docs/configuration/http_conn_man/route_config/route.rst index da751c02b6d57..43cfacab1ffa7 100644 --- a/docs/configuration/http_conn_man/route_config/route.rst +++ b/docs/configuration/http_conn_man/route_config/route.rst @@ -351,7 +351,7 @@ by envoy and can be accessed within a user-defined filter. .. code-block:: json [ - {"name": "value", ...} + {"...": "..." } ] name From 380874a0aed186d56a670d197edfedf6123dda7a Mon Sep 17 00:00:00 2001 From: Bill Gallagher Date: Mon, 27 Feb 2017 11:25:31 -0500 Subject: [PATCH 08/13] review comments --- .../configuration/http_conn_man/route_config/route.rst | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/docs/configuration/http_conn_man/route_config/route.rst b/docs/configuration/http_conn_man/route_config/route.rst index 43cfacab1ffa7..3dae5eac6e75b 100644 --- a/docs/configuration/http_conn_man/route_config/route.rst +++ b/docs/configuration/http_conn_man/route_config/route.rst @@ -346,16 +346,12 @@ Opaque Config Additional configuration can be provided to filters through the "Opaque Config" mechanism. A list of properties are specified in the route config. The configuration is uninterpreted -by envoy and can be accessed within a user-defined filter. +by envoy and can be accessed within a user-defined filter. The configuration is a generic +string map. .. code-block:: json [ - {"...": "..." } + {"...": "..."} ] -name - *(required, string)* A user defined configuration name - -value - *(required, string)* A user defined configuration value From 67b8bae077d3b372e9d567c1c8c3ad281f000190 Mon Sep 17 00:00:00 2001 From: Bill Gallagher Date: Mon, 27 Feb 2017 11:25:50 -0500 Subject: [PATCH 09/13] review comments --- docs/configuration/http_conn_man/route_config/route.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/configuration/http_conn_man/route_config/route.rst b/docs/configuration/http_conn_man/route_config/route.rst index 3dae5eac6e75b..899217f5dd49d 100644 --- a/docs/configuration/http_conn_man/route_config/route.rst +++ b/docs/configuration/http_conn_man/route_config/route.rst @@ -344,9 +344,9 @@ header_name Opaque Config ------------- -Additional configuration can be provided to filters through the "Opaque Config" mechanism. A -list of properties are specified in the route config. The configuration is uninterpreted -by envoy and can be accessed within a user-defined filter. The configuration is a generic +Additional configuration can be provided to filters through the "Opaque Config" mechanism. A +list of properties are specified in the route config. The configuration is uninterpreted +by envoy and can be accessed within a user-defined filter. The configuration is a generic string map. .. code-block:: json From c55c3ca8357b461c2e9f86680893a03822fd439b Mon Sep 17 00:00:00 2001 From: Bill Gallagher Date: Mon, 27 Feb 2017 11:28:39 -0500 Subject: [PATCH 10/13] review comments --- docs/configuration/http_conn_man/route_config/route.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/configuration/http_conn_man/route_config/route.rst b/docs/configuration/http_conn_man/route_config/route.rst index 899217f5dd49d..9d366b06ae3aa 100644 --- a/docs/configuration/http_conn_man/route_config/route.rst +++ b/docs/configuration/http_conn_man/route_config/route.rst @@ -347,7 +347,7 @@ Opaque Config Additional configuration can be provided to filters through the "Opaque Config" mechanism. A list of properties are specified in the route config. The configuration is uninterpreted by envoy and can be accessed within a user-defined filter. The configuration is a generic -string map. +string map. Nested objects are not supported. .. code-block:: json From 82f0f6fa193e594b8b6d2a178a09d740910cf705 Mon Sep 17 00:00:00 2001 From: Bill Gallagher Date: Mon, 27 Feb 2017 11:32:31 -0500 Subject: [PATCH 11/13] update test --- include/envoy/json/json_object.h | 5 +++++ source/common/json/json_loader.cc | 7 +++++++ source/common/router/config_impl.cc | 4 ++-- test/common/router/config_impl_test.cc | 11 +++++++++-- 4 files changed, 23 insertions(+), 4 deletions(-) diff --git a/include/envoy/json/json_object.h b/include/envoy/json/json_object.h index a00a84ad37626..4059c6a052ee8 100644 --- a/include/envoy/json/json_object.h +++ b/include/envoy/json/json_object.h @@ -142,6 +142,11 @@ class Object { */ virtual void validateSchema(const std::string& schema) const PURE; + /** + * @return the value of the object as a string + */ + virtual std::string asString() const PURE; + /** * @return true if the JSON object is empty; */ diff --git a/source/common/json/json_loader.cc b/source/common/json/json_loader.cc index 532210fdce60d..bb1a5a7d019f9 100644 --- a/source/common/json/json_loader.cc +++ b/source/common/json/json_loader.cc @@ -182,6 +182,13 @@ class ObjectImplBase : public Object { } } + std::string asString() const { + if (!value_.IsString()) { + throw Exception(fmt::format("'{}' is not a string", name_)); + } + return value_.GetString(); + } + bool empty() const override { return value_.IsObject() && value_.ObjectEmpty(); } private: diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index c89c9f95823ef..d80825b185d88 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -255,8 +255,8 @@ RouteEntryImplBase::parseOpaqueConfig(const Json::Object& route) { std::multimap ret; if (route.hasObject("opaque_config")) { Json::ObjectPtr obj = route.getObject("opaque_config"); - obj->iterate([&ret, &obj](const std::string& name, const Json::Object&) { - ret.emplace(name, obj->getString(name)); + obj->iterate([&ret, &obj](const std::string& name, const Json::Object& value) { + ret.emplace(name, value.asString()); return true; }); } diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index 4dfb3381955ce..2a286f14eb2a8 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -1641,7 +1641,8 @@ TEST(RouteMatcherTest, TestOpaqueConfig) { "cluster": "ats", "opaque_config" : { "name1": "value1", - "name2": "value2" + "name2": "value2", + "name1": "value3" } } ] @@ -1658,7 +1659,13 @@ TEST(RouteMatcherTest, TestOpaqueConfig) { const std::multimap& opaque_config = config.route(genHeaders("api.lyft.com", "/api", "GET"), 0)->routeEntry()->opaqueConfig(); - EXPECT_EQ("value1", opaque_config.find("name1")->second); + EXPECT_EQ(2u, opaque_config.count("name1")); + auto range = opaque_config.equal_range("name1"); + auto it = range.first; + EXPECT_EQ("value1", it->second); + ++it; + EXPECT_EQ("value3", it->second); + EXPECT_EQ("value2", opaque_config.find("name2")->second); } From 19706a0c8467d4d3583a3b00b410638464a3c13a Mon Sep 17 00:00:00 2001 From: Bill Gallagher Date: Tue, 28 Feb 2017 09:19:05 -0500 Subject: [PATCH 12/13] add test for asString --- test/common/json/json_loader_test.cc | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/test/common/json/json_loader_test.cc b/test/common/json/json_loader_test.cc index 556c818a559b8..4f5159a1fc4ce 100644 --- a/test/common/json/json_loader_test.cc +++ b/test/common/json/json_loader_test.cc @@ -208,4 +208,18 @@ TEST(JsonLoaderTest, Schema) { EXPECT_NO_THROW(json->validateSchema(valid_schema)); } +TEST(JsonLoaderTest, AsString) { + ObjectPtr json = Factory::LoadFromString("{\"name1\": \"value1\", \"name2\": true}"); + json->iterate([&](const std::string& key, const Json::Object& value) { + EXPECT_TRUE(key == "name1" || key == "name2"); + + if (key == "name1") { + EXPECT_EQ("value1", value.asString()); + } else { + EXPECT_THROW(value.asString(), Exception); + } + return true; + }); +} + } // Json From 8861f39331115742e0f95b60e1fb3d4d8da0a1af Mon Sep 17 00:00:00 2001 From: Bill Gallagher Date: Tue, 28 Feb 2017 09:21:15 -0500 Subject: [PATCH 13/13] fix format --- include/envoy/json/json_object.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/envoy/json/json_object.h b/include/envoy/json/json_object.h index 4059c6a052ee8..e4daf77eb7089 100644 --- a/include/envoy/json/json_object.h +++ b/include/envoy/json/json_object.h @@ -143,7 +143,7 @@ class Object { virtual void validateSchema(const std::string& schema) const PURE; /** - * @return the value of the object as a string + * @return the value of the object as a string */ virtual std::string asString() const PURE;