Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces support for Regional Access Boundaries (RAB) across various credential types, including Compute Engine, External Account, and Service Account credentials. It adds a RegionalAccessBoundaryManager to handle the asynchronous fetching, caching, and cooldown logic for RAB data, which is then included as an x-allowed-locations header in outgoing requests. Feedback includes removing unnecessary .rej files from the repository, replacing an unbounded thread pool with a bounded one for safety, and simplifying the logic for adding quota project IDs to request metadata.
| @@ -0,0 +1,26 @@ | |||
| diff a/google-auth-library-java/oauth2_http/java/com/google/auth/oauth2/ExternalAccountAuthorizedUserCredentials.java b/google-auth-library-java/oauth2_http/java/com/google/auth/oauth2/ExternalAccountAuthorizedUserCredentials.java (rejected hunks) | |||
There was a problem hiding this comment.
The pull request contains several .rej files (e.g., ExternalAccountAuthorizedUserCredentials.java.rej, ExternalAccountCredentials.java.rej, etc.). These files are typically generated when a patch fails to apply cleanly and should not be committed to the repository. Please remove all .rej files from the PR.
| private static final ExecutorService REFRESH_EXECUTOR = | ||
| Executors.newCachedThreadPool( | ||
| new ThreadFactory() { | ||
| @Override | ||
| public Thread newThread(Runnable r) { | ||
| Thread t = new Thread(r, "RAB-refresh-thread"); | ||
| t.setDaemon(true); | ||
| return t; | ||
| } | ||
| }); |
There was a problem hiding this comment.
The use of an unbounded thread pool (Executors.newCachedThreadPool()) is discouraged as it can lead to excessive thread creation under high load. Per the general rules, a bounded thread pool should be used instead.
private static final ExecutorService REFRESH_EXECUTOR =
new java.util.concurrent.ThreadPoolExecutor(
0,
100,
60L,
java.util.concurrent.TimeUnit.SECONDS,
new java.util.concurrent.SynchronousQueue<Runnable>(),
new ThreadFactory() {
@Override
public Thread newThread(Runnable r) {
Thread t = new Thread(r, "RAB-refresh-thread");
t.setDaemon(true);
return t;
}
});References
- For safety, use a bounded thread pool (e.g., ThreadPoolExecutor with a defined maximum size) instead of an unbounded one (e.g., Executors.newCachedThreadPool()), even if the current logic seems to limit concurrent tasks.
| Map<String, List<String>> headers = new HashMap<>(super.getAdditionalHeaders()); | ||
|
|
||
| String quotaProjectId = this.getQuotaProjectId(); | ||
| if (quotaProjectId != null) { | ||
| return addQuotaProjectIdToRequestMetadata(quotaProjectId, headers); | ||
| } | ||
| return headers; | ||
| return addQuotaProjectIdToRequestMetadata(quotaProjectId, headers); |
There was a problem hiding this comment.
This method can be simplified by passing the result of super.getAdditionalHeaders() directly to addQuotaProjectIdToRequestMetadata. This avoids creating an unnecessary intermediate HashMap and ensures that an unmodifiable map is returned when quotaProjectId is null.
| Map<String, List<String>> headers = new HashMap<>(super.getAdditionalHeaders()); | |
| String quotaProjectId = this.getQuotaProjectId(); | |
| if (quotaProjectId != null) { | |
| return addQuotaProjectIdToRequestMetadata(quotaProjectId, headers); | |
| } | |
| return headers; | |
| return addQuotaProjectIdToRequestMetadata(quotaProjectId, headers); | |
| return addQuotaProjectIdToRequestMetadata(getQuotaProjectId(), super.getAdditionalHeaders()); |
Contains changes for the feature Regional Access Boundary (Previously Called Trust Boundaries).
The following are salient changes: