Merge pull request #9069 from mhashizume/PUP-11869/main/ca-crl-retry

(PUP-11869) Retry failed CA/CRL refreshes sooner

Author: joshcooper

lib/puppet/ssl/state_machine.rb

@@ -59,9 +59,6 @@ def next_state
         now = Time.now
         last_update = @cert_provider.ca_last_update
         if needs_refresh?(now, last_update)
-          # set last updated time first, then make a best effort to refresh
-          @cert_provider.ca_last_update = now
-
           # If we refresh the CA, then we need to force the CRL to be refreshed too,
           # since if there is a new CA in the chain, then we need its CRL to check
           # the full chain for revocation status.
@@ -114,7 +111,12 @@ def refresh_ca(ssl_ctx, last_update)
       Puppet.info(_("Refreshing CA certificate"))
 
       # return the next_ctx containing the updated ca
-      [download_ca(ssl_ctx, last_update), true]
+      next_ctx = [download_ca(ssl_ctx, last_update), true]
+
+      # After a successful refresh, update ca_last_update
+      @cert_provider.ca_last_update = Time.now
+
+      next_ctx
     rescue Puppet::HTTP::ResponseError => e
       if e.response.code == 304
         Puppet.info(_("CA certificate is unmodified, using existing CA certificate"))
@@ -171,8 +173,6 @@ def next_state
           now = Time.now
           last_update = @cert_provider.crl_last_update
           if needs_refresh?(now, last_update)
-            # set last updated time first, then make a best effort to refresh
-            @cert_provider.crl_last_update = now
             next_ctx = refresh_crl(next_ctx, last_update)
           end
         else
@@ -209,7 +209,12 @@ def refresh_crl(ssl_ctx, last_update)
       Puppet.info(_("Refreshing CRL"))
 
       # return the next_ctx containing the updated crl
-      download_crl(ssl_ctx, last_update)
+      next_ctx = download_crl(ssl_ctx, last_update)
+
+      # After a successful refresh, update crl_last_update
+      @cert_provider.crl_last_update = Time.now
+
+      next_ctx
     rescue Puppet::HTTP::ResponseError => e
       if e.response.code == 304
         Puppet.info(_("CRL is unmodified, using existing CRL"))

spec/unit/ssl/state_machine_spec.rb

@@ -487,14 +487,22 @@ def expect_lockfile_to_contain(pid)
         expect(state.next_state.ssl_context.cacerts.map(&:to_pem)).to eq(new_ca_bundle)
       end
 
-      it 'updates the `last_update` time' do
+      it 'updates the `last_update` time on successful CA refresh' do
         stub_request(:get, %r{puppet-ca/v1/certificate/ca}).to_return(status: 200, body: new_ca_bundle.join)
 
         expect_any_instance_of(Puppet::X509::CertProvider).to receive(:ca_last_update=).with(be_within(60).of(Time.now))
 
         state.next_state
       end
 
+      it "does not update the `last_update` time when CA refresh fails" do
+        stub_request(:get, %r{puppet-ca/v1/certificate/ca}).to_raise(Errno::ECONNREFUSED)
+
+        expect_any_instance_of(Puppet::X509::CertProvider).to receive(:ca_last_update=).never
+
+        state.next_state
+      end
+
       it 'forces the NeedCRLs to refresh' do
         stub_request(:get, %r{puppet-ca/v1/certificate/ca}).to_return(status: 200, body: new_ca_bundle.join)