From 8214d7a204b9b7eb2e07340e2551c45b636a8ce8 Mon Sep 17 00:00:00 2001 From: mostaffatarek124eru Date: Tue, 21 Jan 2025 13:11:23 +0000 Subject: [PATCH 1/5] GH #5445 --- cloudinit/config/cc_power_state_change.py | 5 +++- cloudinit/config/cc_rsyslog.py | 3 ++- cloudinit/config/cc_ubuntu_pro.py | 8 ++++-- pyproject.toml | 6 ----- .../config/test_cc_power_state_change.py | 2 +- tests/unittests/config/test_cc_rsyslog.py | 4 +-- tests/unittests/config/test_cc_ubuntu_pro.py | 25 +++++++++++-------- tools/.github-cla-signers | 1 + 8 files changed, 31 insertions(+), 23 deletions(-) diff --git a/cloudinit/config/cc_power_state_change.py b/cloudinit/config/cc_power_state_change.py index d0640d5111d..4bd9f8ccaa1 100644 --- a/cloudinit/config/cc_power_state_change.py +++ b/cloudinit/config/cc_power_state_change.py @@ -45,7 +45,10 @@ def givecmdline(pid): (output, _err) = subp.subp(["procstat", "-c", str(pid)]) line = output.splitlines()[1] m = re.search(r"\d+ (\w|\.|-)+\s+(/\w.+)", line) - return m.group(2) + if m: + return m.group(2) + else: + return None else: return util.load_text_file("/proc/%s/cmdline" % pid) except IOError: diff --git a/cloudinit/config/cc_rsyslog.py b/cloudinit/config/cc_rsyslog.py index 5b86a0d4d53..f522ee6c7ad 100644 --- a/cloudinit/config/cc_rsyslog.py +++ b/cloudinit/config/cc_rsyslog.py @@ -246,6 +246,7 @@ def __init__( self.proto = proto self.addr = addr + self.port = int(port) if port else None if port: self.port = int(port) else: @@ -284,7 +285,7 @@ def __str__(self): else: buf += self.addr - if self.port: + if self.port is not None: buf += ":%s" % self.port if self.name: diff --git a/cloudinit/config/cc_ubuntu_pro.py b/cloudinit/config/cc_ubuntu_pro.py index 81acf0439d2..764af406c72 100644 --- a/cloudinit/config/cc_ubuntu_pro.py +++ b/cloudinit/config/cc_ubuntu_pro.py @@ -5,7 +5,7 @@ import json import logging import re -from typing import Any, List +from typing import Any, Dict, List from urllib.parse import urlparse from cloudinit import performance, subp, util @@ -196,6 +196,7 @@ def configure_pro(token, enable=None): try: enable_resp = json.loads(enable_stdout) + handle_enable_errors(enable_resp) except json.JSONDecodeError as e: raise RuntimeError( f"Pro response was not json: {enable_stdout}" @@ -225,7 +226,10 @@ def configure_pro(token, enable=None): # related. We can distinguish them by checking if `service` is non-null # or null respectively. - enable_errors: List[dict] = [] + +def handle_enable_errors(enable_resp: Dict[str, Any]): + + enable_errors: List[Dict[str, Any]] = [] for err in enable_resp.get("errors", []): if err["message_code"] == "service-already-enabled": LOG.debug("Service `%s` already enabled.", err["service"]) diff --git a/pyproject.toml b/pyproject.toml index 55b3c3bb054..594cea49681 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -53,9 +53,6 @@ module = [ "cloudinit.config.cc_ca_certs", "cloudinit.config.cc_growpart", "cloudinit.config.cc_ntp", - "cloudinit.config.cc_power_state_change", - "cloudinit.config.cc_rsyslog", - "cloudinit.config.cc_ubuntu_pro", "cloudinit.config.modules", "cloudinit.distros", "cloudinit.distros.alpine", @@ -134,19 +131,16 @@ module = [ "tests.unittests.config.test_cc_mcollective", "tests.unittests.config.test_cc_mounts", "tests.unittests.config.test_cc_phone_home", - "tests.unittests.config.test_cc_power_state_change", "tests.unittests.config.test_cc_puppet", "tests.unittests.config.test_cc_resizefs", "tests.unittests.config.test_cc_resolv_conf", "tests.unittests.config.test_cc_rh_subscription", - "tests.unittests.config.test_cc_rsyslog", "tests.unittests.config.test_cc_runcmd", "tests.unittests.config.test_cc_snap", "tests.unittests.config.test_cc_ssh", "tests.unittests.config.test_cc_timezone", "tests.unittests.config.test_cc_ubuntu_autoinstall", "tests.unittests.config.test_cc_ubuntu_drivers", - "tests.unittests.config.test_cc_ubuntu_pro", "tests.unittests.config.test_cc_update_etc_hosts", "tests.unittests.config.test_cc_users_groups", "tests.unittests.config.test_cc_wireguard", diff --git a/tests/unittests/config/test_cc_power_state_change.py b/tests/unittests/config/test_cc_power_state_change.py index 8a1886caf8d..ce8d74b2824 100644 --- a/tests/unittests/config/test_cc_power_state_change.py +++ b/tests/unittests/config/test_cc_power_state_change.py @@ -47,7 +47,7 @@ def test_empty_mode(self): self.assertRaises(TypeError, psc.load_power_state, cfg, self.dist) def test_valid_modes(self): - cfg = {"power_state": {}} + cfg: dict = {"power_state": {}} for mode in ("halt", "poweroff", "reboot"): cfg["power_state"]["mode"] = mode check_lps_ret(psc.load_power_state(cfg, self.dist), mode=mode) diff --git a/tests/unittests/config/test_cc_rsyslog.py b/tests/unittests/config/test_cc_rsyslog.py index ac662f4c6f5..0fb08130050 100644 --- a/tests/unittests/config/test_cc_rsyslog.py +++ b/tests/unittests/config/test_cc_rsyslog.py @@ -340,7 +340,7 @@ def test_install_rsyslog_on_freebsd(self, m_which): with mock.patch.object( cloud.distro, "install_packages" ) as m_install: - handle("rsyslog", {"rsyslog": config}, cloud, None) + handle("rsyslog", {"rsyslog": config}, cloud, []) m_which.assert_called_with(config["check_exe"]) m_install.assert_called_with(config["packages"]) @@ -356,6 +356,6 @@ def test_no_install_rsyslog_with_check_exe(self, m_which, m_isbsd): m_isbsd.return_value = False m_which.return_value = "/usr/sbin/rsyslogd" with mock.patch.object(cloud.distro, "install_packages") as m_install: - handle("rsyslog", {"rsyslog": config}, cloud, None) + handle("rsyslog", {"rsyslog": config}, cloud, []) m_which.assert_called_with(config["check_exe"]) m_install.assert_not_called() diff --git a/tests/unittests/config/test_cc_ubuntu_pro.py b/tests/unittests/config/test_cc_ubuntu_pro.py index 056a254202b..09d1d8ed57c 100644 --- a/tests/unittests/config/test_cc_ubuntu_pro.py +++ b/tests/unittests/config/test_cc_ubuntu_pro.py @@ -57,19 +57,24 @@ def fake_uaclient(mocker): m_uaclient = mock.Mock() sys.modules["uaclient"] = m_uaclient + mock_exceptions_module = mock.Mock() # Exceptions - _exceptions = namedtuple( - "exceptions", + Exceptions = namedtuple( + "Exceptions", [ "UserFacingError", "AlreadyAttachedError", ], - )( + ) + mock_exceptions_module.UserFacingError = FakeUserFacingError + mock_exceptions_module.AlreadyAttachedError = FakeAlreadyAttachedError + sys.modules["uaclient.api.exceptions"] = mock_exceptions_module + _exceptions = Exceptions( FakeUserFacingError, FakeAlreadyAttachedError, ) - sys.modules["uaclient.api.exceptions"] = _exceptions + return _exceptions @pytest.mark.usefixtures("fake_uaclient") @@ -834,7 +839,7 @@ def test_handle_attach( caplog, ): """Non-Pro schemas and instance.""" - handle("nomatter", cfg=cfg, cloud=cloud, args=None) + handle("nomatter", cfg=cfg, cloud=cloud, args=[]) for record_tuple in log_record_tuples: assert record_tuple in caplog.record_tuples if maybe_install_call_args_list is not None: @@ -961,7 +966,7 @@ def test_handle_auto_attach_vs_attach( m_auto_attach.side_effect = auto_attach_side_effect with expectation: - handle("nomatter", cfg=cfg, cloud=cloud, args=None) + handle("nomatter", cfg=cfg, cloud=cloud, args=[]) for record_tuple in log_record_tuples: assert record_tuple in caplog.record_tuples @@ -1006,7 +1011,7 @@ def test_no_fallback_attach( enable or disable pro auto-attach. """ m_should_auto_attach.return_value = is_pro - handle("nomatter", cfg=cfg, cloud=self.cloud, args=None) + handle("nomatter", cfg=cfg, cloud=self.cloud, args=[]) assert not m_attach.call_args_list @pytest.mark.parametrize( @@ -1061,7 +1066,7 @@ def test_handle_errors(self, cfg, match): "nomatter", cfg=cfg, cloud=self.cloud, - args=None, + args=[], ) @mock.patch(f"{MPATH}.subp.subp") @@ -1087,7 +1092,7 @@ def test_pro_config_error_invalid_url(self, m_subp, caplog): "nomatter", cfg=cfg, cloud=self.cloud, - args=None, + args=[], ) assert not caplog.text @@ -1107,7 +1112,7 @@ def test_fallback_to_attach_no_token( "nomatter", cfg=cfg, cloud=self.cloud, - args=None, + args=[], ) assert [] == m_subp.call_args_list assert ( diff --git a/tools/.github-cla-signers b/tools/.github-cla-signers index 866f4e9081d..62202daa333 100644 --- a/tools/.github-cla-signers +++ b/tools/.github-cla-signers @@ -142,6 +142,7 @@ metajiji michaelrommel mitechie MjMoshiri +mostaffatarek124eru mxwebdev nazunalika nelsonad-ops From 0007f79726bbd648eea018d24f23ee0274f88513 Mon Sep 17 00:00:00 2001 From: mostaffatarek124eru Date: Tue, 21 Jan 2025 13:43:41 +0000 Subject: [PATCH 2/5] update CLA --- tools/.github-cla-signers | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/.github-cla-signers b/tools/.github-cla-signers index 62202daa333..79ca5fac00f 100644 --- a/tools/.github-cla-signers +++ b/tools/.github-cla-signers @@ -142,7 +142,7 @@ metajiji michaelrommel mitechie MjMoshiri -mostaffatarek124eru +MostafaTarek124eru mxwebdev nazunalika nelsonad-ops From f94d4f2ea2411ca431fa5a1aa9236de9f75faba3 Mon Sep 17 00:00:00 2001 From: mostaffatarek124eru Date: Mon, 27 Jan 2025 07:32:42 +0000 Subject: [PATCH 3/5] updated for PR# 0007f79 --- cloudinit/config/cc_rsyslog.py | 2 +- cloudinit/config/cc_ubuntu_pro.py | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/cloudinit/config/cc_rsyslog.py b/cloudinit/config/cc_rsyslog.py index f522ee6c7ad..985c1fd0a33 100644 --- a/cloudinit/config/cc_rsyslog.py +++ b/cloudinit/config/cc_rsyslog.py @@ -246,7 +246,7 @@ def __init__( self.proto = proto self.addr = addr - self.port = int(port) if port else None + self.port = port if port: self.port = int(port) else: diff --git a/cloudinit/config/cc_ubuntu_pro.py b/cloudinit/config/cc_ubuntu_pro.py index 764af406c72..ff288727138 100644 --- a/cloudinit/config/cc_ubuntu_pro.py +++ b/cloudinit/config/cc_ubuntu_pro.py @@ -197,11 +197,15 @@ def configure_pro(token, enable=None): try: enable_resp = json.loads(enable_stdout) handle_enable_errors(enable_resp) + except json.JSONDecodeError as e: raise RuntimeError( f"Pro response was not json: {enable_stdout}" ) from e + +def handle_enable_errors(enable_resp: Dict[str, Any]): + # At this point we were able to load the json response from Pro. This # response contains a list of errors under the key 'errors'. E.g. # @@ -226,9 +230,6 @@ def configure_pro(token, enable=None): # related. We can distinguish them by checking if `service` is non-null # or null respectively. - -def handle_enable_errors(enable_resp: Dict[str, Any]): - enable_errors: List[Dict[str, Any]] = [] for err in enable_resp.get("errors", []): if err["message_code"] == "service-already-enabled": From 78644378489baf35834e54b93decff3332c69c05 Mon Sep 17 00:00:00 2001 From: mostaffatarek124eru Date: Tue, 28 Jan 2025 21:53:19 +0000 Subject: [PATCH 4/5] added type validation for ubuntu_pro --- cloudinit/config/cc_rsyslog.py | 6 +- cloudinit/config/cc_ubuntu_pro.py | 102 ++++++++++++++++-------------- 2 files changed, 56 insertions(+), 52 deletions(-) diff --git a/cloudinit/config/cc_rsyslog.py b/cloudinit/config/cc_rsyslog.py index 985c1fd0a33..059ff8b4a45 100644 --- a/cloudinit/config/cc_rsyslog.py +++ b/cloudinit/config/cc_rsyslog.py @@ -246,11 +246,7 @@ def __init__( self.proto = proto self.addr = addr - self.port = port - if port: - self.port = int(port) - else: - self.port = None + self.port = int(port) if port is not None else None def validate(self): if self.port: diff --git a/cloudinit/config/cc_ubuntu_pro.py b/cloudinit/config/cc_ubuntu_pro.py index ff288727138..69074b57cb2 100644 --- a/cloudinit/config/cc_ubuntu_pro.py +++ b/cloudinit/config/cc_ubuntu_pro.py @@ -5,7 +5,7 @@ import json import logging import re -from typing import Any, Dict, List +from typing import Any, Dict, List, Union from urllib.parse import urlparse from cloudinit import performance, subp, util @@ -204,53 +204,61 @@ def configure_pro(token, enable=None): ) from e -def handle_enable_errors(enable_resp: Dict[str, Any]): - - # At this point we were able to load the json response from Pro. This - # response contains a list of errors under the key 'errors'. E.g. - # - # { - # "errors": [ - # { - # "message": "UA Apps: ESM is already enabled ...", - # "message_code": "service-already-enabled", - # "service": "esm-apps", - # "type": "service" - # }, - # { - # "message": "Cannot enable unknown service 'asdf' ...", - # "message_code": "invalid-service-or-failure", - # "service": null, - # "type": "system" - # } - # ] - # } - # - # From our pov there are two type of errors, service and non-service - # related. We can distinguish them by checking if `service` is non-null - # or null respectively. - - enable_errors: List[Dict[str, Any]] = [] - for err in enable_resp.get("errors", []): - if err["message_code"] == "service-already-enabled": - LOG.debug("Service `%s` already enabled.", err["service"]) - continue - enable_errors.append(err) - - if enable_errors: - error_services: List[str] = [] - for err in enable_errors: - service = err.get("service") - if service is not None: - error_services.append(service) - msg = f'Failure enabling `{service}`: {err["message"]}' - else: - msg = f'Failure of type `{err["type"]}`: {err["message"]}' - util.logexc(LOG, msg) +def handle_enable_errors(enable_resp: Union[List[Any], Dict[str, Any]]): + if isinstance(enable_resp, list): + LOG.warning( + "Unexpected list response from pro enable: %s", enable_resp + ) + return + elif isinstance(enable_resp, dict) and "errors" in enable_resp: + # At this point we were able to load the json response from Pro. This + # response contains a list of errors under the key 'errors'. E.g. + # + # { + # "errors": [ + # { + # "message": "UA Apps: ESM is already enabled ...", + # "message_code": "service-already-enabled", + # "service": "esm-apps", + # "type": "service" + # }, + # { + # "message": "Cannot enable unknown service 'asdf' ...", + # "message_code": "invalid-service-or-failure", + # "service": null, + # "type": "system" + # } + # ] + # } + # + # From our pov there are two type of errors, service and non-service + # related. We can distinguish them by checking if `service` is non-null + # or null respectively. + enable_errors: List[Dict[str, Any]] = [] + for err in enable_resp.get("errors", []): + if err["message_code"] == "service-already-enabled": + LOG.debug("Service `%s` already enabled.", err["service"]) + continue + enable_errors.append(err) + + if enable_errors: + error_services: List[str] = [] + for err in enable_errors: + service = err.get("service") + if service is not None: + error_services.append(service) + msg = f'Failure enabling `{service}`: {err["message"]}' + else: + msg = f'Failure of type `{err["type"]}`: {err["message"]}' + util.logexc(LOG, msg) - raise RuntimeError( - "Failure enabling Ubuntu Pro service(s): " - + ", ".join(error_services) + raise RuntimeError( + "Failure enabling Ubuntu Pro service(s): " + + ", ".join(error_services) + ) + else: + LOG.warning( + "Unexpected response format from pro enable: %s", enable_resp ) From 0a9d25bde7ec7fa08fd08b3d654dda309c9dc055 Mon Sep 17 00:00:00 2001 From: mostaffatarek124eru Date: Thu, 6 Feb 2025 09:36:59 +0000 Subject: [PATCH 5/5] Removed the old changes as requested in PR --- cloudinit/config/cc_rsyslog.py | 2 +- cloudinit/config/cc_ubuntu_pro.py | 101 +++++++++++++----------------- 2 files changed, 45 insertions(+), 58 deletions(-) diff --git a/cloudinit/config/cc_rsyslog.py b/cloudinit/config/cc_rsyslog.py index 059ff8b4a45..d7133bac716 100644 --- a/cloudinit/config/cc_rsyslog.py +++ b/cloudinit/config/cc_rsyslog.py @@ -281,7 +281,7 @@ def __str__(self): else: buf += self.addr - if self.port is not None: + if self.port: buf += ":%s" % self.port if self.name: diff --git a/cloudinit/config/cc_ubuntu_pro.py b/cloudinit/config/cc_ubuntu_pro.py index 69074b57cb2..dd530646b53 100644 --- a/cloudinit/config/cc_ubuntu_pro.py +++ b/cloudinit/config/cc_ubuntu_pro.py @@ -5,7 +5,7 @@ import json import logging import re -from typing import Any, Dict, List, Union +from typing import Any, List from urllib.parse import urlparse from cloudinit import performance, subp, util @@ -196,69 +196,56 @@ def configure_pro(token, enable=None): try: enable_resp = json.loads(enable_stdout) - handle_enable_errors(enable_resp) - except json.JSONDecodeError as e: raise RuntimeError( f"Pro response was not json: {enable_stdout}" ) from e + # At this point we were able to load the json response from Pro. This + # response contains a list of errors under the key 'errors'. E.g. + # + # { + # "errors": [ + # { + # "message": "UA Apps: ESM is already enabled ...", + # "message_code": "service-already-enabled", + # "service": "esm-apps", + # "type": "service" + # }, + # { + # "message": "Cannot enable unknown service 'asdf' ...", + # "message_code": "invalid-service-or-failure", + # "service": null, + # "type": "system" + # } + # ] + # } + # + # From our pov there are two type of errors, service and non-service + # related. We can distinguish them by checking if `service` is non-null + # or null respectively. + + enable_errors: List[dict] = [] + for error in enable_resp.get("errors", []): + if error["message_code"] == "service-already-enabled": + LOG.debug("Service `%s` already enabled.", error["service"]) + continue + enable_errors.append(error) -def handle_enable_errors(enable_resp: Union[List[Any], Dict[str, Any]]): - if isinstance(enable_resp, list): - LOG.warning( - "Unexpected list response from pro enable: %s", enable_resp - ) - return - elif isinstance(enable_resp, dict) and "errors" in enable_resp: - # At this point we were able to load the json response from Pro. This - # response contains a list of errors under the key 'errors'. E.g. - # - # { - # "errors": [ - # { - # "message": "UA Apps: ESM is already enabled ...", - # "message_code": "service-already-enabled", - # "service": "esm-apps", - # "type": "service" - # }, - # { - # "message": "Cannot enable unknown service 'asdf' ...", - # "message_code": "invalid-service-or-failure", - # "service": null, - # "type": "system" - # } - # ] - # } - # - # From our pov there are two type of errors, service and non-service - # related. We can distinguish them by checking if `service` is non-null - # or null respectively. - enable_errors: List[Dict[str, Any]] = [] - for err in enable_resp.get("errors", []): - if err["message_code"] == "service-already-enabled": - LOG.debug("Service `%s` already enabled.", err["service"]) - continue - enable_errors.append(err) - - if enable_errors: - error_services: List[str] = [] - for err in enable_errors: - service = err.get("service") - if service is not None: - error_services.append(service) - msg = f'Failure enabling `{service}`: {err["message"]}' - else: - msg = f'Failure of type `{err["type"]}`: {err["message"]}' - util.logexc(LOG, msg) + if enable_errors: + error_services: List[str] = [] + for error in enable_errors: + service = error.get("service") + if service is not None: + error_services.append(service) + msg = f'Failure enabling `{service}`: {error["message"]}' + else: + msg = f'Failure of type `{error["type"]}`: {error["message"]}' + util.logexc(LOG, msg) - raise RuntimeError( - "Failure enabling Ubuntu Pro service(s): " - + ", ".join(error_services) - ) - else: - LOG.warning( - "Unexpected response format from pro enable: %s", enable_resp + raise RuntimeError( + "Failure enabling Ubuntu Pro service(s): " + + ", ".join(error_services) )