Skip to content

Add AthenzTokenClient API for standalone Athenz token fetching - #6691

Merged
ikhoon merged 7 commits into
line:mainfrom
ikhoon:athenz-token-client
Apr 7, 2026
Merged

Add AthenzTokenClient API for standalone Athenz token fetching#6691
ikhoon merged 7 commits into
line:mainfrom
ikhoon:athenz-token-client

Conversation

@ikhoon

@ikhoon ikhoon commented Apr 1, 2026

Copy link
Copy Markdown
Contributor

Motivation:

Previously, Athenz token fetching was tightly coupled to the AthenzClient
HTTP decorator. Users who wanted to use Athenz tokens with non-Armeria
clients (e.g., Spring WebClient or RestTemplate) had no public API to
fetch tokens independently.

Modifications:

  • Add AthenzTokenClient public interface with domainName(),
    roleNames(), and getToken() methods.
  • Add AthenzTokenClientBuilder for constructing token clients with
    domain, roles, refresh, and preload settings.
  • Add TokenClientSetters interface to share setter methods between
    AthenzClientBuilder and AthenzTokenClientBuilder.
  • Refactor AthenzClientBuilder to delegate to AthenzTokenClientBuilder.
  • Refactor AthenzClient constructor to accept a pre-built
    AthenzTokenClient instead of building one internally.
  • Delete the old internal TokenClient interface.
  • Add domainName(), roleNames(), and toString() to
    AccessTokenClient and RoleTokenClient.

Result:

  • Closes Expose Athenz TokenClient as public API #6431.

  • Users can now create an AthenzTokenClient independently:

    AthenzTokenClient tokenClient =
        AthenzTokenClient.builder(ztsBaseClient)
                          .domainName("my-domain")
                          .roleNames("my-role")
                          .build();
    
    tokenClient.getToken().thenAccept(token -> {
        // Use the token for your own client.
    });

ikhoon added 2 commits March 31, 2026 22:00
Motivation:

Currently, AsyncLoader loads values lazily on the first load() call. For token
clients (Athenz, OAuth2), this means the first request incurs the latency of
fetching a token. A preload option allows tokens to be fetched eagerly at
construction time, eliminating cold-start latency.

Modifications:

- Add `preload(boolean)` to `AsyncLoaderBuilder` that calls `load()` in
  the `DefaultAsyncLoader` constructor when enabled.
- Wire the `preload` option through `OAuth2AuthorizationGrantBuilder` and
  `DefaultOAuth2AuthorizationGrant`.
- Propagate `preload` through `AthenzClientBuilder`, `AthenzClient`,
  `AccessTokenClient`, and `RoleTokenClient`.
- Add tests for preload in `DefaultAsyncLoaderTest`,
  `OAuth2ClientCredentialsGrantTest`, `AccessTokenClientTest`, and
  `RoleTokenClientTest`.

Result:

- Users can set `preload(true)` on `AsyncLoader`, `OAuth2AuthorizationGrantBuilder`,
  or `AthenzClientBuilder` to eagerly fetch tokens at build time, reducing
  first-request latency.
Motivation:

Previously, Athenz token fetching was tightly coupled to the AthenzClient
HTTP decorator. Users who wanted to use Athenz tokens with non-Armeria
clients (e.g., Spring WebClient or RestTemplate) had no public API to
fetch tokens independently.

Modifications:

- Add `AthenzTokenClient` public interface with `domainName()`,
  `roleNames()`, and `getToken()` methods.
- Add `AthenzTokenClientBuilder` for constructing token clients with
  domain, roles, refresh, and preload settings.
- Add `TokenClientSetters` interface to share setter methods between
  `AthenzClientBuilder` and `AthenzTokenClientBuilder`.
- Refactor `AthenzClientBuilder` to delegate to `AthenzTokenClientBuilder`.
- Refactor `AthenzClient` constructor to accept a pre-built
  `AthenzTokenClient` instead of building one internally.
- Delete the old internal `TokenClient` interface.
- Add `domainName()`, `roleNames()`, and `toString()` to
  `AccessTokenClient` and `RoleTokenClient`.

Result:

