Address PR feedback

Author: shayancanonical

src/abstract_charm.py

@@ -110,8 +110,11 @@ def _exposed_read_only_endpoint(self) -> str:
         """The exposed read-only endpoint"""
 
     @abc.abstractmethod
-    def is_externally_accessible(self, event=None) -> typing.Optional[bool]:
-        """Whether endpoints should be externally accessible"""
+    def is_externally_accessible(self, *, event) -> typing.Optional[bool]:
+        """Whether endpoints should be externally accessible.
+
+        Only defined in vm charm to return True/False. In k8s charm, returns None.
+        """
 
     @property
     def _tls_certificate_saved(self) -> bool:
@@ -208,7 +211,7 @@ def set_status(self, *, event, app=True, unit=True) -> None:
             logger.debug(f"Set unit status to {self.unit.status}")
 
     @abc.abstractmethod
-    def wait_until_mysql_router_ready(self) -> None:
+    def wait_until_mysql_router_ready(self, *, event) -> None:
         """Wait until a connection to MySQL Router is possible.
 
         Retry every 5 seconds for up to 30 seconds.
@@ -268,6 +271,7 @@ def reconcile(self, event=None) -> None:  # noqa: C901
             if self._upgrade.unit_state == "outdated":
                 if self._upgrade.authorized:
                     self._upgrade.upgrade_unit(
+                        event=event,
                         workload_=workload_,
                         tls=self._tls_certificate_saved,
                         exporter_config=self._cos_exporter_config(event),
@@ -310,6 +314,7 @@ def reconcile(self, event=None) -> None:  # noqa: C901
                     )
             if workload_.container_ready:
                 workload_.reconcile(
+                    event=event,
                     tls=self._tls_certificate_saved,
                     unit_name=self.unit.name,
                     exporter_config=self._cos_exporter_config(event),

src/machine_charm.py

@@ -83,21 +83,21 @@ def _exposed_read_write_endpoint(self) -> str:
     def _exposed_read_only_endpoint(self) -> str:
         return f"{self.host_address}:{self._READ_ONLY_PORT}"
 
-    def is_externally_accessible(self, event=None) -> typing.Optional[bool]:
+    def is_externally_accessible(self, *, event) -> typing.Optional[bool]:
         return self._database_provides.external_connectivity(event)
 
     def _reconcile_node_port(self, *, event) -> None:
         """Only applies to Kubernetes charm, so no-op."""
         pass
 
     def _reconcile_ports(self, *, event) -> None:
-        if self.is_externally_accessible(event):
+        if self.is_externally_accessible(event=event):
             ports = [self._READ_WRITE_PORT, self._READ_ONLY_PORT]
         else:
             ports = []
         self.unit.set_ports(*ports)
 
-    def wait_until_mysql_router_ready(self) -> None:
+    def wait_until_mysql_router_ready(self, *, event) -> None:
         logger.debug("Waiting until MySQL Router is ready")
         self.unit.status = ops.MaintenanceStatus("MySQL Router starting")
         try:
@@ -107,7 +107,7 @@ def wait_until_mysql_router_ready(self) -> None:
                 wait=tenacity.wait_fixed(5),
             ):
                 with attempt:
-                    if self.is_externally_accessible():
+                    if self.is_externally_accessible(event=event):
                         for port in (
                             self._READ_WRITE_PORT,
                             self._READ_ONLY_PORT,
@@ -173,7 +173,7 @@ def _on_force_upgrade_action(self, event: ops.ActionEvent) -> None:
         logger.debug("Forcing upgrade")
         event.log(f"Forcefully upgrading {self.unit.name}")
         self._upgrade.upgrade_unit(
-            workload_=self.get_workload(event=None), tls=self._tls_certificate_saved
+            event=event, workload_=self.get_workload(event=None), tls=self._tls_certificate_saved
         )
         self.reconcile()
         event.set_results({"result": f"Forcefully upgraded {self.unit.name}"})

src/machine_upgrade.py

@@ -158,13 +158,14 @@ def authorized(self) -> bool:
     def upgrade_unit(
         self,
         *,
+        event,
         workload_: workload.Workload,
         tls: bool,
         exporter_config: "relations.cos.ExporterConfig",
     ) -> None:
         logger.debug(f"Upgrading {self.authorized=}")
         self.unit_state = "upgrading"
-        workload_.upgrade(unit=self._unit, tls=tls, exporter_config=exporter_config)
+        workload_.upgrade(event=event, unit=self._unit, tls=tls, exporter_config=exporter_config)
         self._unit_workload_container_version = snap.REVISION
         self._unit_workload_version = self._current_versions["workload"]
         logger.debug(

src/machine_workload.py

@@ -11,16 +11,21 @@
 
 import workload
 
+if typing.TYPE_CHECKING:
+    import relations.database_requires
+
 logger = logging.getLogger(__name__)
 
 
 class AuthenticatedMachineWorkload(workload.AuthenticatedWorkload):
     """Workload with connection to MySQL cluster and with Unix sockets enabled"""
 
     # TODO python3.10 min version: Use `list` instead of `typing.List`
-    def _get_bootstrap_command(self, password: str) -> typing.List[str]:
-        command = super()._get_bootstrap_command(password)
-        if self._charm.is_externally_accessible():
+    def _get_bootstrap_command(
+        self, *, event, connection_info: "relations.database_requires.ConnectionInformation"
+    ) -> typing.List[str]:
+        command = super()._get_bootstrap_command(connection_info)
+        if self._charm.is_externally_accessible(event=event):
             command.extend(
                 [
                     "--conf-bind-address",
@@ -65,7 +70,7 @@ def _update_configured_socket_file_locations(self) -> None:
             self._container.router_config_file.write_text(output.getvalue())
         logger.debug("Updated configured socket file locations")
 
-    def _bootstrap_router(self, *, tls: bool) -> None:
-        super()._bootstrap_router(tls=tls)
-        if not self._charm.is_externally_accessible():
+    def _bootstrap_router(self, *, event, tls: bool) -> None:
+        super()._bootstrap_router(event=event, tls=tls)
+        if not self._charm.is_externally_accessible(event=event):
             self._update_configured_socket_file_locations()

src/relations/tls.py

@@ -111,10 +111,10 @@ def save_certificate(self, event: tls_certificates.CertificateAvailableEvent) ->
         logger.debug(f"Saved TLS certificate {event=}")
         self._charm.reconcile(event=None)
 
-    def _generate_csr(self, key: bytes) -> bytes:
+    def _generate_csr(self, *, event, key: bytes) -> bytes:
         """Generate certificate signing request (CSR)."""
         sans_ip = ["127.0.0.1"]  # needed for the HTTP server when related with COS
-        if self._charm.is_externally_accessible():
+        if self._charm.is_externally_accessible(event=event):
             sans_ip.append(self._charm.host_address)
 
         return tls_certificates.generate_csr(
@@ -124,23 +124,23 @@ def _generate_csr(self, key: bytes) -> bytes:
             sans_ip=sans_ip,
         )
 
-    def request_certificate_creation(self):
+    def request_certificate_creation(self, *, event):
         """Request new TLS certificate from related provider charm."""
         logger.debug("Requesting TLS certificate creation")
-        csr = self._generate_csr(self.key.encode("utf-8"))
+        csr = self._generate_csr(event=event, key=self.key.encode("utf-8"))
         self._interface.request_certificate_creation(certificate_signing_request=csr)
         self._secrets.set_value(
             relations.secrets.UNIT_SCOPE, _TLS_REQUESTED_CSR, csr.decode("utf-8")
         )
         logger.debug("Requested TLS certificate creation")
 
-    def request_certificate_renewal(self):
+    def request_certificate_renewal(self, *, event):
         """Request TLS certificate renewal from related provider charm."""
         logger.debug("Requesting TLS certificate renewal")
         old_csr = self._secrets.get_value(relations.secrets.UNIT_SCOPE, _TLS_ACTIVE_CSR).encode(
             "utf-8"
         )
-        new_csr = self._generate_csr(self.key.encode("utf-8"))
+        new_csr = self._generate_csr(event=event, key=self.key.encode("utf-8"))
         self._interface.request_certificate_renewal(
             old_certificate_signing_request=old_csr, new_certificate_signing_request=new_csr
         )
@@ -252,7 +252,7 @@ def _on_set_tls_private_key(self, event: ops.ActionEvent) -> None:
             logger.debug("No TLS certificate relation active. Skipped certificate request")
         else:
             try:
-                self._relation.request_certificate_creation()
+                self._relation.request_certificate_creation(event=event)
             except Exception as e:
                 event.fail(f"Failed to request certificate: {e}")
                 logger.exception(
@@ -261,9 +261,9 @@ def _on_set_tls_private_key(self, event: ops.ActionEvent) -> None:
                 raise
         logger.debug("Handled set TLS private key action")
 
-    def _on_tls_relation_created(self, _) -> None:
+    def _on_tls_relation_created(self, event) -> None:
         """Request certificate when TLS relation created."""
-        self._relation.request_certificate_creation()
+        self._relation.request_certificate_creation(event)
 
     def _on_tls_relation_broken(self, _) -> None:
         """Delete TLS certificate."""
@@ -283,4 +283,4 @@ def _on_certificate_expiring(self, event: tls_certificates.CertificateExpiringEv
             logger.warning("Unknown certificate expiring")
             return
 
-        self._relation.request_certificate_renewal()
+        self._relation.request_certificate_renewal(event)

src/workload.py

@@ -77,7 +77,7 @@ def version(self) -> str:
         return ""
 
     def upgrade(
-        self, *, unit: ops.Unit, tls: bool, exporter_config: "relations.cos.ExporterConfig"
+        self, *, event, unit: ops.Unit, tls: bool, exporter_config: "relations.cos.ExporterConfig"
     ) -> None:
         """Upgrade MySQL Router.
 
