You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by "rueian (via GitHub)" <gi...@apache.org> on 2023/04/28 16:49:40 UTC
[GitHub] [incubator-kvrocks] rueian opened a new pull request, #1406: Allow default username in the AUTH part of HELLO, Fix case sensitive check with HELLO options
rueian opened a new pull request, #1406:
URL: https://github.com/apache/incubator-kvrocks/pull/1406
Hi, following the conversation https://github.com/apache/incubator-kvrocks/issues/1403
This PR makes the `HELLO` command:
1. accept the `default` username as well.
2. use case insensitive check on the `AUTH` and `SETNAME`. Clients may send these options in lower or upper cases. For example, go-redis sends them in lower cases.
I think this PR won't break any existing Kvrocks clients but will remove barriers for other clients to adapt Kvrocks by aligning HELLO syntax with Redis.
Please let me know if these changes make sense.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tisonkun commented on a diff in pull request #1406: Allow default username in the AUTH part of HELLO, Fix case sensitive check with HELLO options
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #1406:
URL: https://github.com/apache/incubator-kvrocks/pull/1406#discussion_r1180894027
##########
src/commands/cmd_server.cc:
##########
@@ -657,7 +657,7 @@ class CommandTime : public Commander {
}
};
-/* HELLO [<protocol-version> [AUTH <password>] [SETNAME <name>] ] */
+/* HELLO [<protocol-version> [AUTH [<password>|<username> <password>]] [SETNAME <name>] ] */
Review Comment:
Perhaps add a comment that `username` should be always `default` otherwise it fails. And such syntax is for compatible only.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] mapleFU commented on pull request #1406: Allow default username and fix case sensitive check in HELLO
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #1406:
URL: https://github.com/apache/incubator-kvrocks/pull/1406#issuecomment-1528693139
Seems that only "hello" support authenticate with user, should https://redis.io/commands/auth/ support that?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #1406: Allow default username and fix case sensitive check in HELLO
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on PR #1406:
URL: https://github.com/apache/incubator-kvrocks/pull/1406#issuecomment-1528679022
Merging. Thanks for your contribution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] rueian commented on a diff in pull request #1406: Allow default username in the AUTH part of HELLO, Fix case sensitive check with HELLO options
Posted by "rueian (via GitHub)" <gi...@apache.org>.
rueian commented on code in PR #1406:
URL: https://github.com/apache/incubator-kvrocks/pull/1406#discussion_r1180899694
##########
src/commands/cmd_server.cc:
##########
@@ -657,7 +657,7 @@ class CommandTime : public Commander {
}
};
-/* HELLO [<protocol-version> [AUTH <password>] [SETNAME <name>] ] */
+/* HELLO [<protocol-version> [AUTH [<password>|<username> <password>]] [SETNAME <name>] ] */
Review Comment:
Sure!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #1406: Allow default username and fix case sensitive check in HELLO
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on PR #1406:
URL: https://github.com/apache/incubator-kvrocks/pull/1406#issuecomment-1528717713
> Seems that only "hello" support authenticate with user, should https://redis.io/commands/auth/ support that?
I think it is not urgent as HELLO, since HELLO in redis just does not allow the username to be omitted, but `AUTH password` is valid in redis.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] mapleFU commented on pull request #1406: Allow default username and fix case sensitive check in HELLO
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #1406:
URL: https://github.com/apache/incubator-kvrocks/pull/1406#issuecomment-1528718104
Ok, reasonable. LGTM!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice merged pull request #1406: Allow default username and fix case sensitive check in HELLO
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice merged PR #1406:
URL: https://github.com/apache/incubator-kvrocks/pull/1406
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org