Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(backoff): add backoff to Google Group API calls #1081

Merged
merged 3 commits into from
Apr 28, 2023
Merged

feat(backoff): add backoff to Google Group API calls #1081

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5ecf420
feat(backoff): add backoff to Google Group API calls
Avantol13 Mar 22, 2023
7accbd7
fix(backoff): actually use the new functions
Avantol13 Mar 22, 2023
95a11b6
Merge branch 'master' into feat/backoff
Avantol13 Apr 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions fence/config-default.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -480,6 +480,10 @@ PRIVACY_POLICY_URL: null
# In the absence of this OVERRIDE prefixed config, the legacy NGINX_RATE_LIMIT from the k8s deployment yaml is applied
OVERRIDE_NGINX_RATE_LIMIT: 18

# This is the maximum numbers of retries when exponentially backing off against an
# error from an external API.
DEFAULT_BACKOFF_SETTINGS_MAX_TRIES: 3

# //////////////////////////////////////////////////////////////////////////////////////
# SUPPORT INFO
# //////////////////////////////////////////////////////////////////////////////////////
Expand Down
20 changes: 17 additions & 3 deletions fence/resources/google/access_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,12 @@
Utilities for determine access and validity for service account
registration.
"""
import backoff
import time
import flask
from urllib.parse import unquote

from cirrus.google_cloud.iam import GooglePolicyMember

from cirrus.google_cloud.errors import GoogleAPIError
from cirrus.google_cloud.iam import GooglePolicy
from cirrus import GoogleCloudManager
Expand All @@ -32,7 +32,7 @@
get_monitoring_service_account_email,
is_google_managed_service_account,
)
from fence.utils import get_valid_expiration_from_request
from fence.utils import get_valid_expiration_from_request, DEFAULT_BACKOFF_SETTINGS

logger = get_logger(__name__)

Expand Down Expand Up @@ -77,6 +77,21 @@ def bulk_update_google_groups(google_bulk_mapping):
gcm.remove_member_from_group(member_email, group)


@backoff.on_exception(backoff.expo, Exception, **DEFAULT_BACKOFF_SETTINGS)
def _get_members_from_google_group(gcm, group):
return gcm.get_group_members(group)


@backoff.on_exception(backoff.expo, Exception, **DEFAULT_BACKOFF_SETTINGS)
def _add_member_to_google_group(gcm, add_member_to_group, group):
gcm.add_member_to_group(member_email, group)
Comment on lines +130 to +131
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we update the add_member_to_group calls to _add_member_to_google_group in bulk_update_google_groups()?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same for the other 2 functions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whoops, yes. Just pushed a commit to fix

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if you meant to add backoff to all the calls or just the bulk_update_google_groups process?

get_group_members is called in verify_bucket_access_group

add_member_to_group in add_user_service_account_to_google

remove_member_from_group in a bunch of functions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just the bulk updates, the others should happen dynamically when a user requests access to data and/or during validation job where the number of calls is much less. This is just to alleviate the quota exceeded issues during usersync



@backoff.on_exception(backoff.expo, Exception, **DEFAULT_BACKOFF_SETTINGS)
def _remove_member_to_google_group(gcm, add_member_to_group, group):
gcm.remove_member_from_group(member_email, group)


def get_google_project_number(google_project_id, google_cloud_manager):
"""
Return a project's "projectNumber" which uniquely identifies it.
Expand Down Expand Up @@ -716,7 +731,6 @@ def _revoke_user_service_account_from_google(
if not g_manager.remove_member_from_group(
member_email=service_account.email, group_id=access_group.email
):

logger.debug(
"Removed {} from google group {}".format(
service_account.email, access_group.email
Expand Down
2 changes: 1 addition & 1 deletion fence/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -421,6 +421,6 @@ def get_from_cache(item_id, memory_cache, db_cache_table, db_cache_table_id_fiel
DEFAULT_BACKOFF_SETTINGS = {
"on_backoff": log_backoff_retry,
"on_giveup": log_backoff_giveup,
"max_tries": 3,
"max_tries": config["DEFAULT_BACKOFF_SETTINGS_MAX_TRIES"],
"giveup": exception_do_not_retry,
}