From b228b86b3e644c7c277b8004e3fd9291ba493cc2 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Thu, 22 Sep 2016 11:32:49 +0200 Subject: Make Gitlab::Redis.params safe for mutation --- lib/gitlab/redis.rb | 31 ++++++++++++++++---------- spec/lib/gitlab/redis_spec.rb | 52 +++++++++++++++++++++++++++++++++++++------ 2 files changed, 64 insertions(+), 19 deletions(-) diff --git a/lib/gitlab/redis.rb b/lib/gitlab/redis.rb index 9376b54f43b..69c4ef721d5 100644 --- a/lib/gitlab/redis.rb +++ b/lib/gitlab/redis.rb @@ -9,19 +9,22 @@ module Gitlab SIDEKIQ_NAMESPACE = 'resque:gitlab' MAILROOM_NAMESPACE = 'mail_room:gitlab' 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 - # 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. - PARAMS_MUTEX = Mutex.new + RAW_CONFIG_MUTEX = Mutex.new POOL_MUTEX = Mutex.new - private_constant :PARAMS_MUTEX, :POOL_MUTEX + private_constant :RAW_CONFIG_MUTEX, :POOL_MUTEX class << self + # Do NOT cache in an instance variable. Result may be mutated by caller. def params - @params || PARAMS_MUTEX.synchronize { @params = new.params } + new.params end + # Do NOT cache in an instance variable. Result may be mutated by caller. # @deprecated Use .params instead to get sentinel support def url new.url @@ -36,8 +39,17 @@ module Gitlab @pool.with { |redis| yield redis } end - def reset_params! - @params = nil + def _raw_config + 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 @@ -83,12 +95,7 @@ module Gitlab end def fetch_config - file = config_file - File.exist?(file) ? YAML.load_file(file)[@rails_env] : false - end - - def config_file - File.expand_path('../../../config/resque.yml', __FILE__) + self.class._raw_config ? YAML.load(self.class._raw_config)[@rails_env] : false end end end diff --git a/spec/lib/gitlab/redis_spec.rb b/spec/lib/gitlab/redis_spec.rb index e54f5ffb312..cb54c020b31 100644 --- a/spec/lib/gitlab/redis_spec.rb +++ b/spec/lib/gitlab/redis_spec.rb @@ -3,19 +3,27 @@ require 'spec_helper' describe Gitlab::Redis do let(:redis_config) { Rails.root.join('config', 'resque.yml').to_s } - before(:each) { described_class.reset_params! } - after(:each) { described_class.reset_params! } + before(:each) { clear_raw_config } + after(:each) { clear_raw_config } describe '.params' do 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 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 } context 'with old format' 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.not_to have_key(:url) @@ -24,7 +32,7 @@ describe Gitlab::Redis do context 'with new format' 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.not_to have_key(:url) @@ -38,7 +46,7 @@ describe Gitlab::Redis do context 'with old format' 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.not_to have_key(:url) @@ -47,7 +55,7 @@ describe Gitlab::Redis do context 'with new format' 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.not_to have_key(:url) @@ -56,6 +64,30 @@ describe Gitlab::Redis do 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 it 'returns default redis url when no config file is present' do expect(subject).to receive(:fetch_config) { false } @@ -71,9 +103,15 @@ describe Gitlab::Redis do describe '#fetch_config' 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 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 -- cgit v1.2.3