Merge pull request #31 from canonical/DPE-5268-remove-external-access

[DPE-5268] support removal external access

.gitignore

@@ -2,6 +2,7 @@ venv/
 build/
 *.charm
 
+.coverage*
 coverage*
 __pycache__/
 *.py[cod]

src/charm.py

@@ -93,32 +93,11 @@ def __init__(self, *args):
     # BEGIN: hook functions
     def _on_config_changed(self, event: ConfigChangedEvent) -> None:
         """Listen to changes in the application configuration."""
-        previous_config = self.expose_external
-        external_config = self.model.config["expose-external"]
-        if external_config not in Config.ExternalConnections.VALID_EXTERNAL_CONFIG:
-            logger.error(
-                "External configuration: %s for expose-external is not valid, should be one of: %s",
-                external_config,
-                Config.ExternalConnections.VALID_EXTERNAL_CONFIG,
-            )
-            self.status.set_and_share_status(Config.Status.INVALID_EXTERNAL_CONFIG)
+        if not self.is_user_external_config_valid():
+            self.set_status_invalid_external_config()
             return
 
-        self.expose_external = external_config
-        if external_config == Config.ExternalConnections.EXTERNAL_NODEPORT:
-            # every unit attempts to create a nodeport service - if exists, will silently continue
-            self.node_port_manager.apply_service(
-                service=self.node_port_manager.build_node_port_services(
-                    port=Config.MONGOS_PORT
-                )
-            )
-
-        if (
-            external_config == Config.ExternalConnections.NONE
-            and previous_config == Config.ExternalConnections.EXTERNAL_NODEPORT
-        ):
-            # TODO DPE-5268 - support revoking external access
-            pass
+        self.update_external_services()
 
         # TODO DPE-5235 support updating data-integrator clients to have/not have public IP
         # depending on the result of the configuration
@@ -187,17 +166,10 @@ def _on_start(self, event: StartEvent) -> None:
             event.defer()
 
     def _on_update_status(self, _):
-        """Handle the update status event"""
-        if (
-            self.model.config["expose-external"]
-            not in Config.ExternalConnections.VALID_EXTERNAL_CONFIG
-        ):
-            logger.error(
-                "External configuration: %s for expose-external is not valid, should be one of: %s",
-                self.expose_external,
-                Config.ExternalConnections.VALID_EXTERNAL_CONFIG,
-            )
-            self.unit.status = Config.Status.INVALID_EXTERNAL_CONFIG
+        """Handle the update status event."""
+        if not self.is_user_external_config_valid():
+            self.set_status_invalid_external_config()
+            return
 
         if self.unit.status == Config.Status.UNHEALTHY_UPGRADE:
             return
@@ -228,6 +200,39 @@ def _on_update_status(self, _):
     # END: hook functions
 
     # BEGIN: helper functions
+    def is_user_external_config_valid(self) -> bool:
+        """Returns True if the user set external config is valid."""
+        return (
+            self.model.config["expose-external"]
+            in Config.ExternalConnections.VALID_EXTERNAL_CONFIG
+        )
+
+    def set_status_invalid_external_config(self) -> None:
+        """Sets status for invalid external configuration."""
+        logger.error(
+            "External configuration: %s for expose-external is not valid, should be one of: %s",
+            self.model.config["expose-external"],
+            Config.ExternalConnections.VALID_EXTERNAL_CONFIG,
+        )
+        self.status.set_and_share_status(Config.Status.INVALID_EXTERNAL_CONFIG)
+
+    def update_external_services(self) -> None:
+        """Update external services based on provided configuration."""
+        if (
+            self.model.config["expose-external"]
+            == Config.ExternalConnections.EXTERNAL_NODEPORT
+        ):
+            # every unit attempts to create a nodeport service - if exists, will silently continue
+            self.node_port_manager.apply_service(
+                service=self.node_port_manager.build_node_port_services(
+                    port=Config.MONGOS_PORT
+                )
+            )
+        else:
+            self.node_port_manager.delete_unit_service()
+
+        self.expose_external = self.model.config["expose-external"]
+
     def get_keyfile_contents(self) -> str | None:
         """Retrieves the contents of the keyfile on host machine."""
         # wait for keyFile to be created by leader unit

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__)
 
@@ -43,7 +43,13 @@ def client(self) -> Client:
             namespace=self.namespace,
         )
 
-    # --- GETTERS ---
+    # BEGIN: getters
+    def get_service(self, service_name: str) -> Service | None:
+        """Gets the Service via the K8s API."""
+        return self.client.get(
+            res=Service,
+            name=service_name,
+        )
 
     def get_pod(self, pod_name: str = "") -> Pod:
         """Gets the Pod via the K8s API."""
@@ -53,16 +59,34 @@ def get_pod(self, pod_name: str = "") -> Pod:
             name=pod_name or self.pod_name,
         )
 
+    def get_unit_service_name(self) -> str:
+        """Returns the service name for the current unit."""
+        unit_id = self.charm.unit.name.split("/")[1]
+        return f"{self.app_name}-{unit_id}-external"
+
+    def get_unit_service(self) -> Service | None:
+        """Gets the Service via the K8s API for the current unit."""
+        return self.get_service(self.get_unit_service_name())
+
+    # 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."""
         pod = self.get_pod(pod_name=self.pod_name)
         if not pod.metadata:
             raise Exception(f"Could not find metadata for {pod}")
 
