Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ZOOKEEPER-4819 Fix can't seek for writable tls server if connected to readonly server #2200

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,10 @@
package org.apache.zookeeper;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import java.io.BufferedReader;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.io.InputStreamReader;
import java.net.ConnectException;
import java.net.InetSocketAddress;
import java.net.Socket;
import java.net.SocketAddress;
import java.nio.ByteBuffer;
import java.util.ArrayDeque;
Expand Down Expand Up @@ -67,10 +64,12 @@
import org.apache.zookeeper.ZooDefs.OpCode;
import org.apache.zookeeper.ZooKeeper.States;
import org.apache.zookeeper.ZooKeeper.WatchRegistration;
import org.apache.zookeeper.client.FourLetterWordMain;
import org.apache.zookeeper.client.HostProvider;
import org.apache.zookeeper.client.ZKClientConfig;
import org.apache.zookeeper.client.ZooKeeperSaslClient;
import org.apache.zookeeper.common.Time;
import org.apache.zookeeper.common.X509Exception;
import org.apache.zookeeper.proto.AuthPacket;
import org.apache.zookeeper.proto.ConnectRequest;
import org.apache.zookeeper.proto.Create2Response;
Expand Down Expand Up @@ -1303,42 +1302,16 @@ private void pingRwServer() throws RWServerFoundException {
InetSocketAddress addr = hostProvider.next(0);

LOG.info("Checking server {} for being r/w. Timeout {}", addr, pingRwTimeout);

Socket sock = null;
BufferedReader br = null;
try {
sock = new Socket(addr.getHostString(), addr.getPort());
sock.setSoLinger(false, -1);
sock.setSoTimeout(1000);
sock.setTcpNoDelay(true);
luoxiner marked this conversation as resolved.
Show resolved Hide resolved
sock.getOutputStream().write("isro".getBytes());
sock.getOutputStream().flush();
sock.shutdownOutput();
br = new BufferedReader(new InputStreamReader(sock.getInputStream()));
result = br.readLine();
result = FourLetterWordMain.send4LetterWord(addr.getHostString(), addr.getPort(), "isro", clientConfig, 1000);
} catch (ConnectException e) {
// ignore, this just means server is not up
} catch (IOException e) {
} catch (IOException | X509Exception.SSLContextException e) {
// some unexpected error, warn about it
LOG.warn("Exception while seeking for r/w server.", e);
} finally {
if (sock != null) {
try {
sock.close();
} catch (IOException e) {
LOG.warn("Unexpected exception", e);
}
}
if (br != null) {
try {
br.close();
} catch (IOException e) {
LOG.warn("Unexpected exception", e);
}
}
}

if ("rw".equals(result)) {
if ("rw\n".equals(result)) {
pingRwTimeout = minPingRwTimeout;
// save the found address so that it's used during the next
// connection attempt
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,33 @@ public static String send4LetterWord(
return send4LetterWord(host, port, cmd, secure, timeout, null);
}

/**
* Send the 4letterword
* @param host the destination host
* @param port the destination port
* @param cmd the 4letterword
* @param timeout in milliseconds, maximum time to wait while connecting/reading data
* @param clientConfig client config
kezhuw marked this conversation as resolved.
Show resolved Hide resolved
* @return server response
* @throws SSLContextException
* @throws IOException
*/
public static String send4LetterWord(
String host,
int port,
String cmd,
ZKClientConfig clientConfig,
int timeout) throws SSLContextException, IOException {
boolean useSecure = clientConfig.getBoolean(ZKClientConfig.SECURE_CLIENT);
SSLContext sslContext = null;
if (useSecure) {
try (X509Util x509Util = new ClientX509Util()) {
sslContext = x509Util.createSSLContext(clientConfig);
}
}
return send4LetterWord(host, port, cmd, useSecure, timeout, sslContext);
}

/**
* Send the 4letterword
* @param host the destination host
Expand Down Expand Up @@ -137,7 +164,9 @@ public static String send4LetterWord(
sock = new Socket();
sock.connect(hostaddress, timeout);
}
sock.setSoLinger(false, -1);
sock.setSoTimeout(timeout);
sock.setTcpNoDelay(true);
OutputStream outstream = sock.getOutputStream();
outstream.write(cmd.getBytes(UTF_8));
outstream.flush();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.apache.zookeeper.test;

import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;
import java.io.ByteArrayOutputStream;
import java.io.LineNumberReader;
import java.io.StringReader;
import java.util.regex.Pattern;
import org.apache.zookeeper.CreateMode;
import org.apache.zookeeper.ZKTestCase;
import org.apache.zookeeper.ZooDefs;
import org.apache.zookeeper.ZooKeeper;
import org.apache.zookeeper.client.ZKClientConfig;
import org.apache.zookeeper.common.ClientX509Util;
import org.apache.zookeeper.common.StringUtils;
import org.apache.zookeeper.server.NettyServerCnxnFactory;
import org.apache.zookeeper.server.ServerCnxnFactory;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.Timeout;

public class ReadOnlyModeWithSSLTest extends ZKTestCase {

private static int CONNECTION_TIMEOUT = QuorumBase.CONNECTION_TIMEOUT;
private QuorumUtil qu = new QuorumUtil(1);
private ClientX509Util clientX509Util;
private ZKClientConfig clientConfig;

@BeforeEach
public void setUp() throws Exception {
clientX509Util = new ClientX509Util();
String testDataPath = System.getProperty("test.data.dir", "src/test/resources/data");
clientConfig = new ZKClientConfig();
clientConfig.setProperty(ZKClientConfig.SECURE_CLIENT, "true");
clientConfig.setProperty(ZKClientConfig.ZOOKEEPER_CLIENT_CNXN_SOCKET, "org.apache.zookeeper.ClientCnxnSocketNetty");
clientConfig.setProperty(clientX509Util.getSslKeystoreLocationProperty(), testDataPath + "/ssl/testKeyStore.jks");
clientConfig.setProperty(clientX509Util.getSslKeystorePasswdProperty(), "testpass");
clientConfig.setProperty(clientX509Util.getSslTruststoreLocationProperty(), testDataPath + "/ssl/testTrustStore.jks");
clientConfig.setProperty(clientX509Util.getSslTruststorePasswdProperty(), "testpass");
System.setProperty("readonlymode.enabled", "true");
System.setProperty(NettyServerCnxnFactory.PORT_UNIFICATION_KEY, Boolean.TRUE.toString());
System.setProperty(ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN_FACTORY, "org.apache.zookeeper.server.NettyServerCnxnFactory");
System.setProperty(clientX509Util.getSslKeystoreLocationProperty(), testDataPath + "/ssl/testKeyStore.jks");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These properties are mixed between client and server. I think we should build a ZKClientConfig for client usage prior to set them up for server(there is ZOOKEEPER-2139 counterpart in server) as ZKClientConfig will inherit properties from system.

/**
* Now onwards client code will use properties from this class but older
* clients still be setting properties through system properties. So to make
* this change backward compatible we should set old system properties in
* this configuration.
*/
protected void handleBackwardCompatibility() {
properties.put(JUTE_MAXBUFFER, System.getProperty(JUTE_MAXBUFFER));
properties.put(KINIT_COMMAND, System.getProperty(KINIT_COMMAND));
properties.put(JGSS_NATIVE, System.getProperty(JGSS_NATIVE));
try (ClientX509Util clientX509Util = new ClientX509Util()) {
putSSLProperties(clientX509Util);
properties.put(clientX509Util.getSslAuthProviderProperty(), System.getProperty(clientX509Util.getSslAuthProviderProperty()));
properties.put(clientX509Util.getSslProviderProperty(), System.getProperty(clientX509Util.getSslProviderProperty()));
}
try (X509Util x509Util = new QuorumX509Util()) {
putSSLProperties(x509Util);
}
}
private void putSSLProperties(X509Util x509Util) {
properties.put(x509Util.getSslProtocolProperty(), System.getProperty(x509Util.getSslProtocolProperty()));
properties.put(x509Util.getSslEnabledProtocolsProperty(), System.getProperty(x509Util.getSslEnabledProtocolsProperty()));
properties.put(x509Util.getSslCipherSuitesProperty(), System.getProperty(x509Util.getSslCipherSuitesProperty()));
properties.put(x509Util.getSslKeystoreLocationProperty(), System.getProperty(x509Util.getSslKeystoreLocationProperty()));
properties.put(x509Util.getSslKeystorePasswdProperty(), System.getProperty(x509Util.getSslKeystorePasswdProperty()));
properties.put(x509Util.getSslKeystorePasswdPathProperty(), System.getProperty(x509Util.getSslKeystorePasswdPathProperty()));
properties.put(x509Util.getSslKeystoreTypeProperty(), System.getProperty(x509Util.getSslKeystoreTypeProperty()));
properties.put(x509Util.getSslTruststoreLocationProperty(), System.getProperty(x509Util.getSslTruststoreLocationProperty()));
properties.put(x509Util.getSslTruststorePasswdProperty(), System.getProperty(x509Util.getSslTruststorePasswdProperty()));
properties.put(x509Util.getSslTruststorePasswdPathProperty(), System.getProperty(x509Util.getSslTruststorePasswdPathProperty()));
properties.put(x509Util.getSslTruststoreTypeProperty(), System.getProperty(x509Util.getSslTruststoreTypeProperty()));
properties.put(x509Util.getSslContextSupplierClassProperty(), System.getProperty(x509Util.getSslContextSupplierClassProperty()));
properties.put(x509Util.getSslHostnameVerificationEnabledProperty(), System.getProperty(x509Util.getSslHostnameVerificationEnabledProperty()));
properties.put(x509Util.getSslCrlEnabledProperty(), System.getProperty(x509Util.getSslCrlEnabledProperty()));
properties.put(x509Util.getSslOcspEnabledProperty(), System.getProperty(x509Util.getSslOcspEnabledProperty()));
properties.put(x509Util.getSslClientAuthProperty(), System.getProperty(x509Util.getSslClientAuthProperty()));
properties.put(x509Util.getSslHandshakeDetectionTimeoutMillisProperty(), System.getProperty(x509Util.getSslHandshakeDetectionTimeoutMillisProperty()));
properties.put(x509Util.getFipsModeProperty(), System.getProperty(x509Util.getFipsModeProperty()));
}

Copy link
Member

@kezhuw kezhuw Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test passed since send4LetterWord will inherit these properties in setUp for server side. It would be really good to not populate any properties for this test. But it is not possible currently. I created ZOOKEEPER-4875 instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it preferable to prioritize the use of ClientConfig over system properties? If the properties are not already set in the ClientConfig, then we can retrieve them from the system properties. We can also maintain backward compatibility and ensure that properties in ClientConfig remain isolated in this manner.

Copy link
Member

@kezhuw kezhuw Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it preferable to prioritize the use of ClientConfig over system properties?

Client should use ZKClientConfig instead system properties after constructed. If not, it is a bug. So, 33c19e3#diff-9657d4a14708c9ec1df56fd01581a442e10ef70cae39c7223d4f979b0ae54263R1307 is buggy as it does not use the one in constructor but the new ZkConfig(). These two are not necessary to have same properties. It should use ClientCnxn.clientConfig which means b13e196#diff-9657d4a14708c9ec1df56fd01581a442e10ef70cae39c7223d4f979b0ae54263R1359 is correct.

this.clientConfig = clientConfig != null ? clientConfig : new ZKClientConfig();

If the properties are not already set in the ClientConfig, then we can retrieve them from the system properties.

This is already done in construction of a ZKConfig.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Client should use ZKClientConfig instead system properties after constructed. If not, it is a bug.

This is the whole point of ZOOKEEPER-2139.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I realize that ZKConfig#init is only called when we construct ZKConfig with empty parameters. I had missed this before. I will fix it based on your comments later.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can revise here once ZOOKEEPER-4875 delivered.

System.setProperty(clientX509Util.getSslKeystorePasswdProperty(), "testpass");
System.setProperty(clientX509Util.getSslTruststoreLocationProperty(), testDataPath + "/ssl/testTrustStore.jks");
System.setProperty(clientX509Util.getSslTruststorePasswdProperty(), "testpass");
}

@AfterEach
public void tearDown() throws Exception {
System.clearProperty(NettyServerCnxnFactory.PORT_UNIFICATION_KEY);
System.clearProperty(ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN_FACTORY);
System.clearProperty(clientX509Util.getSslKeystoreLocationProperty());
System.clearProperty(clientX509Util.getSslKeystorePasswdProperty());
System.clearProperty(clientX509Util.getSslKeystorePasswdPathProperty());
System.clearProperty(clientX509Util.getSslTruststoreLocationProperty());
System.clearProperty(clientX509Util.getSslTruststorePasswdProperty());
System.clearProperty(clientX509Util.getSslTruststorePasswdPathProperty());
System.clearProperty(clientX509Util.getFipsModeProperty());
System.clearProperty(clientX509Util.getSslHostnameVerificationEnabledProperty());
System.clearProperty(clientX509Util.getSslProviderProperty());
clientX509Util.close();
System.setProperty("readonlymode.enabled", "false");
qu.tearDown();
}

/**
* Ensures that client seeks for r/w servers while it's connected to r/o
* server.
*/
@Test
@Timeout(value = 90)
public void testSeekForRwServerWithSSL() throws Exception {
qu.enableLocalSession(true);
qu.startQuorum();

try (LoggerTestTool loggerTestTool = new LoggerTestTool("org.apache.zookeeper")) {
ByteArrayOutputStream os = loggerTestTool.getOutputStream();

qu.shutdown(2);
ClientBase.CountdownWatcher watcher = new ClientBase.CountdownWatcher();
ZooKeeper zk = new ZooKeeper(qu.getConnString(), CONNECTION_TIMEOUT, watcher, true, clientConfig);
watcher.waitForConnected(CONNECTION_TIMEOUT);

// if we don't suspend a peer it will rejoin a quorum
qu.getPeer(1).peer
.setSuspended(true);

// start two servers to form a quorum; client should detect this and
// connect to one of them
watcher.reset();
qu.start(2);
qu.start(3);
ClientBase.waitForServerUp(qu.getConnString(), 2000);
watcher.waitForConnected(CONNECTION_TIMEOUT);
zk.create("/test", "test".getBytes(), ZooDefs.Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);

// resume poor fellow
qu.getPeer(1).peer
.setSuspended(false);

String log = os.toString();
assertFalse(StringUtils.isEmpty(log), "OutputStream doesn't have any log messages");

LineNumberReader r = new LineNumberReader(new StringReader(log));
String line;
Pattern p = Pattern.compile(".*Majority server found.*");
boolean found = false;
while ((line = r.readLine()) != null) {
if (p.matcher(line).matches()) {
found = true;
break;
}
}
assertTrue(found, "Majority server wasn't found while connected to r/o server");
}
}
}