From e6f742544432d6066f1fba4666580919eb7859bd Mon Sep 17 00:00:00 2001 From: Eric Fried Date: Mon, 9 Dec 2019 09:58:53 -0600 Subject: [PATCH] Nix os-server-external-events 404 condition The POST /os-server-external-events API had the following confusing behavior: With multiple events in the payload, if *some* (but not all) were dropped, the HTTP response was 207, with per-event 4xx error codes in the payload. But if *all* of the events were dropped, the overall HTTP response was 404 with no payload. Thus, especially for consumers sending only one event at a time, it was impossible to distinguish e.g. "you tried to send an event for a nonexistent instance" from "the instance you specified hasn't landed on a host yet". This fix gets rid of that sweeping 404 condition, so if *any* subset of the events are dropped (including *all* of them), the HTTP response will always be 207, and the payload will always contain granular per-event error codes. This effectively means the API can no longer return 404, ever. Closes-Bug: #1855752 Change-Id: Ibad1b51e2cf50d00102295039b6e82bc00bec058 --- api-ref/source/os-server-external-events.inc | 10 +++++--- .../compute/server_external_events.py | 7 +----- .../compute/test_server_external_events.py | 25 ++++++++++++------- 3 files changed, 24 insertions(+), 18 deletions(-) diff --git a/api-ref/source/os-server-external-events.inc b/api-ref/source/os-server-external-events.inc index f04c98839f5..d96bc263969 100644 --- a/api-ref/source/os-server-external-events.inc +++ b/api-ref/source/os-server-external-events.inc @@ -32,11 +32,15 @@ updated ``code`` and ``status`` indicating their level of success. Normal response codes: 200, 207 A 200 will be returned if all events succeeded, 207 will be returned -if some events could not be processed. The ``code`` attribute for the +if any events could not be processed. The ``code`` attribute for the event will explain further what went wrong. -Error response codes: badRequest(400), unauthorized(401), forbidden(403), -itemNotFound(404) +Error response codes: badRequest(400), unauthorized(401), forbidden(403) + +.. note:: Prior to the fix for `bug 1855752`_, error response code 404 may be + erroneously returned when all events failed. + +.. _bug 1855752: https://bugs.launchpad.net/nova/+bug/1855752 Request ------- diff --git a/nova/api/openstack/compute/server_external_events.py b/nova/api/openstack/compute/server_external_events.py index fdea25db8c3..35f394f41ca 100644 --- a/nova/api/openstack/compute/server_external_events.py +++ b/nova/api/openstack/compute/server_external_events.py @@ -13,14 +13,12 @@ # under the License. from oslo_log import log as logging -import webob from nova.api.openstack.compute.schemas import server_external_events from nova.api.openstack import wsgi from nova.api import validation from nova.compute import api as compute from nova import context as nova_context -from nova.i18n import _ from nova import objects from nova.policies import server_external_events as see_policies @@ -65,7 +63,7 @@ def _get_instances_all_cells(self, context, instance_uuids, return instances - @wsgi.expected_errors((403, 404)) + @wsgi.expected_errors(403) @wsgi.response(200) @validation.schema(server_external_events.create, '2.0', '2.50') @validation.schema(server_external_events.create_v251, '2.51', '2.75') @@ -146,9 +144,6 @@ def create(self, req, body): if accepted_events: self.compute_api.external_instance_event( context, accepted_instances, accepted_events) - else: - msg = _('No instances found for any event') - raise webob.exc.HTTPNotFound(explanation=msg) # FIXME(cyeoh): This needs some infrastructure support so that # we have a general way to do this diff --git a/nova/tests/unit/api/openstack/compute/test_server_external_events.py b/nova/tests/unit/api/openstack/compute/test_server_external_events.py index 99695dd8828..7d0d6e18654 100644 --- a/nova/tests/unit/api/openstack/compute/test_server_external_events.py +++ b/nova/tests/unit/api/openstack/compute/test_server_external_events.py @@ -16,7 +16,6 @@ import mock from oslo_utils.fixture import uuidsentinel as uuids import six -import webob from nova.api.openstack.compute import server_external_events \ as server_external_events_v21 @@ -108,12 +107,17 @@ def _assert_call(self, body, expected_uuids, expected_events): result = response.obj code = response._code - self.assertEqual(1, api_method.call_count) - call = api_method.call_args_list[0] - args = call[0] + if expected_events: + self.assertEqual(1, api_method.call_count) + call = api_method.call_args_list[0] + args = call[0] - call_instances = args[1] - call_events = args[2] + call_instances = args[1] + call_events = args[2] + else: + self.assertEqual(0, api_method.call_count) + call_instances = [] + call_events = [] self.assertEqual(set(expected_uuids), set([instance.uuid for instance in call_instances])) @@ -157,11 +161,14 @@ def test_create_event_instance_has_no_host(self): self.assertEqual(207, code) def test_create_no_good_instances(self): + """Always 207 with granular codes even if all fail; see bug 1855752.""" body = self.default_body body['events'][0]['server_uuid'] = MISSING_UUID - body['events'][1]['server_uuid'] = MISSING_UUID - self.assertRaises(webob.exc.HTTPNotFound, - self.api.create, self.req, body=body) + body['events'][1]['server_uuid'] = fake_instance_uuids[-1] + result, code = self._assert_call(body, [], []) + self.assertEqual(404, result['events'][0]['code']) + self.assertEqual(422, result['events'][1]['code']) + self.assertEqual(207, code) def test_create_bad_status(self): body = self.default_body