19:00:06 <lbragstad> #startmeeting office-hours
19:00:07 <openstack> Meeting started Tue Jul 11 19:00:06 2017 UTC and is due to finish in 60 minutes.  The chair is lbragstad. Information about MeetBot at http://wiki.debian.org/MeetBot.
19:00:09 <openstack> Useful Commands: #action #agreed #help #info #idea #link #topic #startvote.
19:00:11 <openstack> The meeting name has been set to 'office_hours'
19:00:54 <morgan> o/
19:01:00 <lbragstad> o/
19:01:13 <morgan> ok everyone, office hours! turn on the music, order pizza, party time :P
19:13:52 <openstackgerrit> Matthew Edmonds proposed openstack/keystone master: fix assert_admin  https://review.openstack.org/482359
19:14:36 <openstackgerrit> Eric Fried proposed openstack/keystoneauth master: Make Discover.version_data accept null max_version  https://review.openstack.org/482250
19:15:30 <lbragstad> morgan: i agree!
19:18:13 <eandersson> lbragstad, https://bugs.launchpad.net/keystone/+bug/1703666
19:18:15 <openstack> Launchpad bug 1703666 in OpenStack Identity (keystone) "Templated catalog does not handle multi-regions properly" [Undecided,New]
19:18:31 <eandersson> I failed a bit at the markup :D
19:19:29 <eandersson> Wish you could preview before you posted a bug
19:19:53 <morgan> go ahead and edit it :)
19:19:59 <lbragstad> eandersson: you should be able to edit the description
19:20:10 <eandersson> Oh lol
19:21:07 <eandersson> Is there markup for code?
19:21:27 <lbragstad> eandersson: unfortunately no
19:25:32 <gagehugo> o/
19:25:54 <gagehugo> I was looking at https://bugs.launchpad.net/keystone/+bug/1702211 yesterday, it's an odd bug
19:25:55 <openstack> Launchpad bug 1702211 in OpenStack Identity (keystone) "test_password_history_not_enforced_in_admin_reset failed in tempest test" [Undecided,Confirmed]
19:27:55 <openstackgerrit> Merged openstack/keystoneauth master: Fix _run_discovery caching  https://review.openstack.org/481754
19:29:04 <cmurphy> catching up on meeting logs
19:29:12 <openstackgerrit> Erik Olof Gunnar Andersson proposed openstack/keystone master: Fixing multi-region support in templated v3 catalog  https://review.openstack.org/482364
19:29:54 <cmurphy> samueldmq: lbragstad the sample dsta script was useful for having data to work with without having to set up a devstac
19:30:14 <cmurphy> but i don't feel strongly about getting rid of it
19:30:24 <cmurphy> i always devstack now
19:31:07 <lbragstad> gagehugo: interesting - was that a timing issue?
19:31:27 <lbragstad> cmurphy: yeah - i'm in the same boat for the most part :-/
19:32:05 <gagehugo> lbragstad, no idea. either that or maybe some weird race condition
19:32:48 <gagehugo> from the frequency log that mriedem posted it looks like it started failing on 07/01
19:34:11 <openstackgerrit> Lance Bragstad proposed openstack/keystone master: WIP: Implement global role assignments  https://review.openstack.org/481781
19:47:05 <openstackgerrit> Matthew Edmonds proposed openstack/keystone master: don't validate trust in policy  https://review.openstack.org/482190
19:51:35 <lbragstad> here's a patch that closes a bug https://review.openstack.org/#/c/470425/16
19:54:21 * cmurphy is home and ready to officehours
19:54:34 <lbragstad> \o/
19:54:40 <cmurphy> lbragstad: did you see my comments on that one?
19:54:45 <cmurphy> it doesn't fix the bug
19:54:56 <lbragstad> cmurphy: https://review.openstack.org/#/c/470425/16 ?
19:55:08 <cmurphy> lbragstad: ya
19:55:13 <lbragstad> cmurphy: checking
19:56:38 <bknudson> why is it only the token header gets trimmed? seems like all headers should get the same treatment
19:57:04 <bknudson> also, you'd expect the web server would handle fixing up the request.
19:58:03 <cmurphy> i have no idea
19:59:05 <cmurphy> i could be totally wrong, maybe edmondsw_ could test it to see if it actually solves his problem
20:00:07 <edmondsw_> cmurphy I'll try to do that
20:01:25 <edmondsw_> I've got 4 policy-related bug fixes out for review if anyone wants me to give pointers to them
20:03:09 <lbragstad> bknudson: yeah - that's a good point
20:03:26 <lbragstad> edmondsw_: i just started reviewing https://review.openstack.org/#/c/482142/
20:05:16 <edmondsw_> lbragstad tx. The really bad one is https://review.openstack.org/482359
20:11:32 <openstackgerrit> Matthew Edmonds proposed openstack/keystone master: remove default rule  https://review.openstack.org/482164
20:11:37 <morgan> lbragstad: i'm going to propose a deprecation of the template catalog and (hopefully) a YAML-based one to replace that is actually tested...
20:11:55 <morgan> lbragstad: that is based upon convos yesterday
20:12:29 <lbragstad> morgan: it sounds like eandersson is using the templated catalog
20:13:48 <morgan> right, hence the yaml replacement
20:14:12 <lbragstad> morgan: so what exactly would we be deprecating?
20:14:19 <morgan> the current templated one
20:14:28 <morgan> there would be a new one, that is named something else
20:14:28 <lbragstad> what format is that in?
20:14:45 <lbragstad> er - does it have a format?
20:14:46 <morgan> the current one is basically write out a json doc and we replace some stuff in it
20:14:52 <morgan> it's not formatted really.
20:14:59 <lbragstad> ah
20:15:04 <morgan> it is terrible
20:15:16 <lbragstad> that makes sense
20:16:06 <bknudson> I think we still need to support replacement since swift puts the project in the URL?
20:16:16 <morgan> that is the plan
20:16:20 <lbragstad> morgan: since eandersson is currently relying on it - i want to make sure that coordination happens
20:16:33 <morgan> just going to make the input enforced.
20:17:04 <morgan> expect regions, etc all in a yaml format that would be similar to the current in-db model
20:20:40 <lbragstad> morgan: does eandersson on board with the deprecation of the existing templated catalog?
20:20:48 <morgan> dunno
20:20:51 <lbragstad> s/does/is/
20:20:54 <morgan> he was part of the convo esterday
20:21:17 <morgan> it will be much better with something that is actually using the same mechanisms as the DB to render
20:21:34 <morgan> the current templated catalog is ... frightening.
20:22:19 <lbragstad> there is also https://github.com/openstack/keystone/blob/0731dab01a5d2da9650b67ebe8b91e825795c0ba/keystone/catalog/backends/templated.py#L244-L297
20:22:34 <morgan> those will mostly be the same
20:22:44 <morgan> this is a FS based catalog
20:22:53 <morgan> no writes allowed, CMS is there to do that job
20:23:29 <lbragstad> that makes sense
20:23:56 <morgan> i mean... we *could* support writes... but lets not do that silly thing
20:29:56 <eandersson> yes - for sure we should deprecate it
20:30:06 <eandersson> and if needed replace it with a better alternative
20:30:41 <eandersson> lbragstad, can't we just remove those overrides, as they are already implemented in the base class?
20:31:05 <eandersson> e.g. https://github.com/openstack/keystone/blob/0731dab01a5d2da9650b67ebe8b91e825795c0ba/keystone/catalog/backends/base.py#L378
20:32:04 <lbragstad> eandersson: i don't see a problem with that
20:32:18 <lbragstad> we take that approach elsewhere in keystone
20:32:30 <morgan> you can't remove with them being abstract
20:32:40 <morgan> you must redefine abstract methods on the subclass
20:32:41 <edmondsw> lbragstad cmurphy was right, https://review.openstack.org/#/c/470425/16 doesn't fix the bug
20:32:56 <morgan> edmondsw: that code looked suspect
20:33:00 <openstackgerrit> Eric Fried proposed openstack/keystoneauth master: Update docs and add a release note  https://review.openstack.org/477566
20:33:12 <eandersson> lbragstad, Yea I agree, but not all of them are implemented at the moment
20:33:32 <eandersson> (sorry defined, obviously not implemented)
20:33:35 <morgan> basically... don't send the /r /n etc in headers... you will be sad
20:33:51 <cmurphy> ya...found a few sources saying just don't do that
20:33:56 <morgan> cmurphy: ++
20:33:58 <edmondsw> morgan yeah, but when a customer does that...
20:34:11 <morgan> edmondsw: you point them at the docs in curl saying "yeah don't"
20:34:18 <lbragstad> edmondsw: removed my +1 accordingly
20:34:20 <edmondsw> morgan it was odd that neutron worked fine and keystone didn't
20:34:28 <morgan> edmondsw: apache makes a difference
20:34:41 <gagehugo> is it a bug then?
20:34:44 <morgan> unlikely
20:35:34 <edmondsw> morgan :) sure but it was a really hard to pin down issue... wasn't obvious they had the /r in the header unless you're an awk god
20:35:42 <morgan> heh
20:37:00 <edmondsw> morgan I don't think it's an apache thing, actually... the neutron curl command was targeted at an apache reverse proxy
20:37:10 <morgan> ah
20:37:22 <lbragstad> eandersson: are you using the templated catalog backend?
20:37:30 <lbragstad> s/are/aren't/
20:37:44 <cmurphy> edmondsw: morgan my test reproduced it with just uwsgi
20:38:04 <morgan> ah
20:38:06 <morgan> *shrug*
20:38:21 <edmondsw> cmurphy yeah, I'm not sure why the author of this test isn't just reproducing the problem and then testing their fix against it until it actually fixes it
20:38:27 <edmondsw> s/test/fix/
20:38:36 <eandersson> I am yes
20:38:51 <cmurphy> edmondsw: yeah :/
20:39:16 <lbragstad> eandersson: i'm just wondering why you wouldn't be opposed to deprecating and removing it then
20:39:50 <lbragstad> unless i'm missing something obvious
20:40:00 <eandersson> I'll rather move over to sql
20:40:10 <eandersson> and we are not going to pike anytime soon
20:40:32 <eandersson> So we would have to stick with the semi-functional templated implementation for too long
20:41:09 <gagehugo> morgan cmurphy edmondsw: I'll ping kaerie about it
20:41:22 <edmondsw> gagehugo tx
20:41:42 <eandersson> Moving to a yaml based variant would be an alternative, but we couldn't really go there unless we backported that to Mitaka, or maybe Newton/Ocata.
20:42:15 <gagehugo> I can mess around with it too after I take a look at this random failing tempest test
20:42:40 <morgan> eandersson: right, but you might be able to backport the code yourself.
20:42:42 <morgan> for your install
20:42:45 <morgan> until you move to pike
20:43:00 <eandersson> Yep - that is for sure an alternative
20:46:41 <eandersson> It just makes more sense for us to go move to the production ready alternative.
20:47:12 <lbragstad> eandersson: morgan so - let's recap the options for the templated catalog backend
20:47:33 <lbragstad> 1.) formally deprecate it for removal
20:47:58 <lbragstad> 2.) support it in a well known format (like yaml)
20:49:18 <morgan> yep
20:49:34 <lbragstad> is that it?
20:50:40 <lbragstad> i guess there would be a 3rd option
20:51:25 <lbragstad> 3.) perform option 1, deprecating all existing templated catalog stuff and start fresh with option 2 introducing a new backend for YAML officially
20:51:39 <lbragstad> so - maybe option 3 is actually option 2 just spelled out
20:52:06 <eandersson> Nr 3 is what I would recommend as well
20:57:44 <morgan> option 3 was what i was planning
20:57:53 <lbragstad> i certainly wouldn't be opposed to #3 if someone is willing to do the work
20:58:00 <morgan> it isn't a ton of work
20:58:13 <morgan> i have to grab my laptop, ... anyway
20:59:39 <lbragstad> it sounds like we're in agreement that deprecation of the existing templated catalog is in order
21:04:33 <eandersson> having fun writing tests for the multi-region patch
21:05:51 <eandersson> not sure how to handle attributes
21:06:18 <eandersson> > v3_catalog[service_type][attr] = value
21:07:36 <eandersson> guessing the endpoint should have the id?
21:08:10 <lbragstad> it should be consistent with what the sql implementation does with the exception of write operations
21:08:57 <lbragstad> edmondsw: i'm missing the bit here at line 202 - https://review.openstack.org/#/c/482359/2/keystone/common/authorization.py
21:09:13 <lbragstad> edmondsw: the patch ends up doing the same thing as before, right?
21:09:39 <lbragstad> the action ends up being `identity:<operation>` right?
21:10:11 <edmondsw> lbragstad if you call check_protection, yes... but if you call assert_admin, no
21:10:49 <edmondsw> lbragstad the places that use check_protection expect that to be added... the places that call assert_admin don't
21:11:14 <lbragstad> oh - line 130
21:11:28 <edmondsw> yep, 129
21:11:43 <edmondsw> oh, right, 130 on the new file
21:15:17 <lbragstad> edmondsw: how'd you stumble across this?
21:15:23 <lbragstad> edmondsw: testing a custom policy?
21:16:17 <edmondsw> lbragstad digging into why tests failed for https://review.openstack.org/#/c/482164/
21:17:03 <edmondsw> which I had proposed after digging into and proposing https://review.openstack.org/482142
21:17:53 <edmondsw> which was a result of reviewing our customized policy changes for pike and noticing that didn't look right
21:18:09 <edmondsw> https://review.openstack.org/482190 also came out of that as well
21:18:15 <edmondsw> lbragstad so it was quite a chain of events
21:18:55 <edmondsw> I'm quite happy to be fixing a 4 year old defect with that last one...
21:19:42 <lbragstad> so - correct me if i'm wrong
21:20:05 <lbragstad> but https://review.openstack.org/#/c/482359/2 will be tested by default once https://review.openstack.org/#/c/482164/2 merges?
21:22:13 <edmondsw> lbragstad yes... once there's no default rule, that can't hide issues like this
21:22:42 <lbragstad> because identity:admin_required didn't exist and was getting caught by the default - which ended up having the same result
21:23:08 <edmondsw> lbragstad exactly
21:23:14 <lbragstad> hmmm tricky
21:23:16 <edmondsw> yep
21:23:28 <edmondsw> I think we may also need to merge https://review.openstack.org/482142 before the default rule is removed
21:23:41 <edmondsw> s/may //
21:24:05 <edmondsw> but I can't base https://review.openstack.org/482164 on multiple changes
21:24:06 <lbragstad> edmondsw: yeah - that one looks good
21:24:09 <eandersson> So there is a second bug with templated - the endpoint id is used as the id for the service
21:24:29 <eandersson> which is an expected bug
21:24:38 <lbragstad> edmondsw: since a policy is changing, a release note would make sense i think, would you agree?
21:24:49 <edmondsw> lbragstad I'm adding a rel note
21:25:08 <edmondsw> lbragstad do you think it should be a security or fixes note?
21:25:40 <lbragstad> well - the default is still in place and that's true for all previous releases, right?
21:26:43 <openstackgerrit> Monty Taylor proposed openstack/keystoneauth master: Add paragraph clarifying major and micro versions  https://review.openstack.org/482710
21:26:46 <lbragstad> edmondsw: can you walk me through the case where it *should* be considered a security issue?
21:27:26 <edmondsw> lbragston see breton's comments in the bug
21:27:36 <edmondsw> lbragstad ^
21:27:47 <edmondsw> not sure what my fingers were doing there :)
21:28:54 <lbragstad> oh - so it would be considered a security issue if the deployer relaxed the default
21:29:25 <lbragstad> but didn't realize they were also relaxing get_identity_providers
21:29:30 <lbragstad> hmm
21:34:32 <edmondsw> lbragstad yeah... I'm inclined to put it in the security section of the rel notes, but not backport anything
21:35:01 <edmondsw> lbragstad I should probably also add a rel note to the change removing the default rule
21:35:02 <lbragstad> so - policy in code has only been effective for pike
21:35:32 <edmondsw> lbragstad yes, but the typo for identity:get_identity_providers goes back to the default policy.json file we shipped in past releases
21:35:33 <lbragstad> so - if someone wanted to "backport" the fix it would consist of correcting their policy operation
21:36:09 <morgan> lbragstad: deprecation patch about to be posted (catalog)
21:36:14 <edmondsw> lbragstad right, the backport would be to update the default policy.json file and/or to change the code to look for the typo version... neither of which I like
21:36:19 <morgan> i should have a yaml-loading catalog in a couple hours as well.
21:36:35 <lbragstad> morgan: awesome - thank you
21:36:38 <lbragstad> cc eandersson ^
21:36:40 <edmondsw> taht's really an or, not an and/or
21:36:58 <lbragstad> edmondsw: so the options are
21:37:07 <lbragstad> 1.) backport the default that corrects the type
21:37:10 <lbragstad> typo*
21:37:23 <lbragstad> 2.) correct the code to look for get_identity_providers (which breaks conventions)
21:37:33 <lbragstad> i agree in that option 2 seems like the wrong approach
21:37:42 <lbragstad> but what's wrong with option 1?
21:38:01 <eandersson> You'll probably have that done before I get working unit tests working for this lol
21:38:16 <edmondsw> lbragstad I don't know that #1 really helps anyone
21:38:37 <edmondsw> lbragstad if you've already customized policy, you're not going to take a new default policy.json file and apply it
21:38:39 <lbragstad> is there a negative side-effect outside of that?
21:38:48 <edmondsw> if you're not customizing policy, then things are already working ok
21:39:05 <lbragstad> what about consuming a new release note proposed to stable/ocata and stable/newton?
21:39:07 <edmondsw> lbragstad probably not...
21:40:04 <edmondsw> lbragstad we could certainly propose patches to the stable releases that a) updates the policy.json to fix the typo and b) adds a rel note warning about the issue
21:40:26 <lbragstad> edmondsw: that would at least flag things to deployers that read release notes
21:40:30 <openstackgerrit> Morgan Fainberg proposed openstack/keystone master: Deprecate the templated catalog  https://review.openstack.org/482714
21:40:40 <edmondsw> lbragstad that read release notes long after the release :)
21:40:43 <lbragstad> how they apply that change is obviously up to them - but at least they'd know
21:40:43 <morgan> lbragstad: ^ very simple patch.
21:41:09 <lbragstad> edmondsw: true - it's more or less only for following procedure ;)
21:41:14 <openstackgerrit> Matthew Edmonds proposed openstack/keystone master: fix identity:get_identity_providers typo  https://review.openstack.org/482142
21:41:19 <edmondsw> lbragstad I'm fine with it
21:41:29 <lbragstad> edmondsw: ok - i'll update the bug report then
21:41:30 <edmondsw> lbragstad ^ that adds a rel note
21:46:00 <eandersson> Writing tests for this is going to require a lot more fixes than what I did :p
21:47:00 <eandersson> since this is being back-ported from the v2 catalog, IDs are not at all implemented properly
21:48:35 <openstackgerrit> Matthew Edmonds proposed openstack/keystone master: remove default rule  https://review.openstack.org/482164
21:49:55 <lbragstad> edmondsw: looks good - one minor nit inline
21:50:08 <lbragstad> s/inline/in the release note/
21:51:00 <eandersson> do we even care about ids for this? they are not in the example config https://github.com/openstack/keystone/blob/master/etc/default_catalog.templates
21:51:49 <lbragstad> eandersson: i would imagine that would get rewritten to yaml - based on morgan's work
21:52:07 <edmondsw> lbragstad fixed
21:52:09 <eandersson> sure - but do we want to backport the yaml work as well?
21:52:14 <openstackgerrit> Matthew Edmonds proposed openstack/keystone master: fix identity:get_identity_providers typo  https://review.openstack.org/482142
21:52:24 <eandersson> or do we want to fix the current implementation pre-pike?
21:52:25 <openstackgerrit> Merged openstack/keystoneauth master: Make Discover.version_data accept null max_version  https://review.openstack.org/482250
21:57:38 <lbragstad> eandersson: we won't be able to fix stable/ocata without landing something in pike first
21:57:56 <lbragstad> but i doubt we'd backport fixes for the templated catalog to ocata anyway
21:57:57 <eandersson> Yep - so do we want to just skip my patch and go straight to yaml?
21:58:15 <lbragstad> eandersson: yeah - that would seem reasonable
21:58:19 <eandersson> It's just weird to have a feature so broken :D
21:58:42 <lbragstad> eandersson: yeah - the templated stuff is a mess
21:58:48 <eandersson> Could we maybe update our documentation for newton/ocata?
21:59:07 <lbragstad> eandersson: saying what exactly?
21:59:21 <eandersson> Don't try to use with multiple regions!
21:59:31 <openstackgerrit> Gage Hugo proposed openstack/keystone master: WIP - Trims whitespace from request headers  https://review.openstack.org/470425
21:59:35 <eandersson> or not intended for production :D
21:59:51 <eandersson> basically just slap a warning lable on it
22:03:05 <openstackgerrit> Eric Fried proposed openstack/keystoneauth master: Update docs and add a release note  https://review.openstack.org/477566
22:03:14 <openstackgerrit> Merged openstack/keystoneauth master: Expand some discover.py docstrings  https://review.openstack.org/482207
22:04:32 <lbragstad> eandersson: we document a few of the warts with it already https://docs.openstack.org/keystone/latest/configuration.html#service-catalog
22:04:40 <lbragstad> #endmeeting