[feat][client] oauth2 trustcerts file and timeouts

[gulecroc]

Fixes #24437

Motivation

For client oauth2 authentication, allow to configure :

• connect timeout
• read timeout
• trust certs file

Modifications

Verifying this change

• Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

• Added integration tests for end-to-end deployment with large payloads (10MB)
• Extended integration test for recovery after broker failure

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

• Dependencies (add or upgrade a dependency)
• The public API
• The schema
• The default values of configurations
• The threading model
• The binary protocol
• The REST endpoints
• The admin CLI options
• The metrics
• Anything that affects deployment

Documentation

• doc
• [] doc-required
• doc-not-needed
• doc-complete

Matching PR in forked repository

PR in forked repository:

[gulecroc]

Doc : apache/pulsar-site#1052

[lhotari]

Besides having an API for instantiating the Authentication instance, it is necessary to consider how the Authentication instance would be instantiated when parameters are passed in authParams (for example in client.conf).

You should also update

pulsar/pulsar-client/src/main/java/org/apache/pulsar/client/impl/auth/oauth2/ClientCredentialsFlow.java

Lines 117 to 134 in b402985

	/**
	* Constructs a {@link ClientCredentialsFlow} from configuration parameters.
	* @param params
	* @return
	*/
	public static ClientCredentialsFlow fromParameters(Map<String, String> params) {
	URL issuerUrl = parseParameterUrl(params, CONFIG_PARAM_ISSUER_URL);
	String privateKeyUrl = parseParameterString(params, CONFIG_PARAM_KEY_FILE);
	// These are optional parameters, so we only perform a get
	String scope = params.get(CONFIG_PARAM_SCOPE);
	String audience = params.get(CONFIG_PARAM_AUDIENCE);
	return ClientCredentialsFlow.builder()
	.issuerUrl(issuerUrl)
	.audience(audience)
	.privateKey(privateKeyUrl)
	.scope(scope)
	.build();
	}

so that

pulsar/pulsar-client/src/main/java/org/apache/pulsar/client/impl/auth/oauth2/AuthenticationOAuth2.java

Lines 67 to 86 in 671994f

	public void configure(String encodedAuthParamString) {
	if (StringUtils.isBlank(encodedAuthParamString)) {
	throw new IllegalArgumentException("No authentication parameters were provided");
	}
	Map<String, String> params;
	try {
	params = AuthenticationUtil.configureFromJsonString(encodedAuthParamString);
	} catch (IOException e) {
	throw new IllegalArgumentException("Malformed authentication parameters", e);
	}
	String type = params.getOrDefault(CONFIG_PARAM_TYPE, TYPE_CLIENT_CREDENTIALS);
	switch(type) {
	case TYPE_CLIENT_CREDENTIALS:
	this.flow = ClientCredentialsFlow.fromParameters(params);
	break;
	default:
	throw new IllegalArgumentException("Unsupported authentication type: " + type);
	}
	}

would work.

[lhotari]

LGTM

[Copilot]

Pull Request Overview

This PR refactors the OAuth2 authentication implementation to centralize HTTP client management and configuration. The main goal is to improve consistency by having the HTTP client created and configured in one place (FlowBase) and shared across components.

Key changes:

• Centralized HTTP client creation in FlowBase with configurable timeouts and SSL trust certificates
• Removed HTTP client instantiation logic from TokenClient and DefaultMetadataResolver, making them accept the client as a constructor parameter
• Added a builder pattern to AuthenticationFactoryOAuth2 for more flexible configuration
• Added test coverage for the new builder pattern

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
AuthenticationFactoryOAuth2Test.java	New test file validating the builder pattern and factory method functionality
TokenClient.java	Simplified to accept AsyncHttpClient as constructor parameter, removed client creation logic and constants
DefaultMetadataResolver.java	Refactored to accept AsyncHttpClient as constructor parameter and use async HTTP instead of URLConnection
FlowBase.java	Added HTTP client initialization with configurable timeouts and SSL, plus close() method
ClientCredentialsFlow.java	Updated to pass new parameters to parent constructor and pass httpClient to components
AuthenticationFactoryOAuth2.java	Added ClientCredentialsBuilder class for flexible configuration

Comments suppressed due to low confidence (2)

pulsar-client/src/main/java/org/apache/pulsar/client/impl/auth/oauth2/protocol/TokenClient.java:49

