From db87ff79125b6b1bfb575e651fb1cae5921e894d Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Tue, 19 Dec 2023 13:54:47 +0100 Subject: [PATCH] Handle exceptions after re-authentication Currently, no handling is done for the 2nd request (we catch HTTPError, but it won't be raised without raise_for_exceptions). Recurse into the same call instead. Change-Id: I9d593a741344fd7d5d6d59eca48a12222572c1ce --- .../notes/re-raise-1fe9f912691e893e.yaml | 5 +++ sushy/connector.py | 26 ++++++------ sushy/tests/unit/test_connector.py | 40 +++++++++++++++++++ 3 files changed, 58 insertions(+), 13 deletions(-) create mode 100644 releasenotes/notes/re-raise-1fe9f912691e893e.yaml diff --git a/releasenotes/notes/re-raise-1fe9f912691e893e.yaml b/releasenotes/notes/re-raise-1fe9f912691e893e.yaml new file mode 100644 index 00000000..fdb8c3b3 --- /dev/null +++ b/releasenotes/notes/re-raise-1fe9f912691e893e.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - | + Correctly handles errors on a request that is re-tried after refreshing + an authentication token. diff --git a/sushy/connector.py b/sushy/connector.py index b72322c1..3103239a 100644 --- a/sushy/connector.py +++ b/sushy/connector.py @@ -107,7 +107,7 @@ class Connector(object): return retry def _op(self, method, path='', data=None, headers=None, blocking=False, - timeout=60, server_side_retries_left=None, + timeout=60, server_side_retries_left=None, allow_reauth=True, **extra_session_req_kwargs): """Generic RESTful request handler. @@ -124,6 +124,8 @@ class Connector(object): those calls. :param server_side_retries_left: Remaining retries. If not provided will use limit provided by instance's server_side_retries + :param allow_reauth: Whether to allow refreshing the authentication + token. :param extra_session_req_kwargs: Optional keyword argument to pass requests library arguments which would pass on to requests session object. @@ -184,6 +186,10 @@ class Connector(object): LOG.error('Authentication to the session service failed. ' 'Please check credentials and try again.') raise + if not allow_reauth: + LOG.error("Failure occured while attempting to retry " + "request after refreshing the session: %s", e) + raise if self._auth is not None: # self._session.auth value is only set when basic auth is used if self._session.auth is not None: @@ -210,18 +216,12 @@ class Connector(object): raise LOG.debug("Authentication refreshed successfully, " "retrying the call.") - try: - response = self._session.request( - method, url, json=data, - headers=headers, - verify=self._verify, - timeout=timeout, - **extra_session_req_kwargs) - except exceptions.HTTPError as retry_exc: - LOG.error("Failure occured while attempting to retry " - "request after refreshing the session. Error: " - "%s", retry_exc.message) - raise + return self._op( + method, path, data=data, headers=headers, + blocking=blocking, timeout=timeout, + server_side_retries_left=server_side_retries_left, + allow_reauth=False, + **extra_session_req_kwargs) else: if method == 'GET' and url.endswith('SessionService'): LOG.debug('HTTP GET of SessionService failed %s, ' diff --git a/sushy/tests/unit/test_connector.py b/sushy/tests/unit/test_connector.py index a18838de..16a7c780 100644 --- a/sushy/tests/unit/test_connector.py +++ b/sushy/tests/unit/test_connector.py @@ -335,6 +335,46 @@ class ConnectorOpTestCase(base.TestCase): self.auth.authenticate.assert_called_once() self.assertEqual(response.json, third_response.json) + def test_timed_out_session_fail_after_reestablish(self): + self.auth._session_key = 'asdf1234' + self.auth.get_session_key.return_value = 'asdf1234' + self.conn._auth = self.auth + self.session = mock.Mock(spec=requests.Session) + self.session.auth = None + self.conn._session = self.session + self.request = self.session.request + first_response = mock.MagicMock() + first_response.status_code = http_client.FORBIDDEN + second_response = mock.MagicMock() + second_response.status_code = http_client.BAD_REQUEST + self.request.side_effect = [first_response, second_response] + self.assertRaises(exceptions.BadRequestError, + self.conn._op, + 'POST', path='fake/path', data=self.data, + headers=self.headers) + self.auth.refresh_session.assert_called_with() + self.auth.can_refresh_session.assert_called_with() + + def test_timed_out_session_fail_after_reestablish_no_recursion(self): + self.auth._session_key = 'asdf1234' + self.auth.get_session_key.return_value = 'asdf1234' + self.conn._auth = self.auth + self.session = mock.Mock(spec=requests.Session) + self.session.auth = None + self.conn._session = self.session + self.request = self.session.request + first_response = mock.MagicMock() + first_response.status_code = http_client.FORBIDDEN + second_response = mock.MagicMock() + second_response.status_code = http_client.FORBIDDEN + self.request.side_effect = [first_response, second_response] + self.assertRaises(exceptions.AccessError, + self.conn._op, + 'POST', path='fake/path', data=self.data, + headers=self.headers) + self.auth.refresh_session.assert_called_with() + self.auth.can_refresh_session.assert_called_with() + def test_connection_error(self): self.request.side_effect = requests.exceptions.ConnectionError self.assertRaises(exceptions.ConnectionError, self.conn._op, 'GET')