Handle client port that is configured to TLS only

The client port can no longer be distributed through the ZK dynamic
reconfiguration as the protocol does not support SSL client port.
The port must be configured through the static config section instead.

3 files changed, 38 insertions, 21 deletions

diff --git a/configdefinitions/src/vespa/zookeeper-server.def b/configdefinitions/src/vespa/zookeeper-server.def
index 536cd993105..006e266916c 100644
--- a/configdefinitions/src/vespa/zookeeper-server.def
+++ b/configdefinitions/src/vespa/zookeeper-server.def
@@ -16,6 +16,7 @@ maxClientConnections int default=0
 dataDir string default="var/zookeeper"
 
 clientPort int default=2181
+# TODO(bjorncs): remove setting - no longer in use
 secureClientPort int default=2184
 
 snapshotCount int default=50000
@@ -42,7 +43,9 @@ server[].joining bool default=false
 trustEmptySnapshot bool default=true
 
 # TLS options
+# TODO(bjorncs): todo cleanup after migrating to unified Vespa TLS configuration
 tlsForQuorumCommunication enum { OFF, PORT_UNIFICATION, TLS_WITH_PORT_UNIFICATION, TLS_ONLY } default=OFF
+# TODO(bjorncs): todo cleanup after migrating to unified Vespa TLS configuration
 tlsForClientServerCommunication enum { OFF, PORT_UNIFICATION, TLS_WITH_PORT_UNIFICATION, TLS_ONLY } default=OFF
 # TODO(bjorncs): remove setting - no longer in use
 jksKeyStoreFile string default="conf/zookeeper/zookeeper.jks"
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 aff4bb950f6..3c56741adfb 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
@@ -76,7 +76,7 @@ public class Configurator {
         sb.append("skipACL=yes").append("\n");
         sb.append("metricsProvider.className=org.apache.zookeeper.metrics.impl.NullMetricsProvider\n");
         ensureThisServerIsRepresented(config.myid(), config.server());
-        config.server().forEach(server -> addServerToCfg(sb, server, config.clientPort()));
+        config.server().forEach(server -> addServerToCfg(sb, server));
         sb.append(new TlsQuorumConfig().createConfig(config, tlsContext));
         sb.append(new TlsClientServerConfig().createConfig(config, tlsContext));
         return sb.toString();
@@ -101,7 +101,7 @@ public class Configurator {
         }
     }
 
