better lookup of external IP address for ZK endpoints; don't advertis…

…e external IP when listening on loopback; docs

RB_ID=92556

docs/guide.md

@@ -474,7 +474,7 @@ beginning to encounter errors.
 ### ZooKeeper Server Sets
 -------------------------
 
-Kestrel uses Twitter's ServerSet library to support discovery of kestrel
+Kestrel uses Twitter's ServerSet library to support client discovery of kestrel
 servers allowing a given operation. The ServerSet class is documented here:
 [ServerSet](http://twitter.github.com/commons/apidocs/index.html#com.twitter.common.zookeeper.ServerSet)
 
@@ -492,16 +492,27 @@ server set for changes and adjust their connections accordingly. The
 time to detect and react to the status change before they begin receiving
 errors from kestrel.
 
-The ZooKeeper path used to register the server set
-is based on the `pathPrefix` option. Kestrel automatically appends `/write` and
-`/read` to distinguish the write and read sets.
+The ZooKeeper path used to register the server set is based on the `pathPrefix`
+option. Kestrel automatically appends `/write` and `/read` to distinguish the
+write and read sets.
 
 Kestrel advertises all of its endpoints in each server set that it joins.
 The default endpoint is memcache, if configured. The default endpoint falls
 back to the thrift endpoint and then the text protocol endpoint. All three
 endpoints are advertised as additional endpoints under the names `memcache`,
 `thrift` and `text`.
 
+Kestrel advertises only a single IP address per endpoint. This IP address is
+based on Kestrel's `listenAddress`. If the listener address is the wildcard
+address (e.g., `0.0.0.0` or `::`), Kestrel will advertise the first IP address
+it finds by traversing the host's configured network interfaces (via
+`java.net.NetworkInterface`). If your host has multiple, valid external IP
+addresses you can choose the advertised address by setting the listener address
+to that IP. Finally, If the listener address is a loopback address (e.g.,
+`127.0.0.1` or `::1`), Kestrel will not start, since advertising a loopback
+address on ZooKeeper will not work and no external host could connect to
+Kestrel in any event.
+
 Consider setting the  `defaultStatus` option to `Quiescent` to prevent kestrel
 from prematurely advertising its status via ZooKeeper.
 

src/main/scala/net/lag/kestrel/ZooKeeperServerStatus.scala

@@ -24,7 +24,7 @@ import com.twitter.conversions.time._
 import com.twitter.logging.Logger
 import com.twitter.thrift.{Status => TStatus}
 import com.twitter.util.{Duration, Timer}
-import java.net.{InetAddress, InetSocketAddress}
+import java.net.{NetworkInterface, InetAddress, InetSocketAddress, UnknownHostException}
 import scala.collection.JavaConversions
 import config.ZooKeeperConfig
 
@@ -141,18 +141,10 @@ extends ServerStatus(statusFile, timer, defaultStatus, statusChangeGracePeriod)
   override def addEndpoints(mainEndpoint: String, endpoints: Map[String, InetSocketAddress]) {
     if (externalEndpoints ne null) throw new EndpointsAlreadyConfigured
 
-    externalEndpoints = endpoints.map { case (name, givenAddress) =>
-      val givenInetAddress = givenAddress.getAddress
-      val address =
-        if (givenInetAddress.isAnyLocalAddress || givenInetAddress.isLoopbackAddress) {
-          // wildcard (e.g., 0.0.0.0) loopback (e.g., 127.0.0.1) address: replace it
-          // with one external address for this machine
-          val external = InetAddress.getLocalHost.getHostAddress
-          new InetSocketAddress(external, givenAddress.getPort)
-        } else {
-          givenAddress
-        }
-      (name, address)
+    externalEndpoints = endpoints.map { case (name, givenSocketAddress) =>
+      val address = ZooKeeperIP.toExternalAddress(givenSocketAddress.getAddress)
+      val socketAddress = new InetSocketAddress(address, givenSocketAddress.getPort)
+      (name, socketAddress)
     }
 
     mainAddress = externalEndpoints(mainEndpoint)
@@ -225,3 +217,50 @@ extends ServerStatus(statusFile, timer, defaultStatus, statusChangeGracePeriod)
     }
   }
 }
