diff --git a/lib/beaker-puppet/helpers/puppet_helpers.rb b/lib/beaker-puppet/helpers/puppet_helpers.rb index 5a8613a..01bbd63 100644 --- a/lib/beaker-puppet/helpers/puppet_helpers.rb +++ b/lib/beaker-puppet/helpers/puppet_helpers.rb @@ -662,37 +662,49 @@ 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 - # 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}'" + # Confirm readiness -- not just liveness -- over the ssl status port, + # which is always enabled. /status/v1/* (and the pre-4.0 /pdb/meta + # fallback) is allow-unauthenticated and PuppetDB's jetty listener is + # client-auth=want, so `curl -k` gets a real, content-validated body + # with no client cert, and it works even when the cleartext listener + # is disabled by default (PE-45384/PE-44906, SECVULN-1792). + # + # PE-45697 could no longer perform this running-state validation once + # it had to stop relying on the (now disabled-by-default) cleartext + # port: it fell back to a bare curl_with_retries liveness probe, which + # only confirms the tls port answers, not that puppetdb reports a + # running state -- reopening the "port up but not ready" race the + # nonssl content check used to guard. Restore that check over ssl. + ssl_status_command = "curl -m 1 -k https://localhost:#{ssl_port}/#{endpoint} | grep '#{expected_regex}'" begin - retry_on(host, nonssl_status_command, { max_retries: 5 }) + result = retry_on(host, ssl_status_command, { max_retries: 60 }) 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 + # 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") + # 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). + raise unless e.message == "Command `#{ssl_status_command}` failed." + + # The ssl status endpoint didn't serve unauthenticated content + # (older or nonstandard config). Degrade to the previous behaviour + # rather than failing: a bare ssl liveness probe, plus a best-effort + # nonssl content check whose absence -- by design, on a host with the + # cleartext listener disabled -- must not fail this method. + logger.warn("sleep_until_puppetdb_started: ssl status content check did not confirm a running state (#{e.message}); falling back to an ssl liveness probe") + result = curl_with_retries('start puppetdb (ssl)', + host, "https://#{host.node_name}:#{ssl_port}", [35, 60]) + + 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 => nonssl_error + raise unless nonssl_error.message == "Command `#{nonssl_status_command}` failed." + + logger.warn("sleep_until_puppetdb_started: nonssl status check on port #{nonssl_port} did not succeed (#{nonssl_error.message}), but ssl liveness was confirmed") + end end result diff --git a/spec/beaker-puppet/helpers/puppet_helpers_spec.rb b/spec/beaker-puppet/helpers/puppet_helpers_spec.rb index 1ec6ba6..fc94f3c 100644 --- a/spec/beaker-puppet/helpers/puppet_helpers_spec.rb +++ b/spec/beaker-puppet/helpers/puppet_helpers_spec.rb @@ -1034,96 +1034,119 @@ def stub_post_setup allow(subject).to receive(:version_is_less).and_return(true) end - it 'uses the default ports if none given, checking ssl before nonssl' do + it 'content-validates over the ssl status port and, on success, never touches the cleartext 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 - subject.sleep_until_puppetdb_started(host) - end - - it 'allows setting the nonssl_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, /8084/, anything).once.ordered + expect(subject).to receive(:retry_on).with(anything, %r{-k https://localhost:8081/}, { max_retries: 60 }).once + expect(subject).not_to receive(:curl_with_retries) + expect(subject).not_to receive(:retry_on).with(anything, %r{http://localhost:8080/}, anything) - subject.sleep_until_puppetdb_started(host, 8084) + subject.sleep_until_puppetdb_started(host) end it 'allows setting the ssl_port' do host = hosts[0] - 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 + expect(subject).to receive(:retry_on).with(anything, %r{-k https://localhost:8085/}, { max_retries: 60 }).once 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 + context 'when pe_ver is less than 2016.1.0' do + it 'content-validates the version endpoint over ssl' 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 + host['pe_ver'] = '2015.3.3' + expect(subject).to receive(:version_is_less).with(host['pe_ver'], '2016.1.0').and_return(true) + expect(subject).to receive(:retry_on).with(anything, %r{-k https://localhost:8081/pdb/meta/v1/version}, + { max_retries: 60 }).once - expect { subject.sleep_until_puppetdb_started(host) }.not_to raise_error + subject.sleep_until_puppetdb_started(host) end end - context 'when the ssl check itself fails' do - it 'raises without ever attempting the nonssl check' do + context 'when pe_ver is greater than 2015.9.9' do + it 'content-validates the status endpoint over ssl' 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) + host['pe_ver'] = '2016.1.0' + expect(subject).to receive(:version_is_less).with(host['pe_ver'], '2016.1.0').and_return(false) + expect(subject).to receive(:retry_on).with(anything, %r{-k https://localhost:8081/status/v1/services/puppetdb-status}, + { max_retries: 60 }).once - expect { subject.sleep_until_puppetdb_started(host) }.to raise_error(/https/) + subject.sleep_until_puppetdb_started(host) 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 + context 'when the ssl status endpoint does not serve unauthenticated content' do + # Older/nonstandard configs: degrade to the previous behaviour rather + # than failing -- a bare ssl liveness probe plus a best-effort nonssl + # content check. + it 'falls back to an ssl liveness probe and a nonssl content check' 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).to receive(:retry_on).with(anything, %r{-k https://localhost:8081/}, { max_retries: 60 }).once.ordered do |_host, command, _opts| + raise "Command `#{command}` failed." + end + expect(subject).to receive(:curl_with_retries).with(anything, anything, %r{https://[^/]*:8081}, [35, 60]).once.ordered + expect(subject).to receive(:retry_on).with(anything, %r{http://localhost:8080/}, { max_retries: 5 }).once.ordered - expect { subject.sleep_until_puppetdb_started(host) }.to raise_error(/Host unreachable/) + expect { subject.sleep_until_puppetdb_started(host) }.not_to raise_error 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 + it 'allows setting the nonssl_port in the fallback path' 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).to receive(:retry_on).with(anything, %r{-k https://localhost:8081/}, { max_retries: 60 }).once.ordered do |_host, command, _opts| + raise "Command `#{command}` failed." + end + expect(subject).to receive(:curl_with_retries).with(anything, anything, /8081/, [35, 60]).once.ordered + expect(subject).to receive(:retry_on).with(anything, %r{http://localhost:8084/}, { max_retries: 5 }).once.ordered - expect { subject.sleep_until_puppetdb_started(host) }.to raise_error(/some other command/) + subject.sleep_until_puppetdb_started(host, 8084) + end + + context 'and the cleartext status port never responds (e.g. disabled by default)' do + it 'swallows the nonssl failure once ssl liveness is confirmed' do + host = hosts[0] + expect(subject).to receive(:retry_on).with(anything, %r{-k https://localhost:8081/}, { max_retries: 60 }).once.ordered do |_host, command, _opts| + raise "Command `#{command}` failed." + end + expect(subject).to receive(:curl_with_retries).with(anything, anything, /8081/, [35, 60]).once.ordered + expect(subject).to receive(:retry_on).with(anything, %r{http://localhost:8080/}, { 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 + + it 'propagates an unrelated RuntimeError from the nonssl check' do + host = hosts[0] + expect(subject).to receive(:retry_on).with(anything, %r{-k https://localhost:8081/}, { max_retries: 60 }).once.ordered do |_host, command, _opts| + raise "Command `#{command}` failed." + end + expect(subject).to receive(:curl_with_retries).with(anything, anything, /8081/, [35, 60]).once.ordered + expect(subject).to receive(:retry_on).with(anything, %r{http://localhost:8080/}, { max_retries: 5 }).once.ordered + .and_raise('Host unreachable: no route to host') + + expect { subject.sleep_until_puppetdb_started(host) }.to raise_error(/Host unreachable/) + end end end - context 'when pe_ver is less than 2016.1.0' do - it 'uses the version endpoint' do + context 'when the ssl content check raises a RuntimeError unrelated to exhausted retries' do + it 'propagates the error rather than falling back' do host = hosts[0] - host['pe_ver'] = '2015.3.3' - 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(:retry_on).with(anything, %r{-k https://localhost:8081/}, { max_retries: 60 }).once + .and_raise('Host unreachable: no route to host') + expect(subject).not_to receive(:curl_with_retries) - expect(subject).to receive(:version_is_less).with(host['pe_ver'], '2016.1.0').and_return(true) - subject.sleep_until_puppetdb_started(host) + expect { subject.sleep_until_puppetdb_started(host) }.to raise_error(/Host unreachable/) end end - context 'when pe_ver is greater than 2015.9.9' do - it 'uses the status endpoint' do + context 'when the ssl content check raises the templated failure but for a different command' do + it 'propagates the error rather than falling back' 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(:retry_on).with(anything, %r{-k https://localhost:8081/}, { max_retries: 60 }).once + .and_raise('Command `some other command` failed.') + expect(subject).not_to receive(:curl_with_retries) - expect(subject).to receive(:version_is_less).with(host['pe_ver'], '2016.1.0').and_return(false) - subject.sleep_until_puppetdb_started(host) + expect { subject.sleep_until_puppetdb_started(host) }.to raise_error(/some other command/) end end end