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

Work around TLS errors #23

Merged
merged 6 commits into from
Aug 5, 2021

Conversation

cognifloyd
Copy link
Member

@cognifloyd cognifloyd commented May 29, 2021

For some reason the ssl module is hitting an infinite recursion error. I thought this might be CircleCI-specific, but I was able to reproduce it locally with v3.6.13. The 3.7.0 version is supposed to be even more robust, so it could also be something about v3.6.13. I have not reproduced the error elsewhere, however. I was able to reproduce the error locally.

The error began appearing after we updated requests from v2.23.0 to v2.25.1 in StackStorm/st2#5215
In requests v2.24.0, it defaulted to using the python ssl module instead of PyOpenSSL due to various SELinux issues. Before that it used PyOpenSSL whenever it was installed. Since it worked with pyopenssl, we just revert to using it for the tests in this repo.
See: psf/requests#5443

For some reason the ssl module is hitting an infinite recursion error.
This might be CircleCI-specific, depending on the openssl lib they used
to compile python. The 3.7.0 version is supposed to be even more robust,
so it could also be something about v3.6.13. I have not reproduced the
error elsewhere, however.

The error began appearing after we updated requests from v2.23.0 to
v2.25.1 in StackStorm/st2#5215
In requests v2.24.0, it defaulted to using the python ssl module instead
of PyOpenSSL due to various SELinux issues. Before that it used
PyOpenSSL whenever it was installed. Since it worked with pyopenssl, we
just revert to using it for the tests in this repo.
See: psf/requests#5443
@cognifloyd cognifloyd changed the title Work around TLS errors on CircleCI Work around TLS errors Jun 13, 2021
@cognifloyd
Copy link
Member Author

I was able to reproduce it locally. In order to fix this, I had to either downgrade to requests 2.23.0 or include the change from this PR.

@cognifloyd
Copy link
Member Author

cognifloyd commented Jun 13, 2021

The recursion errors look like they could be caused by eventlet interacting poorly with the ssl lib.

I tried adding monkey_patch (as below) at the top of the test files without success.

from st2common.util.monkey_patch import monkey_patch
monkey_patch()

It looks like others have had issues on python3.6 + eventlet + ssl as well: eventlet/eventlet#371
In any case, forcing it to use the python implementaiton (which eventlet has presumably already patched) works.

@cognifloyd
Copy link
Member Author

cognifloyd commented Jun 13, 2021

Hmm. If I add monkey_patch to tests/__init__.py that also resolves the issue. So something in nosetests must be importing requests or the ssl lib before monkey patching happens.

So, which fix is preferrable? monkey_patch in tests/__init__.py? or pyopenssl.inject_into_urllib3() to force re-importing ssl in urllib3 after the monkey_patching has already been done?

@blag
Copy link
Contributor

blag commented Jun 13, 2021

Other than the generic reservations about monkeypatching anything to begin with (although I do understand that we have to here because we've painted ourselves into a bit of a corner with our use of eventlet), I don't see a problem with monkeypatching within tests/__init__.py, as long as the production code still actually works in production.

Instead of injecting pyopenssl (which apparently moves the
import to after the eventlet monkey patching), this moves the
monkey patching to __init__. Importing at the top of
the test files was too late--that still resulted in
the infinite recursion bug.
@cognifloyd
Copy link
Member Author

K. I pushed a new commit that switches over to monkey patching in init.py. I agree that monkey patching is gross, but oh well.

@cognifloyd
Copy link
Member Author

Well that didn't work on CircleCI. It might be working locally because of the st2 sources I used (that have some additional eventlet monkey patching added to the sources).

In any case, for production use, the monkey patching happens very early. With nosetest, we don't have quite as much control about when that monkey patching happens, so it's become a whak-a-mole game. I wonder if there's any way to get that monkey patching even earlier... hmm.

@cognifloyd
Copy link
Member Author

Nope. I switched to a fresh st2 checkout. It still passes locally.

@cognifloyd cognifloyd force-pushed the ssl-handshake-errors branch 2 times, most recently from 94ae110 to b3f1f94 Compare June 14, 2021 15:32
@cognifloyd cognifloyd force-pushed the ssl-handshake-errors branch 2 times, most recently from bac2b5d to 1fb1313 Compare June 14, 2021 16:04
@cognifloyd
Copy link
Member Author

I don't understand. Locally, moving the monkey_patch earlier resolved the issue. On CircleCI, even patching nosetests doesn't seem to resolve it. Any ideas?

@cognifloyd cognifloyd force-pushed the ssl-handshake-errors branch 5 times, most recently from af6e92c to cb5df85 Compare June 14, 2021 16:58
@cognifloyd
Copy link
Member Author

No matter where I put the monkey_patch, the infinite recursion error occurs on CircleCI. I was able to reproduce the infinite recursion error locally, but the monkey_patch fixed it for me.

So, the only thing that seems to fix the tests on circle CI is:

from urllib3.contrib import pyopenssl
pyopenssl.inject_into_urllib3()

@cognifloyd
Copy link
Member Author

@blag where do you want the hack for CircleCI? In the circle config like I did in the recent commits?

@blag
Copy link
Contributor

blag commented Jun 26, 2021

Yeah, I think the CircleCI config is probably the best place for that.

@cognifloyd
Copy link
Member Author

K. I cleaned up the debug commits. This should be ready to merge

@cognifloyd
Copy link
Member Author

@blag can we merge this?

Copy link
Contributor

@blag blag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@blag blag merged commit 1fb9751 into StackStorm-Exchange:master Aug 5, 2021
@cognifloyd cognifloyd deleted the ssl-handshake-errors branch August 5, 2021 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants