Commit b228b86b authored by Jacob Vosmaer's avatar Jacob Vosmaer

Make Gitlab::Redis.params safe for mutation

parent 98b3d6ce
...@@ -9,19 +9,22 @@ module Gitlab ...@@ -9,19 +9,22 @@ module Gitlab
SIDEKIQ_NAMESPACE = 'resque:gitlab' SIDEKIQ_NAMESPACE = 'resque:gitlab'
MAILROOM_NAMESPACE = 'mail_room:gitlab' MAILROOM_NAMESPACE = 'mail_room:gitlab'
DEFAULT_REDIS_URL = 'redis://localhost:6379' DEFAULT_REDIS_URL = 'redis://localhost:6379'
CONFIG_FILE = File.expand_path('../../config/resque.yml', __dir__)
# To be thread-safe we must be careful when writing the class instance # To be thread-safe we must be careful when writing the class instance
# variables @url and @pool. Because @pool depends on @url we need two # variables @_raw_config and @pool. Because @pool depends on @_raw_config we need two
# mutexes to prevent deadlock. # mutexes to prevent deadlock.
PARAMS_MUTEX = Mutex.new RAW_CONFIG_MUTEX = Mutex.new
POOL_MUTEX = Mutex.new POOL_MUTEX = Mutex.new
private_constant :PARAMS_MUTEX, :POOL_MUTEX private_constant :RAW_CONFIG_MUTEX, :POOL_MUTEX
class << self class << self
# Do NOT cache in an instance variable. Result may be mutated by caller.
def params def params
@params || PARAMS_MUTEX.synchronize { @params = new.params } new.params
end end
# Do NOT cache in an instance variable. Result may be mutated by caller.
# @deprecated Use .params instead to get sentinel support # @deprecated Use .params instead to get sentinel support
def url def url
new.url new.url
...@@ -36,8 +39,17 @@ module Gitlab ...@@ -36,8 +39,17 @@ module Gitlab
@pool.with { |redis| yield redis } @pool.with { |redis| yield redis }
end end
def reset_params! def _raw_config
@params = nil return @_raw_config if defined?(@_raw_config)
RAW_CONFIG_MUTEX.synchronize do
begin
@_raw_config = File.read(CONFIG_FILE).freeze
rescue Errno::ENOENT
@_raw_config = false
end
end
@_raw_config
end end
end end
...@@ -83,12 +95,7 @@ module Gitlab ...@@ -83,12 +95,7 @@ module Gitlab
end end
def fetch_config def fetch_config
file = config_file self.class._raw_config ? YAML.load(self.class._raw_config)[@rails_env] : false
File.exist?(file) ? YAML.load_file(file)[@rails_env] : false
end
def config_file
File.expand_path('../../../config/resque.yml', __FILE__)
end end
end end
end end
...@@ -3,19 +3,27 @@ require 'spec_helper' ...@@ -3,19 +3,27 @@ require 'spec_helper'
describe Gitlab::Redis do describe Gitlab::Redis do
let(:redis_config) { Rails.root.join('config', 'resque.yml').to_s } let(:redis_config) { Rails.root.join('config', 'resque.yml').to_s }
before(:each) { described_class.reset_params! } before(:each) { clear_raw_config }
after(:each) { described_class.reset_params! } after(:each) { clear_raw_config }
describe '.params' do describe '.params' do
subject { described_class.params } subject { described_class.params }
it 'withstands mutation' do
params1 = described_class.params
params2 = described_class.params
params1[:foo] = :bar
expect(params2).not_to have_key(:foo)
end
context 'when url contains unix socket reference' do context 'when url contains unix socket reference' do
let(:config_old) { Rails.root.join('spec/fixtures/config/redis_old_format_socket.yml').to_s } let(:config_old) { Rails.root.join('spec/fixtures/config/redis_old_format_socket.yml').to_s }
let(:config_new) { Rails.root.join('spec/fixtures/config/redis_new_format_socket.yml').to_s } let(:config_new) { Rails.root.join('spec/fixtures/config/redis_new_format_socket.yml').to_s }
context 'with old format' do context 'with old format' do
it 'returns path key instead' do it 'returns path key instead' do
expect_any_instance_of(described_class).to receive(:config_file) { config_old } stub_const("#{described_class}::CONFIG_FILE", config_old)
is_expected.to include(path: '/path/to/old/redis.sock') is_expected.to include(path: '/path/to/old/redis.sock')
is_expected.not_to have_key(:url) is_expected.not_to have_key(:url)
...@@ -24,7 +32,7 @@ describe Gitlab::Redis do ...@@ -24,7 +32,7 @@ describe Gitlab::Redis do
context 'with new format' do context 'with new format' do
it 'returns path key instead' do it 'returns path key instead' do
expect_any_instance_of(described_class).to receive(:config_file) { config_new } stub_const("#{described_class}::CONFIG_FILE", config_new)
is_expected.to include(path: '/path/to/redis.sock') is_expected.to include(path: '/path/to/redis.sock')
is_expected.not_to have_key(:url) is_expected.not_to have_key(:url)
...@@ -38,7 +46,7 @@ describe Gitlab::Redis do ...@@ -38,7 +46,7 @@ describe Gitlab::Redis do
context 'with old format' do context 'with old format' do
it 'returns hash with host, port, db, and password' do it 'returns hash with host, port, db, and password' do
expect_any_instance_of(described_class).to receive(:config_file) { config_old } stub_const("#{described_class}::CONFIG_FILE", config_old)
is_expected.to include(host: 'localhost', password: 'mypassword', port: 6379, db: 99) is_expected.to include(host: 'localhost', password: 'mypassword', port: 6379, db: 99)
is_expected.not_to have_key(:url) is_expected.not_to have_key(:url)
...@@ -47,7 +55,7 @@ describe Gitlab::Redis do ...@@ -47,7 +55,7 @@ describe Gitlab::Redis do
context 'with new format' do context 'with new format' do
it 'returns hash with host, port, db, and password' do it 'returns hash with host, port, db, and password' do
expect_any_instance_of(described_class).to receive(:config_file) { config_new } stub_const("#{described_class}::CONFIG_FILE", config_new)
is_expected.to include(host: 'localhost', password: 'mynewpassword', port: 6379, db: 99) is_expected.to include(host: 'localhost', password: 'mynewpassword', port: 6379, db: 99)
is_expected.not_to have_key(:url) is_expected.not_to have_key(:url)
...@@ -56,6 +64,30 @@ describe Gitlab::Redis do ...@@ -56,6 +64,30 @@ describe Gitlab::Redis do
end end
end end
describe '.url' do
it 'withstands mutation' do
url1 = described_class.url
url2 = described_class.url
url1 << 'foobar'
expect(url2).not_to end_with('foobar')
end
end
describe '._raw_config' do
subject { described_class._raw_config }
it 'should be frozen' do
expect(subject).to be_frozen
end
it 'returns false when the file does not exist' do
stub_const("#{described_class}::CONFIG_FILE", '/var/empty/doesnotexist')
expect(subject).to eq(false)
end
end
describe '#raw_config_hash' do describe '#raw_config_hash' do
it 'returns default redis url when no config file is present' do it 'returns default redis url when no config file is present' do
expect(subject).to receive(:fetch_config) { false } expect(subject).to receive(:fetch_config) { false }
...@@ -71,9 +103,15 @@ describe Gitlab::Redis do ...@@ -71,9 +103,15 @@ describe Gitlab::Redis do
describe '#fetch_config' do describe '#fetch_config' do
it 'returns false when no config file is present' do it 'returns false when no config file is present' do
allow(File).to receive(:exist?).with(redis_config) { false } allow(described_class).to receive(:_raw_config) { false }
expect(subject.send(:fetch_config)).to be_falsey expect(subject.send(:fetch_config)).to be_falsey
end end
end end
def clear_raw_config
described_class.remove_instance_variable(:@_raw_config)
rescue NameError
# raised if @_raw_config was not set; ignore
end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment