Another attempt at configuring secure only client port on ZK server

ZK dynamic reconfiguration logic assumes that insecure client port exists.
This commit introduces a new connection factory that overrides 'secure'
flag from configure() and makes the insecure client port become secure.

3 files changed, 60 insertions, 30 deletions

diff --git a/zookeeper-server/zookeeper-server-3.6.2/src/main/java/org/apache/zookeeper/server/VespaNettyServerCnxnFactory.java b/zookeeper-server/zookeeper-server-3.6.2/src/main/java/org/apache/zookeeper/server/VespaNettyServerCnxnFactory.java
new file mode 100644
index 00000000000..7efec454667
--- /dev/null
+++ b/zookeeper-server/zookeeper-server-3.6.2/src/main/java/org/apache/zookeeper/server/VespaNettyServerCnxnFactory.java
@@ -0,0 +1,37 @@
+// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+package org.apache.zookeeper.server;
+
+import com.yahoo.vespa.zookeeper.Configurator;
+
+import java.io.IOException;
+import java.net.InetSocketAddress;
+import java.util.logging.Logger;
+
+/**
+ * Overrides secure setting with value from {@link Configurator}.
+ * Workaround for incorrect handling of clientSecurePort in combination with ZooKeeper Dynamic Reconfiguration in 3.6.2
+ * See https://issues.apache.org/jira/browse/ZOOKEEPER-3577.
+ *
+ * Using package {@link org.apache.zookeeper.server} as {@link NettyServerCnxnFactory#NettyServerCnxnFactory()} is package-private.
+ *
+ * @author bjorncs
+ */
+public class VespaNettyServerCnxnFactory extends NettyServerCnxnFactory {
+
+    private static final Logger log = Logger.getLogger(VespaNettyServerCnxnFactory.class.getName());
+
+    private final boolean isSecure;
+
+    public VespaNettyServerCnxnFactory() {
+        super();
+        this.isSecure = Configurator.VespaNettyServerCnxnFactory_isSecure;
+        boolean portUnificationEnabled = Boolean.getBoolean(NettyServerCnxnFactory.PORT_UNIFICATION_KEY);
+        log.info(String.format("For %h: isSecure=%b, portUnification=%b", this, isSecure, portUnificationEnabled));
+    }
+
+    @Override
+    public void configure(InetSocketAddress addr, int maxClientCnxns, int backlog, boolean secure) throws IOException {
+        log.info(String.format("For %h: configured() invoked with parameter 'secure'=%b, overridden to %b", this, secure, isSecure));
+        super.configure(addr, maxClientCnxns, backlog, isSecure);
+    }
+}
diff --git a/zookeeper-server/zookeeper-server-common/src/main/java/com/yahoo/vespa/zookeeper/Configurator.java b/zookeeper-server/zookeeper-server-common/src/main/java/com/yahoo/vespa/zookeeper/Configurator.java
index c58c97845e3..d662bab8463 100644
--- a/zookeeper-server/zookeeper-server-common/src/main/java/com/yahoo/vespa/zookeeper/Configurator.java
+++ b/zookeeper-server/zookeeper-server-common/src/main/java/com/yahoo/vespa/zookeeper/Configurator.java
@@ -20,6 +20,8 @@ import static com.yahoo.vespa.defaults.Defaults.getDefaults;
 
 public class Configurator {
 
+    public static volatile boolean VespaNettyServerCnxnFactory_isSecure = false;
+
     private static final java.util.logging.Logger log = java.util.logging.Logger.getLogger(Configurator.class.getName());
     private static final String ZOOKEEPER_JMX_LOG4J_DISABLE = "zookeeper.jmx.log4j.disable";
     static final String ZOOKEEPER_JUTE_MAX_BUFFER = "jute.maxbuffer";
@@ -70,14 +72,14 @@ public class Configurator {
         // Includes all available commands in 3.5, except 'wchc' and 'wchp'
         sb.append("4lw.commands.whitelist=conf,cons,crst,dirs,dump,envi,mntr,ruok,srst,srvr,stat,wchs").append("\n");
         sb.append("admin.enableServer=false").append("\n");
-        // Need NettyServerCnxnFactory to be able to use TLS for communication
-        sb.append("serverCnxnFactory=org.apache.zookeeper.server.NettyServerCnxnFactory").append("\n");
+        // Use custom connection factory for TLS on client port - see class' Javadoc for rationale
+        sb.append("serverCnxnFactory=org.apache.zookeeper.server.VespaNettyServerCnxnFactory").append("\n");
         sb.append("quorumListenOnAllIPs=true").append("\n");
         sb.append("standaloneEnabled=false").append("\n");
         sb.append("reconfigEnabled=true").append("\n");
         sb.append("skipACL=yes").append("\n");
         ensureThisServerIsRepresented(config.myid(), config.server());
-        config.server().forEach(server -> addServerToCfg(sb, server));
+        config.server().forEach(server -> addServerToCfg(sb, server, config.clientPort()));
         sb.append(new TlsQuorumConfig().createConfig(config, tlsContext));
         sb.append(new TlsClientServerConfig().createConfig(config, tlsContext));
         return sb.toString();
@@ -102,7 +104,7 @@ public class Configurator {
         }
     }
 
-    private void addServerToCfg(StringBuilder sb, ZookeeperServerConfig.Server server) {
+    private void addServerToCfg(StringBuilder sb, ZookeeperServerConfig.Server server, int clientPort) {
         sb.append("server.")
           .append(server.id())
           .append("=")
@@ -120,7 +122,9 @@ public class Configurator {
             sb.append(":")
               .append("observer");
         }
-        sb.append("\n");
+        sb.append(";")
+          .append(clientPort)
+          .append("\n");
     }
 
     static List<String> zookeeperServerHostnames(ZookeeperServerConfig zookeeperServerConfig) {
@@ -190,12 +194,11 @@ public class Configurator {
                 default:
                     throw new IllegalArgumentException("Unknown value of config setting tlsForClientServerCommunication: " + tlsSetting);
             }
-            // ZooKeeper Dynamic Reconfiguration does not support SSL/secure client port
-            // The secure client port must be configured in the static configuration section instead
+            sb.append("client.portUnification=").append(portUnification).append("\n");
+            // ZooKeeper Dynamic Reconfiguration requires the "non-secure" client port to exist
+            // This is a hack to override the secure parameter through our connection factory wrapper
             // https://issues.apache.org/jira/browse/ZOOKEEPER-3577
-            sb.append("client.portUnification=").append(portUnification).append("\n")
-                    .append("clientPort=").append(secureClientPort ? 0 : config.clientPort()).append("\n")
-                    .append("secureClientPort=").append(secureClientPort ? config.clientPort() : 0).append("\n");
+            VespaNettyServerCnxnFactory_isSecure = secureClientPort;
             appendSharedTlsConfig(sb, tlsContext);
 
             return sb.toString();
diff --git a/zookeeper-server/zookeeper-server-common/src/test/java/com/yahoo/vespa/zookeeper/ConfiguratorTest.java b/zookeeper-server/zookeeper-server-common/src/test/java/com/yahoo/vespa/zookeeper/ConfiguratorTest.java
index f0d3df43c4e..47fed6fceac 100644
--- a/zookeeper-server/zookeeper-server-common/src/test/java/com/yahoo/vespa/zookeeper/ConfiguratorTest.java
+++ b/zookeeper-server/zookeeper-server-common/src/test/java/com/yahoo/vespa/zookeeper/ConfiguratorTest.java
@@ -171,7 +171,7 @@ public class ConfiguratorTest {
                "autopurge.snapRetainCount=15\n" +
                "4lw.commands.whitelist=conf,cons,crst,dirs,dump,envi,mntr,ruok,srst,srvr,stat,wchs\n" +
                "admin.enableServer=false\n" +
-               "serverCnxnFactory=org.apache.zookeeper.server.NettyServerCnxnFactory\n" +
+               "serverCnxnFactory=org.apache.zookeeper.server.VespaNettyServerCnxnFactory\n" +
                "quorumListenOnAllIPs=true\n" +
                "standaloneEnabled=false\n" +
                "reconfigEnabled=true\n" +
@@ -181,12 +181,10 @@ public class ConfiguratorTest {
     private void validateConfigFileSingleHost(File cfgFile) {
         String expected =
                 commonConfig() +
-                "server.0=foo:321:123\n" +
+                "server.0=foo:321:123;2181\n" +
                 "sslQuorum=false\n" +
                 "portUnification=false\n" +
-                "client.portUnification=false\n" +
-                "clientPort=2181\n" +
-                "secureClientPort=0\n";
+                "client.portUnification=false\n";
         validateConfigFile(cfgFile, expected);
     }
 
@@ -209,27 +207,23 @@ public class ConfiguratorTest {
     private void validateConfigFileMultipleHosts(File cfgFile) {
         String expected =
                 commonConfig() +
-                "server.0=foo:321:123\n" +
-                "server.1=bar:432:234\n" +
-                "server.2=baz:543:345:observer\n" +
+                "server.0=foo:321:123;2181\n" +
+                "server.1=bar:432:234;2181\n" +
+                "server.2=baz:543:345:observer;2181\n" +
                 "sslQuorum=false\n" +
                 "portUnification=false\n" +
-                "client.portUnification=false\n" +
-                "clientPort=2181\n" +
-                "secureClientPort=0\n";
+                "client.portUnification=false\n";
         validateConfigFile(cfgFile, expected);
     }
 
     private void validateConfigFilePortUnification(File cfgFile) {
         String expected =
                 commonConfig() +
-                "server.0=foo:321:123\n" +
+                "server.0=foo:321:123;2181\n" +
                 "sslQuorum=false\n" +
                 "portUnification=true\n" +
                 tlsQuorumConfig() +
                 "client.portUnification=true\n" +
-                "clientPort=2181\n" +
-                "secureClientPort=0\n" +
                 tlsClientServerConfig();
         validateConfigFile(cfgFile, expected);
     }
@@ -237,13 +231,11 @@ public class ConfiguratorTest {
     private void validateConfigFileTlsWithPortUnification(File cfgFile) {
         String expected =
                 commonConfig() +
-                "server.0=foo:321:123\n" +
+                "server.0=foo:321:123;2181\n" +
                 "sslQuorum=true\n" +
                 "portUnification=true\n" +
                 tlsQuorumConfig() +
                 "client.portUnification=true\n" +
-                "clientPort=2181\n" +
-                "secureClientPort=0\n" +
                 tlsClientServerConfig();
         validateConfigFile(cfgFile, expected);
     }
@@ -251,13 +243,11 @@ public class ConfiguratorTest {
     private void validateConfigFileTlsOnly(File cfgFile) {
         String expected =
                 commonConfig() +
-                "server.0=foo:321:123\n" +
+                "server.0=foo:321:123;2181\n" +
                 "sslQuorum=true\n" +
                 "portUnification=false\n" +
                 tlsQuorumConfig() +
                 "client.portUnification=false\n" +
-                "clientPort=0\n" +
-                "secureClientPort=2181\n" +
                 tlsClientServerConfig();
         validateConfigFile(cfgFile, expected);
     }