Fix/workaround secret leak2 (#898)

* Delete all secrets with the matching name/id.
   This introduces two changes:
   (1) If there are many matches, we delete all matching secrets.
     Due to a previous issue, we may have many ...
   (2) We extract the UUID from the id string and pass it to
     delete_secret(). This is a workaround to something that looks
     like a bug in the SDK to me.
* Comment on the workaround. Robust against fixed SDK ...
   Let's comment on the SDK issue here.
   If it should change to return a UUID in the id attribute, we would now
   still work, so we are robust against a possible direction that a fix
   might take.
* Appease flake8.
* Cosmetic: Use plural in fn name. More SDK explanation.
  Improve comment explaining the SDK (and possibly API) misbehavior.
  Also add comment on chosen workaround with robustness in mind,
  but without taking it to the extreme of adding 5 additional API calls.
* Add wrapper _delete_secret() that accepts a Secret object.
  This keeps the code cleaner.
* More elegant UUID extraction avoiding case distinction.
   ... based on suggestion by @mbuechse.

Signed-off-by: Kurt Garloff <[email protected]>

Author: garloff

Tests/iaas/key-manager/check-for-key-manager.py

@@ -57,17 +57,39 @@ def check_presence_of_key_manager(conn: openstack.connection.Connection) -> None
             return True
 
 
-def _find_secret(conn: openstack.connection.Connection, secret_name_or_id: str):
+def _find_secrets(conn: openstack.connection.Connection, secret_name_or_id: str) -> list:
     """Replacement method for finding secrets.
 
     Mimicks the behavior of Connection.key_manager.find_secret()
     but fixes an issue with the internal implementation raising an
     exception due to an unexpected microversion parameter.
+    Unlike find_secret(), we return a list with all secrets that match.
     """
     secrets = conn.key_manager.secrets()
-    for s in secrets:
-        if s.name == secret_name_or_id or s.id == secret_name_or_id:
-            return s
+    return [s for s in secrets if s.name == secret_name_or_id or s.id == secret_name_or_id]
+
+
+def _delete_secret(conn: openstack.connection.Connection, secret: openstack.key_manager.v1.secret.Secret):
+    """Replacement method for deleting secrets
+    _delete_secret(connection, secret object)
+
+    Workaround for SDK bugs:
+     - The id field in reality is a href (containg the UUID at the end)
+     - The delete_secret() function contrary to the documentation does
+       not accept openstack.key_manager.v1.secret.Secret objects nor the
+       hrefs, just plain UUIDs.
+     - It does not return an error when I try to delete a secret passing
+       an object or href, just silently does nothing.
+    The code here assumes that the SDK (when fixed) will continue to
+    accept UUIDs as argument for delete_secret() in the future.
+    Code is robust against those being passed directly in the .id attr
+    of the objects. (It would be even more robust to try to pass the
+    object first, then the href, then the UUID extracted from the href,
+    each time checking whether it was effective. But that's three delete
+    plus list calls and very ugly.)
+    """
+    uuid = secret.id.rsplit('/', 1)[-1]
+    conn.key_manager.delete_secret(uuid)
 
 
 def check_key_manager_permissions(conn: openstack.connection.Connection) -> None:
@@ -78,9 +100,12 @@ def check_key_manager_permissions(conn: openstack.connection.Connection) -> None
     """
     secret_name = "scs-member-role-test-secret"
     try:
-        existing_secret = _find_secret(conn, secret_name)
-        if existing_secret:
-            conn.key_manager.delete_secret(existing_secret)
+        existing_secrets = _find_secrets(conn, secret_name)
+        for secret in existing_secrets:
+            _delete_secret(conn, secret)
+
+        if existing_secrets:
+            logger.debug(f'Deleted {len(existing_secrets)} secrets')
 
         conn.key_manager.create_secret(
             name=secret_name,
@@ -89,11 +114,11 @@ def check_key_manager_permissions(conn: openstack.connection.Connection) -> None
             payload="foo",
         )
         try:
-            new_secret = _find_secret(conn, secret_name)
+            new_secret = _find_secrets(conn, secret_name)
             if not new_secret:
                 raise ValueError(f"Secret '{secret_name}' was not discoverable by the user")
         finally:
-            conn.key_manager.delete_secret(new_secret)
+            _delete_secret(conn, new_secret[0])
     except openstack.exceptions.ForbiddenException:
         logger.debug('exception details', exc_info=True)
         logger.error(