INS-12991: Redis authentication support #101

XA21X · 2021-05-28T00:06:17Z

TODO:

Connect to replica nodes to propagate ACL commands.
Extract refactors into separate PR to reduce size.
Benchmark auth vs non-auth for regression.
Write additional tests. (Extended existing test.)
Bump version.

Closes #91

benbromhead

Can you move the name changes (e.g. RawFrame stuff) to a different PR

src/server.rs

src/transforms/redis_transforms/redis_cluster.rs

benbromhead · 2021-06-01T08:03:05Z

Another thing to add to the todo is to do a rough benchmark, e.g. redis auth enabled on against the examples/redis-passthrough and maybe examples/redis-cluster using the redis benchmark toolset (e.g. ./src/redis-benchmark -n 10000000 -t get -P 4 -r 10000 -e -c 50).

The real todo is to include the benchmarks in CI/CD...

src/transforms/util/cluster_connection_pool.rs

benbromhead · 2021-06-17T06:46:40Z

Regarding benchmarking this change. There is now a PR for benchmarking on PR via criterion and cargo bench.

In addition, criterion now supports async function benchmarking, which should make writing micro benchs fairly easy.

src/transforms/redis_transforms/mod.rs

rukai · 2021-07-22T06:04:24Z

Can we pull the DockerComposeContext refactor into its own PR?
I think we need those fixes in master asap.

examples/redis-cluster-dr-auth/docker-compose.yml

rukai · 2021-08-02T03:57:05Z

To resolve the merge conflicts, at this point it might be easier to start from scratch on the latest main branch and copy over the changes manually.
You can then leave out the AsyncContext refactor, its a good idea but such a refactor should go in a separate PR.

shotover-proxy/src/runner.rs

Adapted workaround from tokio-rs/tracing#1466

shotover-proxy/src/transforms/redis_transforms/mod.rs

benbromhead · 2021-08-17T02:56:12Z

shotover-proxy/src/transforms/redis_transforms/redis_cluster.rs

+ args.get(streams_position + 1)
+ .and_then(RoutingInfo::for_key)
+ }
+ b"AUTH" => Some(RoutingInfo::Other(Command::AUTH)),


I think I'm confused here, wouldn't we want to route the auth command to all upstream redis nodes rather than a random one?

AUTH needs special handling (try auth -> switch context -> new connection pool) instead of forwarding to any nodes. The RoutingInfo::Other means this command needs to be handled outside. i.e. it's like Err(command) vs Ok(routes).

benbromhead · 2021-08-17T03:05:00Z

shotover-proxy/src/transforms/redis_transforms/redis_cluster.rs

