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

Add optional ssl_cadata arg #980

Closed
wants to merge 1 commit into from

Conversation

kukushking
Copy link

@kukushking kukushking commented Apr 19, 2021

Hi there,

Starting with Python 3.4 ssl module supports providing CA certs via cadata. I suppose it's useful If you'd like to dynamically fetch the certificate from somewhere and avoid having to create a temporary file.

This is exactly the use case in aws/aws-sdk-pandas#664 - we would like to fetch MySQL connection details from AWS Glue Connections, including the CA certificate stored in AWS S3, if configured, and pass them over to PyMySQL.

@kukushking
Copy link
Author

@grooverdan Hi Dan - could you have a look, please?

@grooverdan
Copy link
Contributor

@kukushking, recommend a little bit more patience. Some maintainers are exceptionally busy people.

On the code itself:

  • Recommend squashing the commits so it look readable in the history
  • Improve test cases to actually cover the use SSL functionality introduced. While the existing mock tests aren't great, taking the time to introduce a improved test case for your case, and the default path based SSL would be appreciated.

I assume your pinging of me and @lecram is based on previous contributions. As you can see, this was a considerable time ago and we're probably not the best people to review.

From aws/aws-sdk-pandas#664, it looks like you actually have a use case for this or potentially something you are working towards. Recommend stating the objective, because at the moment it looks like you are adding this just because you can. Maintainers, particularly busy ones, like to see a tangible case for reviewing and merging code.

@kukushking
Copy link
Author

kukushking commented May 5, 2021

@grooverdan Thanks for your feedback and apologies for any annoyance! Let me tag some of more recent contributors. @methane @darxriggs

  • Updated the PR description with the use case.
  • Squashed the commits (it is also possible to do during merge).
  • There is an existing path-based test case and I added cadata-based one as well. There are no SSL test cases for MariaDB, which is the reason for minor coverage decrease, however, default Docker image for MariaDB isn't configured with SSL so so far I just had to go along with it. I'll see what I can do to improve coverage though.

@methane
Copy link
Member

methane commented May 6, 2021

I'm very conservative about adding more options.

Is the ssl_cadata a well-known option name for MySQL clients? Does mysql CLI have the option?
As far as I checked this list, I can not find ssl-cadata.
https://dev.mysql.com/doc/c-api/8.0/en/mysql-options.html

@methane
Copy link
Member

methane commented May 6, 2021

FYI, you can pass SSLContext object to ssl=ctx option. So you don't need to add this option nor use temporary file.
You have full control of SSLContext already.

@kukushking
Copy link
Author

kukushking commented May 6, 2021

Indeed, passing SSLContext worked just fine. Seems I overlooked that option. Thanks for all help!

@kukushking kukushking closed this May 6, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 7, 2022
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.

None yet

3 participants