Skip to content

http filter: Add overrideable function to http filter factory interface for specifying FilterDependencies #15157

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

Merged
snowp merged 9 commits into from
Mar 5, 2021
Merged

http filter: Add overrideable function to http filter factory interface for specifying FilterDependencies #15157

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
51513a7
Add FilterDependencies to base http filter factory interface
Feb 13, 2021
ec1d1e4
Add docs
Feb 23, 2021
80f3d20
fix format
Feb 23, 2021
91364e7
Revert "Add docs"
Feb 24, 2021
0bf6a1a
Merge branch 'main' into filter
Feb 24, 2021
116922b
change dependencies() call to unique ptr
Mar 2, 2021
dcdb15e
Fix typo
Mar 2, 2021
83358a1
Merge remote-tracking branch 'envoy/main' into filter
Mar 5, 2021
fb8b464
Add ptr
Mar 5, 2021
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
1 change: 1 addition & 0 deletions docs/root/version_history/current.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ New Features
* http: added support for `Envoy::ScopeTrackedObject` for HTTP/1 and HTTP/2 dispatching. Crashes while inside the dispatching loop should dump debug information.
* http: added support for :ref:`preconnecting <envoy_v3_api_msg_config.cluster.v3.Cluster.PreconnectPolicy>`. Preconnecting is off by default, but recommended for clusters serving latency-sensitive traffic, especially if using HTTP/1.1.
* http: change frame flood and abuse checks to the upstream HTTP/2 codec to ON by default. It can be disabled by setting the `envoy.reloadable_features.upstream_http2_flood_checks` runtime key to false.
* http filter: add function to `NamedHttpFilterConfigFactory` which can be overridden to return a :ref`FilterDependencies <envoy_api_file_envoy/extensions/filters/common/dependency/v3/dependency.proto>` specification. Config-plane validation of dependencies is currently WIP.
Comment thread
aunu53 marked this conversation as resolved.
Outdated

@htuch htuch Feb 24, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think you can skip this for now, since its very internally focused. I'd add a single release note when we have the filter chain deps validation working.

@aunu53 aunu53 Feb 24, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Sounds good.

* json: introduced new JSON parser (https://github.com/nlohmann/json) to replace RapidJSON. The new parser is disabled by default. To test the new RapidJSON parser, enable the runtime feature `envoy.reloadable_features.remove_legacy_json`.
* original_dst: added support for :ref:`Original Destination <config_listener_filters_original_dst>` on Windows. This enables the use of Envoy as a sidecar proxy on Windows.
* overload: add support for scaling :ref:`transport connection timeouts<envoy_v3_api_enum_value_config.overload.v3.ScaleTimersOverloadActionConfig.TimerType.TRANSPORT_SOCKET_CONNECT>`. This can be used to reduce the TLS handshake timeout in response to overload.
Expand Down
1 change: 1 addition & 0 deletions include/envoy/server/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,7 @@ envoy_cc_library(
"//source/common/common:assert_lib",
"//source/common/common:macros",
"//source/common/protobuf",
"@envoy_api//envoy/extensions/filters/common/dependency/v3:pkg_cc_proto",
],
)

Expand Down
10 changes: 10 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 @@ -3,6 +3,7 @@
#include <functional>

#include "envoy/config/typed_config.h"
#include "envoy/extensions/filters/common/dependency/v3/dependency.pb.h"
#include "envoy/http/filter.h"
#include "envoy/init/manager.h"
#include "envoy/network/filter.h"
Expand Down Expand Up @@ -201,6 +202,15 @@ class NamedHttpFilterConfigFactory : public ProtocolOptionsFactory {
* @return bool true if this filter must be the last filter in a filter chain, false otherwise.
*/
virtual bool isTerminalFilter() { return false; }

/**
* @return FilterDependencies specification of dependencies required or
Comment thread
aunu53 marked this conversation as resolved.
Outdated
* provided on the decode and encode paths.
*/
virtual envoy::extensions::filters::common::dependency::v3::FilterDependencies dependencies() {

@htuch htuch Feb 24, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: probably const&

@aunu53 aunu53 Feb 24, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

filter_config.h:212:12: error: reference to stack memory associated with local variable 'dependency' returned [-Werror,-Wreturn-stack-address]
    return dependency;

I could try declaring a static helper function to return it? But I imagine this will cause problems for overriders of this function anyway, who will have to construct the proto somehow.

@htuch htuch Feb 25, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh, I see what's going on. This is more similar I think to createEmptyRouteConfigProto() above. I think you should return a unique_ptr with the allocated dependencies in this case.

@aunu53 aunu53 Feb 25, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I don't understand what the value of that would be; the dependency specification protos won't typically be very large, so passing it by copy seems ok to me. What do you think?

@snowp in case you have any thoughts

@snowp snowp Feb 25, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

With the protos being small I don't think it matters a whole lot, but a unique_ptr might better future proof this in case this changes? I can imagine some filter potentially providing a lot of different dependencies even if none do so today

@htuch htuch Feb 28, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Right, the protos contain multiple strings and we try not copy them around in general.

envoy::extensions::filters::common::dependency::v3::FilterDependencies dependency;
return dependency;
}
};

} // namespace Configuration
Expand Down
22 changes: 22 additions & 0 deletions test/server/filter_config_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@
namespace Envoy {
namespace {

using envoy::extensions::filters::common::dependency::v3::Dependency;
using envoy::extensions::filters::common::dependency::v3::FilterDependencies;

class TestHttpFilterConfigFactory : public Server::Configuration::NamedHttpFilterConfigFactory {
public:
TestHttpFilterConfigFactory() = default;
Expand All @@ -25,6 +28,14 @@ class TestHttpFilterConfigFactory : public Server::Configuration::NamedHttpFilte
ProtobufTypes::MessagePtr createEmptyConfigProto() override { return nullptr; }
ProtobufTypes::MessagePtr createEmptyProtocolOptionsProto() override { return nullptr; }

FilterDependencies dependencies() override {
FilterDependencies dependencies;
Dependency* d = dependencies.add_decode_required();
d->set_name("foobar");
d->set_type(Dependency::FILTER_STATE_KEY);
return dependencies;
}

std::string name() const override { return "envoy.test.http_filter"; }
std::string configType() override { return ""; };
};
Expand All @@ -38,5 +49,16 @@ TEST(NamedHttpFilterConfigFactoryTest, CreateFilterFactory) {
factory.createFilterFactoryFromProto(*message, stats_prefix, context);
}

TEST(NamedHttpFilterConfigFactoryTest, Dependencies) {
TestHttpFilterConfigFactory factory;
const std::string stats_prefix = "foo";
Server::Configuration::MockFactoryContext context;
ProtobufTypes::MessagePtr message{new Envoy::ProtobufWkt::Struct()};

factory.createFilterFactoryFromProto(*message, stats_prefix, context);

EXPECT_EQ(factory.dependencies().decode_required().size(), 1);
}

} // namespace
} // namespace Envoy