Skip to content
This repository was archived by the owner on Jun 20, 2023. It is now read-only.

Update NodeChecker.java #195

Closed

Conversation

mlazzari-tribecube
Copy link

Two issues:

  • incorrect cleanHttpAddress resulting from incorrect substring (easy fix suggested, see below for a little bit more details)
  • hard-coded "http://" protocol forces the client to go HTTP, while it should be possible to choose the protocol I think via a proper ClientConfig setting.

Testing the node checker, I found that the response has this format

{
   "cluster_name": "cluster",
   "nodes": {
      "node": {
         "name": "instance-01",
         "transport_address": "inet[<host>/<ip>:<port>]",
         "host": "<host>",
         "ip": "<ip>",
         "version": "1.4.4",
         "build": "c88f77f",
         "http_address": "inet[<domain>/<ip>:<port>]",
         ...
      },
      ...
  }
}

hence the changed substring incorrectly strips the first char in causing any further action not to failed because of an unknown host.

Two issues:
* incorrect cleanHttpAddress resulting from incorrect substring (easy fix suggested, see below for a little bit more details)
* hard-coded "http://" protocol forces the client to go HTTP, while it should be possible to choose the protocol I think via a proper ClientConfig setting.
 
Testing the node checker, I found that the response has this format
```
{
   "cluster_name": "cluster",
   "nodes": {
      "node": {
         "name": "instance-01",
         "transport_address": "inet[<host>/<ip>:<port>]",
         "host": "<host>",
         "ip": "<ip>",
         "version": "1.4.4",
         "build": "c88f77f",
         "http_address": "inet[<domain>/<ip>:<port>]",
         ...
      },
      ...
  }
}
```
hence the changed substring incorrectly strips the first char in <host> causing any further action not to failed because of an unknown host.
@kramer
Copy link
Member

kramer commented Apr 1, 2015

  1. Hmm no, we have an integration test for that which is working fine and breaks with your suggested change (see: JestClientFactoryIntegrationTest.testDiscovery). That's because returned address is in the format: "http_address": "inet[/192.168.0.2:9503]" and the first slash has no use for us.
  2. You are right on the hard-coded protocol being a potential problem. This needs investigation to see if we can also get protocol info from nodesinfo API.

@kramer kramer closed this Apr 1, 2015
@mlazzari-tribecube
Copy link
Author

  1. Ok, my bad not checking the test case to get a better understanding of the expected format. Nevertheless it is still wrong since I needs to parse content on the string, detect the last slash and substring from there to the end - 1 (or use a proper regexp to be a little bit more precise)
  2. Ok, please also notice that you are also losing any authentication used in the first connection URL provided via the client configuration builder or add server method. I think that also authentication credentials should be passed separately to the client confing so that they can be used along with the selected protocol when reading creating the connection url during node check.

@kramer
Copy link
Member

kramer commented Apr 2, 2015

  1. You're right, sorry :). For some reason I was misinterpreting that slash as a quirky way of ES to represent addresses but turns out it is just InetSocketAddress.toString() (see: 1 and 2). We should see if there is a way to parse it without going manual (regex etc.). Would you like to look into that?

Thanks for reporting!

@mlazzari-tribecube
Copy link
Author

My best take of the topic understanding is that InetSocketAddress.toString() toString concats the internal InetAddress if available (or the hostname if not) and the port

public String toString()
{
  // Note: if addr is null, then hostname != null.
  return (addr == null ? hostname : addr.toString()) + ":" + port;
}

while InetAddress

public String toString()
{
  String addr = getHostAddress();
  String host = (hostName != null) ? hostName : "";
  return host + "/" + addr;
}

so at the end of this madness we may have different resulting strings. The following test code, while not pixel perfect in terms of safeness, should work fairly well

public static void main(String [] args) {
  String inetRegex = "^inet\\[(.*:.*)\\]$";
  Pattern inetPattern = Pattern.compile(inetRegex);

  String[] tests = new String[]{
    "inet[mydomanin123123123-1231231231212.com/12.34.56.78:2315]", // should match
    "inet[s/12.12.12.12:12]", // should match
    "inet[/12.12.12.12:12]", // should match
    "inet[host:12]", // should match
    "inet[aaaaa]" // should not match
  };  

  for (String test : tests) {
    Matcher inetMatcher = inetPattern.matcher(test);
    if (inetMatcher.matches()) {
      String address = inetMatcher.group(1);
      if (address.lastIndexOf("/") >= 0) {
        address = address.substring(address.lastIndexOf("/") + 1);
      }
      System.out.println("\"" + test + "\" matches, address is " + address); // <-- address will contain the needed address
    } else {
      System.out.println("\"" + test + "\" does not match");
    }
  }
}

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants