Refactor ZtsBaseClient to an interface and add ArmeriaJwtsSigningKeyResolver#6848
Refactor ZtsBaseClient to an interface and add ArmeriaJwtsSigningKeyResolver#6848jrhee17 wants to merge 3 commits into
ZtsBaseClient to an interface and add ArmeriaJwtsSigningKeyResolver#6848Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughRefactors ChangesZtsBaseClient refactor and JWKS resolver wiring
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@athenz/src/main/java/com/linecorp/armeria/client/athenz/ZtsBaseClient.java`:
- Around line 112-118: The default ZtsBaseClient.webClient(Consumer<? super
WebClientBuilder>) implementation only null-checks and returns webClient(), so
it silently drops the supplied configurer despite the Javadoc implying
otherwise. Update the Javadoc on webClient(Consumer) to clearly state that the
configurer is ignored in the default method unless an implementation overrides
it, and keep DefaultZtsBaseClient as the place that actually applies the
WebClientBuilder customization.
In
`@athenz/src/main/java/com/yahoo/athenz/auth/token/jwts/ArmeriaJwtsSigningKeyResolver.java`:
- Around line 45-53: Add explicit null validation to the public factory method
create in ArmeriaJwtsSigningKeyResolver by checking both webClient and
oauth2KeysPath with Objects.requireNonNull before any use. The current
implementation can fail later with an unclear NPE at webClient.uri() or inside
ArmeriaResourceRetriever, so update create to reject null inputs up front and
keep the INIT_RETRIEVER lifecycle unchanged.
🪄 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: bba3bfd5-1c6f-46f9-8c4a-4ea265f9a617
📒 Files selected for processing (8)
athenz/src/main/java/com/linecorp/armeria/client/athenz/DefaultZtsBaseClient.javaathenz/src/main/java/com/linecorp/armeria/client/athenz/ZtsBaseClient.javaathenz/src/main/java/com/linecorp/armeria/client/athenz/ZtsBaseClientBuilder.javaathenz/src/main/java/com/linecorp/armeria/server/athenz/MinifiedAuthZpeClient.javaathenz/src/main/java/com/yahoo/athenz/auth/token/jwts/ArmeriaJwtsSigningKeyResolver.javaathenz/src/main/java/com/yahoo/athenz/auth/token/jwts/ArmeriaResourceRetriever.javaathenz/src/main/java/com/yahoo/athenz/auth/token/jwts/package-info.javaathenz/src/test/java/com/linecorp/armeria/client/athenz/ArmeriaJwtsSigningKeyResolverTest.java
|
Tick the box to add this pull request to the merge queue (same as
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6848 +/- ##
============================================
- Coverage 74.46% 0 -74.47%
============================================
Files 1963 0 -1963
Lines 82437 0 -82437
Branches 10764 0 -10764
============================================
- Hits 61385 0 -61385
+ Misses 15918 0 -15918
+ Partials 5134 0 -5134 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
|
||
| @Override | ||
| public void close() { | ||
| defaultWebClient.options().factory().closeAsync(); |
There was a problem hiding this comment.
Close clientFactory directly?
| final ArmeriaResourceRetriever retriever = INIT_RETRIEVER.get(); | ||
| if (retriever != null) { | ||
| return retriever; | ||
| } |
There was a problem hiding this comment.
Should we leave a warning log if retriever is null?
| * Returns a new {@link WebClient} that can connect to the ZTS server with the specified configurer. | ||
| */ | ||
| public WebClient webClient(Consumer<? super WebClientBuilder> configurer) { | ||
| default WebClient webClient(Consumer<? super WebClientBuilder> configurer) { |
There was a problem hiding this comment.
configurer is ignored. Can we make this method as just a method instead of default method?
| builder.alpnProtocols(SslContextUtil.DEFAULT_ALPN_PROTOCOLS); | ||
| builder.allowUnsafeCiphers(allowUnsafeCiphers); | ||
| return builder.build(); | ||
| final String proxyUrl = System.getProperty(ZpeConsts.ZPE_PROP_JWK_PROXY_URI); |
There was a problem hiding this comment.
Couldn't this be a breaking change if the proxy URL specified in ZtsBaseClientBuilder is ignored?
There was a problem hiding this comment.
I see, I wasn't sure of the intention of the original behavior either.
I was assuming that if the branch at L104 is passed, that we are defaulting back to using java properties since ZPE_PROP_JWK_URI, ZPE_PROP_JWK_PRIVATE_KEY_PATH, ZPE_PROP_JWK_X509_CERT_PATH are used.
If we should avoid a breaking change, I can revert this change. Let me know what you think
There was a problem hiding this comment.
The intention behind the existing behavior was to make ZtsBaseClient work only through Armeria’s builder API, without depending on Athenz system properties.
Although we cannot migrate every property, we intended to make the essential properties configurable through ZtsBaseClientBuilder.
For example, the proxy URL may vary depending on the user, and communication may not be possible without it. Therefore, I considered it an essential property.
There was a problem hiding this comment.
Just to make sure we're on the same page
Using non-system properties, we already uses armeria's proxy mechanism with the current proposed changes
Using java properties, we should still ignore other properties set on the builder except for proxyUrl
Is my understanding correct?
There was a problem hiding this comment.
I assumed we could reuse the existing proxy value, since the proxy had already been configured for communication with the ZTS server. I also thought that having multiple proxy servers would be uncommon.
As for the other system properties, I thought it would be fine to keep the upstream behavior as is, since there were no conflicting settings.
That said, after searching the internal repositories, I found that ZPE_PROP_JWK_PROXY_URI and athenz.zpe.jwk_proxy_uri do not appear to be used anywhere, so I believe this change will not have any impact. Given that, I think we can keep the current implementation instead of reverting it.
By the way, why don't we use ArmeriaJwtsSigningKeyResolver in this case?
|
|
||
| clientBuilder.factory(clientFactory); | ||
|
|
||
| if (LoggerFactory.getLogger(DefaultZtsBaseClient.class).isTraceEnabled()) { |
There was a problem hiding this comment.
Chaining the logger name would be a weak breaking change.
| if (LoggerFactory.getLogger(DefaultZtsBaseClient.class).isTraceEnabled()) { | |
| if (LoggerFactory.getLogger(ZtsBaseClient.class).isTraceEnabled()) { |
3 participants
See #6847 for how this refactoring may be used.
Motivation:
ZtsBaseClientis a concrete final class, which makes it impossible to provide custom implementations (e.g., for xDS-based Athenz integration where theWebClientis constructed from a cluster snapshot rather than aZtsBaseClientBuilder). Additionally,MinifiedAuthZpeClientextracts anSSLContextfrom theClientFactory'sTlsProviderto construct aJwtsSigningKeyResolver, which is fragile and tightly coupled to implementation details.Modifications:
ZtsBaseClientfrom a final class to an interface with a single required method:webClient().ztsUri(),proxyUri(),clientFactory(),webClient(Consumer),addTlsKeyPairListener(),removeTlsKeyPairListener()) are retained as default methods for backward compatibility.ztsUri()andproxyUri()as they leak implementation details.DefaultZtsBaseClient.ArmeriaJwtsSigningKeyResolverandArmeriaResourceRetrieverwhich use an ArmeriaWebClientto fetch JWKS keys, replacing the previous approach of extracting anSSLContextfrom theClientFactory.MinifiedAuthZpeClientto useArmeriaJwtsSigningKeyResolverinstead of manually constructingSSLContextandJwtsSigningKeyResolver.proxyUrifield fromZtsBaseClientBuilderas it is already applied to theClientFactoryand does not need to be stored separately.ArmeriaJwtsSigningKeyResolverTestto verify JWKS key resolution via a mock server.Result:
ZtsBaseClientcan now be implemented by custom classes that provide their ownWebClient, enabling use cases such as xDS-driven Athenz integration.WebClient(with its TLS and proxy configuration) instead of constructing a separateSSLContext, reducing complexity and potential configuration drift.ZtsBaseClient.builder()continues to work as before.