Support rename-command (#1817) #1822

gkorland · 2018-06-18T15:58:22Z

No description provided.

sazzad16

Please use 2 spaces for a tab in entire implementation.

src/test/java/redis/clients/jedis/tests/commands/RenameProtocolCommandsTest.java

src/main/java/redis/clients/jedis/Protocol.java

src/test/java/redis/clients/jedis/tests/commands/RenameProtocolCommandsTest.java

src/main/java/redis/clients/jedis/Protocol.java

src/test/java/redis/clients/jedis/tests/commands/RenameProtocolCommandsTest.java

sazzad16 · 2018-07-15T11:54:33Z

LGTM!

@marcosnils @HeartSaVioR

marcosnils · 2018-07-15T15:02:10Z

What if we also add a rename method to the Jedis class which takes a Command and a String as a rename?. The reason why I'm suggesting this is because from the API perspective Jedis users will expect to see the rename method. This is how @mp911de did it in lettuce also (https://github.com/lettuce-io/lettuce-core/blob/dde37483a31215ebf17f7141fea6454971ea9a08/src/test/java/io/lettuce/core/commands/KeyCommandTest.java#L212-L214)

Thoughts?

sazzad16 · 2018-07-15T17:20:51Z

@marcosnils Your provided link is about rename command which is already implemented in Jedis

https://github.com/xetorthio/jedis/blob/d1fb17ce3021f8bac135aef3fd306d62c877c13c/src/main/java/redis/clients/jedis/Jedis.java#L304-L308

But this PR is about supporting rename-command of Redis Security.

marcosnils · 2018-07-16T00:41:41Z

@smadasu you're right. How about if we add the renameCommand or something similar to the Jedis API.
I'm not entirely sure if doing Protocol.<CMD>.rename("something") is the cleanest way from the API perspective.

sazzad16 · 2018-07-16T05:51:39Z

@marcosnils

How about if we add the renameCommand or something similar to the Jedis API.

What we have in Jedis/BinaryJedis/JedisCluster etc are Redis commands. But rename-command is not a Redis command.
Unlike Redis commands, rename-command is done by configuration file.

I'm not entirely sure if doing Protocol..rename("something") is the cleanest way from the API perspective.

I'm also concerned about that. But I couldn't find another way, let alone a better one, to support this feature.

I can see only two options regarding this. Either use an available solution or leave that feature out from supporting.

gkorland · 2018-07-16T06:28:23Z

@marcosnils rename-command shouldn't be part of the API since as @sazzad16 mentioned a command is renamed using the Redis configuration file. Meaning, a user shouldn't really rename methods in runtime.

@sazzad16 I think it will be a shame to leave this capability aside since command rename is a simple but efficient security method. Which indeed doesn't add any really security layer but protect from most of crawlers attacks.

marcosnils · 2018-07-16T12:19:34Z

ename-command shouldn't be part of the API since as @sazzad16 mentioned a command is renamed using the Redis configuration file. Meaning, a user shouldn't really rename methods in runtime.

Right. Still the Protocol.<CMD>.rename("something") doesn't convince me. Can we try to look for an alternative for this?. Maybe check how / if other libraries are doing it?

gkorland · 2018-07-16T16:12:45Z

@marcosnils node_redis is doing it on client initialization

redis.createClient([options])

*rename_commands* -
Passing an object with renamed commands to use instead of the original functions. For example, if you renamed the command KEYS to "DO-NOT-USE" then the rename_commands object would be: { KEYS : "DO-NOT-USE" } .

gkorland · 2018-07-16T16:20:03Z

Same with .NET client StackExchange.Redis

var commands = new Dictionary<string,string> {
        { "info", null }, // disabled
        { "select", "use" }, // renamed to SQL equivalent for some reason
};

var options = new ConfigurationOptions {
    // ...
    CommandMap = CommandMap.Create(commands),
    // ...
}

gkorland · 2018-07-22T11:41:36Z

@marcosnils I don't think our current design fit adding it constructor settings

marcosnils · 2018-07-23T04:00:17Z

Maybe we can resurrect #1787 and add it there? @gkorland WDYT?

sazzad16 · 2018-07-23T06:56:50Z

@marcosnils Don't know if it's relevant, but a note: currently, there is a way to alter localhost in Jedis which is done without constructor.

https://github.com/xetorthio/jedis/blob/d1fb17ce3021f8bac135aef3fd306d62c877c13c/src/main/java/redis/clients/jedis/HostAndPort.java#L120-L124

gkorland · 2018-07-23T07:47:44Z

@marcosnils I don't think #1787 will help here unless we also moving from Enums and make a bigger redesign. It's probably cleaner to do it as part of the constructor but are we ready to make this refactor?

As @sazzad16 mentioned I think this pattern is already being used in the product for another rare use case so I think it can be a good compromise.

marcosnils · 2018-07-23T13:09:31Z

As @sazzad16 mentioned I think this pattern is already being used in the product for another rare use case so I think it can be a good compromise.

I'm not against this change, I'm just trying to evaluate all other alternatives as I'm not 100% convinced about this approach. As you're saying, the HostAndPort case is rare. In this case, the rename feature is something common to Redis / Jedis and there might be some other things like this in the future.

I guess we could move forward with this approach and refactor it later in the future, but taking the opportunity that we'll be probably releasing 3.0 next, I'd like to take advantage of the interface breaking changes and do it the right way.

sazzad16 · 2019-07-20T04:39:43Z

Makefile

 daemonize yes
 protected-mode no
 port 6386
+requirepass foobared


Test requires instance at 6386 port to be password-less. ~~Either create a new instance or create a new one.~~

"a new instance or create a new one"? what is the different between "new instance" and "new one"?

Oops!

"Either use password-less instance or create a new one."

sazzad16 · 2019-07-20T04:40:12Z

@gkorland I vote to include this feature. IMO, adding solution for one configuration is far better than not having solution for multiple configuration.

And I'm okay with your implementation. Not to mention, we've waited a year for an alternate solution.

jaipreet-chhatwal · 2019-07-24T06:37:05Z

@gkorland @sazzad16 : When we configure Redis with Sentinel for HA, arent we suppoosed to configure the same Redis password in every instance. Isn't this expected already that we need to have same configuration across nodes.
So i am not able to understand the problem with
"adding solution for one configuration is far better than not having solution for multiple configuration." since we are already "
Please correct me if i am missing a very basic point and not on same page with you guys.

Also, can we conclude on the version in which this rename-command feature will be supported.

sazzad16 · 2019-07-24T07:22:10Z

@jaipreet-chhatwal

Isn't this expected already that we need to have same configuration across nodes.

Yes and No.

Within same sentinel, it is expected that the nodes within that sentinel are of same configuration. For example, they are expected to have same password.

But Jedis is not limited to connect with only one Redis instance/sentinel/cluster. It allows you to connect with many of them at once. So you can set up two different sentinels (two different group of nodes, say A and B) and connect with them simultaneously within same application using Jedis. Those two set of sentinel nodes are not required to have same configuration. For example, both set of nodes A and B are not required to have same password, even though all nodes of sentinel A are expected to have same password as well as all nodes of sentinel B are expected to have same password.

I hope this clears your confusion about my statement.

sazzad16 · 2019-07-24T07:22:43Z

@jaipreet-chhatwal

can we conclude on the version in which this rename-command feature will be supported.

Not yet.

gkorland · 2019-07-24T08:05:44Z

@sazzad16 if we agree this solution is better than nothing, let's mark it as 3.2.0?

sazzad16 · 2019-07-24T08:10:55Z

@gkorland sure thing!

@marcosnils It has been a year. Humbly speaking, we have waited enough for an alternate solution.

sazzad16 · 2019-07-30T12:37:14Z

src/main/java/redis/clients/jedis/Protocol.java

    }
