Fix SSL support for MongoDB and RabbitMQ under Python 3.x

CHANGELOG.rst

@@ -47,6 +47,10 @@ Changed
   function must be called separately. (improvement)
 * Update various internal dependencies to latest stable versions (cryptography, jinja2, requests,
   apscheduler, eventlet, amqp, kombu, semver, six) #4819 (improvement)
+* Improve MongoDB connection timeout related code. Connection and server selection timeout is now
+  set to 3 seconds. Previously a default value of 30 seconds was used which means that for many
+  connection related errors, our code would first wait for this timeout to be reached (30 seconds)
+  before returning error to the end user. #4384
 
 Fixed
 ~~~~~
@@ -91,6 +95,11 @@ Fixed
 * Update all the various rule criteria comparison operators which also work with strings (equals,
   icontains, nequals, etc.) to work correctly on Python 3 deployments if one of the operators is
   of a type bytes and the other is of a type unicode / string. (bug fix) #4831
+* Fix SSL connection support for MongoDB and RabbitMQ which wouldn't work under Python 3 and would
+  result in cryptic "maximum recursion depth exceeded while calling a Python object" error on
+  connection failure. (bug fix) #4382 #4384
+
+  Reported by @alexku7.
 
 3.1.0 - June 27, 2019
 ---------------------

conf/st2.conf.sample

@@ -119,6 +119,8 @@ ssl_ca_certs = None
 ssl_certfile = None
 # Connection retry backoff max (seconds).
 connection_retry_backoff_max_s = 10
+# If True and `ssl_cert_reqs` is not None, enables hostname verification
+ssl_match_hostname = True
 # Specifies whether a certificate is required from the other side of the connection, and whether it will be validated if provided
 ssl_cert_reqs = None
 # Create the connection to mongodb using SSL
@@ -133,8 +135,8 @@ connection_retry_backoff_mul = 1
 authentication_mechanism = None
 # Private keyfile used to identify the local connection against MongoDB.
 ssl_keyfile = None
-# If True and `ssl_cert_reqs` is not None, enables hostname verification
-ssl_match_hostname = True
+# Connection and server selection timeout (in ms).
+connection_timeout = 3000
 # password for db login
 password = None
 # port of db server

st2api/st2api/app.py

@@ -24,7 +24,6 @@
 from st2common.middleware.instrumentation import RequestInstrumentationMiddleware
 from st2common.middleware.instrumentation import ResponseInstrumentationMiddleware
 from st2common.router import Router
-from st2common.util.monkey_patch import monkey_patch
 from st2common.constants.system import VERSION_STRING
 from st2common.service_setup import setup as common_setup
 from st2common.util import spec_loader
@@ -33,16 +32,14 @@
 LOG = logging.getLogger(__name__)
 
 
-def setup_app(config={}):
+def setup_app(config=None):
+    config = config or {}
+
     LOG.info('Creating st2api: %s as OpenAPI app.', VERSION_STRING)
 
     is_gunicorn = config.get('is_gunicorn', False)
     if is_gunicorn:
-        # Note: We need to perform monkey patching in the worker. If we do it in
-        # the master process (gunicorn_config.py), it breaks tons of things
-        # including shutdown
-        monkey_patch()
-
+        # NOTE: We only want to perform this logic in the WSGI worker
         st2api_config.register_opts()
         capabilities = {
             'name': 'api',

st2api/st2api/cmd/api.py

@@ -15,14 +15,20 @@
 import os
 import sys
 
+# NOTE: It's important that we perform monkey patch as early as possible before any other modules
+# are important, otherwise SSL support for MongoDB won't work.
+# See https://github.com/StackStorm/st2/issues/4832 and https://github.com/gevent/gevent/issues/1016
+# for details.
+from st2common.util.monkey_patch import monkey_patch
+monkey_patch()
+
 import eventlet
 from oslo_config import cfg
 from eventlet import wsgi
 
 from st2common import log as logging
 from st2common.service_setup import setup as common_setup
 from st2common.service_setup import teardown as common_teardown
-from st2common.util.monkey_patch import monkey_patch
 from st2api import config
 config.register_opts()
 from st2api import app
@@ -33,8 +39,6 @@
     'main'
 ]
 
