[feat][client] PIP-234: Support shared resources in AuthenticationOAuth2 to reduce thread usage#25072
[feat][client] PIP-234: Support shared resources in AuthenticationOAuth2 to reduce thread usage#250723pacccccc wants to merge 15 commits into
Conversation
lhotari
left a comment
There was a problem hiding this comment.
I think that there's a need to revisit the original design expressed in#24795. The comment in #24944 (comment) contains some details of the direction:
For authentication plugins, one possibility would be that there would be a way to directly get a HTTP client with given configuration (such as trustCertsFilePath) that is sufficiently abstracted so that there's no need to couple to AsyncHttpClient in authentication plugin implementations.
One of the problems in exposing Netty and AsyncHttpClient classes in the public interfaces is that the classes are different when using a shaded Pulsar client.
That is why there should be a sufficient abstraction for the HTTP client.
For Pulsar internal authentication plugins, it wouldn't be a problem to support the solution in this PR since the classes are also shaded in the shaded Pulsar client and there wouldn't be a problem with unshaded Netty and shaded Netty classes, for example. However, it might be work trying to find a more stable abstraction that expressed in the original design.
# Conflicts: # pulsar-client/src/main/java/org/apache/pulsar/client/impl/auth/oauth2/AuthenticationFactoryOAuth2.java # pulsar-client/src/main/java/org/apache/pulsar/client/impl/auth/oauth2/AuthenticationOAuth2.java # pulsar-client/src/main/java/org/apache/pulsar/client/impl/auth/oauth2/ClientCredentialsFlow.java # pulsar-client/src/main/java/org/apache/pulsar/client/impl/auth/oauth2/FlowBase.java # pulsar-client/src/main/java/org/apache/pulsar/client/impl/auth/oauth2/protocol/DefaultMetadataResolver.java # pulsar-client/src/test/java/org/apache/pulsar/client/impl/auth/oauth2/AuthenticationOAuth2Test.java
|
@lhotari hi, Lari, could you please take a look again? |
|
For Pulsar 5.0 there's PIP-478 in voting, https://lists.apache.org/thread/6n6q014236l4tstrxn9t4jbrp8xty997. Please check it. It also covers shared resources for auth plugins. |
fix: #24795
🔗 Related links
Motivation
OAuth2 authentication currently creates its own HTTP client resources for metadata discovery and token exchange. These resources are not integrated with the client resource sharing introduced by PIP-234, so OAuth2 can create separate thread pools and DNS resolver/cache state instead of using the client-provided configuration.
At the same time, authentication implementations should not expose shaded AsyncHttpClient or Netty classes through the public client API. This PR adds a small authentication-specific abstraction so authentication providers can obtain a configured HTTP client from the Pulsar client container without depending on shaded implementation types.
OAuth2 token exchange is still synchronous today, so authentication HTTP I/O must not run on the Pulsar client I/O event loop. Otherwise, broker internal clients can block during authentication and cause topic loading to time out, such as in
Oauth2PerformanceTransactionTest.testTransactionPerf.Modifications
Added
AuthenticationInitContextand a defaultAuthentication.start(AuthenticationInitContext)method.Added authentication HTTP abstractions:
AuthenticationHttpClientFactoryAuthenticationHttpClientAuthenticationHttpClientConfigAuthenticationHttpRequestAuthenticationHttpResponseAdded
AuthenticationInitContextImplin the client implementation.Updated
PulsarClientImplto provide an authentication HTTP client factory through the initialization context.Reused the client DNS resolver group configuration/cache for authentication HTTP requests.
Reused the client timer for authentication HTTP requests.
Kept authentication HTTP I/O on a dedicated lazy event loop so synchronous OAuth2 token exchange does not block Pulsar client I/O threads.
Reused the same authentication HTTP client factory across
updateAuthentication()calls instead of creating a new factory every time.Refactored OAuth2 metadata resolution and token exchange to use the new HTTP abstraction instead of directly depending on AsyncHttpClient.
Preserved the previous fallback behavior when
trustCertsFilePathcannot build a custom SSL context.Added cleanup for TLS refresh/SSL resources when HTTP client construction fails.
Make sure that the change passes the CI checks.
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: 3pacccccc#35