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

dadoonet · 2017-12-20T13:57:56Z

Document that we are using by default ec2.us-east-1.amazonaws.com as
the EC2 endpoint if not explicitly set.

Also switch to non deprecated method when building the EC2 Client.

Document that we are using by default `ec2.us-east-1.amazonaws.com` as the EC2 endpoint if not explicitly set. Also switch to non deprecated method when building the EC2 Client. Closes elastic#27464.

rjernst

I left a few comments.

rjernst · 2017-12-21T19:53:18Z

plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/AwsEc2ServiceImpl.java

+            String region = buildRegion(endpoint);
+            // TODO according to the documentation we should set the region instead of setting a endpoint
+            // See https://github.com/elastic/elasticsearch/issues/27924
+            logger.debug("Setting endpoint to [{}] with region [{}]", endpoint, region);


We already have a debug level message in findEndpoint that is roughly equivalent.

rjernst · 2017-12-21T19:54:29Z

plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/AwsEc2ServiceImpl.java

+            logger.debug("Setting endpoint to [{}] with region [{}]", endpoint, region);
+            builder.setEndpointConfiguration(new AwsClientBuilder.EndpointConfiguration(endpoint, region));
+        } else {
+            logger.debug("No endpoint defined. We are falling back to [ec2.us-east-1.amazonaws.com].");


Normally we wouldn't use "we" in a log message like this. I suggest Using default endpoint [...].

rjernst · 2017-12-21T19:57:06Z

plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/AwsEc2ServiceImpl.java

        String endpoint = findEndpoint(logger, settings);
        if (endpoint != null) {
-            client.setEndpoint(endpoint);
+            String region = buildRegion(endpoint);
+            // TODO according to the documentation we should set the region instead of setting a endpoint


Part of the reason we moved away from region was it required bumping the aws client dep whenever a new region was added. By setting endpoint, we avoid that. For this reason, I do not think we should go back to setting region.

Make sense.

rjernst · 2017-12-21T19:58:22Z

plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/AwsEc2ServiceImpl.java

+import java.io.IOException;
+import java.util.Random;
+
+import static com.amazonaws.util.AwsHostNameUtils.parseRegion;


Please don't static import methods like this. I was very confused looking for this method within this file.

dadoonet · 2017-12-22T16:22:20Z

Thanks @rjernst! I pushed a new commit to address your points.

rjernst

A couple more comments.

rjernst · 2017-12-22T19:35:35Z

docs/plugins/discovery-ec2.asciidoc

-    figured out by the ec2 client based on the instance location, but
-    can be specified explicitly. See http://docs.aws.amazon.com/general/latest/gr/rande.html#ec2_region.
+    The ec2 service endpoint to connect to. See http://docs.aws.amazon.com/general/latest/gr/rande.html#ec2_region.
+    This will default to `ec2.us-east-1.amazonaws.com`.


nit: will default to -> defaults to

rjernst · 2017-12-22T19:36:25Z

plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/AwsEc2ServiceImpl.java

        return this.client;
    }

+    protected static String buildRegion(String endpoint) {


This can be private

rjernst · 2017-12-22T19:37:35Z

plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/AwsEc2ServiceImpl.java

        return this.client;
    }

+    protected static String buildRegion(String endpoint) {
+        // We try to get the region from the metadata instance information
+        String region = new InstanceMetadataRegionProvider().getRegion();


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

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?

dadoonet · 2018-02-21T14:32:27Z

@rjernst Could you tell me what you think of the PR now?

rjernst

Looks good, one last request.

rjernst · 2018-02-22T07:39:22Z

plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/AwsEc2ServiceImpl.java

+        String region = new InstanceMetadataRegionProvider().getRegion();
+        if (region == null) {
+            // Or we try to get it from the endpoint itself
+            region = AwsHostNameUtils.parseRegion(endpoint, null);


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)?

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

Also adding some tests

dadoonet · 2018-02-22T10:25:07Z

@rjernst I pushed a new change. LMK. Thanks!

dadoonet · 2018-02-22T11:23:24Z

jenkins test this please

rjernst · 2018-02-25T22:58:11Z

@dadoonet I think you need to sync with master, it looks like CI failed because it can't find gradlew.

dadoonet · 2018-04-01T13:01:01Z

@elastic/es-distributed Any idea on how to solve the test issue I mentioned earlier on? #27925 (comment)

wedneyyuri · 2018-04-13T19:51:34Z

The documentation is the most important part of this pull request (took me hours to discover this behavior).

What do you think about merging only the docs?

dadoonet · 2018-04-13T19:54:26Z

Yeah. I probably should have start only with that.

wedneyyuri

 The only necessary configuration change to enable the plugin
 is setting the unicast host provider for zen discovery

I guess it's not true if the cluster is not in us-east-1.

I split elastic#27925 in two parts: * The documentation fix (this PR) * The code fix (still in elastic#27925) Closes elastic#27464.

dadoonet · 2018-05-02T06:23:17Z

I opened #30323 to fix only the documentation issue which will leave time to merge this PR.

javanna · 2018-08-16T14:00:04Z

any updates here @dadoonet ? do we still want to merge this one?

dadoonet · 2018-08-16T20:40:12Z

@javanna I'm still waiting for an answer about #27925 (comment)

javanna · 2018-08-17T06:37:59Z

thanks @dadoonet . ping @elastic/es-distributed your input is needed here. Can we move this forward?

bleskes · 2018-09-09T17:47:51Z

@tlrx can you maybe help?

vladimirdolzhenko · 2018-09-14T12:33:40Z

@dadoonet
Sorry for the so long delay in response. To your #27925 (comment) :

It looks you need some kind of fixture that mimics ec2 like it is done for repository-s3:

AmazonS3Fixture is started before integration test and passing a system property com.amazonaws.sdk.ec2MetadataServiceEndpointOverride (it is like 127.0.0.1:58245) to integration test:

elasticsearch/plugins/repository-s3/build.gradle

Line 373 in 7c0fc20

    
           systemProperty "com.amazonaws.sdk.ec2MetadataServiceEndpointOverride", "http://${-> s3Fixture.addressAndPort}"

AmazonS3Fixture handles some EC2 requests (auth for the case of repository-s3) :

elasticsearch/plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/AmazonS3Fixture.java

Line 392 in 7c0fc20

    
           // http://docs.aws.amazon.com/AWSEC2/latest/UserGuide/iam-roles-for-amazon-ec2.html

At least /latest/dynamic/instance-identity/document has to be handled, so region could be specified in that fixture respone.

minor: within buildRegion the call to InstanceMetadataRegionProvider has to be wrapped with SocketAccess.doPrivileged(() -> {...}).

Hope it helps you to proceed further.

# Conflicts: # docs/plugins/discovery-ec2.asciidoc # plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/AwsEc2ServiceImpl.java # plugins/discovery-ec2/src/test/java/org/elasticsearch/discovery/ec2/AwsEc2ServiceImplTests.java

dadoonet · 2019-02-07T15:02:22Z

I don't know if this PR is still useful or not. I'd assume it might be useless now. Thoughts?
If not, I'll need to change testClientSettingsReInit test as ec2_endpoint_1 and ec2_endpoint_2 are not valid endpoints so we can not guess a region from those values.

Note that documentation was updated with #30323.

There is something which bugs me though. The fact that we are still using deprecated methods to build the AmazonEc2 client.

new AmazonEC2Client(credentials, configuration);

We should now use instead a builder as proposed with the current PR:

AmazonEC2ClientBuilder builder = AmazonEC2ClientBuilder.standard()
     .withCredentials(credentials)
     .withClientConfiguration(configuration)
     .withEndpointConfiguration(endpointConfiguration);
final AmazonEC2 client = builder.build();

Would you like another PR for this?

henningandersen

@dadoonet having discussed this with @DaveCTurner we think it still makes sense to change the discovery plugin such that it by default uses the endpoint for the current region.

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), we should:

Make the new default for discovery.ec2.endpoint explicit by a magic constant like 'current_region_endpoint' (ie. make that the default value for the setting)
Add a breaking changes documentation piece on it.
Update docs to mention this default (kind of back to what it was?)
Only do this in 8.0

Regarding the test failure, it looks like @tlrx may have addressed this in #31107, please check if that resolves the test issue.

henningandersen · 2019-03-13T12:54:26Z

plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/AwsEc2ServiceImpl.java

+            endpointConfiguration = new AwsClientBuilder.EndpointConfiguration(clientSettings.endpoint,
+                buildRegion(clientSettings.endpoint));
+        } else {
+            logger.debug("No endpoint defined. Using default endpoint [ec2.us-east-1.amazonaws.com].");


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.

@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

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

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.

@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.

henningandersen · 2019-03-13T13:07:26Z

plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/AwsEc2ServiceImpl.java

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


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.

henningandersen · 2019-03-13T13:10:51Z

plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/AwsEc2ServiceImpl.java

        return client;
    }

