Dropped error in etcd/v2 lets multiple clients think they've acquired a lock.

[jlhawn]

There's some bad error checking around here:

libkv/store/etcd/v2/etcd.go

Lines 528 to 534 in 5e4bb28

	if err != nil {
	if etcdError, ok := err.(etcd.Error); ok {
	if etcdError.Code != etcd.ErrorCodeNodeExist {
	return nil, err
	}
	setOpts.PrevIndex = ^uint64(0)
	}

Where if the error is not of type etcd.Error then it is ignored completely. Then, another request to Set() the key is made with the PrevIndex value not set. This will just overwrite whatever value was at that key as long as it already exists. This results in two processes thinking they have acquired the lock at the same time. This only occurs under the condition that the first Set call fails due to some transient connection issue and the second call succeeds. We have seen this occur several times.

Why is there even a second call to Set() the key? Why not immediately go into a wait loop if the first Set() attempt results in etcd.ErrorCodeNodeExist?

[abronan]

Thank you for the report @jlhawn ,

etcdv2 backend was not the core of my focus with this fork as this was deprecated with the support of etcdv3 client. I would advise switching because etcdv3 includes a proper locking interface while this one was just a hack to circumvent a missing lock implementation with etcdv2.

Returning to the core of the issue, I think we assumed that if the first call to Set returned an error, then it was unlikely that the second would succeed. But the scenario you describe is indeed possible so returning an error on the first call to Set if it fails and the error is different than etcd.Error makes sense. With a basic retry mechanism outside of the call to Lock, this would prevent the scenario you described.

This should be easy enough to fix, feel free to submit a patch or I'll do this if I have some free time.