From 96c645cf5119934aadfdfad4d50622a081fc6ac3 Mon Sep 17 00:00:00 2001 From: Stephan Zuercher Date: Tue, 13 Mar 2018 11:43:46 -0700 Subject: [PATCH] router: support for request/response header manipulation in weighted clusters (#2765) Implements the request_headers_to_add, response_headers_to_add, and response_headers_to_remove fields added to weighted clusters by envoyproxy/data-plane-api#441. Risk Level: Low - no change in behavior without configuration changes Testing: unit and integration tests Docs Changes: envoyproxy/data-plane-api#531 Release Notes: updated Fixes: #2455 Signed-off-by: Stephan Zuercher stephan@turbinelabs.io --- RAW_RELEASE_NOTES.md | 1 + source/common/router/config_impl.cc | 44 ++--- source/common/router/config_impl.h | 22 ++- test/common/router/config_impl_test.cc | 68 ++++++++ test/integration/header_integration_test.cc | 177 ++++++++++++++++++-- 5 files changed, 271 insertions(+), 41 deletions(-) diff --git a/RAW_RELEASE_NOTES.md b/RAW_RELEASE_NOTES.md index c0457efb942c..de5d2ba6dc6f 100644 --- a/RAW_RELEASE_NOTES.md +++ b/RAW_RELEASE_NOTES.md @@ -76,4 +76,5 @@ final version. * Added support for stripping query string for redirects. * Added support for specifying a metadata matcher for upstream clusters in the tcp filter * Added support for listening on UNIX domain sockets. +* Added support for downstream request/upstream response header manipulation in weighted cluster. * Added support for range based header matching for request routing. diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index 136f48aac22d..f16175ad4943 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -269,26 +269,8 @@ RouteEntryImplBase::RouteEntryImplBase(const VirtualHostImpl& vhost, const std::string& runtime_key_prefix = route.route().weighted_clusters().runtime_key_prefix(); for (const auto& cluster : route.route().weighted_clusters().clusters()) { - const std::string& cluster_name = cluster.name(); - - MetadataMatchCriteriaImplConstPtr cluster_metadata_match_criteria; - if (cluster.has_metadata_match()) { - const auto filter_it = cluster.metadata_match().filter_metadata().find( - Envoy::Config::MetadataFilters::get().ENVOY_LB); - if (filter_it != cluster.metadata_match().filter_metadata().end()) { - if (metadata_match_criteria_) { - cluster_metadata_match_criteria = - metadata_match_criteria_->mergeMatchCriteria(filter_it->second); - } else { - cluster_metadata_match_criteria.reset(new MetadataMatchCriteriaImpl(filter_it->second)); - } - } - } - - std::unique_ptr cluster_entry( - new WeightedClusterEntry(this, runtime_key_prefix + "." + cluster_name, loader_, - cluster_name, PROTOBUF_GET_WRAPPED_REQUIRED(cluster, weight), - std::move(cluster_metadata_match_criteria))); + std::unique_ptr cluster_entry(new WeightedClusterEntry( + this, runtime_key_prefix + "." + cluster.name(), loader_, cluster)); weighted_clusters_.emplace_back(std::move(cluster_entry)); total_weight += weighted_clusters_.back()->clusterWeight(); } @@ -537,6 +519,28 @@ void RouteEntryImplBase::validateClusters(Upstream::ClusterManager& cm) const { } } +RouteEntryImplBase::WeightedClusterEntry::WeightedClusterEntry( + const RouteEntryImplBase* parent, const std::string runtime_key, Runtime::Loader& loader, + const envoy::api::v2::route::WeightedCluster_ClusterWeight& cluster) + : DynamicRouteEntry(parent, cluster.name()), runtime_key_(runtime_key), loader_(loader), + 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())) { + if (cluster.has_metadata_match()) { + const auto filter_it = cluster.metadata_match().filter_metadata().find( + Envoy::Config::MetadataFilters::get().ENVOY_LB); + if (filter_it != cluster.metadata_match().filter_metadata().end()) { + if (parent->metadata_match_criteria_) { + cluster_metadata_match_criteria_ = + parent->metadata_match_criteria_->mergeMatchCriteria(filter_it->second); + } else { + cluster_metadata_match_criteria_.reset(new MetadataMatchCriteriaImpl(filter_it->second)); + } + } + } +} + PrefixRouteEntryImpl::PrefixRouteEntryImpl(const VirtualHostImpl& vhost, const envoy::api::v2::route::Route& route, Runtime::Loader& loader) diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h index 5973f4cfe6f8..359df2ce2f6f 100644 --- a/source/common/router/config_impl.h +++ b/source/common/router/config_impl.h @@ -443,12 +443,9 @@ class RouteEntryImplBase : public RouteEntry, */ class WeightedClusterEntry : public DynamicRouteEntry { public: - WeightedClusterEntry(const RouteEntryImplBase* parent, const std::string runtime_key, - Runtime::Loader& loader, const std::string& name, uint64_t weight, - MetadataMatchCriteriaImplConstPtr cluster_metadata_match_criteria) - : DynamicRouteEntry(parent, name), runtime_key_(runtime_key), loader_(loader), - cluster_weight_(weight), - cluster_metadata_match_criteria_(std::move(cluster_metadata_match_criteria)) {} + WeightedClusterEntry(const RouteEntryImplBase* parent, const std::string rutime_key, + Runtime::Loader& loader, + const envoy::api::v2::route::WeightedCluster_ClusterWeight& cluster); uint64_t clusterWeight() const { return loader_.snapshot().getInteger(runtime_key_, cluster_weight_); @@ -461,11 +458,24 @@ class RouteEntryImplBase : public RouteEntry, return DynamicRouteEntry::metadataMatchCriteria(); } + void finalizeRequestHeaders(Http::HeaderMap& headers, + const RequestInfo::RequestInfo& request_info) const override { + request_headers_parser_->evaluateHeaders(headers, request_info); + DynamicRouteEntry::finalizeRequestHeaders(headers, request_info); + } + void finalizeResponseHeaders(Http::HeaderMap& headers, + const RequestInfo::RequestInfo& request_info) const override { + response_headers_parser_->evaluateHeaders(headers, request_info); + DynamicRouteEntry::finalizeResponseHeaders(headers, request_info); + } + private: const std::string runtime_key_; Runtime::Loader& loader_; const uint64_t cluster_weight_; MetadataMatchCriteriaImplConstPtr cluster_metadata_match_criteria_; + HeaderParserPtr request_headers_parser_; + HeaderParserPtr response_headers_parser_; }; typedef std::shared_ptr WeightedClusterEntrySharedPtr; diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index a3179791f9b4..946efde942b1 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -2865,6 +2865,74 @@ TEST(RouteMatcherTest, TestWeightedClusterInvalidClusterName) { EnvoyException); } +TEST(RouteMatcherTest, TestWeightedClusterHeaderManipulation) { + std::string yaml = R"EOF( +virtual_hosts: + - name: www2 + domains: ["www.lyft.com"] + routes: + - match: { prefix: "/" } + route: + weighted_clusters: + clusters: + - name: cluster1 + weight: 50 + request_headers_to_add: + - header: + key: x-req-cluster + value: cluster1 + response_headers_to_add: + - header: + key: x-resp-cluster + value: cluster1 + response_headers_to_remove: [ "x-remove-cluster1" ] + - name: cluster2 + weight: 50 + request_headers_to_add: + - header: + key: x-req-cluster + value: cluster2 + response_headers_to_add: + - header: + key: x-resp-cluster + value: cluster2 + response_headers_to_remove: [ "x-remove-cluster2" ] + )EOF"; + + NiceMock runtime; + NiceMock cm; + ConfigImpl config(parseRouteConfigurationFromV2Yaml(yaml), runtime, cm, true); + NiceMock request_info; + + { + Http::TestHeaderMapImpl headers = genHeaders("www.lyft.com", "/foo", "GET"); + Http::TestHeaderMapImpl resp_headers({{"x-remove-cluster1", "value"}}); + const RouteEntry* route = config.route(headers, 0)->routeEntry(); + EXPECT_EQ("cluster1", route->clusterName()); + + route->finalizeRequestHeaders(headers, request_info); + EXPECT_EQ("cluster1", headers.get_("x-req-cluster")); + + route->finalizeResponseHeaders(resp_headers, request_info); + EXPECT_EQ("cluster1", resp_headers.get_("x-resp-cluster")); + EXPECT_FALSE(resp_headers.has("x-remove-cluster1")); + } + + { + Http::TestHeaderMapImpl headers = genHeaders("www.lyft.com", "/foo", "GET"); + Http::TestHeaderMapImpl resp_headers({{"x-remove-cluster2", "value"}}); + const RouteEntry* route = config.route(headers, 55)->routeEntry(); + EXPECT_EQ("cluster2", route->clusterName()); + + route->finalizeRequestHeaders(headers, request_info); + EXPECT_EQ("cluster2", headers.get_("x-req-cluster")); + + route->finalizeResponseHeaders(resp_headers, request_info); + EXPECT_EQ("cluster2", resp_headers.get_("x-resp-cluster")); + EXPECT_FALSE(resp_headers.has("x-remove-cluster2")); + } +} + TEST(NullConfigImplTest, All) { NullConfigImpl config; Http::TestHeaderMapImpl headers = genRedirectHeaders("redirect.lyft.com", "/baz", true, false); diff --git a/test/integration/header_integration_test.cc b/test/integration/header_integration_test.cc index d352fba9dc86..f04170a2ec83 100644 --- a/test/integration/header_integration_test.cc +++ b/test/integration/header_integration_test.cc @@ -57,6 +57,30 @@ stat_prefix: header_test key: "x-route-response" value: "route" response_headers_to_remove: ["x-route-response-remove"] + - match: { prefix: "/vhost-route-and-weighted-clusters" } + route: + request_headers_to_add: + - header: + key: "x-route-request" + value: "route" + response_headers_to_add: + - header: + key: "x-route-response" + value: "route" + response_headers_to_remove: ["x-route-response-remove"] + weighted_clusters: + clusters: + - name: cluster_0 + weight: 100 + request_headers_to_add: + - header: + key: "x-weighted-cluster-request" + value: "weighted-cluster-1" + response_headers_to_add: + - header: + key: "x-weighted-cluster-response" + value: "weighted-cluster-1" + response_headers_to_remove: ["x-weighted-cluster-response-remove"] - name: route-headers domains: ["route-headers.com"] routes: @@ -154,7 +178,7 @@ class HeaderIntegrationTest : public HttpIntegrationTest, use_eds_ = true; } - void initializeFilter(HeaderMode mode, bool include_route_config_headers) { + void initializeFilter(HeaderMode mode, bool inject_route_config_headers) { config_helper_.addConfigModifier( [&](envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager& hcm) { @@ -164,7 +188,7 @@ class HeaderIntegrationTest : public HttpIntegrationTest, const bool append = mode == HeaderMode::Append; auto* route_config = hcm.mutable_route_config(); - if (include_route_config_headers) { + if (inject_route_config_headers) { // Configure route config level headers. addHeader(route_config->mutable_response_headers_to_add(), "x-routeconfig-response", "routeconfig", append); @@ -177,16 +201,25 @@ class HeaderIntegrationTest : public HttpIntegrationTest, addHeader(route_config->mutable_response_headers_to_add(), "x-routeconfig-dynamic", "%UPSTREAM_METADATA([\"test.namespace\", \"key\"])%", append); - // Iterate over VirtualHosts and nested Routes, adding a dynamic response header. + // Iterate over VirtualHosts, nested Routes and WeightedClusters, adding a dynamic + // response header. for (auto& vhost : *route_config->mutable_virtual_hosts()) { addHeader(vhost.mutable_response_headers_to_add(), "x-vhost-dynamic", - "%UPSTREAM_METADATA([\"test.namespace\", \"key\"])%", append); + "vhost:%UPSTREAM_METADATA([\"test.namespace\", \"key\"])%", append); for (auto& rte : *vhost.mutable_routes()) { if (rte.has_route()) { - addHeader(rte.mutable_route()->mutable_response_headers_to_add(), - "x-route-dynamic", "%UPSTREAM_METADATA([\"test.namespace\", \"key\"])%", - append); + auto* mutable_rte = rte.mutable_route(); + addHeader(mutable_rte->mutable_response_headers_to_add(), "x-route-dynamic", + "route:%UPSTREAM_METADATA([\"test.namespace\", \"key\"])%", append); + + if (mutable_rte->has_weighted_clusters()) { + for (auto& c : *mutable_rte->mutable_weighted_clusters()->mutable_clusters()) { + addHeader(c.mutable_response_headers_to_add(), "x-weighted-cluster-dynamic", + "weighted:%UPSTREAM_METADATA([\"test.namespace\", \"key\"])%", + append); + } + } } } } @@ -204,10 +237,17 @@ class HeaderIntegrationTest : public HttpIntegrationTest, for (auto& rte : *vhost.mutable_routes()) { if (rte.has_route()) { - disableHeaderValueOptionAppend( - *rte.mutable_route()->mutable_request_headers_to_add()); - disableHeaderValueOptionAppend( - *rte.mutable_route()->mutable_response_headers_to_add()); + auto* mutable_rte = rte.mutable_route(); + + disableHeaderValueOptionAppend(*mutable_rte->mutable_request_headers_to_add()); + disableHeaderValueOptionAppend(*mutable_rte->mutable_response_headers_to_add()); + + if (mutable_rte->has_weighted_clusters()) { + for (auto& c : *mutable_rte->mutable_weighted_clusters()->mutable_clusters()) { + disableHeaderValueOptionAppend(*c.mutable_request_headers_to_add()); + disableHeaderValueOptionAppend(*c.mutable_response_headers_to_add()); + } + } } } } @@ -643,6 +683,108 @@ TEST_P(HeaderIntegrationTest, TestRouteConfigVirtualHostAndRouteReplaceHeaderMan }); } +// Validates the relationship between route configuration, virtual host, route, and weighted +// cluster header manipulations when appending. +TEST_P(HeaderIntegrationTest, TestRouteConfigVirtualHostRouteAndClusterAppendHeaderManipulation) { + initializeFilter(HeaderMode::Append, true); + performRequest( + Http::TestHeaderMapImpl{ + {":method", "GET"}, + {":path", "/vhost-route-and-weighted-clusters"}, + {":scheme", "http"}, + {":authority", "vhost-headers.com"}, + {"x-routeconfig-request", "downstream"}, + {"x-vhost-request", "downstream"}, + {"x-route-request", "downstream"}, + {"x-weighted-cluster-request", "downstream"}, + }, + Http::TestHeaderMapImpl{ + {":authority", "vhost-headers.com"}, + {"x-routeconfig-request", "downstream"}, + {"x-vhost-request", "downstream"}, + {"x-route-request", "downstream"}, + {"x-weighted-cluster-request", "downstream"}, + {"x-weighted-cluster-request", "weighted-cluster-1"}, + {"x-route-request", "route"}, + {"x-vhost-request", "vhost"}, + {"x-routeconfig-request", "routeconfig"}, + {":path", "/vhost-route-and-weighted-clusters"}, + {":method", "GET"}, + }, + Http::TestHeaderMapImpl{ + {"server", "envoy"}, + {"content-length", "0"}, + {":status", "200"}, + {"x-routeconfig-response", "upstream"}, + {"x-routeconfig-response-remove", "upstream"}, + {"x-vhost-response", "upstream"}, + {"x-vhost-response-remove", "upstream"}, + {"x-route-response", "upstream"}, + {"x-route-response-remove", "upstream"}, + {"x-weighted-cluster-response", "upstream"}, + {"x-weighted-cluster-response-remove", "upstream"}, + }, + Http::TestHeaderMapImpl{ + {"server", "envoy"}, + {"x-routeconfig-response", "upstream"}, + {"x-vhost-response", "upstream"}, + {"x-route-response", "upstream"}, + {"x-weighted-cluster-response", "upstream"}, + {"x-weighted-cluster-response", "weighted-cluster-1"}, + {"x-route-response", "route"}, + {"x-vhost-response", "vhost"}, + {"x-routeconfig-response", "routeconfig"}, + {":status", "200"}, + }); +} + +// Validates the relationship between route configuration, virtual host, route and weighted cluster +// header manipulations when replacing. +TEST_P(HeaderIntegrationTest, TestRouteConfigVirtualHostRouteAndClusterReplaceHeaderManipulation) { + initializeFilter(HeaderMode::Replace, true); + performRequest( + Http::TestHeaderMapImpl{ + {":method", "GET"}, + {":path", "/vhost-route-and-weighted-clusters"}, + {":scheme", "http"}, + {":authority", "vhost-headers.com"}, + {"x-routeconfig-request", "downstream"}, + {"x-vhost-request", "downstream"}, + {"x-route-request", "downstream"}, + {"x-weighted-cluster-request", "downstream"}, + {"x-unmodified", "request"}, + }, + Http::TestHeaderMapImpl{ + {":authority", "vhost-headers.com"}, + {"x-unmodified", "request"}, + {"x-weighted-cluster-request", "weighted-cluster-1"}, + {"x-route-request", "route"}, + {"x-vhost-request", "vhost"}, + {"x-routeconfig-request", "routeconfig"}, + {":path", "/vhost-route-and-weighted-clusters"}, + {":method", "GET"}, + }, + Http::TestHeaderMapImpl{ + {"server", "envoy"}, + {"content-length", "0"}, + {":status", "200"}, + {"x-routeconfig-response", "upstream"}, + {"x-vhost-response", "upstream"}, + {"x-route-response", "upstream"}, + {"x-weighted-cluster-response", "upstream"}, + {"x-unmodified", "response"}, + }, + Http::TestHeaderMapImpl{ + {"server", "envoy"}, + {"x-unmodified", "response"}, + {"x-weighted-cluster-response", "weighted-cluster-1"}, + {"x-route-response", "route"}, + {"x-vhost-response", "vhost"}, + {"x-routeconfig-response", "routeconfig"}, + {":status", "200"}, + }); +} + // Validates that upstream host metadata can be emitted in headers. TEST_P(HeaderIntegrationTest, TestDynamicHeaders) { prepareEDS(); @@ -650,21 +792,23 @@ TEST_P(HeaderIntegrationTest, TestDynamicHeaders) { performRequest( Http::TestHeaderMapImpl{ {":method", "GET"}, - {":path", "/vhost-and-route"}, + {":path", "/vhost-route-and-weighted-clusters"}, {":scheme", "http"}, {":authority", "vhost-headers.com"}, {"x-routeconfig-request", "downstream"}, {"x-vhost-request", "downstream"}, {"x-route-request", "downstream"}, + {"x-weighted-cluster-request", "downstream"}, {"x-unmodified", "request"}, }, Http::TestHeaderMapImpl{ {":authority", "vhost-headers.com"}, {"x-unmodified", "request"}, + {"x-weighted-cluster-request", "weighted-cluster-1"}, {"x-route-request", "route"}, {"x-vhost-request", "vhost"}, {"x-routeconfig-request", "routeconfig"}, - {":path", "/vhost-and-route"}, + {":path", "/vhost-route-and-weighted-clusters"}, {":method", "GET"}, }, Http::TestHeaderMapImpl{ @@ -674,15 +818,18 @@ TEST_P(HeaderIntegrationTest, TestDynamicHeaders) { {"x-routeconfig-response", "upstream"}, {"x-vhost-response", "upstream"}, {"x-route-response", "upstream"}, + {"x-weighted-cluster-response", "upstream"}, {"x-unmodified", "response"}, }, Http::TestHeaderMapImpl{ {"server", "envoy"}, {"x-unmodified", "response"}, + {"x-weighted-cluster-response", "weighted-cluster-1"}, + {"x-weighted-cluster-dynamic", "weighted:metadata-value"}, {"x-route-response", "route"}, - {"x-route-dynamic", "metadata-value"}, + {"x-route-dynamic", "route:metadata-value"}, {"x-vhost-response", "vhost"}, - {"x-vhost-dynamic", "metadata-value"}, + {"x-vhost-dynamic", "vhost:metadata-value"}, {"x-routeconfig-response", "routeconfig"}, {"x-routeconfig-dynamic", "metadata-value"}, {":status", "200"},