From f81524d40e7bc71bef86d5f1f860a7d1c6a9df7d Mon Sep 17 00:00:00 2001 From: ghanshyam Date: Thu, 20 Sep 2018 04:15:33 +0000 Subject: [PATCH] Spec for API inconsistency cleanup APIImpact Change-Id: I7b71f5b3ec00b5f59fca6b1b0e239c1dd9dfa75d --- .../approved/api-consistency-cleanup.rst | 300 ++++++++++++++++++ 1 file changed, 300 insertions(+) create mode 100644 specs/train/approved/api-consistency-cleanup.rst diff --git a/specs/train/approved/api-consistency-cleanup.rst b/specs/train/approved/api-consistency-cleanup.rst new file mode 100644 index 000000000..64de987bb --- /dev/null +++ b/specs/train/approved/api-consistency-cleanup.rst @@ -0,0 +1,300 @@ +.. + This work is licensed under a Creative Commons Attribution 3.0 Unported + License. + + http://creativecommons.org/licenses/by/3.0/legalcode + +======================= +API Consistency Cleanup +======================= + +https://blueprints.launchpad.net/nova/+spec/api-consistency-cleanup + +This blueprint proposes some of the cleanups in API for consistency +and better usage. + +Problem description +=================== +Currently, there are lot of inconsistency and loose validation in APIs. +Those inconsistencies are because of v2 API compatibility. + +Because of loose validation for request body and query param, APIs ignore +the unknown and invalid inputs silently. This gives the impressions to user +that, requested unknown or invalid inputs are valid and taken care by Nova. +But Nova ignore all unknown or invalid inputs at API layer itself without +any warning to users. + +server representation is not consistent among all APIs which return the +server info in response body. +GET, PUT, REBUILD /servers APIs return the server representation (with all +server attributes). But PUT and REBUILD /servers response does not +match with GET /servers API. + +These are 2 examples of API inconsistency and there might be more such example +which are described in Proposed section. + +Use Cases +--------- +As an API consumer, I would like to use Nova API in more consistent way and +with strong validation for better usage. + +As a Developer, I would like to provide and maintain better/clean/consistent +APIs. + +There are maintenance benefits for client side code for parsing the API +response. For developers, maintenance can be easy only if we are able to +bump the minimum microversion in future. + +Proposed change +=============== +Add a new microversion to the APIs to cleanup multiple issues and +inconsistency. + +Proposal is to do all the mentioned cleanup in single microverison. + +Cleanup List: + +#. 400 for unknown param for query param and for request body. + + Currently unknown param in server query param or in many other APIs request + body are ignored silently. This leads to lot of inconsistency, + one good example of this is ``--deleted`` and ``--status DELETED`` query for + ``nova --list``. + + If you are non-admin then, filter ``--deleted`` and ``--status deleted`` + behave as 200 and 403 respectively. Both are same filter from end user + point of view but due to our implementation we return different behavior. + + * ``nova list --deleted``: + 200 and it is silenlty ignored due to additionalProperty=True and for + backward compatibility API accept and ignore the invalid filters. + ``--deleted`` is invalid filter for non-admin + + * ``nova list --status DELETED`` : + 403. ``--status`` is valid filter for non-admin so API does not ignore + this. + + We have explicit check for status=DELETED request and if requester + is non-admin then, 403. + Details discussion [1]_ + + We can fix that by making ``additionalProperties: False`` for query param + and request body of all the APIs where ``additionalProperties`` is True. + For Example [2]_ and [3]_ + + APIs need modification: + https://github.com/openstack/nova/search?p=1&q=additionalProperties%3A+True&unscoped_q=additionalProperties%3A+True + +#. Making server representation always consistent among all APIs + returning the complete server representation. + + GET, PUT, REBUILD /servers APIs return the complete server representation + (with all server attributes). But PUT and REBUILD /servers response does not + match with GET /servers API. + + Difference between server representation in PUT, REBUILD from GET might + be with historic reason that PUT and REBUILD only returning the fields + in the response that could be taken on the request which modify the server, + but over time we have started returning more fields to the response to make + it consistent with GET response. That way only newly added fields in GET + response started return in PUT, REBUILD also but old fields were missed. + It end up, not keeping the original intent of PUT, REBUILD response and not + completely consistent with GET response. + + There are many field which are only returned in GET API but not in PUT or + REBUILD. + Response difference which is attributes added as extensions: + + * OS-EXT-AZ:availability_zone + * OS-EXT-SRV-ATTR:host + * OS-EXT-SRV-ATTR:hostname + * OS-EXT-SRV-ATTR:hypervisor_hostname + * OS-EXT-SRV-ATTR:instance_name + * OS-EXT-SRV-ATTR:kernel_id + * OS-EXT-SRV-ATTR:launch_index + * OS-EXT-SRV-ATTR:ramdisk_id + * OS-EXT-SRV-ATTR:reservation_id + * OS-EXT-SRV-ATTR:root_device_name + * OS-EXT-SRV-ATTR:user_data + * OS-EXT-STS:power_state + * OS-EXT-STS:task_state + * OS-EXT-STS:vm_state + * OS-SRV-USG:launched_at + * OS-SRV-USG:terminated_at + * os-extended-volumes:volumes_attached + + APIs need modification: + + * PUT /servers + * POST /servers/{server_id}/action {rebuild} + +#. Change the default return value of ``swap`` field from the empty string + to 0 (integer) in flavor APIs. + + Currently while creating a flavor, if you don't set the optional ``swap`` + property, the value of the ``swap`` property in the flavor API's response + will return as an empty string. + Bug: https://bugs.launchpad.net/nova/+bug/1815476 + While processing this empty string on CLI side, it is shown as blank + which is confusing and not consistent with other fields for example, + "OS-FLV-EXT-DATA:ephemeral". + Flavor representation in server API response has the ``swap`` default + value as 0. + Proposal is to make it consistant and return the ``swap`` default value + as 0 (integer) in below APIs: + + * POST /flavors + * GET /flavors/detail + * GET /flavors/{flavor_id} + * PUT /flavors/{flavor_id} + +#. Return ``servers`` field always in the response of GET + hypervisors API even there are no servers on hypervisor + + Currently, if there are no servers on requested hypervisors then, + ``servers`` field is omitted from API response. This is not + consistent response, ideally all fields should be present in + response even with the empty value. + Proposal is to return the ``servers`` field always in response + of below APIs. ``servers`` field will be an empty list if there are + no servers. + + * GET /os-hypervisors?with_servers=True + * GET /os-hypervisors/detail?with_servers=True + * GET /os-hypervisors/{hypervisor_id}?with_servers=True + +Alternatives +------------ +We leave APIs as it is and use it in same way they are currently or +we can choose the set of issues from above list to fix as single go. + +Below cleanup already filtered out from this proposal: + +#. Remove extensions (OS-) prefix from request and response field. +#. Fix inconsistent/incorrect response codes + +Data model impact +----------------- +None + +REST API impact +--------------- +This proposal is to fix the multiple issues in APIs. I am listing +the REST API impact of each issue in same order as they are listed above. + +#. 400 for unknown param for query param and for request body. + + APIs which allow unknown request and query param and ignore silently + wil be changed to return 400. + Below are the APIs which has ``additionalProperties: True`` and will + be modified to ``additionalProperties: False``: + https://github.com/openstack/nova/search?p=1&q=additionalProperties%3A+True&unscoped_q=additionalProperties%3A+True + +#. Making server representation always consistent among all APIs + + ``PUT /servers/{server_id}`` and + ``POST /servers/{server_id}/action {rebuild}`` API response + will be modified to add all the missing fields which are return by + ``GET /servers/{server_id}``. + + NOTE: new fields will be added with same name they are present in GET + /servers API response (means with ``OS-`` prefix). + +#. Change the default return value of ``swap`` field from the empty string + to 0 (integer) in flavor APIs. + + Below APIs response will be changed to return the ``swap`` default value + as 0 (integer): + + * POST /flavors + * GET /flavors/detail + * GET /flavors/{flavor_id} + * PUT /flavors/{flavor_id} + +#. Return ``servers`` field always in the response of GET + hypervisors API even there are no servers on hypervisor + + Below APIs response will be changed to return the ``servers`` + field always in response body. ``servers`` field will be an + empty list if there are no servers: + + * GET /os-hypervisors?with_servers=True + * GET /os-hypervisors/detail?with_servers=True + * GET /os-hypervisors/{hypervisor_id}?with_servers=True + +Security impact +--------------- +None + +Notifications impact +-------------------- +None + +Other end user impact +--------------------- +The python novaclient and openstack-client will be updated. + +Performance Impact +------------------ +None + +Other deployer impact +--------------------- +None + +Developer impact +---------------- +None + +Upgrade impact +-------------- +None + +Implementation +============== +Assignee(s) +----------- +Primary assignee: + Ghanshyam Mann + +Work Items +---------- +* Single microversion change on Nova API +* Add tests for changes +* python client (python-novaclient and python-openstackclient) change + +Dependencies +============ +None + +Testing +======= +* Add related unit test. +* Add related functional tests. +* Response change schema test in Tempest + +Documentation Impact +==================== +Modify the api-ref to reflect the API change. + +References +========== +* Train PTG agreement: http://lists.openstack.org/pipermail/openstack-discuss/2019-May/005824.html + +* https://etherpad.openstack.org/p/nova-api-cleanup + Nova API cleanup list Etherpad + +.. [1] http://eavesdrop.openstack.org/irclogs/%23openstack-nova/%23openstack-nova.2018-07-10.log.html#t2018-07-10T14:14:18 +.. [2] https://github.com/openstack/nova/blob/c5a80f4843d1f1a5289e0a3f8dbb4921b6fa44bb/nova/api/openstack/compute/schemas/servers.py#L602 +.. [3] https://github.com/openstack/nova/blob/c6218428e9b29a2c52808ec7d27b4b21aadc0299/nova/api/openstack/compute/schemas/agents.py#L93 + +History +======= +.. list-table:: Revisions + :header-rows: 1 + + * - Release Name + - Description + * - Train + - Introduced +