Unpredictable response on invalid query parameters

Bug #1749820 reported by Hongbin Lu
16
This bug affects 2 people
Affects Status Importance Assigned to Milestone
neutron
Fix Released
Medium
Hongbin Lu

Bug Description

Problem
=======
If users provide different kinds of invalid query parameters, the behavior of neutron server looks unpredictable. Also Neutron not provide which fields/sort_dir/sort_type are valid, and even adding/ignoring some fields internally. Sometimes,
it tolerates the invalid inputs and return a successful response. Sometimes, it invalidates the inputs and return an error response.
In addition, the error type is unpredictable and the format of the error message is inconsistent.

1. Invalid sort_key and/or sort_dir:

Neutron returns at least two error types: BadRequest [1] [2] [3] [4], HTTPBadRequest [5] [6] [7] [8]. In addition, the format of error message looks inconsistent. For example, it has the following forms:
* Bad networks request: Attribute '...' references another resource and cannot be used to sort '...' resources. [1]
  - Here we need to split whether the attribute fields are valid to sort or Neutron dosen't support it for sorting(maybe the show fields is just foreign constraints from other db tables). [3] [4] [8]
* Bad networks request: '...' is an invalid attribute for sort key. [2]
* [u'...'] is invalid attribute for sort_keys [5]

2. Invalid limit and/or marker:

Sometimes, neutron returns error with one of the following types: BadRequest [9], NetworkNotFound [10]. Sometimes, neutron doesn't return error [11].

3. Invalid filter

Sometimes, neutron returns error with one of the following types: InvalidInput [12], NotImplementedError [13]. Sometimes, neutron doesn't return error [14]

4. Invalid fields

Neutron always returns a successful response [15] regardless of validity of the inputs.

