Refactor Client constructor options, add (m)TLS support #17

imiric · 2023-07-07T16:00:49Z

This is a fix/feature implementation for #13.

It refactors the Client constructor to accept options that are better aligned with the Node Redis API, to increase familiarity with an established JS library, and to allow specifying (m)TLS options, as proposed here. This means that these are breaking changes for users of the previous API.

See the updated README and the tests for usage examples.

If you want to test this manually with a real Redis TLS server, see this gist for instructions.

What's pending:

There's a strange issue when using mTLS and the original example script, that causes Redis to abort with ERR Protocol error: unauthenticated multibulk length.
I tracked it down to this issue, and this comment mentions this being some MSET limitation. We use regular SET in the script, but maybe this is a general limitation of some kind(?). I'm not sure, but we'll need to look into it.
According to this other comment, there's a nasty fix for this... 🙈
UPDATE 2023-08-10: I haven't been able to reproduce this again, but we should be aware that it might be an issue to look into later.
General cleanup and DRYing.
Fix existing tests, and adding more.
Address all the TODOs, and feedback from this PR.
Maybe split this PR into separate refactor and TLS changes PRs, but I wanted to leave them together as a draft for now.
UPDATE 2023-08-10: I decided to leave it as a single PR, since the changes are closely related.

redis/options.go

imiric · 2023-08-10T15:17:36Z

@oleiade This is now ready for a proper review. ☺️

The commit log is a bit messy, but it's a PITA to clean it up at this point. 😓 You can just review it as a general diff, going by file.

Please take a look and let me know.

imiric · 2023-08-10T15:55:53Z

redis/client_test.go