• Potential NullPointerException if httpClient is null. Since the constructor now requires httpClient as a parameter and doesn't validate it, calling close() with a null httpClient will throw an NPE. Consider adding null check in constructor or in close() method.

    public void close() throws Exception {
        httpClient.close();

pulsar-client/src/main/java/org/apache/pulsar/client/impl/auth/oauth2/AuthenticationFactoryOAuth2.java:1

• Potential NullPointerException if credentialsUrl is null. The build() method calls credentialsUrl.toExternalForm() without validating that credentialsUrl is not null. Consider adding validation for required fields (issuerUrl and credentialsUrl) in the build() method.

/*

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[gulecroc]

I fix Copilot review

[lhotari]

@gulecroc for future PRs, you might want to use "Personal CI" to run Pulsar CI in your own fork to get early CI feedback when working on a larger PR like this one. The benefit is that you have full control over the CI and wouldn't have to wait for one of the project maintainers to approve a CI run.

Regarding flaky tests, in apache/pulsar CI, there is a way for anyone to trigger a retry run. That happens by adding "/pulsarbot rerun-failure-checks" comment on the PR. We have a lot of flaky tests and retrying 1 to 3 times for a PR is common. Retrying can only be requested after all jobs in the workflow have completed.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 75.60976% with 30 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.32%. Comparing base (0896c0a) to head (6a1dc71).
⚠️ Report is 4 commits behind head on master.

Files with missing lines	Patch %	Lines
...pache/pulsar/client/impl/auth/oauth2/FlowBase.java	70.27%	9 Missing and 2 partials ⚠️
...client/impl/auth/oauth2/ClientCredentialsFlow.java	77.14%	6 Missing and 2 partials ⚠️
.../client/impl/auth/oauth2/protocol/TokenClient.java	33.33%	6 Missing ⚠️
.../auth/oauth2/protocol/DefaultMetadataResolver.java	73.33%	4 Missing ⚠️
.../impl/auth/oauth2/AuthenticationFactoryOAuth2.java	96.29%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #24944      +/-   ##
============================================
- Coverage     74.44%   74.32%   -0.12%     
+ Complexity    34071    33528     -543     
============================================
  Files          1920     1920              
  Lines        150062   150108      +46     
  Branches      17404    17407       +3     
============================================
- Hits         111708   111569     -139     
- Misses        29495    29645     +150     
- Partials       8859     8894      +35     

Flag	Coverage Δ
inttests	26.18% <0.00%> (-0.45%)	⬇️
systests	22.88% <0.00%> (-0.03%)	⬇️
unittests	73.86% <75.60%> (-0.10%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
.../impl/auth/oauth2/AuthenticationFactoryOAuth2.java	93.10% <96.29%> (+4.21%)	⬆️
.../auth/oauth2/protocol/DefaultMetadataResolver.java	71.42% <73.33%> (+11.42%)	⬆️
.../client/impl/auth/oauth2/protocol/TokenClient.java	74.28% <33.33%> (-10.50%)	⬇️
...client/impl/auth/oauth2/ClientCredentialsFlow.java	79.36% <77.14%> (+4.36%)	⬆️
...pache/pulsar/client/impl/auth/oauth2/FlowBase.java	67.27% <70.27%> (+0.60%)	⬆️

... and 88 files with indirect coverage changes

🚀 New features to boost your workflow:

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

[gulecroc]

/pulsarbot rerun-failure-checks

[gulecroc]

/pulsarbot rerun-failure-checks

[gulecroc]

/pulsarbot rerun-failure-checks

[gulecroc]

Hi @lhotari, as I see AHC is moving to Duration for timeouts in v3.x, I update my code and documentation in this way

[gulecroc]

/pulsarbot rerun-failure-checks

[lhotari]

LGTM

[gulecroc]

Thank you @lhotari !
Can I carry over these changes to version 4.0.x LTS?

[lhotari]

Thank you @lhotari! Can I carry over these changes to version 4.0.x LTS?

@gulecroc Usually new features aren't immediately added to LTS versions to ensure that changes don't cause regressions.
New features can be added to an LTS release by starting a discussion on the dev mailing list. In most cases, it's better to have a PIP available for referencing the changes.

I'm just wondering whether this feature is completed by this PR. Is the intention to support updates for trustCertsFilePath? In that case, more changes would be needed to add support for trustCertsFilePath changes. I think that decoupling from AsyncHttpClient would be useful and instead having an internal abstraction that hides implementation details.

One detail to take into account is that there is PIP-337: SSL Factory Plugin to customize SSLContext/SSLEngine generation, which is expected to be used for SSLContext/SSLEngine instance creation. For consistency, that solution should be used. It's also possible that the PIP-337 solution might require some changes to support this new use case.

I think it would be useful to write a PIP document to capture the requirements and at least the high-level design. A PIP will also help when starting the community discussion and decision-making to include changes to 4.0.x LTS regarding this area.

This might help in creating a PIP: (how to create an LLM generated draft)

Btw. Regarding the AsyncHttpClient used in OAuth authentication, I'd also like it support PIP-234 changes so that the client doesn't create it's own set of threads, but has a way to reuse them from the PulsarClient / PulsarAdminClient. The solution for that is still missing. PIP-234 was planned a long time ago and it wasn't implemented until recently. The implementation is in PRs #24790, #24784 and #24893 and will be available in 4.1.2 version. More changes are needed so that there would be an interface to be used by authentication plugins for getting access to the shared resources.
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.