[tcp-balancer] Fix HAProxy 3.3 startup regression - #2321

Code Review

This pull request updates the HAProxy configuration by renaming backends to include a '_backend' suffix and modifies the deployment to use a versioned HAProxy image. The reviewer noted that while the image is now versioned, the current Chart.yaml appVersion (2.9.7) is incompatible with the intended HAProxy 3.3+ requirement, and recommends updating the Chart.yaml to reflect a compatible version.

Pull request overview

Fixes a HAProxy 3.3+ breaking change in the tcp-balancer Helm chart by ensuring frontend/backend names no longer collide and by pinning the HAProxy image tag to the chart’s appVersion (instead of :latest), aligning with issue #1966.

Changes:

• Renamed generated HAProxy backends (e.g., http_backend, https_backend) to avoid name collisions with frontends on HAProxy 3.3+.
• Updated the Deployment to use haproxy:{{ .Chart.AppVersion }} rather than haproxy:latest.
• Bumped chart appVersion to 3.3.6.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

NOT LGTM — the rename itself is correct and verified, but the PR rewrites frontend blocks that contain a second fatal config bug with the same "pod won't start" symptom, and the fix ships without any regression test.

Business context: haproxy:latest started resolving to 3.3+, where identical frontend/backend names are a fatal config error, so tcp-balancer pods fail to start (#1966).

Blockers

B1: Undefined ACL whitelist is a fatal config error in the same frontends this PR rewrites

File: packages/apps/tcp-balancer/templates/configmap.yaml:92-98 (and the talos block below)

Issue: tcp-request content accept if whitelist / tcp-request content reject render unconditionally in the kubernetes/talos frontends, while acl whitelist src ... only renders when .Values.whitelist is non-empty (default: []). Same trap in http/https when whitelistHTTP: true with an empty whitelist.

Evidence: rendered the chart with kubernetesAndTalos.endpoints set and the default empty whitelist, then validated with haproxy:3.3.6 haproxy -c:

[ALERT] config : parsing [...] : 'tcp-request content accept' : error detected in frontend 'kubernetes' while parsing 'if' condition : no such ACL : 'whitelist'
[ALERT] config : Fatal errors found in configuration.

This is fatal on every HAProxy version, not just 3.3 — pre-existing, but it produces the exact failure mode this PR claims to fix, in the exact lines it touches.

Impact: any kubernetesAndTalos deployment without a whitelist keeps crash-looping even after this fix.

Fix: move the acl + accept + reject trio inside one shared {{- with $.Values.whitelist }} guard (empty whitelist → no filtering).

B2: No regression test pinning the fix

File: packages/apps/tcp-balancer/ (no tests/ directory)

Issue: CI runs make unit-tests (hack/helm-unit-tests.sh, auto-discovers per-chart tests/, see packages/apps/mongodb/tests/ for the pattern), but nothing asserts the renamed backends or the pinned image. The next template refactor can silently reintroduce the collision.

Fix: add a minimal helm-unittest suite asserting (a) every backend name in the rendered haproxy.cfg differs from every frontend name, (b) the deployment image equals haproxy:<appVersion> and never latest, (c) once B1 is fixed — accept if whitelist never renders without a matching acl whitelist line.

B3: kubernetesAndTalos is used by the template but declared nowhere

File: packages/apps/tcp-balancer/values.yaml, values.schema.json, README.md

Issue: the PR renames backends inside the kubernetesAndTalos blocks, but that key is absent from values.yaml, the schema, the README and the generated types — the values contract for the area being modified is out of sync with the template.

Fix: add the @param annotations and run make generate in the package.

Non-blocking follow-ups

1. HAProxy 3.3 is a short-term branch (EOL 2027-Q1). Consider pinning an LTS instead — 3.2.x (supported to 2030) or 3.4.x (to 2031); at minimum bump to the current patch (3.3.10 — 3.3.6 is four patch releases behind). The renamed config validates on 3.2/3.3 alike.
2. The identical bug (haproxy:latest + frontend http/backend http) exists in packages/apps/http-cache; the config half is already addressed by open PR #2654 — no action needed here, noting for completeness.
3. PR title isn't in Conventional Commits format (fix(tcp-balancer): ...), so the auto-labeler applied no area/* label; the title also feeds the changelog.
4. Please squash the appVersion follow-up commit into the main commit — this repo merges with merge commits, so both messages land in history as-is.

Re-reviewing on 57ccc6d3. The whitelist ACL problem I raised in June is genuinely fixed, and the rename is complete: all four frontend and backend pairs renamed with every default_backend updated, and backendoptions takes a mode string rather than a backend name so it was never affected. Pinning the image to {{ .Chart.AppVersion }} instead of haproxy:latest is the right move and I would take that on its own.

Still NOT LGTM, because the fix introduced a new state that is worse in one specific way.

With whitelistHTTP: true and whitelist left empty, the http frontend now renders with no access-control rules at all. I rendered both branches to be sure. On main that combination emits tcp-request content accept if whitelist while the acl whitelist line is inside the with block and never emitted, so HAProxy refuses to start with no such ACL: 'whitelist'. On this branch the whole block is gone and the frontend comes up clean, passing everything. Nobody's working configuration changes, since that combination never started, so this is not a regression against a functioning setup. But an explicit security opt-in silently becoming a no-op is worse than a loud crash, because the crash gets noticed within a minute and this does not.

The fix is small: a fail when whitelistHTTP is true and whitelist is empty, matching the pattern already at templates/configmap.yaml:11. This is only needed for the http and https pair. For the kubernetes and talos frontends an empty list correctly means no filtering, because there whitelist is itself the toggle.

Still carried from June, and it is the reason this file has now shipped two independent fatal-config bugs: packages/apps/tcp-balancer has no tests/ directory. CI discovers per-chart helm unittest suites automatically, and a handful of app charts already have them. A suite pinning the rendered config for the default case, the whitelist-set case and the new failure case would have caught both bugs and costs very little.

One correction to my own earlier review: I flagged .Values.kubernetesAndTalos being referenced at configmap.yaml:88 while absent from values.yaml, the schema, the README and the API type. That is entirely pre-existing and this diff neither introduces nor worsens it, so I am not holding the PR on it. It is worth a follow-up while the file is open in front of you.

Two smaller things. haproxy:3.4.2 shipped in July, so the pin is one patch behind the current LTS line, though 3.4 is the right line to be on. And the PR title and one commit message still say 3.3 while the value is 3.4.1, which will read oddly in the changelog.

Non-blocking observation worth acting on separately: nothing will ever bump this pin. .github/renovate.json sets enabledManagers without the helm managers and has no custom matcher for chart image literals, which is the same rot the existing custom matchers were written to prevent. An entry covering chart appVersion image pins would keep this from going stale the way latest did, just in the opposite direction.

Separately from the code: DCO is red. The head commit and the merge commit carry no Signed-off-by, and the two signed commits use a different address than the others. That blocks merge regardless of the review.