From 7c179bc942e2b9f5864c41e210474d228ea1b5fc Mon Sep 17 00:00:00 2001 From: jonathannewman Date: Mon, 10 Aug 2026 09:48:09 -0700 Subject: [PATCH] (PE-45697) Fall back to the ssl check when PuppetDB's cleartext status port is unavailable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PE-45384 (puppet-enterprise-modules) and PE-44906 (puppetdb-private) disabled PuppetDB's cleartext HTTP status listener (port 8080) by default on fresh installs, as a security fix (SECVULN-1792) for an unauthenticated local endpoint that could accept `replace_catalog` commands. `sleep_until_puppetdb_started` unconditionally assumed that port was reachable and checked it before the ssl status port, retrying 120 times before giving up -- on an affected host it now exhausts every retry and fails outright, even though the method already has a second, independent ssl-based liveness check. Fix: check ssl liveness first (it's always enabled, and it's what the method's return value already reflects), then treat the nonssl check as a quick, best-effort secondary confirmation with a much smaller retry budget (120 -> 5) rather than raising, since by the time it runs ssl has already proven the service is up. The rescue only swallows the exact "exhausted retries" RuntimeError `retry_on` raises for this specific command -- compared by exact message match rather than a loose regex -- so an unrelated RuntimeError, or a same-shaped error for a different command, still propagates instead of being misclassified. Logs via `logger.warn` (not `logger.notify`, which can be silently dropped below `:notify` verbosity) so the fallback is always visible to an operator. This collapses the worst-case delay from ~180s (120 nonssl retries plus ssl's own retry budget) down to roughly ssl's retry budget alone, and it applies to every pe_ver without needing a version gate against an unreleased PE line. Confirmed live via a puppet-enterprise-modules frankenbuild (PR #2450, build `enterprise_puppet-enterprise-modules_init-multijob_frankenbuilder-main-main` #194, 2026-08-08): the monolithic-upgrade smoke job's "Upgrade Puppet Enterprise" step failed with `curl: (7) Failed to connect to localhost:8080` after exhausting all 120 retries, tracing back to this method (`beaker-pe`'s `upgrade_pe` -> `generic_install` -> `sleep_until_puppetdb_started`). This affects every consumer of `beaker-pe`/`beaker-puppet` performing a PE install or upgrade in acceptance testing, not just `pe_acceptance_tests` (tracked upstream as PE-45695, which fixed the two `pe_acceptance_tests` call sites with the same root cause). Testing: - Specs cover: default happy path, custom ports, ssl-then-nonssl ordering, the disabled-nonssl-port fallback, the ssl-check-fails-first case, an unrelated RuntimeError propagating, a differently-commanded templated RuntimeError propagating, and both pe_ver endpoint branches. - Full suite: 356 examples, 0 failures. rubocop: no offenses. - Not yet re-verified live against a real frankenbuild with this revised version -- this PR isn't merged/released yet, and `pe_acceptance_tests`' `beaker-puppet` pin needs bumping once it is. Tracked as an open acceptance criterion on PE-45697. PE-45697: https://perforce.atlassian.net/browse/PE-45697 Not merging this PR yet -- opening for CI/review only per explicit instruction. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 5 --- lib/beaker-puppet/helpers/puppet_helpers.rb | 39 +++++++++++-- .../helpers/puppet_helpers_spec.rb | 57 +++++++++++++++++-- 2 files changed, 85 insertions(+), 11 deletions(-) diff --git a/lib/beaker-puppet/helpers/puppet_helpers.rb b/lib/beaker-puppet/helpers/puppet_helpers.rb index 31ba0ab..5a8613a 100644 --- a/lib/beaker-puppet/helpers/puppet_helpers.rb +++ b/lib/beaker-puppet/helpers/puppet_helpers.rb @@ -662,11 +662,40 @@ def sleep_until_puppetdb_started(host, nonssl_port = nil, ssl_port = nil) endpoint = 'status/v1/services/puppetdb-status' expected_regex = '\"state\" \{0,\}: \{0,\}\"running\"' end - retry_on(host, - "curl -m 1 http://localhost:#{nonssl_port}/#{endpoint} | grep '#{expected_regex}'", - { max_retries: 120 }) - curl_with_retries('start puppetdb (ssl)', - host, "https://#{host.node_name}:#{ssl_port}", [35, 60]) + # The ssl status port is always enabled and is the check this method + # ultimately reports the result of, so use it as the primary + # liveness gate. Checking the nonssl port FIRST (as this method used + # to) meant every host with it disabled -- the default for fresh + # installs as of PE-45384/PE-44906 (SECVULN-1792) -- burned through + # 120 failed retries (~2 minutes, at retry_on's default 1s + # retry_interval) before ever reaching the check that actually + # matters (PE-45697). + result = curl_with_retries('start puppetdb (ssl)', + host, "https://#{host.node_name}:#{ssl_port}", [35, 60]) + + # Now that ssl liveness is already confirmed, the nonssl check is + # just a quick, best-effort secondary confirmation: a small retry + # budget is enough, and its absence -- by design, on a host with + # the cleartext listener disabled -- shouldn't fail this method. + nonssl_status_command = "curl -m 1 http://localhost:#{nonssl_port}/#{endpoint} | grep '#{expected_regex}'" + begin + retry_on(host, nonssl_status_command, { max_retries: 5 }) + rescue RuntimeError => e + # Only treat this as the known "exhausted all retries" failure + # retry_on itself raises for THIS command (a plain RuntimeError + # with this exact templated message, see beaker's + # Beaker::DSL::Helpers::HostHelpers#retry_on) -- not any other + # RuntimeError, or a same-shaped error for a different command, + # that might surface from deeper in the retry loop (e.g. an + # SSH/connection error from the underlying on() call). Re-raise + # anything else unrecognized rather than silently reclassifying + # it below (PE-45697). + raise unless e.message == "Command `#{nonssl_status_command}` failed." + + logger.warn("sleep_until_puppetdb_started: nonssl status check on port #{nonssl_port} did not succeed (#{e.message}), but ssl status check already confirmed puppetdb is running") + end + + result end # Waits until a successful curl check has happened against puppetserver diff --git a/spec/beaker-puppet/helpers/puppet_helpers_spec.rb b/spec/beaker-puppet/helpers/puppet_helpers_spec.rb index 2868742..1ec6ba6 100644 --- a/spec/beaker-puppet/helpers/puppet_helpers_spec.rb +++ b/spec/beaker-puppet/helpers/puppet_helpers_spec.rb @@ -1034,35 +1034,80 @@ def stub_post_setup allow(subject).to receive(:version_is_less).and_return(true) end - it 'uses the default ports if none given' do + it 'uses the default ports if none given, checking ssl before nonssl' do host = hosts[0] - expect(subject).to receive(:retry_on).with(anything, /8080/, anything).once.ordered expect(subject).to receive(:curl_with_retries).with(anything, anything, /8081/, anything).once.ordered + expect(subject).to receive(:retry_on).with(anything, /8080/, anything).once.ordered subject.sleep_until_puppetdb_started(host) end it 'allows setting the nonssl_port' do host = hosts[0] - expect(subject).to receive(:retry_on).with(anything, /8084/, anything).once.ordered expect(subject).to receive(:curl_with_retries).with(anything, anything, /8081/, anything).once.ordered + expect(subject).to receive(:retry_on).with(anything, /8084/, anything).once.ordered subject.sleep_until_puppetdb_started(host, 8084) end it 'allows setting the ssl_port' do host = hosts[0] - expect(subject).to receive(:retry_on).with(anything, /8080/, anything).once.ordered expect(subject).to receive(:curl_with_retries).with(anything, anything, /8085/, anything).once.ordered + expect(subject).to receive(:retry_on).with(anything, /8080/, anything).once.ordered subject.sleep_until_puppetdb_started(host, nil, 8085) end + context 'when the cleartext status port never responds (e.g. disabled by default)' do + it 'swallows the failure once ssl liveness is already confirmed' do + host = hosts[0] + expect(subject).to receive(:curl_with_retries).with(anything, anything, /8081/, anything).once.ordered + expect(subject).to receive(:retry_on).with(anything, anything, { max_retries: 5 }).once.ordered do |_host, command, _opts| + raise "Command `#{command}` failed." + end + + expect { subject.sleep_until_puppetdb_started(host) }.not_to raise_error + end + end + + context 'when the ssl check itself fails' do + it 'raises without ever attempting the nonssl check' do + host = hosts[0] + expect(subject).to receive(:curl_with_retries).with(anything, anything, /8081/, anything).once + .and_raise('Command `curl -m 1 https://.../` failed.') + expect(subject).not_to receive(:retry_on) + + expect { subject.sleep_until_puppetdb_started(host) }.to raise_error(/https/) + end + end + + context 'when retry_on raises a RuntimeError unrelated to exhausted retries' do + it 'propagates the error rather than treating it as a disabled port' do + host = hosts[0] + expect(subject).to receive(:curl_with_retries).with(anything, anything, /8081/, anything).once.ordered + expect(subject).to receive(:retry_on).with(anything, /8080/, anything).once.ordered + .and_raise('Host unreachable: no route to host') + + expect { subject.sleep_until_puppetdb_started(host) }.to raise_error(/Host unreachable/) + end + end + + context 'when retry_on raises a RuntimeError formatted like the templated failure but for a different command' do + it 'propagates the error rather than treating it as a disabled port' do + host = hosts[0] + expect(subject).to receive(:curl_with_retries).with(anything, anything, /8081/, anything).once.ordered + expect(subject).to receive(:retry_on).with(anything, /8080/, anything).once.ordered + .and_raise('Command `some other command` failed.') + + expect { subject.sleep_until_puppetdb_started(host) }.to raise_error(/some other command/) + end + end + context 'when pe_ver is less than 2016.1.0' do it 'uses the version endpoint' do host = hosts[0] host['pe_ver'] = '2015.3.3' - expect(subject).to receive(:retry_on).with(anything, %r{pdb/meta/v1/version}, anything).once.ordered expect(subject).to receive(:curl_with_retries).with(anything, anything, /8081/, anything).once.ordered + expect(subject).to receive(:retry_on).with(anything, %r{pdb/meta/v1/version}, anything).once.ordered expect(subject).to receive(:version_is_less).with(host['pe_ver'], '2016.1.0').and_return(true) subject.sleep_until_puppetdb_started(host) @@ -1073,9 +1118,9 @@ def stub_post_setup it 'uses the status endpoint' do host = hosts[0] host['pe_ver'] = '2016.1.0' + expect(subject).to receive(:curl_with_retries).with(anything, anything, /8081/, anything).once.ordered expect(subject).to receive(:retry_on).with(anything, %r{status/v1/services/puppetdb-status}, anything).once.ordered - expect(subject).to receive(:curl_with_retries).with(anything, anything, /8081/, anything).once.ordered expect(subject).to receive(:version_is_less).with(host['pe_ver'], '2016.1.0').and_return(false) subject.sleep_until_puppetdb_started(host)