Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

S3 Integration based on AWS SDK v2 #275

Merged
merged 38 commits into from
Apr 1, 2022
Merged

S3 Integration based on AWS SDK v2 #275

merged 38 commits into from
Apr 1, 2022

Conversation

maciejwalkowiak
Copy link
Contributor

@maciejwalkowiak maciejwalkowiak commented Mar 24, 2022 •
edited

  • auto-configuration for S3
  • cross-region S3 client
  • S3Resource and integration with ResourceLoader
  • writeable resource with disk buffering S3 OutputStream and option to provide custom output stream implementation
  • object metadata support
  • sample
  • Path-matching ResourcePatternResolver (perhaps we should skip it)

Fixes issues:

In subsequent PR:

@github-actions github-actions bot added the type: dependency-upgrade Dependency version bump label Mar 24, 2022
@maciejwalkowiak maciejwalkowiak added component: s3 S3 integration related issue type: feature Integration with a new AWS service or bigger change in existing integration and removed type: dependency-upgrade Dependency version bump labels Mar 24, 2022
@github-actions github-actions bot added the type: dependency-upgrade Dependency version bump label Mar 24, 2022
@maciejwalkowiak maciejwalkowiak mentioned this pull request Mar 25, 2022
Closed
@maciejwalkowiak maciejwalkowiak mentioned this pull request Mar 25, 2022
Closed
@maciejwalkowiak maciejwalkowiak marked this pull request as ready for review March 25, 2022 21:36
eddumelendez
eddumelendez suggested changes Mar 26, 2022
View reviewed changes
Makefile Outdated
@@ -4,7 +4,7 @@ format:
mvnd spring-javaformat:apply

build:
mvnd spring-javaformat:apply verify
mvnd spring-javaformat:apply verify spring-javaformat:apply
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why we need this again in the same command line?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First we format, then in verify phase code generation runs which can generate unformatted code, so that we need to format again.

Alternative would be to generate code in generated-sources but then, if generated file changes for whatever reason (like SDK update) its easy to miss it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Figured out a better way - class is now generated into a separate module - spring-cloud-aws-s3-cross-region which is not a subject of spring-formatting or checkstyle (as its not easy to generate class that matches these conventions).

pom.xml
@@ -59,9 +59,11 @@
<module>spring-cloud-aws-parameter-store</module>
<module>spring-cloud-aws-secrets-manager</module>
<module>spring-cloud-aws-ses</module>
<module>spring-cloud-aws-s3-parent</module>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the purpose of s3-parent module? should be only s3?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally we would. I've added parent only to wrap actual s3 module and another module for code generation. If we could add code generation to s3 module, without polluting s3 jar with classes responsible for code generation we could get rid of parent, but i haven't figured out how to do it. Suggestions welcome!

...-aws-autoconfigure/src/main/java/io/awspring/cloud/autoconfigure/s3/S3AutoConfiguration.java
Comment on lines +57 to +66
@Bean
@ConditionalOnMissingBean
S3ClientBuilder s3ClientBuilder(AwsCredentialsProvider credentialsProvider, AwsRegionProvider regionProvider) {
Region region = StringUtils.hasLength(this.properties.getRegion()) ? Region.of(this.properties.getRegion())
: regionProvider.getRegion();
S3ClientBuilder builder = S3Client.builder().credentialsProvider(credentialsProvider).region(region)
.overrideConfiguration(SpringCloudClientConfiguration.clientOverrideConfiguration())
.serviceConfiguration(s3ServiceConfiguration());
Optional.ofNullable(this.properties.getEndpoint()).ifPresent(builder::endpointOverride);
return builder;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in order to keep consistency with other modules should we move it to the s3Client? I think in the future we can add customizer for all clients. wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With s3ClientBuilder as a bean, we give an easy way to configure how clients are created in CrossRegionS3Client.

Users still can provide their own s3Client and then builder bean is not used. This means that from the bean override point of view, we are consistent with other integrations.

...-aws-autoconfigure/src/main/java/io/awspring/cloud/autoconfigure/s3/S3AutoConfiguration.java Outdated Show resolved Hide resolved
...g-cloud-aws-autoconfigure/src/main/java/io/awspring/cloud/autoconfigure/s3/S3Properties.java
}

public Boolean getPathStyleAccessEnabled() {
return pathStyleAccessEnabled;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return pathStyleAccessEnabled;
return this.pathStyleAccessEnabled;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a convention we used neither in 2.x nor 3.x. We can discuss (at this point i don't see value) but if so we would enforce it in checkstyle.

...g-cloud-aws-autoconfigure/src/main/java/io/awspring/cloud/autoconfigure/s3/S3Properties.java
}

public Boolean getChecksumValidationEnabled() {
return checksumValidationEnabled;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return checksumValidationEnabled;
return this.checksumValidationEnabled;

...g-cloud-aws-autoconfigure/src/main/java/io/awspring/cloud/autoconfigure/s3/S3Properties.java
}

public Boolean getChunkedEncodingEnabled() {
return chunkedEncodingEnabled;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return chunkedEncodingEnabled;
return this.chunkedEncodingEnabled;

...g-cloud-aws-autoconfigure/src/main/java/io/awspring/cloud/autoconfigure/s3/S3Properties.java
}

public Boolean getDualstackEnabled() {
return dualstackEnabled;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return dualstackEnabled;
return this.dualstackEnabled;

...g-cloud-aws-autoconfigure/src/main/java/io/awspring/cloud/autoconfigure/s3/S3Properties.java
}

public Boolean getUseArnRegionEnabled() {
return useArnRegionEnabled;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return useArnRegionEnabled;
return this.useArnRegionEnabled;

...-aws-autoconfigure/src/main/java/io/awspring/cloud/autoconfigure/s3/S3AutoConfiguration.java Show resolved Hide resolved
@maciejwalkowiak maciejwalkowiak mentioned this pull request Mar 31, 2022
Closed
9 tasks
@maciejwalkowiak maciejwalkowiak merged commit ac85823 into 3.0.x Apr 1, 2022
@maciejwalkowiak maciejwalkowiak deleted the 3.0.x-s3 branch April 1, 2022 10:10
@maciejwalkowiak maciejwalkowiak added this to the 3.0.x milestone Apr 1, 2022
This was referenced Apr 2, 2022
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eddumelendez eddumelendez eddumelendez requested changes

@MatejNedic MatejNedic Awaiting requested review from MatejNedic MatejNedic is a code owner

Assignees
No one assigned
Labels
component: s3 S3 integration related issue type: dependency-upgrade Dependency version bump type: feature Integration with a new AWS service or bigger change in existing integration
Projects
None yet
Milestone
3.0.0 M1
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@maciejwalkowiak @eddumelendez