Skip to content

Conversation

@algchoo
Copy link
Contributor

@algchoo algchoo commented Jul 26, 2023

Changes

Details

The latest version of the MySQL Prometheus exporter works with MySQL 5.7 and MySQL 8.0.34. This new version of the exporter removes the DATA_SOURCE_NAME environment variable and instead we can pass in the --mysqld.username argument and the MYSQLD_EXPORTER_PASSWORD environment variable for authentication, or we can mount a configuration file with the credentials in it and pass the --config.my-cnf=<PATH_TO_MOUNTED_CONFIG_FILE>.

Example configuration file:

[client]
user=root
password=password

I'm not sure which option to go with for the example configuration and wanted to get an opinion, we could change it to look something like

config_mods: |
apiVersion: v1
kind: ConfigMap
metadata:
  name: mysql-exporter-config
data:
  .my-cnf: |
    [client]
    user=root
    password=password
-------
  apiVersion: apps/v1
  kind: StatefulSet
  metadata:
    name: mysql
  spec:
    serviceName: mysql
    selector:
      matchLabels:
  +     app.kubernetes.io/name: mysql
    template:
      metadata:
        labels:
  +       app.kubernetes.io/name: mysql
      spec:
        containers:
  +     - name: exporter
  +       image: prom/mysqld-exporter:v0.15.0
  +       args:
  +         - --config.my-cnf=<PATH_TO_MOUNTED_CONFIGURATION>
  +       ports:
  +       - containerPort: 9104
  +         name: prometheus
  +       volumeMounts:
  +       - mountPath: /home/.my.cnf
  +         subPath: .my.cnf
  +         name: mysql-exporter-config
        - image: mysql:5.7
          name: mysql
          env:
          - name: MYSQL_ROOT_PASSWORD
            value: password
          - name: MYSQL_USER
            value: sbtest
          - name: MYSQL_PASSWORD
            value: password
          - name: MYSQL_DATABASE
            value: sbtest
          ports:
          - containerPort: 3306
            name: mysql

@github-actions github-actions bot requested a review from EvanSimpson July 26, 2023 18:16
@yqlu
Copy link
Collaborator

yqlu commented Jul 27, 2023

Looking at the mysql-exporter README, I think the config map approach is more recommended as a best practice for security:

To avoid putting sensitive information like username and password in the URL, you can have multiple configurations in config.my-cnf file and match it by adding &auth_module=<section> to the request.

On the topic of best practices, I see that we're defining a non-root user for mysql in this toy example:

          - name: MYSQL_USER
            value: sbtest
          - name: MYSQL_PASSWORD
            value: password

So should we use that instead of root for the exporter's auth?

Happy to help test this if you update the yaml!

@algchoo
Copy link
Contributor Author

algchoo commented Jul 27, 2023

Looking at the mysql-exporter README, I think the config map approach is more recommended as a best practice for security:

To avoid putting sensitive information like username and password in the URL, you can have multiple configurations in config.my-cnf file and match it by adding &auth_module=<section> to the request.

On the topic of best practices, I see that we're defining a non-root user for mysql in this toy example:

          - name: MYSQL_USER
            value: sbtest
          - name: MYSQL_PASSWORD
            value: password

So should we use that instead of root for the exporter's auth?

Happy to help test this if you update the yaml!

Sounds good, I will update the documentation to show the example that uses the configuration file. The non-root user that is defined is used while running sysbench to generate load. I attempted to use that user for to authenticate against the exporter and they didn't have the appropriate permissions, so I went back to using the root user.

@algchoo
Copy link
Contributor Author

algchoo commented Jul 27, 2023

@yqlu For this repo the name of the branch that'll have changes is dashboards/currency-test.

+ volumeMounts:
+ - mountPath: /home/my.cnf
+ subPath: my.cnf
+ name: mysql-exporter-config
Copy link
Collaborator

Choose a reason for hiding this comment

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

I tried applying this yaml and got

create Pod mysql-0 in StatefulSet mysql failed error: Pod "mysql-0" is invalid: spec.containers[1].volumeMounts[0].name: Not found: "mysql-exporter-config"

Looks like we need to define a volume that corresponds to the volumeMount?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the yaml to include a volumeMount and I'm wondering, do we want the + symbols with the configmap that is included in the example as well?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think so.

In some cases like the nginx one, you can reasonably expect that anyone setting up nginx on k8s will have a configmap set up, so we just use "+"s to indicate the lines they need to add to the config map.

In this case, if you already had a mysql instance up and running, and then you were adding an exporter, you wouldn't already have a configmap with credentials beforehand, so the volume, volumemount, configmap are all indicated with "+"s.

Does that make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe so, I pushed changes to include the + symbols with the configmap unsure if we need all of them.

multiple_dashboards: false
dashboard_display_name: {{app_name_short}} Prometheus Overview
config_mods: |
apiVersion: apps/v1
Copy link
Collaborator

Choose a reason for hiding this comment

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

As per the other example, we can update https://github.com/GoogleCloudPlatform/prometheus-engine/blob/main/examples/mysql/exporter.yaml.snippet directly (after addressing the other comments here)

But unlike the consul example, this PR doesn't become completely obsolete -- we do still need to update this file because of the change you made in line 70 (additional_install_info).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, just for clarity I wanted to make sure that the line you're mentioning here doesn't need to be changed? We just want to update things in this repo and the other one, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep

@yqlu yqlu merged commit 6e5c330 into GoogleCloudPlatform:master Aug 11, 2023
algchoo added a commit to observIQ/monitoring-dashboard-samples that referenced this pull request Aug 15, 2023
* updated the minimum exporter version, example configuration, and additional install info

* updated the minimum supported version in metadata

* update config example to pass a config file as an argument

* reverted min versions, added volume to the config example that mounts the mysql exporter config

* added + symbols for configmap in example yaml

* removed a few dashes, updated the volume section, added mysql image to container section
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants