diff --git a/lib/redis/cluster.rb b/lib/redis/cluster.rb index 6206864b1..cb35c14eb 100644 --- a/lib/redis/cluster.rb +++ b/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&.each(&:disconnect) end def fetch_command_details(nodes) diff --git a/lib/redis/cluster/node_key.rb b/lib/redis/cluster/node_key.rb index 8b9829b14..f0e825a60 100644 --- a/lib/redis/cluster/node_key.rb +++ b/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) diff --git a/lib/redis/cluster/option.rb b/lib/redis/cluster/option.rb index e594982d2..7e0f19100 100644 --- a/lib/redis/cluster/option.rb +++ b/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 }.reject { |_, v| v.nil? } 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] ||= node_opts.first[key] end end end diff --git a/makefile b/makefile index 46ee8f17d..b0a439914 100644 --- a/makefile +++ b/makefile @@ -22,23 +22,11 @@ define kill-redis (ls $1 > /dev/null 2>&1 && kill $$(cat $1) && rm -f $1) || true endef -all: - @make --no-print-directory start_all - @make --no-print-directory test - @make --no-print-directory stop_all - -start_all: - @make --no-print-directory start - @make --no-print-directory start_slave - @make --no-print-directory start_sentinel - @make --no-print-directory start_cluster - @make --no-print-directory create_cluster - -stop_all: - @make --no-print-directory stop_sentinel - @make --no-print-directory stop_slave - @make --no-print-directory stop - @make --no-print-directory stop_cluster +all: start_all test stop_all + +start_all: start start_slave start_sentinel start_cluster create_cluster + +stop_all: stop_sentinel stop_slave stop stop_cluster ${TMP}: @mkdir -p $@ diff --git a/test/cluster_client_options_test.rb b/test/cluster_client_options_test.rb index 01e6177f4..24d7df41f 100644 --- a/test/cluster_client_options_test.rb +++ b/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:foobar@127.0.0.1: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://:bazzap@127.0.0.1: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