You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2022/07/25 19:12:38 UTC
[GitHub] [accumulo] dlmarion opened a new pull request, #2825: Cache SSLContext
dlmarion opened a new pull request, #2825:
URL: https://github.com/apache/accumulo/pull/2825
Working with user testing SSL setup with Accumulo 2.1.0 and seeing general slowness. I created modified SslIT to include the following:
```
@Override
public void configure(MiniAccumuloConfigImpl cfg, Configuration hadoopCoreSite) {
+ System.setProperty("javax.net.debug", "ssl:handshake");
+ Map<String,String> sys = new HashMap<>();
+ sys.put("javax.net.debug", "ssl:handshake");
+ cfg.setSystemProperties(sys);
+ cfg.setProperty(Property.RPC_SSL_ENABLED_PROTOCOLS, "TLSv1.3");
+ cfg.setProperty(Property.RPC_SSL_CLIENT_PROTOCOL, "TLSv1.3");
+ cfg.setProperty(Property.MONITOR_SSL_INCLUDE_PROTOCOLS, "TLSv1.3");
+ cfg.setClientProperty(Property.RPC_SSL_CLIENT_PROTOCOL.getKey(), "TLSv1.3");
super.configure(cfg, hadoopCoreSite);
configureForSsl(cfg,
getSslDir(createTestDir(this.getClass().getName() + "_" + this.testName())));
}
+ @Test
+ public void testScan() throws Exception {
+ try (AccumuloClient client = Accumulo.newClient().from(getClientProperties()).build()) {
+ String tableName = getUniqueNames(1)[0];
+ client.tableOperations().create(tableName);
+ BatchWriter bw = client.createBatchWriter(tableName);
+ Mutation m = new Mutation("m");
+ m.put("u", "t", new ColumnVisibility(), "t");
+ bw.addMutation(m);
+ bw.flush();
+ bw.close();
+ Scanner s = client.createScanner(tableName);
+ s.iterator().next();
+ s.iterator().next();
+ s.iterator().next();
+ s.iterator().next();
+ s.iterator().next();
+ }
+ }
```
Looking at the output of the `test` thread running the `testScan` test above, there are 14 instances of `"ClientHello":`, meaning that there are 14 TLS handshakes. Running the same test on this new code yields 2 or 3 TLS handshakes (I'm assuming 1 to the Manager and then 1 or 2 to the MAC TabletServers).
This PR introduces a Cache for the SSLContext that is created for the Accumulo connections. Caching was not working intially because of the bug in `SslConnectionParams.hashCode` and the fact that a new `SslConnectionParams` object was being returned because the default `ClientContext.memoizeWithExpiration` expires the object after 100ms.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #2825: Cache SSLContext
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #2825:
URL: https://github.com/apache/accumulo/pull/2825#discussion_r929234882
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ClientContext.java:
##########
@@ -169,7 +176,8 @@ public ClientContext(SingletonReservation reservation, ClientInfo info,
this.serverConf = serverConf;
timeoutSupplier = memoizeWithExpiration(
() -> getConfiguration().getTimeInMillis(Property.GENERAL_RPC_TIMEOUT));
- sslSupplier = memoizeWithExpiration(() -> SslConnectionParams.forClient(getConfiguration()));
+ sslSupplier = memoizeWithExpiration(() -> SslConnectionParams.forClient(getConfiguration()), 1,
+ TimeUnit.DAYS);
Review Comment:
I don't think it can. I think this was just for lazy initialization, and nothing more.
##########
core/src/main/java/org/apache/accumulo/core/rpc/SslConnectionParams.java:
##########
@@ -264,7 +264,7 @@ public int hashCode() {
}
hash = 31 * hash + clientProtocol.hashCode();
hash = 31 * hash + Arrays.hashCode(serverProtocols);
- return super.hashCode();
+ return hash;
Review Comment:
I wonder if the original intent was something like `super.hashCode() + hash`, but they left it unfinished?
##########
core/src/main/java/org/apache/accumulo/core/rpc/ThriftUtil.java:
##########
@@ -53,12 +54,21 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import com.github.benmanes.caffeine.cache.Cache;
+import com.github.benmanes.caffeine.cache.Caffeine;
+import com.github.benmanes.caffeine.cache.Scheduler;
+
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
/**
* Factory methods for creating Thrift client objects
*/
public class ThriftUtil {
+
+ private static final Cache<SslConnectionParams,SSLContext> SSL_CONTEXT_CACHE =
+ Caffeine.newBuilder().scheduler(Scheduler.systemScheduler())
+ .expireAfterAccess(Duration.ofMinutes(10)).initialCapacity(1).maximumSize(8).build();
Review Comment:
The transport is already cached. I would expect a new SSLContext to be created for each new transport... we don't know if the underlying JCE library or some other state has changed that would result in a differently-constructed context for the same SSL params. If this is expensive, then perhaps we need to focus on caching the transport longer, rather than caching at both the SSLContext and the transport?
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ClientContext.java:
##########
@@ -153,6 +154,12 @@ private static <T> Supplier<T> memoizeWithExpiration(Supplier<T> s) {
return () -> Suppliers.memoizeWithExpiration(s::get, 100, MILLISECONDS).get();
}
+ private static <T> Supplier<T> memoizeWithExpiration(Supplier<T> s, long duration,
+ TimeUnit units) {
+ // This insanity exists to make modernizer plugin happy. We are living in the future now.
+ return () -> Suppliers.memoizeWithExpiration(s::get, duration, units).get();
+ }
+
Review Comment:
I am not sure this is actually needed. Also, the comment isn't very helpful. I was able to inline this easily and use a static import, and completely remove this method, as well as the method it was copied from. Modernizer is perfectly fine with it.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a diff in pull request #2825: Cache SSLContext
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #2825:
URL: https://github.com/apache/accumulo/pull/2825#discussion_r929215574
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ClientContext.java:
##########
@@ -169,7 +176,8 @@ public ClientContext(SingletonReservation reservation, ClientInfo info,
this.serverConf = serverConf;
timeoutSupplier = memoizeWithExpiration(
() -> getConfiguration().getTimeInMillis(Property.GENERAL_RPC_TIMEOUT));
- sslSupplier = memoizeWithExpiration(() -> SslConnectionParams.forClient(getConfiguration()));
+ sslSupplier = memoizeWithExpiration(() -> SslConnectionParams.forClient(getConfiguration()), 1,
+ TimeUnit.DAYS);
Review Comment:
Can the SSL configuration for a client change after the client has been created? If not, then there is no reason for this to expire.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a diff in pull request #2825: Cache SSLContext
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #2825:
URL: https://github.com/apache/accumulo/pull/2825#discussion_r929261302
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ClientContext.java:
##########
@@ -153,6 +154,12 @@ private static <T> Supplier<T> memoizeWithExpiration(Supplier<T> s) {
return () -> Suppliers.memoizeWithExpiration(s::get, 100, MILLISECONDS).get();
}
+ private static <T> Supplier<T> memoizeWithExpiration(Supplier<T> s, long duration,
+ TimeUnit units) {
+ // This insanity exists to make modernizer plugin happy. We are living in the future now.
+ return () -> Suppliers.memoizeWithExpiration(s::get, duration, units).get();
+ }
+
Review Comment:
This was removed in f8583e0 and I just used a call `Suppliers.memoize`
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a diff in pull request #2825: Cache SSLContext
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #2825:
URL: https://github.com/apache/accumulo/pull/2825#discussion_r929260485
##########
core/src/main/java/org/apache/accumulo/core/rpc/SslConnectionParams.java:
##########
@@ -264,7 +264,7 @@ public int hashCode() {
}
hash = 31 * hash + clientProtocol.hashCode();
hash = 31 * hash + Arrays.hashCode(serverProtocols);
- return super.hashCode();
+ return hash;
Review Comment:
Not sure, but I think this is what was breaking the caching at the ThriftTranportPool. The ThritTransportKey would get a different hash and create a new connection.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a diff in pull request #2825: Cache SSLContext
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #2825:
URL: https://github.com/apache/accumulo/pull/2825#discussion_r929218116
##########
core/src/main/java/org/apache/accumulo/core/rpc/ThriftUtil.java:
##########
@@ -53,12 +54,21 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import com.github.benmanes.caffeine.cache.Cache;
+import com.github.benmanes.caffeine.cache.Caffeine;
+import com.github.benmanes.caffeine.cache.Scheduler;
+
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
/**
* Factory methods for creating Thrift client objects
*/
public class ThriftUtil {
+
+ private static final Cache<SslConnectionParams,SSLContext> SSL_CONTEXT_CACHE =
+ Caffeine.newBuilder().scheduler(Scheduler.systemScheduler())
+ .expireAfterAccess(Duration.ofMinutes(10)).initialCapacity(1).maximumSize(8).build();
Review Comment:
I'm not sure a Map/Cache is needed here. If so, the expiration can certainly be longer and may help with TLS session resumption. My thought here is that there is *only* one SSLContext per client.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion merged pull request #2825: Cache SSLContext
Posted by GitBox <gi...@apache.org>.
dlmarion merged PR #2825:
URL: https://github.com/apache/accumulo/pull/2825
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a diff in pull request #2825: Cache SSLContext
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #2825:
URL: https://github.com/apache/accumulo/pull/2825#discussion_r929260944
##########
core/src/main/java/org/apache/accumulo/core/rpc/SslConnectionParams.java:
##########
@@ -264,7 +264,7 @@ public int hashCode() {
}
hash = 31 * hash + clientProtocol.hashCode();
hash = 31 * hash + Arrays.hashCode(serverProtocols);
- return super.hashCode();
+ return hash;
Review Comment:
No idea.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #2825: Cache SSLContext
Posted by GitBox <gi...@apache.org>.
dlmarion commented on PR #2825:
URL: https://github.com/apache/accumulo/pull/2825#issuecomment-1194579045
> The transport is already cached. I would expect a new SSLContext to be created for each new transport... we don't know if the underlying JCE library or some other state has changed that would result in a differently-constructed context for the same SSL params. If this is expensive, then perhaps we need to focus on caching the transport longer, rather than caching at both the SSLContext and the transport?
I'm not sure that's necessary, but I don't think it hurts anything. From what I understand an SSLContext is the set of security parameters used to create SSLEngines and SSLSockets. Instead of having a single SSLContext with many cached SSLSession's, we have one SSLContext per Connection with one cached SSLSession.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #2825: Cache SSLContext
Posted by GitBox <gi...@apache.org>.
dlmarion commented on PR #2825:
URL: https://github.com/apache/accumulo/pull/2825#issuecomment-1194576329
I removed the caching in f8583e0 and the TLS handshakes remained at 2. Looking at what @ctubbsii referenced, it looks like the incorrect `SslConnectionParams.hashCode` caused a different `ThriftTransportKey.hashCode` which means that a new connection was being created due to a `miss` in the `ThriftTranportPool`.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org