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

upstream: allow extension protocol options to be used with http filters #4165

Merged
merged 5 commits into from
Aug 20, 2018
Merged

upstream: allow extension protocol options to be used with http filters #4165

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
53345c7
allow extension protocol options to be used with http filters
yuval-k Aug 15, 2018
014bf2e
move default extension impl to base class
yuval-k Aug 16, 2018
fe502fc
fix exception string
yuval-k Aug 16, 2018
fab1d2e
ProtocolOptionsConsumer -> ProtocolOptionsFactory
yuval-k Aug 16, 2018
b5a1a6d
add comments to ExtensionProtocolOptionsForFilterWithOptions
yuval-k Aug 17, 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
51 changes: 30 additions & 21 deletions include/envoy/server/filter_config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,11 +181,39 @@ class NamedListenerFilterConfigFactory {
virtual std::string name() PURE;
};

/**
* Implemented by filter factories that require more options to process the protocol used by the
* upstream cluster.
*/
class ProtocolOptionsFactory {
public:
virtual ~ProtocolOptionsFactory() {}

/**
* Create a particular filter's protocol specific options implementation. If the factory
* implementation is unable to produce a factory with the provided parameters, it should throw an
* EnvoyException.
* @param config supplies the protobuf configuration for the filter
* @return Upstream::ProtocoOptionsConfigConstSharedPtr the protocol options
*/
virtual Upstream::ProtocolOptionsConfigConstSharedPtr
createProtocolOptionsConfig(const Protobuf::Message& config) {
UNREFERENCED_PARAMETER(config);
return nullptr;
}

/**
* @return ProtobufTypes::MessagePtr a newly created empty protocol specific options message or
* nullptr if protocol specific options are not available.
*/
virtual ProtobufTypes::MessagePtr createEmptyProtocolOptionsProto() { return nullptr; }
};

