add ssl support

[cancan101]

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

• I think the code is well written
• Unit tests for the changes exist
• Documentation reflects the changes
• Add a new news fragment into the CHANGES folder
  • name it <issue_id>.<type> (e.g. 588.bugfix)
  • if you don't have an issue_id change it to the pr id after creating the PR
  • ensure type is one of the following:
    • .feature: Signifying a new feature.
    • .bugfix: Signifying a bug fix.
    • .doc: Signifying a documentation improvement.
    • .removal: Signifying a deprecation or removal of public API.
    • .misc: A ticket has been closed, but it is not of interest to users.
  • Make sure to use full sentences with correct case and punctuation, for example: Fix issue with non-ascii contents in doctest text files.

[Dreamsorcerer]

The example here suggests we only need to pass ssl=True:
https://redis-py.readthedocs.io/en/stable/examples/ssl_connection_examples.html#Connecting-to-a-Redis-instance-via-SSL

I'm wondering if we should maybe just accept arbitrary arguments and pass them through to Redis() instead. Then the user can configure however they like.

[cancan101]

The single SSL parameter is not enough in the general sense. There are a set of parameters specific to SSL, for example to ignore checking the certificate chain, which is required on heroku.

In addition there is a strong case to be made to allow specifying the connection pool class, for example, to allow turning on blocking mode. See redis/redis-py#2517

[Dreamsorcerer]

More reason to just pass all arguments through, rather than trying to maintain a list of arguments.

[Dreamsorcerer]

My first thought is to just move all the existing arguments into a dict, which can be used as default values. Then just do defaults.update(kwargs) and pass that to Redis().

That would disallow positional arguments, but not sure that's a major loss.

[cancan101]

Right and I have connection_pool_kwargs. One of the parameters is specifically overridden per comment and a couple of the others are remapped based on names that this library uses more generally but sure there might be one or two that can be factored out as a general spread through

[Dreamsorcerer]

I don't think there's any need for naming consistency, ultimately they are arguments for Redis(), not our cache (MemoryCache doesn't use any of the arguments for example).

Just realised we probably can't use kwargs though, because some arguments are needed for BaseCache. So, maybe just have a single redis_args parameter which takes a dict of arguments to be passed in.

[cancan101]

Right, that is why I did the connection_pool_kwargs. That being said, I totally follow how you would suggest changing what I have in the PR?

[Dreamsorcerer]

Right, that is why I did the connection_pool_kwargs.

If we just allow arbitrary arguments for the entire Redis() class, then this awkwardness is unneeded. A user can simple do:
redis_args={"connection_pool": SSLConnection(...)}

[Dreamsorcerer]

Maybe this PR is reasonable as a backwards-compatible approach for 0.x though.

If you're happy to go with that, then please retarget this PR to the 0.12 branch and create a new one for master that uses redis_args.

[cancan101]

I want to make sure I understand what you are suggesting for the change to master. At present, a redis Cache would be instantiated as:

cache = Cache(
  Cache.REDIS,
  endpoint="127.0.0.1",
  port=6379,
  password="xyz"
)

with this PR (as is), it could be instantiated as (for SSL + Blocking support):

cache = Cache(
  Cache.REDIS,
  endpoint="127.0.0.1",
  port=6379,
  password="xyz",
  ssl=True,
  connection_pool_class=redis.BlockingConnectionPool,
  connection_pool_kwargs={"ssl_cert_reqs": None}
)

and it seems like you are suggesting:

cache = Cache(
  Cache.REDIS,
  redis_args={"connection_pool": BlockingConnectionPool(dict(
    host="127.0.0.1",
    port=6379,
    password="xyz",
    decode_responses=False, # make sure to not forget this!
    connection_class=redis.SSLConnection
    ssl_cert_reqs=None
  ))}
)

which IMHO is not great DX and it makes the params very different for different Cache types.

[Dreamsorcerer]

it makes the params very different for different Cache types.

The params are different for different cache types..
You can't just expect to pass the same connection params for a redis backend to a memcached backend (or some other backend) and have them work. This library literally just passes them to another library, so we have little control over how they work or what values are valid. I can't see a reason why they'd be expected to be the same.

There are BaseCache parameters which are the same across caches, and then there are connection parameters (maybe we can call it conn_args for consistency, rather than redis_args) which depend entirely on the backend library and could be vastly different from one cache to another.

By doing something like this, the advantages are that the user has full flexibility to pass any valid arguments desired to customise the backend, the code is kept very simple resulting in not needing to make changes to keep up-to-date with the upstream library and we don't get more bug reports when another user wants to change SSL ciphers or whatever other details come up in future.

[cancan101]

then there are connection parameters

Ideally there would be a way to still parse the DB url to get a dict rather than an instantiation of the Cache itself.

cls, cache_params = Cache.params_from_url(cache_url)
# For: rediss://:pass@host:port/db (injected by Heroku for example)
# -> 
Cache.REDIS,
#-> 
{"connection_pool": {
    "host": "127.0.0.1",
    "port": 6379,
    "password": "xyz",
    "decode_responses": False, # okay, but where does this come from?
    "connection_class": redis.SSLConnection, # would be cool to get this from the url
    #ssl_cert_reqs=None # fine, this would have to be set by the user since is Heroku specific
}}

Having a nice utility function like that would be nice and perhaps act a bridge between to the new API .

[Dreamsorcerer]

That could be nice, I'd look at it as a separate feature though. e.g. the prefix (redis) could be used to select the correct cache class, and then the URL could be passed to a from_url() class method or similar (note there is a Redis.from_url() so we wouldn't want to duplicate that code, just pass it through unmodified).

