Maxwell-Guo commented on code in PR #3985:
URL: https://github.com/apache/cassandra/pull/3985#discussion_r2024911395
##########
src/java/org/apache/cassandra/net/InboundConnectionSettings.java:
##########
@@ -146,7 +147,7 @@ public InboundConnectionSettings
withLegacySslStoragePortDefaults()
ServerEncryptionOptions encryption = this.encryption;
if (encryption == null)
encryption =
DatabaseDescriptor.getInternodeMessagingEncyptionOptions();
- encryption =
encryption.withOptional(false).withInternodeEncryption(ServerEncryptionOptions.InternodeEncryption.all);
+ encryption = new
Builder(encryption).withOptional(false).withInternodeEncryption(ServerEncryptionOptions.InternodeEncryption.all).build();
Review Comment:
what about :
```
encryption =
ServerEncryptionOptions.builder(encryption).withOptional(false).withInternodeEncryption(ServerEncryptionOptions.InternodeEncryption.all).build();
```
That is to say ,you can have another method which just wrap the new
Builder();
just like [TableMetadata's
builder](https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/schema/TableMetadata.java#L276)
It's just my personal preference, you don't have to adopt it, this method is
already very good now. It doesn't affect me from +1 in the end.
##########
test/unit/org/apache/cassandra/tools/LoaderOptionsTest.java:
##########
@@ -85,6 +85,34 @@ public void testEncryptionSettings() throws Exception
assertEquals("test.jks", options.clientEncOptions.keystore);
}
+ /**
+ * Tests for client_encryption_options override from the command line.
+ */
+ @Test
+ public void testEncryptionSettingsOverride() throws Exception
+ {
+ // Default Cassandra config
+ File config = new File(Paths.get(".", "test", "conf",
"cassandra-mtls.yaml").normalize());
+ String[] args = { "-d", "127.9.9.1", "-f", config.absolutePath(),
+ "-ts", "test.jks", "-tspw", "truststorePass1",
+ "-ks", "test.jks", "-kspw", "testdata1",
+ "--ssl-ciphers", "TLS_RSA_WITH_AES_256_CBC_SHA",
+ "--ssl-alg", "SunX509", "--store-type", "JKS",
"--ssl-protocol", "TLS",
+ sstableDirName("legacy_sstables",
"legacy_ma_simple") };
+ LoaderOptions options =
LoaderOptions.builder().parseArgs(args).build();
+ // Below two lines validating server encryption options is to verify
that we are loading config from the yaml
+ assertEquals("test/conf/cassandra_ssl_test.keystore",
options.serverEncOptions.keystore);
Review Comment:
I think we should not use hard code value
``test/conf/cassandra_ssl_test.keystore ``
here, the left value should be the configuration from loaded cassandra's
DatabaseDescriptor.
we can set the loaded config yaml file by set
[CASSANDRA_CONFIG](https://github.com/apache/cassandra/blob/trunk/test/unit/org/apache/cassandra/auth/MutualTlsInternodeAuthenticatorTest.java#L76)
I have tested locally , and it worked. @smiklosovic WDYT ?
--
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: pr-unsubscr...@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscr...@cassandra.apache.org
For additional commands, e-mail: pr-h...@cassandra.apache.org
