From 47ad1804c48c1b2ae0e16f836a6e0039a31c9901 Mon Sep 17 00:00:00 2001
From: sd109 <sdavidson327@gmail.com>
Date: Mon, 4 Nov 2024 16:43:56 +0000
Subject: [PATCH] Strip /v3 suffix from app cred auth url and improve error
 handling

---
 capi_janitor/openstack/openstack.py | 19 ++++++++++++++-----
 capi_janitor/openstack/operator.py  | 14 ++++++++++----
 2 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/capi_janitor/openstack/openstack.py b/capi_janitor/openstack/openstack.py
index 1845343..034d715 100644
--- a/capi_janitor/openstack/openstack.py
+++ b/capi_janitor/openstack/openstack.py
@@ -1,5 +1,6 @@
 import asyncio
 import contextlib
+import re
 import urllib.parse
 
 import httpx
@@ -14,6 +15,13 @@ class UnsupportedAuthenticationError(Exception):
     def __init__(self, auth_type):
         super().__init__(f"unsupported authentication type: {auth_type}")
 
+class AuthenticationError(Exception):
+    """
+    Raised when an unknown authentication error is encountered.
+    """
+    def __init__(self, user):
+        super().__init__(f"failed to authenticate as user: {user}")
+
 
 class Auth(httpx.Auth):
     """
@@ -55,7 +63,7 @@ def _build_token_request(self):
                 },
             }
         )
-    
+
     def _handle_token_response(self, response):
         response.raise_for_status()
         self._token = response.headers["X-Subject-Token"]
@@ -91,7 +99,7 @@ def _extract_list(self, response):
         # Some resources support a /detail endpoint
         # In this case, we just want to use the name up to the slash
         return response.json()[self._plural_name]
-    
+
     def _extract_next_page(self, response):
         next_url = next(
             (
@@ -131,7 +139,7 @@ def __init__(self, /, base_url, prefix = None, **kwargs):
     def __aenter__(self):
         # Prevent individual clients from being used in a context manager
         raise RuntimeError("clients must be used via a cloud object")
-    
+
     def resource(self, name, prefix = None, plural_name = None, singular_name = None):
         # If an additional prefix is given, combine it with the existing prefix
         if prefix:
@@ -199,7 +207,7 @@ def apis(self):
         The APIs supported by the cloud.
         """
         return list(self._endpoints.keys())
-    
+
     def api_client(self, name, prefix = None):
         """
         Returns a client for the named API.
@@ -218,8 +226,9 @@ def from_clouds(cls, clouds, cloud, cacert):
         config = clouds["clouds"][cloud]
         if config["auth_type"] != "v3applicationcredential":
             raise UnsupportedAuthenticationError(config["auth_type"])
+        auth_url = re.sub("/v3/?$", "", config["auth"]["auth_url"])
         auth = Auth(
-            config["auth"]["auth_url"],
+            auth_url,
             config["auth"]["application_credential_id"],
             config["auth"]["application_credential_secret"]
         )
diff --git a/capi_janitor/openstack/operator.py b/capi_janitor/openstack/operator.py
index 32cbea1..3248588 100644
--- a/capi_janitor/openstack/operator.py
+++ b/capi_janitor/openstack/operator.py
@@ -152,9 +152,15 @@ async def purge_openstack_resources(
     """
     # Use the credential to delete external resources as required
     async with openstack.Cloud.from_clouds(clouds, cloud_name, cacert) as cloud:
-        # If the session is not authenticated, there is nothing we can do
         if not cloud.is_authenticated:
-            logger.warn("application credential has been deleted")
+            if include_appcred:
+                # If the session is not authenticated then we've already
+                # cleaned up and deleted the app cred.
+                logger.warn("application credential has been deleted")
+            else:
+                # Raise an error and skip removing the finalizer to block cluster
+                # deletion to avoid leaking resources.
+                raise openstack.AuthenticationError(cloud.current_user_id)
             return
 
         # Release any floating IPs associated with loadbalancer services for the cluster
@@ -301,7 +307,7 @@ async def wrapper(**kwargs):
                 if exc.status_code != 404:
                     raise
     return wrapper
-    
+
 
 @kopf.on.event(CAPO_API_GROUP, "openstackclusters")
 @retry_event
@@ -326,7 +332,7 @@ async def on_openstackcluster_event(name, namespace, meta, spec, logger, **kwarg
             )
             logger.info("added janitor finalizer to cluster")
         return
-    
+
     # NOTE: If we get to here, the cluster is deleting
 
     # If our finalizer is not present, we don't do anything