From ddc9b434a72ce4d7f28891f237de48ebbde6af91 Mon Sep 17 00:00:00 2001 From: James Valleroy Date: Sat, 20 Apr 2024 18:43:35 -0400 Subject: [PATCH] diagnostics: Add optional component_id to DiagnosticCheck Signed-off-by: James Valleroy Reviewed-by: Sunil Mohan Adapa --- plinth/app.py | 12 ++--- plinth/config.py | 2 +- plinth/daemon.py | 18 +++++--- plinth/diagnostic_check.py | 4 +- plinth/modules/apache/components.py | 21 ++++++--- .../modules/apache/tests/test_components.py | 45 ++++++++++++------- plinth/modules/firewall/components.py | 6 ++- .../modules/firewall/tests/test_components.py | 16 +++---- plinth/package.py | 5 ++- plinth/tests/test_config.py | 6 ++- plinth/tests/test_daemon.py | 20 +++++---- plinth/tests/test_diagnostic_check.py | 9 +++- plinth/tests/test_package.py | 12 ++--- 13 files changed, 110 insertions(+), 66 deletions(-) diff --git a/plinth/app.py b/plinth/app.py index c093ceb32..5df311ba7 100644 --- a/plinth/app.py +++ b/plinth/app.py @@ -222,9 +222,9 @@ class App: Return value must be a list of results. Each result is a :class:`~plinth.diagnostic_check.DiagnosticCheck` with a - unique check_id, a user visible description of the test, and the - result. The test result is a string enumeration from 'failed', - 'passed', 'error', 'warning' and 'not_done'. + unique check_id, a user visible description of the test, the result, + test parameters, and the component ID. The test result is a string + enumeration from 'failed', 'passed', 'error', 'warning' and 'not_done'. Results are typically collected by diagnosing each component of the app and then supplementing the results with any app level diagnostic tests. @@ -314,9 +314,9 @@ class Component: Return value must be a list of results. Each result is a :class:`~plinth.diagnostic_check.DiagnosticCheck` with a - unique check_id, a user visible description of the test, and the - result. The test result is a string enumeration from 'failed', - 'passed', 'error', 'warning' and 'not_done'. + unique check_id, a user visible description of the test, the result, + test parameters, and the component ID. The test result is a string + enumeration from 'failed', 'passed', 'error', 'warning' and 'not_done'. Also see :meth:`.has_diagnostics`. diff --git a/plinth/config.py b/plinth/config.py index cf15821e1..1b7f6c8f0 100644 --- a/plinth/config.py +++ b/plinth/config.py @@ -104,7 +104,7 @@ class DropinConfigs(app_module.FollowerComponent): parameters: DiagnosticCheckParameters = {'etc_path': str(etc_path)} results.append( DiagnosticCheck(check_id, description, result_string, - parameters)) + parameters, self.component_id)) return results diff --git a/plinth/daemon.py b/plinth/daemon.py index ae946800d..b70d2e099 100644 --- a/plinth/daemon.py +++ b/plinth/daemon.py @@ -110,7 +110,9 @@ class Daemon(app.LeaderComponent): results = [] results.append(self._diagnose_unit_is_running()) for port in self.listen_ports: - results.append(diagnose_port_listening(port[0], port[1])) + results.append( + diagnose_port_listening(port[0], port[1], None, + self.component_id)) return results @@ -124,7 +126,8 @@ class Daemon(app.LeaderComponent): 'service_name': str(self.unit) } - return DiagnosticCheck(check_id, description, result, parameters) + return DiagnosticCheck(check_id, description, result, parameters, + self.component_id) class RelatedDaemon(app.FollowerComponent): @@ -200,7 +203,8 @@ def app_is_running(app_): def diagnose_port_listening( port: int, kind: str = 'tcp', - listen_address: str | None = None) -> DiagnosticCheck: + listen_address: str | None = None, + component_id: str | None = None) -> DiagnosticCheck: """Run a diagnostic on whether a port is being listened on. Kind must be one of inet, inet4, inet6, tcp, tcp4, tcp6, udp, @@ -222,7 +226,7 @@ def diagnose_port_listening( return DiagnosticCheck(check_id, description, Result.PASSED if result else Result.FAILED, - parameters) + parameters, component_id) def _check_port(port: int, kind: str = 'tcp', @@ -272,7 +276,8 @@ def _check_port(port: int, kind: str = 'tcp', def diagnose_netcat(host: str, port: int, remote_input: str = '', - negate: bool = False) -> DiagnosticCheck: + negate: bool = False, + component_id: str | None = None) -> DiagnosticCheck: """Run a diagnostic using netcat.""" try: process = subprocess.Popen(['nc', host, str(port)], @@ -298,4 +303,5 @@ def diagnose_netcat(host: str, port: int, remote_input: str = '', check_id = f'daemon-netcat-negate-{host}-{port}' description = gettext_noop('Cannot connect to {host}:{port}') - return DiagnosticCheck(check_id, description, result, parameters) + return DiagnosticCheck(check_id, description, result, parameters, + component_id) diff --git a/plinth/diagnostic_check.py b/plinth/diagnostic_check.py index f67661421..d3d6164d2 100644 --- a/plinth/diagnostic_check.py +++ b/plinth/diagnostic_check.py @@ -30,6 +30,7 @@ class DiagnosticCheck: description: str result: Result = Result.NOT_DONE parameters: DiagnosticCheckParameters = field(default_factory=dict) + component_id: str | None = None @property def translated_description(self): @@ -65,6 +66,7 @@ class CheckJSONDecoder(json.JSONDecoder): """Convert tagged data to DiagnosticCheck.""" if data.get('__class__') == 'DiagnosticCheck': return DiagnosticCheck(data['check_id'], data['description'], - data['result'], data['parameters']) + data['result'], data['parameters'], + data.get('component_id')) return data diff --git a/plinth/modules/apache/components.py b/plinth/modules/apache/components.py index 2a9ed9e1d..81f770e6b 100644 --- a/plinth/modules/apache/components.py +++ b/plinth/modules/apache/components.py @@ -69,11 +69,13 @@ class Webserver(app.LeaderComponent): for url in self.urls: if '{host}' in url: results.extend( - diagnose_url_on_all( - url, check_certificate=False, - expect_redirects=self.expect_redirects)) + diagnose_url_on_all(url, check_certificate=False, + expect_redirects=self.expect_redirects, + component_id=self.component_id)) else: - results.append(diagnose_url(url, check_certificate=False)) + results.append( + diagnose_url(url, check_certificate=False, + component_id=self.component_id)) return results @@ -141,7 +143,8 @@ def diagnose_url(url: str, kind: str | None = None, check_certificate: bool = True, extra_options: list[str] | None = None, wrapper: str | None = None, - expected_output: str | None = None) -> DiagnosticCheck: + expected_output: str | None = None, + component_id: str | None = None) -> DiagnosticCheck: """Run a diagnostic on whether a URL is accessible. Kind can be '4' for IPv4 or '6' for IPv6. @@ -161,10 +164,12 @@ def diagnose_url(url: str, kind: str | None = None, check_id = f'apache-url-{url}' description = gettext_noop('Access URL {url}') - return DiagnosticCheck(check_id, description, result, parameters) + return DiagnosticCheck(check_id, description, result, parameters, + component_id) def diagnose_url_on_all(url: str, expect_redirects: bool = False, + component_id: str | None = None, **kwargs) -> list[DiagnosticCheck]: """Run a diagnostic on whether a URL is accessible.""" results = [] @@ -174,7 +179,9 @@ def diagnose_url_on_all(url: str, expect_redirects: bool = False, if not expect_redirects: diagnose_kwargs.setdefault('kind', address['kind']) - results.append(diagnose_url(current_url, **diagnose_kwargs)) + results.append( + diagnose_url(current_url, component_id=component_id, + **diagnose_kwargs)) return results diff --git a/plinth/modules/apache/tests/test_components.py b/plinth/modules/apache/tests/test_components.py index 39d6e1229..9a7a2cd83 100644 --- a/plinth/modules/apache/tests/test_components.py +++ b/plinth/modules/apache/tests/test_components.py @@ -72,13 +72,16 @@ def test_webserver_disable(disable): def test_webserver_diagnose(diagnose_url_on_all, diagnose_url): """Test running diagnostics.""" - def on_all_side_effect(url, check_certificate, expect_redirects): + def on_all_side_effect(url, check_certificate, expect_redirects, + component_id): return [ - DiagnosticCheck('test-all-id', 'test-result-' + url, 'success') + DiagnosticCheck('test-all-id', 'test-result-' + url, 'success', {}, + component_id) ] - def side_effect(url, check_certificate): - return DiagnosticCheck('test-id', 'test-result-' + url, 'success') + def side_effect(url, check_certificate, component_id): + return DiagnosticCheck('test-id', 'test-result-' + url, 'success', {}, + component_id) diagnose_url_on_all.side_effect = on_all_side_effect diagnose_url.side_effect = side_effect @@ -86,19 +89,26 @@ def test_webserver_diagnose(diagnose_url_on_all, diagnose_url): urls=['{host}url1', 'url2'], expect_redirects=True) results = webserver1.diagnose() assert results == [ - DiagnosticCheck('test-all-id', 'test-result-{host}url1', 'success'), - DiagnosticCheck('test-id', 'test-result-url2', 'success') + DiagnosticCheck('test-all-id', 'test-result-{host}url1', 'success', {}, + 'test-webserver'), + DiagnosticCheck('test-id', 'test-result-url2', 'success', {}, + 'test-webserver') ] - diagnose_url_on_all.assert_has_calls( - [call('{host}url1', check_certificate=False, expect_redirects=True)]) - diagnose_url.assert_has_calls([call('url2', check_certificate=False)]) + diagnose_url_on_all.assert_has_calls([ + call('{host}url1', check_certificate=False, expect_redirects=True, + component_id='test-webserver') + ]) + diagnose_url.assert_has_calls( + [call('url2', check_certificate=False, component_id='test-webserver')]) diagnose_url_on_all.reset_mock() webserver2 = Webserver('test-webserver', 'test-config', urls=['{host}url1', 'url2'], expect_redirects=False) results = webserver2.diagnose() - diagnose_url_on_all.assert_has_calls( - [call('{host}url1', check_certificate=False, expect_redirects=False)]) + diagnose_url_on_all.assert_has_calls([ + call('{host}url1', check_certificate=False, expect_redirects=False, + component_id='test-webserver') + ]) @patch('plinth.privileged.service.restart') @@ -244,20 +254,23 @@ def test_diagnose_url(get_addresses, check): 'test-1': 'value-1' }, 'wrapper': 'test-wrapper', - 'expected_output': 'test-expected' + 'expected_output': 'test-expected', + 'component_id': 'test-component', } parameters = {key: args[key] for key in ['url', 'kind']} check.return_value = True result = diagnose_url(**args) assert result == DiagnosticCheck( 'apache-url-kind-https://localhost/test-4', - 'Access URL {url} on tcp{kind}', Result.PASSED, parameters) + 'Access URL {url} on tcp{kind}', Result.PASSED, parameters, + 'test-component') check.return_value = False result = diagnose_url(**args) assert result == DiagnosticCheck( 'apache-url-kind-https://localhost/test-4', - 'Access URL {url} on tcp{kind}', Result.FAILED, parameters) + 'Access URL {url} on tcp{kind}', Result.FAILED, parameters, + 'test-component') del args['kind'] args['url'] = 'https://{host}/test' @@ -287,10 +300,10 @@ def test_diagnose_url(get_addresses, check): assert results == [ DiagnosticCheck('apache-url-kind-https://test-host-1/test-4', 'Access URL {url} on tcp{kind}', Result.PASSED, - parameters[0]), + parameters[0], 'test-component'), DiagnosticCheck('apache-url-kind-https://test-host-2/test-6', 'Access URL {url} on tcp{kind}', Result.PASSED, - parameters[1]), + parameters[1], 'test-component'), ] diff --git a/plinth/modules/firewall/components.py b/plinth/modules/firewall/components.py index e3c381b26..b0f7239b7 100644 --- a/plinth/modules/firewall/components.py +++ b/plinth/modules/firewall/components.py @@ -142,7 +142,8 @@ class Firewall(app.FollowerComponent): 'details': details } results.append( - DiagnosticCheck(check_id, description, result, parameters)) + DiagnosticCheck(check_id, description, result, parameters, + self.component_id)) # External zone if self.is_external: @@ -161,7 +162,8 @@ class Firewall(app.FollowerComponent): parameters = {'name': port, 'details': details} results.append( - DiagnosticCheck(check_id, description, result, parameters)) + DiagnosticCheck(check_id, description, result, parameters, + self.component_id)) return results diff --git a/plinth/modules/firewall/tests/test_components.py b/plinth/modules/firewall/tests/test_components.py index a6d943db9..4d0668f21 100644 --- a/plinth/modules/firewall/tests/test_components.py +++ b/plinth/modules/firewall/tests/test_components.py @@ -161,28 +161,28 @@ def test_diagnose(get_enabled_services, get_port_details): 'networks', Result.PASSED, { 'name': 'test-port1', 'details': '1234/tcp, 1234/udp' - }), + }, 'test-firewall-1'), DiagnosticCheck( 'firewall-port-external-unavailable-test-port1', 'Port {name} ({details}) unavailable for external ' 'networks', Result.PASSED, { 'name': 'test-port1', 'details': '1234/tcp, 1234/udp' - }), + }, 'test-firewall-1'), DiagnosticCheck( 'firewall-port-internal-test-port2', 'Port {name} ({details}) available for internal networks', Result.FAILED, { 'name': 'test-port2', 'details': '2345/udp' - }), + }, 'test-firewall-1'), DiagnosticCheck( 'firewall-port-external-unavailable-test-port2', 'Port {name} ({details}) unavailable for external networks', Result.FAILED, { 'name': 'test-port2', 'details': '2345/udp' - }), + }, 'test-firewall-1'), ] firewall = Firewall('test-firewall-1', ports=['test-port3', 'test-port4'], @@ -195,28 +195,28 @@ def test_diagnose(get_enabled_services, get_port_details): Result.PASSED, { 'name': 'test-port3', 'details': '3456/tcp' - }), + }, 'test-firewall-1'), DiagnosticCheck( 'firewall-port-external-available-test-port3', 'Port {name} ({details}) available for external networks', Result.PASSED, { 'name': 'test-port3', 'details': '3456/tcp' - }), + }, 'test-firewall-1'), DiagnosticCheck( 'firewall-port-internal-test-port4', 'Port {name} ({details}) available for internal networks', Result.FAILED, { 'name': 'test-port4', 'details': '4567/udp' - }), + }, 'test-firewall-1'), DiagnosticCheck( 'firewall-port-external-available-test-port4', 'Port {name} ({details}) available for external networks', Result.FAILED, { 'name': 'test-port4', 'details': '4567/udp' - }), + }, 'test-firewall-1'), ] diff --git a/plinth/package.py b/plinth/package.py index 6bb0a71ae..22a158f41 100644 --- a/plinth/package.py +++ b/plinth/package.py @@ -210,7 +210,7 @@ class Packages(app_module.FollowerComponent): } results.append( DiagnosticCheck(check_id, description, Result.FAILED, - parameters)) + parameters, self.component_id)) continue result = Result.WARNING @@ -230,7 +230,8 @@ class Packages(app_module.FollowerComponent): 'latest_version': str(latest_version) } results.append( - DiagnosticCheck(check_id, description, result, parameters)) + DiagnosticCheck(check_id, description, result, parameters, + self.component_id)) return results diff --git a/plinth/tests/test_config.py b/plinth/tests/test_config.py index bbab5a343..f0dc12d82 100644 --- a/plinth/tests/test_config.py +++ b/plinth/tests/test_config.py @@ -165,11 +165,13 @@ def test_dropin_config_diagnose_symlinks(dropin_configs, tmp_path): DiagnosticCheck( f'dropin-config-{tmp_path}/etc/test/path1', 'Static configuration {etc_path} is setup properly', - Result.FAILED, {'etc_path': f'{tmp_path}/etc/test/path1'}), + Result.FAILED, {'etc_path': f'{tmp_path}/etc/test/path1'}, + 'test-component'), DiagnosticCheck( f'dropin-config-{tmp_path}/etc/path2', 'Static configuration {etc_path} is setup properly', - Result.FAILED, {'etc_path': f'{tmp_path}/etc/path2'}), + Result.FAILED, {'etc_path': f'{tmp_path}/etc/path2'}, + 'test-component'), ] # Proper symlinks exist diff --git a/plinth/tests/test_daemon.py b/plinth/tests/test_daemon.py index bdf74d3d6..a7019ad2c 100644 --- a/plinth/tests/test_daemon.py +++ b/plinth/tests/test_daemon.py @@ -181,9 +181,9 @@ def test_ensure_running(subprocess_call, subprocess_run, service_is_running, def test_diagnose(port_listening, service_is_running, daemon): """Test running diagnostics.""" - def side_effect(port, kind): + def side_effect(port, kind, _listen_address, component_id): name = f'test-result-{port}-{kind}' - return DiagnosticCheck(name, name, Result.PASSED) + return DiagnosticCheck(name, name, Result.PASSED, {}, component_id) daemon = Daemon('test-daemon', 'test-unit', listen_ports=[(8273, 'tcp4'), (345, 'udp')]) @@ -193,13 +193,16 @@ def test_diagnose(port_listening, service_is_running, daemon): assert results == [ DiagnosticCheck('daemon-running-test-unit', 'Service {service_name} is running', Result.PASSED, - {'service_name': 'test-unit'}), + {'service_name': 'test-unit'}, 'test-daemon'), DiagnosticCheck('test-result-8273-tcp4', 'test-result-8273-tcp4', - Result.PASSED), + Result.PASSED, {}, 'test-daemon'), DiagnosticCheck('test-result-345-udp', 'test-result-345-udp', - Result.PASSED) + Result.PASSED, {}, 'test-daemon') ] - port_listening.assert_has_calls([call(8273, 'tcp4'), call(345, 'udp')]) + port_listening.assert_has_calls([ + call(8273, 'tcp4', None, 'test-daemon'), + call(345, 'udp', None, 'test-daemon') + ]) service_is_running.assert_has_calls([call('test-unit')]) service_is_running.return_value = False @@ -342,12 +345,13 @@ def test_diagnose_netcat(popen): assert popen.mock_calls[2] == call().communicate(input=b'test-input') result = diagnose_netcat('test-host', 3300, remote_input='test-input', - negate=True) + negate=True, component_id='test-component') parameters2 = parameters.copy() parameters2['negate'] = True assert result == DiagnosticCheck('daemon-netcat-negate-test-host-3300', 'Cannot connect to {host}:{port}', - Result.FAILED, parameters2) + Result.FAILED, parameters2, + 'test-component') popen().returncode = 1 result = diagnose_netcat('test-host', 3300, remote_input='test-input') diff --git a/plinth/tests/test_diagnostic_check.py b/plinth/tests/test_diagnostic_check.py index caf6f1199..66bbf3261 100644 --- a/plinth/tests/test_diagnostic_check.py +++ b/plinth/tests/test_diagnostic_check.py @@ -23,13 +23,14 @@ def test_result(): def test_diagnostic_check(): """Test the diagnostic check data class.""" with pytest.raises(TypeError): - DiagnosticCheck() + DiagnosticCheck() # pylint: disable=E1120 check = DiagnosticCheck('some-check-id', 'sample check') assert check.check_id == 'some-check-id' assert check.description == 'sample check' assert check.translated_description == 'sample check' assert check.result == Result.NOT_DONE + assert check.component_id is None assert not check.parameters check = DiagnosticCheck('some-check-id', 'sample check', Result.PASSED) @@ -40,6 +41,10 @@ def test_diagnostic_check(): {'key': 'value'}) assert check.parameters['key'] == 'value' + check = DiagnosticCheck('some-check-id', 'sample check', Result.FAILED, {}, + 'some-component') + assert check.component_id == 'some-component' + def test_translate(): """Test formatting the translated description.""" @@ -58,7 +63,7 @@ def test_json_encoder_decoder(): check_json = json.dumps(check, cls=CheckJSONEncoder) for string in [ '"check_id": "some-check-id"', '"description": "sample check"', - '"result": "passed"', '"parameters": {}', + '"result": "passed"', '"parameters": {}', '"component_id": null', '"__class__": "DiagnosticCheck"' ]: assert string in check_json diff --git a/plinth/tests/test_package.py b/plinth/tests/test_package.py index d5308d88b..3a811e4e4 100644 --- a/plinth/tests/test_package.py +++ b/plinth/tests/test_package.py @@ -284,32 +284,34 @@ def test_diagnose(cache): DiagnosticCheck( 'package-available-package1', 'Package {package_expression} is not available for install', - Result.FAILED, {'package_expression': 'package1'}), + Result.FAILED, {'package_expression': 'package1'}, + 'test-component'), DiagnosticCheck( 'package-latest-package2', 'Package {package_name} is the latest version ({latest_version})', Result.PASSED, { 'package_name': 'package2', 'latest_version': '2.0' - }), + }, 'test-component'), DiagnosticCheck( 'package-latest-package3', 'Package {package_name} is the latest version ({latest_version})', Result.WARNING, { 'package_name': 'package3', 'latest_version': '3.0' - }), + }, 'test-component'), DiagnosticCheck( 'package-available-package4 | package5', 'Package {package_expression} is not available for install', - Result.FAILED, {'package_expression': 'package4 | package5'}), + Result.FAILED, {'package_expression': 'package4 | package5'}, + 'test-component'), DiagnosticCheck( 'package-latest-package7', 'Package {package_name} is the latest version ({latest_version})', Result.PASSED, { 'package_name': 'package7', 'latest_version': '4.0' - }), + }, 'test-component'), ]