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

Support for Elastic Cloud ID #923

Merged
merged 17 commits into from Aug 12, 2019
Merged

Support for Elastic Cloud ID #923

merged 17 commits into from Aug 12, 2019

Conversation

philkra
Copy link
Contributor

@philkra philkra commented Jul 29, 2019

This PR adds support for:

  • Elastic Cloud ID
  • APIKey Authentication

When connecting to Elastic Cloud, the best practices should be used such as e.g. http compression.

Copy link
Contributor

@ezimuel ezimuel left a comment

Choose a reason for hiding this comment

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

I would like to propose the following changes in order to simplify the user experience:

  • since the base64 value of id:api_key is not generated by Elastic Cloud (UI or throught API), I don't think we need to offer ClientBuilder::setApiKeyAuthentication() function;

  • removing the setApiKeyAuthentication method, we can simplify the naming using ClientBuilder::setApiKey($id, $key) instead of setApiKeyPairAuthentication($id, $key);

  • when we use the ApiKey we should remove the Basic Authentication in the HTTP request;

  • the usage of the helper class ElasticCloudIdParser just to extract the cluster name and DNS it seems to be over-engineering. I think we can manage it directly into ClientBuilder.

  • use of ClientBuilder::setElasticCloud(string $cloudId, string $username, string $password) instead of setElasticCloudId(string $cloudId, ?string $username = null, ?string $password = null); since we are using Elastic Cloud service we are building the hosts array explicit and these parameters cannot be optional.

Moreover, I just discovered that Elasticsearch supports also OAuth2 token, I think we should support also this feature. We can discuss it later if you want.

@ezimuel ezimuel merged commit 1348fbe into master Aug 12, 2019
@philkra philkra deleted the elastic-cloud branch February 20, 2020 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants