Fix cluster connecting option bug and resolve #888

Author: supercaracal

lib/redis/cluster.rb

@@ -112,12 +112,11 @@ def fetch_cluster_info!(option)
       node = Node.new(option.per_node_key)
       available_slots = SlotLoader.load(node)
       node_flags = NodeLoader.load_flags(node)
-      available_node_urls = NodeKey.to_node_urls(available_slots.keys, secure: option.secure?)
-      option.update_node(available_node_urls)
+      option.update_node(available_slots.keys.map { |k| NodeKey.optionize(k) })
       [Node.new(option.per_node_key, node_flags, option.use_replica?),
        Slot.new(available_slots, node_flags, option.use_replica?)]
     ensure
-      node.map(&:disconnect)
+      node&.map(&:disconnect)
     end
 
     def fetch_command_details(nodes)

lib/redis/cluster/node_key.rb

@@ -6,17 +6,13 @@ class Cluster
     # It is different from node id.
     # Node id is internal identifying code in Redis Cluster.
     module NodeKey
-      DEFAULT_SCHEME = 'redis'
-      SECURE_SCHEME = 'rediss'
       DELIMITER = ':'
 
       module_function
 
-      def to_node_urls(node_keys, secure:)
-        scheme = secure ? SECURE_SCHEME : DEFAULT_SCHEME
-        node_keys
-          .map { |k| k.split(DELIMITER) }
-          .map { |k| URI::Generic.build(scheme: scheme, host: k[0], port: k[1].to_i).to_s }
+      def optionize(node_key)
+        host, port = split(node_key)
+        { host: host, port: port }
       end
 
       def split(node_key)

lib/redis/cluster/option.rb

@@ -15,35 +15,33 @@ class Option
       def initialize(options)
         options = options.dup
         node_addrs = options.delete(:cluster)
-        @node_uris = build_node_uris(node_addrs)
+        @node_opts = build_node_options(node_addrs)
         @replica = options.delete(:replica) == true
+        add_common_node_option_if_needed(options, @node_opts, :scheme)
+        add_common_node_option_if_needed(options, @node_opts, :password)
         @options = options
       end
 
       def per_node_key
-        @node_uris.map { |uri| [NodeKey.build_from_uri(uri), @options.merge(url: uri.to_s)] }
+        @node_opts.map { |opt| [NodeKey.build_from_host_port(opt[:host], opt[:port]), @options.merge(opt)] }
                   .to_h
       end
 
-      def secure?
-        @node_uris.any? { |uri| uri.scheme == SECURE_SCHEME } || @options[:ssl_params] || false
-      end
-
       def use_replica?
         @replica
       end
 
       def update_node(addrs)
-        @node_uris = build_node_uris(addrs)
+        @node_opts = build_node_options(addrs)
       end
 
       def add_node(host, port)
-        @node_uris << parse_node_hash(host: host, port: port)
+        @node_opts << { host: host, port: port }
       end
 
       private
 
-      def build_node_uris(addrs)
+      def build_node_options(addrs)
         raise InvalidClientOptionError, 'Redis option of `cluster` must be an Array' unless addrs.is_a?(Array)
         addrs.map { |addr| parse_node_addr(addr) }
       end
@@ -53,7 +51,7 @@ def parse_node_addr(addr)
         when String
           parse_node_url(addr)
         when Hash
-          parse_node_hash(addr)
+          parse_node_option(addr)
         else
           raise InvalidClientOptionError, 'Redis option of `cluster` must includes String or Hash'
         end
@@ -62,15 +60,27 @@ def parse_node_addr(addr)
       def parse_node_url(addr)
         uri = URI(addr)
         raise InvalidClientOptionError, "Invalid uri scheme #{addr}" unless VALID_SCHEMES.include?(uri.scheme)
-        uri
+
+        db = uri.path.split('/')[1]&.to_i
+        { scheme: uri.scheme, password: uri.password, host: uri.host, port: uri.port, db: db }.compact
       rescue URI::InvalidURIError => err
         raise InvalidClientOptionError, err.message
       end
 
-      def parse_node_hash(addr)
+      def parse_node_option(addr)
         addr = addr.map { |k, v| [k.to_sym, v] }.to_h
         raise InvalidClientOptionError, 'Redis option of `cluster` must includes `:host` and `:port` keys' if addr.values_at(:host, :port).any?(&:nil?)
