Add Redis connection parameters full control #8578

pierre-claranet · 2024-07-03T16:07:50Z

SUMMARY

In Ansible Redis facts cache plugin, add Redis connection parameters full access in order to be able to set advanced socket options, like enabling keep alive for example.

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

redis cache

ADDITIONAL INFORMATION

The parameter Ansible facts cache parameter fact_caching_connection accepts (for Redis cache plugin) a new URI format (the 2 other formats are still accepted too): key=value pairs, separated by &. Keys are Redis Python connection parameters name. See https://redis-py2.readthedocs.io/en/latest/ for parameters name and accepted values.

felixfontein

Thanks for your contribution!

changelogs/fragments/8578-feat-add-redis-connection-parameters-full-control.yml

felixfontein · 2024-07-05T06:53:11Z

plugins/cache/redis.py

+          - To specify advanced connection parameters (keep alive, etc ...), use the 3rd format.
+          - This last format consists in a '&' separated string of redis connection parameters as <name>=<value> pairs.
+          - For example V(host=localhost&port=6379&db=0&password=changeme&socket_keepalive=true).
+          - See https://redis-py2.readthedocs.io/en/latest/ for Redis connection parameters name and accepted values.


Why not simply expose these extra parameters as proper options? That makes everything a lot more readable and thus simpler to use as well.

Hello @felixfontein, i've suggested this because there are a lot of parameters in the Redis connection, and some complex ones like socket_keepalive_options. And I don't wanted to add too much complexity to the module, for some options not that much commonly used. But ok, maybe you're right, it might be better to expose all parameters as options. I'll prepare this, and I'll push the updated code here in few hours/days. Thank you !

@pierre-claranet if I may suggest, one possible way would be to have a parameter (either a simple string or a list) for the user to pass those extra parameters. If that option is used, then it gets '&'.join()-ed with the URL.

Another thing to keep in mind is that if password=changeme is used, as in the example, then this option should be marked no_log: true, personally I find that an undesirable side-effect, but YMMV.

One thing to consider is also to consolidate the cache plugin's options with the module and/or lookup. Right now we have three separate pieces of code and options for handling Redis logins:

The cache plugin (what this PR touches);

The lookup plugin;

The modules, which use the redis docs fragment and the redis module utils.

Hello,

PR's code updated.

@russoz, thanks for your suggestions. I took care of obfuscating the Redis connection password in the verbose (-vvv) output. The password's value will be replaced with ****. About your 1st suggestion, if I'm right, it's close to this PR's original code. The only difference would be to use an extra option, and not the _url one, to pass the extra parameters. And as discussed with @felixfontein previously, exposing options separately seems to be better for simplicity and readability.

