add httpconfig and remove original tlsconfig and httpengineconfig #47
Conversation
CennyMo
force-pushed
the
subscription
branch
3 times, most recently
from
005b206 to
5a3414d
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #47 +/- ##
============================================
+ Coverage 53.06% 55.34% +2.27%
- Complexity 95 102 +7
============================================
Files 21 22 +1
Lines 603 636 +33
Branches 57 59 +2
============================================
+ Hits 320 352 +32
Misses 235 235
- Partials 48 49 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: moxiaoying <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the HTTP client configuration by replacing the original TLS and HTTP engine configurations with a unified HttpClientConfig approach. The changes modernize the configuration API by using external configuration classes from the io.github.openfacade.http package.
- Replaces direct TLS configuration with HttpClientConfig-based approach
- Updates test classes to use parameterized tests and builder patterns
- Migrates from custom TlsConfig to external TlsConfig implementation
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| TlsClientWrongHostTest.java | Updated to use new TlsConfig.Builder and HttpClientConfig.Builder pattern |
| TlsClientTest.java | Refactored all test methods to use builder pattern and removed helper method |
| ClustersTest.java | Converted to parameterized test using MethodSource |
| BrokersTest.java | Converted to parameterized test using MethodSource |
| BaseTest.java | Updated to use HttpClientConfig.Builder instead of direct Configuration |
| PulsarAdminBuilderImpl.java | Replaced TLS-specific methods with HttpClientConfig methods |
| PulsarAdminBuilder.java | Updated interface to use HttpClientConfig instead of TlsConfig |
| InnerHttpClient.java | Simplified TLS detection logic using HttpClientConfig |
| SslContextUtil.java | Updated to use external TlsConfig with method-based access |
| InnerReactiveClient.java | Replaced custom HTTP client creation with ReactorHttpClientFactory |
| Configuration.java | Removed TLS-specific fields and replaced with HttpClientConfig |
| this.httpPrefix = "https://" + conf.host + ":" + conf.port; | ||
| } else { | ||
| this.httpPrefix = "http://" + conf.host + ":" + conf.port; |
There was a problem hiding this comment.
The conditional logic is inverted. When TLS config is null, it should use HTTP (not HTTPS), and when TLS config exists, it should use HTTPS. The current logic assigns HTTPS prefix when TLS config is null.
| this.httpPrefix = "https://" + conf.host + ":" + conf.port; | |
| } else { | |
| this.httpPrefix = "http://" + conf.host + ":" + conf.port; | |
| this.httpPrefix = "http://" + conf.host + ":" + conf.port; | |
| } else { | |
| this.httpPrefix = "https://" + conf.host + ":" + conf.port; |
Copilot uses AI. Check for mistakes.
| private PulsarAdmin createPulsarAdmin(TlsConfig tlsConfig) throws PulsarAdminException { | ||
| HttpClientConfig.Builder builder = new HttpClientConfig.Builder(); | ||
| builder.tlsConfig(tlsConfig); | ||
| return PulsarAdmin.builder() | ||
| .port(getPort()) | ||
| .tlsEnabled(true) | ||
| .tlsConfig(tlsConfig) | ||
| .build(); | ||
| .port(getPort()) | ||
| .httpClientConfig(builder.build()) | ||
| .build(); | ||
| } | ||
|
|
There was a problem hiding this comment.
[nitpick] The createPulsarAdmin helper method is now only used once and duplicates the same builder pattern used elsewhere. Consider removing this method and inlining the builder code in the remaining test that uses it.
Copilot uses AI. Check for mistakes.
| TlsConfig.Builder builder = new TlsConfig.Builder(); | ||
| builder.verifyDisabled(false); | ||
| builder.hostnameVerifyDisabled(true); | ||
| builder.versions(null); | ||
| builder.cipherSuites(null); | ||
| builder.keyStore(new File(CLIENT_KEYSTORE_FILE).getAbsolutePath(), | ||
| CLIENT_CERT_PASSWORD.toCharArray()); | ||
| builder.trustStore(new File(CLIENT_TRUSTSTORE_FILE).getAbsolutePath(), | ||
| CLIENT_CERT_PASSWORD.toCharArray()); | ||
| HttpClientConfig.Builder clientConfigBuilder = new HttpClientConfig.Builder(); | ||
| clientConfigBuilder.tlsConfig(builder.build()); | ||
| PulsarAdmin pulsarAdmin = PulsarAdmin.builder() | ||
| .port(getPort()) | ||
| .httpClientConfig(clientConfigBuilder.build()) | ||
| .build(); | ||
| pulsarAdmin.brokers().healthcheck(TopicVersion.V1); | ||
| } | ||
|
|
||
| @Test | ||
| public void testTlsCustomProtocol() throws PulsarAdminException { | ||
| TlsConfig tlsConfig = new TlsConfig( | ||
| new File(CLIENT_KEYSTORE_FILE).getAbsolutePath(), | ||
| CLIENT_CERT_PASSWORD.toCharArray(), | ||
| new File(CLIENT_TRUSTSTORE_FILE).getAbsolutePath(), | ||
| CLIENT_CERT_PASSWORD.toCharArray(), | ||
| false, | ||
| true, | ||
| new String[]{"TLSv1.2"}, | ||
| null | ||
| ); | ||
| PulsarAdmin pulsarAdmin = createPulsarAdmin(tlsConfig); | ||
| TlsConfig.Builder builder = new TlsConfig.Builder(); | ||
| builder.verifyDisabled(false); | ||
| builder.hostnameVerifyDisabled(true); | ||
| builder.versions(new String[]{"TLSv1.2"}); | ||
| builder.cipherSuites(null); | ||
| builder.keyStore(new File(CLIENT_KEYSTORE_FILE).getAbsolutePath(), | ||
| CLIENT_CERT_PASSWORD.toCharArray()); | ||
| builder.trustStore(new File(CLIENT_TRUSTSTORE_FILE).getAbsolutePath(), | ||
| CLIENT_CERT_PASSWORD.toCharArray()); |
There was a problem hiding this comment.
[nitpick] The TLS configuration builder pattern is repeated across multiple test methods. Consider extracting a helper method to create the base TlsConfig.Builder with common settings (keyStore, trustStore) to reduce code duplication.
Copilot uses AI. Check for mistakes.
3 participants
No description provided.