Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

auth: context.WithValue has string type keys #8826

Closed
gyuho opened this issue Nov 6, 2017 · 5 comments
Closed

auth: context.WithValue has string type keys #8826

gyuho opened this issue Nov 6, 2017 · 5 comments

Comments

@gyuho
Copy link
Contributor

gyuho commented Nov 6, 2017

// 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
Copy link
Contributor

mitake commented Nov 8, 2017

@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
Copy link
Contributor Author

gyuho commented Nov 8, 2017

@mitake
Copy link
Contributor

mitake commented Nov 9, 2017

@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
Copy link
Contributor Author

gyuho commented Nov 9, 2017

@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
Copy link
Contributor

mitake commented Nov 21, 2017

@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.

mitake added a commit to mitake/etcd that referenced this issue Nov 21, 2017
The keys of context shouldn't be string. They should be a struct of
their own type.

Fix etcd-io#8826
mitake added a commit to mitake/etcd that referenced this issue Nov 21, 2017
The keys of context shouldn't be string. They should be a struct of
their own type.

Fix etcd-io#8826
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants