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

set_filter_state: enable per-route configuration override #37507

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+114 −2
Open

set_filter_state: enable per-route configuration override #37507

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
71ce473
set_filter_state: enable per-route configuration override
howardjohn Dec 4, 2024
46a6a0c
Add integration test
howardjohn Dec 11, 2024
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 source/extensions/filters/common/set_filter_state/filter_config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ struct Value {
Formatter::FormatterConstSharedPtr value_;
};

class Config : public Logger::Loggable<Logger::Id::config> {
class Config : public ::Envoy::Router::RouteSpecificFilterConfig,
public Logger::Loggable<Logger::Id::config> {
public:
Config(const Protobuf::RepeatedPtrField<FilterStateValueProto>& proto_values, LifeSpan life_span,
Server::Configuration::GenericFactoryContext& context)
Expand Down
24 changes: 23 additions & 1 deletion source/extensions/filters/http/set_filter_state/config.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include "envoy/formatter/substitution_formatter.h"
#include "envoy/registry/registry.h"

#include "source/common/http/utility.h"
#include "source/common/protobuf/utility.h"
#include "source/server/generic_factory_context.h"

Expand All @@ -19,7 +20,16 @@ SetFilterState::SetFilterState(const Filters::Common::SetFilterState::ConfigShar
: config_(config) {}

Http::FilterHeadersStatus SetFilterState::decodeHeaders(Http::RequestHeaderMap& headers, bool) {
config_->updateFilterState({&headers}, decoder_callbacks_->streamInfo());
// Apply listener level configuration first.
config_.get()->updateFilterState({&headers}, decoder_callbacks_->streamInfo());

// If configured, apply route level configuration next.
auto per_route_config =
Http::Utility::resolveMostSpecificPerFilterConfig<Filters::Common::SetFilterState::Config>(
decoder_callbacks_);
if (per_route_config) {
per_route_config->updateFilterState({&headers}, decoder_callbacks_->streamInfo());
}
Comment on lines +24 to +32
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason you run both, typically a route level config will override the listener level

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking it would be nice to have common listener level ones and then have additional route specific ones. You can override by applying both (route is last so it wins).

We do have this use case - but it could also be done by just copying the listener ones into every route of course.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would say in that case you should have explicit options most likely, as in override, merge, etc. Also acceptable at first of course to just say there's only one behavior, but this would be different from most filters today afaik

Copy link
Member

Choose a reason for hiding this comment

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

I partly agree with @EItanya . I will prefer let this filter:

  1. always use the most specific config or 2. iterate all configs contains both global config, vh config, route config, etc.

It's a little weird to apply both filter level config and rotue level config, but ignore the vh config, esp they all are completely same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought I did (2). Tbh I did not know there is vhost and route level?? I will need to look into that I thought it was just 2 levels

return Http::FilterHeadersStatus::Continue;
}

Expand All @@ -35,6 +45,18 @@ Http::FilterFactoryCb SetFilterStateConfig::createFilterFactoryFromProtoTyped(
};
}

absl::StatusOr<Router::RouteSpecificFilterConfigConstSharedPtr>
SetFilterStateConfig::createRouteSpecificFilterConfigTyped(
const envoy::extensions::filters::http::set_filter_state::v3::Config& proto_config,
Server::Configuration::ServerFactoryContext& context, ProtobufMessage::ValidationVisitor&) {

Server::GenericFactoryContextImpl generic_context(context, context.messageValidationVisitor());

return std::make_shared<const Filters::Common::SetFilterState::Config>(
proto_config.on_request_headers(), StreamInfo::FilterState::LifeSpan::FilterChain,
generic_context);
}

Http::FilterFactoryCb SetFilterStateConfig::createFilterFactoryFromProtoWithServerContextTyped(
const envoy::extensions::filters::http::set_filter_state::v3::Config& proto_config,
const std::string&, Server::Configuration::ServerFactoryContext& context) {
Expand Down
4 changes: 4 additions & 0 deletions source/extensions/filters/http/set_filter_state/config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,10 @@ class SetFilterStateConfig
const envoy::extensions::filters::http::set_filter_state::v3::Config& proto_config,
const std::string& stats_prefix, Server::Configuration::FactoryContext& context) override;

absl::StatusOr<Router::RouteSpecificFilterConfigConstSharedPtr> createRouteSpecificFilterConfigTyped(
const envoy::extensions::filters::http::set_filter_state::v3::Config& proto_config,
Server::Configuration::ServerFactoryContext&, ProtobufMessage::ValidationVisitor&) override;

Http::FilterFactoryCb createFilterFactoryFromProtoWithServerContextTyped(
const envoy::extensions::filters::http::set_filter_state::v3::Config& proto_config,
const std::string& stats_prefix,
Expand Down
85 changes: 85 additions & 0 deletions test/extensions/filters/http/set_filter_state/integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include "gtest/gtest.h"

using testing::NiceMock;
using testing::Return;
using testing::ReturnRef;

namespace Envoy {
Expand Down Expand Up @@ -66,6 +67,44 @@ class SetMetadataIntegrationTest : public testing::Test {
EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter->decodeHeaders(headers_, true));
}

void runPerRouteFilter(const std::string& listener_yaml_config,
const std::string& per_route_yaml_config) {
Server::GenericFactoryContextImpl generic_context(context_);

envoy::extensions::filters::http::set_filter_state::v3::Config listener_proto_config;
Copy link
Member

Choose a reason for hiding this comment

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

nit: this actually is filter level rather than listener level, it would be better to name it filter_proto_config

TestUtility::loadFromYaml(listener_yaml_config, listener_proto_config);
auto listener_config = std::make_shared<Filters::Common::SetFilterState::Config>(
listener_proto_config.on_request_headers(), StreamInfo::FilterState::LifeSpan::FilterChain,
generic_context);

envoy::extensions::filters::http::set_filter_state::v3::Config route_proto_config;
TestUtility::loadFromYaml(per_route_yaml_config, route_proto_config);
Filters::Common::SetFilterState::Config route_config(
route_proto_config.on_request_headers(), StreamInfo::FilterState::LifeSpan::FilterChain,
generic_context);

NiceMock<Http::MockStreamDecoderFilterCallbacks> decoder_callbacks;
EXPECT_CALL(*decoder_callbacks.route_, mostSpecificPerFilterConfig(_))
.WillOnce(Return(&route_config));

auto filter = std::make_shared<SetFilterState>(listener_config);
filter->setDecoderFilterCallbacks(decoder_callbacks);
EXPECT_CALL(decoder_callbacks, streamInfo()).WillRepeatedly(ReturnRef(info_));
EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter->decodeHeaders(headers_, true));

// Test the factory method.
{
NiceMock<Server::Configuration::MockServerFactoryContext> context;
SetFilterStateConfig factory;
Router::RouteSpecificFilterConfigConstSharedPtr route_config =
factory
.createRouteSpecificFilterConfig(route_proto_config, context,
ProtobufMessage::getNullValidationVisitor())
.value();
EXPECT_TRUE(route_config.get());
}
Comment on lines +95 to +105
Copy link
Member

Choose a reason for hiding this comment

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

This should be part of config_test, I think, if there is one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think there is one, just this test. I was following how the existing filter level test is written

}

NiceMock<Server::Configuration::MockFactoryContext> context_;
Http::TestRequestHeaderMapImpl headers_{{"test-header", "test-value"}};
NiceMock<StreamInfo::MockStreamInfo> info_;
Expand All @@ -85,6 +124,52 @@ TEST_F(SetMetadataIntegrationTest, FromHeader) {
EXPECT_EQ(foo->serializeAsString(), "test-value");
}

TEST_F(SetMetadataIntegrationTest, RouteLevel) {
const std::string listener_config = R"EOF(
on_request_headers:
- object_key: both
factory_key: envoy.string
format_string:
text_format_source:
inline_string: "listener-%REQ(test-header)%"
- object_key: listener-only
factory_key: envoy.string
format_string:
text_format_source:
inline_string: "listener"
)EOF";
const std::string route_config = R"EOF(
on_request_headers:
- object_key: both
factory_key: envoy.string
format_string:
text_format_source:
inline_string: "route-%REQ(test-header)%"
- object_key: route-only
factory_key: envoy.string
format_string:
text_format_source:
inline_string: "route"
)EOF";
runPerRouteFilter(listener_config, route_config);

const auto* both = info_.filterState()->getDataReadOnly<Router::StringAccessor>("both");
ASSERT_NE(nullptr, both);
// Route takes precedence
EXPECT_EQ(both->serializeAsString(), "route-test-value");

const auto* listener =
info_.filterState()->getDataReadOnly<Router::StringAccessor>("listener-only");
ASSERT_NE(nullptr, listener);
// Only set on listener
EXPECT_EQ(listener->serializeAsString(), "listener");

const auto* route = info_.filterState()->getDataReadOnly<Router::StringAccessor>("route-only");
ASSERT_NE(nullptr, route);
// Only set on route
EXPECT_EQ(route->serializeAsString(), "route");
}

} // namespace SetFilterState
} // namespace HttpFilters
} // namespace Extensions
Expand Down