formatter: expose CEL expression config in header mutations#45815
formatter: expose CEL expression config in header mutations#45815kamalmarhubi wants to merge 4 commits into
Conversation
|
Hi @kamalmarhubi, welcome and thank you for your contribution. We will try to review your Pull Request as quickly as possible. In the meantime, please take a look at the contribution guidelines if you have not done so already. |
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
|
@adisuissa fyi the API was already given the ok by @wbpcode here: #45420 (comment) |
7044561 to
999c2c7
Compare
999c2c7 to
6c24f65
Compare
6c24f65 to
e9592c3
Compare
CelExpressionConfig on the CEL formatter|
Reviewer note: this is broken down into hopefully digestible commits. |
c2b848f to
76150e5
Compare
76150e5 to
4dd2a3a
Compare
4dd2a3a to
8a1f4c0
Compare
8a1f4c0 to
cbb0af8
Compare
Add `cel_config` to `envoy.formatter.cel` configuration, allowing CEL runtime options to be enabled on a per-formatter basis. This is prerequisite work for envoyproxy#45420: the header mutation filters need to enable CEL string functions through formatter configuration, but the CEL formatter extension did not expose the underlying CEL expression runtime options. The built-in CEL parser still uses the active server context, so existing unconfigured CEL formatter command usage is unchanged. Signed-off-by: Kamal Al Marhubi <kamal@marhubi.com>
Header mutation values already use substitution format strings, but the HTTP filter had no API surface for configured formatter parsers. Add `formatters` to the filter config and thread the parsed parsers through top-level and per-route mutation construction. Built-in parsers remain available when the field is empty. Signed-off-by: Kamal Al Marhubi <kamal@marhubi.com>
Early header mutation values also use substitution format strings, but the extension had no API surface for configured formatter parsers. Add `formatters` to the early header mutation config and pass parsed parsers into header mutation construction. Built-in parsers remain available when the field is empty. Fixes envoyproxy#45420 Signed-off-by: Kamal Al Marhubi <kamal@marhubi.com>
cbb0af8 to
82839a5
Compare
wbpcode
left a comment
There was a problem hiding this comment.
I think this! Thanks for this great contribution. Only one comment.
| 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) {} |
There was a problem hiding this comment.
I guess the previous header mutation constructor is unnecessary (except for test)? Could we remove it?
There was a problem hiding this comment.
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.
|
/gemini review |
|
/wait |
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to configure extension formatter command parsers (such as CEL with custom runtime options) for header mutation substitution values in both the HTTP header mutation filter and the early header mutation extension. It also exposes cel_config on the CEL formatter to allow enabling CEL runtime options like string functions. One improvement opportunity was identified in header_mutation.cc to use the context's messageValidationVisitor() instead of a null validation visitor to ensure proper configuration validation.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| 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) {} |
There was a problem hiding this comment.
Using ProtobufMessage::getNullValidationVisitor() bypasses message validation. Since ServerFactoryContext provides a valid messageValidationVisitor(), we should use it instead to ensure any configuration issues or deprecations are properly validated and logged.
| 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) {} | |
| PerRouteHeaderMutation::PerRouteHeaderMutation(const PerRouteProtoConfig& config, | |
| Server::Configuration::ServerFactoryContext& context, | |
| absl::Status& creation_status) | |
| : PerRouteHeaderMutation(config, context, context.messageValidationVisitor(), | |
| creation_status) {} |
References
- In Envoy, the validation_visitor object has a server-level lifetime, making it safe to capture by reference in lambdas even when those lambdas are stored in long-lived components like singleton-managed providers.
There was a problem hiding this comment.
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.
Signed-off-by: Kamal Al Marhubi <kamal@marhubi.com>
adisuissa
left a comment
There was a problem hiding this comment.
The api change LGTM.
/lgtm api
IMHO having a single PR doing 3 concurrent changes is a bit much, but I'll leave it to the extension owners reviewers.
adisuissa
left a comment
There was a problem hiding this comment.
/assign @wbpcode @yanavlasov @kyessenov
Commit Message:
formatter: expose CEL expression config in header mutations
Add
cel_configtoenvoy.formatter.celconfiguration, allowing CEL runtime options such as string functions to be enabled on a per-formatter basis. Include aformattersfield on the header mutation and early header mutation filters so those filters can use configured formatter command parsers, including CEL withcel_config.Specific changes:
envoy.formatter.celparsers now carrycel_config. The built-in%CEL%parser still uses the active server context, so existing unconfigured usage is unchanged.formattersfor configuring extension formatter command parsers used in mutation values.Built-in command parsers remain available when
formattersis empty, so there is no behaviour change unless a config opts in.Fixes #45420
Additional Description: See #45420 for motivation: enabling CEL string functions in (early) header mutation, eg to transform trace-context headers.
AI Disclosure: This was developed by iterating with OpenAI Codex (GPT-5.5), with review and further iteration using Claude Code (Opus 4.8). I reviewed and edited the generated output, understand the submitted changes, and take responsibility for them.
Risk Level: Low
Testing:
Docs Changes: Inline proto documentation for the new
cel_configandformattersfields.Release Notes: Added (formatter, header_mutation, early_header_mutation).
Platform Specific Features: N/A. (The CEL formatter is already excluded on Windows via WINDOWS_SKIP_TARGETS; I haven't changed that.)
API Considerations: adds optional
cel_config(reusingconfig.core.v3.CelExpressionConfig) toenvoy.extensions.formatter.cel.v3.Cel, and optionalformatters(repeated config.core.v3.TypedExtensionConfig) to the two header mutation messages. All additive; omitting them preserves existing behaviour.