Sentinel: release the previous connection with the instance when setting password #8710

yz1509 · 2021-03-27T12:56:39Z

I always use the following commands to initialize the sentinel configuration:

SENTINEL monitor test master_ip master_port 2
SENTINEL set test auth-pass passwd
SENTINEL set ....

It worked well before 8631e64, because the send INFO command blocks sending PING, the condition of this case (https://github.com/redis/redis/blob/6.2.1/src/sentinel.c#L4178) will be satisfied in 15 seconds, the unauthorized connection will be released and the authenticated connection will be reestablished between sentinel and the instance.

However, in the current implementation, because of the refresh of link->last_pong_time, the above case cannot be satisfied, unauthorized connection cannot be released, sentinel thinks the instance is sdown.

…ing password

yossigo · 2021-04-13T18:09:36Z

Thank you @yz1509, do you think you can add a test for this case?

yz1509 · 2021-04-14T13:12:12Z

@yossigo
Test Setting

instance	version	addr
redis-server	5.0.9	127.0.0.147:6309 (Master)
redis-server	5.0.9	127.0.0.50:6309 (Slave)
redis-server	5.0.9	127.0.0.73:6309 (Slave)
redis-sentinel	3.2.11	127.0.0.73:13211
redis-sentinel	6.2.1	127.0.0.73:16210
redis-sentinel	current	127.0.0.73:23333

Script

def monitor_master(sentinel_host, sentinel_port, master_host, master_port):
    r = redis.Redis(sentinel_host, sentinel_port)
    r.execute_command('SENTINEL', 'monitor', 'test', master_host, str(master_port), '2')
    # Adding sleep makes it easier to reproduce
    # make sure https://github.com/redis/redis/blob/6.2.1/src/sentinel.c#L2413 is executed before `SENTINEL set test auth-pass kumiko`
    time.sleep(1)
    r.execute_command('SENTINEL', 'set', 'test', 'auth-pass', 'kumiko')
    # ...
    # ...

Sentinel log

// 3.2.11
50509:X 14 Apr 20:26:44.205 # +monitor master test 127.0.0.147 6309 quorum 2
50509:X 14 Apr 20:26:45.254 # +set master test 127.0.0.147 6309 auth-pass kumiko
50509:X 14 Apr 20:26:45.302 - Client closed connection
// 15 seconds later
50509:X 14 Apr 20:26:59.324 * +slave slave 127.0.0.73:6309 127.0.0.73 6309 @ test 127.0.0.147 6309
50509:X 14 Apr 20:26:59.324 * +slave slave 127.0.0.50:6309 127.0.0.50 6309 @ test 127.0.0.147 6309


// 6.2.1
49270:X 14 Apr 2021 20:27:20.955 # +monitor master test 127.0.0.147 6309 quorum 2
49270:X 14 Apr 2021 20:27:22.003 # +set master test 127.0.0.147 6309 auth-pass kumiko
49270:X 14 Apr 2021 20:27:22.004 . Rewritten config file (/home/yz1509/redis/6.2.1/sentinel.conf) successfully
49270:X 14 Apr 2021 20:27:22.052 - Client closed connection
49270:X 14 Apr 2021 20:27:24.167 * +sentinel sentinel 9be34ec15fdecfbe0aadee37d7985ac942a8d838 127.0.0.73 13211 @ test 127.0.0.147 6309
49270:X 14 Apr 2021 20:27:24.167 . Rewritten config file (/home/yz1509/redis/6.2.1/sentinel.conf) successfully
49270:X 14 Apr 2021 20:27:26.262 - Accepted 127.0.0.73:47265
// sdown
49270:X 14 Apr 2021 20:27:50.996 # +sdown master test 127.0.0.147 6309


// current
33521:X 15 Apr 2021 16:13:26.710 . Rewritten config file (/home/yz1509/redis/current/sentinel.conf) successfully
33521:X 15 Apr 2021 16:13:26.710 # +monitor master test 127.0.0.147 6309 quorum 2
33521:X 15 Apr 2021 16:13:27.763 # +set master test 127.0.0.147 6309 auth-pass kumiko
33521:X 15 Apr 2021 16:13:27.763 . Rewritten config file (/home/yz1509/redis/current/sentinel.conf) successfully
33521:X 15 Apr 2021 16:13:27.801 * +slave slave 127.0.0.73:6309 127.0.0.73 6309 @ test 127.0.0.147 6309
33521:X 15 Apr 2021 16:13:27.802 . Rewritten config file (/home/yz1509/redis/current/sentinel.conf) successfully
33521:X 15 Apr 2021 16:13:27.802 * +slave slave 127.0.0.50:6309 127.0.0.50 6309 @ test 127.0.0.147 6309
33521:X 15 Apr 2021 16:13:27.803 . Rewritten config file (/home/yz1509/redis/current/sentinel.conf) successfully

hwware

Thanks @yz1509 , I have three comments for this PR:

Instead of only disconnecting with masters, I think you also need to disconnect with all slaves(but not sentinels) since the group of redis instances for a single master and a several replica are sharing with the same credenticals when sentinel trying to do authentication.You might need to do similar logic like this: https://github.com/redis/redis/blob/unstable/src/sentinel.c#L3194
I think setting the auth-pass you also need to disconnect with the master/replicas.
Can you provide some simple unit test for this? please check: https://github.com/redis/redis/tree/unstable/tests/sentinel for some existing examples.
@yossigo do you also have other comments? thanks

yz1509 · 2021-04-30T09:04:12Z

@hwware Thanks for your comments.

Sentinel cannot get the correct master's info response before authorization, master->slaves is empty.
I'm not familiar with Tcl, it may take some time to add unit tests.

yossigo · 2021-05-02T14:11:27Z

@hwware @yz1509
Don't we also need to apply the same to auth-user?
I agree it makes sense to drop slave connections as well, but I'm not sure I follow the problem @yz1509 points to as the information about the slaves remains from the last successful INFO.

I think setting the auth-pass you also need to disconnect with the master/replicas.

What do you mean? Force replication to restart?

hwware · 2021-05-03T06:52:43Z

I think setting the auth-pass you also need to disconnect with the master/replicas.

@yossigo sorry i type it wrong, I mean auth-user :)

