From b95cfc7deba10fe54a1ee636124288765c018fd9 Mon Sep 17 00:00:00 2001 From: dushu Date: Wed, 21 Feb 2024 10:17:51 -0500 Subject: [PATCH] refactor: refactor the decorators used in the charm --- pyproject.toml | 12 ++-- src/charm.py | 33 +++++---- src/constants.py | 1 + src/utils.py | 103 +++++++++++---------------- tests/unit/conftest.py | 11 +++ tests/unit/test_charm.py | 76 ++++++++++++++++---- tests/unit/test_utils.py | 147 +++++++++++++++++---------------------- 7 files changed, 208 insertions(+), 175 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 1d1aaaaa..57f8f7e9 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -30,7 +30,12 @@ target-version = ["py38"] # Linting tools configuration [tool.ruff] line-length = 99 +include = ["pyproject.toml", "src/**/*.py", "tests/**/*.py", "lib/charms/glauth_k8s/**/.py"] +extend-exclude = ["__pycache__", "*.egg_info"] + +[too.ruff.lint] select = ["E", "W", "F", "C", "N", "D", "I001"] +ignore = ["D100", "D101", "D102", "D103", "D105", "D107", "E501", "N818"] extend-ignore = [ "D203", "D204", @@ -44,15 +49,12 @@ extend-ignore = [ "D409", "D413", ] -include = ["pyproject.toml", "src/**/*.py", "tests/**/*.py", "lib/charms/glauth_k8s/**/.py"] -extend-exclude = ["__pycache__", "*.egg_info"] -ignore = ["D100", "D101", "D102", "D103", "D105", "D107", "E501", "N818"] per-file-ignores = {"tests/*" = ["D100","D101","D102","D103","D104"]} -[tool.ruff.mccabe] +[tool.ruff.lint.mccabe] max-complexity = 10 -[tool.ruff.pydocstyle] +[tool.ruff.lint.pydocstyle] convention = "google" [tool.mypy] diff --git a/src/charm.py b/src/charm.py index bcfbfe0c..9dba6f20 100755 --- a/src/charm.py +++ b/src/charm.py @@ -23,6 +23,7 @@ from charms.prometheus_k8s.v0.prometheus_scrape import MetricsEndpointProvider from configs import ConfigFile, DatabaseConfig, StartTLSConfig, pebble_layer from constants import ( + CERTIFICATES_INTEGRATION_NAME, CERTIFICATES_TRANSFER_INTEGRATION_NAME, DATABASE_INTEGRATION_NAME, GLAUTH_CONFIG_DIR, @@ -57,12 +58,13 @@ from ops.pebble import ChangeError from utils import ( after_config_updated, - block_on_missing, - demand_tls_certificates, + block_when, + container_not_connected, + database_not_ready, + integration_not_exists, leader_unit, - validate_container_connectivity, - validate_database_resource, - validate_integration_exists, + tls_certificates_not_ready, + wait_when, ) logger = logging.getLogger(__name__) @@ -158,10 +160,15 @@ def _restart_glauth_service(self) -> None: "Failed to restart the service, please check the logs" ) - @validate_container_connectivity - @demand_tls_certificates - @validate_integration_exists(DATABASE_INTEGRATION_NAME, on_missing=block_on_missing) - @validate_database_resource + @block_when( + integration_not_exists(DATABASE_INTEGRATION_NAME), + integration_not_exists(CERTIFICATES_INTEGRATION_NAME), + ) + @wait_when( + container_not_connected, + database_not_ready, + tls_certificates_not_ready, + ) def _handle_event_update(self, event: HookEvent) -> None: self.unit.status = MaintenanceStatus("Configuring GLAuth container") @@ -230,7 +237,7 @@ def _on_config_changed(self, event: ConfigChangedEvent) -> None: data=self._ldap_integration.provider_base_data ) - @validate_container_connectivity + @wait_when(container_not_connected) def _on_pebble_ready(self, event: PebbleReadyEvent) -> None: if not self._container.isdir(LOG_DIR): self._container.make_dir(path=LOG_DIR, make_parents=True) @@ -239,7 +246,7 @@ def _on_pebble_ready(self, event: PebbleReadyEvent) -> None: self._handle_event_update(event) @leader_unit - @validate_database_resource + @wait_when(database_not_ready) def _on_ldap_requested(self, event: LdapRequestedEvent) -> None: if not (requirer_data := event.data): logger.error(f"The LDAP requirer {event.app.name} does not provide necessary data.") @@ -251,14 +258,14 @@ def _on_ldap_requested(self, event: LdapRequestedEvent) -> None: data=self._ldap_integration.provider_data, ) - @validate_database_resource + @wait_when(database_not_ready) def _on_auxiliary_requested(self, event: AuxiliaryRequestedEvent) -> None: self.auxiliary_provider.update_relation_app_data( relation_id=event.relation.id, data=self._auxiliary_integration.auxiliary_data, ) - @validate_container_connectivity + @wait_when(container_not_connected) def _on_cert_changed(self, event: CertChanged) -> None: try: self._certs_integration.update_certificates() diff --git a/src/constants.py b/src/constants.py index 1b949cf9..dc96d92b 100644 --- a/src/constants.py +++ b/src/constants.py @@ -8,6 +8,7 @@ LOKI_API_PUSH_INTEGRATION_NAME = "logging" PROMETHEUS_SCRAPE_INTEGRATION_NAME = "metrics-endpoint" GRAFANA_DASHBOARD_INTEGRATION_NAME = "grafana-dashboard" +CERTIFICATES_INTEGRATION_NAME = "certificates" CERTIFICATES_TRANSFER_INTEGRATION_NAME = "send-ca-cert" GLAUTH_CONFIG_DIR = PurePath("/etc/config") diff --git a/src/utils.py b/src/utils.py index 69e26aea..5349067c 100644 --- a/src/utils.py +++ b/src/utils.py @@ -12,61 +12,48 @@ logger = logging.getLogger(__name__) +ConditionEvaluation = tuple[bool, str] +Condition = Callable[[CharmBase], ConditionEvaluation] -def _default_on_missing(charm: CharmBase, event: EventBase, **kwargs: Any) -> None: - logger.debug(f"Integration {kwargs.get('integration_name')} is missing.") +def container_not_connected(charm: CharmBase) -> ConditionEvaluation: + not_connected = not charm._container.can_connect() + return not_connected, ("Container is not connected yet" if not_connected else "") -def block_on_missing(charm: CharmBase, event: EventBase, **kwargs: Any) -> None: - integration_name = kwargs.get("integration_name") - logger.debug(f"Integration {integration_name} is missing, defer event {event}.") - event.defer() - charm.unit.status = BlockedStatus(f"Missing required integration {integration_name}") +def integration_not_exists(integration_name: str) -> Condition: + def wrapped(charm: CharmBase) -> ConditionEvaluation: + not_exists = not charm.model.relations[integration_name] + return not_exists, (f"Missing integration {integration_name}" if not_exists else "") + return wrapped -def leader_unit(func: Callable) -> Callable: - @wraps(func) - def wrapper(charm: CharmBase, *args: Any, **kwargs: Any) -> Optional[Any]: - if not charm.unit.is_leader(): - return None - - return func(charm, *args, **kwargs) - - return wrapper - - -def validate_container_connectivity(func: Callable) -> Callable: - @wraps(func) - def wrapper(charm: CharmBase, *args: EventBase, **kwargs: Any) -> Optional[Any]: - event, *_ = args - logger.debug(f"Handling event: {event}.") - if not charm._container.can_connect(): - logger.debug(f"Cannot connect to container, defer event {event}.") - event.defer() - - charm.unit.status = WaitingStatus("Waiting to connect to container") - return None - return func(charm, *args, **kwargs) +def tls_certificates_not_ready(charm: CharmBase) -> ConditionEvaluation: + not_exists = charm.config.get("starttls_enabled", True) and not ( + charm._container.exists(SERVER_KEY) and charm._container.exists(SERVER_CERT) + ) + return not_exists, ("Missing TLS certificate and private key" if not_exists else "") - return wrapper +def database_not_ready(charm: CharmBase) -> ConditionEvaluation: + not_exists = not charm.database_requirer.is_resource_created() + return not_exists, ("Waiting for database creation" if not_exists else "") -def validate_integration_exists( - integration_name: str, on_missing: Optional[Callable] = None -) -> Callable: - on_missing_request = on_missing or _default_on_missing +def block_when(*conditions: Condition) -> Callable: def decorator(func: Callable) -> Callable: @wraps(func) def wrapper(charm: CharmBase, *args: EventBase, **kwargs: Any) -> Optional[Any]: event, *_ = args logger.debug(f"Handling event: {event}.") - if not charm.model.relations[integration_name]: - on_missing_request(charm, event, integration_name=integration_name) - return None + for condition in conditions: + resp, msg = condition(charm) + if resp: + event.defer() + charm.unit.status = BlockedStatus(msg) + return None return func(charm, *args, **kwargs) @@ -75,37 +62,31 @@ def wrapper(charm: CharmBase, *args: EventBase, **kwargs: Any) -> Optional[Any]: return decorator -def validate_database_resource(func: Callable) -> Callable: - @wraps(func) - def wrapper(charm: CharmBase, *args: EventBase, **kwargs: Any) -> Optional[Any]: - event, *_ = args - logger.debug(f"Handling event: {event}.") +def wait_when(*conditions: Condition) -> Callable: + def decorator(func: Callable) -> Callable: + @wraps(func) + def wrapper(charm: CharmBase, *args: EventBase, **kwargs: Any) -> Optional[Any]: + event, *_ = args + logger.debug(f"Handling event: {event}.") - if not charm.database_requirer.is_resource_created(): - logger.debug(f"Database has not been created yet, defer event {event}.") - event.defer() + for condition in conditions: + resp, msg = condition(charm) + if resp: + event.defer() + charm.unit.status = WaitingStatus(msg) + return None - charm.unit.status = WaitingStatus("Waiting for database creation") - return None + return func(charm, *args, **kwargs) - return func(charm, *args, **kwargs) + return wrapper - return wrapper + return decorator -def demand_tls_certificates(func: Callable) -> Callable: +def leader_unit(func: Callable) -> Callable: @wraps(func) def wrapper(charm: CharmBase, *args: Any, **kwargs: Any) -> Optional[Any]: - event, *_ = args - logger.debug(f"Handling event: {event}.") - - if charm.config.get("starttls_enabled", True) and not ( - charm._container.exists(SERVER_KEY) and charm._container.exists(SERVER_CERT) - ): - logger.debug(f"TLS certificate and private key not ready. defer event {event}.") - event.defer() - - charm.unit.status = BlockedStatus("Missing required TLS certificate and private key") + if not charm.unit.is_leader(): return None return func(charm, *args, **kwargs) diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index 960d47b9..0e1cb8d9 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -8,6 +8,7 @@ from charm import GLAuthCharm from charms.glauth_k8s.v0.ldap import LdapProviderData from constants import ( + CERTIFICATES_INTEGRATION_NAME, CERTIFICATES_TRANSFER_INTEGRATION_NAME, DATABASE_INTEGRATION_NAME, WORKLOAD_CONTAINER, @@ -40,6 +41,7 @@ "password": DB_PASSWORD, } +CERTIFICATE_PROVIDER_APP = "self-signed-certificates" CERTIFICATES_TRANSFER_CLIENT_APP = "sssd" @@ -186,6 +188,15 @@ def ldap_auxiliary_relation_data(harness: Harness, ldap_auxiliary_relation: int) return LDAP_AUXILIARY_RELATION_DATA +@pytest.fixture +def certificates_relation(harness: Harness) -> int: + relation_id = harness.add_relation( + CERTIFICATES_INTEGRATION_NAME, + CERTIFICATE_PROVIDER_APP, + ) + return relation_id + + @pytest.fixture def certificates_transfer_relation(harness: Harness) -> int: relation_id = harness.add_relation( diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index 3326c9e0..d6a7bf90 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -61,29 +61,53 @@ def test_on_remove(self, harness: Harness, mocked_configmap: MagicMock) -> None: class TestPebbleReadyEvent: - def test_when_container_not_connected(self, harness: Harness) -> None: + def test_when_container_not_connected( + self, + harness: Harness, + database_relation: int, + certificates_relation: int, + ) -> None: harness.set_can_connect(WORKLOAD_CONTAINER, False) container = harness.model.unit.get_container(WORKLOAD_CONTAINER) harness.charm.on.glauth_pebble_ready.emit(container) assert isinstance(harness.model.unit.status, WaitingStatus) - def test_when_missing_database_relation(self, harness: Harness) -> None: + def test_when_missing_database_relation( + self, + harness: Harness, + certificates_relation: int, + ) -> None: container = harness.model.unit.get_container(WORKLOAD_CONTAINER) harness.charm.on.glauth_pebble_ready.emit(container) assert isinstance(harness.model.unit.status, BlockedStatus) - def test_when_tls_certificates_not_exist(self, harness: Harness) -> None: + def test_when_missing_certificates_relation( + self, + harness: Harness, + database_relation: int, + ) -> None: + harness.charm.on.config_changed.emit() + + assert isinstance(harness.model.unit.status, BlockedStatus) + + def test_when_tls_certificates_not_exist( + self, + harness: Harness, + certificates_relation: int, + database_resource: MagicMock, + ) -> None: container = harness.model.unit.get_container(WORKLOAD_CONTAINER) harness.charm.on.glauth_pebble_ready.emit(container) - assert isinstance(harness.model.unit.status, BlockedStatus) + assert isinstance(harness.model.unit.status, WaitingStatus) def test_when_database_not_created( self, harness: Harness, database_relation: int, + certificates_relation: int, mocked_tls_certificates: MagicMock, ) -> None: container = harness.model.unit.get_container(WORKLOAD_CONTAINER) @@ -94,7 +118,7 @@ def test_when_database_not_created( def test_pebble_ready_event( self, harness: Harness, - database_relation: int, + certificates_relation: int, database_resource: MagicMock, mocked_tls_certificates: MagicMock, ) -> None: @@ -112,7 +136,7 @@ def test_database_created_event( self, harness: Harness, mocked_tls_certificates: MagicMock, - database_relation: int, + certificates_relation: int, database_resource: MagicMock, ) -> None: container = harness.model.unit.get_container(WORKLOAD_CONTAINER) @@ -123,33 +147,50 @@ def test_database_created_event( class TestConfigChangedEvent: - def test_when_container_not_connected(self, harness: Harness) -> None: + def test_when_container_not_connected( + self, + harness: Harness, + database_relation: int, + certificates_relation: int, + ) -> None: harness.set_can_connect(WORKLOAD_CONTAINER, False) harness.charm.on.config_changed.emit() assert isinstance(harness.model.unit.status, WaitingStatus) - def test_when_tls_certificates_not_exist( + def test_when_missing_database_relation( self, harness: Harness, - database_relation: MagicMock, - database_resource: MagicMock, + certificates_relation: int, ) -> None: harness.charm.on.config_changed.emit() assert isinstance(harness.model.unit.status, BlockedStatus) - def test_when_missing_database_relation( - self, harness: Harness, mocked_tls_certificates: MagicMock + def test_when_missing_certificates_relation( + self, + harness: Harness, + database_relation: int, ) -> None: harness.charm.on.config_changed.emit() assert isinstance(harness.model.unit.status, BlockedStatus) + def test_when_tls_certificates_not_exist( + self, + harness: Harness, + certificates_relation: int, + database_resource: MagicMock, + ) -> None: + harness.charm.on.config_changed.emit() + + assert isinstance(harness.model.unit.status, WaitingStatus) + def test_when_database_not_created( self, harness: Harness, database_relation: int, + certificates_relation: int, mocked_tls_certificates: MagicMock, ) -> None: harness.charm.on.config_changed.emit() @@ -159,7 +200,7 @@ def test_when_database_not_created( def test_on_config_changed_event( self, harness: Harness, - database_relation: int, + certificates_relation: int, database_resource: MagicMock, mocked_tls_certificates: MagicMock, ) -> None: @@ -174,13 +215,18 @@ def test_on_config_changed_event( class TestLdapRequestedEvent: def test_when_database_not_created( - self, harness: Harness, database_relation: int, ldap_relation_data: MagicMock + self, + harness: Harness, + database_relation: int, + certificates_relation: int, + ldap_relation_data: MagicMock, ) -> None: assert isinstance(harness.model.unit.status, WaitingStatus) def test_when_requirer_data_not_ready( self, harness: Harness, + certificates_relation: int, database_resource: MagicMock, ldap_relation: int, ) -> None: @@ -190,6 +236,7 @@ def test_when_ldap_requested( self, harness: Harness, mocked_tls_certificates: MagicMock, + certificates_relation: int, database_resource: MagicMock, mocked_ldap_integration: MagicMock, ldap_relation: int, @@ -214,6 +261,7 @@ def test_on_ldap_auxiliary_requested( self, harness: Harness, mocked_tls_certificates: MagicMock, + certificates_relation: int, database_resource: MagicMock, ldap_auxiliary_relation: int, ldap_auxiliary_relation_data: MagicMock, diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py index 22d1abc9..8739c321 100644 --- a/tests/unit/test_utils.py +++ b/tests/unit/test_utils.py @@ -10,130 +10,113 @@ from ops.testing import Harness from utils import ( after_config_updated, - block_on_missing, - demand_tls_certificates, + block_when, + container_not_connected, + database_not_ready, + integration_not_exists, leader_unit, - validate_container_connectivity, - validate_database_resource, - validate_integration_exists, + tls_certificates_not_ready, + wait_when, ) -class TestUtils: - def test_leader_unit(self, harness: Harness) -> None: - @leader_unit - def wrapped_func(charm: CharmBase) -> sentinel: - return sentinel - - assert wrapped_func(harness.charm) is sentinel +class TestConditions: + def test_container_not_connected(self, harness: Harness) -> None: + harness.set_can_connect(WORKLOAD_CONTAINER, False) + res, msg = container_not_connected(harness.charm) - def test_not_leader_unit(self, harness: Harness) -> None: - @leader_unit - def wrapped(charm: CharmBase) -> sentinel: - return sentinel + assert res is True and msg - harness.set_leader(False) + def test_container_connected(self, harness: Harness) -> None: + res, msg = container_not_connected(harness.charm) - assert wrapped(harness.charm) is None + assert res is False and not msg - def test_container_connected(self, harness: Harness, mocked_hook_event: MagicMock) -> None: - @validate_container_connectivity - def wrapped(charm: CharmBase, event: HookEvent) -> sentinel: - return sentinel + def test_integration_not_exists(self, harness: Harness) -> None: + condition = integration_not_exists(DATABASE_INTEGRATION_NAME) + res, msg = condition(harness.charm) - harness.set_can_connect(WORKLOAD_CONTAINER, True) + assert res is True and msg - assert wrapped(harness.charm, mocked_hook_event) is sentinel + def test_integration_exists(self, harness: Harness, database_relation: int) -> None: + condition = integration_not_exists(DATABASE_INTEGRATION_NAME) + res, msg = condition(harness.charm) - def test_container_not_connected(self, harness: Harness, mocked_hook_event: MagicMock) -> None: - @validate_container_connectivity - def wrapped(charm: CharmBase, event: HookEvent) -> sentinel: - return sentinel + assert res is False and not msg - harness.set_can_connect(WORKLOAD_CONTAINER, False) + def test_tls_certificates_not_ready(self, harness: Harness) -> None: + res, msg = tls_certificates_not_ready(harness.charm) - assert wrapped(harness.charm, mocked_hook_event) is None - assert isinstance(harness.model.unit.status, WaitingStatus) + assert res is True and msg - def test_when_relation_exists_with_block_request( - self, - harness: Harness, - database_relation: int, - mocked_hook_event: MagicMock, + def test_tls_certificates_ready( + self, harness: Harness, mocked_tls_certificates: MagicMock ) -> None: - @validate_integration_exists(DATABASE_INTEGRATION_NAME, on_missing=block_on_missing) - def wrapped(charm: CharmBase, event: HookEvent) -> sentinel: - return sentinel + res, msg = tls_certificates_not_ready(harness.charm) - assert wrapped(harness.charm, mocked_hook_event) is sentinel + assert res is False and not msg - def test_when_relation_not_exists_with_block_request( - self, harness: Harness, mocked_hook_event: MagicMock - ) -> None: - @validate_integration_exists(DATABASE_INTEGRATION_NAME, on_missing=block_on_missing) - def wrapped(charm: CharmBase, event: HookEvent) -> sentinel: - return sentinel + def test_database_not_ready(self, harness: Harness) -> None: + res, msg = database_not_ready(harness.charm) - assert wrapped(harness.charm, mocked_hook_event) is None - assert isinstance(harness.model.unit.status, BlockedStatus) + assert res is True and msg - def test_when_relation_not_exists_without_request( - self, harness: Harness, mocked_hook_event: MagicMock - ) -> None: - harness.model.unit.status = ActiveStatus() + def test_database_ready(self, harness: Harness, database_resource: MagicMock) -> None: + res, msg = database_not_ready(harness.charm) + + assert res is False and not msg + + def test_block_when(self, harness: Harness, mocked_hook_event: MagicMock) -> None: + harness.set_can_connect(WORKLOAD_CONTAINER, False) - @validate_integration_exists(DATABASE_INTEGRATION_NAME) + @block_when(container_not_connected) def wrapped(charm: CharmBase, event: HookEvent) -> sentinel: return sentinel assert wrapped(harness.charm, mocked_hook_event) is None - assert isinstance(harness.model.unit.status, ActiveStatus) + assert isinstance(harness.model.unit.status, BlockedStatus) - def test_database_resource_created( - self, harness: Harness, database_resource: MagicMock, mocked_hook_event: MagicMock - ) -> None: - @validate_database_resource + def test_not_block_when(self, harness: Harness, mocked_hook_event: MagicMock) -> None: + @block_when(container_not_connected) def wrapped(charm: CharmBase, event: HookEvent) -> sentinel: return sentinel assert wrapped(harness.charm, mocked_hook_event) is sentinel - def test_database_resource_not_created( - self, harness: Harness, mocked_hook_event: MagicMock - ) -> None: - @validate_database_resource + def test_wait_when(self, harness: Harness, mocked_hook_event: MagicMock) -> None: + harness.set_can_connect(WORKLOAD_CONTAINER, False) + + @wait_when(container_not_connected) def wrapped(charm: CharmBase, event: HookEvent) -> sentinel: return sentinel assert wrapped(harness.charm, mocked_hook_event) is None assert isinstance(harness.model.unit.status, WaitingStatus) - def test_tls_certificates_not_exist( - self, - mocked_tls_certificates: MagicMock, - harness: Harness, - mocked_hook_event: MagicMock, - ) -> None: - @demand_tls_certificates + def test_not_wait_when(self, harness: Harness, mocked_hook_event: MagicMock) -> None: + @wait_when(container_not_connected) def wrapped(charm: CharmBase, event: HookEvent) -> sentinel: - charm.unit.status = ActiveStatus() return sentinel - mocked_tls_certificates.return_value = False - assert wrapped(harness.charm, mocked_hook_event) is None - assert isinstance(harness.model.unit.status, BlockedStatus) + assert wrapped(harness.charm, mocked_hook_event) is sentinel - def test_demand_tls_certificates( - self, - harness: Harness, - mocked_hook_event: MagicMock, - mocked_tls_certificates: MagicMock, - ) -> None: - @demand_tls_certificates - def wrapped(charm: CharmBase, event: HookEvent) -> sentinel: + +class TestUtils: + def test_leader_unit(self, harness: Harness) -> None: + @leader_unit + def wrapped_func(charm: CharmBase) -> sentinel: return sentinel - assert wrapped(harness.charm, mocked_hook_event) is sentinel + assert wrapped_func(harness.charm) is sentinel + + def test_not_leader_unit(self, harness: Harness) -> None: + @leader_unit + def wrapped(charm: CharmBase) -> sentinel: + return sentinel + + harness.set_leader(False) + + assert wrapped(harness.charm) is None @patch("ops.model.Container.pull", return_value=StringIO("abc")) @patch("charm.ConfigFile.content", new_callable=PropertyMock, return_value="abc")