amaechler commented on code in PR #20540:
URL: https://github.com/apache/druid/pull/20540#discussion_r4285845758
##########
extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/ServerSideEncryptingAmazonS3.java:
##########
@@ -411,6 +412,12 @@ public static ServerSideEncryptingAmazonS3.Builder builder(
// Apply assume role if configured
if (!Strings.isNullOrEmpty(assumeRoleArn)) {
+ credentialsProvider = maybeAssumeIntermediateRole(
+ credentialsProvider,
+ intermediateRoleConfig,
+ awsClientConfig
+ );
Review Comment:
Suggestion for the spec-credentials thread on line 415: take the
intermediate hop only when the spec doesn't supply its own keys.
```suggestion
if (s3InputSourceConfig == null ||
!s3InputSourceConfig.isCredentialsConfigured()) {
credentialsProvider = maybeAssumeIntermediateRole(
credentialsProvider,
intermediateRoleConfig,
awsClientConfig
);
}
```
##########
extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/ServerSideEncryptingAmazonS3.java:
##########
@@ -461,6 +469,30 @@ private static StaticCredentialsProvider
createStaticCredentialsProvider(S3Input
}
}
+ /**
+ * Assumes an intermediate role if one is configured.
+ */
+ public static AwsCredentialsProvider maybeAssumeIntermediateRole(
Review Comment:
nit: only called from `builder()`, so it can be `private`.
```suggestion
private static AwsCredentialsProvider maybeAssumeIntermediateRole(
```
##########
extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/ServerSideEncryptingAmazonS3.java:
##########
@@ -411,6 +412,13 @@ public static ServerSideEncryptingAmazonS3.Builder builder(
// Apply assume role if configured
if (!Strings.isNullOrEmpty(assumeRoleArn)) {
+ credentialsProvider = maybeAssumeIntermediateRole(
Review Comment:
My snippet only gates the intermediate hop, not `assumeRoleArn`. A spec with
its own keys would keep using them to assume `assumeRoleArn` directly, the same
as on master, so it's the variant without a behaviour change.
On why only the intermediate role fails: for a spec that works today with
its own keys, the role at `assumeRoleArn` already lets the principal behind
those keys assume it. The intermediate role is set up to trust the cluster's
credentials. On this branch the spec's keys sign the intermediate AssumeRole
call (its STS client is backed by a `StaticCredentialsProvider` holding them),
and
[AssumeRole](https://docs.aws.amazon.com/STS/latest/APIReference/API_AssumeRole.html)
only succeeds for principals the role's trust policy allows. So an existing
spec with its own keys starts failing as soon as an operator sets
`druid.s3.intermediateAssumeRoleArn`, unless the intermediate role's trust
policy also admits those keys.
I left the change as a suggestion on lines 415-419, with a test and a docs
line. `input-sources.md` already describes this behaviour, since the new
sentence there sits under "If `accessKeyId` and `secretAccessKey` are not
given".
##########
extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/ServerSideEncryptingAmazonS3.java:
##########
@@ -461,6 +469,31 @@ private static StaticCredentialsProvider
createStaticCredentialsProvider(S3Input
}
}
+ /**
+ * Assumes an intermediate role if one is configured.
+ */
+ public static AwsCredentialsProvider maybeAssumeIntermediateRole(
+ AwsCredentialsProvider baseCredentialsProvider,
+ @Nullable S3IntermediateRoleConfig intermediateRoleConfig,
+ @Nullable AWSEndpointConfig awsEndpointConfig,
+ @Nullable AWSClientConfig awsClientConfig
+ )
+ {
+ final String intermediateAssumeRoleArn =
+ intermediateRoleConfig == null ? null :
intermediateRoleConfig.getIntermediateAssumeRoleArn();
+ if (Strings.isNullOrEmpty(intermediateAssumeRoleArn)) {
+ return baseCredentialsProvider;
+ }
+
+ return createAssumeRoleCredentialsProvider(
+ intermediateAssumeRoleArn,
+ null,
+ awsEndpointConfig,
Review Comment:
Both comments are right about different cases, so neither `null` nor the
spec's `endpointConfig` works everywhere. The intermediate role belongs to the
operator, so I think that hop should use the process's own region:
`druid.s3.endpoint.signingRegion` when set, otherwise the SDK default chain.
- Not the spec's region: per the [AWS
docs](https://docs.aws.amazon.com/IAM/latest/UserGuide/id_credentials_temp_enable-regions.html#sts-regions-activate-deactivate),
an AssumeRole call to the STS endpoint of a manually enabled (opt-in) Region
needs that Region enabled in the account that contains the role. With the
spec's region, an ingestion from a bucket in an opt-in Region that the
intermediate role's account hasn't enabled would fail at the first hop.
- Not plain `null`: I reproduced the case from the other thread on this
branch. With no `AWS_REGION`, `aws.region`, profile or IMDS region and
`druid.s3.endpoint.signingRegion` set, both an `S3InputSource` whose spec sets
`endpointConfig.signingRegion` and an `S3ExportStorageProvider` work without
`intermediateAssumeRoleArn` and throw `SdkClientException: Unable to load
region from any of the providers in the chain` once it is set.
This is what I tried locally, on top of the suggestion on lines 415-419.
`S3ExportStorageProvider` already injects the process `AWSEndpointConfig`, and
`S3InputSource` gets it from the injected `Builder`, the same way as
`getIntermediateRoleConfig()`:
```diff
diff --git
a/extensions-core/s3-extensions/src/main/java/org/apache/druid/data/input/s3/S3InputSource.java
b/extensions-core/s3-extensions/src/main/java/org/apache/druid/data/input/s3/S3InputSource.java
index fc996f9853..9164c9bad2 100644
---
a/extensions-core/s3-extensions/src/main/java/org/apache/druid/data/input/s3/S3InputSource.java
+++
b/extensions-core/s3-extensions/src/main/java/org/apache/druid/data/input/s3/S3InputSource.java
@@ -131,5 +131,6 @@ public class S3InputSource extends CloudObjectInputSource
s3InputSourceConfig,
null,
- s3ClientBuilder.getIntermediateRoleConfig()
+ s3ClientBuilder.getIntermediateRoleConfig(),
+ s3ClientBuilder.getIntermediateRoleEndpointConfig()
).build();
});
diff --git
a/extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/S3StorageDruidModule.java
b/extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/S3StorageDruidModule.java
index 47352a3e6d..8ada5b5016 100644
---
a/extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/S3StorageDruidModule.java
+++
b/extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/S3StorageDruidModule.java
@@ -153,5 +153,6 @@ public class S3StorageDruidModule implements DruidModule
null,
null,
- intermediateRoleConfig
+ intermediateRoleConfig,
+ endpointConfig
);
}
diff --git
a/extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/ServerSideEncryptingAmazonS3.java
b/extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/ServerSideEncryptingAmazonS3.java
index 6cdcab631c..bf5da9c2c2 100644
---
a/extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/ServerSideEncryptingAmazonS3.java
+++
b/extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/ServerSideEncryptingAmazonS3.java
@@ -324,5 +324,6 @@ public class ServerSideEncryptingAmazonS3
@Nullable S3InputSourceConfig s3InputSourceConfig,
@Nullable S3ExportStorageProvider s3ExportStorageProvider,
- @Nullable S3IntermediateRoleConfig intermediateRoleConfig
+ @Nullable S3IntermediateRoleConfig intermediateRoleConfig,
+ @Nullable AWSEndpointConfig intermediateRoleEndpointConfig
)
{
@@ -417,4 +418,5 @@ public class ServerSideEncryptingAmazonS3
credentialsProvider,
intermediateRoleConfig,
+ intermediateRoleEndpointConfig,
awsClientConfig
);
@@ -438,5 +440,6 @@ public class ServerSideEncryptingAmazonS3
.setS3AsyncClientSupplier(asyncClientBuilder::build)
.setS3StorageConfig(s3StorageConfig)
-
.setIntermediateRoleConfig(intermediateRoleConfig);
+
.setIntermediateRoleConfig(intermediateRoleConfig)
+
.setIntermediateRoleEndpointConfig(intermediateRoleEndpointConfig);
}
@@ -478,4 +481,5 @@ public class ServerSideEncryptingAmazonS3
AwsCredentialsProvider baseCredentialsProvider,
@Nullable S3IntermediateRoleConfig intermediateRoleConfig,
+ @Nullable AWSEndpointConfig intermediateRoleEndpointConfig,
@Nullable AWSClientConfig awsClientConfig
)
@@ -490,5 +494,5 @@ public class ServerSideEncryptingAmazonS3
intermediateAssumeRoleArn,
null,
- null,
+ intermediateRoleEndpointConfig,
baseCredentialsProvider,
awsClientConfig
@@ -537,4 +541,6 @@ public class ServerSideEncryptingAmazonS3
@Nullable
private S3IntermediateRoleConfig intermediateRoleConfig;
+ @Nullable
+ private AWSEndpointConfig intermediateRoleEndpointConfig;
public Builder setS3ClientSupplier(Supplier<S3Client> s3ClientSupplier)
@@ -573,4 +579,16 @@ public class ServerSideEncryptingAmazonS3
}
+ public Builder setIntermediateRoleEndpointConfig(@Nullable
AWSEndpointConfig intermediateRoleEndpointConfig)
+ {
+ this.intermediateRoleEndpointConfig = intermediateRoleEndpointConfig;
+ return this;
+ }
+
+ @Nullable
+ public AWSEndpointConfig getIntermediateRoleEndpointConfig()
+ {
+ return this.intermediateRoleEndpointConfig;
+ }
+
/**
* Builds a new {@link ServerSideEncryptingAmazonS3} instance.
diff --git
a/extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/output/S3ExportStorageProvider.java
b/extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/output/S3ExportStorageProvider.java
index 48f4194dd2..566d06c869 100644
---
a/extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/output/S3ExportStorageProvider.java
+++
b/extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/output/S3ExportStorageProvider.java
@@ -119,5 +119,6 @@ public class S3ExportStorageProvider implements
ExportStorageProvider
null,
this,
- intermediateRoleConfig
+ intermediateRoleConfig,
+ awsEndpointConfig
).build();
});
```
The three `builder(...)` calls in tests then need one more `null`, and
`S3InputSourceTest` needs a `getIntermediateRoleEndpointConfig()` stub next to
each `getIntermediateRoleConfig()` stub. Passing a second endpoint config
(`eu-south-1`) in `testIntermediateRoleIsAssumedBeforeTheRoleTheSpecNames` pins
both regions:
```java
Assertions.assertEquals(Region.US_EAST_1,
targetStsClient.serviceClientConfiguration().region());
Assertions.assertEquals(Region.EU_SOUTH_1, ((StsClient)
stsClientField.get(intermediate)).serviceClientConfiguration().region());
```
With that, the S3 extension tests pass, and in the same no-region setup the
intermediate hop goes to the `druid.s3.endpoint.signingRegion` Region while the
target hop keeps the spec's. A process with no region anywhere except in a spec
still fails, but the docs already require `aws.region` or `AWS_REGION`, and a
spec without `signingRegion` fails on such a process today too.
##########
docs/development/extensions-core/s3.md:
##########
@@ -81,6 +81,24 @@ For more information, refer to the [Amazon Developer
Guide](https://docs.aws.ama
Alternatively, you can bypass this chain by specifying an access key and
secret key using a [Properties
Object](../../ingestion/input-sources.md#s3-input-source) inside your ingestion
specification.
+Whichever credentials are resolved are then used to assume any `assumeRoleArn`
named by an ingestion spec or export destination. Setting
`druid.s3.intermediateAssumeRoleArn` inserts one role in between: the resolved
credentials assume that role first, and the resulting credentials assume
`assumeRoleArn`.
Review Comment:
With the suggestion on `ServerSideEncryptingAmazonS3` lines 415-419, keys
from the spec skip the intermediate role, so this paragraph could say so.
```suggestion
Whichever credentials are resolved are then used to assume any
`assumeRoleArn` named by an ingestion spec or export destination. Setting
`druid.s3.intermediateAssumeRoleArn` inserts one role in between: the resolved
credentials assume that role first, and the resulting credentials assume
`assumeRoleArn`. Keys given in an ingestion spec's Properties Object skip the
intermediate role and assume `assumeRoleArn` directly.
```
##########
extensions-core/s3-extensions/src/test/java/org/apache/druid/storage/s3/ServerSideEncryptingAmazonS3Test.java:
##########
@@ -282,13 +305,91 @@ public RetryStrategy getRetryStrategy()
null,
clientConfig,
null,
+ null,
null
);
Assertions.assertEquals(2, issued.size(), "one strategy per client, sync
and async");
Assertions.assertNotSame(issued.get(0), issued.get(1));
}
+ @Test
+ public void testIntermediateRoleIsAssumedBeforeTheRoleTheSpecNames() throws
Exception
Review Comment:
I left a suggested test for this on line 317.
##########
extensions-core/s3-extensions/src/test/java/org/apache/druid/storage/s3/ServerSideEncryptingAmazonS3Test.java:
##########
@@ -282,13 +305,91 @@ public RetryStrategy getRetryStrategy()
null,
clientConfig,
null,
+ null,
null
);
Assertions.assertEquals(2, issued.size(), "one strategy per client, sync
and async");
Assertions.assertNotSame(issued.get(0), issued.get(1));
}
+ @Test
+ public void testIntermediateRoleIsAssumedBeforeTheRoleTheSpecNames() throws
Exception
Review Comment:
A test for spec-supplied keys plus `assumeRoleArn`. It fails on the current
branch (`expected: <false> but was: <true>`) and passes with the suggestion on
`ServerSideEncryptingAmazonS3` lines 415-419.
```suggestion
public void testSpecCredentialsAssumeTheRoleTheSpecNamesDirectly() throws
Exception
{
final ServerSideEncryptingAmazonS3 s3 =
ServerSideEncryptingAmazonS3.builder(
AnonymousCredentialsProvider.create(),
new S3StorageConfig(new NoopServerSideEncryption(), new
S3TransferConfig()),
null,
REGION_CONFIG,
new AWSClientConfig(),
new S3InputSourceConfig(() -> "specAccessKey", () ->
"specSecretKey", TARGET_ROLE_ARN, null, null),
null,
new S3IntermediateRoleConfig(INTERMEDIATE_ROLE_ARN)
).build();
final StsAssumeRoleCredentialsProvider target = assumeRoleProvider(
s3.getS3Client().serviceClientConfiguration().credentialsProvider(),
"the S3 client should assume a role"
);
final Field stsClientField =
StsCredentialsProvider.class.getDeclaredField("stsClient");
stsClientField.setAccessible(true);
final StsClient targetStsClient = (StsClient) stsClientField.get(target);
try {
Assertions.assertEquals(TARGET_ROLE_ARN, roleArnRequestedBy(target));
Assertions.assertFalse(
targetStsClient.serviceClientConfiguration().credentialsProvider()
instanceof StsAssumeRoleCredentialsProvider,
"the spec's own credentials assume the spec's role without the
intermediate role"
);
Assertions.assertEquals(
"specAccessKey",
targetStsClient.serviceClientConfiguration().credentialsProvider().resolveIdentity().join().accessKeyId()
);
}
finally {
target.close();
}
}
@Test
public void testIntermediateRoleIsAssumedBeforeTheRoleTheSpecNames()
throws Exception
```
--
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]
