| *** gokrokve has joined #openstack-keystone | 00:00 | |
| ayoung | jamielennox, I merged the 3 different patches into one. But the heart is contrib/revoke/model.py and core.py | 00:06 |
|---|---|---|
| *** tellesnobrega has joined #openstack-keystone | 00:17 | |
| ayoung | jamielennox, a couple things about the extension. I explained the revocation checking http://adam.younglogic.com/2014/02/efficient-revocation-checking/ | 00:23 |
| jamielennox | ayoung: yea, i'm going to have to have another read through of how the revocation tree stuff is happening | 00:23 |
| jamielennox | ayoung: just going through the weekends emails | 00:23 |
| ayoung | the SQL backend required a few changes to the setup code as the migration had to be called at the same time as the test code called the migration for the main database | 00:23 |
| ayoung | jamielennox, also, a lot of the testing is "replace revoke by id with revoke by token" and run the existing tests | 00:24 |
| *** dolphm_503 is now known as dolphm | 00:37 | |
| *** leseb has joined #openstack-keystone | 00:45 | |
| *** dolphm is now known as dolphm_503 | 00:47 | |
| *** leseb has quit IRC | 00:50 | |
| *** stevemar has quit IRC | 01:15 | |
| *** amerine_ has joined #openstack-keystone | 01:22 | |
| *** amerine has quit IRC | 01:24 | |
| *** tellesnobrega has quit IRC | 01:35 | |
| *** tellesnobrega has joined #openstack-keystone | 01:36 | |
| *** gokrokve has quit IRC | 01:39 | |
| *** gokrokve has joined #openstack-keystone | 01:41 | |
| *** dolphm_503 is now known as dolphm | 01:59 | |
| *** stevemar has joined #openstack-keystone | 02:01 | |
| *** ChanServ sets mode: +v stevemar | 02:01 | |
| *** amcrn has joined #openstack-keystone | 02:03 | |
| *** dolphm is now known as dolphm_503 | 02:34 | |
| *** dolphm_503 is now known as dolphm | 02:52 | |
| *** gokrokve has quit IRC | 03:13 | |
| *** gokrokve has joined #openstack-keystone | 03:13 | |
| *** gokrokve has quit IRC | 03:17 | |
| *** stevemar has quit IRC | 03:56 | |
| *** dolphm is now known as dolphm_503 | 04:04 | |
| *** bvandenh has joined #openstack-keystone | 04:26 | |
| *** stevemar has joined #openstack-keystone | 04:53 | |
| *** ChanServ sets mode: +v stevemar | 04:53 | |
| *** bvandenh has quit IRC | 04:58 | |
| *** dolphm_503 is now known as dolphm | 05:05 | |
| *** dolphm is now known as dolphm_503 | 05:14 | |
| *** theocean154 has joined #openstack-keystone | 05:50 | |
| *** gokrokve has joined #openstack-keystone | 05:55 | |
| *** stevemar has quit IRC | 06:22 | |
| *** saju_m has joined #openstack-keystone | 06:31 | |
| *** topol has quit IRC | 06:45 | |
| *** chandankumar_ has quit IRC | 06:45 | |
| *** chandan_kumar has joined #openstack-keystone | 06:51 | |
| *** chandan_kumar has quit IRC | 06:55 | |
| *** YorikSar has joined #openstack-keystone | 07:00 | |
| *** amerine_ is now known as amerine | 07:03 | |
| *** amerine has quit IRC | 07:08 | |
| *** amerine has joined #openstack-keystone | 07:12 | |
| *** YorikSar has quit IRC | 07:36 | |
| *** YorikSar has joined #openstack-keystone | 07:37 | |
| *** YorikSar has quit IRC | 07:41 | |
| *** YorikSar has joined #openstack-keystone | 07:42 | |
| *** gokrokve has quit IRC | 07:45 | |
| *** marekd|away is now known as marekd | 08:12 | |
| *** theocean154 has quit IRC | 08:12 | |
| *** gokrokve has joined #openstack-keystone | 08:16 | |
| *** gokrokve has quit IRC | 08:38 | |
| *** saju_m has quit IRC | 08:40 | |
| *** leseb has joined #openstack-keystone | 08:42 | |
| *** saju_m has joined #openstack-keystone | 08:43 | |
| *** saju_m has quit IRC | 09:02 | |
| *** chandan_kumar has joined #openstack-keystone | 09:03 | |
| *** theocean154 has joined #openstack-keystone | 09:06 | |
| *** leseb has quit IRC | 09:09 | |
| *** theocean154 has quit IRC | 09:11 | |
| *** gokrokve has joined #openstack-keystone | 09:16 | |
| *** gokrokve has quit IRC | 09:21 | |
| *** saju_m has joined #openstack-keystone | 09:34 | |
| *** leseb has joined #openstack-keystone | 09:38 | |
| *** leseb has quit IRC | 10:10 | |
| *** leseb has joined #openstack-keystone | 10:11 | |
| *** leseb has quit IRC | 10:15 | |
| *** gokrokve has joined #openstack-keystone | 10:17 | |
| *** gokrokve has quit IRC | 10:22 | |
| *** leseb has joined #openstack-keystone | 10:24 | |
| *** saju_m has quit IRC | 10:25 | |
| *** amichon has joined #openstack-keystone | 10:29 | |
| *** saju_m has joined #openstack-keystone | 10:42 | |
| *** david-lyle has quit IRC | 10:45 | |
| *** theocean154 has joined #openstack-keystone | 10:55 | |
| *** theocean154 has quit IRC | 10:59 | |
| *** saju_m has quit IRC | 11:11 | |
| *** gokrokve has joined #openstack-keystone | 11:17 | |
| *** gokrokve has quit IRC | 11:21 | |
| *** saju_m has joined #openstack-keystone | 11:25 | |
| *** amichon has quit IRC | 11:40 | |
| *** gokrokve has joined #openstack-keystone | 12:17 | |
| *** gokrokve has quit IRC | 12:22 | |
| *** theocean154 has joined #openstack-keystone | 12:43 | |
| *** theocean154 has quit IRC | 12:47 | |
| *** chandan_kumar has quit IRC | 12:48 | |
| *** chandan_kumar has joined #openstack-keystone | 12:49 | |
| *** saju_m has quit IRC | 12:52 | |
| *** orion195 has joined #openstack-keystone | 12:54 | |
| *** chandankumar_ has joined #openstack-keystone | 12:54 | |
| orion195 | hi guys, after integrating keystone with AD (only for identity and leaving SQL as assignment backend).... I cannot login anymore in the dashboard | 12:55 |
| orion195 | the auth_strategy setup is setup as : keystone | 12:55 |
| orion195 | and everytime I try to access the /dashboard I receive this: | 12:56 |
| orion195 | TypeError: unsupported operand type(s) for +: 'NoneType' and 'str' | 12:56 |
| orion195 | http://ur1.ca/gqnjv | 12:56 |
| *** chandan_kumar has quit IRC | 12:57 | |
| *** topol has joined #openstack-keystone | 12:59 | |
| orion195 | anyway, a question more related with keystone: | 13:05 |
| orion195 | if I setup the identity on LDAP and Assignment in SQL | 13:05 |
| orion195 | if I list the users: keystone user-list, should I see the admin user? | 13:05 |
| *** dolphm_503 is now known as dolphm | 13:07 | |
| dstanek | orion195: i'm not familiar with Horizon's code, but at first looking at your traceback i would guess that the auth url isn't specified | 13:08 |
| orion195 | the auth_url in neutron.conf file right? | 13:09 |
| dstanek | that's what it looks like because the tb is from neutron client | 13:09 |
| dstanek | or where ever it is supposed to get it from | 13:09 |
| orion195 | dstanek: the auth_url is setup now, but still the same | 13:11 |
| orion195 | dstanek: I always get this in the logs: WARNING keystone.common.wsgi [-] Authorization failed. Invalid user / password from | 13:12 |
| orion195 | the question is: the admin_user in neutron.conf, is neutron right? | 13:12 |
| orion195 | (following the default conf) | 13:12 |
| orion195 | maybe I misunderstood what user to use, quite odd | 13:13 |
| orion195 | or, let's put the question in another way: the user I use for the ldap search wthin the ldap tree, should be used anywhere else then the keystone.conf file? | 13:14 |
| dstanek | orion195: i would assume not, but i'm not the greatest source of information for LDAP | 13:16 |
| dstanek | your talking about the LDAP user used to query and not a keystone user right? | 13:17 |
| orion195 | the LDAP user, right | 13:17 |
| orion195 | but, the keystone user, is the admn user, right? | 13:17 |
| orion195 | admin | 13:17 |
| *** gokrokve has joined #openstack-keystone | 13:19 | |
| *** gokrokve has quit IRC | 13:23 | |
| *** wchrisj has joined #openstack-keystone | 13:37 | |
| dstanek | orion195: are you talking about using the admin_token? | 13:39 |
| *** wchrisj has quit IRC | 13:42 | |
| *** jagee has joined #openstack-keystone | 13:47 | |
| *** lbragstad has quit IRC | 13:47 | |
| *** achampion has quit IRC | 13:49 | |
| orion195 | dstanek: "User .. is unauthorized for tenant admin", "code": 401, "title": "Unauthorized"}} | 13:51 |
| orion195 | this is the error I'm getting | 13:51 |
| orion195 | my user if provided by the LDAP | 13:51 |
| orion195 | I don't really get the problem | 13:53 |
| orion195 | my user is authorized on the admin tenant | 13:54 |
| orion195 | if I: keystone user-role-list --tenant-id=admin --user-id= | 13:54 |
| orion195 | I do see my user being part of: admin and _member_ | 13:54 |
| orion195 | (I'm now using the admin_token to query the keystone) | 13:54 |
| mhu | orion195, do you have access to the keystone logs ? | 13:55 |
| *** stevemar has joined #openstack-keystone | 13:55 | |
| *** ChanServ sets mode: +v stevemar | 13:55 | |
| orion195 | but, somehow, the nova client and neutron client, while authenticating with my credentials..... it doesn't work | 13:55 |
| orion195 | mhu: I do have indeed | 13:55 |
| *** koolhead17 has quit IRC | 13:57 | |
| orion195 | mhu: http://fpaste.org/81849/ | 13:58 |
| mhu | orion195, gs is the user id ? | 13:59 |
| orion195 | it's my full name | 13:59 |
| orion195 | that I reduced to my initials | 14:00 |
| orion195 | and this is provided out of LDAP | 14:00 |
| orion195 | in LDAP I only have USERS, I hav sql to perform assignments | 14:00 |
| mhu | orion195, can I see your [ldap] section in keystone.conf ? specifically the user_id mapping | 14:01 |
| orion195 | yes | 14:01 |
| orion195 | just a sec | 14:01 |
| orion195 | user_filter section? | 14:02 |
| mhu | user_id_attribute | 14:02 |
| *** koolhead17 has joined #openstack-keystone | 14:03 | |
| mhu | I think you might be hitting this: https://bugs.launchpad.net/keystone/+bug/1254849 | 14:03 |
| orion195 | user_id_attribute = cn | 14:05 |
| mhu | ah, nevermind, it should not be that | 14:06 |
| orion195 | mhu: if I change it to use sAMAccountName | 14:08 |
| orion195 | it doesn't find my username anymore | 14:08 |
| orion195 | mhu: another question | 14:12 |
| orion195 | the admin tenant | 14:12 |
| *** browne has joined #openstack-keystone | 14:12 | |
| orion195 | is something should be setup in the LDAP or in the SQL? | 14:13 |
| orion195 | can it be that somehow there is a 'misunderstanding' within ldap and/or mysql? | 14:13 |
| orion195 | I don't know, the story here is that my user, has roles on the admin tenant (if I check with the admin_token). If I do the same with my user... nothing works so.. I don't know where to look for | 14:14 |
| *** gokrokve has joined #openstack-keystone | 14:17 | |
| ayoung | orion195, samAccountName is an open bug already | 14:18 |
| ayoung | there are workarounds. | 14:19 |
| orion195 | ayoung, thank you for telling me this, unfortunately I didn't find any workaround | 14:20 |
| *** wchrisj has joined #openstack-keystone | 14:20 | |
| orion195 | and most important: I still don't know the root cause about this | 14:21 |
| *** gokrokve has quit IRC | 14:22 | |
| *** lbragstad has joined #openstack-keystone | 14:23 | |
| ayoung | orion195, Blackest of magic. Best to blame Microsoft | 14:27 |
| mhu | orion195, if you can, run keystone with debug logging activated, and try to authenticate with your user. The logged LDAP search and bind might hold some answers | 14:28 |
| orion195 | mhu: I may have found something | 14:30 |
| orion195 | keystone --os-auth-url http://10.139.24.3:5000/v2.0 --os-username username user-list | 14:31 |
| orion195 | "metadata": {"is_admin": 0, "roles": []}}}, | 14:31 |
| *** theocean154 has joined #openstack-keystone | 14:31 | |
| orion195 | I would say that something is wrong with the DB | 14:31 |
| dolphm | orion195: the metadata block is garbage, but you're not going to get authorization without specifying a tenant there | 14:32 |
| *** achampion has joined #openstack-keystone | 14:32 | |
| dolphm | orion195: --os-project-name=admin for example | 14:33 |
| orion195 | eystone --os-auth-url http://10.139.24.3:5000/v2.0 --os-tenant-name amdin --os-username myuser tenant-list | 14:34 |
| orion195 | I do see everything | 14:34 |
| orion195 | it can be somehow that the neutron-user-password is not somehow syncronized? | 14:35 |
| *** theocean154 has quit IRC | 14:35 | |
| orion195 | I mean the only error I have is with the network component | 14:35 |
| orion195 | is there a way how I can check is a service_user authenticate correctly? | 14:36 |
| orion195 | s/is/if | 14:43 |
| *** wchrisj has quit IRC | 14:45 | |
| orion195 | thank you guys for your help | 14:45 |
| orion195 | the service account user was blocked in the Active Directory | 14:45 |
| orion195 | damned | 14:45 |
| ayoung | dolphm, got a moment to talk about henrynash's patch? | 14:51 |
| ayoung | https://review.openstack.org/#/c/74214/ | 14:51 |
| *** wchrisj has joined #openstack-keystone | 14:53 | |
| ayoung | dolphm, I can see your point on Keystone's role as the mapping point for other IdPs (including LDAP) to Openstack. I think the biggest mistake there is that it brute forces the mapping, what would be much simpler and les fragile if done via a calculation | 14:54 |
| *** leseb has quit IRC | 14:57 | |
| *** leseb has joined #openstack-keystone | 14:59 | |
| *** richm has joined #openstack-keystone | 15:09 | |
| richm | When reviewers comment on a commit, and I reply to those comments, do the reviewers get notified? | 15:10 |
| lbragstad | yes | 15:11 |
| ayoung | richm, email notifications are enabled. | 15:13 |
| *** gokrokve has joined #openstack-keystone | 15:17 | |
| richm | ok | 15:20 |
| *** gokrokve has quit IRC | 15:21 | |
| *** leseb has quit IRC | 15:25 | |
| *** gokrokve has joined #openstack-keystone | 15:26 | |
| *** nkinder has joined #openstack-keystone | 15:36 | |
| dstanek | ayoung: i'm a little worried about the memory impact of the RevokeTree with a ton of events | 15:36 |
| ayoung | dstanek, it should be pretty efficient | 15:36 |
| ayoung | but...we could generate a test | 15:36 |
| *** david-lyle has joined #openstack-keystone | 15:37 | |
| dolphm | ayoung: i haven't looked at the review since last week - when you say "brute forces the mapping", is it iterating through all backends until it finds something? | 15:37 |
| ayoung | dstanek, the old way? Yeah | 15:37 |
| ayoung | linear search | 15:37 |
| ayoung | dolphm, sorry, yeah, lienar search, and since the item is likely not to be found, it will be an expected case | 15:38 |
| ayoung | dstanek, if there are N events, the memory impact should be under O(2N) | 15:38 |
| ayoung | N for the current events list, and then a handful of extra values for the extra keys in the tree | 15:39 |
| ayoung | dstanek, but it would be much smaller than revoking by "ID" in the case where a single user has thousands of tokens...which is something our QA is testing | 15:39 |
| ayoung | IN general, it should minimize the number of events | 15:40 |
| dolphm | ayoung: that doesn't sound like the solution we talked about last week then | 15:40 |
| ayoung | dolphm, "Brute force" was the precursor to the tree | 15:40 |
| dolphm | ayoung: i'm talking about henry's patch, not yours | 15:40 |
| ayoung | it was what I was doing in Patch-1-of-3 | 15:40 |
| ayoung | dolphm, ah... | 15:40 |
| ayoung | dolphm, brute force is the creating a new UUID | 15:41 |
| ayoung | and recording it | 15:41 |
| dolphm | ayoung: dstanek is asking you about your patch :P | 15:41 |
| ayoung | dolphm, 'salright, I think I can keep those two clear | 15:41 |
| ayoung | I hope | 15:41 |
| *** chandan_kumar has joined #openstack-keystone | 15:41 | |
| ayoung | dolphm I want to avoid the ephemeral mapping table. Even if we were to do ephemeral users, I would not put them in a mapping table like that, but that is a different topic. | 15:43 |
| ayoung | Lets not make ephemeral users. | 15:43 |
| ayoung | So, the thought process went like this | 15:43 |
| ayoung | We need to deconflict Ids from different IdPs. | 15:43 |
| dolphm | ayoung: henry's solution shouldn't be implementing ephemeral users | 15:44 |
| dolphm | ayoung: i'll check out the review later today -- i want to focus on the two remaining bp's first | 15:44 |
| ayoung | dolphm, the mapping table is bascially an ephemeral ID | 15:45 |
| ayoung | dolphm, with all the same drawbacks. | 15:45 |
| ayoung | dolphm, I have feedback from jamielennox and lbragstad on the Revocaton review that I am working through now, but if you are revisitng, I will hold off on reposting until I can address your comments, and dstanek 's as well | 15:46 |
| dolphm | ayoung: i haven't started yours this morning yet - i'm looking at remaining_uses first | 15:46 |
| ayoung | last comment was on revoke, not henry;s | 15:46 |
| dolphm | ayoung: go ahead with a rev | 15:46 |
| ayoung | cool | 15:46 |
| ayoung | is remaining_uses close? | 15:46 |
| dolphm | ayoung: i'm about halfway through and only have nits so far | 15:47 |
| lbragstad | remaing_uses is looking pretty good | 15:47 |
| dstanek | ayoung: i reviewed revocation this morning - which is why i started thinking about this in memory cache | 15:47 |
| dolphm | ayoung: mostly in tests | 15:47 |
| lbragstad | remaining* | 15:47 |
| ayoung | ++ | 15:47 |
| *** leseb has joined #openstack-keystone | 15:47 | |
| ayoung | dstanek, the thing is, most of the use cases that would generate a huge number of revocations in the past should be caught by som form of collection (project, domain) with the exception of Groups.... | 15:48 |
| ayoung | we can optimize for gropus in the future, too, I think, once we have a clearer query mechanism for them | 15:48 |
| dolphm | dstanek: side note -- i ran into problems with testr last week returning SUCCESS as a false positive when specifying a specific class to test | 15:49 |
| *** henrynash has joined #openstack-keystone | 15:49 | |
| dolphm | dstanek: i.e. testr run keystone.tests.test_module:TestClass would return SUCCESS repeatedly with obviously broken tests | 15:50 |
| dstanek | dolphm: really? that's really bad | 15:50 |
| dolphm | dstanek: yeah -- i couldn't reproduce consistently though, and a complete run (testr run) always seemed to behave correctly | 15:51 |
| lbragstad | dolphm: dstanek I've hit that too.. I usually just keep backing up a module/dir until it starts working | 15:51 |
| ayoung | I think it is the : versys . for specifying the Class. Was it actually running the test, or just returning SUCCESS 0 test run? | 15:52 |
| ayoung | dstanek, do we have a "Token Expired" exception? I think it is not a separate exception | 15:54 |
| dstanek | dolphm, lbragstad: i tend to use nosetests still - it's more reliable in general for me | 15:54 |
| dolphm | ayoung: are you not supposed to use the colon? it appeared to be executing the correct class when it did work | 15:55 |
| ayoung | dstanek, I could just return an Unauthorized when the token times out, but Not found seems to be in keeping with what we are doing | 15:56 |
| ayoung | dolphm, I don | 15:56 |
| ayoung | 't know. I've been running using 3 or 4 different methods, and one of the IDEs uses a test running that does the : and one does . | 15:56 |
| dolphm | ayoung: raise not found from the backend, and recast as 401 if emitting in response to auth | 15:56 |
| dolphm | ... which is usually/always the case for tokens | 15:56 |
| ayoung | dolphm, agree that is what I am doing... | 15:56 |
| ayoung | https://review.openstack.org/#/c/55908/71/keystone/token/provider.py | 15:57 |
| dstanek | ayoung: what code are you talking about? re:Unauthorized vs. NotFound | 15:58 |
| ayoung | dstanek, your comment on https://review.openstack.org/#/c/55908/71/keystone/token/provider.py | 15:59 |
| dstanek | ayoung: that was lbragstad | 15:59 |
| ayoung | dstanek, ah... | 16:00 |
| ayoung | sorry | 16:00 |
| ayoung | dolphm, are we supposed to remove copyright heads on files like test_v3_auth.py that were pre-existing: Copyright 2012 OpenStack Foundation | 16:03 |
| *** chandan_kumar has quit IRC | 16:03 | |
| henrynash | ayoung, dolphm: so we seem to have a little impasse here on the multi-backend drivers….ayoung I know you are totally against the mapping scheme, and Dolphm, I think you are the opposite… | 16:04 |
| ayoung | henrynash, no, we are close...I think the mapping should be "calculated not recorded" | 16:04 |
| ayoung | the recorded approach gives us all of the problems with ephemeral users | 16:04 |
| henrynash | ayoung: and by recorded you mean (since it has to be stored somewhere) stored in the ID field itself | 16:05 |
| ayoung | henrynash, the mapping table itself, yes. When I wrote the LDAP code, I had to decide whether to do just that. I would have stored the user in the USER Table" with a lookup to LDAP..its bad practice | 16:06 |
| henrynash | ayoung: so we are still recording it….it is just whether we keep a separate table or store it inside the ID….. | 16:06 |
| ayoung | you don't want to force two systems to be consistent in order to keep from losing data. With the approach of: append domain-specific-somethjing to LDAP-specific-somehting we can always regenerate the origianl intent | 16:07 |
| ayoung | henrynash, I would propose this: | 16:07 |
| dolphm | mhu: i just left a bunch of comments on https://review.openstack.org/#/c/56243/ -- i'd be happy to address some of them if you don't mind | 16:07 |
| dolphm | mhu: (unless you have another revision in the works anyway) | 16:07 |
| henrynash | ayoung: (so remember I pushed for storing it in the ID field too, so you don't need to convince me of the issues there)….and I proposed using the domain_index idea to the dev list….. | 16:08 |
| ayoung | for LDAP userid@@domain_suffix. domain suffix is calculated from the first 6 chars of the domain ID, the way GIT hashes are done. If we are worried about collisions, we can recrod the domain suffix in a table with a unique constraint. But we stick by the rule that userid cannot be longer than 64 chars | 16:08 |
| ayoung | henrynash, I've tracked the conversation. | 16:10 |
| dolphm | henrynash: i haven't done a code review on your patch since last week, nor read ayoung's argument(s) against the current implementation yet | 16:10 |
| *** thedodd has joined #openstack-keystone | 16:10 | |
| henrynash | ayoung: I think the issue is more fundamental - i.e. whether (as per my comment on the patch) keystone's role is to provide a normalised/sanitized representation of identity (from wherever it has come) to its clients (and providing it in a way that makes their life as easy as possible), or to intervene as little as possible and not worry about exposing details of the raw identity…. | 16:12 |
| henrynash | ayoung: that is the issue being argued on the list (and by others, e.g. dolphm) | 16:13 |
| ayoung | henrynash, keystone was originaly stated to have the goal of being a thin wrapper around exisiting Identity Providers. | 16:14 |
| mhu | dolphm, I am currently adding tests to test_sql_upgrade as suggested by bknudson, turns out there were some errors in the upgrade script | 16:15 |
| mhu | dolphm, I'll check your comments, thx | 16:16 |
| ayoung | henrynash, I think my biggest complaint is that if either the LDAP or the Keystone database got corrupted, we would have no way of reproducing the ownership information. Keystone here would be adding an additional degree of risk. | 16:16 |
| dolphm | mhu: thank you! | 16:16 |
| *** chandan_kumar has joined #openstack-keystone | 16:16 | |
| ayoung | henrynash, however...I think we can state that there is room for both approaches. However, the mapping approach would be second implemented, and would be done via the existing user table, not a separate mapping table. | 16:16 |
| dolphm | mhu: free diff: | 16:16 |
| dolphm | mhu: curl http://pasteraw.com/30qh2an3u7w9qc4119qgzmjn164kvsy | git apply | 16:16 |
| henrynash | ayoung: not sure about the LDAP corruption, but certainly if we lose the keystone DB, then there would be no way to recreate the correct IDs for ownership, I agree | 16:17 |
| ayoung | the first implementation should be the suffix | 16:17 |
| mhu | dolphm, sweet ! thx | 16:17 |
| dolphm | ayoung: why does keystone need to solve for ldap / sql backend corruption...? | 16:18 |
| ayoung | dolphm, not my point. My point is that it should not increase the synchronization burden. I was using the corruption as an example (an extreme one) of how that kind of duplication can be trouble. | 16:19 |
| *** theocean154 has joined #openstack-keystone | 16:19 | |
| dolphm | alright i'm going to review the patch so i have some idea of what you're talking about when you say "synchronization"... | 16:20 |
| *** marekd is now known as marekd|away | 16:23 | |
| *** theocean154 has quit IRC | 16:23 | |
| *** leseb has quit IRC | 16:25 | |
| *** leseb has joined #openstack-keystone | 16:26 | |
| ayoung | dstanek, I made class TokenAPITests(object): to keep it from getting called directly. If it descended from TestCase, the tests in it would get run. That is how the test_backend code works. The difference is that the setUp here needs to be called by all subclasses | 16:29 |
| ayoung | https://review.openstack.org/#/c/55908/71/keystone/tests/test_v3_auth.py | 16:29 |
| ayoung | Is there some other way, so that I can just make the mixin class implement setUp? I don;t see one. | 16:29 |
| *** leseb has quit IRC | 16:30 | |
| *** bknudson has quit IRC | 16:31 | |
| henrynash | anyone know anything about jenkins failing with ImportError: cannot import name ansisql on keystone patches…is that just me? | 16:34 |
| *** dims_ has joined #openstack-keystone | 16:37 | |
| *** marcoemorais has joined #openstack-keystone | 16:37 | |
| dolphm | henrynash: link? | 16:37 |
| henrynash | dolphm; http://logs.openstack.org/14/74214/19/check/gate-keystone-python26/cbec744/console.html | 16:38 |
| *** gokrokve has quit IRC | 16:39 | |
| *** bknudson has joined #openstack-keystone | 16:39 | |
| *** marcoemorais has quit IRC | 16:40 | |
| dstanek | ayoung: if everything is calling super() does your setUp not get called? | 16:40 |
| dstanek | it shouldn't matter what the parent class is | 16:40 |
| ayoung | dstanek, as I recall I was getting an error that there was no super. | 16:40 |
| ayoung | Let me try again...maybe it was an artifact of something else I was doing. | 16:41 |
| dstanek | ayoung: just looking at it i would thing renaming doSetUp to setUp and removing the empty setUps would work | 16:41 |
| ayoung | dstanek, testing.. | 16:42 |
| *** devlaps has joined #openstack-keystone | 16:48 | |
| ayoung | dstanek, setUp in the non-test baseclass is getting skipped | 16:50 |
| dstanek | ayoung: hmmm, that's odd. i'll dig in once i finish this review i'm looking at now | 16:52 |
| ayoung | dstanek, in testtools.testcase it does a self.setUp() and that calls directly into the setUp function in test_v3 | 16:52 |
| bknudson | http://python-history.blogspot.com/2010/06/method-resolution-order.html | 16:52 |
| ayoung | dstanek, yeah, I was troubled by it as well. | 16:52 |
| bknudson | " when looking up a method, base classes were searched using a simple depth-first left-to-right scheme" | 16:52 |
| bknudson | "The first matching object found during this search would be returned." | 16:52 |
| ayoung | bknudson, that is kindof what I figured. | 16:53 |
| ayoung | and why I had to avoid putting the setUp function outside the hierarchy | 16:53 |
| ayoung | the testcase hierarchy | 16:53 |
| bknudson | ayoung: makes sense to me. Not the prettiest, but seems necessary. | 16:54 |
| ayoung | bknudson, yeah...its due to the depth of the hierarchy here | 16:54 |
| bknudson | sometime we need to do a cleanup of the tests... I think dstanek had some things written down. | 16:55 |
| bknudson | here's an example of problems: https://review.openstack.org/#/c/77438/ | 16:56 |
| ayoung | bknudson, yeah. I think it comes down to "favor composition over inheritance" here. We are counting too much on the hierarchy for setting up tests, and then putting in one offs to work around places where the assumptions are right for all tests but "just this one" | 16:57 |
| *** gokrokve has joined #openstack-keystone | 16:58 | |
| bknudson | ayoung: exactly | 16:58 |
| *** gyee has joined #openstack-keystone | 17:05 | |
| *** thedodd has quit IRC | 17:10 | |
| *** gokrokve has quit IRC | 17:14 | |
| *** thedodd has joined #openstack-keystone | 17:14 | |
| *** gokrokve has joined #openstack-keystone | 17:17 | |
| *** leseb has joined #openstack-keystone | 17:17 | |
| *** gokrokve has quit IRC | 17:20 | |
| *** theocean154 has joined #openstack-keystone | 17:22 | |
| *** chandan_kumar has quit IRC | 17:25 | |
| ayoung | bknudson, https://review.openstack.org/#/c/55908/71/keystone/contrib/revoke/migrate_repo/versions/001_revoke_table.py Jamie suggests a descending index. However, I don't want to predefine the index name | 17:27 |
| ayoung | I can do sql.Column('revoked_at', sql.DateTime(), index=True, nullable=False)) | 17:27 |
| ayoung | is that sufficient, or do I need to do | 17:28 |
| ayoung | like the example says, Index('someindex', mytable.c.somecol.desc()) | 17:28 |
| ayoung | http://docs.sqlalchemy.org/en/latest/core/constraints.html#functional-indexes | 17:28 |
| bknudson | ayoung: why don't you want to predefine the index name? | 17:29 |
| ayoung | bknudson, because that is one thing that is different for each RDBMS | 17:29 |
| *** amcrn has quit IRC | 17:29 | |
| ayoung | I'd rather not wade into that particular nastiness. Defining an index on MySQL that looks like Postgres one, for example | 17:30 |
| ayoung | question is whether a descending index is required here, too? | 17:30 |
| bknudson | I don't think it makes much of a difference if it's ascending/descending... the database can walk through the index either forwards or backwards. | 17:31 |
| bknudson | unless you're storing your database on a tape drive. | 17:31 |
| *** orion195 has quit IRC | 17:31 | |
| bknudson | I never trust my guesses on whether indexes will be used or not though... I usually use the query explainer to tell me what affect it has. | 17:32 |
| ayoung | I'm going to leave it as just an index for now. | 17:33 |
| ayoung | Keeps the code cleaner | 17:33 |
| ayoung | OK. so we broken the live migration tests again. I'm guessing this happend back when we refactored the test code into the fixtures... | 17:42 |
| ayoung | which means that this code has not been run in quite some time....something we need to fix | 17:42 |
| ayoung | We can't force people to run the DB test, but running the mysql and LDAP tests should be required before any database change checkin. I know, I'm guilty here, too | 17:43 |
| *** harlowja_away is now known as harlowja | 17:46 | |
| *** harlowja is now known as harlowja_away | 17:47 | |
| *** harlowja_away is now known as harlowja | 17:50 | |
| *** rwsu has joined #openstack-keystone | 17:55 | |
| *** marcoemorais has joined #openstack-keystone | 17:58 | |
| dolphm | ayoung: tests that aren't run in some automated fashion are hardly useful... i'd suggest you actually build CI around those tests if you care about them at all. as-is, they don't really belong in-tree | 17:59 |
| ayoung | dolphm, yeah...question is what that should look like. Its been on my todo list for a while | 17:59 |
| dolphm | ayoung: it should look just like grenade | 18:00 |
| *** jsavak has joined #openstack-keystone | 18:00 | |
| ayoung | live LDAP and live sql. The migrations are tested for SQL via grenade, just not to the same degree as our own tests | 18:00 |
| ayoung | liveldap is a different beast | 18:00 |
| ayoung | I need to learn about the whole "Run this setup once per suite" function | 18:01 |
| ayoung | is that the setup_tests function, I am assuming? | 18:02 |
| *** gyee has quit IRC | 18:03 | |
| *** arborism has joined #openstack-keystone | 18:10 | |
| *** arborism is now known as amcrn | 18:10 | |
| ayoung | and..it was my own fault for not clearing out the keystone_test database... | 18:16 |
| dolphm | ayoung: how has the revocation patch doubled in size in the last week? | 18:19 |
| dolphm | it's also tracking against a second blueprint now? | 18:22 |
| ayoung | dolphm, the revocation patch was merged with the two folow on patches: 1 for the sql backend and one for "efficient revocation checking" but those were both in the last version you reviewed | 18:33 |
| lbragstad | stevemar: ping? | 18:36 |
| stevemar | lbragstad, ahoy | 18:36 |
| lbragstad | qq here | 18:36 |
| lbragstad | https://review.openstack.org/#/c/77294/2/doc/source/api_curl_examples.rst | 18:36 |
| stevemar | i just had it open | 18:36 |
| lbragstad | does http://localhost:35357/v2.0/tokens have to https://? | 18:36 |
| lbragstad | ... https://localhost:35357/v2.0/tokens | 18:37 |
| *** gyee has joined #openstack-keystone | 18:38 | |
| lbragstad | should that be updated too if so? | 18:38 |
| dolphm | ayoung: start thinking about waiting for juno to land revocation events | 18:44 |
| ayoung | dolphm I figured it would come to that....I'm mixed. On the one hand, I've been working on it heads down for the past two months | 18:45 |
| ayoung | cutting it would be painful | 18:45 |
| dolphm | ayoung: understood and agree | 18:45 |
| ayoung | there is at least one use case I know of that needs it | 18:45 |
| stevemar | lbragstad, whats the rest of the doc like? | 18:45 |
| ayoung | which is driven by QA, but I think reflects a real world concern | 18:45 |
| stevemar | lbragstad, if it's http:// everywhere else, then i'm okay with updating it all to https:// on another patch | 18:45 |
| ayoung | dolphm, say a user generates a gazillion tokens, and then gets booted | 18:45 |
| ayoung | all of their tokens get revoked...it might be a memory/performance problem | 18:46 |
| ayoung | we've seen it in tests already | 18:46 |
| dolphm | ayoung: yep; that's one reason why it's high priority | 18:46 |
| *** amerine has left #openstack-keystone | 18:46 | |
| ayoung | I think the code is ready to go | 18:46 |
| *** theocean154 has quit IRC | 18:46 | |
| ayoung | there may be bugs...but the bones are solid | 18:46 |
| dolphm | ayoung: it's clear the tests were an after thought... which shouldn't be the case for a security feature | 18:47 |
| lbragstad | stevemar: looks like it's http:// everywhere... so probably done in another patch | 18:47 |
| stevemar | lbragstad, cool, mention that in the change then | 18:47 |
| *** morganfainberg_Z is now known as morganfainberg | 18:47 | |
| ayoung | dolphm, hmmm....the thinking was that the existing tests should be sufficient if we replace the old mechanism with the new... | 18:48 |
| ayoung | I missed that for the V2 api until this weekend.... | 18:48 |
| lbragstad | stevemar: thanks, done and done | 18:48 |
| dolphm | ayoung: you're implementing a new API, and i simply can't read the tests and write a client against that API | 18:48 |
| ayoung | dolphm, the new API is not really the case here, though. It is what happens with the tokens internally that is a security question for the server. | 18:49 |
| ayoung | hmmm | 18:49 |
| ayoung | dolphm, I see what you are saying...on the client side | 18:49 |
| ayoung | we need to know that any token that would have been revoked under the old system would be revoked under the new, but the code to test it remotely needs to be ported to keystoneclient | 18:50 |
| ayoung | But that is why so much of the testing effort went in to test_v3_auth | 18:51 |
| ayoung | the rules should apply the same local as remote. | 18:51 |
| dolphm | ayoung: the tests in test_v3_auth are a great start, but they don't provide sufficient coverage over the /events response itself | 18:52 |
| dolphm | ayoung: self.assertEqual(events_response, {'events': [{'domain_id': deleted_domain_id}]}) | 18:53 |
| stevemar | lbragstad, np boss, now to return the favor, muahahaha https://review.openstack.org/#/c/60820/12 | 18:53 |
| dolphm | ayoung: that's all you need for really strong coverage ^ just do something like that in every test, and dump the "domain in list" checks | 18:53 |
| lbragstad | stevemar: I was just about to start looking at that... I gave it a quick once over and it was looking good | 18:54 |
| *** leseb has quit IRC | 18:54 | |
| ayoung | dolphm, interesting.... | 18:54 |
| dolphm | ayoung: i think dates would be the only gotcha (?) | 18:55 |
| ayoung | dolphm, would making "domain in list" check the format of the result also be acceptable? I could do the same kind of check where the date is sandwiched between the test run start and check times | 18:55 |
| dolphm | maybe use the assertCloseEnoughForGovernmentWork and then drop dates from the response | 18:55 |
| dolphm | ayoung: at minimum, it *does* need to check the format of the result | 18:56 |
| ayoung | OK...let me see if I can do that with a minimum of refactoring | 18:56 |
| *** leseb has joined #openstack-keystone | 19:02 | |
| lbragstad | stevemar: had a couple questions, but it's looking pretty good. | 19:09 |
| stevemar | lbragstad, question away | 19:10 |
| lbragstad | I published them in the review, not sure about the client, but do messages have to be wrapped in _()? | 19:10 |
| stevemar | lbragstad, replied | 19:12 |
| stevemar | lbragstad, https://github.com/openstack/python-keystoneclient/blob/master/keystoneclient/v3/roles.py#L61 we don't translate on client side | 19:13 |
| lbragstad | good to know | 19:13 |
| morganfainberg | dolphm, ping - I'm going to propose a schema collapse post I3 if there is no complaints about it | 19:23 |
| morganfainberg | dolphm, so I migration repo would assume you're on H already (036) | 19:24 |
| dolphm | morganfainberg: awesome! | 19:30 |
| dolphm | morganfainberg: do you want to push this to RC1 or juno? https://bugs.launchpad.net/keystone/+bug/1282229 | 19:32 |
| morganfainberg | dolphm, Lets target it to J. We've lived with it long enough now. | 19:32 |
| morganfainberg | dolphm, if i can squeeze it in (fix) before RC1, we can evaluate it | 19:32 |
| dolphm | morganfainberg: i won't hold my breath :P | 19:33 |
| morganfainberg | dolphm, ideally we should have it fixed for I, but I've heard of 0 complaints from users about it, therefore it must be a smallllll surface area of issue | 19:33 |
| dolphm | morganfainberg: yeah... | 19:33 |
| dolphm | morganfainberg: i want to make sure this lands before i3 https://review.openstack.org/#/c/73026/ | 19:38 |
| morganfainberg | dolphm, ++ | 19:41 |
| morganfainberg | dolphm, looking at it now | 19:47 |
| morganfainberg | i'll plan to land the last helpstrings stuff post I3 | 19:47 |
| morganfainberg | want to prioritize the "important stuff" | 19:47 |
| dolphm | morganfainberg: brant also has a series of fixes for a regression since havana, starts here: https://review.openstack.org/#/c/77038/ | 19:49 |
| morganfainberg | dolphm, ok, on my list for today. | 19:49 |
| bknudson | dolphm: so those changes cover the "endpoints", but I think essentially the same thing has to happen for "services" | 19:51 |
| bknudson | so it would be nice to get an idea that the endpoints fix is the right way to do it | 19:51 |
| bknudson | and then I'd go on to services... should be easier to do services since I can essentially do the same as what was done for endpoints. | 19:52 |
| dolphm | bknudson: lgtm so far | 20:05 |
| dolphm | bknudson: and yeah, endpoints was just the one i ran into with openstackclient | 20:08 |
| morganfainberg | dolphm, yay we don't have broken pam identity anymore ;) | 20:17 |
| dolphm | morganfainberg: that's bugged me for a long time lol | 20:17 |
| morganfainberg | dolphm, i was going to fix it until i saw how broken it was | 20:17 |
| morganfainberg | and how long it had been broken | 20:19 |
| morganfainberg | bknudson, i'll fix the helpstrings for post I3 | 20:19 |
| morganfainberg | bknudson, no reason to make extra rebase work for the milestone | 20:20 |
| bknudson | morganfainberg: ok... I don't think there's rebase work unless someone was changing the same lines. | 20:20 |
| bknudson | at this point | 20:20 |
| morganfainberg | bknudson, right. but if someone somehow added sample config lines | 20:20 |
| morganfainberg | bknudson, i think ayoung's and henry-nash's bps are adding options | 20:21 |
| morganfainberg | bknudson, so we can wait a couple days for the last of the helpstring.s i agree with all of your comments btw | 20:21 |
| bknudson | they're going to conflict anyways | 20:21 |
| henrynash | bknudson: although those kind of rebases are pretty easy | 20:21 |
| bknudson | I think we've got 4 migration 41s. | 20:21 |
| henrynash | dolphm: trying to ensure I concentrate on the right things…..as far as you are concerned, with regard to multi-backends, is it mapping table or nothing? I was considering working on a version that encoded the domain into the ID (based on patch 14, but constrained to 64 bits)…so that we could compare…but if, as far as you are concerned, that's a no go…then not sure I want to waste time there…. | 20:24 |
| morganfainberg | henrynash, i'd really rather not do concat stuff personally | 20:25 |
| dolphm | henrynash: is there another viable solution? there doesn't seem to be support on list for the @@ alternative | 20:25 |
| morganfainberg | henrynash, i think that is a recipe for needing to support 3 forms of IDs in the long term | 20:25 |
| henrynash | dolphm: agreed…well, vish gave it limited support….but otherwise no | 20:26 |
| dolphm | henrynash: the encoding solution won't work within 64 bytes without truncating... it'll just run into the same problem we have now, only quicker | 20:27 |
| morganfainberg | i also like the idea that we should support a single data-type and style for user_ids if possible. | 20:27 |
| dolphm | henrynash: and i agree with vish to an extent, but maximum string length of our IDs should be definable if we expect anyone to use them | 20:27 |
| henrynash | dolphm: yes, the best you can do in 64 bits is something like 54chars if ID, 2 separators, 8 chars of domain_index | 20:27 |
| henrynash | dolphm: so that practical consequence would be a loss of 10 chars from the IDs used by LDAP | 20:28 |
| morganfainberg | henrynash, | 20:29 |
| dolphm | morganfainberg, | 20:29 |
| morganfainberg | yeah. that sounds about right | 20:29 |
| morganfainberg | dolphm, | 20:29 |
| henrynash | ,,,,, | 20:29 |
| morganfainberg | lol | 20:29 |
| dolphm | henrynash: am i missing anything in thinking that the sql driver could/should become domain-unaware first? | 20:31 |
| morganfainberg | dolphm, that is the one case i don't think we can do it | 20:31 |
| morganfainberg | dolphm, we support multiple domains in SQL and people use it | 20:31 |
| morganfainberg | dolphm, i agree though, i would prefer it to be domain unaware | 20:31 |
| bknudson | how can sql become domain-unaware? | 20:32 |
| dolphm | morganfainberg: i'm not proposing that we lose the ability to store multiple domains worth of identity information in sql... | 20:32 |
| bknudson | multiple sql backends? | 20:32 |
| dolphm | bknudson: sort of | 20:32 |
| morganfainberg | bknudson, that would be my choice. | 20:32 |
| henrynash | dolphm: well it doesn't have to…I agree it would make the code simpler, but since all we would do is use the exitsing user_id as the public_ID (since it is always a UUID), and encode the domain in the mapping table, we could do it later and it would be invisible to keystone clients | 20:32 |
| dolphm | bknudson: they could have the same connection string though | 20:32 |
| morganfainberg | dolphm, could they? wouldn't that create name conflicts? | 20:32 |
| dolphm | morganfainberg: ooh, there you go. we'd have to check for conflicts ourselves :( | 20:33 |
| bknudson | each sql backend would have its domain_id configured. | 20:33 |
| morganfainberg | and/or bleed users between domains | 20:33 |
| morganfainberg | hmm. | 20:33 |
| henrynash | morganfainberg: yes, as per my comment on the patch, we'd have to keep the name space ourselves (unless you can do constraints across tables easily?) | 20:33 |
| bknudson | then would just put that in the db table. | 20:33 |
| morganfainberg | bknudson, ah yeah doable | 20:34 |
| dolphm | domain_id could become an identity driver hint... and then only sql would use it | 20:34 |
| morganfainberg | dolphm, ++ | 20:34 |
| bknudson | where would the manager get domain_id? | 20:35 |
| morganfainberg | nkinder, ping - have an LDAP question might be able to answer for me. | 20:35 |
| morganfainberg | nkinder, regarding the maximum length of a DN. | 20:35 |
| nkinder | morganfainberg: you just want to know if there is an official limit? | 20:35 |
| dolphm | bknudson: i was thinking the manager wouldn't... instead... | 20:36 |
| morganfainberg | nkinder, less official, more implementation specific | 20:36 |
| morganfainberg | nkinder, could the first element of the DN "cn=<blah blah blah>" be > 64 bytes? | 20:36 |
| morganfainberg | well the part after 'cn=' for example | 20:36 |
| nkinder | morganfainberg: yes | 20:36 |
| morganfainberg | dolphm, ^ we need something to map user ids to a consumable value based upon that | 20:37 |
| dolphm | the controllers need a dictionary of identity_api references, indexed by domain_id; each identity manager could be instantiated with a domain_id so that it could be passed as a hint, but ultimately it would be controllers simply calling the right driver, e.g. self.identity_api[some_domain_id].some_operation() | 20:37 |
| bknudson | dolphm: so multiple managers. | 20:37 |
| morganfainberg | if a ldap id is > 64 bytes, we potentially can't use that DN object in assignment (sql based) | 20:38 |
| dolphm | bknudson: ++ | 20:38 |
| henrynash | dolphm: so today we just do that at the Manager level…i.e. we have a driver per specific domain | 20:38 |
| morganfainberg | which almost requires the map table. | 20:38 |
| henrynash | morganfainberg: we can't….doesn't work in practice (for the reasons you are expressing) | 20:38 |
| morganfainberg | henrynash, that argues we must implement a mapping table. | 20:38 |
| nkinder | morganfainberg: it's not common for it to be that large for user entries, but it's certainly possible. | 20:38 |
| henrynash | morganfainberg: I see email address used most often | 20:39 |
| morganfainberg | nkinder, correct. | 20:39 |
| dolphm | self.identity_api[self.id_mapping_api.get_domain_id(user_id)].get_user(self.id_mapping_api.get_internal_id(user_id)) | 20:39 |
| dolphm | sorry for length. | 20:39 |
| morganfainberg | nkinder, i hope no one does that | 20:39 |
| bknudson | dolphm: pep8 violation! | 20:39 |
| dolphm | # noqa | 20:39 |
| morganfainberg | dolphm, OMG eye bleeding violation | 20:39 |
| morganfainberg | dolphm, :P | 20:39 |
| morganfainberg | anyway | 20:40 |
| morganfainberg | dolphm, i'm not sure how to reasonably counter ayoung's valid complaints about the map table. | 20:40 |
| dolphm | henrynash: regarding "local_id" ... "local" might be LDAP, right? | 20:40 |
| morganfainberg | dolphm, it is more brittle than the id encoding the information in it. | 20:40 |
| dolphm | morganfainberg: define brittle in this case | 20:41 |
| *** leseb has quit IRC | 20:41 | |
| henrynash | dolphm, morganfainberg: so the question is how much of this do we attempt before i3 :-) The current patch would be (externally) ID compatible with the above controller/manager approach….but not sure if we want to try and go the whole way tonight (so to speak!) | 20:42 |
| dolphm | bknudson: morganfainberg: henrynash: PEP8'd https://gist.github.com/dolph/9334237 | 20:42 |
| morganfainberg | dolphm, it's an extra moving part that can get out of sync. | 20:42 |
| morganfainberg | dolphm, ayoung does make valid points in his -2 comment on henry's review | 20:43 |
| ayoung | henrynash, the limitation that the user_id < 65 chars has been there for a while. LDAP has to live with it now. Putting and additional restriction on it for the multie-LDAP case will not break thing for any existing users. emailaddres > 64 chars is...just not going to happend | 20:43 |
| dolphm | morganfainberg: i guess i don't have much in the way of expectations for "sync" | 20:43 |
| henrynash | ayoung: I don't disagree | 20:43 |
| bknudson | I'd think if someone really needed longer IDs they would have modified their schema. | 20:43 |
| morganfainberg | bknudson, lol don't disagree | 20:44 |
| morganfainberg | bknudson, s/lol// | 20:44 |
| ayoung | bknudson, more correct to say "chose a different value" | 20:44 |
| ayoung | differnt attribute actually | 20:44 |
| morganfainberg | ayoung, the more i think about it the more i _really_ dislike the @@ mechanism | 20:44 |
| morganfainberg | ayoung, but barring a map table, I don't have a good answer | 20:45 |
| henrynash | ayoung: my point was that worrying about DN length as the rationale for having to do mapping is probably not valid…since I doubt anyone will use DN as their ID in practice | 20:45 |
| ayoung | morganfainberg what is wrong with @@? Can you vocalize it | 20:45 |
| ayoung | henrynash, we discussed using DN at one point...I was firmly talked out of it | 20:45 |
| morganfainberg | ayoung, i very much dislike exposing that much of the identity internals to services outside of keystone. | 20:45 |
| ayoung | it *is* the LDAP way for deconflicting | 20:46 |
| ayoung | morganfainberg, they are going to make assumptions anyway | 20:46 |
| ayoung | like the code in the client that checked if an id was a uuid.... | 20:46 |
| ayoung | tossers | 20:46 |
| morganfainberg | ayoung, you can't guarantee the DN is de-conflicted | 20:46 |
| morganfainberg | ayoung, ldap server A could be bad and configured w/ the same base as b, and then we (keystone) can't trust it | 20:47 |
| ayoung | morganfainberg, you can't guarantee UUIDs are deconfliceted strictly speaking | 20:47 |
| ayoung | but, I agree | 20:47 |
| *** wchrisj has quit IRC | 20:47 | |
| ayoung | you need to salt the answer | 20:47 |
| morganfainberg | ayoung, also i think we are much much more likely to hit the 64 byte limit with the whole dn | 20:47 |
| morganfainberg | and the DN is not exactly url friendly | 20:47 |
| ayoung | DN is out. I agree there | 20:47 |
| morganfainberg | like i said, i don't have a good answer | 20:48 |
| ayoung | so this is a uniqueness scheme. We are not locked into it forever, but it is better than the Map table. | 20:49 |
| * dolphm continues dreaming of not leaking identity information into openstack | 20:49 | |
| ayoung | dolphm, lets discuss that, cuz it is the one thing that is, I think at the core of your support for this approach | 20:50 |
| ayoung | is it a problem if the ID has something about the user in it? | 20:50 |
| morganfainberg | dolphm, ++++++++++++++ | 20:50 |
| dolphm | ayoung: not really, it's in complete opposition to the whole argument and all solutions to it | 20:51 |
| ayoung | LDAP implies an internal call. But what about Federation? The solution for one should stand for both | 20:51 |
| dolphm | ayoung: as it stands, federation generates tokens with meaningless, deployer defined IDs | 20:51 |
| ayoung | dolphm, there is some risk there | 20:51 |
| ayoung | deployer defined can conflic | 20:51 |
| ayoung | t | 20:51 |
| ayoung | no? | 20:51 |
| dolphm | ayoung: absolutely | 20:51 |
| ayoung | so...either we map federated IDs, or we deconflict | 20:52 |
| ayoung | same with LDAP | 20:52 |
| dolphm | ayoung: if it were me, i'd configure my mapping to insert a static value there, like "ephemeral-user" | 20:52 |
| dolphm | ayoung: or "do-not-track" ;) | 20:52 |
| ayoung | and how long do we leave that record in place? And if that user comes back after the record has been deleted,then what? | 20:52 |
| dolphm | ayoung: good questions, but we don't need to answer them immediately | 20:53 |
| ayoung | THe issues are magnified with Federation | 20:53 |
| morganfainberg | ayoung, well with federated you auth w/ not the ID | 20:53 |
| dolphm | ayoung: i imagine the answer will look like token purging | 20:53 |
| ayoung | I want a single solution for both Federation and LDAP | 20:53 |
| morganfainberg | ayoung, if we used uuid5() we could guarantee the same id. | 20:53 |
| ayoung | morganfainberg, why not just make 10 louder? | 20:53 |
| morganfainberg | ayoung, we could remove the record if there are no assignment mapping(s) if we support assignment to federated users vs group | 20:54 |
| ayoung | morganfainberg, nope | 20:54 |
| ayoung | we don't have all the cards | 20:54 |
| morganfainberg | ayoung, or ignore it if you can't do an assignment to a federated user directly, which case we don't care | 20:54 |
| morganfainberg | ayoung, e.g. not "store" the ID. | 20:55 |
| ayoung | a user might be mapped via a group, and then there is some resource outside of Keystone that is associated with that user | 20:55 |
| ayoung | exactly...."Don't store the ID" is what I am shooting for here. | 20:55 |
| ayoung | but ephemeral users/mapping table stores the id | 20:55 |
| morganfainberg | you can't do that w/ LDAP | 20:55 |
| morganfainberg | you need to store the id | 20:55 |
| morganfainberg | so. | 20:55 |
| *** arunkant has joined #openstack-keystone | 20:56 | |
| ayoung | we need to salt the IDs that come from LDAP and Federated | 20:56 |
| ayoung | @@{salt} | 20:56 |
| morganfainberg | and i thought ephemeral users were mapped to groups based upon the attributes | 20:56 |
| morganfainberg | e.g. it's not "federated user id" -> group | 20:56 |
| morganfainberg | it's "user is part of group because it has attrs blah blah and blah"" | 20:56 |
| ayoung | morganfainberg, might be based on an attribute in the SAML assertion. Just cuz Red Hat's assertion doesn't provide useful groups doesn't mean none of them will | 20:57 |
| ayoung | we'll haveboth | 20:57 |
| morganfainberg | ayoung, i think you just described a need for a mechanism to store the ID | 20:57 |
| dolphm | morganfainberg: i'm playing with uuid5() since you mentioned it... what would be the namespace & name ? | 20:57 |
| morganfainberg | dolphm, namespace is the UUID.UUID4(<domain_id>) | 20:58 |
| morganfainberg | dolphm, name would be the other value (e.g. ldap, dn bit we use as Id today) | 20:58 |
| henrynash | morganfainberg: but isn't the problem with uuid5 that we can't get the namespace back? | 20:58 |
| morganfainberg | henrynash, no we can't un-do it. but we can ensure the id is always the same. | 20:58 |
| ayoung | morganfainberg, so one way hashes of the ID have exactly that problem | 20:58 |
| ayoung | but...maybe that is ok | 20:58 |
| dolphm | morganfainberg: can you send me a one liner? not sure i follow | 20:59 |
| morganfainberg | dolphm, dex | 20:59 |
| morganfainberg | sec* | 20:59 |
| ayoung | what if...we say that "if you need to do a backwards mapping, you can't, but we can reconfirm a forwards mapping" | 20:59 |
| dolphm | morganfainberg: that was an inexplicable typo | 20:59 |
| henrynash | morganfainberg: ie..given the resulting uuid5 value, we can't reverse it….but that's what we need to do….get the domain_id back from an ID so to know where to route the call | 20:59 |
| ayoung | IE, we couldn't get user by userid, but we could always confirm a given user has a specific userid | 21:00 |
| morganfainberg | >>> uuid.uuid5(uuid.UUID('c4729c137e3742788b38786ca52fa68d'), "something cool").hex | 21:00 |
| morganfainberg | 'eb3d4594887a5b0ca8cd93b43e41da32' | 21:00 |
| dolphm | ayoung: so you're proposing we avoid solving the problem of "what identity backend did this user come from?" | 21:00 |
| morganfainberg | dolphm, you'd reconstruct the UUID object as the namespace | 21:00 |
| ayoung | dolphm, yep...just chewing on it | 21:00 |
| morganfainberg | henrynash, we'd still need a map table | 21:00 |
| henrynash | morganfainger:, ok | 21:00 |
| henrynash | oops | 21:01 |
| morganfainberg | henrynash, nice! | 21:01 |
| dolphm | morganfainberg: interesting... | 21:01 |
| ayoung | dolphm, so long as mapped attribute->userid is always unique and reproducible... | 21:01 |
| morganfainberg | ayoung, that was my thought. | 21:01 |
| morganfainberg | ayoung, it is sha1, so there is potential for conflict... | 21:01 |
| ayoung | so if I want to assign to ayoung@@redhat versus ayoung@@rackspace I get a uniqe value | 21:02 |
| morganfainberg | dolphm, we could use hashlib if we wanted instead. | 21:02 |
| ayoung | sha256 | 21:02 |
| dolphm | ayoung: why 256? | 21:02 |
| morganfainberg | ayoung, uuid5 is sha1 iirc | 21:02 |
| morganfainberg | ayoung, but we could just use hashlib, same net effect | 21:02 |
| ayoung | dolphm, as I recall it was recommended...I'd have to dig | 21:02 |
| morganfainberg | ayoung, dolphm, if we are rolling our own id generator, use a better hashing algo than sha1, if we are usign a tool like uuid5, we use sha1 | 21:03 |
| morganfainberg | orwhatever iut uses | 21:03 |
| morganfainberg | it* | 21:03 |
| ayoung | dolphm, dstanek morganfainberg jamielennox I'm going to submit too versions of my code for review...the first will be just a rebased version of the existing patch, the second will have my changes in it. It should make it easier to review... | 21:03 |
| morganfainberg | ayoung, a .. chain? or two patchsets? | 21:04 |
| ayoung | no, two reviews | 21:04 |
| ayoung | same changeset | 21:04 |
| morganfainberg | ayoung, oh | 21:04 |
| morganfainberg | ok | 21:04 |
| ayoung | just one will be the old version rebased | 21:04 |
| morganfainberg | i see | 21:04 |
| ayoung | I can't just push "rebase" due to minor changes that have happend outside of my code. | 21:05 |
| dolphm | morganfainberg: sha256 produces 32 *bytes* / 64 chars | 21:05 |
| dolphm | ayoung: the patch is so big that i have yet to go through the entire thing at once anyway | 21:05 |
| ayoung | dolphm, fair enough | 21:06 |
| dolphm | +2k lines is nuts | 21:06 |
| morganfainberg | dolphm, hashlib.sha256 hex is 64 bytes | 21:06 |
| morganfainberg | uuid4/5 is 32 bytes | 21:06 |
| dolphm | morganfainberg: sha356(s).hexdigest() is 64 chars | 21:07 |
| dolphm | 256** | 21:07 |
| dolphm | python -c "import hashlib; print(hashlib.sha256('something').hexdigest())" | 21:07 |
| morganfainberg | yeah | 21:08 |
| morganfainberg | hashlib.sha256(<data>).hexdigest() -> '476749ec53f7627b81502123d68303ce5e128076913532804ce0ecc38969e5ab' | 21:08 |
| ayoung | OK, so to see what is different between 71 and 73, you can just look at the difference between 72 and 73. OTOH, if you diff between 71 and 73, you could go by the files that have comments on them | 21:09 |
| dstanek | style question: inline comments already need a space after the #, but block comments have such rule. i've been mentioning that i'd like to see the space on reviews. how does everyone feel about that? | 21:10 |
| ayoung | You'd have to set Old Version History to Patch Set 73 and then click on 71 to see them | 21:10 |
| ayoung | dstanek, I don't care. Concerned that it leads to bike shedding. | 21:12 |
| dstanek | ayoung: nice, that should make it much easier | 21:12 |
| ayoung | dstanek, yeah...the diff thing is wonky | 21:12 |
| ayoung | OH, I need to post my comments | 21:12 |
| morganfainberg | dstanek, we should just propose a change to hacking to enforce block comments to work like that | 21:12 |
| morganfainberg | dstanek, i like having a space, way easier to read | 21:12 |
| dstanek | ayoung: bike shedding about the number of spaces? | 21:12 |
| ayoung | dstanek, yep | 21:13 |
| ayoung | and format in general | 21:13 |
| morganfainberg | dstanek, but i wouldn't -1 for it | 21:13 |
| ayoung | if we can't write an automated tool for it, forget it | 21:13 |
| morganfainberg | unless it's in hacking | 21:13 |
| bknudson | that should be an easy one for hacking to enforce | 21:13 |
| dstanek | morganfainberg: over the weekend i added a bunch of custom checks; this is just one of them | 21:13 |
| morganfainberg | bknudson, ++ | 21:13 |
| ayoung | dolphm, one question you raised earlier: should SQL be the default backend....It probably should be, but right now, I don;t think we default to running any of the extension migrations on db_sync unless they are done explicitly | 21:14 |
| dstanek | i did that, mutables as default args, no % in log statements, assertNone when using None with assertEqual | 21:14 |
| dstanek | and i think 1 or two more | 21:14 |
| morganfainberg | dstanek, nice | 21:14 |
| dstanek | i got tired of saying the same things in reviews over and over again | 21:15 |
| ayoung | making it default would require making sure we ran the migration, otherwise deploying will break | 21:15 |
| bknudson | dstanek: where's the code? | 21:15 |
| ayoung | dstanek, OK, if it is an autocheck, you get my vote. I assume the existing code base would have to run clean first, right? | 21:16 |
| dstanek | bknudson: i just picked it up again now - going to started submitting reviews to get our code to pass my new checks and also the checks | 21:16 |
| dstanek | ayoung: yep, it should soon :-) | 21:16 |
| ayoung | ++ | 21:16 |
| dstanek | oh, and i added a spelling check for comments and docstrings | 21:17 |
| ayoung | dstanek, still not 100% on the config file autochecker, as it is leading to a lot of automated merge failures, though. I think maybe we want to remove that check, and just kick people on code reviews to rerun it | 21:17 |
| ayoung | HA! | 21:17 |
| dstanek | not sure if that one will actually work out | 21:17 |
| ayoung | dolphm, OK...so say we do a one-way hash function from Attribute to ID. What will that break? We can't look up users by ID. It...I think it will break auditing, though. | 21:20 |
| morganfainberg | ayoung, hm. doesn't henry's patch store the other data as well in the map table? or are you thinking w/o the mapping table | 21:21 |
| morganfainberg | ayoung, for audit reasons you can lookup id->external info | 21:22 |
| dolphm | morganfainberg: he's trying to avoid the lookup table | 21:22 |
| morganfainberg | dolphm, ah | 21:22 |
| dolphm | so i proposed a giant delete of KDS ... i'm trying to figure out how to create a feature branch now... https://review.openstack.org/#/c/77701/ | 21:23 |
| morganfainberg | dolphm, might be a infra question. | 21:24 |
| *** leseb has joined #openstack-keystone | 21:25 | |
| ayoung | morganfainberg, it does seem to indicate "mapping is forever" | 21:27 |
| ayoung | and needs to be reversable | 21:28 |
| morganfainberg | ayoung, yeah. | 21:28 |
| ayoung | so really it is a question of recording the mapping versus calculating in a reversable manner | 21:28 |
| ayoung | calculateing from ID to attribute is going to "leak" | 21:28 |
| ayoung | I'd really like to go with the @@ solution, for both LDAP and Federation. It would work for the exsiting keystone users too, if we said the username@@domid is the userid | 21:30 |
| dstanek | morganfainberg, ayoung, bknudson: i'm going to track my changes against https://blueprints.launchpad.net/keystone/+spec/more-code-style-automation | 21:30 |
| ayoung | morganfainberg, then make the mapping table an extension for people that are willing to accept it and want the anonymity | 21:31 |
| morganfainberg | ayoung, thats fine, i think we're going to be ramming this down the throat of the community though. there was a fairly universal "don't make composite ids" vibe. | 21:31 |
| ayoung | morganfainberg, nah, just a few vocal opponents. One of whom is jaypipes, mind you, and I respect him a lot | 21:32 |
| ayoung | henrynash, you still awake? | 21:32 |
| morganfainberg | ayoung, i much prefer a consistent user_id format, rather than something where someone could have 'dksajfasdkjfksd@@@#$!!!' because saml | 21:32 |
| morganfainberg | or whatever in their name. | 21:32 |
| ayoung | morganfainberg, I would say "deafult is composite, uuid is extension" | 21:33 |
| ayoung | default | 21:33 |
| morganfainberg | ayoung, i'd go the other way tbh | 21:33 |
| ayoung | morganfainberg, no, make the mapping table optional | 21:33 |
| morganfainberg | ayoung, i still don't like exposing the internals | 21:33 |
| ayoung | morganfainberg, I don't like UUIDs | 21:33 |
| ayoung | we all have our El Guapos to face | 21:34 |
| morganfainberg | ayoung, is it possible for us to get non-url safe characters into the username via saml? | 21:34 |
| ayoung | morganfainberg, almost certainly | 21:34 |
| morganfainberg | ayoung, or via a CN | 21:34 |
| morganfainberg | erm | 21:34 |
| morganfainberg | DN | 21:34 |
| morganfainberg | then we need to also urlencode the user_id. | 21:34 |
| ayoung | morganfainberg, its based o nwhatever attribute the LDAP admin choses | 21:34 |
| ayoung | nope | 21:34 |
| morganfainberg | urlencode(username/id)@@domain | 21:34 |
| ayoung | if they break it, they bought it | 21:35 |
| morganfainberg | ayoung, no. | 21:35 |
| ayoung | a very big yes | 21:35 |
| ayoung | so much more than this will break | 21:35 |
| morganfainberg | ayoung, because we didn't do something sane w/ the attribute we're using to build the reference for our unique imutable identifier? | 21:36 |
| *** henrynash has quit IRC | 21:36 | |
| ayoung | Heh, I chased henrynash away | 21:36 |
| ayoung | because they did something not sane with their value | 21:36 |
| morganfainberg | ayoung, then why are we even trying to care about uniqueness? | 21:37 |
| morganfainberg | ayoung, it's the deployer's fault if the idps aren't providing unique data | 21:37 |
| ayoung | morganfainberg, yes, but they can step on each other | 21:37 |
| morganfainberg | ayoung, but by that argument, @@ isn't needed either | 21:37 |
| ayoung | I don't care if they mess themselves up, just not other customers | 21:38 |
| morganfainberg | ayoung, same argument dude, IDP provides bad data, and is not usable in keystone. bug is opened, we need to fix | 21:38 |
| ayoung | and we say "fix your mapping" | 21:38 |
| ayoung | "user id field must be url safe" | 21:39 |
| morganfainberg | ayoung, negative | 21:39 |
| ayoung | we do that already | 21:39 |
| *** jsavak has quit IRC | 21:39 | |
| morganfainberg | ayoung, sure we do that now, but we're also now accepting outside data | 21:39 |
| ayoung | we allow one LDAP server only | 21:39 |
| morganfainberg | ayoung, and if we are "fixing this" we should fix it right. | 21:39 |
| morganfainberg | not go ½ way | 21:39 |
| ayoung | nope | 21:40 |
| ayoung | don't try to fix this | 21:40 |
| ayoung | this is LDAP | 21:40 |
| morganfainberg | ayoung, i think you're 100% in the wrong here | 21:40 |
| morganfainberg | if we are accepting the data we should ensure it's usable across the api | 21:40 |
| morganfainberg | if we are masking the data w/ our own uuid we can ignore url safety | 21:41 |
| morganfainberg | or we need to validate lack of url safety and throw an exception...and wait for someone to open a bug for us to fix it | 21:41 |
| morganfainberg | not let the bad data get into keystone at all | 21:41 |
| dolphm | ayoung: you're breaking other services to support a broken integration with LDAP; the mapping table would avoid any impact on other services AND allow for a better integration with LDAP | 21:42 |
| dolphm | zero limitations on the LDAP user id attribute value | 21:42 |
| dolphm | it can literally be a blob in keystone | 21:42 |
| ayoung | "We can solve any problem by introducing an extra level of indirection." | 21:43 |
| *** david-lyle has quit IRC | 21:43 | |
| ayoung | morganfainberg, you are going off on a tangent here. LDAP as is is fine | 21:43 |
| ayoung | its the multi domain thing that is broken | 21:43 |
| dolphm | bknudson: i've been trying to type 'blk-u' in IRC all day and thinking you weren't on | 21:44 |
| dolphm | bknudson: anyway, just a heads up that i revised the commit message in https://review.openstack.org/#/c/77701/ with a link to the feature branch | 21:44 |
| ayoung | and that is broken for LDAP, and in Federation. URL Encoding is a different issue. | 21:44 |
| dolphm | ayoung: an hour ago you were talking about how broken LDAP was with length-limited ID's... | 21:45 |
| dolphm | ayoung: mutli-domain just compounds the problem with additional requirements | 21:45 |
| dolphm | multi-identity-driver* | 21:45 |
| ayoung | dolphm, I think you are misinterpreting what I am saying. We put restrictions on what an LDAP adminstrator can use as a valid ID field. Those restrictions are well within the normal expectations of things that have to interoperate with LDAP. It is no real burden | 21:46 |
| ayoung | the multi domain thing is outside that | 21:46 |
| dolphm | ayoung: unnecessary restrictions that a mapping table could solve for | 21:47 |
| morganfainberg | dolphm, ++ | 21:47 |
| ayoung | it is why ryan_lane was saying "don't do it" | 21:47 |
| dolphm | ayoung: link? | 21:47 |
| ayoung | dolphm, Ryan's comments were either in IRC or mailing list, a long while back | 21:47 |
| ayoung | he's in #openstack-dev if you really want | 21:48 |
| ayoung | length of the field and URL encoding are red herrings here | 21:48 |
| ayoung | having two users named ayoung coming from different IdPs is the proble,m | 21:49 |
| dolphm | ayoung: i pinged him | 21:49 |
| bknudson | it would be nice if we could merge some of the changes that will conflict with kds removal | 21:50 |
| bknudson | although rebase to remove the changes isn't a big deal either | 21:50 |
| *** joesavak has joined #openstack-keystone | 21:50 | |
| *** stevemar has quit IRC | 21:51 | |
| ayoung | bknudson, dstanek want to see how Zuul is treating your patch? http://paste.fedoraproject.org/82009/93883604/ | 21:54 |
| ayoung | http://paste.fedoraproject.org/82011/93883677/ | 21:55 |
| bknudson | dolphm: for example https://review.openstack.org/#/c/75549/ | 21:56 |
| *** harlowja is now known as harlowja_away | 22:05 | |
| *** harlowja_away is now known as harlowja | 22:05 | |
| *** henrynash has joined #openstack-keystone | 22:06 | |
| henrynash | ayoung: yep | 22:06 |
| *** marcoemorais has quit IRC | 22:07 | |
| *** jamielennox is now known as jamielennox|away | 22:07 | |
| henrynash | ayoung: just re-joined | 22:07 |
| ayoung | henrynash, I think mapping table is an extension, deconflicted UserIDs via @@ is an extension, and we let it battle its way out in the "real" world. Sportsmanlike | 22:07 |
| *** gokrokve has joined #openstack-keystone | 22:08 | |
| *** ayoung is now known as ayoung_afk | 22:09 | |
| henrynash | ayoung: so are you saying that we provide two different extensions and a cloud provider decides which one to load? | 22:09 |
| *** marcoemorais has joined #openstack-keystone | 22:15 | |
| dolphm | bknudson: i did one as a test | 22:16 |
| dolphm | bknudson: git checkout feature/key-dist; git pull; git review -x [existing review number]; git review | 22:17 |
| dolphm | bknudson: it created a new review in gerrit (since it's on a different branch) and I -2'd the one on master | 22:17 |
| *** joesavak has quit IRC | 22:22 | |
| *** henrynash has quit IRC | 22:35 | |
| bknudson | dolphm: we'll have to do that for the oslo.db syncs. | 22:36 |
| *** nkinder has quit IRC | 22:37 | |
| *** jamielennox|away is now known as jamielennox | 22:47 | |
| jamielennox | dolphm, bknudson: so is it a feature branch for KDS? | 22:51 |
| jamielennox | how do we go about splitting it out | 22:52 |
| bknudson | jamielennox: https://review.openstack.org/#/q/project:openstack/keystone+branch:feature/key-dist,n,z | 22:52 |
| jamielennox | bknudson: yep, i just saw that one come through | 22:52 |
| *** dims_ has quit IRC | 22:52 | |
| jamielennox | but that is a minor patch that i had put through, not a split out | 22:52 |
| bknudson | jamielennox: that change was made to the feature branch | 22:53 |
| jamielennox | bknudson: yep, but are we doing a rm in keystone ro a rm of everything else in the feature branch? | 22:54 |
| dolphm | +2 on limited use trusts from me https://review.openstack.org/#/c/56243/ | 22:54 |
| bknudson | jamielennox: https://review.openstack.org/#/c/77701/ | 22:54 |
| jamielennox | bknudson: ah - i need to pay closer attention to the keystone review emails | 22:54 |
| dolphm | jamielennox: reading back... | 22:55 |
| dolphm | jamielennox: so, the feature branch already exists based on the current master | 22:55 |
| jamielennox | dolphm: the rm KDS patch misses the requirements.txt additions | 22:55 |
| dolphm | jamielennox: ooh, good call | 22:56 |
| bknudson | dolphm: do you merge master to the branch every once in a while? | 22:56 |
| jamielennox | (though i am fine to leave them in there as i want to do pecan early in juno anyway) | 22:56 |
| dolphm | bknudson: yes, which means we'll need to merge this to the feature branch and then immediately revert it? or land a weird half-merge to the feature branch | 22:56 |
| dolphm | since we're going about the feature branch business all backwards | 22:56 |
| jamielennox | i wrote this more for the barbican guys but: https://etherpad.openstack.org/p/kite | 22:57 |
| bknudson | dolphm: yes, revert it in the feature branch | 22:57 |
| dolphm | jamielennox: bknudson: removed KDS-specific requirements https://review.openstack.org/#/c/77701/ | 22:58 |
| jamielennox | dolphm: if we branch like that can i just delete all the keystone stuff and move KDS to the root? | 22:59 |
| dolphm | jamielennox: NICE! | 22:59 |
| dolphm | jamielennox: lol probably | 22:59 |
| jamielennox | i can't see that we ever want to merge it back and it means i don't have to work around weird CONF and DB testing issues | 23:00 |
| dolphm | jamielennox: very true | 23:00 |
| bknudson | we're not going to merge it back? | 23:00 |
| jamielennox | bknudson: not as a contrib to keystone as i see it | 23:00 |
| bknudson | I've got a change that touched the kds code -- https://review.openstack.org/#/c/75549/ | 23:00 |
| bknudson | do we want that merged before kds is deleted, or before the branch has diverged? | 23:01 |
| jamielennox | there is this one too: https://review.openstack.org/#/c/77701/2 | 23:01 |
| dolphm | bknudson: it's already forked, so landing the delete would be more appropriate so they don't diverge | 23:01 |
| dolphm | delete first* | 23:02 |
| bknudson | dolphm: not sure what's going to happen... I cherry-pick the change as is to the feature branch? | 23:02 |
| bknudson | otherwise I'm going to rebase it and then it'll lose all the kds changes. | 23:03 |
| bknudson | not that it's hard to make the changes. | 23:03 |
| dolphm | bknudson: you want to land your change to master, right? | 23:03 |
| bknudson | dolphm: yes, the changes are mostly for keystone... just also affected kds | 23:03 |
| jamielennox | it works for me to have the db sync first | 23:03 |
| dolphm | bknudson: i'd suggest rebasing onto the KDS delete in master then | 23:05 |
| morganfainberg | dolphm, from a code stand-point i think limited trusts is looking good to me. | 23:05 |
| bknudson | dolphm: ok, either way works for me. | 23:05 |
| bknudson | morganfainberg: I'd like to run the migration with db2 first... | 23:05 |
| morganfainberg | bknudson, yeah sounds good | 23:06 |
| dolphm | bknudson: have time for that today? | 23:06 |
| bknudson | I'll work on it right now. | 23:06 |
| morganfainberg | bknudson, i haven't +2'd i was going to +1 and wait for you | 23:06 |
| dolphm | bknudson: otherwise, any issues can be RC-blockers | 23:06 |
| *** dims_ has joined #openstack-keystone | 23:08 | |
| morganfainberg | bknudson, +1 and i'll leave the +2 for you based on DB2 stuff | 23:08 |
| *** wchrisj has joined #openstack-keystone | 23:09 | |
| jamielennox | do we have a solution for this yet: https://review.openstack.org/#/c/75731/ | 23:09 |
| jamielennox | it is essentially another attempt at ayoung's mangle the URL for v3 | 23:09 |
| jamielennox | i was going to try to do it as part of a summit session but we may not have that kind of time | 23:10 |
| dolphm | jamielennox: i think i'm in favor of the approach, but i want to make sure the impact is limited to keystone | 23:10 |
| dolphm | jamielennox: i don't want to start re-writing the entire catalog for other services, etc | 23:11 |
| morganfainberg | dolphm, ++ | 23:11 |
| jamielennox | dolphm: i do prefer that one to ayoungs | 23:11 |
| morganfainberg | dolphm, jamielennox, i think that is the cleanest approach provided it only impacts keystone | 23:11 |
| morganfainberg | cleanest given all the other bits we need to address | 23:11 |
| dolphm | the else: pass is silly there | 23:12 |
| jamielennox | i'm not sure it works with some of my session stuff.. | 23:12 |
| jamielennox | but i'm assuming i can find the right place for it | 23:12 |
| dstanek | morganfainberg: just saw the note about limited use trusts - was just about to +2 it | 23:14 |
| jamielennox | actually it will just be deprecated by any client using an auth plugin which is fine by me | 23:15 |
| morganfainberg | dstanek, that is why i left that note :) | 23:15 |
| *** thedodd has quit IRC | 23:15 | |
| dstanek | morganfainberg: i'm glad i spent a few extra minutes running down a change is get_trusts or i would probably +2ed it before you got there | 23:17 |
| dolphm | jamielennox: and re: your comment -- i'd definitely like to land this (or something like it) before icehouse ships | 23:17 |
| dolphm | jamielennox: after the summit would be painful, as the number of bug reports / confusion around this is already building | 23:18 |
| jamielennox | dolphm: yea, i kind of agree - if we keep it in process_token like the current patch proposes then it's nice and isolated | 23:18 |
| jamielennox | it will only affect client v3 working as it currently does | 23:18 |
| dolphm | jamielennox: ++ | 23:18 |
| jamielennox | it won't work for this passing session so i'll need to figure out a work around for that | 23:19 |
| *** david-lyle has joined #openstack-keystone | 23:19 | |
| jamielennox | i didn't like it when he had it in __init__ | 23:19 |
| dolphm | jamielennox: __init__ of what? | 23:19 |
| jamielennox | v3.Client.__init__ | 23:19 |
| dolphm | ah | 23:19 |
| dolphm | jamielennox: i'll +2 when you're good with it | 23:20 |
| bknudson | morganfainberg: https://review.openstack.org/#/c/56243/25/keystone/trust/backends/sql.py | 23:20 |
| dolphm | jamielennox: slash, i also want to check it out and play with it | 23:20 |
| bknudson | isn't "filter_by(deleted_at=None)." going to break the sqlalchemy code? | 23:20 |
| bknudson | morganfainberg: you had a change that was similar to these. | 23:20 |
| dolphm | bknudson: how so? | 23:21 |
| bknudson | dolphm: https://review.openstack.org/#/c/77391/2/keystone/tests/test_sql_upgrade.py | 23:21 |
| morganfainberg | bknudson, oh crap sec | 23:21 |
| dolphm | bknudson: it's a copy/paste from get_trust() since there's no _get_trust() in that driver | 23:21 |
| bknudson | is it just the delete that cares? | 23:21 |
| morganfainberg | bknudson, oh yeah | 23:21 |
| morganfainberg | bknudson, filter_by shouldn't be an issue | 23:21 |
| bknudson | ok, thanks. | 23:22 |
| bknudson | morganfainberg: what is it with delete()? | 23:22 |
| morganfainberg | its that you're doing non dialect specific stiuff with .delete(<thing>) vs .delete().where | 23:22 |
| morganfainberg | bknudson, they were doing an embedded where clause in the .delete() before rather than constructing the query | 23:22 |
| dolphm | morganfainberg: link for my edumacation? | 23:22 |
| morganfainberg | dolphm, https://review.openstack.org/#/c/77391/ | 23:23 |
| morganfainberg | dolphm, table.delete(id=<thing>) | 23:23 |
| morganfainberg | vs table.delete().where(table.c.id == <thing>) | 23:23 |
| morganfainberg | dolphm, in sqlalchemy 0.9.3 the former is non-dialect specific and will raise an exception | 23:24 |
| morganfainberg | dolphm, i assume it is because there is no type checking / casting | 23:24 |
| dolphm | morganfainberg: ah, hrm | 23:24 |
| morganfainberg | dolphm, when you do table.c.id you're referencing the table datatype/colum info | 23:24 |
| bknudson | hopefully we'll be on sqlalchemy 0.9.3 soon. | 23:24 |
| *** achampion has quit IRC | 23:25 | |
| bknudson | the upgrade works fine on db2, but it's got hard-coded sql so I'd rather that wasn't there. | 23:25 |
| morganfainberg | according to zigo that was the only issue we had in keystone for 0.9.3 | 23:25 |
| bknudson | since this has caused problems in the past with different dbs | 23:25 |
| morganfainberg | and it was specific for a test case | 23:25 |
| morganfainberg | so, easy to fix | 23:25 |
| bknudson | I mean there's hardcoded SQL in the upgrade test : https://review.openstack.org/#/c/56243/25/keystone/tests/test_sql_upgrade.py -- line 2078 | 23:27 |
| bknudson | this file is too big. | 23:27 |
| dolphm | bknudson: "even better" but no +2? :P https://review.openstack.org/#/c/77701/ | 23:27 |
| bknudson | dolphm: I was waiting for jenkins | 23:28 |
| *** nkinder has joined #openstack-keystone | 23:28 | |
| *** jagee has quit IRC | 23:31 | |
| dolphm | bknudson: speaking of, this failed after +A, but it looks like a check job that failed rather than the gate job, and it looks like it should have failed in the first place anyway https://review.openstack.org/#/c/75816/ | 23:34 |
| bknudson | dolphm: I think it got caught in between another change that merged on it. | 23:35 |
| bknudson | it worked on 2/28 then failed on 3/2 | 23:36 |
| *** lbragstad has quit IRC | 23:36 | |
| *** dims_ has quit IRC | 23:46 | |
| *** andreaf has joined #openstack-keystone | 23:48 | |
Generated by irclog2html.py 2.14.0 by Marius Gedminas - find it at mg.pov.lt!