feat: Regional Access Boundary Update #1880
Conversation
…pe. Updated tests.
…rors are now retryable.
lqiu96
changed the base branch from
feat-tb-sa
to
feat/agentic-identities-cloudrun
oauth2_http/java/com/google/auth/http/HttpCredentialsAdapter.java
Outdated
Show resolved
Hide resolved
| try { | ||
| // Check for the stale regional access boundary error message in the response body. | ||
| // Note: This consumes the response stream. | ||
| String content = response.parseAsString(); |
There was a problem hiding this comment.
I think this should be fine since this is part of the flow to initialize this request. But if it consumes the content and closes the stream, I wonder if this may break anything that tries to consume it later.
There was a problem hiding this comment.
There is no easy way to create a copy of an HTTPResponse, in order to peek into its contents we have to consume the response stream.
However, before consuming the error:
- The HTTP code is a 406 which is relatively uncommon
- The request actually carried RAB headers
This should ensure that for the majority of scenarios this downstream error won't be a problem.
There was a problem hiding this comment.
I set up a sync with you tomorrow to discuss this. I have a bit of concerns for this, but not for the reasons above. I'll create a doc with questions and share with you to review prior to the meeting
| // this thread "won the race" and is responsible for starting the background task. | ||
| // All other concurrent threads will return false and exit immediately. | ||
| if (refreshFuture.compareAndSet(null, future)) { | ||
| CompletableFuture.runAsync( |
There was a problem hiding this comment.
Can you use Guava's ListeningFuture to keep with the rest of the auth-library?
Since this is called from both getReqestMetadata(...) methods (one of which takes in an executor), I think we'll also need to respect that.
I think this method should take in the executor as a param
There was a problem hiding this comment.
The way I understand: the executor in getRequestMetadataAsync is used to provide a pool of threads to execute the token refresh in the background.
My initial assumption with doing this via CompletableFuture was that the user might want the background thread to exclusively focus on token refresh and not the RAB refresh.
If we are okay with the user's background thread waiting longer for the RAB to refresh then I am okay with making this change.
There was a problem hiding this comment.
I see, that's a good point. The async executor is intended to not block the calling thread while getting the headers (before the only headers was just the access token).
I believe the ideal state would as you're mentioning:
Sync: Main thread blocks for the token call, then rab would be a different async executor thread
async: async executor thread would run the token call, then rab would any async executor thread (even the same one)
Looks like CompletableFuture uses ForkJoinPool.commonPool() which wouldn't be ideal for IO bound tasks. Also looks like it's a static shared executor for the entire JVM which may impact other processes.
I think I'm leaning with having the background task wait for the RAB call.
oauth2_http/java/com/google/auth/oauth2/RegionalAccessBoundary.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| /** | ||
| * Refreshes the Regional Access Boundary if it is expired or not yet fetched. |
There was a problem hiding this comment.
nit: the method name and docs isn't fully accurate as it does more than just checking if the boundary has expired.
If possible, can this be broken into two methods:
- shouldRefresh...()
- refresh...()
I think it might be easier to test as well. Let me know what you think
oauth2_http/java/com/google/auth/oauth2/RegionalAccessBoundaryManager.java
Outdated
Show resolved
Hide resolved
vverman
changed the base branch from
feat/agentic-identities-cloudrun
to
feat-tb-sa
|
|
||
| /** | ||
| * Returns the trust boundary URI. | ||
| * Returns the regional access boundary URI. |
There was a problem hiding this comment.
Can you add that this URL is constructed only for GDU
2 participants
Contains changes for the feature Regional Access Boundary (Previously Called Trust Boundaries).
The following are salient changes:
Calls to refresh RAB are now all async and in a separate thread.
Logic for refreshing RAB now exists in its own class for cleaner maintenance.
Self-signed jwts are within scope.
Changes to how we trigger RAB refresh and deal with refresh errors.