Merge branch...

Merge branch '30146-let-s-encrypt-integration-doesn-t-scale-and-does-not-give-any-feedback-to-user-on-errors-2' into 'master'

Save errored status for pages_domain if acme order is invalid

See merge request gitlab-org/gitlab!27676

app/services/pages_domains/obtain_lets_encrypt_certificate_service.rb

@@ -36,8 +36,8 @@ module PagesDomains
       when 'valid'
         save_certificate(acme_order.private_key, api_order)
         acme_order.destroy!
-        # when 'invalid'
-        # TODO: implement error handling
+      when 'invalid'
+        save_order_error(acme_order, api_order)
       end
     end
 
@@ -47,5 +47,28 @@ module PagesDomains
       certificate = api_order.certificate
       pages_domain.update!(gitlab_provided_key: private_key, gitlab_provided_certificate: certificate)
     end
+
+    def save_order_error(acme_order, api_order)
+      log_error(api_order)
+
+      return unless Feature.enabled?(:pages_letsencrypt_errors, pages_domain.project)
+
+      pages_domain.assign_attributes(auto_ssl_failed: true)
+      pages_domain.save!(validate: false)
+
+      acme_order.destroy!
+    end
+
+    def log_error(api_order)
+      Gitlab::AppLogger.error(
+        message: "Failed to obtain Let's Encrypt certificate",
+        acme_error: api_order.challenge_error,
+        project_id: pages_domain.project_id,
+        pages_domain: pages_domain.domain
+      )
+    rescue => e
+      # getting authorizations is an additional network request which can raise errors
+      Gitlab::ErrorTracking.track_exception(e)
+    end
   end
 end

lib/gitlab/lets_encrypt/challenge.rb

@@ -7,7 +7,7 @@ module Gitlab
         @acme_challenge = acme_challenge
       end
 
-      delegate :token, :file_content, :status, :request_validation, to: :acme_challenge
+      delegate :token, :file_content, :status, :request_validation, :error, to: :acme_challenge
 
       private
 

lib/gitlab/lets_encrypt/order.rb

@@ -8,7 +8,6 @@ module Gitlab
       end
 
       def new_challenge
-        authorization = @acme_order.authorizations.first
         challenge = authorization.http
         ::Gitlab::LetsEncrypt::Challenge.new(challenge)
       end
@@ -22,11 +21,19 @@ module Gitlab
         acme_order.finalize(csr: csr)
       end
 
+      def challenge_error
+        authorization.challenges.first&.error
+      end
+
       delegate :url, :status, :expires, :certificate, to: :acme_order
 
       private
 
       attr_reader :acme_order
+
+      def authorization
+        @acme_order.authorizations.first
+      end
     end
   end
 end

spec/lib/gitlab/lets_encrypt/order_spec.rb

@@ -38,4 +38,23 @@ describe ::Gitlab::LetsEncrypt::Order do
       order.request_certificate(domain: 'example.com', private_key: private_key)
     end
   end
+
+  describe '#challenge_error' do
+    it 'returns error if challenge has errors' do
+      challenge = acme_challenge_double
+
+      # error just to give an example
+      error = {
+        "type" => "urn:ietf:params:acme:error:dns",
+        "detail" => "No valid IP addresses found for test.example.com",
+        "status" => 400
+      }
+
+      allow(challenge).to receive(:error).and_return(error)
+
+      acme_order = acme_order_double(authorizations: [acme_authorization_double(challenge)])
+
+      expect(described_class.new(acme_order).challenge_error).to eq(error)
+    end
+  end
 end

spec/services/pages_domains/obtain_lets_encrypt_certificate_service_spec.rb

@@ -163,4 +163,22 @@ describe PagesDomains::ObtainLetsEncryptCertificateService do
       expect(PagesDomainAcmeOrder.find_by_id(existing_order.id)).to be_nil
     end
   end
+
+  context 'when order is invalid' do
+    let(:existing_order) do
+      create(:pages_domain_acme_order, pages_domain: pages_domain)
+    end
+
+    let!(:api_order) do
+      stub_lets_encrypt_order(existing_order.url, 'invalid')
+    end
+
+    it 'saves error to domain and deletes acme order' do
+      expect do
+        service.execute
+      end.to change { pages_domain.reload.auto_ssl_failed }.from(false).to(true)
+
+      expect(PagesDomainAcmeOrder.find_by_id(existing_order.id)).to be_nil
+    end
+  end
 end

spec/support/helpers/lets_encrypt_helpers.rb

@@ -11,7 +11,8 @@ module LetsEncryptHelpers
     status: 'pending',
     token: 'tokenvalue',
     file_content: 'hereisfilecontent',
-    request_validation: true
+    request_validation: true,
+    error: nil
   }.freeze
 
   def stub_lets_encrypt_settings
@@ -43,16 +44,17 @@ module LetsEncryptHelpers
     challenge
   end
 
-  def acme_authorization_double
+  def acme_authorization_double(challenge = acme_challenge_double)
     authorization = instance_double('Acme::Client::Resources::Authorization')
-    allow(authorization).to receive(:http).and_return(acme_challenge_double)
+    allow(authorization).to receive(:http).and_return(challenge)
+    allow(authorization).to receive(:challenges).and_return([challenge])
     authorization
   end
 
   def acme_order_double(attributes = {})
     acme_order = instance_double('Acme::Client::Resources::Order')
     allow(acme_order).to receive_messages(ACME_ORDER_METHODS.merge(attributes))
-    allow(acme_order).to receive(:authorizations).and_return([acme_authorization_double])
+    allow(acme_order).to receive(:authorizations).and_return([acme_authorization_double]) unless attributes[:authorizations]
     allow(acme_order).to receive(:finalize)
     acme_order
   end