Default S3 endpoint scheme to HTTPS when not specified #127489

nicktindall · 2025-04-29T02:47:23Z

The protocol configuration option was removed in the AWS v2 client

Our interpretation of that was that we could provide a host name and the scheme would be defaulted/derived. The code appears to contradict that. While you can't specify the protocol using the setProtocol anymore, it seems if you want to specify an endpoint override, it must include a scheme in the URI.

See validation here

This PR just appends https:// when no scheme is specified. This seemed to match the defaulting of the new builder now that setProtocol has been removed.

We used to have code that used our s3.client.{client-name}.protocol setting to prepend a scheme if it were missing:

elasticsearch/modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Service.java

Lines 205 to 222 in 4ce1d9c

    
           String endpoint = Strings.hasLength(clientSettings.endpoint) ? clientSettings.endpoint : Constants.S3_HOSTNAME; 
        
           if ((endpoint.startsWith("http://") || endpoint.startsWith("https://")) == false) { 
        
               // Manually add the schema to the endpoint to work around https://github.com/aws/aws-sdk-java/issues/2274 
        
               // TODO: Remove this once fixed in the AWS SDK 
        
               endpoint = clientSettings.protocol.toString() + "://" + endpoint; 
        
           } 
        
           final String region = Strings.hasLength(clientSettings.region) ? clientSettings.region : null; 
        
           LOGGER.debug("using endpoint [{}] and region [{}]", endpoint, region); 
        
           // If the endpoint configuration isn't set on the builder then the default behaviour is to try 
        
           // and work out what region we are in and use an appropriate endpoint - see AwsClientBuilder#setRegion. 
        
           // In contrast, directly-constructed clients use s3.amazonaws.com unless otherwise instructed. We currently 
        
           // use a directly-constructed client, and need to keep the existing behaviour to avoid a breaking change, 
        
           // so to move to using the builder we must set it explicitly to keep the existing behaviour. 
        
           // 
        
           // We do this because directly constructing the client is deprecated (was already deprecated in 1.1.223 too) 
        
           // so this change removes that usage of a deprecated API. 
        
           builder.withEndpointConfiguration(new AwsClientBuilder.EndpointConfiguration(endpoint, region));

I thought it was better to match the new client behaviour (defaulting to HTTPS) rather than resurrecting the s3.client.{client-name}.protocol setting (which was deprecated in the client upgrade). It seems the linked issue is not relevant to the v2 client behaviour.

You can still use a HTTP server by setting the scheme explicitly in the s3.client.{client-name}.endpoint

Relates: CP-10914

elasticsearchmachine · 2025-04-29T02:47:47Z

Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination)

elasticsearchmachine · 2025-04-29T02:47:47Z

Hi @nicktindall, I've created a changelog YAML for you.

ywangd · 2025-04-29T03:54:42Z

The change makes sense to me. Just a minor comment on the label: Since #126843 is not released, this PR should be a >non-issue.

ywangd · 2025-04-29T06:56:34Z

modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Service.java

+            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;


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.

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

Added in d3836c4

modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Service.java

DiannaHohensee · 2025-04-29T12:55:39Z

modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Service.java

+                // 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);


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?

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

Fixed in 0365fda

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.

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

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

PR up here elastic/docs-content#1356

modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Service.java

modules/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3ServiceTests.java

…ries/s3/S3Service.java Co-authored-by: Dianna Hohensee <[email protected]>

DiannaHohensee

LGTM, with a typo fix and a suggestion. Thanks for fixing this!

modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Service.java

…ries/s3/S3Service.java Co-authored-by: Dianna Hohensee <[email protected]>

DaveCTurner

+1 to this quick fix to address the ECH use case, but then I think we should follow-up with one that chooses between http and https according to the .protocol client setting to preserve the SDKv1 behaviour. I still think .protocol should be deprecated, we don't need to support both ways to set the URL scheme; I will be opening a breaking-change proposal to recommend the removal of all the SDK-bwc shims in v10, including this one.

(cherry picked from commit 8dc6bf8)