- Closes line#6431.
- Users can now create an `AthenzTokenClient` independently:

  ```java
  AthenzTokenClient tokenClient =
      AthenzTokenClient.builder(ztsBaseClient)
                        .domainName("my-domain")
                        .roleNames("my-role")
                        .build();

  tokenClient.getToken().thenAccept(token -> {
      // Use the token for your own client.
  });
  ```
@ikhoon ikhoon added this to the 1.38.0 milestone Apr 1, 2026
@coderabbitai

coderabbitai Bot commented Apr 1, 2026

Copy link
Copy Markdown
Contributor

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 6d722d0b-c512-4952-aa7b-7860b446371b

📥 Commits

Reviewing files that changed from the base of the PR and between fec70ba and 8ce2af6.

📒 Files selected for processing (2)
  • athenz/src/main/java/com/linecorp/armeria/client/athenz/AthenzClientBuilder.java
  • athenz/src/main/java/com/linecorp/armeria/server/athenz/AbstractAthenzAuthorizerBuilder.java
✅ Files skipped from review due to trivial changes (1)
  • athenz/src/main/java/com/linecorp/armeria/client/athenz/AthenzClientBuilder.java

📝 Walkthrough

Walkthrough

The Athenz token client API was refactored: a new public AthenzTokenClient interface and AthenzTokenClientBuilder replace the old TokenClient pattern. AccessTokenClient and RoleTokenClient implement the new interface with optional preload. AthenzClient and its builder now accept/configure pre-built token clients. Server-side key providers gain explicit init semantics.

Changes

Cohort / File(s) Summary
New public token interface & setters
athenz/src/main/java/com/linecorp/armeria/client/athenz/AthenzTokenClient.java, athenz/src/main/java/com/linecorp/armeria/client/athenz/TokenClientSetters.java
Adds AthenzTokenClient public interface (builder factory, domainName(), roleNames(), getToken()) and fluent TokenClientSetters for builder-style configuration (domainName, roleNames, refreshBefore, preload).
Token client builder
athenz/src/main/java/com/linecorp/armeria/client/athenz/AthenzTokenClientBuilder.java
New public builder implementing TokenClientSetters that validates domainName, accepts roleNames, refreshBefore, preload, roleToken flag, and builds either RoleTokenClient or AccessTokenClient.
Token client implementations
athenz/src/main/java/com/linecorp/armeria/client/athenz/AccessTokenClient.java, athenz/src/main/java/com/linecorp/armeria/client/athenz/RoleTokenClient.java
Both classes now implement AthenzTokenClient; constructors accept preload boolean; expose domainName() and roleNames(); internal role-name handling adjusted; toString() overrides added.
Athenz client & builder integration
athenz/src/main/java/com/linecorp/armeria/client/athenz/AthenzClient.java, athenz/src/main/java/com/linecorp/armeria/client/athenz/AthenzClientBuilder.java
AthenzClient now takes an AthenzTokenClient instance instead of raw domain/roles/refreshBefore; builder delegates token configuration to AthenzTokenClientBuilder and passes the built token client into AthenzClient.
Removed legacy interface
athenz/src/main/java/com/linecorp/armeria/client/athenz/TokenClient.java
The simple TokenClient functional interface was removed (replaced by AthenzTokenClient).
OAuth2 preload support
oauth2/src/main/java/com/linecorp/armeria/client/auth/oauth2/OAuth2AuthorizationGrantBuilder.java
Adds preload(boolean) to the OAuth2 authorization-grant builder and threads preload into the constructed grant.
Server-side key init changes
athenz/src/main/java/com/linecorp/armeria/server/athenz/AbstractAthenzAuthorizerBuilder.java, athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzPublicKeyProvider.java
AthenzPublicKeyProvider gains a blocking init() that waits (30s) for initial key loads; builder invokes this init() before creating policy clients.

Sequence Diagram(s)

sequenceDiagram
    participant Builder as AthenzClientBuilder
    participant TokenBuilder as AthenzTokenClientBuilder
    participant TokenClient as AthenzTokenClient
    participant ZTS as ZtsBaseClient
    participant Loader as AsyncLoader / OAuth2Grant

    Builder->>TokenBuilder: delegate domain/roles/refreshBefore/preload
    TokenBuilder->>TokenClient: build() -> RoleTokenClient / AccessTokenClient
    TokenClient->>ZTS: request token (via HTTP)
    TokenClient->>Loader: AsyncLoader / OAuth2Grant (may preload)
    Loader-->>TokenClient: token (CompletableFuture)
    TokenClient-->>Builder: token client instance used by AthenzClient
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • trustin
  • jrhee17
  • minwoox

Poem

🐰 I nibble code and pull a thread,

Tokens brewed before they're fed.
Builders whisper, roles align,
Preload hums — the harvest fine.
Hops for devs, a carrot sign.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 32.86% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately reflects the main objective of the PR: introducing a public API for standalone Athenz token fetching, which is the primary focus of all changes.
Description check ✅ Passed The description clearly explains the motivation, modifications, and results, covering the main changes including the new AthenzTokenClient interface, builder, and refactoring work.
Linked Issues check ✅ Passed The PR fully addresses #6431 by exposing Athenz token-fetching as a public API through the new AthenzTokenClient interface and builder, enabling users to fetch tokens for non-Armeria clients.
Out of Scope Changes check ✅ Passed All changes are within scope: core token client refactoring, new public interfaces, builder implementation, and integration with AthenzClient. Minor OAuth2 preload feature addition supports the main objective.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
athenz/src/main/java/com/linecorp/armeria/client/athenz/RoleTokenClient.java (1)

1-15: ⚠️ Potential issue | 🟡 Minor

Copyright header uses "LINE Corporation" instead of "LY Corporation".

Same inconsistency as noted in other files. Consider using "LY Corporation" for consistency.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@athenz/src/main/java/com/linecorp/armeria/client/athenz/RoleTokenClient.java`
around lines 1 - 15, Update the file header comment in RoleTokenClient (top of
athenz/src/main/java/com/linecorp/armeria/client/athenz/RoleTokenClient.java) to
use "LY Corporation" instead of "LINE Corporation" so the copyright header
matches other files; replace all occurrences in the header block (the file-level
comment above the RoleTokenClient class) accordingly.
athenz/src/main/java/com/linecorp/armeria/client/athenz/AccessTokenClient.java (1)

1-15: ⚠️ Potential issue | 🟡 Minor

Copyright header uses "LINE Corporation" instead of "LY Corporation".

Same inconsistency as noted in AthenzTokenClient.java. Consider using "LY Corporation" for consistency with other files in this PR.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@athenz/src/main/java/com/linecorp/armeria/client/athenz/AccessTokenClient.java`
around lines 1 - 15, Update the file-level copyright header in
AccessTokenClient.java to match the project's standard by replacing "LINE
Corporation" with "LY Corporation"; locate the top-of-file header in
AccessTokenClient.java (the same header pattern used in AthenzTokenClient.java)
and adjust the copyright holder string so it is consistent across both files.
🧹 Nitpick comments (1)
core/src/main/java/com/linecorp/armeria/common/util/AsyncLoaderBuilder.java (1)

141-145: Polish preload Javadoc wording and semantics.

Tiny doc cleanup: “This options” → “This option”, and consider explicitly saying preload is async fire-and-forget (build does not wait for completion).

