From d04c9d8a0cd2f74199548e411f91bf0038cd707c Mon Sep 17 00:00:00 2001 From: Lucas Moura Date: Mon, 30 Oct 2023 17:25:44 -0300 Subject: [PATCH] Merge UrlError and ConnectivityError exceptions Since ConnectityError is only raised when we detect an UrlError exception, we have decided to merge those exceptios. Additionally, we have also updated the ConnectivityError message to be more explicit on what the actual error is. Fixes: #2647 --- features/unattached_status.feature | 36 ++++++++-------- lib/reboot_cmds.py | 2 +- uaclient/actions.py | 11 +++-- .../test_api_u_pro_attach_magic_wait_v1.py | 16 +++---- uaclient/cli/__init__.py | 16 ++++--- uaclient/cli/tests/test_cli.py | 11 +++-- uaclient/cli/tests/test_cli_auto_attach.py | 4 +- uaclient/cli/tests/test_cli_refresh.py | 4 +- uaclient/cli/tests/test_cli_status.py | 12 +++--- uaclient/contract.py | 17 +++----- uaclient/daemon/retry_auto_attach.py | 4 +- .../daemon/tests/test_retry_auto_attach.py | 8 ++-- uaclient/exceptions.py | 43 +++++++++++-------- uaclient/http/__init__.py | 3 +- uaclient/messages/__init__.py | 7 +-- uaclient/tests/test_actions.py | 6 +-- uaclient/tests/test_contract.py | 35 +++++++++++---- uaclient/tests/test_livepatch.py | 7 ++- 18 files changed, 140 insertions(+), 102 deletions(-) diff --git a/features/unattached_status.feature b/features/unattached_status.feature index 00306ef145..a958468246 100644 --- a/features/unattached_status.feature +++ b/features/unattached_status.feature @@ -9,26 +9,28 @@ Feature: Unattached status When I run `sed -i 's/contracts.can/invalidurl.notcan/' /etc/ubuntu-advantage/uaclient.conf` with sudo And I verify that running `pro status --format json` `as non-root` exits `1` Then stdout is a json matching the `ua_status` schema - And I will see the following on stdout: - """ - {"environment_vars": [], "errors": [{"message": "Failed to connect to authentication server\nCheck your Internet connection and try again.", "message_code": "connectivity-error", "service": null, "type": "system"}], "result": "failure", "services": [], "warnings": []} - """ + And stdout matches regexp: + """ + {"environment_vars": \[\], "errors": \[{"message": "Failed to connect to .*\\n\[Errno -2\] Name or service not known\\n", "message_code": "connectivity-error", "service": null, "type": "system"}\], "result": "failure", "services": \[\], "warnings": \[\]} + """ And I verify that running `pro status --format yaml` `as non-root` exits `1` Then stdout is a yaml matching the `ua_status` schema - And I will see the following on stdout: - """ - environment_vars: [] - errors: - - message: 'Failed to connect to authentication server + And stdout matches regexp: + """ + environment_vars: \[\] + errors: + - message: 'Failed to connect to .* - Check your Internet connection and try again.' - message_code: connectivity-error - service: null - type: system - result: failure - services: [] - warnings: [] - """ + \[Errno -2\] Name or service not known + + ' + message_code: connectivity-error + service: null + type: system + result: failure + services: \[\] + warnings: \[\] + """ Examples: ubuntu release | release | machine_type | diff --git a/lib/reboot_cmds.py b/lib/reboot_cmds.py index 8dc0569167..40e121e340 100644 --- a/lib/reboot_cmds.py +++ b/lib/reboot_cmds.py @@ -66,7 +66,7 @@ def fix_pro_pkg_holds(cfg: config.UAConfig): def refresh_contract(cfg: config.UAConfig): try: contract.refresh(cfg) - except exceptions.UrlError: + except exceptions.ConnectivityError: LOG.warning("Failed to refresh contract") raise diff --git a/uaclient/actions.py b/uaclient/actions.py index 7f4deaff90..d080c8b9dc 100644 --- a/uaclient/actions.py +++ b/uaclient/actions.py @@ -50,7 +50,7 @@ def attach_with_token( ) -> None: """ Common functionality to take a token and attach via contract backend - :raise UrlError: On unexpected connectivity issues to contract + :raise ConnectivityError: On unexpected connectivity issues to contract server or inability to access identity doc from metadata service. :raise ContractAPIError: On unexpected errors when talking to the contract server. @@ -64,9 +64,8 @@ def attach_with_token( new_machine_token = contract_client.add_contract_machine( contract_token=token, attachment_dt=attached_at ) - except exceptions.UrlError as e: - LOG.exception(str(e)) - raise exceptions.ConnectivityError() + except exceptions.ConnectivityError as e: + raise e cfg.machine_token_file.write(new_machine_token) @@ -86,7 +85,7 @@ def attach_with_token( cfg.machine_token_file.entitlements, allow_enable, ) - except (exceptions.UrlError, exceptions.UbuntuProError) as exc: + except (exceptions.ConnectivityError, exceptions.UbuntuProError) as exc: # Persist updated status in the event of partial attach attachment_data_file.write(AttachmentData(attached_at=attached_at)) ua_status.status(cfg=cfg) @@ -109,7 +108,7 @@ def auto_attach( allow_enable=True, ) -> None: """ - :raise UrlError: On unexpected connectivity issues to contract + :raise ConnectivityError: On unexpected connectivity issues to contract server or inability to access identity doc from metadata service. :raise ContractAPIError: On unexpected errors when talking to the contract server. diff --git a/uaclient/api/tests/test_api_u_pro_attach_magic_wait_v1.py b/uaclient/api/tests/test_api_u_pro_attach_magic_wait_v1.py index aa65d04c45..c6f22f4cdf 100644 --- a/uaclient/api/tests/test_api_u_pro_attach_magic_wait_v1.py +++ b/uaclient/api/tests/test_api_u_pro_attach_magic_wait_v1.py @@ -67,10 +67,10 @@ def test_wait_fails_after_number_of_connectiviry_errors( ): magic_token = "test-id" m_attach_token_info.side_effect = [ - exceptions.ConnectivityError(), - exceptions.ConnectivityError(), - exceptions.ConnectivityError(), - exceptions.ConnectivityError(), + exceptions.ConnectivityError(url="url", cause="cause"), + exceptions.ConnectivityError(url="url", cause="cause"), + exceptions.ConnectivityError(url="url", cause="cause"), + exceptions.ConnectivityError(url="url", cause="cause"), ] options = MagicAttachWaitOptions(magic_token=magic_token) @@ -86,9 +86,9 @@ def test_wait_succeeds_after_number_of_connectivity_errors( ): magic_token = "test-id" m_attach_token_info.side_effect = [ - exceptions.ConnectivityError(), - exceptions.ConnectivityError(), - exceptions.ConnectivityError(), + exceptions.ConnectivityError(url="url", cause="cause"), + exceptions.ConnectivityError(url="url", cause="cause"), + exceptions.ConnectivityError(url="url", cause="cause"), { "token": magic_token, "expires": "2100-06-09T18:14:55.323733Z", @@ -113,7 +113,7 @@ def test_wait_succeeds_after_unavailable_server( ): magic_token = "test-id" m_attach_token_info.side_effect = [ - exceptions.ConnectivityError(), + exceptions.ConnectivityError(url="url", cause="cause"), exceptions.MagicAttachUnavailable(), exceptions.MagicAttachUnavailable(), { diff --git a/uaclient/cli/__init__.py b/uaclient/cli/__init__.py index 1d02e01ad5..976f5a5cd1 100644 --- a/uaclient/cli/__init__.py +++ b/uaclient/cli/__init__.py @@ -1065,7 +1065,7 @@ def action_enable(args, *, cfg, **kwargs): event.info(messages.REFRESH_CONTRACT_ENABLE) try: contract.refresh(cfg) - except (exceptions.UrlError, exceptions.UbuntuProError): + except (exceptions.ConnectivityError, exceptions.UbuntuProError): # Inability to refresh is not a critical issue during enable LOG.warning("Failed to refresh contract", exc_info=True) event.warning(warning_msg=messages.E_REFRESH_CONTRACT_FAILURE) @@ -1231,7 +1231,7 @@ def action_auto_attach(args, *, cfg: config.UAConfig, **kwargs) -> int: cfg=cfg, mode=event_logger.EventLoggerMode.CLI, ) - except exceptions.UrlError: + except exceptions.ConnectivityError: event.info(messages.E_ATTACH_FAILURE.msg) return 1 else: @@ -1301,7 +1301,7 @@ def action_attach(args, *, cfg, **kwargs): try: actions.attach_with_token(cfg, token=token, allow_enable=allow_enable) - except exceptions.UrlError: + except exceptions.ConnectivityError: raise exceptions.AttachError() else: ret = 0 @@ -1527,8 +1527,7 @@ def _action_refresh_config(args, cfg: config.UAConfig): def _action_refresh_contract(_args, cfg: config.UAConfig): try: contract.refresh(cfg) - except exceptions.UrlError as exc: - LOG.exception(exc) + except exceptions.ConnectivityError: raise exceptions.RefreshContractFailure() print(messages.REFRESH_CONTRACT_SUCCESS) @@ -1707,7 +1706,7 @@ def wrapper(*args, **kwargs): print(messages.CLI_INTERRUPT_RECEIVED, file=sys.stderr) lock.clear_lock_file_if_present() sys.exit(1) - except exceptions.UrlError as exc: + except exceptions.ConnectivityError as exc: if "CERTIFICATE_VERIFY_FAILED" in str(exc): tmpl = messages.SSL_VERIFICATION_ERROR_CA_CERTIFICATES if apt.is_installed("ca-certificates"): @@ -1720,7 +1719,10 @@ def wrapper(*args, **kwargs): "Failed to access URL: %s", exc.url, exc_info=exc ) - msg = messages.E_CONNECTIVITY_ERROR + msg = messages.E_CONNECTIVITY_ERROR.format( + url=exc.url, + cause_error=exc.cause_error, + ) event.error(error_msg=msg.msg, error_code=msg.name) event.info(info_msg=msg.msg, file_type=sys.stderr) diff --git a/uaclient/cli/tests/test_cli.py b/uaclient/cli/tests/test_cli.py index 79d84ff99a..489fd5b84d 100644 --- a/uaclient/cli/tests/test_cli.py +++ b/uaclient/cli/tests/test_cli.py @@ -645,8 +645,9 @@ def test_url_error_handled_gracefully( expected_log_call, ): m_args = m_get_parser.return_value.parse_args.return_value - m_args.action.side_effect = exceptions.UrlError( - socket.gaierror(-2, "Name or service not known"), url=error_url + m_args.action.side_effect = exceptions.ConnectivityError( + cause=socket.gaierror(-2, "Name or service not known"), + url=error_url, ) with pytest.raises(SystemExit) as excinfo: @@ -657,7 +658,11 @@ def test_url_error_handled_gracefully( assert [ mock.call( - info_msg=messages.E_CONNECTIVITY_ERROR.msg, file_type=mock.ANY + info_msg=messages.E_CONNECTIVITY_ERROR.format( + url=error_url, + cause_error="[Errno -2] Name or service not known", + ).msg, + file_type=mock.ANY, ) ] == m_event_info.call_args_list assert [expected_log_call] == m_log_exception.call_args_list diff --git a/uaclient/cli/tests/test_cli_auto_attach.py b/uaclient/cli/tests/test_cli_auto_attach.py index 5c72539ce9..a56517c68e 100644 --- a/uaclient/cli/tests/test_cli_auto_attach.py +++ b/uaclient/cli/tests/test_cli_auto_attach.py @@ -85,7 +85,9 @@ def test_happy_path( "api_side_effect,expected_err,expected_ret", ( ( - exceptions.UrlError("does-not-matter", "url"), + exceptions.ConnectivityError( + cause=Exception("does-not-matter"), url="url" + ), messages.E_ATTACH_FAILURE.msg, 1, ), diff --git a/uaclient/cli/tests/test_cli_refresh.py b/uaclient/cli/tests/test_cli_refresh.py index 32facff7c7..b595318fc8 100644 --- a/uaclient/cli/tests/test_cli_refresh.py +++ b/uaclient/cli/tests/test_cli_refresh.py @@ -100,7 +100,9 @@ def test_refresh_contract_error_on_failure_to_update_contract( FakeConfig, ): """On failure in request_updates_contract emit an error.""" - refresh.side_effect = exceptions.UrlError(mock.MagicMock(), "url") + refresh.side_effect = exceptions.ConnectivityError( + mock.MagicMock(), "url" + ) cfg = FakeConfig.for_attached_machine() diff --git a/uaclient/cli/tests/test_cli_status.py b/uaclient/cli/tests/test_cli_status.py index 9142179b7b..4085cb8b88 100644 --- a/uaclient/cli/tests/test_cli_status.py +++ b/uaclient/cli/tests/test_cli_status.py @@ -958,14 +958,14 @@ def test_error_on_connectivity_errors( _m_on_supported_kernel, FakeConfig, ): - """Raise UrlError on connectivity issues""" - m_get_avail_resources.side_effect = exceptions.UrlError( - socket.gaierror(-2, "Name or service not known"), "url" + """Raise ConnectivityError on connectivity issues""" + m_get_avail_resources.side_effect = exceptions.ConnectivityError( + cause=socket.gaierror(-2, "Name or service not known"), url="url" ) cfg = FakeConfig() - with pytest.raises(exceptions.UrlError): + with pytest.raises(exceptions.ConnectivityError): action_status( mock.MagicMock(all=False, simulate_with_token=None), cfg=cfg ) @@ -1023,8 +1023,8 @@ def test_unicode_dash_replacement_when_unprintable( "exception_to_throw,exception_type,exception_message", ( ( - exceptions.UrlError("Not found", "url"), - exceptions.UrlError, + exceptions.ConnectivityError(Exception("Not found"), "url"), + exceptions.ConnectivityError, "Not found", ), ( diff --git a/uaclient/contract.py b/uaclient/contract.py index 0418956d97..3e8a59556d 100644 --- a/uaclient/contract.py +++ b/uaclient/contract.py @@ -264,9 +264,8 @@ def get_magic_attach_token_info(self, magic_token: str) -> Dict[str, Any]: response = self.request_url( API_V1_GET_MAGIC_ATTACH_TOKEN_INFO, headers=headers ) - except exceptions.UrlError as e: - LOG.exception(e) - raise exceptions.ConnectivityError() + except exceptions.ConnectivityError as e: + raise e if response.code == 401: raise exceptions.MagicAttachTokenError() if response.code == 503: @@ -290,9 +289,8 @@ def new_magic_attach_token(self) -> Dict[str, Any]: headers=headers, method="POST", ) - except exceptions.UrlError as e: - LOG.exception(e) - raise exceptions.ConnectivityError() + except exceptions.ConnectivityError as e: + raise e if response.code == 503: raise exceptions.MagicAttachUnavailable() if response.code != 200: @@ -315,9 +313,8 @@ def revoke_magic_attach_token(self, magic_token: str): headers=headers, method="DELETE", ) - except exceptions.UrlError as e: - LOG.exception(e) - raise exceptions.ConnectivityError() + except exceptions.ConnectivityError as e: + raise e if response.code == 400: raise exceptions.MagicAttachTokenAlreadyActivated() if response.code == 401: @@ -659,7 +656,7 @@ def refresh(cfg): :raise UbuntuProError: on failure to update contract or error processing contract deltas - :raise UrlError: On failure during a connection + :raise ConnectivityError: On failure during a connection """ orig_entitlements = cfg.machine_token_file.entitlements orig_token = cfg.machine_token diff --git a/uaclient/daemon/retry_auto_attach.py b/uaclient/daemon/retry_auto_attach.py index 5e4df97907..3960cf9de6 100644 --- a/uaclient/daemon/retry_auto_attach.py +++ b/uaclient/daemon/retry_auto_attach.py @@ -52,11 +52,9 @@ def full_auto_attach_exception_to_failure_reason(e: Exception) -> str: error_msg=e.body ) elif isinstance(e, api_exceptions.ConnectivityError): - return messages.RETRY_ERROR_DETAIL_CONNECTIVITY_ERROR - elif isinstance(e, api_exceptions.UrlError): return messages.RETRY_ERROR_DETAIL_URL_ERROR_URL.format( url=e.url - ) + ': "{}"'.format(str(e)) + ) + ': "{}"'.format(str(e.cause_error)) elif isinstance(e, api_exceptions.UbuntuProError): return '"{}"'.format(e.msg) else: diff --git a/uaclient/daemon/tests/test_retry_auto_attach.py b/uaclient/daemon/tests/test_retry_auto_attach.py index 5be81d687e..720495da08 100644 --- a/uaclient/daemon/tests/test_retry_auto_attach.py +++ b/uaclient/daemon/tests/test_retry_auto_attach.py @@ -43,11 +43,9 @@ class TestFullAutoAttachToFailureReason: 'an error from Canonical servers: "response"', ), ( - exceptions.ConnectivityError(), - "a connectivity error", - ), - ( - exceptions.UrlError(error.URLError("urlerror"), "url"), + exceptions.ConnectivityError( + cause=error.URLError("urlerror"), url="url" + ), 'an error while reaching url: "urlerror"', ), (fakes.FakeUbuntuProError(), '"This is a test"'), diff --git a/uaclient/exceptions.py b/uaclient/exceptions.py index 38d7c899b5..9a250a974c 100644 --- a/uaclient/exceptions.py +++ b/uaclient/exceptions.py @@ -19,20 +19,6 @@ class DelayProLicensePolling(IsProLicensePresentError): pass -class UrlError(IOError): - def __init__( - self, - cause: Exception, - url: str, - ): - if getattr(cause, "reason", None): - cause_error = str(getattr(cause, "reason")) - else: - cause_error = str(cause) - super().__init__(cause_error) - self.url = url - - class ProcessExecutionError(IOError): def __init__( self, @@ -206,10 +192,6 @@ class ProxyAuthenticationFailed(UbuntuProError): _msg = messages.E_PROXY_AUTH_FAIL -class ConnectivityError(UbuntuProError): - _msg = messages.E_CONNECTIVITY_ERROR - - class ExternalAPIError(UbuntuProError): _formatted_msg = messages.E_EXTERNAL_API_ERROR code = None # type: int @@ -236,6 +218,31 @@ def __init__(self, url, **kwargs) -> None: self.url = url +class ConnectivityError(UbuntuProError, IOError): + _formatted_msg = messages.E_CONNECTIVITY_ERROR + + def __init__( + self, + cause: Exception, + url: str, + ): + if getattr(cause, "reason", None): + cause_error = str(getattr(cause, "reason")) + else: + cause_error = str(cause) + IOError.__init__(self, cause_error) + UbuntuProError.__init__(self, cause_error=cause_error, url=url) + + # Even though we already set those variable through UbuntuProError + # we need to set them again to avoid mypy warnings + self.cause_error = cause_error + self.url = url + + +# We are doing that just to keep backwards compatibility +# for our custom UrlError exception +UrlError = ConnectivityError + ############################################################################### # ATTACH/ENABLE # ############################################################################### diff --git a/uaclient/http/__init__.py b/uaclient/http/__init__.py index 9863ac033e..0ebfc1547d 100644 --- a/uaclient/http/__init__.py +++ b/uaclient/http/__init__.py @@ -168,7 +168,8 @@ def _readurl_urllib( except error.HTTPError as e: resp = e except error.URLError as e: - raise exceptions.UrlError(e, url=req.full_url) + LOG.exception(str(e.reason)) + raise exceptions.ConnectivityError(cause=e, url=req.full_url) body = resp.read().decode("utf-8") diff --git a/uaclient/messages/__init__.py b/uaclient/messages/__init__.py index af4b60ce32..8e1d0ff5c2 100644 --- a/uaclient/messages/__init__.py +++ b/uaclient/messages/__init__.py @@ -2031,12 +2031,13 @@ def __repr__(self): "proxy-auth-fail", t.gettext("Proxy authentication failed") ) -E_CONNECTIVITY_ERROR = NamedMessage( +E_CONNECTIVITY_ERROR = FormattedNamedMessage( "connectivity-error", t.gettext( """\ -Failed to connect to authentication server -Check your Internet connection and try again.""" +Failed to connect to {url} +{cause_error} +""" ), ) diff --git a/uaclient/tests/test_actions.py b/uaclient/tests/test_actions.py index cce0266672..25926653ae 100644 --- a/uaclient/tests/test_actions.py +++ b/uaclient/tests/test_actions.py @@ -43,7 +43,7 @@ class TestAttachWithToken: ( "token", True, - exceptions.UrlError(Exception(), "url"), + exceptions.ConnectivityError(cause=Exception(), url="url"), None, None, None, @@ -67,7 +67,7 @@ class TestAttachWithToken: [{"machineTokenInfo": {"machineId": "machine-id"}}], "get-machine-id-result", mock.sentinel.entitlements, - exceptions.UrlError(Exception(), "url"), + exceptions.ConnectivityError(cause=Exception(), url="url"), None, [mock.call(contract_token="token", attachment_dt=mock.ANY)], [mock.call({"machineTokenInfo": {"machineId": "machine-id"}})], @@ -80,7 +80,7 @@ class TestAttachWithToken: [mock.call()], [], [], - pytest.raises(exceptions.UrlError), + pytest.raises(exceptions.ConnectivityError), ), ( "token", diff --git a/uaclient/tests/test_contract.py b/uaclient/tests/test_contract.py index 8f7c6141e7..905610cd20 100644 --- a/uaclient/tests/test_contract.py +++ b/uaclient/tests/test_contract.py @@ -528,9 +528,12 @@ def test_new_magic_attach_token_successfull( "request_side_effect,expected_exception,message", ( ( - exceptions.UrlError("cause", "url"), + exceptions.ConnectivityError(cause="cause", url="url"), exceptions.ConnectivityError, - messages.E_CONNECTIVITY_ERROR, + messages.E_CONNECTIVITY_ERROR.format( + url="url", + cause_error="cause", + ), ), ( [ @@ -604,12 +607,20 @@ def test_request_magic_attach_id_info_fails( cfg = FakeConfig() client = UAContractClient(cfg) magic_token = "test-id" - request_url.side_effect = exceptions.UrlError("cause", "url") + request_url.side_effect = exceptions.ConnectivityError( + cause=Exception("cause"), url="url" + ) with pytest.raises(exceptions.ConnectivityError) as exc_error: client.get_magic_attach_token_info(magic_token=magic_token) - assert messages.E_CONNECTIVITY_ERROR.msg == exc_error.value.msg + assert ( + messages.E_CONNECTIVITY_ERROR.format( + url="url", + cause_error="cause", + ).msg + == exc_error.value.msg + ) assert messages.E_CONNECTIVITY_ERROR.name == exc_error.value.msg_code @pytest.mark.parametrize( @@ -651,12 +662,20 @@ def test_revoke_magic_attach_token_fails( cfg = FakeConfig() client = UAContractClient(cfg) magic_token = "test-id" - request_url.side_effect = exceptions.UrlError("cause", "url") + request_url.side_effect = exceptions.ConnectivityError( + cause=Exception("cause"), url="url" + ) with pytest.raises(exceptions.ConnectivityError) as exc_error: client.revoke_magic_attach_token(magic_token=magic_token) - assert messages.E_CONNECTIVITY_ERROR.msg == exc_error.value.msg + assert ( + messages.E_CONNECTIVITY_ERROR.format( + url="url", + cause_error="cause", + ).msg + == exc_error.value.msg + ) assert messages.E_CONNECTIVITY_ERROR.name == exc_error.value.msg_code @pytest.mark.parametrize( @@ -1161,12 +1180,12 @@ def test_available_resources_error_on_network_disconnected( """Raise error get_available_resources can't contact backend""" cfg = FakeConfig() - urlerror = exceptions.UrlError( + urlerror = exceptions.ConnectivityError( socket.gaierror(-2, "Name or service not known"), "url" ) m_available_resources.side_effect = urlerror - with pytest.raises(exceptions.UrlError) as exc: + with pytest.raises(exceptions.ConnectivityError) as exc: get_available_resources(cfg) assert urlerror == exc.value diff --git a/uaclient/tests/test_livepatch.py b/uaclient/tests/test_livepatch.py index 7713e0948d..a2ecc700fe 100644 --- a/uaclient/tests/test_livepatch.py +++ b/uaclient/tests/test_livepatch.py @@ -327,7 +327,12 @@ def test_is_kernel_supported_calls_api_with_correct_params( ], LivepatchSupport.UNSUPPORTED, ), - (exceptions.UrlError(mock.MagicMock(), "url"), None), + ( + exceptions.ConnectivityError( + cause=mock.MagicMock(), url="url" + ), + None, + ), (Exception(), None), ( [