You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2022/04/03 16:37:56 UTC
[GitHub] [druid] cryptoe commented on a change in pull request #12339: Make AWS WebIdentityToken actually working and usable from inside EKS.
cryptoe commented on a change in pull request #12339:
URL: https://github.com/apache/druid/pull/12339#discussion_r841241916
##########
File path: cloud/aws-common/pom.xml
##########
@@ -46,6 +46,10 @@
<groupId>com.amazonaws</groupId>
<artifactId>aws-java-sdk-s3</artifactId>
</dependency>
+ <dependency>
Review comment:
Is this required? I do not see this package being used anywhere in aws-common
##########
File path: extensions-core/s3-extensions/src/main/java/org/apache/druid/data/input/s3/S3InputSourceConfig.java
##########
@@ -90,6 +91,24 @@ public boolean isCredentialsConfigured()
secretAccessKey != null;
}
+ @JsonIgnore
+ public boolean isAssumeRoleArnConfigured()
+ {
+ return assumeRoleArn != null && !assumeRoleArn.trim().isEmpty();
Review comment:
I donot think a trim is required here and below.
If a user sets a wrong property its a wrong property even if its just a space at the start or the end.
##########
File path: extensions-core/s3-extensions/src/main/java/org/apache/druid/data/input/s3/S3InputSourceConfig.java
##########
@@ -90,6 +91,24 @@ public boolean isCredentialsConfigured()
secretAccessKey != null;
}
+ @JsonIgnore
+ public boolean isAssumeRoleArnConfigured()
+ {
+ return assumeRoleArn != null && !assumeRoleArn.trim().isEmpty();
+ }
+
+ @JsonIgnore
+ public boolean isAssumeRoleArnEnvConfigured()
+ {
+ return !System.getenv(SDKGlobalConfiguration.AWS_ROLE_ARN_ENV_VAR).trim().isEmpty();
Review comment:
This can throw NPE no ?
##########
File path: extensions-core/s3-extensions/src/main/java/org/apache/druid/data/input/s3/S3InputSource.java
##########
@@ -166,15 +175,21 @@ private void applyAssumeRole(
AWSCredentialsProvider awsCredentialsProvider
)
{
- String assumeRoleArn = s3InputSourceConfig.getAssumeRoleArn();
- if (assumeRoleArn != null) {
+ // Do not run if WebIdentityToken file and assumeRole ARN are detected from the environment variable,
+ // we want the default s3ClientBuilder behavior for ServiceAccount + eks.amazonaws.com/role-arn annotation to work.
+ if (s3InputSourceConfig.isWebIdentityTokenEnvConfigured() && s3InputSourceConfig.isAssumeRoleArnEnvConfigured()) {
Review comment:
Isnt just this condition required and we can remove all the refactoring from 108:132 ?
This is purely based on boolean logic.
##########
File path: extensions-core/s3-extensions/src/main/java/org/apache/druid/data/input/s3/S3InputSourceConfig.java
##########
@@ -90,6 +91,24 @@ public boolean isCredentialsConfigured()
secretAccessKey != null;
}
+ @JsonIgnore
+ public boolean isAssumeRoleArnConfigured()
+ {
+ return assumeRoleArn != null && !assumeRoleArn.trim().isEmpty();
+ }
+
+ @JsonIgnore
+ public boolean isAssumeRoleArnEnvConfigured()
+ {
+ return !System.getenv(SDKGlobalConfiguration.AWS_ROLE_ARN_ENV_VAR).trim().isEmpty();
+ }
+
+ @JsonIgnore
+ public boolean isWebIdentityTokenEnvConfigured()
+ {
+ return !System.getenv(SDKGlobalConfiguration.AWS_WEB_IDENTITY_ENV_VAR).trim().isEmpty();
Review comment:
NPE here to ?
##########
File path: extensions-core/s3-extensions/src/main/java/org/apache/druid/data/input/s3/S3InputSource.java
##########
@@ -166,15 +175,21 @@ private void applyAssumeRole(
AWSCredentialsProvider awsCredentialsProvider
)
{
- String assumeRoleArn = s3InputSourceConfig.getAssumeRoleArn();
- if (assumeRoleArn != null) {
+ // Do not run if WebIdentityToken file and assumeRole ARN are detected from the environment variable,
+ // we want the default s3ClientBuilder behavior for ServiceAccount + eks.amazonaws.com/role-arn annotation to work.
Review comment:
Can you please tell us how did you arrive at the above conclusion. Link to the relevant AWS documentation would be helpful.
##########
File path: pom.xml
##########
@@ -1835,6 +1835,8 @@
<exclude>.editorconfig</exclude>
<exclude>**/hadoop.indexer.libs.version</exclude>
<exclude>**/codegen/**</exclude>
+ <exclude>website/target/**</exclude>
Review comment:
This looks like a setup issue to me.
Please remove these excludes.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org