@felixfontein, about the options consolidation, I'm not sure to completely understand your point. Do you suggest we use the same names, allowed values and format, for the same parameters in all pieces of codes ? For some parameters, for example these in Redis cache plugin _uri connection string (the host, port, db, password), I think we can't change them. The cache plugin documentation (https://docs.ansible.com/ansible/latest/dev_guide/developing_plugins.html#cache-plugins) says New cache plugins should take the options _uri [...]. Could you please add some precision, maybe one example, for you point ? Thanks.

pierre-claranet · 2024-08-05T15:17:34Z

bot_status

ansibullbot · 2024-08-05T15:22:15Z

Components

changelogs/fragments/8578-feat-add-redis-connection-parameters-full-control.yml
support: community
maintainers:

plugins/cache/redis.py
support: core
maintainers:

Metadata

waiting_on: pierre-claranet
changes_requested_by: null
needs_info: False
needs_revision: True
needs_rebase: True
merge_commits: []
too many files or commits: False
mergeable_state: dirty
ci_status: None
maintainer_shipits (module maintainers): False
community_shipits (namespace maintainers): False
ansible_shipits (core team members): False
shipit_actors (maintainer or core team member): None
shipit_actors_other:
automerge: automerge shipit test failed

click here for bot help

…l support

pierre-claranet · 2024-08-08T07:35:07Z

bot_status

ansibullbot · 2024-08-08T07:40:25Z

Components

changelogs/fragments/8578-feat-add-redis-connection-parameters-full-control.yml
support: community
maintainers:

plugins/cache/redis.py
support: core
maintainers:

Metadata

waiting_on: ansible
changes_requested_by: null
needs_info: False
needs_revision: False
needs_rebase: False
merge_commits: []
too many files or commits: False
mergeable_state: clean
ci_status: success
maintainer_shipits (module maintainers): 0
community_shipits (namespace maintainers): 0
ansible_shipits (core team members): 0
shipit_actors (maintainer or core team member): []
shipit_actors_other: []
automerge: automerge shipit test failed

click here for bot help

russoz

Hi @pierre-claranet,

There are some adjustments needed, others up to you. Thanks once again for your contribution!

russoz · 2024-08-06T01:12:42Z

plugins/cache/redis.py

+    def _parse_socket_options(self, options):
+        if not self.re_socket_keepalive_opts.match(options):
+            raise AnsibleError("Unable to parse Redis cache socket keepalive options string")
+        import socket


The import statement should be by the top of the file, after the docs.

russoz · 2024-08-06T01:31:16Z

plugins/cache/redis.py

@@ -132,6 +310,7 @@ def __init__(self, *args, **kwargs):
            self._db = StrictRedis(*connection, **kw)

        display.vv('Redis connection: %s' % self._db)
+        display.vvv("Redis connection kwargs: %s" % ({**self._db.get_connection_kwargs(), **{'password': '****'}}))


I tried to find documentation related to this method get_connection_kwargs() in the Redis lib, but could not find anything very straightforward. Code also seems to be referencing a dynamic connection class. I am concerned that some sensitive info might be leaked by printing the entire result.

russoz · 2024-08-06T01:35:44Z

plugins/cache/redis.py

+            from os.path import isfile
+            import ssl
+
+            if not self._ssl_cert_reqs.upper() in list(map(lambda x: x.name.split('_')[1], ssl.VerifyMode)):


Coercing to list is redundant here:

Suggested change

if not self._ssl_cert_reqs.upper() in list(map(lambda x: x.name.split('_')[1], ssl.VerifyMode)):

if not self._ssl_cert_reqs.upper() in map(lambda x: x.name.split('_')[1], ssl.VerifyMode):

russoz · 2024-08-06T01:42:32Z

plugins/cache/redis.py

-          - To use encryption in transit, prefix the connection with V(tls://), as in V(tls://localhost:6379:0:changeme).
-          - To use redis sentinel, use separator V(;), for example V(localhost:26379;localhost:26379;0:changeme). Requires redis>=2.9.0.
+      _decode_responses:
+        description: If set to `true`, returned values from Redis commands get decoded automatically using the client's charset value.


Suggested change

description: If set to `true`, returned values from Redis commands get decoded automatically using the client's charset value.

description: If set to C(true), returned values from Redis commands get decoded automatically using the client's charset value.

russoz · 2024-08-06T01:42:53Z

plugins/cache/redis.py

+      _encoding_errors:
+        description:
+          - The error handling scheme to use for encoding errors.
+          - The default is `strict` meaning that encoding errors raise a `UnicodeEncodeError`.


Suggested change

- The default is `strict` meaning that encoding errors raise a `UnicodeEncodeError`.

- The default is V(strict) meaning that encoding errors raise a C(UnicodeEncodeError).

Please use (here and elsewhere) the semantic markup for Ansible.
See: https://docs.ansible.com/ansible/latest/dev_guide/developing_modules_documenting.html#semantic-markup-within-module-documentation

russoz · 2024-08-06T01:43:58Z

changelogs/fragments/8578-feat-add-redis-connection-parameters-full-control.yml

@@ -0,0 +1,2 @@
+minor_changes:
+  - redis cache plugin - add Redis connection parameters full access in order to be able to set advanced socket options, like enabling keep alive (https://github.com/ansible-collections/community.general/pull/8578)


Suggested change

- redis cache plugin - add Redis connection parameters full access in order to be able to set advanced socket options, like enabling keep alive (https://github.com/ansible-collections/community.general/pull/8578)

- redis cache plugin - add Redis connection parameters full access in order to be able to set advanced socket options, like enabling keep alive (https://github.com/ansible-collections/community.general/pull/8578).

russoz · 2024-08-15T09:36:17Z

plugins/cache/redis.py

+        self._socket_connect_timeout = int(self.get_option('_socket_connect_timeout')) \
+            if self._socket_keepalive and self.get_option('_socket_connect_timeout') else None


As a personal preference for style, I do like using the x = a if b else c construct for smaller expressions. In this case, I think it might be more readable expanding this to a "normal" if block.

ansibullbot added WIP Work in progress cache cache plugin feature This issue/PR relates to a feature request new_contributor Help guide this first time contributor plugins plugin (any type) labels Jul 3, 2024

This comment was marked as outdated.

Sign in to view

pierre-claranet force-pushed the main branch from fa9475b to f15ba37 Compare July 4, 2024 07:13

ansibullbot added the needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI label Jul 4, 2024

pierre-claranet marked this pull request as ready for review July 4, 2024 07:16

ansibullbot removed WIP Work in progress needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI labels Jul 4, 2024

pierre-claranet force-pushed the main branch 3 times, most recently from 73ac680 to be4651c Compare July 4, 2024 08:08

felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-9 Automatically create a backport for the stable-9 branch labels Jul 4, 2024

felixfontein reviewed Jul 5, 2024

View reviewed changes

ansibullbot added stale_ci CI is older than 7 days, rerun before merging and removed stale_ci CI is older than 7 days, rerun before merging labels Jul 18, 2024

This comment has been minimized.

Sign in to view

ansibullbot added the ci_verified Push fixes to PR branch to re-run CI label Jul 18, 2024

pierre-claranet force-pushed the main branch from 791db34 to 15eb6b3 Compare July 18, 2024 13:13

ansibullbot removed the ci_verified Push fixes to PR branch to re-run CI label Jul 18, 2024

pierre-claranet force-pushed the main branch 3 times, most recently from 0b4d87e to b606b85 Compare July 18, 2024 13:59

ansibullbot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Jul 18, 2024

ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Jul 26, 2024

ansibullbot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Aug 3, 2024

This comment was marked as outdated.

Sign in to view

ansibullbot added merge_commit This PR contains at least one merge commit. Please resolve! and removed stale_ci CI is older than 7 days, rerun before merging needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Aug 5, 2024

pierre-claranet force-pushed the main branch from 4d20ec4 to dd339e0 Compare August 5, 2024 14:31

This comment was marked as outdated.

Sign in to view

ansibullbot added merge_commit This PR contains at least one merge commit. Please resolve! and removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI labels Aug 5, 2024

pierre-claranet force-pushed the feat_redis_cache_plugin branch from 048094e to dd339e0 Compare August 5, 2024 15:02

ansibullbot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI and removed merge_commit This PR contains at least one merge commit. Please resolve! labels Aug 5, 2024

pierre-claranet force-pushed the feat_redis_cache_plugin branch from dd339e0 to 4b1b1d3 Compare August 5, 2024 15:36

ansibullbot removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Aug 5, 2024

feat[redis cache plugin]: Add Redis connection parameters full contro…

00b3468

…l support

pierre-claranet force-pushed the feat_redis_cache_plugin branch from 4b1b1d3 to 00b3468 Compare August 5, 2024 15:44

ansibullbot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Aug 5, 2024

russoz requested changes Aug 15, 2024

View reviewed changes

ansibullbot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR stale_ci CI is older than 7 days, rerun before merging labels Aug 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Redis connection parameters full control #8578

Add Redis connection parameters full control #8578

pierre-claranet commented Jul 3, 2024 •

edited

Loading

This comment was marked as outdated.

felixfontein left a comment

felixfontein Jul 5, 2024

pierre-claranet Jul 9, 2024

russoz Jul 10, 2024

russoz Jul 10, 2024

felixfontein Jul 10, 2024

pierre-claranet Jul 18, 2024

This comment has been minimized.

This comment was marked as outdated.

This comment was marked as outdated.

pierre-claranet commented Aug 5, 2024

ansibullbot commented Aug 5, 2024

pierre-claranet commented Aug 8, 2024

ansibullbot commented Aug 8, 2024

russoz left a comment

russoz Aug 6, 2024

russoz Aug 6, 2024 •

edited

Loading

russoz Aug 6, 2024

russoz Aug 6, 2024

russoz Aug 6, 2024

russoz Aug 15, 2024

russoz Aug 6, 2024

russoz Aug 15, 2024

	if not self._ssl_cert_reqs.upper() in list(map(lambda x: x.name.split('_')[1], ssl.VerifyMode)):
	if not self._ssl_cert_reqs.upper() in map(lambda x: x.name.split('_')[1], ssl.VerifyMode):

	description: If set to `true`, returned values from Redis commands get decoded automatically using the client's charset value.
	description: If set to C(true), returned values from Redis commands get decoded automatically using the client's charset value.

	- The default is `strict` meaning that encoding errors raise a `UnicodeEncodeError`.
	- The default is V(strict) meaning that encoding errors raise a C(UnicodeEncodeError).

		@@ -0,0 +1,2 @@
		minor_changes:
		- redis cache plugin - add Redis connection parameters full access in order to be able to set advanced socket options, like enabling keep alive (https://github.com/ansible-collections/community.general/pull/8578)

		self._socket_connect_timeout = int(self.get_option('_socket_connect_timeout')) \
		if self._socket_keepalive and self.get_option('_socket_connect_timeout') else None

Add Redis connection parameters full control #8578

Are you sure you want to change the base?

Add Redis connection parameters full control #8578

Conversation

pierre-claranet commented Jul 3, 2024 • edited Loading

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

This comment was marked as outdated.

felixfontein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment was marked as outdated.

This comment was marked as outdated.

pierre-claranet commented Aug 5, 2024

ansibullbot commented Aug 5, 2024

pierre-claranet commented Aug 8, 2024

ansibullbot commented Aug 8, 2024

russoz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

russoz Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierre-claranet commented Jul 3, 2024 •

edited

Loading

russoz Aug 6, 2024 •

edited

Loading