Add support for TLS connections to redis #2357

myoung34 · 2018-03-01T19:03:58Z

The current method does not allow secure redis rediss://

redash@1dfd88a3e15e:/app$ bin/docker-entrypoint shell                                                                                                
[2018-03-01 18:20:54,568][PID:160][INFO][root] Generating grammar tables from /usr/lib/python2.7/lib2to3/Grammar.txt                                                                                                                     
[2018-03-01 18:20:54,593][PID:160][INFO][root] Generating grammar tables from /usr/lib/python2.7/lib2to3/PatternGrammar.txt                                                                                                                   Traceback (most recent call last):                                                                                                                                                                                                              File "/app/manage.py", line 6, in <module>                                                                                                                                                                                                      from redash.cli import manager                                                                                                                                                                                                              File "/app/redash/__init__.py", line 74, in <module>                                                                                                                                                                                            reset_new_version_status()                                                                                                                                                                                                               
  File "/app/redash/version_check.py", line 34, in reset_new_version_status
    latest_version = get_latest_version()                                                                  
  File "/app/redash/version_check.py", line 40, in get_latest_version                                                                                                                                                                    
    return redis_connection.get(REDIS_KEY)                                                                                                                                                                                               
  File "/usr/local/lib/python2.7/dist-packages/redis/client.py", line 880, in get
    return self.execute_command('GET', name)                                                                                                                                                                                                 
  File "/usr/local/lib/python2.7/dist-packages/redis/client.py", line 578, in execute_command
    connection.send_command(*args)                             
  File "/usr/local/lib/python2.7/dist-packages/redis/connection.py", line 563, in send_command
    self.send_packed_command(self.pack_command(*args))                     
  File "/usr/local/lib/python2.7/dist-packages/redis/connection.py", line 538, in send_packed_command
    self.connect()                                              
  File "/usr/local/lib/python2.7/dist-packages/redis/connection.py", line 446, in connect                                                                                                                                                         self.on_connect()                                                                                                                                                                                                                        
  File "/usr/local/lib/python2.7/dist-packages/redis/connection.py", line 514, in on_connect                                                                                                                                                      if nativestr(self.read_response()) != 'OK':                                                                             
  File "/usr/local/lib/python2.7/dist-packages/redis/connection.py", line 577, in read_response                                                                                                                                              
    response = self._parser.read_response()                                  
  File "/usr/local/lib/python2.7/dist-packages/redis/connection.py", line 238, in read_response                            
    response = self._buffer.readline()                                                                                                                                                                                                          File "/usr/local/lib/python2.7/dist-packages/redis/connection.py", line 168, in readline
    self._read_from_socket()    
  File "/usr/local/lib/python2.7/dist-packages/redis/connection.py", line 143, in _read_from_socket
    (e.args,))                     
redis.exceptions.ConnectionError: Error while reading from socket: (104, 'Connection reset by peer')

After this change:

redash@a29e20353e5d:/app$ bin/docker-entrypoint shell
[GCC 5.4.0 20160609] on linux2
App: redash
Instance: /app/instance

myoung34 · 2018-03-01T20:34:21Z

Tests seem to be failing on Postgres related queries which are not related to my changes. Unsure if this is something I can fix

myoung34 · 2018-03-01T20:35:16Z

tests/__init__.py

-os.environ['REDASH_CELERY_BROKER'] = os.environ.get('REDASH_REDIS_URL', "redis://localhost:6379/0").replace("/5", "/6")
+PYTEST_ENABLE_REDIS_CLEANUP = ast.literal_eval(os.environ.get('PYTEST_ENABLE_REDIS_CLEANUP', 'True'))
+if PYTEST_ENABLE_REDIS_CLEANUP:
+    os.environ['REDASH_REDIS_URL'] = os.environ.get('REDASH_REDIS_URL', "redis://localhost:6379/0").replace("/0", "/5")


This is pretty useless when just using it locally

myoung34 · 2018-03-01T23:44:19Z

There appears to be more to it, celery is not picking up any SSL options.

The app starts but on login, celery/kombu throws a 500 with

KeyError

KeyError: 'No such transport: rediss.  Did you mean redis?'

arikfr

Thank you for adding this, supporting rediss:// can be nice.

The tests aren't failing on "Postgres related queries" but on Redis calls (check the stacktrace). I guess it's because of the use of Redis instead of StrictRedis (see comments).

If Celery really doesn't support this, then it's a blocker to adopting this... and we need to wait for Celery to support it or check if we can pass a Redis client to it instead of connection details.

And one last thing -- please try to keep the scope of each pull request limited to one thing. Makes reviewing easier and also merging faster.

arikfr · 2018-03-04T19:31:03Z

redash/__init__.py

