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

Adds check_hostname parameter To MySQL #11713

Merged
merged 8 commits into from
Apr 4, 2022

Conversation

mawais54013
Copy link
Contributor

@mawais54013 mawais54013 commented Mar 22, 2022

What does this PR do?

Adds check_hostname parameter for PyMySQL Connection

Motivation

We have a customer who is trying to get database monitoring working on a Cloud SQL cluster, but due to security reasons, they have to connect directly to individual IPs within the cluster, rather than the external DNS address. This is a problem because their certificate is attached to the external DNS instead. PyMySQL (the lib we use) has a strict relationship between the certificate and the hostname, but there’s an option to disable it for situations such as this. We should update the SSL config to include this parameter.

Additional Notes

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have changelog/ and integration/ labels attached

@mawais54013 mawais54013 requested review from a team as code owners March 22, 2022 17:43
mysql/assets/configuration/spec.yaml Outdated Show resolved Hide resolved
mysql/assets/configuration/spec.yaml Outdated Show resolved Hide resolved
@github-actions
Copy link

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

@codecov
Copy link

codecov bot commented Mar 22, 2022

Codecov Report

Merging #11713 (97dbb4b) into master (e7256a2) will increase coverage by 0.00%.
The diff coverage is n/a.

Flag Coverage Δ
mysql 87.56% <ø> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@github-actions
Copy link

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

@github-actions
Copy link

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

1 similar comment
@github-actions
Copy link

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

@@ -98,6 +98,7 @@ class Config:

ca: Optional[str]
cert: Optional[str]
check_hostname: Optional[bool]
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this actually used in the code?

Copy link
Contributor

Choose a reason for hiding this comment

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

Out of scope for this PR, but we might want to think about explicitly declaring these options in the code rather than passing the raw values directly to a python dict:

ssl = dict(self._config.ssl) if self._config.ssl else None

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Code wise I am adding a variable in the conf yaml file while also keeping it as an optional setting with default of true to allow DBM on MySQL to happen. In some cases, due to which db type they are using for MySQL, the permissions for the connection can strict as I believe the hostname must be the same as the one set in certificate but setting this to false can prevent this checking from happening and allow DBM to monitor their db. But user do have the option to change this value in the conf yaml file.

The check_hostname is used in the self._config.ssl and then assigned to ssl as a dictionary

@steveny91 steveny91 merged commit 92cd589 into master Apr 4, 2022
@steveny91 steveny91 deleted the muhammad/add_checkhostname_to_mysql branch April 4, 2022 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants