Add ability to reset password in JedisFactory #1606

yapei123 · 2017-09-13T21:12:23Z

Change JedisFactory to public
add a new method to reset password

marcosnils · 2017-09-13T21:13:30Z

dupe of #1605 ?

yapei123 · 2017-09-13T21:15:33Z

@marcosnils I removed that branch since of something messed up. I have closed #1605.

yapei123 · 2017-09-13T21:19:31Z

@marcosnils I would love to add it to JedisCluster also add tests. Can I have some helps from you or other members on how to run the tests? I've tried to run 'make start', 'make test' but failed.

marcosnils · 2017-09-13T21:21:01Z

@yapei123 sure, please share your erros and I'll try to help you to get the tests passed.

marcosnils · 2017-09-15T03:41:20Z

@yapei123 do you have maven installed?. Seems like the makefile is complaining that maven is not installed. Which linux distribution are you using?

marcosnils · 2017-09-15T16:43:36Z

@yapei123 maybe some weird race condition or something. If tests pass on you IDE then it's ok. Is this PR ready to be reviewed?. Can you please squash the commits?

marcosnils · 2017-09-15T16:46:35Z

As you can see this PR has 15 commits. It's simpler to review if you just squash them all together and you just send few of them. Check https://stackoverflow.com/questions/5189560/squash-my-last-x-commits-together-using-git

yapei123 · 2017-09-18T03:49:28Z

@marcosnils just realized the JedisCluster might not be able to add the feature of 'reset password at runtime' since the JedisFactory needs to be created for each node and put into a List and it's hard to get hold of the JedsFactory for each node.

yapei123 · 2017-09-22T14:56:45Z

ready for code reviewing. Thanks!

chrisribble · 2017-09-29T15:49:40Z

It would be really cool to get this merged so that an application can update the Redis password at runtime. @marcosnils is there anything holding up merging this?

HeartSaVioR

Left minor comments but overall LGTM. @marcosnils Please continue review this.

HeartSaVioR · 2017-10-22T09:30:12Z

src/test/java/redis/clients/jedis/tests/JedisPoolTest.java

add fail() below pool.getResource()

HeartSaVioR · 2017-10-22T09:31:58Z

src/test/java/redis/clients/jedis/tests/JedisPoolTest.java

Let's wrap with try-catch and remove expected statement. The test will pass even JedisConnectionException is thrown from obj1.

HeartSaVioR · 2017-10-22T09:32:13Z

src/test/java/redis/clients/jedis/tests/JedisSentinelPoolTest.java

Let's wrap with try-catch and remove expected statement. The test will pass even JedisConnectionException is thrown from obj1.

HeartSaVioR · 2017-10-22T09:32:31Z

src/test/java/redis/clients/jedis/tests/JedisSentinelPoolTest.java

add fail() below pool.getResource()

sazzad16 · 2018-01-24T06:35:16Z

@chrisribble IMO, both scenarios should be successful. Choosing only one doesn't sound right to me.

Waiting for your response on my query about creating new pool.

yapei123 · 2018-01-24T17:42:48Z

@chrisribble @sazzad16 There are two tests: one tests resetting password from bad to good, another from good to bad. The first one has no issue.

chrisribble · 2018-01-24T21:43:18Z

@sazzad16 Yes, the purpose of this PR is to avoid creating a new pool when you want to change the password that is used for new connections (resources).

The purpose of the PR is not to force the existing resources to be closed/released/etc when the password is changed.

Instead of creating a new pool in the test, what if we request a second resource while holding the first resource? That will ensure that the pool has to create a new connection with the bad password.

Sequence of actions:

Create pool with good password
Request resource
Pool uses good password to make connection
Pool adds resource to pool and hands out to caller
Change password to bad password
Without closing first resource (and thus avoiding release back to pool), request another resource
Pool uses bad password to attempt to make connection
Pool throws exception and caller verifies thrown

How does that sound?

JedisSentinelPool

sazzad16 · 2018-01-27T03:49:32Z

@chrisribble

Without closing first resource (and thus avoiding release back to pool), request another resource

I'm strongly against this idea that already requested resources would enforced to be unclosed for a valid operation in Pool.getResource(). I hope you'd understand my viewpoint.

PS: This is my own view. Other collaborators may agree with you. We can wait for them to respond.

chrisribble · 2018-01-27T14:13:34Z

@sazzad16 I guess I don't understand completely because it's not that the underlying code has to enforce that the requested resource is unclosed; it's just that there's no straightforward way to validate that the password has been changed without keeping the previously-requested resource unclosed because there is no external way for the caller to tell the pool to evict a closed resource.

Without the closed (released) resource being evicted from the pool, we will get it again the next time we call getResource. Do you have any other ideas on a safe way to evict the resource from the pool without resorting to clearing the entire pool?

I think maybe it's a mistake to be writing a test for this behavior (switching good password to bad password) using JedisPool and that the test should be directly validating the behavior on JedisFactory. What do you think?

sazzad16

@yapei123 I know it has been a long time, but would you be able to address change requests only within this review?

sazzad16 · 2019-01-05T02:12:15Z

src/test/java/redis/clients/jedis/tests/JedisPoolTest.java