+
+object ZooKeeperIP {
+  import JavaConversions._
+
+  /**
+   * Converts the given IP address into an external IP address to be advertised
+   * by ZooKeeper. If the given IP address is not a wildcard address (e.g.,
+   * "0.0.0.0" or "::") it is returned unmodified.
+   *
+   * Exceptions are thrown if:
+   * <ul>
+   * <li>the given IP address is a loopback address (e.g., "127.0.0.1" or "::1").
+   *     Such an address should not be advertised via ZooKeeper.</li>
+   * <li>there are no configured interfaces</li>
+   * <li>all configured interfaces have only loopback or non-point-to-point link
+   *     local addresses</li>
+   * </ul>
+   *
+   * Otherwise this method returns an external IP address for this host.
+   */
+  def toExternalAddress(givenAddress: InetAddress): InetAddress = {
+    if (givenAddress.isLoopbackAddress) {
+      throw new UnknownHostException("cannot advertise loopback host via zookeeper")
+    }
+
+    if (!givenAddress.isAnyLocalAddress) {
+      // N.B. this address might not be this host
+      return givenAddress
+    }
+
+    val interfaces = NetworkInterface.getNetworkInterfaces()
+    if (interfaces eq null) {
+      throw new UnknownHostException("no network interfaces configured")
+    }
+
+    val candidates = interfaces.flatMap { iface =>
+      iface.getInetAddresses().map { addr => (iface, addr) }
+    }.filter { case (iface, addr) =>
+      !addr.isLoopbackAddress && (iface.isPointToPoint || !addr.isLinkLocalAddress)
+    }.map { case (iface, addr) => addr }.take(1).toList
+
+    candidates.headOption match {
+      case Some(candidate) => candidate
+      case None => throw new UnknownHostException("no acceptable network interfaces found")
+    }
+  }
+}

src/main/scala/net/lag/kestrel/config/KestrelConfig.scala

@@ -130,8 +130,9 @@ class ZooKeeperBuilder {
 
   /**
    * Overrides ServerSet intialization. The default implementation uses a ZooKeeperClient, the
-   * configured pathPrefix and "read" or "write" to produce a ServerSet. The default implementation
-   * is ZooKeeperServerStatus.createServerSet.
+   * configured pathPrefix and the node type (the third argument to this function; always "read" or
+   * "write") to produce a ServerSet.  The default implementation is
+   * ZooKeeperServerStatus.createServerSet.
    *
    * It is strongly recommended that you reference an object method provided in an external JAR
    * rather than placing arbitary code in this configuration file.

src/test/scala/net/lag/kestrel/ZooKeeperServerStatusSpec.scala

@@ -24,7 +24,7 @@ import com.twitter.logging.TestLogging
 import com.twitter.thrift.{Status => TStatus}
 import com.twitter.util.{MockTimer, TempFolder, Time, TimeControl}
 import java.io._
-import java.net.{InetAddress, InetSocketAddress}
+import java.net.{InetAddress, InetSocketAddress, UnknownHostException}
 import org.specs.Specification
 import org.specs.mock.{ClassMocker, JMocker}
 import scala.collection.JavaConversions
@@ -253,8 +253,8 @@ with TestLogging {
     }
 
     "adding endpoints" in {
-      val expectedAddr = new InetSocketAddress(InetAddress.getLocalHost.getHostAddress, 22133)
-      expectedAddr.getAddress.isLoopbackAddress mustEqual false
+      val hostAddr = ZooKeeperIP.toExternalAddress(InetAddress.getByAddress(Array[Byte](0, 0, 0, 0)))
+      val expectedAddr = new InetSocketAddress(hostAddr, 22133)
 
       "should convert wildcard addresses to a proper local address" in {
         withZooKeeperServerStatus { (serverStatus, _) =>
@@ -265,12 +265,11 @@ with TestLogging {
         }
       }
 
-      "should convert the loopback address to a proper local address" in {
+      "should explode if given the loopback address" in {
         withZooKeeperServerStatus { (serverStatus, _) =>
           val localhostAddr = new InetSocketAddress("localhost", 22133)
-          val (readStatus, writeStatus) = expectInitialEndpointStatus(Some(expectedAddr))
 
-          serverStatus.addEndpoints("memcache", Map("memcache" -> localhostAddr))
+          serverStatus.addEndpoints("memcache", Map("memcache" -> localhostAddr)) must throwA[UnknownHostException]
         }
       }