-
-
Notifications
You must be signed in to change notification settings - Fork 746
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
Fix SSL support for MongoDB and RabbitMQ under Python 3.x #4834
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
connection errors better. Without specifying a lower serverSelectionTimeoutMs value, the client would wait up to 30 seconds (default value) in case there are SSL errors (e.g. handshake failed or similar). With lower timeout we ensure a faster failure on fatal errors.
as early as possible. This important, because if we don't do it early enough and "ssl" module is imported before monkey patching is performed, SSL support for MongoDB won't work. Fixes issue reported in #4832.
pull-request-size
bot
added
the
size/M
PR that changes 30-99 lines. Good size to review.
label
Dec 16, 2019
Kami
commented
Dec 16, 2019
connection = mongoengine.connection.connect(db_name, host=db_host, | ||
port=db_port, tz_aware=True, | ||
username=username, password=password, | ||
serverSelectionTimeoutMS=5000, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For "just in case", I will also make this configurable via st2.conf.
pull-request-size
bot
added
size/L
PR that changes 100-499 lines. Requires some effort to review.
and removed
size/M
PR that changes 30-99 lines. Good size to review.
labels
Dec 16, 2019
arm4b
reviewed
Dec 16, 2019
arm4b
reviewed
Dec 16, 2019
arm4b
approved these changes
Dec 16, 2019
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for investigating and fixing this! 👍
tests will fail under Python 3.
Sadly due to how tests run, we need to add monkey patch to another file which is imported before the actual affected test file (nose imports all the tests in the same process and relies on the ordering).
Co-Authored-By: Eugen C. <[email protected]>
arm4b
reviewed
Dec 16, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request fixes #4832 SSL support for MongoD and RabbitMQ when running under Python 3.6.
Background / Details
When running under Python 3 and enabling SSL support for MongoDB, st2api and st2auth would fail with a cryptic error:
After some more digging in and manual instrumentation, I managed to track down the original exception / root cause - https://gist.github.com/Kami/ea8e63cdc539fd879fff41271969d650.
The root cause was an SSL error which happened because
st2api
andst2auth
didn't perform eventlet monkey patching early enough. They performed monkey patching after some other module (likely mongoengine or pymongo) already importedssl
which has undefined behavior and won't work.Proposed Fix
This pull request fixes the issue by making sure we perform eventlet monkey patching as early as possible (before any other modules are imported) inside
st2api
andst2auth
service.In addition to that, I made another change by setting
serverSelectionTimeoutMS
MongoClient option to 5 seconds.It defaults to 30 seconds which means if there is a SSL related connection error (e.g. handshake failed), exception won't be raised until 30 seconds has passed.
Keep in mind though that in such scenarios, only "connection closed" exception will be returned to the user and for the actual root cause, user will need to check the mongo server logs (that's the limitation of the client / server and nothing we can do).
This will also provide a better user-experience because previously in many scenarios our code would wait 30 seconds for this timeout to be reached before propagating the connection error.
Affected Components / Services
Based on my digging it, this issue only affected
st2api
andst2auth
because other services already performed eventlet monkey patching as early as possible.This also explains why user reported that
st2-register-content
worked in #4832 (we intentionally don't perform any monkey patching there so it works fine).Another thing worth keeping in mind is that we have two entry points for
st2api
andst2auth
- WSGI one (for production gunicorn deployments) and the direct eventlet WSGI server one (for local testing and development).Both entry points were affected so I needed to fix both.
Configuration
For completeness, here is the st2.conf and mongod.conf configuration I used:
st2.conf:
mongod.conf: