Repository navigation
fix(proxy): validate the loadbalancer key and make true resolvable - #21
Merged
Merged
Conversation
## Summary The dash-only `proxy/loadbalancer` key had no validator entry, so anything stringish slipped through: `loadbalancer: 123` reached SSHKit as a host, and `loadbalancer: ""` silently fell back to auto-activation. `loadbalancer: true` was worse — `effective_loadbalancer` returned the boolean itself, so `on(true)` and `proxy_hosts.include?(true)` ran with a non-host. The validator now accepts `true`, `false`, or a single non-blank host string (rejecting whitespace/comma lists), and `effective_loadbalancer` resolves `true` to the primary role's first host. An accessories-only config with `loadbalancer: true` has no host to resolve to, so it raises at config time. Host membership is deliberately not checked: a dedicated loadbalancer host outside `servers:` is the documented primary use case (docs/proxy.yml, deploy_with_loadbalancer.yml), so only the shape can be validated. ## Test Coverage - `true` resolves to the primary role's first host and enables load balancing - `false` opts out of auto-activation for a multi-host primary role - `true` without any servers raises a ConfigurationError - 123, "", " ", comma lists and whitespace hosts are rejected - an array value still reports the generic type error ## Verification - [x] bundle exec rubocop --parallel passes - [x] unit tests pass (7 pre-existing builder/Docker failures unrelated, same on dash) Refs #3
mhenrixon
added
documentation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
proxy/loadbalancerhad no validator entry, so the generic example-based check (loadbalancer: lb.example.comis a String) let anything "stringish" through:loadbalancer: 123123reached SSHKit as a hostproxy/loadbalancer: should be true, false, or a single hostloadbalancer: ""loadbalancer: "lb1,lb2"lb1,lb2loadbalancer: trueeffective_loadbalancerreturnedtrue→on(true)andproxy_hosts.include?(true)loadbalancer: falseChanges:
lib/kamal/configuration/validator/proxy.rb—validate_loadbalancer!: acceptstrue/falseor a non-blank single-host string (no whitespace, no commas).lib/kamal/configuration/proxy.rb—effective_loadbalancerresolvestrueto the primary role's first host, via a newprimary_role_first_hosthelper the auto-activation branch also uses.lib/kamal/configuration.rb—ensure_valid_loadbalancer:loadbalancer: truewith noservers:(accessories-only) has nothing to resolve to, so it raises at config time instead of silently disabling the loadbalancer.lib/kamal/configuration/docs/proxy.yml— documenttrue/falseand that the host may live outsideservers:.Deviation from the issue
The issue asks for the host to be validated as "a host that belongs to a proxy role". That would break the feature's documented primary use case: a dedicated loadbalancer host outside
servers:—docs/proxy.ymlshowsloadbalancer: lb.example.com,test/fixtures/deploy_with_loadbalancer.ymluses it, andloadbalancer_on_proxy_host?exists precisely to branch on whether the host is a proxy host (it picks theload-balancercontainer name when it isn't). Since a dedicated host is legal and unverifiable fromdeploy.yml, only the shape can be validated — so this PR validates shape and fixes the one value that was genuinely broken (true).Closes #3
Test plan
bundle exec ruby -Itest test/configuration/proxy_test.rb— 49 runs, 0 failuresdash, host-arch dependent)bundle exec rubocop --parallelloadbalancer: <host>andloadbalancer: falseconfigs are unaffected;deploy_with_loadbalancer*.ymlfixtures still pass