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.

Sign up for GitHub

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

Jump to bottom

Create SSLContexts in the main thread. #2356

Merged
merged 3 commits into from
Feb 7, 2024
Merged

Create SSLContexts in the main thread. #2356

merged 3 commits into from
Feb 7, 2024

Conversation

jsirois
Copy link
Member

@jsirois jsirois commented Feb 7, 2024

This solves #2355 without yet understanding why that issue exists.

Fixes #2355

@jsirois
Copy link
Member Author

jsirois commented Feb 7, 2024

With just the 1st commit (the failing test / repro):

$ tox -epy311-integration -- --devpi -vvsk test_issue_2355 -n0
...
============================================================================================== test session starts ===============================================================================================
platform linux -- Python 3.11.7, pytest-7.4.0, pluggy-1.4.0 -- /home/jsirois/dev/pantsbuild/jsirois-pex/.tox/py311-integration/bin/python
cachedir: .tox/py311-integration/.pytest_cache
rootdir: /home/jsirois/dev/pantsbuild/jsirois-pex
plugins: xdist-1.34.0, forked-1.6.0
collected 671 items / 670 deselected / 1 selected

[+] Building 0.1s (6/6) FINISHED                                                                                                                                                                   docker:default
 => [internal] load .dockerignore                                                                                                                                                                            0.0s
 => => transferring context: 2B                                                                                                                                                                              0.0s
 => [internal] load build definition from Dockerfile                                                                                                                                                         0.0s
 => => transferring dockerfile: 321B                                                                                                                                                                         0.0s
 => [internal] load metadata for docker.io/library/fedora:37                                                                                                                                                 0.0s
 => [1/2] FROM docker.io/library/fedora:37                                                                                                                                                                   0.0s
 => CACHED [2/2] RUN curl --fail -sSL -O https://github.com/indygreg/python-build-standalone/releases/download/20240107/cpython-3.9.18+20240107-x86_64-unknown-linux-gnu-install_only.tar.gz && curl --fail  0.0s
 => exporting to image                                                                                                                                                                                       0.0s
 => => exporting layers                                                                                                                                                                                      0.0s
 => => writing image sha256:316fa459f8410db8e347fe50ab51147145d003dfe59424ca213e6cc8eb5a0f84                                                                                                                 0.0s
 => => naming to docker.io/library/test_issue_2355                                                                                                                                                           0.0s
Failed to spawn a job for /usr/bin/python3.11: unknown error (_ssl.c:3161)
FAILED
...

jsirois
jsirois commented Feb 7, 2024
View reviewed changes
pex/resolve/resolver_options.py
retries=options.retries,
timeout=options.timeout,
proxy=options.proxy,
cert=options.cert,
client_cert=options.client_cert,
)
initialize_ssl_context(network_configuration=network_configuration)
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not super happy with this back door way of threading in an SSLContext from the main thread, but there are alot of paths / layers as it stands right now and this is solid, since the configuration was centralized a good while ago and it unbreaks users now leaving room for improvement (explicit threading of an SSLContext from main on down) later.

@jsirois jsirois merged commit 79a4d86 into pex-tool:main Feb 7, 2024
26 checks passed
@jsirois jsirois deleted the issue/2355 branch February 7, 2024 14:51
jsirois
jsirois commented Feb 9, 2024
View reviewed changes
tests/integration/test_issue_2355.py
"docker",
"run",
"--rm",
"-v" "{pex_project_dir}:/code".format(pex_project_dir=pex_project_dir),
Copy link
Member Author

Choose a reason for hiding this comment

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

Got away with weirdness here since -vfoo:bar is the same as -v foo:bar. I'll fix this in a follow-up where I add links to pantsbuild/pants#20467 (comment) or similar that explain how this paper-over works to solve issues with RedHat bespoke OpenSSL config options.

Copy link
Member Author

Choose a reason for hiding this comment

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

Both fixed in #2358.

jsirois added a commit to jsirois/pex that referenced this pull request Feb 9, 2024
@jsirois
Explain why pex-tool#2356 "works".
b88855f 
The explanation was hard won and a bit embarrasing in outcome. The
original behavior was correct afaict and PBS use on a RedHat system with
custom RedHat OpenSSL config keys should fail, obviously with a much
better error message, and prompt OpenSSL configuration evalutaion on the
machine. That said, the behavior is released now and Pex stands behind
it.

It may make sense to add a `--strict-ssl` option or something similar to
restore the old behavior and let the (confusing) error bubble, perhaps
with a pointer to what may be wrong.

This should also serve to close
indygreg/python-build-standalone#207 or at
least give Gregory enough information to decide what to do over in PBS.
jsirois added a commit that referenced this pull request Feb 10, 2024
@jsirois
Explain why #2356 "works". (#2358)
e70d5d6 
The explanation was hard won and a bit embarrasing in outcome. The
original behavior was correct afaict and PBS use on a RedHat system with
custom RedHat OpenSSL config keys should fail, obviously with a much
better error message, and prompt OpenSSL configuration evaluation on the
machine. That said, the behavior is released now and Pex stands behind
it.

It may make sense to add a `--strict-ssl` option or something similar to
restore the old behavior and let the (confusing) error bubble, perhaps
with a pointer to what may be wrong.

This should also serve to close
indygreg/python-build-standalone#207 or at
least give Gregory enough information to decide what to do over in PBS.

Closes the loose ends in #2355.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benjyw benjyw benjyw approved these changes

@kaos kaos kaos approved these changes

@cburroughs cburroughs Awaiting requested review from cburroughs

@huonw huonw Awaiting requested review from huonw

@tgolsson tgolsson Awaiting requested review from tgolsson

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Generating lockfiles fails with: unknown error (_ssl.c:3161)
3 participants
@jsirois @kaos @benjyw