Pass health check protocol through to kamal-proxy - #1928
Conversation
Kamal::Configuration::Proxy#deploy_options passes health-check-path,
-interval and -timeout. There is no way to reach kamal-proxy's other
health check flags from deploy.yml, so a WebSocket-only accessory cannot
be health checked at all.
proxy:
healthcheck:
protocol: websocket
websocket_subprotocol: mqtt
Accessories get this for free -- Kamal::Configuration::Accessory builds
the same Proxy object.
An unknown protocol is rejected in Validator::Proxy, so a typo fails at
config parse instead of silently becoming an HTTP check. A websocket
subprotocol without the websocket protocol is rejected for the same
reason.
There was a problem hiding this comment.
Pull request overview
Adds configurable WebSocket health checks to application and accessory proxies.
Changes:
- Passes protocol and WebSocket subprotocol flags to kamal-proxy.
- Validates protocol combinations.
- Documents and tests the new configuration.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
lib/kamal/configuration/proxy.rb |
Adds health-check CLI options. |
lib/kamal/configuration/validator/proxy.rb |
Validates protocol settings. |
lib/kamal/configuration/docs/proxy.yml |
Documents WebSocket health checks. |
test/configuration/proxy_test.rb |
Tests passthrough and validation. |
test/configuration/accessory_test.rb |
Tests accessory passthrough. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| "health-check-protocol": proxy_config.dig("healthcheck", "protocol"), | ||
| "health-check-websocket-subprotocol": proxy_config.dig("healthcheck", "websocket_subprotocol"), |
There was a problem hiding this comment.
Yes — that's the dependency called out in the description. The proxy flags are proposed in basecamp/kamal-proxy#231 (discussion, per their CONTRIBUTING); this shouldn't merge until a release contains them and MINIMUM_VERSION is bumped here.
| if protocol.present? && !HEALTHCHECK_PROTOCOLS.include?(protocol) | ||
| error "Invalid healthcheck protocol: #{protocol} (must be one of #{HEALTHCHECK_PROTOCOLS.join(", ")})" | ||
| end |
There was a problem hiding this comment.
Half right, and fixed in 4aaed5f. An empty string did skip validation and reach the proxy as --health-check-protocol='' — deploy_options compacts nil, not "". Now compacted via .presence, with a test.
One correction: it wouldn't have failed at the proxy. kamal-proxy's HealthCheckConfig.Validate accepts an empty protocol explicitly (case "", HealthCheckProtocolHTTP, HealthCheckProtocolWebSocket) and treats it as http. So the flag was pointless rather than broken.
| if healthcheck["websocket_subprotocol"].present? && protocol != "websocket" | ||
| error "Cannot set websocket_subprotocol unless the healthcheck protocol is websocket" | ||
| end |
There was a problem hiding this comment.
Accurate, but I'd argue it's the house behaviour rather than a defect in this rule.
Role proxies are validated per subtree throughout, so pre-existing cross-field rules reject partial role overrides the same way. Verified side by side against this exact config shape:
MINE: REJECTED — servers/web/proxy: Cannot set websocket_subprotocol unless the healthcheck protocol is websocket
SSL: REJECTED — servers/web/proxy: Must set a host to enable automatic SSL
That's ssl: true at the role with host inherited from the root — the same valid-after-merge configuration, rejected identically.
Deferring only this check until after the deep merge would make it behave differently from the rule directly above it in the same validator. Happy to move both if maintainers would prefer post-merge validation generally, but that seems like its own change.
An empty string is not `present?`, so it skipped validation, but deploy_options only compacts nil -- so it reached kamal-proxy as `--health-check-protocol=''`. Harmless (the proxy treats an empty protocol as http) but pointless; `.presence` compacts it away instead.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (2)
lib/kamal/configuration/validator/proxy.rb:29
protocol: falsebypasses this check because ActiveSupport treatsfalseas not present, while the generic schema accepts booleans for string-valued fields.deploy_optionsthen drops it via.presence, so this invalid protocol is silently treated as omitted instead of failing configuration parsing. Explicitly rejectfalsewhile retaining the intentional blank-string behavior.
if protocol.present? && !HEALTHCHECK_PROTOCOLS.include?(protocol)
test/configuration/accessory_test.rb:343
@configis constructed insetupbefore the healthcheck hash is added, so this test mutates an already-validated proxy configuration and bypasses the accessory/proxy construction and validation path it intends to cover. Rebuild the configuration after the mutation so the test would catch accessory-path schema or initialization regressions.
options = @config.accessory(:monitoring).proxy.deploy_options
Problem
Kamal::Configuration::Proxy#deploy_optionspasseshealth-check-path,-intervaland-timeout. There is no way to reach kamal-proxy's other health check options fromdeploy.yml.That leaves non-HTTP accessories with nowhere to go. A WebSocket-only target — an MQTT broker fronted as
wss://, for instance — answers a plain GET by closing the connection, so it can never become healthy.Solution
Pass the protocol and subprotocol through:
Accessories get this for free —
Kamal::Configuration::Accessorybuilds the sameProxyobject.An unknown protocol is rejected in
Validator::Proxy, so a typo fails at config parse rather than silently becoming an HTTP check. A subprotocol without the websocket protocol is rejected for the same reason.Omitted keys are compacted out, so unset config passes no flags and the proxy's defaults apply.
Depends on kamal-proxy
The flags this passes don't exist in kamal-proxy yet. Proposed there in basecamp/kamal-proxy#231, with an implementation ready.
This PR should not merge until a kamal-proxy release contains those flags, at which point
MINIMUM_VERSIONneeds bumping to that release here — otherwise the version gate passes on the current pin and the deploy fails mid-flight on an unknown flag. Opening it now so both halves can be considered together; happy to hold it as a draft or close and reopen when the proxy side lands.Tests
bin/test— 860 runs, with the same pre-existing integration failures asmain(verified against a clean checkout). New coverage for the passthrough, the accessory path, and both validation rules.Written by me in collaboration with Claude. I've reviewed all of it myself — quickly, but all of it.