-        r = redis.StrictRedis(host=redis_url.hostname, port=redis_url.port, db=redis_db, password=redis_password)
-
-    return r
+    return redis.from_url(settings.REDIS_URL)


You need to use StrictRedis.from_url, because redis.from_url returns a Redis object instead of StrictRedis.

arikfr · 2018-03-04T19:31:57Z

docker-compose.yml

    restart: unless-stopped
  postgres:
    image: postgres:9.5.6-alpine
+    ports:
+      - 5432:5432


While exposing the ports is useful, I don't think we should include this in the default configuration as it might conflict with other configurations the user might have.

arikfr · 2018-03-04T19:34:10Z

README.md

+$ npm install
+$ npm run build
+$ pytest tests
+```


This doesn't belong in the README, but in the developer docs. Also there is a much simpler method to run the tests (see Running Tests here).

arikfr · 2018-03-04T19:38:16Z

tests/__init__.py

-os.environ['REDASH_REDIS_URL'] = os.environ.get('REDASH_REDIS_URL', "redis://localhost:6379/0").replace("/0", "/5")
-# Use different url for Celery to avoid DB being cleaned up:
-os.environ['REDASH_CELERY_BROKER'] = os.environ.get('REDASH_REDIS_URL', "redis://localhost:6379/0").replace("/5", "/6")
+PYTEST_ENABLE_REDIS_CLEANUP = ast.literal_eval(os.environ.get('PYTEST_ENABLE_REDIS_CLEANUP', 'True'))


Please use true and redash.settings.parse_boolean to be consistent with other env variables we have.

But I don't see the value in adding this setting. It is useful locally, because when you use the same Redis server both for tests and development it's annoying (and causes issues) when Redis gets flushed.

The database replacement method here can be more elaborate, but because it's always possible to override with env variables, I figured we could just aim at the default.

myoung34 · 2018-03-05T12:46:58Z

Im stuck on the celery portion still for sure. It does support it

The relevant options are redis_backend_use_ssl and broker_use_ssl (which has a section on redis).

However it doesnt seem to support the rediss:// portion
I think the resolution to celery is to modify the broker settings here, but its beyond the scope of what I can accomplish with time being a factor

myoung34 · 2018-03-05T12:52:39Z

I went ahead and removed the docs/irrelevant changes @arikfr , its down to the TLS portion only

nason · 2019-04-30T18:02:29Z

I've added this capability in our fork (from the latest master as of 3/30/19) via narratorai#1

I'd be happy to add more tests, docs, etc and open up a PR upstream here -- would love any feedback!

arikfr · 2019-06-06T08:17:48Z

Closing this in favor of @nason's more complete PR.

Thank you for starting this, @myoung34 !

myoung34 force-pushed the master branch 3 times, most recently from c0edb2c to 2129eb7 Compare March 1, 2018 20:33

myoung34 commented Mar 1, 2018

View reviewed changes

arikfr requested changes Mar 4, 2018

View reviewed changes

Add support for TLS connections to redis

3cb82c4

myoung34 force-pushed the master branch from 2129eb7 to 3cb82c4 Compare March 5, 2018 12:52

weekly-digest bot mentioned this pull request May 6, 2019

Weekly Digest (29 April, 2019 - 6 May, 2019) #3763

Closed

nason mentioned this pull request May 29, 2019

Add support ssl connections to redis #3848

Merged

1 task

arikfr closed this Jun 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for TLS connections to redis #2357

Add support for TLS connections to redis #2357

myoung34 commented Mar 1, 2018 •

edited

Loading

myoung34 commented Mar 1, 2018

myoung34 Mar 1, 2018

myoung34 commented Mar 1, 2018

arikfr left a comment

arikfr Mar 4, 2018

arikfr Mar 4, 2018

arikfr Mar 4, 2018

arikfr Mar 4, 2018

myoung34 commented Mar 5, 2018

myoung34 commented Mar 5, 2018

nason commented Apr 30, 2019

arikfr commented Jun 6, 2019

Add support for TLS connections to redis #2357

Add support for TLS connections to redis #2357

Conversation

myoung34 commented Mar 1, 2018 • edited Loading

myoung34 commented Mar 1, 2018

myoung34 Mar 1, 2018

Choose a reason for hiding this comment

myoung34 commented Mar 1, 2018

arikfr left a comment

Choose a reason for hiding this comment

arikfr Mar 4, 2018

Choose a reason for hiding this comment

arikfr Mar 4, 2018

Choose a reason for hiding this comment

arikfr Mar 4, 2018

Choose a reason for hiding this comment

arikfr Mar 4, 2018

Choose a reason for hiding this comment

myoung34 commented Mar 5, 2018

myoung34 commented Mar 5, 2018

nason commented Apr 30, 2019

arikfr commented Jun 6, 2019

myoung34 commented Mar 1, 2018 •

edited

Loading