-        URI::Generic.build(scheme: DEFAULT_SCHEME, host: addr[:host], port: addr[:port].to_i)
+
+        addr
+      end
+
+      # Redis cluster node returns only host and port information.
+      # So we should complement additional information such as:
+      #   scheme, password and so on.
+      def add_common_node_option_if_needed(options, node_opts, key)
+        return options if options[key].nil? && node_opts.first[key].nil?
+
+        options[key] = options[key] || node_opts.first[key]
       end
     end
   end

test/cluster_client_options_test.rb

@@ -7,20 +7,44 @@ class TestClusterClientOptions < Minitest::Test
   include Helper::Cluster
 
   def test_option_class
-    option = Redis::Cluster::Option.new(cluster: %w[rediss://127.0.0.1:7000], replica: true)
-    assert_equal({ '127.0.0.1:7000' => { url: 'rediss://127.0.0.1:7000' } }, option.per_node_key)
-    assert_equal true, option.secure?
+    option = Redis::Cluster::Option.new(cluster: %w[redis://127.0.0.1:7000], replica: true)
+    assert_equal({ '127.0.0.1:7000' => { scheme: 'redis', host: '127.0.0.1', port: 7000 } }, option.per_node_key)
     assert_equal true, option.use_replica?
 
     option = Redis::Cluster::Option.new(cluster: %w[redis://127.0.0.1:7000], replica: false)
-    assert_equal({ '127.0.0.1:7000' => { url: 'redis://127.0.0.1:7000' } }, option.per_node_key)
-    assert_equal false, option.secure?
+    assert_equal({ '127.0.0.1:7000' => { scheme: 'redis', host: '127.0.0.1', port: 7000 } }, option.per_node_key)
     assert_equal false, option.use_replica?
 
     option = Redis::Cluster::Option.new(cluster: %w[redis://127.0.0.1:7000])
-    assert_equal({ '127.0.0.1:7000' => { url: 'redis://127.0.0.1:7000' } }, option.per_node_key)
-    assert_equal false, option.secure?
+    assert_equal({ '127.0.0.1:7000' => { scheme: 'redis', host: '127.0.0.1', port: 7000 } }, option.per_node_key)
     assert_equal false, option.use_replica?
+
+    option = Redis::Cluster::Option.new(cluster: %w[rediss://johndoe:[email protected]:7000/1/namespace])
+    assert_equal({ '127.0.0.1:7000' => { scheme: 'rediss', password: 'foobar', host: '127.0.0.1', port: 7000, db: 1 } }, option.per_node_key)
+
+    option = Redis::Cluster::Option.new(cluster: %w[rediss://127.0.0.1:7000], scheme: 'redis')
+    assert_equal({ '127.0.0.1:7000' => { scheme: 'rediss', host: '127.0.0.1', port: 7000 } }, option.per_node_key)
+
+    option = Redis::Cluster::Option.new(cluster: %w[redis://:[email protected]:7000], password: 'foobar')
+    assert_equal({ '127.0.0.1:7000' => { scheme: 'redis', password: 'bazzap', host: '127.0.0.1', port: 7000 } }, option.per_node_key)
+
+    option = Redis::Cluster::Option.new(cluster: %w[redis://127.0.0.1:7000/0], db: 1)
+    assert_equal({ '127.0.0.1:7000' => { scheme: 'redis', host: '127.0.0.1', port: 7000, db: 0 } }, option.per_node_key)
+
+    option = Redis::Cluster::Option.new(cluster: [{ host: '127.0.0.1', port: 7000 }])
+    assert_equal({ '127.0.0.1:7000' => { host: '127.0.0.1', port: 7000 } }, option.per_node_key)
+
+    assert_raises(Redis::InvalidClientOptionError) do
+      Redis::Cluster::Option.new(cluster: nil)
+    end
+
+    assert_raises(Redis::InvalidClientOptionError) do
+      Redis::Cluster::Option.new(cluster: %w[invalid_uri])
+    end
+
+    assert_raises(Redis::InvalidClientOptionError) do
+      Redis::Cluster::Option.new(cluster: [{ host: '127.0.0.1' }])
+    end
   end
 
   def test_client_accepts_valid_node_configs
@@ -43,7 +67,9 @@ def test_client_ignores_invalid_options
   end
 
   def test_client_works_even_if_so_many_unavailable_nodes_specified
-    nodes = (6001..7005).map { |port| "redis://127.0.0.1:#{port}" }
+    min = 7000
+    max = min + Process.getrlimit(Process::RLIMIT_NOFILE).first / 3 * 2
+    nodes = (min..max).map { |port| "redis://127.0.0.1:#{port}" }
     redis = build_another_client(cluster: nodes)
 
     assert_equal 'PONG', redis.ping