AmazonS3InstallationService should accept aws credential in constructor

[smallufo]

https://slack.dev/java-slack-sdk/guides/app-distribution

// The standard AWS env variables are expected
// export AWS_REGION=us-east-1
// export AWS_ACCESS_KEY_ID=AAAA*************
// export AWS_SECRET_ACCESS_KEY=4o7***********************

// Please be careful about the security policies on this bucket.
String awsS3BucketName = "YOUR_OWN_BUCKET_NAME_HERE";

InstallationService installationService = new AmazonS3InstallationService(awsS3BucketName);

I realize you utilize DefaultAWSCredentialsProviderChain.getInstance().getCredentials() to retrieve credentials from env. But it truly sucks (lacks flexibility)

You should provide a constructor that accepts these 3 parameters in constructor : awsAccessKey , awsSecretKey , region , and build AWS credential in the constructor.

[filmaj]

Thanks for the feature suggestion! We would love a pull request for this addition if you think it is something you can contribute 😄

[smallufo]

BTW , it seems AWS_REGION , AWS_ACCESS_KEY_ID , AWS_SECRET_ACCESS_KEY is not sufficient.
I set 3 values , and I see error message :

2023-06-11 18:22:52.407 [http-nio-8080-exec-1] WARN  c.a.i.InstanceMetadataServiceResourceFetcher - Fail to retrieve token 
com.amazonaws.SdkClientException: Failed to connect to service endpoint: 
	at com.amazonaws.internal.EC2ResourceFetcher.doReadResource(EC2ResourceFetcher.java:112)
	at com.amazonaws.internal.InstanceMetadataServiceResourceFetcher.getToken(InstanceMetadataServiceResourceFetcher.java:91)
	at com.amazonaws.internal.InstanceMetadataServiceResourceFetcher.readResource(InstanceMetadataServiceResourceFetcher.java:69)
	at com.amazonaws.internal.EC2ResourceFetcher.readResource(EC2ResourceFetcher.java:66)
	at com.amazonaws.util.EC2MetadataUtils.getItems(EC2MetadataUtils.java:407)
	at com.amazonaws.util.EC2MetadataUtils.getData(EC2MetadataUtils.java:376)
	at com.amazonaws.util.EC2MetadataUtils.getData(EC2MetadataUtils.java:372)
	at com.amazonaws.util.EC2MetadataUtils.getEC2InstanceRegion(EC2MetadataUtils.java:287)
	at com.amazonaws.regions.InstanceMetadataRegionProvider.tryDetectRegion(InstanceMetadataRegionProvider.java:59)
	at com.amazonaws.regions.InstanceMetadataRegionProvider.getRegion(InstanceMetadataRegionProvider.java:50)
	at com.amazonaws.regions.AwsRegionProviderChain.getRegion(AwsRegionProviderChain.java:46)
	at com.amazonaws.client.builder.AwsClientBuilder.determineRegionFromRegionProvider(AwsClientBuilder.java:475)
	at com.amazonaws.client.builder.AwsClientBuilder.setRegion(AwsClientBuilder.java:458)
	at com.amazonaws.client.builder.AwsClientBuilder.configureMutableProperties(AwsClientBuilder.java:424)
	at com.amazonaws.client.builder.AwsSyncClientBuilder.build(AwsSyncClientBuilder.java:46)
	at com.amazonaws.services.s3.AmazonS3ClientBuilder.defaultClient(AmazonS3ClientBuilder.java:54)
	at com.slack.api.bolt.service.builtin.AmazonS3OAuthStateService.createS3Client(AmazonS3OAuthStateService.java:91)
	at com.slack.api.bolt.service.builtin.AmazonS3OAuthStateService.lambda$initializer$0(AmazonS3OAuthStateService.java:43)
	at com.slack.api.bolt.App.initialize(App.java:544)
	at com.slack.api.bolt.App.start(App.java:557)
	at com.slack.api.bolt.App.run(App.java:588)

After digging into s3's code , it seems it needs additional 3 system properties : aws.region , aws.accessKeyId , aws.secretKey
( see SystemPropertiesCredentialsProvider : https://github.com/aws/aws-sdk-java/blob/master/aws-java-sdk-core/src/main/java/com/amazonaws/auth/SystemPropertiesCredentialsProvider.java )

Anyway , I don't suggest using DefaultAWSCredentialsProviderChain as it assumes your server is running on AWS's image.

After I set the 3 values : aws.region , aws.accessKeyId , aws.secretKey . When event comes , bolt output :

INFO  c.s.a.b.m.b.MultiTeamsAuthorization - auth.test result: null, io error: null, api error: null

No stacktrace :(
And I see no folder created under my s3 bucket .

I think maybe the AWS storage didn't run test outside AWS's image ?

While using FileInstallationService(config, "/tmp") , it correctly creates folders under /tmp/xxx

/tmp/xxx
total 0
drwxr-xr-x   4 smallufo  wheel  128  6 11 00:29 .
drwxrwxrwt  15 root      wheel  480  6 11 17:02 ..
drwxr-xr-x   4 smallufo  wheel  128  6 10 20:26 installation
drwxr-xr-x   6 smallufo  wheel  192  6 11 17:29 state

[seratch]

Hey @smallufo

BTW , it seems AWS_REGION , AWS_ACCESS_KEY_ID , AWS_SECRET_ACCESS_KEY is not sufficient.

I don't think so. At least, this is not always true. From my past experience, the default resolver works very well with those env variables. As long as you provide sufficient env variables, Java AWS SDK works for you without any issues.

I don't have time to discuss details right now, but I recommend checking https://docs.aws.amazon.com/AWSJavaSDK/latest/javadoc/com/amazonaws/auth/DefaultAWSCredentialsProviderChain.html The default credential resolver tries to read env variables first, then Java system properties, and so on. Also, I believe that it's the best recommended way to resolve credentials on AWS infra (and even on your local machine).

No stacktrace :( And I see no folder created under my s3 bucket .

What about enabling slf4j debug-level logging? The debug logs by this SDK and AWS SDK may provide more detailed information, which can be helpful for you.

Lastly, as for your PR to add a new constructor to add a new way to initialize the credentials, I am open to add such in addition to the current default one. But I still don't agree the current approach is not recommended. I will check your PR next week. Thanks again for your inputs and contribution.

[bruceadowns]

This issue/pr seems misguided and stale. The DefaultCredentialsProvider provider chain as-is the canonical way of specifying AWS credentials.

If yet more flexibility is needed, a potentially better solution is exposing AwsCredentialsProvider.

i.e.

public AmazonS3InstallationService(String bucketName, AwsCredentialsProvider credentialsProvider) {
    this.bucketName = bucketName;
    this.credentialsProvider = credentialsProvider;
}

[seratch]

let me close this issue in favor of #1418