@@ -157,6 +157,7 @@ def _disable_router(self) -> None:
     def reconcile(
         self,
         *,
+        event,
         tls: bool,
         unit_name: str,
         exporter_config: "relations.cos.ExporterConfig",
@@ -253,15 +254,17 @@ def _get_bootstrap_command(
             "--conf-use-gr-notifications",
         ]
 
-    def _bootstrap_router(self, *, tls: bool) -> None:
+    def _bootstrap_router(self, *, event, tls: bool) -> None:
         """Bootstrap MySQL Router."""
         logger.debug(
             f"Bootstrapping router {tls=}, {self._connection_info.host=}, {self._connection_info.port=}"
         )
         # Redact password from log
-        logged_command = self._get_bootstrap_command(self._connection_info.redacted)
+        logged_command = self._get_bootstrap_command(
+            event=event, connection_info=self._connection_info.redacted
+        )
 
-        command = self._get_bootstrap_command(self._connection_info)
+        command = self._get_bootstrap_command(event=event, connection_info=self._connection_info)
         try:
             self._container.run_mysql_router(command, timeout=30)
         except container.CalledProcessError as e:
@@ -311,31 +314,31 @@ def _router_username(self) -> str:
         """
         return self._parse_username_from_config(self._container.router_config_file.read_text())
 
-    def _restart(self, *, tls: bool) -> None:
+    def _restart(self, *, event, tls: bool) -> None:
         """Restart MySQL Router to enable or disable TLS."""
         logger.debug("Restarting MySQL Router")
         assert self._container.mysql_router_service_enabled is True
         self._container.update_mysql_router_service(enabled=True, tls=tls)
         logger.debug("Restarted MySQL Router")
-        self._charm.wait_until_mysql_router_ready()
+        self._charm.wait_until_mysql_router_ready(event)
         # wait_until_mysql_router_ready will set WaitingStatus—override it with current charm
         # status
         self._charm.set_status(event=None)
 
-    def _enable_router(self, *, tls: bool, unit_name: str) -> None:
+    def _enable_router(self, *, event, tls: bool, unit_name: str) -> None:
         """Enable router after setting up all the necessary prerequisites."""
         logger.debug("Enabling MySQL Router service")
         self._cleanup_after_upgrade_or_potential_container_restart()
         # create an empty credentials file, if the file does not exist
         self._container.create_router_rest_api_credentials_file()
-        self._bootstrap_router(tls=tls)
+        self._bootstrap_router(event=event, tls=tls)
         self.shell.add_attributes_to_mysql_router_user(
             username=self._router_username, router_id=self._router_id, unit_name=unit_name
         )
         self._container.update_mysql_router_service(enabled=True, tls=tls)
         self._logrotate.enable()
         logger.debug("Enabled MySQL Router service")
-        self._charm.wait_until_mysql_router_ready()
+        self._charm.wait_until_mysql_router_ready(event)
 
     def _enable_exporter(
         self, *, tls: bool, exporter_config: "relations.cos.ExporterConfig"
@@ -356,6 +359,7 @@ def _enable_exporter(
     def reconcile(
         self,
         *,
+        event,
         tls: bool,
         unit_name: str,
         exporter_config: "relations.cos.ExporterConfig",
@@ -376,23 +380,23 @@ def reconcile(
                 key=key, certificate=certificate, certificate_authority=certificate_authority
             )
             if custom_certificate != certificate and self._container.mysql_router_service_enabled:
-                self._restart(tls=tls)
+                self._restart(event=event, tls=tls)
         else:
             self._disable_tls()
             if custom_certificate and self._container.mysql_router_service_enabled:
-                self._restart(tls=tls)
+                self._restart(event=event, tls=tls)
 
         # If the host or port changes, MySQL Router will receive topology change
         # notifications from MySQL.
         # Therefore, if the host or port changes, we do not need to restart MySQL Router.
-        is_charm_exposed = self._charm.is_externally_accessible()
+        is_charm_exposed = self._charm.is_externally_accessible(event=event)
         socket_file_exists = self._container.path("/run/mysqlrouter/mysql.sock").exists()
         require_rebootstrap = is_charm_exposed == socket_file_exists
         if require_rebootstrap:
             self._disable_router()
 
         if not self._container.mysql_router_service_enabled:
-            self._enable_router(tls=tls, unit_name=unit_name)
+            self._enable_router(event=event, tls=tls, unit_name=unit_name)
 
         if (not self._container.mysql_router_exporter_service_enabled and exporter_config) or (
             self._container.mysql_router_exporter_service_enabled
@@ -417,7 +421,7 @@ def status(self) -> typing.Optional[ops.StatusBase]:
             )
 
     def upgrade(
-        self, *, unit: ops.Unit, tls: bool, exporter_config: "relations.cos.ExporterConfig"
+        self, *, event, unit: ops.Unit, tls: bool, exporter_config: "relations.cos.ExporterConfig"
     ) -> None:
         enabled = self._container.mysql_router_service_enabled
         exporter_enabled = self._container.mysql_router_exporter_service_enabled
@@ -429,7 +433,7 @@ def upgrade(
         super().upgrade(unit=unit, tls=tls, exporter_config=exporter_config)
         if enabled:
             logger.debug("Re-enabling MySQL Router service after upgrade")
-            self._enable_router(tls=tls, unit_name=unit.name)
+            self._enable_router(event=event, tls=tls, unit_name=unit.name)
         if exporter_enabled:
             self._enable_exporter(tls=tls, exporter_config=exporter_config)