Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/679#discussion_r233661888
--- Diff:
zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/UnifiedServerSocketTest.java
---
@@ -17,156 +17,644 @@
*/
package org.apache.zookeeper.server.quorum;
+import java.io.BufferedInputStream;
+import java.io.IOException;
+import java.net.ConnectException;
+import java.net.InetSocketAddress;
+import java.net.Socket;
+import java.net.SocketException;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import java.util.Random;
+
+import javax.net.ssl.HandshakeCompletedEvent;
+import javax.net.ssl.HandshakeCompletedListener;
+import javax.net.ssl.SSLSocket;
+
import org.apache.zookeeper.PortAssignment;
import org.apache.zookeeper.client.ZKClientConfig;
+import org.apache.zookeeper.common.BaseX509ParameterizedTestCase;
import org.apache.zookeeper.common.ClientX509Util;
-import org.apache.zookeeper.common.Time;
+import org.apache.zookeeper.common.KeyStoreFileType;
+import org.apache.zookeeper.common.X509Exception;
+import org.apache.zookeeper.common.X509KeyType;
+import org.apache.zookeeper.common.X509TestContext;
import org.apache.zookeeper.common.X509Util;
import org.apache.zookeeper.server.ServerCnxnFactory;
import org.junit.Assert;
import org.junit.Before;
import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
-import javax.net.ssl.HandshakeCompletedEvent;
-import javax.net.ssl.HandshakeCompletedListener;
-import javax.net.ssl.SSLSocket;
-import java.io.IOException;
-import java.net.ConnectException;
-import java.net.InetSocketAddress;
-import java.net.Socket;
-
-import static org.hamcrest.CoreMatchers.equalTo;
-import static org.junit.Assert.assertThat;
+@RunWith(Parameterized.class)
+public class UnifiedServerSocketTest extends BaseX509ParameterizedTestCase
{
-public class UnifiedServerSocketTest {
+ @Parameterized.Parameters
+ public static Collection<Object[]> params() {
+ ArrayList<Object[]> result = new ArrayList<>();
+ int paramIndex = 0;
+ for (X509KeyType caKeyType : X509KeyType.values()) {
+ for (X509KeyType certKeyType : X509KeyType.values()) {
+ for (Boolean hostnameVerification : new Boolean[] { true,
false }) {
+ result.add(new Object[]{
+ caKeyType,
+ certKeyType,
+ hostnameVerification,
+ paramIndex++
+ });
+ }
+ }
+ }
+ return result;
+ }
private static final int MAX_RETRIES = 5;
private static final int TIMEOUT = 1000;
+ private static final byte[] DATA_TO_CLIENT = "hello client".getBytes();
+ private static final byte[] DATA_FROM_CLIENT = "hello
server".getBytes();
private X509Util x509Util;
private int port;
- private volatile boolean handshakeCompleted;
+ private InetSocketAddress localServerAddress;
+ private final Object handshakeCompletedLock = new Object();
+ // access only inside synchronized(handshakeCompletedLock) { ... }
blocks
+ private boolean handshakeCompleted = false;
+
+ public UnifiedServerSocketTest(
+ final X509KeyType caKeyType,
+ final X509KeyType certKeyType,
+ final Boolean hostnameVerification,
+ final Integer paramIndex) {
+ super(paramIndex, () -> {
+ try {
+ return X509TestContext.newBuilder()
+ .setTempDir(tempDir)
+ .setKeyStoreKeyType(certKeyType)
+ .setTrustStoreKeyType(caKeyType)
+ .setHostnameVerification(hostnameVerification)
+ .build();
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ });
+ }
@Before
public void setUp() throws Exception {
- handshakeCompleted = false;
-
port = PortAssignment.unique();
+ localServerAddress = new InetSocketAddress("localhost", port);
- String testDataPath = System.getProperty("test.data.dir",
"build/test/data");
System.setProperty(ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN_FACTORY,
"org.apache.zookeeper.server.NettyServerCnxnFactory");
System.setProperty(ZKClientConfig.ZOOKEEPER_CLIENT_CNXN_SOCKET,
"org.apache.zookeeper.ClientCnxnSocketNetty");
System.setProperty(ZKClientConfig.SECURE_CLIENT, "true");
x509Util = new ClientX509Util();
- System.setProperty(x509Util.getSslKeystoreLocationProperty(),
testDataPath + "/ssl/testKeyStore.jks");
- System.setProperty(x509Util.getSslKeystorePasswdProperty(),
"testpass");
- System.setProperty(x509Util.getSslTruststoreLocationProperty(),
testDataPath + "/ssl/testTrustStore.jks");
- System.setProperty(x509Util.getSslTruststorePasswdProperty(),
"testpass");
-
System.setProperty(x509Util.getSslHostnameVerificationEnabledProperty(),
"false");
+ x509TestContext.setSystemProperties(x509Util,
KeyStoreFileType.JKS, KeyStoreFileType.JKS);
}
- @Test
- public void testConnectWithSSL() throws Exception {
- class ServerThread extends Thread {
- public void run() {
- try {
- Socket unifiedSocket = new
UnifiedServerSocket(x509Util, port).accept();
- ((SSLSocket)unifiedSocket).getSession(); // block
until handshake completes
- } catch (IOException e) {
- e.printStackTrace();
+ private static void forceClose(java.io.Closeable s) {
+ if (s == null) {
+ return;
+ }
+ try {
+ s.close();
+ } catch (IOException e) {
+ e.printStackTrace();
+ }
+ }
+
+ private static final class UnifiedServerThread extends Thread {
+ private final byte[] dataToClient;
+ private List<byte[]> dataFromClients;
+ private List<Thread> workerThreads;
--- End diff --
Creating tests as close as possible to the original impl is nice, but
you're still testing a different thing anyway. Executors are good for dealing
with threads and I think they're preferred over manual thread creation if
acceptable. There're various different kinds of them: you can control the
thread creation behaviour, number of threads, etc. And I think it leads to more
readable code.
---
