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

greendns: Patch ssl #684

Merged
merged 1 commit into from Mar 3, 2021
Merged

Conversation

tipabu
Copy link
Contributor

@tipabu tipabu commented Feb 4, 2021

Newer (2.0+) dnspython imports ssl (and requests, which would have eventually imported ssl), so greendns needs a monkey-patched ssl, too. This should prevent a RecursionError with SSLContext.

Note that this does not bring full dnspython>=2.0 support, but our version pin hasn't stopped people from trying to run with newer dnspython anyway. May as well make it hurt a little less.

Fixes #677, but see also #619.

Newer (2.0+) dnspython imports ssl (and requests, which would have
eventually imported ssl), so greendns needs a monkey-patched ssl, too.
This should prevent a RecursionError with SSLContext.

Note that this *does not* bring full dnspython>=2.0 support, but our
version pin hasn't stopped people from trying to run with newer
dnspython anyway. May as well make it hurt a little less.

Fixes eventlet#677, but see also eventlet#619.
@codecov
Copy link

codecov bot commented Feb 4, 2021

Codecov Report

Merging #684 (04f9afb) into master (a21539d) will decrease coverage by 0%.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #684   +/-   ##
======================================
- Coverage      44%     44%   -1%     
======================================
  Files          87      87           
  Lines       11913   11914    +1     
  Branches     1780    1780           
======================================
- Hits         5290    5289    -1     
- Misses       6220    6222    +2     
  Partials      403     403           
