Conversation
Agent-Logs-Url: https://github.com/dsmiley/zookeeper/sessions/f61cc9b9-0ac8-4f8f-a366-2a7752cdbedc Co-authored-by: dsmiley <377295+dsmiley@users.noreply.github.com>
…ic numbers Agent-Logs-Url: https://github.com/dsmiley/zookeeper/sessions/f61cc9b9-0ac8-4f8f-a366-2a7752cdbedc Co-authored-by: dsmiley <377295+dsmiley@users.noreply.github.com>
dsmiley
left a comment
dsmiley
left a comment
There was a problem hiding this comment.
NettyServerCnxnFactory.java: Updated CLIENT_CERT_RELOAD_KEY to reference X509Util.CLIENT_CERT_RELOAD_KEY for backward compatibility.
What is the backward compatibility concern?
And finally, how is this going to be enforced to prevent accidental use of Netty stuff? Are the pom.xml changes sufficient to enforce that? FYI the original issue pointed to an ArchUnit test.
| if (serverCnxnFactoryName == null) { | ||
| if (secure) { | ||
| serverCnxnFactoryName = "org.apache.zookeeper.server.NettyServerCnxnFactory"; | ||
| LOG.info("Defaulting to {} for secure connections", serverCnxnFactoryName); |
| boolean secureClient = getClientConfig().getBoolean(ZKClientConfig.SECURE_CLIENT); | ||
| if (secureClient) { | ||
| clientCnxnSocketName = ClientCnxnSocketNetty.class.getName(); | ||
| LOG.info("Defaulting to {} for secure connections", clientCnxnSocketName); |
… optional Agent-Logs-Url: https://github.com/dsmiley/zookeeper/sessions/34abe384-a396-4ef6-9ba0-006a7318faa8 Co-authored-by: dsmiley <377295+dsmiley@users.noreply.github.com>
For enforcement, I added Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
There was a problem hiding this comment.
My original attempt at an ArchUnit test is the following. If you think your approach could be improved, then expand it accordingly. I'm not sure.
JavaClasses importedClasses = new ClassFileImporter(
Collections.singletonList(DO_NOT_INCLUDE_TESTS)).importPackages("org.apache.zookeeper")
.that(new DescribedPredicate<JavaClass>(
"ZK Non-Netty classes") {
@Override
public boolean test(JavaClass javaClass) {
return !javaClass.getName().contains("Netty");
}
});
ArchRule rule = ArchRuleDefinition.noClasses().should()
.dependOnClassesThat()
.resideInAnyPackage("io.netty..")
.orShould().dependOnClassesThat()
.haveSimpleNameContaining("Netty");
rule.check(importedClasses);There was a problem hiding this comment.
Updated in b7abf32. I adopted your approach with two expansions needed to make it pass:
-
ClientX509UtilandUnifiedServerSocketdon't have "Netty" in their names but legitimately use the Netty SSL API — so their names (and those of their inner/anonymous classes likeClientX509Util$1,UnifiedServerSocket$UnifiedSocket) are added to the predicate exclusion. -
Your test also caught two pre-existing hard dependencies that were defeating the Netty-optional goal:
ZooKeeper.getClientCnxnSocket()referencedClientCnxnSocketNetty.classdirectly — replaced with string literals"org.apache.zookeeper.ClientCnxnSocketNetty"/"ClientCnxnSocketNetty".ZooKeeperServer.getOutstandingHandshakeNum()usedinstanceof NettyServerCnxnFactory— fixed by adding agetOutstandingHandshakeNum()default method (returns 0) toServerCnxnFactory, soZooKeeperServercan delegate without knowing aboutNettyServerCnxnFactory.
…rd-refs in ZooKeeper/ZooKeeperServer Agent-Logs-Url: https://github.com/dsmiley/zookeeper/sessions/698a4b0f-1e5b-46f8-bf10-f21f9a65a382 Co-authored-by: dsmiley <377295+dsmiley@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
…nd ClientNettyX509Util subclass Agent-Logs-Url: https://github.com/dsmiley/zookeeper/sessions/2dc4e1b6-53f1-4851-91ed-b4427d332564 Co-authored-by: dsmiley <377295+dsmiley@users.noreply.github.com>
X509Util.ClientAuthenumClientX509Utilto be Netty-free (just config prefix + property name methods)ClientNettyX509Util extends ClientX509Utilwith all Netty SSL context creation methodsNettyServerCnxnFactoryto useClientNettyX509UtilClientCnxnSocketNettyto useClientNettyX509UtilClientX509Utilexclusion (it's now Netty-free),ClientNettyX509Utilis covered by the "Netty" name checkX509UtilTestto useClientNettyX509Utilwhere Netty SSL context creation is testedHexDumpOutputFormatterwith pure Java implementationServerCnxnFactory.createFactory(boolean secure)to auto-select Netty for SSLServerCnxnFactory.getOutstandingHandshakeNum()default methodZooKeeperServerMainandQuorumPeerMainto usecreateFactory(true)for secure factoryClientCnxnSocketNettyinZooKeeper.javausing string literalsZooKeeperServer.getOutstandingHandshakeNum()to delegate toserverCnxnFactoryoptionalinzookeeper-server/pom.xmlzookeeper-assembly/pom.xmlNettyOptionalArchTest(1),X509UtilTest(328),ClientSSLTest(24),ZooKeeperServerMainTest(15)