Repository navigation
xds: lowercase RDS route header matcher names before matching - #9353
Conversation
The metadata that the header matchers run against always has lowercase keys, so a route header matcher configured with a name such as X-Role matched no header and the route never fired. Lowercase the name when the route configuration is parsed, which is what Envoy does (it holds the name in a LowerCaseString), what the RBAC HTTP filter does since grpc#9332, and what authz/rbac_translator.go does on the non-xDS path.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9353 +/- ##
==========================================
+ Coverage 87.42% 87.59% +0.16%
==========================================
Files 424 429 +5
Lines 30283 30622 +339
==========================================
+ Hits 26476 26824 +348
+ Misses 3806 3798 -8
+ Partials 1 0 -1
🚀 New features to boost your workflow:
|
eshitachandwani
left a comment
eshitachandwani
left a comment
There was a problem hiding this comment.
LGTM modulo minor nits. Adding @easwars as a second reviewer
| // The metadata the matchers run against always has lowercase keys, | ||
| // so a name that contains an uppercase character matches no header | ||
| // and the route holding it never fires. Lowercase the name, as | ||
| // Envoy and the RBAC filter do. |
There was a problem hiding this comment.
nit: we can remove the last sentence.
| // header matcher name is not lowercase still matches an RPC carrying that | ||
| // header. Metadata keys are always lowercase, so the name has to be | ||
| // normalized when the route configuration is parsed. | ||
| func (s) TestRoutesProtoToSliceHeaderMatcherNameCase(t *testing.T) { |
There was a problem hiding this comment.
nit: I think this test is redundant and we can remove this since we have already added a test case in the test above
Trim the trailing sentence of the header-name normalization comment, and remove the standalone TestRoutesProtoToSliceHeaderMatcherNameCase test (the non-lowercase header-name case is already covered by a table entry in TestRoutesProtoToSlice), dropping the now-unused metadata import.
|
Thanks @eshitachandwani — addressed both nits: trimmed the last sentence of the comment, and removed the standalone |
|
/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 updates the RDS unmarshaling logic to convert header matcher names to lowercase, ensuring they match correctly against metadata keys which are always lowercase. A corresponding unit test has been added to verify this behavior. There are no review comments, so I have no feedback to provide.
Fixes #9352
xDS RDS route header matchers store the header name verbatim from the config in
routesProtoToSlice:The matcher engine looks that name up against
metadata.MD, whose keys are always lowercase, so a matcher configured with a mixed-case name such asX-Rolematches no header and the route never fires.This is the last consumer of that matcher engine that does not normalize the name. The RBAC HTTP filter started lowercasing it at parse time in #9332 (
normalizeHeaderMatcher), andauthz/rbac_translator.godoes the same on the non-xDS path. Envoy storesHeaderMatcher.namein aHttp::LowerCaseString, so this is valid config that works on other proxies.The fix lowercases the name where the route header matcher is built, matching the sibling paths. Added a table test covering a mixed-case name against lowercased metadata, and asserting the parsed matcher stores the lowercased name; it fails on
masterand passes with the change.RELEASE NOTES: