Enable handling Let's Encrypt errors by default

remove feature flag
Add the changelog for retrying Let's Encrypt errors
Use UNION instead OR's in needs_ssl_renewal scope
Also add two indexes for different components of union
Rename example in spec

app/models/pages_domain.rb

@@ -2,6 +2,7 @@
 
 class PagesDomain < ApplicationRecord
   include Presentable
+  include FromUnion
 
   VERIFICATION_KEY = 'gitlab-pages-verification-code'
   VERIFICATION_THRESHOLD = 3.days.freeze
@@ -58,12 +59,14 @@ class PagesDomain < ApplicationRecord
   end
 
   scope :need_auto_ssl_renewal, -> do
-    expiring = where(certificate_valid_not_after: nil).or(
-      where(arel_table[:certificate_valid_not_after].lt(SSL_RENEWAL_THRESHOLD.from_now)))
+    enabled_and_not_failed = where(auto_ssl_enabled: true, auto_ssl_failed: false)
 
-    user_provided_or_expiring = certificate_user_provided.or(expiring)
+    user_provided = enabled_and_not_failed.certificate_user_provided
+    certificate_not_valid = enabled_and_not_failed.where(certificate_valid_not_after: nil)
+    certificate_expiring = enabled_and_not_failed
+                             .where(arel_table[:certificate_valid_not_after].lt(SSL_RENEWAL_THRESHOLD.from_now))
 
-    where(auto_ssl_enabled: true).merge(user_provided_or_expiring)
+    from_union([user_provided, certificate_not_valid, certificate_expiring])
   end
 
   scope :for_removal, -> { where("remove_at < ?", Time.now) }

app/presenters/pages_domain_presenter.rb

@@ -8,8 +8,6 @@ class PagesDomainPresenter < Gitlab::View::Presenter::Delegated
   end
 
   def show_auto_ssl_failed_warning?
-    return false unless Feature.enabled?(:pages_letsencrypt_errors, pages_domain.project)
-
     # validations prevents auto ssl from working, so there is no need to show that warning until
     return false if needs_verification?
 

app/services/pages_domains/obtain_lets_encrypt_certificate_service.rb

@@ -51,8 +51,6 @@ module PagesDomains
     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)
 

app/workers/pages_domain_ssl_renewal_cron_worker.rb

@@ -10,11 +10,6 @@ class PagesDomainSslRenewalCronWorker # rubocop:disable Scalability/IdempotentWo
     return unless ::Gitlab::LetsEncrypt.enabled?
 
     PagesDomain.need_auto_ssl_renewal.with_logging_info.find_each do |domain|
-      # Ideally that should be handled in PagesDomain.need_auto_ssl_renewal scope
-      # but it's hard to make scope work with feature flags
-      # once we remove feature flag we can modify scope to implement this behaviour
-      next if Feature.enabled?(:pages_letsencrypt_errors, domain.project) && domain.auto_ssl_failed
-
       with_context(project: domain.project) do
         PagesDomainSslRenewalWorker.perform_async(domain.id)
       end

changelogs/unreleased/30146-let-s-encrypt-errors.yml

+---
+title: Allow users to retry obtaining Let's Encrypt certificates for GitLab Pages
+merge_request: 28784
+author:
+type: added

db/migrate/20200406095930_add_needs_ssl_renewal_user_provided_pages_domains_index.rb

+# frozen_string_literal: true
+
+class AddNeedsSslRenewalUserProvidedPagesDomainsIndex < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  INDEX_NAME = 'index_pages_domains_need_auto_ssl_renewal_user_provided'
+  INDEX_SCOPE = "auto_ssl_enabled = true AND auto_ssl_failed = false AND certificate_source = 0"
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index(:pages_domains, :id, where: INDEX_SCOPE, name: INDEX_NAME)
+  end
+
+  def down
+    remove_concurrent_index(:pages_domains, :id, where: INDEX_SCOPE, name: INDEX_NAME)
+  end
+end

