openstack/ironic-inspector
Code Issues Proposed changes

Respond so Apache HTTPd doesn't think the request failed

Browse Source 
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
This commit is contained in:
Julia Kreger 2020-07-09 11:07:18 -07:00
parent a1f8926cd1
commit 6e2cb60e77
8 changed files with 65 additions and 13 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
ironic_inspector/introspect.py
View File

@ -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)

35
ironic_inspector/main.py
View File

@ -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/<node_id>/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)

2
ironic_inspector/node_cache.py
View File

@ -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)

6
ironic_inspector/test/functional.py
View File

@ -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)

16
ironic_inspector/test/unit/test_main.py
View File

@ -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):

2
lower-constraints.txt
View File

@ -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

9
releasenotes/notes/reply-with-content-type-644b741261c87c8c.yaml Normal file
View File

@ -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.

2
requirements.txt
View File

@ -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