249: Introduce basic and extended ssl settings

[capuccinofan]

No description provided.

[capuccinofan]

@KarelCemus Could u please have a look?

[KarelCemus]

I am not comfortable with this PR as it is binary incompatible and you are introducing plenty of breaking changes, and I am not about to release a new major anytime soon. And it is not appreciated making incompatible changes in minor or even patch releases

I am surprised you proposed changes to your own PR one day after your PR was merged. It makes me feel that your changes were not well thought through, and I probably accepted them to the lib prematurely.

[KarelCemus]

Why do you need this? Feels redundant given SSLConfig is optional, meaning if you want to disable it, then don't provide it. This would complicate debugging and I am not sure what the benefits of this would be

[KarelCemus]

For some weird unknown reason, I cannot put the comment at the correct line, it belongs here

    def none: VerifyPeerMode = NONE
    def full: VerifyPeerMode = FULL
    def ca: VerifyPeerMode = CA

Why do you need these aliases?

[KarelCemus]

another case

      config.getOption(path, _.getString) match {
        case Some(NONE.name) => Some(none)
        case Some(FULL.name) => Some(full)
        case Some(CA.name)   => Some(ca)
        case _               => None
      }

unrecognized matches shouldn't be ignored silently, it is illegal configuration, I believe it should fail but check other places, how it is usually done there

[KarelCemus]

Why do we need 2 types of settings?

[capuccinofan]

I am not comfortable with this PR as it is binary incompatible and you are introducing plenty of breaking changes, and I am not about to release a new major anytime soon. And it is not appreciated making incompatible changes in minor or even patch releases
I am not comfortable with this PR as it is binary incompatible and you are introducing plenty of breaking changes, and I am not about to release a new major anytime soon. And it is not appreciated making incompatible changes in minor or even patch releases

I am surprised you proposed changes to your own PR one day after your PR was merged. It makes me feel that your changes were not well thought through, and I probably accepted them to the lib prematurely.

Yeah, I am sorry, that I didn't take some things into account. The thing is, I need to have an option to enable ssl settings without any settings.

1. First thought was to make all arguments optional and having something like this:

play.cache.redis.ssl-settings {}

Like existence of object means that ssl should be enabled

But it doesn't look very clear,
2) the next idea was to add the field enabled: boolean to this object
But I am a bit uncomfortable that we can have something like this (disabled ssl with defined settings):

play.cache.redis.ssl-settings {
   a: 1,
   b: 2,
   enabled: false
}

And going to the fact that enabled is responsible for withSsl for RedisURI builder and SslOptions are set on ClientOptions level we can end up in the situation when we disabled SSL with enabled: false but still SslOptions went to ClientOptions. Of course we can check in the ClientOptions builder whether enabled: true|false and then either bring SslOptions to ClientOptions or not. I have just thought it gave us not clear picture of what is going on.

3. That is why I came up with the idea, that either we have object with enabled:true|false or the settings for ClientOptions that imply that SSL is enabled.

That is what I would like to achieve. Probably we can make it with the second approach. What do you think?

[KarelCemus]

I need to have an option to enable ssl settings without any settings.

This is very suspicious, how is that possible? Which configuration would be used then? Isn't this basically the extended one with defaults? Could you elaborate more on this?

However, I agree that empty object is not the best option but also having enabled: false is not welcome either

Btw there is still the issue of the compatibility to make your changes binary compatible with your previous attempt

[capuccinofan]

I need to have an option to enable ssl settings without any settings.

This is very suspicious, how is that possible? Which configuration would be used then? Isn't this basically the extended one with defaults? Could you elaborate more on this?

As far as I understood, we should be able to connect using ssl only by enabling withSsl(true) on RedisURI, as stated here:
https://github.com/redis/lettuce/wiki/SSL-Connections
So the idea of having all defaults doesn't seem to work, because we don't need them at all if we wanna just enable it on the RedisURI level. Maybe then we can just define another setting?

play.cache.redis.ssl-settings {...} // filled as in the previous PR or not defined at all
play.cache.redis.ssl = { enabled: boolean, verifyPeer: string }

It will be backward compatible as well

However, I agree that empty object is not the best option but also having enabled: false is not welcome either

Btw there is still the issue of the compatibility to make your changes binary compatible with your previous attempt

[capuccinofan]

@KarelCemus Friendly reminder :)

[KarelCemus]

Not nice but ok

[capuccinofan]

Not nice but ok

Let me think more about this topic, maybe I can come up with something else