adutra commented on code in PR #17389:
URL: https://github.com/apache/iceberg/pull/17389#discussion_r4122945075
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Util.java:
##########
@@ -651,6 +655,130 @@ 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,
+ fileRefreshDelayMillis(expiresAtMillis, refreshBufferMillis),
+ 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 =
Review Comment:
Hmm this can in theory race with `close()` + `stopRefreshing()`, because
`refreshFromFile` rebuilds the config from a stale `keepRefreshed=true`.
TBH `refresh()` has the same race condition (line 531), so this pattern
already exists. But here it's even worse: on a read failure or an unrotated
token it retries every 5s (`FILE_REFRESH_RETRY_WAIT_MILLIS`). Its only exit
condition is `keepRefreshed=false`, which is precisely the flag the race could
erase. So this refresh loop could keep running forever after the session is
closed.
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Util.java:
##########
@@ -651,6 +655,130 @@ 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,
+ fileRefreshDelayMillis(expiresAtMillis, refreshBufferMillis),
+ 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() {
Review Comment:
The file is read once when the session is created. After that it's only
re-read by a timer set to `exp - token-path-refresh-buffer-ms`, so whether we
pick up a rotated token depends on the `exp` claim rather than on the file
actually changing. Some cases where this breaks down:
- The kubelet rotates projected tokens at ~80% of their lifetime, and
sidecars like Vault agent can rotate at any time. Until the timer fires we keep
sending the old token, which fails if the issuer revokes it on rotation.
- Legacy auto-mounted tokens can have an `exp` about a year out while the
file is rotated hourly.
- Opaque tokens fall back to a fixed 1h.
- `token-refresh-enabled=false` silently turns off file rotation too.
Could we instead resolve the token lazily when the session is used
(`headers()` / `authenticate()`), with a short cache? For example, that's
roughly what [client-go's
`CachedFileTokenSource`](https://github.com/kubernetes/client-go/blob/master/transport/token_source.go)
does.
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Util.java:
##########
@@ -651,6 +655,130 @@ 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)
Review Comment:
This builds the session config with both `token` (a snapshot of the file)
and `tokenPath` which is weird. This combination is rejected by
`AuthConfig.fromProperties` as invalid BTW. Besides, the two values can drift
apart, and `tokenPath` leaks into every child session through
`.from(parent.config())` in `fromAccessToken`, `fromTokenExchange` and
`fromTokenResponse`. It also makes the session look eligible for the generic
OAuth2 `refresh()` path, since `token() != null`.
--
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]
