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 5 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
9 changes: 8 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,14 @@ 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

LOGGER.info("Defaulting to https for endpoint with no scheme [{}]", clientSettings.endpoint);
Copy link
Contributor

@DiannaHohensee DiannaHohensee Apr 29, 2025

Choose a reason for hiding this comment

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

For other backward compatibility issues that we handle gracefully, we added a WARN log message. Since AWS requires a scheme on the endpoint, I'm leaning towards a WARN here, too.

IIUC, this is a stop-gap for CP-10914 to fix the issue? If the downstream teams intend to change this, then their warning message would eventually be fixed, and that sounds reasonable to me. I think I ideally we wouldn't want to support this forever?

Copy link
Contributor Author

@nicktindall nicktindall Apr 30, 2025

Choose a reason for hiding this comment

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

Yes, perhaps we need to update documentation for the setting also to indicate that scheme is required.

Copy link
Contributor Author

@nicktindall nicktindall May 1, 2025

Choose a reason for hiding this comment

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

Fixed in 0365fda

Copy link
Contributor Author

@nicktindall nicktindall May 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The documentation still has an example without a scheme in it. It also still talks about the protocol property. I don't know where that documentation lives now, but it's not in this repo. We should create a ticket to update it to reflect the upgrade at some point, if that work is not already planned.

Copy link
Member

@ywangd ywangd May 1, 2025

Choose a reason for hiding this comment

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

If you click "Edit this page" on the documentation, it takes you to https://github.com/elastic/docs-content/edit/main/deploy-manage/tools/snapshot-and-restore/s3-repository.md

Copy link
Contributor Author

@nicktindall nicktindall May 1, 2025

Choose a reason for hiding this comment

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

Thanks! TIL. I'll add a second PR to address these changes.

Copy link
Contributor Author

@nicktindall nicktindall May 5, 2025

Choose a reason for hiding this comment

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

PR up here elastic/docs-content#1356

}
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)));
}
}