-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat: Skip RAB lookup in case of non-email response from MDS. #13331
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: regional-access-boundaries
Are you sure you want to change the base?
Changes from 1 commit
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 |
|---|---|---|
|
|
@@ -800,8 +800,15 @@ public String getAccount() { | |
| @InternalApi | ||
| @Override | ||
| public String getRegionalAccessBoundaryUrl() throws IOException { | ||
| String account = getAccount(); | ||
| // In GKE environments, the default service account might return a non-email placeholder. | ||
| // Since RAB lookup requires a valid email-based service account, we skip RAB lookup | ||
| // in non-email scenarios by returning null. | ||
| if (account == null || !account.contains("@")) { | ||
| return null; | ||
| } | ||
|
Comment on lines
+807
to
+809
Member
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. I think we will also need to follow the recommendation for a one-time log message that the RAB instance is skipped (maybe here, maybe in the RABManager) |
||
| return String.format( | ||
| OAuth2Utils.IAM_CREDENTIALS_ALLOWED_LOCATIONS_URL_FORMAT_SERVICE_ACCOUNT, getAccount()); | ||
| OAuth2Utils.IAM_CREDENTIALS_ALLOWED_LOCATIONS_URL_FORMAT_SERVICE_ACCOUNT, account); | ||
| } | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -196,6 +196,10 @@ void triggerAsyncRefresh( | |
| () -> { | ||
| try { | ||
| String url = provider.getRegionalAccessBoundaryUrl(); | ||
| if (url == null) { | ||
| future.set(null); | ||
| return; | ||
| } | ||
|
Comment on lines
+199
to
+202
Member
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. qq, can you remind me again why we need to do |
||
| RegionalAccessBoundary newRAB = | ||
| RegionalAccessBoundary.refresh( | ||
| transportFactory, url, accessToken, clock, maxRetryElapsedTimeMillis); | ||
|
|
||
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 know the document may have changed since this PR was raised, but I think the the standard email pattern is a regex and we will need to confirm to that.