Suggested Javadoc tweak
-     * Preloads the value by calling the loader function immediately when {`@link` `#build`()} is called.
-     * This option is disabled by default, and the value is loaded lazily when {`@link` AsyncLoader#load()} is
-     * called for the first time.
+     * Preloads the value by calling the loader function immediately when {`@link` `#build`()} is called.
+     * This option is disabled by default, and the value is loaded lazily when {`@link` AsyncLoader#load()} is
+     * called for the first time.
+     * The preload is asynchronous and does not block {`@link` `#build`()}.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@core/src/main/java/com/linecorp/armeria/common/util/AsyncLoaderBuilder.java`
around lines 141 - 145, Fix the Javadoc on AsyncLoaderBuilder: correct "This
options" to "This option" and explicitly state preload is async fire-and-forget
by saying that when preload is enabled the loader is invoked asynchronously
during build() and build() does not wait for completion; retain that preload is
disabled by default and that, otherwise, the value is loaded lazily when
AsyncLoader#load() is called for the first time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@athenz/src/main/java/com/linecorp/armeria/client/athenz/AthenzTokenClient.java`:
- Around line 1-15: The file AthenzTokenClient.java has an outdated copyright
header reading "LINE Corporation"; update the file header to match the project
standard by replacing "LINE Corporation" with "LY Corporation" (follow the same
header format used in AthenzTokenClientBuilder.java and AthenzClient.java) so
the AthenzTokenClient class file uses the correct LY copyright block.

In
`@athenz/src/main/java/com/linecorp/armeria/client/athenz/TokenClientSetters.java`:
- Around line 28-64: Add the `@UnstableApi` annotation to the new public interface
TokenClientSetters to mark this API as unstable: import
com.linecorp.armeria.common.annotation.UnstableApi and place `@UnstableApi`
immediately above the TokenClientSetters declaration; no other behavioral
changes are required (the annotation only marks the interface and its public
methods as unstable).

In
`@oauth2/src/main/java/com/linecorp/armeria/client/auth/oauth2/OAuth2AuthorizationGrantBuilder.java`:
- Around line 166-167: Fix the Javadoc typo in OAuth2AuthorizationGrantBuilder's
preload API comment: change "This options is disabled by default..." to "This
option is disabled by default..." in the Javadoc for the preload behavior (the
comment associated with the OAuth2AuthorizationGrantBuilder/preload option).

---

Outside diff comments:
In
`@athenz/src/main/java/com/linecorp/armeria/client/athenz/AccessTokenClient.java`:
- Around line 1-15: Update the file-level copyright header in
AccessTokenClient.java to match the project's standard by replacing "LINE
Corporation" with "LY Corporation"; locate the top-of-file header in
AccessTokenClient.java (the same header pattern used in AthenzTokenClient.java)
and adjust the copyright holder string so it is consistent across both files.

In
`@athenz/src/main/java/com/linecorp/armeria/client/athenz/RoleTokenClient.java`:
- Around line 1-15: Update the file header comment in RoleTokenClient (top of
athenz/src/main/java/com/linecorp/armeria/client/athenz/RoleTokenClient.java) to
use "LY Corporation" instead of "LINE Corporation" so the copyright header
matches other files; replace all occurrences in the header block (the file-level
comment above the RoleTokenClient class) accordingly.

---

Nitpick comments:
In `@core/src/main/java/com/linecorp/armeria/common/util/AsyncLoaderBuilder.java`:
- Around line 141-145: Fix the Javadoc on AsyncLoaderBuilder: correct "This
options" to "This option" and explicitly state preload is async fire-and-forget
by saying that when preload is enabled the loader is invoked asynchronously
during build() and build() does not wait for completion; retain that preload is
disabled by default and that, otherwise, the value is loaded lazily when
AsyncLoader#load() is called for the first time.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: a3df7f51-494e-435f-98f4-aeeb2e95b72b

📥 Commits

Reviewing files that changed from the base of the PR and between 61c0fbf and 8107b81.

📒 Files selected for processing (16)
  • athenz/src/main/java/com/linecorp/armeria/client/athenz/AccessTokenClient.java
  • athenz/src/main/java/com/linecorp/armeria/client/athenz/AthenzClient.java
  • athenz/src/main/java/com/linecorp/armeria/client/athenz/AthenzClientBuilder.java
  • athenz/src/main/java/com/linecorp/armeria/client/athenz/AthenzTokenClient.java
  • athenz/src/main/java/com/linecorp/armeria/client/athenz/AthenzTokenClientBuilder.java
  • athenz/src/main/java/com/linecorp/armeria/client/athenz/RoleTokenClient.java
  • athenz/src/main/java/com/linecorp/armeria/client/athenz/TokenClient.java
  • athenz/src/main/java/com/linecorp/armeria/client/athenz/TokenClientSetters.java
  • athenz/src/test/java/com/linecorp/armeria/client/athenz/AccessTokenClientTest.java
  • athenz/src/test/java/com/linecorp/armeria/client/athenz/RoleTokenClientTest.java
  • core/src/main/java/com/linecorp/armeria/common/util/AsyncLoaderBuilder.java
  • core/src/main/java/com/linecorp/armeria/common/util/DefaultAsyncLoader.java
  • core/src/test/java/com/linecorp/armeria/common/util/DefaultAsyncLoaderTest.java
  • oauth2/src/main/java/com/linecorp/armeria/client/auth/oauth2/DefaultOAuth2AuthorizationGrant.java
  • oauth2/src/main/java/com/linecorp/armeria/client/auth/oauth2/OAuth2AuthorizationGrantBuilder.java
  • oauth2/src/test/java/com/linecorp/armeria/client/auth/oauth2/OAuth2ClientCredentialsGrantTest.java
💤 Files with no reviewable changes (1)
  • athenz/src/main/java/com/linecorp/armeria/client/athenz/TokenClient.java

@codecov

codecov Bot commented Apr 1, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 63.38028% with 26 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.94%. Comparing base (8150425) to head (fec70ba).
⚠️ Report is 396 commits behind head on main.

Files with missing lines Patch % Lines
...rmeria/client/athenz/AthenzTokenClientBuilder.java 70.83% 6 Missing and 1 partial ⚠️
...ecorp/armeria/client/athenz/AccessTokenClient.java 25.00% 6 Missing ⚠️
...inecorp/armeria/client/athenz/RoleTokenClient.java 50.00% 5 Missing and 1 partial ⚠️
...armeria/server/athenz/AthenzPublicKeyProvider.java 55.55% 4 Missing ⚠️
...orp/armeria/client/athenz/AthenzClientBuilder.java 70.00% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #6691      +/-   ##
============================================
- Coverage     74.46%   73.94%   -0.53%     
- Complexity    22234    24049    +1815     
============================================
  Files          1963     2174     +211     
  Lines         82437    90238    +7801     
  Branches      10764    11826    +1062     
============================================
+ Hits          61385    66724    +5339     
- Misses        15918    17914    +1996     
- Partials       5134     5600     +466     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzPublicKeyProvider.java`:
- Around line 75-78: The init() method in AthenzPublicKeyProvider currently
calls ztsKeyLoader.load().join() and zmsKeyLoader.load().join() which can block
indefinitely; replace these unbounded joins with bounded waits (e.g., use
CompletableFuture.get(timeout, TimeUnit.SECONDS) or
CompletableFuture.orTimeout(...)) for both ztsKeyLoader.load() and
zmsKeyLoader.load(), choose a sensible timeout (e.g., 5–15s), and handle
InterruptedException, ExecutionException and TimeoutException by logging (via
the class logger) and either proceeding safely or failing fast with a
descriptive exception so authorizer initialization cannot hang forever. Ensure
you update the init() method and use the symbols ztsKeyLoader, zmsKeyLoader, and
their load() calls when making the change.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 8478da04-dc77-44d7-a6cb-c4617cadd568

📥 Commits

Reviewing files that changed from the base of the PR and between 8107b81 and 342c592.

📒 Files selected for processing (2)
  • athenz/src/main/java/com/linecorp/armeria/server/athenz/AbstractAthenzAuthorizerBuilder.java
  • athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzPublicKeyProvider.java

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
athenz/src/main/java/com/linecorp/armeria/client/athenz/RoleTokenClient.java (1)

95-108: ⚠️ Potential issue | 🔴 Critical

Non-forbidden failures can be masked by a NullPointerException

At Line 96-107, when cause != null but status is not FORBIDDEN, the code falls through to response.content(). In exceptional completion, response can be null, which replaces the original failure with an NPE.

Suggested fix
+import java.util.concurrent.CompletionException;
...
         return preparation
                 .asJson(RoleToken.class)
                 .execute()
                 .handle((response, cause) -> {
                     if (cause != null) {
                         cause = Exceptions.peel(cause);
                         if (cause instanceof InvalidHttpResponseException) {
                             final InvalidHttpResponseException exception = (InvalidHttpResponseException) cause;
                             if (exception.response().status() == HttpStatus.FORBIDDEN) {
                                 throw new AccessDeniedException(
                                         "Failed to obtain an Athenz role token. (domain: " + domainName +
                                         ", roles: " + roleNamesString + ')', exception);
                             }
                         }
+                        throw new CompletionException(cause);
                     }
                     return response.content();
                 });
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@athenz/src/main/java/com/linecorp/armeria/client/athenz/RoleTokenClient.java`
around lines 95 - 108, The handler in RoleTokenClient's .handle((response,
cause) -> ...) accesses response.content() even when cause != null, which can be
null and cause an NPE; change the control flow so that after peeling the cause
and handling the FORBIDDEN case (InvalidHttpResponseException -> throw
AccessDeniedException), you rethrow or propagate the original cause (e.g.,
rethrow the peeled throwable or wrap it in a CompletionException) instead of
falling through to response.content(); only call response.content() when cause
== null to avoid masking the original failure.
🧹 Nitpick comments (1)
athenz/src/main/java/com/linecorp/armeria/client/athenz/RoleTokenClient.java (1)

