fix(gateway): normalize operation HTTP method case#2144
Conversation
📝 WalkthroughOverviewFixes HTTP method routing failures (404 Not Found) caused by a mismatch between how the validator and xDS translator handled HTTP method case. The validator was accepting lowercase methods by uppercasing them only for enum validation while leaving the stored value unchanged, which the translator then used directly in Envoy's case-sensitive ChangesCore Translation Logic (translator.go)
Validation (api_validator_test.go)
Test Coverage
RationaleNormalizing methods at translation time rather than rejecting them heals existing deployments with lowercase methods on their next xDS rebuild without requiring redeploy or migration. The approach is consistent with existing on-prem behavior and adds no new failure modes while fixing the 404 routing issue. WalkthroughThe xDS translator is updated to normalize HTTP methods to uppercase at three translation call sites: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain modules listed in go.work or their selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
Lowercase methods passed validation but were stored as-is and never matched Envoy's case-sensitive :method matcher, so the API deployed but every request returned 404. Require the uppercase contract exactly.
mehara-rothila
force-pushed
the
fix/lowercase-http-method-404
branch
from
45b2889 to
a47d1c8
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
Rejecting non-uppercase methods would risk existing deployments: an API already running with a lowercase method would newly fail validation on its next update or restart, while still not routing. Instead, normalize the method to uppercase in the xDS translator when building the route, where it becomes Envoy's case-sensitive :method matcher. Any-case methods now route, and configurations stored before this change are healed on their next xDS rebuild without a redeploy. Revert the deploy-time rejection so the validator again accepts any case, drop its tests, and add integration tests covering lowercase, mixed-case, and two methods on one path. Fixes wso2#2140
mehara-rothila
changed the title
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gateway/it/features/http-method-case.feature (1)
88-118:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScenario intent and assertions are incomplete for dual-method routing.
This scenario defines both
getandPoston the same path, but only validatesGET. Add aPOSTrequest assertion so the test fully validates the stated behavior.Suggested patch
Scenario: Two methods on one path, mixed case, each routes @@ When I clear all headers And I set request host to "method-case.local" And I send a GET request to "http://localhost:8080/method-multi/v1.0/resource" Then the response should be successful And the JSON response field "path" should be "/resource" + + When I clear all headers + And I set request host to "method-case.local" + And I send a POST request to "http://localhost:8080/method-multi/v1.0/resource" + Then the response should be successful + And the JSON response field "path" should be "/resource"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gateway/it/features/http-method-case.feature` around lines 88 - 118, The scenario "Two methods on one path, mixed case, each routes" defines two operations (get and Post) on the same path but only validates the GET request behavior. Add a POST request test after the existing GET request assertions to complete the test coverage. The new test should send a POST request to the same endpoint "http://localhost:8080/method-multi/v1.0/resource" with the same host header, assert that the response is successful, and verify that the JSON response field "path" matches "/resource", mirroring the validation structure used for the GET request.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@gateway/it/features/http-method-case.feature`:
- Around line 88-118: The scenario "Two methods on one path, mixed case, each
routes" defines two operations (get and Post) on the same path but only
validates the GET request behavior. Add a POST request test after the existing
GET request assertions to complete the test coverage. The new test should send a
POST request to the same endpoint
"http://localhost:8080/method-multi/v1.0/resource" with the same host header,
assert that the response is successful, and verify that the JSON response field
"path" matches "/resource", mirroring the validation structure used for the GET
request.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4771dd88-27f7-47c2-a869-849a03c2bbda
📒 Files selected for processing (2)
gateway/gateway-controller/pkg/xds/translator.gogateway/it/features/http-method-case.feature
Cover that the xDS translator uppercases the operation method into Envoy's :method matcher (createRouteFromRDC), and that the validator accepts lower- and mixed-case methods.
The two-methods-on-one-path scenario declared both get and Post but only asserted GET. Add the POST assertion so it fully covers the stated "each routes" behavior.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai does uppercasing the method only in the translator cover every route path, or could a method still reach Envoy un-normalized (policies, request transformation, route name)? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Read the full translator file
cat gateway/gateway-controller/pkg/xds/translator.goLength of output: 106005 🏁 Script executed: #!/bin/bash
# Search for any other places in the codebase where HTTP methods are used for Envoy route/header matching
rg -n --type go 'method\b' gateway/gateway-controller/pkg/xds/ -C 3Length of output: 20115 🏁 Script executed: #!/bin/bash
# Check if there are policy-related or request-transformation files that use HTTP methods
rg -rn --type go '\.Method\b' gateway/gateway-controller/pkg/ -C 3Length of output: 44706 🏁 Script executed: #!/bin/bash
# Find all route name generation patterns
rg -rn --type go 'routeName\|route_name\|RouteName' gateway/gateway-controller/pkg/xds/ -C 3Length of output: 389 🏁 Script executed: #!/bin/bash
# Check if policies or interceptors also use the HTTP method for matching
rg -rn --type go 'method.*header\|header.*method\|:method\|HeaderMatcher' gateway/gateway-controller/pkg/ -C 4Length of output: 439
1. Policy engine route key mismatch (legacy path)In // translator.go – createRoute
method = strings.ToUpper(method) // ← normalized
routeName := GenerateRouteName(method, context, "", path, vhost) // e.g. "GET|/path|vhost"But in the policy builder the method is not uppercased: // policy/builder.go
RouteKey: xds.GenerateRouteName(string(op.Method), apiData.Context, apiData.Version, op.Path, vhost),
// ^^^^^^^^^^^^^^^^^ original case, e.g. "get"For a lowercase-method API on the legacy path, Envoy's route name is 2. Envoy route name retains original case (transformer path)In the transformer path, // transform/restapi.go
routeKey := xds.GenerateRouteName(string(op.Method), ...) // e.g. "get|/path|vhost"
rdc.Routes[routeKey] = &models.Route{Method: string(op.Method), ...}
Summary table
The recommended fix is to uppercase in the two missing spots: // transform/restapi.go
routeKey := xds.GenerateRouteName(strings.ToUpper(string(op.Method)), ...)
rdc.Routes[routeKey] = &models.Route{Method: strings.ToUpper(string(op.Method)), ...}// policy/builder.go
RouteKey: xds.GenerateRouteName(strings.ToUpper(string(op.Method)), apiData.Context, apiData.Version, op.Path, vhost),Or, normalize once in |
The :method matcher was normalized, but route names and policy keys are built by GenerateRouteName from the raw operation method in several callers (createRoute, the RDC transform path, and the policy builder). A lowercase-method route could get an uppercase :method matcher but a lowercase policy key, so the policy chain would not bind to it. Uppercase the method inside GenerateRouteName, the single function that builds both the Envoy route name and the policy-engine key, so they stay consistent with the matcher whatever case the operation was declared in.
|
@coderabbitai good catch. Addressed by normalizing the method inside |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Verify GenerateRouteName implementation
ast-grep --pattern 'func GenerateRouteName($$$) $_ {
$$$
}'
# Also check for the test
rg -n "TestGenerateRouteName_UppercasesMethod" --type go -C 5Length of output: 1250 🏁 Script executed: #!/bin/bash
rg -n "GenerateRouteName" gateway/gateway-controller/pkg/xds/ -C 5Length of output: 7388
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
|
@coderabbitai please do a full review of the latest commits and tell us whether the HTTP method normalization fix is now complete, or if anything else is still needed. Specifically check: the :method matcher, route names and policy keys (GenerateRouteName), request transformation, and whether the unit + integration tests are sufficient. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Read the GenerateRouteName function and surrounding context
rg -n "GenerateRouteName" gateway/gateway-controller/pkg/xds/ --type go -C 8
# Read the test for GenerateRouteName
rg -n "TestGenerateRouteName" gateway/gateway-controller/pkg/xds/ --type go -A 30Length of output: 16025 🏁 Script executed: #!/bin/bash
# Read the createRouteFromRDC and createRoute functions (method-related parts)
rg -n "strings.ToUpper\|ToUpper\|\.Method\b" gateway/gateway-controller/pkg/xds/translator.go -C 5Length of output: 432 🏁 Script executed: #!/bin/bash
# Check policy builder for method usage
rg -n "GenerateRouteName\|\.Method\b\|op\.Method\|strings\.ToUpper" gateway/gateway-controller/pkg/policy/ --type go -C 5
# Check transform/restapi.go for method usage
rg -n "GenerateRouteName\|\.Method\b\|op\.Method\|strings\.ToUpper" gateway/gateway-controller/pkg/transform/ --type go -C 5Length of output: 588 🏁 Script executed: #!/bin/bash
# Read the integration test feature file
cat gateway/it/features/http-method-case.feature
# Check if there are any other places that build :method headers directly
rg -n '":method"\|header.*[Mm]ethod\|[Mm]ethod.*header' gateway/gateway-controller/ --type go -C 4Length of output: 4989 🏁 Script executed: #!/bin/bash
# Check the :method matcher construction in createRoute and createRouteFromRDC
grep -n "method\|:method\|ToUpper" gateway/gateway-controller/pkg/xds/translator.go | head -80Length of output: 1481 🏁 Script executed: #!/bin/bash
# Check policy builder
find gateway/gateway-controller/pkg/policy -name "*.go" | xargs grep -n "GenerateRouteName\|Method\|ToUpper" 2>/dev/null | head -60
# Check transform
find gateway/gateway-controller/pkg/transform -name "*.go" | xargs grep -n "GenerateRouteName\|Method\|ToUpper" 2>/dev/null | head -60Length of output: 2064 🏁 Script executed: #!/bin/bash
# Look at createRoutePerTopic :method matcher - does it normalize the method value in the header matcher?
sed -n '1927,1970p' gateway/gateway-controller/pkg/xds/translator.go
# Also look at createRouteFromRDC method handling
grep -n "rdcRoute.Method\|strings.ToUpper\|:method" gateway/gateway-controller/pkg/xds/translator.goLength of output: 2125
|
| Test | Coverage |
|---|---|
TestGenerateRouteName_UppercasesMethod (translator_test.go) |
lowercase, mixed-case, already-uppercase inputs → ✅ |
:method matcher assertion (translator_test.go Line 779) |
verifies Envoy header matcher value is uppercase → ✅ |
TestGenerateRouteName (xds_test.go) |
only uppercase inputs; not a gap since translator_test.go covers the normalization cases |
Integration tests
Three scenarios in http-method-case.feature cover:
- Lowercase
get→ routes viaGET. ✅ - Mixed-case
gEt→ routes viaGET. ✅ - Two operations on the same path with
getandPost→ both route independently. ✅
One scenario to consider adding: verify that an operation-level policy bound to a lowercase-method operation is applied correctly (i.e., the policy chain actually fires), since the previous gap was specifically a policy key mismatch. This would catch any regression at the policy binding layer.
Summary
The fix is functionally complete. The :method matcher, route names, and policy keys are all consistently uppercase. The only remaining item is:
- Minor: Normalize
Methodintransform/restapi.goLine 166 to avoid storing original-case strings in the RDC model. - Optional: Add an integration test scenario that exercises a policy attached to a lowercase-method operation to confirm the policy chain binds correctly end-to-end.
The integration harness runs an explicit feature list in getFeaturePaths (suite_test.go), not auto-discovery, so the new feature file must be listed there to actually run.
mehara-rothila
requested review from
AnuGayan,
Arshardh,
CrowleyRajapakse,
HeshanSudarshana,
HiranyaKavishani,
Krishanx92,
PasanT9,
RakhithaRR,
Tharsanan1,
VirajSalaka,
ashera96,
chamilaadhi,
dushaniw,
hisanhunais,
malinthaprasan,
pubudu538,
renuka-fernando,
senthuran16,
tgtshanika,
tharikaGitHub and
tharindu1st
as code owners
There was a problem hiding this comment.
🧹 Nitpick comments (1)
gateway/gateway-controller/pkg/xds/translator_test.go (1)
741-803: ⚡ Quick winConsider adding test coverage for
createRoutemethod normalization.The tests verify method uppercasing in
createRouteFromRDC(line 741) andGenerateRouteName(line 785). According to the PR objectives,createRoutealso normalizes the method when building the:methodmatcher, but no test explicitly verifies this behavior. Adding a test case that callscreateRoutewith a lowercase method and asserts the:methodmatcher is uppercased would ensure both translation paths are covered.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gateway/gateway-controller/pkg/xds/translator_test.go` around lines 741 - 803, Add a new test function to verify that the createRoute method normalizes HTTP methods to uppercase in the :method matcher, similar to the existing TestTranslator_NormalizesMethodCase test but specifically targeting the createRoute method instead of createRouteFromRDC. The test should call createRoute with various lowercase and mixed-case method values (like "get", "pOsT", etc.) and assert that the resulting route's :method matcher header contains the uppercased method name (GET, POST, etc.), ensuring both translation paths for method normalization are covered by explicit test cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@gateway/gateway-controller/pkg/xds/translator_test.go`:
- Around line 741-803: Add a new test function to verify that the createRoute
method normalizes HTTP methods to uppercase in the :method matcher, similar to
the existing TestTranslator_NormalizesMethodCase test but specifically targeting
the createRoute method instead of createRouteFromRDC. The test should call
createRoute with various lowercase and mixed-case method values (like "get",
"pOsT", etc.) and assert that the resulting route's :method matcher header
contains the uppercased method name (GET, POST, etc.), ensuring both translation
paths for method normalization are covered by explicit test cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 69470b0d-8daa-406f-8260-613756a5afaf
📒 Files selected for processing (5)
gateway/gateway-controller/pkg/config/api_validator_test.gogateway/gateway-controller/pkg/xds/translator.gogateway/gateway-controller/pkg/xds/translator_test.gogateway/it/features/http-method-case.featuregateway/it/suite_test.go
✅ Files skipped from review due to trivial changes (1)
- gateway/it/suite_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- gateway/it/features/http-method-case.feature
- gateway/gateway-controller/pkg/xds/translator.go
Summary
The xDS translator maps an operation's method straight into Envoy's case-sensitive
:methodexact matcher, so an API declaring a lower or mixed case method (for exampleget) deployed successfully but returned404 Not Foundfor every request, since HTTP clients send uppercase methods. The translator now normalizes the method to uppercase when building the route, so any-case methods route correctly.Why normalize instead of reject
An earlier revision of this PR rejected non-uppercase methods at validation time. That was changed to normalization because rejecting risks existing deployments: an API already running with a lowercase method would start failing validation on its next update or restart, a new failure it did not have before, while still not routing. Normalizing instead heals those configurations (404 becomes 200) on the next xDS rebuild without a redeploy, and adds no new failure mode for anyone. Uppercasing the method is also consistent with existing handling in the codebase: the on-prem APIM sync already uppercases the method for its verb field.
Notes for reviewers
createRouteFromRDCandcreateRoute), so the route name and the:methodmatcher stay consistent.Test coverage
gateway/it/features/http-method-case.feature: a lowercase method routes, a mixed-case method routes, and two methods on one path (mixed case) both deploy and route.go buildand thepkg/configvalidator unit tests pass locally; the integration scenarios run in CI.Fixes #2140