uros-b commented on code in PR #17389:
URL: https://github.com/apache/iceberg/pull/17389#discussion_r3978635814
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Util.java:
##########
@@ -651,6 +655,121 @@ public static AuthSession fromAccessToken(
return session;
}
+ /**
+ * Creates a session whose token is sourced from a file, such as a
Kubernetes-mounted projected
+ * service account token. The file is periodically re-read ahead of the
current token's
+ * expiration (see {@code refreshBufferMillis}) so that a token rotated in
place is picked up
+ * without restarting the process. No {@link RESTClient} is required since
refreshing never
+ * calls out over the network; it only re-reads the file.
+ */
+ public static AuthSession fromTokenFile(
+ ScheduledExecutorService executor,
+ String tokenPath,
+ long refreshBufferMillis,
+ AuthSession parent) {
+ String token;
+ try {
+ token = readTokenFile(tokenPath);
+ } catch (IOException e) {
+ throw new UncheckedIOException("Failed to read token file: " +
tokenPath, e);
+ }
+
+ Long expiresAtMillis = OAuth2Util.expiresAtMillis(token);
+ if (null == expiresAtMillis) {
+ expiresAtMillis = System.currentTimeMillis() +
OAuth2Properties.TOKEN_EXPIRES_IN_MS_DEFAULT;
+ }
+
+ AuthSession session =
+ new AuthSession(
+ RESTUtil.merge(parent.headers(), authHeaders(token)),
+ AuthConfig.builder()
+ .from(parent.config())
+ .token(token)
+ .tokenPath(tokenPath)
+ .tokenType(OAuth2Properties.ACCESS_TOKEN_TYPE)
+ .expiresAtMillis(expiresAtMillis)
+ .tokenPathRefreshBufferMillis(refreshBufferMillis)
+ .build());
+
+ if (null != executor) {
+ scheduleFileTokenRefresh(executor, session, expiresAtMillis,
refreshBufferMillis);
+ }
+
+ return session;
+ }
+
+ private static String readTokenFile(String tokenPath) throws IOException {
+ return Files.readString(Path.of(tokenPath)).trim();
+ }
+
+ /**
+ * Re-reads the token from {@link AuthConfig#tokenPath()} and updates this
session's headers and
+ * config accordingly.
+ *
+ * @return the new token's expiration time in epoch millis, or null if the
file could not be
+ * read
+ */
+ Long refreshFromFile() {
+ String tokenPath = config.tokenPath();
+ String token;
+ try {
+ token = readTokenFile(tokenPath);
+ } catch (IOException e) {
+ LOG.warn("Failed to re-read token file {}, will retry", tokenPath, e);
+ return null;
+ }
+
+ Long expiresAtMillis = OAuth2Util.expiresAtMillis(token);
+ if (null == expiresAtMillis) {
+ expiresAtMillis = System.currentTimeMillis() +
OAuth2Properties.TOKEN_EXPIRES_IN_MS_DEFAULT;
+ }
+
+ this.config =
+ AuthConfig.builder()
+ .from(config())
+ .token(token)
+ .tokenType(OAuth2Properties.ACCESS_TOKEN_TYPE)
+ .expiresAtMillis(expiresAtMillis)
+ .build();
+ this.headers = RESTUtil.merge(this.headers, authHeaders(token));
+
+ return expiresAtMillis;
+ }
+
+ /**
+ * Schedule the next file-based token refresh, {@code refreshBufferMillis}
ahead of {@code
+ * expiresAtMillis}. Unlike {@link #scheduleTokenRefresh}, this never
calls out over the
+ * network: on a transient read failure, it retries after a short fixed
delay instead of giving
+ * up.
+ */
+ @SuppressWarnings("FutureReturnValueIgnored")
+ private static void scheduleFileTokenRefresh(
Review Comment:
scheduleFileTokenRefresh (retry branch): the intended 5-second failure
backoff collapses to ~10 ms in practice. On a failed re-read, the code sets
nextExpiresAtMillis = now + FILE_REFRESH_RETRY_WAIT_MILLIS (now+5 s) and passes
it back as the expiresAtMillis argument to the recursive call, which then
computes waitMillis = max((now+5000) - refreshBufferMillis - now,
MIN_REFRESH_WAIT_MILLIS). With the 300 s default buffer this yields max(5000 -
300000, 10) = 10 ms. A persistently missing, unreadable, or already-expired-JWT
file therefore produces ~100 reads/sec plus a LOG.warn every 10 ms on the
shared authRefreshPool. FILE_REFRESH_RETRY_WAIT_MILLIS is effectively dead
whenever refreshBufferMillis > FILE_REFRESH_RETRY_WAIT_MILLIS (the default).
Fix: on the failure branch, call executor.schedule(retryTask,
FILE_REFRESH_RETRY_WAIT_MILLIS, MILLISECONDS) directly rather than recursing
with a fabricated near-future expiry.
##########
core/src/test/java/org/apache/iceberg/rest/auth/TestOAuth2Util.java:
##########
@@ -253,6 +258,98 @@ private static void
assertRefreshIncludesOptionalOAuthParams(long expiresAtMilli
}
}
+ @Test
+ void fromTokenFileReadsInitialToken(@TempDir Path tempDir) throws
IOException {
+ String token = tokenWithExp(7200);
+ Path tokenFile = tempDir.resolve("token");
+ Files.writeString(tokenFile, token);
+
+ AuthSession parent = new AuthSession(Map.of(),
AuthConfig.builder().build());
+ AuthSession session = AuthSession.fromTokenFile(null,
tokenFile.toString(), 300_000L, parent);
+
+ assertThat(session.token()).isEqualTo(token);
+
assertThat(session.expiresAtMillis()).isEqualTo(TimeUnit.SECONDS.toMillis(7200));
+ assertThat(session.headers()).containsEntry("Authorization", "Bearer " +
token);
+ }
+
+ @Test
+ void fromTokenFileTrimsWhitespace(@TempDir Path tempDir) throws IOException {
+ String token = tokenWithExp(7200);
+ Path tokenFile = tempDir.resolve("token");
+ Files.writeString(tokenFile, token + "\n");
+
+ AuthSession parent = new AuthSession(Map.of(),
AuthConfig.builder().build());
+ AuthSession session = AuthSession.fromTokenFile(null,
tokenFile.toString(), 300_000L, parent);
+
+ assertThat(session.token()).isEqualTo(token);
+ }
+
+ @Test
+ void fromTokenFileMissingFileThrows(@TempDir Path tempDir) {
+ Path missing = tempDir.resolve("does-not-exist");
+ AuthSession parent = new AuthSession(Map.of(),
AuthConfig.builder().build());
+
+ assertThatThrownBy(() -> AuthSession.fromTokenFile(null,
missing.toString(), 300_000L, parent))
+ .isInstanceOf(UncheckedIOException.class)
+ .hasMessageContaining("Failed to read token file: " + missing);
+ }
+
+ @Test
+ void refreshFromFilePicksUpRotatedToken(@TempDir Path tempDir) throws
IOException {
+ String initialToken = tokenWithExp(7200);
+ Path tokenFile = tempDir.resolve("token");
+ Files.writeString(tokenFile, initialToken);
+
+ AuthSession parent = new AuthSession(Map.of(),
AuthConfig.builder().build());
+ AuthSession session = AuthSession.fromTokenFile(null,
tokenFile.toString(), 300_000L, parent);
+
+ String rotatedToken = tokenWithExp(500);
+ Files.writeString(tokenFile, rotatedToken);
+
+ Long newExpiresAtMillis = session.refreshFromFile();
+
+ assertThat(newExpiresAtMillis).isEqualTo(TimeUnit.SECONDS.toMillis(500));
+ assertThat(session.token()).isEqualTo(rotatedToken);
+
assertThat(session.expiresAtMillis()).isEqualTo(TimeUnit.SECONDS.toMillis(500));
+ assertThat(session.headers()).containsEntry("Authorization", "Bearer " +
rotatedToken);
+ }
+
+ @Test
+ void refreshFromFileOpaqueTokenFallsBackToDefaultExpiry(@TempDir Path
tempDir)
+ throws IOException {
+ Path tokenFile = tempDir.resolve("token");
+ Files.writeString(tokenFile, "opaque-token");
+
+ AuthSession parent = new AuthSession(Map.of(),
AuthConfig.builder().build());
+ AuthSession session = AuthSession.fromTokenFile(null,
tokenFile.toString(), 300_000L, parent);
+
+ long before = System.currentTimeMillis();
+ Long expiresAtMillis = session.refreshFromFile();
+ long after = System.currentTimeMillis();
+
+ assertThat(expiresAtMillis)
+ .isBetween(
+ before + OAuth2Properties.TOKEN_EXPIRES_IN_MS_DEFAULT,
+ after + OAuth2Properties.TOKEN_EXPIRES_IN_MS_DEFAULT);
+ }
+
+ @Test
+ void refreshFromFileTransientFailureKeepsStaleToken(@TempDir Path tempDir)
throws IOException {
+ String token = tokenWithExp(7200);
+ Path tokenFile = tempDir.resolve("token");
+ Files.writeString(tokenFile, token);
+
+ AuthSession parent = new AuthSession(Map.of(),
AuthConfig.builder().build());
+ AuthSession session = AuthSession.fromTokenFile(null,
tokenFile.toString(), 300_000L, parent);
+
+ Files.delete(tokenFile);
+
+ Long result = session.refreshFromFile();
+
+ assertThat(result).isNull();
+ assertThat(session.token()).isEqualTo(token);
+ }
+
Review Comment:
No test exercises scheduleFileTokenRefresh; every refresh test calls
refreshFromFile() directly, so the retry-interval bug above is uncaught by CI.
A test that injects an unreadable file after session creation and asserts the
reschedule delay is at least FILE_REFRESH_RETRY_WAIT_MILLIS would close this
gap.
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Manager.java:
##########
@@ -56,6 +56,7 @@ public class OAuth2Manager implements AuthManager {
private static final Set<String> TABLE_SESSION_ALLOW_LIST =
ImmutableSet.<String>builder()
.add(OAuth2Properties.TOKEN)
+ .add(OAuth2Properties.TOKEN_PATH)
Review Comment:
TOKEN_PATH is added to TABLE_SESSION_ALLOW_LIST, but maybeCreateChildSession
has no handling for it: the code checks TOKEN, CREDENTIAL, and
TOKEN_PREFERENCE_ORDER only, so a table-level token-path silently causes the
child session to inherit the parent rather than creating a file-based one.
Either add a TOKEN_PATH branch parallel to the TOKEN branch in
maybeCreateChildSession, or remove the allow-list addition and document the
limitation; the decision should be explicit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
