-
Notifications
You must be signed in to change notification settings - Fork 12
feat: Regional Access Boundary Changes #891
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: trust_boundary
Are you sure you want to change the base?
feat: Regional Access Boundary Changes #891
Conversation
packages/google-auth-library-nodejs/src/auth/regionalaccessboundary.ts
Outdated
Show resolved
Hide resolved
packages/google-auth-library-nodejs/src/auth/baseexternalclient.ts
Outdated
Show resolved
Hide resolved
packages/google-auth-library-nodejs/src/auth/regionalaccessboundary.ts
Outdated
Show resolved
Hide resolved
packages/google-auth-library-nodejs/src/auth/regionalaccessboundary.ts
Outdated
Show resolved
Hide resolved
packages/google-auth-library-nodejs/src/auth/regionalaccessboundary.ts
Outdated
Show resolved
Hide resolved
packages/google-auth-library-nodejs/src/auth/regionalaccessboundary.ts
Outdated
Show resolved
Hide resolved
| const maxRetryTime = 60 * 1000; | ||
| let shouldContinue = true; | ||
|
|
||
| while (shouldContinue) { |
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.
This library already uses gaxios, which provides a powerful and configurable built-in retry mechanism (including exponential backoff) via its retryConfig. This library already uses gaxios, which provides a powerful and configurable built-in retry mechanism (including exponential backoff) via its retryConfig.
The fetchRegionalAccessBoundary method already enables gaxios retries, and then we're wrapping it here in the custom retry logic. I'd recommend refactoring to remove this while loop. Instead, you can add retry conditions to the retryConfig in fetchRegionalAccessBoundary method to achieve the desired behavior. This would leverage existing gaxios functionality, simplify the code, and align with the library's established patterns for request retries.
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.
Implemented! Thank you for the suggestion!
packages/google-auth-library-nodejs/src/auth/baseexternalclient.ts
Outdated
Show resolved
Hide resolved
| !reAuthRetried | ||
| ) { | ||
| this.clearRegionalAccessBoundaryCache(); | ||
| return await this.requestAsync<T>(opts, true); |
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.
I think we should have two separate flags for the auth retry, and this one (retryWithoutRAB maybe?). Consider this scenario which the single flag approach would fail to handle:
- Initial Call: requestAsync(opts) is called.
- First Failure: The request fails with a stale RAB error. The catch block clears the RAB cache and retries
the request, with reAuthRetried set to True. - Second Failure: The retried request (now sent without the x-allowed-locations header) fails, but this time with a 401 Unauthorized error because the access token has just expired.
With a single reAuthRetried flag, this second failure would not be handled because it thinks the token has already been refreshed and the request has been retried with the updated credential, and an error would be thrown.
If we use two flags, we can ensure that we only try once without the allowed location header, and then we attempt a true reAuth.
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.
Great point, I was of the opinion that we shouldn't hold the user back with too many retries. Thanks for the clarification, I have made the changes.
packages/google-auth-library-nodejs/src/auth/externalAccountAuthorizedUserClient.ts
Show resolved
Hide resolved
packages/google-auth-library-nodejs/src/auth/externalAccountAuthorizedUserClient.ts
Show resolved
Hide resolved
| * @throws {Error} If the URL cannot be constructed for a compatible client. | ||
| */ | ||
| protected async getTrustBoundaryUrl(): Promise<string | null> { | ||
| public async getRegionalAccessBoundaryUrl(): Promise<string | null> { |
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.
Why this method became public?
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.
I made it so for ease of testing the getRegionalAccessBoundaryUrl logic. For ex: in the base external client we have scenarios where we want to test if the right url is returned or error is thrown in case of workload, workforce or a null audience.
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.
I'd go ahead and put an @private on it, as that will cause it to be omitted from docs, but it will still show up in the transpiled class. Anyone digging in the source can see the @private to know they shouldn't use it.
Actually, it looks like @internal omits it from any exported .d.ts, so that may be enough. I'd double-check the output though, to see what it makes.
packages/google-auth-library-nodejs/src/auth/regionalaccessboundary.ts
Outdated
Show resolved
Hide resolved
238aed8 to
f40a3e1
Compare
…e requestAsync level. RAB urls now have googleapis as universe domain.
e3e00da to
3427c0e
Compare
…been excluded and test suite updated.
…504 are the only retryable look up failures.
feywind
left a comment
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.
I don't see anything too weird in this, I guess let me know when you've got one that merges against main?
| if (res && res.status === 400) { | ||
| const data = res.data as {error?: {message?: string}; message?: string}; | ||
| const message = | ||
| data?.error?.message || data?.message || error.message || ''; |
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.
?? might be more idiomatic now, unless e.g. data?.message could be empty but not null/undefined.
| * @throws {Error} If the URL cannot be constructed for a compatible client. | ||
| */ | ||
| protected async getTrustBoundaryUrl(): Promise<string | null> { | ||
| public async getRegionalAccessBoundaryUrl(): Promise<string | null> { |
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.
I'd go ahead and put an @private on it, as that will cause it to be omitted from docs, but it will still show up in the transpiled class. Anyone digging in the source can see the @private to know they shouldn't use it.
Actually, it looks like @internal omits it from any exported .d.ts, so that may be enough. I'd double-check the output though, to see what it makes.
| } | ||
|
|
||
| protected async getTrustBoundaryUrl(): Promise<string> { | ||
| public async getRegionalAccessBoundaryUrl(): Promise<string> { |
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.
Same comment re protected -> public.
| } | ||
|
|
||
| protected async getTrustBoundaryUrl(): Promise<string> { | ||
| public async getRegionalAccessBoundaryUrl(): Promise<string> { |
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.
Ditto protected -> public.
| * Triggers an asynchronous regional access boundary refresh if needed. | ||
| * @param accessToken The access token to use for the lookup. | ||
| */ | ||
| private maybeTriggerRegionalAccessBoundaryRefresh(accessToken: string) { |
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.
Absolutely nitpicky, but I like to try to put an explicit return type on void methods just to make sure that you notice if something changes internal to it.
| accessToken: MOCK_ACCESS_TOKEN, | ||
| expireTime: new Date(now + ONE_HOUR_IN_SECS * 1000).toISOString(), | ||
| accessToken: 'SA_ACCESS_TOKEN', | ||
| expireTime: new Date(Date.now() + 3600000).toISOString(), |
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.
You could still do the multiplication thing here... like 60 * 60 * 1000.
Contains changes for the feature Regional Access Boundary (Previously Called Trust Boundaries).
The following are salient changes: