Repository navigation
feat(logs): guard Loki queries with a cost budget on Grafana Cloud targets - #1759
edwardcqian wants to merge 4 commits into
Conversation
…rgets Before `gcx logs query|metrics` (and the `gcx datasources loki` equivalents) run, estimate the scan with Loki's index/stats and assess it with the shared lokiguard module against the stack's query policy: selective stream selectors, a time-range cap including range vectors, and a byte budget. A query over budget fails with a "Query blocked" error that says how to narrow it, in agent and terminal mode alike. There is no prompt and no bypass flag. When no verdict is possible (an unparseable query or a failed estimate), agent mode blocks and a terminal warns and runs. Self-hosted targets are not guarded. When the policy can't be fetched, a built-in 50 GiB / 24h / enforce default applies. Loki requests now carry X-Query-Tags: Source=gcx. Stacked on #1620 (gcx logs stats), whose stats client and selector helpers the guard uses. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Intent
gcx logs query|metrics (and the gcx datasources loki equivalents) now estimate a Loki query's cost before running it on Grafana Cloud targets, and refuse it when a tenant policy says it is too expensive. The decision logic sits in the shared lokiguard module; this PR adds the policy fetch, the index/stats client, the verdict-to-command-behaviour mapping, and an X-Query-Tags: Source=gcx header on all Loki traffic.
Blocking
defaultQueryPolicy()enforces when no policy was served. Every failure path lands onenforcewith a 50 GiB / 24h cap, including the HTTP 404 the PR body reports from a real Cloud stack today. This narrows two released commands for every Cloud user on merge day, with no flag and no policy to turn it off.GCX_LOKI_POLICY_CACHE_DIRhas no caller, no test, and no entry in the canonical env var reference.
Other findings
- The new help paragraph on
query.goandmetrics.godescribes the byte budget and the range cap but not the selector-selectivity rule, which blocks without any estimate and is the likeliest refusal an agent will hit. splitGuidancereconstructs structure fromlokiguard.Guidance's prose, including ane.gspecial case and dropping the first sentence by index.TestSplitGuidanceuses a synthetic string, so a wording change in the pinned module would not turn it red.queryGuard.nowand theguardStatsTimeoutpackage var are injection seams no caller varies. (nit)statsFuncclampsresp.Bytesagainstmath.MaxInt64for a value Loki cannot return, and the closure discards the contextlokiguardpasses it. (nit)IndexStatsis the fourth copy of the same build-GET-read-decode shape ininternal/query/loki. (nit)
Fix summary
In order: make enforce conditional on a served policy and fall back to shadow; delete policyCacheDirEnv and the policyCacheDir override branch; fold guardStatsTimeout into a const and drop the now field in favour of the now parameter check already takes; drop the MaxInt64 clamp; add one sentence on selectivity to both help strings. That removes roughly 50 lines from a +1096 diff and changes the default posture without changing the design.
Verdict
The approach is sound and the diff is proportionate to a guardrail that has to fetch a policy, estimate a query, and map three verdicts across two modes. The default posture in finding 1 is the one decision that should not ship as written; it also answers the "ship in shadow mode first?" question in the PR body.
comments below are prefixed required (2), recommended (2), nit (3)
Comment @claude review for a fresh review.
| func defaultQueryPolicy() queryPolicy { | ||
| return queryPolicy{Mode: modeEnforce, MaxBytes: defaultMaxBytes, MaxRangeSeconds: 24 * 60 * 60, Source: "default"} |
There was a problem hiding this comment.
required — this makes enforce the posture for every path that could not produce a served policy: no route for the auth method (basic, mtls, anonymous, a missing ProxyEndpoint), a rest.HTTPClientFor failure, any non-200, and a malformed body. The PR body reports the route returning HTTP 404 on a real Cloud stack, so on merge day every Grafana Cloud user of gcx logs query gets a hard 24h range cap and a selectivity requirement on two released commands, with no flag and no tenant policy able to lift it.
Separate the two defaults: keep 50 GiB / 24h as the budget, but return modeShadow when nothing was served and reach modeEnforce only from a policy the server actually sent. That also answers the "ship in shadow mode first?" question in the description without a second switch.
What would change this: is the policy route deployed and returning 200 on every Cloud stack before this merges, or is a hard default cap the intended product behaviour even with no tenant opt-in?
There was a problem hiding this comment.
Done in the latest push: defaultQueryPolicy() is now shadow (50 GiB / 24h). enforce is only reachable from a policy the server actually served, so a 404/unreachable route or bad body never blocks a query; it only logs what would have been blocked.
| // (config.Context.IsCloud); a local Grafana can never satisfy that. | ||
| policyURLEnv = "GCX_LOKI_POLICY_URL" | ||
| // GCX_LOKI_POLICY_CACHE_DIR overrides the on-disk policy cache directory. | ||
| policyCacheDirEnv = "GCX_LOKI_POLICY_CACHE_DIR" |
There was a problem hiding this comment.
required — nothing sets GCX_LOKI_POLICY_CACHE_DIR. No production caller passes it, guard_test.go injects policySource.cacheDir directly with t.TempDir(), and it is absent from docs/design/environment-variables.md, which that file declares the canonical list every other doc links to. The filepath.IsAbs condition at line 146 also discards a relative value in silence, so a user who did set it would get the default with no signal.
Delete the constant and the override branch, keeping policyCacheDir() as the home-directory resolution. Is there a caller for it I have not found?
There was a problem hiding this comment.
Removed GCX_LOKI_POLICY_CACHE_DIR and the override branch; policyCacheDir() is just the home-directory resolution now. No caller existed.
|
|
||
| On Grafana Cloud targets, a query cost guardrail runs first: the query is | ||
| estimated with Loki index-stats and refused, with guidance on how to narrow it, | ||
| when it exceeds the tenant's byte budget or time-range cap. There is no flag to |
There was a problem hiding this comment.
recommended — the paragraph names the byte budget and the time-range cap, but not the selector-selectivity rule. guard_test.go:184 and :227 show {job=~".+"} blocked under a cheap estimate with zero stats round trips, so a non-selective selector is refused on its shape alone. That is the refusal an agent writing a catch-all will actually hit, and the help gives it nothing to act on.
Add a clause: a selector that matches too broadly is refused whatever its estimated size. Same text in metrics.go:44.
There was a problem hiding this comment.
Added to both query and metrics help: a selector that matches too broadly is refused whatever its estimated size.
| func splitGuidance(guidance string) []string { | ||
| var out []string | ||
| rest := guidance | ||
| for { | ||
| i := strings.Index(rest, ". ") | ||
| for i >= 0 && strings.HasSuffix(rest[:i], "e.g") { | ||
| j := strings.Index(rest[i+2:], ". ") | ||
| if j < 0 { | ||
| i = -1 | ||
| break | ||
| } | ||
| i += 2 + j | ||
| } | ||
| if i < 0 { | ||
| break | ||
| } | ||
| out = append(out, strings.TrimSpace(rest[:i])) | ||
| rest = rest[i+2:] | ||
| } | ||
| if s := strings.TrimSuffix(strings.TrimSpace(rest), "."); s != "" { | ||
| out = append(out, s) | ||
| } | ||
| return out | ||
| } |
There was a problem hiding this comment.
recommended — splitGuidance recovers structure from lokiguard.Guidance's prose, and blockedError then drops the first sentence by index because it currently restates the reasons. Both depend on wording in a pinned module. TestSplitGuidance feeds a hand-written string rather than real Guidance output, so if lokiguard adds a leading sentence or reflows the text, the suggestions silently lose content and no test goes red.
Two options: have lokiguard expose the guidance as a slice of lines, or at minimum build the test input by calling lokiguard.Guidance on a real Decision so the pinned version's wording is what the splitter is checked against. Is a structured accessor something you can add on the lokiguard side, given you own that module too?
There was a problem hiding this comment.
Took the second option for now: TestBlockedErrorSuggestionsFromRealGuidance builds a real Decision and runs it through lokiguard.Guidance, so reworded or reflowed output from the pinned module breaks the test. A structured accessor on the lokiguard side is the better long-term fix; I'll follow up there separately.
| now func() time.Time | ||
| timeout time.Duration | ||
| } | ||
|
|
||
| // guardStatsTimeout bounds the whole index-stats estimate (all selectors). | ||
| var guardStatsTimeout = 5 * time.Second //nolint:gochecknoglobals // test-overridable timeout |
There was a problem hiding this comment.
nit — neither seam has a second caller. now is only ever time.Now, in newQueryGuard and in all three test constructions, and it is read once, at line 67, while check already receives a now parameter for the same instant. guardStatsTimeout carries a //nolint comment calling it test-overridable, but the tests set queryGuard.timeout directly instead.
Drop the now field and pass the parameter through, and make guardStatsTimeout a const.
There was a problem hiding this comment.
Dropped the now field (check passes its now parameter to the policy fetch) and made guardStatsTimeout a const.
| return func(_ context.Context, selector string, start, end time.Time) (int64, error) { | ||
| resp, err := g.client.IndexStats(ctx, datasourceUID, selector, start, end) | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
| if resp.Bytes > math.MaxInt64 { | ||
| return math.MaxInt64, nil | ||
| } |
There was a problem hiding this comment.
nit — resp.Bytes is a uint64 from Loki's index/stats. Exceeding math.MaxInt64 means a single selector matching over 8 EiB, which Loki cannot report, so the clamp and its branch can never run. The closure also names its own context _ and uses the outer deadline instead, which discards any cancellation lokiguard attaches to a single stats call — intentional for the shared budget, but worth saying in the comment that the per-call context is deliberately ignored.
There was a problem hiding this comment.
Removed the clamp, and the comment on statsFunc now says the per-call context is deliberately ignored because all selectors share one deadline.
|
|
||
| // IndexStats calls Loki's index/stats endpoint for a stream selector, returning | ||
| // what the selector matches over [start, end) without running a query. | ||
| func (c *Client) IndexStats(ctx context.Context, datasourceUID, selector string, start, end time.Time) (*IndexStatsResponse, error) { |
There was a problem hiding this comment.
nit — this is the fourth copy of the same shape in the package: build URL from restConfig.Host, set query params, httpClient.Do, httputils.ReadResponseBody, status check through queryerror.FromBody, json.Unmarshal. Labels, LabelValues and Series in client.go are the other three. Following the siblings is the right call for this PR; a shared getJSON(ctx, apiPath, params, &out) helper would collapse all four and is worth its own change.
There was a problem hiding this comment.
Agreed, leaving it as a sibling copy here. A shared getJSON helper over all four belongs in its own change.
Only a policy the server actually served can enforce; a missing or failing policy route now falls back to the 50 GiB / 24h budget in shadow mode. Also drops the unused GCX_LOKI_POLICY_CACHE_DIR override, the now seam, the unreachable MaxInt64 clamp, documents selector selectivity in the help, and checks the guidance splitter against real lokiguard output. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
What this changes
On Grafana Cloud targets,
gcx logs query|metrics(and thegcx datasources lokiequivalents) now estimate a Loki query's cost before running it, and refuse queries that are too expensive. Agents write catch-all selectors like{job=~".+"}over long windows, and Loki doesn't enforcemax_query_bytes_readon log queries without a line filter, so these scan terabytes today.The decision comes from the shared
lokiguard(released aslokiguard/v0.1.0) module: selective stream selectors, a time-range cap that includes range vectors, and a byte budget fromindex/stats. The budget comes from the stack's query policy, fetched and cached for 5 minutes. If it can't be fetched, gcx uses a built-in default (50 GiB, 24h) in shadow mode: it logs what would have been blocked but never refuses a query. Only a policy the server actually served can enforce.Query blockederror with how to narrow the query, exit 1There is no prompt and no
--yesbypass: only the policy sets the budget. Self-hosted targets are not guarded, by design, and the command help says so. Loki requests now carryX-Query-Tags: Source=gcx. No new commands.Merge blockers
This is a draft and is not mergeable yet:
main: it adds its ownindex/statsclient (internal/query/loki/index_stats.go) rather than depending on feat(loki): add gcx logs stats and datasources loki stats #1620 (gcx logs stats), so it merges independently of that PR. It replaces feat(logs): guard Loki queries with a cost budget on Grafana Cloud targets #1619, which was stacked on feat(loki): add gcx logs stats and datasources loki stats #1620.--yesfor large scans). The two shouldn't both ship, so the approach needs to be agreed with its author before merging.GCX_LOKI_POLICY_URLis a test-only override that forces the guard on and skips auth, because a local Grafana is never a Cloud target. It should be removed or hidden before merge.Open questions
gcx datasources query <uid> '<logql>',gcx api, and the synth/faro Loki calls bypass the guard. It probably belongs in the Loki client rather than in individual commands.X-Query-Tagson/api/ds/query, so only the stats probes are attributable as gcx in Loki.How it was tested
Unit tests with
httptestservers for stats and policy, a full cobra command test, and the existing agent-conformance and error suites (GCX_AGENT_MODE=false go test ./internal/datasources/... ./internal/query/loki/... ./internal/gcxerrors/... ./cmd/gcx/root/... ./cmd/gcx/fail/...). Also run by hand against a local Loki 3.7 and Grafana 13 with a 50 MiB / 24h policy, in agent and terminal mode and in enforce, shadow and off: a narrow selector runs; a 600 MiB selector, a catch-all selector, and a 48h window are blocked; an unparseable query and a failingindex/statsblock in agent mode and warn in a terminal. Also run against a real Grafana Cloud stack (theopscontext, OAuth) before the default became shadow (so with the 50 GiB default enforcing; it has not been re-run since), since the policy endpoint isn't deployed there yet and the fetch returns HTTP 404: a 52 MiB selector ran; a 700 GiB selector ({cluster="prod-us-east-0"}, 1h), a catch-all selector and a 48h window were blocked without running the query, for bothlogs queryandlogs metrics; an unparseable query was blocked in agent mode and warned then reached Loki's own syntax error in a terminal.-vprints the fallback reason. Not tested: real Cloud OAuth, the token (service account) policy path, orGOOS=wasip1(the tree doesn't build for it).Not run locally:
mise run lint(golangci-lint here is built with Go 1.26, module needs 1.27).🤖 Generated with Claude Code