Make all SSL settings optional.

[mattelacchiato]

One don't need a keystore unless you use client-authentication. We should not force the user to configure stuff (s)he doesn't need.

[patriknw]

can we use immutable.Seq instead of Array, since Array is mutable ?

[mattelacchiato]

Okay, but then I've to do another conversation, because the Java API needs an array :-/

[patriknw]

thanks

[mattelacchiato]

I've no idea, why the "A PersistentActor's performance - must measure: persist()-ing 10000 events" test is failing. If this is a flaky test: How can I re-trigger it?

[patriknw]

yeah, that should not be related to this PR. I don't know how to trigger a rebuild with travis, but I would like that you squashed the two commits to one, and then it will rebuild when you push.

[mattelacchiato]

Hmmm. Any ideas?

[patriknw]

hm, I'll take a look later

[patriknw]

It looks like we have had some problems with that spec in the cassandra-3.x branch and increased the timeouts there: https://github.com/krasserm/akka-persistence-cassandra/blob/cassandra-3.x/src/test/scala/akka/persistence/cassandra/journal/CassandraJournalSpec.scala

Please do the same here, i.e.

akka.test.single-expect-default = 20s
cassandra-journal.circuit-breaker.call-timeout = 20s

override def awaitDurationMillis: Long = 20.seconds.toMillis

[mattelacchiato]

cry

Couldn't you just fix it in the master and I'll rebase it into my branch?

[patriknw]

okey, that was another test that failed this time. I have increased the timeouts for both in #152
merged, so please rebase

[magnusart]

Since I was the original author of the SSL setup code I have some comments.

I agree that I missed the case where client authentication is optional, good catch! However I do have some issues with the implementation as it stands now.

• All config values in the config ssl-block are now optional. This is incorrect. If a keystore/truststore is present then passwords are mandatory. You've even dangerously added a default "changeme" password that will cause runtime errors instead. Don't allow the application to start with a broken config!
• You've bypassed the fact that the ssl-blocks means that some SSL-config is mandatory. Because of this you've had to compensate by making unnecessary code changes. It is now possible for the config to contain empty ssl-blocks. The config.hasPath("ssl") check is already in place. SSL on the server side at least implies a trust store must be present. Don't allow duplicate checks by making trust store configuration optional!.

These are my suggestions:

• Revert code (sorry, not trying to be rude but I think you went wrong early on and had to compensate too much)
• In if (config.hasPath("ssl"))-block check for config path ssl.keystore to be present.
• If present then both ssl.keystore.path and ssl.keystore.password are mandatory config values.
• Instead of passing trustStorePath, trustStorePW, keyStorePath, keyStorePW arguments individually, consider passing them as tuples or a case classes so you can pass Option[(String,String)] to manage that ssl.keystore could be missing.
• In SSLSetup.constructContext skip creating the trust manager factory and replace the call to tmf.getTrustManagers with empty array if ssl.keystore was empty.

I hope above helps. I do realize it might seem harsh to ask for what is almost a complete overhaul, but I mean it in a constructive way. :)

[patriknw]

when completing this, please open PR at https://github.com/akka/akka-persistence-cassandra, which is the new home for akka-persistence-cassandra

[magnusart]

I have actually implemented my own suggestions above. Right now I'm looking into the SSL tests which are still failing. Will do PR afterwards.

[patriknw]

you know about the cassandra-3.x branch?

[magnusart]

I do. :)
Probably have to finish it during the weekend though. I have no idea why test would fail so I need to install cassandra or spin up something to test it out.

[patriknw]

thanks, help with that is very much appreciated