feat: round-robin load balancing for HTTP/HTTPS ports

[mvanhorn]

Summary

Adds round-robin load balancing across multiple HTTP/HTTPS backends for a single proxied port. PortConfig already held targets []*url.URL but the reverse proxy always sent every request to the first target. This change lets a port spread requests across all configured targets when loadbalance=roundrobin is set.

Why this matters

Issue #436 asks for spreading traffic across horizontally scaled backends. Until now each proxy effectively used one backend even when several were configured. This is Phase 1 of that request: round-robin for HTTP/HTTPS, which covers the common case. Least-conn, health-aware draining, and TCP/UDP balancing are left for follow-ups per the maintainer's request to keep the first version small.

Changes

• internal/model/port.go: add a LoadBalance field to PortConfig, strategy constants (first, roundrobin), and a SelectTarget(strategy) method. The round-robin counter lives on targetState as an atomic.Uint64 so concurrent requests rotate safely. Selection returns a deep-copied URL (same round-trip clone GetFirstTarget already used). Default and unknown strategies fall back to first-target, preserving today's behavior.
• internal/proxymanager/port.go: the HTTP/HTTPS rewrite picks the per-request target via SelectTarget. The management auth-token check now tests the target actually selected for the request, so the token is never forwarded to a non-management backend under round-robin.
• internal/targetproviders/docker/: add a loadbalance=first|roundrobin port option (and a tsdproxy.loadbalance label constant). Unknown values log a warning and default to first.
• internal/targetproviders/list/: add a loadBalance key to list-provider ports.

TCP and UDP ports continue to use the first target, so setting round-robin on them is a no-op for now.

Testing

• go build ./...
• go test ./internal/model/... ./internal/proxymanager/... ./internal/targetproviders/docker/... ./internal/targetproviders/list/... -race

New tests cover: 3-target rotation order, single-target always-first, empty-targets returns nil without panic, unknown strategy defaults to first, returned URL is a deep copy, concurrent rotation under -race, and end-to-end round-robin distribution through proxyRewrite.

Fixes #436

AI was used for assistance.

[almeidapaulopt]

Review — Round-robin load balancing for HTTP/HTTPS ports

Thanks for this! Fills a real gap. Verdict: request changes — rebase needed, plus a few gaps worth addressing before merge.

1. Not redundant — real gap

The targets []*url.URL slice has existed for a while, and the list provider already populates N entries from YAML (list.go:427–434). But every consumer collapsed it to targets[0] via GetFirstTarget(). So the data was reachable but dead. This PR wires up actual selection. 👍

2. Bugs / correctness

🔴 Critical — PR will not merge into current main

merge-base is f9d986e. Main has 5 commits since, including b1dd087 refactor(proxymanager): split port.go into per-protocol files. This PR modifies internal/proxymanager/port.go, but on main proxyRewrite now lives in port_http.go. A git merge-tree dry-run reports a conflict in port.go. A rebase is mandatory, and non-trivial because the function moved files. The PR's CI only ran CodeFactor — no Go build/test ran against current main.

🔴 Critical — Health checker still probes only backend[0]

health.go:426 (target := pc.GetFirstTarget()) is untouched. With loadbalance=roundrobin, a dead backend at index ≥1 will still receive traffic because the health checker never probes it. The PR description lists TCP/UDP as deferred but is silent on health. Load balancing without health-aware exclusion is a known foot-gun. At minimum, the PR description should warn users explicitly; ideally SelectTarget skips known-unhealthy targets (Phase 1.5).

🟡 Medium — Docker label is parsed but has zero behavioral effect

