Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-11-26
16:55:07 tssurya https://github.com/openstack/nova/blob/fd67f69cfdaf04620f2e8a5f1fbf5737096965d8/nova/api/openstack/identity.py#L62
16:55:39 mriedem yeah, so my point is, it sounds like this is a config issue in your deployment
16:55:54 mriedem i.e. why can't you grant access to the service user that is doing this check on behalf of nova?
16:56:51 tssurya the service user already has an admin role, I think this happens because the project is not admin project ?
16:57:19 mriedem idk
16:57:31 mriedem isn't that also configurable in keystone?
16:57:59 gmann mriedem: but point is users do not know it was permission issue for them or they are adding wrong project
16:57:59 mriedem i.e. the policy to GET a project by id - it's not hard-coded to admin project only is it?
16:58:20 tssurya probably, I'll see if it can be tweaked on the config side for our particular scenario,
16:58:49 tssurya but in general its strange nova allows entries from users that don't have permission
16:58:53 mriedem gmann: if the service user nova is configured with has permissions to ask the question "does this project exist" that's all this utility code is used for
16:59:07 gmann policy can be changed to make it work properly and with default policy it should work at long as keystone system scope is enabled
16:59:36 mriedem and again, this was added as best effort to close a leak where the flavor access and quotas apis would just blindly take whatever project id and assume it existed and shove it into the nova db
16:59:50 gmann mriedem: from API usage they do not know nova does this check internally ?
17:00:41 tssurya mriedem: yea if its best effort, I'll probably just tweak it downstream then and write it in our documentation
17:00:56 mriedem i don't understand the question. if configured properly and nova determines the project doesn't exist (404 from keystone) the compute API users will also get a 404
17:00:58 gmann mriedem: yeah agree on best effort but if API return the permissions issue to user and then they ask provider to modify the permission or ask other right user to add the tenant in flavor access
17:01:38 mriedem i see 3 options:
17:02:02 mriedem 1. close the bug as invalid - tweak your config (we could add docs, idk if that would be found or help) to do what you need to avoid the 403 from keystone
17:02:13 gmann mriedem: no. if permissions issue then keystone return 403 (which means project may exist or not) and nova proceed the request with 200. so user does not know if that is correct or not.
17:02:26 mriedem 2. change the 403 case as an error and raise it back to the compute api caller - maybe enough time has passed to not worry about backward compat with the old non-validating behavior
17:03:11 mriedem 3. add a config option to toggle what the behavior of the api should be in the 403 case, default to backward compat, but allow making it raise for those that want that, but that means you need to know to twiddle that config which if you know that, you likely know you can just fix the permissions for your service user
17:03:17 tssurya I vote for 2
17:03:29 mriedem of course you do :)
17:03:36 tssurya :D
17:05:22 mriedem one thing to point out for #2 is that the quota set and flavor access add/remove style write APIs are admin-only by default, so if we do change the behavior of that 403 it would only be on admin apis, not end-user apis
17:06:03 gmann 3rd is not good option. config driven API not good.
17:06:08 tssurya ack
17:06:12 gmann even admin only API it still API change in option 2
17:06:46 mriedem option 1 is the only non-api change
17:06:54 mriedem we're not going to microversion this imo
17:07:00 mriedem if that's what you're thinking
17:07:15 gmann with option 1 yes no microversion but option 2 need microverion
17:07:19 openstackgerrit Dan Smith proposed openstack/nova master: Add a way to exit early from a wait_for_instance_event() https://review.opendev.org/695985
17:07:25 mriedem not really
17:07:48 mriedem i don't necessarily think we need a microversion to prevent you from shooting yourself in the foot
17:07:57 mriedem we have a few cases where we've made the api error on cases that are wrong
17:08:08 tssurya dansmith: when you have time would you lightly review the idea in https://review.opendev.org/#/c/689422/ ? thanks in advance :)
17:08:18 mriedem and would just fail later, e.g. attaching sriov ports (not supported), rebuilding volume-backed instances with a new image (not supported)
17:08:56 gmann mriedem: yes but here we are changing the behavior. user does not permissions to GET project and project exist is right request and nvoa is 200 currently which will be 400.
17:09:40 mriedem gmann: the "user" in this case is an admin
17:09:46 gmann yes
17:10:10 dansmith tssurya: -1.9
17:10:11 mriedem i'm agreeing that re-raising the 403 from keystone would be a behavior change
17:10:17 tssurya I would prefer not to have a microversion to fix this, but I don't know if its "okay" to do it
17:10:24 mriedem but i'm not agreeing it requires a microversion
17:10:37 mriedem we didn't add the project id validation in a new microversion either, because it was stopping a leaky bug
17:10:54 tssurya dansmith: bad idea ?
17:11:02 mriedem iow, i shouldn't have to request a microversion to tell nova to properly do validation
17:11:05 dansmith tssurya: can't get past the typo in the commit message ;P
17:11:15 dansmith tssurya: I added two people that should look at it
17:11:19 tssurya dansmith: haha :P
17:11:23 mriedem she said "lightly review"
17:11:24 tssurya thanks :)
17:11:31 dansmith mriedem: true story
17:11:53 gmann mriedem: but we cannot say that this nova API is only for users having GET project permission ?
17:12:04 tssurya mriedem, dansmith: by "light" I mean the idea of removing them, those parts of code are grey areas for me
17:12:30 mriedem i usually ask dan to lightly review my stuff when i want him to just rubberstamp it :)
17:12:32 dansmith tssurya: I'm sure you're right that it needs to happen.. I'm not super strong in that area of the code as well
17:12:40 gmann i mean we cannot say in current code behavior or doc
17:13:10 tssurya dansmith: ack, I'll wait for the experts you tagged to respond
17:13:12 mriedem gmann: i'm lost in what you're asking and what you're proposing
17:13:16 tssurya mriedem:hehe
17:13:37 mriedem gmann: because earlier you said "(10:52:19 AM) gmann: tssurya: mriedem i think we should only proceed the request for 2xx case and rest all return 400 with error message of what we get from keystone"
17:13:38 gmann option 1 (telling via document for right permissions for this API) options 2 do the same via code. I like later one but that need microversion change IMO
17:13:54 gmann yeah but IMO it need microverison change
17:14:00 mriedem and i disagree
17:14:18 gmann on microverison only right ?
17:14:32 mriedem 1. request at 2.1 -> blast off my foot, ouch....reads docs.... 2. oh request at 2.97 - 403 (it works!)
17:15:26 mriedem i am a disagreeable troll on most everything but on this point yes just the microversion thing
17:17:11 sean-k-mooney it might be worth updating the "when do you need a micorverions bump" doc to note its not required if you are blocking knonw broken behavior using an error code that could previously be retruned form teh api
17:17:15 gmann it break the success cases from 200->400. because GET project permission is not actually required for this API only things is how nova check the project exist or not?
17:18:13 gmann sean-k-mooney: i think that is covered, broken behavior is ok to fix without microversion.
17:18:40 tssurya gmann: so you are fine to go ahead without microversion ?
17:18:53 mriedem maybe y'all duke it out in the mailing list and let others weigh in
17:19:00 gmann but this is little tricky where it is mix of success and broken case because keystone 403 does not mean project does not exist always
17:20:17 gmann tssurya: no, i am saying it need microversion if we change the keystone's 403 case to failure case.
17:20:37 tssurya I'll post it on ML and we can see if there are people who care about this
17:20:47 gmann or we request keystone with no auth way ? do not know how
17:20:53 gmann +1
17:34:08 openstackgerrit Merged openstack/nova master: compute: Take an instance.uuid lock when rebooting https://review.opendev.org/673463
18:14:42 openstackgerrit Lee Yarwood proposed openstack/nova stable/train: compute: Take an instance.uuid lock when rebooting https://review.opendev.org/696151
18:15:52 openstackgerrit Lee Yarwood proposed openstack/nova stable/stein: compute: Take an instance.uuid lock when rebooting https://review.opendev.org/696152
18:16:18 openstackgerrit Lee Yarwood proposed openstack/nova stable/rocky: compute: Take an instance.uuid lock when rebooting https://review.opendev.org/696153
18:16:47 openstackgerrit Lee Yarwood proposed openstack/nova stable/queens: compute: Take an instance.uuid lock when rebooting https://review.opendev.org/696154
18:35:51 efried dansmith: I'm confused by some of the new wording, which might just be me, but pretty sure the test has a mistake https://review.opendev.org/#/c/695985/
18:37:36 dansmith efried: ah, frack, that was supposed to get dedented
18:37:43 efried :)
18:37:54 efried dansmith: but please add the self.fail() as well
18:38:18 dansmith efried: you mean a self.fail after the early exit to make sure we don't get there right?
18:38:23 efried correct
18:38:26 dansmith sure
18:38:32 efried self.fail('this code should have been unreachable!')
18:39:12 dansmith self.fail('I could totally take eric in a a street fight')
18:39:20 dansmith we'll never run that code, so we'll never know
18:40:31 efried hahaha
18:41:24 openstackgerrit Dan Smith proposed openstack/nova master: Add a way to exit early from a wait_for_instance_event() https://review.opendev.org/695985
18:51:08 gmann johnthetubaguy: these test change is due to base class assert on error message. these tests will be changed only where granularity is added which should not be many or we can remove the error message checks . - https://review.opendev.org/#/c/648480/20/nova/tests/unit/policies/test_services.py@63
18:54:44 openstackgerrit Merged openstack/nova stable/train: Replace time.sleep(10) with service forced_down in tests https://review.opendev.org/696088
19:05:42 openstackgerrit Ghanshyam Mann proposed openstack/nova master: Add new default rules and mapping in policy base class https://review.opendev.org/645452

Earlier   Later