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

Enable IPv6 URIs in reindex from remote #36874

Merged
merged 3 commits into from Dec 20, 2018

Conversation

Projects
None yet
4 participants
@hub-cap
Copy link
Contributor

commented Dec 20, 2018

Reindex from remote was using a custom regex to dermine what URIs were
valid. This commit removes the custom regex and uses the java.net.URI
class instead, allowing IPv6 support without changing the existing
validation around a URI in reindex from remote.

Enable IPv6 URIs in reindex from remote
Reindex from remote was using a custom regex to dermine what URIs were
valid. This commit removes the custom regex and uses the java.net.URI
class instead, allowing IPv6 support without changing the existing
validation around a URI in reindex from remote.
@elasticmachine

This comment has been minimized.

Copy link

commented Dec 20, 2018

@ywelsch
Copy link
Contributor

left a comment

Would it be possible to have a test that runs some actual reindex operations against ::1 if available?

String pathPrefix = hostMatcher.group("pathPrefix");
int port = Integer.parseInt(hostMatcher.group("port"));

String scheme = uri.getScheme();

This comment has been minimized.

Copy link
@ywelsch

ywelsch Dec 20, 2018

Contributor

also check that scheme is not null in the above validation?

int port = Integer.parseInt(hostMatcher.group("port"));

String scheme = uri.getScheme();
String host = uri.getHost();

This comment has been minimized.

Copy link
@ywelsch

ywelsch Dec 20, 2018

Contributor

same for host, check that it's not null?

This comment has been minimized.

Copy link
@hub-cap

hub-cap Dec 20, 2018

Author Contributor

Ill add a null host check in RestReindexActionTests, but the URI wont parse if its null, so I dont have to add any code here.

https://github.com/elastic/elasticsearch/blob/master/modules/reindex/src/test/java/org/elasticsearch/index/reindex/RestReindexActionTests.java#L79-L83

This comment has been minimized.

Copy link
@hub-cap

hub-cap Dec 20, 2018

Author Contributor

Added some tests in 4229f60

int port = uri.getPort();

String pathPrefix = null;
if (uri.getPath().isEmpty() == false) {

This comment has been minimized.

Copy link
@ywelsch

ywelsch Dec 20, 2018

Contributor

what if getPath returns null?

This comment has been minimized.

Copy link
@hub-cap

hub-cap Dec 20, 2018

Author Contributor

there is an existing test that actually wants it to be null, so i wanted to keep that functionality. It used to return null in the regex, and now it was returning an empty string, so i kept the old functionality.

@hub-cap

This comment has been minimized.

Copy link
Contributor Author

commented Dec 20, 2018

WRT testing [::1], I have a branch that I will be PR'ing once this is merged taht does precisely this in the HLRC.

hub-cap@d8bfbc8

@ywelsch
Copy link
Contributor

left a comment

LGTM. Thanks!

@hub-cap hub-cap merged commit a64fea1 into elastic:master Dec 20, 2018

1 check passed

CLA Commit author is a member of Elasticsearch
Details

hub-cap added a commit that referenced this pull request Dec 20, 2018

Enable IPv6 URIs in reindex from remote (#36874)
Reindex from remote was using a custom regex to dermine what URIs were
valid. This commit removes the custom regex and uses the java.net.URI
class instead, allowing IPv6 support without changing the existing
validation around a URI in reindex from remote.

jasontedor added a commit to ywelsch/elasticsearch that referenced this pull request Dec 21, 2018

Merge remote-tracking branch 'elastic/master' into pr/36137
* elastic/master: (539 commits)
  SQL: documentation improvements and updates (elastic#36918)
  [DOCS] Merges list of discovery and cluster formation settings (elastic#36909)
  Only compress responses if request was compressed (elastic#36867)
  Remove duplicate paragraph (elastic#36942)
  Fix URI to cluster stats endpoint on specific nodes (elastic#36784)
  Fix typo in unitTest task (elastic#36930)
  RecoveryMonitor#lastSeenAccessTime should be volatile (elastic#36781)
  [CCR] Add `ccr.auto_follow_coordinator.wait_for_timeout` setting (elastic#36714)
  Scripting: Remove deprecated params.ctx (elastic#36848)
  Refactor the REST actions to clarify what endpoints are deprecated. (elastic#36869)
  Add JDK 12 to CI rotation (elastic#36915)
  Improve error message for 6.x style realm settings (elastic#36876)
  Send clear session as routable remote request (elastic#36805)
  [DOCS] Remove redundant ILM attributes (elastic#36808)
  SQL: Fix bug regarding histograms usage in scripting (elastic#36866)
  Update index mappings when ccr restore complete (elastic#36879)
  Docs: Bump version to alpha2 after release
  Enable IPv6 URIs in reindex from remote (elastic#36874)
  Watcher: Remove unused local variable in doExecute (elastic#36655)
  [DOCS] Synchs titles of X-Pack APIs
  ...

@jimczi jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.