From 4abfae7e505e5cbcf965164c3805394f07093a16 Mon Sep 17 00:00:00 2001 From: Nick Tindall Date: Tue, 29 Apr 2025 12:40:03 +1000 Subject: [PATCH 1/7] Default S3 endpoint scheme to HTTPS when not specified --- .../repositories/s3/S3Service.java | 8 +++++++- .../repositories/s3/S3ServiceTests.java | 19 +++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Service.java b/modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Service.java index e4b774e414b9a..0bda78f5386ac 100644 --- a/modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Service.java +++ b/modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Service.java @@ -254,7 +254,13 @@ protected S3ClientBuilder buildClientBuilder(S3ClientSettings clientSettings, Sd } if (Strings.hasLength(clientSettings.endpoint)) { - s3clientBuilder.endpointOverride(URI.create(clientSettings.endpoint)); + String endpoint = clientSettings.endpoint; + if ((endpoint.startsWith("http://") || endpoint.startsWith("https://")) == false) { + // Default protocol to https if not specified + // See https://docs.aws.amazon.com/sdk-for-java/latest/developer-guide/client-configuration.html#client-config-other-diffs + endpoint = "https://" + endpoint; + } + s3clientBuilder.endpointOverride(URI.create(endpoint)); } return s3clientBuilder; diff --git a/modules/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3ServiceTests.java b/modules/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3ServiceTests.java index f240432679b4e..8e4590576354d 100644 --- a/modules/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3ServiceTests.java +++ b/modules/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3ServiceTests.java @@ -12,7 +12,9 @@ import software.amazon.awssdk.awscore.exception.AwsServiceException; import software.amazon.awssdk.core.retry.RetryPolicyContext; import software.amazon.awssdk.core.retry.conditions.RetryCondition; +import software.amazon.awssdk.http.SdkHttpClient; import software.amazon.awssdk.regions.Region; +import software.amazon.awssdk.services.s3.S3Client; import software.amazon.awssdk.services.s3.endpoints.S3EndpointParams; import software.amazon.awssdk.services.s3.endpoints.internal.DefaultS3EndpointProvider; import software.amazon.awssdk.services.s3.model.S3Exception; @@ -29,8 +31,10 @@ import org.elasticsearch.watcher.ResourceWatcherService; import java.io.IOException; +import java.net.URI; import java.util.concurrent.atomic.AtomicBoolean; +import static org.hamcrest.Matchers.equalTo; import static org.mockito.Mockito.mock; public class S3ServiceTests extends ESTestCase { @@ -217,4 +221,19 @@ public void testGetClientRegionFallbackToUsEast1() { ); } } + + public void testEndpointOverrideSchemeDefaultsToHttpsWhenNotSpecified() { + final S3Service s3Service = new S3Service( + mock(Environment.class), + Settings.EMPTY, + mock(ResourceWatcherService.class), + () -> Region.of("es-test-region") + ); + String servername = randomIdentifier() + ".ignore"; + S3Client s3Client = s3Service.buildClient( + S3ClientSettings.getClientSettings(Settings.builder().put("s3.client.test-client.endpoint", servername).build(), "test-client"), + mock(SdkHttpClient.class) + ); + assertThat(s3Client.serviceClientConfiguration().endpointOverride().get(), equalTo(URI.create("https://" + servername))); + } } From 3f238d1fadc7cdb3e3ecc5ec9a17f5899f8ebf26 Mon Sep 17 00:00:00 2001 From: Nick Tindall Date: Tue, 29 Apr 2025 12:47:47 +1000 Subject: [PATCH 2/7] Update docs/changelog/127489.yaml --- docs/changelog/127489.yaml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 docs/changelog/127489.yaml diff --git a/docs/changelog/127489.yaml b/docs/changelog/127489.yaml new file mode 100644 index 0000000000000..67c87f1500e29 --- /dev/null +++ b/docs/changelog/127489.yaml @@ -0,0 +1,5 @@ +pr: 127489 +summary: Default S3 endpoint scheme to HTTPS when not specified +area: Snapshot/Restore +type: bug +issues: [] From d3f759423f611bb7e458d69d7a6d529949d5b9f1 Mon Sep 17 00:00:00 2001 From: Nick Tindall Date: Tue, 29 Apr 2025 13:59:22 +1000 Subject: [PATCH 3/7] Delete docs/changelog/127489.yaml --- docs/changelog/127489.yaml | 5 ----- 1 file changed, 5 deletions(-) delete mode 100644 docs/changelog/127489.yaml diff --git a/docs/changelog/127489.yaml b/docs/changelog/127489.yaml deleted file mode 100644 index 67c87f1500e29..0000000000000 --- a/docs/changelog/127489.yaml +++ /dev/null @@ -1,5 +0,0 @@ -pr: 127489 -summary: Default S3 endpoint scheme to HTTPS when not specified -area: Snapshot/Restore -type: bug -issues: [] From d3836c4c8f3a0b21a1884296e17d7a7ef58524d8 Mon Sep 17 00:00:00 2001 From: Nick Tindall Date: Tue, 29 Apr 2025 17:41:33 +1000 Subject: [PATCH 4/7] Log when we default the scheme --- .../main/java/org/elasticsearch/repositories/s3/S3Service.java | 1 + 1 file changed, 1 insertion(+) diff --git a/modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Service.java b/modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Service.java index 0bda78f5386ac..80f5c83d67ce2 100644 --- a/modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Service.java +++ b/modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Service.java @@ -259,6 +259,7 @@ protected S3ClientBuilder buildClientBuilder(S3ClientSettings clientSettings, Sd // Default protocol to https if not specified // See https://docs.aws.amazon.com/sdk-for-java/latest/developer-guide/client-configuration.html#client-config-other-diffs endpoint = "https://" + endpoint; + LOGGER.info("Defaulting to https for endpoint with no scheme [{}]", clientSettings.endpoint); } s3clientBuilder.endpointOverride(URI.create(endpoint)); } From 3b81a0122550da42317f228d0a8715f50024308b Mon Sep 17 00:00:00 2001 From: Nick Tindall Date: Thu, 1 May 2025 09:16:48 +1000 Subject: [PATCH 5/7] Update modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Service.java Co-authored-by: Dianna Hohensee --- .../main/java/org/elasticsearch/repositories/s3/S3Service.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Service.java b/modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Service.java index 80f5c83d67ce2..0fec613f43011 100644 --- a/modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Service.java +++ b/modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Service.java @@ -256,7 +256,8 @@ protected S3ClientBuilder buildClientBuilder(S3ClientSettings clientSettings, Sd if (Strings.hasLength(clientSettings.endpoint)) { String endpoint = clientSettings.endpoint; if ((endpoint.startsWith("http://") || endpoint.startsWith("https://")) == false) { - // Default protocol to https if not specified + // The SDK does not know how to interpret endpoints without a scheme prefix and will error. Therefore, when the scheme is + // absent, we'll supply HTTPS as a default to avoid errors. // See https://docs.aws.amazon.com/sdk-for-java/latest/developer-guide/client-configuration.html#client-config-other-diffs endpoint = "https://" + endpoint; LOGGER.info("Defaulting to https for endpoint with no scheme [{}]", clientSettings.endpoint); From 0365fda6f4b401784d7031e28cabee2db434d01c Mon Sep 17 00:00:00 2001 From: Nick Tindall Date: Thu, 1 May 2025 14:54:06 +1000 Subject: [PATCH 6/7] Warn when we default schema, explain how to make the warning go away --- .../java/org/elasticsearch/repositories/s3/S3Service.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Service.java b/modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Service.java index 0fec613f43011..ba9caaaa61b00 100644 --- a/modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Service.java +++ b/modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Service.java @@ -260,7 +260,13 @@ protected S3ClientBuilder buildClientBuilder(S3ClientSettings clientSettings, Sd // absent, we'll supply HTTPS as a default to avoid errors. // See https://docs.aws.amazon.com/sdk-for-java/latest/developer-guide/client-configuration.html#client-config-other-diffs endpoint = "https://" + endpoint; - LOGGER.info("Defaulting to https for endpoint with no scheme [{}]", clientSettings.endpoint); + LOGGER.warn( + """ + found S3 client with endpoint [{}] that is missing a scheme, guessing it should use 'https://'; \ + to suppress this warning, specify the scheme in the [{}] setting on this node""", + clientSettings.endpoint, + S3ClientSettings.REGION.getConcreteSettingForNamespace("CLIENT_NAME").getKey() + ); } s3clientBuilder.endpointOverride(URI.create(endpoint)); } From b84cf5ebdbcb273e63130defaeb8329a3da9e413 Mon Sep 17 00:00:00 2001 From: Nick Tindall Date: Thu, 1 May 2025 14:56:14 +1000 Subject: [PATCH 7/7] Fix variable naming --- .../elasticsearch/repositories/s3/S3ServiceTests.java | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/modules/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3ServiceTests.java b/modules/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3ServiceTests.java index 8e4590576354d..d4cb72dd04257 100644 --- a/modules/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3ServiceTests.java +++ b/modules/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3ServiceTests.java @@ -229,11 +229,14 @@ public void testEndpointOverrideSchemeDefaultsToHttpsWhenNotSpecified() { mock(ResourceWatcherService.class), () -> Region.of("es-test-region") ); - String servername = randomIdentifier() + ".ignore"; + final String endpointWithoutScheme = randomIdentifier() + ".ignore"; S3Client s3Client = s3Service.buildClient( - S3ClientSettings.getClientSettings(Settings.builder().put("s3.client.test-client.endpoint", servername).build(), "test-client"), + S3ClientSettings.getClientSettings( + Settings.builder().put("s3.client.test-client.endpoint", endpointWithoutScheme).build(), + "test-client" + ), mock(SdkHttpClient.class) ); - assertThat(s3Client.serviceClientConfiguration().endpointOverride().get(), equalTo(URI.create("https://" + servername))); + assertThat(s3Client.serviceClientConfiguration().endpointOverride().get(), equalTo(URI.create("https://" + endpointWithoutScheme))); } }