From 69e0b6c9392996ca61ad42f454986a30a432ee3f Mon Sep 17 00:00:00 2001
From: Alex Rankin <davelister@gmail.com>
Date: Fri, 18 Jan 2019 08:57:43 +0000
Subject: [PATCH] Updating custom SSLContext supplier with review comments

---
 .../java/org/apache/zookeeper/common/X509Util.java   |  8 +++++---
 .../java/org/apache/zookeeper/common/ZKConfig.java   | 12 ++++++------
 .../org/apache/zookeeper/common/X509UtilTest.java    | 12 ++++++------
 3 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java b/zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java
index c42dc4ea115..69cb7ef6288 100644
--- a/zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java
+++ b/zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java
@@ -241,9 +241,11 @@ public int getSslHandshakeTimeoutMillis() {
 
     @SuppressWarnings("unchecked")
     public SSLContext createSSLContext(ZKConfig config) throws SSLContextException {
-        if (config.getProperty(sslContextSupplierClassProperty) != null) {
-            LOG.debug("Loading SSLContext supplier from property '" + sslContextSupplierClassProperty + "'");
-            String supplierContextClassName = config.getProperty(sslContextSupplierClassProperty);
+        final String supplierContextClassName = config.getProperty(sslContextSupplierClassProperty);
+        if (supplierContextClassName != null) {
+            if (LOG.isDebugEnabled()) {
+                LOG.debug("Loading SSLContext supplier from property '{}'", sslContextSupplierClassProperty);
+            }
             try {
                 Class<?> sslContextClass = Class.forName(supplierContextClassName);
                 Supplier<SSLContext> sslContextSupplier = (Supplier<SSLContext>) sslContextClass.getConstructor().newInstance();
diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/common/ZKConfig.java b/zookeeper-server/src/main/java/org/apache/zookeeper/common/ZKConfig.java
index 658afb79471..533abbd83f2 100644
--- a/zookeeper-server/src/main/java/org/apache/zookeeper/common/ZKConfig.java
+++ b/zookeeper-server/src/main/java/org/apache/zookeeper/common/ZKConfig.java
@@ -18,12 +18,6 @@
 
 package org.apache.zookeeper.common;
 
-import org.apache.zookeeper.Environment;
-import org.apache.zookeeper.server.quorum.QuorumPeerConfig.ConfigException;
-import org.apache.zookeeper.server.util.VerifyingFileFactory;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-
 import java.io.File;
 import java.io.FileInputStream;
 import java.io.IOException;
@@ -32,6 +26,12 @@
 import java.util.Map.Entry;
 import java.util.Properties;
 
+import org.apache.zookeeper.Environment;
+import org.apache.zookeeper.server.quorum.QuorumPeerConfig.ConfigException;
+import org.apache.zookeeper.server.util.VerifyingFileFactory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
 /**
  * This class is a base class for the configurations of both client and server.
  * It supports reading client configuration from both system properties and
diff --git a/zookeeper-server/src/test/java/org/apache/zookeeper/common/X509UtilTest.java b/zookeeper-server/src/test/java/org/apache/zookeeper/common/X509UtilTest.java
index 0b26609b4c7..f273b867f81 100644
--- a/zookeeper-server/src/test/java/org/apache/zookeeper/common/X509UtilTest.java
+++ b/zookeeper-server/src/test/java/org/apache/zookeeper/common/X509UtilTest.java
@@ -51,7 +51,7 @@
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.function.Supplier;
 
-import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNull;
 
 @RunWith(Parameterized.class)
@@ -419,18 +419,18 @@ public void testCreateSSLContext_invalidCustomSSLContextClass() throws Exception
     public void testCreateSSLContext_validNullCustomSSLContextClass() throws X509Exception.SSLContextException {
         ZKConfig zkConfig = new ZKConfig();
         ClientX509Util clientX509Util = new ClientX509Util();
-        zkConfig.setProperty(clientX509Util.getSslContextSupplierClassProperty(), X509UtilTest.class.getCanonicalName() + "$" + NullSslContextSupplier.class.getSimpleName());
+        zkConfig.setProperty(clientX509Util.getSslContextSupplierClassProperty(), NullSslContextSupplier.class.getName());
         final SSLContext sslContext = clientX509Util.createSSLContext(zkConfig);
         assertNull(sslContext);
     }
 
     @Test
-    public void testCreateSSLContext_validCustomSSLContextClass() throws X509Exception.SSLContextException {
+    public void testCreateSSLContext_validCustomSSLContextClass() throws Exception {
         ZKConfig zkConfig = new ZKConfig();
         ClientX509Util clientX509Util = new ClientX509Util();
-        zkConfig.setProperty(clientX509Util.getSslContextSupplierClassProperty(), X509UtilTest.class.getCanonicalName() + "$" + SslContextSupplier.class.getSimpleName());
+        zkConfig.setProperty(clientX509Util.getSslContextSupplierClassProperty(), SslContextSupplier.class.getName());
         final SSLContext sslContext = clientX509Util.createSSLContext(zkConfig);
-        assertNotNull(sslContext);
+        assertEquals(SSLContext.getDefault(), sslContext);
     }
 
     private static void forceClose(Socket s) {
@@ -535,7 +535,7 @@ public SSLContext get() {
             try {
                 return SSLContext.getDefault();
             } catch (NoSuchAlgorithmException e) {
-                return null;
+                throw new RuntimeException(e);
             }
         }