Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

redis cache - add support for TLS/encryption in transit #410

Merged
merged 4 commits into from May 30, 2020

Conversation

briantist
Copy link
Contributor

SUMMARY
  • Add support for encryption in transit / TLS connections, via a tls:// prefix added onto the connection uri.
  • Update documentation.

Current connection string looks like this: localhost:6379:0:changeme.
With this change, above still works, but if you need TLS now you can do: tls://localhost:6379:0:changeme.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

redis.py

ADDITIONAL INFORMATION

The connection uri in this plugin is split on : and then each element is sent verbatim positionally to the Redis library's constructor, which works great for the host, port, db, password params as they are the first four.

classredis.Redis(host=u'localhost', port=6379, db=0, password=None, socket_timeout=None, socket_connect_timeout=None, socket_keepalive=None, socket_keepalive_options=None, connection_pool=None, unix_socket_path=None, encoding=u'utf-8', encoding_errors=u'strict', charset=None, errors=None, decode_responses=False, retry_on_timeout=False, ssl=False, ssl_keyfile=None, ssl_certfile=None, ssl_cert_reqs=u'required', ssl_ca_certs=None, ssl_check_hostname=False, max_connections=None, single_connection_client=False, health_check_interval=0, client_name=None, username=None)

But the ssl param is way down the list at number 17, so having to supply all those other values that are almost surely defaulted, doesn't really make sense.

I added support to use a prefix, but otherwise didn't change anything about the uri, so it still just works just the same as it did and existing connection strings won't be affected.

@ansibullbot ansibullbot added affects_2.10 feature This issue/PR relates to a feature request labels May 25, 2020
Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need a changelog fragment (minor_changes:).

plugins/cache/redis.py Show resolved Hide resolved
@ansibullbot
Copy link
Collaborator

The test ansible-test sanity --test pep8 [explain] failed with 2 errors:

plugins/cache/redis.py:92:11: E111: indentation is not a multiple of four
plugins/cache/redis.py:93:11: E111: indentation is not a multiple of four

click here for bot help

@briantist

This comment has been minimized.

@briantist

This comment has been minimized.

@briantist

This comment has been minimized.

Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides that, LGTM.

changelogs/fragments/410-redis_cache-add_tls_support.yaml Outdated Show resolved Hide resolved
Co-authored-by: Felix Fontein <felix@fontein.de>
@ansibullbot ansibullbot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI stale_ci CI is older than 7 days, rerun before merging and removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI stale_ci CI is older than 7 days, rerun before merging labels May 26, 2020
@briantist

This comment has been minimized.

Copy link
Member

@resmo resmo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shipit

@felixfontein felixfontein merged commit f81e562 into ansible-collections:master May 30, 2020
@briantist briantist deleted the cache/redis-tls branch May 30, 2020 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature This issue/PR relates to a feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants