jplana / python-etcd

A python client for etcd
Other
522 stars 210 forks source link

Auth is "broken" when using etcd 2.3.x+ #210

Closed ashcrow closed 7 years ago

ashcrow commented 7 years ago

The authentication data that is returned from the server changed somewhere between etcd 2.1.3 and 2.3.7. This is causing the auth code and unittests to fail for folks using newer versions of etcd.

This is an issue as of 0d0145f5e835aa032c97a0a5e09c4c68b7a03f66 (not the cause, just a reference)

References:

c = Client(port=2379, username='root', password='test')
from etcd import auth
root = auth.EtcdUser(c, 'root')
root.read()
....
TypeError: unhashable type: 'dict'

Possible Solutions

Remove Auth

This is the easiest solution, but not the most attractive.

Upgrade Structure To Latest etcd Version

Updating the auth code to understand the latest etcd auth structure (IE: 2.3.x+) would be the next easiest way to handle this. The downside being that anyone running with older version of the 2 series will have a broken authentication module. However, the code could do a version check and provide the auth module only if the server was within the proper version range.

Support Old and New Structure

The hardest, but most user friendly. Updating the auth module to understand both the 2.1.x and 2.3.x structures. More abstractions would need to be made to ensure that if a user upgraded their server from 2.1.x to 2.3.x they wouldn't need to change their code.

lavagetto commented 7 years ago

@ashcrow my latest PR should fix the issue; would you care to check the code as well?

ashcrow commented 7 years ago

@lavagetto tested on 2.3.0 with a simple user and role. It seems to work:

>>> import etcd
>>> from etcd import auth
>>> c = etcd.Client(port=2379, username='root', password='************')
>>> t = auth.EtcdRole(c, 'test')
>>> t.acls
{}
>>> t.read()
>>> t.acls
{'/*': 'RW'}

My only thought is that it would be nice for read to be called upon instantiation of the class. But that isn't really a problem with the PR :smile:

:+1:

lavagetto commented 7 years ago

@ashcrow yeah I kinda agree, but preferred to allow people to do lazy evaluation at the time. Probably not the best choice at the time.