You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by GitBox <gi...@apache.org> on 2022/09/16 19:16:22 UTC
[GitHub] [incubator-kvrocks] mapleFU opened a new pull request, #881: [WIP] Implement the command `hello`
mapleFU opened a new pull request, #881:
URL: https://github.com/apache/incubator-kvrocks/pull/881
This patch tries to fix this issue: https://github.com/apache/incubator-kvrocks/issues/753
It's a WIP because:
1. I don't know if I need to implement the logic about authentication.
2. I don't know how to write some returning code like "sentinel", "version"
Also, I update some implementions in `Redis::Array`, it will suffer from lots of unnecessary memory allocations
--
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] tanruixiang commented on a diff in pull request #881: Implement the command `hello`
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #881:
URL: https://github.com/apache/incubator-kvrocks/pull/881#discussion_r973596441
##########
src/redis_cmd.cc:
##########
@@ -73,29 +74,47 @@ const char *errUnbalacedStreamList =
const char *errTimeoutIsNegative = "timeout is negative";
const char *errLimitOptionNotAllowed = "syntax error, LIMIT cannot be used without the special ~ option";
+enum class AuthResult {
+ OK,
+ INVALID_PASSWORD,
+ NO_REQUIRE_PASS,
+};
+
+AuthResult AuthenticateUser(Connection *conn, Config* config, const std::string& user_password) {
+ auto iter = config->tokens.find(user_password);
+ if (iter != config->tokens.end()) {
+ conn->SetNamespace(iter->second);
+ conn->BecomeUser();
+ return AuthResult::OK;
+ }
+ const auto& requirepass = config->requirepass;
+ if (!requirepass.empty() && user_password != requirepass) {
+ return AuthResult::INVALID_PASSWORD;
+ }
+ conn->SetNamespace(kDefaultNamespace);
+ conn->BecomeAdmin();
+ if (requirepass.empty()) {
+ return AuthResult::NO_REQUIRE_PASS;
+ }
+ return AuthResult::OK;
+}
+
Review Comment:
Hi. May I ask why do we introduce global `enum class AuthResult` and `AuthenticateUser` function instead of putting them in classes or corresponding constant definition files? Thank you 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] git-hulk commented on a diff in pull request #881: [WIP] Implement the command `hello`
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #881:
URL: https://github.com/apache/incubator-kvrocks/pull/881#discussion_r973564342
##########
src/redis_cmd.cc:
##########
@@ -4132,6 +4151,78 @@ class CommandEcho : public Commander {
}
};
+/* HELLO [<protocol-version> [AUTH <password>] [SETNAME <name>] ] */
+class CommandHello final : public Commander {
+public:
+ Status Execute(Server *svr, Connection *conn, std::string *output) override {
+ size_t next_arg = 1;
+ if (args_.size() >= 2) {
+ int64_t protocol;
+ auto parseResult = ParseInt<int64_t>(args_[next_arg], /* base= */ 10);
+ ++next_arg;
+ if (!parseResult.IsOK()) {
+ *output = Redis::Error("Protocol version is not an integer or out of range");
+ return parseResult.ToStatus();
+ }
+ protocol = parseResult.GetValue();
+
+ // In redis, it will check protocol < 2 or protocol > 3,
+ // but kvrocks only supports REPL2 by now.
+ if (protocol != 2) {
+ *output = Redis::Error("-NOPROTO unsupported protocol version");
+ return Status::OK();
+ }
+ }
+
+ // Handling AUTH and SETNAME
+ for (; next_arg < args_.size(); ++next_arg) {
+ size_t moreargs = args_.size() - next_arg - 1;
+ const std::string& opt = args_[next_arg];
+ if (opt == "AUTH" && moreargs != 0) {
+ const auto& user_password = args_[next_arg + 1];
+ auto authResult = AuthenticateUser(conn, svr->GetConfig(), user_password);
+ switch (authResult) {
+ case AuthResult::INVALID_PASSWORD:
+ *output = Redis::Error("ERR invalid password");
+ break;
+ case AuthResult::NO_REQUIRE_PASS:
+ *output = Redis::Error("ERR Client sent AUTH, but no password is set");
+ break;
+ case AuthResult::OK:
+ break;
+ }
+ if (authResult != AuthResult::OK) {
+ return Status::OK();
+ }
+ next_arg += 1;
+ } else if (opt == "SETNAME" && moreargs != 0) {
+ const std::string& ns = args_[next_arg + 1];
+ conn->SetNamespace(ns);
Review Comment:
[Namespace](https://kvrocks.apache.org/docs/UserGuide/namespace) is used to separate the user data like multi-tenants and `SETNAME [client name]` is only for aliasing the connection name, then users can kill the connection by client name like `CLIENT KILL [client name]`. So it's different between SetName and SetNamespace and you also had switched the namespace in `AuthenticateUser`.
--
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] tanruixiang commented on a diff in pull request #881: Implement the command `hello`
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #881:
URL: https://github.com/apache/incubator-kvrocks/pull/881#discussion_r973599962
##########
src/redis_cmd.cc:
##########
@@ -73,29 +74,47 @@ const char *errUnbalacedStreamList =
const char *errTimeoutIsNegative = "timeout is negative";
const char *errLimitOptionNotAllowed = "syntax error, LIMIT cannot be used without the special ~ option";
+enum class AuthResult {
+ OK,
+ INVALID_PASSWORD,
+ NO_REQUIRE_PASS,
+};
+
+AuthResult AuthenticateUser(Connection *conn, Config* config, const std::string& user_password) {
+ auto iter = config->tokens.find(user_password);
+ if (iter != config->tokens.end()) {
+ conn->SetNamespace(iter->second);
+ conn->BecomeUser();
+ return AuthResult::OK;
+ }
+ const auto& requirepass = config->requirepass;
+ if (!requirepass.empty() && user_password != requirepass) {
+ return AuthResult::INVALID_PASSWORD;
+ }
+ conn->SetNamespace(kDefaultNamespace);
+ conn->BecomeAdmin();
+ if (requirepass.empty()) {
+ return AuthResult::NO_REQUIRE_PASS;
+ }
+ return AuthResult::OK;
+}
+
Review Comment:
I noticed that the helper function of the existed class in `redis_cmd.cc` is also written in the class. Hi @git-hulk @PragmaTwice, What do you think？
--
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] git-hulk commented on pull request #881: Implement the command `hello`
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #881:
URL: https://github.com/apache/incubator-kvrocks/pull/881#issuecomment-1250181501
> ```
> [TIMEOUT]: clients state report follows.
> ```
>
> maybe I should rerun the test?
Yes, this failure isn't caused by the current PR, let's rerun.
--
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 a diff in pull request #881: [WIP] Implement the command `hello`
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #881:
URL: https://github.com/apache/incubator-kvrocks/pull/881#discussion_r973547413
##########
src/redis_reply.cc:
##########
@@ -45,36 +45,43 @@ std::string MultiLen(int64_t len) {
return "*"+std::to_string(len)+"\r\n";
}
-std::string MultiBulkString(std::vector<std::string> values, bool output_nil_for_empty_string) {
+std::string MultiBulkString(const std::vector<std::string>& values, bool output_nil_for_empty_string) {
+ std::vector<std::string> copiedStrings;
+ copiedStrings.resize(values.size());
for (size_t i = 0; i < values.size(); i++) {
if (values[i].empty() && output_nil_for_empty_string) {
- values[i] = NilString();
+ copiedStrings[i] = NilString();
} else {
- values[i] = BulkString(values[i]);
+ copiedStrings[i] = BulkString(values[i]);
}
}
- return Array(values);
+ return Array(copiedStrings);
}
-std::string MultiBulkString(std::vector<std::string> values, const std::vector<rocksdb::Status> &statuses) {
+std::string MultiBulkString(const std::vector<std::string>& values, const std::vector<rocksdb::Status> &statuses) {
+ std::vector<std::string> copiedStrings;
+ copiedStrings.resize(values.size());
for (size_t i = 0; i < values.size(); i++) {
if (i < statuses.size() && !statuses[i].ok()) {
- values[i] = NilString();
+ copiedStrings[i] = NilString();
} else {
- values[i] = BulkString(values[i]);
+ copiedStrings[i] = BulkString(values[i]);
}
}
- return Array(values);
+ return Array(copiedStrings);
}
-std::string Array(std::vector<std::string> list) {
+
+std::string Array(const std::vector<std::string>& list) {
std::string::size_type n = std::accumulate(
list.begin(), list.end(), std::string::size_type(0),
[] ( std::string::size_type n, const std::string &s ) { return ( n += s.size() ); });
Review Comment:
```suggestion
size_t n = std::accumulate(
list.begin(), list.end(), 0, [] (size_t n, const std::string &s) { return n + s.size(); });
```
--
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] git-hulk commented on a diff in pull request #881: [WIP] Implement the command `hello`
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #881:
URL: https://github.com/apache/incubator-kvrocks/pull/881#discussion_r973573170
##########
src/redis_cmd.cc:
##########
@@ -4132,6 +4151,78 @@ class CommandEcho : public Commander {
}
};
+/* HELLO [<protocol-version> [AUTH <password>] [SETNAME <name>] ] */
+class CommandHello final : public Commander {
+public:
Review Comment:
```suggestion
public:
```
##########
src/redis_reply.cc:
##########
@@ -45,36 +45,39 @@ std::string MultiLen(int64_t len) {
return "*"+std::to_string(len)+"\r\n";
}
-std::string MultiBulkString(std::vector<std::string> values, bool output_nil_for_empty_string) {
+std::string MultiBulkString(const std::vector<std::string>& values, bool output_nil_for_empty_string) {
+ std::string result = "*" + std::to_string(values.size()) + CRLF;
for (size_t i = 0; i < values.size(); i++) {
if (values[i].empty() && output_nil_for_empty_string) {
- values[i] = NilString();
+ result += NilString();
} else {
- values[i] = BulkString(values[i]);
+ result += BulkString(values[i]);
}
}
- return Array(values);
+ return result;
}
-std::string MultiBulkString(std::vector<std::string> values, const std::vector<rocksdb::Status> &statuses) {
+std::string MultiBulkString(const std::vector<std::string>& values, const std::vector<rocksdb::Status> &statuses) {
+ std::string result = "*" + std::to_string(values.size()) + CRLF;
for (size_t i = 0; i < values.size(); i++) {
if (i < statuses.size() && !statuses[i].ok()) {
- values[i] = NilString();
+ result += NilString();
} else {
- values[i] = BulkString(values[i]);
+ result += BulkString(values[i]);
}
}
- return Array(values);
+ return result;
}
-std::string Array(std::vector<std::string> list) {
- std::string::size_type n = std::accumulate(
- list.begin(), list.end(), std::string::size_type(0),
- [] ( std::string::size_type n, const std::string &s ) { return ( n += s.size() ); });
+
+std::string Array(const std::vector<std::string>& list) {
+ size_t n = std::accumulate(
+ list.begin(), list.end(), 0, [] (size_t n, const std::string &s) { return n + s.size(); });
std::string result = "*" + std::to_string(list.size()) + CRLF;
- result.reserve(n);
- return std::accumulate(list.begin(), list.end(), result,
- [](std::string &dest, std::string const &item) -> std::string& {dest += item; return dest;});
+ std::string::size_type final_size = result.size() + n;
+ result.reserve(final_size);
+ for(const auto& i : list) result += i;
Review Comment:
```suggestion
for (const auto& i : list) result += i;
```
--
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 a diff in pull request #881: Implement the command `hello`
Posted by GitBox <gi...@apache.org>.
mapleFU commented on code in PR #881:
URL: https://github.com/apache/incubator-kvrocks/pull/881#discussion_r973596707
##########
src/redis_cmd.cc:
##########
@@ -73,29 +74,47 @@ const char *errUnbalacedStreamList =
const char *errTimeoutIsNegative = "timeout is negative";
const char *errLimitOptionNotAllowed = "syntax error, LIMIT cannot be used without the special ~ option";
+enum class AuthResult {
+ OK,
+ INVALID_PASSWORD,
+ NO_REQUIRE_PASS,
+};
+
+AuthResult AuthenticateUser(Connection *conn, Config* config, const std::string& user_password) {
+ auto iter = config->tokens.find(user_password);
+ if (iter != config->tokens.end()) {
+ conn->SetNamespace(iter->second);
+ conn->BecomeUser();
+ return AuthResult::OK;
+ }
+ const auto& requirepass = config->requirepass;
+ if (!requirepass.empty() && user_password != requirepass) {
+ return AuthResult::INVALID_PASSWORD;
+ }
+ conn->SetNamespace(kDefaultNamespace);
+ conn->BecomeAdmin();
+ if (requirepass.empty()) {
+ return AuthResult::NO_REQUIRE_PASS;
+ }
+ return AuthResult::OK;
+}
+
Review Comment:
It's better to put `AuthenticateUser` and status to `Connection`, but here I just want to make it lightweight to implement it, because `auth` isn't widely used.
--
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 #881: [WIP] Implement the command `hello`
Posted by GitBox <gi...@apache.org>.
mapleFU commented on PR #881:
URL: https://github.com/apache/incubator-kvrocks/pull/881#issuecomment-1249718249
@git-hulk mind take a look? I need some help here
--
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 #881: Implement the command `hello`
Posted by GitBox <gi...@apache.org>.
mapleFU commented on PR #881:
URL: https://github.com/apache/incubator-kvrocks/pull/881#issuecomment-1250135392
```
[TIMEOUT]: clients state report follows.
```
maybe I should rerun the test?
--
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] git-hulk commented on pull request #881: [WIP] Implement the command `hello`
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #881:
URL: https://github.com/apache/incubator-kvrocks/pull/881#issuecomment-1250017088
> But I wonder how can I testing hello
Do you mean where to add the test case? if yes, can do it in [gocase/unit/auth](https://github.com/apache/incubator-kvrocks/tree/unstable/tests/gocase/unit/auth)
> would it be ambigious if setName is called when conn is an admin? And should we update the tokens here? And should I add some extra return values?
I didn't get this point. For SetName will only add an alias name for connection without other side effects. And no need to update any token/password here.
--
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 a diff in pull request #881: [WIP] Implement the command `hello`
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #881:
URL: https://github.com/apache/incubator-kvrocks/pull/881#discussion_r973547647
##########
src/redis_reply.cc:
##########
@@ -45,36 +45,43 @@ std::string MultiLen(int64_t len) {
return "*"+std::to_string(len)+"\r\n";
}
-std::string MultiBulkString(std::vector<std::string> values, bool output_nil_for_empty_string) {
+std::string MultiBulkString(const std::vector<std::string>& values, bool output_nil_for_empty_string) {
+ std::vector<std::string> copiedStrings;
+ copiedStrings.resize(values.size());
for (size_t i = 0; i < values.size(); i++) {
if (values[i].empty() && output_nil_for_empty_string) {
- values[i] = NilString();
+ copiedStrings[i] = NilString();
} else {
- values[i] = BulkString(values[i]);
+ copiedStrings[i] = BulkString(values[i]);
}
}
- return Array(values);
+ return Array(copiedStrings);
}
-std::string MultiBulkString(std::vector<std::string> values, const std::vector<rocksdb::Status> &statuses) {
+std::string MultiBulkString(const std::vector<std::string>& values, const std::vector<rocksdb::Status> &statuses) {
+ std::vector<std::string> copiedStrings;
+ copiedStrings.resize(values.size());
for (size_t i = 0; i < values.size(); i++) {
if (i < statuses.size() && !statuses[i].ok()) {
- values[i] = NilString();
+ copiedStrings[i] = NilString();
} else {
- values[i] = BulkString(values[i]);
+ copiedStrings[i] = BulkString(values[i]);
}
}
- return Array(values);
+ return Array(copiedStrings);
}
-std::string Array(std::vector<std::string> list) {
+
+std::string Array(const std::vector<std::string>& list) {
std::string::size_type n = std::accumulate(
list.begin(), list.end(), std::string::size_type(0),
[] ( std::string::size_type n, const std::string &s ) { return ( n += s.size() ); });
std::string result = "*" + std::to_string(list.size()) + CRLF;
- result.reserve(n);
- return std::accumulate(list.begin(), list.end(), result,
+ std::string::size_type final_size = result.size() + n;
+ result.reserve(final_size);
+ auto s = std::accumulate(list.begin(), list.end(), result,
[](std::string &dest, std::string const &item) -> std::string& {dest += item; return dest;});
+ return s;
Review Comment:
```suggestion
for(const auto& i : list) result += i;
return result;
```
--
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] git-hulk commented on a diff in pull request #881: [WIP] Implement the command `hello`
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #881:
URL: https://github.com/apache/incubator-kvrocks/pull/881#discussion_r973550874
##########
src/redis_cmd.cc:
##########
@@ -4132,6 +4151,78 @@ class CommandEcho : public Commander {
}
};
+/* HELLO [<protocol-version> [AUTH <password>] [SETNAME <name>] ] */
+class CommandHello final : public Commander {
+public:
+ Status Execute(Server *svr, Connection *conn, std::string *output) override {
+ size_t next_arg = 1;
+ if (args_.size() >= 2) {
+ int64_t protocol;
+ auto parseResult = ParseInt<int64_t>(args_[next_arg], /* base= */ 10);
+ ++next_arg;
+ if (!parseResult.IsOK()) {
+ *output = Redis::Error("Protocol version is not an integer or out of range");
+ return parseResult.ToStatus();
+ }
+ protocol = parseResult.GetValue();
+
+ // In redis, it will check protocol < 2 or protocol > 3,
+ // but kvrocks only supports REPL2 by now.
+ if (protocol != 2) {
+ *output = Redis::Error("-NOPROTO unsupported protocol version");
+ return Status::OK();
+ }
+ }
+
+ // Handling AUTH and SETNAME
+ for (; next_arg < args_.size(); ++next_arg) {
+ size_t moreargs = args_.size() - next_arg - 1;
+ const std::string& opt = args_[next_arg];
+ if (opt == "AUTH" && moreargs != 0) {
+ const auto& user_password = args_[next_arg + 1];
+ auto authResult = AuthenticateUser(conn, svr->GetConfig(), user_password);
+ switch (authResult) {
+ case AuthResult::INVALID_PASSWORD:
+ *output = Redis::Error("ERR invalid password");
+ break;
+ case AuthResult::NO_REQUIRE_PASS:
+ *output = Redis::Error("ERR Client sent AUTH, but no password is set");
+ break;
+ case AuthResult::OK:
+ break;
+ }
+ if (authResult != AuthResult::OK) {
+ return Status::OK();
+ }
+ next_arg += 1;
+ } else if (opt == "SETNAME" && moreargs != 0) {
+ const std::string& ns = args_[next_arg + 1];
+ conn->SetNamespace(ns);
Review Comment:
```suggestion
conn->SetName(args_[next_arg + 1]);
```
--
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] tanruixiang commented on a diff in pull request #881: Implement the command `hello`
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #881:
URL: https://github.com/apache/incubator-kvrocks/pull/881#discussion_r973597658
##########
src/redis_cmd.cc:
##########
@@ -73,29 +74,47 @@ const char *errUnbalacedStreamList =
const char *errTimeoutIsNegative = "timeout is negative";
const char *errLimitOptionNotAllowed = "syntax error, LIMIT cannot be used without the special ~ option";
+enum class AuthResult {
+ OK,
+ INVALID_PASSWORD,
+ NO_REQUIRE_PASS,
+};
+
+AuthResult AuthenticateUser(Connection *conn, Config* config, const std::string& user_password) {
+ auto iter = config->tokens.find(user_password);
+ if (iter != config->tokens.end()) {
+ conn->SetNamespace(iter->second);
+ conn->BecomeUser();
+ return AuthResult::OK;
+ }
+ const auto& requirepass = config->requirepass;
+ if (!requirepass.empty() && user_password != requirepass) {
+ return AuthResult::INVALID_PASSWORD;
+ }
+ conn->SetNamespace(kDefaultNamespace);
+ conn->BecomeAdmin();
+ if (requirepass.empty()) {
+ return AuthResult::NO_REQUIRE_PASS;
+ }
+ return AuthResult::OK;
+}
+
Review Comment:
I think we can at least put them in `class CommandAuth`.
--
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] git-hulk commented on a diff in pull request #881: Implement the command `hello`
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #881:
URL: https://github.com/apache/incubator-kvrocks/pull/881#discussion_r973652974
##########
src/redis_cmd.cc:
##########
@@ -73,29 +74,47 @@ const char *errUnbalacedStreamList =
const char *errTimeoutIsNegative = "timeout is negative";
const char *errLimitOptionNotAllowed = "syntax error, LIMIT cannot be used without the special ~ option";
+enum class AuthResult {
+ OK,
+ INVALID_PASSWORD,
+ NO_REQUIRE_PASS,
+};
+
+AuthResult AuthenticateUser(Connection *conn, Config* config, const std::string& user_password) {
+ auto iter = config->tokens.find(user_password);
+ if (iter != config->tokens.end()) {
+ conn->SetNamespace(iter->second);
+ conn->BecomeUser();
+ return AuthResult::OK;
+ }
+ const auto& requirepass = config->requirepass;
+ if (!requirepass.empty() && user_password != requirepass) {
+ return AuthResult::INVALID_PASSWORD;
+ }
+ conn->SetNamespace(kDefaultNamespace);
+ conn->BecomeAdmin();
+ if (requirepass.empty()) {
+ return AuthResult::NO_REQUIRE_PASS;
+ }
+ return AuthResult::OK;
+}
+
Review Comment:
Both of them are ok to me
--
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] git-hulk commented on pull request #881: Implement the command `hello`
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #881:
URL: https://github.com/apache/incubator-kvrocks/pull/881#issuecomment-1250231925
Thanks all, will merge this PR after the CI becomes green.
--
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] git-hulk commented on a diff in pull request #881: [WIP] Implement the command `hello`
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #881:
URL: https://github.com/apache/incubator-kvrocks/pull/881#discussion_r973531504
##########
src/redis_cmd.cc:
##########
@@ -4132,6 +4132,52 @@ class CommandEcho : public Commander {
}
};
+class CommandHello final : public Commander {
+public:
+ Status Execute(Server *svr, Connection *conn, std::string *output) override {
+ std::cout << "Execute is called" << std::endl;
+ if (args_.size() == 2) {
+ int64_t protocol;
+ try {
+ protocol = std::stoll(args_[1]);
+ } catch (std::exception& e) {
+ // std::invalid_argument or std::out_of_range
+ *output = Redis::Error("Protocol version is not an integer or out of range");
+ return Status::OK();
+ }
+
+ // In redis, it will check protocol < 2 or protocol > 3,
+ // but kvrocks only supports REPL2 by now.
+ if (protocol != 2) {
+ *output = Redis::Error("-NOPROTO unsupported protocol version");
+ return Status::OK();
+ }
+ }
+
+ // TODO(mapleFU): do we need to implement auth and setname?
Review Comment:
Yes, setname can use [Connection::SetName](https://github.com/apache/incubator-kvrocks/blob/40132067bc7bb696f96675e8a396f0473321a454/src/redis_connection.h#L80)
--
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 a diff in pull request #881: [WIP] Implement the command `hello`
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #881:
URL: https://github.com/apache/incubator-kvrocks/pull/881#discussion_r973563341
##########
src/redis_cmd.cc:
##########
@@ -4132,6 +4151,78 @@ class CommandEcho : public Commander {
}
};
+/* HELLO [<protocol-version> [AUTH <password>] [SETNAME <name>] ] */
+class CommandHello final : public Commander {
+public:
+ Status Execute(Server *svr, Connection *conn, std::string *output) override {
+ size_t next_arg = 1;
+ if (args_.size() >= 2) {
+ int64_t protocol;
+ auto parseResult = ParseInt<int64_t>(args_[next_arg], /* base= */ 10);
+ ++next_arg;
+ if (!parseResult.IsOK()) {
+ *output = Redis::Error("Protocol version is not an integer or out of range");
+ return parseResult.ToStatus();
+ }
+ protocol = parseResult.GetValue();
+
+ // In redis, it will check protocol < 2 or protocol > 3,
+ // but kvrocks only supports REPL2 by now.
+ if (protocol != 2) {
+ *output = Redis::Error("-NOPROTO unsupported protocol version");
+ return Status::OK();
+ }
+ }
+
+ // Handling AUTH and SETNAME
+ for (; next_arg < args_.size(); ++next_arg) {
+ size_t moreargs = args_.size() - next_arg - 1;
+ const std::string& opt = args_[next_arg];
+ if (opt == "AUTH" && moreargs != 0) {
+ const auto& user_password = args_[next_arg + 1];
+ auto authResult = AuthenticateUser(conn, svr->GetConfig(), user_password);
+ switch (authResult) {
+ case AuthResult::INVALID_PASSWORD:
+ *output = Redis::Error("ERR invalid password");
+ break;
+ case AuthResult::NO_REQUIRE_PASS:
+ *output = Redis::Error("ERR Client sent AUTH, but no password is set");
+ break;
+ case AuthResult::OK:
+ break;
+ }
+ if (authResult != AuthResult::OK) {
+ return Status::OK();
+ }
Review Comment:
```suggestion
switch (authResult) {
case AuthResult::INVALID_PASSWORD:
return Status(Status::NotOK, "invalid password");
case AuthResult::NO_REQUIRE_PASS:
return Status(Status::NotOK, "Client sent AUTH, but no password is set");
case AuthResult::OK:;
}
```
--
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 a diff in pull request #881: Implement the command `hello`
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #881:
URL: https://github.com/apache/incubator-kvrocks/pull/881#discussion_r973600563
##########
src/redis_cmd.cc:
##########
@@ -73,29 +74,47 @@ const char *errUnbalacedStreamList =
const char *errTimeoutIsNegative = "timeout is negative";
const char *errLimitOptionNotAllowed = "syntax error, LIMIT cannot be used without the special ~ option";
+enum class AuthResult {
+ OK,
+ INVALID_PASSWORD,
+ NO_REQUIRE_PASS,
+};
+
+AuthResult AuthenticateUser(Connection *conn, Config* config, const std::string& user_password) {
+ auto iter = config->tokens.find(user_password);
+ if (iter != config->tokens.end()) {
+ conn->SetNamespace(iter->second);
+ conn->BecomeUser();
+ return AuthResult::OK;
+ }
+ const auto& requirepass = config->requirepass;
+ if (!requirepass.empty() && user_password != requirepass) {
+ return AuthResult::INVALID_PASSWORD;
+ }
+ conn->SetNamespace(kDefaultNamespace);
+ conn->BecomeAdmin();
+ if (requirepass.empty()) {
+ return AuthResult::NO_REQUIRE_PASS;
+ }
+ return AuthResult::OK;
+}
+
Review Comment:
I think it is OK to me. Actually I think it can be `Status AuthenticateUser(Connection *conn, Config* config, const std::string& user_password)`, but current form also looks good to me.
--
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 #881: Implement the command `hello`
Posted by GitBox <gi...@apache.org>.
mapleFU commented on PR #881:
URL: https://github.com/apache/incubator-kvrocks/pull/881#issuecomment-1250114325
Oh, finally I find out the reason. `go-redis` will send `hello 3` to kvrocks, and it will be filtered by auth checking...
--
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 #881: Implement the command `hello`
Posted by GitBox <gi...@apache.org>.
mapleFU commented on PR #881:
URL: https://github.com/apache/incubator-kvrocks/pull/881#issuecomment-1250109730
It will be no problem when i change `ADD_CMD("hello" ...` to `ADD_CMD("HELLO", ...`, which really makes me mad...
--
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 a diff in pull request #881: [WIP] Implement the command `hello`
Posted by GitBox <gi...@apache.org>.
mapleFU commented on code in PR #881:
URL: https://github.com/apache/incubator-kvrocks/pull/881#discussion_r973566481
##########
src/redis_cmd.cc:
##########
@@ -4132,6 +4151,78 @@ class CommandEcho : public Commander {
}
};
+/* HELLO [<protocol-version> [AUTH <password>] [SETNAME <name>] ] */
+class CommandHello final : public Commander {
+public:
+ Status Execute(Server *svr, Connection *conn, std::string *output) override {
+ size_t next_arg = 1;
+ if (args_.size() >= 2) {
+ int64_t protocol;
+ auto parseResult = ParseInt<int64_t>(args_[next_arg], /* base= */ 10);
+ ++next_arg;
+ if (!parseResult.IsOK()) {
+ *output = Redis::Error("Protocol version is not an integer or out of range");
+ return parseResult.ToStatus();
+ }
+ protocol = parseResult.GetValue();
+
+ // In redis, it will check protocol < 2 or protocol > 3,
+ // but kvrocks only supports REPL2 by now.
+ if (protocol != 2) {
+ *output = Redis::Error("-NOPROTO unsupported protocol version");
+ return Status::OK();
+ }
+ }
+
+ // Handling AUTH and SETNAME
+ for (; next_arg < args_.size(); ++next_arg) {
+ size_t moreargs = args_.size() - next_arg - 1;
+ const std::string& opt = args_[next_arg];
+ if (opt == "AUTH" && moreargs != 0) {
+ const auto& user_password = args_[next_arg + 1];
+ auto authResult = AuthenticateUser(conn, svr->GetConfig(), user_password);
+ switch (authResult) {
+ case AuthResult::INVALID_PASSWORD:
+ *output = Redis::Error("ERR invalid password");
+ break;
+ case AuthResult::NO_REQUIRE_PASS:
+ *output = Redis::Error("ERR Client sent AUTH, but no password is set");
+ break;
+ case AuthResult::OK:
+ break;
+ }
+ if (authResult != AuthResult::OK) {
+ return Status::OK();
+ }
+ next_arg += 1;
+ } else if (opt == "SETNAME" && moreargs != 0) {
+ const std::string& ns = args_[next_arg + 1];
+ conn->SetNamespace(ns);
Review Comment:
Oh, I mix these two functions, that's my fault.
--
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] git-hulk commented on a diff in pull request #881: [WIP] Implement the command `hello`
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #881:
URL: https://github.com/apache/incubator-kvrocks/pull/881#discussion_r973573041
##########
src/redis_cmd.cc:
##########
@@ -4132,6 +4151,78 @@ class CommandEcho : public Commander {
}
};
+/* HELLO [<protocol-version> [AUTH <password>] [SETNAME <name>] ] */
+class CommandHello final : public Commander {
+public:
+ Status Execute(Server *svr, Connection *conn, std::string *output) override {
+ size_t next_arg = 1;
+ if (args_.size() >= 2) {
+ int64_t protocol;
+ auto parseResult = ParseInt<int64_t>(args_[next_arg], /* base= */ 10);
+ ++next_arg;
+ if (!parseResult.IsOK()) {
+ *output = Redis::Error("Protocol version is not an integer or out of range");
+ return parseResult.ToStatus();
+ }
+ protocol = parseResult.GetValue();
+
+ // In redis, it will check protocol < 2 or protocol > 3,
+ // but kvrocks only supports REPL2 by now.
+ if (protocol != 2) {
+ *output = Redis::Error("-NOPROTO unsupported protocol version");
+ return Status::OK();
+ }
+ }
+
+ // Handling AUTH and SETNAME
+ for (; next_arg < args_.size(); ++next_arg) {
+ size_t moreargs = args_.size() - next_arg - 1;
+ const std::string& opt = args_[next_arg];
+ if (opt == "AUTH" && moreargs != 0) {
+ const auto& user_password = args_[next_arg + 1];
+ auto authResult = AuthenticateUser(conn, svr->GetConfig(), user_password);
+ switch (authResult) {
+ case AuthResult::INVALID_PASSWORD:
+ *output = Redis::Error("ERR invalid password");
+ break;
+ case AuthResult::NO_REQUIRE_PASS:
+ *output = Redis::Error("ERR Client sent AUTH, but no password is set");
+ break;
+ case AuthResult::OK:
+ break;
+ }
+ if (authResult != AuthResult::OK) {
+ return Status::OK();
+ }
+ next_arg += 1;
+ } else if (opt == "SETNAME" && moreargs != 0) {
+ const std::string& ns = args_[next_arg + 1];
+ conn->SetNamespace(ns);
Review Comment:
👍 It's also a bit confusing between those two functions indeed.
--
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] git-hulk commented on pull request #881: [WIP] Implement the command `hello`
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #881:
URL: https://github.com/apache/incubator-kvrocks/pull/881#issuecomment-1249985088
Hi, @mapleFU, many thanks for your contribution.
> I don't know if I need to implement the logic about authentication.
Yes, but there's a bit different in Kvrocks since it has NOT username, so we can always ignore the username. For the auth logic can see [redis_cmd.cc#L78](https://github.com/apache/incubator-kvrocks/blob/unstable/src/redis_cmd.cc#L78).
> I don't know how to write some returning code like "sentinel", "version"
Can ignore those sentinel fields since we are never running in sentinel mode.
--
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] git-hulk merged pull request #881: Implement the command `hello`
Posted by GitBox <gi...@apache.org>.
git-hulk merged PR #881:
URL: https://github.com/apache/incubator-kvrocks/pull/881
--
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] git-hulk commented on pull request #881: Implement the command `hello`
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #881:
URL: https://github.com/apache/incubator-kvrocks/pull/881#issuecomment-1250241013
Thanks @mapleFU again
--
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 #881: [WIP] Implement the command `hello`
Posted by GitBox <gi...@apache.org>.
mapleFU commented on PR #881:
URL: https://github.com/apache/incubator-kvrocks/pull/881#issuecomment-1250013372
@git-hulk Now I extract the setting logic from `CommandAuth`, and implement it in `HELLO`.
But I wonder how can I testing `hello`, and would it be ambigious if `setName` is called when `conn` is an admin?
--
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 a diff in pull request #881: [WIP] Implement the command `hello`
Posted by GitBox <gi...@apache.org>.
mapleFU commented on code in PR #881:
URL: https://github.com/apache/incubator-kvrocks/pull/881#discussion_r973560062
##########
src/redis_cmd.cc:
##########
@@ -4132,6 +4151,78 @@ class CommandEcho : public Commander {
}
};
+/* HELLO [<protocol-version> [AUTH <password>] [SETNAME <name>] ] */
+class CommandHello final : public Commander {
+public:
+ Status Execute(Server *svr, Connection *conn, std::string *output) override {
+ size_t next_arg = 1;
+ if (args_.size() >= 2) {
+ int64_t protocol;
+ auto parseResult = ParseInt<int64_t>(args_[next_arg], /* base= */ 10);
+ ++next_arg;
+ if (!parseResult.IsOK()) {
+ *output = Redis::Error("Protocol version is not an integer or out of range");
+ return parseResult.ToStatus();
+ }
+ protocol = parseResult.GetValue();
+
+ // In redis, it will check protocol < 2 or protocol > 3,
+ // but kvrocks only supports REPL2 by now.
+ if (protocol != 2) {
+ *output = Redis::Error("-NOPROTO unsupported protocol version");
+ return Status::OK();
+ }
+ }
+
+ // Handling AUTH and SETNAME
+ for (; next_arg < args_.size(); ++next_arg) {
+ size_t moreargs = args_.size() - next_arg - 1;
+ const std::string& opt = args_[next_arg];
+ if (opt == "AUTH" && moreargs != 0) {
+ const auto& user_password = args_[next_arg + 1];
+ auto authResult = AuthenticateUser(conn, svr->GetConfig(), user_password);
+ switch (authResult) {
+ case AuthResult::INVALID_PASSWORD:
+ *output = Redis::Error("ERR invalid password");
+ break;
+ case AuthResult::NO_REQUIRE_PASS:
+ *output = Redis::Error("ERR Client sent AUTH, but no password is set");
+ break;
+ case AuthResult::OK:
+ break;
+ }
+ if (authResult != AuthResult::OK) {
+ return Status::OK();
+ }
+ next_arg += 1;
+ } else if (opt == "SETNAME" && moreargs != 0) {
+ const std::string& ns = args_[next_arg + 1];
+ conn->SetNamespace(ns);
Review Comment:
I think
```
const std::string& ns = args_[next_arg + 1];
conn->SetNamespace(ns);
```
can represent that `args_[next_arg + 1]` means the `name` of connection, so I think just leave it here is ok.
--
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 a diff in pull request #881: Implement the command `hello`
Posted by GitBox <gi...@apache.org>.
mapleFU commented on code in PR #881:
URL: https://github.com/apache/incubator-kvrocks/pull/881#discussion_r973598708
##########
src/redis_cmd.cc:
##########
@@ -73,29 +74,47 @@ const char *errUnbalacedStreamList =
const char *errTimeoutIsNegative = "timeout is negative";
const char *errLimitOptionNotAllowed = "syntax error, LIMIT cannot be used without the special ~ option";
+enum class AuthResult {
+ OK,
+ INVALID_PASSWORD,
+ NO_REQUIRE_PASS,
+};
+
+AuthResult AuthenticateUser(Connection *conn, Config* config, const std::string& user_password) {
+ auto iter = config->tokens.find(user_password);
+ if (iter != config->tokens.end()) {
+ conn->SetNamespace(iter->second);
+ conn->BecomeUser();
+ return AuthResult::OK;
+ }
+ const auto& requirepass = config->requirepass;
+ if (!requirepass.empty() && user_password != requirepass) {
+ return AuthResult::INVALID_PASSWORD;
+ }
+ conn->SetNamespace(kDefaultNamespace);
+ conn->BecomeAdmin();
+ if (requirepass.empty()) {
+ return AuthResult::NO_REQUIRE_PASS;
+ }
+ return AuthResult::OK;
+}
+
Review Comment:
```
class CommandAuth : public Commandar {
public:
static void AuthenticateUser() ...
};
class CommandHello: public Commandar {
public:
Status Exec() {
CommandAuth::..
}
};
```
The code like that would be confusing, because `auth` is not a part of `Command`. And put it into client needs `Config` in server. So here I just implement it as helper function
--
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] tanruixiang commented on a diff in pull request #881: Implement the command `hello`
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #881:
URL: https://github.com/apache/incubator-kvrocks/pull/881#discussion_r973596223
##########
src/redis_cmd.cc:
##########
@@ -73,29 +74,47 @@ const char *errUnbalacedStreamList =
const char *errTimeoutIsNegative = "timeout is negative";
const char *errLimitOptionNotAllowed = "syntax error, LIMIT cannot be used without the special ~ option";
+enum class AuthResult {
+ OK,
+ INVALID_PASSWORD,
+ NO_REQUIRE_PASS,
+};
+
+AuthResult AuthenticateUser(Connection *conn, Config* config, const std::string& user_password) {
+ auto iter = config->tokens.find(user_password);
+ if (iter != config->tokens.end()) {
+ conn->SetNamespace(iter->second);
+ conn->BecomeUser();
+ return AuthResult::OK;
+ }
+ const auto& requirepass = config->requirepass;
+ if (!requirepass.empty() && user_password != requirepass) {
+ return AuthResult::INVALID_PASSWORD;
+ }
+ conn->SetNamespace(kDefaultNamespace);
+ conn->BecomeAdmin();
+ if (requirepass.empty()) {
+ return AuthResult::NO_REQUIRE_PASS;
+ }
+ return AuthResult::OK;
+}
+
class CommandAuth : public Commander {
Review Comment:
Hi. May I ask why do we introduce global `enum class AuthResult` and `AuthenticateUser` function instead of putting them in classes or corresponding constant definition files? Thank you 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] mapleFU commented on pull request #881: Implement the command `hello`
Posted by GitBox <gi...@apache.org>.
mapleFU commented on PR #881:
URL: https://github.com/apache/incubator-kvrocks/pull/881#issuecomment-1250108689
Hi, I'm trying to adding unit tests for `hello`, but here I come cross a problem:
```
ADD_CMD("hello", -1, "read-only ok-loading", 0, 0, 0, CommandHello),
```
After adding the `ADD_CMD` above, the `auth` test would failed:
```
=== RUN TestAuth
--- FAIL: TestAuth (2.01s)
=== RUN TestAuth/AUTH_fails_when_a_wrong_password_is_given
auth_test.go:56:
Error Trace: /Users/fuxuwei/workspace/CMakeLibs/incubator-kvrocks/tests/gocase/unit/auth/auth_test.go:56
Error: Error "NOAUTH Authentication required." does not contain "invalid password"
Test: TestAuth/AUTH_fails_when_a_wrong_password_is_given
--- FAIL: TestAuth/AUTH_fails_when_a_wrong_password_is_given (0.00s)
=== RUN TestAuth/Arbitrary_command_gives_an_error_when_AUTH_is_required
--- PASS: TestAuth/Arbitrary_command_gives_an_error_when_AUTH_is_required (0.00s)
=== RUN TestAuth/AUTH_succeeds_when_the_right_password_is_given
auth_test.go:66:
Error Trace: /Users/fuxuwei/workspace/CMakeLibs/incubator-kvrocks/tests/gocase/unit/auth/auth_test.go:66
Error: Not equal:
expected: string("OK")
actual : <nil>(<nil>)
Test: TestAuth/AUTH_succeeds_when_the_right_password_is_given
--- FAIL: TestAuth/AUTH_succeeds_when_the_right_password_is_given (0.00s)
Expected :string("OK")
Actual :<nil>(<nil>)
<Click to see difference>
```
Even leaving an empty implements in `CommandHello`, or not changing the logic of `CommandAuth`, the test would failed. I think maybe I miss some details in `redis_cmd`, do you have any ideas? @git-hulk
--
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 #881: Implement the command `hello`
Posted by GitBox <gi...@apache.org>.
mapleFU commented on PR #881:
URL: https://github.com/apache/incubator-kvrocks/pull/881#issuecomment-1250237381
@git-hulk it's able to be merged now （*＾3＾）
--
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