-        unit_id = self.charm.unit.name.split("/")[1]
         return Service(
             metadata=ObjectMeta(
-                name=f"{self.app_name}-{unit_id}-external",
+                name=self.get_unit_service_name(),
                 namespace=self.namespace,
                 # When we scale-down K8s will keep the Services for the deleted units around,
                 # unless the Services' owner is also deleted.
@@ -99,10 +123,35 @@ 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)
                 return
             else:
                 raise
+
+    def delete_unit_service(self) -> None:
+        """Deletes a unit Service, if it exists."""
+        try:
+            service = self.get_unit_service()
+        except ApiError as e:
+            if e.status.code == 404:
+                logger.debug(
+                    f"Could not find {self.get_unit_service_name()} to delete."
+                )
+                return
+
+        if not service.metadata:
+            raise Exception(f"Could not find metadata for {service}")
+
+        try:
+            self.client.delete(Service, service.metadata.name)
+        except ApiError as e:
+            if e.status.code == 403:
+                self.on_deployed_without_trust()
+                return
+            else:
+                raise
+
+    # END: helpers

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."""

tests/integration/client_relations/test_external_client_relations.py

@@ -13,7 +13,12 @@
     wait_for_mongos_units_blocked,
 )
 
-from .helpers import assert_all_unit_node_ports_available
+from .helpers import (
+    assert_all_unit_node_ports_available,
+    assert_all_unit_node_ports_are_unavailable,
+    get_port_from_node_port,
+    is_external_mongos_client_reachable,
+)
 
 
 TEST_USER_NAME = "TestUserName1"
@@ -41,7 +46,7 @@ async def test_mongos_external_connections(ops_test: OpsTest) -> None:
     )
     await ops_test.model.wait_for_idle(apps=[MONGOS_APP_NAME], idle_period=15)
 
-    # # verify each unit has a node port available
+    # verify each unit has a node port available
     await assert_all_unit_node_ports_available(ops_test)
 
 
@@ -81,8 +86,25 @@ async def test_mongos_bad_configuration(ops_test: OpsTest) -> None:
 
 @pytest.mark.group(1)
 @pytest.mark.abort_on_fail
-async def test_turn_off_nodeport(ops_test: OpsTest) -> None:
-    """TODO Future PR, test that when the user toggles nodeport to none, it is no longer exposed."""
+async def test_mongos_disable_external_connections(ops_test: OpsTest) -> None:
+    # get exposed node port before toggling off exposure
+    exposed_node_port = get_port_from_node_port(
+        ops_test, node_port_name=f"{MONGOS_APP_NAME}-0-external"
+    )
+
+    """Tests that mongos can disable external connections."""
+    configuration_parameters = {"expose-external": "none"}
+
+    # apply new configuration options
+    await ops_test.model.applications[MONGOS_APP_NAME].set_config(
+        configuration_parameters
+    )
+    await ops_test.model.wait_for_idle(apps=[MONGOS_APP_NAME], idle_period=15)
+
+    # 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_reachable(ops_test, exposed_node_port)
 
 
 @pytest.mark.group(1)

tests/unit/test_nodeport.py

@@ -0,0 +1,107 @@
+# Copyright 2024 Canonical Ltd.
+# See LICENSE file for licensing details.
+import logging
+import unittest
+from unittest import mock
+from unittest.mock import patch, PropertyMock
+import httpx
+from ops.model import BlockedStatus
+from ops.testing import Harness
+from node_port import 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")
+    def test_delete_unit_service_raises_ApiError(self, get_service):
+        """Verify that when charm needs juju trust a status is logged."""
+        metadata_mock = mock.Mock()
+        metadata_mock.name = "service-name"
+        service = mock.Mock()
+        service.metadata = metadata_mock
+        get_service.return_value = service
+
+        # We need a valid API error due to error handling in lightkube
+        api_error = ApiError(
+            request=httpx.Request(url="http://controller/call", method="DELETE"),
+            response=httpx.Response(409, json={"message": "bad call"}),
+        )
+
+        mocked_client = PropertyMock()
+        delete_mock = mock.Mock()
+        delete_mock.side_effect = api_error
+        mocked_client.delete = delete_mock
+
+        # Patch the actual client here
+        self.harness.charm.node_port_manager.client = mocked_client
+
+        with self.assertRaises(ApiError):
+            self.harness.charm.node_port_manager.delete_unit_service()
+
+    @patch("charm.NodePortManager.get_service")
+    def test_delete_unit_service_needs_juju_trust(self, get_service):
+        """Verify that when charm needs juju trust a status is logged."""
+        metadata_mock = mock.Mock()
+        metadata_mock.name = "service-name"
+        service = mock.Mock()
+        service.metadata = metadata_mock
+        get_service.return_value = service
+
+        # We need a valid API error due to error handling in lightkube
+        api_error = ApiError(
+            request=httpx.Request(url="http://controller/call", method="DELETE"),
+            response=httpx.Response(409, json={"message": "bad call", "code": 403}),
+        )
+
+        mocked_client = PropertyMock()
+        delete_mock = mock.Mock()
+        delete_mock.side_effect = api_error
+        mocked_client.delete = delete_mock
+
+        # Patch the actual client here
+        self.harness.charm.node_port_manager.client = mocked_client
+
+        self.harness.charm.node_port_manager.delete_unit_service()
+
+        self.assertTrue(
+            self.harness.charm.unit.status == BlockedStatus(STATUS_JUJU_TRUST)
+        )