Basic s3 signed URL implementation #620

Belair34 · 2023-01-11T21:10:04Z

📢 Type of change

Bugfix
New feature
Enhancement
Refactoring

📜 Description

Added two methods to S3Operations and thus S3Template that simplify creating signed urls for getting/putting objects in S3.

💡 Motivation and Context

It simplifies the creation of signed URLs for S3 into one function call each.

#318

💚 How did you test it?

So far I've manually tested it by doing ./mvnw install and then importing 3.0.0-SNAPSHOT from my local repository in a fresh spring project.

📝 Checklist

I reviewed submitted code
I added tests to verify changes
I updated reference documentation to reflect the change
All tests passing
No breaking changes

🔮 Next steps

I'm not 100% sure this is what the issue was requesting, so please let me know! I also don't think the way I have the S3 presigner is ideal, but I'm unsure how I should set it up. I imagine it should be configured like the S3Client, but I was unable to figure out how that works exactly and could use some advice. If any of this seems promising, I will go ahead and add tests and what not.

maciejwalkowiak

Thanks a lot @Belair34! I left few comments. Make sure to add integration tests for that. I think Localstack supports signed urls with S3.

...cloud-aws-s3-parent/spring-cloud-aws-s3/src/main/java/io/awspring/cloud/s3/S3Operations.java

...g-cloud-aws-s3-parent/spring-cloud-aws-s3/src/main/java/io/awspring/cloud/s3/S3Template.java

Belair34 · 2023-01-12T20:36:25Z

@maciejwalkowiak I updated everything based on your comments! I do have one concern: I can't get all of the tests to pass even when I'm on the up-to-date main branch. It always fails at ParameterStoreConfigDataLoaderIntegrationTests.whenKeysAreNotSpecifiedFailsWithHumanReadableFailureMessage. I'm on Windows 10 using Java 17 for the record.
Also, about docs: do I need to add anything besides the javadocs in S3Operations.java?

maciejwalkowiak

Thanks for an update. I fixed the compilation issue. There are few more things needed - if you find it unclear please let me know!

...-aws-autoconfigure/src/main/java/io/awspring/cloud/autoconfigure/s3/S3AutoConfiguration.java

...cloud-aws-s3-parent/spring-cloud-aws-s3/src/main/java/io/awspring/cloud/s3/S3Operations.java

...g-cloud-aws-s3-parent/spring-cloud-aws-s3/src/main/java/io/awspring/cloud/s3/S3Template.java

maciejwalkowiak · 2023-01-12T23:08:46Z

...arent/spring-cloud-aws-s3/src/test/java/io/awspring/cloud/s3/S3TemplateIntegrationTests.java

+		HttpResponse response = httpClient.execute(httpPut);
+		httpClient.close();
+
+		HeadObjectResponse headObjectResponse = client.headObject(HeadObjectRequest.builder()


Genuine question - shouldn't we upload something in this test?

I guess it technically isn't uploading a local file, but it is adding a new file "file.txt" to the bucket with the request body as its contents and then checking to make sure it is there at the end. Personally, I think knowing this works is enough to be confident that the signed URL itself works. The difference is just about how you use it. That's just my interpretation, though. I'm open to other opinions!

maciejwalkowiak · 2023-01-12T23:10:57Z

Regarding ParameterStoreConfigDataLoaderIntegrationTests - make sure you are up to date with main branch and just in case run ./mvnw clean - this test used to be flaky but should not be any longer.

Belair34 · 2023-01-13T16:38:35Z

Everything should be ready for review again! By the way, I found out the tests were failing for me because of lines like this:

spring-cloud-aws/spring-cloud-aws-autoconfigure/src/test/java/io/awspring/cloud/autoconfigure/config/parameterstore/ParameterStoreConfigDataLoaderIntegrationTests.java

Lines 118 to 120 in 4d9da9a

    
           	assertThat(output.getOut()) 
        
           			.contains("Description:\n" + "\n" + "Could not import properties from AWS Parameter Store"); 
        
           }

Since I'm on Windows all my new lines were '\r\n' instead of '\n' so the assertions were failing. I temporarily replaced these '\n' with System.lineSeparator() and all of my tests passed.

maciejwalkowiak

Thanks for an update @Belair34. I left few comments we also need to update the reference docs (look s3.adoc). If you have any questions let me know!

...-aws-autoconfigure/src/main/java/io/awspring/cloud/autoconfigure/s3/S3AutoConfiguration.java

Belair34 · 2023-01-21T02:06:01Z

@maciejwalkowiak I tried to make ConfiguredS3Presigner, and based on my evaluation of their internals it seemed like I didn't really need to change anything besides names. It worked perfectly for the first test I added regarding endpoints, but my second test gets null for the common aws properties. I'm really not sure why this is. Any ideas? I could also use some recommendations on which properties to test if this seems like the path forward.

maciejwalkowiak · 2023-01-21T11:45:17Z

@Belair34 I fixed the test and did some final polishing.

maciejwalkowiak · 2023-01-21T11:45:41Z

@MatejNedic if you have time please review.

MatejNedic

I have a few questions regarding what should be returned but otherwise looks good!

MatejNedic · 2023-01-21T12:22:24Z

...cloud-aws-s3-parent/spring-cloud-aws-s3/src/main/java/io/awspring/cloud/s3/S3Operations.java

+	 * @param duration - duration that the URL will work
+	 * @return a {@link URL} representing the signed URL
+	 */
+	URL createSignedGetURL(String bucketName, String key, Duration duration);


Should we return signedHeaders, SignedPayload, and isBrowserExecutable as well here?