+    // Package private for tests
+    static String buildRegion(String endpoint) {


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.

DaveCTurner · 2019-03-13T17:13:21Z

Only do this in 8.0

Note that we can add the new magical value, _current_region_endpoint_, in 7.x. We can then inform people that the default will change to _current_region_endpoint_ in 8.0 via a deprecation warning.

arteam · 2022-07-29T12:44:09Z

The documentation change has already been addressed in #30323 and the change about guessing the region is redundant since the AWS SDK already does it implicitly if the endpoint isn't provided.

See #27924, #30723

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

97e9e09

Document that we are using by default `ec2.us-east-1.amazonaws.com` as the EC2 endpoint if not explicitly set. Also switch to non deprecated method when building the EC2 Client. Closes elastic#27464.

dadoonet added :Plugin Discovery EC2 >bug >docs General docs changes v6.1.2 v6.2.0 v7.0.0 labels Dec 20, 2017

dadoonet self-assigned this Dec 20, 2017

dadoonet requested a review from rjernst December 20, 2017 13:57

rjernst reviewed Dec 21, 2017

View reviewed changes

Update after 1st review

a4d35ca

dadoonet mentioned this pull request Dec 22, 2017

Automatically guess EC2 endpoint based on metadata #27924

Closed

rjernst reviewed Dec 22, 2017

View reviewed changes

Fix part of the comments

8f60084

jasontedor added v6.1.3 and removed v6.1.2 labels Jan 16, 2018

colings86 added v6.3.0 and removed v6.2.0 labels Jan 22, 2018

bleskes added v6.1.4 and removed v6.1.3 labels Jan 29, 2018

clintongormley added :Distributed Coordination/Discovery-Plugins Anything related to our integration plugins with EC2, GCP and Azure and removed :Plugin Discovery EC2 labels Feb 13, 2018

rjernst approved these changes Feb 22, 2018

View reviewed changes

Check for non standard endpoints which will produce NPE

2dbfdd7

Also adding some tests

Merge branch 'master' into pr/27464-endpoint-ec2

4e2e981

dadoonet mentioned this pull request Mar 29, 2018

ec2-discovery plugin document is not accurate. #27464

Closed

dadoonet removed v6.1.5 v6.3.0 labels Apr 1, 2018

wedneyyuri reviewed Apr 14, 2018

View reviewed changes

dadoonet added a commit to dadoonet/elasticsearch that referenced this pull request May 2, 2018

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

9397c61

I split elastic#27925 in two parts: * The documentation fix (this PR) * The code fix (still in elastic#27925) Closes elastic#27464.

dadoonet mentioned this pull request May 2, 2018

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

Merged

dadoonet mentioned this pull request Jun 11, 2018

Update AWS SDK to 1.11.340 in repository-s3 #30723

Merged

tomerleib mentioned this pull request Jul 9, 2018

ec2-discovery not configured correctly at network.host BigDataBoutique/elasticsearch-cloud-deploy#46

Closed

synhershko mentioned this pull request Jul 9, 2018

AWS EC2 endpoint should be explicitly stated BigDataBoutique/elasticsearch-cloud-deploy#50

Closed

dadoonet removed the v7.0.0 label Feb 5, 2019

henningandersen requested changes Mar 13, 2019

View reviewed changes

henningandersen assigned henningandersen and unassigned dadoonet Mar 19, 2019

ywelsch mentioned this pull request May 6, 2019

Upgrade SDK and test discovery-ec2 credential providers #41732

Merged

rjernst added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label May 4, 2020

elasticsearchmachine changed the base branch from master to main July 22, 2022 23:15

arteam closed this Jul 29, 2022

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

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

Conversation

dadoonet commented Dec 20, 2017

rjernst left a 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

Choose a reason for hiding this comment

dadoonet commented Dec 22, 2017

rjernst left a 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

dadoonet commented Feb 21, 2018

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dadoonet commented Feb 22, 2018

dadoonet commented Feb 22, 2018

rjernst commented Feb 25, 2018

dadoonet commented Apr 1, 2018

wedneyyuri commented Apr 13, 2018

dadoonet commented Apr 13, 2018

wedneyyuri left a comment • edited Loading

Choose a reason for hiding this comment

dadoonet commented May 2, 2018

javanna commented Aug 16, 2018

dadoonet commented Aug 16, 2018

javanna commented Aug 17, 2018

bleskes commented Sep 9, 2018

vladimirdolzhenko commented Sep 14, 2018

dadoonet commented Feb 7, 2019

henningandersen left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaveCTurner commented Mar 13, 2019

arteam commented Jul 29, 2022

wedneyyuri left a comment •

edited

Loading