FrankChen021 commented on code in PR #20540:
URL: https://github.com/apache/druid/pull/20540#discussion_r4336984507
##########
extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/ServerSideEncryptingAmazonS3.java:
##########
@@ -411,6 +413,15 @@ public static ServerSideEncryptingAmazonS3.Builder builder(
// Apply assume role if configured
if (!Strings.isNullOrEmpty(assumeRoleArn)) {
+ if (s3InputSourceConfig == null ||
!s3InputSourceConfig.isCredentialsConfigured()) {
+ credentialsProvider = maybeAssumeIntermediateRole(
Review Comment:
[P2] Intermediate role is skipped without a final role
**Finding:** The intermediate provider is created only inside the
assumeRoleArn guard. An S3 export with no final assumeRoleArn therefore returns
the injected default client and never assumes
druid.s3.intermediateAssumeRoleArn, contradicting the updated MSQ documentation
and potentially using the base identity outside the intended trust boundary.
**Suggestion:** Apply the intermediate provider whenever its ARN is
configured, then optionally wrap it with the final-role provider; ensure the
export provider builds that chain even without a target ARN.
##########
extensions-core/s3-extensions/src/main/java/org/apache/druid/data/input/s3/S3InputSource.java:
##########
@@ -129,7 +129,9 @@ public S3InputSource(
awsEndpointConfig,
awsClientConfig,
s3InputSourceConfig,
- null
+ null,
+ s3ClientBuilder.getIntermediateRoleConfig(),
+ s3ClientBuilder.getIntermediateRoleEndpointConfig()
Review Comment:
[P1] Source endpoint region is not forwarded to intermediate STS
**Finding:** These arguments pass only the injected builder's process-level
endpoint configuration to the intermediate-role provider, while the input
source's own endpointConfig is used for the S3 client and final role. If an
input spec supplies endpointConfig.signingRegion without a global signing
region or AWS_REGION/aws.region, the intermediate STS client has no region and
role resolution fails before the final role.
**Suggestion:** Pass the effective source endpoint configuration, or fall
back from the process-level configuration to the source-level signing region,
and cover this override case with a test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
