| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-03-10 | |||
| 14:53:51 | gibi | hm, that feels powerful | |
| 14:54:00 | stephenfin | but gmann's comment there says we should change that to system admin | |
| 14:54:03 | gibi | especially in a non tenant separated cloud | |
| 14:54:48 | gmann | stephenfin: no, i mean system reader get everything, and project admin only go with those filter_tenant_id filters | |
| 14:55:21 | stephenfin | isn't that what I've done? | |
| 14:55:22 | gmann | may be we have to check if context is scoped to system or project | |
| 14:55:50 | gmann | this one? https://review.opendev.org/c/openstack/nova/+/765798/5..6/nova/api/openstack/compute/hypervisors.py#197 | |
| 14:56:01 | gmann | and you will keep default policy as SYSTEM_READER? | |
| 14:57:02 | stephenfin | oh, are you talking about the context I use in my call to 'objects.Aggregate.get_by_metadata'? | |
| 14:58:02 | stephenfin | I'm changing the 'os_compute_api:os-hypervisors:list' policy to project admin or system reader | |
| 14:58:32 | stephenfin | but you'll note that check doesn't pass a context argument | |
| 14:58:45 | stephenfin | so it will never pass for a project admin, right? | |
| 14:59:03 | stephenfin | while it will pass for a system admin or system reader | |
| 14:59:08 | stephenfin | since they don't need context | |
| 14:59:28 | stephenfin | it's a bit of a hack, I'll admit, but it seemed logical to me | |
| 14:59:50 | gmann | stephenfin: it will pass with project admin too right with ' 'os_compute_api:os-hypervisors:list' policy to project admin or system reader' | |
| 15:00:02 | gmann | it will fail for project member/reader | |
| 15:00:51 | stephenfin | the check will fail (non-fatally) for project admins, which means we'll enter that block | |
| 15:01:29 | gmann | but with this it would fail right?> -https://review.opendev.org/c/openstack/nova/+/765798/6/nova/policies/hypervisors.py#37 | |
| 15:01:33 | gmann | *would not | |
| 15:02:21 | stephenfin | I'm confused. Let's start from the top | |
| 15:02:44 | stephenfin | The policy has been changed from "system reader" to "system reader or project admin" | |
| 15:02:49 | stephenfin | We check that here https://review.opendev.org/c/openstack/nova/+/765798/5..6/nova/api/openstack/compute/hypervisors.py#277 | |
| 15:03:17 | stephenfin | If e.g. a project reader tries to access this API, they'll get a HTTP 403 | |
| 15:03:47 | stephenfin | we then check the *same policy* again here https://review.opendev.org/c/openstack/nova/+/765798/5..6/nova/api/openstack/compute/hypervisors.py#197 | |
| 15:04:13 | stephenfin | only this time we do not provide "target={'project_id': context.project_id}" | |
| 15:04:15 | gmann | yeah 277 protect from project member/reader | |
| 15:04:33 | stephenfin | you must provide that to pass any project checks | |
| 15:04:40 | gmann | ahh, i did not notice the target | |
| 15:04:54 | gmann | got it, little confusing to read | |
| 15:04:54 | stephenfin | so that check will return False for project members but true for system admin/member/reader | |
| 15:05:02 | stephenfin | *project admins | |
| 15:05:08 | gmann | yeah | |
| 15:05:13 | stephenfin | yeah, like I said, it's a bit of hack | |
| 15:05:28 | stephenfin | that's what I was trying to call out with this comment | |
| 15:05:30 | stephenfin | # don't pass project information, forcing the check to be a system | |
| 15:05:32 | stephenfin | # admin check | |
| 15:05:48 | gmann | or simple we can check context.system_scope == 'all' or whatever exact value | |
| 15:05:57 | stephenfin | I should have said "don't provide project information as part of the target" | |
| 15:06:05 | stephenfin | that would be simpler :) | |
| 15:06:28 | stephenfin | what happens if we don't have scoping enabled though? | |
| 15:06:47 | stephenfin | would that result in admins not being able to list all hypervisors? | |
| 15:07:35 | gmann | https://github.com/openstack/oslo.context/blob/0d02866365bc8b779aef9ebd0b79a52c96ae40e5/oslo_context/context.py#L251 | |
| 15:08:23 | gmann | humm that is tricky, in old way we allowed to list all hypervisors | |
| 15:09:36 | gmann | so as long as we support deprecated policy it will pass the 277 and @197 if you can check "if CONF.oslo_policy.enforce_scope and context.system_scope == 'all'" then we can support old way too | |
| 15:09:57 | gmann | i mean only enable that check for CONF.oslo_policy.enforce_scope=true | |
| 15:10:14 | gmann | if false then we do old way to keep compatibility | |
| 15:10:32 | gmann | means return all hyperviros | |
| 15:11:20 | stephenfin | Yup, good idea | |
| 15:11:51 | stephenfin | okay, so back to the response if 'filter_tenant_id' is not set on any aggregate | |
| 15:12:20 | stephenfin | are you suggesting we return all hypervisors for project admins if 'filter_tenant_id=project_id' is not present on any aggregate? | |
| 15:12:38 | stephenfin | that would allow users to know all compute nodes in a given deployment | |
| 15:12:49 | stephenfin | which seems too much | |
| 15:14:12 | gmann | yeah, as filter_tenant_id is not there on aggregate scheduler filter aggregate_multitenancy_isolation allow any project to boot instance, | |
| 15:14:37 | gmann | https://github.com/openstack/nova/blob/0e7cd9d1a95a30455e3c91916ece590454235e0e/nova/scheduler/filters/aggregate_multitenancy_isolation.py#L54 | |
| 15:14:53 | gmann | i mean that is how host is kept open for all project | |
| 15:14:58 | gibi | I project admin in a non tenant isolated cloud should not be allowed to explicitly select any target host, I think | |
| 15:15:23 | stephenfin | there's a big difference between being able to run an instance on any host and being able to get information on all those hosts though | |
| 15:15:33 | gibi | gmann: the filters allows using hosts but for me the question is does the project admin needs to know which host is which? | |
| 15:15:42 | gibi | stephenfin: +1 | |
| 15:15:44 | stephenfin | we don't even expose the host that the instance is on for non-admins | |
| 15:16:47 | stephenfin | as in the 'OS-EXT-SRV-ATTR:hypervisor_hostname' attribute | |
| 15:17:01 | gmann | for this use case, only hstname is needed for create server request | |
| 15:17:30 | gmann | yeah for non-admin we will block list hypervisor or create instance on specific host | |
| 15:18:11 | gmann | gibi's has good point 'I project admin in a non tenant isolated cloud should not be allowed to explicitly select any target host, I think' | |
| 15:18:24 | stephenfin | I just checked. It fails on a DevStack deployment for me | |
| 15:18:30 | gmann | non tenant isolated cloud is anotehr thing to consider | |
| 15:18:35 | stephenfin | . devstack/openrc | |
| 15:18:42 | stephenfin | $ openstack --os-compute-api-version 2.latest server create --flavor m1.tiny --image cirros-0.5.1-x86_64-disk --network adec0800-859b-4aee-bf51-75eecb7aacf2 --hypervisor-hostname devstack-1 --wait test-server | |
| 15:18:48 | stephenfin | Policy doesn't allow compute:servers:create:requested_destination to be performed. (HTTP 403) (Request-ID: req-094bc901-ca54-4c9d-80dc-36f21b9d30ee) | |
| 15:19:55 | stephenfin | So unless an admin changes that policy, this information isn't very helpful | |
| 15:20:05 | stephenfin | even for the tenant isolated cloud | |
| 15:20:30 | stephenfin | Am I missing something? | |
| 15:20:32 | gmann | yeah this one https://github.com/openstack/nova/blob/master/nova/policies/servers.py#L189 | |
| 15:22:15 | stephenfin | Ah, wait, I am. 'source devstack/openrc' won't set things up for a project admin | |
| 15:22:50 | stephenfin | it'll use the demo user | |
| 15:23:14 | stephenfin | so I could wrangle things to use a project admin and it should pass since it's actually using this policy https://github.com/openstack/nova/blob/master/nova/policies/servers.py#L205 | |
| 15:23:30 | stephenfin | but the TODO there indicates that the use of PROJECT_ADMIN is a mistake that we need to fix | |
| 15:23:52 | stephenfin | and we shouldn't really be letting project admins select their host | |
| 15:24:02 | stephenfin | at least that's how I read that TODO | |
| 15:24:03 | gmann | yeah that is the use case end up with hypervors policy change | |
| 15:24:40 | gmann | first we discussed of letting system to create instance for project need vm on specific host as project admin cannot get host info | |
| 15:25:04 | gmann | but later we agreed to do policy change in hypervisor itself and let project admin to list host | |
| 15:26:08 | gmann | In current situation, with default policy, unless user get system and project scope both in their token they cannot boot instance on specific host. | |
| 15:26:29 | gmann | use system scope power to get host info and then project admin power to boot instance with host | |
| 15:27:14 | stephenfin | Okay, so this is an alternative to a new microversion for the 'POST /server' API? | |
| 15:27:31 | gmann | yes | |
| 15:27:46 | stephenfin | Instead of adding e.g. a 'project_id' field to the request body for that | |
| 15:27:52 | stephenfin | Gotcha | |
| 15:27:57 | gmann | exactly | |
| 15:28:48 | stephenfin | So my gut says for this to happen, we'd have to insist on tenant-isolation | |
| 15:29:10 | gmann | gibi: stephenfin but if we want to be more secure, I think returning no host if no matching 'filter_tenant_id' is also fine for me | |
| 15:29:46 | gmann | and if we get any use case to allow non-'filter_tenant_id=project_id' host then we can see | |
| 15:30:07 | gmann | i mean the current way stephenfin doing. | |
| 15:30:15 | stephenfin | I think that's necessary. Listing all hosts in a deployment is too much power | |
| 15:31:01 | gibi | I agree | |
| 15:31:02 | gmann | yeah, and it contradict our new concept of system vs project .. | |
| 15:31:02 | stephenfin | (Continuing to think out loud) Even with that though, it's still odd that we will now allow users to list some hypervisors but we won't show them the 'OS-EXT-SRV-ATTR:hypervisor_hostname' attribute | |
| 15:31:30 | stephenfin | But maybe not as odd as having to ask a system admin to create an instance for you | |
| 15:31:45 | stephenfin | gmann: This is a tricky problem :) | |