+
+    public void rename(final String newName) {
+      raw = SafeEncoder.encode(newName);


Couldn't this be an issue in multithreading? Synchronized/Lock may be required here.

You're right, but volatile should do, I'm trying to find a way to avoid the performance penalty on each getRaw() call

+1 to volatile (or AtomicReference), at the moment it's not though, so we need to update that to either volatile or AtomicReference.

We should make this fail (and have tests for) if newName is:

null

empty

an existing command name

yangbodong22011 · 2021-01-19T12:57:47Z

src/test/java/redis/clients/jedis/tests/commands/RenameProtocolCommandsTest.java

+      jedis.del("mykey");
+      fail("'DEL' command should be unknown");
+    } catch (JedisDataException expected) {
+      assertEquals("ERR unknown command 'DEL'", expected.getMessage());


Suggested change

assertEquals("ERR unknown command 'DEL'", expected.getMessage());

// We need use a regex for the error message, because it may has different format

// 1, ERR unknown command 'DEL'

// 2, ERR unknown command `del`, with args beginning with: `abc` (Redis 6.2)

Assert.assertTrue(expected.getMessage().matches("ERR unknown command .DEL.*"));

mina-asham · 2021-01-28T14:06:17Z

src/main/java/redis/clients/jedis/Protocol.java

    }
