Fix non-deterministic router priority for hosts with multiple Targets/paths - #114
Conversation
…s/paths findMatchingRouter() matched routers only on Host(...) and returned the first router found while iterating an unordered Go map. When a host is served by more than one non-redirect router - e.g. Pangolin's multi-target path routing, where one resource with several Targets produces multiple routers sharing an identical Host() but differing PathPrefix() - the function returned whichever router Go's randomized map iteration visited first. Since the router map is freshly deserialized on every 5s cache refresh, the winner changed from call to call, and the caller (applyResourceOverrides) mutates the returned router's `priority` field in place - so a resource's RouterPriority override could land on the wrong sibling router, corrupting its priority and creating a priority tie between routers whose relative order must stay fixed. Fix: sort matches deterministically, and when more than one non-redirect router shares a host, treat it as ambiguous (host alone cannot identify the correct router) and report no match instead of guessing. The caller already skips the override entirely when no router is found, so this leaves Pangolin's own router configuration - including its own priority - untouched in the ambiguous case, which is strictly safer than corrupting it. Fixes hhftechnology#113
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
@strausmann is attempting to deploy a commit to the HHF Technologies' projects Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
📜 Recent review details
|
| Layer / File(s) | Summary |
|---|---|
Deterministic host matching services/config_proxy.go |
findMatchingRouter sorts host matches, prefers one non-redirect router, retains redirect-only fallback, and returns no match for multiple non-redirect routers. |
Override safety regression coverage services/config_proxy_test.go |
Tests cover unmatched hosts, redirect-only fallback, ambiguous matches, and unchanged priorities and middleware overrides. |
Estimated code review effort: 3 (Moderate) | ~20 minutes
Possibly related issues
hhftechnology/middleware-manager#113— The pull request implements deterministic host fallback and prevents ambiguous router overrides.
Poem
A rabbit sorts routers by name,
Rejects two targets in the same lane.
Redirects may still provide a way,
While overrides safely stay.
No random hop, no muddled trail—
The matching rules now prevail.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title clearly and concisely describes the fix for non-deterministic router priority when hosts have multiple targets or paths. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
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 help to get the list of available commands.
There was a problem hiding this comment.
Pull request overview
This PR fixes non-deterministic router selection in ConfigProxy.findMatchingRouter() when multiple Traefik routers share the same Host(...) rule (common with Pangolin multi-target path routing). The change makes router matching deterministic and avoids incorrectly applying RouterPriority overrides to the wrong sibling router, preventing sporadic priority ties and misrouting.
Changes:
- Deterministically sorts host-matched routers before selection to remove dependence on randomized Go map iteration order.
- Treats “multiple non-redirect routers share the same host” as ambiguous and returns no match (so overrides are skipped rather than applied incorrectly).
- Adds targeted tests covering both the router-matching behavior and the end-to-end override-skipping regression case.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| services/config_proxy.go | Makes host-based router fallback deterministic and safely returns no match for ambiguous multi-router hosts. |
| services/config_proxy_test.go | Adds regression and behavior tests to ensure ambiguous multi-target hosts do not receive incorrect overrides. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@services/config_proxy_test.go`:
- Around line 232-259: The tests in services/config_proxy_test.go lack coverage
for findMatchingRouter’s redirect-only fallback (case 0). Add a test with only
host-matching router names ending in “-redirect”, call findMatchingRouter
repeatedly, and assert it consistently returns the same expected name and
router.
In `@services/config_proxy.go`:
- Around line 1064-1088: Remove the ineffective websecure entrypoint loop and
its misleading safety comment from the case 1 branch of the nonRedirect
selection switch. Return the sole non-redirect candidate directly, preserving
the existing behavior that does not require a websecure entrypoint.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: da2bb5a7-a408-47e1-8895-87accedf4a47
📒 Files selected for processing (2)
services/config_proxy.goservices/config_proxy_test.go
📜 Review details
⚠️ CI failures not shown inline (1)
Commit Status: Vercel: Vercel
Conclusion: failure
Authorization required to deploy.
🔇 Additional comments (4)
services/config_proxy.go (2)
989-1015: LGTM!
1045-1063: LGTM!services/config_proxy_test.go (2)
171-230: LGTM!
261-332: LGTM!
…rect-only fallback (review feedback) CodeRabbit review on PR hhftechnology#114 flagged two findings: - The case-1 branch of findMatchingRouter checked router entryPoints for "websecure" but returned the same router either way, making the check dead code. Removed it; behavior is unchanged. - The case-0 branch (host matches only -redirect routers, deterministic fallback to the first match) had no test coverage. Added a subtest that asserts the fallback stays deterministic across repeated calls.
Fixes #113.
Problem
findMatchingRouter()matched routers only onHost(...)and returned the first router encountered while iterating an unordered Go map. When a host is served by more than one non-redirect router — e.g. Pangolin's multi-target path routing, where a single resource with several Targets produces multiple routers sharing an identicalHost()but differingPathPrefix()— the function returned whichever router Go's randomized map iteration visited first.Because the router map is freshly deserialized on every 5s cache refresh, the "winner" changed from call to call. The caller
applyResourceOverridesmutates the returned router'spriorityfield in place, so a resource'sRouterPriorityoverride could land on the wrong sibling router, corrupting its priority and creating a priority tie between routers whose relative order must stay fixed — causing sporadic misrouting on shared hosts.Fix
Tests
Added
TestConfigProxyFindMatchingRouterandTestApplyResourceOverridesSkipsAmbiguousMultiTargetHostinservices/config_proxy_test.go, verified TDD-style: against the pre-fix code the new tests fail (e.g. the catch-all router's priority is mutated to a sibling's value); with the fix they pass.go build ./...is clean and the changed-code tests pass (go test -run 'ConfigProxy|ApplyResourceOverrides' ./services/).Unrelated heads-up (not touched by this PR):
go test ./...currently fails to compile theservicestest binary becauseservices/resource_watcher_test.gocompareswatcher.isRunningas a barebool, while the field is anatomic.Bool. This pre-exists this change; the fix here is confined toconfig_proxy.go.Real-world validation
Built from this branch and run as an isolated, read-only instance against a production Pangolin deployment, sampled side-by-side with the current (unpatched) build over 6 minutes (5s interval):
Possible follow-up
There is a related disambiguation in
services/resource_watcher.go(updateOrCreateResourceselects by host without path disambiguation). I kept it out of this PR to keep the change focused, and its existing tests are currently skipped upstream. Happy to address it separately if that would help.Summary by CodeRabbit
Bug Fixes
Tests