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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
+27 −1
Open

Default S3 endpoint scheme to HTTPS when not specified #127489

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 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
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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why the original code, and now this code, prepends HTTP endpoints with HTTPS? Am I misreading this somehow? 🤔 endpoint.startsWith("https://")) == false would override everything.

// Default protocol to https if not specified
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// 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;
Comment on lines +258 to +261
Copy link
Member

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

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

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

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

Choose a reason for hiding this comment

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

Could you make this message more like other warnings we have that explain what should have been done and how to fix it?

It's a feature of AWS, at this point, not a bug, so it would be best for users to convert to the interface expectations, and not for us to handle it, I'm thinking.

}
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";
Copy link
Contributor

Choose a reason for hiding this comment

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

endpointWithoutScheme?

servername seems like surprising terminology, but maybe I'm missing the context.

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