[cancan101]

I do think there is value is splitting up the parsing of the URL from the instantiation. i.e. Cache.from_url() currently parses and instantiates which means if I want to inject Redis specific kwargs, that isn't possible. In the two step, I can optionally inject the Redis specific kwargs. This comes for example in that locally and in unit testing I use in memory cache (memory://) where as the deployed site uses redis. Since Heroku generates the URL, I can't just modify the redis URL to insert the blocking setting (which isn't easily usable anyways as I need to specify a class rather than a string).

There are still warts / leaks in the abstraction, for example the decode_responses needing to be set to False by default.

[rgbkrk]

As an aside, now that I'm hitting this issue -- should aiocache be using aioredis, from the same org aio-libs?

[Dreamsorcerer]

aioredis migrated to upstream redis. aioredis is no longer supported.

[rgbkrk]

Rad, thank you!

[antont]

How about passing a connection pool instance as an argument?

I don't mean that as a replacement for passing creation arguments, but for a different use case, where the pool exists already. This is a different issue, but related to the same topic, i.e. the cache creation, anyway.

We are using Redis connections also for other purposes, namely storing session tokens in fastapi-users. Initially we had multiple redis clients happening from various libs, but I was then happy to centralize it to one module, and passing client or the pool from there.

I integrated aiocache redis to our app now, but didn't see a way to pass the existing pool, so am for now just letting it do the default behaviour. Am now considering subclassing to customize the initialialization, maybe to pass an existing redis client instance.

Am no Redis expert, so don't really know if it's an issue that multiple clients are created. Earlier just wanted to reduce the amount of moving parts and ensure consistent configuration etc. by having a single place for pool and client creation.

[Dreamsorcerer]

How about passing a connection pool instance as an argument?

That's exactly what I suggested in my change request:
#691 (comment)

It might make sense to just remove all these arguments and have the user pass in a Redis instance all the time. Would simplify what needs to be maintained here considerably, while providing the maximum amount of configuration. I don't think it adds much more effort for the user either.

The only potential issue is that it appears we need decode_responses=False (which is the default). It looks like we could atleast assert it with client.connection_pool.connection_kwargs.get("decode_responses").

[Dreamsorcerer]

Again, if someone wants to create a new PR that accepts just accepts Redis client, I'll look at merging that into the master branch.
This PR can be retargeted to the 0.12 branch as a temporary fix that's backwards compatible.

[antont]

It might make sense to just remove all these arguments and have the user pass in a Redis instance all the time.

I did this for us now by subclassing:

import aiocache
from aiocache.base import BaseCache

class AiocacheRedisCacheForClient(aiocache.RedisCache):
    def __init__(self, client, **kwargs):
        """skips RedisCache.__init__"""
        BaseCache.__init__(self, **kwargs)
        self.client = client

I also encountered exactly the problem with decode_responses=False, because the pool I had created for fastapi-users has that option enabled, which is required there. This was a bit tricky to notice first. I tried using NullSerializer and other things, but finally resorted to creating two pools, one with responses decoded and the other not. It's still nice how we can have the config options in one place, where both pools are created.

So +1 for the assert you suggested.

[ben-cutler-datarobot]

Again, if someone wants to create a new PR that accepts just accepts Redis client, I'll look at merging that into the master branch. This PR can be retargeted to the 0.12 branch as a temporary fix that's backwards compatible.

lmk how you like this

[Dreamsorcerer]

We've updated 1.0 to just accept a Redis client now.
@cancan101 If you'd like to retarget this PR to the 0.12 branch, I'm happy to merge this in if it's needed in the meantime.

[cancan101]

Is there any way to re-open the PR?

[cancan101]

ok, branch changed and rebased. lmk your thoughts.

[Dreamsorcerer]

Looks good to me, want to mark the PR as ready?

[Dreamsorcerer]

Looks like we have a few failing tests too, if you have some time to look at those?

[codecov]

Codecov Report

Attention: Patch coverage is 94.73684% with 1 line in your changes missing coverage. Please review.

Project coverage is 99.55%. Comparing base (b29269c) to head (09b1ee5).
Report is 4 commits behind head on 0.12.

Files with missing lines	Patch %	Lines
aiocache/backends/redis.py	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             0.12     #691      +/-   ##
==========================================
- Coverage   99.57%   99.55%   -0.03%     
==========================================
  Files          35       35              
  Lines        3770     3782      +12     
==========================================
+ Hits         3754     3765      +11     
- Misses         16       17       +1     

Flag	Coverage Δ
unit	99.55% <94.73%> (-0.03%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
tests/ut/backends/test_redis.py	100.00% <100.00%> (ø)
aiocache/backends/redis.py	98.41% <85.71%> (-0.76%)	⬇️

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b29269c...09b1ee5. Read the comment docs.

[cancan101]

Ok tests fixed