Merge branch...

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

Add retry button if we failed to obtain Let's Encrypt certificate

See merge request gitlab-org/gitlab!28128

app/controllers/projects/pages_controller.rb

@@ -10,7 +10,7 @@ class Projects::PagesController < Projects::ApplicationController
 
   # rubocop: disable CodeReuse/ActiveRecord
   def show
-    @domains = @project.pages_domains.order(:domain)
+    @domains = @project.pages_domains.order(:domain).present(current_user: current_user)
   end
   # rubocop: enable CodeReuse/ActiveRecord
 

app/controllers/projects/pages_domains_controller.rb

@@ -26,6 +26,12 @@ class Projects::PagesDomainsController < Projects::ApplicationController
     redirect_to project_pages_domain_path(@project, @domain)
   end
 
+  def retry_auto_ssl
+    PagesDomains::RetryAcmeOrderService.new(@domain.pages_domain).execute
+
+    redirect_to project_pages_domain_path(@project, @domain)
+  end
+
   def edit
     redirect_to project_pages_domain_path(@project, @domain)
   end
@@ -82,6 +88,6 @@ class Projects::PagesDomainsController < Projects::ApplicationController
   end
 
   def domain
-    @domain ||= @project.pages_domains.find_by_domain!(params[:id].to_s)
+    @domain ||= @project.pages_domains.find_by_domain!(params[:id].to_s).present(current_user: current_user)
   end
 end

app/models/pages_domain.rb

 # frozen_string_literal: true
 
 class PagesDomain < ApplicationRecord
+  include Presentable
+
   VERIFICATION_KEY = 'gitlab-pages-verification-code'
   VERIFICATION_THRESHOLD = 3.days.freeze
   SSL_RENEWAL_THRESHOLD = 30.days.freeze
@@ -13,6 +15,8 @@ class PagesDomain < ApplicationRecord
   has_many :acme_orders, class_name: "PagesDomainAcmeOrder"
   has_many :serverless_domain_clusters, class_name: 'Serverless::DomainCluster', inverse_of: :pages_domain
 
+  before_validation :clear_auto_ssl_failure, unless: :auto_ssl_enabled
+
   validates :domain, hostname: { allow_numeric_hostname: true }
   validates :domain, uniqueness: { case_sensitive: false }
   validates :certificate, :key, presence: true, if: :usage_serverless?
@@ -208,6 +212,10 @@ class PagesDomain < ApplicationRecord
     Pages::VirtualDomain.new([project], domain: self)
   end
 
+  def clear_auto_ssl_failure
+    self.auto_ssl_failed = false
+  end
+
   private
 
   def pages_deployed?

app/services/pages_domains/retry_acme_order_service.rb

+# frozen_string_literal: true
+
+module PagesDomains
+  class RetryAcmeOrderService
+    attr_reader :pages_domain
+
+    def initialize(pages_domain)
+      @pages_domain = pages_domain
+    end
+
+    def execute
+      updated = pages_domain.with_lock do
+        next unless pages_domain.auto_ssl_enabled && pages_domain.auto_ssl_failed
+
+        pages_domain.update!(auto_ssl_failed: false)
+      end
+
+      PagesDomainSslRenewalWorker.perform_async(pages_domain.id) if updated
+    end
+  end
+end

app/views/projects/pages/_list.html.haml

 - verification_enabled = Gitlab::CurrentSettings.pages_domain_verification_enabled?
 