db/migrate/20200406100909_add_needs_ssl_renewal_valid_not_after_pages_domains_index.rb

+# frozen_string_literal: true
+
+class AddNeedsSslRenewalValidNotAfterPagesDomainsIndex < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  INDEX_NAME = 'index_pages_domains_need_auto_ssl_renewal_valid_not_after'
+  INDEX_SCOPE = "auto_ssl_enabled = true AND auto_ssl_failed = false"
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index(:pages_domains, :certificate_valid_not_after, where: INDEX_SCOPE, name: INDEX_NAME)
+  end
+
+  def down
+    remove_concurrent_index(:pages_domains, :certificate_valid_not_after, where: INDEX_SCOPE, name: INDEX_NAME)
+  end
+end

db/post_migrate/20200403132349_remove_old_index_pages_domains_need_auto_ssl_renewal.rb

+# frozen_string_literal: true
+
+class RemoveOldIndexPagesDomainsNeedAutoSslRenewal < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  INDEX_NAME = 'index_pages_domains_need_auto_ssl_renewal'
+
+  disable_ddl_transaction!
+
+  def up
+    remove_concurrent_index(:pages_domains, [:certificate_source, :certificate_valid_not_after],
+                            where: "auto_ssl_enabled = true", name: INDEX_NAME)
+  end
+
+  def down
+    add_concurrent_index(:pages_domains, [:certificate_source, :certificate_valid_not_after],
+                         where: "auto_ssl_enabled = true", name: INDEX_NAME)
+  end
+end

db/structure.sql

@@ -9830,7 +9830,9 @@ CREATE INDEX index_pages_domain_acme_orders_on_challenge_token ON public.pages_d
 
 CREATE INDEX index_pages_domain_acme_orders_on_pages_domain_id ON public.pages_domain_acme_orders USING btree (pages_domain_id);
 
-CREATE INDEX index_pages_domains_need_auto_ssl_renewal ON public.pages_domains USING btree (certificate_source, certificate_valid_not_after) WHERE (auto_ssl_enabled = true);
+CREATE INDEX index_pages_domains_need_auto_ssl_renewal_user_provided ON public.pages_domains USING btree (id) WHERE ((auto_ssl_enabled = true) AND (auto_ssl_failed = false) AND (certificate_source = 0));
+
+CREATE INDEX index_pages_domains_need_auto_ssl_renewal_valid_not_after ON public.pages_domains USING btree (certificate_valid_not_after) WHERE ((auto_ssl_enabled = true) AND (auto_ssl_failed = false));
 
 CREATE UNIQUE INDEX index_pages_domains_on_domain_and_wildcard ON public.pages_domains USING btree (domain, wildcard);
 
@@ -13214,9 +13216,12 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200402124802
 20200402135250
 20200402185044
+20200403132349
 20200403184110
 20200403185127
 20200403185422
+20200406095930
+20200406100909
 20200406102111
 20200406102120
 20200406135648

spec/models/pages_domain_spec.rb

@@ -620,7 +620,11 @@ describe PagesDomain do
       create(:pages_domain, :letsencrypt, :with_expired_certificate)
     end
 
-    it 'contains only domains needing verification' do
+    let!(:domain_with_failed_auto_ssl) do
+      create(:pages_domain, auto_ssl_enabled: true, auto_ssl_failed: true)
+    end
+
+    it 'contains only domains needing ssl renewal' do
       is_expected.to(
         contain_exactly(
           domain_with_user_provided_certificate_and_auto_ssl,

spec/presenters/pages_domain_presenter_spec.rb

@@ -45,14 +45,6 @@ describe PagesDomainPresenter do
 
       it { is_expected.to eq(true) }
 
-      context 'when lets_encrypt_error feature flag is disabled' do
-        before do
-          stub_feature_flags(pages_letsencrypt_errors: false)
-        end
-
-        it { is_expected.to eq(false) }
-      end
-
       context "when Let's Encrypt integration is disabled" do
         before do
           allow(::Gitlab::LetsEncrypt).to receive(:enabled?).and_return false