Pass SSL properties from Glue Connection to MySQL #664
Conversation
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
kukushking
changed the title
kukushking
force-pushed
the
mysql_ssl_support
branch
from
735ad12 to
35706a3
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
kukushking
force-pushed
the
mysql_ssl_support
branch
from
35706a3 to
cd19540
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
@jaidisido CB failed here because a new |
|
Ideally, we would want to make this change permanent if it will be needed for future tests. So I would add/modify the cloudformation template holding the connections first (you can change it as part of this PR). I would then update the stack in the AWS account we are using for testing |
|
That modification is already there (see diff). All, right, I'll update it in the account then. |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
jaidisido
left a comment
jaidisido
left a comment
There was a problem hiding this comment.
Looking great, thank you. I have left minor comments
awswrangler/mysql.py
Outdated
| read_timeout=read_timeout, | ||
| write_timeout=write_timeout, | ||
| connect_timeout=connect_timeout, | ||
| ssl=attrs.ssl_context, |
There was a problem hiding this comment.
Minor - I would put this line right after the host=attrs.host (line 137) just to keep all attributes arguments together
| host=secret_value["host"], | ||
| port=secret_value["port"], | ||
| database=_dbname, | ||
| ssl_context=None, |
There was a problem hiding this comment.
I guess there is really no way to specify/pass the ca data via secrets manager... Do you think we should clarify the docs in that regard?
There was a problem hiding this comment.
Doesn't hurt to clarify, of course.
cloudformation/databases.yaml
Outdated
| CatalogId: | ||
| Ref: AWS::AccountId | ||
| ConnectionInput: | ||
| Description: Connect to Aurora (MySQL). |
There was a problem hiding this comment.
Minor - "Connect to Aurora (MySQL) SSL enabled."
tests/test_mysql.py
Outdated
| def test_connection_ssl(): | ||
| wr.mysql.connect("aws-data-wrangler-mysql-ssl", connect_timeout=10).close() |
There was a problem hiding this comment.
Do you think we should be not only testing the ssl connection, but also use that connection for some of the mysql test queries? Was wondering if we could adapt the current pytest fixture to take ssl enabled as an argument.
Something along the lines of:
@pytest.fixture(scope="function")
def mysql_con(ssl_enabled: bool = False):
con_str = "aws-data-wrangler-mysql-ssl" if ssl_enabled else "aws-data-wrangler-mysql"
con = wr.mysql.connect(con_str)
yield con
con.close()
There was a problem hiding this comment.
I feel like it's a bit overkill to test all queries but deffo agree that there should be at least one with SSL enabled. In addition, I was thinking to run ALTER ... REQUIRE SSL; to replicate the same test that was mentioned in the original issue.
There was a problem hiding this comment.
Agreed, it's an overkill. One or two tests should be enough!
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
2 participants
Issue #554:
Description of changes:
Pass SSL properties from Glue Connection to PyMySQL. ~~Depends on this PyMySQL PR merged & released first. ~~
UPD: In conversation with PyMySQL contributors it seems it's possible to pass
SSLContextto PyMySQL so extra argument is not required anymore.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.