The `s3.client.CLIENT_NAME.protocol` setting became unused in elastic#126843 as it is inapplicable in the v2 SDK. However, the v2 SDK requires the `s3.client.CLIENT_NAME.endpoint` setting to be a URL that includes a scheme, so in elastic#127489 we prepend a `https://` to the endpoint if needed. This commit generalizes this slightly so that we prepend `http://` if the endpoint has no scheme and the `.protocol` setting is set to `http`.

The `s3.client.CLIENT_NAME.protocol` setting became unused in #126843 as it is inapplicable in the v2 SDK. However, the v2 SDK requires the `s3.client.CLIENT_NAME.endpoint` setting to be a URL that includes a scheme, so in #127489 we prepend a `https://` to the endpoint if needed. This commit generalizes this slightly so that we prepend `http://` if the endpoint has no scheme and the `.protocol` setting is set to `http`.

The `s3.client.CLIENT_NAME.protocol` setting became unused in elastic#126843 as it is inapplicable in the v2 SDK. However, the v2 SDK requires the `s3.client.CLIENT_NAME.endpoint` setting to be a URL that includes a scheme, so in elastic#127489 we prepend a `https://` to the endpoint if needed. This commit generalizes this slightly so that we prepend `http://` if the endpoint has no scheme and the `.protocol` setting is set to `http`. Backport of elastic#127744 to `8.19`

* Reinstate use of S3 `protocol` client setting The `s3.client.CLIENT_NAME.protocol` setting became unused in #126843 as it is inapplicable in the v2 SDK. However, the v2 SDK requires the `s3.client.CLIENT_NAME.endpoint` setting to be a URL that includes a scheme, so in #127489 we prepend a `https://` to the endpoint if needed. This commit generalizes this slightly so that we prepend `http://` if the endpoint has no scheme and the `.protocol` setting is set to `http`. Backport of #127744 to `8.19` * Fix up warning assertions

The `s3.client.CLIENT_NAME.protocol` setting became unused in elastic#126843 as it is inapplicable in the v2 SDK. However, the v2 SDK requires the `s3.client.CLIENT_NAME.endpoint` setting to be a URL that includes a scheme, so in elastic#127489 we prepend a `https://` to the endpoint if needed. This commit generalizes this slightly so that we prepend `http://` if the endpoint has no scheme and the `.protocol` setting is set to `http`.

Default S3 endpoint scheme to HTTPS when not specified

4abfae7

nicktindall added >bug :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v8.19.0 labels Apr 29, 2025

elasticsearchmachine added Team:Distributed Coordination Meta label for Distributed Coordination team v9.1.0 labels Apr 29, 2025

Update docs/changelog/127489.yaml

3f238d1

nicktindall requested a review from DiannaHohensee April 29, 2025 02:57

Merge branch 'main' into default_protocol_to_https

3bee66f

nicktindall added >non-issue and removed >bug labels Apr 29, 2025

Delete docs/changelog/127489.yaml

d3f7594

ywangd reviewed Apr 29, 2025

View reviewed changes

Log when we default the scheme

d3836c4

DiannaHohensee reviewed Apr 29, 2025

View reviewed changes

nicktindall and others added 4 commits May 1, 2025 09:16

Update modules/repository-s3/src/main/java/org/elasticsearch/reposito…

3b81a01

…ries/s3/S3Service.java Co-authored-by: Dianna Hohensee <[email protected]>

Warn when we default schema, explain how to make the warning go away

0365fda

Fix variable naming

b84cf5e

Merge branch 'main' into default_protocol_to_https

9d42d86

nicktindall requested a review from DiannaHohensee May 1, 2025 04:59

DiannaHohensee approved these changes May 2, 2025

View reviewed changes

modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Service.java Outdated Show resolved Hide resolved

modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Service.java Outdated Show resolved Hide resolved

nicktindall and others added 3 commits May 5, 2025 16:19

Update modules/repository-s3/src/main/java/org/elasticsearch/reposito…

14ed87f

…ries/s3/S3Service.java Co-authored-by: Dianna Hohensee <[email protected]>

Update modules/repository-s3/src/main/java/org/elasticsearch/reposito…

11fd206

