Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

router: vhost/route/w.cluster local filter configuration #3045

Merged
merged 14 commits into from
Apr 16, 2018
Merged

router: vhost/route/w.cluster local filter configuration #3045

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
636e605
Router: Route/VHost/W.Cluster local filter configuration
Apr 6, 2018
b2d0263
format
Apr 10, 2018
134734b
missed async client types
Apr 10, 2018
0cc834f
fix repo locations
Apr 11, 2018
35f37f7
s/RDS/Route/
Apr 11, 2018
4820c48
remove unneccessary template var
Apr 11, 2018
d76ad4d
the nullptr likes to be called just nullptr
Apr 11, 2018
6309d41
checkpoint
Apr 12, 2018
9897138
s/ASSERT/EXPECT/
Apr 12, 2018
118c728
dependency shuffle
Apr 12, 2018
711f59f
format + convert test to use fixtures with dummy filter
Apr 12, 2018
472f7ae
Merge branch 'master' into route-local-config
rodaine Apr 13, 2018
c2f915b
comment nits
Apr 16, 2018
cbd19d4
Merge branch 'master' into route-local-config
rodaine Apr 16, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions include/envoy/router/router.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
* nullptr is returned.
*/
virtual const Protobuf::Message* perFilterConfig(const std::string& name) const PURE;
Copy link
Contributor

Choose a reason for hiding this comment

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

This suggestion is trying to address this issue: #3008
We can do it in a separate PR. But it is so simple, you can also put in in this PR too.

In these VHost/Route/Cluster, add another function

// If perFitlerConfig exists for this filter, get its config hash value to detect if config is updated.
// return 0 if no config for the filter.
size_t perFitlerConfigHash(const std::string& name) const PURE;

Copy link
Member

Choose a reason for hiding this comment

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

I don't quite understand how this will help you. In the filter are you going to check this on a per-route basis? What you really want is another callback which allows a filter to update global structures on a route update.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is to help filters need to pre-process per_route_config. per_route_config can be dynamically changed anytime. But you don't want to pre-process the config for each request. This hash will help to detect config change, and skip pre_processing step:

This is how it can be used, For each request in decodeHeader()

auto hash = route()->perFilterConfigHash("my_filter_name");
if (hash == 0) // handle not per_route_config case

 obj =  global_obj_map.find(hash);
 if (obj == null) {
         obj = pre_process_config( route()->perFilterConfig("my_filter_name") );
         global_obj_map[hash] = obj;
 }

This will also achieve: different routes with same config can share the same pre_processed object.

Copy link
Member

Choose a reason for hiding this comment

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

This is not how I would recommend implementing this. (The above is not thread safe.). I have a pretty good idea of how to efficiently implement this and give you the functionality you need. Let's do this in a follow up.

Copy link
Contributor

@qiwzhang qiwzhang Apr 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

In istio mixer-client filter, the "global_map" actually is per_thread_global_map. so this approach works for us.
the filter_factory callback approach is better, the global_map can be used by all worker threads. But it is not easy for Istio to use it since its "object_map" object is way deep inside a per_thread object.
My request is: can we support both: 1) calculate the hash of per_route_config at config update time and filter can access it. 2) implements factory callback to support a config change update.

};

/**
Expand Down Expand Up @@ -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,
* nullptr is returned.
*/
virtual const Protobuf::Message* perFilterConfig(const std::string& name) const PURE;
};

/**
Expand Down Expand Up @@ -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, nullptr is
* returned.
*/
virtual const Protobuf::Message* perFilterConfig(const std::string& name) const PURE;
};

typedef std::shared_ptr<const Route> RouteConstSharedPtr;
Expand Down
11 changes: 11 additions & 0 deletions include/envoy/server/filter_config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,17 @@ 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 createEmptyRouteConfigProto() {
return createEmptyConfigProto();
}

