panic: runtime error: comparing uncomparable type map[string]interface {}

[cruvie]

Bug report criteria

• This bug report is not security related, security issues should be disclosed privately via etcd maintainers.
• This is not a support request or question, support requests or questions should be raised in the etcd discussion forums.
• You have read the etcd bug reporting guidelines.
• Existing open issues along with etcd frequently asked questions have been checked and this is not a duplicate.

What happened?

use grpc client made by etcd resolver with google.golang.org/grpc v1.71.0 cause panic

What did you expect to happen?

call rpc method successfully

How can we reproduce it (as minimally and precisely as possible)?

get a grpc client like this

import (
	"go.etcd.io/etcd/client/v3/naming/resolver"
	"google.golang.org/grpc"
)

// GetGrpcClient get grpc client for serverName
//
// don't forget to close conn conn.Close()
func GetGrpcClient[T any](serverName string,
	clientBuilder func(grpcConn grpc.ClientConnInterface) (client T),
	opts ...grpc.DialOption) (conn *grpc.ClientConn, client T, err error) {

	//refers to https://etcd.io/docs/v3.5/dev-guide/grpc_naming/
	etcdResolver, err := resolver.NewBuilder(GetClient())
	if err != nil {
		return nil, client, err
	}

	options := []grpc.DialOption{
		grpc.WithResolvers(etcdResolver),
		grpc.WithDefaultServiceConfig(`{"loadBalancingPolicy":"round_robin"}`),
	}
	options = append(options, opts...)

	conn, err = grpc.NewClient("etcd:///grpc/"+serverName,
		options...,
	)
	client = clientBuilder(conn)
	return conn, client, err
}

and use the client call a method

Anything else we need to know?

go 1.24
google.golang.org/grpc v1.70.0 works fine
create a grpc client manually works fine

func TestPanic(t *testing.T) {
	etcdCloseFunc := global_config.InitEtcdClient(false)
	defer etcdCloseFunc()
	_, client, err := GetClient()
	if err != nil {
		t.Error(err)
	} else {
		status, err := client.UpdateLocationOnlineStatus(context.Background(), &UpdateLocationOnlineStatus_Input{
			UserId: "test",
		})
		if err != nil {
			t.Error(err)
		}
		t.Log(status)
	}
}

func TestWorksFine(t *testing.T) {

	conn, err := grpc.NewClient("127.0.0.1:58759",
		grpc.WithTransportCredentials(insecure.NewCredentials()))
	if err != nil {
		log.Fatalf("did not connect: %v", err)
	}
	defer conn.Close()

	client := NewRPCDaZiNearbyClient(conn)
	ctx, cancelFunc := context.WithTimeout(context.Background(), time.Second*2)
	defer cancelFunc()
	resp, err := client.UpdateLocationOnlineStatus(ctx, &UpdateLocationOnlineStatus_Input{
		UserId: "test",
	})
	if err != nil {
		log.Fatalf("could not test: %v", err)
	}
	log.Printf("DeleteTest response: %v", resp)
}

Etcd version (please run commands below)

deployed by docker

  ss-etcd:
    image: quay.io/coreos/etcd:v3.5.17-arm64
    # image: quay.io/coreos/etcd:v3.5.17-amd64
    container_name: ss-etcd
    command:
      [
        "/usr/local/bin/etcd",
        "--data-dir=/etcd-data",
        "--name=node1",
        "--initial-advertise-peer-urls=http://ss-etcd:2380",
        "--listen-peer-urls=http://0.0.0.0:2380",
        "--advertise-client-urls=http://ss-etcd:2379",
        "--listen-client-urls=http://0.0.0.0:2379",
        "--initial-cluster=node1=http://ss-etcd:2380"
      ]
    restart: unless-stopped
    ports:
      - "2379:2379"
      - "2380:2380"
    volumes:
      - ./etcd/data:/etcd-data

Etcd configuration (command line flags or environment variables)

default

Etcd debug information (please run commands below, feel free to obfuscate the IP address or FQDN in the output)

standalone

Relevant log output

=== RUN   TestPanic
panic: runtime error: comparing uncomparable type map[string]interface {}

goroutine 11 [running]:
google.golang.org/grpc/balancer/pickfirst/pickfirstleaf.equalAddressIgnoringBalAttributes(0x1400031c970, 0x1400031ca10)
	/Users/cruvie/go/pkg/mod/google.golang.org/[email protected]/balancer/pickfirst/pickfirstleaf/pickfirstleaf.go:931 +0xe0
google.golang.org/grpc/balancer/pickfirst/pickfirstleaf.(*addressList).seekTo(0x14000333e20, {{0x14000040468, 0x13}, {0x0, 0x0}, 0x0, 0x140001225a0, {0x104f75a00, 0x14000414030}})
	/Users/cruvie/go/pkg/mod/google.golang.org/[email protected]/balancer/pickfirst/pickfirstleaf/pickfirstleaf.go:905 +0xc8
google.golang.org/grpc/balancer/pickfirst/pickfirstleaf.(*pickfirstBalancer).updateSubConnState(0x14000333dd0, 0x1400030d880, {0x2, {0x0, 0x0}, {{0x14000040468, 0x13}, {0x14000152ea0, 0x21}, 0x0, ...}})
	/Users/cruvie/go/pkg/mod/google.golang.org/[email protected]/balancer/pickfirst/pickfirstleaf/pickfirstleaf.go:627 +0x81c
google.golang.org/grpc/balancer/pickfirst/pickfirstleaf.(*pickfirstBalancer).newSCData.func1({0x2, {0x0, 0x0}, {{0x14000040468, 0x13}, {0x14000152ea0, 0x21}, 0x0, 0x140001225a0, {0x104f75a00, ...}}})
	/Users/cruvie/go/pkg/mod/google.golang.org/[email protected]/balancer/pickfirst/pickfirstleaf/pickfirstleaf.go:197 +0x84
