Add AthenzResourceProvider for dynamic athenz resource resolution
#6541
Conversation
Introduce AthenzResourceProvider interface to enable dynamic resource
resolution in AthenzService, replacing the static resource configuration.
Changes:
- Add AthenzResourceProvider interface for dynamic resource provisioning
- Implement PathAthenzResourceProvider to extract resource from request path
- Implement HeaderAthenzResourceProvider to extract resource from HTTP headers
- Implement JsonBodyFieldAthenzResourceProvider to extract resource from JSON request body
- Modify AthenzService to support resourceProvider() in builder pattern
- Update AthenzServiceBuilder to accept AthenzResourceProvider
- Add comprehensive integration tests for all resource provider implementations
- Add unit tests for resource provider interface and implementations
- Update AthenzDocker test infrastructure with additional policies
This enhancement allows users to configure Athenz authorization based on
dynamic request attributes instead of static resource strings, enabling
more flexible access control patterns.
Breaking Changes:
- None (backward compatible - static resource() method still supported)
Example usage:
```java
AthenzService.builder(ztsBaseClient)
.resourceProvider(new PathAthenzResourceProvider())
.action("obtain")
.newDecorator();
```
# Conflicts: # athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzService.java # athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzServiceBuilder.java
…ality explosion
Add mandatory resourceTagValue parameter to AthenzServiceBuilder when using dynamic resource providers to ensure stable metric tags and prevent metric cardinality issues.
Changes:
- Add resourceTagValue field to AthenzServiceBuilder and AthenzService
- Update resourceProvider() method to require resourceTagValue parameter
- Update resource() method to automatically set resourceTagValue to the resource name
- Modify metric tag configuration to use resourceTagValue instead of dynamic resource values
- Add validation to ensure resourceTagValue is not empty
- Update serve() method to handle empty resource resolution with proper error response
Rationale:
When using dynamic resource providers (e.g., PathAthenzResourceProvider), the resource
value can vary per request, potentially creating unlimited unique metric combinations.
The resourceTagValue parameter provides a static identifier (e.g., "admin", "dynamic")
for metric tagging, ensuring stable cardinality while preserving metric usefulness.
Example:
- Static resource: resource("users") → resourceTagValue = "users"
- Dynamic resource: resourceProvider(new PathAthenzResourceProvider(), "admin")
→ resourceTagValue = "admin" (stable tag regardless of actual path)
JungJaeLee-JJ
requested review from
ikhoon,
jrhee17,
minwoox and
trustin
as code owners
|
|
WalkthroughReplaces static Athenz resource configuration with an asynchronous Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AthenzService
participant AthenzResourceProvider
participant AccessControl
participant WrappedService
Client->>AthenzService: HTTP Request
AthenzService->>AthenzResourceProvider: provide(ctx, req)
alt Resource resolved (non-empty)
AthenzResourceProvider-->>AthenzService: CompletableFuture<String> (resource)
AthenzService->>AccessControl: check(action, resource, token)
alt Access allowed
AccessControl-->>AthenzService: allow
AthenzService->>WrappedService: delegate(request)
WrappedService-->>Client: 200 OK
else Access denied
AccessControl-->>AthenzService: deny
AthenzService-->>Client: 401 Unauthorized
end
else Resource missing/empty
AthenzResourceProvider-->>AthenzService: AthenzResourceNotFoundException
AthenzService-->>Client: 403 Forbidden
else Provider exception
AthenzResourceProvider-->>AthenzService: Exception
AthenzService-->>Client: 500 Internal Server Error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 (1)
athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzServiceBuilder.java (1)
106-110: Bug:tokenType(TokenType...)method doesn't store the value.This method validates the input but never assigns to
this.tokenTypes, so the varargs overload is non-functional. TheIterableoverload at lines 116-121 works correctly.public AthenzServiceBuilder tokenType(TokenType... tokenTypes) { requireNonNull(tokenTypes, "tokenTypes"); checkArgument(tokenTypes.length > 0, "tokenTypes must not be empty"); + this.tokenTypes = ImmutableList.copyOf(tokenTypes); return this; }
🧹 Nitpick comments (7)
athenz/src/test/java/com/linecorp/armeria/server/athenz/resource/AthenzResourceProviderTest.java (2)
31-31: Typo in method names: "Exits" should be "Exists".Multiple test method names contain "Exits" instead of "Exists":
shouldProvideHeaderStringIfExits→shouldProvideHeaderStringIfExistsshouldProvideEmptyStringIfHeaderNotExits→shouldProvideEmptyStringIfHeaderNotExistsshouldProvideJsonFieldStringIfExits→shouldProvideJsonFieldStringIfExistsshouldProvideEmptyStringIfJsonFieldNotExits→shouldProvideEmptyStringIfJsonFieldNotExistsAlso applies to: 45-45, 59-59, 75-75
74-87: Misleading test name and description.The test
shouldProvideEmptyStringIfJsonFieldNotExitsactually tests invalid JSON parsing ({invalid json}), not a missing field scenario. Consider either:
- Renaming to
shouldProvideEmptyStringIfJsonIsInvalid, or- Adding a separate test for the actual missing field case with valid JSON like
{"otherField": "value"}@Test -public void shouldProvideEmptyStringIfJsonFieldNotExits() { +public void shouldProvideEmptyStringIfJsonIsInvalid() { // given final ObjectMapper mapper = new ObjectMapper(); final JsonBodyFieldAthenzResourceProvider provider = new JsonBodyFieldAthenzResourceProvider(mapper, "resourceId"); final HttpRequest req = HttpRequest.of(RequestHeaders.of(HttpMethod.POST, "/", "Content-Type", "application/json"), HttpData.ofUtf8("{invalid json}")); final ServiceRequestContext ctx = ServiceRequestContext.of(req); // when final CompletableFuture<String> result = provider.provide(ctx, req); // then assertThat(result.join()).isEmpty(); } + +@Test +public void shouldProvideEmptyStringIfJsonFieldNotExists() { + // given + final ObjectMapper mapper = new ObjectMapper(); + final JsonBodyFieldAthenzResourceProvider provider = new JsonBodyFieldAthenzResourceProvider(mapper, "resourceId"); + final HttpRequest req = HttpRequest.of(RequestHeaders.of(HttpMethod.POST, "/", "Content-Type", "application/json"), HttpData.ofUtf8("{\"otherField\":\"value\"}")); + final ServiceRequestContext ctx = ServiceRequestContext.of(req); + + // when + final CompletableFuture<String> result = provider.provide(ctx, req); + + // then + assertThat(result.join()).isEmpty(); +}athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/AthenzResourceProvider.java (1)
26-31: Consider adding Javadoc for the public interface and method.The interface lacks documentation. Adding Javadoc would improve API usability and align with the other provider implementations that have comprehensive documentation.
+/** + * Provides an Athenz resource string dynamically based on the request context. + * + * <p>Implementations extract the resource identifier from various sources such as + * the request path, headers, or body to be used for Athenz authorization checks. + */ @UnstableApi @FunctionalInterface public interface AthenzResourceProvider { + /** + * Resolves the Athenz resource string for the given request. + * + * @param ctx the service request context + * @param req the HTTP request + * @return a {@link CompletableFuture} containing the resolved resource string, + * or an empty string if resolution fails + */ CompletableFuture<String> provide(ServiceRequestContext ctx, HttpRequest req); }athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/HeaderAthenzResourceProvider.java (1)
50-52: Add null/empty validation forheaderNameparameter.The constructor should validate the input to fail fast with a clear error message, consistent with the validation pattern used in
AthenzServiceBuilder.+import static com.google.common.base.Preconditions.checkArgument; +import static java.util.Objects.requireNonNull; + public HeaderAthenzResourceProvider(String headerName) { + requireNonNull(headerName, "headerName"); + checkArgument(!headerName.isEmpty(), "headerName must not be empty"); this.headerName = headerName; }athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/JsonBodyFieldAthenzResourceProvider.java (2)
66-69: Add null/empty validation for constructor parameters.The constructor should validate inputs to fail fast with clear error messages, consistent with the validation pattern used elsewhere in this PR.
+import static com.google.common.base.Preconditions.checkArgument; +import static java.util.Objects.requireNonNull; + public JsonBodyFieldAthenzResourceProvider(ObjectMapper objectMapper, String jsonFieldName) { + requireNonNull(objectMapper, "objectMapper"); + requireNonNull(jsonFieldName, "jsonFieldName"); + checkArgument(!jsonFieldName.isEmpty(), "jsonFieldName must not be empty"); this.objectMapper = objectMapper; this.jsonFieldName = jsonFieldName; }
85-86: Consider logging the exception for debugging purposes.Silently swallowing exceptions with
catch (Exception ignored)can make debugging difficult in production. Consider logging at DEBUG or TRACE level to aid troubleshooting while still returning an empty string.+import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class JsonBodyFieldAthenzResourceProvider implements AthenzResourceProvider { + + private static final Logger logger = LoggerFactory.getLogger(JsonBodyFieldAthenzResourceProvider.class); + ... - } catch (Exception ignored) { + } catch (Exception e) { + logger.debug("Failed to extract JSON field '{}' from request body", jsonFieldName, e); return ""; }athenz/src/test/java/com/linecorp/armeria/server/athenz/AthenzResourceProviderIntegrationTest.java (1)
153-172: Consider adding a test for missing header scenario.The test validates an invalid header value, but it would be valuable to also test the case where the
X-Athenz-Resourceheader is completely absent. Based onHeaderAthenzResourceProvider, this would return an empty string and should result inINTERNAL_SERVER_ERROR.@ParameterizedTest @EnumSource(TokenType.class) void shouldReturnInternalServerErrorWhenHeaderIsMissing(TokenType tokenType) { try (ZtsBaseClient ztsBaseClient = athenzExtension.newZtsBaseClient(TEST_SERVICE)) { final BlockingWebClient client = WebClient.builder(server.httpUri()) .decorator(AthenzClient.newDecorator(ztsBaseClient, TEST_DOMAIN_NAME, ADMIN_ROLE, tokenType)) .build() .blocking(); // Request without X-Athenz-Resource header final AggregatedHttpResponse response = client.get("/admin/header/test"); assertThat(response.status()).isEqualTo(HttpStatus.INTERNAL_SERVER_ERROR); } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzService.java(4 hunks)athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzServiceBuilder.java(4 hunks)athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/AthenzResourceProvider.java(1 hunks)athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/HeaderAthenzResourceProvider.java(1 hunks)athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/JsonBodyFieldAthenzResourceProvider.java(1 hunks)athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/PathAthenzResourceProvider.java(1 hunks)athenz/src/test/java/com/linecorp/armeria/server/athenz/AthenzDocker.java(2 hunks)athenz/src/test/java/com/linecorp/armeria/server/athenz/AthenzResourceProviderIntegrationTest.java(1 hunks)athenz/src/test/java/com/linecorp/armeria/server/athenz/resource/AthenzResourceProviderTest.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
⚙️ CodeRabbit configuration file
**/*.java: - The primary coding conventions and style guide for this project are defined insite/src/pages/community/developer-guide.mdx. Please strictly adhere to this file as the ultimate source of truth for all style and convention-related feedback.2. Specific check for
@UnstableApi
- Review all newly added public classes and methods to ensure they have the
@UnstableApiannotation.- However, this annotation is NOT required under the following conditions:
- If the class or method is located in a package containing
.internalor.testing.- If the class or method is located in a test source set.
- If a public method is part of a class that is already annotated with
@UnstableApi.
Files:
athenz/src/test/java/com/linecorp/armeria/server/athenz/AthenzDocker.javaathenz/src/main/java/com/linecorp/armeria/server/athenz/resource/JsonBodyFieldAthenzResourceProvider.javaathenz/src/main/java/com/linecorp/armeria/server/athenz/resource/AthenzResourceProvider.javaathenz/src/main/java/com/linecorp/armeria/server/athenz/resource/PathAthenzResourceProvider.javaathenz/src/test/java/com/linecorp/armeria/server/athenz/resource/AthenzResourceProviderTest.javaathenz/src/test/java/com/linecorp/armeria/server/athenz/AthenzResourceProviderIntegrationTest.javaathenz/src/main/java/com/linecorp/armeria/server/athenz/resource/HeaderAthenzResourceProvider.javaathenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzService.javaathenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzServiceBuilder.java
🧬 Code graph analysis (3)
athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/AthenzResourceProvider.java (2)
athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzService.java (1)
UnstableApi(106-220)athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/PathAthenzResourceProvider.java (1)
UnstableApi(43-50)
athenz/src/test/java/com/linecorp/armeria/server/athenz/AthenzResourceProviderIntegrationTest.java (3)
athenz/src/test/java/com/linecorp/armeria/server/athenz/AthenzDocker.java (1)
AthenzDocker(55-342)athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/HeaderAthenzResourceProvider.java (1)
HeaderAthenzResourceProvider(40-59)athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/JsonBodyFieldAthenzResourceProvider.java (1)
JsonBodyFieldAthenzResourceProvider(54-89)
athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzService.java (1)
core/src/main/java/com/linecorp/armeria/common/util/Exceptions.java (1)
Exceptions(55-426)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (10)
athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/PathAthenzResourceProvider.java (1)
43-50: LGTM!Clean implementation with proper
@UnstableApiannotation and comprehensive Javadoc. The class is appropriately markedfinalfor a simple, non-extensible provider.athenz/src/test/java/com/linecorp/armeria/server/athenz/AthenzDocker.java (1)
75-79: LGTM!The new policy constants and their creation in
defaultScaffold()correctly set up test fixtures for path, header, and JSON body-based resource providers. Minor: there's an extra blank line at line 171.Also applies to: 166-171
athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzServiceBuilder.java (2)
66-87: LGTM on the resource provider implementation.The
resource()andresourceProvider()methods are well-implemented with proper validation. The design correctly handles both static resources (viaresource()) and dynamic resolution (viaresourceProvider()), withresourceTagValueused for stable metric tagging to avoid cardinality explosion.
126-138: Approve the updatednewDecorator()implementation.The state checks for
athenzResourceProviderandresourceTagValueare appropriate, and the constructor call correctly passes all required parameters.athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzService.java (3)
48-105: Well-documented API with clear examples.The Javadoc provides clear examples for both static and dynamic resource configurations, which helps users understand the intended usage patterns. The
@seereferences to the concrete provider implementations are helpful.
129-153: Good backward-compatible constructor design.The dual-constructor approach maintains API compatibility while enabling the new provider-based resolution. The static resource constructor at line 142-153 correctly wraps the resource string in a
CompletableFuture, and usingresourceTagValuefor metrics prevents cardinality explosion from dynamic resources.
155-168: LGTM!The metric initialization correctly uses
resourceTagValuefor stable tagging, preventing metric cardinality issues when dynamic resources are used.athenz/src/test/java/com/linecorp/armeria/server/athenz/AthenzResourceProviderIntegrationTest.java (3)
55-94: Well-structured test setup with proper resource management.The
@Orderannotations ensureathenzExtensioninitializes before the server. TheServerListenerproperly closes theztsBaseClienton server shutdown, preventing resource leaks.
96-128: LGTM!The path resource tests properly cover both allowed and denied scenarios with appropriate role configurations. The
try-with-resourcespattern ensures proper cleanup ofZtsBaseClient.
174-219: Comprehensive JSON body resource tests.The tests properly verify JSON body field extraction for both valid and invalid resource values. The content type is correctly set to
MediaType.JSON.Consider adding edge case tests for malformed JSON or missing
resourcefield if thorough coverage is desired, asJsonBodyFieldAthenzResourceProviderreturns empty string in these cases.
athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzService.java
Outdated
Show resolved
Hide resolved
.../src/main/java/com/linecorp/armeria/server/athenz/resource/HeaderAthenzResourceProvider.java
Outdated
Show resolved
Hide resolved
...in/java/com/linecorp/armeria/server/athenz/resource/JsonBodyFieldAthenzResourceProvider.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/JsonBodyFieldAthenzResourceProvider.java (2)
87-89: Consider logging or documenting the silent exception handling.Swallowing all exceptions silently can make debugging difficult when JSON parsing fails unexpectedly. While returning an empty string is acceptable behavior (leading to
INTERNAL_SERVER_ERROR), consider adding a debug-level log or at minimum documenting this behavior in the Javadoc.+import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class JsonBodyFieldAthenzResourceProvider implements AthenzResourceProvider { + + private static final Logger logger = LoggerFactory.getLogger(JsonBodyFieldAthenzResourceProvider.class); + ... } catch (Exception ignored) { + logger.debug("Failed to parse JSON body for field '{}': {}", jsonFieldName, ignored.getMessage()); return ""; }
55-56: Consider making the classfinalfor consistency.
PathAthenzResourceProvideris declaredfinalwhile this class andHeaderAthenzResourceProviderare not. For consistency across resource providers and to prevent unintended subclassing, consider making this classfinal.@UnstableApi -public class JsonBodyFieldAthenzResourceProvider implements AthenzResourceProvider { +public final class JsonBodyFieldAthenzResourceProvider implements AthenzResourceProvider {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzService.java(4 hunks)athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/HeaderAthenzResourceProvider.java(1 hunks)athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/JsonBodyFieldAthenzResourceProvider.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/HeaderAthenzResourceProvider.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
⚙️ CodeRabbit configuration file
**/*.java: - The primary coding conventions and style guide for this project are defined insite/src/pages/community/developer-guide.mdx. Please strictly adhere to this file as the ultimate source of truth for all style and convention-related feedback.2. Specific check for
@UnstableApi
- Review all newly added public classes and methods to ensure they have the
@UnstableApiannotation.- However, this annotation is NOT required under the following conditions:
- If the class or method is located in a package containing
.internalor.testing.- If the class or method is located in a test source set.
- If a public method is part of a class that is already annotated with
@UnstableApi.
Files:
athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzService.javaathenz/src/main/java/com/linecorp/armeria/server/athenz/resource/JsonBodyFieldAthenzResourceProvider.java
🧬 Code graph analysis (1)
athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/JsonBodyFieldAthenzResourceProvider.java (4)
athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzService.java (1)
UnstableApi(106-222)athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/HeaderAthenzResourceProvider.java (1)
UnstableApi(41-61)athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/PathAthenzResourceProvider.java (1)
UnstableApi(43-50)athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzServiceBuilder.java (1)
UnstableApi(41-139)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (3)
athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzService.java (3)
142-153: LGTM on backward-compatible constructor.This package-private constructor maintains backward compatibility by wrapping the static
athenzResourcestring in aCompletableFutureand using it as theresourceTagValue. Clean approach for supporting legacy callers.
190-206: Appropriate use ofblockingTaskExecutorfor ZPE access checks.The
authZpeClient.allowAccess()call is correctly offloaded toctx.blockingTaskExecutor()since ZPE policy evaluation may involve blocking I/O. The error handling within the async block properly propagates exceptions viaExceptions.throwUnsafely(e).
160-167: LGTM on metric tagging withresourceTagValue.Using
resourceTagValueinstead of the dynamically resolved resource for metric tags is the correct approach to avoid metric cardinality explosion. This aligns with the PR objectives.
athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzService.java
Outdated
Show resolved
Hide resolved
...in/java/com/linecorp/armeria/server/athenz/resource/JsonBodyFieldAthenzResourceProvider.java
Outdated
Show resolved
Hide resolved
...in/java/com/linecorp/armeria/server/athenz/resource/JsonBodyFieldAthenzResourceProvider.java
Outdated
Show resolved
Hide resolved
- Improve AthenzService validation
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzService.java (1)
181-188: Consider logging resource resolution failures for debugging.When
athenzResourceProvider.provide()fails, the exception is silently converted to an empty string (line 182), and the user receives a generic "Failed to resolve resource" message. While the error response and timer recording are correct, the root cause is not logged, making it difficult to diagnose why resource resolution failed (e.g., malformed JSON, missing header, network timeout).Consider logging the exception:
- final CompletableFuture<HttpResponse> future = athenzResourceProvider.provide(ctx, req) - .exceptionally(cause -> "") + final CompletableFuture<HttpResponse> future = athenzResourceProvider.provide(ctx, req) + .exceptionally(cause -> { + logger.warn("Failed to resolve Athenz resource", cause); + return ""; + }) .thenCompose(athenzResource -> {Note: You'll need to add a logger field if not already present.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzService.java(4 hunks)athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/HeaderAthenzResourceProvider.java(1 hunks)athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/JsonBodyFieldAthenzResourceProvider.java(1 hunks)athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/PathAthenzResourceProvider.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/JsonBodyFieldAthenzResourceProvider.java
- athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/HeaderAthenzResourceProvider.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
⚙️ CodeRabbit configuration file
**/*.java: - The primary coding conventions and style guide for this project are defined insite/src/pages/community/developer-guide.mdx. Please strictly adhere to this file as the ultimate source of truth for all style and convention-related feedback.2. Specific check for
@UnstableApi
- Review all newly added public classes and methods to ensure they have the
@UnstableApiannotation.- However, this annotation is NOT required under the following conditions:
- If the class or method is located in a package containing
.internalor.testing.- If the class or method is located in a test source set.
- If a public method is part of a class that is already annotated with
@UnstableApi.
Files:
athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/PathAthenzResourceProvider.javaathenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzService.java
🧬 Code graph analysis (1)
athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/PathAthenzResourceProvider.java (4)
athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzService.java (1)
UnstableApi(106-222)athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/HeaderAthenzResourceProvider.java (1)
UnstableApi(45-67)athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/JsonBodyFieldAthenzResourceProvider.java (1)
UnstableApi(58-97)athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzServiceBuilder.java (1)
UnstableApi(41-139)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (3)
athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/PathAthenzResourceProvider.java (1)
26-49: Path-based resource provider implementation and API stability annotation look goodJavadoc, naming, and behavior are consistent: the provider returns the request path via a cheap
completedFuture, matching how other AthenzResourceProvider implementations work. The class is correctly annotated with@UnstableApi, so the new public surface complies with the API stability guidelines. No changes needed.athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzService.java (2)
51-105: Excellent documentation with practical examples.The comprehensive Javadoc clearly distinguishes static and dynamic resource configurations with practical code examples. This will help users understand when and how to use each approach.
160-167: Good use of resourceTagValue for metric stability.Using
resourceTagValueinstead of the dynamic resource value prevents metric cardinality explosion, which is important when resources vary per request.
jrhee17
left a comment
jrhee17
left a comment
There was a problem hiding this comment.
Left some questions, but overall looks good
| final JsonNode node = root.get(jsonFieldName); | ||
| return node != null ? node.asText("") : ""; | ||
| } catch (Exception ignored) { | ||
| return ""; |
There was a problem hiding this comment.
Optional) Rather than the overall pattern of swallowing the exception, it may be worth propagating the exception so that it could possibly be logged with a debug level log. Otherwise, it may be difficult to debug why a certain ResourceProvider is failing
There was a problem hiding this comment.
I’ve added debug logging for both JsonBodyFieldAthenzResourceProvider and HeaderAthenzResourceProvider in the following commit: 5306310
| @FunctionalInterface | ||
| public interface AthenzResourceProvider { | ||
|
|
||
| CompletableFuture<String> provide(ServiceRequestContext ctx, HttpRequest req); |
There was a problem hiding this comment.
Minor Question) It seems like the overall pattern is providing resources. Is there no need to also determine the action dynamically?
There was a problem hiding this comment.
From my experience using Athenz, actions are often mapped to HTTP methods (GET, PUT, POST, DELETE, etc.), while resources correspond to paths. In many cases, we simply allow all actions using * and perform authorization primarily based on resources. For that reason, this PR adds only the resource for now.
If we later find a need to handle actions more granularly, we can introduce an action provider at that time.
| * @param objectMapper the {@link ObjectMapper} used to parse the JSON request body | ||
| * @param jsonFieldName the name of the JSON field to extract the resource from | ||
| */ | ||
| public JsonBodyFieldAthenzResourceProvider(ObjectMapper objectMapper, String jsonFieldName) { |
There was a problem hiding this comment.
nit; it may be worth using json path instead of assuming 1-level.
There was a problem hiding this comment.
The assumption about using a 1-level JSON structure comes from how Athenz typically maps resources to paths.
The JsonBodyFieldAthenzResourceProvider was introduced specifically to support platforms that already operate non-RESTful APIs.
From a general authentication/authorization perspective, tenant-specific resource values are usually provided at the first level of the JSON body, which covers the majority of use cases.
For example, when identifying a specific service with a field like service_id, the payload commonly looks like:
{
"service_id": "service_id",
...
}
We don’t often see cases where such identifiers appear at level 2 or deeper in the JSON structure, so the current assumption seems sufficient for now.
There was a problem hiding this comment.
In most cases, only first-level fields are expected to be accessed. However, since Armeria is a framework, we need to account for a variety of scenarios. It would be nice to support both a simple case—accepting a String to access a first-level field—and a more general implementation that takes Jackson's JsonPointer.
I suggested JsonPointer since Jackson is part of our public API.
interface AthenzResourceProvider {
static AthenzResourceProvider ofJsonField(String jsonFieldName) {
...
}
static AthenzResourceProvider ofJsonField(JsonPointer jsonPointer) {
...
}
}There was a problem hiding this comment.
I've implemented both simple field name and JsonPointer support as you suggested.
Changes in 1aad9a1:
- Made implementation constructors
protectedto encapsulate instantiation logic - Added factory methods in
AthenzResourceProviderinterface:ofPath()- for path-based resource extractionofHeader(String headerName)- for header-based extractionofJsonField(String jsonFieldName)- for simple first-level JSON fieldsofJsonField(JsonPointer jsonPointer)- for nested JSON fields using Jackson's JsonPointer
Now users can choose between simple and advanced usage:
// Simple first-level field
AthenzResourceProvider.ofJsonField("resourceId")
// Nested field with JsonPointer
AthenzResourceProvider.ofJsonField(JsonPointer.compile("/user/id"))|
|
||
| @Override | ||
| public CompletableFuture<String> provide(ServiceRequestContext ctx, HttpRequest req) { | ||
| return CompletableFuture.completedFuture(req.path()); |
There was a problem hiding this comment.
Note) I believe req.path() will include query params, etc.
There was a problem hiding this comment.
Yes, that’s correct. In cases where tenant-specific authorization needs to consider query parameters, the provider also includes the query params when returning the resource value. This ensures that scenarios restricted by query parameters are also properly supported.
There was a problem hiding this comment.
- Should we add an option to determine whether to include query parameters for Athenz resources?
- Armeria typically hides implementation details and provides factory methods through the interface instead. For example:
public interface AthenzResourceProvider {
static AthenzResourceProvider ofPath() {
return PathAthenzResourceProvider.INSTANCE;
}
static AthenzResourceProvider ofPath(boolean includeQuery) {
if (includeQuery) {
return PathAthenzResourceProvider.QUERY_INSTANCE;
} else {
return PathAthenzResourceProvider.INSTANCE;
}
}
}There was a problem hiding this comment.
I've implemented query parameter support and exposed factory methods through the interface as you proposed.
Changes in 1aad9a1
|
|
||
| @Override | ||
| public CompletableFuture<String> provide(ServiceRequestContext ctx, HttpRequest req) { | ||
| return req.aggregate().thenApply(this::extractFieldSafely); |
There was a problem hiding this comment.
I missed this during the initial review, but it probably makes more sense to aggregate only if the request has content-type: json
There was a problem hiding this comment.
I’ve updated the implementation in 5306310
so that aggregation is performed only when the request’s Content-Type is JSON.
…Provider when exception occurred or resource not found
| .exceptionally(cause -> "") | ||
| .thenCompose(athenzResource -> { | ||
| if (athenzResource == null || athenzResource.isEmpty()) { | ||
| assert deniedTimer != null; | ||
| deniedTimer.record(System.nanoTime() - startNanos, TimeUnit.NANOSECONDS); | ||
| return CompletableFuture.completedFuture(HttpResponse.of(HttpStatus.INTERNAL_SERVER_ERROR, MediaType.PLAIN_TEXT, "Failed to resolve resource")); |
There was a problem hiding this comment.
We should distinguish between exceptions and a null resources. I suggest returning INTERNAL_SERVER_ERROR for exceptions and FORBIDDEN when a resource cannot be resolved.
There was a problem hiding this comment.
I've updated the implementation to distinguish between exceptions and null resources.
Changes in 1aad9a1:
- Return
INTERNAL_SERVER_ERROR(500) when an exception occurs while resolving the resource - Return
FORBIDDEN(403) when the resource cannot be resolved (null or empty) - Added test cases in
AthenzResourceProviderIntegrationTest:- Exception handling during resource resolution →
INTERNAL_SERVER_ERROR - Null resource handling →
FORBIDDEN - Empty resource handling →
FORBIDDEN
- Exception handling during resource resolution →
This provides clearer error responses for different failure scenarios.
athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzService.java
Outdated
Show resolved
Hide resolved
| "resource", athenzResource, | ||
| "action", athenzAction)); | ||
| meterIdPrefix.tags("result", "denied", | ||
| "resource", resourceTagValue, |
There was a problem hiding this comment.
How about just setting * to the resource tag if athenzResourceProvider is specified?
There was a problem hiding this comment.
I've simplified the API by making * the default resource tag value when using AthenzResourceProvider.
Changes in 1aad9a1:
- Made
*the default resource tag value forresourceProvider(AthenzResourceProvider) - Kept
resourceProvider(AthenzResourceProvider, String)overload for custom resource tag value use cases
Usage examples:
// Default: resource tag = "*"
AthenzService.builder(ztsBaseClient)
.resourceProvider(AthenzResourceProvider.ofPath())
.action("read")
.newDecorator();
// Custom resource tag
AthenzService.builder(ztsBaseClient)
.resourceProvider(AthenzResourceProvider.ofPath(), "custom-tag")
.action("read")
.newDecorator();| * | ||
| * <p><strong>Mandatory:</strong> Either this or {@link #resource(String)} must be set before calling {@link #newDecorator()}.</p> | ||
| */ | ||
| public AthenzServiceBuilder resourceProvider(AthenzResourceProvider athenzResourceProvider, String resourceTagValue) { |
There was a problem hiding this comment.
Should we make resource() and resourceProvider() mutually exclusive?
For example:
checkState(athenzResource == null,
"resource() and resourceProvider() are mutually exclusive");There was a problem hiding this comment.
I've added validation to make resource() and resourceProvider() mutually exclusive.
| /** | ||
| * Sets the {@link AthenzResourceProvider} used to resolve the Athenz resource dynamically for each request. | ||
| * | ||
| * <p><strong>Mandatory:</strong> Either this or {@link #resource(String)} must be set before calling {@link #newDecorator()}.</p> |
There was a problem hiding this comment.
style) This line exceeds our max line length.
armeria/settings/checkstyle/checkstyle.xml
Lines 26 to 27 in 7148df0
There was a problem hiding this comment.
I've applied the LY OSS code style to match Armeria's formatting convention
|
|
||
| @Override | ||
| public CompletableFuture<String> provide(ServiceRequestContext ctx, HttpRequest req) { | ||
| return CompletableFuture.completedFuture(req.path()); |
There was a problem hiding this comment.
- Should we add an option to determine whether to include query parameters for Athenz resources?
- Armeria typically hides implementation details and provides factory methods through the interface instead. For example:
public interface AthenzResourceProvider {
static AthenzResourceProvider ofPath() {
return PathAthenzResourceProvider.INSTANCE;
}
static AthenzResourceProvider ofPath(boolean includeQuery) {
if (includeQuery) {
return PathAthenzResourceProvider.QUERY_INSTANCE;
} else {
return PathAthenzResourceProvider.INSTANCE;
}
}
}| * @param objectMapper the {@link ObjectMapper} used to parse the JSON request body | ||
| * @param jsonFieldName the name of the JSON field to extract the resource from | ||
| */ | ||
| public JsonBodyFieldAthenzResourceProvider(ObjectMapper objectMapper, String jsonFieldName) { |
There was a problem hiding this comment.
In most cases, only first-level fields are expected to be accessed. However, since Armeria is a framework, we need to account for a variety of scenarios. It would be nice to support both a simple case—accepting a String to access a first-level field—and a more general implementation that takes Jackson's JsonPointer.
I suggested JsonPointer since Jackson is part of our public API.
interface AthenzResourceProvider {
static AthenzResourceProvider ofJsonField(String jsonFieldName) {
...
}
static AthenzResourceProvider ofJsonField(JsonPointer jsonPointer) {
...
}
}Add AthenzResourceProvider interface to enable dynamic Athenz resource resolution from HTTP requests.
Changes:
- Add AthenzResourceProvider interface for dynamic resource extraction
- Add factory methods: ofPath(), ofHeader(), ofJsonField()
- Implement PathAthenzResourceProvider for path-based extraction
- Implement HeaderAthenzResourceProvider for header-based extraction
- Implement JsonBodyFieldAthenzResourceProvider for JSON body extraction
- Update AthenzService to support resourceProvider() in builder
- Add error handling:
- 403 FORBIDDEN for null/empty resources
- 500 INTERNAL_SERVER_ERROR for resolution exceptions
- Add integration tests for all provider types
- Update Javadoc with dynamic resource examples
The builder now supports both static and dynamic resources:
- Static: .resource("admin")
- Dynamic: .resourceProvider(AthenzResourceProvider.ofPath())
There was a problem hiding this comment.
Actionable comments posted: 1
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/server/athenz/AthenzServiceBuilder.java (1)
116-120:tokenType(TokenType...)currently ignores its argumentUnrelated to this PR but visible here: the varargs
tokenType(TokenType... tokenTypes)method only validates thattokenTypes.length > 0and then returnsthiswithout updating thetokenTypesfield, whereas theIterable<TokenType>overload correctly setsthis.tokenTypes = ImmutableList.copyOf(tokenTypes).As a result, calling the varargs overload has no effect. It would be better to either:
- Assign the field in this overload as well, or
- Delegate to the iterable overload for consistency.
🧹 Nitpick comments (3)
athenz/src/test/java/com/linecorp/armeria/server/athenz/resource/AthenzResourceProviderTest.java (1)
47-74: Clarify test names and JSON “not exists” scenarioTest names have a small typo (
Exits→Exists), e.g.shouldProvideHeaderStringIfExitsandshouldProvideEmptyStringIfHeaderNotExits. Also,shouldProvideEmptyStringIfJsonFieldNotExitscurrently exercises the “invalid JSON” path rather than a valid JSON body where the field is actually missing. Consider either:
- Renaming the method to reflect the invalid-JSON case, or
- Adding a separate test for “JSON field missing but body valid” to cover that branch explicitly.
Also applies to: 112-125
athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/HeaderAthenzResourceProvider.java (1)
32-48: Align Javadoc with logging behavior for empty header valuesThe Javadoc states that when the header is “not present or empty”, an empty string is returned and logged at debug level, but the implementation only logs when
value == null(header missing), not when the header is present with an empty value.Consider either:
- Logging for both
nulland empty values, or- Updating the Javadoc to clarify that only the “not present” case is logged.
Also applies to: 67-75
athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzService.java (1)
179-218: Avoid exposing provider exception messages in 500 responsesThe new error handling cleanly distinguishes:
- Provider exceptions → 500
- Unresolved/empty resource → 403
- AuthZ deny/missing token → 401
However, the 500 branch currently returns:
HttpResponse.of(HttpStatus.INTERNAL_SERVER_ERROR, MediaType.PLAIN_TEXT, "Exception occurred while resolving resource: " + cause.getMessage())This can leak internal exception details to clients (and whatever text the provider chose as its message). Consider instead:
- Logging the exception at debug/warn (optionally using
Exceptions.peel(cause)), and- Returning a generic body such as
"Failed to resolve resource"or similar, without includingcause.getMessage().This keeps external behavior stable (still 500) while tightening error information exposure.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzService.java(4 hunks)athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzServiceBuilder.java(4 hunks)athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/AthenzResourceProvider.java(1 hunks)athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/HeaderAthenzResourceProvider.java(1 hunks)athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/JsonBodyFieldAthenzResourceProvider.java(1 hunks)athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/PathAthenzResourceProvider.java(1 hunks)athenz/src/test/java/com/linecorp/armeria/server/athenz/AthenzResourceProviderIntegrationTest.java(1 hunks)athenz/src/test/java/com/linecorp/armeria/server/athenz/resource/AthenzResourceProviderTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/JsonBodyFieldAthenzResourceProvider.java
- athenz/src/test/java/com/linecorp/armeria/server/athenz/AthenzResourceProviderIntegrationTest.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
⚙️ CodeRabbit configuration file
**/*.java: - The primary coding conventions and style guide for this project are defined insite/src/pages/community/developer-guide.mdx. Please strictly adhere to this file as the ultimate source of truth for all style and convention-related feedback.2. Specific check for
@UnstableApi
- Review all newly added public classes and methods to ensure they have the
@UnstableApiannotation.- However, this annotation is NOT required under the following conditions:
- If the class or method is located in a package containing
.internalor.testing.- If the class or method is located in a test source set.
- If a public method is part of a class that is already annotated with
@UnstableApi.
Files:
athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/PathAthenzResourceProvider.javaathenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzService.javaathenz/src/main/java/com/linecorp/armeria/server/athenz/resource/AthenzResourceProvider.javaathenz/src/test/java/com/linecorp/armeria/server/athenz/resource/AthenzResourceProviderTest.javaathenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzServiceBuilder.javaathenz/src/main/java/com/linecorp/armeria/server/athenz/resource/HeaderAthenzResourceProvider.java
🧬 Code graph analysis (4)
athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/PathAthenzResourceProvider.java (4)
athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzService.java (1)
UnstableApi(108-232)athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzServiceBuilder.java (1)
UnstableApi(41-149)athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/HeaderAthenzResourceProvider.java (1)
UnstableApi(49-76)athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/JsonBodyFieldAthenzResourceProvider.java (1)
UnstableApi(77-130)
athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzService.java (3)
core/src/main/java/com/linecorp/armeria/common/metric/MoreMeters.java (1)
MoreMeters(42-203)scala/scala_2.13/src/main/scala/com/linecorp/armeria/client/scala/ScalaRestClient.scala (1)
unwrap(187-187)core/src/main/java/com/linecorp/armeria/common/util/Exceptions.java (1)
Exceptions(55-426)
athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/AthenzResourceProvider.java (5)
athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzService.java (1)
UnstableApi(108-232)athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzServiceBuilder.java (1)
UnstableApi(41-149)athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/HeaderAthenzResourceProvider.java (1)
UnstableApi(49-76)athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/JsonBodyFieldAthenzResourceProvider.java (1)
UnstableApi(77-130)athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/PathAthenzResourceProvider.java (1)
UnstableApi(55-72)
athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/HeaderAthenzResourceProvider.java (4)
athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzService.java (1)
UnstableApi(108-232)athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzServiceBuilder.java (1)
UnstableApi(41-149)athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/JsonBodyFieldAthenzResourceProvider.java (1)
UnstableApi(77-130)athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/PathAthenzResourceProvider.java (1)
UnstableApi(55-72)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (5)
athenz/src/test/java/com/linecorp/armeria/server/athenz/resource/AthenzResourceProviderTest.java (1)
19-45: Good coverage of built‑in providersThe tests exercise path (with and without query), header, and JSON field (name and JsonPointer) providers end‑to‑end using
ServiceRequestContextandHttpRequest, which should give solid confidence in the new factories’ behavior.Also applies to: 47-109
athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/PathAthenzResourceProvider.java (1)
55-71: Path-based provider implementation looks correctUsing
ctx.path()for the no‑query variant andreq.path()for the query‑inclusive variant matches Armeria’s path semantics and the documented examples; the static instances avoid per‑request allocations.athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/AthenzResourceProvider.java (1)
61-89: AthenzResourceProvider API and factory design look solidThe functional interface plus static factories (
ofPath,ofHeader,ofJsonField) provide a clean, discoverable API while keeping implementation classes internal to the package. Marking the interface as@UnstableApiis consistent with the new surface area.athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzService.java (1)
119-151: Constructor overloading and metric tag changes preserve behavior cleanlyWrapping the legacy string resource in a lambda-based
AthenzResourceProviderwhile introducing the new provider‑based constructor keeps existing call sites working and centralizes the resource resolution model. Using a separateresourceTagValuefor metrics is a good way to avoid high cardinality when resources are dynamic.Also applies to: 153-166
athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzServiceBuilder.java (1)
48-58: Builder wiring to provider-based AthenzService looks correctUsing
DEFAULT_RESOURCE_TAG_VALUE = "*"when only a provider is supplied, and passing bothathenzResourceProviderandresourceTagValueinto the newAthenzServiceconstructor ensures:
- Dynamic resources don’t explode metric cardinality by default.
- Static resources still end up with meaningful resource tags.
The overall transition from a static resource string to a provider + tag value is cleanly encapsulated in the builder.
Also applies to: 136-148
athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzServiceBuilder.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
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/server/athenz/AthenzServiceBuilder.java (1)
117-121:tokenType(TokenType...)currently ignores its argumentUnrelated to this PR’s main change but surfaced while reviewing: the varargs
tokenType(TokenType... tokenTypes)method validates the input but never assigns it tothis.tokenTypes, so calls likebuilder.tokenType(TokenType.ACCESS_TOKEN)have no effect (the default list is still used innewDecorator()).Suggest aligning it with the iterable overload:
public AthenzServiceBuilder tokenType(TokenType... tokenTypes) { requireNonNull(tokenTypes, "tokenTypes"); checkArgument(tokenTypes.length > 0, "tokenTypes must not be empty"); - return this; + this.tokenTypes = ImmutableList.copyOf(tokenTypes); + return this; }You can fix this here or in a small follow‑up PR, but it’s a real configuration bug.
🧹 Nitpick comments (2)
athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzServiceBuilder.java (2)
63-75: Consider delegatingresource(String)toresourceProvider(...)to avoid duplicationThe static-resource path is implemented correctly: null/empty checks, mutual exclusivity, and wrapping in a
CompletableFuturelook good. To keep the invariants in one place and reduce duplication, you could delegate to the 2‑arg overload:public AthenzServiceBuilder resource(String athenzResource) { requireNonNull(athenzResource, "athenzResource"); checkArgument(!athenzResource.isEmpty(), "athenzResource must not be empty"); - checkState(this.athenzResourceProvider == null, "resource() and resourceProvider() are mutually exclusive"); - this.athenzResourceProvider = (ctx, req) -> CompletableFuture.completedFuture(athenzResource); - this.resourceTagValue = athenzResource; - return this; + return resourceProvider((ctx, req) -> CompletableFuture.completedFuture(athenzResource), + athenzResource); }This keeps the mutual‑exclusivity and
resourceTagValuelogic centralized.
77-98:resourceProvideroverloads look good; small doc and naming nitsThe overload pair correctly enforces non‑null, non‑empty
resourceTagValueand mutual exclusivity withresource(), and the default"*"tag is a sensible choice for bounded metric cardinality. Two minor polish suggestions:
- In
requireNonNull(athenzResourceProvider, "resourceProvider");, consider using the actual parameter name"athenzResourceProvider"to match the coding style elsewhere.- For the single‑arg overload, you might clarify in Javadoc that the default
resourceTagValueis"*"and briefly mention thatresourceTagValueis used only for metric tagging, not authorization semantics.These are purely cosmetic; the behavior is fine as‑is. As per coding guidelines, this keeps API usage and metrics behavior clearer.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzServiceBuilder.java(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
⚙️ CodeRabbit configuration file
**/*.java: - The primary coding conventions and style guide for this project are defined insite/src/pages/community/developer-guide.mdx. Please strictly adhere to this file as the ultimate source of truth for all style and convention-related feedback.2. Specific check for
@UnstableApi
- Review all newly added public classes and methods to ensure they have the
@UnstableApiannotation.- However, this annotation is NOT required under the following conditions:
- If the class or method is located in a package containing
.internalor.testing.- If the class or method is located in a test source set.
- If a public method is part of a class that is already annotated with
@UnstableApi.
Files:
athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzServiceBuilder.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (1)
athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzServiceBuilder.java (1)
24-58: Resource provider fields and defaults are coherentThe introduction of
DEFAULT_RESOURCE_TAG_VALUE,athenzResourceProvider, andresourceTagValuefits the new dynamic-resource design; the@Nullableusage plus latercheckStatecalls innewDecorator()keep the builder’s state consistent. No changes needed here.
athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzServiceBuilder.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzServiceBuilder.java (1)
77-84: Document the default resource tag value in javadoc.The javadoc should mention that this convenience method uses a default
resourceTagValueof"*"for metric tagging. Users need to understand the implications of using the default versus providing an explicit tag value.Consider updating the javadoc:
/** * Sets the {@link AthenzResourceProvider} used to resolve the Athenz resource dynamically for each request. + * Uses the default resource tag value {@code "*"} for metrics. * * <p><strong>Mandatory:</strong> Either this or {@link #resource(String)} must be set before calling {@link #newDecorator()}.</p> + * + * @see #resourceProvider(AthenzResourceProvider, String) */
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzServiceBuilder.java(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
⚙️ CodeRabbit configuration file
**/*.java: - The primary coding conventions and style guide for this project are defined insite/src/pages/community/developer-guide.mdx. Please strictly adhere to this file as the ultimate source of truth for all style and convention-related feedback.2. Specific check for
@UnstableApi
- Review all newly added public classes and methods to ensure they have the
@UnstableApiannotation.- However, this annotation is NOT required under the following conditions:
- If the class or method is located in a package containing
.internalor.testing.- If the class or method is located in a test source set.
- If a public method is part of a class that is already annotated with
@UnstableApi.
Files:
athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzServiceBuilder.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (2)
athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzServiceBuilder.java (2)
68-75: LGTM! Static resource correctly wrapped as provider.The implementation correctly wraps the static resource in an
AthenzResourceProviderthat returns a completedCompletableFuture, and appropriately setsresourceTagValueto the resource name for metric tagging. The mutual exclusivity check ensures proper builder usage.
138-151: LGTM! Validation and construction logic is solid.The method correctly validates that all required fields (
athenzResourceProvider,athenzAction,resourceTagValue) are set before constructing theAthenzService. The constructor invocation is properly wrapped and passes the provider and tag value as expected for dynamic resource resolution.
athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzServiceBuilder.java
Show resolved
Hide resolved
…ServiceBuilder.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
| return CompletableFuture.completedFuture(""); | ||
| } | ||
| return CompletableFuture.completedFuture(value); |
There was a problem hiding this comment.
- Conventionally,
UnmodifableFutureis used for already completed values. - Should we raise
AthenzResourceNotFoundExceptionin a error case?
return UnmodifiableFuture.exceptionallyCompletedFuture(
new AthenzResourceNotFoundException("Header + headerName + " not found");There was a problem hiding this comment.
I've also updated the return type from CompletableFuture to UnmodifiableFuture and made it throw AthenzResourceNotFoundException when the resource cannot be resolved.
| @UnstableApi | ||
| public class JsonBodyFieldAthenzResourceProvider implements AthenzResourceProvider { |
There was a problem hiding this comment.
| @UnstableApi | |
| public class JsonBodyFieldAthenzResourceProvider implements AthenzResourceProvider { | |
| final class JsonBodyFieldAthenzResourceProvider implements AthenzResourceProvider { |
There was a problem hiding this comment.
Done! Removed public modifiers from all AthenzResourceProvider implementations.
| @UnstableApi | ||
| public class HeaderAthenzResourceProvider implements AthenzResourceProvider { |
There was a problem hiding this comment.
Let's exclude the implementations from public API.
| @UnstableApi | |
| public class HeaderAthenzResourceProvider implements AthenzResourceProvider { | |
| final class HeaderAthenzResourceProvider implements AthenzResourceProvider { |
There was a problem hiding this comment.
Done! Removed public modifiers from all AthenzResourceProvider implementations.
| return new JsonBodyFieldAthenzResourceProvider(jsonPointer); | ||
| } | ||
|
|
||
| static AthenzResourceProvider ofJsonField(String jsonFieldName) { |
There was a problem hiding this comment.
Would you mind writing Javadoc for the public methods?
There was a problem hiding this comment.
Also added Javadoc for the public methods in AthenzResourceProvider.java.
| public CompletableFuture<String> provide(ServiceRequestContext ctx, HttpRequest req) { | ||
| final MediaType contentType = req.contentType(); | ||
| if (contentType == null || !contentType.is(MediaType.JSON)) { | ||
| return CompletableFuture.completedFuture(""); |
There was a problem hiding this comment.
How about returning an exception instead to describe the exception in detail?
if (contentType == null || !contentType.is(MediaType.JSON)) {
return UnmodifiableFuture.exceptionallyCompletedFuture(
new AthenzResourceNotFoundException("Unsupported media type. Only " +
"application/json are supported");
}There was a problem hiding this comment.
Changed to return AthenzResourceNotFoundException
| final JsonNode root = mapper.readTree(agg.contentUtf8()); | ||
| final JsonNode node = root.at(jsonPointer); | ||
| if (node.isMissingNode()) { | ||
| logger.debug("JSON pointer '{}' not found in request body", jsonPointer); |
There was a problem hiding this comment.
Ditto. Should we throw AthenzResourceNotFoundException and include the exception in the returned response body?
There was a problem hiding this comment.
I've introduced AthenzResourceNotFoundException and updated all provider implementations to throw this exception when:
- Resource resolution fails
- Resolved resource is
nullor empty string
The exception is caught in AthenzService.serve() and included in the error response body.
Changes in 5745e58
| @UnstableApi | ||
| public final class PathAthenzResourceProvider implements AthenzResourceProvider { |
There was a problem hiding this comment.
| @UnstableApi | |
| public final class PathAthenzResourceProvider implements AthenzResourceProvider { | |
| final class PathAthenzResourceProvider implements AthenzResourceProvider { |
There was a problem hiding this comment.
Done! Removed public modifiers from all AthenzResourceProvider implementations.
| "Exception occurred while resolving resource: " + cause.getMessage())); | ||
| } | ||
|
|
||
| if (athenzResource == null || athenzResource.isEmpty()) { |
There was a problem hiding this comment.
Should we handle AthenzResourceNotFoundException to return FORBIDDEN?
There was a problem hiding this comment.
Done!
private static HttpResponse createErrorResponse(Throwable cause) {
if (cause instanceof AthenzResourceNotFoundException) {
return HttpResponse.of(HttpStatus.FORBIDDEN, MediaType.PLAIN_TEXT,
"Resource could not be resolved: " + cause.getMessage());
} else {
return HttpResponse.of(HttpStatus.INTERNAL_SERVER_ERROR, MediaType.PLAIN_TEXT,
"Exception occurred while resolving resource: " + cause.getMessage());
}
}
| return HttpResponse.of(HttpStatus.UNAUTHORIZED, MediaType.PLAIN_TEXT, status.toString()); | ||
| } | ||
| }, ctx.blockingTaskExecutor()); | ||
| final CompletableFuture<HttpResponse> future = athenzResourceProvider.provide(ctx, req) |
There was a problem hiding this comment.
athenzResourceProvider.provide(ctx, req) is a user code that may throw exceptions. Let's wrap it with a try-catch block.
There was a problem hiding this comment.
Wrap it with try-catch block!
try {
final CompletableFuture<String> resourceFuture = athenzResourceProvider.provide(ctx, req);
final CompletableFuture<HttpResponse> future = resourceFuture
.thenApplyAsync(
athenzResource -> {
if (athenzResource == null || athenzResource.isEmpty()) {
throw new AthenzResourceNotFoundException(
"Athenz resource could not be resolved.");
}
final AccessCheckStatus status = authZpeClient.allowAccess(token,
athenzResource,
athenzAction);
final long elapsedNanos = System.nanoTime() - startNanos;
if (status == AccessCheckStatus.ALLOW) {
assert allowedTimer != null;
allowedTimer.record(elapsedNanos, TimeUnit.NANOSECONDS);
try {
return unwrap().serve(ctx, req);
} catch (Exception e) {
return Exceptions.throwUnsafely(e);
}
} else {
assert deniedTimer != null;
deniedTimer.record(elapsedNanos, TimeUnit.NANOSECONDS);
return HttpResponse.of(HttpStatus.UNAUTHORIZED, MediaType.PLAIN_TEXT,
status.toString());
}
},
ctx.blockingTaskExecutor())
.exceptionally(cause -> {
final Throwable unwrapped = Exceptions.peel(cause);
assert deniedTimer != null;
deniedTimer.record(System.nanoTime() - startNanos, TimeUnit.NANOSECONDS);
return createErrorResponse(unwrapped);
});
return HttpResponse.of(future);
} catch (Exception e) {
assert deniedTimer != null;
deniedTimer.record(System.nanoTime() - startNanos, TimeUnit.NANOSECONDS);
return createErrorResponse(e);
}
|
Would you run |
- Remove unnecessary try-catch blocks for sync/async separation - Consolidate exception handling into single `.exceptionally()` block - Remove redundant `supplyAsync()` wrapping of already-async operations - Reduce thread context switching by removing unnecessary `thenCompose()` - Fix Checkstyle violations: * Line length exceeds in Javadoc comments * Replace `CompletableFuture.completedFuture()` with `UnmodifiableFuture.completedFuture()` * Add missing copyright header to test files Performance improvements: - Reduced thread transitions from 4 to 2 for PathAthenzResourceProvider - Eliminated unnecessary executor usage for already-completed futures - More efficient resource resolution pipeline This change maintains the same functionality while improving: - Code readability and maintainability - Performance through reduced thread context switching - Consistent error handling across sync and async paths
JungJaeLee-JJ
changed the title
AthenzResourceProvider for dynamic resource resolution
JungJaeLee-JJ
changed the title
AthenzResourceProvider for dynamic resource resolutionAthenzResourceProvider for dynamic resource resolution
JungJaeLee-JJ
changed the title
AthenzResourceProvider for dynamic resource resolutionAthenzResourceProvider for dynamic athenz resource resolution
There was a problem hiding this comment.
Actionable comments posted: 1
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/server/athenz/AthenzServiceBuilder.java (1)
132-136:tokenType(TokenType...)currently ignores its argumentThe varargs overload validates input but never updates
this.tokenTypes, so:builder.tokenType(TokenType.ACCESS_TOKEN);has no effect, and the builder continues to use the default list. That’s surprising and likely not what callers expect.
Consider assigning the field, mirroring the
Iterableoverload:public AthenzServiceBuilder tokenType(TokenType... tokenTypes) { requireNonNull(tokenTypes, "tokenTypes"); checkArgument(tokenTypes.length > 0, "tokenTypes must not be empty"); - return this; + this.tokenTypes = ImmutableList.copyOf(tokenTypes); + return this; }
🧹 Nitpick comments (2)
athenz/src/test/java/com/linecorp/armeria/server/athenz/resource/AthenzResourceProviderTest.java (1)
66-82: Fix typos in test names for clarityThe method names use “Exits” instead of “Exists”, e.g.:
shouldProvideHeaderStringIfExitsshouldThrowExceptionIfHeaderNotExitsshouldProvideJsonFieldStringIfExitsshouldProvideJsonFieldStringIfExitsByJsonPointershouldThrowExceptionIfJsonFieldNotExitsRenaming them to
...IfExists/...NotExistswould improve readability while keeping behavior unchanged.Also applies to: 96-112, 114-163
athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzServiceBuilder.java (1)
63-69: Alignresource()Javadoc with bothresourceProvideroverloadsThe Javadoc for
resource(String)references only
resourceProvider(AthenzResourceProvider, String)as the alternative mandatory configuration:* <p><strong>Mandatory:</strong> Either this or * {@link #resourceProvider(AthenzResourceProvider, String)} must be set ...But you also provide a no-tag overload
resourceProvider(AthenzResourceProvider)that delegates to the 2‑arg method. To avoid confusion, it’s clearer to mentionresourceProvider(AthenzResourceProvider)(or just “resourceProvider(...)”) in this Javadoc as well.Purely a doc tweak; no behavior change required.
Also applies to: 80-91
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzResourceNotFoundException.java(1 hunks)athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzService.java(5 hunks)athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzServiceBuilder.java(3 hunks)athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/AthenzResourceProvider.java(1 hunks)athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/HeaderAthenzResourceProvider.java(1 hunks)athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/JsonBodyFieldAthenzResourceProvider.java(1 hunks)athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/PathAthenzResourceProvider.java(1 hunks)athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/package-info.java(1 hunks)athenz/src/test/java/com/linecorp/armeria/server/athenz/AthenzDocker.java(2 hunks)athenz/src/test/java/com/linecorp/armeria/server/athenz/AthenzResourceProviderIntegrationTest.java(1 hunks)athenz/src/test/java/com/linecorp/armeria/server/athenz/resource/AthenzResourceProviderTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/HeaderAthenzResourceProvider.java
- athenz/src/test/java/com/linecorp/armeria/server/athenz/AthenzDocker.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
⚙️ CodeRabbit configuration file
**/*.java: - The primary coding conventions and style guide for this project are defined insite/src/pages/community/developer-guide.mdx. Please strictly adhere to this file as the ultimate source of truth for all style and convention-related feedback.2. Specific check for
@UnstableApi
- Review all newly added public classes and methods to ensure they have the
@UnstableApiannotation.- However, this annotation is NOT required under the following conditions:
- If the class or method is located in a package containing
.internalor.testing.- If the class or method is located in a test source set.
- If a public method is part of a class that is already annotated with
@UnstableApi.
Files:
athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/package-info.javaathenz/src/main/java/com/linecorp/armeria/server/athenz/resource/JsonBodyFieldAthenzResourceProvider.javaathenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzService.javaathenz/src/test/java/com/linecorp/armeria/server/athenz/AthenzResourceProviderIntegrationTest.javaathenz/src/main/java/com/linecorp/armeria/server/athenz/resource/PathAthenzResourceProvider.javaathenz/src/main/java/com/linecorp/armeria/server/athenz/resource/AthenzResourceProvider.javaathenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzServiceBuilder.javaathenz/src/test/java/com/linecorp/armeria/server/athenz/resource/AthenzResourceProviderTest.javaathenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzResourceNotFoundException.java
🧬 Code graph analysis (3)
athenz/src/test/java/com/linecorp/armeria/server/athenz/AthenzResourceProviderIntegrationTest.java (2)
athenz/src/test/java/com/linecorp/armeria/server/athenz/AthenzDocker.java (1)
AthenzDocker(55-340)core/src/main/java/com/linecorp/armeria/common/util/UnmodifiableFuture.java (1)
UnmodifiableFuture(42-212)
athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/PathAthenzResourceProvider.java (4)
athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzResourceNotFoundException.java (1)
UnstableApi(25-42)athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzService.java (1)
UnstableApi(111-249)athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/HeaderAthenzResourceProvider.java (1)
UnstableApi(48-74)athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/JsonBodyFieldAthenzResourceProvider.java (1)
UnstableApi(78-129)
athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzResourceNotFoundException.java (3)
athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzServiceBuilder.java (1)
UnstableApi(41-167)athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/HeaderAthenzResourceProvider.java (1)
UnstableApi(48-74)athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/JsonBodyFieldAthenzResourceProvider.java (1)
UnstableApi(78-129)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (6)
athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/package-info.java (1)
1-25: Package-level annotations and Javadoc look correctLicense header, package Javadoc, and the use of
@UnstableApiand@NonNullByDefaultat the package level all look consistent with Armeria conventions. This neatly marks the new resource provider APIs as unstable and enforces non-null-by-default semantics for everything under this package.As per coding guidelines, this satisfies the requirement to treat newly added public APIs as unstable.
athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzResourceNotFoundException.java (1)
21-41: Exception type and API shape look good
AthenzResourceNotFoundExceptionis small, focused, correctly annotated with@UnstableApi, and provides the expected constructors for providers andAthenzServiceto use. No changes needed.athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/PathAthenzResourceProvider.java (1)
50-72: Path-based provider implementation matches contractUsing
ctx.path()for the no-query case andreq.path()for the with-query case cleanly matches the documented examples and theofPath(boolean includeQuery)factories. Looks correct and minimal.athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/AthenzResourceProvider.java (1)
28-76: Resource provider interface and factories look well-shaped
AthenzResourceProvideris a clear@UnstableApifunctional interface with concise factories (ofPath,ofHeader,ofJsonFieldvariants) that map cleanly to the internal implementations. The asyncCompletableFuture<String>contract fits the dynamic-resolution use cases.No changes needed here.
Also applies to: 78-125
athenz/src/test/java/com/linecorp/armeria/server/athenz/AthenzResourceProviderIntegrationTest.java (1)
57-131: Integration wiring comprehensively exercises the new providersThe server setup and tests collectively cover:
- Path-based, header-based, and JSON-body resource resolution.
- Valid vs invalid resources across all
TokenTypevalues.- Provider exception, null, and empty-resource scenarios mapping to
FORBIDDEN.The decorators are configured using the new
resourceProvider(...)API and behave consistently withAthenzService’s documented error handling. Looks solid.Also applies to: 134-302
athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzService.java (1)
49-67: Dynamic resource resolution and error handling are consistent and robustThe refactor to use
AthenzResourceProviderplusresourceTagValue:
- Keeps the static-resource constructor behavior by wrapping into a provider.
- Ensures metrics use a stable
resourceTagValueto avoid cardinality issues.- Properly distinguishes:
- 401 for missing token / denied access,
- 403 for
AthenzResourceNotFoundException(unresolvable resources),- 500 for other provider/Resolution exceptions,
while always recording eitherallowedTimerordeniedTimer.The async chain with
thenApplyAsync(..., ctx.blockingTaskExecutor())and the fallbacktry/catcharoundprovide()give good coverage for both async and synchronous provider failures. No changes needed.Also applies to: 122-157, 174-225, 240-247
...in/java/com/linecorp/armeria/server/athenz/resource/JsonBodyFieldAthenzResourceProvider.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/JsonBodyFieldAthenzResourceProvider.java (1)
41-42: Update Javadoc to reflect exceptional completion behavior.The Javadoc states "an empty string is returned and the error is logged at debug level," but the implementation (lines 109-111, 121, 125) throws
AthenzResourceNotFoundExceptionwhich causes the future to complete exceptionally. There is no empty-string fallback.Update the documentation to reflect the actual behavior:
* <p>This provider parses the request body as JSON and extracts the resource value from the field - * specified by a JSON pointer. If the content type is not JSON, the field does not exist, - * or parsing fails, an empty string is returned and the error is logged at debug level. + * specified by a JSON pointer. If the content type is not JSON, the field does not exist, + * or parsing fails, the returned future completes exceptionally with + * {@link AthenzResourceNotFoundException}.Note: This issue was previously identified in past review comments.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzService.java(5 hunks)athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/AthenzResourceNotFoundException.java(1 hunks)athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/HeaderAthenzResourceProvider.java(1 hunks)athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/JsonBodyFieldAthenzResourceProvider.java(1 hunks)athenz/src/test/java/com/linecorp/armeria/server/athenz/AthenzResourceProviderIntegrationTest.java(1 hunks)athenz/src/test/java/com/linecorp/armeria/server/athenz/resource/AthenzResourceProviderTest.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
⚙️ CodeRabbit configuration file
**/*.java: - The primary coding conventions and style guide for this project are defined insite/src/pages/community/developer-guide.mdx. Please strictly adhere to this file as the ultimate source of truth for all style and convention-related feedback.2. Specific check for
@UnstableApi
- Review all newly added public classes and methods to ensure they have the
@UnstableApiannotation.- However, this annotation is NOT required under the following conditions:
- If the class or method is located in a package containing
.internalor.testing.- If the class or method is located in a test source set.
- If a public method is part of a class that is already annotated with
@UnstableApi.
Files:
athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/AthenzResourceNotFoundException.javaathenz/src/main/java/com/linecorp/armeria/server/athenz/resource/HeaderAthenzResourceProvider.javaathenz/src/test/java/com/linecorp/armeria/server/athenz/AthenzResourceProviderIntegrationTest.javaathenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzService.javaathenz/src/test/java/com/linecorp/armeria/server/athenz/resource/AthenzResourceProviderTest.javaathenz/src/main/java/com/linecorp/armeria/server/athenz/resource/JsonBodyFieldAthenzResourceProvider.java
🧬 Code graph analysis (2)
athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/AthenzResourceNotFoundException.java (3)
athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzService.java (1)
UnstableApi(112-250)athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/HeaderAthenzResourceProvider.java (1)
UnstableApi(47-73)athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/JsonBodyFieldAthenzResourceProvider.java (1)
UnstableApi(77-128)
athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/JsonBodyFieldAthenzResourceProvider.java (5)
athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzService.java (1)
UnstableApi(112-250)athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/AthenzResourceNotFoundException.java (1)
UnstableApi(25-42)athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/HeaderAthenzResourceProvider.java (1)
UnstableApi(47-73)athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/PathAthenzResourceProvider.java (1)
UnstableApi(56-73)athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzServiceBuilder.java (1)
UnstableApi(41-167)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (11)
athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/AthenzResourceNotFoundException.java (1)
26-42: LGTM!The exception class is well-structured with proper
@UnstableApiannotation, clear documentation, and standard exception constructors.athenz/src/main/java/com/linecorp/armeria/server/athenz/AthenzService.java (4)
53-111: LGTM!The enhanced documentation clearly explains both static and dynamic resource patterns with practical examples and comprehensive error-handling guidance.
135-158: LGTM!The constructor design cleanly supports both dynamic providers and static resources while maintaining backward compatibility by wrapping static resources in a provider.
186-227: LGTM!The asynchronous resource resolution logic is well-implemented with comprehensive error handling:
- Try-catch for synchronous provider exceptions
exceptionallyhandler for async failures- Null/empty resource validation
- Proper timer recording in all code paths
All concerns from previous reviews have been addressed.
241-249: LGTM!The error response helper correctly distinguishes between resource-not-found scenarios (403 FORBIDDEN) and unexpected exceptions (500 INTERNAL_SERVER_ERROR), as specified in the PR objectives.
athenz/src/test/java/com/linecorp/armeria/server/athenz/resource/AthenzResourceProviderTest.java (1)
36-162: LGTM!Comprehensive test coverage for all provider types (path, header, JSON field) with clear given-when-then structure and proper validation of both success and error paths.
athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/HeaderAthenzResourceProvider.java (2)
57-61: LGTM!Constructor properly validates the header name parameter, consistent with validation patterns in other provider implementations.
64-72: LGTM!The
providemethod correctly returns an exceptionally completed future withAthenzResourceNotFoundExceptionfor missing or empty headers, and usesUnmodifiableFutureappropriately.athenz/src/main/java/com/linecorp/armeria/server/athenz/resource/JsonBodyFieldAthenzResourceProvider.java (2)
89-103: LGTM!Both constructors properly validate inputs and support flexible field specification through simple names or JSON Pointers, with dot-notation conversion for convenience.
106-127: LGTM!The implementation correctly validates JSON content-type before aggregation, properly parses the JSON body, and throws
AthenzResourceNotFoundExceptionfor all error cases (unsupported content-type, missing field, parsing failures).athenz/src/test/java/com/linecorp/armeria/server/athenz/AthenzResourceProviderIntegrationTest.java (1)
52-303: LGTM!Comprehensive integration test suite that validates:
- All provider types (path, header, JSON body)
- Authorization success and denial scenarios
- Error handling (exceptions, null, empty resources)
- Correct HTTP status codes (OK, UNAUTHORIZED, FORBIDDEN)
Parameterized tests over
TokenTypeensure thorough coverage.
.../src/main/java/com/linecorp/armeria/server/athenz/resource/HeaderAthenzResourceProvider.java
Show resolved
Hide resolved
Sorry for bothering you multiple times due to the code conventions. I ran |
JungJaeLee-JJ
changed the title
AthenzResourceProvider for dynamic athenz resource resolutionAthenzResourceProvider for dynamic athenz resource resolution
4 participants
Motivation
Currently,
AthenzServiceonly supports static resource strings configured at build time using theresource(String)method. This limitation makes it difficult to implement dynamic authorization patterns where the resource needs to be determined from the HTTP request itself (e.g., path parameters, headers, or request body).This PR introduces the
AthenzResourceProviderinterface to enable dynamic resource resolution based on request attributes, allowing more flexible Athenz authorization patterns.Modifications
Dynamic Resource Provider Support
AthenzResourceProviderfunctional interface for dynamic resource resolutionPathAthenzResourceProvider: Extract resource from request pathHeaderAthenzResourceProvider: Extract resource from HTTP headerJsonBodyFieldAthenzResourceProvider: Extract resource from JSON request body fieldAthenzServiceto useAthenzResourceProviderinstead of static resource stringresourceProvider()method toAthenzServiceBuilderserve()method to handle async resource resolution with proper error handlingINTERNAL_SERVER_ERRORwhen resource resolution failsDocumentation & Testing
Metric Stability Improvements
resourceTagValueparameter toresourceProvider()method to prevent metric cardinality explosionresourceTagValueprovides a stable identifier for metric tagging (e.g., "admin", "users")resource()method to automatically setresourceTagValueto the resource nameserviceAdded()to useresourceTagValuefor metric tags instead of dynamic resource valuesResult
After this PR is merged:
resource()method remains fully compatible (backward compatible)Breaking Changes
AthenzServiceconstructor now requiresAthenzResourceProviderinstead ofString resource(internal API only)