Skip to content

Commit 02e8bdf

Browse files
committed
fix(auth): Skip RAB lookup in case of non-email response from MDS. (#13331)
In ComputeEngineCredentials when running on GKE platform, the getAccount() call may return a value which isn't an email. In this case the right behaviour is to skip RAB lookup which is what this PR does. Added tests.
1 parent 461843c commit 02e8bdf

3 files changed

Lines changed: 104 additions & 17 deletions

File tree

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

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@
7272
import java.util.Objects;
7373
import java.util.logging.Level;
7474
import java.util.logging.Logger;
75+
import java.util.regex.Pattern;
7576

7677
/**
7778
* OAuth2 credentials representing the built-in service account for a Google Compute Engine VM.
@@ -117,6 +118,7 @@ public class ComputeEngineCredentials extends GoogleCredentials
117118

118119
private static final String PARSE_ERROR_PREFIX = "Error parsing token refresh response. ";
119120
private static final String PARSE_ERROR_ACCOUNT = "Error parsing service account response. ";
121+
private static final Pattern EMAIL_PATTERN = Pattern.compile("^[^@]+@[^@]+\\.[^@]+$");
120122
private static final long serialVersionUID = -4113476462526554235L;
121123

122124
private final String transportFactoryClassName;
@@ -800,8 +802,19 @@ public String getAccount() {
800802
@InternalApi
801803
@Override
802804
public String getRegionalAccessBoundaryUrl() throws IOException {
805+
String account = getAccount();
806+
// The MDS may return a non-email value for the account and we should skip RAB refresh in that
807+
// scenario.
808+
if (account == null || !EMAIL_PATTERN.matcher(account).matches()) {
809+
LoggingUtils.log(
810+
LOGGER_PROVIDER,
811+
Level.INFO,
812+
Collections.emptyMap(),
813+
"Unable to retrieve this instance's email and will skip the regional request routing. Proceeding with request");
814+
return null;
815+
}
803816
return String.format(
804-
OAuth2Utils.IAM_CREDENTIALS_ALLOWED_LOCATIONS_URL_FORMAT_SERVICE_ACCOUNT, getAccount());
817+
OAuth2Utils.IAM_CREDENTIALS_ALLOWED_LOCATIONS_URL_FORMAT_SERVICE_ACCOUNT, account);
805818
}
806819

807820
/**

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

Lines changed: 19 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -37,11 +37,11 @@
3737
import com.google.api.core.InternalApi;
3838
import com.google.auth.http.HttpTransportFactory;
3939
import com.google.common.annotations.VisibleForTesting;
40-
import com.google.common.util.concurrent.SettableFuture;
4140
import java.util.concurrent.ExecutorService;
4241
import java.util.concurrent.LinkedBlockingQueue;
4342
import java.util.concurrent.ThreadPoolExecutor;
4443
import java.util.concurrent.TimeUnit;
44+
import java.util.concurrent.atomic.AtomicBoolean;
4545
import java.util.concurrent.atomic.AtomicInteger;
4646
import java.util.concurrent.atomic.AtomicReference;
4747
import java.util.logging.Level;
@@ -75,16 +75,16 @@ final class RegionalAccessBoundaryManager {
7575
private final AtomicReference<RegionalAccessBoundary> cachedRAB = new AtomicReference<>();
7676

7777
/**
78-
* refreshFuture acts as an atomic gate for request de-duplication. If a future is present, it
79-
* indicates a background refresh is already in progress. It also provides a handle for
80-
* observability and unit testing to track the background task's lifecycle.
78+
* isRefreshing acts as an atomic gate for request de-duplication. If true, it indicates a
79+
* background refresh is already in progress.
8180
*/
82-
private final AtomicReference<SettableFuture<RegionalAccessBoundary>> refreshFuture =
83-
new AtomicReference<>();
81+
private final AtomicBoolean isRefreshing = new AtomicBoolean(false);
8482

8583
private final AtomicReference<CooldownState> cooldownState =
8684
new AtomicReference<>(new CooldownState(0, INITIAL_COOLDOWN_MILLIS));
8785

86+
private final AtomicBoolean skipRAB = new AtomicBoolean(false);
87+
8888
// Unbounded thread creation is discouraged in library code to avoid resource
8989
// exhaustion. A shared, bounded executor service ensures a hard limit (5)
9090
// on concurrent refresh tasks, while threadCount provides unique names
@@ -178,7 +178,7 @@ void triggerAsyncRefresh(
178178
final HttpTransportFactory transportFactory,
179179
final RegionalAccessBoundaryProvider provider,
180180
final AccessToken accessToken) {
181-
if (isCooldownActive()) {
181+
if (skipRAB.get() || isCooldownActive()) {
182182
return;
183183
}
184184

@@ -187,28 +187,28 @@ void triggerAsyncRefresh(
187187
return;
188188
}
189189

190-
SettableFuture<RegionalAccessBoundary> future = SettableFuture.create();
191190
// Atomically check if a refresh is already running. If compareAndSet returns true,
192191
// this thread "won the race" and is responsible for starting the background task.
193192
// All other concurrent threads will return false and exit immediately.
194-
if (refreshFuture.compareAndSet(null, future)) {
193+
if (isRefreshing.compareAndSet(false, true)) {
195194
Runnable refreshTask =
196195
() -> {
197196
try {
198197
String url = provider.getRegionalAccessBoundaryUrl();
198+
if (url == null) {
199+
skipRAB.set(true);
200+
return;
201+
}
199202
RegionalAccessBoundary newRAB =
200203
RegionalAccessBoundary.refresh(
201204
transportFactory, url, accessToken, clock, maxRetryElapsedTimeMillis);
202205
cachedRAB.set(newRAB);
203206
resetCooldown();
204-
// Complete the future so monitors (like unit tests) know we are done.
205-
future.set(newRAB);
206207
} catch (Exception e) {
207208
handleRefreshFailure(e);
208-
future.setException(e);
209209
} finally {
210210
// Open the gate again for future refresh requests.
211-
refreshFuture.set(null);
211+
isRefreshing.set(false);
212212
}
213213
};
214214

@@ -224,8 +224,7 @@ void triggerAsyncRefresh(
224224
"Could not submit background refresh task for Regional Access Boundary. "
225225
+ "This is non-blocking and the library will attempt to refresh on the next access. Error: "
226226
+ e.getMessage());
227-
future.setException(e);
228-
refreshFuture.set(null);
227+
isRefreshing.set(false);
229228
}
230229
}
231230
}
@@ -279,6 +278,11 @@ long getCurrentCooldownMillis() {
279278
return cooldownState.get().durationMillis;
280279
}
281280

281+
@VisibleForTesting
282+
boolean isSkipRAB() {
283+
return skipRAB.get();
284+
}
285+
282286
private static class CooldownState {
283287
/** The time (in milliseconds from epoch) when the current cooldown period expires. */
284288
final long expiryTime;

google-auth-library-java/oauth2_http/javatests/com/google/auth/oauth2/ComputeEngineCredentialsTest.java

Lines changed: 71 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1213,7 +1213,7 @@ void getProjectId_explicitSet_noMDsCall() {
12131213
assertEquals(0, transportFactory.transport.getRequestCount());
12141214
}
12151215

1216-
@org.junit.jupiter.api.Test
1216+
@Test
12171217
void refresh_regionalAccessBoundarySuccess() throws IOException, InterruptedException {
12181218

12191219
String defaultAccountEmail = "default@email.com";
@@ -1242,6 +1242,76 @@ void refresh_regionalAccessBoundarySuccess() throws IOException, InterruptedExce
12421242
Arrays.asList(TestUtils.REGIONAL_ACCESS_BOUNDARY_ENCODED_LOCATION));
12431243
}
12441244

1245+
@Test
1246+
void refresh_regionalAccessBoundaryNonEmail_skipsRABLookup()
1247+
throws IOException, InterruptedException {
1248+
String nonEmailAccount = "non-email-account-value";
1249+
MockMetadataServerTransportFactory transportFactory = new MockMetadataServerTransportFactory();
1250+
RegionalAccessBoundary regionalAccessBoundary =
1251+
new RegionalAccessBoundary(
1252+
TestUtils.REGIONAL_ACCESS_BOUNDARY_ENCODED_LOCATION,
1253+
TestUtils.REGIONAL_ACCESS_BOUNDARY_LOCATIONS,
1254+
null);
1255+
transportFactory.transport.setRegionalAccessBoundary(regionalAccessBoundary);
1256+
transportFactory.transport.setServiceAccountEmail(nonEmailAccount);
1257+
1258+
ComputeEngineCredentials credentials =
1259+
ComputeEngineCredentials.newBuilder().setHttpTransportFactory(transportFactory).build();
1260+
1261+
// Before any call, skipRAB flag should be false
1262+
assertFalse(credentials.regionalAccessBoundaryManager.isSkipRAB());
1263+
1264+
// First call: triggers lookup which determines non-email, returns null, and sets skipRAB to
1265+
// true
1266+
Map<String, List<String>> headers = credentials.getRequestMetadata();
1267+
assertNull(headers.get(X_ALLOWED_LOCATIONS_HEADER_KEY));
1268+
1269+
// Since the task is scheduled asynchronously on the shared executor, wait for it to complete
1270+
long deadline = System.currentTimeMillis() + 5000;
1271+
while (!credentials.regionalAccessBoundaryManager.isSkipRAB()
1272+
&& System.currentTimeMillis() < deadline) {
1273+
Thread.sleep(50);
1274+
}
1275+
1276+
// Verify skipRAB flag has been set to true
1277+
assertTrue(credentials.regionalAccessBoundaryManager.isSkipRAB());
1278+
1279+
// Verify RAB is still null
1280+
assertNull(credentials.getRegionalAccessBoundary());
1281+
1282+
// Second call: should bypass triggerAsyncRefresh completely and remain null
1283+
headers = credentials.getRequestMetadata();
1284+
assertNull(headers.get(X_ALLOWED_LOCATIONS_HEADER_KEY));
1285+
}
1286+
1287+
@Test
1288+
void getRegionalAccessBoundaryUrl_validEmail_returnsUrl() throws IOException {
1289+
MockMetadataServerTransportFactory transportFactory = new MockMetadataServerTransportFactory();
1290+
String defaultAccountEmail = "mail@mail.com";
1291+
1292+
transportFactory.transport.setServiceAccountEmail(defaultAccountEmail);
1293+
ComputeEngineCredentials credentials =
1294+
ComputeEngineCredentials.newBuilder().setHttpTransportFactory(transportFactory).build();
1295+
1296+
String expectedUrl =
1297+
String.format(
1298+
OAuth2Utils.IAM_CREDENTIALS_ALLOWED_LOCATIONS_URL_FORMAT_SERVICE_ACCOUNT,
1299+
defaultAccountEmail);
1300+
assertEquals(expectedUrl, credentials.getRegionalAccessBoundaryUrl());
1301+
}
1302+
1303+
@Test
1304+
void getRegionalAccessBoundaryUrl_invalidEmail_returnsNull() throws IOException {
1305+
MockMetadataServerTransportFactory transportFactory = new MockMetadataServerTransportFactory();
1306+
String defaultAccountEmail = "default"; // non-email account format
1307+
1308+
transportFactory.transport.setServiceAccountEmail(defaultAccountEmail);
1309+
ComputeEngineCredentials credentials =
1310+
ComputeEngineCredentials.newBuilder().setHttpTransportFactory(transportFactory).build();
1311+
1312+
assertNull(credentials.getRegionalAccessBoundaryUrl());
1313+
}
1314+
12451315
private void waitForRegionalAccessBoundary(GoogleCredentials credentials)
12461316
throws InterruptedException {
12471317
long deadline = System.currentTimeMillis() + 5000;

0 commit comments

Comments
 (0)