Sentinel cannot get the correct master's info response before authorization, master->slaves is empty.

@yz1509 @yossigo This is true when sentinel cannot get authenticated with master at first time, but I think we need to pay attention to some edge cases, for example, when master does not have a password, but replica has, sentinel can get replica info and connect with masters but not able connect with replicas, later when we set auth-pass to be replica password in sentinel at run time, in order to let sentinel connect with replica, we need to drop the previous replica connections.

…-user

yz1509 · 2021-05-12T11:56:14Z

@hwware Thanks for your guidance, I made some changes and added unit tests based on it.

hwware · 2021-05-28T03:55:54Z

tests/sentinel/tests/03-runtime-reconf.tcl

+set ::user "testuser"
+set ::password "secret"
+
+proc setup_sever_auth {} {


a small typo in the proc name

hwware · 2021-05-28T04:16:58Z

src/sentinel.c

+        repl_ri = dictGetVal(de);
+        instanceLinkCloseConnection(repl_ri->link, repl_ri->link->cc);
+        instanceLinkCloseConnection(repl_ri->link, repl_ri->link->pc);
+    }


here is a potential memory leak, consider add dictReleaseIterator(di);

hwware · 2021-05-28T04:23:04Z

pinging @yossigo for a second review.

yossigo · 2021-06-08T11:33:53Z

tests/sentinel/tests/03-runtime-reconf.tcl

+        assert_equal {OK} [S $id SENTINEL SET mymaster auth-pass $::password]
+    }
+
+    after 20000


Is there a particular reason that such a long delay is needed? Can the test be optimized instead to have a much shorter down-after-milliseconds and a smaller delay here, or even use wait_for_condition already in this level?

