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

[BUG] 3002.6 redis_cache.py not decoding responses #60273

Closed
major0 opened this issue May 28, 2021 · 4 comments · Fixed by #61229
Closed

[BUG] 3002.6 redis_cache.py not decoding responses #60273

major0 opened this issue May 28, 2021 · 4 comments · Fixed by #61229
Assignees
Labels
Bug broken, incorrect, or confusing behavior severity-high 2nd top severity, seen by most users, causes major problems

Comments

@major0
Copy link
Contributor

major0 commented May 28, 2021

Description
Redis added a decode_responses parameter to deal with the change from Python2 ASCII strings to Python3 bytes. It appears that the redis_return.py source was updated to use this parameter in commit bdaef39 , but redis_cache.py was not.

Have not yet confirmed this is a bug. Noticed this when tracking down a bug in the redis_return.py (issue #60272 )

Proposed fix:

diff --git a/salt/cache/redis_cache.py b/salt/cache/redis_cache.py
index 21bdcae3f7..5c60cb52f1 100644
--- a/salt/cache/redis_cache.py
+++ b/salt/cache/redis_cache.py
@@ -243,6 +243,7 @@ def _get_redis_server(opts=None):
         REDIS_SERVER = StrictRedisCluster(
             startup_nodes=opts["startup_nodes"],
             skip_full_coverage_check=opts["skip_full_coverage_check"],
+            decode_responses=True,
         )
     else:
         REDIS_SERVER = redis.StrictRedis(
@@ -251,6 +252,7 @@ def _get_redis_server(opts=None):
             unix_socket_path=opts["unix_socket_path"],
             db=opts["db"],
             password=opts["password"],
+            decode_responses=True,
         )
     return REDIS_SERVER

Versions Report

salt --versions-report (Provided by running salt --versions-report. Please also mention any differences in master/minion versions.)
$ salt --versions-report
Salt Version:
          Salt: 3002.6
 
Dependency Versions:
          cffi: Not Installed
      cherrypy: unknown
      dateutil: 2.7.3
     docker-py: Not Installed
         gitdb: 2.0.6
     gitpython: 3.0.7
        Jinja2: 2.10.1
       libgit2: 0.28.3
      M2Crypto: Not Installed
          Mako: Not Installed
       msgpack: 0.6.2
  msgpack-pure: Not Installed
  mysql-python: Not Installed
     pycparser: Not Installed
      pycrypto: 2.6.1
  pycryptodome: 3.6.1
        pygit2: 1.0.3
        Python: 3.8.5 (default, Jan 27 2021, 15:41:15)
  python-gnupg: 0.4.5
        PyYAML: 5.3.1
         PyZMQ: 18.1.1
         smmap: 2.0.5
       timelib: Not Installed
       Tornado: 4.5.3
           ZMQ: 4.3.2
 
System Versions:
          dist: ubuntu 20.04 focal
        locale: utf-8
       machine: aarch64
       release: 5.4.0-1038-aws
        system: Linux
       version: Ubuntu 20.04 focal
@major0 major0 added Bug broken, incorrect, or confusing behavior needs-triage labels May 28, 2021
@marbx
Copy link
Contributor

marbx commented May 31, 2021

I had to use decode_responses=True since Python3.

(Unfortunately, I have overlooked redis_cache and therefore reprogrammed it)

@sagetherage sagetherage changed the title [BUG] redis_cache.py not decoding responses [BUG] 3002.6 redis_cache.py not decoding responses Jun 1, 2021
@s0undt3ch
Copy link
Member

Valid issue, the module needs to be updated to use decode_responses=True

@s0undt3ch s0undt3ch added severity-critical top severity, seen by most users, serious issues and removed needs-triage labels Jun 28, 2021
@sagetherage sagetherage added severity-high 2nd top severity, seen by most users, causes major problems Silicon v3004.0 Release code name and removed severity-critical top severity, seen by most users, serious issues labels Jun 28, 2021
@sagetherage sagetherage added this to the Silicon milestone Jun 28, 2021
@s0undt3ch
Copy link
Member

@Ch3LL also mentioned that the sdb redis module also suffers from this.

@sagetherage sagetherage modified the milestones: Silicon, Approved Aug 9, 2021
@anilsil anilsil removed the Silicon v3004.0 Release code name label Oct 20, 2021
@anilsil anilsil modified the milestones: Approved, Phosphorus v3005.0 Oct 20, 2021
@waynew
Copy link
Contributor

waynew commented Nov 18, 2021

Was just trying to verify this and it turns out that for the redis_cache this
isn't actually a problem. We're using salt.payload.dumps/loads to convert
to/from bytes so text goes in and text comes out. My PR #61229 verifies this.
However - sdb redis probably still isn't fixed.

I'm going to go ahead and close this and open a new issue for sdb functional
tests, and note that redis sdb module will need to be updated correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug broken, incorrect, or confusing behavior severity-high 2nd top severity, seen by most users, causes major problems
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants