-
Notifications
You must be signed in to change notification settings - Fork 3
211 support mtls #212
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
211 support mtls #212
Conversation
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.
Pull Request Overview
This PR adds support for encrypted (m)TLS connections for MySQL by allowing users to specify use_ssl and ssl_params in the database credentials file, updates the connector logic to consume these parameters, adjusts relevant type hints, and refreshes tests and documentation accordingly.
- Extract SSL parameters (
ca,cert,key) whenuse_sslis enabled and pass them into the MySQL connector - Change
table_nameanddatabase_nametoOptional[str]inPyExperimenterconstructor - Update docs, example credentials YAML, and disable SSH tunneling in the MySQL integration test
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/test_codecarbon/test_integration_mysql.py | Switch use_ssh_tunnel to False in the MySQL integration test |
| py_experimenter/experimenter.py | Tighten type hints for table_name and database_name |
| py_experimenter/database_connector_mysql.py | Add parsing of SSL/mTLS params and optional typing |
| docs/source/usage/database_credential_file.rst | Document use_ssl and ssl_params in credentials file |
| config/example_database_credentials.yml | Provide example use_ssl and ssl_params entries |
| .cursorignore | Ignore credential and certificate directories |
Comments suppressed due to low confidence (3)
config/example_database_credentials.yml:6
- The
use_sslandssl_paramsentries are currently siblings ofStandardbut should be nested underStandard:. Adjust indentation so they live insideStandard:.
Standard:
docs/source/usage/database_credential_file.rst:21
- [nitpick] This sentence uses both "additionally" and "also"—consider removing one for clarity, e.g., "We also support encrypted (m)TLS connections."
We additionally also support utilizing encrypted connections with (m)tls. To that end, the following parameters can be added to the ``Standard`` section of the database credential file
py_experimenter/database_connector_mysql.py:125
- There are no existing tests verifying that
use_sslandssl_paramsare correctly applied. Consider adding an integration test for SSL/mTLS connections to ensure the connector uses these parameters as intended.
if "use_ssl" in connection_configuration["Standard"]:
mwever
left a comment
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.
LG2M
Description
Motivation and Context
Changes
Type Of Changes
How has This Been Tested?
Does this Close/Impact Existing Issues?
What is Missing?
Checklist
developbranch.mysqlas provider.CHANGELOG.rst.