Merge pull request #856 from supercaracal/fix-sentinel-auth

Fix Sentinel authentication

Author: byroot

README.md

@@ -95,6 +95,15 @@ but a few so that if one is down the client will try the next one. The client
 is able to remember the last Sentinel that was able to reply correctly and will
 use it for the next requests.
 
+If you want to [authenticate](https://redis.io/topics/sentinel#configuring-sentinel-instances-with-authentication) Sentinel itself, you must specify the `password` option per instance.
+
+```ruby
+SENTINELS = [{ host: '127.0.0.1', port: 26380, password: 'mysecret' },
+             { host: '127.0.0.1', port: 26381, password: 'mysecret' }]
+
+redis = Redis.new(host: 'mymaster', sentinels: SENTINELS, role: :master)
+```
+
 ## Cluster support
 
 `redis-rb` supports [clustering](https://redis.io/topics/cluster-spec).

lib/redis/client.rb

@@ -577,6 +577,7 @@ def sentinel_detect
             client = Client.new(@options.merge({
               :host => sentinel[:host],
               :port => sentinel[:port],
+              password: sentinel[:password],
               :reconnect_attempts => 0,
             }))
 
@@ -595,11 +596,6 @@ def sentinel_detect
           end
 
           raise CannotConnectError, "No sentinels available."
-        rescue Redis::CommandError => err
-          # this feature is only available starting with Redis 5.0.1
-          raise if err.message !~ /ERR unknown command (`|')auth(`|')/
-          @options[:password] = DEFAULTS.fetch(:password)
-          retry
         end
 
         def resolve_master

test/sentinel_test.rb

@@ -129,63 +129,56 @@ def test_sentinel_failover_prioritize_healthy_sentinel
   end
 
   def test_sentinel_with_non_sentinel_options
-    sentinels = [{:host => "127.0.0.1", :port => 26381}]
-
-    commands = {
-      :s1 => [],
-      :m1 => []
-    }
+    commands = { s1: [], m1: [] }
 
     sentinel = lambda do |port|
       {
-        :auth => lambda do |pass|
-          commands[:s1] << ["auth", pass]
+        auth: lambda do |pass|
+          commands[:s1] << ['auth', pass]
           '+OK'
         end,
-        :select => lambda do |db|
-          commands[:s1] << ["select", db]
+        select: lambda do |db|
+          commands[:s1] << ['select', db]
           "-ERR unknown command 'select'"
         end,
-        :sentinel => lambda do |command, *args|
+        sentinel: lambda do |command, *args|
           commands[:s1] << [command, *args]
-          ["127.0.0.1", port.to_s]
+          ['127.0.0.1', port.to_s]
         end
       }
     end
 
     master = {
-      :auth => lambda do |pass|
-        commands[:m1] << ["auth", pass]
-        "+OK"
+      auth: lambda do |pass|
+        commands[:m1] << ['auth', pass]
+        '+OK'
       end,
-      :role => lambda do
-        commands[:m1] << ["role"]
-        ["master"]
+      role: lambda do
+        commands[:m1] << ['role']
+        ['master']
       end
     }
 
     RedisMock.start(master) do |master_port|
       RedisMock.start(sentinel.call(master_port)) do |sen_port|
-        sentinels[0][:port] = sen_port
-        redis = Redis.new(:url => "redis://:foo@master1/15", :sentinels => sentinels, :role => :master)
-
+        s = [{ host: '127.0.0.1', port: sen_port }]
+        redis = Redis.new(url: 'redis://:foo@master1/15', sentinels: s, role: :master)
         assert redis.ping
       end
     end
 
-    assert_equal [%w[auth foo], %w[get-master-addr-by-name master1]], commands[:s1]
+    assert_equal [%w[get-master-addr-by-name master1]], commands[:s1]
     assert_equal [%w[auth foo], %w[role]], commands[:m1]
   end
 
-  def test_sentinel_authentication_in_redis_prior_to_version_five
-    sentinels = [{ host: '127.0.0.1', port: 26381 }]
+  def test_authentication_for_sentinel
     commands = { s1: [], m1: [] }
 
     sentinel = lambda do |port|
       {
         auth: lambda do |pass|
           commands[:s1] << ['auth', pass]
-          '-ERR unknown command `auth`'
+          '+OK'
         end,
         select: lambda do |db|
           commands[:s1] << ['select', db]
@@ -201,7 +194,7 @@ def test_sentinel_authentication_in_redis_prior_to_version_five
     master = {
       auth: lambda do |pass|
         commands[:m1] << ['auth', pass]
-        '+OK'
+        '-ERR Client sent AUTH, but no password is set'
       end,
       role: lambda do
         commands[:m1] << ['role']
@@ -211,18 +204,57 @@ def test_sentinel_authentication_in_redis_prior_to_version_five
 
     RedisMock.start(master) do |master_port|
       RedisMock.start(sentinel.call(master_port)) do |sen_port|
-        sentinels[0][:port] = sen_port
-        redis = Redis.new(url: 'redis://master1',
-                          sentinels: sentinels,
-                          role: :master,
-                          password: 'foo')
+        s = [{ host: '127.0.0.1', port: sen_port, password: 'foo' }]
+        r = Redis.new(host: 'master1', sentinels: s, role: :master)
+        assert r.ping
+      end
+    end
 
-        assert redis.ping
+    assert_equal [%w[auth foo], %w[get-master-addr-by-name master1]], commands[:s1]
+    assert_equal [%w[role]], commands[:m1]
+  end
+
+  def test_authentication_for_sentinel_and_redis
+    commands = { s1: [], m1: [] }
+
+    sentinel = lambda do |port|
+      {
+        auth: lambda do |pass|
+          commands[:s1] << ['auth', pass]
+          '+OK'
+        end,
+        select: lambda do |db|
+          commands[:s1] << ['select', db]
+          '-ERR unknown command `select`'
+        end,
+        sentinel: lambda do |command, *args|
+          commands[:s1] << [command, *args]
+          ['127.0.0.1', port.to_s]
+        end
+      }
+    end
+
+    master = {
+      auth: lambda do |pass|
+        commands[:m1] << ['auth', pass]
+        '+OK'
+      end,
+      role: lambda do
+        commands[:m1] << ['role']
+        ['master']
+      end
+    }
+
+    RedisMock.start(master) do |master_port|
+      RedisMock.start(sentinel.call(master_port)) do |sen_port|
+        s = [{ host: '127.0.0.1', port: sen_port, password: 'foo' }]
+        r = Redis.new(host: 'master1', sentinels: s, role: :master, password: 'bar')
+        assert r.ping
       end
     end
 
     assert_equal [%w[auth foo], %w[get-master-addr-by-name master1]], commands[:s1]
-    assert_equal [%w[auth foo], %w[role]], commands[:m1]
+    assert_equal [%w[auth bar], %w[role]], commands[:m1]
   end
 
   def test_sentinel_role_mismatch