summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorStan Hu <stanhu@gmail.com>2017-07-10 15:55:08 +0000
committerJames Edwards-Jones <jedwardsjones@gitlab.com>2017-07-11 15:38:37 +0100
commit474bf6a725a5039ca52c8ab476641ff291eb845a (patch)
treedd8bd2343ba55ebafeed737b6e8553e578b49b17
parente3231f8ef8a41b8cb8fc6edcb3e88f6f13bf0f84 (diff)
downloadgitlab-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.rb3
-rw-r--r--changelogs/unreleased/34728-fix-application-setting-created-when-redis-down.yml4
-rw-r--r--lib/gitlab/current_settings.rb7
-rw-r--r--spec/lib/gitlab/current_settings_spec.rb17
-rw-r--r--spec/models/application_setting_spec.rb12
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'