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.

[miharp]

Thanks for this. I tested it as root on a 9.0.0-rc1 agent against a 9.x server, and it fixes #658: with only server_list in puppet.conf (no server), puppet agent -t does a full run, and SRV-only configs resolve too. Both fail on main. spec/unit/http passes (474 examples) and rubocop is clean.

One case still fails as root: server_list together with report_server or ca_server.

Client#build_resolvers removes :report / :ca from the ServerList resolver when those settings are configured, so those two services fall through to the Settings resolver. That calls create_service with no server, and check_server_setting raises for root before it looks at report_server / ca_server, even though Report.new / Ca.new would have used them. The non-root branch already accounts for this; the root branch doesn't.

Steps to reproduce

As root, on a 9.0.0-rc1 agent that already has certs. The PR only changes one file under lib/, and rc1's copy is identical to the PR base, so it can be dropped in:

F=/opt/puppetlabs/puppet/lib/ruby/vendor_ruby/puppet/http/service.rb
cp -p $F /root/service.rb.orig
curl -fsSL https://raw.githubusercontent.com/OpenVoxProject/openvox/4e814df3cf/lib/puppet/http/service.rb -o $F

cp -p /etc/puppetlabs/puppet/puppet.conf /root/puppet.conf.orig
cat > /etc/puppetlabs/puppet/puppet.conf <<'EOF'
[main]
server_list = puppet.example.com
report_server = puppet.example.com
EOF

puppet agent -t

Result: the catalog is retrieved and applied, then the report is dropped:

Notice: Applied catalog in 0.19 seconds
Error: Could not send report: OpenVox does not default to `server=puppet` as of version 9.0. Please update your configuration appropriately by providing a specific server of your choice.

Remove the report_server line and the same run is clean.

ca_server fails the same way, but agent -t only talks to the CA when a refresh is due, so it is easier to see with:

# puppet ssl download_cert      (puppet.conf: server_list + ca_server, no server)
Error: Could not run: Failed to download certificate: OpenVox does not default to `server=puppet` as of version 9.0. ...

With the suggestion below, both commands succeed and spec/unit/http still passes. As a side effect it also drops the "no longer defaults to server=puppet" deprecation warning for non-root users in this case, which seems right since nothing is falling back on server there.

Spec

A test for this would fit in the new 'when the server setting is not configured' context in spec/unit/http/session_spec.rb (it runs as root there). These fail on the current branch and pass with the suggestion:

it 'resolves the ca service using ca_server when server_list is set' do
  Puppet[:server_list] = 'apple.example.com'
  Puppet[:ca_server] = 'ca.example.com'

  expect(session.route_to(:ca).url.to_s).to eq("https://ca.example.com:8140/puppet-ca/v1")
end

it 'resolves the report service using report_server when server_list is set' do
  Puppet[:server_list] = 'apple.example.com'
  Puppet[:report_server] = 'report.example.com'

  expect(session.route_to(:report).url.to_s).to eq("https://report.example.com:8140/puppet/v3")
end

[miharp]

As root this raises for :ca and :report even when ca_server / report_server is configured, which is the setting those services actually fall back on. Details and a repro in the review summary.

Suggested change

	return if Puppet.settings.set_by_config? :server
	return if Puppet.settings.set_by_config? :server
	return if name == :ca && Puppet.settings.set_by_config?(:ca_server)
	return if name == :report && Puppet.settings.set_by_config?(:report_server)

[corporate-gadfly]

@binford2k #536 was mostly your idea (I was merely the person who fixed the tests and merged it). Please chime in.