Add tls settings to the redis::sentinel class and the redis-sentinel.conf template

[tparkercbn]

Pull Request (PR) description

Added tls settings to the redis sentinel submodule. If you have tls only enabled for redis not having tls on the sentinels makes them useless.

[root-expert]

Thanks for the PR!
Please add some unit tests for the new parameters 😄

[tparkercbn]

Thanks @root-expert is there somewhere I can get a pointer on writing unit tests. That's where my puppet knowledge falls down.

[root-expert]

The unit tests should be added in the redis_sentinel_spec file.

You can add your new parameters with a random value in this block and check that the random value is rendered correctly in this block.

For more in depth knowledge for testing puppet you can check rspec-puppet.com and our CONTRIBUTING.md for instructions on how to run unit tests. (You can always just push and let our CI do the hard job 😄 )

[tparkercbn]

Perfect! Will do. Might be on Monday but it looks straightforward.

[tparkercbn]

I changed tls_replication to use the Enum['yes', 'no'] but that makes it not the same as the identical variable in the redis class. Which way do you prefer?

[tparkercbn]

I changed tls_replication to use the Enum['yes', 'no'] but that makes it not the same as the identical variable in the redis class. Which way do you prefer?

I think I am going to put this back to a boolean. It would be very confusing the people to have the same variable as a Boolean for one module and an Enum for the submodule.

[root-expert]

I changed tls_replication to use the Enum['yes', 'no'] but that makes it not the same as the identical variable in the redis class. Which way do you prefer?

I think I am going to put this back to a boolean. It would be very confusing the people to have the same variable as a Boolean for one module and an Enum for the submodule.

I believe the new options should follow Enum style, but I agree it can be confusing.
@bastelfreak any opinion here? 😄

[tparkercbn]

I have been thinking about the default values for the tls_ options in the sentinel class. Does it make sense to default them to the values from the redis class?

For most instalations they should be in sync all the time.

That would mean that the cert paths, and replcation values default to undef still but pulled from redis::tls_*

[tparkercbn]

I have been thinking about the default values for the tls_ options in the sentinel class. Does it make sense to default them to the values from the redis class?

For most instalations they should be in sync all the time.

That would mean that the cert paths, and replcation values default to undef still but pulled from redis::tls_*

This works in puppet on my servers, however doesn't pass the spec tests because the $redis:: variables are not defined. Is there a way to define a more complete precondition so that we can consume those values? The redis class is required in the sentinel code so there will never be a case where we can have the redis::sentinel without redis already being in the catalog.

[root-expert]

I have been thinking about the default values for the tls_ options in the sentinel class. Does it make sense to default them to the values from the redis class?
For most instalations they should be in sync all the time.
That would mean that the cert paths, and replcation values default to undef still but pulled from redis::tls_*

This works in puppet on my servers, however doesn't pass the spec tests because the $redis:: variables are not defined. Is there a way to define a more complete precondition so that we can consume those values? The redis class is required in the sentinel code so there will never be a case where we can have the redis::sentinel without redis already being in the catalog.

Take a look here

[tparkercbn]

I have been thinking about the default values for the tls_ options in the sentinel class. Does it make sense to default them to the values from the redis class?
For most instalations they should be in sync all the time.
That would mean that the cert paths, and replcation values default to undef still but pulled from redis::tls_*

This works in puppet on my servers, however doesn't pass the spec tests because the $redis:: variables are not defined. Is there a way to define a more complete precondition so that we can consume those values? The redis class is required in the sentinel code so there will never be a case where we can have the redis::sentinel without redis already being in the catalog.

Take a look here

Thanks! I was confused because it already had that set for the custom params. I hadn't noticed it was failing for the default params case.

[tparkercbn]

I have been thinking about the default values for the tls_ options in the sentinel class. Does it make sense to default them to the values from the redis class?
For most instalations they should be in sync all the time.
That would mean that the cert paths, and replcation values default to undef still but pulled from redis::tls_*

This works in puppet on my servers, however doesn't pass the spec tests because the $redis:: variables are not defined. Is there a way to define a more complete precondition so that we can consume those values? The redis class is required in the sentinel code so there will never be a case where we can have the redis::sentinel without redis already being in the catalog.

Take a look here

Thanks! I was confused because it already had that set for the custom params. I hadn't noticed it was failing for the default params case.

I am still not able to get the spec tests to work. Have I missed something?

[root-expert]

Let's use plain defaults @tparkercbn 😄

[tparkercbn]

Let's use plain defaults @tparkercbn smile

Yep. That's what I think too.

I am going to keep the tls_replication setting a Boolean and tls_auth_clients with the default to 'no' so that the types are consistent between the main and sentinel classes.

[root-expert]

Thank for the contribution!

[tparkercbn]

Thank for the contribution!

Thank you for all your help! :)