+    JedisPool pool = new JedisPool(new JedisPoolConfig(), factory);
+    Jedis obj = null;
+    try {
+      pool.getResource();


Get this resource and close it properly, just to avoid any possible connection leak.

sazzad16 · 2019-01-05T02:14:17Z

src/test/java/redis/clients/jedis/tests/JedisSentinelPoolTest.java

+    JedisSentinelPool pool = new JedisSentinelPool(MASTER_NAME, sentinels, new JedisPoolConfig(), factory);
+    Jedis obj = null;
+    try {
+      pool.getResource();


Get this resource and close it properly, just to avoid any possible connection leak. try-with-resources can be used for convenience.

sazzad16 · 2019-01-05T02:18:35Z

src/test/java/redis/clients/jedis/tests/JedisPoolTest.java

+      fail("Could not get resource from pool");
+    } catch (JedisConnectionException e) {
+      factory.setPassword("foobared");
+      obj = pool.getResource();


Close obj within this catch block. Optionally, you can declare obj (aka Jedis obj = pool.getResource();) here with/without try-with-resources.

sazzad16 · 2019-01-05T02:18:56Z

src/test/java/redis/clients/jedis/tests/JedisSentinelPoolTest.java

+      fail("Could not get resource from pool");
+    } catch (JedisConnectionException e) {
+      factory.setPassword("foobared");
+      obj = pool.getResource();


Close obj within this catch block. Optionally, you can declare obj (aka Jedis obj = pool.getResource();) here with/without try-with-resources.

yapei123 mentioned this pull request Sep 13, 2017

Request New Feature: allowing the ability to reset password during runtime for Jedis's auth #1602

Closed

yapei123 mentioned this pull request Sep 13, 2017

Add ability to reset password in JedisFactory at runtime #1605

Closed

yapei123 force-pushed the master branch 6 times, most recently from 59f4f03 to 15aadd1 Compare September 15, 2017 23:04

yapei123 force-pushed the master branch 7 times, most recently from a5370bb to 404d4d6 Compare September 22, 2017 06:59

HeartSaVioR reviewed Oct 22, 2017

View reviewed changes

HeartSaVioR added the wait for more reviews label Oct 22, 2017

HeartSaVioR added this to the 2.10.0 milestone Oct 22, 2017

yapei123 force-pushed the master branch 2 times, most recently from f04f266 to fb581c7 Compare October 23, 2017 16:01

yapei123 force-pushed the master branch 2 times, most recently from b6b0a07 to 813cb62 Compare January 24, 2018 17:22

yapei123 force-pushed the master branch from 813cb62 to c7a4877 Compare January 24, 2018 21:38

yapei123 force-pushed the master branch 2 times, most recently from a875867 to 5394045 Compare January 24, 2018 22:14

Add ability to reset password in JedisFactory for JedisPool and

731bfec

JedisSentinelPool

yapei123 force-pushed the master branch from 5394045 to 731bfec Compare January 24, 2018 22:19

bsnyder788 approved these changes Jun 5, 2018

View reviewed changes

gkorland modified the milestones: 2.10.0, 3.1.0 Dec 6, 2018

sazzad16 suggested changes Jan 5, 2019

View reviewed changes

sazzad16 modified the milestones: 3.1.0, 3.2.0 Jul 17, 2019

sazzad16 modified the milestones: 3.2.0, 3.3.0 Nov 25, 2019

sazzad16 modified the milestones: 3.3.0, 3.4.0 Mar 14, 2020

sazzad16 modified the milestones: 3.4.0, 3.5.0 Nov 24, 2020

sazzad16 mentioned this pull request Dec 13, 2020

Add ability to reset password in JedisFactory for JedisPool and JedisSentinelPool #2314

Closed

sazzad16 modified the milestones: 3.5.0, 3.6.0 Jan 19, 2021

sazzad16 mentioned this pull request Jan 28, 2021

Add ability to reset password in JedisFactory (II) #2315

Merged

sazzad16 closed this in #2315 Mar 17, 2021

sazzad16 removed the wait for more reviews label Apr 22, 2021

Add ability to reset password in JedisFactory #1606

Add ability to reset password in JedisFactory #1606

Uh oh!

Conversation

yapei123 commented Sep 13, 2017

Uh oh!

marcosnils commented Sep 13, 2017

Uh oh!

yapei123 commented Sep 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yapei123 commented Sep 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcosnils commented Sep 13, 2017

Uh oh!

marcosnils commented Sep 15, 2017

Uh oh!

marcosnils commented Sep 15, 2017

Uh oh!

marcosnils commented Sep 15, 2017

Uh oh!

yapei123 commented Sep 18, 2017

Uh oh!

yapei123 commented Sep 22, 2017

Uh oh!

chrisribble commented Sep 29, 2017

Uh oh!

HeartSaVioR left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sazzad16 commented Jan 24, 2018

Uh oh!

yapei123 commented Jan 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisribble commented Jan 24, 2018

Uh oh!

sazzad16 commented Jan 27, 2018

Uh oh!

chrisribble commented Jan 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sazzad16 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

yapei123 commented Sep 13, 2017 •

edited

Loading

yapei123 commented Sep 13, 2017 •

edited

Loading

yapei123 commented Jan 24, 2018 •

edited

Loading

chrisribble commented Jan 27, 2018 •

edited

Loading