Flag Coverage Δ
ipv6 16% <100%> (+<1%) ⬆️
py27epolls 56% <100%> (-1%) ⬇️
py27poll 56% <100%> (-1%) ⬇️
py27selects 55% <100%> (-1%) ⬇️
py35epolls 49% <100%> (-1%) ⬇️
py35poll 49% <100%> (-1%) ⬇️
py35selects 49% <100%> (-1%) ⬇️
py36epolls 49% <100%> (-1%) ⬇️
py36poll 49% <100%> (-1%) ⬇️
py36selects 49% <100%> (-1%) ⬇️
py37epolls 49% <100%> (-1%) ⬇️
py37poll 49% <100%> (-1%) ⬇️
py37selects 49% <100%> (-1%) ⬇️
py38epolls 40% <100%> (+<1%) ⬆️
py38openssl 39% <100%> (+<1%) ⬆️
py38poll 40% <100%> (-1%) ⬇️
py38selects 40% <100%> (-1%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
eventlet/support/greendns.py 63% <100%> (-1%) ⬇️
eventlet/queue.py 77% <0%> (-1%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a21539d...04f9afb. Read the comment docs.

@temoto temoto requested a review from jstasiak February 5, 2021 08:06
Copy link
Member

@temoto temoto left a comment

Choose a reason for hiding this comment

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

LGTM

@astrolox
Copy link

astrolox commented Feb 6, 2021

This is useful to me. Thanks!

benthorner pushed a commit to alphagov/notifications-admin that referenced this pull request Feb 16, 2021
This downgrades various packages so they are mutually compatible
and "pip install -r requirements.txt" succeeds.

Currently we have a situation where we're not running tests against
new versions of dependencies, as requirements_for_test.txt is not
being kept in-sync with requirements.txt by pyup. Deploys are only
working because Concourse silently ignores version issues.

From a deployment log:

awscli 1.18.211 has requirement PyYAML<5.4,>=3.10; python_version != "3.4", but you'll have pyyaml 5.4 which is incompatible.

This downgrades to pyyaml 5.3.1, despite it having a security issue,
in order to fix the build for the time being. This also downgrades
dnspython, due to a suspected issue with eventlet [1], which caused
the admin app to start failing with errors like this:

  File "/home/vcap/deps/0/python/lib/python3.6/site-packages/urllib3/connectionpool.py", line 1010, in _validate_conn
    conn.connect()
  File "/home/vcap/deps/0/python/lib/python3.6/site-packages/urllib3/connection.py", line 394, in connect
    cert_reqs=resolve_cert_reqs(self.cert_reqs),
  File "/home/vcap/deps/0/python/lib/python3.6/site-packages/urllib3/util/ssl_.py", line 303, in create_urllib3_context
    context.options |= options
  File "/home/vcap/deps/0/python/lib/python3.6/ssl.py", line 465, in options
    super(SSLContext, SSLContext).options.__set__(self, value)
  File "/home/vcap/deps/0/python/lib/python3.6/ssl.py", line 465, in options
    super(SSLContext, SSLContext).options.__set__(self, value)
  File "/home/vcap/deps/0/python/lib/python3.6/ssl.py", line 465, in options
    super(SSLContext, SSLContext).options.__set__(self, value)
  [Previous line repeated 280 more times]
  RecursionError: maximum recursion depth exceeded while calling a Python object

[1]: eventlet/eventlet#684
benthorner pushed a commit to alphagov/notifications-admin that referenced this pull request Feb 16, 2021
This downgrades various packages so they are mutually compatible
and "pip install -r requirements.txt" succeeds.

This downgrades to pyyaml 5.3.1, despite it having a security issue,
in order to fix the build for the time being. This also downgrades
dnspython, due to a suspected issue with eventlet [1], which caused
the admin app to start failing with errors like this:

  File "/home/vcap/deps/0/python/lib/python3.6/site-packages/urllib3/connectionpool.py", line 1010, in _validate_conn
    conn.connect()
  File "/home/vcap/deps/0/python/lib/python3.6/site-packages/urllib3/connection.py", line 394, in connect
    cert_reqs=resolve_cert_reqs(self.cert_reqs),
  File "/home/vcap/deps/0/python/lib/python3.6/site-packages/urllib3/util/ssl_.py", line 303, in create_urllib3_context
    context.options |= options
  File "/home/vcap/deps/0/python/lib/python3.6/ssl.py", line 465, in options
    super(SSLContext, SSLContext).options.__set__(self, value)
  File "/home/vcap/deps/0/python/lib/python3.6/ssl.py", line 465, in options
    super(SSLContext, SSLContext).options.__set__(self, value)
  File "/home/vcap/deps/0/python/lib/python3.6/ssl.py", line 465, in options
    super(SSLContext, SSLContext).options.__set__(self, value)
  [Previous line repeated 280 more times]
  RecursionError: maximum recursion depth exceeded while calling a Python object

[1]: eventlet/eventlet#684
benthorner pushed a commit to alphagov/notifications-admin that referenced this pull request Feb 16, 2021
This downgrades various packages so they are mutually compatible
and "pip install -r requirements.txt" succeeds.

This downgrades to pyyaml 5.3.1, despite it having a security issue,
in order to fix the build for the time being. This also downgrades
dnspython, due to a suspected issue with eventlet [1], which caused
the admin app to start failing with errors like this:

  File "/home/vcap/deps/0/python/lib/python3.6/site-packages/urllib3/connectionpool.py", line 1010, in _validate_conn
    conn.connect()
  File "/home/vcap/deps/0/python/lib/python3.6/site-packages/urllib3/connection.py", line 394, in connect
    cert_reqs=resolve_cert_reqs(self.cert_reqs),
  File "/home/vcap/deps/0/python/lib/python3.6/site-packages/urllib3/util/ssl_.py", line 303, in create_urllib3_context
    context.options |= options
  File "/home/vcap/deps/0/python/lib/python3.6/ssl.py", line 465, in options
    super(SSLContext, SSLContext).options.__set__(self, value)
  File "/home/vcap/deps/0/python/lib/python3.6/ssl.py", line 465, in options
    super(SSLContext, SSLContext).options.__set__(self, value)
  File "/home/vcap/deps/0/python/lib/python3.6/ssl.py", line 465, in options
    super(SSLContext, SSLContext).options.__set__(self, value)
  [Previous line repeated 280 more times]
  RecursionError: maximum recursion depth exceeded while calling a Python object

[1]: eventlet/eventlet#684
dvjones89 pushed a commit to bitzesty/notifications-admin that referenced this pull request Feb 17, 2021
This downgrades various packages so they are mutually compatible
and "pip install -r requirements.txt" succeeds.

This downgrades to pyyaml 5.3.1, despite it having a security issue,
in order to fix the build for the time being. This also downgrades
dnspython, due to a suspected issue with eventlet [1], which caused
the admin app to start failing with errors like this:

  File "/home/vcap/deps/0/python/lib/python3.6/site-packages/urllib3/connectionpool.py", line 1010, in _validate_conn
    conn.connect()
  File "/home/vcap/deps/0/python/lib/python3.6/site-packages/urllib3/connection.py", line 394, in connect
    cert_reqs=resolve_cert_reqs(self.cert_reqs),
  File "/home/vcap/deps/0/python/lib/python3.6/site-packages/urllib3/util/ssl_.py", line 303, in create_urllib3_context
    context.options |= options
  File "/home/vcap/deps/0/python/lib/python3.6/ssl.py", line 465, in options
    super(SSLContext, SSLContext).options.__set__(self, value)
  File "/home/vcap/deps/0/python/lib/python3.6/ssl.py", line 465, in options
    super(SSLContext, SSLContext).options.__set__(self, value)
  File "/home/vcap/deps/0/python/lib/python3.6/ssl.py", line 465, in options
    super(SSLContext, SSLContext).options.__set__(self, value)
  [Previous line repeated 280 more times]
  RecursionError: maximum recursion depth exceeded while calling a Python object

[1]: eventlet/eventlet#684
@temoto temoto merged commit 79cf4ea into eventlet:master Mar 3, 2021
@tipabu tipabu deleted the dnspython-ssl-recursion branch June 9, 2021 18:27
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.

0.29.1: RecursionError: maximum recursion depth exceeded while calling a Python object
3 participants