Skip to content

Enable IPv6 URIs in reindex from remote #36874

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

Merged
merged 3 commits into from
Dec 20, 2018

Conversation

hub-cap
Copy link
Contributor

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

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.
@hub-cap hub-cap added >enhancement v7.0.0 v6.7.0 :Distributed Indexing/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. labels Dec 20, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

Copy link
Contributor

@ywelsch ywelsch left a comment

Choose a reason for hiding this comment

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

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();
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

String scheme = uri.getScheme();
String host = uri.getHost();
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some tests in 4229f60

int port = uri.getPort();

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

Choose a reason for hiding this comment

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

what if getPath returns null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

hub-cap 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

Copy link
Contributor

@ywelsch ywelsch left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@hub-cap hub-cap merged commit a64fea1 into elastic:master Dec 20, 2018
hub-cap added a commit that referenced this pull request 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.
jasontedor added a commit to ywelsch/elasticsearch that referenced this pull request Dec 21, 2018
* 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
Labels
:Distributed Indexing/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. >enhancement v6.7.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants