Merge branch 'redis-config-mutation' into 'master'

Make Gitlab::Redis.params safe for mutation

Would have avoided https://gitlab.com/gitlab-com/infrastructure/issues/464

Defends in depth against programming mistakes.

See merge request !6472

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

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