| morganfainberg | dolphm, https://review.openstack.org/#/c/69162/5/keystone/trust/controllers.py this looks like a new string to me. (line 139), is this something we need to hold for Juno then? | 00:04 |
|---|---|---|
| *** derek_c has quit IRC | 00:04 | |
| *** gokrokve has quit IRC | 00:20 | |
| *** derek_c has joined #openstack-keystone | 00:35 | |
| openstackgerrit | A change was merged to openstack/keystone: Sync db, db.sqlalchemy from oslo-incubator 0a3436f https://review.openstack.org/78429 | 00:39 |
| openstackgerrit | A change was merged to openstack/keystone: Filter out nonstring environment variables before rules mapping. https://review.openstack.org/80293 | 00:39 |
| jamielennox | found another use for @positional i can remove things from the argument line and pass them through as kwargs (eg if i want to pass it through to underlying functions) without having to do arg=arg everywhere for compatability | 00:41 |
| *** leseb has joined #openstack-keystone | 00:42 | |
| *** leseb has quit IRC | 00:46 | |
| *** gokrokve has joined #openstack-keystone | 00:47 | |
| *** jraim has quit IRC | 00:53 | |
| *** jraim has joined #openstack-keystone | 00:55 | |
| *** lbragstad has joined #openstack-keystone | 01:03 | |
| *** marcoemorais has quit IRC | 01:05 | |
| gyee | jamielennox, I am blocking https://review.openstack.org/#/c/78241/ for now | 01:08 |
| gyee | I don't think the bug is valid | 01:08 |
| jamielennox | gyee: ok with me - you've seen the security bug that is associated with it? | 01:08 |
| gyee | looking | 01:09 |
| gyee | jamielennox, which security bug, if we want to update th doc on the implication of caching then that's fine | 01:13 |
| *** stevemar has joined #openstack-keystone | 01:14 | |
| gyee | nm | 01:14 |
| jamielennox | gyee: oh no - it's just the linked bug it's public now | 01:15 |
| gyee | that bug is an unrealistic expectation | 01:19 |
| gyee | security is about risk management, nothing is absolute | 01:19 |
| dstanek | gyee: does it really make the cache useless? | 01:19 |
| gyee | dstanek, yes, we are essentially revalidating the token | 01:20 |
| dstanek | gyee: i thought it was just checking to see if it was in the revoked list cached on the client side | 01:20 |
| gyee | dstanek, line 836-840 | 01:22 |
| dstanek | gyee: ah, i see. with a tiny change it could be made to work. | 01:24 |
| gyee | token_cache_time is a essentially a tradeoff between performance and security | 01:24 |
| ayoung | gyee, you are wrong to block that bug | 01:25 |
| dstanek | gyee: jas, i'll show you what i mean | 01:26 |
| ayoung | you can avoid the round trip if you set the memcached timeout to shorter time than the revocation timeout | 01:26 |
| ayoung | "round trip" meaning fetch from the server | 01:26 |
| gyee | sure | 01:27 |
| ayoung | it does not render the cache useless | 01:27 |
| ayoung | it makes sure that a cached token is still checked against the revocation list | 01:27 |
| gyee | ayoung, then there's nothing to change | 01:27 |
| ayoung | gyee, no | 01:27 |
| dstanek | http://paste.openstack.org/show/73716/ <- gyee | 01:28 |
| dstanek | gyee: would that be worth having? | 01:28 |
| ayoung | if you validate a token with PKI or with UUID, and then cache it for a long period of time, you will miss and revocations | 01:28 |
| ayoung | any | 01:28 |
| gyee | ayoung, what is cache used for? | 01:28 |
| ayoung | cache is used so that you do not need to go back to the server to fetch the data again for each token | 01:29 |
| ayoung | one rvoation list fetch every 5 minues is still preferable to a round trip per token (uuid) | 01:29 |
| ayoung | the original version was just for PKI tokens, but it really should not matter | 01:29 |
| ayoung | so why are you blocking it? | 01:30 |
| *** morganfainberg is now known as morganfainberg_Z | 01:30 | |
| gyee | ayoung, because the current change render caching useless | 01:30 |
| ayoung | gyee, and, to jamielennox s point, just cuz a token is valid does not mean that it is valid from anywhere. THis patch makes sure that the bind is always checked | 01:31 |
| ayoung | you are wrong | 01:31 |
| ayoung | the current change checks against revocation list | 01:31 |
| jamielennox | oh, he fixed the bind thing? | 01:31 |
| jamielennox | i haven't looked for a few days | 01:31 |
| gyee | nope | 01:31 |
| ayoung | jamielennox, yep | 01:31 |
| ayoung | https://review.openstack.org/#/c/78241/4/keystoneclient/middleware/auth_token.py | 01:31 |
| ayoung | gyee, so...there is one thing that I thought you were going to say, and I think it is what you are driving at | 01:32 |
| ayoung | a UUID token based system now depends on the revocation list | 01:32 |
| ayoung | but that should be acceptable | 01:32 |
| jamielennox | hmm, i'm not sure he's got what he is looking for there | 01:32 |
| jamielennox | it means caching won't work on signed tokens | 01:33 |
| ayoung | huh? | 01:33 |
| gyee | jamielennox, ayoung, what's bind part are we talking about | 01:33 |
| jamielennox | and he is doing an expired check and a _cache_put that he doesn't need to do for cached tokens | 01:33 |
| gyee | dstanek, I think your alternative looks more reasonable | 01:33 |
| ayoung | jamielennox, cache.put is maintained | 01:33 |
| ayoung | jamielennox, I wrote that last version of the patch | 01:34 |
| jamielennox | gyee: a token bind should be checked on every token usage, if it was stored in cache in wasn't being checked | 01:34 |
| jamielennox | ayoung: right cache.put doesn't need to be called for already cached tokens | 01:34 |
| gyee | jamielennox, easy, just turn off cachine! | 01:34 |
| gyee | caching | 01:34 |
| ayoung | jamielennox, true. | 01:34 |
| jamielennox | ayoung: and memcache will invalid the cached object after expiry time so you don't need that check either | 01:34 |
| gyee | if you need to check the token everytime then what's the point of caching | 01:34 |
| ayoung | jamielennox, only if token expiry time is the same as the memcache expiry | 01:35 |
| gyee | just set the token_cache_time to -1 | 01:35 |
| ayoung | and they are not | 01:35 |
| jamielennox | they are not/ | 01:35 |
| jamielennox | they are not? | 01:35 |
| ayoung | jamielennox, lets say you set memcahe expiry for 10 mijnutes nad token expiry is an hour | 01:35 |
| ayoung | tjhen at 59 minutes, you check the token | 01:35 |
| ayoung | well, every 10 minutes, up until just before the token expires... | 01:35 |
| jamielennox | ayoung: oh wtf did they do that for | 01:36 |
| ayoung | so if you check at 59 minutes, you've just extended the token for 9 minutes past its expiry | 01:36 |
| jamielennox | _cache_put takes an expires argument so i just assume they were using that as the timeout | 01:36 |
| jamielennox | it should store to cache with min(expires, self.token_cache_time) | 01:36 |
| ayoung | actually, I am wrong | 01:37 |
| gyee | jamielennox, it is, take a look at cache_get | 01:37 |
| gyee | it check the expiry there | 01:37 |
| jamielennox | gyee: yea, but that calls _cache_store which puts it into cache with self.token_cache_time | 01:37 |
| ayoung | so the puit is unnecessary | 01:37 |
| ayoung | can wrap that in an if cached: | 01:37 |
| jamielennox | so it's checking expires by why bother when memcache is doing that for us | 01:38 |
| jamielennox | caching in auth_token has been a known disaster for a while | 01:38 |
| gyee | jamielennox, but the net effect is the same | 01:38 |
| gyee | a token will not be valid past expiry | 01:40 |
| *** david-lyle has joined #openstack-keystone | 01:42 | |
| gyee | anyway, dinner time, be back later | 01:42 |
| jamielennox | gyee: yep - net effect is the same | 01:43 |
| ayoung | jamielennox, I was trying to keep the patch as straight forward as possible: treat the cache as only unmarshalled and semarntically checked data, but all of the validity checking for roles etc needs to be confirmed with the revocation list. | 01:45 |
| jamielennox | ayoung: it's going to need a redo soon anyway | 01:46 |
| jamielennox | by that i mean we can do the fixups later if we solve the bug | 01:46 |
| ayoung | sure, but a -2 blocker is not called for. | 01:46 |
| ayoung | We can change the logic now to avoid the spurious put, anything else you see there> | 01:46 |
| ayoung | ? | 01:46 |
| *** morganfainberg_Z is now known as morganfainberg | 01:49 | |
| *** morganfainberg is now known as morganfainberg_Z | 02:08 | |
| jamielennox | do you think a version discovery 'status' of 'supported' is deprecated | 02:14 |
| jamielennox | glance lists all it's versions in discovery | 02:14 |
| jamielennox | so it has eg v2.2 as 'CURRENT', then v2.1, v2.0 as 'SUPPORTED' pointing to the same URL | 02:14 |
| *** harlowja is now known as harlowja_away | 02:21 | |
| *** zhiyan_ is now known as zhiyan | 02:28 | |
| openstackgerrit | Jamie Lennox proposed a change to openstack/python-keystoneclient: Revamp discovery https://review.openstack.org/81146 | 02:39 |
| openstackgerrit | Jamie Lennox proposed a change to openstack/python-keystoneclient: Version independent password authentication plugin https://review.openstack.org/81147 | 02:39 |
| openstackgerrit | Jamie Lennox proposed a change to openstack/python-keystoneclient: Discover should support other services https://review.openstack.org/72878 | 02:39 |
| *** richm has quit IRC | 02:41 | |
| *** mberlin1 has joined #openstack-keystone | 02:59 | |
| *** mberlin has quit IRC | 03:00 | |
| *** amcrn has quit IRC | 03:01 | |
| *** harlowja_away is now known as harlowja | 03:06 | |
| *** daneyon has quit IRC | 03:37 | |
| *** daneyon has joined #openstack-keystone | 03:38 | |
| *** morganfainberg_Z is now known as morganfainberg | 03:52 | |
| ayoung | morganfainberg, I think that the revoke tree algorithm is broken. I think that one "dailure to match" which causes short circuit logic can mask other paths that would match the tokne (and thus show it is revoked) | 03:58 |
| ayoung | I'm headed to bed now, but wanted to share before I did | 03:59 |
| morganfainberg | ayoung the return out because the filter list is empty? | 03:59 |
| ayoung | yeah | 03:59 |
| ayoung | that is way to aggressive | 03:59 |
| morganfainberg | ayoung, i'll take a closer look at it, but yeah | 03:59 |
| ayoung | there are unpruned paths still in the tree. | 03:59 |
| morganfainberg | i think we need to build the whole tree then do the match | 03:59 |
| ayoung | I think the logic is something like this | 03:59 |
| morganfainberg | vs. short circuit | 03:59 |
| ayoung | revoke a role | 03:59 |
| ayoung | then delete a user | 04:00 |
| ayoung | token for the user with a different role is still considered valid | 04:00 |
| ayoung | the deliberate match path gets taken first | 04:00 |
| ayoung | for the role_id match | 04:00 |
| ayoung | and once it fails out, none of the wildcard paths are taken | 04:01 |
| morganfainberg | yeah | 04:01 |
| ayoung | I've seen problems on both "delete user" and "deleting_project" which show this behavior | 04:01 |
| ayoung | test_revoking role seems to be even more succeptable. | 04:02 |
| ayoung | I'm going to post my code WIP | 04:02 |
| morganfainberg | k | 04:02 |
| ayoung | morganfainberg, https://review.openstack.org/#/c/81166/ its a draft | 04:03 |
| ayoung | to see the behavior, run a server with revocation events enabled, and then run examples/scripts/generate_revocation_events.py | 04:04 |
| morganfainberg | ok | 04:04 |
| morganfainberg | i'll take a look | 04:04 |
| ayoung | morganfainberg, its not "build the whole tree" but rather "search the whole tree" | 04:04 |
| ayoung | morganfainberg, I did the following | 04:04 |
| ayoung | put a breakpoint right before test_deleting_user | 04:05 |
| ayoung | then ran from the cli | 04:05 |
| ayoung | mysql keystone -u keystone -e "delete from revocation_event;" | 04:05 |
| ayoung | and the test passed | 04:05 |
| ayoung | same thing for test_deleting_project | 04:05 |
| ayoung | the middle test (delete role) still failed | 04:05 |
| ayoung | so it is definitely masked events | 04:06 |
| ayoung | ok, gnight | 04:06 |
| *** ayoung is now known as ayoung__zzzZZZZ | 04:06 | |
| ayoung__zzzZZZZ | morganfainberg, I think what is missing is that it should not be a return, it should be "remove this path from the bundle." and only if the bundle is empty, with no valid paths to pursue, should be exit out | 04:11 |
| ayoung__zzzZZZZ | ok, really to bed now | 04:12 |
| morganfainberg | ayoung__zzzZZZZ, lol nighut | 04:12 |
| *** chandan_kumar has joined #openstack-keystone | 04:31 | |
| *** derek_c has quit IRC | 04:47 | |
| *** stevemar has quit IRC | 05:07 | |
| *** derek_c has joined #openstack-keystone | 05:10 | |
| *** gyee has quit IRC | 05:11 | |
| *** gokrokve has quit IRC | 05:26 | |
| *** marcoemorais has joined #openstack-keystone | 05:27 | |
| *** marcoemorais1 has joined #openstack-keystone | 05:29 | |
| *** marcoemorais has quit IRC | 05:32 | |
| *** harlowja is now known as harlowja_away | 05:40 | |
| *** derek_c has quit IRC | 05:56 | |
| *** gokrokve has joined #openstack-keystone | 05:57 | |
| *** gokrokve_ has joined #openstack-keystone | 06:03 | |
| openstackgerrit | Jenkins proposed a change to openstack/keystone: Imported Translations from Transifex https://review.openstack.org/78525 | 06:05 |
| *** gokrokve has quit IRC | 06:06 | |
| *** gokrokve_ has quit IRC | 06:08 | |
| openstackgerrit | wanghong proposed a change to openstack/keystone: remove redundant code in catalog/core.py https://review.openstack.org/81181 | 06:21 |
| *** derek_c has joined #openstack-keystone | 06:25 | |
| openstackgerrit | wanghong proposed a change to openstack/keystone: support conventional domain name with one or more dot https://review.openstack.org/79829 | 06:27 |
| *** gokrokve has joined #openstack-keystone | 06:29 | |
| *** gokrokve has quit IRC | 06:33 | |
| openstackgerrit | A change was merged to openstack/keystone: trust creation allowed with empty roles list https://review.openstack.org/69162 | 06:42 |
| *** saju_m has joined #openstack-keystone | 06:44 | |
| *** saju_m has quit IRC | 06:45 | |
| *** saju_m has joined #openstack-keystone | 06:47 | |
| openstackgerrit | A change was merged to openstack/keystone: Provide option to make domain_id immutable https://review.openstack.org/80769 | 07:10 |
| *** gokrokve has joined #openstack-keystone | 07:29 | |
| *** gokrokve has quit IRC | 07:34 | |
| *** derek_c has quit IRC | 07:35 | |
| openstackgerrit | Morgan Fainberg proposed a change to openstack/keystone: Uses explicit imports for _ https://review.openstack.org/58766 | 08:24 |
| *** gokrokve has joined #openstack-keystone | 08:30 | |
| *** morganfainberg is now known as morganfainberg_Z | 08:34 | |
| *** gokrokve has quit IRC | 08:35 | |
| *** saju_m has quit IRC | 08:50 | |
| openstackgerrit | Jenkins proposed a change to openstack/python-keystoneclient: Updated from global requirements https://review.openstack.org/79695 | 08:51 |
| *** henrynash has joined #openstack-keystone | 08:52 | |
| *** saju_m has joined #openstack-keystone | 08:56 | |
| *** andreaf has joined #openstack-keystone | 09:02 | |
| *** leseb has joined #openstack-keystone | 09:02 | |
| *** leseb has quit IRC | 09:16 | |
| *** leseb has joined #openstack-keystone | 09:17 | |
| *** leseb has quit IRC | 09:21 | |
| *** andreaf has quit IRC | 09:23 | |
| *** leseb has joined #openstack-keystone | 09:24 | |
| *** gokrokve has joined #openstack-keystone | 09:31 | |
| *** gokrokve has quit IRC | 09:35 | |
| *** marcoemorais1 has quit IRC | 09:53 | |
| *** cflmarques has joined #openstack-keystone | 09:54 | |
| cflmarques | Hi guys.Does Keystone use oslo to send notifications events? "tenant creation/deleted... | 09:54 |
| cflmarques | I manage to retrieve nova events, but I also needed keystone events. does it send notifications or it must be enabled on the conf file? | 09:54 |
| cflmarques | can anyone enlighten me on this matter? | 09:55 |
| chmouel | cflmarques: just a quick google search leaded me to this http://docs.openstack.org/developer/keystone/event_notifications.html | 10:05 |
| *** andreaf has joined #openstack-keystone | 10:06 | |
| cflmarques | chmouel:hi chomouel. Thank you for your reply. Does keystone send that notifications over RabbitMQ like nova? | 10:10 |
| chmouel | cflmarques: it uses oslo.notify AFAIK from https://blueprints.launchpad.net/keystone/+spec/notifications | 10:11 |
| cflmarques | chmouel: Thanks again chmouel. I will dig a little more. :) | 10:21 |
| openstackgerrit | Dolph Mathews proposed a change to openstack/python-keystoneclient: Docs link to middlewarearchitecture https://review.openstack.org/70611 | 10:30 |
| openstackgerrit | Dolph Mathews proposed a change to openstack/python-keystoneclient: Docs link to middlewarearchitecture https://review.openstack.org/70611 | 10:30 |
| *** bvandenh has joined #openstack-keystone | 10:31 | |
| *** gokrokve has joined #openstack-keystone | 10:32 | |
| *** gokrokve has quit IRC | 10:36 | |
| *** cflmarques has quit IRC | 11:12 | |
| *** rwsu has quit IRC | 11:13 | |
| *** rwsu has joined #openstack-keystone | 11:14 | |
| openstackgerrit | Dolph Mathews proposed a change to openstack/keystone: Ability to turn off ldap referral chasing https://review.openstack.org/78521 | 11:20 |
| openstackgerrit | Dolph Mathews proposed a change to openstack/keystone: Ability to turn off ldap referral chasing https://review.openstack.org/78521 | 11:21 |
| openstackgerrit | Yuriy Taraday proposed a change to openstack/keystone: Fix is_revoked algorithm and add some comments to it https://review.openstack.org/81235 | 11:27 |
| *** gokrokve has joined #openstack-keystone | 11:32 | |
| *** gokrokve has quit IRC | 11:37 | |
| *** leseb has quit IRC | 11:39 | |
| *** leseb has joined #openstack-keystone | 11:40 | |
| *** leseb_ has joined #openstack-keystone | 11:41 | |
| *** leseb has quit IRC | 11:45 | |
| *** david-lyle has quit IRC | 11:46 | |
| openstackgerrit | Dolph Mathews proposed a change to openstack/keystone: Use oslo db.sqlalchemy.session.EngineFacade.from_config https://review.openstack.org/78459 | 11:48 |
| *** cflmarques has joined #openstack-keystone | 11:50 | |
| cflmarques | chmouel: Hi chmouel again. I manage to get keystone events already :) Just to let you you know. Thank's | 11:52 |
| dstanek | this title 'malformed endpoint URLs are destroying the API' attracked by eye on https://bugs.launchpad.net/keystone/+bug/1230279 | 11:59 |
| dolphm | dstanek: i love that title | 12:13 |
| dolphm | it's like the fox news of bug reports | 12:16 |
| dims | LOL :) | 12:18 |
| dstanek | :-) | 12:21 |
| *** thiagop_ is now known as thiagop | 12:21 | |
| dstanek | dolphm: that problem would appear to exist in v2 and v3, but the original fix was only for v3 | 12:21 |
| dstanek | dolphm: blah, scratch that. i'm not typing i'm thinking | 12:22 |
| dstanek | it does exist in v2 and v3, but only fixed in the sql backend - it would appear to exist in the templated backend as well | 12:23 |
| dolphm | dstanek: the abandoned fix? | 12:24 |
| dstanek | yeah | 12:25 |
| *** dims has quit IRC | 12:31 | |
| *** gokrokve has joined #openstack-keystone | 12:33 | |
| *** dims has joined #openstack-keystone | 12:33 | |
| *** flaper87|afk is now known as flaper87 | 12:35 | |
| *** gokrokve has quit IRC | 12:38 | |
| *** cflmarques has quit IRC | 12:49 | |
| *** ayoung__zzzZZZZ is now known as ayoung | 13:02 | |
| ayoung | openstack-dev has been a wasteland since we abandoned it | 13:03 |
| *** pcargnel has joined #openstack-keystone | 13:06 | |
| *** flaper87 is now known as flaper87|afk | 13:10 | |
| *** browne has joined #openstack-keystone | 13:15 | |
| *** flaper87|afk is now known as flaper87 | 13:19 | |
| openstackgerrit | Ilya Pekelny proposed a change to openstack/keystone: Sync test_migrations https://review.openstack.org/80618 | 13:21 |
| openstackgerrit | Ilya Pekelny proposed a change to openstack/keystone: Comparision of database models and migrations. https://review.openstack.org/80630 | 13:21 |
| *** bknudson has joined #openstack-keystone | 13:31 | |
| *** gokrokve has joined #openstack-keystone | 13:34 | |
| *** gokrokve has quit IRC | 13:38 | |
| *** leseb_ has quit IRC | 13:51 | |
| *** leseb has joined #openstack-keystone | 13:51 | |
| *** nkinder has quit IRC | 13:52 | |
| *** gokrokve has joined #openstack-keystone | 13:52 | |
| *** zhiyan is now known as zhiyan_ | 13:58 | |
| *** marekd has quit IRC | 13:59 | |
| *** marekd has joined #openstack-keystone | 14:01 | |
| *** leseb has quit IRC | 14:04 | |
| *** leseb has joined #openstack-keystone | 14:04 | |
| openstackgerrit | Brant Knudson proposed a change to openstack/keystone: Configurable token hash algorithm https://review.openstack.org/80401 | 14:05 |
| *** lbragstad has quit IRC | 14:17 | |
| *** stevemar has joined #openstack-keystone | 14:17 | |
| *** lbragstad has joined #openstack-keystone | 14:17 | |
| *** wchrisj has joined #openstack-keystone | 14:21 | |
| *** flaper87 is now known as flaper87|afk | 14:25 | |
| *** derek_c has joined #openstack-keystone | 14:31 | |
| openstackgerrit | A change was merged to openstack/python-keystoneclient: Docs link to middlewarearchitecture https://review.openstack.org/70611 | 14:35 |
| openstackgerrit | A change was merged to openstack/keystone: Removal of test .conf files https://review.openstack.org/79525 | 14:35 |
| *** david-lyle has joined #openstack-keystone | 14:36 | |
| *** nkinder has joined #openstack-keystone | 14:39 | |
| openstackgerrit | Marek Denis proposed a change to openstack/keystone: Store groups ids objects list in the OS-FEDERATION object. https://review.openstack.org/81277 | 14:41 |
| *** thedodd has joined #openstack-keystone | 14:41 | |
| openstackgerrit | Ilya Pekelny proposed a change to openstack/keystone: Sync test_migrations https://review.openstack.org/80618 | 14:48 |
| openstackgerrit | Ilya Pekelny proposed a change to openstack/keystone: Comparision of database models and migrations. https://review.openstack.org/80630 | 14:48 |
| dstanek | dolphm: the python-memcached client library appear to be using a thread local for the Client | 14:56 |
| *** raildo has joined #openstack-keystone | 14:58 | |
| openstackgerrit | Dolph Mathews proposed a change to openstack/identity-api: merge OS-FEDERATION objects together https://review.openstack.org/81022 | 14:59 |
| *** chandan_kumar has quit IRC | 15:02 | |
| *** chandan_kumar has joined #openstack-keystone | 15:04 | |
| *** saju_m has quit IRC | 15:23 | |
| *** richm has joined #openstack-keystone | 15:25 | |
| *** henrynash has quit IRC | 15:25 | |
| *** derek_c has quit IRC | 15:30 | |
| *** vhoward has joined #openstack-keystone | 15:31 | |
| *** pcargnel has quit IRC | 15:33 | |
| *** gokrokve_ has joined #openstack-keystone | 15:34 | |
| *** devlaps has joined #openstack-keystone | 15:37 | |
| openstackgerrit | Florent Flament proposed a change to openstack/python-keystoneclient: Allow applications using keystoneclient to close connexions. https://review.openstack.org/81290 | 15:37 |
| *** gokrokve has quit IRC | 15:37 | |
| *** gyee has joined #openstack-keystone | 15:38 | |
| *** saju_m has joined #openstack-keystone | 15:41 | |
| openstackgerrit | Florent Flament proposed a change to openstack/python-keystoneclient: Allow applications using keystoneclient to close connexions. https://review.openstack.org/81290 | 15:41 |
| richm | hello - question about test_backend.py and the use of user_sna - where is user_sna defined? git grep shows several places where user_sna is referenced, but not defined | 15:52 |
| YorikSar | ayoung: Hi | 15:52 |
| ayoung | YorikSar, Hey | 15:52 |
| YorikSar | ayoung: I've posted a fix for that is_revoked method | 15:52 |
| ayoung | YorikSar, what was the flaw you found? | 15:53 |
| YorikSar | ayoung: Please take a look whenever you have some time. | 15:53 |
| ayoung | looking. very excited | 15:53 |
| YorikSar | ayoung: I didn't actually read backlog here... | 15:53 |
| bknudson | richm: http://git.openstack.org/cgit/openstack/keystone/tree/keystone/tests/default_fixtures.py#n77 | 15:53 |
| ayoung | YorikSar, add me to the review, please...makes it much easier to find | 15:53 |
| dolphm | richm: probably in a fixture as user name="sna" | 15:53 |
| YorikSar | ayoung: It was the one I've posted in comment to your original change request. | 15:53 |
| ayoung | ? | 15:53 |
| dolphm | richm: they're dynamically assigned as attributes of the test case during setUp | 15:54 |
| YorikSar | ayoung: Added | 15:54 |
| YorikSar | https://review.openstack.org/81235 - my fix | 15:54 |
| YorikSar | https://review.openstack.org/55908 - where I've commented the day before (or so) | 15:55 |
| *** daneyon has joined #openstack-keystone | 15:55 | |
| richm | ah - ok - I see - tests/core.py:510 - the setattr | 15:55 |
| richm | ok - thanks | 15:55 |
| YorikSar | ayoung: I was very wrong to abandon reviewing your CRs... And I've missed a really simple issue with the code that you posted after my load of comments. | 15:55 |
| YorikSar | ayoung: The problem was with indentation | 15:56 |
| *** rupsky has joined #openstack-keystone | 15:56 | |
| ayoung | YorikSar, can we drop the dor...else semantics? I find it kindof hard top grok, and most of the other reviewer have, too | 15:56 |
| YorikSar | ayoung: It's actually makes code cleaner... | 15:57 |
| ayoung | YorikSar, yeah, I realize that the "return" was happening at the wrong point | 15:57 |
| *** saju_m has quit IRC | 15:57 | |
| YorikSar | ayoung: But I've added some comments, so it should be fine now :) | 15:57 |
| ayoung | you need to clear the bundle before you return, but it waws only looking at the local set of branches | 15:57 |
| ayoung | yeah, its a novel | 15:57 |
| *** rupsky has quit IRC | 15:58 | |
| ayoung | if not bundle: | 15:58 |
| ayoung | 199 return True212 # Note that else: branch will not happen after this. | 15:58 |
| ayoung | 213 break | 15:58 |
| ayoung | YorikSar, I had gotten that far last night | 15:58 |
| ayoung | YorikSar, I am going to add you to a draft client review | 15:59 |
| ayoung | https://review.openstack.org/#/c/81166/ | 16:00 |
| ayoung | I'm going to give your code a run, now | 16:00 |
| *** daneyon has quit IRC | 16:01 | |
| ayoung | ah...I need to port your new code over to that review, too, though YorikSar as it does the same logic.... | 16:01 |
| *** daneyon has joined #openstack-keystone | 16:02 | |
| YorikSar | ayoung: Oh... To which one? | 16:04 |
| ayoung | YorikSar, client code | 16:04 |
| ayoung | https://review.openstack.org/#/c/81166/ YorikSar | 16:04 |
| YorikSar | ayoung: Wow.. There's revoke tree there too?.. | 16:04 |
| ayoung | YorikSar, yeah, it will eventually replace the server code | 16:04 |
| YorikSar | ayoung: Maybe we should isolate it in client and use the code in client from server? | 16:05 |
| ayoung | since the logic needs to be executed the same in client and server, we want it in one and only one place | 16:05 |
| ayoung | so the goal is to get the client review done, then change the server to use the client code | 16:05 |
| ayoung | YorikSar, dead on accurate :) | 16:05 |
| YorikSar | ayoung: Great :) | 16:06 |
| ayoung | YorikSar, OK...so there is still something wrong logic wise...I'll show you how to reproduce in a second, once I'posted the updated client code | 16:07 |
| ayoung | YorikSar, OK, grab the client patch: https://review.openstack.org/#/c/81166/ and run your keystone server | 16:08 |
| openstackgerrit | Florent Flament proposed a change to openstack/python-keystoneclient: Allow applications using keystoneclient to close connexions. https://review.openstack.org/81290 | 16:08 |
| ayoung | you need to make sure you have the revoke_api in the pipeline: | 16:08 |
| ayoung | [pipeline:api_v3] | 16:08 |
| ayoung | pipeline = sizelimit url_normalize build_auth_context token_auth admin_token_auth xml_body json_body ec2_extension_v3 s3_extension simple_cert_extension revoke_extension service_v3 | 16:08 |
| ayoung | and you can do a simple test using CURL to confirm that | 16:09 |
| ayoung | curl http://localhost:35357/v3/OS-REVOKE/events | 16:09 |
| ayoung | once that is done, look in the client patch: examples/scripts/generate_revocation_events.py | 16:09 |
| ayoung | you need to chantge the password on the ADMIN_TOKEN | 16:10 |
| ayoung | so that it matches yours, but once you get it running you should see output like this | 16:10 |
| ayoung | YorikSar, http://paste.openstack.org/show/73771/ | 16:10 |
| openstackgerrit | Florent Flament proposed a change to openstack/python-keystoneclient: Allow applications using keystoneclient to close connexions. https://review.openstack.org/81290 | 16:10 |
| *** wchrisj has quit IRC | 16:11 | |
| *** marcoemorais has joined #openstack-keystone | 16:11 | |
| *** wchrisj_ has joined #openstack-keystone | 16:13 | |
| dstanek | dolphm: i think your pooling impl is fine - i was trying to find out why we actually need to, but i'm not having any luck | 16:13 |
| dstanek | dolphm: so i think maybe we just go with it | 16:13 |
| *** marcoemorais has quit IRC | 16:15 | |
| *** henrynash has joined #openstack-keystone | 16:15 | |
| *** marcoemorais has joined #openstack-keystone | 16:15 | |
| *** marcoemorais has quit IRC | 16:15 | |
| YorikSar | ayoung: Crap... git review -d is broken... | 16:15 |
| *** marcoemorais has joined #openstack-keystone | 16:16 | |
| ayoung | YorikSar, probably nort | 16:18 |
| ayoung | YorikSar, you need to be in the right projct | 16:18 |
| ayoung | pooling impl? dstanek link? | 16:18 |
| YorikSar | ayoung: It tried to fetch from review.oprnstack.org | 16:18 |
| ayoung | YorikSar, probably a misconfig on you machine | 16:19 |
| ayoung | look in .git/config | 16:19 |
| YorikSar | ayoung: Nah... .gitreview is fine and it used to work earlier... | 16:19 |
| ayoung | [remote "gerrit"] | 16:19 |
| ayoung | url = ssh://ayoung@review.openstack.org:29418/openstack/python-keystoneclient.git | 16:19 |
| ayoung | fetch = +refs/heads/*:refs/remotes/gerrit/* | 16:19 |
| dstanek | https://review.openstack.org/#/c/81078/2 <- ayoung | 16:19 |
| ayoung | dstanek, thanks | 16:19 |
| YorikSar | ayoung: Oh, wow. | 16:20 |
| YorikSar | ayoung: Yeah. I never used it in keystoneclient, I guess :) | 16:20 |
| ayoung | YorikSar, suspect that will be changing... | 16:20 |
| YorikSar | ayoung: My magic git-my-clone script was very young the day I've cloned it and it installed bad push-url for gerrit remote | 16:21 |
| ayoung | YorikSar, let me know when you get the client script to run | 16:22 |
| YorikSar | ayoung: I can't find admin_token in examples/scripts/generate_revocation_events.py | 16:23 |
| ayoung | I want to take that logic and make it into a unit test of some sort | 16:23 |
| ayoung | YorikSar, heh | 16:23 |
| ayoung | its not in there | 16:23 |
| YorikSar | ayoung: Will it use the default one ... by default? | 16:23 |
| ayoung | its in the other scripts in that dir | 16:23 |
| ayoung | YorikSar, those are supposed to be example scripts, so I didn't make it very Object oriented | 16:24 |
| YorikSar | ayoung: Oh, you and FreeIPA :) | 16:24 |
| ayoung | instead I subprocessed the setup script | 16:24 |
| ayoung | freeipa4all | 16:24 |
| ayoung | YorikSar, I want to pull the value out of /etc/keystone/keystone.conf eventually, but not certain if I should pull in Oslo config to do that | 16:25 |
| *** derek_c has joined #openstack-keystone | 16:25 | |
| YorikSar | ayoung: It won't help me because I'm running from .tox/py27/bin/keystone-all :) | 16:26 |
| ayoung | YorikSar, there is that, too | 16:26 |
| ayoung | although, strangly, it still picks up keystone-paste.api from /etc on my system | 16:27 |
| YorikSar | ayoung: I remember some time around Essex there was such problem in Nova... | 16:28 |
| ayoung | YorikSar, let me confirm that the test in the middle is valid | 16:29 |
| richm | dolphm: ping - re: your review comments for bug/1282676 - you suggested mutating the object to test rather than have less strict checking - so something like this? http://fpaste.org/86414/ | 16:29 |
| ayoung | https://review.openstack.org/#/c/81166/2/examples/scripts/generate_revocation_events.py | 16:30 |
| ayoung | line 102: test_revoking_role | 16:30 |
| *** flaper87|afk is now known as flaper87 | 16:30 | |
| ayoung | I guess if the user never had that role...the token would still be valid. OK, let me assert that | 16:31 |
| YorikSar | ayoung: Will Keystone generate cert for itself if there is no cert file? | 16:33 |
| ayoung | Nah, run pki_setup | 16:33 |
| ayoung | keystone-manage pki_setu | 16:33 |
| *** derek_c has quit IRC | 16:33 | |
| ayoung | keystone-manage pki_setup | 16:33 |
| *** chandankumar_ has joined #openstack-keystone | 16:34 | |
| *** chandan_kumar has quit IRC | 16:37 | |
| *** pete5 has quit IRC | 16:38 | |
| YorikSar | ayoung: Ok... It claims smth is not setup properly... | 16:38 |
| YorikSar | ayoung: Do I need some config for client? | 16:38 |
| ayoung | keystone or the client? | 16:38 |
| YorikSar | ayoung: Like... Where to store certs? | 16:38 |
| YorikSar | keystoneclient.exceptions.CertificateConfigError: Unable to load certificate. Ensure your system is configured properly. | 16:39 |
| ayoung | certs go in /etc/keystone/ssl on my machine, so something comparable | 16:39 |
| ayoung | might be a permissions issue. I suspect my was origianlly setup by devstack and done as root | 16:39 |
| YorikSar | ayoung: Finally it worked. | 16:41 |
| YorikSar | ayoung: No, I just don't like apps I develop to to anything with my OS :) | 16:41 |
| YorikSar | ayoung: So I've changed paths in the script. | 16:42 |
| ayoung | YorikSar, ++ | 16:42 |
| YorikSar | ayoung: But... Everythin's OK | 16:42 |
| YorikSar | ayoung: I mean, all tests are | 16:42 |
| ayoung | curl works | 16:42 |
| ayoung | ah, the script runs for you, and runs clean? | 16:42 |
| YorikSar | ayoung: Yeah | 16:42 |
| ayoung | interesting...not for me | 16:42 |
| YorikSar | ayoung: It was with py27 env | 16:43 |
| YorikSar | ayoung: Installint py33 one.. | 16:43 |
| ayoung | ooh, maybe I am running py33 there. | 16:43 |
| YorikSar | ayoung: You are :) | 16:43 |
| YorikSar | according to that paste | 16:43 |
| YorikSar | ayoung: Oh, that'll take a while... Didn't update wheelhouse for py33 for some time already... | 16:44 |
| ayoung | let me try running py27 | 16:44 |
| YorikSar | ayoung: Can't stop you :) | 16:44 |
| ayoung | YorikSar, and now it is failing consistantly for me | 16:48 |
| YorikSar | ayoung: Looks like I'm first. py33 is OK here too | 16:48 |
| YorikSar | ayoung: Looks like smth's wrong with your environment... | 16:49 |
| YorikSar | ayoung: Maybe recreate venvs, DB and ssl dirs? | 16:49 |
| ayoung | YorikSar, lets test the daylights out of this code | 16:49 |
| YorikSar | ayoung: test the daylighs out? | 16:50 |
| *** fabiomorais has joined #openstack-keystone | 16:50 | |
| YorikSar | ayoung: Ok, dictionary is my friend :) | 16:51 |
| ayoung | YorikSar, yeah...you might want to look up the common meaning of the word you used which I replaced wiht "Bundle" | 16:52 |
| *** harlowja_away is now known as harlowja | 16:53 | |
| YorikSar | ayoung: It's actually the first translation dictionary gave me. But it sounded familiar from some movies or series, so I checked up with urbandictionary. And decided to play "no know English" :) | 16:54 |
| *** pcargnel has joined #openstack-keystone | 16:54 | |
| YorikSar | ayoung: It's just so strange that a word can switch meaning like that. | 16:54 |
| ayoung | Alas, poor word choice, YorikSar | 16:55 |
| ayoung | YorikSar, I think it was due to the Yiddish word http://en.wiktionary.org/wiki/feygele | 16:56 |
| *** pcargnel has quit IRC | 16:57 | |
| YorikSar | ayoung: Wow... It's not very similar actually. | 16:57 |
| ayoung | YorikSar, it is to the short form in common usage | 16:58 |
| ayoung | http://en.wikipedia.org/wiki/Faggot_%28slang%29 | 16:58 |
| YorikSar | ayoung: Oh, yes. | 16:58 |
| YorikSar | ayoung: Anyway. I'll try not to name a list of validated and verified tokens "gay_tokens" in code :) | 16:59 |
| *** pcargnel_ has joined #openstack-keystone | 17:00 | |
| ayoung | YorikSar, that would be imprecise anyway, as they were only added due to potential | 17:00 |
| ayoung | not confirmation | 17:00 |
| ayoung | :) | 17:00 |
| dolphm | richm: not quite -- don't make it conditional/dynamic -- explicitly mutate the object so there's no magic going on in the tests | 17:01 |
| *** morganfainberg_Z is now known as morganfainberg | 17:01 | |
| YorikSar | ayoung: Ok, I'll switch to rootwrap work for now... Let me know if you find smth interesting with fixed algorithm | 17:02 |
| *** marcoemorais has quit IRC | 17:02 | |
| *** marcoemorais has joined #openstack-keystone | 17:02 | |
| richm | dolphm: so something like - user_ref.pop('description') ? | 17:03 |
| richm | i.e. hardcoding the property which may have a value of None? | 17:03 |
| * morganfainberg forgets about daylight savings... | 17:03 | |
| richm | help | 17:03 |
| dolphm | richm: yes, exactly | 17:03 |
| morganfainberg | stupid caldendar that doesn't do UTC. | 17:04 |
| richm | sorry, wrong window | 17:04 |
| richm | dolphm: and I can assume, for the purposes of testing, that the list of properties will be fixed, so I can have a hard coded list like noneprops = ['description', 'enabled'] - and that the object being tested will only contain these attributes and no others? | 17:05 |
| richm | because the actual list of properties is dynamic and mutable | 17:06 |
| dolphm | richm: if you can't make that assumption, then something is wrong with the tests... | 17:06 |
| richm | ok | 17:06 |
| ayoung | YorikSar, OK, I'm running your code. I wiped the revocation table prior to the run. The tree looks like this: | 17:11 |
| ayoung | wildcards for | 17:11 |
| ayoung | consumer_id, access_token_id, expires_at | 17:12 |
| ayoung | at domain_id, I see two options: wildcard and an explicit domain | 17:12 |
| ayoung | now, I just delete the user, so the revocation event should only have user_id in it | 17:13 |
| ayoung | which means it would match domain_id=* here | 17:13 |
| ayoung | just hit a breakpoint where name = 'domain_id' | 17:13 |
| ayoung | bundle gets populated with two "none" entries, due to alternatives | 17:14 |
| ayoung | and one that looks like the "right path down" | 17:14 |
| *** arborism has joined #openstack-keystone | 17:15 | |
| ayoung | project_id=*, user_id=eb5..., role_id=* | 17:16 |
| ayoung | nope..userid is wrong there... | 17:16 |
| ayoung | YorikSar, I notice that the debugger shows bundle=<filter object> | 17:16 |
| ayoung | is it possible python33 broke something there? | 17:17 |
| *** arborism is now known as amcrn | 17:17 | |
| ayoung | tree does seem to be extracted out of bundle...so it looks correct | 17:18 |
| YorikSar | ayoung: Yes, that filter object is from py33 | 17:18 |
| YorikSar | ayoung: But it shouldn't break anything. Only probably speedup. | 17:19 |
| ayoung | yep | 17:19 |
| ayoung | just wish I could see inside it | 17:19 |
| YorikSar | ayoung: Do list(bundle) | 17:19 |
| YorikSar | ayoung: It shouldn't break it... I guess. | 17:20 |
| YorikSar | ayoung: Oh, no, it might... | 17:20 |
| ayoung | nah, it was OK | 17:21 |
| ayoung | it seemed to get down to the role_id level | 17:21 |
| YorikSar | ayoung: That object is an iterator. So if you run list(bundle) once, second list(bundle) will give you [] | 17:21 |
| *** pcargnel_ has quit IRC | 17:21 | |
| ayoung | lemme try that again | 17:22 |
| openstackgerrit | David Chadwick proposed a change to openstack/identity-api: Adding support for SAML federated authentication (Federation Pt3) https://review.openstack.org/62417 | 17:22 |
| *** devlaps1 has joined #openstack-keystone | 17:23 | |
| ayoung | forest is empty | 17:24 |
| *** devlaps has quit IRC | 17:24 | |
| ayoung | it should be matching on user_id | 17:24 |
| YorikSar | I don't quite follow you... | 17:25 |
| *** leseb has quit IRC | 17:30 | |
| ayoung | YorikSar, I'm debugging. tes first test is failing...and it seems to be OK dowen to role_id | 17:30 |
| ayoung | down | 17:30 |
| ayoung | I've stopped where name=role_id | 17:30 |
| *** leseb has joined #openstack-keystone | 17:30 | |
| ayoung | actually, one up | 17:30 |
| ayoung | name=user_id | 17:30 |
| ayoung | bundle has two entries | 17:30 |
| *** thedodd has quit IRC | 17:31 | |
| ayoung | one is role_id=4bcc... the other is role_id=* | 17:31 |
| ayoung | its the second one that should match | 17:31 |
| YorikSar | ayoung: And what roles are in token? | 17:31 |
| *** andreaf has quit IRC | 17:32 | |
| ayoung | YorikSar, doesn't matter, the user has been deleted | 17:32 |
| ayoung | but... | 17:32 |
| ayoung | does have one | 17:32 |
| ayoung | stars with 5dd | 17:32 |
| ayoung | OK, so stepped until names=role_id | 17:32 |
| ayoung | and forest is empty | 17:33 |
| ayoung | jumped right over... | 17:33 |
| ayoung | I wonder if that is a side effect of debugging | 17:33 |
| ayoung | did you think list(bundle) would 0 it out ? | 17:33 |
| YorikSar | Yep | 17:33 |
| YorikSar | ayoung: So if you need it, you should do smth like bundle = list(bundle) | 17:34 |
| YorikSar | and then check it out | 17:34 |
| *** leseb has quit IRC | 17:34 | |
| ayoung | yeah, doing list(forest) zeros it out. | 17:36 |
| ayoung | can't do that...OK | 17:37 |
| marekd | stevemar: https://review.openstack.org/#/c/81277/1/keystone/tests/test_v3_federation.py the point in serializer.to_xml() was -> without the fix all the tests pass. So I felt I should add something that would actually check if not the xml content can be built... | 17:37 |
| YorikSar | ayoung: btw, how do you run it in py33? | 17:37 |
| ayoung | YorikSar, I'm going to temporarily replace that code with | 17:37 |
| ayoung | YorikSar, I'm using pycharm | 17:37 |
| marekd | stevemar: plus, instead of doing one dedicated test for that I added it in one helper class, so more tests (using _issue_unscoped_token()) would also test this behaviour... | 17:37 |
| *** morganfainberg is now known as morganfainberg_Z | 17:38 | |
| ayoung | bundle = [item for item in bundle if item] | 17:38 |
| ayoung | just so I can see it | 17:38 |
| YorikSar | ayoung: You could do just bundle = list(filter(None, bundle)) :) | 17:39 |
| ayoung | OK, userid is now caa13a445e0a46edb731033d37279e52 and I have two roles entries in the bundle | 17:39 |
| ayoung | YorikSar, nope, we assign that to bundle | 17:39 |
| ayoung | nothing left to filter at the point I needed it | 17:39 |
| ayoung | 'salright...logic should be the same... | 17:40 |
| *** jamielennox is now known as jamielennox|away | 17:40 | |
| YorikSar | I mean, just wrap that filter(...) in text with list(...). You'll get the same effect with less modifications. | 17:40 |
| ayoung | tree.get(wildcard) == None for Roleid | 17:40 |
| YorikSar | But that's not the point. | 17:40 |
| ayoung | ah..but that is the other tree | 17:41 |
| YorikSar | Hm... Do you have some combination of tree and token that doesn't work as expected? | 17:42 |
| YorikSar | Can you dump them to paste so that I can check it out? | 17:42 |
| ayoung | YorikSar, and...it just succeeded | 17:44 |
| ayoung | hmmm | 17:44 |
| ayoung | but the last test failed | 17:45 |
| YorikSar | ayoung: And it failed with filter()?.. | 17:45 |
| ayoung | oooh...don't know if that was the difference | 17:45 |
| YorikSar | Can you try to run tests with list comprehension? | 17:45 |
| ayoung | YorikSar, run the tests a bunc of times and see if any of them fail for you? | 17:45 |
| YorikSar | ayoung: yeah... | 17:46 |
| ayoung | YorikSar, I suspect it has to do with ABC order and the ids | 17:46 |
| YorikSar | ayoung: Nothing fails on my setup. | 17:46 |
| YorikSar | ayoung: Hm... I don't see anything order-related at all in the code... | 17:48 |
| ayoung | YorikSar, not every test fails every time. try running it in a loop | 17:48 |
| openstackgerrit | Marek Denis proposed a change to openstack/keystone: Filter SAML2 assertion parameters with certain prefix. https://review.openstack.org/80946 | 17:49 |
| ayoung | for i in `seq 1 3` ; do python generate_revocation_events.py ; done | 17:49 |
| dstanek | this is a bummer - https://bugs.launchpad.net/python-keystoneclient/+bug/1282089 | 17:50 |
| YorikSar | ayoung: Did just that... | 17:50 |
| YorikSar | ayoung: Nothing fails... | 17:50 |
| ayoung | hmmm | 17:51 |
| stevemar | marekd, cool, didnt rwalize that | 17:51 |
| *** gokrokve_ has quit IRC | 17:52 | |
| *** flaper87 is now known as flaper87|afk | 17:53 | |
| marekd | stevemar: otherwise the 'good way' for new tests would be: rewrite all the test cases with XML :-) | 17:53 |
| YorikSar | ayoung: Started this: while true; do ../../.tox/py33/bin/python generate_revocation_events.py; done | sed 's/FAIL/FAIL\a/' | 17:54 |
| ayoung | YorikSar, OK, I'm going to step through one last time and see if I can figure out where the problem is. I wonder if it has to do with the clock | 17:54 |
| YorikSar | ayoung: Let's see if it bangs | 17:54 |
| ayoung | I'm going to be a sleep 1 in there | 17:54 |
| YorikSar | ayoung: You can try to dump tree and token on every call to is_revoked | 17:55 |
| ayoung | YorikSar, yeah, that is true, too | 17:55 |
| *** fabiog has joined #openstack-keystone | 17:55 | |
| ayoung | might be a good debugging tool | 17:56 |
| ayoung | YorikSar, yep | 17:57 |
| ayoung | sleep one made it work | 17:58 |
| dolphm | richm: thanks! replied to your comment on this with a follow up question https://review.openstack.org/#/c/78521/4/keystone/common/ldap/core.py | 17:58 |
| *** morganfainberg_Z is now known as morganfainberg | 17:58 | |
| morganfainberg | dolphm, hopefully this one will pass now https://review.openstack.org/#/c/58766/ | 17:58 |
| morganfainberg | dolphm, the trust with no roles patch introduced a new string and the trust controller didn't have use of _ before, so it failed gate. | 17:58 |
| YorikSar | ayoung: Hm... I don't see any possibility for some race or smth like that there... Do you? | 17:58 |
| ayoung | YorikSar, I suspec that the delete was happening too soon to the token issuing, and some rounding error is getting in there | 17:59 |
| ayoung | YorikSar, maybe one with microsecond one without? | 17:59 |
| *** gyee has quit IRC | 17:59 | |
| YorikSar | ayoung: Oh... Maybe. | 17:59 |
| *** gyee has joined #openstack-keystone | 17:59 | |
| dolphm | morganfainberg: hrm, so new translated strings are now bad? | 18:00 |
| *** marcoemorais has quit IRC | 18:00 | |
| ayoung | YorikSar, keystone-meeting time | 18:00 |
| ayoung | care to join #openstack-meeting? | 18:01 |
| *** harlowja is now known as harlowja_away | 18:01 | |
| YorikSar | ayoung: Sure :) | 18:01 |
| *** marcoemorais has joined #openstack-keystone | 18:01 | |
| *** harlowja_away is now known as harlowja | 18:02 | |
| *** shakamunyi has joined #openstack-keystone | 18:04 | |
| *** marcoemorais has quit IRC | 18:08 | |
| *** marcoemorais has joined #openstack-keystone | 18:09 | |
| *** jamielennox|away is now known as jamielennox | 18:09 | |
| *** shakamunyi has quit IRC | 18:18 | |
| *** shakamunyi has joined #openstack-keystone | 18:27 | |
| *** henrynash_ has joined #openstack-keystone | 18:45 | |
| *** henrynash has quit IRC | 18:48 | |
| *** henrynash_ is now known as henrynash | 18:48 | |
| *** shakamunyi has quit IRC | 18:49 | |
| openstackgerrit | ayoung proposed a change to openstack/python-keystoneclient: Compressed Signature and Validation https://review.openstack.org/71181 | 18:50 |
| *** fabiomorais has quit IRC | 18:52 | |
| *** gokrokve has joined #openstack-keystone | 18:56 | |
| *** fabiomorais has joined #openstack-keystone | 18:57 | |
| *** flaper87|afk is now known as flaper87 | 18:59 | |
| *** fabiog has quit IRC | 19:01 | |
| *** marekd is now known as marekd|bbl | 19:01 | |
| morganfainberg | ok ayoung , stevemar , marekd , dolphm, i'm ok with making the only way we revoke IDP tokens being by way of revocation events, if we are comfortable claiming federation is as expirimental as revocation events | 19:01 |
| dolphm | morganfainberg: i think they're both in the same boat as of icehouse | 19:02 |
| ayoung | morganfainberg, it is as experimental, if not more so | 19:02 |
| dolphm | morganfainberg: neither are proven | 19:02 |
| morganfainberg | dolphm, cool. | 19:02 |
| morganfainberg | dolphm, that was my only concern with it having a required dep on revocation events to close the security hole | 19:02 |
| stevemar | makes sense to me | 19:02 |
| morganfainberg | dolphm, and that answer works 100% for me | 19:02 |
| morganfainberg | stevemar, ayoung, can we include a line in the documenation about this requirement? for Icehouse release? | 19:03 |
| marekd|bbl | morganfainberg: federation is not yet fully usable so it can be claimed as 'experimental' | 19:03 |
| *** marekd|bbl is now known as marekd | 19:03 | |
| morganfainberg | marekd|bbl, great. just wanted us to be all on that same page | 19:03 |
| jamielennox | also, i realize everyone's busy - i currently have 17 open (non-WIP) reviews for keystoneclient without votes on them - if the problem is understanding them let me know as i'm around now | 19:03 |
| morganfainberg | all my concerns with this approach are resolved. | 19:03 |
| morganfainberg | jamielennox, i've started going through them. it's slow but i'm working on it | 19:04 |
| jamielennox | morganfainberg: appreciated, thanks | 19:04 |
| morganfainberg | marekd, ^ on the documentation for how revoking tokens w/ IDPs. | 19:04 |
| gyee | jamielennox, question on the v3 password auth plugin | 19:04 |
| jamielennox | gyee: shoot | 19:04 |
| morganfainberg | lunch time, will fix keystone error then back to keystoneclient reviews. | 19:04 |
| morganfainberg | bbib | 19:04 |
| gyee | for passwordCredentials, we don't see to support userId | 19:04 |
| marekd | morganfainberg: hm? | 19:04 |
| gyee | just username and password | 19:04 |
| gyee | the protocol does support userId and password it seems | 19:05 |
| ayoung | YorikSar, so..what kind of propblem is the corrected algorithm fixing, and how do we right a test for it? | 19:05 |
| jamielennox | gyee: for v3? | 19:05 |
| marekd | dolphm: https://review.openstack.org/#/c/81277/1/keystone/tests/test_v3_federation.py does 'lazy' here mean 'bad, should not be done this way'? :-) | 19:06 |
| YorikSar | ayoung: I'm formulating it in the bug... | 19:06 |
| ayoung | nice | 19:06 |
| gyee | jamielennox, for v2 | 19:06 |
| gyee | jamielennox, https://github.com/openstack/keystone/blob/master/keystone/token/controllers.py#L243 | 19:06 |
| YorikSar | ayoung: And then I can synthesise test for you | 19:06 |
| *** pcargnel has joined #openstack-keystone | 19:06 | |
| dolphm | marekd: lazy means you could write an explicit test in the other module | 19:08 |
| dolphm | marekd: and it would do fairly robust two way json -> xml -> json and xml -> json -> xml transformation testing | 19:09 |
| YorikSar | ayoung: smth like this: https://bugs.launchpad.net/keystone/+bug/1294292 | 19:09 |
| jamielennox | gyee: yea, you appear to be right - do you know if that was originally supported by a v2 client and i removed it or was never supported? | 19:09 |
| dolphm | YorikSar: can you describe the real world impact on that? | 19:09 |
| YorikSar | ayoung: I'll go now and add a line to commit message and some test | 19:10 |
| dolphm | YorikSar: bugs that contain nothing but implementation details aren't very useful to read later on | 19:10 |
| YorikSar | dolphm: This bug is really very implementation-related | 19:10 |
| marekd | dolphm: allrighty, will add it. | 19:10 |
| YorikSar | dolphm: I can add an example of failing check | 19:10 |
| gyee | jamielennox, seem like we never supported it on the client side | 19:11 |
| jamielennox | gyee: yep, that's what i'm seeing: https://github.com/openstack/python-keystoneclient/blob/0.6.0/keystoneclient/v2_0/client.py#L188 | 19:11 |
| gyee | k, I'll submit a patch to fix that | 19:12 |
| gyee | should be a trivial one | 19:12 |
| jamielennox | gyee: ++ | 19:13 |
| ayoung | YorikSar, add a test for that in test_v3_auth, | 19:13 |
| YorikSar | ayoung: I though about adding a test in test_revoke... | 19:13 |
| YorikSar | ayoung: Isn't it the right place? | 19:14 |
| ayoung | YorikSar, if you can get it in test_revoke, do so | 19:14 |
| ayoung | I was working with existing tokens, and it is pretty clear how to translate that to the auth API | 19:14 |
| ayoung | but, yeah, it should be do-able in test_revoke | 19:15 |
| thiagop | Guys, quick question: the python-keystoneclient can already authenticate a user in a domain other then 'default' or I can do that only via REST API? | 19:17 |
| *** marekd is now known as marekd|away | 19:17 | |
| gyee | thiagop, for V3 yes | 19:17 |
| dolphm | morganfainberg: make it go https://review.openstack.org/#/c/58766/ | 19:18 |
| jamielennox | thiagop: by keystoneclient do you mean the CLI or the library? | 19:18 |
| *** saju_m has joined #openstack-keystone | 19:18 | |
| thiagop | jamielennox: the library (used by Horizon, isn't it?) | 19:19 |
| jamielennox | thiagop: should be yea, yes you can give a domain_id or a domain_name to the client to scope to a different domain | 19:19 |
| jamielennox | thiagop: ah sorry, user_domain_id or user_domain_name to specify the domain the user belongs to | 19:20 |
| gyee | thiagop, may want to check with david-lyle, I think horizon is working on v3 support | 19:20 |
| david-lyle | thiagop, Horizon has support for authenticating to domains other than the default domain, problem is once you do, you can't do anything | 19:22 |
| *** gyee has quit IRC | 19:26 | |
| *** daneyon has quit IRC | 19:32 | |
| dstanek | jamielennox, dolphm: what do you guys think about this issue? https://bugs.launchpad.net/python-keystoneclient/+bug/1282089/comments/4 | 19:34 |
| *** Nathan255 has joined #openstack-keystone | 19:35 | |
| bknudson | dstanek: would having a close() function on the client fix it? | 19:37 |
| jamielennox | dstanek: what's the circular ref you refer to? | 19:38 |
| dstanek | bknudson: only if the close is actually killing the references | 19:39 |
| jamielennox | i think the problem is the assumption of making many httpclients, i am hoping to push people to make 1 session object and then make multiple clients based on that session | 19:39 |
| bknudson | I assume the client creates a bunch of managers that refer back to it? | 19:39 |
| jamielennox | this is a problem in horizons case where we are juggling a lot of different auths though | 19:39 |
| dstanek | jamielennox: http://git.openstack.org/cgit/openstack/python-keystoneclient/tree/keystoneclient/v3/client.py#n95 - the client hold only the things it creates and passes in a reference to self | 19:40 |
| dolphm | Nathan255: o/ | 19:40 |
| bknudson | dstanek: close() could delete all the managers? | 19:41 |
| jamielennox | dstanek: oh, yuk - never thought of that, that manager code has been around forever | 19:42 |
| dstanek | bknudson: yes, it definitely could and probably should | 19:42 |
| jamielennox | i think this must be a problem for all the clients? | 19:43 |
| jamielennox | or maybe having httpclient and client seperate objects breaks that | 19:43 |
| dstanek | bknudson: but that still leaves a developer with having to rely on explicitly closing - a __del__ impl may also be nice to have here | 19:43 |
| jamielennox | dstanek: right, a close() then makes the client object unusable | 19:44 |
| jamielennox | in which case you should just del client | 19:44 |
| dstanek | jamielennox: from my 30 seconds of looking it seemed like the oslo client stuff encourages this sort of design | 19:44 |
| bknudson | dstanek: then they'd have to del it? | 19:44 |
| bknudson | dstanek: why do the managers need the self ref? | 19:45 |
| bknudson | could they instead be passed a function? | 19:45 |
| dstanek | bknudson: no, i would do that in addition to close | 19:45 |
| jamielennox | bknudson: i'm not advocating it - but if they have to .close() it it would be almost the same | 19:45 |
| dstanek | bknudson: no idea, i've just been walking through bugs the last few days | 19:45 |
| jamielennox | can we just weakref the passing of client to manager? | 19:46 |
| *** david-lyle has quit IRC | 19:46 | |
| dstanek | the __del__ should be called automatically when the object goes out of scope; no need to explicitly call it | 19:47 |
| openstackgerrit | Yuriy Taraday proposed a change to openstack/keystone: Fix is_revoked algorithm and add some comments to it https://review.openstack.org/81235 | 19:47 |
| dstanek | the challenge is that sometimes the things you want to reference in a __del__ have already been removed, but in our case i don't think that can be true because of the circular refs | 19:47 |
| jamielennox | dstanek: but would the weakrefs work? i think that would be relatively simple to do? | 19:48 |
| YorikSar | ayoung, dolphm: Added test to commit, link to bug to commit message and an example to bug description. | 19:49 |
| ayoung | YorikSar, thanks | 19:49 |
| bknudson | weakrefs seem like not the thing to use in this situation | 19:49 |
| YorikSar | The example is a bit synthetic though... | 19:49 |
| dstanek | jamielennox: i tried hacking kc.openstack.client.base (or something like that) and i didn't work | 19:50 |
| dstanek | jamielennox: i didn't follow all of the references around; i just wanted enough info to know if the proposed fix is a good one | 19:50 |
| jamielennox | bknudson: oh? this would be exactly my definition of where weakrefs should be used | 19:51 |
| jamielennox | dstanek: there are two different issues i think, session closing and client cleanup | 19:51 |
| jamielennox | obviously they relate because if the client cleaned up correctly then you wouldn't need to close the session | 19:51 |
| dstanek | exactly | 19:52 |
| jamielennox | but the close() i see very much relating to the HTTP stuff rather than the cleaning up of managers etc | 19:52 |
| jamielennox | dstanek: give me today to have a look at fixing client cleanup and we can see if it helps with the HTTP closing? | 19:52 |
| bknudson | maybe weakrefs are used differently in python... | 19:53 |
| dstanek | jamielennox: sounds good to me :-) | 19:53 |
| bknudson | every time you use a weakref you need to check if it's still valid? | 19:53 |
| dstanek | bknudson: in this case i don't think you need to because if it's invalid then the managers can't be called anyway | 19:53 |
| jamielennox | bknudson: yes | 19:54 |
| jamielennox | bknudson: but as the standard way to call a manager is client.users.list() i don't think that's a problem | 19:54 |
| jamielennox | we would just have an error if someone grabs users = client.users and lets client fall out of scope | 19:54 |
| bknudson | jamielennox: I agree that would be a pretty wacky way to use the api | 19:55 |
| bknudson | I'm still wondering what the manager uses that's in the Client? | 19:56 |
| bknudson | could it be passed in instead | 19:56 |
| jamielennox | bknudson: i'm not sure if it was done initially for testing or whatever but all the actual request logic is done via the client | 19:57 |
| jamielennox | bknudson: so the manager develops the request data and asks the client to deliver it | 19:58 |
| dstanek | bknudson: this is where i tracked to to http://git.openstack.org/cgit/openstack/python-keystoneclient/tree/keystoneclient/openstack/common/apiclient/base.py#n468 | 19:58 |
| dstanek | there are probably other places those references and store for later use | 19:58 |
| jamielennox | dstanek: erghh... are we using apiclient for this? | 19:58 |
| bknudson | would have to be fixed in oslo | 19:59 |
| dstanek | jamielennox: yes - http://git.openstack.org/cgit/openstack/python-keystoneclient/tree/keystoneclient/base.py#n388 | 19:59 |
| jamielennox | dstanek: oh, that's resource | 20:00 |
| jamielennox | which has a dep on manager - which has a dep on client | 20:00 |
| jamielennox | that's a problem | 20:00 |
| dstanek | jamielennox: lol -> self.manager.delete(self) | 20:01 |
| dstanek | i do not want to kill me...you do it! | 20:01 |
| jamielennox | because whilst people might not use the managers directly it means a eg User resource would hold a reference to the client | 20:01 |
| jamielennox | so that you couldn't do user.update(x=x) if the client fell out of scope | 20:01 |
| jamielennox | dstanek: welcome to the client | 20:02 |
| ayoung | YorikSar, i suspect it just as efficient, and clearer, to drop tjhe filter and to not append None valuies to the bundle | 20:02 |
| *** marcoemorais has quit IRC | 20:02 | |
| *** marcoemorais has joined #openstack-keystone | 20:02 | |
| jamielennox | dstanek: i'm not sure how we fix it either, i've been working on auth and leaving that stuff alone - but i'm not a fan of that oslo apiclient stuff reenforcing all this | 20:03 |
| *** marcoemorais has quit IRC | 20:03 | |
| *** marcoemorais has joined #openstack-keystone | 20:03 | |
| dstanek | j | 20:03 |
| dstanek | blah | 20:04 |
| ayoung | jamielennox, and *that* is why I submitted the patch to rollback the API client in oslo and pissedo the Oslo deities | 20:04 |
| ayoung | erm...annoyed | 20:04 |
| jamielennox | ayoung: i remember, i +1ed it | 20:04 |
| dstanek | jamielennox: usually this means that the abstraction is wrong or reversed - i need to think about it a little more before i know for sure | 20:04 |
| ayoung | dstanek, I have a chunk of code that does bundle.append(tree.get('role_id=%s' % role_id)) and then later | 20:05 |
| ayoung | bundle = filter(None, bundle) | 20:05 |
| ayoung | is there a neater way to say "append only if not None" | 20:05 |
| jamielennox | dstanek: i would love an outside opinion, i've been messing with this stuff for so long that i've gotten used to it's stupidity | 20:06 |
| morganfainberg | ayoung, if not none, append? | 20:06 |
| ayoung | morganfainberg, then I need a temp variable | 20:06 |
| dstanek | morganfainberg, ayoung: yeah, but you'll avoid the extra loop | 20:06 |
| ayoung | a = get(x) ; if a is not None, bundle.append(a) | 20:06 |
| morganfainberg | ayoung, that seems like a low cost for readability | 20:06 |
| morganfainberg | ayoung, i'd go for readability in that code | 20:06 |
| jamielennox | dstanek: ideally we would wrap the session object up with some client specific context and pass that to all the managers | 20:06 |
| jamielennox | that wouldn't be too hard i think | 20:07 |
| ayoung | morganfainberg, Oh, I agree, just wonder if there is a some sugar for the syntax | 20:07 |
| morganfainberg | ayoung, use a while loop! :P | 20:07 |
| morganfainberg | ayoung, nah, i think it's cleanest to bind to a local var and explicitly if-check | 20:07 |
| dstanek | ayoung: if you want moar code you can turn it into a generator function | 20:07 |
| jamielennox | ayoung: no there isn't that i've seen unless you start doing yields | 20:08 |
| morganfainberg | dstanek, yeild! | 20:08 |
| dstanek | morganfainberg: standing down | 20:08 |
| morganfainberg | dstanek, LOL :P | 20:08 |
| jamielennox | lol | 20:08 |
| morganfainberg | dstanek, actually a generator isn't a bad idea. though i think it might be overkill | 20:08 |
| jamielennox | morganfainberg: i've always felt they are a good approach you just have to wrap them up into such a little package that it just becomes easier to do it inline | 20:09 |
| dolphm | YorikSar: thanks! | 20:09 |
| jamielennox | .... or recursively | 20:09 |
| morganfainberg | jamielennox, sure. | 20:09 |
| openstackgerrit | Florent Flament proposed a change to openstack/python-keystoneclient: Allow applications using keystoneclient to close connexions. https://review.openstack.org/81290 | 20:09 |
| YorikSar | ayoung: Nope... It'll add a lot of if's in the code. It'll become messy with no benefit. | 20:10 |
| ayoung | YorikSar, disagree | 20:10 |
| ayoung | YorikSar, I'll post | 20:10 |
| jamielennox | ayoung: i also think that if you are doing a lot of var = dict.get(x) if var: then you should be doing try: dict[x] except KeyError | 20:11 |
| jamielennox | but i realize that's a preference | 20:12 |
| ayoung | jamielennox, yep... | 20:12 |
| ayoung | that might be preferable here | 20:12 |
| dstanek | jamielennox: ^ that commit is tied to the bug we were just discussing | 20:13 |
| ayoung | YorikSar, it would end up as | 20:13 |
| ayoung | try: | 20:13 |
| ayoung | bundle.append(tree[wildcard]) | 20:13 |
| ayoung | except KeyError: | 20:13 |
| ayoung | pass | 20:13 |
| ayoung | and then drop the filter | 20:13 |
| ayoung | more lines of code...but clearer | 20:14 |
| ayoung | or | 20:14 |
| YorikSar | ayoung: It's exception-driven control flow. It's bad. And probably less effecient than filter | 20:14 |
| morganfainberg | oh if you keep the filter, make sure you use six.moves ayoung | 20:14 |
| YorikSar | I can't see how it's clearer. | 20:14 |
| ayoung | YorikSar, its python, we gave up on efficiency when we picked the language | 20:15 |
| YorikSar | morganfainberg: No, in this case py3k filter(..) is even better for us. | 20:15 |
| morganfainberg | then we need a comment | 20:15 |
| YorikSar | ayoung: But you suggest add some mess to the code, make it longer and harder to read. For what benefit? | 20:15 |
| morganfainberg | just saying it's intended to be filter() vs. six.moves | 20:15 |
| dolphm | morganfainberg: what happened to filter in py3? | 20:16 |
| ayoung | YorikSar, nah, I'll pull out into a hlepr function | 20:16 |
| morganfainberg | dolphm, iterator changes | 20:16 |
| YorikSar | dolphm: It returns magic iterator wrapper | 20:16 |
| *** leseb has joined #openstack-keystone | 20:16 | |
| ayoung | http://paste.openstack.org/show/73786/ | 20:16 |
| YorikSar | ayoung: Again... For what benefit?.. | 20:16 |
| bknudson | https://pythonhosted.org/six/#module-six.moves -- filter itertools.ifilter() filter() | 20:16 |
| ayoung | YorikSar, don't get too attached to the beuty of your code. Kill your darlings! | 20:16 |
| morganfainberg | YorikSar, i disagree, filter is almost never easier to read especially with the density that is this code | 20:17 |
| YorikSar | ayoung: Function calls are expensive, btw... | 20:17 |
| morganfainberg | or there needs to be some comments explaining the expected behavior | 20:17 |
| ayoung | YorikSar, heh | 20:17 |
| ayoung | YorikSar, premature optimization is the root of all evil | 20:17 |
| YorikSar | morganfainberg: https://review.openstack.org/#/c/81235/2/keystone/contrib/revoke/model.py | 20:17 |
| YorikSar | morganfainberg: Do you need more comments? :) | 20:17 |
| *** david-lyle has joined #openstack-keystone | 20:18 | |
| morganfainberg | YorikSar, that is so much better :) | 20:18 |
| morganfainberg | YorikSar, Thank you :) | 20:18 |
| YorikSar | ayoung: Well... It's you optimizing smth here, not me ;) | 20:18 |
| ayoung | morganfainberg, agreed | 20:19 |
| ayoung | YorikSar, optimizing for readability. | 20:19 |
| morganfainberg | YorikSar, can i add one other complaint, what is the benefit of the for/else syntax here? besides convience, if you remove the else and backdent one it is easier to read and _appears_ to do the same thing, since you're using an explicit return (break out of function) rather than a break of the loop and continuation | 20:19 |
| morganfainberg | s/complaint/question | 20:19 |
| dolphm | conversations concerning optimization aren't usually productive without benchmarks... so you actually have something to discuss | 20:19 |
| dolphm | otherwise, err on the side of readability :) | 20:20 |
| YorikSar | dolphm: You can't benchmark readability :) | 20:20 |
| morganfainberg | YorikSar, i have a personal adversion to for/else because i've had to explain it a bunch. | 20:20 |
| morganfainberg | and it tends to confuse people. | 20:20 |
| ayoung | for else is probably not needed here, either. | 20:20 |
| dolphm | morganfainberg: definitely use it sparingly, and comment it when you do! | 20:20 |
| morganfainberg | dolphm, ++ | 20:20 |
| YorikSar | morganfainberg: If comment explains it and it prevents from some mistakes... Why not use it? | 20:20 |
| ayoung | since the for loop on line 220 effective gates that hole section | 20:21 |
| morganfainberg | YorikSar, because even python verterans have issues with for/else | 20:21 |
| morganfainberg | YorikSar, it should be the exception to the rule "this really makes sense" vs. "well it's pretty so why not" | 20:21 |
| YorikSar | morganfainberg: Really? | 20:21 |
| morganfainberg | yes, for/else is an odd construct that confuses people easily | 20:21 |
| *** marekd|away is now known as marekd | 20:22 | |
| ayoung | YorikSar, I hacve to admit, I learned about it on this commit | 20:22 |
| morganfainberg | and if the code doesn't really benefit from it, using is adds extra readability issues. | 20:22 |
| YorikSar | For me it made a lot of sense... We have a special final iteration of the loop after iterator is finished. | 20:22 |
| morganfainberg | it's not often used, and not "intuitive" | 20:22 |
| dstanek | morganfainberg: i tend to use it when i would otherwise need a temporary variable to track what happened in a loop | 20:23 |
| morganfainberg | YorikSar, sure, but do you disagree that it's more readable backdented, it's not using a infrequent/obscurely used part of the language? | 20:23 |
| YorikSar | Scroll up to line 147. Is it intuitive there? | 20:23 |
| morganfainberg | dstanek, and i think that is a fair usecase | 20:23 |
| jamielennox | without looking at the code in question - an special final iteration of a loop is just something you do after the loop has finished | 20:23 |
| dstanek | morganfainberg: if the for and the else don't fit in the 30 lines or so in my editor i won't use it | 20:23 |
| dolphm | what *other* languages support for/while-else? | 20:24 |
| morganfainberg | YorikSar, again, is it really needed? | 20:24 |
| morganfainberg | dstanek, also python is sloppy in scope | 20:24 |
| morganfainberg | dstanek, :P | 20:24 |
| dolphm | morganfainberg: lazy* | 20:24 |
| morganfainberg | dolphm, ++ | 20:24 |
| morganfainberg | sorry lazy is better term | 20:24 |
| jamielennox | also i really like the for/else pattern if you are actually doing breaks in there | 20:24 |
| dstanek | dolphm: lazy! like me! | 20:24 |
| dolphm | morganfainberg: lazy is efficient | 20:24 |
| dolphm | morganfainberg: efficient is performant! | 20:24 |
| morganfainberg | if there was a break usecase | 20:25 |
| morganfainberg | i'd be for for/else | 20:25 |
| morganfainberg | but both of these cases aren't using break semantics | 20:25 |
| dolphm | dstanek: ++ the best engineers are the laziest ;) | 20:25 |
| morganfainberg | one is a return (bail) and the other is just "do something at the end" | 20:25 |
| dstanek | morganfainberg: what code are you guys talking about? | 20:25 |
| morganfainberg | dstanek, https://review.openstack.org/#/c/81235/2/keystone/contrib/revoke/model.py | 20:25 |
| morganfainberg | line 147 and 219 | 20:26 |
| morganfainberg | though i guess we're using break now in 219 | 20:26 |
| morganfainberg | so it does make more sense. | 20:26 |
| morganfainberg | in that review | 20:26 |
| morganfainberg | vs. the current code | 20:26 |
| YorikSar | jamielennox: Unless you want to exit from the loop with break to let other people add some code after your loop. | 20:26 |
| ayoung | morganfainberg, we don't need to | 20:27 |
| *** flaper87 is now known as flaper87|afk | 20:27 | |
| jamielennox | YorikSar: looking at the code though the only break is essentially a return False | 20:27 |
| jamielennox | if you made that a return False then the final else block would just be run after | 20:27 |
| YorikSar | I made it a break because I had to add "return False" in the end either way... | 20:28 |
| ayoung | YorikSar, | 20:28 |
| ayoung | forest = bundle | 20:28 |
| ayoung | if not forest: | 20:28 |
| ayoung | break | 20:28 |
| ayoung | then | 20:28 |
| jamielennox | yea, but i think it clearer to say that in this case we are just exiting out early | 20:28 |
| ayoung | for leaf in forest: | 20:28 |
| ayoung | will just pass over | 20:28 |
| *** derek_c has joined #openstack-keystone | 20:29 | |
| YorikSar | ayoung: Yes, I though about it. But desided do leave it as is... | 20:29 |
| ayoung | YorikSar, I have top admit, part of the reason I am playing with it is to understand the code better | 20:29 |
| *** flaper87|afk is now known as flaper87 | 20:29 | |
| ayoung | gonna run the tests and submit a new version here in second | 20:30 |
| *** gyee has joined #openstack-keystone | 20:30 | |
| YorikSar | Ok, I guess there's no point to argue with everyone but myself. | 20:30 |
| morganfainberg | YorikSar, my biggest worry with for/else (especially in this context) is that readability is very important especially in the security context. I'd rather anyone looking at this get it than needing to explain why it works like this. it's really my major complaint. | 20:30 |
| morganfainberg | YorikSar, not that it needs to be changed, nor that i would -1 for it or -2 | 20:31 |
| morganfainberg | YorikSar, just keep in mind readability is very important on a project of this size :) | 20:31 |
| YorikSar | for-else was made for smth like search and do smth with what we found. | 20:31 |
| YorikSar | I'll change it though. | 20:31 |
| ayoung | YorikSar, ther real value is in this discussion, in that all of the Pythonistas are looking at the code and finally understanding it | 20:32 |
| YorikSar | I'm ready to give up for-else but I'll never give up filter! :) | 20:32 |
| morganfainberg | YorikSar, haha gotta pick your battles right? | 20:32 |
| YorikSar | Yeah... Even professionals can't understand my Python. What can we say about my Russian with my girlfriend?.. :) | 20:34 |
| ayoung | YorikSar, morganfainberg what do you think of renaming forest to....potential_matches? | 20:34 |
| ayoung | partial_matches/ | 20:34 |
| YorikSar | ayoung: I use forest here because... Well, it's a forest in the graph theory. | 20:34 |
| morganfainberg | ayoung, YorikSar, with the comment in YorikSar's review i don't think it matters | 20:35 |
| morganfainberg | ayoung, sans comments, forest is less explicit | 20:35 |
| ayoung | I can't see the forest for the trees | 20:35 |
| morganfainberg | ayoung, i see what you did there. | 20:35 |
| * ayoung looking like the cheshire cat | 20:35 | |
| bknudson | does python have a graph theory library? | 20:36 |
| morganfainberg | bknudson, good question | 20:36 |
| *** marcoemorais has quit IRC | 20:36 | |
| *** daneyon has joined #openstack-keystone | 20:36 | |
| *** marcoemorais has joined #openstack-keystone | 20:36 | |
| *** flaper87 is now known as flaper87|afk | 20:39 | |
| ayoung | YorikSar, I don't really want to use the tree/forest metaphor here. There is only one tree, and we are searching it from top to bottom. | 20:42 |
| YorikSar | ayoung: How about bundle and new_bundle? :) | 20:42 |
| ayoung | YorikSar, I'll psot what I have in a moment. I'm just modifying some comments | 20:43 |
| ayoung | I'm also not sure if it is necessary to avoid the early return | 20:43 |
| ayoung | I think it is actually prefereable | 20:43 |
| YorikSar | ayoung: Oh... You're modifying this commit too? | 20:43 |
| ayoung | YorikSar, yeah. | 20:43 |
| YorikSar | ayoung: I've already changed it a bit. Should I not push it then? | 20:43 |
| ayoung | Nah, let me first. | 20:44 |
| dolphm | OS-FEDERATION api change/cleanup required for an RC bug :-/ https://review.openstack.org/#/c/81022/ | 20:45 |
| openstackgerrit | ayoung proposed a change to openstack/keystone: is_revoked check all viable subtrees https://review.openstack.org/81235 | 20:48 |
| ayoung | YorikSar, ^^ | 20:48 |
| *** ayoung is now known as ayoung_Dad_mode | 20:48 | |
| YorikSar | ayoung: I really don't see how self.append_if_not_none increases readability... | 20:50 |
| morganfainberg | YorikSar, vs doing the filter? | 20:51 |
| YorikSar | Yeah. | 20:51 |
| YorikSar | https://review.openstack.org/#/c/81235/2..3/keystone/contrib/revoke/model.py | 20:51 |
| morganfainberg | YorikSar, yeah looking at it now. | 20:51 |
| YorikSar | Take a look at new lines 202-203 | 20:51 |
| YorikSar | Or, better, 208-210 | 20:51 |
| morganfainberg | i don't feel strongly either way | 20:52 |
| morganfainberg | i'm ok with going w/ filter vs a method call | 20:52 |
| morganfainberg | i'll let you and ayoung_Dad_mode figure that out. | 20:52 |
| morganfainberg | but i wouldn't block either implementation, in fact, i thing both are perfectly valid. | 20:52 |
| morganfainberg | filter does add the benefit of brevity | 20:53 |
| YorikSar | And I dont like having it another instance method. Of course premature optimization is evil but just moving it to module level saves lots of time (and for me - readability). | 20:53 |
| morganfainberg | YorikSar, it's a staticmethod, it should be a function if anything | 20:54 |
| YorikSar | Ok, it's coming to 1am and I'm getting grumpy. I guess, it doesn't matter much since either way works and it's not performance-critical part. | 20:54 |
| morganfainberg | YorikSar, eh, i'll poke ayoung to move it back to filter | 20:55 |
| morganfainberg | YorikSar, this change isn't a real benefit. | 20:55 |
| morganfainberg | YorikSar, thanks for conversing on everything here. | 20:55 |
| YorikSar | I'd be against @staticmethod too because every call is another attribute search. | 20:55 |
| morganfainberg | no i wouldn't say make it static | 20:56 |
| morganfainberg | i was saying it is effectively static | 20:56 |
| morganfainberg | it should be a function if anything vs. a method | 20:56 |
| YorikSar | morganfainberg: Oh, thanks to you I actually in process of changing my mind about use of for-else in such cases :) | 20:56 |
| morganfainberg | YorikSar, :) | 20:57 |
| YorikSar | Right... | 20:57 |
| morganfainberg | YorikSar, but i think filter really is better than the instance method | 20:57 |
| morganfainberg | looking at it more | 20:57 |
| YorikSar | Ok, time to sleep now. See you around. | 20:57 |
| morganfainberg | cheers | 20:57 |
| *** dims has quit IRC | 21:12 | |
| *** pcargnel has quit IRC | 21:18 | |
| *** ayoung_ has joined #openstack-keystone | 21:21 | |
| *** ayoung_ is now known as ayoung | 21:22 | |
| *** ayoung_Dad_mode has quit IRC | 21:24 | |
| *** dims has joined #openstack-keystone | 21:26 | |
| *** derek_c has quit IRC | 21:31 | |
| openstackgerrit | Diane Fleming proposed a change to openstack/identity-api: Clean up naming to match new conventions https://review.openstack.org/81076 | 21:34 |
| morganfainberg | dolphm, almost have a working patch for the 500 error cleanup | 21:34 |
| marekd | morganfainberg: this apache/pki tokens thingy? | 21:35 |
| dolphm | morganfainberg: \o/ | 21:35 |
| morganfainberg | marekd, https://bugs.launchpad.net/keystone/+bug/1291047 | 21:36 |
| dolphm | marekd: no, leaking internal details on 500's | 21:36 |
| morganfainberg | dolphm, i made unexpected error subclass of securityerror and i also make it explicitly blank 'exception' from kwargs if not conf.debug OR exception not in kwargs | 21:36 |
| morganfainberg | so no replacement error(s). | 21:36 |
| morganfainberg | 1 test needed fixing. | 21:36 |
| *** daneyon has quit IRC | 21:36 | |
| *** leseb has quit IRC | 21:38 | |
| marekd | dolphm: BTW, this (de)serializer json->xml->json seems to be already implemented, so I am just gonna reuse this and make dedicated test: https://github.com/openstack/keystone/blob/master/keystone/tests/test_serializer.py#L25 | 21:38 |
| morganfainberg | I'm almost of the opinion UnexpectedExceptions (globally) should be made generic 500s | 21:38 |
| morganfainberg | dolphm, should i "fix" the other UnexpectedExceptions to be very generic w/o debug=true? | 21:39 |
| morganfainberg | we have it subclassed in 5 areas | 21:39 |
| morganfainberg | erm as 5 exceptions | 21:40 |
| mfisch | was the hash-password option deprecated? | 21:40 |
| morganfainberg | i'm almost leaning towards yes, it should just be "unexpected error" 500 returned from the api instead of details | 21:41 |
| dolphm | morganfainberg: anything specific being exposed? | 21:41 |
| dolphm | morganfainberg: or room for other error messages to be inserted? | 21:42 |
| morganfainberg | dolphm, group_id not found by mapping_id, malformed endpoint, certificate files not found, config file not found | 21:42 |
| *** saju_m has quit IRC | 21:42 | |
| morganfainberg | the federated one i think should be obscured | 21:42 |
| dolphm | morganfainberg: i'd buy that.. the rest could be obscured but will only be raised to deployers anyway | 21:43 |
| morganfainberg | yeah | 21:43 |
| morganfainberg | i'll clean these all up | 21:43 |
| *** derek_c has joined #openstack-keystone | 21:44 | |
| *** bknudson has quit IRC | 22:07 | |
| openstackgerrit | Morgan Fainberg proposed a change to openstack/keystone: Do not expose internal data on UnexpectedError https://review.openstack.org/81385 | 22:09 |
| openstackgerrit | Morgan Fainberg proposed a change to openstack/keystone: Do not expose internal data on UnexpectedError https://review.openstack.org/81385 | 22:10 |
| *** nkinder has quit IRC | 22:12 | |
| openstackgerrit | Morgan Fainberg proposed a change to openstack/keystone: Do not expose internal data on UnexpectedError https://review.openstack.org/81385 | 22:12 |
| * morganfainberg grumbles about some extra changes sneaking in | 22:13 | |
| *** wchrisj_ has quit IRC | 22:14 | |
| *** wchrisj has joined #openstack-keystone | 22:14 | |
| marekd | is it safe to call assertEqual for dicts equality? https://github.com/openstack/keystone/blob/master/keystone/tests/test_serializer.py#L29 | 22:17 |
| *** derek_c has quit IRC | 22:17 | |
| morganfainberg | marekd, i think there is a assertDictEqual | 22:17 |
| *** leseb has joined #openstack-keystone | 22:17 | |
| morganfainberg | marekd, yeah use self.assertDictEqual | 22:17 |
| marekd | morganfainberg: whoa, would be super cool. | 22:17 |
| morganfainberg | mfisch, sorry just saw your question, which option? | 22:18 |
| mfisch | morganfainberg: I found an old web reference to a config option called hash-password | 22:18 |
| morganfainberg | mfisch, in keystone? | 22:19 |
| mfisch | our FreeIPA box is complaining that we're trying to create users with hashed passwords | 22:19 |
| mfisch | morganfainberg: yeah, but I don't see any modern references to it. | 22:19 |
| *** leseb has quit IRC | 22:19 | |
| morganfainberg | oooh | 22:19 |
| morganfainberg | hmmm. | 22:19 |
| *** leseb has joined #openstack-keystone | 22:19 | |
| mfisch | morganfainberg: https://www.ibm.com/developerworks/community/wikis/home?lang=en#!/wiki/OpenStack/page/keystone.conf | 22:19 |
| mfisch | I assume it was deprecated | 22:19 |
| morganfainberg | mfisch, i am guessing so. | 22:21 |
| marekd | dolphm: morganfainberg: hmm, strange, json->xml->json seems to have inconsistency, but i don't feel like it's a topic for my federation patch: http://pasteraw.com/s9eaf818sbz1udlyr43943h7airmqgg. | 22:21 |
| marekd | dolphm: morganfainberg (see 'extras') | 22:21 |
| morganfainberg | marekd, extras thing | 22:22 |
| *** nkinder has joined #openstack-keystone | 22:22 | |
| marekd | morganfainberg: known issue? | 22:22 |
| morganfainberg | marekd, i am guessing you are converting from a SQLAlchemy object based on dictbase? | 22:22 |
| marekd | morganfainberg: yes, basically an unscoped token. | 22:23 |
| morganfainberg | marekd, hmm. | 22:23 |
| marekd | morganfainberg: what i really want to check is to see whether xml will complain about names/tags, nothing more. | 22:23 |
| morganfainberg | marekd, yeah xml doesn't see the extras | 22:23 |
| marekd | morganfainberg: i'd be happy to remove extras from the initial json dict. | 22:24 |
| morganfainberg | marekd, invert your expected/refernce in your dicteqlas | 22:24 |
| morganfainberg | marekd, xml i think smushes those out since they don't exist | 22:24 |
| morganfainberg | marekd, so you either need to pop extras off the original | 22:24 |
| morganfainberg | marekd, or figure out how to add it back in :P (hint, this is the wrong choice) | 22:25 |
| marekd | morganfainberg: thanks! | 22:25 |
| marekd | haha, 100x faster working in your tz :P | 22:25 |
| marekd | and get real-time consultancy :P | 22:25 |
| morganfainberg | marekd, though you might run into issues w/ users et al | 22:25 |
| morganfainberg | marekd, LOL | 22:26 |
| morganfainberg | marekd, likely the fix is 'if not json.get(extras): json.pop(extras)' | 22:26 |
| marekd | morganfainberg: no i won't in this particular case, seriously what would satisfy me to see serializer.to_xml() not failing, nothing more.... | 22:26 |
| morganfainberg | marekd, hehe | 22:26 |
| morganfainberg | marekd, sounds good, but i would check to see if 'extras' has anything in it | 22:27 |
| morganfainberg | before removing it | 22:27 |
| marekd | hmm, but directly in assertSerializeToXML(self, json_body) here in test_serializer's method? | 22:27 |
| marekd | morganfainberg: ^^ | 22:27 |
| morganfainberg | marekd, hmm | 22:30 |
| morganfainberg | marekd, no you're right | 22:30 |
| morganfainberg | not in the assertSerializetoXML | 22:30 |
| marekd | morganfainberg: OK | 22:30 |
| *** lbragstad has left #openstack-keystone | 22:42 | |
| *** lbragstad has joined #openstack-keystone | 22:51 | |
| openstackgerrit | Marek Denis proposed a change to openstack/keystone: Store groups ids objects list in the OS-FEDERATION object. https://review.openstack.org/81277 | 22:52 |
| *** lbragstad is now known as lbragstad__ | 22:55 | |
| *** nkinder has quit IRC | 22:57 | |
| marekd | https://github.com/openstack/keystone/blob/master/keystone/tests/test_v3_federation.py#L891 - just notices one (passing) test is disabled, due to removed test_ prefix. In order to merge this trivial change do I need to go through bug->fix->approve process? I am asking about the bug espcially... | 23:00 |
| *** devlaps has joined #openstack-keystone | 23:00 | |
| *** gokrokve has quit IRC | 23:01 | |
| *** browne has quit IRC | 23:01 | |
| *** devlaps1 has quit IRC | 23:02 | |
| *** leseb has quit IRC | 23:11 | |
| openstackgerrit | A change was merged to openstack/identity-api: merge OS-FEDERATION objects together https://review.openstack.org/81022 | 23:11 |
| *** leseb has joined #openstack-keystone | 23:12 | |
| openstackgerrit | Marek Denis proposed a change to openstack/keystone: Rename scope_to_bad_project() to test_scope_to_bad_project(). https://review.openstack.org/81395 | 23:13 |
| marekd | morganfainberg: dolphm stevemar : ^^ should not take too long for a review :-) | 23:14 |
| stevemar | marekd, hehe, no, it won't | 23:15 |
| *** leseb has quit IRC | 23:16 | |
| marekd | ok, i am out of here. cheers! | 23:19 |
| *** marekd is now known as marekd|away | 23:19 | |
| *** henrynash has quit IRC | 23:23 | |
| stevemar | see ya marekd|away | 23:27 |
| dolphm | marekd|away: good catch! | 23:38 |
| dolphm | morganfainberg: marekd|away: "extras" was never part of the spec and it serves no useful purpose, hence it's not tested for json <-> xml conversion ;) | 23:41 |
| morganfainberg | dolphm, aha | 23:41 |
| morganfainberg | dolphm, https://review.openstack.org/#/c/81385/ there is an outstanding docstring stupid i left in | 23:42 |
| morganfainberg | dolphm, but there ya go | 23:42 |
| morganfainberg | line 294 of exceptions.py is ... well wrong. | 23:43 |
| morganfainberg | except if you factor in the super call | 23:43 |
| dolphm | morganfainberg: why change it from message_format to __message_format just for 500's? | 23:43 |
| morganfainberg | dolphm, @property does the switching. I guess security error could receive a similar treatment? | 23:44 |
| dolphm | morganfainberg: ah, overlooked that | 23:45 |
| morganfainberg | dolphm, actually no security error shouldn't get the same treatment | 23:45 |
| morganfainberg | basically it squashes all the 500s down to a generic 500 unless debug is enabled | 23:46 |
| morganfainberg | regardless of what people pass to it / define on the subclasses (if someone defines message_format, sure it'll override, but we can stop that via reviews) | 23:46 |
| dolphm | morganfainberg: don't we have a test_exception module? | 23:47 |
| morganfainberg | dolphm, no, we test it in test_wsgi | 23:47 |
| morganfainberg | as a side effect i guess | 23:47 |
| morganfainberg | dolphm, want me to add in an explicit test exception? | 23:48 |
| morganfainberg | wouldn't be hard to do | 23:48 |
| dolphm | morganfainberg: https://github.com/openstack/keystone/blob/master/keystone/tests/test_exception.py#L103-L170 | 23:48 |
| morganfainberg | oh wtf. i overlooked it | 23:48 |
| * morganfainberg can't brain. | 23:48 | |
| morganfainberg | ok i'll add explicit for unknown error | 23:48 |
| morganfainberg | dolphm, thanks *is embarassed* | 23:49 |
| dolphm | morganfainberg: exceptions are so stable you've just never had to look at the tests :P | 23:49 |
| morganfainberg | LOL | 23:49 |
| morganfainberg | yeah | 23:49 |
| dolphm | i think this is supposed to be me http://i.imgur.com/tektZzm.jpg | 23:55 |
| dolphm | coming soon to a teespring near you | 23:56 |
| jamielennox | dolphm: can you have another go at: https://review.openstack.org/#/c/60752/ i have people wanting it | 23:56 |
| jamielennox | gyee: ^^ | 23:56 |
| gyee | jamielennox, just a suggestion here https://review.openstack.org/#/c/60752/14/keystoneclient/httpclient.py | 23:58 |
| gyee | otherwise, looks good | 23:58 |
Generated by irclog2html.py 2.14.0 by Marius Gedminas - find it at mg.pov.lt!