/**
* Implemented by each network filter and registered via Registry::registerFactory()
* or the convenience class RegisterFactory.
*/
class NamedNetworkFilterConfigFactory {
class NamedNetworkFilterConfigFactory : public ProtocolOptionsFactory {
public:
virtual ~NamedNetworkFilterConfigFactory() {}

Expand Down Expand Up @@ -220,25 +248,6 @@ class NamedNetworkFilterConfigFactory {
*/
virtual ProtobufTypes::MessagePtr createEmptyConfigProto() { return nullptr; }

/**
* Create a particular network filter's protocol specific options implementation. If the factory
* implementation is unable to produce a factory with the provided parameters, it should throw an
* EnvoyException.
* @param config supplies the protobuf configuration for the filter
* @return Upstream::ProtocoOptionsConfigConstSharedPtr the protocol options
*/
virtual Upstream::ProtocolOptionsConfigConstSharedPtr
createProtocolOptionsConfig(const Protobuf::Message& config) {
UNREFERENCED_PARAMETER(config);
return nullptr;
}

/**
* @return ProtobufTypes::MessagePtr a newly created empty protocol specific options message or
* nullptr if protocol specific options are not available.
*/
virtual ProtobufTypes::MessagePtr createEmptyProtocolOptionsProto() { return nullptr; }

/**
* @return std::string the identifying name for a particular implementation of a network filter
* produced by the factory.
Expand All @@ -250,7 +259,7 @@ class NamedNetworkFilterConfigFactory {
* Implemented by each HTTP filter and registered via Registry::registerFactory or the
* convenience class RegisterFactory.
*/
class NamedHttpFilterConfigFactory {
class NamedHttpFilterConfigFactory : public ProtocolOptionsFactory {
public:
virtual ~NamedHttpFilterConfigFactory() {}

Expand Down
9 changes: 4 additions & 5 deletions source/common/config/utility.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,14 +280,13 @@ class Utility {
* @return ProtobufTypes::MessagePtr the translated config
* @throws EnvoyException if the factory does not support protocol options
*/
static ProtobufTypes::MessagePtr translateToFactoryProtocolOptionsConfig(
const Protobuf::Message& source,
Server::Configuration::NamedNetworkFilterConfigFactory& factory) {
static ProtobufTypes::MessagePtr
translateToFactoryProtocolOptionsConfig(const Protobuf::Message& source, const std::string& name,
Server::Configuration::ProtocolOptionsFactory& factory) {
ProtobufTypes::MessagePtr config = factory.createEmptyProtocolOptionsProto();

if (config == nullptr) {
throw EnvoyException(
fmt::format("filter {} does not support protocol options", factory.name()));
throw EnvoyException(fmt::format("filter {} does not support protocol options", name));
}

MessageUtil::jsonConvert(source, *config);
Expand Down
19 changes: 15 additions & 4 deletions source/common/upstream/upstream_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,12 +119,23 @@ parseExtensionProtocolOptions(const envoy::api::v2::Cluster& config) {
for (const auto& iter : config.extension_protocol_options()) {
const std::string& name = iter.first;
const ProtobufWkt::Struct& config_struct = iter.second;
Server::Configuration::ProtocolOptionsFactory* factory = nullptr;

auto& factory = Envoy::Config::Utility::getAndCheckFactory<
Server::Configuration::NamedNetworkFilterConfigFactory>(name);
factory = Registry::FactoryRegistry<
Server::Configuration::NamedNetworkFilterConfigFactory>::getFactory(name);
if (factory == nullptr) {
factory = Registry::FactoryRegistry<
Server::Configuration::NamedHttpFilterConfigFactory>::getFactory(name);
}

if (factory == nullptr) {
throw EnvoyException(fmt::format(
"Didn't find a registered network or http filter implementation for name: '{}'", name));
}

auto object = factory.createProtocolOptionsConfig(
*Envoy::Config::Utility::translateToFactoryProtocolOptionsConfig(config_struct, factory));
auto object = factory->createProtocolOptionsConfig(
*Envoy::Config::Utility::translateToFactoryProtocolOptionsConfig(config_struct, name,
*factory));
if (object) {
options[name] = object;
}
Expand Down
123 changes: 101 additions & 22 deletions test/common/upstream/upstream_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1543,17 +1543,33 @@ TEST_F(ClusterInfoImplTest, ExtensionProtocolOptionsForUnknownFilter) {
no_such_filter: { option: value }
)EOF";

EXPECT_THROW_WITH_MESSAGE(makeCluster(yaml), EnvoyException,
"Didn't find a registered implementation for name: 'no_such_filter'");
EXPECT_THROW_WITH_MESSAGE(
makeCluster(yaml), EnvoyException,
"Didn't find a registered network or http filter implementation for name: 'no_such_filter'");
}

class TestFilterConfigFactory : public Server::Configuration::NamedNetworkFilterConfigFactory {
class TestFilterConfigFactoryBase {
public:
TestFilterConfigFactory(
TestFilterConfigFactoryBase(
std::function<ProtobufTypes::MessagePtr()> empty_proto,
std::function<Upstream::ProtocolOptionsConfigConstSharedPtr(const Protobuf::Message&)> config)
: empty_proto_(empty_proto), config_(config) {}

ProtobufTypes::MessagePtr createEmptyProtocolOptionsProto() { return empty_proto_(); }
Upstream::ProtocolOptionsConfigConstSharedPtr
createProtocolOptionsConfig(const Protobuf::Message& msg) {
return config_(msg);
}

std::function<ProtobufTypes::MessagePtr()> empty_proto_;
std::function<Upstream::ProtocolOptionsConfigConstSharedPtr(const Protobuf::Message&)> config_;
};

class TestNetworkFilterConfigFactory
: public Server::Configuration::NamedNetworkFilterConfigFactory {
public:
TestNetworkFilterConfigFactory(TestFilterConfigFactoryBase& parent) : parent_(parent) {}
Copy link
Member

Choose a reason for hiding this comment

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

It's a little odd not to use inheritance here, but I guess the alternative is assigning the lambdas to variables in the tests and re-using them.

It's a bit dodgy, but I think you could combine the two factories into a single class as long as you overload all the methods that appear in both. 😬

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The catch is that i'm not sure how that will work with name() method that's shared on both, but doesn't come from a common interface. is there a way to express that in c++ ?

Copy link
Member

Choose a reason for hiding this comment

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

It's probably a bad idea. I'm fine for you to leave this as is.


// NamedNetworkFilterConfigFactory
Network::FilterFactoryCb createFilterFactory(const Json::Object&,
Server::Configuration::FactoryContext&) override {
Expand All @@ -1565,29 +1581,63 @@ class TestFilterConfigFactory : public Server::Configuration::NamedNetworkFilter
NOT_IMPLEMENTED_GCOVR_EXCL_LINE;
}
ProtobufTypes::MessagePtr createEmptyConfigProto() override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; }
ProtobufTypes::MessagePtr createEmptyProtocolOptionsProto() override { return empty_proto_(); }
ProtobufTypes::MessagePtr createEmptyProtocolOptionsProto() override {
return parent_.createEmptyProtocolOptionsProto();
}
Upstream::ProtocolOptionsConfigConstSharedPtr
createProtocolOptionsConfig(const Protobuf::Message& msg) override {
return config_(msg);
return parent_.createProtocolOptionsConfig(msg);
}
std::string name() override { CONSTRUCT_ON_FIRST_USE(std::string, "envoy.test.filter"); }

std::function<ProtobufTypes::MessagePtr()> empty_proto_;
std::function<Upstream::ProtocolOptionsConfigConstSharedPtr(const Protobuf::Message&)> config_;
TestFilterConfigFactoryBase& parent_;
};

class TestHttpFilterConfigFactory : public Server::Configuration::NamedHttpFilterConfigFactory {
public:
TestHttpFilterConfigFactory(TestFilterConfigFactoryBase& parent) : parent_(parent) {}

// NamedNetworkFilterConfigFactory
Http::FilterFactoryCb createFilterFactory(const Json::Object&, const std::string&,
Server::Configuration::FactoryContext&) override {
NOT_IMPLEMENTED_GCOVR_EXCL_LINE;
}
Http::FilterFactoryCb
createFilterFactoryFromProto(const Protobuf::Message&, const std::string&,
Server::Configuration::FactoryContext&) override {
NOT_IMPLEMENTED_GCOVR_EXCL_LINE;
}
ProtobufTypes::MessagePtr createEmptyConfigProto() override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; }
ProtobufTypes::MessagePtr createEmptyRouteConfigProto() override {
NOT_IMPLEMENTED_GCOVR_EXCL_LINE;
}
Router::RouteSpecificFilterConfigConstSharedPtr
createRouteSpecificFilterConfig(const Protobuf::Message&,
Server::Configuration::FactoryContext&) override {
NOT_IMPLEMENTED_GCOVR_EXCL_LINE;
}

ProtobufTypes::MessagePtr createEmptyProtocolOptionsProto() override {
return parent_.createEmptyProtocolOptionsProto();
}
Upstream::ProtocolOptionsConfigConstSharedPtr
createProtocolOptionsConfig(const Protobuf::Message& msg) override {
return parent_.createProtocolOptionsConfig(msg);
}
std::string name() override { CONSTRUCT_ON_FIRST_USE(std::string, "envoy.test.filter"); }

TestFilterConfigFactoryBase& parent_;
};
struct TestFilterProtocolOptionsConfig : public Upstream::ProtocolOptionsConfig {};

// Cluster extension protocol options fails validation when configured for filter that does not
// support options.
TEST_F(ClusterInfoImplTest, ExtensionProtocolOptionsForFilterWithoutOptions) {
TestFilterConfigFactory factory(
TestFilterConfigFactoryBase factoryBase(
[]() -> ProtobufTypes::MessagePtr { return nullptr; },
[](const Protobuf::Message&) -> Upstream::ProtocolOptionsConfigConstSharedPtr {
return nullptr;
});
Registry::InjectFactory<Server::Configuration::NamedNetworkFilterConfigFactory> registry(factory);

const std::string yaml = R"EOF(
name: name
connect_timeout: 0.25s
Expand All @@ -1598,23 +1648,33 @@ TEST_F(ClusterInfoImplTest, ExtensionProtocolOptionsForFilterWithoutOptions) {
envoy.test.filter: { option: value }
)EOF";

EXPECT_THROW_WITH_MESSAGE(makeCluster(yaml), EnvoyException,
"filter envoy.test.filter does not support protocol options");
{
TestNetworkFilterConfigFactory factory(factoryBase);
Registry::InjectFactory<Server::Configuration::NamedNetworkFilterConfigFactory> registry(
factory);
EXPECT_THROW_WITH_MESSAGE(makeCluster(yaml), EnvoyException,
"filter envoy.test.filter does not support protocol options");
}
{
TestHttpFilterConfigFactory factory(factoryBase);
Registry::InjectFactory<Server::Configuration::NamedHttpFilterConfigFactory> registry(factory);
EXPECT_THROW_WITH_MESSAGE(makeCluster(yaml), EnvoyException,
"filter envoy.test.filter does not support protocol options");
}
}

// Cluster retrieval of typed extension protocol options.
TEST_F(ClusterInfoImplTest, ExtensionProtocolOptionsForFilterWithOptions) {
auto protocol_options = std::make_shared<TestFilterProtocolOptionsConfig>();

TestFilterConfigFactory factory(
TestFilterConfigFactoryBase factoryBase(
[]() -> ProtobufTypes::MessagePtr { return std::make_unique<ProtobufWkt::Struct>(); },
[&](const Protobuf::Message& msg) -> Upstream::ProtocolOptionsConfigConstSharedPtr {
const auto& msg_struct = dynamic_cast<const ProtobufWkt::Struct&>(msg);
EXPECT_TRUE(msg_struct.fields().find("option") != msg_struct.fields().end());

return protocol_options;
});
Registry::InjectFactory<Server::Configuration::NamedNetworkFilterConfigFactory> registry(factory);

const std::string yaml = R"EOF(
name: name
Expand All @@ -1626,14 +1686,33 @@ TEST_F(ClusterInfoImplTest, ExtensionProtocolOptionsForFilterWithOptions) {
envoy.test.filter: { option: "value" }
)EOF";

auto cluster = makeCluster(yaml);
// This vector is used to gather clusters with extension_protocol_options from the different
// types of extension factories (network, http).
std::vector<std::unique_ptr<StrictDnsClusterImpl>> clusters;

std::shared_ptr<const TestFilterProtocolOptionsConfig> stored_options =
cluster->info()->extensionProtocolOptionsTyped<TestFilterProtocolOptionsConfig>(
factory.name());
EXPECT_NE(nullptr, protocol_options);
// Same pointer
EXPECT_EQ(stored_options.get(), protocol_options.get());
{
// Get the cluster with extension_protocol_options for a network filter factory.
TestNetworkFilterConfigFactory factory(factoryBase);
Registry::InjectFactory<Server::Configuration::NamedNetworkFilterConfigFactory> registry(
factory);
clusters.push_back(makeCluster(yaml));
}
{
// Get the cluster with extension_protocol_options for an http filter factory.
TestHttpFilterConfigFactory factory(factoryBase);
Registry::InjectFactory<Server::Configuration::NamedHttpFilterConfigFactory> registry(factory);
clusters.push_back(makeCluster(yaml));
}

// Make sure that the clusters created from both factories are as expected.
for (auto&& cluster : clusters) {
std::shared_ptr<const TestFilterProtocolOptionsConfig> stored_options =
cluster->info()->extensionProtocolOptionsTyped<TestFilterProtocolOptionsConfig>(
"envoy.test.filter");
EXPECT_NE(nullptr, protocol_options);
// Same pointer
EXPECT_EQ(stored_options.get(), protocol_options.get());
}
}

// Validate empty singleton for HostsPerLocalityImpl.
Expand Down