feat(auth): Add blocking Regional Access Boundary Lookup and Seed Support#16720
feat(auth): Add blocking Regional Access Boundary Lookup and Seed Support#16720macastelaz wants to merge 12 commits intogoogleapis:mainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request renames the "Trust Boundary" feature to "Regional Access Boundary" (RAB) and introduces a centralized management system for it. Key changes include the implementation of _RegionalAccessBoundaryManager for thread-safe state handling and background refreshes, and the update of multiple credential types to support this new mechanism. The with_trust_boundary method is deprecated in favor of with_regional_access_boundary. Review feedback recommends updating a type hint in the utility module to correctly reflect the expected data types and removing redundant error logging in the base credentials class to reduce log noise.
| if not url: | ||
| raise exceptions.InvalidValue("Failed to build trust boundary lookup URL.") | ||
| _LOGGER.error("Failed to build Regional Access Boundary lookup URL.") | ||
| return None |
There was a problem hiding this comment.
This error log is redundant. All implementations of _build_regional_access_boundary_lookup_url() that can fail and return None already log a more specific error message. This generic log entry adds noise to the logs.
To avoid duplicate logging, please remove this _LOGGER.error call.
| if not url: | |
| raise exceptions.InvalidValue("Failed to build trust boundary lookup URL.") | |
| _LOGGER.error("Failed to build Regional Access Boundary lookup URL.") | |
| return None | |
| if not url: | |
| return None |
References
- Remove duplicate lines of code, especially duplicate assertions in tests, to keep the codebase clean and avoid redundancy.
macastelaz
force-pushed
the
clean-rab-gcloud
branch
from
0785de5 to
23ad64b
Compare
…undary implementation
Renamed 'with_regional_access_boundary' to '_with_regional_access_boundary' to indicate internal use.
Update the comment block of "_with_regional_access_boundary" to inform future maintainers of the necessity to maintain a backwards compatible contract of this method.
macastelaz
force-pushed
the
clean-rab-gcloud
branch
from
7686ffb to
78bd7e6
Compare
macastelaz
force-pushed
the
clean-rab-gcloud
branch
from
8c0bcee to
81a2d2c
Compare
macastelaz
force-pushed
the
clean-rab-gcloud
branch
from
03803f7 to
87880a6
Compare
macastelaz
force-pushed
the
clean-rab-gcloud
branch
from
4bd1c9c to
1872dd1
Compare
macastelaz
changed the title
nbayati
left a comment
nbayati
left a comment
There was a problem hiding this comment.
Overall looks really good! Just a few small comments and questions.
| creds._rab_manager.set_initial_regional_access_boundary(seed) | ||
| return creds | ||
|
|
||
| def with_blocking_regional_access_boundary_lookup(self): |
There was a problem hiding this comment.
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.
| self._rab_manager = d.get("_rab_manager") or ( | ||
| _regional_access_boundary_utils._RegionalAccessBoundaryManager() | ||
| ) | ||
| self._use_blocking_regional_access_boundary_lookup = False |
There was a problem hiding this comment.
Do we need to be setting_use_blocking_regional_access_boundary_lookup here?
It seems redundant because the default value inside the rab manager is already false, and this class is not even looking up the RAB data.
|
|
||
| @_helpers.copy_docstring(credentials.Credentials) | ||
| def refresh(self, request): | ||
| def _build_regional_access_boundary_lookup_url(self, request=None): |
There was a problem hiding this comment.
I'm wondering if it's worth documenting why this method is just returning None and that this wasn't an oversight.
| ) | ||
|
|
||
|
|
||
| def test_lookup_regional_access_boundary_blocking(): |
There was a problem hiding this comment.
Can we add a failure test scenario that asserts the request was not retried after the failure?
|
|
||
| @mock.patch("google.oauth2._client._lookup_regional_access_boundary") | ||
| @mock.patch.object(CredentialsImpl, "_build_regional_access_boundary_lookup_url") | ||
| def test_lookup_regional_access_boundary_null_url( |
There was a problem hiding this comment.
It looks like test_lookup_regional_access_boundary_null_url was removed. Since the new OAuth2 implementation relies on returning None to skip lookups, should we restore this test to ensure it works as intended?
| """Refreshes the access token and triggers the Regional Access Boundary | ||
| lookup if necessary. | ||
| """ | ||
| super(CredentialsWithRegionalAccessBoundary, self).before_request( |
There was a problem hiding this comment.
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!
3 participants
In order for the gcloud CLI to support Regional Access Boundary, the Python auth SDK needs to support blocking lookups as well as allowing an initial seed RAB to be provided (gcloud will set this seed if the CLI has a locally cached valid RAB available).
Additional details can be found at go/rab-python-gcloud-one-pager