-
Notifications
You must be signed in to change notification settings - Fork 16
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
Add certifi to CI images #218
Conversation
@jameslamb @raydouglass This is ready for review. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I support these changes. The comments I left are all of the form "... and I think we should go even further", but up to you how much time you want to invest into this.
@@ -124,6 +125,7 @@ case "${LINUX_VER}" in | |||
"rockylinux"*) | |||
yum -y update | |||
yum -y install --setopt=install_weak_deps=False \ | |||
ca-certificates \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the rockylinux case, I think we also need this:
update-ca-trust extract
Based on https://forums.rockylinux.org/t/ssl-cert-location/10555/6 (which I found while investigating this private issue: https://github.com/rapidsai/build-infra/issues/56).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added update-ca-certificates
for Ubuntu and update-ca-trust extract
for Rocky 8. Hope that's right? See 7361c69.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to go ahead with this and try to merge. Let's see if it helps with our nightly CI tonight.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah perfect, let's merge this when you're ready and see if it helps
python -m pip install "rapids-dependency-file-generator==1.*" | ||
python -m pip install \ | ||
certifi \ | ||
'rapids-dependency-file-generator==1.*' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making an updated version of certifi
available in the image by default helps, but wouldn't have totally avoided the need for changes like rapidsai/cugraph#4825.
I'm not sure if it's possible, via config files or environment variables or something, to make urllib
recognize the certifi
certs by default.
https://peps.python.org/pep-0476/#trust-database suggests using SSL_CERT_DIR
/ SSL_CERT_FILE
, but to "point Python at a different certificate database"... I think ideally we'd want the certifi
certs to be considered in addition to whatever else is in the image (e.g. from the system's ca-certificates
package).
And according to https://stackoverflow.com/a/39358282/3986677 you can check where urllib
is looking like this:
python -c "import ssl; print(ssl.get_default_verify_paths())"
For me, in the latest citestwheel
image built from main
, that prints this:
DefaultVerifyPaths(cafile=None, capath='/usr/lib/ssl/certs', openssl_cafile_env='SSL_CERT_FILE', openssl_cafile='/usr/lib/ssl/cert.pem', openssl_capath_env='SSL_CERT_DIR', openssl_capath='/usr/lib/ssl/certs')
/usr/lib/ssl/certs
is full of tons of certificate files... maybe we can just copy/symlink certifi
's out of site-packages and into there?
https://urllib3.readthedocs.io/en/stable/advanced-usage.html#custom-tls-certificates doesn't say anything about customizing the cert bundle via config files or environment variables... just direct modification of your Python code.
I don't know how to test any of these ideas, because the example that was reproducibly failing last week for rapidsai/cugraph#4825 no longer is:
docker run \
--rm \
rapidsai/citestwheel:latest \
python -c 'import urllib.request; urllib.request.urlretrieve("https://data.rapids.ai/cugraph/results/resultsets.tar.gz", "foo.tar.gz")'
So all that to say... I support this change as-is but wanted to point out that this doesn't totally solve the original issue, in case someone else more knowledgeable than me has an idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree. This can't hurt, but I don't know if it fully helps. Maybe the correct answer is that we don't need to manually install certifi
at all in wheels, just ca-certificates
? I think certifi
is used directly (via requests
) in the ci-conda
image.
This PR adds
certifi
to all CI images. See proposal in rapidsai/gha-tools#127.