- let sender = self.get_channels(&message.original).await;
+ let command = match &message.original {
+ RawFrame::Redis(Frame::Array(ref command)) => command,
+ RawFrame::Redis(_) => {


Redis supports some form of inline commands (e.g. so it's easier for telnet clients to interact with). Iirc the codec should error out long before we get here, but it might be worth short circtuiting an error into responses with a helpful error message back to the client.

Yeah, I only added this to avoid unhandled panics. Not sure what was blocking me from short circuit, will try again.

Inline commands are unsupported and therefore blocked by the codec framing so we can't provide a better message from here. The connection gets killed if as much as a single unexpected byte is out of place.

Syntax errors which have valid framing will now report back to the client:

❯ echo -n -e "*1\r\n:1000\r\n" | nc 127.0.0.1 6379 -ERR transform error: syntax error: bad command name ~ ❯ echo -n -e ":1000\r\n" | nc 127.0.0.1 6379 -ERR transform error: syntax error: bad command

shotover-proxy/src/transforms/redis_transforms/redis_cluster.rs

shotover-proxy/src/transforms/util/cluster_connection_pool.rs

rukai · 2021-08-18T04:58:11Z

I'm really struggling to get my head around this PR.
It seems like theres still a lot of refactoring outside of the actual implementation of authentication.
@benbromhead would it make sense for it to be split again and land these refactors first?

shotover-proxy/src/transforms/util/cluster_connection_pool.rs

shotover-proxy/tests/redis_int_tests/basic_driver_tests.rs

rukai · 2021-09-16T00:50:10Z

shotover-proxy/tests/redis_int_tests/basic_driver_tests.rs

- redis::cmd("GET").arg("{x}key3").query(&mut connection),
- Ok("food".to_string())
- );
+ // To reproduce the auth mixing issue caused by using PoolConnections:


Is this problem solved?
If so lets write a proper test for it instead of this comment.
If not lets raise an issue for us to follow up on it instead of this comment.

The underlying problem isn't solved - using the PoolConnections transform will violate assumptions that the auth implementation relies on. I'll raise an issue and remove this comment (but leave the test).

dont forget to raise the issue!

Raised #204

shotover-proxy/src/transforms/util/cluster_connection_pool.rs

shotover-proxy/tests/redis_int_tests/basic_driver_tests.rs

shotover-proxy/src/transforms/redis_transforms/redis_cluster.rs

rukai

LGTM
I would like ben to give the second review as he has the redis domain knowledge.

benbromhead · 2021-09-19T23:20:15Z

LGTM

Two minor fixes:
Fix the conflict in drivers test
Remove the unused member strict_close_mode: Option<bool> from the config struct

Sort these out and we can merge :D :D :D

benbromhead reviewed May 28, 2021

View reviewed changes

src/server.rs Outdated Show resolved Hide resolved

benbromhead reviewed May 28, 2021

View reviewed changes

src/transforms/redis_transforms/redis_cluster.rs Outdated Show resolved Hide resolved

johndelcastillo reviewed Jun 1, 2021

View reviewed changes

src/transforms/redis_transforms/redis_cluster.rs Outdated Show resolved Hide resolved

XA21X mentioned this pull request Jun 2, 2021

INS-12991: Pre-cleanup #102

Merged

XA21X self-assigned this Jun 2, 2021

benbromhead reviewed Jun 7, 2021

View reviewed changes

src/transforms/util/cluster_connection_pool.rs Outdated Show resolved Hide resolved

src/transforms/util/cluster_connection_pool.rs Outdated Show resolved Hide resolved

benbromhead reviewed Jun 7, 2021

View reviewed changes

src/transforms/util/cluster_connection_pool.rs Outdated Show resolved Hide resolved

XA21X force-pushed the INS-12991-auth-support branch 2 times, most recently from d408419 to 9348d6c Compare June 17, 2021 18:32

XA21X mentioned this pull request Jul 20, 2021

Improve error logging #109

Closed

rukai reviewed Jul 20, 2021

View reviewed changes

src/transforms/redis_transforms/mod.rs Outdated Show resolved Hide resolved

XA21X commented Jul 22, 2021

View reviewed changes

examples/redis-cluster-dr-auth/docker-compose.yml Outdated Show resolved Hide resolved

XA21X force-pushed the INS-12991-auth-support branch from f0cb297 to 8cd8301 Compare August 9, 2021 05:46

rukai reviewed Aug 9, 2021

View reviewed changes

shotover-proxy/src/runner.rs Outdated Show resolved Hide resolved

XA21X added 5 commits August 12, 2021 10:26

Rebase authentication support

e1b601c

Fix RUST_LOG env overrides

8023027

Adapted workaround from tokio-rs/tracing#1466

Log test connections errors

8e0b6af

Seperate volumes for redis-cluster-auth tests

b3b5270

Address or reword minor TODOs

4c44b9e

XA21X force-pushed the INS-12991-auth-support branch from 9c38b84 to 4c44b9e Compare August 12, 2021 01:37

benbromhead reviewed Aug 17, 2021

View reviewed changes

shotover-proxy/src/transforms/redis_transforms/mod.rs Outdated Show resolved Hide resolved

benbromhead reviewed Aug 17, 2021

View reviewed changes

shotover-proxy/src/transforms/redis_transforms/mod.rs Outdated Show resolved Hide resolved

benbromhead reviewed Aug 17, 2021

View reviewed changes

XA21X commented Aug 18, 2021

View reviewed changes

shotover-proxy/src/transforms/util/cluster_connection_pool.rs Outdated Show resolved Hide resolved

XA21X added 5 commits September 16, 2021 00:34

Simplify test_cluster_auth_redis

a14044b

Re-add minimal auth mixing test

c32d169

Tweak error logging

25db114

Set default connection timeout to 3s

fabe2fb

Add AUTH too many args check

da76a9f

XA21X requested a review from rukai September 15, 2021 16:54

rukai reviewed Sep 16, 2021

View reviewed changes

shotover-proxy/tests/redis_int_tests/basic_driver_tests.rs Outdated Show resolved Hide resolved

rukai reviewed Sep 16, 2021

View reviewed changes

shotover-proxy/src/transforms/util/cluster_connection_pool.rs Show resolved Hide resolved

XA21X added 5 commits September 16, 2021 11:12

Simplify on_auth error handling

ae4e108

Merge remote-tracking branch 'origin/main' into INS-12991-auth-support

b0da836

Clean up test_cluster_auth_redis

6dfaaab

Disable rebuild_connections on first message

f7ab249

Remove unnecessary collect

b76d859

rukai reviewed Sep 16, 2021

View reviewed changes

shotover-proxy/tests/redis_int_tests/basic_driver_tests.rs Outdated Show resolved Hide resolved

Make user less confusing in auth test

f3e347b

rukai reviewed Sep 16, 2021

View reviewed changes

shotover-proxy/src/transforms/redis_transforms/redis_cluster.rs Show resolved Hide resolved

rukai reviewed Sep 16, 2021

View reviewed changes

shotover-proxy/src/transforms/redis_transforms/redis_cluster.rs Outdated Show resolved Hide resolved

rukai reviewed Sep 16, 2021

View reviewed changes

shotover-proxy/src/transforms/redis_transforms/redis_cluster.rs Show resolved Hide resolved

Remove unnecessary transpose

f1f4b99

rukai approved these changes Sep 16, 2021

View reviewed changes

XA21X requested a review from benbromhead September 16, 2021 21:44

XA21X added 3 commits September 20, 2021 09:39

Merge remote-tracking branch 'origin/main' into INS-12991-auth-support

56d16b1

Remove unused strict_close_mode

d424c03

Clippy fix unnecesary clone

65a55ef

rukai merged commit 3ae4981 into main Sep 20, 2021

rukai deleted the INS-12991-auth-support branch September 20, 2021 06:09

XA21X mentioned this pull request Sep 21, 2021

ConnectionBalanceAndPool breaks authentication when used with RedisCluster #204

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

INS-12991: Redis authentication support #101

INS-12991: Redis authentication support #101

XA21X commented May 28, 2021 •

edited

Loading

benbromhead left a comment

benbromhead commented Jun 1, 2021

benbromhead commented Jun 17, 2021

rukai commented Jul 22, 2021

rukai commented Aug 2, 2021

benbromhead Aug 17, 2021

XA21X Aug 17, 2021

benbromhead Aug 17, 2021

XA21X Aug 17, 2021

XA21X Sep 3, 2021

rukai commented Aug 18, 2021 •

edited

Loading

rukai Sep 16, 2021

XA21X Sep 16, 2021

rukai Sep 16, 2021

XA21X Sep 21, 2021

rukai left a comment

benbromhead commented Sep 19, 2021

INS-12991: Redis authentication support #101

INS-12991: Redis authentication support #101

Conversation

XA21X commented May 28, 2021 • edited Loading

benbromhead left a comment

Choose a reason for hiding this comment

benbromhead commented Jun 1, 2021

benbromhead commented Jun 17, 2021

rukai commented Jul 22, 2021

rukai commented Aug 2, 2021

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

rukai commented Aug 18, 2021 • 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

rukai left a comment

Choose a reason for hiding this comment

benbromhead commented Sep 19, 2021

XA21X commented May 28, 2021 •

edited

Loading

rukai commented Aug 18, 2021 •

edited

Loading