/**
* @return std::string the identifying name for a particular implementation of an http filter
* produced by the factory.
Expand Down
23 changes: 23 additions & 0 deletions source/common/config/utility.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,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);

Expand All @@ -235,6 +236,28 @@ 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 <class Factory>
static ProtobufTypes::MessagePtr translateToFactoryRouteConfig(const Protobuf::Message& source,
Factory& factory) {
ProtobufTypes::MessagePtr config = factory.createEmptyRouteConfigProto();

// Fail in an obvious way if a plugin does not return a proto.
RELEASE_ASSERT(config != nullptr);

MessageUtil::jsonConvert(source, *config);
return config;
}

/**
* Create TagProducer instance. Check all tag names for conflicts to avoid
* unexpected tag name overwriting.
Expand Down
4 changes: 4 additions & 0 deletions source/common/http/async_client_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_;
Expand Down Expand Up @@ -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_;
Expand All @@ -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_;
};
Expand Down
2 changes: 2 additions & 0 deletions source/common/router/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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", # TODO(rodaine): break dependency on server
"//include/envoy/upstream:cluster_manager_interface",
"//include/envoy/upstream:upstream_interface",
"//source/common/common:assert_lib",
Expand All @@ -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",
Expand Down
46 changes: 43 additions & 3 deletions source/common/router/config_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

#include "envoy/http/header_map.h"
#include "envoy/runtime/runtime.h"
#include "envoy/server/filter_config.h" // TODO(rodaine): break dependency on server
#include "envoy/upstream/cluster_manager.h"
#include "envoy/upstream/upstream.h"

Expand All @@ -21,6 +22,8 @@
#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"
#include "common/protobuf/utility.h"
Expand Down Expand Up @@ -244,7 +247,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);
Expand Down Expand Up @@ -520,14 +524,19 @@ 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)
: 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())) {
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);
Expand All @@ -542,6 +551,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)
Expand Down Expand Up @@ -652,7 +670,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;
Expand Down Expand Up @@ -716,6 +735,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)
Expand Down Expand Up @@ -855,5 +878,22 @@ ConfigImpl::ConfigImpl(const envoy::api::v2::RouteConfiguration& config, Runtime
config.response_headers_to_remove());
}

PerFilterConfigs::PerFilterConfigs(const Protobuf::Map<std::string, ProtobufWkt::Struct>& configs) {
for (const auto& cfg : configs) {
const std::string& name = cfg.first;
const ProtobufWkt::Struct& struct_config = cfg.second;

auto& factory = Envoy::Config::Utility::getAndCheckFactory<
Server::Configuration::NamedHttpFilterConfigFactory>(name);

configs_[name] = Envoy::Config::Utility::translateToFactoryRouteConfig(struct_config, factory);
Copy link
Contributor

@qiwzhang qiwzhang Apr 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

create another map

config_hash_ to store hash of config

The hash can be calculated by
https://github.com/envoyproxy/envoy/blob/master/source/common/protobuf/utility.h#L127

Copy link
Member

Choose a reason for hiding this comment

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

Same comment as above. I don't think this is very helpful and I would rather sort out filter update callbacks as a separate feature?

}
}

const Protobuf::Message* PerFilterConfigs::get(const std::string& name) const {
auto cfg = configs_.find(name);
return cfg == configs_.end() ? nullptr : cfg->second.get();
}

} // namespace Router
} // namespace Envoy
22 changes: 22 additions & 0 deletions source/common/router/config_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,16 @@ class Matchable {
uint64_t random_value) const PURE;
};

class PerFilterConfigs {
public:
PerFilterConfigs(const Protobuf::Map<std::string, ProtobufWkt::Struct>& configs);

const Protobuf::Message* get(const std::string& name) const;

private:
std::unordered_map<std::string, ProtobufTypes::MessagePtr> configs_;
};

class RouteEntryImplBase;
typedef std::shared_ptr<const RouteEntryImplBase> RouteEntryImplBaseConstSharedPtr;

Expand All @@ -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;
Expand Down Expand Up @@ -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 };
Expand Down Expand Up @@ -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<VirtualHostImpl> VirtualHostSharedPtr;
Expand Down Expand Up @@ -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_;
Expand Down Expand Up @@ -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_;
Expand Down Expand Up @@ -469,13 +487,16 @@ 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_;
const uint64_t cluster_weight_;
MetadataMatchCriteriaImplConstPtr cluster_metadata_match_criteria_;
HeaderParserPtr request_headers_parser_;
HeaderParserPtr response_headers_parser_;
PerFilterConfigs per_filter_configs_;
};

typedef std::shared_ptr<WeightedClusterEntry> WeightedClusterEntrySharedPtr;
Expand Down Expand Up @@ -527,6 +548,7 @@ class RouteEntryImplBase : public RouteEntry,
const DecoratorConstPtr decorator_;
const absl::optional<Http::Code> direct_response_code_;
std::string direct_response_body_;
PerFilterConfigs per_filter_configs_;
};

/**
Expand Down
2 changes: 2 additions & 0 deletions test/common/router/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,11 @@ envoy_cc_test(
"//source/common/http:headers_lib",
"//source/common/json:json_loader_lib",
"//source/common/router:config_lib",
"//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",
"//test/test_common:registry_lib",
"//test/test_common:utility_lib",
],
)
Expand Down
Loading