haproxy: T8931: Improve WebSocket support for HAProxy#5226
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🧰 Additional context used📓 Path-based instructions (3)**/*.py📄 CodeRabbit inference engine (AGENTS.md)
Files:
src/conf_mode/**/*.py📄 CodeRabbit inference engine (AGENTS.md)
Files:
{**/*.py,data/templates/**/*}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🔍 Remote MCP Context7Additional factual context for PR review (concise)
Tools / sources used:
🔇 Additional comments (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a ChangesHAProxy tunnel timeout and server-close option support
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
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 |
|
👍 |
|
✅ No typos found in changed files. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@interface-definitions/load-balancing_haproxy.xml.in`:
- Around line 228-233: The backend leaf "http-server-close" can be set for TCP
backends producing invalid HAProxy config; fix by adding a mode guard: either
update the schema or (recommended) add validation in verify() inside
src/conf_mode/load-balancing_haproxy.py to reject/raise when
back_config.http_server_close is true while back_config.mode != "http" (use the
same error type/format as existing HTTP-only checks like
health-check/http-response-headers), and/or update
data/templates/load-balancing/haproxy.cfg.j2 to only render "option
http-server-close" when back_config.mode == "http" (ensure
back_config.http_server_close remains the trigger but is combined with a mode
check).
🪄 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: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 09e9c567-777b-43ab-acc4-70e2b50d0262
📒 Files selected for processing (5)
data/templates/load-balancing/haproxy.cfg.j2interface-definitions/include/haproxy/timeout-tunnel.xml.iinterface-definitions/include/haproxy/timeout.xml.iinterface-definitions/load-balancing_haproxy.xml.insmoketest/scripts/cli/test_load-balancing_haproxy.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build_iso
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🧰 Additional context used
📓 Path-based instructions (4)
data/templates/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Jinja2 templates should be stored as discrete files under data/templates/ rather than as inline Python strings
Files:
data/templates/load-balancing/haproxy.cfg.j2
{**/*.py,data/templates/**/*}
📄 CodeRabbit inference engine (AGENTS.md)
Linting must use ruff 0.6.4, darker, pylint W0611, and Jinja2 lint as enforced by vyos/.github reusables
Files:
data/templates/load-balancing/haproxy.cfg.j2smoketest/scripts/cli/test_load-balancing_haproxy.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Python code must use Python 3.11 or higher
Files:
smoketest/scripts/cli/test_load-balancing_haproxy.py
smoketest/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Smoketest scripts must be nose2 compatible and located in smoketest/
Files:
smoketest/scripts/cli/test_load-balancing_haproxy.py
🧠 Learnings (2)
📚 Learning: 2026-05-26T06:03:59.703Z
Learnt from: c-po
Repo: vyos/vyos-1x PR: 5109
File: smoketest/scripts/cli/test_service_https.py:206-207
Timestamp: 2026-05-26T06:03:59.703Z
Learning: In VyOS smoketests that verify processes running inside a VRF using iproute2, remember that `ip vrf pids <vrf>` outputs one entry per line as `<pid> <process_name>` (e.g., `300431 nginx`), not PIDs alone. Therefore, assertions should check for the presence of the expected process name in the command output (e.g., `assertIn(PROCESS_NAME, cmd(f'ip vrf pids {vrf}'))`) rather than trying to match PID-only output.
Applied to files:
smoketest/scripts/cli/test_load-balancing_haproxy.py
📚 Learning: 2026-05-26T06:04:29.163Z
Learnt from: c-po
Repo: vyos/vyos-1x PR: 5109
File: smoketest/scripts/cli/test_service_https.py:118-120
Timestamp: 2026-05-26T06:04:29.163Z
Learning: In VyOS smoketest scripts under `smoketest/scripts/cli/`, it is intentional to call `self.cli_delete(['vrf'])` in both `setUpClass` and `tearDown` to wipe the entire VRF subtree and ensure a clean slate. During code review, do not recommend narrowing the delete to specific VRF identifiers or name subsets (e.g., `['vrf', 'name', 'mgmt']`)—the broad teardown behavior is the established project-wide pattern for these tests.
Applied to files:
smoketest/scripts/cli/test_load-balancing_haproxy.py
🔍 Remote MCP Context7
Perfect! I've found detailed documentation about both features. Let me compile the findings:
Summary of Additional Context for PR Review
Based on HAProxy documentation research, here is the relevant context for reviewing this pull request:
timeout tunnel Directive
The timeout tunnel directive sets the maximum inactivity time on both client and server sides for tunnels. This timeout supersedes client and server timeouts once a connection becomes a tunnel, such as with WebSocket or CONNECT requests.
The timeout tunnel setting specifies the inactivity time for bidirectional connections that remain inactive in both directions and applies to upgraded connections like WebSocket or after the first response if no keepalive/close option is specified. It is recommended to set timeout client-fin alongside timeout tunnel to handle situations where a client might disappear without acknowledging a close, which can occur on lossy networks with firewalls.
The PR sets a default value of 300 seconds (5 minutes), which aligns with typical WebSocket inactivity timeout requirements.
option http-server-close Directive
The option http-server-close option enables HTTP/1.x connection closing on the server side while maintaining client-side keep-alive. This option can reduce latency for clients on slow networks and improve server resource utilization by enabling faster session reuse.
WebSocket Configuration Best Practices
The PR's implementation aligns with documented HAProxy best practices. A real-world example shows WebSocket backends configured with balance source, timeout tunnel 3600s, and server health checks, demonstrating standard WebSocket proxying patterns.
Validation Notes
- The PR's constraint of
1–3600seconds for the tunnel timeout (as defined intimeout-tunnel.xml.i) represents reasonable bounds for WebSocket connections (minimum 1 second, maximum 1 hour). - The configurable nature of
timeout tunnelper backend (in addition to global defaults) provides flexibility for different application requirements, which is important since WebSocket inactivity tolerance varies significantly across applications. - Both features are standard HAProxy directives with wide adoption.
🔇 Additional comments (6)
data/templates/load-balancing/haproxy.cfg.j2 (2)
180-182: Duplicate of schema-level mode validation issue.
45-45: LGTM!Also applies to: 292-294
interface-definitions/include/haproxy/timeout-tunnel.xml.i (1)
2-13: LGTM!interface-definitions/include/haproxy/timeout.xml.i (1)
10-10: LGTM!interface-definitions/load-balancing_haproxy.xml.in (1)
408-411: LGTM!smoketest/scripts/cli/test_load-balancing_haproxy.py (1)
338-360: LGTM!Also applies to: 645-645, 650-650, 661-661, 673-673, 684-684
indrajitr
force-pushed
the
haproxy-websocket
branch
from
79b68b0 to
49e04f8
Compare
|
CI integration ❌ failed! Details
|
There was a problem hiding this comment.
Pull request overview
Adds WebSocket-oriented HAProxy tuning by exposing timeout tunnel (in the defaults section with per-backend override) and option http-server-close (per backend), with validation and a new smoketest.
Changes:
- New CLI nodes:
load-balancing haproxy timeout tunnel,backend <name> timeout tunnel, andbackend <name> http-server-close; rendered intohaproxy.cfg. - Conf-mode verification that
http-server-closeonly applies when the backendmodeishttp. - New smoketest
test_reverse_proxy_backend_websocketplus tunnel-timeout assertions added totest_reverse_proxy_timeout.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| interface-definitions/include/haproxy/timeout-tunnel.xml.i | New include defining the tunnel timeout leaf (1–3600s). |
| interface-definitions/include/haproxy/timeout.xml.i | Pulls the new tunnel timeout into the shared timeout group used by backends. |
| interface-definitions/load-balancing_haproxy.xml.in | Adds http-server-close to backend and the defaults-section tunnel override (default 300). |
| data/templates/load-balancing/haproxy.cfg.j2 | Emits timeout tunnel (defaults + backend) and option http-server-close in HTTP backends. |
| src/conf_mode/load-balancing_haproxy.py | Validates http-server-close requires HTTP mode; reword of an existing error message. |
| smoketest/scripts/cli/test_load-balancing_haproxy.py | New websocket test and tunnel-timeout coverage in the timeout test. |
Change summary
Improve WebSocket support in HAProxy by allowing control for two configuration parameters:
timeout.tunnelin default section with option to override in backend sectionhttp-server-closein backend sectionGenerally, WebSocket support in HAProxy in VyOS decent. Ability to adjust these couple of parameters provide additional control on WebSocket behavior.
For details of the parameters, see:
timeout tunnel <timeout>option http-server-closeAlso, see general guidelines of setting up WebSocket in HAProxy here.
Types of changes
Related Task(s)
https://vyos.dev/T8931
Related PR(s)
How to test / Smoketest result
Set the maximum inactivity time on the client and server side for tunnels (default: 300)
a. default section:
b. backend section override:
Set Option
http-server-closein backend section:Smoketest result:
Checklist: