auth: context.WithValue has string type keys

[gyuho]

// The provided key must be comparable and should not be of type
// string or any other built-in type to avoid collisions between
// packages using context. Users of WithValue should define their own
// types for keys. To avoid allocating when assigning to an
// interface{}, context keys often have concrete type
// struct{}. Alternatively, exported context key variables' static
// type should be a pointer or interface.
func WithValue(parent Context, key, val interface{}) Context {

https://github.com/coreos/etcd/blob/1d01aaa3958069e062805edd808b4658e1a04133/auth/store.go#L1066-L1073

From #8031.

https://github.com/coreos/etcd/blob/5ed5ee51f57a635c84acde19d96c288f57d1754a/proxy/grpcproxy/util.go#L35-L38

The key should have been typed (e.g. keyABC{}), rather than string.
Otherwise, it could collide with other keys (e.g. tracing).

"index" and "simpleToken" have not been released yet, so should be safe to change.
Do we have any other string context keys in 3.2? /cc @mitake

UPDATE: might need handle 3.1 #5672 compatibility as well.

[mitake]

@gyuho I didn't know the keys should have its own type, thanks for letting me know. We don't have keys other than the two.

BTW we are using context instead of a custom struct for passing the parameters for reducing dependencies from etcdserver to auth (the result of offline discussion with @xiang90 ). But if we need custom types for the params, it must introduce such a dependency between the packages. So I think that stopping using context and introduce a struct for the parameter would be fine. How do you think? @xiang90

[gyuho]

@mitake @xiang90 I meant something like this:

context.WithValue(ctx, simpleToken{}, "bar")

And we have 2 places to handle backward compatibility:

https://github.com/coreos/etcd/blob/5ed5ee51f57a635c84acde19d96c288f57d1754a/auth/simple_token.go#L189-L192

https://github.com/coreos/etcd/blob/5ed5ee51f57a635c84acde19d96c288f57d1754a/proxy/grpcproxy/util.go#L66-L67

[mitake]

@gyuho yes, in such a case, the definition of simpleToken{} should be in auth and used from etcdserver? Then the dependency between the packages can be increased, so I think dropping context and introducing custom structs rather than using context would be simpler.

BTW do we need to care about the backward compatibility between packages which cannot be seen from external ones?

Also, the code of etcd/proxy/grpcproxy/util.go isn't related to other context usages. I think the type of key cannot be changed because the parameter is initialized like this:
https://github.com/coreos/etcd/blob/master/clientv3/client.go#L169-L175

[gyuho]

@mitake Oh, somehow I was thinking context passed from other servers (where we need worry about compatibilities against older versions). But in this case, the context only goes from etcdserver to auth package, and never goes to other servers. So breaking change should be fine.

so I think dropping context and introducing custom structs rather than using context would be simpler.

Agree.

key cannot be changed because the parameter is initialized like this:

I see. Then, proxy should fine, as it is now.

[mitake]

@gyuho I tried to eliminate context styled parameter passing from etcdserver to auth, but found that it is quite useful for LeaseRevoke(): https://github.com/coreos/etcd/blob/master/etcdserver/server.go#L772-L775 . Probably employing a newly defined struct instead of context requires lots of hard to follow changes. I'll replace the string typed keys of the existing contexts for following the correct usage of context.