Listing resources with invalid filters should result in a 400

[Richard]

Using devstack all in one and running the get request:

curl -X GET 'localhost:35357/v3/users/?cake=isgood'

gives me back the entire user list instead of an error.
I also ran this query with httpie and this is what the neat
version of the output looks like:
http://paste.openstack.org/show/593916/

This affects all list operations in keystone that support
querying.

[Lance Bragstad (lbragstad)]

I'm not sure if there is a standard for validating query strings, but I figured I'd triage this since it has an impact on user-experience for sure.

Thanks Richard!

[Steve Martinelli (stevemar)]

According to the HTTP spec the query args are part of the URL, these are invalid and should result in a 4xx error. However, even the great google doesn't adhere to that, try the following:

https://www.google.com/#q=search+for+something&invalid=param&more=stuff

With that said, I tried looking up what the API working group has to say about this:

http://specs.openstack.org/openstack/api-wg/guidelines/pagination_filter_sort.html

But no luck, let's bounce this bug off of them and see what they have to say.

[Chris Dent (cdent)]

We haven't formalized this yet for query strings (that I can find), but have discussed it and agree that 400 is the right response here as it is the only way of letting the client side know that something was less than correct about the query.

There's some related discussion in http://specs.openstack.org/openstack/api-wg/guidelines/http.html#failure-code-clarifications about incorrect object attributes in bodies which uses the same logic:

    ...the server should return a 400 Bad Request response. Do not handle the request as normal by ignoring the bad attribute. Returning an error allows the client side to know which attribute is wrong and have the potential to fix a bad request or bad code.

So I reckon there probably needs to be a generic guideline about this, and the filtering guideline should reference that when saying "return a 400 for bad filter params".

Originally I took the stance that the bad param should just be ignored (be liberal in what you accept), but it was exactly the case in this bug that convinced me otherwise.

[Steve Martinelli (stevemar)]

Thanks Chris, Ed said the same thing on IRC:

edleafe: stevemar: just saw your question. We generally recommend a 400 with a message that explains the problem

I'll re-word this bug to be a general fix for all our list operations in keystone. Though this is backwards incompatible, I believe it is allowed since it fixes a bug.

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/417315

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/417441

[Morgan Fainberg (mdrnstm)]

As said in the meeting, this is an API behavior break that is not security related. We cannot have this bug fixed without microversions or API version rev.

See http://specs.openstack.org/openstack/api-wg/guidelines/evaluating_api_changes.html#guidance

I have marked the assosicated review with a -2 and a similar comment. I will remove the -2 once we have a way forward that is not an API contract break.

An idea floated was a strict query param that new clients can use to change behavior... there are ways to slice this, lets figure out a path forward and we can unblock it.

For the record, I like this being more explicit on non-supported params resulting in a 4xx.

[Chris Dent (cdent)]

In response to Morgan: As I said in response to a similar issue in glance, I think the rules in that 'evaluating' guideline are potentially too strict, especially with regard to changes just like this: making people's lives better.

Making people's lives better should be more important than following fairly arbitrary rules about versioning.

That, however, is just my personal opinion and not some kind of official api-wg statement (I wonder, however, if maybe we should change the rules to make them moderately sane?).

[Steve Martinelli (stevemar)]

Marking this as wishlist since we can't break API compatibility. The only way we could do this is by implementing microversions.

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/417441
Committed: https://git.openstack.org/cgit/openstack/api-wg/commit/?id=b3e3b5f15344d130555f8f49f01336f7781d6ccb
Submitter: Jenkins
Branch: master

commit b3e3b5f15344d130555f8f49f01336f7781d6ccb
Author: EdLeafe <email address hidden>
Date: Fri Jan 6 16:31:00 2017 +0000

    Add guideline for invalid query parameters

    This is in response to the bug "Listing resources with invalid filters
    should result in a 400", which caused problems in Keystone. We had a
    similar guideline for handling invalid entries in a request body; this
    adds similar guidance for query strings.

    Change-Id: I92c1e0e25036c85398ffd0c40f9c16cb3e8a3029
    Partial-Bug: #1654084

[Lance Bragstad (lbragstad)]

Automatically unassigning due to inactivity.

[Morgan Fainberg (mdrnstm)]

I want to point out I disagree with the guidance from the API-SIG. I rather have a server ignore (drop) query params than raise a 400.

[Chris Dent (cdent)]

But but Morgan (said in a whiney voice) how would anyone know that they sent a bad query param, one that might have been a typo for a good one?

[OpenStack Infra (hudson-openstack)]

Change abandoned by Tin Lam (<email address hidden>) on branch: master
Review: https://review.opendev.org/417315