Skip to content

Default ec2 endpoint is ec2.us-east-1.amazonaws.com #27925

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

Closed
wants to merge 6 commits into from
Closed

Default ec2 endpoint is ec2.us-east-1.amazonaws.com #27925

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
97e9e09
Default ec2 endpoint is ec2.us-east-1.amazonaws.com
dadoonet Dec 20, 2017
a4d35ca
Update after 1st review
dadoonet Dec 22, 2017
8f60084
Fix part of the comments
dadoonet Jan 11, 2018
2dbfdd7
Check for non standard endpoints which will produce NPE
dadoonet Feb 22, 2018
4e2e981
Merge branch 'master' into pr/27464-endpoint-ec2
dadoonet Feb 26, 2018
530d885
Merge branch 'master' into pr/27464-endpoint-ec2
dadoonet Feb 7, 2019
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
5 changes: 2 additions & 3 deletions docs/plugins/discovery-ec2.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,8 @@ Those that must be stored in the keystore are marked as `Secure`.

`endpoint`::

The ec2 service endpoint to connect to. See
http://docs.aws.amazon.com/general/latest/gr/rande.html#ec2_region. This
defaults to `ec2.us-east-1.amazonaws.com`.
The ec2 service endpoint to connect to. See http://docs.aws.amazon.com/general/latest/gr/rande.html#ec2_region.
This defaults to `ec2.us-east-1.amazonaws.com`.

`protocol`::

Expand Down
46 changes: 39 additions & 7 deletions plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/AwsEc2ServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,14 @@
import com.amazonaws.auth.AWSCredentials;
import com.amazonaws.auth.AWSCredentialsProvider;
import com.amazonaws.auth.DefaultAWSCredentialsProviderChain;
import com.amazonaws.client.builder.AwsClientBuilder;
import com.amazonaws.http.IdleConnectionReaper;
import com.amazonaws.internal.StaticCredentialsProvider;
import com.amazonaws.regions.InstanceMetadataRegionProvider;
import com.amazonaws.retry.RetryPolicy;
import com.amazonaws.services.ec2.AmazonEC2;
import com.amazonaws.services.ec2.AmazonEC2Client;

import com.amazonaws.services.ec2.AmazonEC2ClientBuilder;
import com.amazonaws.util.AwsHostNameUtils;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.elasticsearch.ElasticsearchException;
Expand All @@ -40,7 +42,7 @@
import java.util.concurrent.atomic.AtomicReference;