+		{
+			name: "err/object/cluster_inconsistent_option",
+			arg: `{
+				cluster: {
+					nodes: [
+						{
+							username: 'user1',
+							password: 'pass',
+							socket: {
+								host: 'host1',
+								port: 6379,
+							},
+						},
+						{
+							username: 'user2',
+							password: 'pass',
+							socket: {
+								host: 'host2',
+								port: 6379,
+							},
+						}
+					]
+				}
+			}`,
+			expErr: `invalid options; reason: inconsistent username option: user1 != user2`,


I'm still not sure about this behavior. redis.UniversalOptions only allows a single username, password and many other values, even if more than one Addrs is specified. So the point of setConsistentOptions() is to ensure that these values are consistent across all nodes.

But I'm not sure if this is something that's also enforced by Redis, or if there can be a case where e.g. nodes within a cluster use a different username or password, or any other setting. Intuitively, I don't see why that shouldn't be allowed, in which case there would be no way for users to connect to such a cluster. But maybe this is also a Redis limitation that go-redis is compliant with, in which case this is fine.

I'm not familiar with Redis Cluster deployments, and didn't find anything specifically about this in the documentation. My hunch is that this is fine, but wanted to point it out.

redis/stub_test.go

CLAassistant · 2023-10-09T16:34:29Z

All committers have signed the CLA.

oleiade · 2023-10-12T07:57:02Z

Hey @imiric, it seems since you left you now need to sign the CLA 😓 Would you mind doing it, please? You mentioned we might have to split the PR, but if we don't I'd prefer to keep your contribution acknowledged 👍🏻

imiric · 2023-10-12T08:37:26Z

Hey @oleiade! This wouldn't be an issue had this PR been reviewed in a timely manner... 😜

I think the CLA here is fine:

But GH/CLA Assistant might be confused since I deleted the Grafana email from my account, so it can't associate the commits with my account. I think the solution is as you have done by recreating the commits as yourself, and leaving me as the author.

BTW, I don't think this PR should be split, as it would be a lot of work for no real gain. I'm happy to address any feedback, though I may be slow to respond.

Hope all is well. Cheers!

oleiade · 2023-10-12T09:19:38Z

@imiric Touché ❤️‍🔥 ! That's totally fair. Sorry it took so long for me to get back to it 🙇🏻

I will proceed with recreating the commits myself, leaving you as an author, and keeping the branch as is then 🤝

All is well on this side of the Pyrenees, I hope it's the same for you 🖖🏻

codebien · 2023-10-13T14:18:20Z

This worked for me for the latest commit.

git commit --amend --author="Ivan Mirić <1009277+imiric@users.noreply.github.com>"

@oleiade You have to loop over the commits applying the command with the specific commit or use the same command as I did doing a rebase and setting all the commits with the edit stage.

oleiade · 2023-10-16T08:01:53Z

@codebien Thanks for the heads-up, the issue I have is not with changing the email itself, but rather which email to use indeed 🙇🏻

At the moment, it is set to ivan@loadimpact.com as the author, which I assume has been deleted.

codebien · 2023-10-16T10:13:20Z

but rather which email to use indeed

@oleiade you can use the mail I've posted that connects Ivan to his GitHub account. You don't need a real e-mail.

imiric · 2023-10-16T13:04:02Z

@codebien is right, and @oleiade please remove my personal emails from your comment. 🙏

oleiade · 2023-10-17T07:18:58Z

@codebien how did you find the information as to which email to use so I know where to look next time?

To allow passing objects similar to node-redis. See #13 (comment)

codebien

LGTM, I left some nitpicks. The unique point I have is regarding the API that seems to have some overlap between the possible ways of providing the same configuration where I would prefer a single way for them, but at this point, I think it's more important to merge this and start collecting feedback.

README.md

redis/client.go

Co-authored-by: Ivan <2103732+codebien@users.noreply.github.com>

oleiade

LGTM 👍🏻 🚀

Took me some time to get all the "infrastructure" I needed to test this properly, but I'm quite happy how this whole PR, and I'm keen to merge it as is. Outstanding work 🙇🏻

oleiade · 2023-10-20T12:07:05Z

I'll start documenting the change in k6-docs PR while we wait for @mstoykov 's review.

mstoykov · 2023-10-24T07:03:26Z

redis/client.go

+		k6dialer, ok := vuState.Dialer.(*netext.Dialer)
+		if !ok {
+			panic(fmt.Sprintf("expected *netext.Dialer, got: %T", vuState.Dialer))
+		}
+		tlsDialer := &tls.Dialer{
+			NetDialer: &k6dialer.Dialer,
+			Config:    tlsCfg,
+		}


Unfortunately I do think this completely circumvent the current tings netext.Dialer does:

Doing the resolving of the hostnames - including over options.hosts

blacklisting both hostnames and ips - which likely will be a big problem in cloud

emtting bytes read/written - which in this case might be an okay thing.

Looking at tls.Dialer though - it seems not possible to put netext.Dialer there. So maybe we should wrap it the other way aroudn 🤔

Good catch indeed 👍🏻 I gave a stab at addressing this concern in 3676bef, by defining a custom dialer function which relies on k6's VU (netext.Dialer) dialer, and manually upgrades the connection to TLS (respecting the provided configuration) when TLS certificates are present.

Testing it showed that blocked hostnames were effectively taken into account.

Let me know if that addresses your point @mstoykov 🙇🏻

mstoykov

I did look more into this but IMO https://github.com/grafana/xk6-redis/pull/17/files#r1369707388 and it security implications are blockign this PR at this point.

I am okay if we can figure out a way to disable this functionality to merge it so that the PR doesn't stay open and continue working on the underlying problem in a separate PR. cc @oleiade

oleiade · 2023-10-25T08:25:58Z

@mstoykov I'm currently looking into this indeed 👀 There's quite a bit of context involved. If I remember correctly, we wanted to do that for a specific reason (because the k6 TLS dialer is specific to HTTP), but I need to spend more time loading the context back into my brain. I will come back to this as soon as I've been able to wrap my head around it again 👍🏻

mstoykov

LGMT! I would still prefer a test around blockhostnames or something like that, but looking at the code I do expect it will work.

My only other feedback is that the current options parsing has a bunch of high level issues that do not have easy fixes:

it exports a goja object instead of reading directly from it - which then requires a bunch of type casting and json marshalling and unmarshalling
The above also prevents you from supporting times as both numbers and strings - which is a thing k6 generally does see Unify duration configuration values k6#1738
Also because you do print the exported struct on error I guess you can hit stack overflow while processing a encrypt http request k6#644 (comment)

Fixes for all of those though in practice require that you do parse the config field by field from the base object which is arguably a thing we do in very few places.

So this is mostly a reminder this is not great then a let's fix it given the amount of options

redis/client_test.go

oleiade · 2023-11-02T10:35:19Z

@mstoykov I'll integrate your feedback in a(multiple?) issue(s) 🙇🏻

oleiade · 2023-11-02T16:19:35Z

Opened #26 and #27 as a result of your comment @mstoykov.

Regarding your last point, and the possibility of grafana/k6#644 (comment) being a thing, could you point me to a specific place you had in mind when mentioning "you do print the exported struct on error" please? 🙇🏻

mkosta · 2024-03-27T08:49:05Z

Guys, sorry dont know where to report properly, but i noticed an issue in new redis client Options (k6 v50)
using smth like this

    executor: 'ramping-arrival-rate',
    exec: "measureRedisPerformance",
    startRate:  10000,
    timeUnit: '10m',
    preAllocatedVUs: 1350,
    stages: [
      { target: 10000, duration: '1m' },
      { target: 10000, duration: '60m' },
    ],

connection string
const redisClient = new redis.Client({socket:{host:"localhost",port:6379,minIdleConns:1500,maxConnAge:10000,poolSize:20}});

if i provide any value in minIdleConns (default value not mentioned in Docs)

connections are consumed non stop, and in a split second like (ok minute) are reaching maxClients =10k

however if minIdleConns not specified, connected_clients reaches around 1353 (looks like around preallocated vus) and is properly reused, not going all the way to maxClients=10k

it seems this paramater if provided causes this mishandling of connections
thanks

codebien · 2024-03-27T09:20:48Z

Hey @mkosta,
can you please open an issue reporting it?

mkosta · 2024-03-27T09:41:29Z

@codebien
done
#30
thanks

imiric requested a review from oleiade July 7, 2023 16:00

imiric marked this pull request as draft July 7, 2023 16:01

imiric commented Jul 7, 2023

View reviewed changes

redis/options.go Outdated Show resolved Hide resolved

oleiade force-pushed the feat/13-tls-refactor branch from b0aa982 to 8c355a4 Compare August 2, 2023 07:57

imiric force-pushed the feat/13-tls-refactor branch 2 times, most recently from 2009287 to 2119bc6 Compare August 10, 2023 15:06

imiric changed the title ~~WIP Refactor Client constructor options, add TLS support~~ Refactor Client constructor options, add (m)TLS support Aug 10, 2023

imiric marked this pull request as ready for review August 10, 2023 15:16

imiric added the breaking change label Aug 10, 2023

imiric linked an issue Aug 10, 2023 that may be closed by this pull request

Failed to connect redis with tls #13

Closed

imiric force-pushed the feat/13-tls-refactor branch from 2119bc6 to 6e17cd7 Compare August 10, 2023 15:29

imiric commented Aug 10, 2023

View reviewed changes

mstoykov mentioned this pull request Aug 28, 2023

SSL connection option for Redis Client module #24

Closed

oleiade force-pushed the feat/13-tls-refactor branch from 6e17cd7 to e6404f1 Compare October 12, 2023 08:01

imiric requested a review from a team as a code owner October 12, 2023 08:01

imiric requested review from mstoykov and removed request for a team October 12, 2023 08:01

imiric mentioned this pull request Oct 12, 2023

Add Usage documentation grafana/k6-benchmarks#28

Merged

imiric added 2 commits October 17, 2023 09:39

Allow specifying URL in Client constructor

7a59cd5

Refactor Client constructor options

ea67f9c

To allow passing objects similar to node-redis. See #13 (comment)

oleiade requested a review from codebien October 19, 2023 12:19

codebien approved these changes Oct 19, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

redis/client.go Outdated Show resolved Hide resolved

oleiade and others added 2 commits October 20, 2023 14:04

Update README.md

1c81032

Co-authored-by: Ivan <2103732+codebien@users.noreply.github.com>

Update redis/client.go

f2da65a

Co-authored-by: Ivan <2103732+codebien@users.noreply.github.com>

oleiade approved these changes Oct 20, 2023

View reviewed changes

mstoykov reviewed Oct 24, 2023

View reviewed changes

mstoykov requested changes Oct 25, 2023

View reviewed changes

Fix example's test certificate paths

d542b1f

oleiade force-pushed the feat/13-tls-refactor branch from 3676bef to 06183eb Compare November 1, 2023 15:05

mstoykov approved these changes Nov 2, 2023

View reviewed changes

oleiade force-pushed the feat/13-tls-refactor branch from 06183eb to efd0a47 Compare November 2, 2023 09:39

mstoykov reviewed Nov 2, 2023

View reviewed changes

redis/client_test.go Outdated Show resolved Hide resolved

Ensure the redis TLS connection uses k6's netext.Dialer under the hood

9bb7ed0

oleiade force-pushed the feat/13-tls-refactor branch from efd0a47 to 9bb7ed0 Compare November 2, 2023 10:03

mstoykov approved these changes Nov 2, 2023

View reviewed changes

oleiade merged commit b5c72ea into master Nov 2, 2023
10 checks passed

oleiade deleted the feat/13-tls-refactor branch November 2, 2023 10:34

This was referenced Nov 2, 2023

Fix and document connecting to redis over TLS #14

Closed

Support durations as both number and string, like k6 does #26

Open

Handle options as goja.Object directly as opposed to exported map[string]interface{} #27

Open

oleiade mentioned this pull request Nov 3, 2023

Update xk6-redis dependency to v0.2.0 grafana/k6#3439

Merged

oleiade mentioned this pull request Nov 15, 2023

Amend redis client constructor type definition grafana/k6-DefinitelyTyped#20

Merged

8 tasks

Porkepix mentioned this pull request Dec 6, 2023

k6 0.48.0 Homebrew/homebrew-core#156571

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Client constructor options, add (m)TLS support #17

Refactor Client constructor options, add (m)TLS support #17

imiric commented Jul 7, 2023 •

edited

Loading

imiric commented Aug 10, 2023 •

edited

Loading

imiric Aug 10, 2023

CLAassistant commented Oct 9, 2023 •

edited

Loading

oleiade commented Oct 12, 2023 •

edited

Loading

imiric commented Oct 12, 2023

oleiade commented Oct 12, 2023

codebien commented Oct 13, 2023 •

edited

Loading

oleiade commented Oct 16, 2023 •

edited

Loading

codebien commented Oct 16, 2023

imiric commented Oct 16, 2023

oleiade commented Oct 17, 2023

codebien left a comment

oleiade left a comment

oleiade commented Oct 20, 2023

mstoykov Oct 24, 2023

oleiade Nov 1, 2023

mstoykov left a comment

oleiade commented Oct 25, 2023

mstoykov left a comment

oleiade commented Nov 2, 2023

oleiade commented Nov 2, 2023

mkosta commented Mar 27, 2024 •

edited

Loading

codebien commented Mar 27, 2024

mkosta commented Mar 27, 2024

Refactor Client constructor options, add (m)TLS support #17

Refactor Client constructor options, add (m)TLS support #17

Conversation

imiric commented Jul 7, 2023 • edited Loading

imiric commented Aug 10, 2023 • edited Loading

imiric Aug 10, 2023

Choose a reason for hiding this comment

CLAassistant commented Oct 9, 2023 • edited Loading

oleiade commented Oct 12, 2023 • edited Loading

imiric commented Oct 12, 2023

oleiade commented Oct 12, 2023

codebien commented Oct 13, 2023 • edited Loading

oleiade commented Oct 16, 2023 • edited Loading

codebien commented Oct 16, 2023

imiric commented Oct 16, 2023

oleiade commented Oct 17, 2023

codebien left a comment

Choose a reason for hiding this comment

oleiade left a comment

Choose a reason for hiding this comment

oleiade commented Oct 20, 2023

mstoykov Oct 24, 2023

Choose a reason for hiding this comment

oleiade Nov 1, 2023

Choose a reason for hiding this comment

mstoykov left a comment

Choose a reason for hiding this comment

oleiade commented Oct 25, 2023

mstoykov left a comment

Choose a reason for hiding this comment

oleiade commented Nov 2, 2023

oleiade commented Nov 2, 2023

mkosta commented Mar 27, 2024 • edited Loading

codebien commented Mar 27, 2024

mkosta commented Mar 27, 2024

imiric commented Jul 7, 2023 •

edited

Loading

imiric commented Aug 10, 2023 •

edited

Loading

CLAassistant commented Oct 9, 2023 •

edited

Loading

oleiade commented Oct 12, 2023 •

edited

Loading

codebien commented Oct 13, 2023 •

edited

Loading

oleiade commented Oct 16, 2023 •

edited

Loading

mkosta commented Mar 27, 2024 •

edited

Loading