Configurable Redis TLS Config through settings #289
Conversation
|
Thanks, please add documentation in the README and also check CI. /wait |
Signed-off-by: Javier Ruiz Arduengo <[email protected]>
JavierR14
force-pushed
the
main
branch
2 times, most recently
from
ff3e8be to
1eec7db
Compare
Signed-off-by: Javier Ruiz Arduengo <[email protected]>
Signed-off-by: Javier Ruiz Arduengo <[email protected]>
|
I can't seem to get integration tests working locally even on the main branch for envoyproxy. |
Signed-off-by: Javier Ruiz Arduengo <[email protected]>
README.md
Outdated
| @@ -542,6 +542,8 @@ As well Ratelimit supports TLS connections and authentication. These can be conf | |||
| 1. `REDIS_AUTH` & `REDIS_PERSECOND_AUTH`: set to `"password"` to enable authentication to the redis host. | |||
| 1. `CACHE_KEY_PREFIX`: a string to prepend to all cache keys | |||
|
|
|||
| Ratelimit also provides the ability to specify TLS settings via `RedisTlsConfig` in the [settings](https://github.com/envoyproxy/ratelimit/blob/master/src/settings/settings.go). | |||
There was a problem hiding this comment.
Sorry it's not clear to me how this is used. Does someone have to instantiate this by code? Or can this be set via env variables?
/wait
There was a problem hiding this comment.
Someone has to instantiate it by code. You shouldn't be setting a TLS config through env variables as it would require dealing with certs and key files and needs code. TLS configs should be configurable and this allows the client using ratelimit to configure it if needed.
There was a problem hiding this comment.
OK, then I guess this isn't a general purpose feature so I'm not sure it's worth documenting like this. It would probably be better long term to allow the config to be read from a YAML file of some type. I would remove this from the README and put a TODO in the settings code to have that setting be out of box user configurable somehow.
/wait
There was a problem hiding this comment.
Done. I've added a TODO line. I do think providing an out of the box user configurable setting is a good idea but ultimately will probably not be used much. Setting up a Redis TLS config will be very different from case to case and most of the time users will provide it through code.
|
Looks like it needs a format fix. /wait |
Signed-off-by: Javier Ruiz Arduengo <[email protected]>
Signed-off-by: Javier Ruiz Arduengo <[email protected]>
fixes isssue envoyproxy#303 Signed-off-by: Vito Sabella <[email protected]>
* TLS config pointer is never nil, fixes regression in pull #289 fixes isssue #303 Signed-off-by: Vito Sabella <[email protected]> * Accidentally put the initialization before the environment variable reading. Signed-off-by: Vito Sabella <[email protected]> * Unit test for settings tlsConfig fix Signed-off-by: Vito Sabella <[email protected]> * Fixing pre-commits Signed-off-by: Vito Sabella <[email protected]> Co-authored-by: Vito Sabella <[email protected]>
…xy#318) * TLS config pointer is never nil, fixes regression in pull envoyproxy#289 fixes isssue envoyproxy#303 Signed-off-by: Vito Sabella <[email protected]> * Accidentally put the initialization before the environment variable reading. Signed-off-by: Vito Sabella <[email protected]> * Unit test for settings tlsConfig fix Signed-off-by: Vito Sabella <[email protected]> * Fixing pre-commits Signed-off-by: Vito Sabella <[email protected]> Co-authored-by: Vito Sabella <[email protected]> Signed-off-by: Joel Damata <[email protected]>
…xy#318) * TLS config pointer is never nil, fixes regression in pull envoyproxy#289 fixes isssue envoyproxy#303 Signed-off-by: Vito Sabella <[email protected]> * Accidentally put the initialization before the environment variable reading. Signed-off-by: Vito Sabella <[email protected]> * Unit test for settings tlsConfig fix Signed-off-by: Vito Sabella <[email protected]> * Fixing pre-commits Signed-off-by: Vito Sabella <[email protected]> Co-authored-by: Vito Sabella <[email protected]>
Signed-off-by: Javier Ruiz Arduengo <[email protected]>
…xy#318) * TLS config pointer is never nil, fixes regression in pull envoyproxy#289 fixes isssue envoyproxy#303 Signed-off-by: Vito Sabella <[email protected]> * Accidentally put the initialization before the environment variable reading. Signed-off-by: Vito Sabella <[email protected]> * Unit test for settings tlsConfig fix Signed-off-by: Vito Sabella <[email protected]> * Fixing pre-commits Signed-off-by: Vito Sabella <[email protected]> Co-authored-by: Vito Sabella <[email protected]>
At the moment, the redis tls config that it's using is a default and there is no way to change it. This makes it so that clients can specify their own TLS config if they choose to.