Skip to content

Add tls_on_demand_url option for kamal-proxy - #1927

Draft
AxelTheGerman wants to merge 1 commit into
basecamp:mainfrom
AxelTheGerman:tls-on-demand-url
Draft

Add tls_on_demand_url option for kamal-proxy#1927
AxelTheGerman wants to merge 1 commit into
basecamp:mainfrom
AxelTheGerman:tls-on-demand-url

Conversation

@AxelTheGerman

Copy link
Copy Markdown
Contributor

This landed in basecamp/kamal-proxy#225 but is not yet exposed to the kamal deploy configuration file.

Also note that while it has been merged to kamal-proxy main branch, there hasn't been an official release since 0.9.2 in December, 2025

Copilot AI balanced review requested due to automatic review settings August 11, 2026 18:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Exposes kamal-proxy’s on-demand TLS endpoint through Kamal deployment configuration.

Changes:

  • Adds tls_on_demand_url configuration and proxy argument.
  • Validates incompatible SSL and host settings.
  • Documents and tests the new option.

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 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
test/configuration/proxy_test.rb Tests configuration, arguments, and validation.
lib/kamal/configuration/validator/proxy.rb Validates on-demand TLS settings.
lib/kamal/configuration/proxy.rb Adds the proxy option and CLI argument.
lib/kamal/configuration/docs/proxy.yml Documents on-demand TLS usage.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

tls: ssl? ? true : nil,
"tls-certificate-path": container_tls_cert,
"tls-private-key-path": container_tls_key,
"tls-on-demand-url": tls_on_demand_url,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes good call, I'm running this with a custom built image from main. Will ask for a release on kamal-proxy

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread lib/kamal/configuration/validator/proxy.rb Outdated
Copilot AI review requested due to automatic review settings August 11, 2026 19:12

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Suppressed comments (3)

lib/kamal/configuration/validator/proxy.rb:57

  • The base validator treats numeric, boolean, and symbol values as valid for string-shaped settings (Validator#stringish?), so a truthy non-String value reaches this call and raises NoMethodError rather than the promised Kamal::ConfigurationError. Normalize the value before applying string/URI validation so malformed scalar input is rejected cleanly.
      return if url.start_with?("/") && !url.start_with?("//")

lib/kamal/configuration/proxy.rb:80

  • This option is always sent to the running proxy, but Kamal still defaults to basecamp/kamal-proxy:v0.9.2 (Proxy::Run::MINIMUM_VERSION), and the PR description confirms that release predates support for this flag. Consequently, using the newly documented setting with the default installation makes kamal-proxy deploy fail on an unknown option. This must be gated on a supporting proxy version, or merged alongside a minimum/default version bump once an official supporting release exists.
      "tls-on-demand-url": tls_on_demand_url,

lib/kamal/configuration/validator/proxy.rb:14

  • A configured empty string bypasses this block because it is not present?, but deploy_options only removes nil, so ssl: true, a host, and tls_on_demand_url: "" emits --tls-on-demand-url="" instead of rejecting the invalid URL. The existing empty-string test does not catch this because hostless SSL fails earlier. Validate whenever the key was supplied.

This issue also appears on line 57 of the same file.

      if config["tls_on_demand_url"].present?

Copilot AI review requested due to automatic review settings August 11, 2026 19:19

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Suppressed comments (2)

lib/kamal/configuration/proxy.rb:80

  • This still sends a flag that the default supported proxy cannot parse: Proxy::Run::MINIMUM_VERSION remains v0.9.2, while the PR description confirms this option was added after 0.9.2 and has no released proxy version yet. Any user enabling the documented setting without a custom main-branch image will fail deployment. Please defer exposure until a supporting release can become the compatibility boundary, or gate the option on a known-supporting configured proxy build.
      "tls-on-demand-url": tls_on_demand_url,

lib/kamal/configuration/validator/proxy.rb:6

  • An explicitly empty tls_on_demand_url reaches this branch first and reports Must set a host to enable automatic SSL, even though adding a host is forbidden for on-demand TLS. Check whether the option is absent rather than whether its value is blank, so the URL validator can emit the actionable error for an empty value.
      if config["host"].blank? && config["hosts"].blank? && config["ssl"] && config["tls_on_demand_url"].blank?

@AxelTheGerman
AxelTheGerman marked this pull request as draft August 11, 2026 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants