Fix the etcd version check and have it return the version string. #4991
Conversation
| return fmt.Errorf("unknown server: %s", string(body)) | ||
|
|
||
| var dat map[string]interface{} | ||
| if err := json.Unmarshal(body, &dat); err != nil { |
There was a problem hiding this comment.
Are we now requiring etcd version >= 2.0?
There was a problem hiding this comment.
No explicit version check yet, but this code will enable it. The previous version was just broken. It seems at some point etcd started returning a simple JSON version object for the version request.
There was a problem hiding this comment.
This unmarshall will return an error with etcd v0.4.6 where /version still returns etcd 0.4.6 so this is essentially a > 0.4.6 minimum version check. Is that a concern? The transition may have happened at etcd v2.0.
There was a problem hiding this comment.
Shouldn't be a concern with the current usage of this code (see NewEtcdClientStartServerIfNecessary below). But, could be an issue if other start relying on this code for a true version check.
| } | ||
| return nil | ||
| return dat["releaseVersion"].(string), dat["internalVersion"].(string), err |
There was a problem hiding this comment.
You should protect these type assertions; a function to check if etcd is OK shouldn't be able to crash on bad input.
| @@ -418,20 +418,24 @@ func (h *EtcdHelper) AtomicUpdate(key string, ptrToType runtime.Object, ignoreNo | |||
| } | |||
| } | |||
|
|
|||
| func checkEtcd(host string) error { | |||
| // Performs a version check against the provided Etcd server, returning a triplet | |||
There was a problem hiding this comment.
The comment should start with the name of the function, e.g. "CheckEtcd performs ..."
fabioy
force-pushed
the
etcdcheck-fix
branch
2 times, most recently
from
04ec9d4 to
d6bf1c8
Compare
| } | ||
| return nil | ||
| if dat["releaseVersion"] != nil { |
There was a problem hiding this comment.
Changed the code to be resilient to missing attributes.
There was a problem hiding this comment.
this is still not safe.
if obj := dat["internalVersion"]; obj != nil {
if s, ok := obj.(string); ok {
// do stuff with s
}
}There was a problem hiding this comment.
In the context of this code, the original version should be safe (I think, since it's unmarshalling a json). But to be extra safe, made the changes.
There was a problem hiding this comment.
You could have ended up with an int, or a map[string]interface{} on unexpected input.
|
LGTM |
Fix the etcd version check and have it return the version string.
Also make the "CheckEtcd" call package public.