Skip to content

Commit 240b92f

Browse files
zoewanggzoewangg
authored
Fixed an issue where port number was not added to host header. (#3724)
1 parent 9b2e969 commit 240b92f

File tree

5 files changed

+76
-17
lines changed

5 files changed

+76
-17
lines changed

‎.changes/next-release/feature-AWSCRTbasedS3Client-dcd8e15.json

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
{
2+
"type": "bugfix",
3+
"category": "AWS CRT-based S3 Client",
4+
"contributor": "",
5+
"description": "Fixed an issue where port number was not added to host header. See [#3721](https://github.com/aws/aws-sdk-java-v2/issues/3721) and [#3682](https://github.com/aws/aws-sdk-java-v2/issues/3682)."
6+
}

‎pom.xml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,7 @@
114114
<rxjava.version>2.2.21</rxjava.version>
115115
<commons-codec.verion>1.10</commons-codec.verion>
116116
<jmh.version>1.29</jmh.version>
117-
<awscrt.version>0.20.3</awscrt.version>
117+
<awscrt.version>0.21.3</awscrt.version>
118118

119119
<!--Test dependencies -->
120120
<junit5.version>5.8.1</junit5.version>

‎services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/crt/S3CrtAsyncHttpClient.java

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@
4444
import software.amazon.awssdk.http.async.SdkAsyncHttpClient;
4545
import software.amazon.awssdk.utils.AttributeMap;
4646
import software.amazon.awssdk.utils.Logger;
47+
import software.amazon.awssdk.utils.http.SdkHttpUtils;
4748

4849
/**
4950
* An implementation of {@link SdkAsyncHttpClient} that uses an CRT S3 HTTP client {@link S3Client} to communicate with S3.
@@ -86,7 +87,7 @@ private S3CrtAsyncHttpClient(Builder builder) {
8687
public CompletableFuture<Void> execute(AsyncExecuteRequest asyncRequest) {
8788
CompletableFuture<Void> executeFuture = new CompletableFuture<>();
8889
URI uri = asyncRequest.request().getUri();
89-
HttpRequest httpRequest = toCrtRequest(uri, asyncRequest);
90+
HttpRequest httpRequest = toCrtRequest(asyncRequest);
9091
S3CrtResponseHandlerAdapter responseHandler =
9192
new S3CrtResponseHandlerAdapter(executeFuture, asyncRequest.responseHandler());
9293

@@ -97,12 +98,13 @@ public CompletableFuture<Void> execute(AsyncExecuteRequest asyncRequest) {
9798

9899
ChecksumConfig checksumConfig =
99100
checksumConfig(httpChecksum, requestType, s3NativeClientConfiguration.checksumValidationEnabled());
101+
URI endpoint = getEndpoint(uri);
100102

101103
S3MetaRequestOptions requestOptions = new S3MetaRequestOptions()
102104
.withHttpRequest(httpRequest)
103105
.withMetaRequestType(requestType)
104106
.withChecksumConfig(checksumConfig)
105-
.withEndpoint(getEndpoint(uri))
107+
.withEndpoint(endpoint)
106108
.withResponseHandler(responseHandler)
107109
.withResumeToken(resumeToken);
108110

@@ -156,7 +158,7 @@ private static void addCancelCallback(CompletableFuture<Void> executeFuture,
156158
});
157159
}
158160

159-
private static HttpRequest toCrtRequest(URI uri, AsyncExecuteRequest asyncRequest) {
161+
private static HttpRequest toCrtRequest(AsyncExecuteRequest asyncRequest) {
160162
SdkHttpRequest sdkRequest = asyncRequest.request();
161163

162164
String method = sdkRequest.method().name();
@@ -169,7 +171,7 @@ private static HttpRequest toCrtRequest(URI uri, AsyncExecuteRequest asyncReques
169171
.map(value -> "?" + value)
170172
.orElse("");
171173

172-
HttpHeader[] crtHeaderArray = createHttpHeaderList(uri, asyncRequest).toArray(new HttpHeader[0]);
174+
HttpHeader[] crtHeaderArray = createHttpHeaderList(asyncRequest).toArray(new HttpHeader[0]);
173175

174176
S3CrtRequestBodyStreamAdapter sdkToCrtRequestPublisher =
175177
new S3CrtRequestBodyStreamAdapter(asyncRequest.requestContentPublisher());
@@ -207,13 +209,14 @@ public SdkAsyncHttpClient buildWithDefaults(AttributeMap serviceDefaults) {
207209
}
208210
}
209211

210-
private static List<HttpHeader> createHttpHeaderList(URI uri, AsyncExecuteRequest asyncRequest) {
212+
private static List<HttpHeader> createHttpHeaderList(AsyncExecuteRequest asyncRequest) {
211213
SdkHttpRequest sdkRequest = asyncRequest.request();
212214
List<HttpHeader> crtHeaderList = new ArrayList<>();
213215

214216
// Set Host Header if needed
215217
if (!sdkRequest.firstMatchingHeader(Header.HOST).isPresent()) {
216-
crtHeaderList.add(new HttpHeader(Header.HOST, uri.getHost()));
218+
String hostHeader = getHostHeaderValue(asyncRequest.request());
219+
crtHeaderList.add(new HttpHeader(Header.HOST, hostHeader));
217220
}
218221

219222
// Set Content-Length if needed
@@ -228,4 +231,10 @@ private static List<HttpHeader> createHttpHeaderList(URI uri, AsyncExecuteReques
228231

229232
return crtHeaderList;
230233
}
234+
235+
private static String getHostHeaderValue(SdkHttpRequest request) {
236+
return SdkHttpUtils.isUsingStandardPort(request.protocol(), request.port())
237+
? request.host()
238+
: request.host() + ":" + request.port();
239+
}
231240
}

‎services/s3/src/test/java/software/amazon/awssdk/services/s3/functionaltests/GetBucketPolicyFunctionalTest.java

Lines changed: 28 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -27,15 +27,16 @@
2727
import org.junit.Test;
2828
import software.amazon.awssdk.auth.credentials.AwsBasicCredentials;
2929
import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider;
30+
import software.amazon.awssdk.crt.Log;
3031
import software.amazon.awssdk.regions.Region;
3132
import software.amazon.awssdk.services.s3.S3AsyncClient;
3233
import software.amazon.awssdk.services.s3.S3AsyncClientBuilder;
3334
import software.amazon.awssdk.services.s3.S3Client;
3435
import software.amazon.awssdk.services.s3.S3ClientBuilder;
36+
import software.amazon.awssdk.services.s3.S3CrtAsyncClientBuilder;
3537
import software.amazon.awssdk.services.s3.model.GetBucketPolicyResponse;
3638

3739
public class GetBucketPolicyFunctionalTest {
38-
private static final URI HTTP_LOCALHOST_URI = URI.create("http://localhost:8080/");
3940
private static final String EXAMPLE_BUCKET = "Example-Bucket";
4041
private static final String EXAMPLE_POLICY =
4142
"{\"Version\":\"2012-10-17\",\"Id\":\"Policy1234\","
@@ -44,21 +45,34 @@ public class GetBucketPolicyFunctionalTest {
4445
+ "\"Resource\":\"arn:aws:s3:::dummy-resource/*\"}]}";
4546

4647
@Rule
47-
public WireMockRule wireMock = new WireMockRule();
48+
public WireMockRule wireMock = new WireMockRule(0);
49+
50+
private URI endpoint() {
51+
return URI.create("http://localhost:" + wireMock.port());
52+
}
4853

4954
private S3ClientBuilder getSyncClientBuilder() {
5055

5156
return S3Client.builder()
5257
.region(Region.US_EAST_1)
53-
.endpointOverride(HTTP_LOCALHOST_URI)
58+
.endpointOverride(endpoint())
5459
.credentialsProvider(
5560
StaticCredentialsProvider.create(AwsBasicCredentials.create("key", "secret")));
5661
}
5762

5863
private S3AsyncClientBuilder getAsyncClientBuilder() {
5964
return S3AsyncClient.builder()
6065
.region(Region.US_EAST_1)
61-
.endpointOverride(HTTP_LOCALHOST_URI)
66+
.endpointOverride(endpoint())
67+
.credentialsProvider(
68+
StaticCredentialsProvider.create(AwsBasicCredentials.create("key", "secret")));
69+
70+
}
71+
72+
private S3CrtAsyncClientBuilder getS3CrtAsyncClientBuilder() {
73+
return S3AsyncClient.crtBuilder()
74+
.region(Region.US_EAST_1)
75+
.endpointOverride(endpoint())
6276
.credentialsProvider(
6377
StaticCredentialsProvider.create(AwsBasicCredentials.create("key", "secret")));
6478

@@ -83,4 +97,14 @@ public void getBucketPolicy_asyncClient() {
8397
GetBucketPolicyResponse response = s3Client.getBucketPolicy(r -> r.bucket(EXAMPLE_BUCKET)).join();
8498
assertThat(response.policy()).isEqualTo(EXAMPLE_POLICY);
8599
}
100+
101+
@Test
102+
public void getBucketPolicy_crtClient() {
103+
stubFor(any(anyUrl()).willReturn(aResponse().withStatus(200).withBody(EXAMPLE_POLICY)));
104+
105+
try(S3AsyncClient s3Client = getS3CrtAsyncClientBuilder().build()) {
106+
GetBucketPolicyResponse response = s3Client.getBucketPolicy(r -> r.bucket(EXAMPLE_BUCKET)).join();
107+
assertThat(response.policy()).isEqualTo(EXAMPLE_POLICY);
108+
}
109+
}
86110
}

‎services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/crt/S3CrtAsyncHttpClientTest.java

Lines changed: 26 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -27,8 +27,13 @@
2727
import java.util.HashMap;
2828
import java.util.Map;
2929
import java.util.concurrent.CompletableFuture;
30+
import java.util.stream.Stream;
31+
import org.assertj.core.internal.Integers;
3032
import org.junit.jupiter.api.BeforeEach;
3133
import org.junit.jupiter.api.Test;
34+
import org.junit.jupiter.params.ParameterizedTest;
35+
import org.junit.jupiter.params.provider.MethodSource;
36+
import org.junit.jupiter.params.provider.ValueSource;
3237
import org.mockito.ArgumentCaptor;
3338
import org.mockito.Mockito;
3439
import software.amazon.awssdk.core.interceptor.trait.HttpChecksum;
@@ -42,6 +47,7 @@
4247
import software.amazon.awssdk.http.async.AsyncExecuteRequest;
4348
import software.amazon.awssdk.http.async.SdkAsyncHttpResponseHandler;
4449
import software.amazon.awssdk.http.async.SdkHttpContentPublisher;
50+
import software.amazon.awssdk.services.s3.S3AsyncClient;
4551

4652
public class S3CrtAsyncHttpClientTest {
4753
private static final URI DEFAULT_ENDPOINT = URI.create("https://127.0.0.1:443");
@@ -69,14 +75,23 @@ public void methodSetup() {
6975
asyncHttpClient = new S3CrtAsyncHttpClient(s3Client, s3NativeClientConfiguration);
7076
}
7177

72-
@Test
73-
public void defaultRequest_shouldSetMetaRequestOptionsCorrectly() {
74-
AsyncExecuteRequest asyncExecuteRequest = getExecuteRequestBuilder().build();
78+
private static Stream<Integer> ports() {
79+
return Stream.of(null, 1234, 443);
80+
}
81+
82+
@ParameterizedTest
83+
@MethodSource("ports")
84+
public void defaultRequest_shouldSetMetaRequestOptionsCorrectly(Integer port) {
85+
AsyncExecuteRequest asyncExecuteRequest = getExecuteRequestBuilder(port).build();
7586

7687
S3MetaRequestOptions actual = makeRequest(asyncExecuteRequest);
7788
assertThat(actual.getMetaRequestType()).isEqualTo(S3MetaRequestOptions.MetaRequestType.DEFAULT);
7889
assertThat(actual.getCredentialsProvider()).isNull();
79-
assertThat(actual.getEndpoint()).isEqualTo(URI.create(DEFAULT_ENDPOINT.getScheme() + "://" + DEFAULT_ENDPOINT.getHost()));
90+
String expectedEndpoint = port == null || port.equals(443) ?
91+
DEFAULT_ENDPOINT.getScheme() + "://" + DEFAULT_ENDPOINT.getHost() :
92+
DEFAULT_ENDPOINT.getScheme() + "://" + DEFAULT_ENDPOINT.getHost() + ":" + port;
93+
assertThat(actual.getEndpoint()).hasToString(expectedEndpoint);
94+
8095

8196
HttpRequest httpRequest = actual.getHttpRequest();
8297
assertThat(httpRequest.getEncodedPath()).isEqualTo("/key");
@@ -86,8 +101,9 @@ public void defaultRequest_shouldSetMetaRequestOptionsCorrectly() {
86101
.collect(HashMap::new, (m, h) -> m.put(h.getName(), h.getValue())
87102
, Map::putAll);
88103

104+
String expectedPort = port == null || port.equals(443) ? "" : ":" + port;
89105
assertThat(headers).hasSize(4)
90-
.containsEntry("Host", DEFAULT_ENDPOINT.getHost())
106+
.containsEntry("Host", DEFAULT_ENDPOINT.getHost() + expectedPort)
91107
.containsEntry("custom-header", "foobar")
92108
.containsEntry("amz-sdk-invocation-id", "1234")
93109
.containsEntry("Content-Length", "100");
@@ -293,14 +309,18 @@ public void closeHttpClient_shouldCloseUnderlyingResources() {
293309
}
294310

295311
private AsyncExecuteRequest.Builder getExecuteRequestBuilder() {
312+
return getExecuteRequestBuilder(443);
313+
}
314+
315+
private AsyncExecuteRequest.Builder getExecuteRequestBuilder(Integer port) {
296316
return AsyncExecuteRequest.builder()
297317
.responseHandler(responseHandler)
298318
.requestContentPublisher(contentPublisher)
299319
.request(SdkHttpRequest.builder()
300320
.protocol(DEFAULT_ENDPOINT.getScheme())
301321
.method(SdkHttpMethod.GET)
302322
.host(DEFAULT_ENDPOINT.getHost())
303-
.port(DEFAULT_ENDPOINT.getPort())
323+
.port(port)
304324
.encodedPath("/key")
305325
.putHeader(CONTENT_LENGTH, "100")
306326
.putHeader("amz-sdk-invocation-id",

0 commit comments

Comments
 (0)