Only require the server setting when no explicit server is given

[silug]

Short description

Since #536 removed the default server=puppet, create_service raised (or warned) whenever server was not set in the config, even when the caller had already resolved a server through server_list, DNS SRV records, or an explicit puppet://host/... URL. An agent configured with only server_list, or with use_srv_records and srv_domain, could not connect at all.

Move the check into check_server_setting and skip it when the resolver passes an explicit server. The settings-based resolver, which is the only one that falls back on server, keeps the existing errors and deprecation warning.

Fixes #658

Generated by Claude Code

Checklist

I have:

• read the CONTRIBUTING.md document
• read and accepted the Developer Certificate of Origin document and added a Signed-off-by annotation to each of my commits
• read and accepted the AI Policy document and added Generated-by or Assisted-by annotations to each of my commits created with the help of an AI agent
• tested this code
• included documentation (including possible behaviour changes)
• documented the code
• added or modified regression test(s)
• added or modified unit test(s)

[corporate-gadfly]

@silug Unless I missed something, the idea is to relax the guard when server_list is configured. Unless I missed it, I don't see the relaxation of the guard.

[silug]

@silug Unless I missed something, the idea is to relax the guard when server_list is configured. Unless I missed it, I don't see the relaxation of the guard.

@corporate-gadfly https://github.com/OpenVoxProject/openvox/pull/659/changes#diff-53d60872a028f347232d3ed8cd4873c0f9b06f5932ed4f4cd9ed463614be6dbfL36 was looking specifically for server. Dropping that lets the other methods for pointing to a server (server_list, use_srv_records) work.

[Copilot]

🟢 Approval recommended

The change is narrowly scoped, aligns with the stated regression fix, and is covered by targeted unit tests for both the resolved and fallback behaviors.

Pull request overview

This PR adjusts the HTTP service creation path so the server setting is only required when the resolver truly needs to fall back to Puppet[:server], fixing regressions introduced when the implicit default server=puppet was removed (#536). This restores connectivity for agents that resolve a server via server_list, DNS SRV records, or an explicit puppet://host/... URL (Fixes #658).

Changes:

• Move the “server setting required / warn / raise” logic into Puppet::HTTP::Service.check_server_setting.
• Only enforce the server setting requirement in create_service when no explicit server argument is provided.
• Add/adjust unit tests to cover resolution via server_list, SRV records, explicit URLs, and the remaining failure case when falling back to the server setting.

File summaries

File	Description
lib/puppet/http/service.rb	Centralizes and scopes the server-setting enforcement to only cases where no explicit server is provided.
spec/unit/http/session_spec.rb	Adds coverage for resolving without server configured when using server_list, SRV, or explicit puppet URLs; asserts fallback still raises.
spec/unit/http/service_spec.rb	Verifies create_service does not warn/raise about missing server when an explicit server is passed.
spec/unit/http/resolver_spec.rb	Adds/adjusts resolver specs to ensure server-less resolution works when resolver supplies an explicit server.

Review details

• Files reviewed: 4/4 changed files
• Comments generated: 0
• Review effort level: Lite

---

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

[silug]

Note

This comment was generated by Claude Code at @silug's request, to expand on the explanation above.

@corporate-gadfly The guard itself is not relaxed; the same checks, messages, and deprecation warning are still there, now in Puppet::HTTP::Service.check_server_setting. What changed is when it runs: only when a service is about to fall back on the server setting.

Why that is the right trigger

Every service constructor picks its host with server || Puppet[:server] (or Puppet[:ca_server] / Puppet[:report_server] for CA and reports). So the server argument to create_service tells you exactly whether Puppet[:server] will be consulted:

Caller	server argument	Puppet[:server] read?	Guard runs?
Resolver::Settings	nil	yes	yes, unchanged behavior
Resolver::ServerList	host from server_list (after a successful /status/v1/simple check)	no	no
Resolver::SRV	host from the DNS SRV record	no	no
Session#route_to with a puppet://host/... URL	host from the URL	no	no

Before this change the guard ran unconditionally at the top of create_service, so it raised on the last three rows even though the caller had already decided which host to talk to and Puppet[:server] would never have been used. That is the failure in #658: an agent with only server_list (or use_srv_records + srv_domain) never got as far as opening a connection.

Why nothing slips through

The concern behind #536 is an agent silently talking to an implicit puppet host. Walking the resolver chain in Session#route_to:

• Nothing configured. Only Resolver::Settings is built, it passes nil, and the guard raises exactly as before.
• server_list configured, every entry down. Resolver::ServerList calls canceled_handler.call(true), so route_to stops without ever reaching Resolver::Settings and raises No more routes. There was never a fallback to Puppet[:server] here.
• use_srv_records configured, no records or none reachable. Resolver::SRV returns nil, route_to moves on to Resolver::Settings, which passes nil, and the guard raises. So SRV without a fallback server still fails loudly rather than trying puppet.
• Explicit puppet://host/... URL. The host comes from the manifest, not from a default, so there is nothing to guard.

In other words, every path that ends up reading Puppet[:server] still goes through the guard, and every path that skips the guard got its host from somewhere the user configured (server_list, DNS, or the URL).

Coverage

The new when the server setting is not configured context in spec/unit/http/session_spec.rb exercises each row above through a real client.create_session, including the "falls back on the settings resolver and raises" case. I also ran it end to end with puppet ssl bootstrap as non-root: server_list pointing at a closed port now fails with connection refused / No more routes to ca; SRV against a domain with real _x-puppet._tcp records resolves and fetches the CA certificate; an empty config still produces the Neither \server` nor `ca_server` is specified` error.