[DPE-8473] Handle invalid database name (databases) (#1108)

* Put container in a cached property, and update lib to handle new invalid database name and to handle storage ownership and permissions

Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>

* Update lib

Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>

* Update lib

Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>

* Update data directory permission to 0o700

Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>

* Update lib

Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>

* Update lib

Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>

* Update lib

Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>

* Update lib

Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>

---------

Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>

Author: marceloneppel

poetry.lock

@@ -17,7 +17,7 @@ jinja2 = "^3.1.6"
 lightkube = "^0.17.2"
 lightkube-models = "^1.28.1.4"
 psycopg2 = "^2.9.10"
-postgresql-charms-single-kernel = "0.0.1"
+postgresql-charms-single-kernel = "16.1.0"
 
 [tool.poetry.group.charm-libs.dependencies]
 # data_platform_libs/v0/data_interfaces.py

pyproject.toml

@@ -84,6 +84,9 @@
     ServiceStatus,
 )
 from requests import ConnectionError as RequestsConnectionError
+from single_kernel_postgresql.config.literals import (
+    Substrates,
+)
 from single_kernel_postgresql.utils.postgresql import (
     ACCESS_GROUP_IDENTITY,
     ACCESS_GROUPS,
@@ -452,10 +455,16 @@ def is_unit_stopped(self) -> bool:
         """Returns whether the unit is stopped."""
         return "stopped" in self.unit_peer_data
 
+    @cached_property
+    def _container(self) -> Container:
+        """Returns the postgresql container."""
+        return self.unit.get_container("postgresql")
+
     @property
     def postgresql(self) -> PostgreSQL:
         """Returns an instance of the object used to interact with the database."""
         return PostgreSQL(
+            substrate=Substrates.K8S,
             primary_host=self.primary_endpoint,
             current_host=self.endpoint,
             user=USER,
@@ -623,8 +632,7 @@ def _on_peer_relation_changed(self, event: HookEvent) -> None:  # noqa: C901
 
         # Update the list of the cluster members in the replicas to make them know each other.
         # Update the cluster members in this unit (updating patroni configuration).
-        container = self.unit.get_container("postgresql")
-        if not container.can_connect():
+        if not self._container.can_connect():
             logger.debug(
                 "Early exit on_peer_relation_changed: Waiting for container to become available"
             )
@@ -641,11 +649,11 @@ def _on_peer_relation_changed(self, event: HookEvent) -> None:  # noqa: C901
             logger.debug("on_peer_relation_changed early exit: Unit in blocked status")
             return
 
-        services = container.pebble.get_services(names=[self.postgresql_service])
+        services = self._container.pebble.get_services(names=[self.postgresql_service])
         if (
             (self.is_cluster_restoring_backup or self.is_cluster_restoring_to_time)
             and len(services) > 0
-            and not self._was_restore_successful(container, services[0])
+            and not self._was_restore_successful(self._container, services[0])
         ):
             logger.debug("on_peer_relation_changed early exit: Backup restore check failed")
             return
@@ -1004,7 +1012,7 @@ def _create_pgdata(self, container: Container):
         """Create the PostgreSQL data directory."""
         if not container.exists(self.pgdata_path):
             container.make_dir(
-                self.pgdata_path, permissions=0o750, user=WORKLOAD_OS_USER, group=WORKLOAD_OS_GROUP
+                self.pgdata_path, permissions=0o700, user=WORKLOAD_OS_USER, group=WORKLOAD_OS_GROUP
             )
         # Also, fix the permissions from the parent directory.
         container.exec([
@@ -1609,11 +1617,10 @@ def _check_pgdata_storage_size(self) -> None:
 
     def _on_update_status(self, _) -> None:
         """Update the unit status message."""
-        container = self.unit.get_container("postgresql")
-        if not self._on_update_status_early_exit_checks(container):
+        if not self._on_update_status_early_exit_checks(self._container):
             return
 
-        services = container.pebble.get_services(names=[self.postgresql_service])
+        services = self._container.pebble.get_services(names=[self.postgresql_service])
         if len(services) == 0:
             # Service has not been added nor started yet, so don't try to check Patroni API.
             logger.debug("on_update_status early exit: Service has not been added nor started yet")
@@ -1629,7 +1636,7 @@ def _on_update_status(self, _) -> None:
                 f"{self.postgresql_service} pebble service inactive, restarting service"
             )
             try:
-                container.restart(self.postgresql_service)
+                self._container.restart(self.postgresql_service)
             except ChangeError:
                 logger.exception("Failed to restart patroni")
             # If service doesn't recover fast, exit and wait for next hook run to re-check
@@ -1639,7 +1646,7 @@ def _on_update_status(self, _) -> None:
 
         if (
             self.is_cluster_restoring_backup or self.is_cluster_restoring_to_time
-        ) and not self._was_restore_successful(container, services[0]):
+        ) and not self._was_restore_successful(self._container, services[0]):
             return
 
         # Update the sync-standby endpoint in the async replication data.
@@ -1970,28 +1977,32 @@ def _push_file_to_workload(self, container: Container, file_path: str, file_data
 
     def push_tls_files_to_workload(self) -> bool:
         """Uploads TLS files to the workload container."""
-        container = self.unit.get_container("postgresql")
-
         key, ca, cert = self.tls.get_client_tls_files()
         if key is not None:
-            self._push_file_to_workload(container, f"{self._storage_path}/{TLS_KEY_FILE}", key)
+            self._push_file_to_workload(
+                self._container, f"{self._storage_path}/{TLS_KEY_FILE}", key
+            )
         if ca is not None:
-            self._push_file_to_workload(container, f"{self._storage_path}/{TLS_CA_FILE}", ca)
-            self._push_file_to_workload(container, f"{self._certs_path}/ca.crt", ca)
-            container.exec(["update-ca-certificates"]).wait()
+            self._push_file_to_workload(self._container, f"{self._storage_path}/{TLS_CA_FILE}", ca)
+            self._push_file_to_workload(self._container, f"{self._certs_path}/ca.crt", ca)
+            self._container.exec(["update-ca-certificates"]).wait()
         if cert is not None:
-            self._push_file_to_workload(container, f"{self._storage_path}/{TLS_CERT_FILE}", cert)
+            self._push_file_to_workload(
+                self._container, f"{self._storage_path}/{TLS_CERT_FILE}", cert
+            )
 
         key, ca, cert = self.tls.get_peer_tls_files()
         if key is not None:
             self._push_file_to_workload(
-                container, f"{self._storage_path}/peer_{TLS_KEY_FILE}", key
+                self._container, f"{self._storage_path}/peer_{TLS_KEY_FILE}", key
             )
         if ca is not None:
-            self._push_file_to_workload(container, f"{self._storage_path}/peer_{TLS_CA_FILE}", ca)
+            self._push_file_to_workload(
+                self._container, f"{self._storage_path}/peer_{TLS_CA_FILE}", ca
+            )
         if cert is not None:
             self._push_file_to_workload(
-                container, f"{self._storage_path}/peer_{TLS_CERT_FILE}", cert
+                self._container, f"{self._storage_path}/peer_{TLS_CERT_FILE}", cert
             )
 
         # CA bundle is not secret
@@ -2002,24 +2013,22 @@ def push_tls_files_to_workload(self) -> bool:
 
     def push_ca_file_into_workload(self, secret_name: str) -> bool:
         """Uploads CA certificate into the workload container."""
-        container = self.unit.get_container("postgresql")
         certificates = self.get_secret(UNIT_SCOPE, secret_name)
 
         if certificates is not None:
             self._push_file_to_workload(
-                container=container,
+                container=self._container,
                 file_path=f"{self._certs_path}/{secret_name}.crt",
                 file_data=certificates,
             )
-            container.exec(["update-ca-certificates"]).wait()
+            self._container.exec(["update-ca-certificates"]).wait()
 
         return self.update_config()
 
     def clean_ca_file_from_workload(self, secret_name: str) -> bool:
         """Cleans up CA certificate from the workload container."""
-        container = self.unit.get_container("postgresql")
-        container.remove_path(f"{self._certs_path}/{secret_name}.crt")
-        container.exec(["update-ca-certificates"]).wait()
+        self._container.remove_path(f"{self._certs_path}/{secret_name}.crt")
+        self._container.exec(["update-ca-certificates"]).wait()
 
         return self.update_config()
 
@@ -2055,57 +2064,54 @@ def _restart(self, event: RunWithLock) -> None:
 
     def _restart_metrics_service(self) -> None:
         """Restart the monitoring service if the password was rotated."""
-        container = self.unit.get_container("postgresql")
-        current_layer = container.get_plan()
+        current_layer = self._container.get_plan()
 
         metrics_service = current_layer.services[self.metrics_service]
         data_source_name = metrics_service.environment.get("DATA_SOURCE_NAME", "")
 
         if metrics_service and not data_source_name.startswith(
             f"user={MONITORING_USER} password={self.get_secret('app', MONITORING_PASSWORD_KEY)} "
         ):
-            container.add_layer(
+            self._container.add_layer(
                 self.metrics_service,
                 Layer({"services": {self.metrics_service: self._generate_metrics_service()}}),
                 combine=True,
             )
-            container.restart(self.metrics_service)
+            self._container.restart(self.metrics_service)
 
     def _restart_ldap_sync_service(self) -> None:
         """Restart the LDAP sync service in case any configuration changed."""
         if not self._patroni.member_started:
             logger.debug("Restart LDAP sync early exit: Patroni has not started yet")
             return
 
-        container = self.unit.get_container("postgresql")
-        sync_service = container.pebble.get_services(names=[self.ldap_sync_service])
+        sync_service = self._container.pebble.get_services(names=[self.ldap_sync_service])
 
         if not self.is_primary and sync_service[0].is_running():
             logger.debug("Stopping LDAP sync service. It must only run in the primary")
-            container.stop(self.ldap_sync_service)
+            self._container.stop(self.ldap_sync_service)
 
         if self.is_primary and not self.is_ldap_enabled:
             logger.debug("Stopping LDAP sync service")
-            container.stop(self.ldap_sync_service)
+            self._container.stop(self.ldap_sync_service)
             return
 
         if self.is_primary and self.is_ldap_enabled:
-            container.add_layer(
+            self._container.add_layer(
                 self.ldap_sync_service,
                 Layer({"services": {self.ldap_sync_service: self._generate_ldap_service()}}),
                 combine=True,
             )
             logger.debug("Starting LDAP sync service")
-            container.restart(self.ldap_sync_service)
+            self._container.restart(self.ldap_sync_service)
 
     @property
     def _is_workload_running(self) -> bool:
         """Returns whether the workload is running (in an active state)."""
-        container = self.unit.get_container("postgresql")
-        if not container.can_connect():
+        if not self._container.can_connect():
             return False
 
-        services = container.pebble.get_services(names=[self.postgresql_service])
+        services = self._container.pebble.get_services(names=[self.postgresql_service])
         if len(services) == 0:
             return False
 
@@ -2279,25 +2285,23 @@ def _handle_postgresql_restart_need(self):
 
     def _update_pebble_layers(self, replan: bool = True) -> None:
         """Update the pebble layers to keep the health check URL up-to-date."""
-        container = self.unit.get_container("postgresql")
-
         # Get the current layer.
-        current_layer = container.get_plan()
+        current_layer = self._container.get_plan()
 
         # Create a new config layer.
         new_layer = self._postgresql_layer()
 
         # Check if there are any changes to layer services.
         if current_layer.services != new_layer.services:
             # Changes were made, add the new layer.
-            container.add_layer(self.postgresql_service, new_layer, combine=True)
+            self._container.add_layer(self.postgresql_service, new_layer, combine=True)
             logging.info("Added updated layer 'postgresql' to Pebble plan")
             if replan:
-                container.replan()
+                self._container.replan()
                 logging.info("Restarted postgresql service")
         if current_layer.checks != new_layer.checks:
             # Changes were made, add the new layer.
-            container.add_layer(self.postgresql_service, new_layer, combine=True)
+            self._container.add_layer(self.postgresql_service, new_layer, combine=True)
             logging.info("Updated health checks")
 
     def _unit_name_to_pod_name(self, unit_name: str) -> str:

src/charm.py

@@ -18,6 +18,7 @@
     ACCESS_GROUP_RELATION,
     ACCESS_GROUPS,
     INVALID_DATABASE_NAME_BLOCKING_MESSAGE,
+    INVALID_DATABASE_NAMES,
     INVALID_EXTRA_USER_ROLE_BLOCKING_MESSAGE,
     PostgreSQLCreateDatabaseError,
     PostgreSQLCreateUserError,
@@ -441,7 +442,7 @@ def check_for_invalid_database_name(self, relation_id: int) -> bool:
             for data in relation.data.values():
                 database = data.get("database")
                 if database is not None and (
-                    len(database) > 49 or database in ["postgres", "template0", "template1"]
+                    len(database) > 49 or database in INVALID_DATABASE_NAMES
                 ):
                     return True
         return False

src/relations/postgresql_provider.py

@@ -72,6 +72,7 @@ def test_database(juju: jubilant.Juju, predefined_roles) -> None:
     """Check that an invalid database name makes the database charm block."""
     del predefined_roles[""]
     invalid_database_names = [
+        "databases",
         "postgres",
         "template0",
         "template1",

tests/integration/test_invalid_database.py

@@ -1561,7 +1561,7 @@ def test_create_pgdata(harness):
     container.exists.return_value = False
     harness.charm._create_pgdata(container)
     container.make_dir.assert_called_once_with(
-        "/var/lib/postgresql/data/pgdata", permissions=488, user="postgres", group="postgres"
+        "/var/lib/postgresql/data/pgdata", permissions=448, user="postgres", group="postgres"
     )
     container.exec.assert_has_calls([
         call(["chown", "postgres:postgres", "/var/lib/postgresql/archive"]),