errors.Is not work for rpctypes.ErrGRPCxxxx

[cruvie]

What would you like to be added?

support errors.Is for error generated from client

or provide a method to compare two errors

Why is this needed?

func TestErr(t *testing.T) {
	client, err := clientv3.New(clientv3.Config{
		Endpoints: []string{"127.0.0.1:2379"},
		Username:  "notexistuser",
		Password:  "",
	})
	if err != nil {
		panic(err)
	}
	resp, err := client.AuthStatus(context.Background())
	log.Println(resp)
	if err != nil {
		if errors.Is(err, rpctypes.ErrGRPCUserEmpty) {
			//should enter to this branch
			log.Println("user not exist")
		}
		log.Println(err)//etcdserver: user name is empty
	}
}

go 1.23.0
go.etcd.io/etcd/client/v3 v3.5.15

[ArkaSaha30]

Can you please help with the steps to reproduce the issue?

[cruvie]

@ArkaSaha30 I have updated the reproduce code above
use a not exist user create etcd clietn
the return err should match if errors.Is(err, rpctypes.ErrGRPCUserEmpty) {, but it skiped

[ivanvc]

@cruvie, please try using rpctypes.ErrUserNotFound.

However, I think instantiating the client should return an authentication error before it gets to client.AuthStatus

[cruvie]

@ivanvc rpctypes.ErrUserNotFound does not work either

I think instantiating the client should return an authentication error before it gets to client.AuthStatus

I agree

[ivanvc]

@cruvie, got it. The issue is that I'm not able to reproduce. If I start a new cluster, enable authentication, and then run the snippet you provided:

        client, err := clientv3.New(clientv3.Config{
		Endpoints: []string{"127.0.0.1:2379"},
		Username:  "notexistuser",
		Password:  "",
	})
	if errors.Is(err, rpctypes.ErrAuthFailed) {
		log.Fatal(err)
	}

It fails with ErrAuthFailed, as expected. I'd appreciate it if you could provide how you have your setup so we can reproduce it.

[cruvie]

@ivanvc here are my steps
run docker-compose

version: "3.9"
services:
# https://etcd.io/docs/v3.5/op-guide/container/
  etcd:
    image: quay.io/coreos/etcd:v3.5.15-arm64
    container_name: etcd
    command:
      [
        "/usr/local/bin/etcd",
        "--data-dir=/etcd-data",
        "--name=node1",
        "--initial-advertise-peer-urls=http://etcd:2380",
        "--listen-peer-urls=http://0.0.0.0:2380",
        "--advertise-client-urls=http://etcd:2379",
        "--listen-client-urls=http://0.0.0.0:2379",
        "--initial-cluster=node1=http://etcd:2380"
      ]
    restart: unless-stopped
    ports:
      - "2379:2379"
      - "2380:2380"

run code

func TestErr(t *testing.T) {
	client, err := clientv3.New(clientv3.Config{
		Endpoints: []string{"127.0.0.1:2379"},
		Username:  "notexistuser",
		Password:  "",
	})
	if err != nil {
		panic(err)
	}
	{
		_, err := client.UserAdd(context.Background(), "root", "root")
		if err != nil {
			panic(err)
		}
		_, err = client.RoleAdd(context.Background(), "root")
		if err != nil {
			panic(err)
		}
		_, err = client.UserGrantRole(context.Background(), "root", "root")
		if err != nil {
			panic(err)
		}
		_, err = client.AuthEnable(context.Background())
		if err != nil {
			panic(err)
		}
	}
	resp, err := client.AuthStatus(context.Background())
	log.Println(resp)
	if err != nil {
		//if errors.Is(err, rpctypes.ErrGRPCUserEmpty) {
		if errors.Is(err, rpctypes.ErrUserNotFound) {
			//should enter to this branch
			log.Println("user not exist")
		}
		log.Println(err) //etcdserver: user name is empty
	}
}

output

=== RUN   TestErr
{"level":"warn","ts":"2024-09-11T11:58:04.374368+0800","logger":"etcd-client","caller":"[email protected]/retry_interceptor.go:63","msg":"retrying of unary invoker failed","target":"etcd-endpoints://0x140002425a0/127.0.0.1:2379","attempt":0,"error":"rpc error: code = InvalidArgument desc = etcdserver: user name is empty"}
2024/09/11 11:58:04 <nil>
2024/09/11 11:58:09 etcdserver: user name is empty
--- PASS: TestErr (9.02s)
PASS

[ivanvc]

Good catch, @cruvie. I added a unit test to confirm we're not wrapping up the errors. I have a fix for it, too.

diff --git a/api/v3rpc/rpctypes/error_test.go b/api/v3rpc/rpctypes/error_test.go
index bf3e0c680..3f95265dd 100644
--- a/api/v3rpc/rpctypes/error_test.go
+++ b/api/v3rpc/rpctypes/error_test.go
@@ -15,6 +15,7 @@
 package rpctypes
 
 import (
+       "errors"
        "testing"
 
        "google.golang.org/grpc/codes"
@@ -40,3 +41,20 @@ func TestConvert(t *testing.T) {
                t.Fatalf("expected them to be equal, got %v / %v", ev2.Code(), e3.(EtcdError).Code())
        }
 }
+
+func TestComparingWrappedError(t *testing.T) {
+       errTest := errors.New("test error")
+       e1 := Error(ErrGRPCEmptyKey)
+       e2 := Error(status.Error(codes.InvalidArgument, "etcdserver: key is not provided"))
+       e3 := Error(errTest)
+
+       if !errors.Is(e1, ErrGRPCEmptyKey) {
+               t.Fatalf("expected %v to be an ErrGRPCEmptyKey wrapped error", e1)
+       }
+       if !errors.Is(e2, ErrGRPCEmptyKey) {
+               t.Fatalf("expected %v to be an ErrGRPCEmptyKey wrapped error", e1)
+       }
+       if !errors.Is(e3, errTest) {
+               t.Fatalf("expected %v to be an errTest wrapped error", e3)
+       }
+}

[ivanvc]

A workaround right now would be to do the following:

err.Error() == rpctypes.Error(rpctypes.ErrUserNotFound).Error()

There are still some integration and e2e tests failing in my PR that I'd need to review. Unless I'm missing something obvious, I thought my solution was valid (https://github.com/etcd-io/etcd/pull/18578/files#diff-f8af16dfa5f99f7004c7c7b8939b74a797fa8dfd63af7664cfc850efe9056bc9). @ahrtr, I saw in some places that you wrote some code that uses errors.Is() with rpctypes errors (client/v3/retry_interceptor.go:333). Would you happen to have any feedback?

[ahrtr]

Let's be cautious to make any change because it may break K8s or any etcd protocol (etcdclient <--> etcdserver) compatible system. I recall @liggitt mentioned that some Kubernetes instances might use the etcd client SDK to communicate with non-etcd servers.

There are lots of errors in etcd,

• server side gRPC errors
• server side errors
• client side error

I'd like to have a summary of all these confusing errors, including how they are handled and converted between different formats or contexts. Let's avoid making changes until everything has been thoroughly discussed and understood. Otherwise, it’s better to do nothing at all.

[liggitt]

cc @logicalhan @serathius