From e01c1c6472de156e2be29aa3e5652b3c02b190bc Mon Sep 17 00:00:00 2001 From: Sailesh Mukil Date: Thu, 29 Aug 2019 21:39:59 -0700 Subject: [PATCH] Use IPAddresses from HostSupplier rather than from AbstractTokenMapSupplier We always used the IPs from the HostSupplier rather than the AbstractTokenMapSupplier as an undocumented subtlety. In the previous patch, this was changed as it seemed like a harmless change. https://github.com/Netflix/dyno/commit/7b253925f8bae6355a16164efe8469d7929f0591 Howver, the implication is that the client operates on Public IPs (which the AbstractTokenMapSupplier supplies), as opposed to the Private IPs (which the HostSupplier supplies), and in production, the Public IP range are generally not accessible by clients. Why does the AbstractTokenMapSupplier return public IPs? This is because the AbstractTokenMapSupplier derives its values from dynomite-manager which returns the topology seen by it and the server processes, and they use Public IPs to communicate with each other. TODO: Can we ensure that both the HostSupplier and AbstractTokenMapSupplier agree on the same values? --- .../connectionpool/impl/HostsUpdater.java | 43 ++++++++++++++++++- .../dyno/recipes/lock/LocalRedisLockTest.java | 10 ++++- 2 files changed, 50 insertions(+), 3 deletions(-) diff --git a/dyno-core/src/main/java/com/netflix/dyno/connectionpool/impl/HostsUpdater.java b/dyno-core/src/main/java/com/netflix/dyno/connectionpool/impl/HostsUpdater.java index 649429dd..a7b02a21 100644 --- a/dyno-core/src/main/java/com/netflix/dyno/connectionpool/impl/HostsUpdater.java +++ b/dyno-core/src/main/java/com/netflix/dyno/connectionpool/impl/HostsUpdater.java @@ -16,6 +16,7 @@ package com.netflix.dyno.connectionpool.impl; import com.netflix.dyno.connectionpool.Host; +import com.netflix.dyno.connectionpool.HostBuilder; import com.netflix.dyno.connectionpool.HostSupplier; import com.netflix.dyno.connectionpool.TokenMapSupplier; import com.netflix.dyno.connectionpool.exception.DynoException; @@ -116,12 +117,50 @@ public HostStatusTracker refreshHosts() { throw new DynoException("Could not find " + hostFromHostSupplier.getHostName() + " in token map supplier."); } - hostsUpFromHostSupplier.add(Host.clone(hostFromTokenMapSupplier).setStatus(Host.Status.Up)); + // XXX: There's slight subtlety here. We get the hostname and IPAddress from 'hostFromHostSupplier' + // which is derived from the HostSupplier, whereas we get everything else from the 'hostFromTokenMapSupplier' + // which is basically derived from the AbstractTokenMapSupplier. + // The reason for this subtlety is due to the fact that the host supplier returns the preferred IPAddress + // and the AbstractTokenMapSupplier may return an alternative IPAddress (public IP vs. private IP) that + // we may not be able to access. (The same applies to the following 'else' case.) + // + // Why does the AbstractTokenMapSupplier return public IPs? + // This is because the AbstractTokenMapSupplier derives its values from dynomite-manager which + // returns the topology seen by it and the server processes, and they use Public IPs to communicate with + // each other. + // TODO: Can we ensure that both the HostSupplier and AbstractTokenMapSupplier agree on the same values? + HostBuilder upHostBuilder = new HostBuilder() + .setHostname(hostFromHostSupplier.getHostName()) + .setIpAddress(hostFromHostSupplier.getIpAddress()) + .setStatus(Host.Status.Up); + + upHostBuilder.setPort(hostFromTokenMapSupplier.getPort()) + .setSecurePort(hostFromTokenMapSupplier.getSecurePort()) + .setDatastorePort(hostFromTokenMapSupplier.getDatastorePort()) + .setRack(hostFromTokenMapSupplier.getRack()) + .setDatacenter(hostFromTokenMapSupplier.getDatacenter()) + .setHashtag(hostFromHostSupplier.getHashtag()) + .setPassword(hostFromTokenMapSupplier.getPassword()); + + hostsUpFromHostSupplier.add(upHostBuilder.createHost()); allHostSetFromTokenMapSupplier.remove(hostFromTokenMapSupplier); } else { Host hostFromTokenMapSupplier = allHostSetFromTokenMapSupplier.get(hostFromHostSupplier); - hostsDownFromHostSupplier.add(Host.clone(hostFromTokenMapSupplier).setStatus(Host.Status.Down)); + HostBuilder downHostBuilder = new HostBuilder() + .setHostname(hostFromHostSupplier.getHostName()) + .setIpAddress(hostFromHostSupplier.getIpAddress()) + .setStatus(Host.Status.Down); + + downHostBuilder.setPort(hostFromTokenMapSupplier.getPort()) + .setSecurePort(hostFromTokenMapSupplier.getSecurePort()) + .setDatastorePort(hostFromTokenMapSupplier.getDatastorePort()) + .setRack(hostFromTokenMapSupplier.getRack()) + .setDatacenter(hostFromTokenMapSupplier.getDatacenter()) + .setHashtag(hostFromHostSupplier.getHashtag()) + .setPassword(hostFromTokenMapSupplier.getPassword()); + + hostsDownFromHostSupplier.add(downHostBuilder.createHost()); allHostSetFromTokenMapSupplier.remove(hostFromTokenMapSupplier); } } diff --git a/dyno-recipes/src/test/java/com/netflix/dyno/recipes/lock/LocalRedisLockTest.java b/dyno-recipes/src/test/java/com/netflix/dyno/recipes/lock/LocalRedisLockTest.java index 2493dac4..f62aded5 100644 --- a/dyno-recipes/src/test/java/com/netflix/dyno/recipes/lock/LocalRedisLockTest.java +++ b/dyno-recipes/src/test/java/com/netflix/dyno/recipes/lock/LocalRedisLockTest.java @@ -25,7 +25,15 @@ public void setUp() throws IOException { redisServer = new RedisServer(REDIS_PORT); redisServer.start(); Assume.assumeFalse(System.getProperty("os.name").toLowerCase().startsWith("win")); - host = new HostBuilder().setHostname("localhost").setDatastorePort(REDIS_PORT).setPort(REDIS_PORT).setRack(REDIS_RACK).setStatus(Host.Status.Up).createHost(); + host = new HostBuilder() + .setHostname("localhost") + .setIpAddress("127.0.0.1") + .setDatastorePort(REDIS_PORT) + .setPort(REDIS_PORT) + .setRack(REDIS_RACK) + .setStatus(Host.Status.Up) + .createHost(); + tokenMapSupplier = new TokenMapSupplierImpl(host); dynoLockClient = constructDynoLockClient(); }