52-53: Defensively snapshot roleNames to keep internal state consistent

At Line 52-53 and Line 70-71, this class stores and returns the incoming list reference directly. If a mutable list is ever passed by an in-package caller, roleNames() can diverge from roleNamesString (used for requests/errors). Snapshotting once avoids this class invariant risk.

Suggested refactor
+import com.google.common.collect.ImmutableList;
...
-        this.roleNames = roleNames;
-        roleNamesString = ROLE_JOINER.join(roleNames);
+        this.roleNames = ImmutableList.copyOf(roleNames);
+        roleNamesString = ROLE_JOINER.join(this.roleNames);

Also applies to: 70-71

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@athenz/src/main/java/com/linecorp/armeria/client/athenz/RoleTokenClient.java`
around lines 52 - 53, The class stores and returns the incoming List reference
directly causing a potential mismatch between the mutable roleNames and
roleNamesString; defensively snapshot the incoming list in the constructor
(e.g., replace assigning roleNames = roleNames with an immutable or new
ArrayList copy) and ensure the roleNames() accessor returns an
unmodifiable/immutable view or the same snapshot (not the original mutable
reference) so roleNamesString and roleNames remain consistent; update the
constructor and the roleNames() method (referencing fields roleNames and
roleNamesString) to use the defensive copy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In
`@athenz/src/main/java/com/linecorp/armeria/client/athenz/RoleTokenClient.java`:
- Around line 95-108: The handler in RoleTokenClient's .handle((response, cause)
-> ...) accesses response.content() even when cause != null, which can be null
and cause an NPE; change the control flow so that after peeling the cause and
handling the FORBIDDEN case (InvalidHttpResponseException -> throw
AccessDeniedException), you rethrow or propagate the original cause (e.g.,
rethrow the peeled throwable or wrap it in a CompletionException) instead of
falling through to response.content(); only call response.content() when cause
== null to avoid masking the original failure.

---

Nitpick comments:
In
`@athenz/src/main/java/com/linecorp/armeria/client/athenz/RoleTokenClient.java`:
- Around line 52-53: The class stores and returns the incoming List reference
directly causing a potential mismatch between the mutable roleNames and
roleNamesString; defensively snapshot the incoming list in the constructor
(e.g., replace assigning roleNames = roleNames with an immutable or new
ArrayList copy) and ensure the roleNames() accessor returns an
unmodifiable/immutable view or the same snapshot (not the original mutable
reference) so roleNamesString and roleNames remain consistent; update the
constructor and the roleNames() method (referencing fields roleNames and
roleNamesString) to use the defensive copy.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: f7f6eeee-17b4-442d-ac89-abbd507ae1e3

📥 Commits

Reviewing files that changed from the base of the PR and between 342c592 and 531a6bc.

📒 Files selected for processing (4)
  • athenz/src/main/java/com/linecorp/armeria/client/athenz/AthenzTokenClient.java
  • athenz/src/main/java/com/linecorp/armeria/client/athenz/RoleTokenClient.java
  • athenz/src/main/java/com/linecorp/armeria/client/athenz/TokenClientSetters.java
  • oauth2/src/main/java/com/linecorp/armeria/client/auth/oauth2/OAuth2AuthorizationGrantBuilder.java
✅ Files skipped from review due to trivial changes (1)
  • athenz/src/main/java/com/linecorp/armeria/client/athenz/AthenzTokenClient.java
🚧 Files skipped from review as they are similar to previous changes (1)
  • athenz/src/main/java/com/linecorp/armeria/client/athenz/TokenClientSetters.java

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzPublicKeyProvider.java (1)

81-83: ⚠️ Potential issue | 🟠 Major

Restore thread interrupt status when handling InterruptedException.

Lines 81 and 86 catch InterruptedException and swallow it into IllegalStateException without restoring the interrupt flag via Thread.currentThread().interrupt(). This breaks cooperative cancellation on caller threads.

Proposed fix
     void init() {
         try {
             ztsKeyLoader.load().get(30, TimeUnit.SECONDS);
-        } catch (InterruptedException | ExecutionException | TimeoutException e) {
+        } catch (InterruptedException e) {
+            Thread.currentThread().interrupt();
+            throw new IllegalStateException("Interrupted while loading ZTS keys", e);
+        } catch (ExecutionException | TimeoutException e) {
             throw new IllegalStateException("Failed to load ZTS keys in 30 seconds", e);
         }
         try {
             zmsKeyLoader.load().get(30, TimeUnit.SECONDS);
-        } catch (InterruptedException | ExecutionException | TimeoutException e) {
+        } catch (InterruptedException e) {
+            Thread.currentThread().interrupt();
+            throw new IllegalStateException("Interrupted while loading ZMS keys", e);
+        } catch (ExecutionException | TimeoutException e) {
             throw new IllegalStateException("Failed to load ZMS keys in 30 seconds", e);
         }
     }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzPublicKeyProvider.java`
