Redis cache broken on python3 due to string literals

[cheald]

Description of Issue

Using the redis cache module on Python 3 fails to ever return any cached data. Data is written with "SET" "$KEY_minions/minionname/data", but it is then attempted to be read with "GET" "$KEY_minions/b'minionname'/data".

Setup

Just set cache: redis and install a local redis server.

Steps to Reproduce Issue

1. Start the redis server. redis-cli into it, and execute monitor.
2. Start the salt-master. Watch it query your minion names with b''
3. Execute salt someminion pillar.update. Watch it set the cache without the b''.

Versions Report

Salt Version:
           Salt: 2019.2.0

Dependency Versions:
           cffi: Not Installed
       cherrypy: unknown
       dateutil: 2.6.1
      docker-py: Not Installed
          gitdb: 2.0.3
      gitpython: 2.1.8
          ioflo: Not Installed
         Jinja2: 2.10
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: Not Installed
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.5.6
   mysql-python: Not Installed
      pycparser: Not Installed
       pycrypto: 2.6.1
   pycryptodome: Not Installed
         pygit2: Not Installed
         Python: 3.6.8 (default, Aug 20 2019, 17:12:48)
   python-gnupg: 0.4.1
         PyYAML: 3.12
          PyZMQ: 16.0.2
           RAET: Not Installed
          smmap: 2.0.3
        timelib: Not Installed
        Tornado: 4.5.3
            ZMQ: 4.2.5

System Versions:
           dist: Ubuntu 18.04 bionic
         locale: UTF-8
        machine: x86_64
        release: 4.15.0-64-generic
         system: Linux
        version: Ubuntu 18.04 bionic

[cheald]

https://github.com/saltstack/salt/commit/7ff72eb01fdd4645d68a813fd3f4bac024f42b0c is the commit that broke it. It appears that reinstating decode_responses=True fixes the cache fetch. However, it may be that this breaks the retrieval of the actual cache values (which should be binary msgpack strings, rather than UTF-8).

[cheald]

Manually decoding the list in cache.list seems to be closer to the right fix:

def list_(bank):
    '''
    Lists entries stored in the specified bank.
    '''
    redis_server = _get_redis_server()
    bank_redis_key = _get_bank_redis_key(bank)
    try:
-        banks = redis_server.smembers(bank_redis_key)
+        banks = [bank.decode("utf-8") for bank in redis_server.smembers(bank_redis_key)]
    except (RedisConnectionError, RedisResponseError) as rerr:
        mesg = 'Cannot list the Redis cache key {rkey}: {rerr}'.format(rkey=bank_redis_key,
                                                                       rerr=rerr)

We only ever sadd keys. Redis does permit binary keys, but since we're never doing any encoding on the keys themselves, I suspect it should be safe enough to interpret them as UTF-8 on the way back out.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.

[cheald]

This issue is still outstanding and should not be stale.

[stale[bot]]

Thank you for updating this issue. It is no longer marked as stale.

[marcosvbuzo]

This is still an issue and the proposed fix works.