| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-03-10 | |||
| 14:37:01 | gmann | sean-k-mooney: +1, as long as it inherit from forbidden error, wsgi layer will handle it as 403 otherwise you need to handle it explicitly in API controller. | |
| 14:37:28 | sean-k-mooney | ok ill add you to the patch to review if that is ok when i get time to write it | |
| 14:37:43 | gmann | sean-k-mooney: and yes 403 return for error cases does not need microversion as it is already generic return code as you mentioned. | |
| 14:37:52 | yonglihe | gibi： hope you have some bandwidth . I can fix comments at you guys night. wish me lucky. -:) | |
| 14:38:11 | gibi | yonglihe: I plan to get to the smartnic series before I leave for today | |
| 14:38:15 | gibi | yonglihe: have a nice night | |
| 14:38:22 | gmann | stephenfin: but if we drop we again go back to the problem of using PROJECT_AMDIN for POST /server API on specific host | |
| 14:38:58 | stephenfin | gmann: That's a fair point | |
| 14:39:00 | yonglihe | gibi: thanks have a good day. | |
| 14:39:14 | stephenfin | gmann: In that case, we can persist with it. I wasn't sure of the use case but that sounds like a good one. | |
| 14:49:00 | gmann | stephenfin: 1 comment, for case where there is no 'filter_tenant_id' in host's aggregate then that host we can return for project admin | |
| 14:49:48 | stephenfin | oh, that makes things more useful | |
| 14:50:06 | stephenfin | isn't that a lot of information to expose though? | |
| 14:51:11 | stephenfin | Maybe not. They'll just see the summary view so ID, hostname, state, and status | |
| 14:51:14 | gibi | that would basically mean that all project admin in a non tenant separated cloud see every compute | |
| 14:51:38 | gmann | yeah | |
| 14:51:43 | gibi | do we allow project admins to boot on all the computes in that case directly? | |
| 14:52:30 | gmann | all you mean all or only host's computes with no 'filter_tenant_id' | |
| 14:52:30 | gibi | 15:38 < gmann> stephenfin: but if we drop we again go back to the problem of using | |
| 14:52:30 | gibi | I mean what exactly this means? | |
| 14:52:33 | gibi | PROJECT_AMDIN for POST /server API on specific host | |
| 14:53:10 | gibi | does the above mean that we want to allow project admins to boot on specific compute host? | |
| 14:53:22 | stephenfin | I'm looking and we already do | |
| 14:53:24 | gmann | if it pass the filter_tenant_id filter (host match with filter_tenant_id or there is host with no filter_tenant_id) then that compute canbe allowed and rest 403 | |
| 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 | stephenfin | so that check will return False for project members but true for system admin/member/reader | |
| 15:04:54 | gmann | got it, little confusing to read | |
| 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 | |