-
Notifications
You must be signed in to change notification settings - Fork 750
Pass health check protocol through to kamal-proxy #1928
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,6 @@ | ||
| class Kamal::Configuration::Validator::Proxy < Kamal::Configuration::Validator | ||
| HEALTHCHECK_PROTOCOLS = [ "http", "websocket" ].freeze | ||
|
|
||
| def validate! | ||
| unless config.nil? | ||
| super | ||
|
|
@@ -21,6 +23,18 @@ def validate! | |
| end | ||
| end | ||
|
|
||
| if healthcheck = config["healthcheck"] | ||
| protocol = healthcheck["protocol"] | ||
|
|
||
| if protocol.present? && !HEALTHCHECK_PROTOCOLS.include?(protocol) | ||
| error "Invalid healthcheck protocol: #{protocol} (must be one of #{HEALTHCHECK_PROTOCOLS.join(", ")})" | ||
| end | ||
|
Comment on lines
+29
to
+31
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Half right, and fixed in 4aaed5f. An empty string did skip validation and reach the proxy as One correction: it wouldn't have failed at the proxy. kamal-proxy's |
||
|
|
||
| if healthcheck["websocket_subprotocol"].present? && protocol != "websocket" | ||
| error "Cannot set websocket_subprotocol unless the healthcheck protocol is websocket" | ||
| end | ||
|
Comment on lines
+33
to
+35
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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: That's 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. |
||
| end | ||
|
|
||
| if run_config = config["run"] | ||
| if run_config["bind_ips"].present? | ||
| ensure_valid_bind_ips(config["bind_ips"]) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_VERSIONis bumped here.