-- if can?(current_user, :update_pages, @project) && @domains.load.any?
+- if can?(current_user, :update_pages, @project) && @domains.any?
   .card
     .card-header
       Domains (#{@domains.size})
     %ul.list-group.list-group-flush.pages-domain-list{ class: ("has-verification-status" if verification_enabled) }
       - @domains.each do |domain|
-        - domain = Gitlab::View::Presenter::Factory.new(domain, current_user: current_user).fabricate!
         %li.pages-domain-list-item.list-group-item.d-flex.justify-content-between
           - if verification_enabled
             - tooltip, status = domain.unverified? ? [s_('GitLabPages|Unverified'), 'failed'] : [s_('GitLabPages|Verified'), 'success']
@@ -35,6 +34,6 @@
           %li.list-group-item.bs-callout-warning
             - details_link_start = "<a href='#{project_pages_domain_path(@project, domain)}'>".html_safe
             - details_link_end = '</a>'.html_safe
-            = s_("GitLabPages|Something went wrong while obtaining Let's Encrypt certificate for %{domain}. To retry visit your %{link_start}domain details%{link_end}.").html_safe % { domain: domain.domain,
+            = s_("GitLabPages|Something went wrong while obtaining the Let's Encrypt certificate for %{domain}. To retry visit your %{link_start}domain details%{link_end}.").html_safe % { domain: domain.domain,
                 link_start: details_link_start,
                 link_end: details_link_end }

app/views/projects/pages_domains/_certificate.html.haml

@@ -36,7 +36,7 @@
             = _('Certificate')
           .d-flex.justify-content-between.align-items-center.p-3
             %span
-              = @domain.subject || _('missing')
+              = @domain.pages_domain.subject || _('missing')
             = link_to _('Remove'),
               clean_certificate_project_pages_domain_path(@project, @domain),
               data: { confirm: _('Are you sure?') },

app/views/projects/pages_domains/_lets_encrypt_callout.html.haml

 - if @domain.enabled?
-  - if @domain.auto_ssl_enabled && !@domain.certificate
+  - if @domain.auto_ssl_enabled
+    - if @domain.show_auto_ssl_failed_warning?
+      .form-group.border-section.js-shown-if-auto-ssl{ class: ("d-none" unless auto_ssl_available_and_enabled) }
+        .row
+          .col-sm-10.offset-sm-2
+            .bs-callout.bs-callout-warning.mt-0
+              .row.align-items-center.mx-2
+                = icon('warning', class: 'mr-2')
+                = _("Something went wrong while obtaining the Let's Encrypt certificate.")
+              .row.mx-0.mt-3
+                = link_to s_('GitLabPagesDomains|Retry'), retry_auto_ssl_project_pages_domain_path(@project, @domain), class: "btn btn-sm btn-grouped btn-warning", method: :post
+    - elsif !@domain.certificate_gitlab_provided?
       .form-group.border-section.js-shown-if-auto-ssl{ class: ("d-none" unless auto_ssl_available_and_enabled) }
         .row
           .col-sm-10.offset-sm-2

config/routes/project.rb

@@ -338,6 +338,7 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
         resources :domains, except: :index, controller: 'pages_domains', constraints: { id: %r{[^/]+} } do
           member do
             post :verify
+            post :retry_auto_ssl
             delete :clean_certificate
           end
         end

doc/user/project/pages/custom_domains_ssl_tls_certification/lets_encrypt_integration.md

@@ -60,11 +60,11 @@ associated Pages domain. It also will be renewed automatically by GitLab.
 
 ## Troubleshooting
 
-### Error "Something went wrong while obtaining Let's Encrypt certificate"
+### Error "Something went wrong while obtaining the Let's Encrypt certificate"
 
 > [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/30146) in GitLab 13.0.
 
-If you get an error **Something went wrong while obtaining Let's Encrypt certificate**, you can try obtaining the certificate again by following these steps:
+If you get an error **Something went wrong while obtaining the Let's Encrypt certificate**, you can try obtaining the certificate again by following these steps:
 
 1. Go to your project's **Settings > Pages**.
 1. Click **Edit** on your domain.

locale/gitlab.pot

@@ -9801,6 +9801,9 @@ msgstr ""
 msgid "GitLab.com import"
 msgstr ""
 
+msgid "GitLabPagesDomains|Retry"
+msgstr ""
+
 msgid "GitLabPages|%{domain} is not verified. To learn how to verify ownership, visit your %{link_start}domain details%{link_end}."
 msgstr ""
 
@@ -9867,7 +9870,7 @@ msgstr ""
 msgid "GitLabPages|Save"
 msgstr ""
 
-msgid "GitLabPages|Something went wrong while obtaining Let's Encrypt certificate for %{domain}. To retry visit your %{link_start}domain details%{link_end}."
+msgid "GitLabPages|Something went wrong while obtaining the Let's Encrypt certificate for %{domain}. To retry visit your %{link_start}domain details%{link_end}."
 msgstr ""
 
 msgid "GitLabPages|Support for domains and certificates is disabled.  Ask your system's administrator to enable it."

spec/controllers/projects/pages_domains_controller_spec.rb

@@ -181,6 +181,24 @@ describe Projects::PagesDomainsController do
     end
   end
 
+  describe 'POST retry_auto_ssl' do
+    before do
+      pages_domain.update!(auto_ssl_enabled: true, auto_ssl_failed: true)
+    end
+
+    let(:params) { request_params.merge(id: pages_domain.domain) }
+
+    it 'calls retry service and redirects' do
+      expect_next_instance_of(PagesDomains::RetryAcmeOrderService, pages_domain) do |service|
+        expect(service).to receive(:execute)
+      end
+
+      post :retry_auto_ssl, params: params
+
+      expect(response).to redirect_to project_pages_domain_path(project, pages_domain)
+    end
+  end
+
   describe 'DELETE destroy' do
     it "deletes the pages domain" do
       expect do

spec/features/projects/pages_lets_encrypt_spec.rb

@@ -85,6 +85,22 @@ describe "Pages with Let's Encrypt", :https_pages_enabled do
     end
   end
 
+  context "when we failed to obtain Let's Encrypt certificate", :js do
+    let(:domain) do
+      create(:pages_domain, auto_ssl_enabled: true, auto_ssl_failed: true, project: project)
+    end
+
+    it 'user can retry obtaining certificate' do
+      visit project_pages_domain_path(project, domain)
+
+      expect(page).to have_text("Something went wrong while obtaining the Let's Encrypt certificate.")
+
+      click_on('Retry')
+
+      expect(page).to have_text("GitLab is obtaining a Let's Encrypt SSL certificate for this domain. This process can take some time. Please try again later.")
+    end
+  end
+
   shared_examples 'user sees private keys only for user provided certificate' do
     shared_examples 'user do not see private key' do
       it 'user do not see private key' do

spec/models/pages_domain_spec.rb

@@ -536,6 +536,24 @@ describe PagesDomain do
                      'user_provided', 'gitlab_provided')
   end
 
+  describe '#save' do
+    context 'when we failed to obtain ssl certificate' do
+      let(:domain) { create(:pages_domain, auto_ssl_enabled: true, auto_ssl_failed: true) }
+
+      it 'clears failure if auto ssl is disabled' do
+        expect do
+          domain.update!(auto_ssl_enabled: false)
+        end.to change { domain.auto_ssl_failed }.from(true).to(false)
+      end
+
+      it 'does not clear failure on unrelated updates' do
+        expect do
+          domain.update!(verified_at: Time.now)
+        end.not_to change { domain.auto_ssl_failed }.from(true)
+      end
+    end
+  end
+
   describe '.for_removal' do
     subject { described_class.for_removal }
 

spec/services/pages_domains/retry_acme_order_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe PagesDomains::RetryAcmeOrderService do
+  let(:domain) { create(:pages_domain, auto_ssl_enabled: true, auto_ssl_failed: true) }
+
+  let(:service) { described_class.new(domain) }
+
+  it 'clears auto_ssl_failed' do
+    expect do
+      service.execute
+    end.to change { domain.auto_ssl_failed }.from(true).to(false)
+  end
+
+  it 'schedules renewal worker' do
+    expect(PagesDomainSslRenewalWorker).to receive(:perform_async).with(domain.id).and_return(nil).once
+
+    service.execute
+  end
+
+  it "doesn't schedule renewal worker if Let's Encrypt integration is not enabled" do
+    domain.update!(auto_ssl_enabled: false)
+
+    expect(PagesDomainSslRenewalWorker).not_to receive(:new)
+
+    service.execute
+  end
+
+  it "doesn't schedule renewal worker if auto ssl has not failed yet" do
+    domain.update!(auto_ssl_failed: false)
+
+    expect(PagesDomainSslRenewalWorker).not_to receive(:new)
+
+    service.execute
+  end
+end

spec/views/projects/pages/show.html.haml_spec.rb

@@ -17,7 +17,7 @@ describe 'projects/pages/show' do
 
     assign(:project, project)
     allow(view).to receive(:current_user).and_return(user)
-    assign(:domains, project.pages_domains)
+    assign(:domains, [domain.present(current_user: user)])
   end
 
   describe 'validation warning' do
@@ -47,7 +47,7 @@ describe 'projects/pages/show' do
 
   describe "warning about failed Let's Encrypt" do
     let(:error_message) do
-      "Something went wrong while obtaining Let's Encrypt certificate for #{domain.domain}. "\
+      "Something went wrong while obtaining the Let's Encrypt certificate for #{domain.domain}. "\
       "To retry visit your domain details."
     end
 

spec/views/projects/pages_domains/show.html.haml_spec.rb

@@ -7,7 +7,7 @@ describe 'projects/pages_domains/show' do
 
   before do
     assign(:project, project)
-    assign(:domain, domain)
+    assign(:domain, domain.present)
     stub_pages_setting(external_https: true)
   end