| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-03-22 | |||
| 18:08:26 | mriedem | idk how i feel about returning an at-the-time-of-request policy check boolean saying if you can or cannot unlock it with a subsequent unlock call, i don't think we have anything else like that in the api | |
| 18:08:27 | elbragstad | someone outside the project? | |
| 18:08:53 | dansmith | I totally don't understand the concern over the policy thing | |
| 18:09:01 | dansmith | we return data (or not) based on policy | |
| 18:09:32 | mriedem | for faults and event traceback yeah | |
| 18:09:33 | mriedem | true | |
| 18:09:47 | dansmith | and we used to for things like flavor extra specs, iirc | |
| 18:09:47 | mriedem | and the numa blob that is being proposed | |
| 18:09:58 | dansmith | and hostnames if you're admin | |
| 18:10:12 | dansmith | or fake host ids if you're not | |
| 18:10:16 | mriedem | it's slightly different though, those are just exposing things or not | |
| 18:10:45 | dansmith | if it really matters, then unlockable:true or omit it entirely, but I don't think that's better :) | |
| 18:10:47 | mriedem | i'd be much more comfortable with this locked_reason spec if we didn't have to deal with gd locked_by in the first place | |
| 18:11:58 | mriedem | locked_by is just internal meta and i'm not sure why we need to expose it, and it's only related to a policy check on unlock | |
| 18:12:12 | mriedem | if you can't unlock you find out with a 403 as normal | |
| 18:12:13 | dansmith | well, we can do that too | |
| 18:12:19 | dansmith | but I don't understand the concern | |
| 18:12:30 | mriedem | i wouldn't block on it | |
| 18:12:59 | mriedem | just seems like we're maybe trying to solve a problem we're making for ourselves | |
| 18:13:04 | mriedem | which we maybe don't need to care about | |
| 18:13:20 | dansmith | I tell you who's making problems.. mriedem :) | |
| 18:13:33 | mriedem | i've said a million times that guy is an insufferable asshole | |
| 18:13:41 | dansmith | heh | |
| 18:14:30 | elbragstad | dumb question: does exposing the attribute actually have an impact on someone being able to unlock or lock an instance? | |
| 18:14:55 | mriedem | it's an indication if they can unlock it | |
| 18:15:10 | gmann | i agree that 'can i unlock' is policy control things. exposing locked_by I consider the benefit of as user i know my instances is locked by someone else like admin (system-admin or project-admin) and i can ask them to unlock | |
| 18:15:21 | dansmith | I imagine the impetus for this whole thing is, someone tries to do something, can't because locked, so they don't understand why and have to call support to figure out why.. if there was a "locked by an admin, reason: because you didn't pay" then .. they know | |
| 18:15:23 | elbragstad | but if that attribute wasn't exposed to them, could they still build the same request and send it to nova and have it behave the same way? | |
| 18:15:35 | dansmith | elbragstad: yes | |
| 18:15:49 | dansmith | elbragstad: but they may not understand why it fails if they don't see it was locked by someone else | |
| 18:15:50 | mriedem | right i see the value in the reason field for sure | |
| 18:16:04 | gmann | then we can leave locked_by to expose | |
| 18:16:23 | openstackgerrit | Merged openstack/nova stable/queens: Allow utime call to fail on qcow2 image base file https://review.openstack.org/633493 | |
| 18:16:39 | openstackgerrit | Merged openstack/nova master: Update master for stable/stein https://review.openstack.org/645463 | |
| 18:16:41 | mriedem | elbragstad: this is the check code on unlock https://github.com/openstack/nova/blob/master/nova/api/openstack/compute/lock_server.py#L47 | |
| 18:16:47 | openstackgerrit | Merged openstack/nova stable/stein: Update .gitreview for stable/stein https://review.openstack.org/645461 | |
| 18:17:01 | mriedem | https://github.com/openstack/nova/blob/master/nova/compute/api.py#L4025 | |
| 18:17:07 | dansmith | gmann: you're saying you think exposing "can I unlock" is bad right? | |
| 18:17:28 | dansmith | what if we just expose locked_by_me:True|False or locked_by=user|other | |
| 18:17:29 | gmann | yeah. that is all control by policy | |
| 18:17:40 | dansmith | okay I don't understand that at all, but whatever :) | |
| 18:17:44 | gmann | if locked by other then policy should say 403 | |
| 18:18:14 | dansmith | gmann: right, but you're going to make them try to determine that instead of just doing the check ourselves? we can ask policy about it, just like we do for the conditional exposure of fault | |
| 18:18:25 | dansmith | try the put I mean | |
| 18:18:51 | elbragstad | if the policy is written like "(role:admin and system_scope:all) or rule:owner" and locked_by_me is True|False, is locked_by useful? | |
| 18:18:58 | mriedem | if i'm following this unlock logic, if an admin in my project locks my server, i can unlock it. but if an admin outside my project (global god like admin) locks my server, then i can only unlock it if i pass the override policy check right? | |
| 18:19:14 | elbragstad | oh - maybe i was missing ^ that bit | |
| 18:19:37 | gmann | mriedem: right | |
| 18:19:44 | mriedem | and the override policy defaults to rule:is_admin i.e. system level god admin | |
| 18:19:47 | mriedem | in legacy terms | |
| 18:20:57 | gmann | elbragstad: we compare only project_id (not user id) for deciding it is owner or not. | |
| 18:21:00 | mriedem | so with dan's proposal, on response, we checking if you pass is_expected_locked_by and if not, do you pass the override policy, exactly like the unlock code, | |
| 18:21:16 | mriedem | are we going to want to perform that policy check when listing servers with details? ^ for each server? | |
| 18:21:55 | elbragstad | mmm - so owner == anyone with a token scoped to the project that contains the instance | |
| 18:21:57 | gmann | humm, no we should not perform that check in show. | |
| 18:22:01 | mriedem | elbragstad: yes | |
| 18:22:04 | elbragstad | and admin == anyone with the `admin` role on any project | |
| 18:22:05 | gmann | yeah | |
| 18:22:26 | openstackgerrit | Merged openstack/nova-specs master: Update Network Bandwidth resource provider spec https://review.openstack.org/644810 | |
| 18:22:38 | mriedem | gmann: "no we should not perform that check in show." is what we would have to do to return an "can i unlock this server" boolean | |
| 18:22:54 | dansmith | and it's what we do when we expose (or not) fault in instance | |
| 18:23:08 | mriedem | i don't agree on that exactly being the same | |
| 18:23:16 | dansmith | is there some api guideline that says "only reveal if someone can do something when they try" ? | |
| 18:23:19 | mriedem | with fault traceback it's just a show/hide | |
| 18:23:21 | gmann | yeah, seems like. | |
| 18:23:56 | mriedem | maybe we do a policy check on every instance when listing, for faults i mean, would have to look | |
| 18:23:57 | dansmith | you guys keep saying "we shouldn't do that" but I haven't seen any *reason* :) | |
| 18:23:58 | mriedem | anywho | |
| 18:25:21 | mriedem | well i guess we don't do a context.can check when listing servers to see if we can show the fault traceback https://github.com/openstack/nova/blob/master/nova/api/openstack/compute/views/servers.py#L550 but it's close, just based on the hard-coded is_admin flag in the context | |
| 18:25:24 | mriedem | which elbragstad wants to remove | |
| 18:25:24 | dansmith | also, locked_by_me=True|False is something we can do today, something we can do in the future if we store the locking user, and something that doesn't require a policy check on show.. why is that not okay? | |
| 18:26:22 | mriedem | dansmith: you mean locked_by_owner yeah? | |
| 18:26:29 | dansmith | sure | |
| 18:26:33 | mriedem | where the logic would just be locked_by_owner = instance.locked_by == 'owner' | |
| 18:26:39 | dansmith | or locked_by=Owner|NotOwner | |
| 18:26:45 | dansmith | right | |
| 18:26:57 | dansmith | don't say admin, just say "not you" | |
| 18:27:03 | elbragstad | ++ | |
| 18:27:21 | mriedem | i'd be more ok with doing locked_by=owner|other | |
| 18:27:42 | mriedem | i'm trying to express this... | |
| 18:28:06 | dansmith | *simpsons | |
| 18:28:27 | mriedem | but doing a policy check when building a response based on policy for something that would be checked during a subsequent unlock request, seems very weird and capability-ish to me, which we don't do elsewhere. it's true we check policy when building a response to hide things for non-admins, like system details (hosts, fault traceback, etc), but those aren't capability things | |
| 18:28:30 | gmann | I am also ok for locked_by=owner|other if we cannot expose the UUID. | |
| 18:28:44 | mriedem | gmann: we don't store the uuid anywhere for who locked the instance | |
| 18:28:45 | elbragstad | if instances stored user information instead of project ids, we wouldn't need two lock properties, would we? | |
| 18:29:01 | gmann | mriedem: yeah, i mean if we do not want to change that as it will be complex | |
| 18:29:04 | dansmith | mriedem: so the reason is we don't do exactly this elsewhere and it "feels wrong" ? | |
| 18:29:22 | mriedem | correct | |
| 18:29:32 | dansmith | I mean, I understand that things feel wrong.. I don't understand why this one does, but that's okay | |
| 18:29:34 | dansmith | alright | |
| 18:29:36 | mriedem | i could argue it'd be nice to have a "can_be_resized" flag in the server response | |
| 18:29:41 | mriedem | that doesn't mean we're going to add that | |
| 18:29:55 | dansmith | people have asked for that :) | |
| 18:29:59 | mriedem | i'm sure they have | |
| 18:30:00 | dansmith | based on all the reasons you *can't* do that | |
| 18:30:11 | mriedem | and we've talked about that capabilities API for several years | |
| 18:30:18 | dansmith | how's that coming? :) | |
| 18:30:31 | mriedem | it's mostly rotted flesh in my storage room | |
| 18:30:32 | dansmith | because to me, | |
| 18:30:34 | mriedem | don't tell the cops please | |