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

SNOW-926289 updating vendored libraries #1793

Merged
merged 6 commits into from
Nov 7, 2023

Conversation

sfc-gh-mkeller
Copy link
Collaborator

Please answer these questions before submitting your pull requests. Thanks!

  1. What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.

    Fixes SNOW-926289

  2. Fill out the following pre-review checklist:

    • I am adding a new automated test(s) to verify correctness of my new code
    • I am adding new logging messages
    • I am adding a new telemetry message
    • I am modifying authorization mechanisms
    • I am adding new credentials
    • I am modifying OCSP code
    • I am adding a new dependency
  3. Please describe how your code solves the related issue.

    Bumping the vendored requests and urllib3 libraries to the newest versions (ignoring v2 releases of urllib3).
    I also remove the transient urllib3 dependency of the vendored` requests (discussed in SNOW-926289: Why is urllib3 both vendored and marked as a dependency? #1743 )

@sfc-gh-mkeller sfc-gh-mkeller self-assigned this Nov 4, 2023
@sfc-gh-mkeller sfc-gh-mkeller changed the title SNOW-926289 updating vendored requests to 2.31.0 SNOW-926289 updating vendored libraries Nov 4, 2023
@sfc-gh-mkeller
Copy link
Collaborator Author

E ImportError: urllib3 v2.0 only supports OpenSSL 1.1.1+, currently the 'ssl' module is compiled with 'OpenSSL 1.0.2k-fips 26 Jan 2017'. See: urllib3/urllib3#2168

We might need to still pin urllib3<2.0.0, not what I was hoping for...

@github-actions github-actions bot locked and limited conversation to collaborators Nov 6, 2023
@sfc-gh-mkeller sfc-gh-mkeller reopened this Nov 6, 2023
@snowflakedb snowflakedb unlocked this conversation Nov 6, 2023
@timostrunk
Copy link
Contributor

@sfc-gh-mkeller Please explain, because I just closed our incident with your intention to drop the urllib pin. As you mention urllib is a transient dependency of snowflake-connector-python. I understand that requests is a direct dependency for the OCSP code and cannot be removed (but unpinned).

Why are you therefore pinning urllib < 2? Is it only for asserting that a normal pip install snowflake-connector-python will not run into problems on Centos 7? Why don't you let the users pin urllib in their environment by themselves? The urllib documentation even mentions pinning to urllib < 2 in these cases.

@pquentin Could you please comment on the best practices here?

@pquentin
Copy link

pquentin commented Nov 7, 2023

The best practice is definitely to let users pin in their environments. https://hynek.me/articles/semver-will-not-save-you/ covers that and mentions urllib3<2 even if it was published before we released 2.0.

That said, if you have many customers that rely on CentOS 7 or Amazon Linux 2 you can do like botocore and only allow urllib3 2.0 on Python 3.10 and above, as those versions of Python do not even compile without OpenSSL 1.1.1+, per https://peps.python.org/pep-0644/.

@sfc-gh-mkeller
Copy link
Collaborator Author

@timostrunk It's because of our FIPS mode. If we allow urllib3 2.0.0+ then that won't work with openssl 1.0.2-fips. See this test job.
@pquentin brings up a good point though!

@sfc-gh-stan sfc-gh-stan merged commit 4f46237 into main Nov 7, 2023
78 of 81 checks passed
@sfc-gh-stan sfc-gh-stan deleted the mkeller/SNOW-926289-update_vendored_libraries branch November 7, 2023 21:52
@github-actions github-actions bot locked and limited conversation to collaborators Nov 7, 2023
@sfc-gh-stan
Copy link
Contributor

Sorry folks @timostrunk @timostrunk @sfc-gh-mkeller , I needed to merge this PR ASAP to unblock a requested security fix release. We will continue to work on removing the pin for #1743, please feel free to continue the conversation there.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants