Address PR feedback

Author: shayancanonical

.github/workflows/ci.yaml

@@ -159,9 +159,7 @@ jobs:
             echo "mark_expression=not unstable" >> "$GITHUB_OUTPUT"
           fi
       - name: Run integration tests
-        if: 
-        run:
-          tox run -e integration -- "${{ matrix.groups.path_to_test_file }}" --group="${{ matrix.groups.group_number }}" -m '${{ steps.select-test-stability.outputs.mark_expression }}' --mysql-router-charm-series=${{ matrix.ubuntu-versions.series }} --mysql-router-charm-bases-index=${{ matrix.ubuntu-versions.bases-index }}
+        run: tox run -e integration -- "${{ matrix.groups.path_to_test_file }}" --group="${{ matrix.groups.group_number }}" -m '${{ steps.select-test-stability.outputs.mark_expression }}' --mysql-router-charm-series=${{ matrix.ubuntu-versions.series }} --mysql-router-charm-bases-index=${{ matrix.ubuntu-versions.bases-index }}
         env:
           LIBJUJU_VERSION_SPECIFIER: ${{ matrix.libjuju-version }}
           SECRETS_FROM_GITHUB: |

poetry.lock

@@ -10,15 +10,15 @@ authors = []
 
 [tool.poetry.dependencies]
 python = "^3.8.1"  # ^3.8.1 required by flake8
-ops = "^2.0.0"
+ops = "^2.8.0"
 tenacity = "^8.2.3"
 poetry-core = "^1.7.0"
 jinja2 = "^3.1.2"
 requests = "^2.31.0"
 
 [tool.poetry.group.charm-libs.dependencies]
 # data_platform_libs/v0/data_interfaces.py
-ops = ">=2.0.0"
+ops = ">=2.8.0"
 # tls_certificates_interface/v1/tls_certificates.py
 cryptography = ">=42.0.5"
 jsonschema = "*"
@@ -52,7 +52,7 @@ pytest = "^7.4.0"
 pytest-xdist = "^3.3.1"
 pytest-cov = "^4.1.0"
 ops-scenario = "^5.4.1"
-ops = ">=2.0.0"
+ops = ">=2.8.0"
 pytest-mock = "^3.11.1"
 
 [tool.poetry.group.integration.dependencies]
@@ -64,7 +64,7 @@ pytest-github-secrets = {git = "https://github.com/canonical/data-platform-workf
 juju = "3.2.0.1"
 mysql-connector-python = "~8.0.33"
 tenacity = "^8.2.2"
-ops = ">=2.0.0"
+ops = ">=2.8.0"
 pytest-mock = "^3.11.1"
 
 

pyproject.toml

@@ -29,6 +29,9 @@
 class MySQLRouterCharm(ops.CharmBase, abc.ABC):
     """MySQL Router charm"""
 
+    _READ_WRITE_PORT = 6446
+    _READ_ONLY_PORT = 6447
+
     def __init__(self, *args) -> None:
         super().__init__(*args)
         # Instantiate before registering other event observers
@@ -96,6 +99,16 @@ def _read_write_endpoint(self) -> str:
     def _read_only_endpoint(self) -> str:
         """MySQL Router read-only endpoint"""
 
+    @property
+    @abc.abstractmethod
+    def _exposed_read_write_endpoint(self) -> str:
+        """The exposed read-write endpoint"""
+
+    @property
+    @abc.abstractmethod
+    def _exposed_read_only_endpoint(self) -> str:
+        """The exposed read-only endpoint"""
+
     @property
     @abc.abstractmethod
     def _tls_certificate_saved(self) -> bool:
@@ -116,6 +129,11 @@ def _tls_certificate_authority(self) -> typing.Optional[str]:
     def _tls_certificate(self) -> typing.Optional[str]:
         """Custom TLS certificate"""
 
+    @property
+    @abc.abstractmethod
+    def _substrate(self) -> str:
+        """Returns the substrate of the charm: vm or k8s"""
+
     @abc.abstractmethod
     def is_exposed(self, relation=None) -> bool:
         """Whether router is exposed externally"""
@@ -208,19 +226,38 @@ def wait_until_mysql_router_ready(self) -> None:
                 wait=tenacity.wait_fixed(5),
             ):
                 with attempt:
-                    if self.is_exposed():
+                    if self._substrate == "k8s" or self.is_exposed():
                         for port in (6446, 6447):
                             with socket.socket() as s:
                                 assert s.connect_ex(("localhost", port)) == 0
                     else:
-                        assert self._container.path("/run/mysqlrouter/mysql.sock").exists()
-                        assert self._container.path("/run/mysqlrouter/mysqlro.sock").exists()
+                        for socket_file in (
+                            "/run/mysqlrouter/mysql.sock",
+                            "/run/mysqlrouter/mysqlro.sock",
+                        ):
+                            assert self._container.path(socket_file).exists()
+                            with socket.socket(socket.AF_UNIX) as s:
+                                assert s.connect_ex(str(self._container.path(socket_file))) == 0
         except AssertionError:
             logger.exception("Unable to connect to MySQL Router")
             raise
         else:
             logger.debug("MySQL Router is ready")
 
