-
Notifications
You must be signed in to change notification settings - Fork 1.1k
chore(auth): Address remaining Regional Access Boundary feedback #12867
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
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 |
|---|---|---|
|
|
@@ -40,10 +40,11 @@ | |
| import com.google.api.client.http.HttpResponse; | ||
| import com.google.api.client.http.HttpUnsuccessfulResponseHandler; | ||
| import com.google.api.client.json.GenericJson; | ||
| import com.google.api.client.json.JsonParser; | ||
| import com.google.api.client.json.JsonObjectParser; | ||
| import com.google.api.client.util.Clock; | ||
| import com.google.api.client.util.ExponentialBackOff; | ||
| import com.google.api.client.util.Key; | ||
| import com.google.api.core.InternalApi; | ||
| import com.google.auth.http.HttpTransportFactory; | ||
| import com.google.common.annotations.VisibleForTesting; | ||
| import com.google.common.base.MoreObjects; | ||
|
|
@@ -53,7 +54,6 @@ | |
| import java.io.Serializable; | ||
| import java.util.Collections; | ||
| import java.util.List; | ||
| import javax.annotation.Nullable; | ||
|
|
||
| /** | ||
| * Represents the regional access boundary configuration for a credential. This class holds the | ||
|
|
@@ -67,10 +67,24 @@ final class RegionalAccessBoundary implements Serializable { | |
| static final String X_ALLOWED_LOCATIONS_HEADER_KEY = "x-allowed-locations"; | ||
| private static final long serialVersionUID = -2428522338274020302L; | ||
|
|
||
| // Note: this is for internal testing use use only. | ||
| // TODO: Fix unit test mocks so this can be removed | ||
| // Refer -> https://github.com/googleapis/google-auth-library-java/issues/1898 | ||
| static final String ENABLE_EXPERIMENT_ENV_VAR = "GOOGLE_AUTH_TRUST_BOUNDARY_ENABLE_EXPERIMENT"; | ||
| private static final ThreadLocal<Boolean> DISABLE_RAB_FOR_TESTS = | ||
| ThreadLocal.withInitial(() -> false); | ||
|
|
||
| @VisibleForTesting | ||
| static void disableForTests() { | ||
| DISABLE_RAB_FOR_TESTS.set(true); | ||
| } | ||
|
|
||
| @VisibleForTesting | ||
| static void enableForTests() { | ||
| DISABLE_RAB_FOR_TESTS.set(false); | ||
| } | ||
|
|
||
| @VisibleForTesting | ||
| static void resetForTests() { | ||
| DISABLE_RAB_FOR_TESTS.remove(); | ||
| } | ||
|
|
||
| static final long TTL_MILLIS = 6 * 60 * 60 * 1000L; // 6 hours | ||
| static final long REFRESH_THRESHOLD_MILLIS = 1 * 60 * 60 * 1000L; // 1 hour | ||
|
|
||
|
|
@@ -79,8 +93,6 @@ final class RegionalAccessBoundary implements Serializable { | |
| private final long refreshTime; | ||
| private transient Clock clock; | ||
|
|
||
| private static EnvironmentProvider environmentProvider = SystemEnvironmentProvider.getInstance(); | ||
|
|
||
| /** | ||
| * Creates a new RegionalAccessBoundary instance. | ||
| * | ||
|
|
@@ -172,28 +184,16 @@ public String toString() { | |
| } | ||
| } | ||
|
|
||
| @VisibleForTesting | ||
| static void setEnvironmentProviderForTest(@Nullable EnvironmentProvider provider) { | ||
|
Contributor
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. This is called in LoggingTest.java which also needs to be updated. |
||
| environmentProvider = provider == null ? SystemEnvironmentProvider.getInstance() : provider; | ||
| } | ||
|
|
||
| /** | ||
| * Checks if the regional access boundary feature is enabled. The feature is enabled if the | ||
| * environment variable or system property "GOOGLE_AUTH_TRUST_BOUNDARY_ENABLE_EXPERIMENT" is set | ||
| * to "true" or "1" (case-insensitive). | ||
| * Checks if the regional access boundary feature is enabled. | ||
| * | ||
| * <p>This method is for internal use only and may be changed or removed in future releases. | ||
| * | ||
| * @return True if the regional access boundary feature is enabled, false otherwise. | ||
| */ | ||
| @InternalApi | ||
| static boolean isEnabled() { | ||
| String enabled = environmentProvider.getEnv(ENABLE_EXPERIMENT_ENV_VAR); | ||
| if (enabled == null) { | ||
| enabled = System.getProperty(ENABLE_EXPERIMENT_ENV_VAR); | ||
| } | ||
| if (enabled == null) { | ||
| return false; | ||
| } | ||
| String lowercased = enabled.toLowerCase(); | ||
| return "true".equals(lowercased) || "1".equals(enabled); | ||
| return !DISABLE_RAB_FOR_TESTS.get(); | ||
|
Contributor
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. Why was this behavior changed to remove the gate? Are you planning on holding this PR until we're ready to launch?
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. This PR is to a feature branch so it won't be released till we're ready to launch. |
||
| } | ||
|
|
||
| /** | ||
|
|
@@ -249,15 +249,20 @@ static RegionalAccessBoundary refresh( | |
| HttpIOExceptionHandler ioExceptionHandler = new HttpBackOffIOExceptionHandler(backoff); | ||
| request.setIOExceptionHandler(ioExceptionHandler); | ||
|
|
||
| request.setParser(new JsonObjectParser(OAuth2Utils.JSON_FACTORY)); | ||
|
|
||
| RegionalAccessBoundaryResponse json; | ||
| HttpResponse response = null; | ||
| try { | ||
| HttpResponse response = request.execute(); | ||
| String responseString = response.parseAsString(); | ||
| JsonParser parser = OAuth2Utils.JSON_FACTORY.createJsonParser(responseString); | ||
| json = parser.parseAndClose(RegionalAccessBoundaryResponse.class); | ||
| response = request.execute(); | ||
| json = response.parseAs(RegionalAccessBoundaryResponse.class); | ||
| } catch (IOException e) { | ||
| throw new IOException( | ||
| "RegionalAccessBoundary: Failure while getting regional access boundaries:", e); | ||
| } finally { | ||
| if (response != null) { | ||
| response.disconnect(); | ||
| } | ||
| } | ||
| String encodedLocations = json.getEncodedLocations(); | ||
| // The encodedLocations is the value attached to the x-allowed-locations header, and | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,6 +36,11 @@ | |
| 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.AtomicInteger; | ||
| import java.util.concurrent.atomic.AtomicReference; | ||
| import java.util.logging.Level; | ||
| import javax.annotation.Nullable; | ||
|
|
@@ -78,6 +83,32 @@ final class RegionalAccessBoundaryManager { | |
| private final AtomicReference<CooldownState> cooldownState = | ||
| new AtomicReference<>(new CooldownState(0, INITIAL_COOLDOWN_MILLIS)); | ||
|
|
||
| // 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 | ||
| // for easier debugging. | ||
| private static final AtomicInteger threadCount = new AtomicInteger(0); | ||
| private static final ExecutorService EXECUTOR; | ||
|
|
||
| static { | ||
| ThreadPoolExecutor executor = | ||
| new ThreadPoolExecutor( | ||
| 5, // corePoolSize: threads to keep alive | ||
| 5, // maximumPoolSize: max threads allowed | ||
| 1, // keepAliveTime: time to wait before terminating idle threads | ||
| TimeUnit.HOURS, // unit for keepAliveTime | ||
| new LinkedBlockingQueue<>(), // work queue | ||
|
Contributor
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.
Can we use a bounded queue here? |
||
| r -> { | ||
| Thread t = new Thread(r, "RAB-refresh-" + threadCount.getAndIncrement()); | ||
| t.setDaemon(true); | ||
| return t; | ||
| }); | ||
| // Allow core threads to time out so the executor can shrink to 0 when idle. | ||
| // Ensures threads are released when idle to avoid unnecessary resource usage. | ||
| executor.allowCoreThreadTimeOut(true); | ||
|
vverman marked this conversation as resolved.
|
||
| EXECUTOR = executor; | ||
| } | ||
|
|
||
| private final transient Clock clock; | ||
| private final int maxRetryElapsedTimeMillis; | ||
|
|
||
|
|
@@ -161,14 +192,7 @@ void triggerAsyncRefresh( | |
| }; | ||
|
|
||
| try { | ||
| // We use new Thread() here instead of | ||
| // CompletableFuture.runAsync() (which uses ForkJoinPool.commonPool()). | ||
| // This avoids consuming CPU resources since | ||
| // The common pool has a small, fixed number of threads designed for | ||
| // CPU-bound tasks. | ||
| Thread refreshThread = new Thread(refreshTask, "RAB-refresh-thread"); | ||
| refreshThread.setDaemon(true); | ||
| refreshThread.start(); | ||
| EXECUTOR.submit(refreshTask); | ||
| } catch (Exception | Error e) { | ||
| // If scheduling fails (e.g., RejectedExecutionException, OutOfMemoryError for threads), | ||
| // the task's finally block will never execute. We must release the lock here. | ||
|
|
||
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.
can you explain this part a bit more? I'm not sure I'm following why this is needed
Uh oh!
There was an error while loading. Please reload this page.
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.
With this PR, the env variable gate on RAB refresh is removed. Which means on each request call, the RAB refresh will be triggered.
This causes all the tests which test the request headers flow in the auth library to trigger RAB refresh.
I did spend some time trying to fix this but it is complicated for the following reasons:
Tests that verify the header flow (such as getDefaultCredentials_compute_providesToken or tests calling the testUserProvidesToken helper) explicitly call getRequestMetadata to check the bearer token. Because getRequestMetadata now triggers the async RAB refresh on every call, these tests will spawn background requests that interfere with assertions on mock transports.
Without this isolation, a background thread started in one test could leak into subsequent tests and interfere with their mocks, leading to flaky tests in CI
Hence we have an injectable disableRABForTests which ensures the tests for the non-RAB flow don't trigger the RAB refresh.
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.
But nothing calls
disableForTests(),enableForTests(), orresetForTests()?Also, since this is only disables RAB on the thread that calls it. That may not cover async
getRequestMetadata(...)tests where the callback runs on an executor thread.