feat: add TLS certificate and key support for Grafana for HTTPS#284
Merged
richm merged 1 commit intolinux-system-roles:mainfrom Jan 20, 2026
Merged
feat: add TLS certificate and key support for Grafana for HTTPS#284richm merged 1 commit intolinux-system-roles:mainfrom
richm merged 1 commit intolinux-system-roles:mainfrom
Conversation
Reviewer's GuideAdds configurable TLS/HTTPS support for the Grafana metrics graphing service via new role variables and certificate role integration, and fixes test flakiness by flushing handlers before verification while extending tests to cover HTTPS and certificate workflows. Sequence diagram for configuring Grafana HTTPS with certificate rolesequenceDiagram
actor Admin
participant AnsibleController
participant MetricsRole
participant CertificateRole
participant Filesystem
participant GrafanaRole
participant GrafanaService
Admin->>AnsibleController: Run playbook with metrics_graph_service true
Admin->>AnsibleController: Set metrics_grafana_certificates
AnsibleController->>MetricsRole: Include linux_system_roles.metrics
MetricsRole->>MetricsRole: Compute grafana_cert and grafana_private_key
alt metrics_grafana_certificates not empty and os_family RedHat
MetricsRole->>MetricsRole: Validate distribution_version and ca
MetricsRole-->>Admin: Fail if distribution_version 7 and ca self_sign
MetricsRole->>CertificateRole: Include fedora.linux_system_roles.certificate
CertificateRole->>Filesystem: Create key and certificate
CertificateRole-->>MetricsRole: certificate_requests completed
else metrics_grafana_certificates empty
MetricsRole-->>MetricsRole: Skip certificate_role
end
MetricsRole->>MetricsRole: Optionally copy cert and key from src paths
MetricsRole->>Filesystem: Copy grafana_cert_src to grafana_cert
MetricsRole->>Filesystem: Copy grafana_private_key_src to grafana_private_key
MetricsRole->>GrafanaRole: Include grafana role with grafana_metrics_provider
GrafanaRole->>GrafanaService: Configure and restart with TLS cert and key
GrafanaService-->>Admin: HTTPS endpoint available
Sequence diagram for fixing tests to flush handlers before verificationsequenceDiagram
actor Tester
participant AnsibleController
participant MetricsRole
participant Handlers
participant VerifyTasks
participant Services
Tester->>AnsibleController: Run metrics role tests
AnsibleController->>MetricsRole: Apply linux_system_roles.metrics
MetricsRole->>Handlers: Notify service handlers
AnsibleController->>Handlers: meta flush_handlers
Handlers->>Services: Restart or reload services
Services-->>Handlers: Services refreshed
AnsibleController->>VerifyTasks: Run verification tasks
VerifyTasks->>Services: Check metrics and HTTPS endpoints
Services-->>VerifyTasks: Return actual post_handler state
VerifyTasks-->>Tester: Deterministic test results (no flakes)
Class diagram for metrics role variables and related roles for Grafana TLSclassDiagram
class MetricsRole {
+bool metrics_graph_service
+string metrics_provider
+list~dict~ metrics_grafana_certificates
+string metrics_grafana_cert
+string metrics_grafana_private_key
+string metrics_grafana_cert_src
+string metrics_grafana_private_key_src
+string __metrics_grafana_cert_dir
+string __metrics_grafana_private_key_dir
+void manage_graphing_service()
}
class CertificateRole {
+list~dict~ certificate_requests
+void create_certificates()
}
class GrafanaRole {
+string grafana_metrics_provider
+string grafana_cert
+string grafana_private_key
+void configure_grafana()
}
class Filesystem {
+string cert_dir
+string private_key_dir
+void write_cert()
+void write_key()
}
MetricsRole --> CertificateRole : uses
MetricsRole --> GrafanaRole : includes
MetricsRole --> Filesystem : copies_cert_and_key
CertificateRole --> Filesystem : generates_files
GrafanaRole --> Filesystem : reads_cert_and_key
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In tasks/main.yml the copy tasks use
grafana_cert_src/grafana_private_key_srcassrc, but those variables are never defined (the user-facing vars aremetrics_grafana_cert_src/metrics_grafana_private_key_src), so the copy will fail or not behave as intended; consider wiring the copy tasks directly to the metrics_* variables or defininggrafana_*_srcin the vars block. - The Jinja expressions that derive
grafana_certandgrafana_private_keycombine multiple conditions and fallbacks in a single line, which is hard to read and reason about; consider breaking this into a few simplerset_factsteps or usingternary/defaultto make the precedence between the three configuration modes more explicit. - Since
metrics_grafana_certificatesis intended to be mutually exclusive withmetrics_grafana_cert/metrics_grafana_private_keyand their *_src variants, it would be helpful to add an explicitassert/failearly in the role when conflicting variables are set, instead of silently ignoring the latter.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In tasks/main.yml the copy tasks use `grafana_cert_src`/`grafana_private_key_src` as `src`, but those variables are never defined (the user-facing vars are `metrics_grafana_cert_src`/`metrics_grafana_private_key_src`), so the copy will fail or not behave as intended; consider wiring the copy tasks directly to the metrics_* variables or defining `grafana_*_src` in the vars block.
- The Jinja expressions that derive `grafana_cert` and `grafana_private_key` combine multiple conditions and fallbacks in a single line, which is hard to read and reason about; consider breaking this into a few simpler `set_fact` steps or using `ternary`/`default` to make the precedence between the three configuration modes more explicit.
- Since `metrics_grafana_certificates` is intended to be mutually exclusive with `metrics_grafana_cert`/`metrics_grafana_private_key` and their *_src variants, it would be helpful to add an explicit `assert`/`fail` early in the role when conflicting variables are set, instead of silently ignoring the latter.
## Individual Comments
### Comment 1
<location> `tasks/main.yml:179-181` </location>
<code_context>
+ vars:
+ certificate_requests: "{{ metrics_grafana_certificates }}"
+
+ - name: Copy grafana cert
+ copy:
+ src: "{{ grafana_cert_src }}"
+ dest: "{{ grafana_cert }}"
+ mode: "0644"
</code_context>
<issue_to_address>
**issue (bug_risk):** `grafana_cert_src` and `grafana_private_key_src` are used but never defined as variables.
The defaults only define `metrics_grafana_cert_src` / `metrics_grafana_private_key_src`, and the `vars:` block defines only the destination paths (`grafana_cert`, `grafana_private_key`). Using the current `src` values will cause an undefined variable error at runtime. Consider switching to:
```yaml
actions:
- name: Copy grafana cert
copy:
src: "{{ metrics_grafana_cert_src }}"
dest: "{{ grafana_cert }}"
```
(and similarly for the private key).
</issue_to_address>
### Comment 2
<location> `README.md:192-193` </location>
<code_context>
+
+For generating a new certificate for grafana it is recommended to set the
+`metrics_grafana_certificates` variable. If you have your own certs/keys, or
+will create them from your own CA provider, see below `metrics_grafana_cert` et.
+al. for information about how to pass in and/or use your own certs.
+
+The value of `metrics_grafana_certificates` is passed on to the
</code_context>
<issue_to_address>
**issue (typo):** Use the correct form "et al." instead of "et."
Use "et al." (with a single period at the end), e.g. `metrics_grafana_cert et al.`
```suggestion
will create them from your own CA provider, see below `metrics_grafana_cert` et al. for information about how to pass in and/or use your own certs.
```
</issue_to_address>
### Comment 3
<location> `README.md:226` </location>
<code_context>
+`ca: self-sign` or `ca: local`, depending on your certmonger usage, see the
+[linux-system-roles.certificate documentation](https://github.com/linux-system-roles/certificate/#cas-and-providers) for details.
+
+NOTE: This creating a self-signed certificate is not supported on RHEL/CentOS-7.
+
+### metrics_grafana_cert: ''
</code_context>
<issue_to_address>
**issue (typo):** Fix the ungrammatical phrase "This creating" in the NOTE sentence
For example: "NOTE: Creating a self-signed certificate is not supported on RHEL/CentOS-7."
```suggestion
NOTE: Creating a self-signed certificate is not supported on RHEL/CentOS-7.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Collaborator
Author
|
[citest] |
richm
force-pushed
the
feat-grafana-cert-key
branch
from
33c6bde to
a150999
Compare
Collaborator
Author
|
[citest] |
Collaborator
Author
|
[citest] |
Feature: Add the ability to configure TLS/HTTPS for the Grafana graphing service using the following new role variables: * metrics_grafana_certificates - use the certificate role to generate certs * metrics_grafana_cert/metrics_grafana_private_key - paths to existing certs * metrics_grafana_cert_src/metrics_grafana_private_key_src - copy local files Reason: Users need the ability to secure Grafana connections with TLS/HTTPS to protect metrics data in transit, which is a common security requirement for production deployments. Result: The metrics role can now configure Grafana to use HTTPS by either: 1. Generating certificates via the certificate system role 2. Using existing certificate files on the target system 3. Copying certificate files from the control node All tests updated to verify HTTPS functionality, and a new test added to verify certificate role integration. In addition - the tests were written incorrectly to run the handlers. In order to refresh the services, the handlers must be run immediately after running the role, so that the checks and verify tasks are testing the actual result of running the role. This fixes several test flakes that are caused when the services were not refreshed when doing the verification. Signed-off-by: Rich Megginson <rmeggins@redhat.com>
richm
force-pushed
the
feat-grafana-cert-key
branch
from
a150999 to
829a5e2
Compare
Collaborator
Author
|
fedora-42 failure is selinux - needs policy update |
Collaborator
Author
|
[citest] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Feature:
Add the ability to configure TLS/HTTPS for the Grafana graphing service
using the following new role variables:
Reason:
Users need the ability to secure Grafana connections with TLS/HTTPS to
protect metrics data in transit, which is a common security requirement
for production deployments.
Result:
The metrics role can now configure Grafana to use HTTPS by either:
All tests updated to verify HTTPS functionality, and a new test added
to verify certificate role integration.
In addition - the tests were written incorrectly to run the handlers.
In order to refresh the services, the handlers must be run immediately
after running the role, so that the checks and verify tasks are testing
the actual result of running the role. This fixes several test flakes
that are caused when the services were not refreshed when doing the
verification.
Signed-off-by: Rich Megginson rmeggins@redhat.com
Summary by Sourcery
Add TLS certificate support for the Grafana metrics graphing service and stabilize service-dependent tests by flushing handlers before verification.
New Features:
Enhancements:
Documentation:
Tests: