Repository: keephq/keep
Pull request: feat: Add infra for cleaning up providers when deleted
<!-- 
Thanks for creating this pull request 🤗

Please make sure that the pull request is limited to one type (docs, feature, etc.) and keep it as small as possible. You can open multiple prs instead of opening a huge one.
-->

<!-- If this pull request closes an issue, please mention the issue number below -->
Closes #392

## 📑 Description
Added base functionality for cleaning up provider leftovers when it's deleted in Keep.
Now base provider has clean_up function that each inheritor can override in order to implement its own clean up logic. The function is called during provider deletion.

Add cleaning up webhooks for PagerDuty provider.

<!-- You can also choose to add a list of changes and if they have been completed or not by using the markdown to-do list syntax
- [ ] Not Completed
- [x] Completed
-->

## ✅ Checks
<!-- Make sure your pr passes the CI checks and do check the following fields as needed - -->
- [ ] My pull request adheres to the code style of this project
- [ ] My code requires changes to the documentation
- [ ] I have updated the documentation as required
- [ ] All the tests have passed

## ℹ Additional Information
<!-- Any additional information like breaking changes, dependencies added, screenshots, comparisons between new and old behavior, etc. -->


Problem statement:
Deleting a provider does not clean up the changes on the infrastructure
**Is your feature request related to a problem? Please describe.**
The delete provider option does not undo the changes or recommends you info about how to undo them. Say you add subscriptions to your sns topics, they will stay there and keep sending data to keep.

**Describe the solution you'd like**
Either clean up the changes done, or recommend actions to clean up

Hints from issue discussion:

Commit message: Log original exception when cleaning up fails

Patch to review:
diff --git a/keep/contextmanager/contextmanager.py b/keep/contextmanager/contextmanager.py
index 2dad2563b7..9ec99ed9b5 100644
--- a/keep/contextmanager/contextmanager.py
+++ b/keep/contextmanager/contextmanager.py
@@ -4,6 +4,7 @@
 import click
 from pympler.asizeof import asizeof
 
+from keep.api.core.config import config
 from keep.api.core.db import get_last_workflow_execution_by_workflow_id, get_session
 from keep.api.logging import WorkflowLoggerAdapter
 from keep.api.models.alert import AlertDto
@@ -59,6 +60,13 @@ def __init__(self, tenant_id, workflow_id=None, workflow_execution_id=None):
         self._api_key = None
         self.__loggers = {}
 
+    @property
+    def api_url(self):
+        """
+        The URL of the Keep API
+        """
+        return config("KEEP_API_URL")
+
     @property
     def api_key(self):
         # avoid circular import
diff --git a/keep/providers/base/base_provider.py b/keep/providers/base/base_provider.py
index 44e2ebe20e..3a8e55e292 100644
--- a/keep/providers/base/base_provider.py
+++ b/keep/providers/base/base_provider.py
@@ -558,6 +558,15 @@ def setup_webhook(
         """
         raise NotImplementedError("setup_webhook() method not implemented")
 
+    def clean_up(self):
+        """
+        Clean up the provider.
+
+        Raises:s
+            NotImplementedError: for providers who does not implement this method.
+        """
+        raise NotImplementedError("clean_up() method not implemented")
+
     @staticmethod
     def get_alert_schema() -> dict:
         """
diff --git a/keep/providers/pagerduty_provider/pagerduty_provider.py b/keep/providers/pagerduty_provider/pagerduty_provider.py
index d58ddff9a6..c2a69b4389 100644
--- a/keep/providers/pagerduty_provider/pagerduty_provider.py
+++ b/keep/providers/pagerduty_provider/pagerduty_provider.py
@@ -475,6 +475,41 @@ def _trigger_incident(
             )
             # This will give us a better error message in Keep workflows
             raise Exception(r.text) from e
+        
+    def clean_up(self):
+        """
+        Clean up the provider.
+        It will remove the webhook from PagerDuty if it exists.
+        """
+        self.logger.info("Cleaning up %s provider with id %s", self.PROVIDER_DISPLAY_NAME, self.provider_id)
+        keep_webhook_incidents_api_url = (
+                f"{self.context_manager.api_url}/incidents/event/{self.provider_type}?provider_id={self.provider_id}"
+            )
+        headers = self.__get_headers()
+        request = requests.get(self.SUBSCRIPTION_API_URL, headers=headers)
+        if not request.ok:
+            raise Exception("Could not get existing webhooks")
+        existing_webhooks = request.json().get("webhook_subscriptions", [])
+        webhook_exists = next(
+            iter(
+                [
+                    webhook
+                    for webhook in existing_webhooks
+                    if keep_webhook_incidents_api_url == webhook.get("delivery_method", {}).get("url", "")
+                ]
+            ),
+            False,
+        )
+        if webhook_exists:
+            self.logger.info("Webhook exists, removing it")
+            webhook_id = webhook_exists.get("id")
+            request = requests.delete(
+                f"{self.SUBSCRIPTION_API_URL}/{webhook_id}", headers=headers
+            )
+            if not request.ok:
+                raise Exception("Could not remove existing webhook")
+            self.logger.info("Webhook removed", extra={"webhook_id": webhook_id})
+
 
     def dispose(self):
         """
diff --git a/keep/providers/providers_service.py b/keep/providers/providers_service.py
index b58a2c0a2c..3cc2d6e38b 100644
--- a/keep/providers/providers_service.py
+++ b/keep/providers/providers_service.py
@@ -227,34 +227,49 @@ def update_provider(
     def delete_provider(
         tenant_id: str, provider_id: str, session: Session, allow_provisioned=False
     ):
-        provider = session.exec(
+        provider_model: Provider = session.exec(
             select(Provider).where(
                 (Provider.tenant_id == tenant_id) & (Provider.id == provider_id)
             )
         ).one_or_none()
 
-        if not provider:
+        if not provider_model:
             raise HTTPException(404, detail="Provider not found")
 
-        if provider.provisioned and not allow_provisioned:
+        if provider_model.provisioned and not allow_provisioned:
             raise HTTPException(403, detail="Cannot delete a provisioned provider")
 
         context_manager = ContextManager(tenant_id=tenant_id)
         secret_manager = SecretManagerFactory.get_secret_manager(context_manager)
+        config = secret_manager.read_secret(provider_model.configuration_key, is_json=True)
+        provider = ProvidersFactory.get_provider(
+                context_manager, provider_model.id, provider_model.type, config
+            )
 
         try:
-            secret_manager.delete_secret(provider.configuration_key)
+            secret_manager.delete_secret(provider_model.configuration_key)
         except Exception:
             logger.exception("Failed to delete the provider secret")
 
-        if provider.consumer:
+        if provider_model.consumer:
             try:
                 event_subscriber = EventSubscriber.get_instance()
-                event_subscriber.remove_consumer(provider)
+                event_subscriber.remove_consumer(provider_model)
             except Exception:
                 logger.exception("Failed to unregister provider as a consumer")
 
-        session.delete(provider)
+        try:
+            provider.clean_up()
+        except NotImplementedError:
+            logger.info(
+                "Being deleted provider of type %s does not have a clean_up method",
+                provider_model.type
+            )
+        except Exception as exc:
+            logger.exception(msg="Failed to clean up provider",
+                             extra={ "error_msg": str(exc) })
+
+        session.delete(provider_model)
         session.commit()
 
     @staticmethod


Code review comments:
diff_hunk: @@ -227,34 +227,49 @@ def update_provider(
     def delete_provider(
         tenant_id: str, provider_id: str, session: Session, allow_provisioned=False
     ):
-        provider = session.exec(
+        provider_model: Provider = session.exec(
             select(Provider).where(
                 (Provider.tenant_id == tenant_id) & (Provider.id == provider_id)
             )
         ).one_or_none()
 
-        if not provider:
+        if not provider_model:
             raise HTTPException(404, detail="Provider not found")
 
-        if provider.provisioned and not allow_provisioned:
+        if provider_model.provisioned and not allow_provisioned:
             raise HTTPException(403, detail="Cannot delete a provisioned provider")
 
         context_manager = ContextManager(tenant_id=tenant_id)
         secret_manager = SecretManagerFactory.get_secret_manager(context_manager)
+        config = secret_manager.read_secret(provider_model.configuration_key, is_json=True)
+        provider = ProvidersFactory.get_provider(
+                context_manager, provider_model.id, provider_model.type, config
+            )
 
         try:
-            secret_manager.delete_secret(provider.configuration_key)
+            secret_manager.delete_secret(provider_model.configuration_key)
         except Exception:
             logger.exception("Failed to delete the provider secret")
 
-        if provider.consumer:
+        if provider_model.consumer:
             try:
                 event_subscriber = EventSubscriber.get_instance()
-                event_subscriber.remove_consumer(provider)
+                event_subscriber.remove_consumer(provider_model)
             except Exception:
                 logger.exception("Failed to unregister provider as a consumer")
 
-        session.delete(provider)
+        try:
+            provider.clean_up()
+        except NotImplementedError:
+            logger.info(
+                "Being deleted provider of type %s does not have a clean_up method",
+                provider_model.type
+            )
+        except Exception as exc:
+            logger.exception(msg="Failed to clean up provider",
+                             extra={ "error_msg": str(exc) })
path: keep/providers/providers_service.py
text: @user1:
Nice PR! actually `logger.exception` already logs the exception itself alone so no need for that extra param!

@author:
Nice!
I'll fix,

@author:
Fixed :)