-    private void addServerToCfg(StringBuilder sb, ZookeeperServerConfig.Server server, int clientPort) {
+    private void addServerToCfg(StringBuilder sb, ZookeeperServerConfig.Server server) {
         sb.append("server.")
           .append(server.id())
           .append("=")
@@ -119,9 +119,7 @@ public class Configurator {
             sb.append(":")
               .append("observer");
         }
-        sb.append(";")
-          .append(clientPort)
-          .append("\n");
+        sb.append("\n");
     }
 
     static List<String> zookeeperServerHostnames(ZookeeperServerConfig zookeeperServerConfig) {
@@ -176,21 +174,28 @@ public class Configurator {
 
             StringBuilder sb = new StringBuilder();
             boolean portUnification;
+            boolean secureClientPort;
             switch (tlsSetting) {
                 case "OFF":
+                    secureClientPort = false; portUnification = false;
+                    break;
                 case "TLS_ONLY":
-                    portUnification = false;
+                    secureClientPort = true; portUnification = false;
                     break;
                 case "PORT_UNIFICATION":
                 case "TLS_WITH_PORT_UNIFICATION":
-                    portUnification = true;
+                    secureClientPort = false; portUnification = true;
                     break;
                 default:
                     throw new IllegalArgumentException("Unknown value of config setting tlsForClientServerCommunication: " + tlsSetting);
             }
-            sb.append("client.portUnification=").append(portUnification).append("\n");
-            // TODO This should override "clientPort" if TLS enabled without port unification);
-            tlsContext.ifPresent(ctx -> sb.append("secureClientPort=").append(config.secureClientPort()).append("\n"));
+            // ZooKeeper Dynamic Reconfiguration does not support SSL/secure client port
+            // The secure client port must be configured in the static configuration section instead
+            // 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");
+
             appendTlsConfig(sb, config, 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 aee44c90cbf..147b61a804c 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
@@ -182,10 +182,12 @@ public class ConfiguratorTest {
     private void validateConfigFileSingleHost(File cfgFile) {
         String expected =
                 commonConfig() +
-                "server.0=foo:321:123;2181\n" +
+                "server.0=foo:321:123\n" +
                 "sslQuorum=false\n" +
                 "portUnification=false\n" +
-                "client.portUnification=false\n";
+                "client.portUnification=false\n" +
+                "clientPort=2181\n" +
+                "secureClientPort=0\n";
         validateConfigFile(cfgFile, expected);
     }
 
@@ -198,8 +200,7 @@ public class ConfiguratorTest {
     }
 
     private String tlsClientServerConfig() {
-        return "secureClientPort=2184\n" +
-                "ssl.context.supplier.class=com.yahoo.vespa.zookeeper.VespaSslContextProvider\n" +
+        return "ssl.context.supplier.class=com.yahoo.vespa.zookeeper.VespaSslContextProvider\n" +
                 "ssl.ciphersuites=TLS_AES_128_GCM_SHA256,TLS_AES_256_GCM_SHA384,TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256," +
                 "TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384\n" +
                 "ssl.enabledProtocols=TLSv1.2\n" +
@@ -209,23 +210,27 @@ public class ConfiguratorTest {
     private void validateConfigFileMultipleHosts(File cfgFile) {
         String expected =
                 commonConfig() +
-                "server.0=foo:321:123;2181\n" +
-                "server.1=bar:432:234;2181\n" +
-                "server.2=baz:543:345:observer;2181\n" +
+                "server.0=foo:321:123\n" +
+                "server.1=bar:432:234\n" +
+                "server.2=baz:543:345:observer\n" +
                 "sslQuorum=false\n" +
                 "portUnification=false\n" +
-                "client.portUnification=false\n";
+                "client.portUnification=false\n" +
+                "clientPort=2181\n" +
+                "secureClientPort=0\n";
         validateConfigFile(cfgFile, expected);
     }
 
     private void validateConfigFilePortUnification(File cfgFile) {
         String expected =
                 commonConfig() +
-                "server.0=foo:321:123;2181\n" +
+                "server.0=foo:321:123\n" +
                 "sslQuorum=false\n" +
                 "portUnification=true\n" +
                 tlsQuorumConfig() +
                 "client.portUnification=true\n" +
+                "clientPort=2181\n" +
+                "secureClientPort=0\n" +
                 tlsClientServerConfig();
         validateConfigFile(cfgFile, expected);
     }
@@ -233,11 +238,13 @@ public class ConfiguratorTest {
     private void validateConfigFileTlsWithPortUnification(File cfgFile) {
         String expected =
                 commonConfig() +
-                "server.0=foo:321:123;2181\n" +
+                "server.0=foo:321:123\n" +
                 "sslQuorum=true\n" +
                 "portUnification=true\n" +
                 tlsQuorumConfig() +
                 "client.portUnification=true\n" +
+                "clientPort=2181\n" +
+                "secureClientPort=0\n" +
                 tlsClientServerConfig();
         validateConfigFile(cfgFile, expected);
     }
@@ -245,11 +252,13 @@ public class ConfiguratorTest {
     private void validateConfigFileTlsOnly(File cfgFile) {
         String expected =
                 commonConfig() +
-                "server.0=foo:321:123;2181\n" +
+                "server.0=foo:321:123\n" +
                 "sslQuorum=true\n" +
                 "portUnification=false\n" +
                 tlsQuorumConfig() +
                 "client.portUnification=false\n" +
+                "clientPort=0\n" +
+                "secureClientPort=2181\n" +
                 tlsClientServerConfig();
         validateConfigFile(cfgFile, expected);
     }