-
Notifications
You must be signed in to change notification settings - Fork 5.5k
formatter: expose CEL expression config in header mutations #45815
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
base: main
Are you sure you want to change the base?
Changes from 3 commits
fc1df8b
064165d
82839a5
dcd5965
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 |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| Added :ref:`formatters <envoy_v3_api_field_extensions.http.early_header_mutation.header_mutation.v3.HeaderMutation.formatters>` | ||
| to the early header mutation extension, allowing extension formatter command parsers to be | ||
| configured for header mutation substitution values. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| Exposed :ref:`cel_config <envoy_v3_api_field_extensions.formatter.cel.v3.Cel.cel_config>` on the | ||
| ``envoy.formatter.cel`` formatter configuration, allowing CEL runtime options such as string | ||
| functions to be enabled for configured CEL formatters. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| Added :ref:`formatters <envoy_v3_api_field_extensions.filters.http.header_mutation.v3.Mutations.formatters>` | ||
| to the header mutation filter, allowing extension formatter command parsers to be configured for | ||
| header mutation substitution values. |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -3,7 +3,7 @@ | |||||||||||||||||||||||
| #include <cstdint> | ||||||||||||||||||||||||
| #include <memory> | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| #include "source/common/config/utility.h" | ||||||||||||||||||||||||
| #include "source/common/formatter/substitution_format_string.h" | ||||||||||||||||||||||||
| #include "source/common/http/header_map_impl.h" | ||||||||||||||||||||||||
| #include "source/common/http/utility.h" | ||||||||||||||||||||||||
| #include "source/common/runtime/runtime_features.h" | ||||||||||||||||||||||||
|
|
@@ -54,22 +54,31 @@ void QueryParameterMutationAppend::mutateQueryParameter( | |||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| Mutations::Mutations(const MutationsProto& config, | ||||||||||||||||||||||||
| Server::Configuration::ServerFactoryContext& context, | ||||||||||||||||||||||||
| ProtobufMessage::ValidationVisitor& validation_visitor, | ||||||||||||||||||||||||
| absl::Status& creation_status) { | ||||||||||||||||||||||||
| auto request_mutations_or_error = HeaderMutations::create(config.request_mutations(), context); | ||||||||||||||||||||||||
| auto command_parsers_or_error = Formatter::SubstitutionFormatStringUtils::parseFormatters( | ||||||||||||||||||||||||
| config.formatters(), context, validation_visitor); | ||||||||||||||||||||||||
| SET_AND_RETURN_IF_NOT_OK(command_parsers_or_error.status(), creation_status); | ||||||||||||||||||||||||
| Formatter::CommandParserPtrVector command_parsers = | ||||||||||||||||||||||||
| std::move(command_parsers_or_error.value()); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| auto request_mutations_or_error = | ||||||||||||||||||||||||
| HeaderMutations::create(config.request_mutations(), context, command_parsers); | ||||||||||||||||||||||||
| SET_AND_RETURN_IF_NOT_OK(request_mutations_or_error.status(), creation_status); | ||||||||||||||||||||||||
| request_mutations_ = std::move(request_mutations_or_error.value()); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| auto response_mutations_or_error = HeaderMutations::create(config.response_mutations(), context); | ||||||||||||||||||||||||
| auto response_mutations_or_error = | ||||||||||||||||||||||||
| HeaderMutations::create(config.response_mutations(), context, command_parsers); | ||||||||||||||||||||||||
| SET_AND_RETURN_IF_NOT_OK(response_mutations_or_error.status(), creation_status); | ||||||||||||||||||||||||
| response_mutations_ = std::move(response_mutations_or_error.value()); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| auto response_trailers_mutations_or_error = | ||||||||||||||||||||||||
| HeaderMutations::create(config.response_trailers_mutations(), context); | ||||||||||||||||||||||||
| HeaderMutations::create(config.response_trailers_mutations(), context, command_parsers); | ||||||||||||||||||||||||
| SET_AND_RETURN_IF_NOT_OK(response_trailers_mutations_or_error.status(), creation_status); | ||||||||||||||||||||||||
| response_trailers_mutations_ = std::move(response_trailers_mutations_or_error.value()); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| auto request_trailers_mutations_or_error = | ||||||||||||||||||||||||
| HeaderMutations::create(config.request_trailers_mutations(), context); | ||||||||||||||||||||||||
| HeaderMutations::create(config.request_trailers_mutations(), context, command_parsers); | ||||||||||||||||||||||||
| SET_AND_RETURN_IF_NOT_OK(request_trailers_mutations_or_error.status(), creation_status); | ||||||||||||||||||||||||
| request_trailers_mutations_ = std::move(request_trailers_mutations_or_error.value()); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
|
|
@@ -92,8 +101,8 @@ Mutations::Mutations(const MutationsProto& config, | |||||||||||||||||||||||
| return; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| auto value_or_error = | ||||||||||||||||||||||||
| Formatter::FormatterImpl::create(mutation.append().record().value().string_value(), true); | ||||||||||||||||||||||||
| auto value_or_error = Formatter::FormatterImpl::create( | ||||||||||||||||||||||||
| mutation.append().record().value().string_value(), true, command_parsers); | ||||||||||||||||||||||||
| SET_AND_RETURN_IF_NOT_OK(value_or_error.status(), creation_status); | ||||||||||||||||||||||||
| query_query_parameter_mutations_.emplace_back(std::make_unique<QueryParameterMutationAppend>( | ||||||||||||||||||||||||
| mutation.append().record().key(), std::move(value_or_error.value()), | ||||||||||||||||||||||||
|
|
@@ -148,12 +157,18 @@ void Mutations::mutateRequestTrailers(Http::RequestTrailerMap& trailers, | |||||||||||||||||||||||
| PerRouteHeaderMutation::PerRouteHeaderMutation(const PerRouteProtoConfig& config, | ||||||||||||||||||||||||
| Server::Configuration::ServerFactoryContext& context, | ||||||||||||||||||||||||
| absl::Status& creation_status) | ||||||||||||||||||||||||
| : mutations_(config.mutations(), context, creation_status) {} | ||||||||||||||||||||||||
| : PerRouteHeaderMutation(config, context, ProtobufMessage::getNullValidationVisitor(), | ||||||||||||||||||||||||
| creation_status) {} | ||||||||||||||||||||||||
|
Contributor
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. Using
Suggested change
References
Author
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. Fair point. prior to these changes there was no validation visitor used in this path. In these changes I left the tests using the 3-arg constructor, and defaulted to the null validation visitor. But I've now dropped it, and pass in the visitor explicitly from call sites instead. Pushed as fixup commit. |
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| PerRouteHeaderMutation::PerRouteHeaderMutation( | ||||||||||||||||||||||||
| const PerRouteProtoConfig& config, Server::Configuration::ServerFactoryContext& context, | ||||||||||||||||||||||||
| ProtobufMessage::ValidationVisitor& validation_visitor, absl::Status& creation_status) | ||||||||||||||||||||||||
| : mutations_(config.mutations(), context, validation_visitor, creation_status) {} | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| HeaderMutationConfig::HeaderMutationConfig(const ProtoConfig& config, | ||||||||||||||||||||||||
| Server::Configuration::ServerFactoryContext& context, | ||||||||||||||||||||||||
| absl::Status& creation_status) | ||||||||||||||||||||||||
| : mutations_(config.mutations(), context, creation_status), | ||||||||||||||||||||||||
| : mutations_(config.mutations(), context, context.messageValidationVisitor(), creation_status), | ||||||||||||||||||||||||
| most_specific_header_mutations_wins_(config.most_specific_header_mutations_wins()) {} | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| void HeaderMutation::maybeInitializeRouteConfigs(Http::StreamFilterCallbacks* callbacks) { | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
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.
I guess the previous header mutation constructor is unnecessary (except for test)? Could we remove it?
Uh oh!
There was an error while loading. Please reload this page.
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.
For some reason I missed the human review and only saw the robot review below. In any case, you're sort of in agreement, and the 3-arg constructor is gone.