-monkey_patch()
-
 LOG = logging.getLogger(__name__)
 
 # How much time to give to the request in progress to finish in seconds before killing them

st2api/st2api/wsgi.py

@@ -12,8 +12,22 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+"""
+WSGI entry point used by gunicorn.
+"""
+
 import os
 
+from st2common.util.monkey_patch import monkey_patch
+# Note: We need to perform monkey patching in the worker. If we do it in
+# the master process (gunicorn_config.py), it breaks tons of things
+# including shutdown
+# NOTE: It's important that we perform monkey patch as early as possible before any other modules
+# are important, otherwise SSL support for MongoDB won't work.
+# See https://github.com/StackStorm/st2/issues/4832 and https://github.com/gevent/gevent/issues/1016
+# for details.
+monkey_patch()
+
 from st2api import app
 
 config = {

st2auth/st2auth/app.py

@@ -22,7 +22,6 @@
 from st2common.middleware.instrumentation import RequestInstrumentationMiddleware
 from st2common.middleware.instrumentation import ResponseInstrumentationMiddleware
 from st2common.router import Router
-from st2common.util.monkey_patch import monkey_patch
 from st2common.constants.system import VERSION_STRING
 from st2common.service_setup import setup as common_setup
 from st2common.util import spec_loader
@@ -32,16 +31,14 @@
 LOG = logging.getLogger(__name__)
 
 
-def setup_app(config={}):
+def setup_app(config=None):
+    config = config or {}
+
     LOG.info('Creating st2auth: %s as OpenAPI app.', VERSION_STRING)
 
     is_gunicorn = config.get('is_gunicorn', False)
     if is_gunicorn:
-        # Note: We need to perform monkey patching in the worker. If we do it in
-        # the master process (gunicorn_config.py), it breaks tons of things
-        # including shutdown
-        monkey_patch()
-
+        # NOTE: We only want to perform this logic in the WSGI worker
         st2auth_config.register_opts()
         capabilities = {
             'name': 'auth',

st2auth/st2auth/wsgi.py

@@ -14,6 +14,16 @@
 
 import os
 
+from st2common.util.monkey_patch import monkey_patch
+# Note: We need to perform monkey patching in the worker. If we do it in
+# the master process (gunicorn_config.py), it breaks tons of things
+# including shutdown
+# NOTE: It's important that we perform monkey patch as early as possible before any other modules
+# are important, otherwise SSL support for MongoDB won't work.
+# See https://github.com/StackStorm/st2/issues/4832 and https://github.com/gevent/gevent/issues/1016
+# for details.
+monkey_patch()
+
 from st2auth import app
 
 config = {

st2common/st2common/config.py

@@ -176,6 +176,9 @@ def register_opts(ignore_errors=False):
         cfg.StrOpt(
             'password',
             help='password for db login'),
+        cfg.IntOpt(
+            'connection_timeout', default=3 * 1000,
+            help='Connection and server selection timeout (in ms).'),
         cfg.IntOpt(
             'connection_retry_max_delay_m', default=3,
             help='Connection retry total time (minutes).'),

st2common/st2common/models/db/__init__.py

@@ -20,11 +20,13 @@
 import ssl as ssl_lib
 
 import six
+from oslo_config import cfg
 import mongoengine
 from mongoengine.queryset import visitor
 from pymongo import uri_parser
 from pymongo.errors import OperationFailure
 from pymongo.errors import ConnectionFailure
+from pymongo.errors import ServerSelectionTimeoutError
 
 from st2common import log as logging
 from st2common.util import isotime
@@ -122,9 +124,15 @@ def _db_connect(db_name, db_host, db_port, username=None, password=None,
                                  authentication_mechanism=authentication_mechanism,
                                  ssl_match_hostname=ssl_match_hostname)
 
+    # NOTE: We intentionally set "serverSelectionTimeoutMS" to 3 seconds. By default it's set to
+    # 30 seconds, which means it will block up to 30 seconds and fail if there are any SSL related
+    # or other errors
+    connection_timeout = cfg.CONF.database.connection_timeout
     connection = mongoengine.connection.connect(db_name, host=db_host,
                                                 port=db_port, tz_aware=True,
                                                 username=username, password=password,
+                                                connectTimeoutMS=connection_timeout,
+                                                serverSelectionTimeoutMS=connection_timeout,
                                                 **ssl_kwargs)
 
     # NOTE: Since pymongo 3.0, connect() method is lazy and not blocking (always returns success)
@@ -134,7 +142,10 @@ def _db_connect(db_name, db_host, db_port, username=None, password=None,
     try:
         # The ismaster command is cheap and does not require auth
         connection.admin.command('ismaster')
-    except ConnectionFailure as e:
+    except (ConnectionFailure, ServerSelectionTimeoutError) as e:
+        # NOTE: ServerSelectionTimeoutError can also be thrown if SSLHandShake fails in the server
+        # Sadly the client doesn't include more information about the error so in such scenarios
+        # user needs to check MongoDB server log
         LOG.error('Failed to connect to database "%s" @ "%s" as user "%s": %s' %
                   (db_name, host_string, str(username_string), six.text_type(e)))
         raise e

st2common/tests/unit/test_db.py

@@ -15,12 +15,16 @@
 from __future__ import absolute_import
 
 import ssl
+import time
 
 import jsonschema
 import mock
 import mongoengine.connection
+from mongoengine.connection import disconnect
 from oslo_config import cfg
 from pymongo.errors import ConnectionFailure
+from pymongo.errors import ServerSelectionTimeoutError
+
 from st2common.constants.triggers import TRIGGER_INSTANCE_PROCESSED
 from st2common.models.system.common import ResourceReference
 from st2common.transport.publishers import PoolPublisher
@@ -86,6 +90,13 @@ def test_index_name_length(self):
 
 
 class DbConnectionTestCase(DbTestCase):
+    def setUp(self):
+        # NOTE: It's important we re-establish a connection on each setUp
+        self.setUpClass()
+
+    def tearDown(self):
+        # NOTE: It's important we disconnect here otherwise tests will fail
+        disconnect()
 
     def test_check_connect(self):
         """
@@ -180,7 +191,9 @@ def test_db_setup(self, mock_mongoengine):
             'tz_aware': True,
             'authentication_mechanism': 'MONGODB-X509',
             'ssl': True,
-            'ssl_match_hostname': True
+            'ssl_match_hostname': True,
+            'connectTimeoutMS': 3000,
+            'serverSelectionTimeoutMS': 3000
         })
 
     @mock.patch('st2common.models.db.mongoengine')
@@ -299,6 +312,37 @@ def test_db_setup_connecting_info_logging(self, mock_log, mock_mongoengine):
         actual_message = mock_log.error.call_args_list[0][0][0]
         self.assertEqual(expected_message, actual_message)
 
+    def test_db_connect_server_selection_timeout_ssl_on_non_ssl_listener(self):
+        # Verify that the we wait connection_timeout ms (server selection timeout ms) before failing
+        # and propagating the error
+        disconnect()
+
+        db_name = 'st2'
+        db_host = 'localhost'
+        db_port = 27017
+
+        cfg.CONF.set_override(name='connection_timeout', group='database', override=1000)
+
+        start = time.time()
+        self.assertRaises(ServerSelectionTimeoutError, db_setup, db_name=db_name, db_host=db_host,
+                          db_port=db_port, ssl=True)
+        end = time.time()
+        diff = (end - start)
+
+        self.assertTrue(diff >= 1)
+
+        disconnect()
+
+        cfg.CONF.set_override(name='connection_timeout', group='database', override=400)
+
+        start = time.time()
+        self.assertRaises(ServerSelectionTimeoutError, db_setup, db_name=db_name, db_host=db_host,
+                          db_port=db_port, ssl=True)
+        end = time.time()
+        diff = (end - start)
+
+        self.assertTrue(diff >= 0.4)
+
 
 class DbCleanupTestCase(DbTestCase):
     ensure_indexes = True