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

Import features from ssl module with more granularity #2052

Merged
merged 1 commit into from Nov 12, 2020

Conversation

sethmlarson
Copy link
Member

@sethmlarson sethmlarson commented Nov 12, 2020

Very subtle bug here that was caused by isort reordering the imports within try-except. Here's the diff of urllib3.util.ssl_ between 1.25.11 and 1.26.1 that caused the bug:

 try:  # Test for SSL features
     import ssl
-    from ssl import wrap_socket, CERT_REQUIRED
     from ssl import HAS_SNI  # Has SNI?
+    from ssl import CERT_REQUIRED, wrap_socket
+
+    from .ssltransport import SSLTransport
 except ImportError:
     pass

Notice how HAS_SNI is now being imported before wrap_socket and CERT_REQUIRED and HAS_SNI is a Python 2.7.9 feature. This meant that imports on Python 2.7.8 and earlier weren't getting wrap_socket imported in time before HAS_SNI was causing an ImportError.

Verified this locally with the python:2.7.8 Docker container, we currently don't have CI for this Python version.

Closes #2051

@codecov
Copy link

codecov bot commented Nov 12, 2020

Codecov Report

Merging #2052 (260a7bd) into master (6611153) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #2052   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           25        25           
  Lines         2296      2298    +2     
=========================================
+ Hits          2296      2298    +2     
Impacted Files Coverage Δ
src/urllib3/util/ssl_.py 100.00% <100.00%> (ø)

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 6611153...260a7bd. Read the comment docs.

@sethmlarson sethmlarson added this to the v1.26 milestone Nov 12, 2020
Copy link
Member

@nateprewitt nateprewitt left a comment

Choose a reason for hiding this comment

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

:shipit:

@yaodingyd
Copy link

appreciate the quick turnaround!

This was referenced Mar 12, 2021
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.

v.1.26+ doesn't support SSL in python 2.7.6
3 participants