Add scheme to destination region endpoint #1572
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1572 +/- ##
============================================
+ Coverage 75.39% 75.55% +0.15%
Complexity 638 638
============================================
Files 896 896
Lines 28049 28064 +15
Branches 2203 2214 +11
============================================
+ Hits 21148 21203 +55
+ Misses 5889 5844 -45
- Partials 1012 1017 +5
Continue to review full report at Codecov.
|
| return Ec2Client.serviceMetadata().endpointFor(region); | ||
| URI endpoint = Ec2Client.serviceMetadata().endpointFor(region); | ||
| if (endpoint.getScheme() == null) { | ||
| return URI.create("https://" + endpoint); |
There was a problem hiding this comment.
Can we pull the client configured protocol in an interceptor?
There was a problem hiding this comment.
I considered this, but given that this URL is used by EC2 and not consumed by the customer/client, it doesn't really make sense to be anything other than HTTPS. It's also the same behavior as V1 https://github.com/aws/aws-sdk-java/blob/7b1e5b87b0bf03456df9e77716b14731adf9a7a7/aws-java-sdk-ec2/src/main/java/com/amazonaws/services/ec2/model/transform/GeneratePreSignUrlRequestHandler.java#L145-L147
There was a problem hiding this comment.
+1 on pulling client configured protocol. Customers might use this against a mock server (maybe for testing purpose) which only supports HTTP. I think we should honor the customer configuration.
There was a problem hiding this comment.
I'm not sure I understand the use case. The presigned URL is explicitly used by the EC2 service to presumably pull the snapshot for cross region copying, it doesn't affect where or how the actual HTTP request being modified is sent.
There was a problem hiding this comment.
I meant there might be some mockEc2 server library available for customers to test their applications. The mockEc2 server might have a similar copySnapshot function to mimic the real service's behavior. This use case might be rare or doesn't exist at all, but I still think it'd be better to honor the customer configuration.
There was a problem hiding this comment.
I disagree that it's important in this case; we've had the same behavior in v1 for a long time without issue, the same with RDS. Technically the protocol configured on the client specifies the protocol it speaks to service we're calling the API on anyway, I don't think it necessarily governs things like the presigned URL(s) it generates.
IMO it's also a potential dangerous if the customer happens to being using HTTP and now their EBS snapshots are also being transferred in plaintext.
|
Kudos, SonarCloud Quality Gate passed!
|
Description
Fixes #1564
Motivation and Context
Bug fix.
Testing
New unit test, integ tests.
Screenshots (if appropriate)
Types of changes
Checklist
mvn installsucceedsLicense