| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-03-22 | |||
| 18:04:01 | mriedem | the 'can i unlock' is a policy check | |
| 18:04:01 | dansmith | mriedem: sure, but same for exposing admin|user .. you have to now what the policy is to know whether it matters | |
| 18:04:31 | elbragstad | well - to calculate that boolean you'd just need to run the context through that policy check - then populate that accordingly in the data model (and response?) | |
| 18:04:43 | dansmith | right | |
| 18:04:54 | mriedem | not the data model, but the response yes | |
| 18:04:55 | dansmith | but if you don't use the boolean, and expose "locked_by: admin" | |
| 18:05:07 | dansmith | the user has to know whether policy considers them an admin or not right? | |
| 18:05:16 | dansmith | whereas "no you can't unlock this" is pretty clear | |
| 18:05:20 | openstackgerrit | Merged openstack/nova stable/queens: libvirt: Add workaround to cleanup instance dir when using rbd https://review.openstack.org/628726 | |
| 18:05:27 | elbragstad | ++ | |
| 18:05:29 | openstackgerrit | Merged openstack/nova stable/queens: Avoid BadRequest error log on volume attachment https://review.openstack.org/640116 | |
| 18:05:55 | dansmith | the fact that it could change between check and attempt is pretty much the case for everything :) | |
| 18:05:56 | elbragstad | "admin" is kinda confusing, what is locked by a project admin, a domain admin or a system admin? | |
| 18:06:11 | elbragstad | s/what/was/ | |
| 18:06:17 | mriedem | in this case project admin == 'owner' | |
| 18:06:24 | mriedem | same as if the owner of the instance locked it themselves | |
| 18:06:28 | mriedem | it's the system admin case that is wonky | |
| 18:07:00 | elbragstad | i'm missing the part why the system-admin case is wonky | |
| 18:07:26 | mriedem | i mean the case where the locked_by is 'admin' today is if someone outside the project that owns the instance locks the instance | |
| 18:07:38 | mriedem | project admin locking the instance treats locked_by as 'owner' | |
| 18:08:18 | elbragstad | ... interesting | |
| 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 | |