diff options
author | Stan Hu <stanhu@gmail.com> | 2017-07-10 15:55:08 +0000 |
---|---|---|
committer | James Edwards-Jones <jedwardsjones@gitlab.com> | 2017-07-11 15:38:37 +0100 |
commit | 474bf6a725a5039ca52c8ab476641ff291eb845a (patch) | |
tree | dd8bd2343ba55ebafeed737b6e8553e578b49b17 | |
parent | e3231f8ef8a41b8cb8fc6edcb3e88f6f13bf0f84 (diff) | |
download | gitlab-ce-474bf6a725a5039ca52c8ab476641ff291eb845a.tar.gz |
Merge branch '34728-fix-application-setting-created-when-redis-down' into 'master'
Prevent bad data being added to application settings when Redis is unavailable
Closes #34728
See merge request !12750
-rw-r--r-- | app/models/application_setting.rb | 3 | ||||
-rw-r--r-- | changelogs/unreleased/34728-fix-application-setting-created-when-redis-down.yml | 4 | ||||
-rw-r--r-- | lib/gitlab/current_settings.rb | 7 | ||||
-rw-r--r-- | spec/lib/gitlab/current_settings_spec.rb | 17 | ||||
-rw-r--r-- | spec/models/application_setting_spec.rb | 12 |
5 files changed, 35 insertions, 8 deletions
diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 14516091495..98e3906a932 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 00000000000..4fddabebf36 --- /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 818b3d9c46b..791a3c36476 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 a566f24f6a6..d57ffcae8e1 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 fb485d0b2c6..e600eab6565 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' |