16:00:19 <lbragstad> #startmeeting keystone
16:00:20 <openstack> Meeting started Tue Jun 19 16:00:19 2018 UTC and is due to finish in 60 minutes.  The chair is lbragstad. Information about MeetBot at http://wiki.debian.org/MeetBot.
16:00:21 <openstack> Useful Commands: #action #agreed #help #info #idea #link #topic #startvote.
16:00:23 <cmurphy> o/
16:00:24 <openstack> The meeting name has been set to 'keystone'
16:00:28 <hrybacki> o/
16:00:32 <lbragstad> #link https://etherpad.openstack.org/p/keystone-weekly-meeting
16:00:36 <lbragstad> agenda ^
16:00:40 <gagehugo> o/
16:00:45 <wxy|_> o/
16:00:52 <lbragstad> ping ayoung, breton, cmurphy, dstanek, gagehugo, hrybacki, knikolla, lamt, lbragstad, lwanderley, kmalloc, rodrigods, samueldmq, spilla, aselius, dpar, jdennis, ruan_he, wxy, sonuk
16:01:06 <knikolla> o/
16:01:26 <knikolla> i'll be mostly lurking as I'm in another meeting
16:01:50 <lbragstad> we have a lot to talk about today
16:02:09 <lbragstad> we'll give folks another minute or two though and then get started
16:03:21 <lbragstad> #topic announcements
16:03:27 <lbragstad> real quick
16:03:36 <lbragstad> if you haven't seen yet, we're starting to get a bunch of things in flight
16:03:56 <lbragstad> (e.g. default roles, unified limits, capability lists, mfa receipts)
16:04:12 <lbragstad> plenty of things to review if you're looking for something
16:04:53 <lbragstad> since I don't see kmalloc yet, we'll wait for him before jumping into the unified limits topics
16:05:05 <lbragstad> hrybacki: do you want to go through your topics in the mean time?
16:05:21 <hrybacki> sure
16:05:24 <lbragstad> #topic Ensure default roles breaking folks
16:05:47 <hrybacki> So, I more or less wanted to ask if we should review ^^
16:05:54 <lbragstad> #link https://review.openstack.org/#/c/572243/ default roles implementation
16:06:14 <hrybacki> it sounds like lbragstad has been working with folks from sahara/horizon to resolve the breakages
16:06:26 <lbragstad> #link https://review.openstack.org/#/c/576056/ proposed revert
16:06:28 <cmurphy> so is this just breaking the gate or is this going to affect users?
16:06:55 <hrybacki> cmurphy: that's hard to say. Role implications concern me a bit more now
16:07:08 <lbragstad> from what i've been seeing, it's mostly breaking on tests/infrastructure that makes assumptions about role names and casing
16:07:23 <cmurphy> but users could also be making those assumptions
16:07:27 <lbragstad> sure
16:07:28 <hrybacki> right
16:07:54 <hrybacki> I'm concerned that folks won't read the release notes, run the bootstrap, and have some implied roles they did not anticipate
16:08:18 <lbragstad> to be fair, we can only protect against that kind of stuff so much IMO
16:08:19 <cmurphy> we can't help people who don't read the release notes
16:08:31 <hrybacki> but I have no idea how to research that. Is it generally accepted that people do read them / it's on them if they dont?
16:08:40 <hrybacki> okay
16:08:57 <tosky> related question: was this change on openstack-dev with the [all] tag in the email?
16:08:57 <hrybacki> well that puts me at ease. Thanks for the push for more verbose reno cmurphy lbragstad
16:09:03 <lbragstad> it's helps protect us (as bad as that sounds?)
16:09:22 <cmurphy> i'm not worried any more about surprise implied roles, but i'm worried this bug with the duplicate role name is going to hit people
16:09:53 <cmurphy> tosky: no, i don't think we anticipated it breaking people's CI
16:10:04 <lbragstad> tosky: the approach and specification detailing all of this work has been socialized on the mailing lists since the Denver PTG last year
16:11:02 <tosky> lbragstad: with the [all] tag?
16:11:11 <lbragstad> tosky: no - we didn't use the all tag
16:11:20 <lbragstad> if i would have expected these types of issues i would have
16:11:26 <lbragstad> #link http://lists.openstack.org/pipermail/openstack-dev/2018-May/130207.html
16:11:56 <tosky> please don't take it as what's not (it's just a statement), but when something changes in keystone it's pretty much going to hit someone
16:12:02 <tosky> just a fact
16:13:33 <kmalloc> o/
16:13:33 <hrybacki> lbragstad: that's all I had for that topic
16:13:51 <cmurphy> well what's the plan though?
16:13:55 <cmurphy> roll back? or?
16:13:56 <lbragstad> ^
16:13:59 <gagehugo> are we doing the revert, or does the list of changes fix it?
16:14:01 <lbragstad> IMO we have two options
16:14:11 <lbragstad> 1.) roll back and implement case sensitivity for roles
16:14:21 <lbragstad> 2.) push forward and help clarify assumptions about role names
16:14:58 <hrybacki> I thought implementing case sensitivity for role names wasn't an option?
16:14:59 <cmurphy> i'm in favor of 1 but i wonder if that will also break people
16:15:05 <cmurphy> yeah
16:15:13 <lbragstad> it could, it's also going to be involved
16:15:24 <cmurphy> what a mess
16:15:26 <kmalloc> hrybacki: it isn't if the behavior is already case-insentivie
16:15:40 <lbragstad> and if we take that approach with role names, we also have to take it with project names and user names, etc.....
16:15:56 <tosky> so far, was the fact the roles were not case-sensitive documented somewhere?
16:16:16 <kmalloc> tosky: documented or not, behavior is our API contract
16:16:21 <lbragstad> it's a behavior across keystone API - it's not specific to role names
16:16:29 <kmalloc> and we should document it as well.
16:16:39 <kmalloc> but changing behavior outside of a security issue is mostly off the table
16:17:05 <kmalloc> (if we had all our expected behaviors documented more clearly, it would be a different thing)
16:17:08 <lbragstad> ok - so sounds like we have an action item
16:17:20 <hrybacki> well if it is an option to enforce the existing behavior across the board I'm a fan
16:17:43 <kmalloc> FTR: In names for things, i like case preserving, but case-insensitive
16:17:49 <kmalloc> in APIs.
16:17:57 <lbragstad> keystone consideres POST /v3/projects {name: Foobar} and POST /v3/projects {name: foobar} a 409
16:18:12 <kmalloc> because representing AUser and Auser and auser and auSer as all different things is somewhat painful
16:18:15 <kmalloc> UX wise.
16:18:39 <kmalloc> (not going to argue posix-isms though)
16:18:44 <cmurphy> lbragstad: is that dependent on your database backend? or is it always?
16:18:55 <gagehugo> kmalloc ++ on case preserving & case insensitive
16:18:59 <lbragstad> i tested it with mysql
16:18:59 <tosky> fine as documenting and forcing this, but consider that as a user I never realized this non-case-sensitivness of Keystone
16:19:04 <lbragstad> i'm not sure about other backends
16:19:11 <kmalloc> cmurphy: it is... default behavior in mysql.
16:19:14 <kmalloc> it *could* be changed.
16:19:19 <tosky> I've frequently seens and described roles like Member and member, with code to distinguish between both
16:19:24 <tosky> so this will be a surprise for many people
16:19:27 <cmurphy> tosky: as a maintainer i didn't realize it either ;)
16:19:36 <tosky> oh
16:19:57 <kmalloc> we would need to swap to varbinary or binary vs varchar etc to ensure case-sensitivity
16:20:15 <lbragstad> does anyone want to take a shot at writing up a doc patch for our API reference to clarify these bits (for all name resources not just role names)
16:20:15 <kmalloc> i am unsure about non-MySQL RDBMS varbinary vs varchar
16:20:52 <kmalloc> or we could change collation
16:20:59 <cmurphy> kmalloc: yeah i'm concerned that different database backends will cause different api behaviors
16:21:10 <kmalloc> e.g. utf8mb4_0900_as_cs
16:21:17 <kmalloc> but that is also MySQL-specific.
16:21:30 <lbragstad> ^ that is the suggestion i've been stumbling across a lot
16:21:58 <lbragstad> but it's operator specific, isn't is?
16:22:11 <kmalloc> you can define the column with a collation
16:22:35 <lbragstad> ah - so a migration at the very least
16:22:37 <kmalloc> we could also encode something like that via our queries (will need to look into it) through the ORM
16:23:11 <lbragstad> we could also implement this in business logic
16:23:23 <lbragstad> (trying to remove ambiguity based on the backend being used)
16:23:51 <kmalloc> for case-preserving but case-insensitive across all backends, we will need to do something like "lookup-name" and "display-name"
16:24:04 <kmalloc> where we lower(lookup-name)
16:24:07 <lbragstad> but that could still be considered backwards incompatible if someone is using a backend that supports case sensitive names
16:24:39 <kmalloc> i'm ok with backwards incompat for "this is crazypants"
16:25:00 <kmalloc> we're solidifying behavior (closest aligned with current "general" behavior)
16:25:29 <lbragstad> we also have stuff like this in our tests - which is misleading https://git.openstack.org/cgit/openstack/keystone/tree/keystone/tests/unit/test_backend_sql.py#n296
16:25:36 <lbragstad> #link https://git.openstack.org/cgit/openstack/keystone/tree/keystone/tests/unit/test_backend_sql.py#n296
16:26:02 <cmurphy> well user is special because ldap right?
16:26:11 <kmalloc> ftr, looks like PGSQL is case-sensitive by default for string comparison
16:26:12 <kmalloc> s
16:26:19 <lbragstad> but that looks like a sql specific test (given it's location)
16:26:23 <kmalloc> unless you explicitly lower() in the query.
16:26:43 <lbragstad> and we do the same thing for projects https://git.openstack.org/cgit/openstack/keystone/tree/keystone/tests/unit/test_backend_sql.py#n310
16:27:01 <cmurphy> lbragstad: i assume that's because we want to enforce the same behavior between sql and ldap backends
16:27:24 <cmurphy> did we used to allow ldap as a project backend?
16:27:31 <kmalloc> cmurphy: long time ago
16:27:33 <lbragstad> a long time ago we did - i think
16:27:40 <cmurphy> so that's probably where that's from
16:27:54 <lbragstad> i pulled up the commit - it was merge like four and a half years ago
16:27:55 <kmalloc> cmurphy: used to be IDENTITY+ASSIGNMENT was one backend, then we still allowed LDAP assignment, then we split resource/killed ldap assignment
16:28:26 <kmalloc> keystone used to have a single driver (barring token) back in... folsom?
16:28:30 <kmalloc> essex
16:28:41 <lbragstad> yeah
16:28:43 <kmalloc> all or nothing.
16:28:52 <kmalloc> ok so.
16:28:58 <kmalloc> i think the options are:
16:29:11 <kmalloc> 1) Force Case Insensitivity across all backends [somehow]
16:29:20 <kmalloc> 2) Force Case Sensitivity across all backends [somehow]
16:29:27 <kmalloc> 3) Revert change.
16:29:40 <kmalloc> 4) document inconsistent behavior based upon backend and clarify what we expect
16:30:17 <lbragstad> we know this had issues with various projects CI
16:30:27 <lbragstad> but what's the worst case for end users
16:30:29 <kmalloc> fwiw, we cater to MySQL. I am ok with forcing Case Insensitivity since that is our test-bed.
16:30:31 <cmurphy> I lean toward 4 I think, the more I think about it the more I think that attempting to recreate the member and/or Member role is more going to be a CI problem than a regular user problem
16:30:49 <kmalloc> i less ok with 2 and 3.
16:30:54 <cmurphy> and if they do try to recreate it then it 409s and no harm done
16:31:01 <lbragstad> ++
16:31:03 <kmalloc> i think 4 is going to happen in either case - documentation expansion.
16:31:09 <cmurphy> sure
16:31:27 <lbragstad> worst case a deployer already has Member and hrybacki already accounted for that in the patch that implements this
16:32:00 <cmurphy> right, bootstrap itself isn't 409ing right? it's just the openstack role create Member afterward that does?
16:32:07 <lbragstad> correct
16:32:21 <lbragstad> we try/except role creation in bootstrap to explicitly handle conflicts
16:32:46 <lbragstad> #link https://git.openstack.org/cgit/openstack/keystone/tree/keystone/cmd/bootstrap.py#n120
16:33:31 <kmalloc> bootstrap is way smarter about that than it needs to be.
16:33:33 <cmurphy> what's up with this though https://review.openstack.org/#/c/576163
16:33:35 <hrybacki> lbragstad: but is that case insensitive? It attempts to pull the role based on role name 'member' not 'Member'
16:33:37 <kmalloc> but it works to our benefit there.
16:33:40 <cmurphy> why are those ones case sensitive?
16:34:02 <kmalloc> cmurphy: JSON is case preserving.
16:34:11 <kmalloc> and clients/etc all look for case
16:34:29 <kmalloc> so Member and member match in MySQL String Comparison
16:34:35 <lbragstad> https://launchpad.net/bugs/1777359
16:34:36 <openstack> Launchpad bug 1777359 in OpenStack Dashboard (Horizon) "Unable to create a project from horizon on devstack" [Undecided,New] - Assigned to Lance Bragstad (lbragstad)
16:34:39 <kmalloc> but anything in python wont match Member and member
16:34:55 <lbragstad> frickler asked me to make some additional changes there so that the names were consistent
16:35:36 <kmalloc> this is pushing me towards needing to support case [in]sensitivity directly in keystone
16:36:12 <lbragstad> that sounds like a long term direction thing
16:36:14 <kmalloc> and ... based upon emitting data (API) behavior, we might want to do case sensitive (wince) because we can't just lower() all names on output without potentially breaking a lot of things.
16:36:29 <lbragstad> that we should apply in our api consistently for all resource names
16:37:19 <kmalloc> and that is a migration to apply that, basically swap varchar -> varbinary or add collation to the queries
16:37:46 <kmalloc> initially we can just document.
16:38:16 <kmalloc> but i think we're going to need to be consistent in string matching not just let the DB be "smart enough" [and change behaviors]
16:38:48 <lbragstad> #action gagehugo to propose a patch that clarifies the behavior with resource names and SQL-based deployments
16:38:59 <lbragstad> (gagehugo volunteered in -keystone)
16:39:02 <gagehugo> lol
16:39:08 <gagehugo> yeah we should document regardless
16:39:28 <cmurphy> ++
16:39:44 <kmalloc> #action spin up bug/task[trello] to work on long-term alignment on case-[in]sensitivity within our API (names/other filters specifically)
16:39:53 <lbragstad> any other action items we need to deal with at the moment?
16:39:55 <lbragstad> kmalloc: ++
16:40:10 <lbragstad> tosky: is there anything you'd like to see in addition to those?
16:40:56 <tosky> it looks fine, thanks
16:41:19 <tosky> (and of course please the support for people trying to understand what to fix and how to fix it)
16:41:53 <lbragstad> i'll send a post with [all] today
16:41:59 <lbragstad> to the -dev mailing list
16:42:43 <lbragstad> let's circle back to the first topic quick if no one has anything else on this specific topci
16:43:15 <lbragstad> #topic Unified Limits Update
16:43:37 <lbragstad> kmalloc: raised some concerns about unified limits last week, and thought it would be good to go through them as a group
16:43:54 <lbragstad> that way we're all on the same page as far as the plan forward goes
16:44:07 <kmalloc> ok
16:44:26 <kmalloc> if you look at the agenda there are some recommended and required changes for the limits backend
16:44:46 <kmalloc> based upon my understanding after talking it through with lbragstad
16:45:09 <kmalloc> 1) we need to index columns we are matching/searching on (e.g. "name") otherwise it gets really ugly
16:45:37 <lbragstad> to level set, we have two different tables, one for registered limits and the other for project-specific limits
16:46:00 <kmalloc> 2) a FK on a column that is not unique (very mysql-specific, in fact innodb-extension specific, doesn't work on other backends) means that any value in the column that matches is sufficient for the FK.
16:46:32 <kmalloc> right now, we have name being non-unique and have a very complex FK between three columns to avoid conflicts.
16:47:10 <kmalloc> my recommendation is go for data normalization: and eliminate duplicated data in the limit table, and FK to a specific registered_limit
16:47:24 <kmalloc> instead of FK across to name, service, region[optional]
16:47:30 <kmalloc> and duplicate the storage
16:48:03 <kmalloc> the big change to the backend code is that the registered_limit would need to be looked up (based upon name, service, region) to find the right PK to use as the FK target in the limit table
16:48:17 <kmalloc> in the etherpad i gave an example of a potential "new" limit model
16:49:11 <kmalloc> and when we do the limit lookup, we do the SQLAlchemy joined load, and just build the ref from the registered limit (service_id, region, etc).
16:49:58 <kmalloc> and override to_dict and make the model super smart on finding the right registered limit
16:50:07 <kmalloc> for from_dict.
16:50:12 <kmalloc> i know... this is super dense.
16:50:39 <kmalloc> this is getting into something it took lbragstad and I 30-40 mins via video to hammer out the behaviors and potential issues.
16:50:58 * cmurphy trusts kmalloc is right
16:51:15 <wxy|_> ++
16:51:26 <lbragstad> i think this will be good for performance in the long run, too
16:51:27 <kmalloc> but tl;dr - we need indexes. we can also eliminate duplicated data and simplify the models with some in-code changes/model smarts.
16:51:58 <kmalloc> i've tagged each item as recommended/required. the only absolutly required thing is adding indexes
16:52:07 <kmalloc> the really hard part: data migration.
16:52:14 <lbragstad> wxy|_: already has a patch up that starts this (thanks!)
16:52:20 <wxy|_> https://review.openstack.org/#/c/576025 here is a WIP patch for the Recommendation. the Index, sql model and manage logic are still missing.
16:52:21 <cmurphy> not duplicating sounds like a win
16:52:30 <kmalloc> the simplest solution if we break apart and re-make the tables, drop all current data
16:52:37 <kmalloc> and just create a new set of tables
16:52:42 <hrybacki> #link https://review.openstack.org/#/c/576025
16:52:44 <kmalloc> this is expirmental
16:52:51 <wxy|_> the schema change is most done.
16:52:55 <kmalloc> it is pretty much ok to do that.
16:53:08 <kmalloc> but i'm also ok with preserving data if we cna migrate cleanly
16:53:16 <kmalloc> wxy|_: you're doing an awesome job on this :)
16:53:26 <lbragstad> ++
16:53:39 <cmurphy> it's experimental but dropping all data sounds like an overextension of the definition of experimental
16:53:46 <wxy|_> kmalloc: lbragstad: really thanks for the recommendation.
16:53:49 <kmalloc> i am also going to spin up a change to prevent new tables from having uuid PK columns.
16:53:59 <kmalloc> unless there is a specific override set.
16:54:27 <kmalloc> this change would have been a lot less painful if we also didn't need to change the PK.
16:54:48 <kmalloc> (we don't, but uuid pks are bad(tm))
16:55:25 <kmalloc> anyway. in short, please review wxy's change. come bug me if you need insight into what we're doing and why
16:55:38 <kmalloc> and can't divine it from the code changes.
16:56:11 <cmurphy> kmalloc: maybe you could write up a little guidance on why no more uuid pks in https://docs.openstack.org/keystone/latest/contributor/ ?
16:56:21 <cmurphy> so we don't keep making the mistake
16:56:34 <kmalloc> sure. i'll also cover it in the error that is generated when a table is added with a uuid pk
16:56:51 <cmurphy> ++
16:56:52 <lbragstad> we should walk through options for the migration though
16:57:18 <lbragstad> and vet any options we have other that 'truncate limit; truncate registered_limit;'
16:57:29 <lbragstad> s/that/than/
16:57:31 <kmalloc> sure.
16:57:55 <lbragstad> whatever comes out of that discussion will be good context to include for justificiation
16:58:01 <kmalloc> data migration is going to be ugly regardless.
16:58:03 <lbragstad> (i've never done this before)
16:58:04 <cmurphy> create new tables; copy data ; drop old tables; rename tables?
16:58:27 <kmalloc> pretty much
16:58:28 <lbragstad> ^ that's the approach we usually take
16:58:35 <kmalloc> or: create new table, reference new table
16:58:41 <kmalloc> eventually drop old table
16:58:47 <lbragstad> (two minutes remaining)
16:59:10 <lbragstad> we can continue this in -keystone, too
16:59:56 <lbragstad> hrybacki: sorry we didn't get back around to your last topic
17:00:03 <hrybacki> no worries lbragstad
17:00:15 <lbragstad> is that something we can cover next week?
17:00:22 <lbragstad> or is it time sensitive?
17:00:29 <hrybacki> yeah, it can wait
17:00:32 <lbragstad> ok
17:00:35 <hrybacki> or I'll bring it up in channel as needed
17:00:39 <lbragstad> i appreciate the time everyone
17:00:43 <lbragstad> #endmeeting