17:01:03 #startmeeting keystone-office-hours 17:01:04 Meeting started Tue Jun 19 17:01:03 2018 UTC and is due to finish in 60 minutes. The chair is lbragstad. Information about MeetBot at http://wiki.debian.org/MeetBot. 17:01:05 Useful Commands: #action #agreed #help #info #idea #link #topic #startvote. 17:01:07 The meeting name has been set to 'keystone_office_hours' 17:01:46 lookit i spy a gyee 17:02:12 stepping away to grab lunch real quick and i'll back back to talk about migration options 17:03:18 kmalloc, IRC is my source of enlightenment :-) 17:03:55 I'll review the IRC log tomorrow. Then the patch will be ready tomorrow I think. 17:20:33 knikolla: cmurphy: there's a thread on the Edge ML on Keystone architectures which touches on federation 17:21:02 knikolla: cmurphy: is either of you subscribed to that ML and could chime in? 17:21:19 lbragstad: kmalloc: the question applies to you too in case you're interested ^^ 17:30:35 * cmurphy resubscribes 17:34:59 thanks wxy 18:12:04 http://lists.openstack.org/pipermail/openstack-dev/2018-June/131630.html 18:15:20 lbragstad++ 18:54:55 Jeremy Freudberg proposed openstack/keystone master: Expose duplicate role names bug in trusts https://review.openstack.org/576610 18:54:55 Jeremy Freudberg proposed openstack/keystone master: Fix duplicate role names in trusts bug https://review.openstack.org/576611 18:55:24 ^ lbragstad, kmalloc: there it is 19:03:46 jeremyfreudberg: awesome 19:03:59 kmalloc: lemme know when you free to continue to migration stuff 19:04:06 (caught up in TC scrollback) 19:38:33 lbragstad: i am 19:38:46 lbragstad: landlords just left (and I have 2 more errands to run today but they can wait) 19:38:58 need to get the AC from the storage unit and a 2nd one for my office. 19:39:28 Merged openstack/keystoneauth master: Add minimum version for requirements https://review.openstack.org/575685 19:39:39 lbragstad: we need to fix @wip to be smarter 19:39:51 lbragstad: i want it to handle explicit "what kind of error ot expect" 19:39:55 not just "an error" 19:40:25 jeremyfreudberg: OH it's trust with implied roles that get expanded 19:40:29 jeremyfreudberg: thats... wow. 19:40:32 jeremyfreudberg: nice catch 19:41:13 lbragstad: because i dislike that @wip doesn't lock us into a specific error. what if we change something and the error case changes... thats bad =/ 19:43:15 kmalloc: happy to help. since my newest patch prevents keystone from shooting itself in the foot, do you think we even need https://review.openstack.org/#/c/576548 (which covers up the foot shooting) 19:43:44 nah, we don't need it. it isn't a bad case to use suspenders and a belt though :P 19:53:05 jeremyfreudberg: was that the cause of your issue in sahara? 19:53:37 lbragstad: yes, this bug was the only cause (the case sensitivity stuff did NOT affect us) 19:54:20 so sahara was affected by duplicate role names in the token response? 19:54:24 strange 19:57:30 Ben Nemec proposed openstack/oslo.policy master: Avoid redundant policy syntax checks https://review.openstack.org/511426 19:57:38 lbragstad: yes, because the token which had duplicate role names was used to create a second trust (in heat) 19:58:18 oh - so sahara was pulling that list, which included duplicates, and then put them in a request to build a new trust, which broken 19:58:20 broke8 19:58:27 /me sigh 19:58:32 broke* 19:58:45 correct 20:03:19 lbragstad, kmalloc, mordred: https://review.openstack.org/576634 Release keystoneauth 3.9.0 20:03:58 * efried hopes that was done correctly, been a while 20:04:12 efried: looks good to me 20:04:42 jeremyfreudberg: i assume you already have a patch that corrects that behavior in sahara? 20:05:21 lbragstad: no, because it's all tied up in how sahara and heat interact 20:05:45 sahara passes a token to heat which just so happens to have this problem, and heat tries to create a trust with it 20:07:45 Morgan Fainberg proposed openstack/keystone master: Add Flask-RESTful as a requirement https://review.openstack.org/574414 20:08:12 Morgan Fainberg proposed openstack/keystone master: Implement scaffolding for Flask-RESTful use https://review.openstack.org/574415 20:08:28 Morgan Fainberg proposed openstack/keystone master: Keystone adheres to public_endpoint opt only https://review.openstack.org/574502 20:08:34 Morgan Fainberg proposed openstack/keystone master: Convert json_home and version discovery to Flask https://review.openstack.org/574736 20:09:34 the tl;dr version is that it's usually not sahara's fault, and that if sahara with the integration for all components works, then OpenStack works 20:09:37 * tosky runs :) 20:11:11 lbragstad: ^ did I get the minimum bit added ok on 574414? 20:11:14 this is the start of a bug fix if anyone is interested in reviewing - https://review.openstack.org/#/c/562714/ 20:11:45 kmalloc: looks ok to me 20:17:29 lbragstad: migrations? 20:17:45 sure 20:19:37 so - would we be able to take the traditional path for the migration? 20:19:50 create new table with correct schema, migrate data, drop old table? 20:20:18 probably 20:20:58 it is likely to be a headache, because you have to do a lot of heavy lifting to determine which registered limit to FK to 20:21:03 because name is non-unique 20:21:27 so its, (name, service[, region]) for each migration to figure out what to FK to 20:22:52 name as in service name? 20:23:00 no, you mean resource_name 20:23:10 resource_name 20:23:11 yeah 20:23:55 Morgan Fainberg proposed openstack/keystone master: Add support for before and after request functions https://review.openstack.org/576637 20:27:02 hmm 20:29:19 lbragstad: question regarding DocumentedRuleDefaults 20:29:28 so, what was the intended purpose of the description field? 20:29:59 I ask because Barbican uses the same rule for multiple parts of the API with similar but not the same purpose 20:30:06 it's to describe the API in human terms as opposed to just being POST /v3/users 20:30:23 you could say description='Create a new user' 20:30:30 right. I'm wondering if moving the description /into/ the operations dict might be useful 20:31:33 example: https://docs.openstack.org/barbican/latest/api/reference/secret_metadata.html#put-v1-secrets-uuid-metadata-key and https://docs.openstack.org/barbican/latest/api/reference/secret_metadata.html#put-v1-secrets-uuid-metadata 20:31:40 utilize the same rule 20:32:37 but server different functions. We could 1) abstract the description to something that fits both 2) cover one but not the other 3) change DocumentedRuleDefault to handle description at the operation level 20:33:05 I'm guessing they aren't the only ones using the same rule for similar but different apis 20:34:42 i'm not quite understanding i don't think 20:35:01 is the problem that they use the same rule to protect the policy? 20:38:20 lbragstad: lemme submit a PS and show you 20:38:30 gimmie 5, wrapping up this one controller 20:45:21 okay lbragstad: so for their secretmeta controller they have two endpoints: /v1/secrets/{secret-id}/metadata and /v1/secrets/{secret-id}/metadata/{key-id} 20:45:31 https://review.openstack.org/#/c/575218/8/barbican/common/policies/secretmeta.py 20:45:37 four rules, with 6 actions 20:45:56 secret_meta:put and secret_meta:get rules are used in both endpoints 20:46:24 similar actions but not the same. However the way that they have laid out their poicy makes a single description field hard to use imo 20:46:24 oh 20:46:28 unless I'm being too verbose 20:46:39 yeah. and if they did it, you know others did too 20:46:47 I'm taking notes on re-occuring patterns 20:47:01 so - in that case, is there anything stopping barbican from creating a new policy for other of those endpoints? 20:47:27 lbragstad: not that I am aware of 20:47:38 you can keep the policy value/check_str the same 20:47:38 other than they don't want to make any 'policy changes' in Rocky 20:48:40 breaking them apart would let you be more descriptive about each policy since it's making each more specific 20:48:50 right. I'll make a note of that 20:49:02 lbragstad: I've been using https://wiki.openstack.org/wiki/Barbican/Policy to help me track this 20:49:09 and expanding it quite a bit 20:49:15 do we have anything akin to this in keystone? 20:49:21 then you'd just change the barbican code to enforce one of the endpoint on the new policy 20:49:32 * hrybacki nods 20:49:32 we haven't used a wiki in ages 20:49:44 ack 20:49:47 if an operator overrides the default 20:49:53 it can be aliased 20:50:29 https://docs.openstack.org/oslo.policy/queens/reference/api/oslo_policy.policy.html#oslo_policy.policy.DeprecatedRule 20:50:43 I wrote up an example in there - but you might not need something that involved 20:51:11 oh that's nice lbragstad 20:51:19 they do have a decprecated api 20:51:35 i wrote an example of breaking a policy into more granular policies https://docs.openstack.org/oslo.policy/queens/reference/api/oslo_policy.policy.html#oslo_policy.policy.DocumentedRuleDefault 20:51:45 its the example right above ^ 20:51:56 * hrybacki nods 20:52:11 Morgan Fainberg proposed openstack/keystone master: Implement base for new RBAC Enforcer https://review.openstack.org/576639 20:53:42 Gage Hugo proposed openstack/keystone master: WIP Add docs for case-insensitivity in keystone https://review.openstack.org/576640 20:56:25 kmalloc: pulling down the client patches quick for limits and checking some existing constraints 20:58:29 okie 20:58:37 lbragstad: ... i hate our enforcement model 20:58:48 it's such a PITA to work with 20:58:55 i think i almost have it working without decorators. 20:58:56 ? 20:59:08 oh... @controller.protected? 20:59:16 yeah 20:59:17 https://review.openstack.org/#/c/576639/ 20:59:18 see 20:59:28 it's starting to come together in a much cleaner way 21:00:37 all methods on flask dispatched requests will have an "after request" check to ensure enforcement was called 21:01:01 and in the method it is the responsibility of the contributor to do ".enforce_call() 21:01:13 which will do a lot of the magic @protected does now. 21:01:41 but eliminates all the stupid callback bits. 21:04:05 http://paste.openstack.org/raw/723860/ 21:04:33 so - for the migration 21:04:49 1.) create a new table called limits 21:04:53 ok 21:05:01 that has the correct schema, FK relationship 21:06:31 yes 21:06:41 and the new model object that is smarter/loads the right things. 21:06:44 2.) for every limit, check the limit.resource_name, limit.service_id, limit.region_id (if applicable) 21:07:06 use those values to query the registered limit table 21:07:17 finding the id of the registered limit those values apply to 21:07:48 and update the limit.registered_limit_id to the registered limit id of that query 21:08:09 yes. 21:09:06 hmmm 21:09:15 could this be done without another table? 21:09:42 what if step 1 is just adding a new column for limit.registered_limit_id 21:09:45 ? 21:11:30 well, we are already doing a pivot for the PK change 21:11:44 is that being done on the limit table though? 21:11:49 yah 21:11:51 on both 21:11:53 or the registered limit table? 21:11:56 oh 21:12:10 https://etherpad.openstack.org/p/keystone-unified-limit-migration-notepad 21:12:31 oh holy crap we're adding more triggers 21:12:36 * kmalloc is going to cry 21:12:47 * lbragstad pumps brakes 21:12:53 we need to stop adding triggers in mysql migrations 21:12:58 they are terrifying and horrible 21:13:13 https://review.openstack.org/#/c/576025/5/keystone/common/sql/expand_repo/versions/046_expand_update_pk_for_unified_limit.py 21:13:33 hold up, if we're able to isolate the migrations a bit, and do one of the in place, we might not have to deal with the triggers bit (since we're not creating a new table) 21:13:34 and ultimately just cause tears 21:14:14 the PK change is going to need to be new tables 21:14:29 i pretty much want to -2 that for the trigger reason only 21:14:36 triggers should never have been used 21:14:48 ok - let's try and write down the changes we need 21:14:50 https://etherpad.openstack.org/p/keystone-unified-limit-migration-notepad 21:14:53 they are a terrible choice and are very very hard to debug. 21:14:58 and then see if we can break up the migrations to be a bit easier 21:18:01 lbragstad, kmalloc: i'm undecided on a review as to what score to give and what is considerent nitpicking or not. would be helpful to get your opinion 21:18:14 knikolla: what's up? 21:18:34 https://review.openstack.org/#/c/538371/6/keystone/tests/unit/limit/test_backends.py 21:19:05 the test correctly tests deletion of the limits, but it doesn't test that it didn't delete limits of other projects 21:19:12 the WHERE part of the sql query. 21:21:59 oh - that seems like it would be a sane test case 21:22:04 if not in that patch, at least in a follow on 21:24:21 lbragstad: alright. didn't want to look like I was nitpicking. 21:25:13 i think it's worthwhile to consider the author - wxy is always responsive to feedback and isn't going to be discouraged by constructive criticism 21:25:52 ++ 21:25:56 nit picking: This is a bad way to do X, please reimplement stylistically different -- or "your verbiage is not accurate". 21:26:11 "This needs an expanded test case like FOO" - not nit picking 21:26:17 and very reasonable 21:26:39 esp. for someone like wxy who gets it's not personal and is super responsive 21:26:59 knikolla: are you asking because of the ml culture change thread? 21:27:43 lbragstad: yes, in part. 21:29:49 advocating for further testing... is not nitpicking. you could easily also say "this can be a followup" and/or take on that followup yourself 21:30:15 ++ 21:30:34 just felt I should get some feedback first before I give feedback. 21:31:35 thanks! 21:32:57 knikolla: also if you see my style of review where i say [NIT] 21:33:07 it helps clarify what you think is a trivial thing 21:33:16 often i might have a review with 10+ nits but still +2 21:33:28 all nits can be fixed down the line. 21:33:43 kmalloc: agree. most of my undecisiviness was in the score between +1/+2 21:34:10 often +1 "everything looks good, but i have a serious question that needs answering, aka "what happens when X"" 21:34:15 imo 21:34:19 (from a core) 21:34:38 i avoid no-score because it seems like those get lost 21:34:52 so -1 = hey, I have a concern this might not do waht you want... or there is just no testing 21:35:04 i've been trying to do more no scores that i have in the past, but yeah... they are harder to see 21:35:16 unless i'm parsing emails from gerrit 21:35:19 ++ 21:35:19 +1 = looks good, but expand [in the review, not even a patchset] on what i need answered 21:35:30 +2 = anything nits only or close enough that it wont cause issues 21:35:45 that helps 21:35:47 -2 = "not just no, awwww heck no, but here is why we can't do it" 21:36:33 i used to no-score anything that i just had questions about, vs +1, 21:51:37 kmalloc: rewrote the proposed migration without triggers 21:52:06 looking 21:52:17 yep. 21:52:28 that would be my preference. 21:52:34 but that sums up the alternative 21:53:11 so - that is going to require the database model to be cluttered for a release 21:53:26 yes, it does. 21:53:55 does it also mean we have to run keystone at a specific version for a certain amount of time before moving to the next release? 21:54:13 nova had an issue with that with one of their migrations in the past i think, but there wasn't much they could do about it 21:54:27 no. 21:55:03 if you run migrate_data in Rocky, which we use to setup the FK, in stein we just stop referencing the old column. 21:55:15 and we can drop it from the model and have a contract migration 21:55:21 that drops the actual column 21:55:30 what if i'm on Queens 21:55:36 i upgrade to rocky 21:55:44 and the immediately upgrade to Stein 21:55:48 (ff upgrades) 21:55:50 do we support upgrading to rocky and then to stien without running data_migrate? 21:56:09 because it seems like i keep getting a different answer on this 21:56:30 no - i think running data migrate is required 21:56:45 then data migrate moves the data (fk) and stien doesn't care about old code. 21:56:50 and old columns 21:56:55 when the nodes are down 21:57:02 stien contract removes that old column 21:57:07 yep 21:57:17 * lbragstad ventures into edge case land 21:57:26 let's say i do all this without downtime 21:57:39 we are going to have to do a nullable allowed for the extra columns 21:57:44 in rocky. 21:57:57 because stien may or may not know about the columns 21:57:58 and a limit gets updated/created in the rocky migration 21:58:13 so the limit.registered_limit_id of that entry isn't populated 21:58:18 right so 21:58:21 and then i upgrade to stein 21:58:29 in rocky, registered_limit_id is nullable 21:58:41 in rocky service_id, resource_name, region_id are all nullable 21:58:50 (code enforcement on if they are populated) 21:59:10 in stien we drop service_id, resource_name, region_id, and make registered_limit_id not nullable 21:59:19 stein* 22:00:23 what happens if a limit sneaks in after the migration in rocky has been run to populate limit.registered_limit_ids? 22:00:46 on an older node? 22:00:58 like a Queens node accepts the POST /v3/limits requests 22:01:02 isn't migrate done when all nodes are rocky? 22:01:08 nope 22:01:12 *facepalm* 22:01:13 https://docs.openstack.org/keystone/latest/admin/identity-upgrading.html 22:01:14 we should fix that 22:01:17 that is bad. 22:01:21 how do we fix that though? 22:01:29 you need a place to transition things? 22:01:33 migrate happens once all nodes are on new code 22:01:37 data-migration 22:01:47 expand, upgrade code, migrate data, contract 22:01:56 code can run in n-1 scenario 22:02:00 for the schema 22:02:02 but can you run new code if the data hasn't been migrated yet? 22:02:10 app code is smart 22:02:36 mmm i know where this is going 22:02:51 you're going to make new code check all the places 22:02:56 lbragstad: https://github.com/openstack/keystone/blob/03a616d1bf5715ac74756f2cb3aec1f09352de81/keystone/identity/backends/sql_model.py#L104-L112 22:03:08 as an example 22:03:23 password_hash is the new column 22:03:27 password is the old 22:03:37 check for the new, if not there, do the thing with the old 22:03:59 writes write to both locations [in password case, because of security, we had a config option for that] 22:04:16 this is not security related, so you can just blindly write to both locations with the relevant data 22:04:26 migrate is run when the code is 100% on 22:05:39 yeah 22:05:48 another vesrion of that would be to have the new code check the old location 22:05:56 and make things consistent 22:06:14 except you still need to make sure in Stein all data is in the new place 22:06:18 which would allow you to have old and new nodes running during migrateion 22:06:31 in rocky, code does look in both places 22:06:32 yep 22:06:39 but migrate still has to occur. 22:07:12 mmmm 22:07:14 damn 22:07:16 nevermind 22:07:43 fi - data problems are hard 22:08:54 yep 22:10:23 Merged openstack/keystone master: Add policy for limit model protection https://review.openstack.org/562714 22:18:30 that's going to be a total pain 22:19:20 we're going to either need to change our upgrade process to make operators get all nodes on the new release before running migrate... or we break our rules and perform the migration in contract 22:23:09 i've done that a couple times 22:23:17 because it was the only way to avoid breaking rules 22:23:19 it sucked 22:23:57 if i think about it, it would easier to do that then change our upgrade process which likely breaks upgrade automation for operators :( 22:24:42 we should still change our upgrade process 22:25:52 even if it isn't this cycle 22:25:59 telegraph "we're changing to X" 22:29:32 Morgan Fainberg proposed openstack/keystone master: Migrate all password hashes to the new location if needed https://review.openstack.org/576660 22:29:42 lbragstad: ^ should be an easy review 22:29:57 ack 22:30:11 lbragstad: that does the lifting of moving password to password_hash if password_hash is none [cleanup] 22:30:23 and then in S, we can contract and drop the password.password column 22:30:29 :) 22:30:37 local test succeeded, we'll see what zuul says 23:05:39 attempted to summarize the migrations https://etherpad.openstack.org/p/keystone-unified-limit-migration-notepad 01:51:41 lbragstad: sqlite doesn't support change primary key, that's the reason I re-create the table 01:54:37 lbragstad: a similar case is here: https://github.com/openstack/keystone/blob/master/keystone/common/sql/migrate_repo/versions/095_add_integer_pkey_to_revocation_event_table.py#L20-L22 02:01:21 kmalloc: lbragstad : If I understand correctly, trigger is used for sync the old data which is newly created during upgrading to the new schema. If drop the triggers, how to deal with this case? 02:04:32 Use app-level logic to keep the data in sync for a release. Triggers are very hard to debug, are not well tested and could end up causing issues for the small number of cases that use them. 02:05:25 So, keystone just writes to both places for rocky, and in stien we drop support for the old way. But defer to lbragstad if we are doing that or triggers are acceptable. 02:05:42 And contract happens in stein then. 02:05:49 kmalloc: emm, that's a way, let the code deal with the mix version data 02:07:19 That is how I usually handle these cases, easier to test/get right. And not a shot in the dark for say pgsql (very under tested). And not fighting with sqlite to 'test' the code. 02:09:51 But again, I am not blocking triggers, I just prefer to not use them. Checking with lbragstad on the way forward is best. I'll roll with what he recommends. 02:10:59 kmalloc: Ok, got it. 02:12:03 kmalloc: and for schema change, adding PK is not allowed in sqlite. So I tried to re-create the table in my new PS. 02:12:20 Hm. That is annoying. 02:13:43 yeah, I tested the in-place way in my env which Lance wrote here https://etherpad.openstack.org/p/keystone-unified-limit-migration-notepad ,all works well, then I upload the PS2 in this way, but the CI tells the sqlite doesn't like it. :( 02:14:47 I can help write some sqlite specific code. 02:14:50 If needed. 02:15:28 We have done that in the past a few times, special case for upgrade test. We will get some better tests in gate. Iirc. 02:18:00 kmalloc: so you prefer to use the in-place way with specific sqlite related code? 02:21:02 That is my preference, but I can't really impose my view here as the way forward if the general consensus is "use triggers" 02:21:35 Merged openstack/oslo.policy master: Add examples and clarification around scope_types https://review.openstack.org/568901 02:22:48 kmalloc: OK, let's wait for others opinion as well. Thanks for your suggestion. 02:23:07 +( 02:23:10 ++ 02:51:16 sunguangning proposed openstack/oslo.policy master: Remove some description from oslo policy https://review.openstack.org/576683 03:58:36 out of curiosity, are they any upstream tools for testing custom policy? 04:11:53 Chason Chan proposed openstack/python-keystoneclient master: Update IdentityProviderManager docstring https://review.openstack.org/576708 07:00:04 Morgan Fainberg proposed openstack/keystone master: Implement base for new RBAC Enforcer https://review.openstack.org/576639 07:02:24 Morgan Fainberg proposed openstack/keystone master: Implement base for new RBAC Enforcer https://review.openstack.org/576639 07:24:04 Morgan Fainberg proposed openstack/keystone master: Add Flask-RESTful as a requirement https://review.openstack.org/574414 07:24:32 Morgan Fainberg proposed openstack/keystone master: Implement scaffolding for Flask-RESTful use https://review.openstack.org/574415 07:24:43 Morgan Fainberg proposed openstack/keystone master: Keystone adheres to public_endpoint opt only https://review.openstack.org/574502 07:24:58 Morgan Fainberg proposed openstack/keystone master: Convert json_home and version discovery to Flask https://review.openstack.org/574736 07:25:05 Morgan Fainberg proposed openstack/keystone master: Add support for before and after request functions https://review.openstack.org/576637 07:25:13 Morgan Fainberg proposed openstack/keystone master: Implement base for new RBAC Enforcer https://review.openstack.org/576639 07:27:45 adriant: https://docs.openstack.org/patrole/latest/ 07:51:31 wangxiyuan proposed openstack/keystone master: Strict two level hierarchical limit https://review.openstack.org/557696 09:54:22 hi, https://bugs.launchpad.net/keystone/+bug/1777671 in this bug what exactly we need to do? 09:54:23 Launchpad bug 1777671 in OpenStack Identity (keystone) "Incorrect use of translation _()" [Medium,Triaged] - Assigned to Deepak Mourya (mourya007) 10:26:04 deepak_mourya: here's an example of what needs to be fixed: http://git.openstack.org/cgit/openstack/keystone/tree/keystone/auth/core.py#n170 10:26:26 the string is being marked for translation with _() and then being passed to both the LOG and the exception 10:26:52 but we don't actually want to have the string for the LOG translated, only for the exception 10:27:37 so it should change to something like msg = 'Domain name cannot contain reserved characters.' ; LOG.warning(msg) ; raise exception.Unauthorized(message=_(msg)) 10:28:04 cmurphy: ok got it now 10:28:27 Thanks for the reply 10:28:46 no problem 12:39:49 o/ 13:32:10 this is a good documentation patch if anyone is interested https://review.openstack.org/#/c/569741/ 13:43:28 cmurphy: :) 13:44:47 kmalloc: sup 13:45:55 good morning! 13:46:07 good afternoon! 13:46:07 or evening... or whatever it is wherever you are 13:46:18 * kmalloc is pre-coffee. 13:47:26 lbragstad: i think i've done a reasonable job breaking down @protected and what we're extracting so a proper .enforce_call can be made. https://git.openstack.org/cgit/openstack/keystone/tree/keystone/common/rbac_enforcer/enforcer.py?h=refs/changes/39/576639/4#n157 13:47:40 lbragstad: it's not done, but it's on it's way. 13:48:15 sounds good 13:48:16 ayoung, adriant: ^ cc, just because i know you tried to take a stab at diving into @protected as well 13:48:25 i can start looking at that today or tomorrow 13:48:28 this is very flask-specific. 13:48:35 yeah just a "hey does this make sense" pass 13:48:55 is fine, because if it looks better than @protected, i've done something right. 13:49:23 yeah - that's my main goal 13:49:57 if we can remove @protected in favor of something that puts the authorization logic closer to business code 13:50:57 or makes authorization logic more clean/clear i think that'll be a big win 13:52:12 which will also be super handy for the default roles + scope types work 13:52:19 yeah. 13:52:28 the docstrings need further expansion too. 13:52:45 and we can add another wrapper syntactic sugar-style to it on top of enforce_call 13:53:36 but i am feeling much better about the enforcer having spent a ton of time diving into @protected and trying to understand the dense craziness. 13:54:14 yeah - it's intense 14:07:04 going back to the database migration discussions we were having yesterday 14:07:16 i _think_ we'll need three migrations 14:07:43 1. for auto-incrementing primary keys in registered limits 14:07:54 2. for auto-incrementing primary keys in limits 14:08:09 3. for reducing duplicate data between limit and registered limit tables 14:10:20 i think we're at a point with the notes in https://etherpad.openstack.org/p/keystone-unified-limit-migration-notepad that we can probably move them to bugs instead 14:23:54 https://bugs.launchpad.net/keystone/+bug/1777892 14:23:55 Launchpad bug 1777892 in OpenStack Identity (keystone) "Reduce duplicate data between unified limit tables" [Medium,Triaged] 14:30:36 https://bugs.launchpad.net/keystone/+bug/1777893 14:30:37 Launchpad bug 1777893 in OpenStack Identity (keystone) "Limit and registered limit tables should auto-increment primary keys" [Medium,Triaged] 14:40:23 cmurphy: would i be able to get your eyes on https://review.openstack.org/#/c/571309/ whenever you have a minute? 14:40:45 lbragstad: looking 14:41:04 it should be all squared away per your last set of comments 14:43:39 o/ 14:48:18 lbragstad: lgtm! 14:48:30 thanks cmurphy 15:12:30 lbragstad: i also advised wxy to confirm with you the direction we're going, trigger or not 15:12:51 lbragstad: i will stand behind whichever is the end choice, but i've made my opinion clear 15:13:09 sure - it's a big part of the reason why i wanted to write down a couple of the approachs 15:13:16 i'd like more feedback on it 15:13:30 and it's probably easier for people to parse if they have something they can look at 15:13:36 yep. 15:13:50 but yeah... it's hard problem 15:22:30 o/ 15:40:34 Lance Bragstad proposed openstack/keystone master: Simplify the issue token code path https://review.openstack.org/545450 15:40:45 kmalloc: ^ 15:41:34