Use a static variable to communicate

Some third party code somewhere is calling setProperties,
wiping all properties set inside the VM at random times,
so communicate through a static variable instead.

5 files changed, 45 insertions, 30 deletions

diff --git a/container-core/src/main/java/com/yahoo/container/jdisc/HttpRequest.java b/container-core/src/main/java/com/yahoo/container/jdisc/HttpRequest.java
index 7c114e0097b..f4dd655d80c 100644
--- a/container-core/src/main/java/com/yahoo/container/jdisc/HttpRequest.java
+++ b/container-core/src/main/java/com/yahoo/container/jdisc/HttpRequest.java
@@ -359,17 +359,15 @@ public class HttpRequest {
     public static HttpRequest createRequest(CurrentContainer container,
                                             URI uri, Method method, InputStream requestData,
                                             Map<String, String> properties) {
-
-        final com.yahoo.jdisc.http.HttpRequest clientRequest = com.yahoo.jdisc.http.HttpRequest
-                .newClientRequest(new Request(container, uri), uri, method);
+        com.yahoo.jdisc.http.HttpRequest clientRequest = 
+                com.yahoo.jdisc.http.HttpRequest.newClientRequest(new Request(container, uri), uri, method);
         setProperties(clientRequest, properties);
         return new HttpRequest(clientRequest, requestData);
     }
 
     private static void setProperties(com.yahoo.jdisc.http.HttpRequest clientRequest, Map<String, String> properties) {
-        if (properties == null) {
-            return;
-        }
+        if (properties == null) return;
+
         for (Map.Entry<String, String> entry : properties.entrySet()) {
             clientRequest.parameters().put(entry.getKey(), wrap(entry.getValue()));
         }
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 b94fe4288b4..208034358cf 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
@@ -7,10 +7,12 @@ 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;
 import java.util.logging.Logger;
 
 /**
- * Maintains the system property which tells ZooKeeper which nodes should have access to it.
+ * Maintains the list of hosts that should be allowed to access ZooKeeper in this runtime.
  * These are the zokeeper servers and all tenant and proxy nodes. This is maintained in the background because
  * nodes could be added or removed on another server. 
  * 
@@ -32,20 +34,17 @@ public class ZooKeeperAccessMaintainer extends Maintainer {
     
     @Override
     protected void maintain() {
-        StringBuilder hostList = new StringBuilder();
+        Set<String> hosts = new HashSet<>();
 
         for (Node node : nodeRepository().getNodes(NodeType.tenant))
-            hostList.append(node.hostname()).append(",");
+            hosts.add(node.hostname());
         for (Node node : nodeRepository().getNodes(NodeType.proxy))
-            hostList.append(node.hostname()).append(",");
+            hosts.add(node.hostname());
         for (String hostPort : curator.connectionSpec().split(","))
-            hostList.append(hostPort.split(":")[0]).append(",");
+            hosts.add(hostPort.split(":")[0]);
 
-        if (hostList.length() > 0)
-            hostList.setLength(hostList.length()-1); // remove last comma
-
-        log.fine("On " + Runtime.getRuntime().toString()+ ": Setting " + ZooKeeperServer.ZOOKEEPER_VESPA_CLIENTS_PROPERTY + " to " + hostList.toString());
-        System.setProperty(ZooKeeperServer.ZOOKEEPER_VESPA_CLIENTS_PROPERTY, hostList.toString());
+        log.fine("Restricting ZooKeeper access to " + hosts);
+        ZooKeeperServer.setAllowedClientHostnames(hosts);
     }
 
     @Override
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 89b4995e22f..9ba12fae285 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,7 @@ import java.util.Set;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertFalse;
 
 /**
  * @author bratseth
@@ -26,9 +26,9 @@ public class ZooKeeperAccessMaintainerTest {
         tester.curator().setConnectionSpec("server1:1234,server2:5678");
         ZooKeeperAccessMaintainer maintainer = new ZooKeeperAccessMaintainer(tester.nodeRepository(), 
                                                                              tester.curator(), Duration.ofHours(1));
-        assertNull(System.getProperty(ZooKeeperServer.ZOOKEEPER_VESPA_CLIENTS_PROPERTY));
+        assertFalse(ZooKeeperServer.getAllowedClientHostnames().isPresent());
         maintainer.maintain();
-        assertEquals(asSet("server1,server2"), asSet(System.getProperty(ZooKeeperServer.ZOOKEEPER_VESPA_CLIENTS_PROPERTY)));
+        assertEquals(asSet("server1,server2"), ZooKeeperServer.getAllowedClientHostnames().get());
 
         tester.addNode("id1", "host1", "default", NodeType.tenant);
         tester.addNode("id2", "host2", "default", NodeType.tenant);
@@ -37,7 +37,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"), asSet(System.getProperty(ZooKeeperServer.ZOOKEEPER_VESPA_CLIENTS_PROPERTY)));
+        assertEquals(asSet("host1,host2,host3,server1,server2"), ZooKeeperServer.getAllowedClientHostnames().get());
 
         tester.addNode("proxy1", "host4", "default", NodeType.proxy);
         tester.addNode("proxy2", "host5", "default", NodeType.proxy);
@@ -45,7 +45,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"), asSet(System.getProperty(ZooKeeperServer.ZOOKEEPER_VESPA_CLIENTS_PROPERTY)));
+        assertEquals(asSet("host1,host2,host3,host4,host5,server1,server2"), ZooKeeperServer.getAllowedClientHostnames().get());
 
         tester.nodeRepository().move("host2", Node.State.parked);
         assertTrue(tester.nodeRepository().remove("host2"));
@@ -53,7 +53,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"), asSet(System.getProperty(ZooKeeperServer.ZOOKEEPER_VESPA_CLIENTS_PROPERTY)));
+        assertEquals(asSet("host1,host3,host4,host5,server1,server2"), ZooKeeperServer.getAllowedClientHostnames().get());
     }
 
     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 51c9ef681a1..1c60587b6e4 100644
--- a/zkfacade/src/main/java/com/yahoo/vespa/zookeeper/RestrictedServerCnxnFactory.java
+++ b/zkfacade/src/main/java/com/yahoo/vespa/zookeeper/RestrictedServerCnxnFactory.java
@@ -1,5 +1,6 @@
 package com.yahoo.vespa.zookeeper;
 
+import com.google.common.collect.ImmutableSet;
 import org.apache.zookeeper.server.NIOServerCnxn;
 import org.apache.zookeeper.server.NIOServerCnxnFactory;
 
@@ -8,6 +9,7 @@ import java.net.InetSocketAddress;
 import java.nio.channels.SelectionKey;
 import java.nio.channels.SocketChannel;
 import java.util.HashSet;
+import java.util.Optional;
 import java.util.Set;
 import java.util.logging.Logger;
 
@@ -29,16 +31,15 @@ public class RestrictedServerCnxnFactory extends NIOServerCnxnFactory {
     protected NIOServerCnxn createConnection(SocketChannel socket, SelectionKey selection) throws IOException {
         String remoteHost = ((InetSocketAddress)socket.getRemoteAddress()).getHostName();
 
-        String zookeeperClients = System.getProperty(ZooKeeperServer.ZOOKEEPER_VESPA_CLIENTS_PROPERTY);
-        if (zookeeperClients == null || zookeeperClients.isEmpty()) {
-            log.fine("On " + Runtime.getRuntime().toString() + ": Allowing connection to ZooKeeper from " + remoteHost + ", as " + ZooKeeperServer.ZOOKEEPER_VESPA_CLIENTS_PROPERTY + " is not set");
+        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
         }
 
-        Set<String> zooKeeperClients = toHostnameSet(zookeeperClients);
-        if ( ! remoteHost.equals("localhost") && ! zooKeeperClients.contains(remoteHost)) {
+        if ( ! remoteHost.equals("localhost") && ! allowedZooKeeperClients.get().contains(remoteHost)) {
             String errorMessage = "Rejecting connection to ZooKeeper from " + remoteHost +
-                                  ": This cluster only allow connection from hosts in: " + zooKeeperClients;
+                                  ": 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);
@@ -47,7 +48,7 @@ public class RestrictedServerCnxnFactory extends NIOServerCnxnFactory {
                 log.fine("Would reject if activated: " + errorMessage);
             }
         }
-        log.fine("On " + Runtime.getRuntime().toString()+ ": Allowing connection to ZooKeeper from " + remoteHost + ", as it is in " + zookeeperClients);
+        log.fine("Allowing connection to ZooKeeper from " + remoteHost + ", as it is in " + allowedZooKeeperClients.get());
         return super.createConnection(socket, selection);
     }
 
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 f037e3c9265..f6219877f5e 100644
--- a/zkfacade/src/main/java/com/yahoo/vespa/zookeeper/ZooKeeperServer.java
+++ b/zkfacade/src/main/java/com/yahoo/vespa/zookeeper/ZooKeeperServer.java
@@ -1,6 +1,7 @@
 // Copyright 2016 Yahoo Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.zookeeper;
 
+import com.google.common.collect.ImmutableSet;
 import com.google.inject.Inject;
 import com.yahoo.cloud.config.ZookeeperServerConfig;
 import com.yahoo.component.AbstractComponent;
@@ -9,7 +10,9 @@ import com.yahoo.vespa.defaults.Defaults;
 
 import java.io.FileWriter;
 import java.io.IOException;
+import java.util.Collection;
 import java.util.List;
+import java.util.Optional;
 
 /**
  * Writes zookeeper config and starts zookeeper server.
@@ -19,7 +22,13 @@ import java.util.List;
  */
 public class ZooKeeperServer extends AbstractComponent implements Runnable {
 
-    public static final String ZOOKEEPER_VESPA_CLIENTS_PROPERTY = "zookeeper.vespa.clients";
+    /** 
+     * 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 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 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";
@@ -45,6 +54,14 @@ public class ZooKeeperServer extends AbstractComponent implements Runnable {
         this(config, true);
     }
     
+    /** Restrict access to this ZooKeeper server to the given client hosts */
+    public static void setAllowedClientHostnames(Collection<String> hostnames) {
+        allowedClientHostnames = Optional.of(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; }
+    
     private void writeConfigToDisk(ZookeeperServerConfig config) {
        String cfg = transformConfigToString(config);
        try (FileWriter writer = new FileWriter(Defaults.getDefaults().underVespaHome(config.zooKeeperConfigFile()))) {