Monday, 2014-03-03

*** gokrokve has joined #openstack-keystone00:00
ayoungjamielennox, I merged the 3 different patches into one.  But the heart is  contrib/revoke/  and core.py00:06
*** tellesnobrega has joined #openstack-keystone00:17
ayoungjamielennox, a couple things about the extension.   I explained the  revocation checking
jamielennoxayoung: yea, i'm going to have to have another read through of how the revocation tree stuff is happening00:23
jamielennoxayoung: just going through the weekends emails00:23
ayoungthe 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 database00:23
ayoungjamielennox, also, a lot of the testing is "replace revoke by id with revoke by token" and run the existing tests00:24
*** dolphm_503 is now known as dolphm00:37
*** leseb has joined #openstack-keystone00:45
*** dolphm is now known as dolphm_50300:47
*** leseb has quit IRC00:50
*** stevemar has quit IRC01:15
*** amerine_ has joined #openstack-keystone01:22
*** amerine has quit IRC01:24
*** tellesnobrega has quit IRC01:35
*** tellesnobrega has joined #openstack-keystone01:36
*** gokrokve has quit IRC01:39
*** gokrokve has joined #openstack-keystone01:41
*** dolphm_503 is now known as dolphm01:59
*** stevemar has joined #openstack-keystone02:01
*** ChanServ sets mode: +v stevemar02:01
*** amcrn has joined #openstack-keystone02:03
*** dolphm is now known as dolphm_50302:34
*** dolphm_503 is now known as dolphm02:52
*** gokrokve has quit IRC03:13
*** gokrokve has joined #openstack-keystone03:13
*** gokrokve has quit IRC03:17
*** stevemar has quit IRC03:56
*** dolphm is now known as dolphm_50304:04
*** bvandenh has joined #openstack-keystone04:26
*** stevemar has joined #openstack-keystone04:53
*** ChanServ sets mode: +v stevemar04:53
*** bvandenh has quit IRC04:58
*** dolphm_503 is now known as dolphm05:05
*** dolphm is now known as dolphm_50305:14
*** theocean154 has joined #openstack-keystone05:50
*** gokrokve has joined #openstack-keystone05:55
*** stevemar has quit IRC06:22
*** saju_m has joined #openstack-keystone06:31
*** topol has quit IRC06:45
*** chandankumar_ has quit IRC06:45
*** chandan_kumar has joined #openstack-keystone06:51
*** chandan_kumar has quit IRC06:55
*** YorikSar has joined #openstack-keystone07:00
*** amerine_ is now known as amerine07:03
*** amerine has quit IRC07:08
*** amerine has joined #openstack-keystone07:12
*** YorikSar has quit IRC07:36
*** YorikSar has joined #openstack-keystone07:37
*** YorikSar has quit IRC07:41
*** YorikSar has joined #openstack-keystone07:42
*** gokrokve has quit IRC07:45
*** marekd|away is now known as marekd08:12
*** theocean154 has quit IRC08:12
*** gokrokve has joined #openstack-keystone08:16
*** gokrokve has quit IRC08:38
*** saju_m has quit IRC08:40
*** leseb has joined #openstack-keystone08:42
*** saju_m has joined #openstack-keystone08:43
*** saju_m has quit IRC09:02
*** chandan_kumar has joined #openstack-keystone09:03
*** theocean154 has joined #openstack-keystone09:06
*** leseb has quit IRC09:09
*** theocean154 has quit IRC09:11
*** gokrokve has joined #openstack-keystone09:16
*** gokrokve has quit IRC09:21
*** saju_m has joined #openstack-keystone09:34
*** leseb has joined #openstack-keystone09:38
*** leseb has quit IRC10:10
*** leseb has joined #openstack-keystone10:11
*** leseb has quit IRC10:15
*** gokrokve has joined #openstack-keystone10:17
*** gokrokve has quit IRC10:22
*** leseb has joined #openstack-keystone10:24
*** saju_m has quit IRC10:25
*** amichon has joined #openstack-keystone10:29
*** saju_m has joined #openstack-keystone10:42
*** david-lyle has quit IRC10:45
*** theocean154 has joined #openstack-keystone10:55
*** theocean154 has quit IRC10:59
*** saju_m has quit IRC11:11
*** gokrokve has joined #openstack-keystone11:17
*** gokrokve has quit IRC11:21
*** saju_m has joined #openstack-keystone11:25
*** amichon has quit IRC11:40
*** gokrokve has joined #openstack-keystone12:17
*** gokrokve has quit IRC12:22
*** theocean154 has joined #openstack-keystone12:43
*** theocean154 has quit IRC12:47
*** chandan_kumar has quit IRC12:48
*** chandan_kumar has joined #openstack-keystone12:49
*** saju_m has quit IRC12:52
*** orion195 has joined #openstack-keystone12:54
*** chandankumar_ has joined #openstack-keystone12:54
orion195hi guys, after integrating keystone with AD (only for identity and leaving SQL as assignment backend).... I cannot login anymore in the dashboard12:55
orion195the auth_strategy setup is setup as : keystone12:55
orion195and everytime I try to access the /dashboard I receive this:12:56
orion195TypeError: unsupported operand type(s) for +: 'NoneType' and 'str'12:56
*** chandan_kumar has quit IRC12:57
*** topol has joined #openstack-keystone12:59
orion195anyway, a question more related with keystone:13:05
orion195if I setup the identity on LDAP and Assignment in SQL13:05
orion195if I list the users: keystone user-list, should I see the admin user?13:05
*** dolphm_503 is now known as dolphm13:07
dstanekorion195: i'm not familiar with Horizon's code, but at first looking at your traceback i would guess that the auth url isn't specified13:08
orion195the auth_url in neutron.conf file right?13:09
dstanekthat's what it looks like because the tb is from neutron client13:09
dstanekor where ever it is supposed to get it from13:09
orion195dstanek: the auth_url is setup now, but still the same13:11
orion195dstanek: I always get this in the logs: WARNING keystone.common.wsgi [-] Authorization failed. Invalid user / password from13:12
orion195the question is: the admin_user in neutron.conf, is neutron right?13:12
orion195(following the default conf)13:12
orion195maybe I misunderstood what user to use, quite odd13:13
orion195or, 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
dstanekorion195: i would assume not, but i'm not the greatest source of information for LDAP13:16
dstanekyour talking about the LDAP user used to query and not a keystone user right?13:17
orion195the LDAP user, right13:17
orion195but, the keystone user, is the admn user, right?13:17
*** gokrokve has joined #openstack-keystone13:19
*** gokrokve has quit IRC13:23
*** wchrisj has joined #openstack-keystone13:37
dstanekorion195: are you talking about using the admin_token?13:39
*** wchrisj has quit IRC13:42
*** jagee has joined #openstack-keystone13:47
*** lbragstad has quit IRC13:47
*** achampion has quit IRC13:49
orion195dstanek: "User .. is unauthorized for tenant admin", "code": 401, "title": "Unauthorized"}}13:51
orion195this is the error I'm getting13:51
orion195my user if provided by the LDAP13:51
orion195I don't really get the problem13:53
orion195my user is authorized on the admin tenant13:54
orion195if I: keystone user-role-list --tenant-id=admin --user-id=13:54
orion195I 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
mhuorion195, do you have access to the keystone logs ?13:55
*** stevemar has joined #openstack-keystone13:55
*** ChanServ sets mode: +v stevemar13:55
orion195but, somehow, the nova client and neutron client, while authenticating with my credentials..... it doesn't work13:55
orion195mhu: I do have indeed13:55
*** koolhead17 has quit IRC13:57
mhuorion195, gs is the user id ?13:59
orion195it's my full name13:59
orion195that I reduced to my initials14:00
orion195and this is provided out of LDAP14:00
orion195in LDAP I only have USERS, I hav sql to perform assignments14:00
mhuorion195, can I see your [ldap] section in keystone.conf ? specifically the user_id mapping14:01
orion195just a sec14:01
orion195user_filter section?14:02
*** koolhead17 has joined #openstack-keystone14:03
mhuI think you might be hitting this:
orion195user_id_attribute = cn14:05
mhuah, nevermind, it should not be that14:06
orion195mhu: if I change it to use sAMAccountName14:08
orion195it doesn't find my username anymore14:08
orion195mhu: another question14:12
orion195the admin tenant14:12
*** browne has joined #openstack-keystone14:12
orion195is something should be setup in the LDAP or in the SQL?14:13
orion195can it be that somehow there is a 'misunderstanding' within ldap and/or mysql?14:13
orion195I 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 for14:14
*** gokrokve has joined #openstack-keystone14:17
ayoungorion195, samAccountName is an open bug already14:18
ayoungthere are workarounds.14:19
orion195ayoung, thank you for telling me this, unfortunately I didn't find any workaround14:20
*** wchrisj has joined #openstack-keystone14:20
orion195and most important: I still don't know the root cause about this14:21
*** gokrokve has quit IRC14:22
*** lbragstad has joined #openstack-keystone14:23
ayoungorion195, Blackest of magic.   Best to blame Microsoft14:27
mhuorion195, 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 answers14:28
orion195mhu: I may have found something14:30
orion195keystone --os-auth-url --os-username username user-list14:31
orion195"metadata": {"is_admin": 0, "roles": []}}},14:31
*** theocean154 has joined #openstack-keystone14:31
orion195I would say that something is wrong with the DB14:31
dolphmorion195: the metadata block is garbage, but you're not going to get authorization without specifying a tenant there14:32
*** achampion has joined #openstack-keystone14:32
dolphmorion195: --os-project-name=admin for example14:33
orion195eystone --os-auth-url --os-tenant-name amdin --os-username myuser tenant-list14:34
orion195I do see everything14:34
orion195it can be somehow that the neutron-user-password is not somehow syncronized?14:35
*** theocean154 has quit IRC14:35
orion195I mean the only error I have is with the network component14:35
orion195is there a way how I can check is a service_user authenticate correctly?14:36
*** wchrisj has quit IRC14:45
orion195thank you guys for your help14:45
orion195the service account user was blocked in the Active Directory14:45
ayoungdolphm, got a moment to talk about henrynash's patch?14:51
*** wchrisj has joined #openstack-keystone14:53
ayoungdolphm,  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 calculation14:54
*** leseb has quit IRC14:57
*** leseb has joined #openstack-keystone14:59
*** richm has joined #openstack-keystone15:09
richmWhen reviewers comment on a commit, and I reply to those comments, do the reviewers get notified?15:10
ayoungrichm, email notifications are enabled.15:13
*** gokrokve has joined #openstack-keystone15:17
*** gokrokve has quit IRC15:21
*** leseb has quit IRC15:25
*** gokrokve has joined #openstack-keystone15:26
*** nkinder has joined #openstack-keystone15:36
dstanekayoung: i'm a little worried about the memory impact of the RevokeTree with a ton of events15:36
ayoungdstanek, it should be pretty efficient15:36
ayoungbut...we could generate a test15:36
*** david-lyle has joined #openstack-keystone15:37
dolphmayoung: 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
ayoungdstanek, the old way?  Yeah15:37
ayounglinear search15:37
ayoungdolphm, sorry, yeah, lienar search, and since the item is likely not to be found, it will be an expected case15:38
ayoungdstanek, if there are N events, the memory impact should be under O(2N)15:38
ayoungN for the current events list, and then  a handful of extra values for the extra keys in the tree15:39
ayoungdstanek, 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 testing15:39
ayoungIN general, it should minimize the number of events15:40
dolphmayoung: that doesn't sound like the solution we talked about last week then15:40
ayoungdolphm, "Brute force" was the precursor to the tree15:40
dolphmayoung: i'm talking about henry's patch, not yours15:40
ayoungit was what I was doing in Patch-1-of-315:40
ayoungdolphm, ah...15:40
ayoungdolphm, brute force is the creating  a new UUID15:41
ayoungand recording it15:41
dolphmayoung: dstanek is asking you about your patch :P15:41
ayoungdolphm, 'salright, I think I can keep those two clear15:41
ayoungI hope15:41
*** chandan_kumar has joined #openstack-keystone15:41
ayoungdolphm  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
ayoungLets not make ephemeral users.15:43
ayoungSo, the thought process went like this15:43
ayoungWe need to deconflict Ids from different IdPs.15:43
dolphmayoung: henry's solution shouldn't be implementing ephemeral users15:44
dolphmayoung: i'll check out the review later today -- i want to focus on the two remaining bp's first15:44
ayoungdolphm, the mapping table is bascially an ephemeral ID15:45
ayoungdolphm, with all the same drawbacks.15:45
ayoungdolphm, 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 well15:46
dolphmayoung: i haven't started yours this morning yet - i'm looking at remaining_uses first15:46
ayounglast comment was on revoke, not henry;s15:46
dolphmayoung: go ahead with a rev15:46
ayoungis remaining_uses close?15:46
dolphmayoung: i'm about halfway through and only have nits so far15:47
lbragstadremaing_uses is looking pretty good15:47
dstanekayoung: i reviewed revocation this morning - which is why i started thinking about this in memory cache15:47
dolphmayoung: mostly in tests15:47
*** leseb has joined #openstack-keystone15:47
ayoungdstanek, 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
ayoungwe can optimize for gropus in the future, too, I think, once we have a clearer query mechanism for them15:48
dolphmdstanek: side note -- i ran into problems with testr last week returning SUCCESS as a false positive when specifying a specific class to test15:49
*** henrynash has joined #openstack-keystone15:49
dolphmdstanek: i.e. testr run keystone.tests.test_module:TestClass would return SUCCESS repeatedly with obviously broken tests15:50
dstanekdolphm: really? that's really bad15:50
dolphmdstanek: yeah -- i couldn't reproduce consistently though, and a complete run (testr run) always seemed to behave correctly15:51
lbragstaddolphm: dstanek I've hit that too.. I usually just keep backing up a module/dir until it starts working15:51
ayoungI think it is the : versys .  for specifying the Class. Was it actually running the test, or just returning SUCCESS 0 test run?15:52
ayoungdstanek, do we have a "Token Expired" exception?  I think it is not a separate exception15:54
dstanekdolphm, lbragstad: i tend to use nosetests still - it's more reliable in general for me15:54
dolphmayoung: are you not supposed to use the colon? it appeared to be executing the correct class when it did work15:55
ayoungdstanek, I could just return an Unauthorized when the token times out, but Not found seems to be in keeping with what we are doing15:56
ayoungdolphm, I don15: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
dolphmayoung: raise not found from the backend, and recast as 401 if emitting in response to auth15:56
dolphm... which is usually/always the case for tokens15:56
ayoungdolphm, agree that is what I am doing...15:56
dstanekayoung: what code are you talking about? re:Unauthorized vs. NotFound15:58
ayoungdstanek, your comment on
dstanekayoung: that was lbragstad15:59
ayoungdstanek, ah...16:00
ayoungdolphm, are we supposed to remove copyright heads on files like  that were pre-existing:  Copyright 2012 OpenStack Foundation16:03
*** chandan_kumar has quit IRC16:03
henrynashayoung, 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
ayounghenrynash, no, we are close...I think the mapping should be "calculated not recorded"16:04
ayoungthe recorded approach gives us all of the problems with ephemeral users16:04
henrynashayoung: and by recorded you mean (since it has to be stored somewhere) stored in the ID field itself16:05
ayounghenrynash, 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 practice16:06
henrynashayoung: so we are still recording it….it is just whether we keep a separate table or store it inside the ID…..16:06
ayoungyou 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 intent16:07
ayounghenrynash, I would propose this:16:07
dolphmmhu: i just left a bunch of comments on -- i'd be happy to address some of them if you don't mind16:07
dolphmmhu: (unless you have another revision in the works anyway)16:07
henrynashayoung: (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
ayoungfor 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 chars16:08
ayounghenrynash, I've tracked the conversation.16:10
dolphmhenrynash: i haven't done a code review on your patch since last week, nor read ayoung's argument(s) against the current implementation yet16:10
*** thedodd has joined #openstack-keystone16:10
henrynashayoung: 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
henrynashayoung: that is the issue being argued on the list (and by others, e.g. dolphm)16:13
ayounghenrynash, keystone was originaly stated to have the goal of being a thin wrapper around  exisiting Identity Providers.16:14
mhudolphm, I am currently adding tests to test_sql_upgrade as suggested by bknudson, turns out there were some errors in the upgrade script16:15
mhudolphm, I'll check your comments, thx16:16
ayounghenrynash, 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
dolphmmhu: thank you!16:16
*** chandan_kumar has joined #openstack-keystone16:16
ayounghenrynash, 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
dolphmmhu: free diff:16:16
dolphmmhu: curl | git apply16:16
henrynashayoung: 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 agree16:17
ayoungthe first implementation should be the suffix16:17
mhudolphm, sweet ! thx16:17
dolphmayoung: why does keystone need to solve for ldap / sql backend corruption...?16:18
ayoungdolphm, 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-keystone16:19
dolphmalright 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|away16:23
*** theocean154 has quit IRC16:23
*** leseb has quit IRC16:25
*** leseb has joined #openstack-keystone16:26
ayoungdstanek, 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 subclasses16:29
ayoungIs 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 IRC16:30
*** bknudson has quit IRC16:31
henrynashanyone know anything about jenkins failing with ImportError: cannot import name ansisql on keystone patches…is that just me?16:34
*** dims_ has joined #openstack-keystone16:37
*** marcoemorais has joined #openstack-keystone16:37
dolphmhenrynash: link?16:37
*** gokrokve has quit IRC16:39
*** bknudson has joined #openstack-keystone16:39
*** marcoemorais has quit IRC16:40
dstanekayoung: if everything is calling super() does your setUp not get called?16:40
dstanekit shouldn't matter what the parent class is16:40
ayoungdstanek, as I recall I was getting an error that there was no super.16:40
ayoungLet me try again...maybe it was an artifact of something else I was doing.16:41
dstanekayoung: just looking at it i would thing renaming doSetUp to setUp and removing the empty setUps would work16:41
ayoungdstanek, testing..16:42
*** devlaps has joined #openstack-keystone16:48
ayoungdstanek, setUp in the non-test baseclass is getting skipped16:50
dstanekayoung: hmmm, that's odd. i'll dig in once i finish this review i'm looking at now16:52
ayoungdstanek, in testtools.testcase it does a self.setUp() and that calls directly into the setUp function in test_v316:52
ayoungdstanek, 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
ayoungbknudson, that is kindof what I figured.16:53
ayoungand why I had to avoid putting the setUp function outside the hierarchy16:53
ayoungthe testcase hierarchy16:53
bknudsonayoung: makes sense to me. Not the prettiest, but seems necessary.16:54
ayoungbknudson, yeah...its due to the depth of the hierarchy here16:54
bknudsonsometime we need to do a cleanup of the tests... I think dstanek had some things written down.16:55
bknudsonhere's an example of problems:
ayoungbknudson, 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-keystone16:58
bknudsonayoung: exactly16:58
*** gyee has joined #openstack-keystone17:05
*** thedodd has quit IRC17:10
*** gokrokve has quit IRC17:14
*** thedodd has joined #openstack-keystone17:14
*** gokrokve has joined #openstack-keystone17:17
*** leseb has joined #openstack-keystone17:17
*** gokrokve has quit IRC17:20
*** theocean154 has joined #openstack-keystone17:22
*** chandan_kumar has quit IRC17:25
ayoungbknudson,  Jamie suggests a descending index.  However, I don't want to predefine the index name17:27
ayoungI can do sql.Column('revoked_at', sql.DateTime(), index=True, nullable=False))17:27
ayoungis that sufficient, or do I need to do17:28
ayounglike the example says, Index('someindex', mytable.c.somecol.desc())17:28
bknudsonayoung: why don't you want to predefine the index name?17:29
ayoungbknudson, because that is one thing that is different for each RDBMS17:29
*** amcrn has quit IRC17:29
ayoungI'd rather not wade into that particular nastiness.  Defining an index on MySQL that looks like Postgres one, for example17:30
ayoungquestion is whether a descending index is required here, too?17:30
bknudsonI 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
bknudsonunless you're storing your database on a tape drive.17:31
*** orion195 has quit IRC17:31
bknudsonI 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
ayoungI'm going to leave it as just an index for now.17:33
ayoungKeeps the code cleaner17:33
ayoungOK.  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
ayoungwhich means that this code has not been run in quite some time....something we need to fix17:42
ayoungWe 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, too17:43
*** harlowja_away is now known as harlowja17:46
*** harlowja is now known as harlowja_away17:47
*** harlowja_away is now known as harlowja17:50
*** rwsu has joined #openstack-keystone17:55
*** marcoemorais has joined #openstack-keystone17:58
dolphmayoung: 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-tree17:59
ayoungdolphm, yeah...question is what that should look like.  Its been on my todo list for a while17:59
dolphmayoung: it should look just like grenade18:00
*** jsavak has joined #openstack-keystone18:00
ayounglive LDAP and live sql.  The migrations are tested for SQL via grenade, just not to the same degree as our own tests18:00
ayoungliveldap is a different beast18:00
ayoungI need to learn about the whole "Run this setup once per suite" function18:01
ayoungis that the setup_tests function, I am assuming?18:02
*** gyee has quit IRC18:03
*** arborism has joined #openstack-keystone18:10
*** arborism is now known as amcrn18:10 was my own fault for not clearing out the keystone_test database...18:16
dolphmayoung: how has the revocation patch doubled in size in the last week?18:19
dolphmit's also tracking against a second blueprint now?18:22
ayoungdolphm, 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 reviewed18:33
lbragstadstevemar: ping?18:36
stevemarlbragstad, ahoy18:36
lbragstadqq here18:36
stevemari just had it open18:36
lbragstaddoes http://localhost:35357/v2.0/tokens have to https://?18:36
lbragstad... https://localhost:35357/v2.0/tokens18:37
*** gyee has joined #openstack-keystone18:38
lbragstadshould that be updated too if so?18:38
dolphmayoung: start thinking about waiting for juno to land revocation events18: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 months18:45
ayoungcutting it would be painful18:45
dolphmayoung: understood and agree18:45
ayoungthere is at least one use case I know of that needs it18:45
stevemarlbragstad, whats the rest of the doc like?18:45
ayoungwhich is driven by QA, but I think reflects a real world concern18:45
stevemarlbragstad, if it's http:// everywhere else, then i'm okay with updating it all to https:// on another patch18:45
ayoungdolphm, say a user generates a gazillion tokens, and then gets booted18:45
ayoungall of their tokens get might be a memory/performance problem18:46
ayoungwe've seen it in tests already18:46
dolphmayoung: yep; that's one reason why it's high priority18:46
*** amerine has left #openstack-keystone18:46
ayoungI think the code is ready to go18:46
*** theocean154 has quit IRC18:46
ayoungthere may be bugs...but the bones are solid18:46
dolphmayoung: it's clear the tests were an after thought... which shouldn't be the case for a security feature18:47
lbragstadstevemar: looks like it's http:// everywhere... so probably done in another patch18:47
stevemarlbragstad, cool, mention that in the change then18:47
*** morganfainberg_Z is now known as morganfainberg18:47
ayoungdolphm, hmmm....the thinking was that the existing tests should be sufficient if we replace  the old mechanism with the new...18:48
ayoungI missed that for the V2 api until this weekend....18:48
lbragstadstevemar: thanks, done and done18:48
dolphmayoung: you're implementing a new API, and i simply can't read the tests and write a client against that API18:48
ayoungdolphm, 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
ayoungdolphm, I see what you are saying...on the client side18:49
ayoungwe 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 keystoneclient18:50
ayoungBut that is why so much of the testing effort went in to test_v3_auth18:51
ayoung  the rules should apply the same local as remote.18:51
dolphmayoung: the tests in test_v3_auth are a great start, but they don't provide sufficient coverage over the /events response itself18:52
dolphmayoung: self.assertEqual(events_response, {'events': [{'domain_id': deleted_domain_id}]})18:53
stevemarlbragstad, np boss, now to return the favor, muahahaha
dolphmayoung: that's all you need for really strong coverage ^ just do something like that in every test, and dump the "domain in list" checks18:53
lbragstadstevemar: I was just about to start looking at that... I gave it a quick once over and it was looking good18:54
*** leseb has quit IRC18:54
ayoungdolphm, interesting....18:54
dolphmayoung: i think dates would be the only gotcha (?)18:55
ayoungdolphm, 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 times18:55
dolphmmaybe use the assertCloseEnoughForGovernmentWork and then drop dates from the response18:55
dolphmayoung: at minimum, it *does* need to check the format of the result18:56
ayoungOK...let me see if I can do that with a minimum of refactoring18:56
*** leseb has joined #openstack-keystone19:02
lbragstadstevemar: had a couple questions, but it's looking pretty good.19:09
stevemarlbragstad, question away19:10
lbragstadI published them in the review, not sure about the client, but do messages have to be wrapped in _()?19:10
stevemarlbragstad, replied19:12
stevemarlbragstad, we don't translate on client side19:13
lbragstadgood to know19:13
morganfainbergdolphm, ping - I'm going to propose a schema collapse post I3 if there is no complaints about it19:23
morganfainbergdolphm, so I migration repo would assume you're on H already (036)19:24
dolphmmorganfainberg: awesome!19:30
dolphmmorganfainberg: do you want to push this to RC1 or juno?
morganfainbergdolphm, Lets target it to J. We've lived with it long enough now.19:32
morganfainbergdolphm, if i can squeeze it in (fix) before RC1, we can evaluate it19:32
dolphmmorganfainberg: i won't hold my breath :P19:33
morganfainbergdolphm, 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 issue19:33
dolphmmorganfainberg: yeah...19:33
dolphmmorganfainberg: i want to make sure this lands before i3
morganfainbergdolphm, ++19:41
morganfainbergdolphm, looking at it now19:47
morganfainbergi'll plan to land the last helpstrings stuff post I319:47
morganfainbergwant to prioritize the "important stuff"19:47
dolphmmorganfainberg: brant also has a series of fixes for a regression since havana, starts here:
morganfainbergdolphm, ok, on my list for today.19:49
bknudsondolphm: so those changes cover the "endpoints", but I think essentially the same thing has to happen for "services"19:51
bknudsonso it would be nice to get an idea that the endpoints fix is the right way to do it19:51
bknudsonand 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
dolphmbknudson: lgtm so far20:05
dolphmbknudson: and yeah, endpoints was just the one i ran into with openstackclient20:08
morganfainbergdolphm, yay we don't have broken pam identity anymore ;)20:17
dolphmmorganfainberg: that's bugged me for a long time lol20:17
morganfainbergdolphm, i was going to fix it until i saw how broken it was20:17
morganfainbergand how long it had been broken20:19
morganfainbergbknudson, i'll fix the helpstrings for post I320:19
morganfainbergbknudson, no reason to make extra rebase work for the milestone20:20
bknudsonmorganfainberg: ok... I don't think there's rebase work unless someone was changing the same lines.20:20
bknudsonat this point20:20
morganfainbergbknudson, right. but if someone somehow added sample config lines20:20
morganfainbergbknudson, i think ayoung's and henry-nash's bps are adding options20:21
morganfainbergbknudson, so we can wait a couple days for the last of the helpstring.s  i agree with all of your comments btw20:21
bknudsonthey're going to conflict anyways20:21
henrynashbknudson: although those kind of rebases are pretty easy20:21
bknudsonI think we've got 4 migration 41s.20:21
henrynashdolphm: trying to ensure I concentrate on the right things… 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
morganfainberghenrynash, i'd really rather not do concat stuff personally20:25
dolphmhenrynash: is there another viable solution? there doesn't seem to be support on list for the @@ alternative20:25
morganfainberghenrynash, i think that is a recipe for needing to support 3 forms of IDs in the long term20:25
henrynashdolphm: agreed…well, vish gave it limited support….but otherwise no20:26
dolphmhenrynash: the encoding solution won't work within 64 bytes without truncating... it'll just run into the same problem we have now, only quicker20:27
morganfainbergi also like the idea that we should support a single data-type and style for user_ids if possible.20:27
dolphmhenrynash: and i agree with vish to an extent, but maximum string length of our IDs should be definable if we expect anyone to use them20:27
henrynashdolphm: yes, the best you can do in 64 bits is something like 54chars if ID, 2 separators, 8 chars of domain_index20:27
henrynashdolphm: so that practical consequence would be a loss of 10 chars from the IDs used by LDAP20:28
morganfainbergyeah. that sounds about right20:29
dolphmhenrynash: am i missing anything in thinking that the sql driver could/should become domain-unaware first?20:31
morganfainbergdolphm, that is the one case i don't think we can do it20:31
morganfainbergdolphm, we support multiple domains in SQL and people use it20:31
morganfainbergdolphm, i agree though, i would prefer it to be domain unaware20:31
bknudsonhow can sql become domain-unaware?20:32
dolphmmorganfainberg: i'm not proposing that we lose the ability to store multiple domains worth of identity information in sql...20:32
bknudsonmultiple sql backends?20:32
dolphmbknudson: sort of20:32
morganfainbergbknudson, that would be my choice.20:32
henrynashdolphm: 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 clients20:32
dolphmbknudson: they could have the same connection string though20:32
morganfainbergdolphm, could they? wouldn't that create name conflicts?20:32
dolphmmorganfainberg: ooh, there you go. we'd have to check for conflicts ourselves :(20:33
bknudsoneach sql backend would have its domain_id configured.20:33
morganfainbergand/or bleed users between domains20:33
henrynashmorganfainberg: 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
bknudsonthen would just put that in the db table.20:33
morganfainbergbknudson, ah yeah doable20:34
dolphmdomain_id could become an identity driver hint... and then only sql would use it20:34
morganfainbergdolphm, ++20:34
bknudsonwhere would the manager get domain_id?20:35
morganfainbergnkinder, ping - have an LDAP question might be able to answer for me.20:35
morganfainbergnkinder, regarding the maximum length of a DN.20:35
nkindermorganfainberg: you just want to know if there is an official limit?20:35
dolphmbknudson: i was thinking the manager wouldn't... instead...20:36
morganfainbergnkinder, less official, more implementation specific20:36
morganfainbergnkinder, could the first element of the DN "cn=<blah blah blah>" be > 64 bytes?20:36
morganfainbergwell the part after 'cn=' for example20:36
nkindermorganfainberg: yes20:36
morganfainbergdolphm, ^ we need something to map user ids to a consumable value based upon that20:37
dolphmthe 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
bknudsondolphm: so multiple managers.20:37
morganfainbergif a ldap id is > 64 bytes, we potentially can't use that DN object in assignment (sql based)20:38
dolphmbknudson: ++20:38
henrynashdolphm: so today we just do that at the Manager level…i.e. we have a driver per specific domain20:38
morganfainbergwhich almost requires the map table.20:38
henrynashmorganfainberg: we can't….doesn't work in practice (for the reasons you are expressing)20:38
morganfainberghenrynash, that argues we must implement a mapping table.20:38
nkindermorganfainberg: it's not common for it to be that large for user entries, but it's certainly possible.20:38
henrynashmorganfainberg: I see email address used most often20:39
morganfainbergnkinder, correct.20:39
dolphmsorry for length.20:39
morganfainbergnkinder, i hope no one does that20:39
bknudsondolphm: pep8 violation!20:39
dolphm  # noqa20:39
morganfainbergdolphm, OMG eye bleeding violation20:39
morganfainbergdolphm, :P20:39
morganfainbergdolphm, i'm not sure how to reasonably counter ayoung's valid complaints about the map table.20:40
dolphmhenrynash: regarding "local_id" ... "local" might be LDAP, right?20:40
morganfainbergdolphm, it is more brittle than the id encoding the information in it.20:40
dolphmmorganfainberg: define brittle in this case20:41
*** leseb has quit IRC20:41
henrynashdolphm, 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
dolphmbknudson: morganfainberg: henrynash: PEP8'd
morganfainbergdolphm, it's an extra moving part that can get out of sync.20:42
morganfainbergdolphm, ayoung does make valid points in his -2 comment on henry's review20:43
ayounghenrynash, 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 happend20:43
dolphmmorganfainberg: i guess i don't have much in the way of expectations for "sync"20:43
henrynashayoung: I don't disagree20:43
bknudsonI'd think if someone really needed longer IDs they would have modified their schema.20:43
morganfainbergbknudson, lol don't disagree20:44
morganfainbergbknudson, s/lol//20:44
ayoungbknudson, more correct to say "chose a different value"20:44
ayoungdiffernt attribute actually20:44
morganfainbergayoung, the more i think about it the more i _really_ dislike the @@ mechanism20:44
morganfainbergayoung, but barring a map table, I don't have a good answer20:45
henrynashayoung: 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 practice20:45
ayoungmorganfainberg what is wrong with @@?  Can you vocalize it20:45
ayounghenrynash, we discussed using DN at one point...I was firmly talked out of it20:45
morganfainbergayoung, i very much dislike exposing that much of the identity internals to services outside of keystone.20:45
ayoungit *is* the LDAP way for deconflicting20:46
ayoungmorganfainberg, they are going to make assumptions anyway20:46
ayounglike the code in the client that checked if an id was a uuid....20:46
morganfainbergayoung, you can't guarantee the DN is de-conflicted20:46
morganfainbergayoung, ldap server A could be bad and configured w/ the same base as b, and then we (keystone) can't trust it20:47
ayoungmorganfainberg, you can't guarantee UUIDs are deconfliceted strictly speaking20:47
ayoungbut, I agree20:47
*** wchrisj has quit IRC20:47
ayoungyou need to salt the answer20:47
morganfainbergayoung, also i think we are much much more likely to hit the 64 byte limit with the whole dn20:47
morganfainbergand the DN is not exactly url friendly20:47
ayoungDN is out.  I agree there20:47
morganfainberglike i said, i don't have a good answer20:48
ayoungso 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 openstack20:49
ayoungdolphm, lets discuss that, cuz it is the one thing that is, I think at the core of your support for this approach20:50
ayoungis it a problem if the ID has something about the user in it?20:50
morganfainbergdolphm, ++++++++++++++20:50
dolphmayoung: not really, it's in complete opposition to the whole argument and all solutions to it20:51
ayoungLDAP implies an internal call.  But what about Federation?  The solution for one should stand for both20:51
dolphmayoung: as it stands, federation generates tokens with meaningless, deployer defined IDs20:51
ayoungdolphm, there is some risk there20:51
ayoungdeployer defined can conflic20:51
dolphmayoung: absolutely20:51
ayoungso...either we map federated IDs, or we deconflict20:52
ayoungsame with LDAP20:52
dolphmayoung: if it were me, i'd configure my mapping to insert a static value there, like "ephemeral-user"20:52
dolphmayoung: or "do-not-track" ;)20:52
ayoungand 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
dolphmayoung: good questions, but we don't need to answer them immediately20:53
ayoungTHe issues are magnified with Federation20:53
morganfainbergayoung, well with federated you auth w/ not the ID20:53
dolphmayoung: i imagine the answer will look like token purging20:53
ayoungI want a single solution for both Federation and LDAP20:53
morganfainbergayoung, if we used uuid5() we could guarantee the same id.20:53
ayoungmorganfainberg, why not just make 10 louder?20:53
morganfainbergayoung, we could remove the record if there are no assignment mapping(s) if we support assignment to federated users vs group20:54
ayoungmorganfainberg, nope20:54
ayoungwe don't have all the cards20:54
morganfainbergayoung, or ignore it if you can't do an assignment to a federated user directly, which case we don't care20:54
morganfainbergayoung, e.g. not "store" the ID.20:55
ayounga user might be mapped via a group, and then there is some resource outside of Keystone that is associated with that user20:55
ayoungexactly...."Don't store the ID"  is what I am shooting for here.20:55
ayoungbut ephemeral users/mapping table stores the id20:55
morganfainbergyou can't do that w/ LDAP20:55
morganfainbergyou need to store the id20:55
*** arunkant has joined #openstack-keystone20:56
ayoungwe need to salt the IDs that come from LDAP and Federated20:56
morganfainbergand i thought ephemeral users were mapped to groups based upon the attributes20:56
morganfainberge.g. it's not "federated user id" -> group20:56
morganfainbergit's "user is part of group because it has attrs blah blah and blah""20:56
ayoungmorganfainberg, 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 will20:57
ayoungwe'll haveboth20:57
morganfainbergayoung, i think you just described a need for a mechanism to store the ID20:57
dolphmmorganfainberg: i'm playing with uuid5() since you mentioned it... what would be the namespace & name ?20:57
morganfainbergdolphm, namespace is the UUID.UUID4(<domain_id>)20:58
morganfainbergdolphm, name would be the other value (e.g. ldap, dn bit we use as Id today)20:58
henrynashmorganfainberg: but isn't the problem with uuid5 that we can't get the namespace back?20:58
morganfainberghenrynash, no we can't un-do it. but we can ensure the id is always the same.20:58
ayoungmorganfainberg, so one way hashes of the ID have exactly that problem20:58
ayoungbut...maybe that is ok20:58
dolphmmorganfainberg: can you send me a one liner? not sure i follow20:59
morganfainbergdolphm, dex20:59
ayoungwhat if...we say that "if you need to do a backwards mapping, you can't, but we can reconfirm a forwards mapping"20:59
dolphmmorganfainberg: that was an inexplicable typo20:59
henrynashmorganfainberg: 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 call20:59
ayoungIE, we couldn't get user by userid,  but we could always confirm a given user has a specific userid21:00
morganfainberg>>> uuid.uuid5(uuid.UUID('c4729c137e3742788b38786ca52fa68d'), "something cool").hex21:00
dolphmayoung: so you're proposing we avoid solving the problem of "what identity backend did this user come from?"21:00
morganfainbergdolphm, you'd reconstruct the UUID object as the namespace21:00
ayoungdolphm, yep...just chewing on it21:00
morganfainberghenrynash, we'd still need a map table21:00
henrynashmorganfainger:, ok21:00
morganfainberghenrynash, nice!21:01
dolphmmorganfainberg: interesting...21:01
ayoungdolphm, so long as mapped attribute->userid is always unique and reproducible...21:01
morganfainbergayoung, that was my thought.21:01
morganfainbergayoung, it is sha1, so there is potential for conflict...21:01
ayoungso if I want to assign to ayoung@@redhat   versus ayoung@@rackspace I get a uniqe value21:02
morganfainbergdolphm, we could use hashlib if we wanted instead.21:02
dolphmayoung: why 256?21:02
morganfainbergayoung, uuid5 is sha1 iirc21:02
morganfainbergayoung, but we could just use hashlib, same net effect21:02
ayoungdolphm, as I recall it was recommended...I'd have to dig21:02
morganfainbergayoung, 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 sha121:03
morganfainbergorwhatever iut uses21:03
ayoungdolphm, 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
morganfainbergayoung, a .. chain? or two patchsets?21:04
ayoungno, two reviews21:04
ayoungsame changeset21:04
morganfainbergayoung, oh21:04
ayoungjust one will be the old version rebased21:04
morganfainbergi see21:04
ayoungI can't just push "rebase" due to minor changes that have happend outside of my code.21:05
dolphmmorganfainberg: sha256 produces 32 *bytes* / 64 chars21:05
dolphmayoung: the patch is so big that i have yet to go through the entire thing at once anyway21:05
ayoungdolphm, fair enough21:06
dolphm+2k lines is nuts21:06
morganfainbergdolphm, hashlib.sha256 hex is 64 bytes21:06
morganfainberguuid4/5 is 32 bytes21:06
dolphmmorganfainberg: sha356(s).hexdigest() is 64 chars21:07
dolphmpython -c "import hashlib; print(hashlib.sha256('something').hexdigest())"21:07
morganfainberghashlib.sha256(<data>).hexdigest() -> '476749ec53f7627b81502123d68303ce5e128076913532804ce0ecc38969e5ab'21:08
ayoungOK, 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 them21:09
dstanekstyle 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
ayoungYou'd have to set Old Version History  to Patch Set 73 and then click on 71 to see them21:10
ayoungdstanek, I don't care. Concerned that it leads to  bike shedding.21:12
dstanekayoung: nice, that should make it much easier21:12
ayoungdstanek, yeah...the diff thing is wonky21:12
ayoungOH, I need to post my comments21:12
morganfainbergdstanek, we should just propose a change to hacking to enforce block comments to work like that21:12
morganfainbergdstanek, i like having a space, way easier to read21:12
dstanekayoung: bike shedding about the number of spaces?21:12
ayoungdstanek, yep21:13
ayoungand format in general21:13
morganfainbergdstanek, but i wouldn't -1 for it21:13
ayoungif we can't write an automated tool for it, forget it21:13
morganfainbergunless it's in hacking21:13
bknudsonthat should be an easy one for hacking to enforce21:13
dstanekmorganfainberg: over the weekend i added a bunch of custom checks; this is just one of them21:13
morganfainbergbknudson, ++21:13
ayoungdolphm, 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 explicitly21:14
dstaneki did that, mutables as default args, no % in log statements, assertNone when using None with assertEqual21:14
dstanekand i think 1 or two more21:14
morganfainbergdstanek, nice21:14
dstaneki got tired of saying the same things in reviews over and over again21:15
ayoungmaking it default would require making sure we ran the migration, otherwise deploying  will break21:15
bknudsondstanek: where's the code?21:15
ayoungdstanek, 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
dstanekbknudson: i just picked it up again now - going to started submitting reviews to get our code to pass my new checks and also the checks21:16
dstanekayoung: yep, it should soon :-)21:16
dstanekoh, and i added a spelling check for comments and docstrings21:17
ayoungdstanek, 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 it21:17
dstaneknot sure if that one will actually work out21:17
ayoungdolphm, 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
morganfainbergayoung, hm. doesn't henry's patch store the other data as well in the map table? or are you thinking w/o the mapping table21:21
morganfainbergayoung, for audit reasons you can lookup id->external info21:22
dolphmmorganfainberg: he's trying to avoid the lookup table21:22
morganfainbergdolphm, ah21:22
dolphmso i proposed a giant delete of KDS ... i'm trying to figure out how to create a feature branch now...
morganfainbergdolphm, might be a infra question.21:24
*** leseb has joined #openstack-keystone21:25
ayoungmorganfainberg, it does seem to indicate  "mapping is forever"21:27
ayoungand needs to be reversable21:28
morganfainbergayoung, yeah.21:28
ayoungso really it is a question of recording the mapping versus calculating in a reversable manner21:28
ayoungcalculateing from ID to attribute is going to "leak"21:28
ayoungI'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 userid21:30
dstanekmorganfainberg, ayoung, bknudson: i'm going to track my changes against
ayoungmorganfainberg, then make the mapping table an extension for people that are willing to accept it and want the anonymity21:31
morganfainbergayoung, 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
ayoungmorganfainberg, nah, just a few  vocal opponents.  One of whom is jaypipes, mind you, and I respect him a lot21:32
ayounghenrynash, you still awake?21:32
morganfainbergayoung, i much prefer a consistent user_id format, rather than something where someone could have 'dksajfasdkjfksd@@@#$!!!' because saml21:32
morganfainbergor whatever in their name.21:32
ayoungmorganfainberg, I would say "deafult is composite, uuid is extension"21:33
morganfainbergayoung, i'd go the other way tbh21:33
ayoungmorganfainberg, no, make the mapping table optional21:33
morganfainbergayoung, i still don't like exposing the internals21:33
ayoungmorganfainberg, I don't like UUIDs21:33
ayoungwe all have our El Guapos to face21:34
morganfainbergayoung, is it possible for us to get non-url safe characters into the username via saml?21:34
ayoungmorganfainberg, almost certainly21:34
morganfainbergayoung, or via a CN21:34
morganfainbergthen we need to also urlencode the user_id.21:34
ayoungmorganfainberg, its based o nwhatever attribute the LDAP admin choses21:34
ayoungif they break it, they bought it21:35
morganfainbergayoung, no.21:35
ayounga very big yes21:35
ayoungso much more than this will break21:35
morganfainbergayoung, 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 IRC21:36
ayoungHeh, I chased henrynash away21:36
ayoungbecause they did something not sane with their value21:36
morganfainbergayoung, then why are we even trying to care about uniqueness?21:37
morganfainbergayoung, it's the deployer's fault if the idps aren't providing unique data21:37
ayoungmorganfainberg, yes, but they can step on each other21:37
morganfainbergayoung, but by that argument, @@ isn't needed either21:37
ayoungI don't care if they mess themselves up,  just not other customers21:38
morganfainbergayoung, same argument dude, IDP provides bad data, and is not usable in keystone.  bug is opened, we need to fix21:38
ayoungand we say "fix your mapping"21:38
ayoung"user id field must be url safe"21:39
morganfainbergayoung, negative21:39
ayoungwe do that already21:39
*** jsavak has quit IRC21:39
morganfainbergayoung, sure we do that now, but we're also now accepting outside data21:39
ayoungwe allow one LDAP server only21:39
morganfainbergayoung, and if we are "fixing this" we should fix it right.21:39
morganfainbergnot go ½ way21:39
ayoungdon't try to fix this21:40
ayoungthis is LDAP21:40
morganfainbergayoung, i think you're 100% in the wrong here21:40
morganfainbergif we are accepting the data we should ensure it's usable across the api21:40
morganfainbergif we are masking the data w/ our own uuid we can ignore url safety21:41
morganfainbergor we need to validate lack of url safety and throw an exception...and wait for someone to open a bug for us to fix it21:41
morganfainbergnot let the bad data get into keystone at all21:41
dolphmayoung: 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 LDAP21:42
dolphmzero limitations on the LDAP user id attribute value21:42
dolphmit can literally be a blob in keystone21:42
ayoung"We can solve any problem by introducing an extra level of indirection."21:43
*** david-lyle has quit IRC21:43
ayoungmorganfainberg, you are going off on a tangent here.  LDAP as is is fine21:43
ayoungits the multi domain thing that is broken21:43
dolphmbknudson: i've been trying to type 'blk-u' in IRC all day and thinking you weren't on21:44
dolphmbknudson: anyway, just a heads up that i revised the commit message in with a link to the feature branch21:44
ayoungand that is broken for LDAP, and in Federation.  URL Encoding is a different issue.21:44
dolphmayoung: an hour ago you were talking about how broken LDAP was with length-limited ID's...21:45
dolphmayoung: mutli-domain just compounds the problem with additional requirements21:45
ayoungdolphm, 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 burden21:46
ayoungthe multi domain thing is outside that21:46
dolphmayoung: unnecessary restrictions that a mapping table could solve for21:47
morganfainbergdolphm, ++21:47
ayoungit is why ryan_lane was saying "don't do it"21:47
dolphmayoung: link?21:47
ayoungdolphm, Ryan's comments were either in IRC or mailing list, a long while back21:47
ayounghe's in #openstack-dev if you really want21:48
ayounglength of the field and URL encoding are red herrings here21:48
ayounghaving two users named ayoung coming from different IdPs is the proble,m21:49
dolphmayoung: i pinged him21:49
bknudsonit would be nice if we could merge some of the changes that will conflict with kds removal21:50
bknudsonalthough rebase to remove the changes isn't a big deal either21:50
*** joesavak has joined #openstack-keystone21:50
*** stevemar has quit IRC21:51
ayoungbknudson, dstanek  want to see   how Zuul is treating your patch?
bknudsondolphm: for example
*** harlowja is now known as harlowja_away22:05
*** harlowja_away is now known as harlowja22:05
*** henrynash has joined #openstack-keystone22:06
henrynashayoung: yep22:06
*** marcoemorais has quit IRC22:07
*** jamielennox is now known as jamielennox|away22:07
henrynashayoung: just re-joined22:07
ayounghenrynash, 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.   Sportsmanlike22:07
*** gokrokve has joined #openstack-keystone22:08
*** ayoung is now known as ayoung_afk22:09
henrynashayoung: 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-keystone22:15
dolphmbknudson: i did one as a test22:16
dolphmbknudson: git checkout feature/key-dist; git pull; git review -x [existing review number]; git review22:17
dolphmbknudson: it created a new review in gerrit (since it's on a different branch) and I -2'd the one on master22:17
*** joesavak has quit IRC22:22
*** henrynash has quit IRC22:35
bknudsondolphm: we'll have to do that for the oslo.db syncs.22:36
*** nkinder has quit IRC22:37
*** jamielennox|away is now known as jamielennox22:47
jamielennoxdolphm, bknudson: so is it a feature branch for KDS?22:51
jamielennoxhow do we go about splitting it out22:52
jamielennoxbknudson: yep, i just saw that one come through22:52
*** dims_ has quit IRC22:52
jamielennoxbut that is a minor patch that i had put through, not a split out22:52
bknudsonjamielennox: that change was made to the feature branch22:53
jamielennoxbknudson: 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
jamielennoxbknudson: ah - i need to pay closer attention to the keystone review emails22:54
dolphmjamielennox: reading back...22:55
dolphmjamielennox: so, the feature branch already exists based on the current master22:55
jamielennoxdolphm: the rm KDS patch misses the requirements.txt additions22:55
dolphmjamielennox: ooh, good call22:56
bknudsondolphm: 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
dolphmbknudson: 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 branch22:56
dolphmsince we're going about the feature branch business all backwards22:56
jamielennoxi wrote this more for the barbican guys but:
bknudsondolphm: yes, revert it in the feature branch22:57
dolphmjamielennox: bknudson: removed KDS-specific requirements
jamielennoxdolphm: if we branch like that can i just delete all the keystone stuff and move KDS to the root?22:59
dolphmjamielennox: NICE!22:59
dolphmjamielennox: lol probably22:59
jamielennoxi 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 issues23:00
dolphmjamielennox: very true23:00
bknudsonwe're not going to merge it back?23:00
jamielennoxbknudson: not as a contrib to keystone as i see it23:00
bknudsonI've got a change that touched the kds code --
bknudsondo we want that merged before kds is deleted, or before the branch has diverged?23:01
jamielennoxthere is this one too:
dolphmbknudson: it's already forked, so landing the delete would be more appropriate so they don't diverge23:01
dolphmdelete first*23:02
bknudsondolphm: not sure what's going to happen... I cherry-pick the change as is to the feature branch?23:02
bknudsonotherwise I'm going to rebase it and then it'll lose all the kds changes.23:03
bknudsonnot that it's hard to make the changes.23:03
dolphmbknudson: you want to land your change to master, right?23:03
bknudsondolphm: yes, the changes are mostly for keystone... just also affected kds23:03
jamielennoxit works for me to have the db sync first23:03
dolphmbknudson: i'd suggest rebasing onto the KDS delete in master then23:05
morganfainbergdolphm, from a code stand-point i think limited trusts is looking good to me.23:05
bknudsondolphm: ok, either way works for me.23:05
bknudsonmorganfainberg: I'd like to run the migration with db2 first...23:05
morganfainbergbknudson, yeah sounds good23:06
dolphmbknudson: have time for that today?23:06
bknudsonI'll work on it right now.23:06
morganfainbergbknudson, i haven't +2'd  i was going to +1 and wait for you23:06
dolphmbknudson: otherwise, any issues can be RC-blockers23:06
*** dims_ has joined #openstack-keystone23:08
morganfainbergbknudson, +1 and i'll leave the +2 for you based on DB2 stuff23:08
*** wchrisj has joined #openstack-keystone23:09
jamielennoxdo we have a solution for this yet:
jamielennoxit is essentially another attempt at ayoung's mangle the URL for v323:09
jamielennoxi was going to try to do it as part of a summit session but we may not have that kind of time23:10
dolphmjamielennox: i think i'm in favor of the approach, but i want to make sure the impact is limited to keystone23:10
dolphmjamielennox: i don't want to start re-writing the entire catalog for other services, etc23:11
morganfainbergdolphm, ++23:11
jamielennoxdolphm: i do prefer that one to ayoungs23:11
morganfainbergdolphm, jamielennox, i think that is the cleanest approach provided it only impacts keystone23:11
morganfainbergcleanest given all the other bits we need to address23:11
dolphmthe else: pass is silly there23:12
jamielennoxi'm not sure it works with some of my session stuff..23:12
jamielennoxbut i'm assuming i can find the right place for it23:12
dstanekmorganfainberg: just saw the note about limited use trusts - was just about to +2 it23:14
jamielennoxactually it will just be deprecated by any client using an auth plugin which is fine by me23:15
morganfainbergdstanek, that is why i left that note :)23:15
*** thedodd has quit IRC23:15
dstanekmorganfainberg: 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 there23:17
dolphmjamielennox: and re: your comment -- i'd definitely like to land this (or something like it) before icehouse ships23:17
dolphmjamielennox: after the summit would be painful, as the number of bug reports / confusion around this is already building23:18
jamielennoxdolphm: yea, i kind of agree - if we keep it in process_token like the current patch proposes then it's nice and isolated23:18
jamielennoxit will only affect client v3 working as it currently does23:18
dolphmjamielennox: ++23:18
jamielennoxit won't work for this passing session so i'll need to figure out a work around for that23:19
*** david-lyle has joined #openstack-keystone23:19
jamielennoxi didn't like it when he had it in __init__23:19
dolphmjamielennox: __init__ of what?23:19
dolphmjamielennox: i'll +2 when you're good with it23:20
dolphmjamielennox: slash, i also want to check it out and play with it23:20
bknudsonisn't "filter_by(deleted_at=None)." going to break the sqlalchemy code?23:20
bknudsonmorganfainberg: you had a change that was similar to these.23:20
dolphmbknudson: how so?23:21
morganfainbergbknudson, oh crap sec23:21
dolphmbknudson: it's a copy/paste from get_trust() since there's no _get_trust() in that driver23:21
bknudsonis it just the delete that cares?23:21
morganfainbergbknudson, oh yeah23:21
morganfainbergbknudson, filter_by shouldn't be an issue23:21
bknudsonok, thanks.23:22
bknudsonmorganfainberg: what is it with delete()?23:22
morganfainbergits that you're doing non dialect specific stiuff with .delete(<thing>) vs .delete().where23:22
morganfainbergbknudson, they were doing an embedded where clause in the .delete() before rather than constructing the query23:22
dolphmmorganfainberg: link for my edumacation?23:22
morganfainbergdolphm, table.delete(id=<thing>)23:23
morganfainbergvs table.delete().where( == <thing>)23:23
morganfainbergdolphm, in sqlalchemy 0.9.3 the former is non-dialect specific and will raise an exception23:24
morganfainbergdolphm, i assume it is because there is no type checking / casting23:24
dolphmmorganfainberg: ah, hrm23:24
morganfainbergdolphm, when you do you're referencing the table datatype/colum info23:24
bknudsonhopefully we'll be on sqlalchemy 0.9.3 soon.23:24
*** achampion has quit IRC23:25
bknudsonthe upgrade works fine on db2, but it's got hard-coded sql so I'd rather that wasn't there.23:25
morganfainbergaccording to zigo that was the only issue we had in keystone for 0.9.323:25
bknudsonsince this has caused problems in the past with different dbs23:25
morganfainbergand it was specific for a test case23:25
morganfainbergso, easy to fix23:25
bknudsonI mean there's hardcoded SQL in the upgrade test : -- line 207823:27
bknudsonthis file is too big.23:27
dolphmbknudson: "even better" but no +2? :P
bknudsondolphm: I was waiting for jenkins23:28
*** nkinder has joined #openstack-keystone23:28
*** jagee has quit IRC23:31
dolphmbknudson: 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
bknudsondolphm: I think it got caught in between another change that merged on it.23:35
bknudsonit worked on 2/28 then failed on 3/223:36
*** lbragstad has quit IRC23:36
*** dims_ has quit IRC23:46
*** andreaf has joined #openstack-keystone23:48

Generated by 2.14.0 by Marius Gedminas - find it at!