-
Notifications
You must be signed in to change notification settings - Fork 25.3k
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
Conversation
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.
Pinging @elastic/es-distributed |
There was a problem hiding this 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(); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
WRT testing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
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.
* 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 ...
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.