davidesalerno commented on issue #2866: URL: https://github.com/apache/apisix-ingress-controller/issues/2866#issuecomment-5952315121
## Code Review: Fix for HTTPRoute Priority Bug I have analyzed the uncommitted changes on branch `fix/httprout-path-priority` against this issue. Below is my review. ### Summary of Changes **Files modified:** - `internal/adc/translator/httproute.go` (+105/-31) - `internal/adc/translator/httproute_test.go` (+193/-5) ### What the Changes Do 1. **PathPrefix and PathExact now use `vars` + `/*` instead of specific URIs** — Previously, `PathPrefix` generated `uris: ["/myservice", "/myservice/*"]` which created a specific URI match that always beat `vars`-based routes regardless of priority. Now all path types use `uris: ["/*"]` with matching logic in `vars`, making priority the sole ordering mechanism. 2. **Priority calculation now includes path type and path length** — New `httpRoutePathPriority()` function ranks: `Exact (3) > RegularExpression (2) > PathPrefix (1)`. Path length is also factored in (up to 1023 chars) so longer prefixes get higher priority. 3. **HTTPRoutePolicy priority is now optional** — Policies with `nil` priority preserve the controller-calculated priority. Only explicit priorities override it. 4. **Deterministic policy ordering** — Policies are sorted by namespace/name before application, ensuring reproducible results regardless of list order. 5. **PathPrefix regex uses `regexp.QuoteMeta()`** — Prevents regex injection when the prefix contains special characters like `.`, `+`, etc. ### Alignment with Issue #2866 The issue reports that `PathPrefix` routes with specific URIs always beat `RegularExpression` routes using `vars`, regardless of the `priority` field. This fix directly addresses that by: - Making PathPrefix use `vars` instead of specific URIs — **✓ fixes the core issue** - Including path-length in priority — **✓ as requested** (referencing Kong approach) - Ranking Regex > Prefix in priority — **✓ as requested** ### Test Coverage - `TestTranslateGatewayHTTPRoutePathMatches` — verifies all path types produce correct `vars` output, including regex metacharacter escaping and segment boundary enforcement - `TestCalculateHTTPRoutePriorityPathPrecedence` — verifies priority ordering: Exact > Regex > longerPrefix > prefix, and that values stay < 2^53 (Lua number safety) - `TestFillHTTPRoutePoliciesPriorityAndVars` — verifies nil vs explicit policy priority behavior - `TestFillHTTPRoutePoliciesUsesDeterministicPriorityOrder` — verifies deterministic policy application - Existing `TestTranslateHTTPRouteServerPortVarsByMode` updated for new root-prefix `vars` format **All 226 tests pass, `go vet` is clean.** ### Potential Concerns 1. **Backward compatibility**: Existing APISIX routes using `uris: ["/prefix", "/prefix/*"]` will be replaced with `uris: ["/*"]` + `vars`. This is a behavioral change — if users have external tools or monitoring that inspect the `uris` field directly, they will see different values. However, the matching behavior should be identical for standard Gateway API semantics. 2. **Performance**: Adding `vars` checks for every route means APISIX evaluates an extra condition per route. For most deployments this is negligible, but very high-traffic setups with many routes should benchmark. 3. **Root prefix (`/`) edge case**: The regex `^/` correctly matches all paths. Good. 4. **Path length cap (1023)**: Paths longer than 1023 chars are capped. This is reasonable since such paths are extremely rare, and the Kong reference implementation uses similar bounds. ### Suggestions Before PR - Consider adding a changelog entry or release notes fragment documenting this behavioral change - The branch name has a typo (`httprout` → `httproute`), but that is cosmetic - Consider adding an integration/e2e test that reproduces the exact scenario from the issue (PathPrefix + Regex route conflict resolved by priority) ### Verdict: **Looks solid** — the changes directly address the reported issue with good test coverage and align with the Kong ingress controller approach referenced in the issue. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