google.golang.org/grpc/internal/balancer/gracefulswitch.(*Balancer).updateSubConnState(0x140000f05a0, {0x105116900, 0x1400003ba40}, {0x2, {0x0, 0x0}, {{0x14000040468, 0x13}, {0x14000152ea0, 0x21}, ...}}, ...)
	/Users/cruvie/go/pkg/mod/google.golang.org/[email protected]/internal/balancer/gracefulswitch/gracefulswitch.go:262 +0x2bc
google.golang.org/grpc/internal/balancer/gracefulswitch.(*balancerWrapper).NewSubConn.func1({0x2, {0x0, 0x0}, {{0x14000040468, 0x13}, {0x14000152ea0, 0x21}, 0x0, 0x140001225a0, {0x104f75a00, ...}}})
	/Users/cruvie/go/pkg/mod/google.golang.org/[email protected]/internal/balancer/gracefulswitch/gracefulswitch.go:378 +0x98
google.golang.org/grpc.(*acBalancerWrapper).updateState.func1({0x105112108?, 0x140004f8c30?})
	/Users/cruvie/go/pkg/mod/google.golang.org/[email protected]/balancer_wrapper.go:341 +0x274
google.golang.org/grpc/internal/grpcsync.(*CallbackSerializer).run(0x14000111d80, {0x105112108, 0x140004f8c30})
	/Users/cruvie/go/pkg/mod/google.golang.org/[email protected]/internal/grpcsync/callback_serializer.go:94 +0x10c
created by google.golang.org/grpc/internal/grpcsync.NewCallbackSerializer in goroutine 25
	/Users/cruvie/go/pkg/mod/google.golang.org/[email protected]/internal/grpcsync/callback_serializer.go:52 +0x118

[siyuanfoundation]

Thanks @cruvie for raising the issue.
We have tests for the resolver in tests/integration/clientv3/naming/resolver_test.go. Would you be willing to add a unit test to reproduce your issue? This could help testing potential fix and catch future problems.

[cruvie]

@siyuanfoundation Hi, I made a pr which includes two unit tests and TestEtcdGrpcResolverRoundRobinWithMetaInfo will fail

[siyuanfoundation]

@cruvie
Thanks for the PR. It's really helpful.
The failure is because they added https://github.com/grpc/grpc-go/blob/51d6a43ec59753d42ccd02bb12d2e9e40c164f0f/clientconn.go#L944 in grpc.
If the type of Metadata is not comparable, it would panic.

The issue is on the grpc side, they should probably use reflect.DeepEqual(a.Metadata, b.Metadata) instead of a.Metadata == b.Metadata, even though the Metadata is deprecated.

Meanwhile, we should add Attributes to deprecate Metadata in etcd (#19706)

@serathius @ahrtr Do you think it is necessary to revert back to google.golang.org/grpc v1.70.0?

[dfawley]

The Metadata field is in an experimental package. And it has been marked as deprecated since 2019.

See also #15145 from 2023.

If you are using gRPC APIs that are clearly marked as experimental, you need to vendor gRPC, or you need to consider yourself experimental.

If some functionality you need is experimental and there is no way to get it through stable APIs, then I'm always happy to discuss how we can make that happen. gRPC has pending changes we'd like to make to these packages that are mostly just blocked on etcd's lingering usages of these APIs.

[cruvie]

they are working on it grpc/grpc-go#8225

for etcd client next version, I think make a break change to remove Metadata any

etcd/client/v3/naming/endpoints/endpoints.go

Line 35 in d6416b8

Metadata any

and add Attributes *attributes.Attributes like
https://github.com/grpc/grpc-go/blob/ce35fd41c56908f4e703e3fd63cf85b2cba9d8f1/resolver/resolver.go#L107
is ok, since Metadata has not been fully tested for a long time?

[dfawley]

I'm not sure what the right replacement is, until I understand what it is you need to do. It could be that you don't need a custom resolver at all anymore. With the advent of https://pkg.go.dev/google.golang.org/grpc#ClientConnInterface - added in Jan 2020 - I think most of the reasons for having a custom resolver can be replaced by using multiple gRPC channels and wrapping them, similar to how GCP does it:

https://github.com/googleapis/google-api-go-client/blob/bac81c61de0c586306eb621dcabca1687fb99753/transport/grpc/pool.go#L31

If that wouldn't work: why?

[cruvie]

In my use case, I overlooked the relation between metadata and resolver. I just want to store some additional information to Endpoint.

So for my use case add Attributes *attributes.Attributes as a replacement of metadata is a reasonable solution.

[ahrtr]

etcd itself isn't affected by this problem. But I am not sure whether grpcproxy is affected, because the metadata is set as a struct (which I believe will be a map as well after marshaling & Unmarshaling). Given our grpcproxy related e2e / integration tests all pass, so I assume it isn't affected either.

etcd/server/proxy/grpcproxy/register.go

Line 75 in 3b66c82

endpoint := endpoints.Endpoint{Addr: addr, Metadata: getMeta()}

etcd/server/proxy/grpcproxy/register.go

Lines 94 to 98 in 3b66c82

	func getMeta() string {
	hostname, _ := os.Hostname()
	bts, _ := json.Marshal(meta{Name: hostname})
	return string(bts)
	}

So in general, I don't want to patch this local issue. Instead, we should spend more effort on #15145 to resolve such problems once for all. We need to have a summary on existing design/implementation first, and proposals to replace them later, and invite grpc's experts to review. Just updated the #15145 as well. cc. @fuweid @serathius