+    @abc.abstractmethod
+    def _reconcile_node_port(self, event) -> None:
+        """Reconcile node port.
+
+        Only applies to Kubernetes charm
+        """
+
+    @abc.abstractmethod
+    def _reconcile_ports(self) -> None:
+        """Reconcile exposed ports.
+
+        Only applies to Machine charm
+        """
+
     # =======================
     #  Handlers
     # =======================
@@ -286,21 +323,29 @@ def reconcile(self, event=None) -> None:  # noqa: C901
                     and isinstance(workload_, workload.AuthenticatedWorkload)
                     and workload_.container_ready
                 ):
+                    self._reconcile_node_port(event=event)
                     self._database_provides.reconcile_users(
                         event=event,
                         router_read_write_endpoint=self._read_write_endpoint,
                         router_read_only_endpoint=self._read_only_endpoint,
+                        exposed_read_write_endpoint=self._exposed_read_write_endpoint,
+                        exposed_read_only_endpoint=self._exposed_read_only_endpoint,
                         shell=workload_.shell,
                     )
             if workload_.container_ready:
                 workload_.reconcile(
-                    tls=self.tls.relation_exists and not self.tls.is_relation_breaking(event),
+                    tls=self._tls_certificate_saved,
                     unit_name=self.unit.name,
                     exporter_config=self._cos_exporter_config(event),
                     key=self._tls_key,
                     certificate=self._tls_certificate,
                     certificate_authority=self._tls_certificate_authority,
                 )
+                if not self._upgrade.in_progress and isinstance(
+                    workload_, workload.AuthenticatedWorkload
+                ):
+                    self._reconcile_ports()
+
             # Empty waiting status means we're waiting for database requires relation before
             # starting workload
             if not workload_.status or workload_.status == ops.WaitingStatus():

src/abstract_charm.py

