add keystone auth backend #1732
Conversation
|
Nice. And yeah, as far as testing goes, we would need a mock endpoint. We have some mocks in Libcloud and in some of the Rackspace projects I've worked on, we started mock Keystone server as part of out integration test suite (https://github.com/racker/node-rproxy/blob/master/tests/mock-auth-server.js). I believe there is now also an OpenStack project which provides some of the required mocks. |
| :type keystone_version: ``int`` | ||
| """ | ||
| self._keystone_url = keystone_url | ||
| self._keystone_version = keystone_version |
There was a problem hiding this comment.
Looks like this argument is unused and we only support v2 right now?
If so, we should make that clear and throw if an unsupported version is provided.
There was a problem hiding this comment.
On a related note, it might be better to just use python-keystoneclient.
Ideally, eventually we would also switch to using setup.py entry_points for plugins and provide plugins as separate Python packages with it's own requirements.txt file, but for now I'm OK with putting keystone-client dependency to the shared requirements.txt file.
There was a problem hiding this comment.
Actually, Im doing the v2/v3 split now and some other checks. I wanted the PR to go up quick due to the testing question.
Re: keystone client. Totally against that tbh. It turns a simple authentication plugin with one dependency into dependency hell and also ties into a big external projects, while requests is one package with minimal dependencies and probably already installed on every decent python env which needs to do a network call.
And this auth backend just needs to do one simple call. To have keystoneclient as dependency is overkill IMO.
| import requests | ||
|
|
||
| try: | ||
| from urlparse import urlparse |
There was a problem hiding this comment.
We depend on six so you can instead use:
from six.moves.urllib import parse as urlparse
from six.moves.urllib.parse import urljoin
|
@Kami Finally all passing with all changes requested :D |
|
+1 Will wait for @Kami to take a look and merge. |
| } | ||
| } | ||
| } | ||
| login = requests.post(urljoin(self._keystone_url, 'v3/auth/tokens'), json=creds) |
There was a problem hiding this comment.
Just curious, is it v3.0 or just v3? I see v2.0 for the version 2 APIs :). <3 to openstack.
There was a problem hiding this comment.
Just v3. Its the openstack way (tm) of never agreeing to use the same thing from release to release :D
|
Thanks for the contribution! |
|
@Itxaka I made the "payload method" change and merge it into master. Thanks! |
|
@Kami @lakshmi-kannan One last question. How are dependencies managed for backends? Are you gonna use the extras_require keyword in the setup.py? How does that translates to a debian/rpm pakcage? Just a thougth :) |
|
@Itxaka as noted in my previous comment (#1732 (comment)), right now we put backend dependencies in the global requirements.txt file. It's not ideal, but it works. Eventually we will want to move to "repository & Python package" per auth backend model, but to get there we also need to improve out current packaging situation. There were some attempts to improve it (e.g. #1677), but there were a lot of issues with our current CI system and I simply didn't have the time to finish it yet (it was very slow and time consuming debugging process). I believe @dennybaa is also currently working on some packaging improvements. In short, eventually we will get there :) |
|
@Kami Thanks for the info 👍 |
|
@Itxaka you are talking about saving space by not bringing extra dependencies. Yeah it's generally okay, as soon as st2 is distributed bi pypi it can be easily done. We can manage that behavior... But if we are talking about os packages, it will bring unnecessary complexity in native packaging. The current chosen approach is to provide os package with a built-in virtualenv which contains all need dependencies (including extra). In this case packages like st2api or st2auth and all others don't depend on each other, so you can easily install/upgrade/remove any of st2* packages independently. @ltxaka, @Kami currently st2 sources are not fully prepared to be merged with new packaging support, the code of st2 is updated with modifications from here https://github.com/dennybaa/st2-packages/tree/master/sources. The code in https://github.com/dennybaa/st2-packages actually builds st2 packages and then performs integration testing. (PS. it's almost finished for debian). |
Not sure how I would go about doing the tests as it needs a keystone endpoint. Should I just mock it?