Syntax of HELLO is different from Redis

[rueian]

Search before asking

• I had searched in the issues and found no similar issues.

Version

2.3.0

Minimal reproduce step

1. Set requirepass mypass
2. Send command: HELLO 2 AUTH default mypass after connected.

What did you expect to see?

mypass should be accepted.

What did you see instead?

invalid password error.

Anything Else?

Hi,

According to https://redis.io/commands/hello, the syntax of HELLO should be

HELLO [protover [AUTH username password] [SETNAME clientname]]

and the default username should be default.

However, kvrocks' syntax seems to lack the username part:
https://github.com/apache/incubator-kvrocks/blob/6889b5916ab560ecd5eed79bb9786e36342f0e17/src/commands/cmd_server.cc#L660

Not sure if this is intended? But I think it will be great if matching redis' syntax.

Are you willing to submit a PR?

• I'm willing to submit a PR!

[mapleFU]

Hi @rueian . When I implement "hello", kvrocks didn't support auth like that.
#881 (comment)

[PragmaTwice]

Hi @rueian, it seems meaningless in kvrocks to give a username, since the AUTH command in kvrocks is used to authorize accessibility to a corresponding namespace.

[rueian]

Hi @rueian, it seems meaningless in kvrocks to give a username, since the AUTH command in kvrocks is used to authorize accessibility to a corresponding namespace.

Hi @PragmaTwice, I understand.

However, from a redis client implementation perspective, this difference in syntax means that a client implementation may need to treat Kvrocks differently to work with it. I think this may put some barriers for a client to adapt to Kvrocks.

[PragmaTwice]

@rueian Thanks for you explaination. It make sense. I am not averse to accept an AUTH default <pass> form. cc @git-hulk

[git-hulk]

I also prefer to allow passing the username, even though it's meaningless to Kvrocks.

[tisonkun]

@rueian IIRC the goredis client handle the AUTH command specially for kvrocks. If you're implementing a redis client and hope it can be compatible with kvrocks, feel free to start a discussion to add (compatible) integration tests. As our gocases are written on the top of goredis.

[tisonkun]

Aha. We change for being compatible with go-redis in #830.

[rueian]

Hi @tisonkun,

Yes, I am also making a go redis client, rueidis, and got a report that it doesn't work with kvrocks because of the syntax difference with redis: redis/rueidis#204

But I am not sure if goredis do special work for kvrocks or not, since its latest version doesn't work with kvrocks as well.

For example:

package main

import (
	"context"
	"fmt"
	"github.com/redis/go-redis/v9"
)

func main() {
	client := redis.NewClient(&redis.Options{Addr: "127.0.0.1:6666", Password: "mypass"})
	fmt.Println(client.Ping(context.Background()))
}

goredis actually send hello 3 auth default mypass to kvrocs. And then it returned ping: Syntax error in HELLO option auth error.

[tisonkun]

@rueian Thanks for your repro! That's a good new test case we can add to the integration test suite for verifying this issue is resolved :D