Merge branch 'rs-issue-34941' into 'master'

Make `Redis::Wrapper#_raw_config` and `#fetch_config` more resilient

Closes #34941

See merge request !12797

2 files changed, 33 insertions, 17 deletions

diff --git a/lib/gitlab/redis/wrapper.rb b/lib/gitlab/redis/wrapper.rb
index 10c16a962ca..c43b37dde74 100644
--- a/lib/gitlab/redis/wrapper.rb
+++ b/lib/gitlab/redis/wrapper.rb
@@ -33,13 +33,16 @@ module Gitlab
         def _raw_config
           return @_raw_config if defined?(@_raw_config)
 
-          begin
-            @_raw_config = ERB.new(File.read(config_file_name)).result.freeze
-          rescue Errno::ENOENT
-            @_raw_config = false
-          end
-
-          @_raw_config
+          @_raw_config =
+            begin
+              if filename = config_file_name
+                ERB.new(File.read(filename)).result.freeze
+              else
+                false
+              end
+            rescue Errno::ENOENT
+              false
+            end
         end
 
         def default_url
@@ -116,7 +119,16 @@ module Gitlab
       end
 
       def fetch_config
-        self.class._raw_config ? YAML.load(self.class._raw_config)[@rails_env] : false
+        return false unless self.class._raw_config
+
+        yaml = YAML.load(self.class._raw_config)
+
+        # If the file has content but it's invalid YAML, `load` returns false
+        if yaml
+          yaml.fetch(@rails_env, false)
+        else
+          false
+        end
       end
     end
   end
diff --git a/spec/support/redis/redis_shared_examples.rb b/spec/support/redis/redis_shared_examples.rb
index 95a5df181c1..f9552e41894 100644
--- a/spec/support/redis/redis_shared_examples.rb
+++ b/spec/support/redis/redis_shared_examples.rb
@@ -65,14 +65,6 @@ RSpec.shared_examples "redis_shared_examples" do
   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
-
     context 'when yml file with env variable' do
       let(:config_file_name) { config_with_environment_variable_inside }
 
@@ -97,6 +89,12 @@ RSpec.shared_examples "redis_shared_examples" do
     it 'returns false when the file does not exist' do
       expect(subject).to eq(false)
     end
+
+    it "returns false when the filename can't be determined" do
+      expect(described_class).to receive(:config_file_name).and_return(nil)
+
+      expect(subject).to eq(false)
+    end
   end
 
   describe '.with' do
@@ -192,7 +190,13 @@ RSpec.shared_examples "redis_shared_examples" do
     it 'returns false when no config file is present' do
       allow(described_class).to receive(:_raw_config) { false }
 
-      expect(subject.send(:fetch_config)).to be_falsey
+      expect(subject.send(:fetch_config)).to eq false
+    end
+
+    it 'returns false when config file is present but has invalid YAML' do
+      allow(described_class).to receive(:_raw_config) { "# development: true" }
+
+      expect(subject.send(:fetch_config)).to eq false
     end
   end