eclipse-hawkbit / hawkbit-extensions

Eclipse Public License 2.0
20 stars 30 forks source link

feat: allow overwrite aws region through application.properties #26

Closed subhadippramanik closed 6 years ago

subhadippramanik commented 6 years ago

Problem Statement: File upload fails if S3 bucket is not created in eu-central-1

Root Cause: Application not allowing to set region

Solution: AWS region is by default eu-central-1. With application property aws.region this can be over written.

Signed-off-by: Subhadip Pramanik pramaniksubhadip@gmail.com

saumilsdk commented 6 years ago

Please merge this changes as this is much required change. @schabdo can you please merge?

schabdo commented 6 years ago

@stefbehl may I have a look?

stefbehl commented 6 years ago

That would be great! Thanks!

saumilsdk commented 6 years ago

I gave it a tried and got success on getting file uploaded to S3 bucket which I created in being in us-east-1 region. @subhadippramanik I think bucket is independent of region as its shared across. No need for specifying region for that.

I am not sure for what other thing hawkbit will require region to override. But will be good if any feature is dependent on it.

subhadippramanik commented 6 years ago

@saumilsdk you are right, a bucket on S3 is global, but the AWS API used in s3-extension instantiates AWSClient with eu-central-1 and then throws exception if the bucket is in different region. This I tried with 0.2.0M9 of hawkbit-core and 0.2.0M3 of s3-extension.

saumilsdk commented 6 years ago

hi @subhadippramanik if overriding region help in fixing this issue then definitely there must be some dependency in the region.

schabdo commented 6 years ago

@subhadippramanik thanks for figuring this out! According to the documentation this shouldn't be an issue since the region will be set dynamically and can be influenced by a pre-defined property aws.region. Exactly what you did manually 👍. With the environment variable AWS_REGION it is working properly.

After a while I found out that Amazon added the AwsSystemPropertyRegionProvider in a newer version than we're currently using. But I didn't manage to update the version of the S3 SDK. Even with the latest BOM version it remains at 1.11.18

schabdo commented 6 years ago

Successfully tested your pull request, works great! However may I ask you for a small change before we merge? I'm not sure how newer versions of the aws-sdk will handle an empty string as region. Better safe than sorry: Could you set the region only if it present? Maybe something like this?

@Value("${aws.region:#{null}}")
private Optional<String> region;

@Bean
@ConditionalOnMissingBean
public AmazonS3 amazonS3() {

  AmazonS3ClientBuilder builder = AmazonS3ClientBuilder.standard()
      .withCredentials(awsCredentialsProvider())
      .withClientConfiguration(awsClientConfiguration());

  if (region.isPresent()) {
     builder = builder.withRegion(region.get());
  }

  return builder.build();
}

Please also move the local variable above the JavaDoc comment of awsCredentialsProvider() method.

subhadippramanik commented 6 years ago

Thanks @schabdo for the review. I have pushed the recommended changes. Please check and merge if it works.

schabdo commented 6 years ago

Awesome, thanks for taking time to contribute!