around lines 81 - 83, The catch block in AthenzPublicKeyProvider that currently
catches InterruptedException | ExecutionException | TimeoutException and throws
new IllegalStateException("Failed to load ZTS keys in 30 seconds", e) swallows
the interrupt; restore the thread interrupt status before rethrowing. Modify the
error handling in the method that loads ZTS keys (the catch handling
InterruptedException | ExecutionException | TimeoutException) so that either you
split out a dedicated catch(InterruptedException ie) {
Thread.currentThread().interrupt(); throw new IllegalStateException("Failed to
load ZTS keys in 30 seconds", ie); } before the other catches, or keep the
multi-catch but check (e instanceof InterruptedException) and call
Thread.currentThread().interrupt() prior to throwing the IllegalStateException.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In
`@athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzPublicKeyProvider.java`:
- Around line 81-83: The catch block in AthenzPublicKeyProvider that currently
catches InterruptedException | ExecutionException | TimeoutException and throws
new IllegalStateException("Failed to load ZTS keys in 30 seconds", e) swallows
the interrupt; restore the thread interrupt status before rethrowing. Modify the
error handling in the method that loads ZTS keys (the catch handling
InterruptedException | ExecutionException | TimeoutException) so that either you
split out a dedicated catch(InterruptedException ie) {
Thread.currentThread().interrupt(); throw new IllegalStateException("Failed to
load ZTS keys in 30 seconds", ie); } before the other catches, or keep the
multi-catch but check (e instanceof InterruptedException) and call
Thread.currentThread().interrupt() prior to throwing the IllegalStateException.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 37a64615-e855-493c-8611-65725e2648bd

📥 Commits

Reviewing files that changed from the base of the PR and between 531a6bc and fec70ba.

📒 Files selected for processing (1)
  • athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzPublicKeyProvider.java

@ikhoon
ikhoon marked this pull request as ready for review April 6, 2026 05:00

@minwoox minwoox left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 👍 👍

throw new IllegalStateException("Failed to load ZTS keys in 30 seconds", e);
}
try {
zmsKeyLoader.load().get(30, TimeUnit.SECONDS);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to call load() together before calling get()?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are already called at the end of the constructor.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was imagining something like:

var future1 = ztsKeyLoader.load();
var future2 = zmsKeyLoader.load();
future1.get();
future2.get();

@ikhoon ikhoon Apr 6, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The loaders will work as you imagined because ztsKeyLoader.load() and zmsKeyLoader.load() return cached futures that were already triggered in the constructor.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah thanks! I missed it. 😓

@jrhee17 jrhee17 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 👍

@ikhoon
ikhoon merged commit 9a51433 into line:main Apr 7, 2026
13 of 17 checks passed
@ikhoon
ikhoon deleted the athenz-token-client branch April 7, 2026 01:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Expose Athenz TokenClient as public API

3 participants