feat: relax rule/alert name validation and escape ruler URL paths - #76
Conversation
71da6ae to
9243d9c
Compare
The provider rejected alert and rule-group names that Grafana exports contain (spaces, quotes, braces), and interpolated names unescaped into ruler API URL paths, breaking read/delete for names with special characters. - replace the strict name regexes with a validity guard: reject only empty/whitespace-only names, control characters, '/' and '.'/'..'; metric, record and label name validation stays strict - URL-escape every ruler path segment via shared helpers (rulesGroupPath/rulesNamespacePath); an AST-based test guards against raw path literals reappearing anywhere in the package - validate namespace and org_id on all resources and data sources, and harden the Terraform ID encode/parse (escape segments, re-validate org_id/namespace/name on read and import) so a '/' in a namespace or org_id can never fabricate an X-Scope-OrgID and read or delete another tenant's rules Signed-off-by: Sandro Manke <hello@sandrom.de>
9243d9c to
44c3c29
Compare
fgouteroux
left a comment
There was a problem hiding this comment.
Nice one — this is a good relaxation and the reasoning is sound. I double-checked against upstream: alert names just need to be valid label values, and label values allow any UTF-8, so spaces/quotes/unicode are all fair game. Group names only have to be non-empty + unique too. So the bulk of this is spot on. 👍
Just one small nit on the braces: upstream rulefmt actually rejects { and } in rule names as a "common mistake check" (prometheus/prometheus#15851, in Prometheus 3.2.0 → vendored into mimir-prometheus). So a name with braces would sail through the provider but get a 400 from the ruler at apply time. Might be worth either dropping {/} from the accepted set on the name check (record/alert only — group names are fine as-is), or just tweaking the PR description so it doesn't promise braces work.
If it's easy, a one-liner like:
if strings.ContainsAny(name, "{}") {
return fmt.Errorf("rule/alert name must not contain braces { or }: %q", name)
}plus a tiny test would keep plan/apply in sync. No big deal either way — the rest looks great. Out of curiosity, did the braces case actually 400 on the Mimir versions you tested (2.17.10 / 3.0.6)? Would be good to confirm, but not blocking. 🙂
Upstream's braces common-mistake check (prometheus/prometheus#15851) applies to recording rule names only; alert and rule-group names accept braces. Verified live against Mimir 2.17.10 and 3.0.6 (record: 400, alert/group: 202) and pinned with an acceptance round-trip so a future scope change surfaces in CI. Record names already reject braces here via the strict metric-name validation, keeping plan and apply in sync. Signed-off-by: Sandro Manke <hello@sandrom.de>
|
Thanks for the careful review — and for pushing on the braces question, it made us verify instead of assume. 🙂 To answer your question directly: we hadn't exercised braces against a live Mimir (only plan-level unit tests). Now we have — plus a source check — and it turned up a nuance in the check's scope: The Live results (direct ruler POST, both CI versions — 2.17.10 and 3.0.6 behave identically):
So plan/apply are already in sync on all three: record names here keep the strict metric-name validation (rejects braces — matching the 400), and alert/group names accept them (matching the 202s). Rather than change validation, I've added an acceptance round-trip ( Happy to add an alert-name braces reject anyway if you'd prefer it as a UX guard — but since the server accepts these names (and generated/templated rules can plausibly carry them), we leaned toward mirroring the server exactly. |
No if mimir accept it it's fine ! Thanks. |
This PR makes the provider accept the rule-group and alert names that Grafana (and other tooling) actually produces, and fixes how those names travel through the ruler API so they survive the full lifecycle.
The problem
We manage alerts that are generated by other software (Grafana exports) and deployed through a CD pipeline. Those tools don't follow the provider's naming regexes, so perfectly valid rules fail to deploy:
The backend is much more permissive than these regexes (per Prometheus
rulefmt, a group name only needs to be non-empty; an alert name is a label value). Renaming every export by hand defeats the point of managing them as code, since we want to keep pulling updated exports.What this PR changes
1. Name validation matches reality. Alert and rule-group names now accept what Grafana produces (spaces, quotes, braces, unicode,
%,#, …). Still rejected: empty/whitespace-only names, control characters,/, and./..— things that cannot work in a URL path or the Terraform resource ID. Metric names, recording-rule (record) names, and label/annotation names keep their strict Prometheus rules — those regexes are correct for those fields.2. Ruler API paths are percent-escaped — the same thing mimirtool does. Previously names were interpolated raw into the URL (
fmt.Sprintf("/config/v1/rules/%s/%s", ns, name)). That only appeared to work for simple names: a%crashed the provider, a#silently truncated the path so the provider talked to the wrong namespace, and the Grafana ecosystem's own client escapes these paths — seeurl.PathEscape(namespace)/url.PathEscape(groupName)in mimirtool's rules client. This PR uses the exact same escaping (sharedrulesGroupPath/rulesNamespacePathhelpers), so the provider now addresses the server identically to the official tooling. An AST-based test fails the build if a raw ruler path ever reappears.3. A cross-tenant bug is closed. The typed resources rebuild their state from the Terraform ID by splitting on
/. Since namespaces were never validated, a namespace containing/would shift that split and fabricate anX-Scope-OrgID— silently reading/deleting another tenant's rules. Namespaces andorg_idare now validated (no/, control chars, or./..), and the ID parse re-validates every segment on read and import, so this can't happen through config,terraform import, or legacy state.Compatibility
%in a namespace crashed the provider, and#/?silently truncated the path (the provider talked to the wrong namespace). These now work and address the literal name — same semantics as mimirtool.team%20a), which the old code let the server decode toteam a. The warning explains that the value is now sent literally and that writing the decoded form addresses the same server-side namespace as before. Happy to switch this to a hard error (or drop it) if you prefer a different compatibility stance.Invalid Group Rule Name/Invalid Alerting Rule Nameprefixes are unchanged so log matching keeps working.Testing
-race.%/#(with an out-of-band check that no orphan group is left behind), namespace policy (space works,/rejected), and import round-trip plus rejection of a crafted%2Fimport ID.