diff --git a/charms/garm/src/charm.py b/charms/garm/src/charm.py index 8edcac2a..ecb72009 100755 --- a/charms/garm/src/charm.py +++ b/charms/garm/src/charm.py @@ -120,6 +120,36 @@ def _parse_pre_install_scripts(raw: str) -> dict[str, str]: return {} +_PAAS_CHARM_HOOKS: typing.Final[tuple[str, ...]] = ( + "_on_config_changed", + "_on_rotate_secret_key_action", + "_on_secret_changed", + "_on_secret_storage_relation_changed", + "_on_secret_storage_relation_departed", + "_on_postgresql_database_database_created", + "_on_postgresql_database_endpoints_changed", + "_on_postgresql_database_relation_broken", + "_on_ingress_ready", + "_on_ingress_revoked", + "_on_pebble_ready", + "_on_update_status", +) + + +def _validate_paas_charm_hook_contract() -> None: + """Fail before startup if a paas-charm hook alias would stop guarding teardown.""" + base_classes = paas_charm.go.Charm.__mro__ + missing = [ + hook + for hook in _PAAS_CHARM_HOOKS + if not any(hook in base_class.__dict__ for base_class in base_classes) + ] + if missing: + raise RuntimeError( + "Unsupported paas-charm lifecycle API; missing expected hooks: " + ", ".join(missing) + ) + + class GarmCharm(paas_charm.go.Charm): """GARM charm — manages the GARM service via Pebble.""" @@ -129,50 +159,97 @@ def __init__(self, *args: typing.Any) -> None: Args: args: Passed through to CharmBase. """ + _validate_paas_charm_hook_contract() super().__init__(*args) - self.framework.observe(self.on.install, self._reconcile) - self.framework.observe(self.on.leader_elected, self._reconcile) - self.framework.observe( + for event in ( + self.on.install, + self.on.leader_elected, + self.on.update_status, self.on[GARM_CONFIGURATOR_RELATION_NAME].relation_joined, - self._reconcile, - ) - self.framework.observe( self.on[GARM_CONFIGURATOR_RELATION_NAME].relation_changed, - self._reconcile, - ) - self.framework.observe( self.on[GARM_CONFIGURATOR_RELATION_NAME].relation_departed, - self._reconcile, - ) - self.framework.observe( self.on[GARM_CONFIGURATOR_RELATION_NAME].relation_broken, - self._reconcile, - ) - self.framework.observe(self.on.get_credentials_action, self._on_get_credentials_action) - self.framework.observe( self.on[DEBUG_SSH_INTEGRATION_NAME].relation_joined, - self._reconcile, - ) - self.framework.observe( self.on[DEBUG_SSH_INTEGRATION_NAME].relation_changed, - self._reconcile, - ) - self.framework.observe( self.on[DEBUG_SSH_INTEGRATION_NAME].relation_departed, - self._reconcile, - ) - self.framework.observe( self.on[DEBUG_SSH_INTEGRATION_NAME].relation_broken, - self._reconcile, - ) - self.framework.observe(self.on.update_status, self._reconcile) + ): + self.framework.observe(event, self._reconcile) + + self.framework.observe(self.on.get_credentials_action, self._on_get_credentials_action) self.framework.observe(self.on.remove, self._on_remove) + def _is_tearing_down(self) -> bool: + """Return whether Juju plans no remaining units for the local application.""" + return self.app.planned_units() == 0 + + def _reconcile(self, event: ops.EventBase) -> None: + """Reconcile GARM, or handle local teardown before normal state construction.""" + if self._is_tearing_down(): + self._teardown(event) + return + self._normal_reconcile(event) + + def _reconcile_with_migrations(self, event: ops.EventBase) -> None: + """Reconcile a database event, preserving the teardown gate.""" + if self._is_tearing_down(): + self._teardown(event) + return + self._normal_reconcile_with_migrations(event) + + def _teardown(self, event: ops.EventBase) -> None: + """Handle a local teardown event without normal reconciliation.""" + logger.info( + "Skipping normal GARM reconciliation for %s during local teardown", + event.handle.kind, + ) + @block_if_invalid_data - def _reconcile(self, _: ops.EventBase) -> None: - """Reconcile charm state.""" + def _normal_reconcile(self, _: ops.EventBase) -> None: + """Reconcile active GARM charm state.""" self.restart() + @block_if_invalid_data + def _normal_reconcile_with_migrations(self, _: ops.EventBase) -> None: + """Reconcile active GARM state and rerun database migrations.""" + self.restart(rerun_migrations=True) + + def _route_reconcile(self, event: ops.EventBase) -> None: + """Route an inherited framework event through GARM's teardown gate.""" + self._reconcile(event) + + def _route_reconcile_with_migrations(self, event: ops.EventBase) -> None: + """Route an inherited database event through GARM's migration gate.""" + self._reconcile_with_migrations(event) + + # PaasCharm.__init__ resolves these hook names dynamically. Keep all aliases together so + # the teardown adapters remain visible as one compatibility boundary. The contract check + # above makes this adapter fail loudly when the base API changes. + _on_config_changed = _route_reconcile + _on_secret_changed = _route_reconcile + _on_secret_storage_relation_changed = _route_reconcile + _on_secret_storage_relation_departed = _route_reconcile + _on_postgresql_database_relation_broken = _route_reconcile + _on_ingress_ready = _route_reconcile + _on_ingress_revoked = _route_reconcile + _on_pebble_ready = _route_reconcile + _on_postgresql_database_database_created = _route_reconcile_with_migrations + _on_postgresql_database_endpoints_changed = _route_reconcile_with_migrations + + def _on_update_status(self, event: ops.HookEvent) -> None: + """Run the framework update-status handler only while active.""" + if self._is_tearing_down(): + logger.info("Skipping update-status handling during local teardown") + return + super()._on_update_status(event) + + def _on_rotate_secret_key_action(self, event: ops.ActionEvent) -> None: + """Reject secret rotation during teardown before the base decorator runs.""" + if self._is_tearing_down(): + event.fail("cannot rotate the secret key during local teardown") + return + super()._on_rotate_secret_key_action(event) + def _on_remove(self, _: ops.RemoveEvent) -> None: """Drain GARM resources before Juju removes the application.""" if not self.unit.is_leader(): @@ -287,6 +364,10 @@ def restart(self, rerun_migrations: bool = False) -> None: Args: rerun_migrations: Passed through to the parent restart. """ + if self._is_tearing_down(): + logger.info("Skipping GARM workload restart during local teardown") + return + self._ensure_secrets() if not self.is_ready(): diff --git a/charms/garm/tests/unit/test_charm.py b/charms/garm/tests/unit/test_charm.py index 0922a4cf..d15c7b26 100644 --- a/charms/garm/tests/unit/test_charm.py +++ b/charms/garm/tests/unit/test_charm.py @@ -30,6 +30,7 @@ except ImportError: import tomli as tomllib # type: ignore[no-redef] +import charm as charm_module import garm_template from charm import GARM_ADMIN_CREDENTIALS_LABEL, GARM_PORT, GARM_SECRETS_LABEL, GarmCharm from charm_state import DEBUG_SSH_INTEGRATION_NAME, GARM_CONFIGURATOR_RELATION_NAME @@ -178,9 +179,12 @@ def _state( leader: bool = True, can_connect: bool = True, postgresql_data: dict | None = None, + postgresql_local_unit_data: dict | None = None, configurator_related: bool = True, configurator_units_data: dict[int, dict] | None = None, debug_ssh_related: bool = False, + secret_storage_peers_data: dict[int, dict] | None = None, + ingress_data: dict | None = None, secrets: typing.Sequence[Secret] = (), unit_status: ops.StatusBase | None = None, app_status: ops.StatusBase | None = None, @@ -192,16 +196,31 @@ def _state( connection data, and whose single configurator unit publishes a full OpenStack provider and scaleset spec — the state in which restart() runs end to end. """ + postgresql_relation = Relation( + endpoint="postgresql", + remote_app_name="postgresql", + remote_app_data=_POSTGRESQL_DATA if postgresql_data is None else postgresql_data, + ) + if postgresql_local_unit_data is not None: + postgresql_relation = dataclasses.replace( + postgresql_relation, local_unit_data=postgresql_local_unit_data + ) relations: list[Relation | PeerRelation] = [ - Relation( - endpoint="postgresql", - remote_app_name="postgresql", - remote_app_data=_POSTGRESQL_DATA if postgresql_data is None else postgresql_data, - ), + postgresql_relation, PeerRelation( - endpoint="secret-storage", local_app_data={_SECRET_STORAGE_KEY: "peer-secret-key"} + endpoint="secret-storage", + local_app_data={_SECRET_STORAGE_KEY: "peer-secret-key"}, + peers_data={} if secret_storage_peers_data is None else secret_storage_peers_data, ), ] + if ingress_data is not None: + relations.append( + Relation( + endpoint="ingress", + remote_app_name="traefik", + remote_app_data=ingress_data, + ) + ) if configurator_related: relations.append( Relation( @@ -1013,15 +1032,54 @@ def test_controller_urls_derive_from_the_ingress_url( # --- Event wiring ------------------------------------------------------------------------- +@dataclasses.dataclass(frozen=True) +class _EventCase: + """A Scenario event and the state needed to emit it.""" + + emit: typing.Callable[[Context, State], object] + state_factory: typing.Callable[[], State] + + +def _event_case( + emit: typing.Callable[[Context, State], object], + state_factory: typing.Callable[[], State] = _state, +) -> _EventCase: + """Build an event case with a default ready state.""" + return _EventCase(emit=emit, state_factory=state_factory) + + def _relation(state: State, endpoint: str) -> Relation: """Return the state's relation on the given endpoint.""" return next(relation for relation in state.relations if relation.endpoint == endpoint) # type: ignore[return-value] +def _ready_state() -> State: + """Return a ready state with both GARM relation types present.""" + return _state(debug_ssh_related=True) + + +def _postgresql_relation_changed(ctx: Context, state: State) -> object: + """Emit the raw relation event that produces a database custom event.""" + return ctx.on.relation_changed(_relation(state, "postgresql")) + + +def _postgresql_endpoints_changed_state() -> State: + """Return a state whose PostgreSQL endpoint differs from the previous snapshot.""" + previous_data = {**_POSTGRESQL_DATA, "endpoints": "10.0.0.4:5432"} + return _state( + debug_ssh_related=True, + postgresql_local_unit_data={"data": json.dumps(previous_data)}, + ) + + _OBSERVED_EVENTS = [ - pytest.param(lambda ctx, _: ctx.on.install(), id="install"), - pytest.param(lambda ctx, _: ctx.on.leader_elected(), id="leader-elected"), - pytest.param(lambda ctx, _: ctx.on.update_status(), id="update-status"), + pytest.param(_event_case(lambda ctx, _: ctx.on.install(), _ready_state), id="install"), + pytest.param( + _event_case(lambda ctx, _: ctx.on.leader_elected(), _ready_state), id="leader-elected" + ), + pytest.param( + _event_case(lambda ctx, _: ctx.on.update_status(), _ready_state), id="update-status" + ), ] for _endpoint, _id in ( (GARM_CONFIGURATOR_RELATION_NAME, "configurator"), @@ -1029,35 +1087,60 @@ def _relation(state: State, endpoint: str) -> Relation: ): _OBSERVED_EVENTS += [ pytest.param( - lambda ctx, state, endpoint=_endpoint: ctx.on.relation_joined( - _relation(state, endpoint) + _event_case( + lambda ctx, state, endpoint=_endpoint: ctx.on.relation_joined( + _relation(state, endpoint) + ), + _ready_state, ), id=f"{_id}-relation-joined", ), pytest.param( - lambda ctx, state, endpoint=_endpoint: ctx.on.relation_changed( - _relation(state, endpoint) + _event_case( + lambda ctx, state, endpoint=_endpoint: ctx.on.relation_changed( + _relation(state, endpoint) + ), + _ready_state, ), id=f"{_id}-relation-changed", ), pytest.param( - lambda ctx, state, endpoint=_endpoint: ctx.on.relation_departed( - _relation(state, endpoint), remote_unit=0 + _event_case( + lambda ctx, state, endpoint=_endpoint: ctx.on.relation_departed( + _relation(state, endpoint), remote_unit=0 + ), + _ready_state, ), id=f"{_id}-relation-departed", ), pytest.param( - lambda ctx, state, endpoint=_endpoint: ctx.on.relation_broken( - _relation(state, endpoint) + _event_case( + lambda ctx, state, endpoint=_endpoint: ctx.on.relation_broken( + _relation(state, endpoint) + ), + _ready_state, ), id=f"{_id}-relation-broken", ), ] -@pytest.mark.parametrize("event", _OBSERVED_EVENTS) +_MIGRATION_EVENTS = [ + pytest.param( + _event_case(_postgresql_relation_changed, _ready_state), + id="postgresql-database-created", + ), + pytest.param( + _event_case(_postgresql_relation_changed, _postgresql_endpoints_changed_state), + id="postgresql-endpoints-changed", + ), +] +_OBSERVED_EVENTS += _MIGRATION_EVENTS + + +@pytest.mark.parametrize("event_case", _OBSERVED_EVENTS) def test_every_observed_event_reconciles( - ctx: Context, garm_api: _GarmApiMocks, event: typing.Callable + ctx: Context, garm_api: _GarmApiMocks, event_case: _EventCase ): """ arrange: A ready charm related to garm-configurator and debug-ssh. @@ -1065,8 +1148,326 @@ def test_every_observed_event_reconciles( assert: Every one drives a full reconcile. The charm holds no per-event delta logic, so an observer that is missed leaves GARM unsynced until the next hook happens to fire. """ - state = _state(debug_ssh_related=True) + state = event_case.state_factory() - ctx.run(event(ctx, state), state) + ctx.run(event_case.emit(ctx, state), state) garm_api.auth.from_login.assert_called_once() + + +@pytest.mark.parametrize("event_case", _MIGRATION_EVENTS) +def test_database_events_reconcile_with_migrations(ctx: Context, event_case: _EventCase): + """ + arrange: An active charm receives a PostgreSQL database lifecycle event. + act: Run database-created or endpoints-changed. + assert: The active route calls restart with migrations enabled. + """ + state = event_case.state_factory() + + with patch.object(GarmCharm, "restart") as restart: + ctx.run(event_case.emit(ctx, state), state) + + restart.assert_called_once_with(rerun_migrations=True) + + +# The external secret is included in State because Scenario requires the exact object for +# secret-changed events. Teardown must not resolve it through normal charm-state construction. +_TEARDOWN_SECRET = Secret(id="secret:externalabcdefghijkl", tracked_content={"value": "external"}) + + +def _teardown_state() -> State: + """Return a state for events that explicitly enter the teardown fallthrough.""" + return _state(debug_ssh_related=True, planned_units=0, secrets=[_TEARDOWN_SECRET]) + + +def _teardown_endpoints_changed_state() -> State: + """Return a teardown state that emits PostgreSQL endpoints-changed.""" + state = _postgresql_endpoints_changed_state() + return dataclasses.replace(state, planned_units=0, secrets=[_TEARDOWN_SECRET]) + + +_TEARDOWN_EVENTS = [ + pytest.param( + _event_case(lambda ctx, _: ctx.on.config_changed(), _teardown_state), id="config-changed" + ), + pytest.param( + _event_case( + lambda ctx, _: ctx.on.secret_changed(_TEARDOWN_SECRET), + _teardown_state, + ), + id="secret-changed", + ), + pytest.param( + _event_case(lambda ctx, _: ctx.on.update_status(), _teardown_state), id="update-status" + ), + pytest.param( + _event_case( + lambda ctx, state: ctx.on.pebble_ready(state.get_container(CONTAINER_NAME)), + _teardown_state, + ), + id="pebble-ready", + ), + pytest.param( + _event_case( + lambda ctx, state: ctx.on.relation_departed( + _relation(state, GARM_CONFIGURATOR_RELATION_NAME), remote_unit=0 + ), + _teardown_state, + ), + id="configurator-relation-departed", + ), + pytest.param( + _event_case( + lambda ctx, state: ctx.on.relation_broken( + _relation(state, GARM_CONFIGURATOR_RELATION_NAME) + ), + _teardown_state, + ), + id="configurator-relation-broken", + ), + pytest.param( + _event_case( + lambda ctx, state: ctx.on.relation_departed( + _relation(state, DEBUG_SSH_INTEGRATION_NAME), remote_unit=0 + ), + _teardown_state, + ), + id="debug-ssh-relation-departed", + ), + pytest.param( + _event_case( + lambda ctx, state: ctx.on.relation_broken( + _relation(state, DEBUG_SSH_INTEGRATION_NAME) + ), + _teardown_state, + ), + id="debug-ssh-relation-broken", + ), + pytest.param( + _event_case( + lambda ctx, state: ctx.on.relation_broken(_relation(state, "postgresql")), + _teardown_state, + ), + id="postgresql-relation-broken", + ), + pytest.param( + _event_case(_postgresql_relation_changed, _teardown_state), + id="postgresql-database-created", + ), + pytest.param( + _event_case(_postgresql_relation_changed, _teardown_endpoints_changed_state), + id="postgresql-endpoints-changed", + ), +] + + +# These callbacks still need to be intercepted before paas-charm reconstructs state, but they +# have no cleanup-specific behavior in this change. Keep them separate from the explicit teardown +# cases so adding a guard does not accidentally expand the cleanup fallthrough. +_INHERITED_GUARDED_EVENTS = [ + pytest.param( + _event_case( + lambda ctx, state: ctx.on.relation_changed(_relation(state, "secret-storage")), + lambda: _state( + debug_ssh_related=True, + planned_units=0, + secrets=[_TEARDOWN_SECRET], + secret_storage_peers_data={1: {}}, + ), + ), + id="secret-storage-relation-changed", + ), + pytest.param( + _event_case( + lambda ctx, state: ctx.on.relation_departed( + _relation(state, "secret-storage"), remote_unit=1 + ), + lambda: _state( + debug_ssh_related=True, + planned_units=0, + secrets=[_TEARDOWN_SECRET], + secret_storage_peers_data={1: {}}, + ), + ), + id="secret-storage-relation-departed", + ), + pytest.param( + _event_case( + lambda ctx, state: ctx.on.relation_changed(_relation(state, "ingress")), + lambda: _state( + debug_ssh_related=True, + planned_units=0, + secrets=[_TEARDOWN_SECRET], + ingress_data={"ingress": json.dumps({"url": "https://garm.example"})}, + ), + ), + id="ingress-ready", + ), + pytest.param( + _event_case( + lambda ctx, state: ctx.on.relation_broken(_relation(state, "ingress")), + lambda: _state( + debug_ssh_related=True, + planned_units=0, + secrets=[_TEARDOWN_SECRET], + ingress_data={"ingress": json.dumps({"url": "https://garm.example"})}, + ), + ), + id="ingress-revoked", + ), +] + + +def _assert_teardown_event_is_gated( + ctx: Context, garm_api: _GarmApiMocks, event_case: _EventCase +) -> None: + """Assert that one event cannot enter normal state or GARM reconciliation.""" + state = event_case.state_factory() + + with ( + patch.object( + GarmCharm, + "_create_charm_state", + side_effect=AssertionError("charm state was created during teardown"), + ), + patch( + "charm_state.CharmState.from_charm", + side_effect=AssertionError("GARM charm state was reconstructed"), + ), + patch.object( + GarmCharm, "_ensure_secrets", side_effect=AssertionError("secrets were read") + ), + patch.object( + GarmCharm, + "_get_postgresql_config", + side_effect=AssertionError("postgresql relation was read"), + ), + patch.object( + GarmCharm, + "_get_configurator_provider_configs", + side_effect=AssertionError("configurator relation was read"), + ), + patch.object(GarmCharm, "_teardown", autospec=True) as teardown, + ): + out = ctx.run(event_case.emit(ctx, state), state) + + assert out is not None + teardown.assert_called_once() + garm_api.client.assert_not_called() + garm_api.auth.from_login.assert_not_called() + garm_api.github.assert_not_called() + garm_api.entity.assert_not_called() + garm_api.scaleset.assert_not_called() + + +@pytest.mark.parametrize("event_case", _TEARDOWN_EVENTS) +def test_teardown_events_skip_framework_state_and_garm_reconciliation( + ctx: Context, garm_api: _GarmApiMocks, event_case: _EventCase +): + """ + arrange: No GARM units remain planned, and an explicit teardown event is emitted. + act: Emit the event. + assert: The event returns without reconstructing state or reconciling GARM. + """ + _assert_teardown_event_is_gated(ctx, garm_api, event_case) + + +@pytest.mark.parametrize("event_case", _INHERITED_GUARDED_EVENTS) +def test_inherited_events_skip_normal_reconciliation_during_teardown( + ctx: Context, garm_api: _GarmApiMocks, event_case: _EventCase +): + """ + arrange: No GARM units remain planned, and an inherited callback needs interception. + act: Emit the callback's underlying event. + assert: The pre-decorator guard returns without normal state construction or reconciliation. + """ + _assert_teardown_event_is_gated(ctx, garm_api, event_case) + + +def test_teardown_handlers_skip_charm_state_creation(ctx: Context, garm_api: _GarmApiMocks): + """ + arrange: No GARM units remain planned. + act: Emit an inherited config-changed event while state creation is forbidden. + assert: The subclass observer guard returns before the framework state factory. + """ + state = _state(planned_units=0) + with patch.object( + GarmCharm, + "_create_charm_state", + side_effect=AssertionError("charm state was created during teardown"), + ): + out = ctx.run(ctx.on.config_changed(), state) + + assert out is not None + garm_api.auth.from_login.assert_not_called() + + +def test_paas_charm_hook_contract_matches_pinned_base(): + """ + arrange: The installed paas-charm base exposes the supported 1.12.x hook names. + act: Validate the lifecycle hook contract. + assert: The compatibility check accepts the pinned base API. + """ + charm_module._validate_paas_charm_hook_contract() + + +def test_paas_charm_hook_contract_fails_loudly_when_base_changes(monkeypatch): + """ + arrange: The base class no longer exposes the expected lifecycle hooks. + act: Validate the lifecycle hook contract. + assert: Validation fails before the base can register observers. + """ + + class IncompatibleBase: + pass + + monkeypatch.setattr(charm_module.paas_charm.go, "Charm", IncompatibleBase) + + with pytest.raises(RuntimeError, match="_on_config_changed"): + charm_module._validate_paas_charm_hook_contract() + + +def test_teardown_update_status_skips_ingress_refresh(ctx: Context, garm_api: _GarmApiMocks): + """ + arrange: The local GARM service is tearing down. + act: Emit update-status while ingress refresh would fail if invoked. + assert: The outer status guard returns before the base ingress refresh. + """ + state = _state(planned_units=0) + with ctx(ctx.on.update_status(), state) as manager: + with patch.object( + manager.charm._ingress, + "_publish_auto_data", + side_effect=AssertionError("ingress refresh ran during teardown"), + ): + out = manager.run() + + assert out is not None + garm_api.auth.from_login.assert_not_called() + + +def test_rotate_secret_key_fails_before_state_creation_during_teardown( + ctx: Context, garm_api: _GarmApiMocks +): + """ + arrange: No GARM units remain planned and normal state construction is forbidden. + act: Run the rotate-secret-key action. + assert: The action fails before state construction, secret reset, or restart. + """ + state = _state(planned_units=0) + with ( + patch.object( + GarmCharm, + "_create_charm_state", + side_effect=AssertionError("charm state was created during teardown"), + ), + patch.object(GarmCharm, "restart") as restart, + ): + with ctx(ctx.on.action("rotate-secret-key"), state) as manager: + with patch.object(manager.charm._secret_storage, "reset_secret_key") as reset_secret: + with pytest.raises(ActionFailed, match="local teardown"): + manager.run() + + reset_secret.assert_not_called() + restart.assert_not_called() + garm_api.auth.from_login.assert_not_called()