Add support for more Redis Cluster commands #10434

jakule · 2022-02-17T18:03:11Z

This PR adds support for more Redis command in cluster mode. When connecting to Redis through Teleport client should always connect in a standalone mode (redis-cli without -c flag for example). If Redis works in the cluster mode then the Teleport responsibility is to "emulate" standalone behaviour. The main implication of that is that Teleport need to send GET/SET command to the correct instance to retrieve/set keys. go-redis already implements most of the functionality for us, but not all commands are supported.
I was looking on the list commands supported by Envoy https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/other_protocols/redis#redis-cluster-support which works similar to Teleport in this case.

This PR should get as close to all commands supported by Envoy, with a few exceptions:
MSET - I don't think we should support it. MSET allows to set multiple keys as once. Envoy internally splits the requested keys and sends multiple MSET requests to multiple servers. The problem here is error handling. If one of the commands fails (only on one server for example), Teleport could return an error, but that leaves the cluster in inconsistent state where only some keys are updated. Currently we just pass the MSET command the the server. It will work only if all keys are on the server.

Redis transactions are blocked. Similar as above. Currently a transaction will fail if it affects multiple servers. I think that clocking them explicitly is safer and less confusing for users.

This PR doesn't contain any tests as currently we don't have a way to simulate Redis Cluster behaviour in tests. I'm working on getting Redis Cluster to our system tests, but I'll create a separate PR for that when it's ready.

lib/srv/db/redis/client.go

r0mant · 2022-02-18T06:43:50Z

lib/srv/db/redis/client.go

+	}
+}
+
+// Process add supports for additional cluster commands.


Nit: Explain why it is required like you did in this PR's description.

r0mant · 2022-02-18T06:44:20Z

lib/srv/db/redis/client.go

