-
Notifications
You must be signed in to change notification settings - Fork 54
Conversation
(It will probably be easier for everyone if you guys look at this before I've nailed down every last thing and am just blocked on review). |
@@ -70,7 +70,7 @@ def _get_readme(): | |||
'importlib==1.0.3', | |||
'passlib==1.6.2', | |||
'pexpect==3.3', | |||
'requests==2.4.1', | |||
'requests>=2.4.1,<3.0', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does requests 3.0 break something? If so, a comment mentioning that could help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't exist yet, but see http://semver.org/
Maybe worth having a comment about that in general.
for key in cfg.options(__name__): | ||
keystone_cfg[key] = cfg.get(__name__, key) | ||
|
||
# Great job with the API design Openstack! </sarcasm> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
factories!!!!
Hm. I'm not sure tests running against a real Keystone should be considered unit tests. What I would do for unit tests is: fake the Keystone calls, make sure that it determines authorization correctly, for a few different kinds of calls. Then, save the actual running against Keystone for integration tests. |
Yeah, it's in the wrong directory. Will fix |
6fad9d0
to
2239bfa
Compare
@gsilvis, it's been moved. I rebased; trying to not pollute the history too much. Perhaps useful for mitigating the negative effects of that: https://zenhack.net/2015/09/27/github-code-review-workflow.html |
49caef5 looks good to me |
Probably already on your radar, but would it be possible to include some documentation that talks about where projects get created and roles get assigned under keystone? (ie - is it in HIL or keystone; does it require being a HIL || keystone admin, etc) |
@henn: @gsilvis, @knikolla and I talked about this on IRC for a while. I'm not 100% sure we actually came to an agreement, but I think the plan for the time being is to match projects up by name; an admin will have to create a project in hil corresponding to the openstack project name, and then you can authenticate with keystone. @gsilvis, @knikolla, did I forget anything important from that conversation? |
Of course whatever gets implemented will be clearly documented. |
@zenhack, @gsilvis, @knikolla - is that how it works in other OpenStack projects, like Nova or Cinder? I'd be interested in documenting somewhere (maybe here?) the pros/cons for the different possibilities. Also - If we implemented quotas (not sure if keystone stores that or the service?), all OpenStack projects could be referenced without being created in HIL first, but just the ones with HIL quotas would be able to do anything more than query stuff. |
We're logging the IRC channel, correct? Could we fish out the logs for that conversation, rather than having it again? I don't know where those are. |
Or perhaps #614; not sure. |
moc.2016-06-09.log.txt |
@zenhack If that's not the right one let me know, I can zip up all the logs from June for you |
hm, I remember a longer discussion about how to do the project mapping, but apparently it didn't happen when I thought. @gsilvis, @knikolla: do you remember when that was? @henn, you said you thought you had something in your buffer that seemed relevant, when was that? (as an aside, might be better if we could just access this ourselves in the future...) |
Getting the whole month's logs might just be the easy thing to do. |
Here are the relevant bits of the conversation that @gsilvis, @knikolla and I had on IRC: https://gist.github.com/zenhack/5ea1b615681f43c14718a2a6e8e8629e This was interleaved with discussion about what became #614. I've removed what clearly isn't relevant, but the topics overlapped in places. @kamfonik, thanks for digging these out. |
@henn, if you would read over that log and then weigh in, that would be appreciated. @gsilvis, @knikolla, let's come to a consensus on this so I can plow ahead. Also: regretably this hasn't gone in quite as quickly as I'd hoped. Most of that is to do with me; I've been dealing with sleep debt, and other commitments which I did a poor job of planning around. A thing to be aware of is that I'm going to be unavailable (and unable to work on this) starting Tuesday of next week through July 4th. Getting this in before then is going to be tight, but we come to a consensus on these issues tomorrow, I can probably have something working and ready for review by Monday. If we don't get it in before then it will have to wait another week. |
Folks, can we come to a consensus on this? |
Since it's perhaps easier than rifling through the IRC log, here's a summary of the questions at hand, and proposed solutions:
My preferences are to map projects to UUIDs, and do manual project creation, @gsilvis has expressed that he strongly prefers manual project creation. The IRC thread also discussed some database schema changes re: the //cc @henn |
Re: UI issues around using UUIDs for projects: we can always write tools that look up projects by name before invoking the API, so the user doesn't have to deal with it. General philosophy has been to keep the server-side fairly dumb. |
I like the idea of manual project creation and mapping projects to UUIDs I am less clear about the domain issue for project names. is there problems with the uglyness of having huge UUIds in the project name of HIL, not obvious why/if this is a problem? |
I believe I've either implemented or replied to all comments; let me know if I missed something. travis is still running at the moment, but stuff passes on my local machine. |
Looks good to me, +1. |
This still all looks good to me. @henn had more requests than I did, so I think it'd be good for him to give this his +2. |
...rather than keystone master.
Added the docs re: admin role and a thing to test against mitaka. |
To grant the Openstack project with that UUID access to HaaS. | ||
|
||
Note that the plugin recognizes any user with an `admin` role on any | ||
project as a HaaS administrator, similar to the default policy for core |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you say as a global HaaS administrator, able to administrate all projects,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The clarification was for keystone folks, where you could have a project
admin vs. a global admin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That just isn't true. In both Openstack and haas, 'admin' refers to a global admin (or at least the admin of a domain), and there is no concept of a project admin.
re: the test failures, it looks like keystone's Perhaps using tag 10.0.0.0b2 instead of stable/mitaka would be a good workaround? |
Quoting Jason Hennessey (2016-08-09 03:40:29)
Not a bad idea. I will fuss with it and find a version that works. |
mitaka doesn't seem to work; @henn suggested this tag instead.
One final question - what is the error given back to the user if the keystone client credentials are incorrect in some way (username, password, inaccessible project, etc)? |
@zenhack - Just wanted to check that the error was something sane. |
@henn, sorry for the delay. I poked at it a bit, most mistakes give you a 401. You get a connection error from keystone if |
Also, a piece of this that I'd meant to deal with at some point, but that got forgotten about: how do we bootstrap the list of projects? We could add another command like create_initial_admin or whatever it was called, but for projects. I just rememberd because as part of testing this I ended up having to create a first project by turning on the null auth backend, then switching back to the keystone backend. That's not really a workable solution though. Shouldn't be hard to just add another similar command, if folks think that's a good approach. |
@zenhack I'm going to merge this now since the code is good and we want to prevent bitrotting. Two requests:
|
+1; merging |
Quoting Jason Hennessey (2016-08-18 23:32:46)
It's not immediately obvious to me how to differentiate different forms
You mean as an Openstack project that has admin rights? Not unless that |
Hmmm... it sounds like we do have a problem then. What if we allowed an admin from any OpenStack project to be a HIL admin, regardless of whether the project was registered? In normal OpenStack, I don't believe you need to pre-register projects anyhow - we did it more for keeping our own internal tables straight and maybe in lieu of quotas. |
Agree with @henn here: admins on projects not yet registered with HIL should still be able to perform any function (unless it's a function that is scoped to the user's project). The authorization layer shouldn't care if a project has been created. But, if the admin tries to do something like add a node to their project, then they should get the non-auth error "Project does not exist." |
Quoting George Silvis, III (2016-08-19 08:34:36)
Agree with ***@***.*** here: admins on projects not yet registered with
HIL should still be able to perform any function (unless it's a
function that is scoped to the user's project). The authorization layer
shouldn't care if a project has been created.
I'm fine with this. I can toss up another pr with this change soonish.
But, if the admin tries to do something like add a node to their
project, then they should get the non-auth error "Project does not
exist."
Yep, and this should be hard to screw up -- in the code you need a
project object to act on, so if it's not in the db the code won't be
able to fetch the project at all.
|
Awesome - I think we have a plan here. |
WIP. Most of the effort on this one is going to be testing; yay openstack. I'm in the midst of debugging the CI setup. Would appreciate comments on the design, in particular the direction I'm going with the tests.
I'd hoped to work on this more today, but I've been running around doing last minitue prep for something that I half-forgot was this weekend. I'm going to be busy Monday too, but wil dive back into this as soon as I can. possible I can put a bit of time in Sunday night.
//cc @gsilvis, @henn, @knikolla