Repository navigation
CAMEL-26343: camel-aws - set the AWS SDK HTTP client explicitly instead of the classpath lookup - #28563
Conversation
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
The fix is well-targeted: explicitly setting Apache5HttpClient/NettyNioAsyncHttpClient instead of triggering the SDK's ServiceLoader scan avoids the classpath pollution problem at its root. The META-INF/services fake-registration test setup is a clean way to reproduce the Kafka Connect failure as a plain unit test. useDefaultHttpClient's Optional.isEmpty() logic is correct — SdkSystemSetting.getStringValue() returns Optional<String> with no default for SYNC_HTTP_SERVICE_IMPL/ASYNC_HTTP_SERVICE_IMPL, so isEmpty() correctly identifies "setting not configured."
One gap: the async escape hatch (ASYNC_HTTP_SERVICE_IMPL) has no test exercising the override path (see inline).
This review was generated by an AI agent, Hermès on behalf of @gnodet.
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 42 of 708 tested, 2 compile-only — current: 42 all testedMaveniverse Scalpel detected 42 affected modules (current approach: 42). Skip-tests mode would test 42 modules (2 direct + 0 downstream), skip tests for 2 (generated code, meta-modules) Modules Scalpel would test (42)
Modules with tests skipped (2)
Build reactor — dependencies compiled but only changed modules were tested (2 modules, 18.9s total)Total reactor time: 18.9s
Top 20 slowest modules:
|
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
TL;DR
Comment: the fix is right. Setting Apache5HttpClient / NettyNioAsyncHttpClient through httpClientBuilder() keeps what the SDK lookup did (the SDK still applies the service defaults with buildWithDefaults, so Kinesis HTTP/2 and the like keep working, and closes the client with the AWS client), and it no longer depends on every SdkHttpService registration on the classpath being loadable.
Blockers
- None
Other findings: 1 non-blocking (upgrade guide wording vs. clients built outside AwsClientBuilderUtil), 1 suggestion — see inline.
Thanks for the careful write-up and for a test that reproduces the Kafka Connect failure as a plain unit test with a foreign service registration; that makes the fix easy to verify.
I checked the 39 production callers of AwsClientBuilderUtil: none sets its own httpClient(...) in the service-specific callback, so the SDK's "httpClient and httpClientBuilder can't both be configured" check does not trip on the new default.
This review does not replace specialized review tools or static analysis.
Claude Code on behalf of davsclaus
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
|
Addressed review feedback:
Claude Code on behalf of oscerd |
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
TL;DR
Verdict: Thanks for the quick turnaround: the async test is in, and the 4.24 note now scopes the change to the clients built for endpoints. Two small things are left before merge.
Blockers
- The branch conflicts with main in
camel-4x-upgrade-guide-4_22.adoc(new 4.22.3 notes landed there), so CI has not run on bfdafdb; a rebase will trigger it.
Other findings: 1 non-blocking (the 4.22.3 note still says "they create", see the open gnodet-bot suggestion), 1 nit.
Status of my earlier points:
- Upgrade note scope: partially. Fixed in the 4.24 note; the same sentence in the 4.22.3 note (
camel-4x-upgrade-guide-4_22.adoc:326) still needs "for their endpoints". - Async test: fixed.
sdkAsyncSettingLeavesTheChoiceToTheSdkexercises theASYNC_HTTP_SERVICE_IMPLbranch, and the SDK error message does contain the class name.
Nit: the edited 4.24 line is 124 characters; reflowing it to the 120 columns of the rest of the guide would keep it tidy.
Reviewed with Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
…ad of the classpath lookup
AwsClientBuilderUtil set an HTTP client on the SDK client builder only
with a proxy or trustAllCertificates. Otherwise the AWS SDK looked the
client up with ServiceLoader, and that lookup fails as a whole when one
registration cannot be loaded, as when a Kafka Connect plugin sees the
apache-client registration of the worker's own AWS SDK
("ApacheSdkHttpService not a subtype").
Set Apache5HttpClient (sync) and NettyNioAsyncHttpClient (async), the
implementations the lookup selects from Camel's dependencies, unless the
AWS SDK setting software.amazon.awssdk.http.service.impl (or
software.amazon.awssdk.http.async.service.impl) names one.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
…nd clarify upgrade note - Add sdkAsyncSettingLeavesTheChoiceToTheSdk in AwsClientBuilderUtilTest - Clarify upgrade guide note to specify clients created for endpoints Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
…ated for endpoints The 4.22.3 note now says, as the 4.24 note does, that the camel-aws components set the HTTP client of the AWS SDK clients they create for their endpoints. The first paragraph of both notes is reflowed to 120 columns. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
bfdafdb to
acd5e11
Compare
|
@davsclaus thanks for the second look. Your points:
The PR description is updated to match (the async setting test, 8 tests, the scope of the notes). Claude Code on behalf of oscerd |
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier review — all findings have been addressed.
- Async escape-hatch test (
sdkAsyncSettingLeavesTheChoiceToTheSdk) — ✅ added and correct. - 4.24 upgrade note scope ("for their endpoints") — ✅ fixed in prior commit.
- 4.22.3 upgrade note scope ("for their endpoints") — ✅ fixed in this push (commit
acd5e11f). - Line reflow to 120 columns — ✅ both notes now stay within the limit.
- Merge conflict on
camel-4x-upgrade-guide-4_22.adoc— ✅ resolved by rebase.
The fix and tests are solid. useDefaultHttpClient() correctly gates on SdkSystemSetting so users who named a custom HTTP client via the system property are unaffected. The ForeignHttpService test setup cleanly reproduces the Kafka Connect failure as a plain unit test, and sdkLookupFailsOnTheForeignRegistration confirms the bug is observable without the fix.
LGTM.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Reviewed the changes since my last review: the points I raised are addressed, and CI is green on the current head.
Reviewed with Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
Jira: https://issues.apache.org/jira/browse/CAMEL-26343
Problem
Since CAMEL-23866 (4.22.0),
AwsClientBuilderUtilsets an HTTP client on the AWS SDK client builder only when a proxy ortrustAllCertificatesis configured. Otherwise the AWS SDK looks the HTTP client up withServiceLoader.ClasspathSdkHttpServiceProviderloads every registeredSdkHttpServicebefore ranking them, so one registration that cannot be loaded fails the whole lookup withServiceConfigurationError, even though theApache5SdkHttpServicethat Camel ships is there.This breaks the AWS connectors of Camel Kafka Connector 4.22.0 on
confluentinc/cp-kafka-connect:8.2.4(apache/camel-kafka-connector#1849). The worker classpath has its own AWS SDK with the classicapache-client. The plugin class loader also returns the worker'sMETA-INF/servicesfiles, soApacheSdkHttpService, which the plugin no longer ships, is loaded from the worker:software.amazon.awssdk.http.apache.ApacheSdkHttpService not a subtype.Change
AwsClientBuilderUtil.buildClientsetsApache5HttpClient.builder()andbuildAsyncClientsetsNettyNioAsyncHttpClient.builder(). These are the implementations the SDK lookup selects from Camel's dependencies. The SDK builds them with the same service defaults (buildWithDefaults) and closes them with the client, as it does for the client it looks up, so a working setup behaves as before.software.amazon.awssdk.http.service.impl(orsoftware.amazon.awssdk.http.async.service.impl) names an implementation, Camel still leaves the choice to the SDK.trustAllCertificatespaths are unchanged.camel-4x-upgrade-guide-4_24.adoc, and in the 4.22.3 section ofcamel-4x-upgrade-guide-4_22.adocfor the backport, for users who replacedapache5-client/netty-nio-clientwith another HTTP client. Both notes are scoped to the AWS SDK clients the components create for their endpoints (the clients built throughAwsClientBuilderUtil): SDK clients that other camel-aws code builds directly, such as the Secrets Manager and Parameter Store properties functions or the KCL consumer, still use the SDK lookup. The notes stay onmain; the backport takes only the code and test changes.Tests
AwsClientBuilderUtilTestregisters, in its test resources, a class that is not an HTTP client as anSdkHttpServiceand anSdkAsyncHttpService. This reproduces the Kafka Connect failure in a plain unit test:sdkLookupFailsOnTheForeignRegistration: a plain SDK builder fails withServiceConfigurationError ... not a subtype, so the setup reproduces the bug.buildClientDoesNotUseTheSdkLookup/buildAsyncClientDoesNotUseTheSdkLookup: clients built throughAwsClientBuilderUtilare created. Without the fix, both fail with the reported error.sdkSettingLeavesTheChoiceToTheSdk/sdkAsyncSettingLeavesTheChoiceToTheSdk: with the SDK system property for synchronous (or asynchronous) clients set, the SDK's own choice applies.The tests build STS clients, so
software.amazon.awssdk:stsis added as a test dependency ofcamel-aws-common, which had no AWS service module on its test classpath.Validation:
camel-aws-common: all 8 tests pass (5 inAwsClientBuilderUtilTest), also after the rebase on the currentmain. Onmainwithout the fix, the two client tests fail with the reported error.mvn verify -f components/camel-aws/pom.xmlon the first revision: the unit tests of all 35 camel-aws modules pass (3476 tests, 0 failures, 0 errors, 180 skipped).Backport
Draft backport to
camel-4.22.x(AWS SDK 2.50.2, identicalAwsClientBuilderUtil): #28564. It carries the code and test commit; the async setting test (sdkAsyncSettingLeavesTheChoiceToTheSdk), added here later, is still to be ported to it. The 4.22.3 upgrade note stays in this PR, onmain. All 35 camel-aws modules' unit tests also pass on that branch.Claude Code on behalf of Andrea Cosentino
🤖 Generated with Claude Code