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

fix: Don't log config deployments #73

Merged
merged 1 commit into from
Mar 17, 2023
Merged

fix: Don't log config deployments #73

merged 1 commit into from
Mar 17, 2023

Conversation

SuperQ
Copy link
Contributor

@SuperQ SuperQ commented Mar 16, 2023

Avoid logging on some config files may contain secrets.

Fixes: #72

@SuperQ SuperQ requested a review from gardar March 16, 2023 07:59
Copy link
Contributor

@laurent-indermuehle laurent-indermuehle left a comment

Choose a reason for hiding this comment

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

Because the variable is used in a template, arguments_spec won't work?

Because to me, it would be better to have the no_log on the variable instead of the whole task.
You wouldn't need to check the __testing vars and in case of issue on production the task will output the error instead of the "no_log" warning.

roles/alertmanager/tasks/configure.yml Outdated Show resolved Hide resolved
Copy link
Member

@gardar gardar left a comment

Choose a reason for hiding this comment

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

Rather than using our own variables I would prefer to use variables that are already present, some of these:

  • no_log: "{{ molecule_no_log }}"
  • no_log: "{{ (lookup('env', 'MOLECULE_DEBUG')) }}"
  • no_log: "{{ (lookup('env', 'CI')) }}" (comes from github ci)
  • etc.

@gardar
Copy link
Member

gardar commented Mar 16, 2023

Because the variable is used in a template, arguments_spec won't work?

Because to me, it would be better to have the no_log on the variable instead of the whole task. You wouldn't need to check the __testing vars and in case of issue on production the task will output the error instead of the "no_log" warning.

Ansible doesn't support no_log for individual parameters of tasks, so you either hide the logging of the whole task or not at all.
Ansible modules however can define individual parameters as no log, but that's unfortunately no help here.

@SuperQ SuperQ force-pushed the superq/mysql_no_log branch from 521cfe4 to ea87c12 Compare March 17, 2023 09:32
@github-actions github-actions bot added bugfix and removed bugfix labels Mar 17, 2023
@SuperQ SuperQ requested review from laurent-indermuehle and gardar and removed request for laurent-indermuehle March 17, 2023 09:33
@SuperQ
Copy link
Contributor Author

SuperQ commented Mar 17, 2023

Verified with a local verbose molecule test:

TASK [prometheus.prometheus.alertmanager : Copy alertmanager config] ***********
changed: [debian-11] => {
    "censored": "the output has been hidden due to the fact that 'no_log: true' was specified for this result",
    "changed": true
}

roles/mysqld_exporter/tasks/configure.yml Outdated Show resolved Hide resolved
Avoid logging on some config files may contain secrets.

Fixes: #72

Signed-off-by: prombot <prometheus-team@googlegroups.com>
@SuperQ SuperQ force-pushed the superq/mysql_no_log branch from ea87c12 to e39934b Compare March 17, 2023 10:34
@SuperQ SuperQ requested review from gardar and laurent-indermuehle and removed request for laurent-indermuehle March 17, 2023 10:34
@github-actions github-actions bot added bugfix and removed bugfix labels Mar 17, 2023
@SuperQ SuperQ merged commit 81fde69 into main Mar 17, 2023
@SuperQ SuperQ deleted the superq/mysql_no_log branch March 17, 2023 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Role mysqld_exporter is leaking the exporter mysql password
4 participants