feat(monitoring): add optional SMTP configuration for Grafana - #2751
feat(monitoring): add optional SMTP configuration for Grafana#2751Sergei Makarov (SerjioTT) wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds Grafana SMTP configuration: JSON schema and type additions, default values and README docs, Monitoring ApplicationDefinition schema updates, and Helm template wiring that conditionally creates an SMTP Secret and injects GF_SMTP_* env vars into the Grafana container. ChangesSMTP Configuration Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The E2E test was failure and it's not related to this PR. The |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces support for SMTP configuration in the Grafana monitoring stack. By allowing users to define SMTP settings, it enables email notifications directly through Grafana Unified Alerting. The implementation uses environment variables to bypass limitations in Grafana's configuration file parsing, while maintaining security by utilizing Kubernetes Secrets for sensitive credentials. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces SMTP configuration support for Grafana email notifications across the monitoring packages, adding schema definitions, default values, documentation, and Kubernetes manifests (Secret and environment variables). The review feedback highlights that Grafana's SMTP configuration key is start_tls_policy (with underscores) rather than startTLS_policy, and recommends renaming this parameter across the schema, values, and documentation. Additionally, the feedback suggests conditionally creating the SMTP Secret and injecting user/password environment variables only when a username is provided, which enables support for passwordless SMTP relays. Finally, it notes that the generated monitoring.yaml file should be regenerated after these schema updates.
Add GF_SMTP_* environment variables injected into the Grafana container to enable email notifications through Grafana Unified Alerting. SMTP credentials are stored in a Kubernetes Secret (grafana-smtp) and the feature is disabled by default via grafana.smtp.enabled=false. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: SerjioTT <serjiott@gmail.com>
8b84a3c to
b4860d9
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces SMTP configuration support for Grafana within the monitoring package. It updates the documentation, values schema, default values, and cozyrds monitoring schema to include SMTP-related parameters such as host, user, password, and TLS policies. Additionally, it updates the Grafana deployment template to conditionally create a Kubernetes Secret for SMTP credentials and injects the SMTP configuration as environment variables into the Grafana container. There are no review comments, so I have no feedback to provide.
|
This PR has had no activity for 60 days and was marked |
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
NOT LGTM. The feature is wanted and the plumbing is mostly right, but the password ends up somewhere a tenant can read it.
The PR body says credentials never appear in the Grafana CR spec in plain text, and that is true of the Grafana CR. The exposure moved up a level rather than away. packages/extra/monitoring/templates/helmrelease.yaml:60 is values: {{- .Values | toYaml | nindent 4 }}, which copies the entire values tree into the nested HelmRelease verbatim, so helm template monitoring packages/extra/monitoring --set grafana.smtp.password=SUPERSECRET renders spec.values.grafana.smtp.password: SUPERSECRET. The password therefore lands in the Monitoring CR, in the monitoring-system HelmRelease and in Flux's release-storage Secret, all in the tenant namespace and all readable by the tenant.
The shape this repo uses for secret material is secretRef.name pointing at an existing Secret in the release namespace, mutually exclusive with an inline value and guarded by a hard fail. It lives in this very chart at packages/system/monitoring/templates/_helpers.tpl:115-122, with the tenant-facing contract in packages/extra/monitoring/values.yaml. To be clear about the timing: that convention landed on 15 July, and this PR was opened on 27 May, so it is not something you ignored. It is simply what the chart now expects, and adopting it is what unblocks this.
Two more that need fixing.
The grafana.smtp block was only added to packages/extra/monitoring/values.yaml, never to packages/system/monitoring/values.yaml, so rendering the system chart on its own emits a bare value: for GF_SMTP_FROM_ADDRESS and GF_SMTP_HOST. That is YAML null rather than an empty string, because quote passes a nil through untouched instead of quoting it into "". Worth knowing generally: {{ .Values.missing | quote }} renders nothing at all, while {{ .Values.missing | default "" | quote }} renders "".
There is no validation on the block. enabled: true with an empty host renders GF_SMTP_ENABLED=true next to GF_SMTP_HOST="", and every mail send then fails silently, which is the failure mode operators find hardest to diagnose. The chart's habit elsewhere is a hard fail at render time, as in templates/vlogs/vlogs.yaml:9 and the helper above.
The branch also conflicts with main now, in packages/system/monitoring/templates/grafana/grafana.yaml and packages/system/monitoring-rd/cozyrds/monitoring.yaml.
Things I checked that are fine, so you do not spend time on them. The generated artifacts are clean: regenerating the schema, the README table and the RD on your head produces zero drift, which is unusual and appreciated. Nothing here derives a name from .Release.Name, so the release-naming confusion tracked in #3338 does not touch this diff. And the one red check is not yours: the E2E run died on the kubevirt-operator HelmRelease hanging in an upgrade for 49 minutes, cascading into the Cluster-API provider wait, with no monitoring or SMTP line anywhere in the log.
One judgement call I am recording rather than blocking on: a tenant who can edit the Monitoring CR can point Grafana at any SMTP host and set skip_verify. That is consistent with what the same CR already allows through the Slack, Telegram and remote-write settings, so I do not think it needs gating.
Given how much of the chart around this was rewritten since May, I would treat this as a redo on current main using secretRef rather than a rebase and patch. If you have moved on from it, say so and I will close it with a pointer to the helper, so whoever picks it up next starts from the right shape.
|
Closed because no activity followed the |
|
Closing this intentionally rather than letting it lapse — #3800 covers the need, at a better layer. Our actual requirement was email delivery for alerts. This PR approached it through Grafana's own SMTP ( For the record, what #3800 deliberately does not cover is Grafana-native SMTP — UI-configured contact points, user invites, password reset mail, but we do not need those today. Thanks for the reviews! |
What this PR does
Add optional SMTP configuration for Grafana to enable email notifications
through Grafana Unified Alerting.
SMTP parameters are injected as
GF_SMTP_*environment variables into theGrafana container. Credentials are stored in a Kubernetes Secret and injected
via
secretKeyRef.Why
CozyStack's monitoring stack routes alerts through
VMRule → vmalert → VMAlertmanager → Alerta → Telegram/Slack. This works well for infrastructurealerts defined as YAML manifests.
However, Grafana Unified Alerting (default since Grafana v9) lets users
create custom alert rules directly in the Grafana UI — with visual threshold
editors, arbitrary PromQL queries, and per-dashboard alerting. These alerts
use Grafana's internal Alertmanager, which is separate from
VMAlertmanager/Alerta, and cannot send email without SMTP configured in Grafana.
This is not duplication of existing functionality. This PR allows end users to enable email alerts.
Email is the most universal notification channel in enterprise environments,
and many organizations require it.
Implementation: env vars vs spec.config.smtp
This PR uses environment variables (
GF_SMTP_*) rather than adding asmtpsection tospec.configof the Grafana CR.Reason: when SMTP credentials are placed in
spec.config.smtpwith${VAR}placeholders (the pattern used by
GF_DATABASE_PASSWORD), grafana-operatorwrites those placeholders into
grafana.iniliterally. Grafana does notperform environment variable expansion in the
[smtp]section ofgrafana.ini,so it attempts authentication with the literal string
${GF_SMTP_PASSWORD}and fails with
535 incorrect password.Environment variables of the form
GF_<SECTION>_<KEY>are read directly byGrafana at startup and override
grafana.inivalues. This works reliablywith grafana-operator. Verified empirically on a live CozyStack cluster.
Security
SMTP credentials follow the existing chart pattern:
The chart creates
grafana-smtpSecret from values. Credentials never appearin the Grafana CR spec in plain text.
Screenshots
Testing
Tested on a live CozyStack (v1.3.5) cluster (Grafana 11.4.0) by patching the Grafana CR
with
GF_SMTP_*env vars includingsecretKeyReffor credentials, then verifying:kubectl exec ... env | grep GF_SMTP)smtp.enabled=true, password masked)spec.config.smtpwith${VAR}placeholdersconfirmed NOT working — fails with 535
Backward compatibility
Fully backward-compatible.
grafana.smtp.enableddefaults tofalse,so existing deployments are unaffected. The Secret and env vars are only
rendered when explicitly enabled.
Release note
Summary by CodeRabbit
New Features
Documentation