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

Allow proxysql checks to specify stats database name #6835

Merged
merged 3 commits into from
Jun 23, 2020

Conversation

tabacco
Copy link
Contributor

@tabacco tabacco commented Jun 5, 2020

What does this PR do?

Allow the instance config for the proxysql check to set an alternate database name to run stats queries against.

Motivation

The previosuly-hardcoded stats database is correct if you're connected as an admin user, but doesn't exist if you're connected as a stats-only user.

From a security point of view, it's probably better for datadog to connect as a stats user, since those aren't granted access to sensitive data. The existing checks should all work as a stats user, but for those users the 'stats' database is called 'main' instead.

Additional Notes

The default config for database_name is the same as the previously hardcoded name in the queries, so this should be backwards compatible.

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

The previosuly-hardcoded stats database is correct if you're connected as an admin user, but doesn't exist if you're connected as a stats-only user.

From a security point of view, it's probably better for datadog to connect as a stats user, since those aren't granted access to sensitive data. The existing checks should all work as a stats user, but for those users the 'stats' database is called 'main' instead.

This change drops the hardcoded database name from queries and specifies a default database at connection time instead, with the default set to 'stats'. This should be backwards-compatible with the old behavior.
@tabacco tabacco requested review from a team as code owners June 5, 2020 19:55
Copy link
Contributor

@ruthnaebeck ruthnaebeck left a comment

Choose a reason for hiding this comment

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

Docs review

proxysql/datadog_checks/proxysql/data/conf.yaml.example Outdated Show resolved Hide resolved
proxysql/datadog_checks/proxysql/data/conf.yaml.example Outdated Show resolved Hide resolved
proxysql/datadog_checks/proxysql/data/conf.yaml.example Outdated Show resolved Hide resolved
@tabacco tabacco requested a review from a team as a code owner June 5, 2020 22:11
@codecov
Copy link

codecov bot commented Jun 5, 2020

@tabacco tabacco requested a review from ruthnaebeck June 5, 2020 22:20
@tabacco
Copy link
Contributor Author

tabacco commented Jun 11, 2020

By the way, I don't appear to have permission to add labels. I'm guessing that's what the "PR Labels / apply" action that failed was supposed to do - if anyone has any thoughts on why it's failing, I'd appreciate it.

Copy link
Contributor

@ruthnaebeck ruthnaebeck left a comment

Choose a reason for hiding this comment

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

👍 for docs

@hithwen hithwen merged commit 71fa47a into DataDog:master Jun 23, 2020
@tabacco tabacco deleted the support-proxysql-stats-user branch June 23, 2020 15:13
@AlexandreYang
Copy link
Member

Hi @tabacco ,

It seems passing database to pymysql.connect() has no effect. Removing stats. prefix from queries seems enough.

It's possibly due to the fact that stats users (listed in stats_credentials) are limited (defaulted) to stats database.

A user that connects to Admin with admin-stats_credentials credentials can only access this schema.

Any though on this ?


Note: somehow, queries like SELECT * FROM stats_mysql_global also work for admin users (proxy) even though the "selected database" is "admin".

root@4ba27ac178ac:/# mysql -u proxy -pproxy -h proxysql -P6032 --prompt='Admin> '
Admin> select database();
+------------+
| DATABASE() |
+------------+
| admin      |
+------------+
1 row in set (0.00 sec)

Admin> SELECT * FROM stats_mysql_query_rules;
+---------+------+
| rule_id | hits |
+---------+------+
| 1       | 0    |
+---------+------+
1 row in set (0.01 sec)

@tabacco
Copy link
Contributor Author

tabacco commented Jul 2, 2020

Well that's interesting. It didn't occur to me to not try specifying the db name. I wonder if that's a byproduct of the fact that proxysql's admin database is actually sqlite.

FlorianVeaux pushed a commit that referenced this pull request Jul 3, 2020
* Revert "Allow proxysql checks to specify stats database name (#6835)"

This reverts commit 71fa47a.

* Keep removal of stats. prefix and tests
FlorianVeaux pushed a commit that referenced this pull request Jul 3, 2020
* Revert "Allow proxysql checks to specify stats database name (#6835)"

This reverts commit 71fa47a.

* Keep removal of stats. prefix and tests
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.

None yet

4 participants