Since in AWS docs for signedHeaders -> Returns the subset of headers that were signed, and MUST be included in the presigned request to prevent the request from failing.

~~Now I am quite confident we need an end to end test for that.~~ createsWorkingSignedPutURL tests the whole flow.

MatejNedic · 2023-01-21T12:22:25Z

...cloud-aws-s3-parent/spring-cloud-aws-s3/src/main/java/io/awspring/cloud/s3/S3Operations.java

+	 * @param duration - duration that the URL will work
+	 * @return a {@link URL} representing the signed URL
+	 */
+	URL createSignedPutURL(String bucketName, String key, Duration duration, @Nullable ObjectMetadata metadata,


Should we return signedHeaders, SignedPayload, and isBrowserExecutable as well here?

Basic s3 signed URL implementation

f024a70

github-actions bot added the component: s3 S3 integration related issue label Jan 11, 2023

maciejwalkowiak reviewed Jan 12, 2023

View reviewed changes

...cloud-aws-s3-parent/spring-cloud-aws-s3/src/main/java/io/awspring/cloud/s3/S3Operations.java Outdated Show resolved Hide resolved

...cloud-aws-s3-parent/spring-cloud-aws-s3/src/main/java/io/awspring/cloud/s3/S3Operations.java Outdated Show resolved Hide resolved

maciejwalkowiak reviewed Jan 12, 2023

View reviewed changes

...g-cloud-aws-s3-parent/spring-cloud-aws-s3/src/main/java/io/awspring/cloud/s3/S3Template.java Outdated Show resolved Hide resolved

Belair34 and others added 4 commits January 12, 2023 10:34

Improved signed URL methods and added presigner to autoconfiguration

daed133

Implemented signed url test with httpurlconnection

4acccf7

Switched to apachehttpclient for integration tests

17a374a

Merge branch 'main' into S3SignedUrls

e9f264c

Belair34 marked this pull request as ready for review January 12, 2023 20:26

Belair34 requested review from MatejNedic and tomazfernandes as code owners January 12, 2023 20:26

Fix compilation issue

10fe8ba

maciejwalkowiak requested changes Jan 12, 2023

View reviewed changes

Belair34 and others added 3 commits January 13, 2023 09:01

Merge branch 'awspring:main' into S3SignedUrls

bec7b6b

added providers and such to S3Presigner builder

c7bc9b8

Added default method and reformatted a bit

c7869f1

maciejwalkowiak requested changes Jan 15, 2023

View reviewed changes

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

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

Belair34 added 2 commits January 16, 2023 09:48

Allow for global or s3 endpoint override

6180d1e

Updated docs

72cdc8b

github-actions bot added the type: documentation Documentation or Samples related issue label Jan 16, 2023

Belair34 and others added 2 commits January 20, 2023 18:30

Merge branch 'awspring:main' into S3SignedUrls

9c92498

Started autoconfiguration tests

8be3eb0

maciejwalkowiak added 3 commits January 21, 2023 12:35

Polish tests

7faec98

Format code

457ba6d

Polish docs

19d274b

maciejwalkowiak approved these changes Jan 21, 2023

View reviewed changes

MatejNedic requested changes Jan 21, 2023

View reviewed changes

maciejwalkowiak merged commit 77a7080 into awspring:main Jan 23, 2023

maciejwalkowiak added this to the 3.0.0 RC1 milestone Jan 23, 2023

maciejwalkowiak mentioned this pull request Jan 23, 2023

Auto Configure S3Presigner #603

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basic s3 signed URL implementation #620

Basic s3 signed URL implementation #620

Belair34 commented Jan 11, 2023 •

edited

Loading

maciejwalkowiak left a comment

Belair34 commented Jan 12, 2023

maciejwalkowiak left a comment

maciejwalkowiak Jan 12, 2023

Belair34 Jan 13, 2023

maciejwalkowiak commented Jan 12, 2023

Belair34 commented Jan 13, 2023

maciejwalkowiak left a comment

Belair34 commented Jan 21, 2023

maciejwalkowiak commented Jan 21, 2023

maciejwalkowiak commented Jan 21, 2023

MatejNedic left a comment •

edited

Loading

MatejNedic Jan 21, 2023

maciejwalkowiak Jan 21, 2023 •

edited

Loading

MatejNedic Jan 21, 2023

Basic s3 signed URL implementation #620

Basic s3 signed URL implementation #620

Conversation

Belair34 commented Jan 11, 2023 • edited Loading

📢 Type of change

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

maciejwalkowiak left a comment

Choose a reason for hiding this comment

Belair34 commented Jan 12, 2023

maciejwalkowiak left a comment

Choose a reason for hiding this comment

maciejwalkowiak Jan 12, 2023

Choose a reason for hiding this comment

Belair34 Jan 13, 2023

Choose a reason for hiding this comment

maciejwalkowiak commented Jan 12, 2023

Belair34 commented Jan 13, 2023

maciejwalkowiak left a comment

Choose a reason for hiding this comment

Belair34 commented Jan 21, 2023

maciejwalkowiak commented Jan 21, 2023

maciejwalkowiak commented Jan 21, 2023

MatejNedic left a comment • edited Loading

Choose a reason for hiding this comment

MatejNedic Jan 21, 2023

Choose a reason for hiding this comment

maciejwalkowiak Jan 21, 2023 • edited Loading

Choose a reason for hiding this comment

MatejNedic Jan 21, 2023

Choose a reason for hiding this comment

Belair34 commented Jan 11, 2023 •

edited

Loading

MatejNedic left a comment •

edited

Loading

maciejwalkowiak Jan 21, 2023 •

edited

Loading