Skip to content

Commit 5c279c6

Browse files
authored
chore(auth): Address remaining Regional Access Boundary feedback (#12867)
1. The RAB refresh uses a direct executor with a fixed thread pool as opposed to instantiating a new thread each time. 2. The RAB env gate -> GOOGLE_AUTH_TRUST_BOUNDARY_ENABLE_EXPERIMENT has been removed. This means RAB refresh triggers by default. 3. Added other fixes/suggestions made in the previous Java [PR](googleapis/google-auth-library-java#1880).
1 parent 30f2c14 commit 5c279c6

16 files changed

Lines changed: 425 additions & 179 deletions

google-auth-library-java/oauth2_http/java/com/google/auth/oauth2/GoogleCredentials.java

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -373,9 +373,7 @@ final RegionalAccessBoundary getRegionalAccessBoundary() {
373373
*/
374374
void refreshRegionalAccessBoundaryIfExpired(@Nullable URI uri, @Nullable AccessToken token)
375375
throws IOException {
376-
if (!(this instanceof RegionalAccessBoundaryProvider)
377-
|| !RegionalAccessBoundary.isEnabled()
378-
|| !isDefaultUniverseDomain()) {
376+
if (!(this instanceof RegionalAccessBoundaryProvider) || !isDefaultUniverseDomain()) {
379377
return;
380378
}
381379

google-auth-library-java/oauth2_http/java/com/google/auth/oauth2/RegionalAccessBoundary.java

Lines changed: 12 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -40,20 +40,18 @@
4040
import com.google.api.client.http.HttpResponse;
4141
import com.google.api.client.http.HttpUnsuccessfulResponseHandler;
4242
import com.google.api.client.json.GenericJson;
43-
import com.google.api.client.json.JsonParser;
43+
import com.google.api.client.json.JsonObjectParser;
4444
import com.google.api.client.util.Clock;
4545
import com.google.api.client.util.ExponentialBackOff;
4646
import com.google.api.client.util.Key;
4747
import com.google.auth.http.HttpTransportFactory;
48-
import com.google.common.annotations.VisibleForTesting;
4948
import com.google.common.base.MoreObjects;
5049
import com.google.common.base.Preconditions;
5150
import java.io.IOException;
5251
import java.io.ObjectInputStream;
5352
import java.io.Serializable;
5453
import java.util.Collections;
5554
import java.util.List;
56-
import javax.annotation.Nullable;
5755

5856
/**
5957
* Represents the regional access boundary configuration for a credential. This class holds the
@@ -67,10 +65,6 @@ final class RegionalAccessBoundary implements Serializable {
6765
static final String X_ALLOWED_LOCATIONS_HEADER_KEY = "x-allowed-locations";
6866
private static final long serialVersionUID = -2428522338274020302L;
6967

70-
// Note: this is for internal testing use use only.
71-
// TODO: Fix unit test mocks so this can be removed
72-
// Refer -> https://github.com/googleapis/google-auth-library-java/issues/1898
73-
static final String ENABLE_EXPERIMENT_ENV_VAR = "GOOGLE_AUTH_TRUST_BOUNDARY_ENABLE_EXPERIMENT";
7468
static final long TTL_MILLIS = 6 * 60 * 60 * 1000L; // 6 hours
7569
static final long REFRESH_THRESHOLD_MILLIS = 1 * 60 * 60 * 1000L; // 1 hour
7670

@@ -79,8 +73,6 @@ final class RegionalAccessBoundary implements Serializable {
7973
private final long refreshTime;
8074
private transient Clock clock;
8175

82-
private static EnvironmentProvider environmentProvider = SystemEnvironmentProvider.getInstance();
83-
8476
/**
8577
* Creates a new RegionalAccessBoundary instance.
8678
*
@@ -172,30 +164,6 @@ public String toString() {
172164
}
173165
}
174166

175-
@VisibleForTesting
176-
static void setEnvironmentProviderForTest(@Nullable EnvironmentProvider provider) {
177-
environmentProvider = provider == null ? SystemEnvironmentProvider.getInstance() : provider;
178-
}
179-
180-
/**
181-
* Checks if the regional access boundary feature is enabled. The feature is enabled if the
182-
* environment variable or system property "GOOGLE_AUTH_TRUST_BOUNDARY_ENABLE_EXPERIMENT" is set
183-
* to "true" or "1" (case-insensitive).
184-
*
185-
* @return True if the regional access boundary feature is enabled, false otherwise.
186-
*/
187-
static boolean isEnabled() {
188-
String enabled = environmentProvider.getEnv(ENABLE_EXPERIMENT_ENV_VAR);
189-
if (enabled == null) {
190-
enabled = System.getProperty(ENABLE_EXPERIMENT_ENV_VAR);
191-
}
192-
if (enabled == null) {
193-
return false;
194-
}
195-
String lowercased = enabled.toLowerCase();
196-
return "true".equals(lowercased) || "1".equals(enabled);
197-
}
198-
199167
/**
200168
* Refreshes the regional access boundary by making a network call to the lookup endpoint.
201169
*
@@ -223,6 +191,8 @@ static RegionalAccessBoundary refresh(
223191

224192
HttpRequestFactory requestFactory = transportFactory.create().createRequestFactory();
225193
HttpRequest request = requestFactory.buildGetRequest(new GenericUrl(url));
194+
// Disable automatic logging by google-http-java-client to prevent leakage of sensitive tokens.
195+
request.setLoggingEnabled(false);
226196
request.getHeaders().setAuthorization("Bearer " + accessToken.getTokenValue());
227197

228198
// Add retry logic
@@ -249,15 +219,20 @@ static RegionalAccessBoundary refresh(
249219
HttpIOExceptionHandler ioExceptionHandler = new HttpBackOffIOExceptionHandler(backoff);
250220
request.setIOExceptionHandler(ioExceptionHandler);
251221

222+
request.setParser(new JsonObjectParser(OAuth2Utils.JSON_FACTORY));
223+
252224
RegionalAccessBoundaryResponse json;
225+
HttpResponse response = null;
253226
try {
254-
HttpResponse response = request.execute();
255-
String responseString = response.parseAsString();
256-
JsonParser parser = OAuth2Utils.JSON_FACTORY.createJsonParser(responseString);
257-
json = parser.parseAndClose(RegionalAccessBoundaryResponse.class);
227+
response = request.execute();
228+
json = response.parseAs(RegionalAccessBoundaryResponse.class);
258229
} catch (IOException e) {
259230
throw new IOException(
260231
"RegionalAccessBoundary: Failure while getting regional access boundaries:", e);
232+
} finally {
233+
if (response != null) {
234+
response.disconnect();
235+
}
261236
}
262237
String encodedLocations = json.getEncodedLocations();
263238
// The encodedLocations is the value attached to the x-allowed-locations header, and

google-auth-library-java/oauth2_http/java/com/google/auth/oauth2/RegionalAccessBoundaryManager.java

Lines changed: 65 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -31,11 +31,18 @@
3131

3232
package com.google.auth.oauth2;
3333

34+
import static com.google.auth.oauth2.LoggingUtils.log;
35+
3436
import com.google.api.client.util.Clock;
3537
import com.google.api.core.InternalApi;
3638
import com.google.auth.http.HttpTransportFactory;
3739
import com.google.common.annotations.VisibleForTesting;
3840
import com.google.common.util.concurrent.SettableFuture;
41+
import java.util.concurrent.ExecutorService;
42+
import java.util.concurrent.LinkedBlockingQueue;
43+
import java.util.concurrent.ThreadPoolExecutor;
44+
import java.util.concurrent.TimeUnit;
45+
import java.util.concurrent.atomic.AtomicInteger;
3946
import java.util.concurrent.atomic.AtomicReference;
4047
import java.util.logging.Level;
4148
import javax.annotation.Nullable;
@@ -59,7 +66,7 @@ final class RegionalAccessBoundaryManager {
5966
* The default maximum elapsed time in milliseconds for retrying Regional Access Boundary lookup
6067
* requests.
6168
*/
62-
private static final int DEFAULT_MAX_RETRY_ELAPSED_TIME_MILLIS = 60000;
69+
static final int DEFAULT_MAX_RETRY_ELAPSED_TIME_MILLIS = 60000;
6370

6471
/**
6572
* cachedRAB uses AtomicReference to provide thread-safe, lock-free access to the cached data for
@@ -78,22 +85,62 @@ final class RegionalAccessBoundaryManager {
7885
private final AtomicReference<CooldownState> cooldownState =
7986
new AtomicReference<>(new CooldownState(0, INITIAL_COOLDOWN_MILLIS));
8087

88+
// Unbounded thread creation is discouraged in library code to avoid resource
89+
// exhaustion. A shared, bounded executor service ensures a hard limit (5)
90+
// on concurrent refresh tasks, while threadCount provides unique names
91+
// for easier debugging.
92+
private static final AtomicInteger threadCount = new AtomicInteger(0);
93+
94+
// Bounded executor service ensures hard limits on concurrent refresh tasks and queued tasks
95+
// to avoid resource exhaustion.
96+
private static final int EXECUTOR_POOL_SIZE = 5;
97+
private static final int EXECUTOR_QUEUE_CAPACITY = 100;
98+
99+
private static final ExecutorService DEFAULT_SHARED_EXECUTOR;
100+
101+
static {
102+
ThreadPoolExecutor executor =
103+
new ThreadPoolExecutor(
104+
EXECUTOR_POOL_SIZE, // corePoolSize: threads to keep alive
105+
EXECUTOR_POOL_SIZE, // maximumPoolSize: max threads allowed
106+
1, // keepAliveTime: time to wait before terminating idle threads
107+
TimeUnit.HOURS, // unit for keepAliveTime
108+
new LinkedBlockingQueue<>(EXECUTOR_QUEUE_CAPACITY), // work queue with bound
109+
r -> {
110+
Thread t = new Thread(r, "RAB-refresh-" + threadCount.getAndIncrement());
111+
t.setDaemon(true);
112+
return t;
113+
});
114+
// Allow core threads to time out so the executor can shrink to 0 when idle.
115+
// Ensures threads are released when idle to avoid unnecessary resource usage.
116+
executor.allowCoreThreadTimeOut(true);
117+
DEFAULT_SHARED_EXECUTOR = executor;
118+
}
119+
81120
private final transient Clock clock;
82121
private final int maxRetryElapsedTimeMillis;
122+
private final ExecutorService executor;
83123

84124
/**
85125
* Creates a new RegionalAccessBoundaryManager with the default retry timeout of 60 seconds.
86126
*
87127
* @param clock The clock to use for cooldown and expiration checks.
88128
*/
89129
RegionalAccessBoundaryManager(Clock clock) {
90-
this(clock, DEFAULT_MAX_RETRY_ELAPSED_TIME_MILLIS);
130+
this(clock, DEFAULT_MAX_RETRY_ELAPSED_TIME_MILLIS, DEFAULT_SHARED_EXECUTOR);
91131
}
92132

93133
@VisibleForTesting
94134
RegionalAccessBoundaryManager(Clock clock, int maxRetryElapsedTimeMillis) {
135+
this(clock, maxRetryElapsedTimeMillis, DEFAULT_SHARED_EXECUTOR);
136+
}
137+
138+
@VisibleForTesting
139+
RegionalAccessBoundaryManager(
140+
Clock clock, int maxRetryElapsedTimeMillis, ExecutorService executor) {
95141
this.clock = clock != null ? clock : Clock.SYSTEM;
96142
this.maxRetryElapsedTimeMillis = maxRetryElapsedTimeMillis;
143+
this.executor = executor;
97144
}
98145

99146
/**
@@ -111,6 +158,11 @@ RegionalAccessBoundary getCachedRAB() {
111158
return null;
112159
}
113160

161+
@VisibleForTesting
162+
void setCachedRAB(RegionalAccessBoundary rab) {
163+
this.cachedRAB.set(rab);
164+
}
165+
114166
/**
115167
* Triggers an asynchronous refresh of the RegionalAccessBoundary if it is not already being
116168
* refreshed and if the cooldown period is not active.
@@ -161,19 +213,17 @@ void triggerAsyncRefresh(
161213
};
162214

163215
try {
164-
// We use new Thread() here instead of
165-
// CompletableFuture.runAsync() (which uses ForkJoinPool.commonPool()).
166-
// This avoids consuming CPU resources since
167-
// The common pool has a small, fixed number of threads designed for
168-
// CPU-bound tasks.
169-
Thread refreshThread = new Thread(refreshTask, "RAB-refresh-thread");
170-
refreshThread.setDaemon(true);
171-
refreshThread.start();
216+
this.executor.submit(refreshTask);
172217
} catch (Exception | Error e) {
173218
// If scheduling fails (e.g., RejectedExecutionException, OutOfMemoryError for threads),
174219
// the task's finally block will never execute. We must release the lock here.
175-
handleRefreshFailure(
176-
new Exception("Regional Access Boundary background refresh failed to schedule", e));
220+
log(
221+
LOGGER_PROVIDER,
222+
Level.FINE,
223+
null,
224+
"Could not submit background refresh task for Regional Access Boundary. "
225+
+ "This is non-blocking and the library will attempt to refresh on the next access. Error: "
226+
+ e.getMessage());
177227
future.setException(e);
178228
refreshFuture.set(null);
179229
}
@@ -201,13 +251,13 @@ private void handleRefreshFailure(Exception e) {
201251
// concurrent failures from logging redundant messages or incorrectly calculating
202252
// the exponential backoff.
203253
if (cooldownState.compareAndSet(currentCooldownState, next)) {
204-
LoggingUtils.log(
254+
log(
205255
LOGGER_PROVIDER,
206256
Level.FINE,
207257
null,
208-
"Regional Access Boundary lookup failed; entering cooldown for "
258+
"Regional Access Boundary lookup was not successful; will retry after a cooldown of "
209259
+ (next.durationMillis / 60000)
210-
+ "m. Error: "
260+
+ "m. This is handled automatically. Details: "
211261
+ e.getMessage());
212262
}
213263
}

0 commit comments

Comments
 (0)