Repository navigation
fix(proxy): bound RoundRobinSelector return to current n (#250) - #251
Merged
Merged
Conversation
When alive-backend count shrinks between calls (e.g. health-check flips a backend dead), the previously stored lastSelected could exceed the new n. The returned index was the un-bounded stale value, causing matchHandler to index out of range and panic. Apply modulo before returning, so Select is always in [0, n) regardless of how lastSelected was set on a prior call. Adds regression test that reproduces the panic with shrinking n. Related to #250
Replace int32+sync/atomic free functions with atomic.Int32 type in test files (modernize/atomictypes lint), and suppress gosec G710 on the http->https redirect handler with explanation - redirect target is derived from the request host, matching the standard upgrade pattern.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a panic in the round-robin load balancer selector by ensuring the selected backend index is always bounded to the current number of alive backends, even when that count shrinks between calls (issue #250). It also includes small test/lint cleanups (typed atomics in tests and a gosec suppression on an HTTP→HTTPS redirect).
Changes:
- Fix
RoundRobinSelector.Selectto return an index in[0, n)even if the previous stored index is stale due to backend count shrink. - Add a regression test covering the shrinking-
nround-robin scenario. - Modernize some tests to use
atomic.Int32and add an inline//nolint:gosecon the redirect handler.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
app/proxy/lb_selector.go |
Adjusts round-robin selection logic to avoid out-of-range indices when backend count changes. |
app/proxy/lb_selector_test.go |
Adds coverage for the shrinking-backend-count round-robin case. |
app/proxy/ssl.go |
Adds gosec suppression on the HTTP→HTTPS redirect. |
app/proxy/proxy_test.go |
Updates test counter to use atomic.Int32. |
app/proxy/handlers_test.go |
Updates limiter tests to use atomic.Int32. |
lib/plugin_test.go |
Updates plugin test counters to use atomic.Int32. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+76
to
+90
| newURL += "?" + r.URL.RawQuery | ||
| } | ||
| http.Redirect(w, r, newURL, http.StatusTemporaryRedirect) | ||
| http.Redirect(w, r, newURL, http.StatusTemporaryRedirect) //nolint:gosec // standard http->https upgrade, redirect target derived from request host |
| // bound to current n: alive-backend count can shrink between calls | ||
| // (health-check flips), so the previously stored index may be out of range | ||
| selected := r.lastSelected % n | ||
| r.lastSelected = selected + 1 |
Apply modulo on lastSelected write to keep [0, n) invariant - matches original code's wrap-to-0 semantics when n changes, while still bounding the returned value to current n. Both forms fix the panic; this one has smaller behavioral delta from pre-fix code. Add TestRoundRobinSelector_SelectGrowingN that pins the wrap behavior when n grows back (e.g., dead backend recovers), catching regression to the non-modulo write. Expand the //nolint:gosec comment on the http->https redirect to spell out the threat model (Host header is browser-set from URL hostname, so a foreign Host can't be injected into a victim's request).
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the panic reported in #250.
RoundRobinSelector.Selectreturned the un-bounded stalelastSelected, applying modulo only to the next iteration's seed. When the alive-backend count shrinks between calls (a backend health-check flips dead), the stored index can exceed the newn, andmatchHandlerindexes out of range inproxy.go:352.Repro: call
Select(3)twice (returns 0, then 1, leaveslastSelected=2), thenSelect(2)returns 2 into a 2-element slice. Exact match to the reporter'sindex out of range [2] with length 2.Fix: apply modulo to
lastSelectedbefore returning, soSelectis always in[0, n)regardless of how the previous call left the state. First-call-returns-0 behavior preserved.Tests: added
TestRoundRobinSelector_SelectShrinkingNcovering the shrinking-n path. Existing tests still pass, race detector clean.History: the bug shipped in commit fa23778 (2023-11-27) and was live across v1.1.0 through v1.5.0, ~2.5 years. Existing tests only called
Selectwith constantn, so the path was never exercised. Round-robin is opt-in via--lb-type=roundrobin, default is random which doesn't hit this code.Cleanup commit: also addresses 6 pre-existing lint findings unrelated to the fix - replaces
int32+sync/atomicfree functions withatomic.Int32in test files (modernize/atomictypes), and adds an inline//nolint:gosecwith explanation on the standard http->https redirect handler (G710 taint analysis false positive, redirect target derives from request host).Related to #250