+func (c *clusterClient) Process(ctx context.Context, inCmd redis.Cmder) error {
+	cmd, ok := inCmd.(*redis.Cmd)
+	if !ok {
+		return trace.BadParameter("failed to cast Redis command")


Include %T in the message?

r0mant · 2022-02-18T06:45:49Z

lib/srv/db/redis/client.go

+				return trace.Wrap(err)
+			}
+
+			mtx.Lock()


Why is locking needed? Does ForEachMaster execute functions concurrently? If so, explain that with a comment.

Yes, ForEachMaster is called in parallel. I'll add a comment.

r0mant · 2022-02-18T06:47:21Z

lib/srv/db/redis/engine.go

-	default:
-		// We've checked that while validating the config, but checking again can help with regression.
-		return trace.BadParameter("incorrect connection mode %s", connectionOptions.mode)
+	connectionAddr := fmt.Sprintf("%s:%s", connectionOptions.address, connectionOptions.port)


Use net.JoinHostPort instead of Sprintf.

smallinsky · 2022-02-18T13:37:16Z

lib/srv/db/redis/client.go

+			cmd.SetVal(singleCmd.Val())
+			cmd.SetErr(singleCmd.Err())


race condition c.ForEachMaster calls the ForEachMaster in for each master asynchronously in new goroutine.

We can talk about this 😅 But I think that there is a different issue here. If one of the commands returns error we should return that error and not override it later with "OK" value. I'll fix that.

Added mutex and error check above.

smallinsky · 2022-02-18T13:38:47Z

lib/srv/db/redis/client.go

+				mtx.Lock()
+				resultsKeys = append(resultsKeys, redis.Nil)
+				mtx.Unlock()


Why lock is needed here ?

You're right. This is not called in parallel. I'll remove it. Thanks

smallinsky · 2022-02-18T13:39:41Z

lib/srv/db/redis/client.go

+			mtx.Lock()
+			resultsKeys = append(resultsKeys, result.Val())
+			mtx.Unlock()


Same here. I'm not sure based on code to see why this lock is needed.

You're right. Thanks.

smallinsky · 2022-02-28T13:51:34Z

lib/srv/db/redis/protocol/resp2.go

@@ -99,6 +99,10 @@ func WriteCmd(wr *redis.Writer, vals interface{}) error {
 		if err := writeStringSlice(wr, val); err != nil {
 			return trace.Wrap(err)
 		}
+	case []bool:


Does the following case case []interface{}' should already handle the bool array ? Based on the code it looks that writeSliceand writeBoolSlice` functions are the same.

Yes, they are the same except the function signature. Go distinguish between slices of different types, so generic []interface{} won't be called when []bool is passed: https://go.dev/play/p/betqiX7vl1L
The same with the function call; []bool cannot be passed as []interface{}.

Go 1.18 would be helpful here: https://go.dev/play/p/c6WIHvhAN2n?v=gotip

For now the best what I can do is to use reflection (and add a UT).

smallinsky · 2022-02-28T14:39:49Z

lib/srv/db/redis/client.go

+// Commands with additional processing in Teleport when using cluster mode.
+const (
+	multiCmd    = "multi"
+	execCmd     = "exec"
+	watchCmd    = "watch"
+	dbsizeCmd   = "dbsize"
+	scanCmd     = "scan"
+	keysCmd     = "keys"
+	mgetCmd     = "mget"
+	flushallCmd = "flushall"
+	flushdbCmd  = "flushdb"
+	scriptCmd   = "script"
+)


The comment says that this commands have additional processing during cluster mode but based on the but some of them are not supported right now in cluster mode like: execCmd https://github.com/gravitational/teleport/pull/10434/files#diff-6230c3f30596bb3b46e45d42bc9da09c2fac0fb440e69ed818e6daf73570b55bR138

Good catch. I move them inside the case at some point and forgot to move them here.

smallinsky · 2022-02-28T15:12:21Z

lib/srv/db/redis/client.go

+				return nil
+			}
+
+			cmd.SetVal(singleCmd.Val())


Wonder if in case where the flow is skipped by the continue call when cmd.Err() != nil the cmd.Val and cmd.Err can be both nil.
In that case processSeverResponse can return nil, nil so further flow can panic.

Unify Redis not supported error.

* Add support for more Redis commands in Cluster mode. * Add reconnect on AUTH logic to Redis. * Redis code refactoring. * Update list of unsupported Redis commands in Cluster mode.

* Add support for more Redis commands in Cluster mode. * Add reconnect on AUTH logic to Redis. * Redis code refactoring. * Update list of unsupported Redis commands in Cluster mode. Backport #10434.

jakule marked this pull request as ready for review February 17, 2022 21:12

jakule requested a review from r0mant February 17, 2022 21:12

github-actions bot added the database-access Database access related issues and PRs label Feb 17, 2022

github-actions bot requested review from lxea and nklaassen February 17, 2022 21:12

jakule requested review from smallinsky and removed request for lxea and nklaassen February 17, 2022 21:12

jakule force-pushed the jakule/redis-cluster-cmds branch from 18622e8 to eae9258 Compare February 17, 2022 21:15

r0mant approved these changes Feb 18, 2022

View reviewed changes

smallinsky reviewed Feb 18, 2022

View reviewed changes

jakule added merge-for-v9 and removed merge-for-v9 labels Feb 18, 2022

jakule requested a review from smallinsky February 22, 2022 16:34

jakule force-pushed the jakule/redis-cluster-cmds branch from 88fab1a to 92dc806 Compare February 22, 2022 16:36

jakule added 10 commits February 23, 2022 22:26

Add support for more Redis Cluster commands

d427299

Move Redis Cluster client to a new file.

325b214

Add godocs

7048f24

Build fix

0dd5f43

Fix typo

410ffba

Address PR comments.

92f50c2

Add support for more commands.

8611dc6

Correct function comment

9b9b7c3

Add reconnect on AUTH logic to Redis.

ec8dbba

Redis code refactoring.

89a3989

jakule force-pushed the jakule/redis-cluster-cmds branch from 92dc806 to 89a3989 Compare February 25, 2022 02:22

jakule added 2 commits February 25, 2022 01:17

Update list of unsupported Redis commands in Cluster mode.

d50106c

Add reconnect comment.

50d5f75

jakule requested a review from r0mant February 25, 2022 06:20

smallinsky reviewed Feb 28, 2022

View reviewed changes

jakule added 4 commits March 1, 2022 02:03

Create generic Redis slice conversion function.

5f99946

Unify Redis not supported error.

Move Redis consts to the right section.

b4eb8ee

Go 1.17 build fix.

943645d

Fix lining error.

413d39e

smallinsky approved these changes Mar 2, 2022

View reviewed changes

jakule added 2 commits March 2, 2022 10:42

Merge branch 'master' into jakule/redis-cluster-cmds

efc7a94

Add type check for Redis Cluster MGET

a0c01bf

jakule enabled auto-merge (squash) March 2, 2022 16:40

jakule merged commit 705f042 into master Mar 2, 2022

jakule deleted the jakule/redis-cluster-cmds branch March 2, 2022 16:44

jakule mentioned this pull request Mar 2, 2022

[v9] Add support for more Redis Cluster commands #10760

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for more Redis Cluster commands #10434

Add support for more Redis Cluster commands #10434

jakule commented Feb 17, 2022 •

edited

Loading

r0mant Feb 18, 2022

r0mant Feb 18, 2022

jakule Feb 18, 2022

r0mant Feb 18, 2022

jakule Feb 18, 2022

r0mant Feb 18, 2022

smallinsky Feb 18, 2022

jakule Feb 18, 2022

jakule Feb 22, 2022

smallinsky Feb 18, 2022

jakule Feb 18, 2022

smallinsky Feb 18, 2022

jakule Feb 18, 2022

smallinsky Feb 28, 2022

jakule Mar 1, 2022

smallinsky Feb 28, 2022

jakule Mar 1, 2022

smallinsky Feb 28, 2022

Add support for more Redis Cluster commands #10434

Add support for more Redis Cluster commands #10434

Conversation

jakule commented Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakule commented Feb 17, 2022 •

edited

Loading