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.

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

filters: allow envoy filters to be created as exception free #30765

Merged
merged 3 commits into from
Nov 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion contrib/checksum/filters/http/test/config_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ TEST(ChecksumFilterConfigTest, ChecksumFilter) {
NiceMock<Server::Configuration::MockFactoryContext> context;
ChecksumFilterFactory factory;
envoy::extensions::filters::http::checksum::v3alpha::ChecksumConfig proto_config;
Http::FilterFactoryCb cb = factory.createFilterFactoryFromProto(proto_config, "stats", context);
Http::FilterFactoryCb cb =
factory.createFilterFactoryFromProto(proto_config, "stats", context).value();
Http::MockFilterChainFactoryCallbacks filter_callback;
EXPECT_CALL(filter_callback, addStreamFilter(_));
cb(filter_callback);
Expand Down
3 changes: 2 additions & 1 deletion contrib/dynamo/filters/http/test/config_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ TEST(DynamoFilterConfigTest, DynamoFilter) {
NiceMock<Server::Configuration::MockFactoryContext> context;
DynamoFilterConfig factory;
envoy::extensions::filters::http::dynamo::v3::Dynamo proto_config;
Http::FilterFactoryCb cb = factory.createFilterFactoryFromProto(proto_config, "stats", context);
Http::FilterFactoryCb cb =
factory.createFilterFactoryFromProto(proto_config, "stats", context).value();
Http::MockFilterChainFactoryCallbacks filter_callback;
EXPECT_CALL(filter_callback, addStreamFilter(_));
cb(filter_callback);
Expand Down
13 changes: 9 additions & 4 deletions contrib/golang/filters/http/test/config_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,11 @@ std::string genSoPath(std::string name) {
TEST(GolangFilterConfigTest, InvalidateEmptyConfig) {
NiceMock<Server::Configuration::MockFactoryContext> context;
EXPECT_THROW_WITH_REGEX(
GolangFilterConfig().createFilterFactoryFromProto(
envoy::extensions::filters::http::golang::v3alpha::Config(), "stats", context),
GolangFilterConfig()
.createFilterFactoryFromProto(envoy::extensions::filters::http::golang::v3alpha::Config(),
"stats", context)
.status()
.IgnoreError(),
Envoy::ProtoValidationException,
"ConfigValidationError.LibraryId: value length must be at least 1 characters");
}
Expand All @@ -54,7 +57,8 @@ TEST(GolangFilterConfigTest, GolangFilterWithValidConfig) {
TestUtility::loadFromYaml(yaml_string, proto_config);
NiceMock<Server::Configuration::MockFactoryContext> context;
GolangFilterConfig factory;
Http::FilterFactoryCb cb = factory.createFilterFactoryFromProto(proto_config, "stats", context);
Http::FilterFactoryCb cb =
factory.createFilterFactoryFromProto(proto_config, "stats", context).value();
Http::MockFilterChainFactoryCallbacks filter_callback;
EXPECT_CALL(filter_callback, addStreamFilter(_));
EXPECT_CALL(filter_callback, addAccessLogHandler(_));
Expand All @@ -77,7 +81,8 @@ TEST(GolangFilterConfigTest, GolangFilterWithNilPluginConfig) {
TestUtility::loadFromYaml(yaml_string, proto_config);
NiceMock<Server::Configuration::MockFactoryContext> context;
GolangFilterConfig factory;
Http::FilterFactoryCb cb = factory.createFilterFactoryFromProto(proto_config, "stats", context);
Http::FilterFactoryCb cb =
factory.createFilterFactoryFromProto(proto_config, "stats", context).value();
Http::MockFilterChainFactoryCallbacks filter_callback;
EXPECT_CALL(filter_callback, addStreamFilter(_));
EXPECT_CALL(filter_callback, addAccessLogHandler(_));
Expand Down
4 changes: 2 additions & 2 deletions contrib/golang/filters/http/test/golang_integration_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,8 @@ class RetrieveDynamicMetadataFilterConfig
RetrieveDynamicMetadataFilterConfig()
: Extensions::HttpFilters::Common::EmptyHttpFilterConfig("validate-dynamic-metadata") {}

Http::FilterFactoryCb createFilter(const std::string&,
Server::Configuration::FactoryContext&) override {
absl::StatusOr<Http::FilterFactoryCb>
createFilter(const std::string&, Server::Configuration::FactoryContext&) override {
return [](Http::FilterChainFactoryCallbacks& callbacks) -> void {
callbacks.addStreamEncoderFilter(std::make_shared<::Envoy::RetrieveDynamicMetadataFilter>());
};
Expand Down
3 changes: 2 additions & 1 deletion contrib/squash/filters/http/test/config_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@ TEST(SquashFilterConfigFactoryTest, SquashFilterCorrectYaml) {
NiceMock<Server::Configuration::MockFactoryContext> context;
context.cluster_manager_.initializeClusters({"fake_cluster"}, {});
SquashFilterConfigFactory factory;
Http::FilterFactoryCb cb = factory.createFilterFactoryFromProto(proto_config, "stats", context);
Http::FilterFactoryCb cb =
factory.createFilterFactoryFromProto(proto_config, "stats", context).value();
Http::MockFilterChainFactoryCallbacks filter_callback;
EXPECT_CALL(filter_callback, addStreamDecoderFilter(_));
cb(filter_callback);
Expand Down
8 changes: 5 additions & 3 deletions contrib/sxg/filters/http/test/config_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,8 @@ void expectCreateFilter(std::string yaml, bool is_sds_config) {
EXPECT_CALL(context, api());
EXPECT_CALL(context, initManager()).Times(2);
EXPECT_CALL(context, getTransportSocketFactoryContext());
Http::FilterFactoryCb cb = factory.createFilterFactoryFromProto(*proto_config, "stats", context);
Http::FilterFactoryCb cb =
factory.createFilterFactoryFromProto(*proto_config, "stats", context).value();
Http::MockFilterChainFactoryCallbacks filter_callback;
EXPECT_CALL(filter_callback, addStreamFilter(_));
cb(filter_callback);
Expand Down Expand Up @@ -76,8 +77,9 @@ validity_url: "/.sxg/validity.msg"
.WillByDefault(Return(std::make_shared<Secret::GenericSecretConfigProviderImpl>(
envoy::extensions::transport_sockets::tls::v3::GenericSecret())));

EXPECT_THROW_WITH_MESSAGE(factory.createFilterFactoryFromProto(*proto_config, "stats", context),
EnvoyException, exception_message);
EXPECT_THROW_WITH_MESSAGE(
factory.createFilterFactoryFromProto(*proto_config, "stats", context).status().IgnoreError(),
EnvoyException, exception_message);
}

} // namespace
Expand Down
7 changes: 4 additions & 3 deletions envoy/server/filter_config.h
Original file line number Diff line number Diff line change
Expand Up @@ -279,9 +279,10 @@ class NamedHttpFilterConfigFactory : public virtual HttpFilterConfigFactoryBase
* configuration.
* @param stat_prefix prefix for stat logging
* @param context supplies the filter's context.
* @return Http::FilterFactoryCb the factory creation function.
* @return absl::StatusOr<Http::FilterFactoryCb> the factory creation function or an error if
* creation fails.
*/
virtual Http::FilterFactoryCb
virtual absl::StatusOr<Http::FilterFactoryCb>
createFilterFactoryFromProto(const Protobuf::Message& config, const std::string& stat_prefix,
Server::Configuration::FactoryContext& context) PURE;

Expand Down Expand Up @@ -316,7 +317,7 @@ class UpstreamHttpFilterConfigFactory : public virtual HttpFilterConfigFactoryBa
* @param context supplies the filter's context.
* @return Http::FilterFactoryCb the factory creation function.
*/
virtual Http::FilterFactoryCb
virtual absl::StatusOr<Http::FilterFactoryCb>
createFilterFactoryFromProto(const Protobuf::Message& config, const std::string& stat_prefix,
Server::Configuration::UpstreamFactoryContext& context) PURE;
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -549,7 +549,8 @@ TEST(AssertionFilterFactoryTest, Config) {

TestUtility::loadFromYamlAndValidate(config_str, proto_config);

Http::FilterFactoryCb cb = factory.createFilterFactoryFromProto(proto_config, "test", context);
Http::FilterFactoryCb cb =
factory.createFilterFactoryFromProto(proto_config, "test", context).value();
Http::MockFilterChainFactoryCallbacks filter_callbacks;
EXPECT_CALL(filter_callbacks, addStreamFilter(_));
cb(filter_callbacks);
Expand Down
9 changes: 7 additions & 2 deletions source/common/filter/config_discovery_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -219,8 +219,13 @@ class HttpDynamicFilterConfigProviderImpl
instantiateFilterFactory(const Protobuf::Message& message) const override {
auto* factory = Registry::FactoryRegistry<NeutralHttpFilterConfigFactory>::getFactoryByType(
message.GetTypeName());
return {factory->name(),
factory->createFilterFactoryFromProto(message, getStatPrefix(), factory_context_)};
absl::StatusOr<Http::FilterFactoryCb> error_or_factory =
factory->createFilterFactoryFromProto(message, getStatPrefix(), factory_context_);
if (!error_or_factory.status().ok()) {
throwEnvoyExceptionOrPanic(std::string(error_or_factory.status().message()));
}

return {factory->name(), error_or_factory.value()};
}

Server::Configuration::ServerFactoryContext& server_context_;
Expand Down
6 changes: 5 additions & 1 deletion source/common/http/filter_chain_helper.h
Original file line number Diff line number Diff line change
Expand Up @@ -122,8 +122,12 @@ class FilterChainHelper : Logger::Loggable<Logger::Id::config> {
}
ProtobufTypes::MessagePtr message = Config::Utility::translateToFactoryConfig(
proto_config, server_context_.messageValidationVisitor(), *factory);
Http::FilterFactoryCb callback =
absl::StatusOr<Http::FilterFactoryCb> callback_or_error =
factory->createFilterFactoryFromProto(*message, stats_prefix_, factory_context_);
if (!callback_or_error.status().ok()) {
return callback_or_error.status();
}
Http::FilterFactoryCb callback = callback_or_error.value();
dependency_manager.registerFilter(factory->name(), *factory->dependencies());
const bool is_terminal = factory->isTerminalFilterByProto(*message, server_context_);
Config::Utility::validateTerminalFilters(proto_config.name(), factory->name(),
Expand Down
6 changes: 5 additions & 1 deletion source/common/http/match_delegate/config.cc
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,11 @@ Envoy::Http::FilterFactoryCb MatchDelegateConfig::createFilterFactoryFromProtoTy

auto message = Config::Utility::translateAnyToFactoryConfig(
proto_config.extension_config().typed_config(), context.messageValidationVisitor(), factory);
auto filter_factory = factory.createFilterFactoryFromProto(*message, prefix, context);
auto filter_factory_or_error = factory.createFilterFactoryFromProto(*message, prefix, context);
if (!filter_factory_or_error.ok()) {
throwEnvoyExceptionOrPanic(std::string(filter_factory_or_error.status().message()));
}
auto filter_factory = filter_factory_or_error.value();

Factory::MatchTreeValidationVisitor validation_visitor(*factory.matchingRequirements());

Expand Down
2 changes: 1 addition & 1 deletion source/common/router/upstream_codec_filter.h
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ class UpstreamCodecFilterFactory
UpstreamCodecFilterFactory() : CommonFactoryBase("envoy.filters.http.upstream_codec") {}

std::string category() const override { return "envoy.filters.http.upstream"; }
Http::FilterFactoryCb
absl::StatusOr<Http::FilterFactoryCb>
createFilterFactoryFromProto(const Protobuf::Message&, const std::string&,
Server::Configuration::UpstreamFactoryContext&) override {
return [](Http::FilterChainFactoryCallbacks& callbacks) -> void {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ namespace AdmissionControl {

static constexpr std::chrono::seconds defaultSamplingWindow{30};

Http::FilterFactoryCb AdmissionControlFilterFactory::createFilterFactoryFromProtoTyped(
absl::StatusOr<Http::FilterFactoryCb>
AdmissionControlFilterFactory::createFilterFactoryFromProtoTyped(
const envoy::extensions::filters::http::admission_control::v3::AdmissionControl& config,
const std::string& stats_prefix, DualInfo dual_info,
Server::Configuration::ServerFactoryContext& context) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ class AdmissionControlFilterFactory
public:
AdmissionControlFilterFactory() : DualFactoryBase("envoy.filters.http.admission_control") {}

Http::FilterFactoryCb createFilterFactoryFromProtoTyped(
absl::StatusOr<Http::FilterFactoryCb> createFilterFactoryFromProtoTyped(
const envoy::extensions::filters::http::admission_control::v3::AdmissionControl& proto_config,
const std::string& stats_prefix, DualInfo dual_info,
Server::Configuration::ServerFactoryContext& context) override;
Expand Down
2 changes: 1 addition & 1 deletion source/extensions/filters/http/buffer/config.cc
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ namespace Extensions {
namespace HttpFilters {
namespace BufferFilter {

Http::FilterFactoryCb BufferFilterFactory::createFilterFactoryFromProtoTyped(
absl::StatusOr<Http::FilterFactoryCb> BufferFilterFactory::createFilterFactoryFromProtoTyped(
const envoy::extensions::filters::http::buffer::v3::Buffer& proto_config, const std::string&,
DualInfo, Server::Configuration::ServerFactoryContext&) {
ASSERT(proto_config.has_max_request_bytes());
Expand Down
2 changes: 1 addition & 1 deletion source/extensions/filters/http/buffer/config.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ class BufferFilterFactory
BufferFilterFactory() : DualFactoryBase("envoy.filters.http.buffer") {}

private:
Http::FilterFactoryCb createFilterFactoryFromProtoTyped(
absl::StatusOr<Http::FilterFactoryCb> createFilterFactoryFromProtoTyped(
const envoy::extensions::filters::http::buffer::v3::Buffer& proto_config,
const std::string& stats_prefix, DualInfo,
Server::Configuration::ServerFactoryContext& context) override;
Expand Down
30 changes: 26 additions & 4 deletions source/extensions/filters/http/common/factory_base.h
Original file line number Diff line number Diff line change
Expand Up @@ -58,13 +58,14 @@ class CommonFactoryBase : public virtual Server::Configuration::HttpFilterConfig

const std::string name_;
};

template <class ConfigProto, class RouteConfigProto = ConfigProto>
class FactoryBase : public CommonFactoryBase<ConfigProto, RouteConfigProto>,
public Server::Configuration::NamedHttpFilterConfigFactory {
public:
FactoryBase(const std::string& name) : CommonFactoryBase<ConfigProto, RouteConfigProto>(name) {}

Envoy::Http::FilterFactoryCb
absl::StatusOr<Envoy::Http::FilterFactoryCb>
createFilterFactoryFromProto(const Protobuf::Message& proto_config,
const std::string& stats_prefix,
Server::Configuration::FactoryContext& context) override {
Expand Down Expand Up @@ -95,6 +96,27 @@ class FactoryBase : public CommonFactoryBase<ConfigProto, RouteConfigProto>,
}
};

template <class ConfigProto, class RouteConfigProto = ConfigProto>
class ExceptionFreeFactoryBase : public CommonFactoryBase<ConfigProto, RouteConfigProto>,
public Server::Configuration::NamedHttpFilterConfigFactory {
public:
ExceptionFreeFactoryBase(const std::string& name)
: CommonFactoryBase<ConfigProto, RouteConfigProto>(name) {}

absl::StatusOr<Envoy::Http::FilterFactoryCb>
createFilterFactoryFromProto(const Protobuf::Message& proto_config,
const std::string& stats_prefix,
Server::Configuration::FactoryContext& context) override {
return createFilterFactoryFromProtoTyped(MessageUtil::downcastAndValidate<const ConfigProto&>(
proto_config, context.messageValidationVisitor()),
stats_prefix, context);
}
virtual absl::StatusOr<Envoy::Http::FilterFactoryCb>
createFilterFactoryFromProtoTyped(const ConfigProto& proto_config,
const std::string& stats_prefix,
Server::Configuration::FactoryContext& context) PURE;
};

template <class ConfigProto, class RouteConfigProto = ConfigProto>
class DualFactoryBase : public CommonFactoryBase<ConfigProto, RouteConfigProto>,
public Server::Configuration::NamedHttpFilterConfigFactory,
Expand All @@ -112,7 +134,7 @@ class DualFactoryBase : public CommonFactoryBase<ConfigProto, RouteConfigProto>,
Stats::Scope& scope;
};

Envoy::Http::FilterFactoryCb
absl::StatusOr<Envoy::Http::FilterFactoryCb>
createFilterFactoryFromProto(const Protobuf::Message& proto_config,
const std::string& stats_prefix,
Server::Configuration::FactoryContext& context) override {
Expand All @@ -122,7 +144,7 @@ class DualFactoryBase : public CommonFactoryBase<ConfigProto, RouteConfigProto>,
context.getServerFactoryContext());
}

Envoy::Http::FilterFactoryCb
absl::StatusOr<Envoy::Http::FilterFactoryCb>
createFilterFactoryFromProto(const Protobuf::Message& proto_config,
const std::string& stats_prefix,
Server::Configuration::UpstreamFactoryContext& context) override {
Expand All @@ -132,7 +154,7 @@ class DualFactoryBase : public CommonFactoryBase<ConfigProto, RouteConfigProto>,
stats_prefix, DualInfo(context), context.getServerFactoryContext());
}

virtual Envoy::Http::FilterFactoryCb
virtual absl::StatusOr<Envoy::Http::FilterFactoryCb>
createFilterFactoryFromProtoTyped(const ConfigProto& proto_config,
const std::string& stats_prefix, DualInfo info,
Server::Configuration::ServerFactoryContext& context) PURE;
Expand Down
6 changes: 4 additions & 2 deletions source/extensions/filters/http/composite/action.cc
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,10 @@ Matcher::ActionFactoryCb ExecuteFilterActionFactory::createActionFactoryCb(

// First, try to create the filter factory creation function from factory context (if exists).
if (context.factory_context_.has_value()) {
callback = factory.createFilterFactoryFromProto(*message, context.stat_prefix_,
context.factory_context_.value());
auto callback_or_status = factory.createFilterFactoryFromProto(
*message, context.stat_prefix_, context.factory_context_.value());
THROW_IF_STATUS_NOT_OK(callback_or_status, throw);
callback = callback_or_status.value();
}

// If above failed, try to create the filter factory creation function from server factory
Expand Down
4 changes: 2 additions & 2 deletions source/extensions/filters/http/compressor/config.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ namespace Extensions {
namespace HttpFilters {
namespace Compressor {

Http::FilterFactoryCb CompressorFilterFactory::createFilterFactoryFromProtoTyped(
absl::StatusOr<Http::FilterFactoryCb> CompressorFilterFactory::createFilterFactoryFromProtoTyped(
const envoy::extensions::filters::http::compressor::v3::Compressor& proto_config,
const std::string& stats_prefix, Server::Configuration::FactoryContext& context) {
const std::string type{TypeUtil::typeUrlToDescriptorFullName(
Expand All @@ -19,7 +19,7 @@ Http::FilterFactoryCb CompressorFilterFactory::createFilterFactoryFromProtoTyped
Registry::FactoryRegistry<
Compression::Compressor::NamedCompressorLibraryConfigFactory>::getFactoryByType(type);
if (config_factory == nullptr) {
throwEnvoyExceptionOrPanic(
return absl::InvalidArgumentError(
fmt::format("Didn't find a registered implementation for type: '{}'", type));
}
ProtobufTypes::MessagePtr message = Config::Utility::translateAnyToFactoryConfig(
Expand Down
6 changes: 3 additions & 3 deletions source/extensions/filters/http/compressor/config.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,14 @@ namespace Compressor {
* Config registration for the compressor filter. @see NamedHttpFilterConfigFactory.
*/
class CompressorFilterFactory
: public Common::FactoryBase<
: public Common::ExceptionFreeFactoryBase<
envoy::extensions::filters::http::compressor::v3::Compressor,
envoy::extensions::filters::http::compressor::v3::CompressorPerRoute> {
public:
CompressorFilterFactory() : FactoryBase("envoy.filters.http.compressor") {}
CompressorFilterFactory() : ExceptionFreeFactoryBase("envoy.filters.http.compressor") {}

private:
Http::FilterFactoryCb createFilterFactoryFromProtoTyped(
absl::StatusOr<Http::FilterFactoryCb> createFilterFactoryFromProtoTyped(
const envoy::extensions::filters::http::compressor::v3::Compressor& proto_config,
const std::string& stats_prefix, Server::Configuration::FactoryContext& context) override;

Expand Down
6 changes: 3 additions & 3 deletions source/extensions/filters/http/decompressor/config.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ namespace Extensions {
namespace HttpFilters {
namespace Decompressor {

Http::FilterFactoryCb DecompressorFilterFactory::createFilterFactoryFromProtoTyped(
absl::StatusOr<Http::FilterFactoryCb> DecompressorFilterFactory::createFilterFactoryFromProtoTyped(
const envoy::extensions::filters::http::decompressor::v3::Decompressor& proto_config,
const std::string& stats_prefix, Server::Configuration::FactoryContext& context) {
const std::string decompressor_library_type{TypeUtil::typeUrlToDescriptorFullName(
Expand All @@ -20,8 +20,8 @@ Http::FilterFactoryCb DecompressorFilterFactory::createFilterFactoryFromProtoTyp
Compression::Decompressor::NamedDecompressorLibraryConfigFactory>::
getFactoryByType(decompressor_library_type);
if (decompressor_library_factory == nullptr) {
throwEnvoyExceptionOrPanic(fmt::format("Didn't find a registered implementation for type: '{}'",
decompressor_library_type));
return absl::InvalidArgumentError(fmt::format(
"Didn't find a registered implementation for type: '{}'", decompressor_library_type));
}
ProtobufTypes::MessagePtr message = Config::Utility::translateAnyToFactoryConfig(
proto_config.decompressor_library().typed_config(), context.messageValidationVisitor(),
Expand Down
Loading
Loading