xds: implement HeaderMutator to apply HTTP header mutations (A102)#9192
xds: implement HeaderMutator to apply HTTP header mutations (A102)#9192mbissa wants to merge 4 commits into
Conversation
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces HeaderMutator in internal/xds/httpfilter to compile and apply header mutations on gRPC metadata based on Envoy's mutation rules. The review feedback highlights a critical missing feature: according to gRFC A102, binary headers (keys ending in -bin) must be base64-decoded before being added to the metadata to prevent double-encoding. The reviewer suggests adding the necessary "encoding/base64" import, implementing the decoding logic to support both padded and unpadded base64 formats, and adding comprehensive test cases to verify this behavior.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #9192 +/- ##
==========================================
- Coverage 83.27% 83.22% -0.05%
==========================================
Files 419 420 +1
Lines 33863 33923 +60
==========================================
+ Hits 28198 28234 +36
- Misses 4252 4266 +14
- Partials 1413 1423 +10
🚀 New features to boost your workflow:
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the HeaderMutator utility to apply header mutations on gRPC metadata based on Envoy's mutation rules. The feedback suggests two key improvements: first, correcting the raw_value precedence check to properly handle empty byte slices (and adding a test case for this scenario); second, optimizing the Mutate function using a copy-on-write pattern to prevent unnecessary metadata copies when no mutations are performed.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the HeaderMutator utility in internal/xds/httpfilter to apply header mutations on gRPC metadata based on Envoy's header mutation rules, utilizing a copy-on-write strategy to avoid unnecessary allocations. The feedback suggests optimizing the mutation loop by performing direct map operations on the metadata instead of calling helper methods, which avoids redundant lowercasing and slice allocations in the hot path.
mbissa
changed the title
| // with ":") or the "host" header, regardless of the configured mutation | ||
| // rules. Other headers (including "grpc-*") are subject to the | ||
| // allow/disallow rules below. | ||
| if strings.HasPrefix(key, ":") || key == "host" { |
There was a problem hiding this comment.
the gRFC says that headers with grpc- prefix are also invalid
[key](https://github.com/envoyproxy/envoy/blob/cdd19052348f7f6d85910605d957ba4fe0538aec/api/envoy/config/core/v3/base.proto#L404): The header name. When reading, the entry will be considered invalid if empty, if not all lower-case, if length exceeds 16384, if the key is host, if it starts with : or grpc-, or if it is not a valid gRPC header name.
|
|
||
| if m.rules.AllowExpr != nil { | ||
| if !m.rules.AllowExpr.MatchString(key) { | ||
| if m.rules.DisallowIsError { |
There was a problem hiding this comment.
I dont think we should check the DisallowIsError here , because the gRFC says that
[disallow_is_error]: If false, a disallowed header mutation will simply be ignored.
and what I understand from it is if a header mutation is under the Disallowed array , only then we fail.
But under current scenario if a header key is not part of either Allowed or Disallowed will also fail this check. WHich I think should not happen.
WDYT ?
There was a problem hiding this comment.
the gRFC mentions check like if the key exceed 16384 length it is considered invalid. Where will these checks be added ?
| } | ||
| } | ||
|
|
||
| func TestHeaderMutator_Mutate(t *testing.T) { |
There was a problem hiding this comment.
We should make these tests function of s
| ) | ||
|
|
||
| // hvo builds a HeaderValueOption with the legacy value field. | ||
| func hvo(key, val string, action v3corepb.HeaderValueOption_HeaderAppendAction) *v3corepb.HeaderValueOption { |
There was a problem hiding this comment.
Shouldn't we also have a test that uses values stored in RawValue
3 participants
Series Context
This is a standalone PR implementing the HTTP header mutation engine for
gRFC A102 (xDS GrpcService Support and Header Representations). It does not
depend on any other pending A102 PRs.
Overview
Implements the
HeaderMutatorengine ininternal/xds/httpfilter/headers.goas specified in gRFC A102. It evaluates Envoy's
HeaderMutationRulesagainst aset of
HeaderValueOptionmutations and applies the allowed ones to gRPCmetadata. The mutator operates on a copy of the metadata and holds only
read-only compiled state, so it is safe for concurrent use — intended for the
side-channel call paths (e.g. ext_proc) that A102 enables.
Key Changes
HeaderMutationRules(compiledallow_expression/disallow_expressionregexes), honoringdisallow_alland
disallow_is_error.:-prefixed) and thehostheader are always blocked, regardless of control-plane settings. Otherheaders (including
grpc-*) are subject to the allow/disallow rules.APPEND_IF_EXISTS_OR_ADD,ADD_IF_ABSENT,OVERWRITE_IF_EXISTS_OR_ADD, andOVERWRITE_IF_EXISTS.raw_valuetakes precedence over the legacyvaluefield; headers are retained even when a mutation yields an empty value
(
keep_empty_valueis intentionally unsupported).Testing
internal/xds/httpfilter/headers_test.gocoveringall four actions, empty-value retention,
raw_valueprecedence, thealways-protected headers, the allow/disallow rule combinations, and that the
input metadata is left unmodified.
RELEASE NOTES: n/a