The PR adds tsdproxy.loadbalance=first|roundrobin parsing in container.go:307–320, but generateTargetFromFirstTarget (container.go:346–350) still produces exactly one target per port (existing comment: // multiple targets not supported in this TargetProvider). SelectTarget("roundrobin") with count() <= 1 short-circuits to getFirst(). So setting the docker label today has no effect. Either:

• Implement multi-target discovery for docker (Swarm replicas, multiple tsdproxy.target.* labels, DNS SRV), or
• Document explicitly that the docker label is forward-compatible only and round-robin currently works only via the list provider.

🟡 Medium — Silent behavior change for existing list-provider users

Today, targets: [b1, b2, b3] silently uses only b1. After this PR, the same config still uses only b1 (because LoadBalance defaults to "first"). ✅ Good back-compat decision. But docs/content/docs/providers/lists.md:128 currently says "list of targets (in this version only the first will be used)" — that comment becomes stale and is not updated.

🟢 Low — List provider accepts any string for loadBalance without warning

Docker's applyPortOptions validates and warns on unknown strategies. The list provider's YAML schema (port.LoadBalance string yaml:"loadBalance") accepts anything, and SelectTarget silently falls through to first on unknown values. A typo like loadBalance: roundRobin (camelCase) or loadBalance: random silently does nothing. Suggest startup validation/warning.

🟢 Low — count() takes RLock just to read len()

Called once per request on the hot path. Negligible, but if profiling matters later, consider an atomic.Int64 count maintained in add()/replace().

🟢 Low — Atomic counter never resets on target-set changes

AddTarget/ReplaceTarget mutate the slice but nextIndex keeps growing. After an IP swap, the rotation phase shifts arbitrarily. Not a bug, just surprising. Document or reset on replace.

3. Security — the one thing this PR got right ✅

The change to check the actually-selected target for the management token is correct:

// BEFORE (vulnerable under round-robin):
if isManagementTarget(pconfig.GetFirstTarget(), httpPortStr) {
    r.Out.Header.Set(consts.HeaderAuthToken, proxyAuthToken)
}
// AFTER (correct):
if isManagementTarget(target, httpPortStr) { ... }  // target = selected for THIS request

Without this, round-robin to a non-management backend could leak the management auth token. Good catch. But there's no regression test guarding this — see test gaps below.

One thing to verify on rebase: port_http.go:337 (redirect handler) still calls GetFirstTarget(). Probably fine (redirect target ≠ backend), but worth confirming redirects cannot be combined with round-robin in a way that matters.

4. Documentation gaps — none updated

The PR adds zero files under docs/:

File	Gap
docs/content/docs/providers/docker.md:141–143	Port Options table missing loadbalance row
docs/content/docs/providers/docker-reference.md:93–95	Same — Port Options table missing loadbalance
docs/content/docs/providers/lists.md:128	Stale comment "only the first will be used" — now incorrect
docs/content/docs/providers/lists.md	No loadBalance YAML key example
docs/content/docs/advanced/roadmap.md:13	Mentions round-robin as future work — should reflect Phase 1 shipped
CHANGELOG	No entry

5. Test gaps

Tests added (good quality):

• TestPortConfig_SelectTarget_RoundRobin — 3-target rotation ✓
• TestPortConfig_SelectTarget_SingleTargetAlwaysFirst ✓
• TestPortConfig_SelectTarget_EmptyTargets ✓
• TestPortConfig_SelectTarget_UnknownStrategyDefaultsToFirst ✓
• TestPortConfig_SelectTarget_ReturnsDeepCopy ✓
• TestPortConfig_SelectTarget_ConcurrentRoundRobin — 16 goroutines × 500 under -race ✓
• TestProxyRewriteRoundRobinDistribution — end-to-end through proxyRewrite ✓

Missing:

Missing test	Severity
applyPortOptions with loadbalance=first|roundrobin|invalid — existing TestApplyPortOptions (utils_test.go:205) covers all other options, PR didn't extend it	High — docker label parsing untested
Case-insensitivity (loadbalance=ROUNDROBIN) — code does strings.ToLower, no test asserts it	Medium
List-provider YAML parsing of loadBalance key — no test in list/	Medium
TCP/UDP round-robin no-op assertion — PR claims no-op but doesn't test it	Medium
Health checker behavior with multiple targets — dead backend still receives traffic	High (ties to the critical bug above)
Management token NOT set when round-robin routes to a non-management target — the security fix has no regression test	High — security-critical logic must be guarded
Redirect handler unchanged under round-robin	Low

I verified the PR's tests pass on its own base (go test -race clean, go vet clean) in an isolated worktree.

6. Scope vs. issue #436

Issue defines three phases; maintainer asked for tight v1. PR delivers Phase 1 (HTTP/HTTPS round-robin) and defers Phase 2/3. Scope is appropriate — except the PR doesn't acknowledge that health-checker interaction is a Phase 1 correctness issue, not a Phase 2 feature. At minimum, the description should warn that round-robin routes to backends even when they're known-dead.

---

TL;DR — blocking items

1. Rebase onto current main — port.go was split into port_http.go / port_tcp.go / port_udp.go; the diff will not apply cleanly.
2. Address health-checker interaction — either skip unhealthy targets in SelectTarget, or add a loud warning in code + docs.
3. Add the security regression test for the isManagementTarget(target, ...) change — token-leak prevention must be guarded.
4. Add applyPortOptions tests for the new loadbalance= branch.
5. Update docs (docker.md, docker-reference.md, lists.md) — and clarify that the docker label currently has no effect.

Non-blocking: list-provider loadBalance validation/warning, stale-comment cleanup, changelog entry.

[mvanhorn]

Thanks for the thorough review. Rebased onto current main and worked through the gaps, pushed as a follow-up commit on top of the rebase.

Rebase (blocking #1): done. proxyRewrite moved from port.go to port_http.go in the split, so I resolved the conflict by taking main's port.go and re-applying the two round-robin edits (SelectTarget(pconfig.LoadBalance) and the isManagementTarget(target, ...) fix) in port_http.go. go build ./..., go vet ./internal/..., and go test ./... are all clean, including -race.

Health-checker interaction (blocking #2): went with the documented-warning route rather than health-aware selection. Making SelectTarget skip unhealthy backends needs per-target health state that the model layer does not currently carry (the checker probes only GetFirstTarget()), so that is genuinely Phase 1.5 and would push past the tight-v1 scope. Instead: a doc comment on SelectTarget spelling out that round-robin is not health-aware in this phase, plus a warning in the list-provider docs and a note in the roadmap. If you would rather this PR carry the health-aware version, I am happy to take that on, but it felt like a separate change.

Security regression test (blocking #3): added TestProxyRewriteRoundRobinManagementTokenIsolation. Two targets under roundrobin with the self-proxy at index 0 and a non-management backend at index 1; it asserts the token header is set only on the request routed to the management target and never on the backend request. Confirmed it fails if the guard is reverted to GetFirstTarget().

applyPortOptions tests (blocking #4): extended TestApplyPortOptions with loadbalance=first|roundrobin, uppercase (case-insensitivity), and invalid-defaults-to-first.

Docs (blocking #5): added the loadbalance row to docker.md and docker-reference.md, each with a note that the docker provider discovers one target per port today so the option is forward-compatible only and has no effect yet, with round-robin working via the list provider. Fixed the stale "only the first will be used" comment in lists.md and added a loadBalance example, and updated the roadmap to reflect Phase 1 shipped.

Non-blocking: added list-provider validation that warns and falls back to first on unknown strategies, matching the docker path. Documented that nextIndex is not reset on target replacement. Left count()'s RLock as-is since you flagged it negligible. There is no CHANGELOG file in the repo, so no changelog entry.

On your redirect question: redirect ports and round-robin proxy ports are mutually exclusive per port (gated on IsRedirect in proxyports.go, newPortRedirect vs newPortProxy), so a redirect cannot combine with round-robin. The redirect handler's GetFirstTarget() is correct.