[DPE-3593] Only check config values against the DB in on_config_chang…

…ed (canonical#395)

* Don't check the DB if config values are the defaults

* Unit test

* Move config DB validation in _on_config_changed

src/charm.py

@@ -11,6 +11,7 @@
 import subprocess
 from typing import Dict, List, Literal, Optional, Set, get_args
 
+import psycopg2
 from charms.data_platform_libs.v0.data_interfaces import DataPeer, DataPeerUnit
 from charms.data_platform_libs.v0.data_models import TypedCharmBase
 from charms.grafana_agent.v0.cos_agent import COSAgentProvider
@@ -862,19 +863,25 @@ def _on_leader_elected(self, event: LeaderElectedEvent) -> None:
         else:
             self.unit.status = WaitingStatus(PRIMARY_NOT_REACHABLE_MESSAGE)
 
-    def _on_config_changed(self, _) -> None:
+    def _on_config_changed(self, event) -> None:
         """Handle configuration changes, like enabling plugins."""
         if not self.is_cluster_initialised:
-            logger.debug("Early exit on_config_changed: cluster not initialised yet")
+            logger.debug("Defer on_config_changed: cluster not initialised yet")
+            event.defer()
             return
 
         if not self.upgrade.idle:
-            logger.debug("Early exit on_config_changed: upgrade in progress")
+            logger.debug("Defer on_config_changed: upgrade in progress")
+            event.defer()
             return
-
         try:
+            self._validate_config_options()
             # update config on every run
             self.update_config()
+        except psycopg2.OperationalError:
+            logger.debug("Defer on_config_changed: Cannot connect to database")
+            event.defer()
+            return
         except ValueError as e:
             self.unit.status = BlockedStatus("Configuration Error. Please check the logs")
             logger.error("Invalid configuration: %s", str(e))
@@ -1476,7 +1483,6 @@ def update_config(self, is_creating_backup: bool = False) -> bool:
         if not self._can_connect_to_postgresql:
             logger.warning("Early exit update_config: Cannot connect to Postgresql")
             return False
-        self._validate_config_options()
 
         self._patroni.bulk_update_parameters_controller_by_patroni({
             "max_connections": max(4 * os.cpu_count(), 100),
@@ -1504,24 +1510,18 @@ def update_config(self, is_creating_backup: bool = False) -> bool:
     def _validate_config_options(self) -> None:
         """Validates specific config options that need access to the database or to the TLS status."""
         if (
-            self.config.instance_default_text_search_config is not None
-            and self.config.instance_default_text_search_config
+            self.config.instance_default_text_search_config
             not in self.postgresql.get_postgresql_text_search_configs()
         ):
-            raise Exception(
+            raise ValueError(
                 "instance_default_text_search_config config option has an invalid value"
             )
 
-        if self.config.request_date_style is not None and not self.postgresql.validate_date_style(
-            self.config.request_date_style
-        ):
-            raise Exception("request_date_style config option has an invalid value")
+        if not self.postgresql.validate_date_style(self.config.request_date_style):
+            raise ValueError("request_date_style config option has an invalid value")
 
-        if (
-            self.config.request_time_zone is not None
-            and self.config.request_time_zone not in self.postgresql.get_postgresql_timezones()
-        ):
-            raise Exception("request_time_zone config option has an invalid value")
+        if self.config.request_time_zone not in self.postgresql.get_postgresql_timezones():
+            raise ValueError("request_time_zone config option has an invalid value")
 
     def _handle_postgresql_restart_need(self, enable_tls: bool) -> None:
         """Handle PostgreSQL restart need based on the TLS configuration and configuration changes."""

tests/integration/test_config.py

@@ -81,6 +81,13 @@ async def test_config_parameters(ops_test: OpsTest) -> None:
         {
             "vacuum_vacuum_freeze_table_age": ["-1", "150000000"]
         },  # config option is between 0 and 2000000000
+        {
+            "instance_default_text_search_config": [test_string, "pg_catalog.simple"]
+        },  # config option is validated against the db
+        {
+            "request_date_style": [test_string, "ISO, MDY"]
+        },  # config option is validated against the db
+        {"request_time_zone": [test_string, "UTC"]},  # config option is validated against the db
     ]
 
     charm_config = {}

tests/unit/test_charm.py

@@ -25,6 +25,7 @@
 )
 from ops.testing import Harness
 from parameterized import parameterized
+from psycopg2 import OperationalError
 from tenacity import RetryError, wait_fixed
 
 from charm import (
@@ -258,12 +259,18 @@ def test_is_cluster_initialised(self):
             )
         self.assertTrue(self.charm.is_cluster_initialised)
 
+    @patch("charm.PostgresqlOperatorCharm._validate_config_options")
     @patch("charm.PostgresqlOperatorCharm.update_config")
     @patch("relations.db.DbProvides.set_up_relation")
     @patch("charm.PostgresqlOperatorCharm.enable_disable_extensions")
     @patch("charm.PostgresqlOperatorCharm.is_cluster_initialised", new_callable=PropertyMock)
     def test_on_config_changed(
-        self, _is_cluster_initialised, _enable_disable_extensions, _set_up_relation, _update_config
+        self,
+        _is_cluster_initialised,
+        _enable_disable_extensions,
+        _set_up_relation,
+        _update_config,
+        _validate_config_options,
     ):
         # Test when the cluster was not initialised yet.
         _is_cluster_initialised.return_value = False
@@ -274,9 +281,17 @@ def test_on_config_changed(
         # Test when the unit is not the leader.
         _is_cluster_initialised.return_value = True
         self.charm.on.config_changed.emit()
+        _validate_config_options.assert_called_once()
         _enable_disable_extensions.assert_not_called()
         _set_up_relation.assert_not_called()
 
+        # Test unable to connect to db
+        _update_config.reset_mock()
+        _validate_config_options.side_effect = OperationalError
+        self.charm.on.config_changed.emit()
+        assert not _update_config.called
+        _validate_config_options.side_effect = None
+
         # Test after the cluster was initialised.
         with self.harness.hooks_disabled():
             self.harness.set_leader()
@@ -1166,7 +1181,6 @@ def test_restart(self, _are_all_members_ready, _restart_postgresql):
     @patch("charm.snap.SnapCache")
     @patch("charm.PostgresqlOperatorCharm._handle_postgresql_restart_need")
     @patch("charm.Patroni.bulk_update_parameters_controller_by_patroni")
-    @patch("charm.PostgresqlOperatorCharm._validate_config_options")
     @patch("charm.Patroni.member_started", new_callable=PropertyMock)
     @patch("charm.PostgresqlOperatorCharm._is_workload_running", new_callable=PropertyMock)
     @patch("charm.Patroni.render_patroni_yml_file")
@@ -1178,10 +1192,9 @@ def test_update_config(
         _is_workload_running,
         _member_started,
         _,
-        __,
         _handle_postgresql_restart_need,
+        __,
         ___,
-        ____,
     ):
         with patch.object(PostgresqlOperatorCharm, "postgresql", Mock()) as postgresql_mock:
             # Mock some properties.
@@ -1321,6 +1334,50 @@ def test_on_cluster_topology_change_clear_blocked(
         _primary_endpoint.assert_called_once_with()
         self.assertTrue(isinstance(self.harness.model.unit.status, ActiveStatus))
 
+    @patch("charm.PostgresqlOperatorCharm.postgresql", new_callable=PropertyMock)
+    @patch("config.subprocess")
+    def test_validate_config_options(self, _, _charm_lib):
+        _charm_lib.return_value.get_postgresql_text_search_configs.return_value = []
+        _charm_lib.return_value.validate_date_style.return_value = []
+        _charm_lib.return_value.get_postgresql_timezones.return_value = []
+
+        # Test instance_default_text_search_config exception
+        with self.harness.hooks_disabled():
+            self.harness.update_config({"instance_default_text_search_config": "pg_catalog.test"})
+
+        with self.assertRaises(ValueError) as e:
+            self.charm._validate_config_options()
+            assert (
+                e.msg == "instance_default_text_search_config config option has an invalid value"
+            )
+
+        _charm_lib.return_value.get_postgresql_text_search_configs.assert_called_once_with()
+        _charm_lib.return_value.get_postgresql_text_search_configs.return_value = [
+            "pg_catalog.test"
+        ]
+
+        # Test request_date_style exception
+        with self.harness.hooks_disabled():
+            self.harness.update_config({"request_date_style": "ISO, TEST"})
+
+        with self.assertRaises(ValueError) as e:
+            self.charm._validate_config_options()
+            assert e.msg == "request_date_style config option has an invalid value"
+
+        _charm_lib.return_value.validate_date_style.assert_called_once_with("ISO, TEST")
+        _charm_lib.return_value.validate_date_style.return_value = ["ISO, TEST"]
+
+        # Test request_time_zone exception
+        with self.harness.hooks_disabled():
+            self.harness.update_config({"request_time_zone": "TEST_ZONE"})
+
+        with self.assertRaises(ValueError) as e:
+            self.charm._validate_config_options()
+            assert e.msg == "request_time_zone config option has an invalid value"
+
+        _charm_lib.return_value.get_postgresql_timezones.assert_called_once_with()
+        _charm_lib.return_value.get_postgresql_timezones.return_value = ["TEST_ZONE"]
+
     @patch_network_get(private_address="1.1.1.1")
     @patch("charm.snap.SnapCache")
     @patch("charm.PostgresqlOperatorCharm._update_relation_endpoints")