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

Do not log grafana.ini contents when setting facts #325

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

root-expert
Copy link

Fixes #324

@CLAassistant
Copy link

CLAassistant commented Feb 13, 2025

CLA assistant check
All committers have signed the CLA.

@intermittentnrg
Copy link
Contributor

no_log: true per FAQ sounds reasonable. First time I read about it. https://docs.ansible.com/ansible/latest/reference_appendices/faq.html#keep-secret-data.

Not sure about "{{ 'false' if lookup('env', 'CI') else 'true' }}" tho? Up to maintainers I guess.

Also there was just report about error from set_fact when grafana_ini is not set. Maybe just need to ansible.builtin.assert that grafana_ini is set if this silences set_fact errors.

@gardar
Copy link
Collaborator

gardar commented Feb 14, 2025

no_log: true per FAQ sounds reasonable. First time I read about it. https://docs.ansible.com/ansible/latest/reference_appendices/faq.html#keep-secret-data.

Not sure about "{{ 'false' if lookup('env', 'CI') else 'true' }}" tho? Up to maintainers I guess.

Also there was just report about error from set_fact when grafana_ini is not set. Maybe just need to ansible.builtin.assert that grafana_ini is set if this silences set_fact errors.

The CI env conditional for the no_log disables the no_log when testing the role here on github, which is a good thing as there is no need to hide it here when developing the role.

@intermittentnrg
Copy link
Contributor

intermittentnrg commented Feb 18, 2025

Ok.But there was recent report on error from set_fact (#320) and not sure how no_log: true affects error reporting?

I think we should add assert that grafana_ini var is set in preflight.yml. Can do a separate PR.

edit: ah, set_fact to inherit from grafana_ini_defaults runs before preflight.

edit2: maybe preflight checks should run before inherit from grafana_ini_defaults. No point checking default values in preflight.

edit3: probably simplest&quickest just to add assert/fail before set_fact

@root-expert
Copy link
Author

@intermittentnrg I think this should be handled in a different PR

@intermittentnrg
Copy link
Contributor

intermittentnrg commented Feb 20, 2025

As long as no_log: true doesn't hide error when grafana_ini is not set.

Error would change from

fatal: [x.x.x.x]: FAILED! => {"msg": "The task includes an option with an undefined variable. The error was: 'grafana_ini' is undefined. 'grafana_ini' is undefined\n\nThe error appears to be in '/workdir/artifacts/collections/ansible_collections/grafana/grafana/roles/grafana/tasks/main.yml': line 2, column 3, but may\nbe elsewhere in the file depending on the exact syntax problem.\n\nThe offending line appears to be:\n\n---\n- name: Inherit default vars\n  ^ here\n"}

to

fatal: [x.x.x.x]: FAILED! => {"censored": "the output has been hidden due to the fact that 'no_log: true' was specified for this result", "changed": false}

(Untested, just found output from google search)

@root-expert
Copy link
Author

@gardar Do you think anything else is needed before merging this one?

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.

Grafana role leaks grafana.ini secrets in the logs
4 participants