Open Stack

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