diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 14516091495d16ce8d417f350ab0160e9af72e0c..98e3906a93299b95719dbf1668e194e5e9d1f92e 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -184,6 +184,9 @@ class ApplicationSetting < ActiveRecord::Base Rails.cache.fetch(CACHE_KEY) do ApplicationSetting.last end + rescue + # Fall back to an uncached value if there are any problems (e.g. redis down) + ApplicationSetting.last end def self.expire diff --git a/changelogs/unreleased/34728-fix-application-setting-created-when-redis-down.yml b/changelogs/unreleased/34728-fix-application-setting-created-when-redis-down.yml new file mode 100644 index 0000000000000000000000000000000000000000..4fddabebf360178819dbbe8429d33df84bb0013e --- /dev/null +++ b/changelogs/unreleased/34728-fix-application-setting-created-when-redis-down.yml @@ -0,0 +1,4 @@ +--- +title: Prevent bad data being added to application settings when Redis is unavailable +merge_request: 12750 +author: diff --git a/lib/gitlab/current_settings.rb b/lib/gitlab/current_settings.rb index 818b3d9c46b77f6eccde9fdeaba7f7668b9359cc..791a3c36476f6f3d30f5e628493a92224c976891 100644 --- a/lib/gitlab/current_settings.rb +++ b/lib/gitlab/current_settings.rb @@ -33,12 +33,7 @@ module Gitlab def uncached_application_settings return fake_application_settings unless connect_to_db? - # This loads from the database into the cache, so handle Redis errors - begin - db_settings = ::ApplicationSetting.current - rescue ::Redis::BaseError, ::Errno::ENOENT - # In case Redis isn't running or the Redis UNIX socket file is not available - end + db_settings = ::ApplicationSetting.current # If there are pending migrations, it's possible there are columns that # need to be added to the application settings. To prevent Rake tasks diff --git a/spec/lib/gitlab/current_settings_spec.rb b/spec/lib/gitlab/current_settings_spec.rb index a566f24f6a6a31d69769ae016bb2344d7804bf7d..d57ffcae8e1262d1e84707ebe26c336d7c0ecf18 100644 --- a/spec/lib/gitlab/current_settings_spec.rb +++ b/spec/lib/gitlab/current_settings_spec.rb @@ -27,10 +27,23 @@ describe Gitlab::CurrentSettings do end it 'falls back to DB if Redis fails' do + db_settings = ApplicationSetting.create!(ApplicationSetting.defaults) + expect(ApplicationSetting).to receive(:cached).and_raise(::Redis::BaseError) - expect(ApplicationSetting).to receive(:last).and_call_original + expect(Rails.cache).to receive(:fetch).with(ApplicationSetting::CACHE_KEY).and_raise(Redis::BaseError) - expect(current_application_settings).to be_a(ApplicationSetting) + expect(current_application_settings).to eq(db_settings) + end + + it 'creates default ApplicationSettings if none are present' do + expect(ApplicationSetting).to receive(:cached).and_raise(::Redis::BaseError) + expect(Rails.cache).to receive(:fetch).with(ApplicationSetting::CACHE_KEY).and_raise(Redis::BaseError) + + settings = current_application_settings + + expect(settings).to be_a(ApplicationSetting) + expect(settings).to be_persisted + expect(settings).to have_attributes(ApplicationSetting.defaults) end context 'with migrations pending' do diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb index fb485d0b2c672502f8914812590b787e8b83eb9e..e600eab656568619b73770f65c0dbbf1422a15d8 100644 --- a/spec/models/application_setting_spec.rb +++ b/spec/models/application_setting_spec.rb @@ -155,6 +155,18 @@ describe ApplicationSetting, models: true do end end + describe '.current' do + context 'redis unavailable' do + it 'returns an ApplicationSetting' do + allow(Rails.cache).to receive(:fetch).and_call_original + allow(ApplicationSetting).to receive(:last).and_return(:last) + expect(Rails.cache).to receive(:fetch).with(ApplicationSetting::CACHE_KEY).and_raise(ArgumentError) + + expect(ApplicationSetting.current).to eq(:last) + end + end + end + context 'restricted signup domains' do it 'sets single domain' do setting.domain_whitelist_raw = 'example.com'