…ries/s3/S3Service.java Co-authored-by: Dianna Hohensee <[email protected]>

Merge branch 'main' into default_protocol_to_https

28f530f

DaveCTurner reviewed May 5, 2025

View reviewed changes

DaveCTurner mentioned this pull request May 5, 2025

Update s3-repository docs after upgrade elastic/docs-content#1356

Open

nicktindall merged commit 8dc6bf8 into elastic:main May 5, 2025
16 of 17 checks passed

nicktindall deleted the default_protocol_to_https branch May 5, 2025 08:47

nicktindall added a commit to nicktindall/elasticsearch that referenced this pull request May 5, 2025

Default S3 endpoint scheme to HTTPS when not specified (elastic#127489)

75b9092

(cherry picked from commit 8dc6bf8)

nicktindall mentioned this pull request May 5, 2025

Default S3 endpoint scheme to HTTPS when not specified (#127489) #127704

Merged

DaveCTurner mentioned this pull request May 6, 2025

Reinstate use of S3 protocol client setting #127744

Merged

DaveCTurner mentioned this pull request May 7, 2025

Reinstate use of S3 protocol client setting #127809

Merged

ywangd pushed a commit to ywangd/elasticsearch that referenced this pull request May 9, 2025

Default S3 endpoint scheme to HTTPS when not specified (elastic#127489)

f8401be

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Default S3 endpoint scheme to HTTPS when not specified #127489

Default S3 endpoint scheme to HTTPS when not specified #127489

nicktindall commented Apr 29, 2025 •

edited

Loading

elasticsearchmachine commented Apr 29, 2025

elasticsearchmachine commented Apr 29, 2025

ywangd commented Apr 29, 2025

ywangd Apr 29, 2025

nicktindall Apr 29, 2025

nicktindall Apr 29, 2025

DiannaHohensee Apr 29, 2025

nicktindall Apr 30, 2025

nicktindall May 1, 2025

nicktindall May 1, 2025 •

edited

Loading

ywangd May 1, 2025

nicktindall May 1, 2025

nicktindall May 5, 2025

DiannaHohensee left a comment

DaveCTurner left a comment

	String endpoint = Strings.hasLength(clientSettings.endpoint) ? clientSettings.endpoint : Constants.S3_HOSTNAME;
	if ((endpoint.startsWith("http://") \|\| endpoint.startsWith("https://")) == false) {
	// Manually add the schema to the endpoint to work around https://github.com/aws/aws-sdk-java/issues/2274
	// TODO: Remove this once fixed in the AWS SDK
	endpoint = clientSettings.protocol.toString() + "://" + endpoint;
	}
	final String region = Strings.hasLength(clientSettings.region) ? clientSettings.region : null;
	LOGGER.debug("using endpoint [{}] and region [{}]", endpoint, region);

	// If the endpoint configuration isn't set on the builder then the default behaviour is to try
	// and work out what region we are in and use an appropriate endpoint - see AwsClientBuilder#setRegion.
	// In contrast, directly-constructed clients use s3.amazonaws.com unless otherwise instructed. We currently
	// use a directly-constructed client, and need to keep the existing behaviour to avoid a breaking change,
	// so to move to using the builder we must set it explicitly to keep the existing behaviour.
	//
	// We do this because directly constructing the client is deprecated (was already deprecated in 1.1.223 too)
	// so this change removes that usage of a deprecated API.
	builder.withEndpointConfiguration(new AwsClientBuilder.EndpointConfiguration(endpoint, region));

Default S3 endpoint scheme to HTTPS when not specified #127489

Default S3 endpoint scheme to HTTPS when not specified #127489

Conversation

nicktindall commented Apr 29, 2025 • edited Loading

elasticsearchmachine commented Apr 29, 2025

elasticsearchmachine commented Apr 29, 2025

ywangd commented Apr 29, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicktindall May 1, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DiannaHohensee left a comment

Choose a reason for hiding this comment

DaveCTurner left a comment

Choose a reason for hiding this comment

nicktindall commented Apr 29, 2025 •

edited

Loading

nicktindall May 1, 2025 •

edited

Loading