Default S3 endpoint scheme to HTTPS when not specified

[nicktindall]

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]

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

[elasticsearchmachine]

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

[ywangd]

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]

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.

[nicktindall]

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

[nicktindall]

Added in d3836c4

[DiannaHohensee]

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.

[DiannaHohensee]

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.

[DiannaHohensee]

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?

[DiannaHohensee]

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.

[DiannaHohensee]

endpointWithoutScheme?

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