class AwsEc2ServiceImpl implements AwsEc2Service {

private static final Logger logger = LogManager.getLogger(AwsEc2ServiceImpl.class);

private final AtomicReference<LazyInitializable<AmazonEc2Reference, ElasticsearchException>> lazyClientReference =
Expand All @@ -49,20 +51,50 @@ class AwsEc2ServiceImpl implements AwsEc2Service {
private AmazonEC2 buildClient(Ec2ClientSettings clientSettings) {
final AWSCredentialsProvider credentials = buildCredentials(logger, clientSettings);
final ClientConfiguration configuration = buildConfiguration(logger, clientSettings);
final AmazonEC2 client = buildClient(credentials, configuration);

final AwsClientBuilder.EndpointConfiguration endpointConfiguration;
if (Strings.hasText(clientSettings.endpoint)) {
logger.debug("using explicit ec2 endpoint [{}]", clientSettings.endpoint);
client.setEndpoint(clientSettings.endpoint);
endpointConfiguration = new AwsClientBuilder.EndpointConfiguration(clientSettings.endpoint,
Copy link
Contributor

@henningandersen henningandersen Mar 13, 2019

Choose a reason for hiding this comment

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

As I understand it, the region and endpoint has to go together and really specifying the endpoint should not be done. The region is used to determine signing as well, which means that using a wrong region for an endpoint is unlikely to work (AFAIK). Given that we calculate the region from the endpoint anyway, we may as well just call AwsClientBuilder.withRegion with the region, this will ensure both the endpoint and signing region are set.

buildRegion(clientSettings.endpoint));
} else {
logger.debug("No endpoint defined. Using default endpoint [ec2.us-east-1.amazonaws.com].");
Copy link
Contributor

@henningandersen henningandersen Mar 13, 2019

Choose a reason for hiding this comment

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

When using the AmazonEC2ClientBuilder, the default endpoint is current regions endpoint, see:
https://docs.aws.amazon.com/sdk-for-java/v1/developer-guide/java-dg-region-selection.html

This seems like a good default to use, but is a breaking change. See general review comment.

Copy link

@suganselvaraj suganselvaraj Jul 5, 2019

Choose a reason for hiding this comment

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

@henningandersen @DaveCTurner @dadoonet

Hello All,

We are looking for multiple region ec2-discovery option.

Can you please update where are we with the below comment. Is it already implemented and in-use or its gonna be in future release ?

Given that this is potentially a breaking change (with master nodes in us-east-1 and data nodes in some other region, the data nodes will no longer be able to discover the master nodes)

Thanks

Copy link
Contributor

@henningandersen henningandersen Jul 5, 2019

Choose a reason for hiding this comment

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

@suganselvaraj this is still outstanding, i.e., not implemented into master or any releases yet.

Copy link
Contributor Author

@dadoonet dadoonet Jul 5, 2019

Choose a reason for hiding this comment

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

Note that:

We are looking for multiple region ec2-discovery option.

This is not something we really support AFAIK. You should have all nodes within the same region for latency reasons.

My 2 cents.

Copy link

@wedneyyuri wedneyyuri Jul 6, 2019

Choose a reason for hiding this comment

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

@suganselvaraj I think it will not work using "ec2-discovery" because under the hood it's searching for nodes inside the same aws region.

You can use a fixed list of IP's, but it can cause latency issues.

endpointConfiguration = null;
}

final AmazonEC2 client = buildClient(credentials, configuration, endpointConfiguration);
return client;
}

// proxy for testing
AmazonEC2 buildClient(AWSCredentialsProvider credentials, ClientConfiguration configuration) {
final AmazonEC2 client = new AmazonEC2Client(credentials, configuration);
AmazonEC2 buildClient(AWSCredentialsProvider credentials, ClientConfiguration configuration,
AwsClientBuilder.EndpointConfiguration endpointConfiguration) {

AmazonEC2ClientBuilder builder = AmazonEC2ClientBuilder.standard()
.withCredentials(credentials)
.withClientConfiguration(configuration);
if (endpointConfiguration != null) {
builder.setEndpointConfiguration(endpointConfiguration);
}
final AmazonEC2 client = builder.build();
return client;
}

// Package private for tests
static String buildRegion(String endpoint) {
Copy link
Contributor

@henningandersen henningandersen Mar 13, 2019

Choose a reason for hiding this comment

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

We should get the region from the endpoint or if endpoint is not set, simply return null. Ie. we do not really need to lookup the region using the InstanceMetadataRegionProvider since the SDK does that by itself when using the client builder.

// We try to get the region from the metadata instance information
String region = new InstanceMetadataRegionProvider().getRegion();
Copy link
Member

@rjernst rjernst Dec 22, 2017

Choose a reason for hiding this comment

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

Shouldn't we only be inferring this if the endpoint is not set?

Copy link
Contributor Author

@dadoonet dadoonet Jan 11, 2018

Choose a reason for hiding this comment

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

That should come in another PR related to #27924 IMO.
Here, the goal is to call new AwsClientBuilder.EndpointConfiguration(endpoint, region). As we don't have the region anymore, I need to guess the region either from the instance metadata and if not available from the endpoint with AwsHostNameUtils.parseRegion(endpoint, null).

Make sense?

if (region == null) {
// Or we try to get it from the endpoint itself
region = AwsHostNameUtils.parseRegion(endpoint, null);
Copy link
Member

@rjernst rjernst Feb 22, 2018

Choose a reason for hiding this comment

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

Don't we need to check for null again here? pareRegion can return null if the endpoint is "non standard". I think we need to throw an error in that case or the endpoint configuration will be incorrect (endpoint but null region)?

Copy link
Contributor Author

@dadoonet dadoonet Feb 22, 2018

Choose a reason for hiding this comment

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

Ah! Good catch. Indeed the AWS implementation returns null. I'm going to add this. Thanks

if (region == null) {
// Endpoint is non-standard
throw new IllegalArgumentException("Can not guess a region from endpoint [" + endpoint + "].");
}
}
return region;
}

// pkg private for tests
static ClientConfiguration buildConfiguration(Logger logger, Ec2ClientSettings clientSettings) {
final ClientConfiguration clientConfiguration = new ClientConfiguration();
Expand Down
7 changes: 7 additions & 0 deletions ...s/discovery-ec2/src/test/java/org/elasticsearch/discovery/ec2/AwsEc2ServiceImplTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,4 +132,11 @@ protected void launchAWSConfigurationTest(Settings settings,
assertThat(configuration.getSocketTimeout(), is(expectedReadTimeout));
}

public void testBuildRegion() {
assertThat(AwsEc2ServiceImpl.buildRegion("ec2.us-east-1.amazonaws.com"), is("us-east-1"));
assertThat(AwsEc2ServiceImpl.buildRegion("ec2.eu-west-3.amazonaws.com"), is("eu-west-3"));
IllegalArgumentException exception = expectThrows(IllegalArgumentException.class,
() -> AwsEc2ServiceImpl.buildRegion("foo.bar.nodomain"));
assertThat(exception.getMessage(), is("Can not guess a region from endpoint [foo.bar.nodomain]."));
}
}
4 changes: 3 additions & 1 deletion plugins/discovery-ec2/src/test/java/org/elasticsearch/discovery/ec2/AwsEc2ServiceMock.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

import com.amazonaws.ClientConfiguration;
import com.amazonaws.auth.AWSCredentialsProvider;
import com.amazonaws.client.builder.AwsClientBuilder;
import com.amazonaws.services.ec2.AmazonEC2;
import com.amazonaws.services.ec2.model.Tag;

Expand All @@ -37,7 +38,8 @@ public AwsEc2ServiceMock(int nodes, List<List<Tag>> tagsList) {
}

@Override
AmazonEC2 buildClient(AWSCredentialsProvider credentials, ClientConfiguration configuration) {
AmazonEC2 buildClient(AWSCredentialsProvider credentials, ClientConfiguration configuration,
AwsClientBuilder.EndpointConfiguration endpointConfiguration) {
return new AmazonEC2Mock(nodes, tagsList, credentials, configuration);
}

Expand Down