Don't send SELECT + ROLE commands to Sentinels

piotrp · pintsized · commit 03dff6da124e · 2022-01-07T12:08:49.000Z
diff --git a/README.md b/README.md
@@ -197,16 +197,16 @@ the server
 
 `syntax: rc = redis_connector.new(params)`
 
-Creates the Redis Connector object, overring default params with the ones given.
+Creates the Redis Connector object, overriding default params with the ones given.
 In case of failures, returns `nil` and a string describing the error.
 
 
 ### connect
 
 `syntax: redis, err = rc:connect(params)`
 
-Attempts to create a connection, according to the [params](#parameters)
-supplied, falling back to defaults given in `new` or the predefined defaults. If
+Attempts to create a connection, according to the `params` supplied, falling back
+to [defaults](#default-parameters) given in `new` or the predefined defaults. If
 a connection cannot be made, returns `nil` and a string describing the reason.
 
 Note that `params` given here do not change the connector's own configuration,
diff --git a/lib/resty/redis/connector.lua b/lib/resty/redis/connector.lua
@@ -1,17 +1,36 @@
-local ipairs, pcall, error, tostring, type, next, setmetatable, getmetatable =
-    ipairs, pcall, error, tostring, type, next, setmetatable, getmetatable
+local ipairs, pairs, pcall, error, tostring, type, next, setmetatable, getmetatable =
+    ipairs, pairs, pcall, error, tostring, type, next, setmetatable, getmetatable
 
 local ngx_log = ngx.log
 local ngx_ERR = ngx.ERR
 local ngx_re_match = ngx.re.match
+local null = ngx.null
 
 local str_find = string.find
 local str_sub = string.sub
 local tbl_remove = table.remove
 local tbl_sort = table.sort
-local ok, tbl_new = pcall(require, "table.new")
-if not ok then
-    tbl_new = function (narr, nrec) return {} end -- luacheck: ignore 212
+local tbl_clone, tbl_new
+do
+    local ok
+    ok, tbl_new = pcall(require, "table.new")
+    if not ok then
+        tbl_new = function (narr, nrec) return {} end -- luacheck: ignore 212
+    end
+    ok, tbl_clone = pcall(require, "table.clone")
+    if not ok then
+        tbl_clone = function (src)
+            local result = {}
+            for i, v in ipairs(src) do
+                result[i] = v
+            end
+            for k, v in pairs(src) do
+                result[k] = v
+            end
+
+            return result
+        end
+    end
 end
 
 local redis = require("resty.redis")
@@ -253,19 +272,19 @@ end
 
 
 function _M.connect_via_sentinel(self, params)
-    local sentinels = params.sentinels
     local master_name = params.master_name
     local role = params.role
     local db = params.db
     local username = params.username
     local password = params.password
-    local sentinel_username = params.sentinel_username
-    local sentinel_password = params.sentinel_password
-    if sentinel_password then
-        for _, host in ipairs(sentinels) do
-            host.username = sentinel_username
-            host.password = sentinel_password
-        end
+
+    local sentinels = tbl_new(#params.sentinels, 0)
+    for i, sentinel in ipairs(params.sentinels) do
+        local host = tbl_clone(sentinel)
+        host.db = null
+        host.username = host.username or params.sentinel_username
+        host.password = host.password or params.sentinel_password
+        sentinels[i] = host
     end
 
     local sentnl, err, previous_errors = self:try_hosts(sentinels)
@@ -344,7 +363,7 @@ function _M.connect_to_host(self, host)
 
     -- config options in 'host' should override the global defaults
     -- host contains keys that aren't in config
-    -- this can break tbl_copy_merge_defaults, hence the mannual loop here
+    -- this can break tbl_copy_merge_defaults, hence the manual loop here
     local config = tbl_copy(self.config)
     for k, _ in pairs(config) do
         if host[k] then
@@ -402,20 +421,20 @@ function _M.connect_to_host(self, host)
             end
         end
 
-        -- No support for DBs in proxied Redis.
-        if config.connection_is_proxied ~= true and host.db ~= nil then
-            local res, err = r:select(host.db)
+        -- No support for DBs in proxied Redis and Redis Sentinel
+        if config.connection_is_proxied ~= true and host.db ~= nil and host.db ~= null then
+            local res, select_err = r:select(host.db)
 
             -- SELECT will fail if we are connected to sentinel:
             -- detect it and ignore error message it that's the case
-            if err and str_find(err, "ERR unknown command") then
+            if select_err and str_find(select_err, "ERR unknown command") then
                 local role = r:role()
                 if role and role[1] == "sentinel" then
-                    err = nil
+                    select_err = nil
                 end
             end
-            if err then
-                return res, err
+            if select_err then
+                return res, select_err
             end
         end
         return r, nil
diff --git a/t/sentinel.t b/t/sentinel.t
@@ -26,15 +26,42 @@ run_tests();
 
 __DATA__
 
-=== TEST 1: Get the master
+=== TEST 1: Connect using redis:// protocol
+--- http_config eval: $::HttpConfig
+--- config
+location /t {
+    content_by_lua_block {
+        local rc = require("resty.redis.connector").new()
+        local sentinel, err, pong
+
+        sentinel, err = rc:connect{ url = "redis://127.0.0.1:$TEST_NGINX_SENTINEL_PORT1", db = ngx.null }
+        assert(sentinel and not err, "sentinel should connect without errors but got " .. tostring(err))
+        pong, err = sentinel:ping()
+        assert(pong and not err, "sentinel should respond to PING got " .. tostring(err))
+        sentinel:close()
+
+        sentinel, err = rc:connect{ url = "redis://127.0.0.1:$TEST_NGINX_SENTINEL_PORT1", db = 0 }
+        assert(sentinel and not err, "sentinel should connect with db=0 without errors but got " .. tostring(err))
+        pong, err = sentinel:ping()
+        assert(pong and not err, "sentinel should respond to PING got " .. tostring(err))
+        sentinel:close()
+    }
+}
+--- request
+GET /t
+--- no_error_log
+[error]
+
+
+=== TEST 2: Get the master
 --- http_config eval: $::HttpConfig
 --- config
 location /t {
     content_by_lua_block {
         local rc = require("resty.redis.connector").new()
         local rs = require("resty.redis.sentinel")
 
-        local sentinel, err = rc:connect{ url = "redis://127.0.0.1:$TEST_NGINX_SENTINEL_PORT1" }
+        local sentinel, err = rc:connect{ url = "redis://127.0.0.1:$TEST_NGINX_SENTINEL_PORT1", db = ngx.null }
         assert(sentinel and not err, "sentinel should connect without errors but got " .. tostring(err))
 
         local master, err = rs.get_master(sentinel, "mymaster")
@@ -57,7 +84,7 @@ GET /t
 [error]
 
 
-=== TEST 1b: Get the master directly
+=== TEST 2b: Get the master directly
 --- http_config eval: $::HttpConfig
 --- config
 location /t {
@@ -84,15 +111,15 @@ GET /t
 [error]
 
 
-=== TEST 2: Get slaves
+=== TEST 3: Get slaves
 --- http_config eval: $::HttpConfig
 --- config
 location /t {
     content_by_lua_block {
         local rc = require("resty.redis.connector").new()
         local rs = require("resty.redis.sentinel")
 
-        local sentinel, err = rc:connect{ url = "redis://127.0.0.1:$TEST_NGINX_SENTINEL_PORT1" }
+        local sentinel, err = rc:connect{ url = "redis://127.0.0.1:$TEST_NGINX_SENTINEL_PORT1", db = ngx.null }
         assert(sentinel and not err, "sentinel should connect without error")
 
         local slaves, err = rs.get_slaves(sentinel, "mymaster")
@@ -121,14 +148,14 @@ location /t {
 [error]
 
 
-=== TEST 3: Get only healthy slaves
+=== TEST 4: Get only healthy slaves
 --- http_config eval: $::HttpConfig
 --- config
 location /t {
     content_by_lua_block {
         local rc = require("resty.redis.connector").new()
 
-        local sentinel, err = rc:connect({ url = "redis://127.0.0.1:$TEST_NGINX_SENTINEL_PORT1" })
+        local sentinel, err = rc:connect({ url = "redis://127.0.0.1:$TEST_NGINX_SENTINEL_PORT1", db = ngx.null })
         assert(sentinel and not err, "sentinel should connect without error")
 
         local slaves, err = require("resty.redis.sentinel").get_slaves(
@@ -183,7 +210,7 @@ GET /t
 [error]
 
 
-=== TEST 4: connector.connect_via_sentinel
+=== TEST 5: connector.connect_via_sentinel
 --- http_config eval: $::HttpConfig
 --- config
 location /t {
@@ -215,7 +242,7 @@ GET /t
 [error]
 
 
-=== TEST 5: regression for slave sorting (iss12)
+=== TEST 6: regression for slave sorting (iss12)
 --- http_config eval: $::HttpConfig
 --- config
 location /t {
@@ -252,7 +279,7 @@ GET /t
 --- no_error_log
 [error]
 
-=== TEST 6: connect with acl
+=== TEST 7: connect with acl
 --- http_config eval: $::HttpConfig
 --- config
 location /t {
