-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
http: support creating filters with match tree #14430
Changes from 21 commits
ecf47e2
910d3f1
5734a5c
e63ff82
ce3e503
6b407f5
6fec69f
baff1bd
c39c870
571f096
e83b3b2
9afefee
3d223ad
d124637
ecb49e7
13a552d
a66c51f
b66ef32
01fced2
142c70b
9ce8523
be5f50b
711114b
d8e5cde
465f9a4
e88be1d
487822f
6cb618f
2cfb10c
0fb52fb
91e62af
b2c884f
e4987b5
8868cd2
9abac4b
899a365
193a95c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -838,3 +838,17 @@ message RequestIDExtension { | |
// Request ID extension specific configuration. | ||
google.protobuf.Any typed_config = 1; | ||
} | ||
|
||
// Match input indicates that matching should be done on a specific request header. | ||
// TODO(snowp): Link to unified matching docs. | ||
message HttpRequestHeaderMatchInput { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will this also be needed in the route table? If so, should we put this somewhere more generic, so that it can be used in both places? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assume yes if/when we get around to modifying the route table to use some of this stuff. I agree it might be worthwhile to make a new proto for some of these fields so it can be shared. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was imagining that we wouldn't depend on this directly in the route table if we ended up using the matching API for it? Wouldn't we just refer to this via opaque TypedExtensionConfigs? Any suggestions for where this would fit better? I can dump it into the extensions/common/matcher package I put the ExtensionWithMatcher, but there might be a better location for it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, we'd use Maybe a better location would be type/matcher/http_header.proto? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this handle multi-valued headers? I think it should concatenate multiple values with ',' delimiters, and we should document that behavior. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed we should be very specific about this. In the current impl (given the previous CVE) we would do that concat that you comment on. I would be explicit about that in the docs. |
||
// The request header to match on. | ||
string header_name = 1; | ||
} | ||
|
||
// Match input indicating that matching should be done on a specific response header. | ||
// TODO(snowp): Link to unified matching docs. | ||
message HttpResponseHeaderMatchInput { | ||
// The response header to match on. | ||
string header_name = 1; | ||
} |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,7 @@ | ||
#pragma once | ||
|
||
#include <memory> | ||
|
||
#include "envoy/extensions/filters/common/matcher/action/v3/skip_action.pb.h" | ||
#include "envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.pb.h" | ||
#include "envoy/http/filter.h" | ||
#include "envoy/http/header_map.h" | ||
#include "envoy/matcher/matcher.h" | ||
|
@@ -103,6 +102,23 @@ class HttpRequestHeadersDataInput : public HttpHeadersDataInputBase<RequestHeade | |
} | ||
}; | ||
|
||
class HttpRequestHeadersDataInputFactory : public Matcher::DataInputFactory<HttpMatchingData> { | ||
public: | ||
std::string name() const override { return "request-headers"; } | ||
Matcher::DataInputPtr<HttpMatchingData> | ||
createDataInput(const Protobuf::Message& config) override { | ||
const auto& typed_config = | ||
dynamic_cast<const envoy::extensions::filters::network::http_connection_manager::v3:: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you want to downcastAndValidate here? Same elsewhere. |
||
HttpRequestHeaderMatchInput&>(config); | ||
|
||
return std::make_unique<HttpRequestHeadersDataInput>(typed_config.header_name()); | ||
}; | ||
ProtobufTypes::MessagePtr createEmptyConfigProto() override { | ||
return std::make_unique<envoy::extensions::filters::network::http_connection_manager::v3:: | ||
HttpRequestHeaderMatchInput>(); | ||
} | ||
}; | ||
|
||
class HttpResponseHeadersDataInput : public HttpHeadersDataInputBase<ResponseHeaderMap> { | ||
public: | ||
explicit HttpResponseHeadersDataInput(const std::string& name) : HttpHeadersDataInputBase(name) {} | ||
|
@@ -113,9 +129,36 @@ class HttpResponseHeadersDataInput : public HttpHeadersDataInputBase<ResponseHea | |
} | ||
}; | ||
|
||
class HttpResponseHeadersDataInputFactory : public Matcher::DataInputFactory<HttpMatchingData> { | ||
public: | ||
std::string name() const override { return "response-headers"; } | ||
Matcher::DataInputPtr<HttpMatchingData> | ||
createDataInput(const Protobuf::Message& config) override { | ||
const auto& typed_config = | ||
dynamic_cast<const envoy::extensions::filters::network::http_connection_manager::v3:: | ||
HttpResponseHeaderMatchInput&>(config); | ||
|
||
return std::make_unique<HttpResponseHeadersDataInput>(typed_config.header_name()); | ||
}; | ||
ProtobufTypes::MessagePtr createEmptyConfigProto() override { | ||
return std::make_unique<envoy::extensions::filters::network::http_connection_manager::v3:: | ||
HttpResponseHeaderMatchInput>(); | ||
} | ||
}; | ||
|
||
class SkipAction : public Matcher::ActionBase< | ||
envoy::extensions::filters::common::matcher::action::v3::SkipFilter> {}; | ||
|
||
class SkipActionFactory : public Matcher::ActionFactory { | ||
public: | ||
std::string name() const override { return "skip"; } | ||
Matcher::ActionFactoryCb createActionFactoryCb(const Protobuf::Message&) override { | ||
return []() { return std::make_unique<SkipAction>(); }; | ||
} | ||
ProtobufTypes::MessagePtr createEmptyConfigProto() override { | ||
return std::make_unique<envoy::extensions::filters::common::matcher::action::v3::SkipFilter>(); | ||
} | ||
}; | ||
/** | ||
* Base class wrapper for both stream encoder and decoder filters. | ||
* | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
load( | ||
"//bazel:envoy_build_system.bzl", | ||
"envoy_cc_library", | ||
"envoy_package", | ||
) | ||
|
||
licenses(["notice"]) # Apache 2 | ||
|
||
envoy_package() | ||
|
||
envoy_cc_library( | ||
name = "config", | ||
srcs = ["config.cc"], | ||
hdrs = ["config.h"], | ||
deps = [ | ||
"//include/envoy/registry", | ||
"//include/envoy/server:filter_config_interface", | ||
"//source/common/matcher:matcher_lib", | ||
"//source/extensions/filters/http:well_known_names", | ||
"//source/extensions/filters/http/common:factory_base_lib", | ||
"@envoy_api//envoy/extensions/common/matching/v3:pkg_cc_proto", | ||
], | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,92 @@ | ||
#include "common/http/match_wrapper/config.h" | ||
|
||
#include "envoy/http/filter.h" | ||
#include "envoy/matcher/matcher.h" | ||
#include "envoy/registry/registry.h" | ||
|
||
#include "common/matcher/matcher.h" | ||
|
||
namespace Envoy { | ||
namespace Common { | ||
namespace Http { | ||
namespace MatchWrapper { | ||
|
||
namespace { | ||
struct DelegatingFactoryCallbacks : public Envoy::Http::FilterChainFactoryCallbacks { | ||
DelegatingFactoryCallbacks(Envoy::Http::FilterChainFactoryCallbacks& delegated_callbacks, | ||
Matcher::MatchTreeSharedPtr<Envoy::Http::HttpMatchingData> match_tree) | ||
: delegated_callbacks_(delegated_callbacks), match_tree_(std::move(match_tree)) {} | ||
|
||
void addStreamDecoderFilter(Envoy::Http::StreamDecoderFilterSharedPtr filter) override { | ||
delegated_callbacks_.addStreamDecoderFilter(std::move(filter), match_tree_); | ||
} | ||
void addStreamDecoderFilter( | ||
Envoy::Http::StreamDecoderFilterSharedPtr filter, | ||
Matcher::MatchTreeSharedPtr<Envoy::Http::HttpMatchingData> match_tree) override { | ||
delegated_callbacks_.addStreamDecoderFilter(std::move(filter), std::move(match_tree)); | ||
} | ||
void addStreamEncoderFilter(Envoy::Http::StreamEncoderFilterSharedPtr filter) override { | ||
delegated_callbacks_.addStreamEncoderFilter(std::move(filter), match_tree_); | ||
} | ||
void addStreamEncoderFilter( | ||
Envoy::Http::StreamEncoderFilterSharedPtr filter, | ||
Matcher::MatchTreeSharedPtr<Envoy::Http::HttpMatchingData> match_tree) override { | ||
delegated_callbacks_.addStreamEncoderFilter(std::move(filter), std::move(match_tree)); | ||
} | ||
void addStreamFilter(Envoy::Http::StreamFilterSharedPtr filter) override { | ||
delegated_callbacks_.addStreamFilter(std::move(filter), match_tree_); | ||
} | ||
void | ||
addStreamFilter(Envoy::Http::StreamFilterSharedPtr filter, | ||
Matcher::MatchTreeSharedPtr<Envoy::Http::HttpMatchingData> match_tree) override { | ||
delegated_callbacks_.addStreamFilter(std::move(filter), std::move(match_tree)); | ||
} | ||
void addAccessLogHandler(AccessLog::InstanceSharedPtr handler) override { | ||
delegated_callbacks_.addAccessLogHandler(std::move(handler)); | ||
} | ||
|
||
Envoy::Http::FilterChainFactoryCallbacks& delegated_callbacks_; | ||
Matcher::MatchTreeSharedPtr<Envoy::Http::HttpMatchingData> match_tree_; | ||
}; | ||
} // namespace | ||
|
||
Envoy::Http::FilterFactoryCb MatchWrapperConfig::createFilterFactoryFromProtoTyped( | ||
const envoy::extensions::common::matching::v3::ExtensionWithMatcher& proto_config, | ||
const std::string& prefix, Server::Configuration::FactoryContext& context) { | ||
|
||
if (proto_config.has_extension_config()) { | ||
auto& factory = | ||
Config::Utility::getAndCheckFactory<Server::Configuration::NamedHttpFilterConfigFactory>( | ||
proto_config.extension_config()); | ||
|
||
auto message = factory.createEmptyConfigProto(); | ||
proto_config.extension_config().typed_config(); | ||
Config::Utility::translateOpaqueConfig(proto_config.extension_config().typed_config(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you can use |
||
ProtobufWkt::Struct(), | ||
context.messageValidationVisitor(), *message); | ||
auto filter_factory = factory.createFilterFactoryFromProto(*message, prefix, context); | ||
|
||
auto match_tree = | ||
Matcher::MatchTreeFactory<Envoy::Http::HttpMatchingData>(context.messageValidationVisitor()) | ||
.create(proto_config.matcher()); | ||
|
||
return | ||
[filter_factory, match_tree](Envoy::Http::FilterChainFactoryCallbacks& callbacks) -> void { | ||
DelegatingFactoryCallbacks delegated_callbacks(callbacks, match_tree); | ||
|
||
return filter_factory(delegated_callbacks); | ||
}; | ||
} | ||
|
||
return [](Envoy::Http::FilterChainFactoryCallbacks&) -> void {}; | ||
} | ||
|
||
/** | ||
* Static registration for the Lua filter. @see RegisterFactory. | ||
*/ | ||
REGISTER_FACTORY(MatchWrapperConfig, Server::Configuration::NamedHttpFilterConfigFactory){"blah"}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm confused. Where is all of this code actually used? Are you registering this as an HTTP filter factory? (I think so). If so can you add more comments here and do something other than "blah"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Woops yeah this should have a better name for sure. Yeah I'm making use of the fact that by registering it as a filter all the locations that try to instantiate a filter will end up looking up this factory that unwraps the proto and allocates the matcher. I'll update all of this and add some more documentation |
||
|
||
} // namespace MatchWrapper | ||
} // namespace Http | ||
} // namespace Common | ||
} // namespace Envoy |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
#pragma once | ||
|
||
#include "envoy/extensions/common/matching/v3/extension_matcher.pb.validate.h" | ||
#include "envoy/server/filter_config.h" | ||
|
||
#include "extensions/filters/http/common/factory_base.h" | ||
#include "extensions/filters/http/well_known_names.h" | ||
|
||
namespace Envoy { | ||
namespace Common { | ||
namespace Http { | ||
namespace MatchWrapper { | ||
|
||
class MatchWrapperConfig : public Extensions::HttpFilters::Common::FactoryBase< | ||
envoy::extensions::common::matching::v3::ExtensionWithMatcher> { | ||
public: | ||
MatchWrapperConfig() : FactoryBase("match-wrapper") {} | ||
|
||
private: | ||
Envoy::Http::FilterFactoryCb createFilterFactoryFromProtoTyped( | ||
const envoy::extensions::common::matching::v3::ExtensionWithMatcher& proto_config, | ||
const std::string&, Server::Configuration::FactoryContext& context) override; | ||
}; | ||
|
||
} // namespace MatchWrapper | ||
} // namespace Http | ||
} // namespace Common | ||
} // namespace Envoy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use the doc comment format here and below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you talking about
/** */
comments or something else? iirc we just use//
for all proto commentsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean
[#comment
(see other examples). Can you check CI also?/wait