Proposal
========
Improve the predictability of neutron server by handling invalid query parameters consistently. Optimally, neutron server behaves as following:
* Always returns error (i.e. 400 response) on invalid user inputs. IMHO, ignoring invalid inputs (like [11][14][15]) is bad.
* Make error type predictable. There are several options to achieve this. One option is to group different kinds of invalid inputs and define
 a specific error type for each group (i.e. Simple split them as InvalidSorting for #1, InvalidPagination for #2, InvalidFilter for #3, etc. Or more sophisticated).
* Make the format of error message consistent. For example, the format of error messages [1][2][3] can be consolidated into one.

Referred examples
=================

[1] GET "/v2.0/networks?sort_dir=desc&sort_key=segments"
{"NeutronError": {"message": "Bad networks request: Attribute 'segments' references another resource and cannot be used to sort 'networks' resources.", "type": "BadRequest", "detail": ""}}

[2] GET "/v2.0/networks?sort_dir=desc&sort_key=provider:physical_network"
{"NeutronError": {"message": "Bad networks request: 'provider:physical_network' is an invalid attribute for sort key.", "type": "BadRequest", "detail": ""}}

[3] GET "/v2.0/networks?sort_dir=asc&sort_key=segments&sort_dir=asc&sort_key=subnets"
{"NeutronError": {"message": "Bad networks request: The attribute 'segments' is reference to other resource, can't used by sort 'networks'.", "type": "BadRequest", "detail": ""}}

[4] GET "/v2.0/networks?sort_dir=asc&sort_key=port_security_enabled"
{"NeutronError": {“message”: “Bad networks request: port_security_enabled is invalid attribute for sort_key.”, "type": "BadRequest", "detail": ""}}

[5] GET "/v2.0/networks?sort_dir=desc&sort_key=xxx"
{"NeutronError": {"message": "[u'xxx'] is invalid attribute for sort_keys", "type": "HTTPBadRequest", "detail": ""}}

[6] GET "/v2.0/networks?sort_dir=xxx"
{"NeutronError": {"message": "The number of sort_keys and sort_dirs must be same", "type": "HTTPBadRequest", "detail": ""}}

[7] GET "/v2.0/networks?sort_dir=xxx&sort_key=id"
{"NeutronError": {"message": "[u'xxx'] is invalid value for sort_dirs, valid value is 'asc' and 'desc'", "type": "HTTPBadRequest", "detail": ""}}

[8] GET "/v2.0/networks?sort_dir=asc&sort_key=qos_policy_id"
{"NeutronError": {"message": "[u'qos_policy_id'] is invalid attribute for sort_keys", "type": "HTTPBadRequest", "detail": ""}}

[9] GET "/v2.0/networks?limit=-1"
{"NeutronError": {"message": "Bad limit request: Limit must be an integer 0 or greater and not '-1'.", "type": "BadRequest", "detail": ""}}

[10] GET "/v2.0/networks?limit=1&marker=xxx"
{"NeutronError": {"message": "Network xxx could not be found.", "type": "NetworkNotFound", "detail": ""}}

[11] GET "/v2.0/networks?marker=xxx"
{"networks":...}

[12] GET "/v2.0/networks?admin_state_up=xxx"
{"NeutronError": {"message": "Invalid input for operation: 'xxx' cannot be converted to boolean.", "type": "InvalidInput", "detail": ""}}

[13] GET "/v2.0/networks?subnets=11"
{"NotImplementedError": {"message": "in_() not yet supported for relationships. For a simple many-to-one, use in_() against the set of foreign key values.", "type": "NotImplementedError", "detail": ""}}

[14] GET "/v2.0/networks?xxx=11"
{"networks":...}

[15] GET "/v2.0/networks?fields=xxx"
{"networks":...}

Hongbin Lu (hongbin.lu)
description: updated
description: updated
description: updated
Hongbin Lu (hongbin.lu)
description: updated
description: updated
description: updated
description: updated
description: updated
Hongbin Lu (hongbin.lu)
description: updated
Changed in neutron:
assignee: nobody → Hongbin Lu (hongbin.lu)
James Anziano (janzian)
Changed in neutron:
status: New → Confirmed
zhaobo (zhaobo6)
description: updated
Revision history for this message
Akihiro Motoki (amotoki) wrote :

The recommended behavior for non-existing marker is explained here: http://specs.openstack.org/openstack/api-wg/guidelines/pagination_filter_sort.html (the last paragraph of "Pagination" section)

Miguel Lavalle (minsel)
Changed in neutron:
importance: Undecided → Medium
Miguel Lavalle (minsel)
Changed in neutron:
milestone: none → rocky-1
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to neutron (master)

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

Changed in neutron:
status: Confirmed → In Progress
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix proposed to neutron-lib (master)

Related fix proposed to branch: master
Review: https://review.openstack.org/554369

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Related fix proposed to branch: master
Review: https://review.openstack.org/558894

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix merged to neutron-lib (master)

Reviewed: https://review.openstack.org/554369
Committed: https://git.openstack.org/cgit/openstack/neutron-lib/commit/?id=0abe67c6ebb07eeb02236cb373b7c42cde03b3ec
Submitter: Zuul
Branch: master

commit 0abe67c6ebb07eeb02236cb373b7c42cde03b3ec
Author: Hongbin Lu <email address hidden>
Date: Mon Mar 19 22:08:05 2018 +0000

    Annotate all the filter parameters for networks

    Add a 'is_filter' keyword for each network's attribute that can be
    used as filter. In the future, we will rely on this to perform
    filter validation: https://review.openstack.org/#/c/554368/ .

    Change-Id: I0cdc1ff482d4ff7ec6afa391392c27c2c099cc0e
    Related-Bug: #1749820

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Reviewed: https://review.openstack.org/558894
Committed: https://git.openstack.org/cgit/openstack/neutron-lib/commit/?id=b03226d59739fafe036bc60c62f165da598ca947
Submitter: Zuul
Branch: master

commit b03226d59739fafe036bc60c62f165da598ca947
Author: Hongbin Lu <email address hidden>
Date: Wed Apr 4 18:37:05 2018 +0000

    Annotate network parameters for sort_key

    Add a 'is_sort_key' keyword for each network's attribute that can
    be used as a sort_key. In the future, we will rely on this keyword
    to perform validation: https://review.openstack.org/#/c/554368/ .

    Change-Id: I6104a1b534a7ddc4856bd1a25d77e12b1ed5421f
    Related-Bug: #1749820

Miguel Lavalle (minsel)
Changed in neutron:
milestone: rocky-1 → rocky-2
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix proposed to neutron-lib (master)

Related fix proposed to branch: master
Review: https://review.openstack.org/566970

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Related fix proposed to branch: master
Review: https://review.openstack.org/567033

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Related fix proposed to branch: master
Review: https://review.openstack.org/567056

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Related fix proposed to branch: master
Review: https://review.openstack.org/567067

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix proposed to neutron (master)

Related fix proposed to branch: master
Review: https://review.openstack.org/567071

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Related fix proposed to branch: master
Review: https://review.openstack.org/567072

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Change abandoned on neutron-lib (master)

Change abandoned by Hongbin Lu (<email address hidden>) on branch: master
Review: https://review.openstack.org/567056
Reason: Squashed into: https://review.openstack.org/#/c/566970/

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Change abandoned by Hongbin Lu (<email address hidden>) on branch: master
Review: https://review.openstack.org/567067
Reason: Squashed into: https://review.openstack.org/#/c/567033/

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix merged to neutron (master)

Reviewed: https://review.openstack.org/567071
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=c69a570c87e5beeb1bdc7a913bf8e3b6275d7cd7
Submitter: Zuul
Branch: master

commit c69a570c87e5beeb1bdc7a913bf8e3b6275d7cd7
Author: Hongbin Lu <email address hidden>
Date: Tue May 8 22:26:00 2018 +0000

    Annotate filter parameters for standard attributes

    Add 'is_filter' keyword for attributes 'revision_number' and
    'description'. This indicates that these two attributes can be used
    as filter parameters for standard resources. In the future, we will
    rely on this to perform filter validation:
    https://review.openstack.org/#/c/554368/

    Change-Id: I847a849720297fb6b316098e278fa5c71dd1010a
    Related-Bug: #1749820

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix merged to neutron-lib (master)

Reviewed: https://review.openstack.org/566970
Committed: https://git.openstack.org/cgit/openstack/neutron-lib/commit/?id=eb27424cb04ef67188fad358ae356ca33bae46f3
Submitter: Zuul
Branch: master

commit eb27424cb04ef67188fad358ae356ca33bae46f3
Author: Hongbin Lu <email address hidden>
Date: Tue May 8 18:29:48 2018 +0000

    Annotate filter parameters for all resources

    Add a 'is_filter' keyword for each resource's attribute that can be
    used as filter. In the future, we will rely on this to perform
    filter validation: https://review.openstack.org/#/c/554368/ .

    This patch processes the following resources: ports, segments,
    trunks, address scopes, floating IPs, routers, subnetpools,
    subnets, agents, availability zones, flavors, log, metering,
    qos related resources.

    Note: This patch processes the attributes that are documented
    as filter parameter in api-ref.

    Change-Id: I595b27a83fb2c185a4889030d619ce4d877fa881
    Related-Bug: #1749820

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Reviewed: https://review.openstack.org/567033
Committed: https://git.openstack.org/cgit/openstack/neutron-lib/commit/?id=969cf9ecb867e1844191b156098085b09df80cd6
Submitter: Zuul
Branch: master

commit 969cf9ecb867e1844191b156098085b09df80cd6
Author: Hongbin Lu <email address hidden>
Date: Tue May 8 20:15:37 2018 +0000

    Annotate sort_key parameters for all resources

    Add a 'is_sort_key' keyword for each attribute that can
    be used as a sort_key. In the future, we will rely on this keyword
    to perform validation: https://review.openstack.org/#/c/554368/ .

    This patch processes the following resources: ports, segments,
    trunks, address scopes, floating IPs, routers, subnetpools,
    subnets, flavors, logging, metering, qos related resources.

    Note: This patch only processes the attributes that are documented
    as sort_key in api-ref.

    Change-Id: I936378855ba43bd7dfdb25a5e324a4bd3e21345d
    Related-Bug: #1749820

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to neutron (master)

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

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

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

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

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

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix merged to neutron (master)

Reviewed: https://review.openstack.org/567072
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=8b19db164aa9ecdf2c0bbc97f6e2799f15cebf27
Submitter: Zuul
Branch: master

commit 8b19db164aa9ecdf2c0bbc97f6e2799f15cebf27
Author: Hongbin Lu <email address hidden>
Date: Tue May 8 22:35:35 2018 +0000

    Annotate filter parameters for tag attributes

    * Add additional attributes 'tags-any', 'not-tags' and 'not-tags-any'
      to the attribute map. These three attributes represent the query
      parameters on list request.
    * Add 'is_filter' keyword for attributes 'tags', 'tags-any', 'not-tags'
      and 'not-tags-any'. This indicates that these four attributes can be
      used as filter parameters. In the future, we will rely on this metadata
      to perform filter validation: https://review.openstack.org/#/c/554368/

    Change-Id: I9e2033e7ba9d198f96c1ce6089a809d6721dd82d
    Related-Bug: #1749820

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to neutron-lib (master)

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

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix proposed to neutron (master)

Related fix proposed to branch: master
Review: https://review.openstack.org/575767

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Related fix proposed to branch: master
Review: https://review.openstack.org/575856

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Change abandoned on neutron (master)

Change abandoned by Hongbin Lu (<email address hidden>) on branch: master
Review: https://review.openstack.org/574908

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Change abandoned by Hongbin Lu (<email address hidden>) on branch: master
Review: https://review.openstack.org/574483

Miguel Lavalle (minsel)
Changed in neutron:
milestone: rocky-2 → rocky-3
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to neutron-lib (master)

Reviewed: https://review.openstack.org/575560
Committed: https://git.openstack.org/cgit/openstack/neutron-lib/commit/?id=d7615f5a82358e83fc7c72d05eb98fce7ccd43f5
Submitter: Zuul
Branch: master

commit d7615f5a82358e83fc7c72d05eb98fce7ccd43f5
Author: Hongbin Lu <email address hidden>
Date: Thu Jun 14 22:23:41 2018 +0000

    Add the two missing 'is_filter' keyword

    * Add is_filter to 'shared' attribute of subnet
    * Add is_filter to 'floating_ip_address' attribute of floatingip

    Change-Id: Ifcb989ec5e7b3be0699029e257f5adf4a66a2452
    Partial-Bug: #1749820

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to neutron-lib (master)

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

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to neutron (master)

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

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to neutron-lib (master)

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

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix merged to neutron (master)

Reviewed: https://review.openstack.org/575856
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=6f5946d34bcc4a52920533f1a4ee81d7615a00ea
Submitter: Zuul
Branch: master

commit 6f5946d34bcc4a52920533f1a4ee81d7615a00ea
Author: Hongbin Lu <email address hidden>
Date: Fri Jun 15 21:48:06 2018 +0000

    Fix the tests for filtering with qos_policy_id

    Some QoS tests tried to list ports by attribute qos_policy_id
    but this attribute is not a valid filter. In before, the tests
    passed because neutron ignored the invalid filter and returned
    all the ports which happened to be the correct set. However,
    using qos_policy_id as filter is incorrect and this patch fixes it.

    Change-Id: Ic3ab5b3ffdc378d570678b9c967cb42b0c7a8a9b
    Related-Bug: #1749820

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Reviewed: https://review.openstack.org/575767
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=9f6a4ceecfef858f503df3f63de4f945374eee4f
Submitter: Zuul
Branch: master

commit 9f6a4ceecfef858f503df3f63de4f945374eee4f
Author: Hongbin Lu <email address hidden>
Date: Fri Jun 15 15:12:10 2018 +0000

    Remove the unit test 'test_ports_vnic_type_list'

    This test created three ports and tried to list and filter those
    ports by their vnic type. However, the current implementation
    doesn't support filter by vnic type. The test passed is a false
    positive because neutron ignored the invalid filter and returned
    all the ports which happened to be those three ports.

    This patch remove this invalid test case.

    Change-Id: I4397df1c35463a8b532afdc9c5d28b37224a37b4
    Related-Bug: #1749820

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to neutron-lib (master)

Reviewed: https://review.openstack.org/581870
Committed: https://git.openstack.org/cgit/openstack/neutron-lib/commit/?id=befb16ed9ef50edd1316bc1db5b906fbd3c8e98d
Submitter: Zuul
Branch: master

commit befb16ed9ef50edd1316bc1db5b906fbd3c8e98d
Author: Hongbin Lu <email address hidden>
Date: Wed Jul 11 19:30:17 2018 +0000

    Add shim extension sort-key-validation

    Neutron patch: https://review.openstack.org/#/c/580217/

    Change-Id: I00e2aafc271e67977105c866ed9f9f586605fdb4
    Partial-Bug: #1749820

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Reviewed: https://review.openstack.org/580190
Committed: https://git.openstack.org/cgit/openstack/neutron-lib/commit/?id=94516d1e7e402978f6ef82ddab4f1230e5901231
Submitter: Zuul
Branch: master

commit 94516d1e7e402978f6ef82ddab4f1230e5901231
Author: Hongbin Lu <email address hidden>
Date: Wed Jul 4 15:23:10 2018 +0000

    Add shim extension filter-validation

    Neutron patch: https://review.openstack.org/#/c/574907/

    Change-Id: Ib361ad3dbb8bf79a67dae7aeedf1fb5c87aec6ab
    Partial-Bug: #1749820

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to neutron (master)

Reviewed: https://review.openstack.org/574907
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=2b1d8ea4a202f24c1b485ccebdbf831c505a4e6a
Submitter: Zuul
Branch: master

commit 2b1d8ea4a202f24c1b485ccebdbf831c505a4e6a
Author: Hongbin Lu <email address hidden>
Date: Tue Jun 12 22:20:35 2018 +0000

    Implement filter validation

    Enforce validation on filter parameters on list requests.
    If an API request contains an unknown or unsupported parameter,
    the server will return a 400 response instead of silently ignoring
    the invalid input.

    In resource attributes map, all filter parameters are annotated by
    the ``is_filter`` keyword. Attributes with is_filter set to True
    are candidates for validation.

    Enabling filter validation requires support from core plugin and
    all service plugins so each plugin need to indicate if it supports
    the validation by setting ``__filter_validation_support`` to True.
    If this field is not set, the default is False and validation is
    turned off. Right now, the ML2 plugin and all the in-tree service
    plugin support filter validation. Out-of-tree plugins will have
    filter validation disabled by default.

    An API extension is introduced to allow API users to discover this
    new API behavior. This feature can be disabled by cloud operators
    if they choose to do that. If it is disabled, the extension won't
    be presented.

    Depends-On: Ic3ab5b3ffdc378d570678b9c967cb42b0c7a8a9b
    Depends-On: I4397df1c35463a8b532afdc9c5d28b37224a37b4
    Depends-On: I3f2e6e861adaeef81a1a5819a57b28f5c6281d80
    Depends-On: I1189bc9a50308df5c7e18c329f3a1262c90b9e12
    Depends-On: I057cd917628c77dd20c0ff7747936c3fec7b4844
    Depends-On: I0b24a304cc3466a2c05426cdbb6f9d99f1797edd

    Change-Id: I21bf8a752813802822fd9966dda6ab3b6c4abfdc
    Partial-Bug: #1749820

Hongbin Lu (hongbin.lu)
Changed in neutron:
status: In Progress → Fix Committed
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Change abandoned on neutron (master)

Change abandoned by Slawek Kaplonski (<email address hidden>) on branch: master
Review: https://review.openstack.org/580217
Reason: This review is > 4 weeks without comment, and failed Jenkins the last time it was checked. We are abandoning this for now. Feel free to reactivate the review by pressing the restore button and leaving a 'recheck' comment to get fresh test results.

Yue Qu (bruceq+)
Changed in neutron:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Duplicates of this bug

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.