@@ -103,7 +103,7 @@ def __init__(
         self._mysql_router_command = mysql_router_command
         self._mysql_shell_command = mysql_shell_command
         self._mysql_router_password_command = mysql_router_password_command
-        self.unit_name = unit_name
+        self._unit_name = unit_name
 
     @property
     @abc.abstractmethod

src/container.py

@@ -26,9 +26,6 @@
 class MachineSubordinateRouterCharm(abstract_charm.MySQLRouterCharm):
     """MySQL Router machine subordinate charm"""
 
-    READ_WRITE_PORT = 6446
-    READ_ONLY_PORT = 6447
-
     def __init__(self, *args) -> None:
         super().__init__(*args)
         # DEPRECATED shared-db: Enable legacy "mysql-shared" interface
@@ -64,22 +61,26 @@ def _logrotate(self) -> machine_logrotate.LogRotate:
         return machine_logrotate.LogRotate(container_=self._container)
 
     @property
-    def _host_address(self) -> str:
+    def host_address(self) -> str:
         """The host address for the machine."""
-        return self.model.get_binding("juju-info").network.bind_address
+        return str(self.model.get_binding("juju-info").network.bind_address)
 
     @property
     def _read_write_endpoint(self) -> str:
-        if self.is_exposed():
-            return f"{self._host_address}:{self.READ_WRITE_PORT}"
         return f'file://{self._container.path("/run/mysqlrouter/mysql.sock")}'
 
     @property
     def _read_only_endpoint(self) -> str:
-        if self.is_exposed():
-            return f"{self._host_address}:{self.READ_ONLY_PORT}"
         return f'file://{self._container.path("/run/mysqlrouter/mysqlro.sock")}'
 
+    @property
+    def _exposed_read_write_endpoint(self) -> str:
+        return f"{self.host_address}:{self._READ_WRITE_PORT}"
+
+    @property
+    def _exposed_read_only_endpoint(self) -> str:
+        return f"{self.host_address}:{self._READ_ONLY_PORT}"
+
     @property
     def _tls_certificate_saved(self) -> bool:
         """Whether a TLS certificate is available to use"""
@@ -102,6 +103,21 @@ def _tls_certificate_authority(self) -> typing.Optional[str]:
     def is_exposed(self, relation=None) -> bool:
         return self._database_provides.is_exposed
 
+    def _reconcile_node_port(self, event) -> None:
+        """Only applies to Kubernetes charm, so no-op."""
+        pass
+
+    def _reconcile_ports(self) -> None:
+        if self.is_exposed():
+            ports = [self._READ_WRITE_PORT, self._READ_ONLY_PORT]
+        else:
+            ports = []
+        self.unit.set_ports(*ports)
+
+    @property
+    def _substrate(self) -> str:
+        return "vm"
+
     # =======================
     #  Handlers
     # =======================
@@ -146,10 +162,6 @@ def _on_force_upgrade_action(self, event: ops.ActionEvent) -> None:
         event.set_results({"result": f"Forcefully upgraded {self.unit.name}"})
         logger.debug("Forced upgrade")
 
-    def reconcile(self, event=None) -> None:
-        self._database_provides.reconcile_ports()
-        super().reconcile(event=event)
-
 
 if __name__ == "__main__":
     ops.main.main(MachineSubordinateRouterCharm)

src/machine_charm.py

@@ -43,16 +43,14 @@ def is_exposed(self) -> bool:
         """Whether the relation is exposed"""
         return self._database_provides.is_exposed
 
-    def reconcile_ports(self) -> None:
-        """Reconcile ports for this unit"""
-        self._database_provides.reconcile_ports()
-
     def reconcile_users(
         self,
         *,
         event,
         router_read_write_endpoint: str,
         router_read_only_endpoint: str,
+        exposed_read_write_endpoint: str,
+        exposed_read_only_endpoint: str,
         shell: mysql_shell.Shell,
     ) -> None:
         """Create requested users and delete inactive users.
@@ -65,6 +63,8 @@ def reconcile_users(
             event=event,
             router_read_write_endpoint=router_read_write_endpoint,
             router_read_only_endpoint=router_read_only_endpoint,
+            exposed_read_write_endpoint=exposed_read_write_endpoint,
+            exposed_read_only_endpoint=exposed_read_only_endpoint,
             shell=shell,
         )
         self._deprecated_shared_db.reconcile_users(event=event, shell=shell)

src/relations/database_providers_wrapper.py

@@ -70,6 +70,11 @@ def __init__(
         # Application charm databag
         databag = remote_databag.RemoteDatabag(interface=interface, relation=relation)
         self._database: str = databag["database"]
+        # Whether endpoints should be externally accessible
+        # (e.g. when related to `data-integrator` charm)
+        # Implements DA073 - Add Expose Flag to the Database Interface
+        # https://docs.google.com/document/d/1Y7OZWwMdvF8eEMuVKrqEfuFV3JOjpqLHL7_GPqJpRHU
+        self._external_connectivity = databag.get("external-node-connectivity") == "true"
         if databag.get("extra-user-roles"):
             raise _UnsupportedExtraUserRole(
                 app_name=relation.app.name, endpoint_name=relation.name
@@ -100,6 +105,8 @@ def create_database_and_user(
         *,
         router_read_write_endpoint: str,
         router_read_only_endpoint: str,
+        exposed_read_write_endpoint: str,
+        exposed_read_only_endpoint: str,
         shell: mysql_shell.Shell,
     ) -> None:
         """Create database & user and update databag."""
@@ -115,11 +122,23 @@ def create_database_and_user(
         password = shell.create_application_database_and_user(
             username=username, database=self._database
         )
+
+        rw_endpoint = (
+            exposed_read_write_endpoint
+            if self._external_connectivity
+            else router_read_write_endpoint
+        )
+        ro_endpoint = (
+            exposed_read_only_endpoint
+            if self._external_connectivity
+            else router_read_only_endpoint
+        )
+
         self._set_databag(
             username=username,
             password=password,
-            router_read_write_endpoint=router_read_write_endpoint,
-            router_read_only_endpoint=router_read_only_endpoint,
+            router_read_write_endpoint=rw_endpoint,
+            router_read_only_endpoint=ro_endpoint,
         )
 
 
@@ -189,21 +208,14 @@ def is_exposed(self) -> bool:
             [data.get("external-node-connectivity") == "true" for data in relation_data.values()]
         )
 
-    def reconcile_ports(self) -> None:
-        """Reconcile ports for this unit"""
-        if self.is_exposed:
-            self._charm.unit.open_port("tcp", self._charm.READ_WRITE_PORT)
-            self._charm.unit.open_port("tcp", self._charm.READ_ONLY_PORT)
-        else:
-            self._charm.unit.close_port("tcp", self._charm.READ_WRITE_PORT)
-            self._charm.unit.close_port("tcp", self._charm.READ_ONLY_PORT)
-
     def reconcile_users(
         self,
         *,
         event,
         router_read_write_endpoint: str,
         router_read_only_endpoint: str,
+        exposed_read_write_endpoint: str,
+        exposed_read_only_endpoint: str,
         shell: mysql_shell.Shell,
     ) -> None:
         """Create requested users and delete inactive users.
@@ -235,6 +247,8 @@ def reconcile_users(
                 relation.create_database_and_user(
                     router_read_write_endpoint=router_read_write_endpoint,
                     router_read_only_endpoint=router_read_only_endpoint,
+                    exposed_read_write_endpoint=exposed_read_write_endpoint,
+                    exposed_read_only_endpoint=exposed_read_only_endpoint,
                     shell=shell,
                 )
         for relation in self._shared_users: