Merge pull request #5252 from vespa-engine/revert-5244-musum/only-allow-zk-access-from-config-servers-in-hosted-vespa

Revert "Only allow Zookeeper access for config servers in hosted Vespa"

8 files changed, 151 insertions, 29 deletions

diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/zookeeper/ConfigCurator.java b/configserver/src/main/java/com/yahoo/vespa/config/server/zookeeper/ConfigCurator.java
index faf25a16321..c7a79556397 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/zookeeper/ConfigCurator.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/zookeeper/ConfigCurator.java
@@ -88,7 +88,7 @@ public class ConfigCurator {
 
     @Inject
     public ConfigCurator(Curator curator, ZooKeeperServer server) {
-        this(curator, server.getZookeeperServerConfig().juteMaxBuffer());
+        this(curator, server.getConfig().juteMaxBuffer());
     }
 
     private ConfigCurator(Curator curator, int maxNodeSize) {
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRepositoryMaintenance.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRepositoryMaintenance.java
index 7b191538ad8..7b0606b809b 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRepositoryMaintenance.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRepositoryMaintenance.java
@@ -38,6 +38,7 @@ public class NodeRepositoryMaintenance extends AbstractComponent {
     private final NodeFailer nodeFailer;
     private final PeriodicApplicationMaintainer periodicApplicationMaintainer;
     private final OperatorChangeApplicationMaintainer operatorChangeApplicationMaintainer;
+    private final ZooKeeperAccessMaintainer zooKeeperAccessMaintainer;
     private final ReservationExpirer reservationExpirer;
     private final InactiveExpirer inactiveExpirer;
     private final RetiredExpirer retiredExpirer;
@@ -69,6 +70,7 @@ public class NodeRepositoryMaintenance extends AbstractComponent {
         nodeFailer = new NodeFailer(deployer, hostLivenessTracker, serviceMonitor, nodeRepository, durationFromEnv("fail_grace").orElse(defaults.failGrace), clock, orchestrator, throttlePolicyFromEnv("throttle_policy").orElse(defaults.throttlePolicy), metric, jobControl, configserverConfig);
         periodicApplicationMaintainer = new PeriodicApplicationMaintainer(deployer, nodeRepository, durationFromEnv("periodic_redeploy_interval").orElse(defaults.periodicRedeployInterval), jobControl);
         operatorChangeApplicationMaintainer = new OperatorChangeApplicationMaintainer(deployer, nodeRepository, clock, durationFromEnv("operator_change_redeploy_interval").orElse(defaults.operatorChangeRedeployInterval), jobControl);
+        zooKeeperAccessMaintainer = new ZooKeeperAccessMaintainer(nodeRepository, curator, durationFromEnv("zookeeper_access_maintenance_interval").orElse(defaults.zooKeeperAccessMaintenanceInterval), jobControl);
         reservationExpirer = new ReservationExpirer(nodeRepository, clock, durationFromEnv("reservation_expiry").orElse(defaults.reservationExpiry), jobControl);
         retiredExpirer = new RetiredExpirer(nodeRepository, orchestrator, deployer, clock, durationFromEnv("retired_interval").orElse(defaults.retiredInterval), durationFromEnv("retired_expiry").orElse(defaults.retiredExpiry), jobControl);
         inactiveExpirer = new InactiveExpirer(nodeRepository, clock, durationFromEnv("inactive_expiry").orElse(defaults.inactiveExpiry), jobControl);
@@ -89,6 +91,7 @@ public class NodeRepositoryMaintenance extends AbstractComponent {
         nodeFailer.deconstruct();
         periodicApplicationMaintainer.deconstruct();
         operatorChangeApplicationMaintainer.deconstruct();
+        zooKeeperAccessMaintainer.deconstruct();
         reservationExpirer.deconstruct();
         inactiveExpirer.deconstruct();
         retiredExpirer.deconstruct();
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
new file mode 100644
index 00000000000..b392d670a77
--- /dev/null
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ZooKeeperAccessMaintainer.java
@@ -0,0 +1,49 @@
+// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+package com.yahoo.vespa.hosted.provision.maintenance;
+
+import com.yahoo.vespa.curator.Curator;
+import com.yahoo.vespa.hosted.provision.Node;
+import com.yahoo.vespa.hosted.provision.NodeRepository;
+import com.yahoo.vespa.zookeeper.ZooKeeperServer;
+
+import java.time.Duration;
+import java.util.HashSet;
+import java.util.Set;
+
+/**
+ * Maintains the list of hosts that should be allowed to access ZooKeeper in this runtime.
+ * These are the zookeeper servers and all nodes in node repository. This is maintained in the background
+ * because nodes could be added or removed on another server.
+ * 
+ * We could limit access to the <i>active</i> subset of nodes, but that 
+ * does not seem to have any particular operational or security benefits and might make it more problematic
+ * for this job to be behind actual changes to the active set of nodes.
+ * 
+ * @author bratseth
+ */
+public class ZooKeeperAccessMaintainer extends Maintainer {
+
+    private final Curator curator;
+    
+    public ZooKeeperAccessMaintainer(NodeRepository nodeRepository, Curator curator, Duration maintenanceInterval, 
+                                     JobControl jobControl) {
+        super(nodeRepository, maintenanceInterval, jobControl);
+        this.curator = curator;
+    }
+
+    @Override
+    protected void maintain() {
+        Set<String> hosts = new HashSet<>();
+
+        for (Node node : nodeRepository().getNodes())
+            hosts.add(node.hostname());
+
+        if ( ! hosts.isEmpty()) { // no nodes -> not a hosted instance: Pass an empty list to deactivate restriction
+            for (String hostPort : curator.zooKeeperEnsembleConnectionSpec().split(","))
+                hosts.add(hostPort.split(":")[0]);
+        }
+
+        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
new file mode 100644
index 00000000000..93cf19f5450
--- /dev/null
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/ZooKeeperAccessMaintainerTest.java
@@ -0,0 +1,68 @@
+// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+package com.yahoo.vespa.hosted.provision.maintenance;
+
+import com.yahoo.config.provision.NodeType;
+import com.yahoo.vespa.hosted.provision.NodeRepositoryTester;
+import com.yahoo.vespa.hosted.provision.node.Agent;
+import com.yahoo.vespa.zookeeper.ZooKeeperServer;
+import org.junit.Test;
+
+import java.time.Duration;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Set;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+/**
+ * @author bratseth
+ */
+public class ZooKeeperAccessMaintainerTest {
+
+    @Test
+    public void test() {
+        NodeRepositoryTester tester = new NodeRepositoryTester();
+        tester.curator().setZooKeeperEnsembleConnectionSpec("server1:1234,server2:5678");
+        ZooKeeperAccessMaintainer maintainer = new ZooKeeperAccessMaintainer(tester.nodeRepository(), 
+                                                                             tester.curator(), Duration.ofHours(1), new JobControl(tester.nodeRepository().database()));
+        assertTrue(ZooKeeperServer.getAllowedClientHostnames().isEmpty());
+        maintainer.maintain();
+        assertTrue("We don't restrict to only config servers", ZooKeeperServer.getAllowedClientHostnames().isEmpty());
+
+        tester.addNode("id1", "host1", "default", NodeType.tenant);
+        tester.addNode("id2", "host2", "default", NodeType.tenant);
+        tester.addNode("id3", "host3", "default", NodeType.tenant);
+        maintainer.maintain();
+
+        assertEquals(3, tester.getNodes(NodeType.tenant).size());
+        assertEquals(0, tester.getNodes(NodeType.proxy).size());
+        assertEquals(asSet("host1,host2,host3,server1,server2"), ZooKeeperServer.getAllowedClientHostnames());
+
+        tester.addNode("proxy1", "host4", "default", NodeType.proxy);
+        tester.addNode("proxy2", "host5", "default", NodeType.proxy);
+        maintainer.maintain();
+
+        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());
+
+        tester.nodeRepository().park("host2", Agent.system, "Parking to unit test");
+        tester.nodeRepository().removeRecursively("host2");
+        maintainer.maintain();
+
+        assertEquals(2, tester.getNodes(NodeType.tenant).size());
+        assertEquals(2, tester.getNodes(NodeType.proxy).size());
+        assertEquals(asSet("host1,host3,host4,host5,server1,server2"), ZooKeeperServer.getAllowedClientHostnames());
+
+        tester.addNode("docker-host-1", "host6", "default", NodeType.host);
+        tester.addNode("docker-host-2", "host7", "default", NodeType.host);
+        maintainer.maintain();
+        assertEquals(asSet("host1,host3,host4,host5,host6,host7,server1,server2"), ZooKeeperServer.getAllowedClientHostnames());
+    }
+
+    private Set<String> asSet(String s) {
+        return new HashSet<>(Arrays.asList(s.split(",")));
+    }
+
+}
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/maintenance.json b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/maintenance.json
index 28e28f9678e..979d846704e 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/maintenance.json
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/maintenance.json
@@ -34,6 +34,9 @@
       "name":"MetricsReporter"
     },
     {
+      "name":"ZooKeeperAccessMaintainer"
+    },
+    {
       "name":"NodeFailer"
     }
   ],
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 d7f42c7e6e9..a0c8b845aca 100644
--- a/zkfacade/src/main/java/com/yahoo/vespa/zookeeper/RestrictedServerCnxnFactory.java
+++ b/zkfacade/src/main/java/com/yahoo/vespa/zookeeper/RestrictedServerCnxnFactory.java
@@ -71,9 +71,9 @@ public class RestrictedServerCnxnFactory extends NIOServerCnxnFactory {
         return ZooKeeperServer.getAllowedClientHostnames();
     }
 
-    private Set<String> toHostnameSet(String hostnamesString) {
+    private Set<String> toHostnameSet(String hosatnamesString) {
         Set<String> hostnames = new HashSet<>();
-        for (String hostname : StringUtilities.split(hostnamesString)) {
+        for (String hostname : StringUtilities.split(hosatnamesString)) {
             if ( ! hostname.trim().isEmpty())
                 hostnames.add(hostname.trim());
         }
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 352635ac920..74f9d01b833 100644
--- a/zkfacade/src/main/java/com/yahoo/vespa/zookeeper/ZooKeeperServer.java
+++ b/zkfacade/src/main/java/com/yahoo/vespa/zookeeper/ZooKeeperServer.java
@@ -3,7 +3,6 @@ package com.yahoo.vespa.zookeeper;
 
 import com.google.common.collect.ImmutableSet;
 import com.google.inject.Inject;
-import com.yahoo.cloud.config.ConfigserverConfig;
 import com.yahoo.cloud.config.ZookeeperServerConfig;
 import com.yahoo.component.AbstractComponent;
 import com.yahoo.log.LogLevel;
@@ -11,41 +10,40 @@ import static com.yahoo.vespa.defaults.Defaults.getDefaults;
 
 import java.io.FileWriter;
 import java.io.IOException;
+import java.util.Collection;
 import java.util.List;
-import java.util.Set;
+import java.util.Optional;
 import java.util.stream.Collectors;
 
 /**
  * Writes zookeeper config and starts zookeeper server.
  *
- * @author Ulf Lilleengen
+ * @author lulf
+ * @since 5.3
  */
 public class ZooKeeperServer extends AbstractComponent implements Runnable {
 
     /** 
      * The set of hosts which can access the ZooKeeper server in this VM, or empty
      * to allow access from anywhere.
-     * This belongs logically to the server instance and is final, but must be static to make it accessible
+     * 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 ImmutableSet<String> allowedClientHostnames = ImmutableSet.of();
+    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";
     static final String ZOOKEEPER_JUTE_MAX_BUFFER = "jute.maxbuffer";
     private final Thread zkServerThread;
-    private final ZookeeperServerConfig zookeeperServerConfig;
+    private final ZookeeperServerConfig config;
 
-    ZooKeeperServer(ZookeeperServerConfig zookeeperServerConfig, ConfigserverConfig configserverConfig, boolean startServer) {
-        this.zookeeperServerConfig = zookeeperServerConfig;
+    ZooKeeperServer(ZookeeperServerConfig config, boolean startServer) {
+        this.config = config;
         System.setProperty("zookeeper.jmx.log4j.disable", "true");
-        System.setProperty(ZOOKEEPER_JUTE_MAX_BUFFER, "" + zookeeperServerConfig.juteMaxBuffer());
+        System.setProperty(ZOOKEEPER_JUTE_MAX_BUFFER, "" + config.juteMaxBuffer());
         System.setProperty("zookeeper.serverCnxnFactory", "com.yahoo.vespa.zookeeper.RestrictedServerCnxnFactory");
 
-        if (configserverConfig.hostedVespa()) // restrict access to config servers only
-            allowedClientHostnames =  ImmutableSet.copyOf(zookeeperServerHostnames(zookeeperServerConfig));
-
-        writeConfigToDisk(zookeeperServerConfig);
+        writeConfigToDisk(config);
         zkServerThread = new Thread(this, "zookeeper server");
         if (startServer) {
             zkServerThread.start();
@@ -53,10 +51,15 @@ public class ZooKeeperServer extends AbstractComponent implements Runnable {
     }
 
     @Inject
-    public ZooKeeperServer(ZookeeperServerConfig zookeeperServerConfig, ConfigserverConfig configserverConfig) {
-        this(zookeeperServerConfig, configserverConfig, true);
+    public ZooKeeperServer(ZookeeperServerConfig config) {
+        this(config, true);
     }
-
+    
+    /** Restrict access to this ZooKeeper server to the given client hosts */
+    public static void setAllowedClientHostnames(Collection<String> 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 ImmutableSet<String> getAllowedClientHostnames() { return allowedClientHostnames; }
     
@@ -127,9 +130,10 @@ public class ZooKeeperServer extends AbstractComponent implements Runnable {
     @Override
     public void run() {
         System.setProperty(ZOOKEEPER_JMX_LOG4J_DISABLE, "true");
-        String[] args = new String[]{getDefaults().underVespaHome(zookeeperServerConfig.zooKeeperConfigFile())};
+        String[] args = new String[]{getDefaults().underVespaHome(config.zooKeeperConfigFile())};
         log.log(LogLevel.DEBUG, "Starting ZooKeeper server with config: " + args[0]);
-        log.log(LogLevel.INFO, "Trying to establish ZooKeeper quorum (from " + zookeeperServerHostnames(zookeeperServerConfig) + ")");
+        log.log(LogLevel.INFO, "Trying to establish ZooKeeper quorum (from " +
+                config.server().stream().map(ZookeeperServerConfig.Server::hostname).collect(Collectors.toList()) + ")");
         org.apache.zookeeper.server.quorum.QuorumPeerMain.main(args);
     }
 
@@ -139,10 +143,6 @@ public class ZooKeeperServer extends AbstractComponent implements Runnable {
         super.deconstruct();
     }
 
-    public ZookeeperServerConfig getZookeeperServerConfig() { return zookeeperServerConfig; }
-
-    private static Set<String> zookeeperServerHostnames(ZookeeperServerConfig zookeeperServerConfig) {
-        return zookeeperServerConfig.server().stream().map(ZookeeperServerConfig.Server::hostname).collect(Collectors.toSet());
-    }
+    public ZookeeperServerConfig getConfig() { return config; }
 
 }
diff --git a/zkfacade/src/test/java/com/yahoo/vespa/zookeeper/ZooKeeperServerTest.java b/zkfacade/src/test/java/com/yahoo/vespa/zookeeper/ZooKeeperServerTest.java
index 626e5bf0627..8dd33f3d744 100644
--- a/zkfacade/src/test/java/com/yahoo/vespa/zookeeper/ZooKeeperServerTest.java
+++ b/zkfacade/src/test/java/com/yahoo/vespa/zookeeper/ZooKeeperServerTest.java
@@ -1,7 +1,6 @@
 // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.zookeeper;
 
-import com.yahoo.cloud.config.ConfigserverConfig;
 import com.yahoo.cloud.config.ZookeeperServerConfig;
 import com.yahoo.io.IOUtils;
 import org.junit.Rule;
@@ -54,11 +53,11 @@ public class ZooKeeperServerTest {
     }
 
     private void createServer(ZookeeperServerConfig.Builder builder) {
-        new ZooKeeperServer(new ZookeeperServerConfig(builder), new ConfigserverConfig(new ConfigserverConfig.Builder()), false);
+        new ZooKeeperServer(new ZookeeperServerConfig(builder), false);
     }
 
     @Test(expected = RuntimeException.class)
-    public void require_that_this_id_must_be_present_amongst_servers() {
+    public void require_that_this_id_must_be_present_amongst_servers() throws IOException {
         ZookeeperServerConfig.Builder builder = new ZookeeperServerConfig.Builder();
         builder.server(newServer(2, "bar", 234, 432));
         builder.server(newServer(3, "baz", 345, 543));