Skip to content

Default S3 endpoint scheme to HTTPS when not specified #127489

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

Merged
merged 12 commits into from
May 5, 2025
Merged

Default S3 endpoint scheme to HTTPS when not specified #127489

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
4abfae7
Default S3 endpoint scheme to HTTPS when not specified
nicktindall Apr 29, 2025
3f238d1
Update docs/changelog/127489.yaml
nicktindall Apr 29, 2025
3bee66f
Merge branch 'main' into default_protocol_to_https
nicktindall Apr 29, 2025
d3f7594
Delete docs/changelog/127489.yaml
nicktindall Apr 29, 2025
d3836c4
Log when we default the scheme
nicktindall Apr 29, 2025
3b81a01
Update modules/repository-s3/src/main/java/org/elasticsearch/reposito…
nicktindall Apr 30, 2025
0365fda
Warn when we default schema, explain how to make the warning go away
nicktindall May 1, 2025
b84cf5e
Fix variable naming
nicktindall May 1, 2025
9d42d86
Merge branch 'main' into default_protocol_to_https
nicktindall May 1, 2025
14ed87f
Update modules/repository-s3/src/main/java/org/elasticsearch/reposito…
nicktindall May 5, 2025
11fd206
Update modules/repository-s3/src/main/java/org/elasticsearch/reposito…
nicktindall May 5, 2025
28f530f
Merge branch 'main' into default_protocol_to_https
nicktindall May 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Service.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

@ywangd ywangd Apr 29, 2025

Choose a reason for hiding this comment

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

Another minor comment: It might be useful to have an INFO log here? If the cluster was relying on an explicit http protocol setting and default to https here would not work for them. In that case, it feels useful to see a log message to indicate this have happened? Since #126843 is a breaking change, I am not entirely sure what the expectation is for end-users. It would be a moot point if they are expected to fix the protocol and endpoint settings before upgrade.

Copy link
Contributor Author

@nicktindall nicktindall Apr 29, 2025

Choose a reason for hiding this comment

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

That's a good point, I'll add one

Copy link
Contributor Author

@nicktindall nicktindall Apr 29, 2025

Choose a reason for hiding this comment

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

Added in d3836c4

}
s3clientBuilder.endpointOverride(URI.create(endpoint));
}

return s3clientBuilder;
Expand Down
19 changes: 19 additions & 0 deletions modules/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3ServiceTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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 {
Expand Down Expand Up @@ -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)));
}
}