Skip to content
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.

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

Upgrade SDK and test discovery-ec2 credential providers #41732

Merged
merged 5 commits into from May 8, 2019

Conversation

ywelsch
Copy link
Contributor

@ywelsch ywelsch commented May 2, 2019

Upgrades the AWS SDK to the same version that we're using for the repository-s3 plugin, providing testing capabilities to override certain SDK endpoints in order to point them to localhost for testing. Adds tests for the various credential providers.

Relates to #41630 (comment)

@ywelsch ywelsch added >enhancement :Distributed/Discovery-Plugins Anything related to our integration plugins with EC2, GCP and Azure v8.0.0 v7.2.0 labels May 2, 2019
@ywelsch ywelsch requested a review from DaveCTurner May 2, 2019 06:30
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

Could we fix the usage of the deprecated new AmazonEC2Client() constructor in AwsEc2ServiceImpl#buildClient() too? This is how it was done in S3Service:

AmazonS3 buildClient(final S3ClientSettings clientSettings) {
final AmazonS3ClientBuilder builder = AmazonS3ClientBuilder.standard();
builder.withCredentials(buildCredentials(logger, clientSettings));
builder.withClientConfiguration(buildConfiguration(clientSettings));
final String endpoint = Strings.hasLength(clientSettings.endpoint) ? clientSettings.endpoint : Constants.S3_HOSTNAME;
logger.debug("using endpoint [{}]", endpoint);
// 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, null))
.enablePathStyleAccess();
return builder.build();
}

@ywelsch
Copy link
Contributor Author

ywelsch commented May 6, 2019

Could we fix the usage of the deprecated new AmazonEC2Client() constructor in AwsEc2ServiceImpl#buildClient() too?

I would rather not in this PR, as that entails other changes (namely change in default endpoint, see #27925)

@DaveCTurner DaveCTurner dismissed their stale review May 6, 2019 10:57

Requested change is tracked in #27925.

@ywelsch ywelsch requested a review from DaveCTurner May 6, 2019 17:33
Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

Ok, LGTM

@ywelsch ywelsch merged commit 89d31de into elastic:master May 8, 2019
ywelsch added a commit that referenced this pull request May 8, 2019
Upgrades the AWS SDK to the same version that we're using for the repository-s3 plugin, providing
testing capabilities to override certain SDK endpoints in order to point them to localhost for testing.
Adds tests for the various credential providers.
Megamiun pushed a commit to Megamiun/elasticsearch that referenced this pull request May 18, 2019
Upgrades the AWS SDK to the same version that we're using for the repository-s3 plugin, providing
testing capabilities to override certain SDK endpoints in order to point them to localhost for testing.
Adds tests for the various credential providers.
gurkankaymak pushed a commit to gurkankaymak/elasticsearch that referenced this pull request May 27, 2019
Upgrades the AWS SDK to the same version that we're using for the repository-s3 plugin, providing
testing capabilities to override certain SDK endpoints in order to point them to localhost for testing.
Adds tests for the various credential providers.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed/Discovery-Plugins Anything related to our integration plugins with EC2, GCP and Azure >enhancement v7.2.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants