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

Fix sentinel authentication for Redis5

Author: byroot

lib/redis/client.rb

@@ -525,7 +525,6 @@ class Sentinel < Connector
         def initialize(options)
           super(options)
 
-          @options[:password] = DEFAULTS.fetch(:password)
           @options[:db] = DEFAULTS.fetch(:db)
 
           @sentinels = @options.delete(:sentinels).dup
@@ -586,6 +585,11 @@ 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 unless err.message.start_with?('ERR unknown command `auth`')
+          @options[:password] = DEFAULTS.fetch(:password)
+          retry
         end
 
         def resolve_master

test/helper.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 require "test/unit"
 require "mocha/test_unit"
 require "logger"
@@ -183,6 +185,40 @@ def _new_client(options = {})
     end
   end
 
+  module Sentinel
+    include Generic
+
+    MASTER_PORT = PORT.to_s
+    SLAVE_PORT = '6382'
+    SENTINEL_PORT = '6400'
+    SENTINEL_PORTS = %w[6400 6401 6402].freeze
+    MASTER_NAME = 'master1'
+    LOCALHOST = '127.0.0.1'
+
+    def build_sentinel_client(options = {})
+      opts = { host: LOCALHOST, port: SENTINEL_PORT, timeout: TIMEOUT, logger: ::Logger.new(@log) }
+      Redis.new(opts.merge(options))
+    end
+
+    def build_slave_role_client(options = {})
+      _new_client(options.merge(role: :slave))
+    end
+
+    private
+
+    def _format_options(options = {})
+      {
+        url: "redis://#{MASTER_NAME}",
+        sentinels: [{ host: LOCALHOST, port: SENTINEL_PORT }],
+        role: :master, timeout: TIMEOUT, logger: ::Logger.new(@log)
+      }.merge(options)
+    end
+
+    def _new_client(options = {})
+      Redis.new(_format_options(options).merge(driver: ENV['DRIVER']))
+    end
+  end
+
   module Distributed
     include Generic
 

test/sentinel_command_test.rb

@@ -4,17 +4,7 @@
 
 # @see https://redis.io/topics/sentinel#sentinel-commands Sentinel commands
 class SentinelCommandsTest < Test::Unit::TestCase
-  include Helper::Client
-
-  MASTER_PORT = PORT.to_s
-  SLAVE_PORT = '6382'
-  SENTINEL_PORT = '6400'
-  MASTER_NAME = 'master1'
-  LOCALHOST = '127.0.0.1'
-
-  def build_sentinel_client
-    Redis.new(host: LOCALHOST, port: SENTINEL_PORT, timeout: TIMEOUT)
-  end
+  include Helper::Sentinel
 
   def test_sentinel_command_master
     redis = build_sentinel_client
@@ -49,7 +39,7 @@ def test_sentinel_command_sentinels
     assert_equal result[0]['ip'], LOCALHOST
 
     actual_ports = result.map { |r| r['port'] }.sort
-    expected_ports = (SENTINEL_PORT.to_i + 1..SENTINEL_PORT.to_i + 2).map(&:to_s)
+    expected_ports = SENTINEL_PORTS[1..-1]
     assert_equal actual_ports, expected_ports
   end
 

test/sentinel_test.rb

@@ -1,39 +1,23 @@
-require_relative "helper"
+# frozen_string_literal: true
 
-class SentinelTest < Test::Unit::TestCase
+require_relative 'helper'
 
-  include Helper::Client
+class SentinelTest < Test::Unit::TestCase
+  include Helper::Sentinel
 
-  def test_sentinel_connection
-    sentinels = [{:host => "127.0.0.1", :port => 26381},
-                 {:host => "127.0.0.1", :port => 26382}]
+  def test_sentinel_master_role_connection
+    actual = redis.role
 
-    commands = {
-      :s1 => [],
-      :s2 => [],
-    }
-
-    handler = lambda do |id|
-      {
-        :sentinel => lambda do |command, *args|
-          commands[id] << [command, *args]
-          ["127.0.0.1", "6381"]
-        end
-      }
-    end
-
-    RedisMock.start(handler.call(:s1)) do |s1_port|
-      RedisMock.start(handler.call(:s2)) do |s2_port|
-        sentinels[0][:port] = s1_port
-        sentinels[1][:port] = s2_port
-        redis = Redis.new(:url => "redis://master1", :sentinels => sentinels, :role => :master)
+    assert_equal 'master', actual[0]
+    assert_equal SLAVE_PORT, actual[2][0][1]
+  end
 
-        assert redis.ping
-      end
-    end
+  def test_sentinel_slave_role_connection
+    redis = build_slave_role_client
+    actual = redis.role
 
-    assert_equal commands[:s1], [%w[get-master-addr-by-name master1]]
-    assert_equal commands[:s2], []
+    assert_equal 'slave', actual[0]
+    assert_equal MASTER_PORT.to_i, actual[2]
   end
 
   def test_sentinel_failover
@@ -126,7 +110,7 @@ def test_sentinel_with_non_sentinel_options
       {
         :auth => lambda do |pass|
           commands[:s1] << ["auth", pass]
-          "-ERR unknown command 'auth'"
+          '+OK'
         end,
         :select => lambda do |db|
           commands[:s1] << ["select", db]
@@ -159,7 +143,55 @@ def test_sentinel_with_non_sentinel_options
       end
     end
 
-    assert_equal [%w[get-master-addr-by-name master1]], commands[:s1]
+    assert_equal [%w[auth foo], %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 }]
+    commands = { s1: [], m1: [] }
+
+    sentinel = lambda do |port|
+      {
+        auth: lambda do |pass|
+          commands[:s1] << ['auth', pass]
+          '-ERR unknown command `auth`'
+        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|
+        sentinels[0][:port] = sen_port
+        redis = Redis.new(url: 'redis://master1',
+                          sentinels: sentinels,
+                          role: :master,
+                          password: 'foo')
+
+        assert redis.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]
   end