-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix(auth): 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 all commits
821c70a
e610ef4
d5658ba
420a0fd
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 |
|---|---|---|
|
|
@@ -72,6 +72,7 @@ | |
| import java.util.Objects; | ||
| import java.util.logging.Level; | ||
| import java.util.logging.Logger; | ||
| import java.util.regex.Pattern; | ||
|
|
||
| /** | ||
| * OAuth2 credentials representing the built-in service account for a Google Compute Engine VM. | ||
|
|
@@ -117,6 +118,7 @@ public class ComputeEngineCredentials extends GoogleCredentials | |
|
|
||
| private static final String PARSE_ERROR_PREFIX = "Error parsing token refresh response. "; | ||
| private static final String PARSE_ERROR_ACCOUNT = "Error parsing service account response. "; | ||
| private static final Pattern EMAIL_PATTERN = Pattern.compile("^[^@]+@[^@]+\\.[^@]+$"); | ||
| private static final long serialVersionUID = -4113476462526554235L; | ||
|
|
||
| private final String transportFactoryClassName; | ||
|
|
@@ -800,8 +802,20 @@ 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 || !EMAIL_PATTERN.matcher(account).matches()) { | ||
| LoggingUtils.log( | ||
| LOGGER_PROVIDER, | ||
| Level.INFO, | ||
| Collections.emptyMap(), | ||
| "Regional Access Boundary lookup is skipped for this instance because it is a non-email instance."); | ||
|
Comment on lines
+810
to
+814
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. two small nits:
Maybe something like (ff to change this to something clearer): |
||
| return null; | ||
| } | ||
| 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 |
|---|---|---|
|
|
@@ -37,11 +37,11 @@ | |
| import com.google.api.core.InternalApi; | ||
| import com.google.auth.http.HttpTransportFactory; | ||
| import com.google.common.annotations.VisibleForTesting; | ||
| import com.google.common.util.concurrent.SettableFuture; | ||
| import java.util.concurrent.ExecutorService; | ||
| import java.util.concurrent.LinkedBlockingQueue; | ||
| import java.util.concurrent.ThreadPoolExecutor; | ||
| import java.util.concurrent.TimeUnit; | ||
| import java.util.concurrent.atomic.AtomicBoolean; | ||
| import java.util.concurrent.atomic.AtomicInteger; | ||
| import java.util.concurrent.atomic.AtomicReference; | ||
| import java.util.logging.Level; | ||
|
|
@@ -75,16 +75,16 @@ final class RegionalAccessBoundaryManager { | |
| private final AtomicReference<RegionalAccessBoundary> cachedRAB = new AtomicReference<>(); | ||
|
|
||
| /** | ||
| * refreshFuture acts as an atomic gate for request de-duplication. If a future is present, it | ||
| * indicates a background refresh is already in progress. It also provides a handle for | ||
| * observability and unit testing to track the background task's lifecycle. | ||
| * isRefreshing acts as an atomic gate for request de-duplication. If true, it indicates a | ||
| * background refresh is already in progress. | ||
| */ | ||
| private final AtomicReference<SettableFuture<RegionalAccessBoundary>> refreshFuture = | ||
| new AtomicReference<>(); | ||
| private final AtomicBoolean isRefreshing = new AtomicBoolean(false); | ||
|
|
||
| private final AtomicReference<CooldownState> cooldownState = | ||
| new AtomicReference<>(new CooldownState(0, INITIAL_COOLDOWN_MILLIS)); | ||
|
|
||
| private final AtomicBoolean skipRAB = new AtomicBoolean(false); | ||
|
|
||
| // Unbounded thread creation is discouraged in library code to avoid resource | ||
| // exhaustion. A shared, bounded executor service ensures a hard limit (5) | ||
| // on concurrent refresh tasks, while threadCount provides unique names | ||
|
|
@@ -178,7 +178,7 @@ void triggerAsyncRefresh( | |
| final HttpTransportFactory transportFactory, | ||
| final RegionalAccessBoundaryProvider provider, | ||
| final AccessToken accessToken) { | ||
| if (isCooldownActive()) { | ||
| if (skipRAB.get() || isCooldownActive()) { | ||
| return; | ||
| } | ||
|
|
||
|
|
@@ -187,28 +187,28 @@ void triggerAsyncRefresh( | |
| return; | ||
| } | ||
|
|
||
| SettableFuture<RegionalAccessBoundary> future = SettableFuture.create(); | ||
| // Atomically check if a refresh is already running. If compareAndSet returns true, | ||
| // 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)) { | ||
| if (isRefreshing.compareAndSet(false, true)) { | ||
| Runnable refreshTask = | ||
| () -> { | ||
| try { | ||
| String url = provider.getRegionalAccessBoundaryUrl(); | ||
| if (url == null) { | ||
| skipRAB.set(true); | ||
| return; | ||
| } | ||
|
Comment on lines
+198
to
+201
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
Contributor
Author
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 was using a SettableFuture as a gate to deduplicate RAB refreshes which let's us set the future to indicate to dependent threads as to the result of the refresh. However, I realized the same can be done with AtomicBoolean which is more easier to maintain. Hence updated the logic. |
||
| RegionalAccessBoundary newRAB = | ||
| RegionalAccessBoundary.refresh( | ||
| transportFactory, url, accessToken, clock, maxRetryElapsedTimeMillis); | ||
| cachedRAB.set(newRAB); | ||
| resetCooldown(); | ||
| // Complete the future so monitors (like unit tests) know we are done. | ||
| future.set(newRAB); | ||
| } catch (Exception e) { | ||
| handleRefreshFailure(e); | ||
| future.setException(e); | ||
| } finally { | ||
| // Open the gate again for future refresh requests. | ||
| refreshFuture.set(null); | ||
| isRefreshing.set(false); | ||
| } | ||
| }; | ||
|
|
||
|
|
@@ -224,8 +224,7 @@ void triggerAsyncRefresh( | |
| "Could not submit background refresh task for Regional Access Boundary. " | ||
| + "This is non-blocking and the library will attempt to refresh on the next access. Error: " | ||
| + e.getMessage()); | ||
| future.setException(e); | ||
| refreshFuture.set(null); | ||
| isRefreshing.set(false); | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -279,6 +278,11 @@ long getCurrentCooldownMillis() { | |
| return cooldownState.get().durationMillis; | ||
| } | ||
|
|
||
| @VisibleForTesting | ||
| boolean isSkipRAB() { | ||
| return skipRAB.get(); | ||
| } | ||
|
|
||
| private static class CooldownState { | ||
| /** The time (in milliseconds from epoch) when the current cooldown period expires. */ | ||
| final long expiryTime; | ||
|
|
||
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.
nit: Since this is a temp change, can you link to the internal ticket tracking this (b/XXXX)