Sentinel tests are already very slow and we should avoid making them slower if it's not absolutely necessary.

@yossigo When the value of down-after-milliseconds is greater than SENTINEL_MIN_LINK_RECONNECT_PERIOD, the sentinel with a version lower than 5.0 can pass "auth test".

@yz1509 @hwware The problem is sentinel tests are already very slow, and we shouldn't slow them much further. Do you think it makes sense to make this define configurable, at least for testing purposes?

@yossigo I agree with you.

@yossigo Yes, I totally agree with you, and I will create a separated PR for this configuration.

hwware · 2021-08-25T14:10:48Z

Hey @yz1509, The debug command has been implemented which adds configurable parameters in #9291, you can use that to reduce test times as @yossigo asked.

yossigo · 2022-03-13T08:46:24Z

Fixed by #10400

Sentinel: release the previous connection with the instance when sett…

7a7fd9f

…ing password

itamarhaber requested a review from hwware April 21, 2021 10:49

hwware reviewed Apr 27, 2021

View reviewed changes

hwware assigned hwware and unassigned hwware Apr 27, 2021

hwware added the sentinel label Apr 27, 2021

yz1509 added 2 commits May 6, 2021 21:42

Sentinel: disconnect with master/replicas when setting auth-pass/auth…

ec8f6d8

…-user

add unittests

ce15ccc

hwware suggested changes May 28, 2021

View reviewed changes

fix typos & memory leak

718a30f

yossigo reviewed Jun 8, 2021

View reviewed changes

yossigo added the state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten label Jul 5, 2021

This was referenced Jul 5, 2021

[Sentinel] set known sentinel instance port to 0 when original instance come online #8891

Closed

[BUG?] Sentinel cannot connect to any master (using password) after switching it manually via CLI #9183

Closed

make sentinel tests faster

5c329b4

hwware mentioned this pull request Jul 9, 2021

Add configurable variable for Sentinel Test file hwware/redis#2

Open

hwware mentioned this pull request Jul 20, 2021

Add configurable variable for Sentinel Test file #9260

Closed

hwware mentioned this pull request Jul 29, 2021

Add sentinel debug option command #9291

Merged

sola and others added 2 commits August 26, 2021 15:42

Merge branch 'redis:unstable' into unstable

621a5ca

[Sentinel] use configurable parameters to reduce test times

0560d2e

moticless added state:to-be-closed requesting the core team to close the issue and removed state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten labels Mar 13, 2022

yossigo closed this Mar 13, 2022

Sentinel: release the previous connection with the instance when setting password #8710

Sentinel: release the previous connection with the instance when setting password #8710

Uh oh!

Conversation

yz1509 commented Mar 27, 2021

Uh oh!

yossigo commented Apr 13, 2021

Uh oh!

yz1509 commented Apr 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hwware left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yz1509 commented Apr 30, 2021

Uh oh!

yossigo commented May 2, 2021

Uh oh!

hwware commented May 3, 2021

Uh oh!

yz1509 commented May 12, 2021

Uh oh!

hwware May 28, 2021

Choose a reason for hiding this comment

Uh oh!

hwware May 28, 2021

Choose a reason for hiding this comment

Uh oh!

hwware commented May 28, 2021

Uh oh!

yossigo Jun 8, 2021

Choose a reason for hiding this comment

Uh oh!

yz1509 Jun 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yossigo Jul 5, 2021

Choose a reason for hiding this comment

Uh oh!

yz1509 Jul 8, 2021

Choose a reason for hiding this comment

Uh oh!

hwware Jul 9, 2021

Choose a reason for hiding this comment

Uh oh!

hwware commented Aug 25, 2021

Uh oh!

yossigo commented Mar 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yz1509 commented Apr 14, 2021 •

edited

Loading

hwware left a comment •

edited

Loading

yz1509 Jun 9, 2021 •

edited

Loading