From 6e2cb60e778180a40418dbc2011fa25763c32363 Mon Sep 17 00:00:00 2001 From: Julia Kreger Date: Thu, 9 Jul 2020 11:07:18 -0700 Subject: [PATCH] Respond so Apache HTTPd doesn't think the request failed When sending a literal empty response, Flask does not include a ContentType in the response. While in many cases, we don't need need a ContentType nor expect one on the API client, Apache webserver can treat this as an error and generate an Error indicating a Bad Gateway. When doing this, we also now include an empty JSON body in the response for 202 messages. For 204 message errors, the message body is expected to be empty. However, when this Bad Gateway error occurs, the API/Conductor were proceeding like there was no issue. The API client on the other hand thinks that a hard failure has occured. Also adds some additional catches to provide additional logging which turned out not to be needed in this case, but it would be useful for others. Change-Id: If2e7697e3fde58ab0a4193787e29d3acdca81ebf --- ironic_inspector/introspect.py | 6 +++- ironic_inspector/main.py | 35 ++++++++++++++++--- ironic_inspector/node_cache.py | 2 ++ ironic_inspector/test/functional.py | 6 ++-- ironic_inspector/test/unit/test_main.py | 16 +++++++-- lower-constraints.txt | 2 +- ...ly-with-content-type-644b741261c87c8c.yaml | 9 +++++ requirements.txt | 2 +- 8 files changed, 65 insertions(+), 13 deletions(-) create mode 100644 releasenotes/notes/reply-with-content-type-644b741261c87c8c.yaml diff --git a/ironic_inspector/introspect.py b/ironic_inspector/introspect.py index 39d2b8ea4..45b62cea6 100644 --- a/ironic_inspector/introspect.py +++ b/ironic_inspector/introspect.py @@ -63,7 +63,11 @@ def introspect(node_id, manage_boot=True, token=None): ironic=ironic) if manage_boot: - utils.executor().submit(_do_introspect, node_info, ironic) + try: + utils.executor().submit(_do_introspect, node_info, ironic) + except Exception as exc: + msg = _('Failed to submit introspection job: %s') + raise utils.Error(msg % exc, node_info=node) else: _do_introspect(node_info, ironic) diff --git a/ironic_inspector/main.py b/ironic_inspector/main.py index e5beb0e65..054186e32 100644 --- a/ironic_inspector/main.py +++ b/ironic_inspector/main.py @@ -130,6 +130,31 @@ def error_response(exc, code=500): return res +def _generate_empty_response(code): + """Change the content mime type to text/plain. + + :param code: The HTTP response code as an integer. + :returns: An empty flask response object with the + requested return code. + """ + # NOTE(TheJulia): Explicitly set a mime type and body on the + # response as some proxies view the lack of a mime type as a + # failure when the request was actually successful. + # Strictly speaking, 204s should have no body, where as 202's + # don't strictly require or expect content, but content can + # be included for user friendly response bodies. + if code == 204: + response = flask.make_response('', code) + response.mimetype = 'text/plain' + else: + # Send an empty dictionary to set a mimetype, and ultimately + # with this being a rest API we can, at some point, choose to + # convey some sort of status response back in the message + # body. + response = flask.make_response({}, code) + return response + + def convert_exceptions(func): @functools.wraps(func) def wrapper(*args, **kwargs): @@ -333,7 +358,7 @@ def api_introspection(node_id): client.call({}, 'do_introspection', node_id=node_id, manage_boot=manage_boot, token=flask.request.headers.get('X-Auth-Token')) - return '', 202 + return _generate_empty_response(202) else: node_info = node_cache.get_node(node_id) return flask.json.jsonify(generate_introspection_status(node_info)) @@ -358,7 +383,7 @@ def api_introspection_abort(node_id): client = get_client_compat() client.call({}, 'do_abort', node_id=node_id, token=flask.request.headers.get('X-Auth-Token')) - return '', 202 + return _generate_empty_response(202) @api('/v1/introspection//data', rule="introspection:data", @@ -399,7 +424,7 @@ def api_introspection_reapply(node_id): client = get_client_compat() client.call({}, 'do_reapply', node_uuid=node_id, data=data) - return '', 202 + return _generate_empty_response(202) def rule_repr(rule, short): @@ -421,7 +446,7 @@ def api_rules(): return flask.jsonify(rules=res) elif flask.request.method == 'DELETE': rules.delete_all() - return '', 204 + return _generate_empty_response(204) else: body = flask.request.get_json(force=True) if body.get('uuid') and not uuidutils.is_uuid_like(body['uuid']): @@ -452,7 +477,7 @@ def api_rule(uuid): return flask.jsonify(rule_repr(rule, short=False)) else: rules.delete(uuid) - return '', 204 + return _generate_empty_response(204) @_app.errorhandler(404) diff --git a/ironic_inspector/node_cache.py b/ironic_inspector/node_cache.py index 3c75509b0..7b0616939 100644 --- a/ironic_inspector/node_cache.py +++ b/ironic_inspector/node_cache.py @@ -100,6 +100,8 @@ class NodeInfo(object): :returns: boolean value, whether lock was acquired successfully """ if self._lock.is_locked(): + LOG.debug('Attempting to acquire lock already held', + node_info=self) return True LOG.debug('Attempting to acquire lock', node_info=self) diff --git a/ironic_inspector/test/functional.py b/ironic_inspector/test/functional.py index 60531a183..38d3d55e2 100644 --- a/ironic_inspector/test/functional.py +++ b/ironic_inspector/test/functional.py @@ -629,7 +629,7 @@ class Test(Base): res = self.call_reapply(self.uuid) self.assertEqual(202, res.status_code) - self.assertEqual('', res.text) + self.assertEqual('{}\n', res.text) eventlet.greenthread.sleep(DEFAULT_SLEEP) status = self.call_get_status(self.uuid) @@ -642,7 +642,7 @@ class Test(Base): # second reapply call res = self.call_reapply(self.uuid) self.assertEqual(202, res.status_code) - self.assertEqual('', res.text) + self.assertEqual('{}\n', res.text) eventlet.greenthread.sleep(DEFAULT_SLEEP) # Reapply with provided data @@ -650,7 +650,7 @@ class Test(Base): new_data['inventory']['cpu']['count'] = 42 res = self.call_reapply(self.uuid, data=new_data) self.assertEqual(202, res.status_code) - self.assertEqual('', res.text) + self.assertEqual('{}\n', res.text) eventlet.greenthread.sleep(DEFAULT_SLEEP) self.check_status(status, finished=True, state=istate.States.finished) diff --git a/ironic_inspector/test/unit/test_main.py b/ironic_inspector/test/unit/test_main.py index d1377f303..b826f3b08 100644 --- a/ironic_inspector/test/unit/test_main.py +++ b/ironic_inspector/test/unit/test_main.py @@ -75,6 +75,9 @@ class TestApiIntrospect(BaseAPITest): node_id=self.uuid, manage_boot=True, token=None) + self.assertEqual('application/json', + res.headers['content-type']) + self.assertEqual(b'{}\n', res.data) def test_intospect_failed(self): self.client_mock.call.side_effect = utils.Error("boom") @@ -98,6 +101,9 @@ class TestApiIntrospect(BaseAPITest): node_id=self.uuid, manage_boot=False, token=None) + self.assertEqual('application/json', + res.headers['content-type']) + self.assertEqual(b'{}\n', res.data) def test_introspect_can_manage_boot_false(self): CONF.set_override('can_manage_boot', False) @@ -196,7 +202,9 @@ class TestApiAbort(BaseAPITest): node_id=self.uuid, token='token') self.assertEqual(202, res.status_code) - self.assertEqual(b'', res.data) + self.assertEqual(b'{}\n', res.data) + self.assertEqual('application/json', + res.headers['content-type']) def test_no_authentication(self): @@ -206,7 +214,9 @@ class TestApiAbort(BaseAPITest): node_id=self.uuid, token=None) self.assertEqual(202, res.status_code) - self.assertEqual(b'', res.data) + self.assertEqual(b'{}\n', res.data) + self.assertEqual('application/json', + res.headers['content-type']) def test_node_not_found(self): exc = utils.Error("Not Found.", code=404) @@ -559,6 +569,8 @@ class TestApiRules(BaseAPITest): res = self.app.delete('/v1/rules/' + self.uuid) self.assertEqual(204, res.status_code) delete_mock.assert_called_once_with(self.uuid) + self.assertEqual('text/plain; charset=utf-8', + res.headers['content-type']) class TestApiMisc(BaseAPITest): diff --git a/lower-constraints.txt b/lower-constraints.txt index e3d018260..4f1c22864 100644 --- a/lower-constraints.txt +++ b/lower-constraints.txt @@ -23,7 +23,7 @@ eventlet==0.18.2 extras==1.0.0 fasteners==0.15 fixtures==3.0.0 -Flask==1.0 +Flask==1.1.0 future==0.18.2 futurist==1.2.0 gitdb==4.0.5 diff --git a/releasenotes/notes/reply-with-content-type-644b741261c87c8c.yaml b/releasenotes/notes/reply-with-content-type-644b741261c87c8c.yaml new file mode 100644 index 000000000..347cc4e66 --- /dev/null +++ b/releasenotes/notes/reply-with-content-type-644b741261c87c8c.yaml @@ -0,0 +1,9 @@ +--- +fixes: + - | + Fixes an issue which may occur with Apache httpd webservers acting as a + proxy where the server may report ``Bad Gateway``, however inspector + continues operating as if there was no problem. This was due to a + lack of a ``Content-Type`` header on HTTP 202 and 204 replies, + and lack of message body with HTTP 202 messages which Apache httpd + can error upon. diff --git a/requirements.txt b/requirements.txt index 27b08bbd9..862e81790 100644 --- a/requirements.txt +++ b/requirements.txt @@ -5,7 +5,7 @@ automaton>=1.9.0 # Apache-2.0 alembic>=0.9.6 # MIT construct>=2.9.39 # MIT eventlet!=0.18.3,!=0.20.1,>=0.18.2 # MIT -Flask>=1.0 # BSD +Flask>=1.1.0 # BSD futurist>=1.2.0 # Apache-2.0 ironic-lib>=4.3.0 # Apache-2.0 jsonpath-rw<2.0,>=1.2.0 # Apache-2.0