[fix][io] Fix DynamoDB source client construction with a custom endpoint#79
Conversation
All three client builders set an endpoint configuration and then, in a separate branch, a region. AWS SDK v1 rejects that combination with 'Only one of Region or EndpointConfiguration may be set', and DynamoDBSource.open() requires awsRegion, so every configuration that supplies an endpoint failed at client construction. The endpoint path could not have worked. Make region and endpoint mutually exclusive. Also gate the DynamoDB and CloudWatch builders on their own endpoint fields rather than on awsEndpoint: they were reading dynamoEndpoint and cloudwatchEndpoint while gating on awsEndpoint, so setting awsEndpoint alone built an endpoint configuration from an empty string.
There was a problem hiding this comment.
Pull request overview
Fixes DynamoDB connector AWS SDK v1 client construction when custom endpoints are configured by ensuring region and endpoint configuration are not both set on the same builder, and by gating each client’s endpoint configuration on the correct endpoint field.
Changes:
- Make
setEndpointConfiguration(...)andsetRegion(...)mutually exclusive (else if) for DynamoDB Streams, DynamoDB, and CloudWatch client builders. - Fix endpoint gating so
buildDynamoDBClientusesdynamoEndpointandbuildCloudwatchClientusescloudwatchEndpoint(instead of incorrectly gating onawsEndpoint).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // EndpointConfiguration already carries the signing region, and the AWS SDK v1 forbids | ||
| // setting both an endpoint configuration and a region, so they must be mutually exclusive. | ||
| if (!this.getAwsEndpoint().isEmpty()) { | ||
| builder.setEndpointConfiguration( | ||
| new AwsClientBuilder.EndpointConfiguration(this.getAwsEndpoint(), this.getAwsRegion())); |
…ndling Cover the crash directly: constructing each client with an endpoint and the (mandatory) region must not throw, each builder must select its own endpoint field, and setting awsEndpoint alone must not leak an empty endpoint into the DynamoDB and CloudWatch clients. Three of the four fail against the unfixed builders with the original IllegalStateException; the fourth pins the unaffected region-only path.
|
Good catch — a fix for a user-facing crash with no test guarding it is how this bug survived in the first place. Added
Verified they actually catch the bug rather than just passing alongside the fix. Reverting
With the fix restored, the full |
The regression tests added in the previous commit were committed alongside an accidental revert of DynamoDBSourceConfig, and correctly failed CI with the original IllegalStateException.
Fixes #78
Motivation
The DynamoDB source's
awsEndpoint/dynamoEndpoint/cloudwatchEndpointsettings are unusable. Any configuration supplying an endpoint fails at client construction:All three builders in
DynamoDBSourceConfigset an endpoint configuration and then, in a separate non-elsebranch, a region:AWS SDK v1's
AwsClientBuilderforbids that pairing — theEndpointConfigurationalready carries the signing region. AndawsRegionis mandatory:DynamoDBSource.open()enforcescheckArgument(isNotBlank(getAwsRegion()), "The aws-region must be set"). So whenever an endpoint is configured, both setters run andbuild()throws. The endpoint path could never have worked.It went unnoticed because production usage against real AWS leaves the endpoint empty, and the module had no test beyond config parsing (#50). Master carries a comment that reads like a previous encounter with this, resolved at the wrong layer:
Modifications
else if) in all three builders.buildDynamoDBClientandbuildCloudwatchClienton their own endpoint fields. They readdynamoEndpoint/cloudwatchEndpointwhile gating onawsEndpoint, so settingawsEndpointalone built anEndpointConfigurationfrom an empty string.Compatibility
I do not believe any working behavior is removed:
awsEndpointalone, for the DynamoDB/CloudWatch clients → previously built an endpoint configuration from an empty string; now falls through to the region-based default endpoint.awsEndpointanddynamoEndpointset → unchanged.Verifying this change
Existing config tests pass:
The fix is exercised end-to-end by the LocalStack integration test in #77, which cannot run without it — that PR is now test-only and depends on this one.