Merge pull request #1095 from uc-cdis/feat/more-backoffs

feat(backoff): Add backoff to give_service_account_billing_access

.gitignore

@@ -107,3 +107,4 @@ tests/resources/keys/*.pem
 
 .DS_Store
 .vscode
+.idea

.secrets.baseline

@@ -139,7 +139,7 @@
         "filename": "fence/blueprints/storage_creds/google.py",
         "hashed_secret": "1348b145fa1a555461c1b790a2f66614781091e9",
         "is_verified": false,
-        "line_number": 139
+        "line_number": 140
       }
     ],
     "fence/blueprints/storage_creds/other.py": [
@@ -189,7 +189,7 @@
         "filename": "fence/resources/google/utils.py",
         "hashed_secret": "1348b145fa1a555461c1b790a2f66614781091e9",
         "is_verified": false,
-        "line_number": 133
+        "line_number": 137
       }
     ],
     "fence/utils.py": [
@@ -241,14 +241,14 @@
         "filename": "tests/conftest.py",
         "hashed_secret": "1348b145fa1a555461c1b790a2f66614781091e9",
         "is_verified": false,
-        "line_number": 1574
+        "line_number": 1556
       },
       {
         "type": "Base64 High Entropy String",
         "filename": "tests/conftest.py",
         "hashed_secret": "227dea087477346785aefd575f91dd13ab86c108",
         "is_verified": false,
-        "line_number": 1597
+        "line_number": 1579
       }
     ],
     "tests/credentials/google/test_credentials.py": [
@@ -368,5 +368,5 @@
       }
     ]
   },
-  "generated_at": "2023-05-10T22:20:13Z"
+  "generated_at": "2023-05-15T16:30:09Z"
 }

fence/resources/google/utils.py

@@ -1,6 +1,8 @@
 import time
 import json
 import os
+
+import backoff
 from cryptography.fernet import Fernet
 import flask
 from flask import current_app
@@ -36,6 +38,8 @@
 
 from cdislogging import get_logger
 
+from fence.utils import DEFAULT_BACKOFF_SETTINGS
+
 logger = get_logger(__name__)
 
 
@@ -221,10 +225,7 @@ def give_service_account_billing_access_if_necessary(
             # the SA access to bill the project provided
             # NOTE: this may fail if our fence SA doesn't have the right permissions
             #       to add this role and update the project policy
-            with GoogleCloudManager(project_id=r_pays_project) as g_cloud_manager:
-                g_cloud_manager.give_service_account_billing_access(
-                    sa_account_id, project_id=r_pays_project
-                )
+            _give_service_account_billing_access(sa_account_id, r_pays_project)
         except Exception as exc:
             logger.error(
                 "Unable to create a custom role in Google Project {} to "
@@ -265,6 +266,14 @@ def give_service_account_billing_access_if_necessary(
         )
 
 
+@backoff.on_exception(backoff.expo, Exception, **DEFAULT_BACKOFF_SETTINGS)
+def _give_service_account_billing_access(sa_account_id, r_pays_project):
+    with GoogleCloudManager(project_id=r_pays_project) as g_cloud_manager:
+        g_cloud_manager.give_service_account_billing_access(
+            sa_account_id, project_id=r_pays_project
+        )
+
+
 def create_google_access_key(client_id, user_id, username, proxy_group_id):
     """
     Return an access key for current user and client.

tests/google/test_access_utils.py

@@ -5,7 +5,6 @@
 from sqlalchemy import or_
 
 from cirrus.errors import CirrusError
-from cirrus.google_cloud import GoogleCloudManager
 from cirrus.google_cloud.iam import GooglePolicyMember
 
 import fence

tests/google/test_utils.py

@@ -0,0 +1,34 @@
+from unittest.mock import patch, MagicMock
+
+import pytest
+
+from fence.resources.google.utils import (
+    give_service_account_billing_access_if_necessary,
+    GoogleCloudManager,
+)
+from fence.utils import DEFAULT_BACKOFF_SETTINGS
+
+
+def test_give_service_account_billing_access_if_necessary_fails(cloud_manager):
+    """
+    Tests that the GCM give_service_account_billing_access backs off the right number of times when
+    give_service_account_billing_access_if_necessary calls it.
+    """
+
+    sa_private_key = {"client_email": "paying_requestor@somewhere.com"}
+    r_pays_project = "some_project_id"
+
+    # Grab the GCM instance from the with block in _give_service_account_billing_access
+    cloud_manager_instance = cloud_manager.return_value.__enter__.return_value
+    # Set the GCM method to raise an exception.
+    cloud_manager_instance.give_service_account_billing_access.side_effect = Exception(
+        "Something's wrong"
+    )
+
+    with pytest.raises(Exception):
+        give_service_account_billing_access_if_necessary(sa_private_key, r_pays_project)
+
+    assert (
+        cloud_manager_instance.give_service_account_billing_access.call_count
+        == DEFAULT_BACKOFF_SETTINGS["max_tries"]
+    )