-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(auth): Add blocking Regional Access Boundary Lookup and Seed Support #16720
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
5c3c331
c6c140f
b0163b1
17a1c3d
78bd7e6
9bbba81
5a8ad8b
87880a6
ac15926
1872dd1
28541d4
d2258f4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -361,6 +361,32 @@ def _copy_regional_access_boundary_manager(self, target): | |
| new_manager._data = self._rab_manager._data | ||
| target._rab_manager = new_manager | ||
|
|
||
| def _with_regional_access_boundary(self, seed): | ||
| """Returns a copy of these credentials with the the regional_access_boundary | ||
| set to the provided seed. This is intended for internal use only as invalid | ||
| seeds would produce unexpected results until automatic recovery is supported. | ||
| Currently this is used by the gcloud CLI and therefore changes to the | ||
| contract MUST be backwards compatible (e.g. the method signature must be | ||
| unchanged and a copy of the credenials with the RAB set must be returned). | ||
|
|
||
|
|
||
| Returns: | ||
| google.auth.credentials.Credentials: A new credentials instance. | ||
| """ | ||
| creds = self._make_copy() | ||
| creds._rab_manager.set_initial_regional_access_boundary(seed) | ||
| return creds | ||
|
|
||
| def with_blocking_regional_access_boundary_lookup(self): | ||
| """Returns a copy of these credentials with the blocking lookup mode enabled. | ||
|
|
||
| Returns: | ||
| google.auth.credentials.Credentials: A new credentials instance. | ||
| """ | ||
| creds = self._make_copy() | ||
| creds._rab_manager.use_blocking_regional_access_boundary_lookup() | ||
| return creds | ||
|
|
||
| def _maybe_start_regional_access_boundary_refresh(self, request, url): | ||
| """ | ||
| Starts a background thread to refresh the Regional Access Boundary if needed. | ||
|
|
@@ -421,11 +447,16 @@ def before_request(self, request, method, url, headers): | |
| """Refreshes the access token and triggers the Regional Access Boundary | ||
| lookup if necessary. | ||
| """ | ||
| super(CredentialsWithRegionalAccessBoundary, self).before_request( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we removed the call to super().before_request in and duplicated the logic to control the execution order, it would be good to add a test in test_credentials.py to verify that before_request correctly triggers the RAB refresh flow in sequence with the token refresh. a small, unrelated nit: I noticed that in test_before_request, the arguments for url and method are swapped in the call to before_request. It doesn't cause failures because they aren't used by the base class. If you end up adding the unit test to this file and want to do a quick cleanup, you could swap them back to the correct order! |
||
| request, method, url, headers | ||
| ) | ||
| if self._use_non_blocking_refresh: | ||
| self._non_blocking_refresh(request) | ||
| else: | ||
| self._blocking_refresh(request) | ||
|
|
||
| self._maybe_start_regional_access_boundary_refresh(request, url) | ||
|
|
||
| metrics.add_metric_header(headers, self._metric_header_for_usage()) | ||
| self.apply(headers) | ||
|
|
||
| def refresh(self, request): | ||
| """Refreshes the access token. | ||
|
|
||
|
|
@@ -435,13 +466,16 @@ def refresh(self, request): | |
| self._perform_refresh_token(request) | ||
|
|
||
| def _lookup_regional_access_boundary( | ||
| self, request: "google.auth.transport.Request" # noqa: F821 | ||
| self, | ||
| request: "google.auth.transport.Request", # noqa: F821 | ||
| blocking: bool = False, | ||
| ) -> "Optional[Dict[str, str]]": | ||
| """Calls the Regional Access Boundary lookup API to retrieve the Regional Access Boundary information. | ||
|
|
||
| Args: | ||
| request (google.auth.transport.Request): The object used to make | ||
| HTTP requests. | ||
| blocking (bool): Whether the lookup should be blocking. | ||
|
|
||
| Returns: | ||
| Optional[Dict[str, str]]: The Regional Access Boundary information returned by the lookup API, or None if the lookup failed. | ||
|
|
@@ -456,7 +490,9 @@ def _lookup_regional_access_boundary( | |
| headers: Dict[str, str] = {} | ||
| self._apply(headers) | ||
| self._rab_manager.apply_headers(headers) | ||
| return _client._lookup_regional_access_boundary(request, url, headers=headers) | ||
| return _client._lookup_regional_access_boundary( | ||
| request, url, headers=headers, blocking=blocking | ||
| ) | ||
|
|
||
| @abc.abstractmethod | ||
| def _build_regional_access_boundary_lookup_url( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does gcloud need this method?
If we have to have it, can we make it private? Since the blocking refresh was specifically added to support gcloud, I don't want users accidentally choosing a blocking lookup not knowing what it entails.