Skip to content

Attempt to simplify the cluster configuration. #149

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 3 commits into from
Closed

Attempt to simplify the cluster configuration. #149

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4fb1c2f
Bug 149 — Support RFC-1738 URLs.
Nov 13, 2014
82d6046
Refactor to use urlparse.
Nov 13, 2014
2850245
@HonzaKral review.
Nov 13, 2014
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
32 changes: 19 additions & 13 deletions elasticsearch/client/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
from __future__ import unicode_literals
import weakref
import logging

from ..transport import Transport
from ..exceptions import NotFoundError, TransportError
from ..compat import string_types
from ..compat import string_types, urlparse
from .indices import IndicesClient
from .cluster import ClusterClient
from .cat import CatClient
Expand All @@ -30,22 +31,27 @@ def _normalize_hosts(hosts):
# normalize hosts to dicts
for i, host in enumerate(hosts):
if isinstance(host, string_types):
host = host.strip('/')
# remove schema information
if '://' in host:
if '://' not in host:
host = "//%s" % host

parsed_url = urlparse(host)
h = {"host": parsed_url.hostname}

if parsed_url.port:
h["port"] = parsed_url.port

if parsed_url.scheme == "https":
h['port'] = parsed_url.port or 443
h['use_ssl'] = True
elif parsed_url.scheme == "http":
logger.warning(
"List of nodes should not include schema information (http://): %r.",
Copy link
Contributor

@honzakral honzakral Nov 13, 2014

Choose a reason for hiding this comment

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

well, after this patch we will accept scheme information so this message should go away. We also need to include docs and a line in the Changelog reflecting this change. there is also be no need to add the http:// since without it present the urlparse method still works.

Copy link
Author

@Natim Natim Nov 13, 2014

Choose a reason for hiding this comment

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

Actually tests aren't passing if I don't add the http scheme.

For instance: if you pass this admin:password@elasticsearch.org to urlparse you have something really strange.

And there is a test for this.

Copy link
Contributor

@honzakral honzakral Nov 13, 2014

Choose a reason for hiding this comment

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

oh yes, sorry - just add '//' at the beginning or, maybe better, just the old parsing code, not sure what makes more sense - imho only http:// urls can have user:pass@. I will look into it later, feel free to do anything or leave as is - I will consult with the authors of other es clients' authors to make it consistent across.

Sorry for the confusion

Copy link
Author

@Natim Natim Nov 13, 2014

Choose a reason for hiding this comment

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

For what I understand if we don't specify https it is doing an http connection, isn't it?

Copy link
Contributor

@honzakral honzakral Nov 13, 2014

Choose a reason for hiding this comment

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

no - then it is at the discretion of the connection_class - http is default but it could be different. Providing // instead of http:// will make the scheme part of the parsed result empty so we will know that no explicit scheme has been requested (and might therefore potentially conflict with the choice made by the connection_class).

Copy link
Author

@Natim Natim Nov 13, 2014

Choose a reason for hiding this comment

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

Yes I like it and changed it.

host
)
host = host[host.index('://') + 3:]

h = {"host": host}
if ':' in host:
# TODO: detect auth urls
host, port = host.rsplit(':', 1)
if port.isdigit():
port = int(port)
h = {"host": host, "port": port}

if parsed_url.username or parsed_url.password:
h['http_auth'] = '%s:%s' % (parsed_url.username, parsed_url.password)

out.append(h)
else:
out.append(host)
Expand Down
3 changes: 2 additions & 1 deletion elasticsearch/compat.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,9 @@
if PY2:
string_types = basestring,
from urllib import quote_plus, urlencode
from urlparse import urlparse
from itertools import imap as map
else:
string_types = str, bytes
from urllib.parse import quote_plus, urlencode
from urllib.parse import quote_plus, urlencode, urlparse
map = map
11 changes: 9 additions & 2 deletions test_elasticsearch/test_client/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,19 @@ def test_none_uses_defaults(self):
def test_strings_are_used_as_hostnames(self):
self.assertEquals([{"host": "elasticsearch.org"}], _normalize_hosts(["elasticsearch.org"]))

def test_strings_are_parsed_for_port(self):
def test_strings_are_parsed_for_port_and_user(self):
self.assertEquals(
[{"host": "elasticsearch.org", "port": 42}, {"host": "user:secret@elasticsearch.com"}],
[{"host": "elasticsearch.org", "port": 42}, {"host": "elasticsearch.com", "http_auth": "user:secret"}],
_normalize_hosts(["elasticsearch.org:42", "user:secret@elasticsearch.com"])
)

def test_strings_are_parsed_for_scheme(self):
self.assertEquals(
[{"host": "elasticsearch.org", "port": 42, "use_ssl": True},
{"host": "elasticsearch.com", "http_auth": "user:secret", "use_ssl": True, "port": 443}],
_normalize_hosts(["https://elasticsearch.org:42", "https://user:secret@elasticsearch.com"])
)

def test_dicts_are_left_unchanged(self):
self.assertEquals([{"host": "local", "extra": 123}], _normalize_hosts([{"host": "local", "extra": 123}]))

Expand Down