diff options
author | Jon Bratseth <jonbratseth@yahoo.com> | 2016-11-10 18:07:00 +0100 |
---|---|---|
committer | GitHub <noreply@github.com> | 2016-11-10 18:07:00 +0100 |
commit | a48f1b0328153043df102a66e1e7e6987e5db9e5 (patch) | |
tree | 91441bec3bcfa1639532b16c2e14dd24f0ea356b | |
parent | 03e980e5c4552642f9878a804764debdd62e6576 (diff) |
Revert "Revert "Enable ZK access control by default""
4 files changed, 32 insertions, 31 deletions
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ZooKeeperAccessMaintainer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ZooKeeperAccessMaintainer.java index 208034358cf..1791e47b1bb 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ZooKeeperAccessMaintainer.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ZooKeeperAccessMaintainer.java @@ -9,7 +9,6 @@ import com.yahoo.vespa.zookeeper.ZooKeeperServer; import java.time.Duration; import java.util.HashSet; import java.util.Set; -import java.util.logging.Logger; /** * Maintains the list of hosts that should be allowed to access ZooKeeper in this runtime. @@ -24,7 +23,6 @@ import java.util.logging.Logger; */ public class ZooKeeperAccessMaintainer extends Maintainer { - private static final Logger log = Logger.getLogger(ZooKeeperAccessMaintainer.class.getName()); private final Curator curator; public ZooKeeperAccessMaintainer(NodeRepository nodeRepository, Curator curator, Duration maintenanceInterval) { @@ -43,7 +41,6 @@ public class ZooKeeperAccessMaintainer extends Maintainer { for (String hostPort : curator.connectionSpec().split(",")) hosts.add(hostPort.split(":")[0]); - log.fine("Restricting ZooKeeper access to " + hosts); ZooKeeperServer.setAllowedClientHostnames(hosts); } diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/ZooKeeperAccessMaintainerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/ZooKeeperAccessMaintainerTest.java index 9ba12fae285..a3ef534b1c9 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/ZooKeeperAccessMaintainerTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/ZooKeeperAccessMaintainerTest.java @@ -13,7 +13,6 @@ import java.util.Set; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; -import static org.junit.Assert.assertFalse; /** * @author bratseth @@ -26,9 +25,9 @@ public class ZooKeeperAccessMaintainerTest { tester.curator().setConnectionSpec("server1:1234,server2:5678"); ZooKeeperAccessMaintainer maintainer = new ZooKeeperAccessMaintainer(tester.nodeRepository(), tester.curator(), Duration.ofHours(1)); - assertFalse(ZooKeeperServer.getAllowedClientHostnames().isPresent()); + assertTrue(ZooKeeperServer.getAllowedClientHostnames().isEmpty()); maintainer.maintain(); - assertEquals(asSet("server1,server2"), ZooKeeperServer.getAllowedClientHostnames().get()); + assertEquals(asSet("server1,server2"), ZooKeeperServer.getAllowedClientHostnames()); tester.addNode("id1", "host1", "default", NodeType.tenant); tester.addNode("id2", "host2", "default", NodeType.tenant); @@ -37,7 +36,7 @@ public class ZooKeeperAccessMaintainerTest { assertEquals(3, tester.getNodes(NodeType.tenant).size()); assertEquals(0, tester.getNodes(NodeType.proxy).size()); - assertEquals(asSet("host1,host2,host3,server1,server2"), ZooKeeperServer.getAllowedClientHostnames().get()); + assertEquals(asSet("host1,host2,host3,server1,server2"), ZooKeeperServer.getAllowedClientHostnames()); tester.addNode("proxy1", "host4", "default", NodeType.proxy); tester.addNode("proxy2", "host5", "default", NodeType.proxy); @@ -45,7 +44,7 @@ public class ZooKeeperAccessMaintainerTest { assertEquals(3, tester.getNodes(NodeType.tenant).size()); assertEquals(2, tester.getNodes(NodeType.proxy).size()); - assertEquals(asSet("host1,host2,host3,host4,host5,server1,server2"), ZooKeeperServer.getAllowedClientHostnames().get()); + assertEquals(asSet("host1,host2,host3,host4,host5,server1,server2"), ZooKeeperServer.getAllowedClientHostnames()); tester.nodeRepository().move("host2", Node.State.parked); assertTrue(tester.nodeRepository().remove("host2")); @@ -53,7 +52,7 @@ public class ZooKeeperAccessMaintainerTest { assertEquals(2, tester.getNodes(NodeType.tenant).size()); assertEquals(2, tester.getNodes(NodeType.proxy).size()); - assertEquals(asSet("host1,host3,host4,host5,server1,server2"), ZooKeeperServer.getAllowedClientHostnames().get()); + assertEquals(asSet("host1,host3,host4,host5,server1,server2"), ZooKeeperServer.getAllowedClientHostnames()); } private Set<String> asSet(String s) { diff --git a/zkfacade/src/main/java/com/yahoo/vespa/zookeeper/RestrictedServerCnxnFactory.java b/zkfacade/src/main/java/com/yahoo/vespa/zookeeper/RestrictedServerCnxnFactory.java index 1c60587b6e4..0a1bfdae3a3 100644 --- a/zkfacade/src/main/java/com/yahoo/vespa/zookeeper/RestrictedServerCnxnFactory.java +++ b/zkfacade/src/main/java/com/yahoo/vespa/zookeeper/RestrictedServerCnxnFactory.java @@ -11,6 +11,7 @@ import java.nio.channels.SocketChannel; import java.util.HashSet; import java.util.Optional; import java.util.Set; +import java.util.logging.Level; import java.util.logging.Logger; /** @@ -29,33 +30,37 @@ public class RestrictedServerCnxnFactory extends NIOServerCnxnFactory { @Override protected NIOServerCnxn createConnection(SocketChannel socket, SelectionKey selection) throws IOException { + ImmutableSet<String> allowedZooKeeperClients = findAllowedZooKeeperClients(); + if (allowedZooKeeperClients.isEmpty()) return super.createConnection(socket, selection); + String remoteHost = ((InetSocketAddress)socket.getRemoteAddress()).getHostName(); - - Optional<ImmutableSet<String>> allowedZooKeeperClients = ZooKeeperServer.getAllowedClientHostnames(); - if ( ! allowedZooKeeperClients.isPresent()) { - log.fine("Allowing connection to ZooKeeper from " + remoteHost + ", as allowed zooKeeper clients is not set"); - return super.createConnection(socket, selection); // client checking is not activated - } - - if ( ! remoteHost.equals("localhost") && ! allowedZooKeeperClients.get().contains(remoteHost)) { + if ( ! remoteHost.equals("localhost") && ! allowedZooKeeperClients.contains(remoteHost)) { String errorMessage = "Rejecting connection to ZooKeeper from " + remoteHost + - ": This cluster only allow connection from hosts in: " + allowedZooKeeperClients.get(); - if ("true".equals(System.getenv("vespa_zkfacade__restrict"))) { - log.info(errorMessage); - throw new IllegalArgumentException(errorMessage); - } - else { - log.fine("Would reject if activated: " + errorMessage); - } + ": This cluster only allow connection from hosts in: " + allowedZooKeeperClients; + log.info(errorMessage); + throw new IllegalArgumentException(errorMessage); // log and throw as this exception will be suppressed by zk } - log.fine("Allowing connection to ZooKeeper from " + remoteHost + ", as it is in " + allowedZooKeeperClients.get()); + log.fine(() -> "Allowing connection to ZooKeeper from " + remoteHost + ", as it is in " + allowedZooKeeperClients); return super.createConnection(socket, selection); } + /** Returns the allowed client host names. If the list is empty any host is allowed. */ + private ImmutableSet<String> findAllowedZooKeeperClients() { + // Environment has precedence. Note that this allows setting restrict to "" to turn off client restriction + String environmentAllowedZooKeeperClients = System.getenv("vespa_zkfacade__restrict"); + if (environmentAllowedZooKeeperClients != null) + return ImmutableSet.copyOf(toHostnameSet(environmentAllowedZooKeeperClients)); + + // No environment setting -> use static field + return ZooKeeperServer.getAllowedClientHostnames(); + } + private Set<String> toHostnameSet(String commaSeparatedString) { Set<String> hostnames = new HashSet<>(); - for (String hostname : commaSeparatedString.split(",")) - hostnames.add(hostname.trim()); + for (String hostname : commaSeparatedString.split(",")) { + if ( ! hostname.trim().isEmpty()) + hostnames.add(hostname.trim()); + } return hostnames; } diff --git a/zkfacade/src/main/java/com/yahoo/vespa/zookeeper/ZooKeeperServer.java b/zkfacade/src/main/java/com/yahoo/vespa/zookeeper/ZooKeeperServer.java index f6219877f5e..4eed2173fb5 100644 --- a/zkfacade/src/main/java/com/yahoo/vespa/zookeeper/ZooKeeperServer.java +++ b/zkfacade/src/main/java/com/yahoo/vespa/zookeeper/ZooKeeperServer.java @@ -28,7 +28,7 @@ public class ZooKeeperServer extends AbstractComponent implements Runnable { * This belongs logically to the server instance but must be static to make it accessible * from RestrictedServerCnxnFactory, which is created by ZK through reflection. */ - private static volatile Optional<ImmutableSet<String>> allowedClientHostnames = Optional.empty(); + private static volatile ImmutableSet<String> allowedClientHostnames = ImmutableSet.of(); private static final java.util.logging.Logger log = java.util.logging.Logger.getLogger(ZooKeeperServer.class.getName()); private static final String ZOOKEEPER_JMX_LOG4J_DISABLE = "zookeeper.jmx.log4j.disable"; @@ -56,11 +56,11 @@ public class ZooKeeperServer extends AbstractComponent implements Runnable { /** Restrict access to this ZooKeeper server to the given client hosts */ public static void setAllowedClientHostnames(Collection<String> hostnames) { - allowedClientHostnames = Optional.of(ImmutableSet.copyOf(hostnames)); + allowedClientHostnames = ImmutableSet.copyOf(hostnames); } /** Returns the hosts which are allowed to access this ZooKeeper server, or empty to allow access from anywhere */ - public static Optional<ImmutableSet<String>> getAllowedClientHostnames() { return allowedClientHostnames; } + public static ImmutableSet<String> getAllowedClientHostnames() { return allowedClientHostnames; } private void writeConfigToDisk(ZookeeperServerConfig config) { String cfg = transformConfigToString(config); |