On 12/14/2016 8:54 AM, Sean Dague wrote:
We spent most of the Nova API meeting today to try to get to a concensus space on https://review.openstack.org/#/c/393205/ which is about validating the parameters passed for sorting / filtering on the servers resource. Today, we pretty much take whatever you throw in there and jam it in the sql filters so we're bleeding out internal details, and letting you do weird things like sort on join tables which causes potential major performance issues.The solution is be explicit about the filters that we support. The challenge is going from weird grey space where none of this is well defined, to a well defined space. We all AGREED that we need to be explicit about what's supported, the current state of the world is madness. We still need final agreement on what that whitelist is. John and Alex are going to validate that over the next day or so. The intent is to keep this pretty wide to anything sensible in the REST representation that's not going to kill us on the join tables side. One of the sticking points on the spec was the idea that everything in the filter/sort whitelist needed performant indexes. This was going to make the whitelist smaller, then got push back from ops that really want some of those other things. We all AGREED to *drop* that requirement on indexing everything. With Cells v2 coming, searchlight going to be needed for efficient full cloud searching of instances, the entire performance profile of a lot of these are going to change over the next couple of cycles. Instead of focusing on tightening those up now, we should just include documentation on how to performance tune your db if your users are regularly using filters we don't optimize for. This REMOVES the need for the policy point which was going to allow these things, as we'll keep the whitelist large. It means every cloud has the same behavior. Lastly, there was the question of admin vs. non admin allowed parameters. We AGREED that the only things we'd make admin only are things that leak cloud internals, which is node/host attributes. In an ideal future we'd like non admin users to be able to operate on the hashed versions of these (currently a sha(project_id + host)), however that's all done python side now for display, so it's not really an option today. This would make things like `project_id` and `all_tenants` valid filters for regular users. However, if we interpret them within the user's context, that's fine. `all_tenants` means "All tenants that I am allowed to see". For a Nova admin, that's everyone. In a future with hierarchical multi tenancy, this might be a subtree. project_id is fine as long as it's filtered by the project_id's you have access to in your context. Mostly it will be a noop for normal users, but there is no reason not to allow it. Plus it does create a soft roll into hierarchical multi tenancy. We also AGREED that we really need to get the spec into a merge state by Friday, and that folks would start working on code under the assumption that the agreement above is where we are headed. The only really remaining sticking point is the exact content of the whitelist, which John and Alex are going to focus on. Then clean up language in the spec. For folks not in this meeting, please make sure we didn't miss anything here. Hopefully we can get this closed shortly. Thanks a bunch, -Sean
This all sounds good to me. I'm glad we're keeping the non-joined-column whitelist open to things that we already represent in the server details response, and that we're removing the part about the policy config for the whitelist. And I like that we don't have to obsess over what is indexed and what isn't. I think there is some low hanging fruit that we could index as obvious things a lot of people probably already filter on, which we could actually just handle independently of the spec. We actually did something like that already [1] as a result of an earlier review on this spec.
I could see the all_tenants stuff being a bit confusing at first just because people are probably so used to that being admin-only with no exceptions for so long. But I think as long as non-admins only see servers in their tenant by default, that's fine and backward compatible.
Thanks for recapping the discussion here and moving this forward as I've admittedly not been very involved in reviewing this spec and have probably muddied the waters when I did.
[1] https://github.com/openstack/nova/commit/887cc5243eeef7028aafb393948abd320893179b
-- Thanks, Matt Riedemann __________________________________________________________________________ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
