Fix loss loss of cross region bucket read config for s3 client in the v2 migration#19180
Fix loss loss of cross region bucket read config for s3 client in the v2 migration#19180gianm merged 3 commits intoapache:masterfrom
Conversation
|
Adjacent to #19178. Needed for D37 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| public void testDefaultCrossRegionAccessEnabled() throws Exception | ||
| { | ||
| AWSClientConfig config = MAPPER.readValue("{}", AWSClientConfig.class); | ||
| Assertions.assertNull(config.isForceGlobalBucketAccessEnabled()); |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation Note test
| public void testCrossRegionAccessEnabledExplicitlySet() throws Exception | ||
| { | ||
| AWSClientConfig config = MAPPER.readValue("{\"crossRegionAccessEnabled\": true}", AWSClientConfig.class); | ||
| Assertions.assertNull(config.isForceGlobalBucketAccessEnabled()); |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation Note test
| "{\"crossRegionAccessEnabled\": true}", | ||
| AWSClientConfig.class | ||
| ); | ||
| Assertions.assertNull(config.isForceGlobalBucketAccessEnabled()); |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation Note test
| S3StorageConfig storageConfig | ||
| ) | ||
| { | ||
| if (clientConfig.isForceGlobalBucketAccessEnabled() != null) { |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation Note
|
|
||
| public boolean isCrossRegionAccessEnabled() | ||
| { | ||
| if (forceGlobalBucketAccessEnabled != null) { |
There was a problem hiding this comment.
crossRegionAccessEnabled, if set, should take precedence over forceGlobalBucketAccessEnabled because the latter is deprecated.
| "protocol='" + protocol + '\'' + | ||
| ", disableChunkedEncoding=" + disableChunkedEncoding + | ||
| ", enablePathStyleAccess=" + enablePathStyleAccess + | ||
| ", forceGlobalBucketAccessEnabled=" + forceGlobalBucketAccessEnabled + |
There was a problem hiding this comment.
Should include forceGlobalBucketAccessEnabled, even though it's deprecated it still exists for now.
| |`druid.s3.enablePathStyleAccess`|Enables path style access. See [AWS document](https://docs.aws.amazon.com/AWSJavaSDK/latest/javadoc/com/amazonaws/services/s3/AmazonS3Builder.html#enablePathStyleAccess--) for details.|false| | ||
| |`druid.s3.forceGlobalBucketAccessEnabled`|Enables global bucket access. See [AWS document](https://docs.aws.amazon.com/AWSJavaSDK/latest/javadoc/com/amazonaws/services/s3/AmazonS3Builder.html#setForceGlobalBucketAccessEnabled-java.lang.Boolean-) for details.|false| | ||
| |`druid.s3.crossRegionAccessEnabled`|Enables cross-region access for S3 requests. When enabled, the S3 client automatically detects the correct region for a bucket on first access and caches it for subsequent requests.|false| | ||
| |`druid.s3.forceGlobalBucketAccessEnabled`|**Deprecated.** Use `druid.s3.crossRegionAccessEnabled` instead. If explicitly set, this takes precedence over `crossRegionAccessEnabled`.|null| |
There was a problem hiding this comment.
"this takes precedence" <- mentioned this elsewhere but IMO these should be flipped.
|
@gianm ty for pointing out the backwards logic. I think the latest will meet the desired order |
2 participants
Description
Release note
note: this should go with the s3 sdk upgrade release note/upgrade note section. this is not a breaking change for Druid 37, but eventually it could be if we drop the legacy config in a future release.
For operators: If you currently set druid.s3.forceGlobalBucketAccessEnabled in your runtime properties, please migrate to druid.s3.crossRegionAccessEnabled at your earliest convenience. The legacy property will continue to work
and takes precedence if set, but it will be removed in a future release.
Key changed/added classes in this PR
AwsClientConigThis PR has: