Merge pull request #16962 from vespa-engine/bjorncs/zk-server-client-port-tls-only

Bjorncs/zk server client port tls only [run-systemtest]

3 files changed, 41 insertions, 24 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..9f2144966e0 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) {
@@ -154,7 +152,7 @@ public class Configurator {
 
         String configFieldPrefix();
 
-        default void appendTlsConfig(StringBuilder builder, ZookeeperServerConfig config, Optional<TlsContext> tlsContext) {
+        default void appendTlsConfig(StringBuilder builder, Optional<TlsContext> tlsContext) {
             tlsContext.ifPresent(ctx -> {
                 builder.append(configFieldPrefix()).append(".context.supplier.class=").append(VespaSslContextProvider.class.getName()).append("\n");
                 String enabledCiphers = Arrays.stream(ctx.parameters().getCipherSuites()).sorted().collect(Collectors.joining(","));
@@ -176,22 +174,29 @@ 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"));
-            appendTlsConfig(sb, config, tlsContext);
+            // 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, tlsContext);
 
             return sb.toString();
         }
@@ -234,7 +239,7 @@ public class Configurator {
             }
             sb.append("sslQuorum=").append(sslQuorum).append("\n");
             sb.append("portUnification=").append(portUnification).append("\n");
-            appendTlsConfig(sb, config, tlsContext);
+            appendTlsConfig(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 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);
     }