fix(fn-dashboard): do not mask empty panel query fields - #230
fix(fn-dashboard): do not mask empty panel query fields#230GurinderRawala wants to merge 1 commit into
Conversation
A reporting-backed custom dashboard panel failed to render with:
[Grafana proxy]:: Failed to resolve FN-redacted query
rawSqlPrefix: '[MFE_REDACTED:p:4:A]'
`maskRawQueryFields` masked any query field whose value was a string,
including the empty string. For a panel that intentionally carries no SQL that
fabricates a `[MFE_REDACTED:p:<id>:<refId>]` marker promising the proxy a query
which does not exist. The proxy must parse a valid key out of any value with
that prefix, resolves the panel, finds an empty `rawSql`, and fails the whole
`/api/ds/query` batch closed rather than rendering the panel.
Empty query fields are now left untouched. This is safe by construction: there
is no query text to leak, and the marker's only purpose is to stand in for text
that was removed.
Two shapes depend on an intentionally empty query field:
- CodeRabbit reporting-backed panels, whose data comes from the reporting API
via a `crReportingTag` on the target rather than from SQL.
- Variable metricFindQueries, which already arrive with an empty rawSql and a
`tempVar<N>` refId; the proxy has a dedicated pass-through for these that
the fabricated marker was bypassing.
Scoped to the masking predicate rather than gated on dashboard kind: the fork
has no notion of a "custom" dashboard, and an empty query is meaningless to
mask for any dashboard.
Three regression tests cover an empty rawSql, a whitespace-only query, and a
mixed dashboard where a non-empty query on another panel is still masked.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
🔇 Additional comments (2)
📝 WalkthroughWalkthrough
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
Problem
A reporting-backed custom dashboard panel failed to render:
Root cause
maskRawQueryFieldsmasked any query field whose value was a string — including the empty string:Verified against
simplejson:CheckGet("rawSql")on""returnsok=trueandString()returns("", nil), so an empty value took the mask branch.For a panel that intentionally carries no SQL, that fabricates a marker promising the proxy a query which does not exist. The proxy's contract is to fail closed on any value carrying the redaction prefix — it must parse a valid key rather than forward a literal placeholder upstream. So it resolves panel 4, finds
rawSql: "", hitsif (!originalSql) return null, and rejects the entire/api/ds/querybatch.Fix
Skip masking when the query field is empty or whitespace-only. Safe by construction: there is no query text to leak, and the marker exists solely to stand in for text that was removed.
Two shapes depend on an intentionally empty query field:
crReportingTagon the target rather than from SQL.rawSqland atempVar<N>refId. The proxy has a dedicated pass-through for exactly this shape (if (raw.length === 0)), which the fabricated marker was routing around. So this also removes a latent inconsistency that predates the reporting work.Why not gate on "is this a custom dashboard?"
The requested framing was to skip tag generation for custom dashboards specifically, but that isn't the right seam:
pkg/apihas nocd-prefix or dashboard-kind concept; introducing one would mean threading a classification through the mask path purely to express "this panel has no SQL" — which the panel already states by having no SQL.The narrower predicate fixes the reported failure, keeps every non-empty query masked, and adds no new coupling.
Verification
go build ./pkg/api/go test ./pkg/api/ -run "TestMask|TestIsCodeRabbitMFE|TestMfe|TestPanelAndVariable"gofmt -lThree regression tests added:
rawSqlis left untouched, and a siblingcrReportingTagsurvives intact[MFE_REDACTED:p:5:A]I confirmed the tests actually catch the bug: with the guard removed all three fail with
actual: "[MFE_REDACTED:p:4:A]"; with it restored all pass.Pre-existing failure, unrelated:
TestDashboardSnapshotAPIEndpoint_singleSnapshotpanics withno guardian factory implementation provided. I confirmed it fails identically on a stashed/clean tree.Related
Pairs with mono#25309, which generates these reporting-backed panels. Both are needed for a reporting panel to render.
Summary by CodeRabbit