From 710135e8ca3da3021ddd52ce9b99414de052f476 Mon Sep 17 00:00:00 2001 From: Mia Altieri Date: Mon, 2 Sep 2024 09:33:14 +0000 Subject: [PATCH] PR feedback + unit tests WIP --- src/node_port.py | 12 ++- tests/integration/client_relations/helpers.py | 4 +- .../test_external_client_relations.py | 4 +- tests/unit/test_nodeport.py | 86 +++++++++++++++++++ 4 files changed, 99 insertions(+), 7 deletions(-) create mode 100644 tests/unit/test_nodeport.py diff --git a/src/node_port.py b/src/node_port.py index 2e3ea470..110db1c3 100644 --- a/src/node_port.py +++ b/src/node_port.py @@ -12,7 +12,7 @@ from lightkube.core.exceptions import ApiError from lightkube.resources.core_v1 import Pod, Service from lightkube.models.core_v1 import ServicePort, ServiceSpec - +from ops.model import BlockedStatus logger = logging.getLogger(__name__) @@ -71,6 +71,12 @@ def get_unit_service(self) -> Service | None: # END: getters # BEGIN: helpers + def on_deployed_without_trust(self) -> None: + """Blocks the application and returns a specific error message for deployments made without --trust.""" + logger.error("Could not apply service, application needs `juju trust`") + self.charm.unit.status = BlockedStatus( + f"Insufficient permissions, try: `juju trust {self.app.name} --scope=cluster`" + ) def build_node_port_services(self, port: str) -> Service: """Builds a ClusterIP service for initial client connection.""" @@ -117,7 +123,7 @@ def apply_service(self, service: Service) -> None: self.client.apply(service) except ApiError as e: if e.status.code == 403: - logger.error("Could not apply service, application needs `juju trust`") + self.on_deployed_without_trust() return if e.status.code == 422 and "port is already allocated" in e.status.message: logger.error(e.status.message) @@ -143,7 +149,7 @@ def delete_unit_service(self) -> None: self.client.delete(Service, service.metadata.name) except ApiError as e: if e.status.code == 403: - logger.error("Could not delete service, application needs `juju trust`") + self.on_deployed_without_trust() return else: raise diff --git a/tests/integration/client_relations/helpers.py b/tests/integration/client_relations/helpers.py index 69b5aa55..5c7fd4f4 100644 --- a/tests/integration/client_relations/helpers.py +++ b/tests/integration/client_relations/helpers.py @@ -114,12 +114,12 @@ async def assert_all_unit_node_ports_available(ops_test: OpsTest): ops_test, node_port_name=f"{MONGOS_APP_NAME}-{unit_id}-external" ) - assert await is_external_mongos_client_reachble( + assert await is_external_mongos_client_reachable( ops_test, exposed_node_port ), "client is not reachable" -async def is_external_mongos_client_reachble( +async def is_external_mongos_client_reachable( ops_test: OpsTest, exposed_node_port: str ) -> bool: """Returns True if the mongos client is reachable on the provided node port via the k8s ip.""" diff --git a/tests/integration/client_relations/test_external_client_relations.py b/tests/integration/client_relations/test_external_client_relations.py index cbb58ace..624eb470 100644 --- a/tests/integration/client_relations/test_external_client_relations.py +++ b/tests/integration/client_relations/test_external_client_relations.py @@ -17,7 +17,7 @@ assert_all_unit_node_ports_available, assert_all_unit_node_ports_are_unavailable, get_port_from_node_port, - is_external_mongos_client_reachble, + is_external_mongos_client_reachable, ) @@ -104,7 +104,7 @@ async def test_mongos_disable_external_connections(ops_test: OpsTest) -> None: # verify each unit has a node port available await assert_all_unit_node_ports_are_unavailable(ops_test) - assert not await is_external_mongos_client_reachble(ops_test, exposed_node_port) + assert not await is_external_mongos_client_reachable(ops_test, exposed_node_port) @pytest.mark.group(1) diff --git a/tests/unit/test_nodeport.py b/tests/unit/test_nodeport.py new file mode 100644 index 00000000..b5c30f86 --- /dev/null +++ b/tests/unit/test_nodeport.py @@ -0,0 +1,86 @@ +# Copyright 2024 Canonical Ltd. +# See LICENSE file for licensing details. +import logging +import unittest +from unittest import mock +from unittest.mock import patch, PropertyMock +from ops.model import BlockedStatus +from ops.testing import Harness +from node_port import NodePortManager, ApiError +from charms.data_platform_libs.v0.data_interfaces import DatabaseRequiresEvents +from charm import MongosCharm + + +logger = logging.getLogger(__name__) + + +STATUS_JUJU_TRUST = ( + "Insufficient permissions, try: `juju trust mongos-k8s --scope=cluster`" +) +CLUSTER_ALIAS = "cluster" + + +class TestNodePort(unittest.TestCase): + def setUp(self, *unused): + """Set up the charm for each unit test.""" + try: + # runs before each test to delete the custom events created for the aliases. This is + # needed because the events are created again in the next test, which causes an error + # related to duplicated events. + delattr(DatabaseRequiresEvents, f"{CLUSTER_ALIAS}_database_created") + delattr(DatabaseRequiresEvents, f"{CLUSTER_ALIAS}_endpoints_changed") + delattr( + DatabaseRequiresEvents, f"{CLUSTER_ALIAS}_read_only_endpoints_changed" + ) + except AttributeError: + # Ignore the events not existing before the first test. + pass + + self.harness = Harness(MongosCharm) + self.addCleanup(self.harness.cleanup) + self.harness.begin() + + @patch("charm.NodePortManager.get_service") + def test_delete_unit_service_has_no_metadata(self, get_service): + """Verify that when no metadata is present, the charm raises an error.""" + service = mock.Mock() + service.metadata = None + get_service.return_value = service + + with self.assertRaises(Exception): + self.harness.charm.node_port_manager.delete_unit_service() + + @patch("charm.NodePortManager.get_service") + @patch("charm.NodePortManager.client", new_callable=PropertyMock) + def test_delete_unit_service_raises_ApiError(self, client, get_service): + """Verify that when charm needs juju trust a status is logged.""" + metadata_mock = mock.Mock() + metadata_mock.name = "serice-name" + service = mock.Mock() + service.metadata = metadata_mock + get_service.return_value = service + + client.return_value.delete.side_effect = ApiError + + with self.assertRaises(ApiError): + self.harness.charm.node_port_manager.delete_unit_service() + + @patch("src.node_port.NodePortManager.get_service") + @patch("charm.NodePortManager.client", new_callable=PropertyMock) + def test_delete_unit_service_needs_juju_trust(self, get_service, client): + """Verify that when charm needs juju trust a status is logged.""" + + metadata_mock = mock.Mock() + metadata_mock.name = "serice-name" + service = mock.Mock() + service.metadata = metadata_mock + get_service.return_value = service + + api_error = mock.Mock() + api_error.status.code = 403 + + NodePortManager.delete_unit_service() + + self.assertTrue( + isinstance(self.harness.charm.unit.status, BlockedStatus(STATUS_JUJU_TRUST)) + )