Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[improve][client] Implement HTTP client using javax.ws.rs #23905

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nodece
Copy link
Member

@nodece nodece commented Jan 28, 2025

Motivation

I encountered an issue when using Kerberos and TLS transport to connect to the broker. The pulsar-admin and pulsar-client (for HTTP lookup) failed to work as expected.

In the org.apache.pulsar.client.impl.auth.AuthenticationSasl#authenticationStage method, the broker requests should use the WebTarget provided by the pulsar-admin or pulsar-client HTTP clients. However, the current implementation creates a new HTTP client to construct the WebTarget, causing the TLS context to be lost.

Currently, the pulsar-admin HTTP client is built on javax.ws.rs and AHC (Async Http Client), while the pulsar-client HTTP client relies only on AHC. To resolve this inconsistency and maintain the TLS context, we need to refactor the pulsar-client to provide the required WebTarget.

Modifications

  1. Package Changes:

    • Renamed org.apache.pulsar.client.admin.internal.http to org.apache.pulsar.client.internal.http.
    • Moved the renamed package from pulsar-client-admin to pulsar-client.
  2. Refactored HTTP Client:

    • Refactored HttpClient.java to use AsyncHttpConnectorProvider and AsyncHttpConnector for a unified and consistent implementation.
  3. Fixed Client Calls:

    • Updated pulsar-client calls to utilize the refactored HTTP client.

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jan 28, 2025
@lhotari
Copy link
Member

lhotari commented Jan 28, 2025

This would be a major change to the admin client. It feels that there's something wrong in the AuthenticationSasl if it gets coupled to the client type.

Updated pulsar-client calls to utilize the refactored HTTP client and maintain the TLS context seamlessly.

What does "maintain the TLS context" mean in practice? What are the different implementation options?

@lhotari
Copy link
Member

lhotari commented Jan 28, 2025

One detail to consider is that we'd like to migrate off javax.ws.rs interface to the jakarta.ws.rs one .
It's not directly related, but it would be possible after Jetty 12 migration, https://lists.apache.org/thread/mkxw56bt8jdf5b1zq1gdwvy50xgrdj30 . In Jetty 12, you can choose to continue to use Java EE (javax.*) interfaces. However, it seems that the future would be to use jakarta.* ones.

@nodece
Copy link
Member Author

nodece commented Jan 29, 2025

This would be a major change to the admin client.

Should be client, not admin client?

It feels that there's something wrong in the
AuthenticationSasl if it gets coupled to the client type.

This authentication process requires the broker endpoint, and we pass the URL string, which indicates to the broker which endpoint is being requested.

Updated pulsar-client calls to utilize the refactored HTTP client and maintain the TLS context seamlessly.

What does "maintain the TLS context" mean in practice?

Just pass the WebTarget to the Authtication. I will make a new PR to handle this thing.

What are the different implementation options?

I think we can only pass the WebTarget to the auth. Do you any ideas?

Using jakarta.ws.rs instead of javax.ws.rs is a good idea, but fixing the bug is still important, so I want to keep this path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants