Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support connection schemes valkey:// and valkeys:// #199

Merged
merged 5 commits into from
Apr 23, 2024
Merged

Support connection schemes valkey:// and valkeys:// #199

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
244df8e
Rename connection scheme from redis:// to valkey://.
lipzhu Apr 4, 2024
63a27aa
Support redis:// and rediss:// scheme for compatibility.
lipzhu Apr 4, 2024
0864507
Add unit tests for valid connection scheme.
lipzhu Apr 4, 2024
d789b5b
Fix typo.
lipzhu Apr 4, 2024
9a09228
Resolve conflicts with unstable.
lipzhu Apr 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 17 additions & 4 deletions src/cli_common.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,7 @@ static sds percentDecode(const char *pe, size_t len) {
/* Parse a URI and extract the server connection information.
* URI scheme is based on the provisional specification[1] excluding support
* for query parameters. Valid URIs are:
* scheme: "redis://"
* scheme: "valkey://"
* authority: [[<username> ":"] <password> "@"] [<hostname> [":" <port>]]
* path: ["/" [<db>]]
*
Expand All @@ -318,8 +318,11 @@ void parseRedisUri(const char *uri, const char* tool_name, cliConnInfo *connInfo
UNUSED(tls_flag);
#endif

const char *scheme = "redis://";
const char *tlsscheme = "rediss://";
const char *scheme = "valkey://";
const char *tlsscheme = "valkeys://";
lipzhu marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a concerns about this. With Redis, using rediss was clearer to me that it was SSL because there is obviously a second s. With Valkey, I'm concerned valkeys will be confusing since it's not clear. I want us to least thing about it a bit more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@madolson think about it for a while then. :)

Adding "s" is the most common style for URI schemes, so I think valkeys is the right choice. Here are some from IANA's list:

aaa 		Diameter Protocol
aaas 		Diameter Protocol with Secure Transport
http 		Hypertext Transfer Protocol
https 		Hypertext Transfer Protocol Secure
msrp 		Message Session Relay Protocol
msrps 		Message Session Relay Protocol Secure
rtsp 		Real-Time Streaming Protocol (RTSP)
rtsps 		Real-Time Streaming Protocol (RTSP) over TLS
sip 		session initiation protocol
sips 		secure session initiation protocol

Copy link
Member

@madolson madolson Apr 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I thought about it and will retract my concern :) I still don't really like it, but I guess it's fine.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess now would be the time to rethink the URI scheme if we wanted to do that though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if this scheme is widely used. It's a client feature anyway.

We can postpone it indefinitely and only support the redis scheme. Do you prefer that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can consider resp as the scheme. RESP is the protocol after all. :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like resp more. In fact, I think "redis" was a minomer to begin with. It should've been a protocol name.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the redis scheme contains more than what's specified in RESP.

An example URL redis://user:password@host:port/dbnum contains information for the AUTH command and the SELECT command, to be called after connecting. It doesn't contain the RESP version though (for HELLO). It's a bit ad-hoc all of it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could have included various more things which are useful to do when connecting, like CLIENT SETNAME, CLIENT SETINFO LIBNAME, CLIENT SETINFO LIBVER, CLIENT NO-EVICT.

For example we could support them in a query string, such as

?name=banana&libname=valkey-py&libver=3.14.159&no-evict=1

The RESP version (HELLO N) very much affects what comes out from each command, so it's basically a different protocol. We could embed that in the scheme as resp2: vs resp3:.

.... or we can just keep the redis schemes just for backward compatibility. I'm not sure we need a scheme anyway, do we?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as valkey-cli can accept "redis/rediss", I am fine with other options, "valkey" or "resp2/3".

@zuiderkwast, you have a good point that the scheme implies more than just the wire protocol but also the specific client behavior so this is more of a call on the client side.

/* We need to support redis:// and rediss:// too for compatibility. */
const char *redisScheme = "redis://";
const char *redisTlsscheme = "rediss://";
const char *curr = uri;
const char *end = uri + strlen(uri);
const char *userinfo, *username, *port, *host, *path;
Expand All @@ -330,11 +333,21 @@ void parseRedisUri(const char *uri, const char* tool_name, cliConnInfo *connInfo
*tls_flag = 1;
curr += strlen(tlsscheme);
#else
fprintf(stderr,"rediss:// is only supported when %s is compiled with OpenSSL\n", tool_name);
fprintf(stderr,"valkeys:// and rediss:// are only supported when %s is compiled with OpenSSL\n", tool_name);
exit(1);
#endif
} else if (!strncasecmp(redisTlsscheme, curr, strlen(redisTlsscheme))) {
#ifdef USE_OPENSSL
*tls_flag = 1;
curr += strlen(redisTlsscheme);
#else
fprintf(stderr,"valkeys:// and rediss:// are only supported when %s is compiled with OpenSSL\n", tool_name);
exit(1);
#endif
} else if (!strncasecmp(scheme, curr, strlen(scheme))) {
curr += strlen(scheme);
} else if (!strncasecmp(redisScheme, curr, strlen(redisScheme))) {
curr += strlen(redisScheme);
} else {
fprintf(stderr,"Invalid URI scheme\n");
exit(1);
Expand Down
4 changes: 2 additions & 2 deletions src/valkey-benchmark.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1592,10 +1592,10 @@ int parseOptions(int argc, char **argv) {
" -s <socket> Server socket (overrides host and port)\n"
" -a <password> Password for Valkey Auth\n"
" --user <username> Used to send ACL style 'AUTH username pass'. Needs -a.\n"
" -u <uri> Server URI on format redis://user:password@host:port/dbnum\n"
" -u <uri> Server URI on format valkey://user:password@host:port/dbnum\n"
" User, password and dbnum are optional. For authentication\n"
" without a username, use username 'default'. For TLS, use\n"
" the scheme 'rediss'.\n"
" the scheme 'valkeys'.\n"
" -c <clients> Number of parallel connections (default 50).\n"
" Note: If --cluster is used then number of clients has to be\n"
" the same or higher than the number of nodes.\n"
Expand Down
6 changes: 3 additions & 3 deletions src/valkey-cli.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3040,10 +3040,10 @@ static void usage(int err) {
" --askpass Force user to input password with mask from STDIN.\n"
" If this argument is used, '-a' and " REDIS_CLI_AUTH_ENV "\n"
" environment variable will be ignored.\n"
" -u <uri> Server URI on format redis://user:password@host:port/dbnum\n"
" -u <uri> Server URI on format valkey://user:password@host:port/dbnum\n"
" User, password and dbnum are optional. For authentication\n"
" without a username, use username 'default'. For TLS, use\n"
" the scheme 'rediss'.\n"
" the scheme 'valkeys'.\n"
" -r <repeat> Execute specified command N times.\n"
" -i <interval> When -r is used, waits <interval> seconds per command.\n"
" It is possible to specify sub-second times like -i 0.1.\n"
Expand Down Expand Up @@ -3130,7 +3130,7 @@ version,tls_usage);
" Use --cluster help to list all available cluster manager commands.\n"
"\n"
"Examples:\n"
" valkey-cli -u redis://default:PASSWORD@localhost:6379/0\n"
" valkey-cli -u valkey://default:PASSWORD@localhost:6379/0\n"
" cat /etc/passwd | valkey-cli -x set mypasswd\n"
" valkey-cli -D \"\" --raw dump key > key.dump && valkey-cli -X dump_tag restore key2 0 dump_tag replace < key.dump\n"
" valkey-cli -r 100 lpush mylist x\n"
Expand Down
22 changes: 22 additions & 0 deletions tests/integration/redis-cli.tcl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -606,4 +606,26 @@ if {!$::tls} { ;# fake_redis_node doesn't support TLS
assert_equal 3 [exec {*}$cmdline ZCARD new_zset]
assert_equal "a\n1\nb\n2\nc\n3" [exec {*}$cmdline ZRANGE new_zset 0 -1 WITHSCORES]
}

test "Valid Connection Scheme: redis://" {
set cmdline [valkeycliuri "redis://" [srv host] [srv port]]
exec {*}$cmdline PING
lipzhu marked this conversation as resolved.
Show resolved Hide resolved
}

test "Valid Connection Scheme: valkey://" {
set cmdline [valkeycliuri "valkey://" [srv host] [srv port]]
exec {*}$cmdline PING
}

if {$::tls} {
test "Valid Connection Scheme: rediss://" {
set cmdline [valkeycliuri "rediss://" [srv host] [srv port]]
exec {*}$cmdline PING
}

test "Valid Connection Scheme: valkeys://" {
set cmdline [valkeycliuri "valkeys://" [srv host] [srv port]]
exec {*}$cmdline PING
}
}
}
4 changes: 2 additions & 2 deletions tests/support/benchmark.tcl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,14 @@ proc redisbenchmark {host port {opts {}}} {
}

proc redisbenchmarkuri {host port {opts {}}} {
set cmd [list src/valkey-benchmark -u redis://$host:$port]
set cmd [list src/valkey-benchmark -u valkey://$host:$port]
lappend cmd {*}[redisbenchmark_tls_config "tests"]
lappend cmd {*}$opts
return $cmd
}

proc redisbenchmarkuriuserpass {host port user pass {opts {}}} {
set cmd [list src/valkey-benchmark -u redis://$user:$pass@$host:$port]
set cmd [list src/valkey-benchmark -u valkey://$user:$pass@$host:$port]
lappend cmd {*}[redisbenchmark_tls_config "tests"]
lappend cmd {*}$opts
return $cmd
Expand Down
7 changes: 7 additions & 0 deletions tests/support/cli.tcl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,13 @@ proc rediscli {host port {opts {}}} {
return $cmd
}

proc valkeycliuri {scheme host port {opts {}}} {
set cmd [list src/valkey-cli -u $scheme$host:$port]
lappend cmd {*}[rediscli_tls_config "tests"]
lappend cmd {*}$opts
return $cmd
}

# Returns command line for executing redis-cli with a unix socket address
proc rediscli_unixsocket {unixsocket {opts {}}} {
return [list src/valkey-cli -s $unixsocket {*}$opts]
Expand Down
Loading