+
+    public void rename(final String newName) {
+      raw = SafeEncoder.encode(newName);


+1 to volatile (or AtomicReference), at the moment it's not though, so we need to update that to either volatile or AtomicReference.

We should make this fail (and have tests for) if newName is:

null

empty

an existing command name

sazzad16 · 2021-04-11T14:31:26Z

@gkorland Command renaming is deprecated in Redis. See redis/redis@7604ab7
Should we close this PR? Or, keep this PR open for discussion?

gkorland · 2021-04-11T16:41:53Z

Lets close

guy added 3 commits June 18, 2018 18:43

Allow renaming Command (#1817)

c2e7276

Allow renaming Command - fix pom.xml test (#1817)

7a1527c

Allow renaming Command - clean Commands on tear down (#1817)

d1f41c2

sazzad16 mentioned this pull request Jul 13, 2018

jedis not support rename-command #1817

Closed

sazzad16 suggested changes Jul 14, 2018

View reviewed changes

src/test/java/redis/clients/jedis/tests/commands/RenameProtocolCommandsTest.java Outdated Show resolved Hide resolved

src/main/java/redis/clients/jedis/Protocol.java Outdated Show resolved Hide resolved

gkorland added 2 commits July 15, 2018 11:32

Use try-catch with fail and assertion on exception.

f54fa0b

Fix indentation set two spaces

2374c44

sazzad16 suggested changes Jul 15, 2018

View reviewed changes

gkorland added 2 commits July 15, 2018 12:42

Add message to fail()

7c76999

Replace tab character

dd00cee

sazzad16 changed the title ~~Allow renaming Command (#1817)~~ Support rename-command (#1817) Jul 15, 2018

sazzad16 suggested changes Jul 15, 2018

View reviewed changes

src/test/java/redis/clients/jedis/tests/commands/RenameProtocolCommandsTest.java Outdated Show resolved Hide resolved

change assert fail message

cc100f6

sazzad16 approved these changes Jul 15, 2018

View reviewed changes

sazzad16 added the wait for more reviews label Jul 15, 2018

sazzad16 added this to the 2.10.0 milestone Jul 15, 2018

sazzad16 suggested changes Jul 20, 2019

View reviewed changes

sazzad16 added this to the 3.2.0 milestone Jul 24, 2019

sazzad16 suggested changes Jul 30, 2019

View reviewed changes

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 26, 2020

sazzad16 added the HANGING PR label Dec 9, 2020

sazzad16 modified the milestone: 3.5.0 Dec 9, 2020

sazzad16 requested review from mina-asham and yangbodong22011 January 19, 2021 05:28

yangbodong22011 reviewed Jan 19, 2021

View reviewed changes

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

mina-asham suggested changes Jan 28, 2021

View reviewed changes

sazzad16 removed this from the 3.6.0 milestone Mar 8, 2021

sazzad16 closed this Apr 11, 2021

sazzad16 removed the HANGING PR label Jul 2, 2021

sazzad16 removed the discussion needed label Dec 19, 2021

-      assertEquals("ERR unknown command 'DEL'", expected.getMessage());
+            // We need use a regex for the error message, because it may has different format
+            // 1, ERR unknown command 'DEL'
+            // 2, ERR unknown command `del`, with args beginning with: `abc` (Redis 6.2)
+            Assert.assertTrue(expected.getMessage().matches("ERR unknown command .DEL.*"));

Support rename-command (#1817) #1822

Support rename-command (#1817) #1822

Uh oh!

Conversation

gkorland commented Jun 18, 2018

Uh oh!

sazzad16 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sazzad16 commented Jul 15, 2018

Uh oh!

marcosnils commented Jul 15, 2018

Uh oh!

sazzad16 commented Jul 15, 2018

Uh oh!

marcosnils commented Jul 16, 2018

Uh oh!

sazzad16 commented Jul 16, 2018

Uh oh!

gkorland commented Jul 16, 2018

Uh oh!

marcosnils commented Jul 16, 2018

Uh oh!

gkorland commented Jul 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gkorland commented Jul 16, 2018

Uh oh!

gkorland commented Jul 22, 2018

Uh oh!

marcosnils commented Jul 23, 2018

Uh oh!

sazzad16 commented Jul 23, 2018

Uh oh!

gkorland commented Jul 23, 2018

Uh oh!

marcosnils commented Jul 23, 2018

Uh oh!

sazzad16 Jul 20, 2019 • 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!

gkorland Jul 21, 2019

Choose a reason for hiding this comment

Uh oh!

sazzad16 Jul 21, 2019

Choose a reason for hiding this comment

Uh oh!

sazzad16 commented Jul 20, 2019

Uh oh!

jaipreet-chhatwal commented Jul 24, 2019

Uh oh!

sazzad16 commented Jul 24, 2019

Uh oh!

sazzad16 commented Jul 24, 2019

Uh oh!

gkorland commented Jul 24, 2019

Uh oh!

sazzad16 commented Jul 24, 2019

Uh oh!

sazzad16 Jul 30, 2019

Choose a reason for hiding this comment

Uh oh!

gkorland Nov 25, 2019

Choose a reason for hiding this comment

Uh oh!

mina-asham Jan 28, 2021

Choose a reason for hiding this comment

Uh oh!

yangbodong22011 Jan 19, 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!

gkorland commented Jul 16, 2018 •

edited

Loading

sazzad16 Jul 20, 2019 •

edited

Loading

yangbodong22011 Jan 19, 2021 •

edited

Loading