Apache9 commented on code in PR #4125:
URL: https://github.com/apache/hbase/pull/4125#discussion_r930908720
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/NettyRpcConnection.java:
##########
@@ -339,14 +365,79 @@ public void operationComplete(ChannelFuture future)
throws Exception {
});
}
+ private void writeAndFlushToChannel(Call call, Channel ch) {
+ if (ch == null) {
+ return;
+ }
+
+ scheduleTimeoutTask(call);
+ ch.writeAndFlush(call).addListener(new ChannelFutureListener() {
+ @Override
+ public void operationComplete(ChannelFuture future) {
+ // Fail the call if we failed to write it out. This usually because
the channel is
+ // closed. This is needed because we may shutdown the channel inside
event loop and
+ // there may still be some pending calls in the event loop queue after
us.
+ if (!future.isSuccess()) {
+ call.setException(toIOE(future.cause()));
+ }
+ }
+ });
+ }
+
@Override
public void sendRequest(final Call call, HBaseRpcController hrc) {
- execute(eventLoop, () -> {
- try {
- sendRequest0(call, hrc);
- } catch (Exception e) {
- call.setException(toIOE(e));
+ try {
+ sendRequest0(call, hrc);
+ } catch (Exception e) {
+ call.setException(toIOE(e));
+ }
+ }
+
+ /**
+ * HBaseClientPipelineFactory is the netty pipeline factory for this netty
connection
+ * implementation.
+ */
+ private static class HBaseClientPipelineFactory extends
ChannelInitializer<SocketChannel> {
+
+ private SSLContext sslContext = null;
+ private SSLEngine sslEngine = null;
+ private final String host;
+ private final int port;
+ private final Configuration conf;
+
+ public HBaseClientPipelineFactory(String host, int port, Configuration
conf) {
+ this.host = host;
+ this.port = port;
+ this.conf = conf;
+ }
+
+ @Override
+ protected void initChannel(SocketChannel ch) throws
X509Exception.SSLContextException {
+ ChannelPipeline pipeline = ch.pipeline();
+ if (conf.getBoolean(HBASE_CLIENT_NETTY_TLS_ENABLED, false)) {
+ initSSL(pipeline);
}
- });
+ pipeline.addLast("handler", new BufferCallBeforeInitHandler());
+ }
+
+ // The synchronized is to prevent the race on shared variable "sslEngine".
+ // Basically we only need to create it once.
+ private synchronized void initSSL(ChannelPipeline pipeline)
+ throws X509Exception.SSLContextException {
+ if (sslContext == null || sslEngine == null) {
Review Comment:
I think here we should share the SslContext instance. That means, we'd
better create a SslContext instance per RpcClient, and pass it in when creating
a NettyRpcConnection, and then just call its createSSLEngine method when
initializing a Channel, where we do not need synchronized any more.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/NettyRpcServer.java:
##########
@@ -214,4 +228,21 @@ public int getNumOpenConnections() {
// allChannels also contains the server channel, so exclude that from the
count.
return channelsCount > 0 ? channelsCount - 1 : channelsCount;
}
+
+ private synchronized void initSSL(ChannelPipeline p, boolean
supportPlaintext)
Review Comment:
Oh, this one is shared in NettyRpcServer, not per connection, so
synchronized is needed if we share some common fields which are not thread
safe(but seems not...)
I think we should share the SslContext stuff? That means, when constructing
the NettyRpcServer, we check whether TLS is enabled, if so, we create the
SslContext instance. So then when initializing a Channel, we just need to call
the createSSLEngine method, which should be thread safe so we do not need
synchronized here.
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/SSLContextAndOptions.java:
##########
@@ -0,0 +1,88 @@
+/*
+ * 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.hadoop.hbase.io.crypto.tls;
+
+import static java.util.Objects.requireNonNull;
+import static org.apache.hadoop.hbase.io.crypto.tls.X509Util.CONFIG_PREFIX;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import javax.net.ssl.SSLContext;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.yetus.audience.InterfaceAudience;
+
+import org.apache.hbase.thirdparty.io.netty.handler.ssl.ClientAuth;
+import
org.apache.hbase.thirdparty.io.netty.handler.ssl.IdentityCipherSuiteFilter;
+import org.apache.hbase.thirdparty.io.netty.handler.ssl.JdkSslContext;
+import org.apache.hbase.thirdparty.io.netty.handler.ssl.SslContext;
+
+/**
+ * This file has been copied from the Apache ZooKeeper project.
+ * @see <a href=
+ *
"https://github.com/apache/zookeeper/blob/c74658d398cdc1d207aa296cb6e20de00faec03e/zookeeper-server/src/main/java/org/apache/zookeeper/common/SSLContextAndOptions.java";>Base
+ * revision</a>
+ */
[email protected]
+public class SSLContextAndOptions {
+ private static final String TLS_ENABLED_PROTOCOLS = CONFIG_PREFIX +
"enabledProtocols";
+ private static final String TLS_CIPHER_SUITES = CONFIG_PREFIX +
"ciphersuites";
+
+ private final String[] enabledProtocols;
+ private final List<String> cipherSuitesAsList;
+ private final SSLContext sslContext;
+
+ /**
+ * Note: constructor is intentionally package-private, only the X509Util
class should be creating
+ * instances of this class.
+ * @param config The HBase configuration
+ * @param sslContext The SSLContext.
+ */
+ SSLContextAndOptions(final Configuration config, final SSLContext
sslContext) {
+ this.sslContext = requireNonNull(sslContext);
+ this.enabledProtocols = getEnabledProtocols(requireNonNull(config),
sslContext);
+ String[] ciphers = getCipherSuites(config);
+ this.cipherSuitesAsList =
Collections.unmodifiableList(Arrays.asList(ciphers));
+ }
+
+ public SSLContext getSSLContext() {
+ return sslContext;
+ }
+
+ public SslContext createNettyJdkSslContext(SSLContext sslContext, boolean
isClientSocket) {
+ return new JdkSslContext(sslContext, isClientSocket, cipherSuitesAsList,
Review Comment:
Please use SslContextBuilder instead of hard coded JdkSslContext here.
SslContextBuilder will try to use open ssl if possible, where the
performance is much better than the jdk one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
