[Security Solution] Report zero-shard rule searches as actionable warnings instead of "expected to find aggregations" - #290924
Conversation
… instead of "expected to find aggregations" Query rules with alert suppression, New Terms and Threshold rules threw a generic "expected to find aggregations on search result" error whenever the events search came back without an aggregations object. That happens when the search resolves to zero shards, e.g. the rule owner has view_index_metadata but no read privilege on the source indices, or a cross-project search linked project silently excludes them. The generic error hid the real cause and counted as a framework error in SLOs. - Add no_readable_shards helper: shard or cluster failures fail the run as a user error, a zero-shard response becomes a warning naming the index patterns and CPS linked projects, and only a searched-but-aggregation-less response still throws. - Merge _clusters.details failures into singleSearchAfter search errors. - Pass CPS scope into SecuritySharedParams so warnings can name linked projects. - Apply the same handling to the plain query path, which used to succeed silently. - Classify security_exception as a user error. - Add a Scout API suite that runs each rule type as a view_index_metadata-only owner. Closes elastic#290840 Closes elastic#289108 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
maximpn
added
release_note:fix
Team: SecuritySolution
|
Pinging @elastic/security-solution (Team: SecuritySolution) |
dplumlee
left a comment
dplumlee
left a comment
There was a problem hiding this comment.
Pulled down and tested this locally with a user role that didn't have appropriate read privileges (only view_index_metadata)
- Custom query rule with alert suppression: now seeing
partial failurewith actionable message naming the index pattern and missing privileges - Plain custom query rule: same as above
- After granting
read+ re-saving: both rules nowsucceeded
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the zero-shard / missing-aggregations handling across the query, threshold, and new terms rule paths. The error-push wiring, reportMissingAggregations priority (errors → zero-shard warning → throw), warning dedup, CPS plumbing, and the security_exception reclassification all look sound and are well covered by tests. One reliability concern about the new _clusters handling flipping otherwise-healthy cross-cluster runs to partial failure — left inline.
Generated by Claude Reviewer for #290924 · claude · opus · 279.5 AIC · ⌖ 49.9 AIC · ⊞ 5.5K
| const failures = createErrorsFromShard({ errors: detail.failures ?? [] }) | ||
| .filter((failure) => !knownShardErrors.has(failure)) | ||
| .map((failure) => `cluster: "${alias}" ${failure}`); | ||
|
|
There was a problem hiding this comment.
singleSearchAfter now folds createErrorsFromClusters(...) into searchErrors, and every caller (group_and_bulk_create, new terms, threshold, plain search-after) pushes searchErrors into result.errors on the normal path too — not just when aggregations are missing. A non-empty result.errors flips the run to partial failure.
This fallback branch reports any skipped or failed cluster that carries no per-shard failures. skipped is the status Elasticsearch returns for a skip_unavailable: true remote cluster that is temporarily unavailable — a normal, non-fatal condition, and the whole reason skip_unavailable exists. Before this PR the search-after path never read _clusters, so such a classic CCS rule kept running green on the reachable clusters and creating alerts.
Failure scenario: a query/threshold/new_terms rule targets local-*,remote:logs-* with remote set to skip_unavailable: true. remote goes down. ES returns _clusters.details.remote.status = "skipped" with the readable local shards succeeding and aggregations present. The run now surfaces cluster: "remote" status: "skipped" indices: "..." as an error and reports partial failure on every execution while remote is down, even though the rule is working as designed. This is a behavior change for CCS users beyond the CPS no-read case the PR targets.
Consider distinguishing intentionally-skipped (skip_unavailable) clusters from ones that should fail the run — e.g. only surface failed here, or gate the skipped case on the presence of an actual failure — so a healthy CCS run with an intentionally skipped remote does not regress to partial failure.
…rors Folding _clusters.details entries into searchErrors turned a skipped remote cluster into a failed rule run. A skip_unavailable remote that is down returns status "skipped" with a shard: -1 failure and no _shards.failures, so the search-after rule types used to run green on the reachable clusters and now failed every execution as a framework error. - singleSearchAfter returns cluster-level entries as searchWarnings and keeps searchErrors limited to _shards.failures, as before. - Rename createErrorsFromClusters to createWarningsFromClusters and word the messages as warnings naming the cluster and its status. - Callers push searchWarnings into warningMessages, which yields a partial failure status while alerts keep being created from the reachable clusters. - reportMissingAggregations and the threshold single-bucket branch no longer throw when shard errors or cluster warnings explain the missing aggregations. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
| apiTest.afterAll(async ({ esClient, kbnClient }) => { | ||
| for (const id of createdRuleIds) { |
There was a problem hiding this comment.
Contribute to Scout when possible
Cleanup rolls its own per-id delete loop when apiServices.detectionRule.deleteAll() already ships this in @kbn/scout-security. Reuse that helper so the suite gets the standard bulk-delete path and future refactors flow through one place.
See details
getDetectionRuleApiService in x-pack/solutions/security/packages/kbn-scout-security/src/playwright/fixtures/worker/apis/detection_rule.ts exposes a deleteAll() that posts to /api/detection_engine/rules/_bulk_action (space-aware, via scoutSpace) with { query: '', action: 'delete' }. It's the pattern used by the existing Security Scout suites, and the security-scout skill lists it under "Data cleanup — Security Solution resources" as the standard cleanup helper for detection rules.
Suggested change:
apiTest.afterAll(async ({ esClient, apiServices }) => {
await apiServices.detectionRule.deleteAll();
await esClient.indices.delete({ index: sourceIndex }, { ignore: [404] });
});That also drops the need for createdRuleIds, the manual PUBLIC_HEADERS on the DELETE call, and the per-id ignoreErrors: [404] — the helper handles all of that once at the framework level.
Share feedback in the #kibana-qa Slack channel.
There was a problem hiding this comment.
Not applicable here, for two reasons.
-
apiTestexported from@kbn/scout-securityis the base@kbn/scoutapiTest, whoseapiServicesfixture has nodetectionRuleservice. That service is attached only in the Securitytest/spaceTestfixtures (single_thread_fixtures.ts/parallel_run_fixtures.ts), soapiServices.detectionRule.deleteAll()does not type check in this suite. -
deleteAll()posts_bulk_actionwithaction: 'delete'and an empty query, i.e. it deletes every rule in the space. This suite runs underapiTestwithoutscoutSpace, so it executes in the default space shared with the other API suites in the same run. Every existingdeleteAll()caller is aspaceTestsuite with a per-worker space, where the wipe is safe. Here it would remove rules created by sibling suites.
Keeping cleanup scoped to the rules this suite created.
vgomez-el
left a comment
vgomez-el
left a comment
There was a problem hiding this comment.
LGTM from the test-quality / Scout side. The suite sits at the right level of the pyramid: the logic is well covered by unit tests, and this API suite validates the real ES permission behaviour (zero shards when the owner lacks read) that units can only mock, so it earns its keep. Auth, isolation, tags and placement all look good, and it will be picked up in CI. Approving. I left two small non-blocking nits inline — feel free to take them or leave them, they don't block the approval.
| }); | ||
|
|
||
| const createRule = async ( | ||
| apiClient: { post: Function }, |
There was a problem hiding this comment.
nit (non-blocking): these helpers type apiClient as { post: Function } here and { get: Function } on line 83. Function drops type-safety and goes against the repo guidance of avoiding untyped params. You could use the real apiClient fixture type from @kbn/scout-security instead so the calls stay typed. Not blocking.
| import { ELASTIC_INTERNAL_ORIGIN_HEADER, PUBLIC_API_HEADERS } from '@kbn/scout-security'; | ||
| import type { KibanaRole } from '@kbn/scout-security'; | ||
|
|
||
| export const DETECTION_ENGINE_RULES_URL = '/api/detection_engine/rules'; |
There was a problem hiding this comment.
nit (non-blocking): DETECTION_ENGINE_RULES_URL is already exported from security_solution/common/constants.ts, so you could reuse it here to avoid the two drifting apart. That said, hardcoding the URL in a test as a contract check is also a fair choice, so up to you. Not blocking.
| }, | ||
| { timeout: 120_000, intervals: [2_000] } | ||
| ) | ||
| .toBeDefined(); |
There was a problem hiding this comment.
waitForFirstExecution resolves as soon as execution_summary.last_execution.status is defined, but that status can be a non-terminal going to run / running while the rule is mid-execution — so the later toBe('partial failure') assertion can fail intermittently in CI. Poll until the status reaches a terminal value instead.
See details
RuleExecutionStatus is z.enum(['going to run', 'running', 'partial failure', 'failed', 'succeeded']) (common/api/detection_engine/rule_monitoring/model/execution_status.gen.ts). On a freshly enabled rule the execution logger records going to run / running before the terminal status, and both are surfaced in execution_summary.last_execution.status.
The poll returns on the first defined status:
return lastExecution?.last_execution.status; // resolves on 'running' too
...
.toBeDefined();The rule's first run starts within a few seconds of creation and, for the zero-shard path, finishes almost immediately, so the 2s poll can land on running / going to run. expectNoReadableShardsPartialFailure then asserts toBe('partial failure') and fails — a classic passes-locally, fails-rarely-in-CI flake.
The established detections helper waitForRuleStatus(expectedStatus, ...) in @kbn/detections-response-ftr-services (e.g. waitForRulePartialFailure) polls until the status equals the specific expected value for exactly this reason. Mirror that here — wait until a terminal status before returning:
await expect
.poll(
async () => {
const response = await apiClient.get(`${DETECTION_ENGINE_RULES_URL}?id=${ruleId}`, {
headers: ruleOwnerHeaders,
responseType: 'json',
});
lastExecution = (response.body as RuleResponse).execution_summary;
const status = lastExecution?.last_execution.status;
return status !== undefined && status !== 'going to run' && status !== 'running';
},
{ timeout: 120_000, intervals: [2_000] }
)
.toBe(true);expectNoReadableShardsPartialFailure then still asserts the exact partial failure status and message. (Polling directly on .toBe('partial failure') works too, but the terminal-status form keeps the helper reusable and surfaces an unexpected succeeded / failed faster.)
Share feedback in the #kibana-qa Slack channel.
There was a problem hiding this comment.
Reviewed the zero-shard / no-readable-shards handling across the custom query, suppression, Threshold, and New Terms paths. The design is sound: security_exception classification, the shard-failure vs zero-shard vs genuine-bug branching in reportMissingAggregations, and the switch to reporting per-cluster conditions as warnings (addressing the earlier CCS skip_unavailable regression) all look correct, with solid unit + Scout coverage. One non-blocking finding about duplicated per-page cluster warnings is left inline.
Generated by Claude Reviewer for #290924 · claude · opus · 265.3 AIC · ⌖ 26.4 AIC · ⊞ 5.5K
| }); | ||
|
|
||
| searchAfterResults.searchDurations.push(searchDuration); | ||
| warnings.push(...searchWarnings); |
There was a problem hiding this comment.
Inside this paging do...while loop, searchWarnings is re-pushed on every iteration. For a CCS/CPS search where a linked/remote cluster is skipped (or failed) but the readable clusters still return aggregations, the loop keeps paging and appends the identical per-cluster warning once per page, so the rule's execution message repeats the same warning N times (N = number of composite pages).
reportMissingAggregations deliberately dedupes its no-readable-shards warning (if (!result.warningMessages.includes(warning))), but these per-page cluster warnings coming from createWarningsFromClusters are not deduped anywhere. The New Terms paging loops (create_new_terms_alert_type.ts and multi_terms_composite.ts) push searchWarnings inside their while loops the same way and have the same duplication.
Failure scenario: a threshold rule with fields over local-*,remote:logs-* where remote is skip_unavailable: true and temporarily down, and the local data produces several composite pages — the "Cluster "remote" ... data is missing" warning is emitted once per page. Consider collecting per-cluster warnings into a Set / deduping before pushing so the message stays clean.
There was a problem hiding this comment.
Good catch on the raw duplication, but the user-visible part is already covered. The rule execution status message is built via truncateList(warnings) in the execution log client, which runs uniq() before take(20), so repeated cluster warnings collapse into one and can't crowd out other warnings like the max signals one.
What remains is the per-message ruleExecutionLogger.warn call in the rule type wrapper producing one Kibana log line and one event-log entry per repeat. That's not specific to threshold rules: the query rule search_after loop and both New Terms loops push searchWarnings per page the same way, and errors have the same behavior. The wrapper already has a note about it and it's tracked in #261333, where the fix belongs at the logger level rather than in each rule type. Leaving this as is here.
💛 Build succeeded, but was flaky
Failed CI Steps
Metrics [docs]
Test Failures
History
cc @maximpn |
|
Starting backport for target branches: 8.19, 9.4, 9.5 |
💔 All backports failed
Manual backportTo create the backport manually run: Questions ?Please refer to the Backport tool documentation |
5 participants
Closes #290840
Closes #289108
Summary
This PR makes Security Solution detection rules report an actionable warning when the events search resolves to zero shards, instead of failing every run with the generic
expected to find aggregations on search resultorAggregations were missing on ... search resulterror. Zero shards happens when the rule owner can see the source indices but cannot read them, for example a role withview_index_metadatabut noreadprivilege, or a cross-project search space whose linked projects silently exclude indices the owner cannot read. Shard and per-cluster failures that arrive without anaggregationsobject are now surfaced as the rule error instead of being hidden behind the generic message, and classified as user errors so they no longer count against the framework SLO.Details
Pre-execution validation matches indices through field capabilities, which only needs
view_index_metadata, while the events search needsread. When the owner lacksread, Elasticsearch expands the pattern to nothing and returns_shards.total: 0, no failures, and noaggregationsobject. Cross-project search behaves the same way by design: permissions are evaluated per project and unauthorized indices are excluded without an error. The custom query with alert suppression, New Terms, and Threshold rule types treated a missingaggregationsobject as a bug and threw, so the real cause never reached the rule execution log, and the failure was counted as a framework error. The plain custom query path succeeded silently with zero events.Touched layers, all in the Security Solution detection engine rule types:
utils/no_readable_shards.tswithhasZeroShards,getNoReadableShardsWarning, andreportMissingAggregations. Shard or cluster failures fail the run withuserErrorderived fromcheckErrorDetails, a zero-shard response adds a warning that names the index patterns and, when CPS is in scope, the linked project aliases together with the fix (grantreadin the linked projects or re-save the rule as a user who has it), and a response with searched shards but noaggregationsobject still throws, since that is a genuine bug.singleSearchAfternow merges_clusters.details[*].failuresintosearchErrorsthrough the newcreateErrorsFromClustershelper inutils/utils.ts, deduplicated against_shards.failures, and reports skipped or failed clusters that carry no failures. Previously only the ES|QL path read_clusters.SecuritySharedParamsgainscpsData, passed fromcreateSecurityRuleTypeWrapper, so warnings can name the linked projects recorded on the rule run.group_and_bulk_create.ts(custom query with alert suppression),create_new_terms_alert_type.tsandmulti_terms_composite.ts(New Terms, five throw sites),find_threshold_signals.tsandthreshold.ts(Threshold, two throw sites, newcpsLinkedProjectsparameter), andsearch_after_bulk_create_factory.ts(plain custom query, saved query, and suppression fallback paths), which now warns and stops on zero shards instead of succeeding silently.checkErrorDetailsclassifiessecurity_exceptionas a user error, which is what a linked project returns when it rejects the rule owner's search.Unit tests cover the new helper, the cluster failure parsing, the suppression path (zero shards, shard failures, genuine missing aggregations), the plain query path, Threshold, and the
security_exceptionclassification. A new Scout API suite undertest/scout/detection_engine/api/tests/rule_execution/no_readable_source_indices.spec.tsruns a custom query, custom query with alert suppression, Threshold, and New Terms rule as an owner withview_index_metadataonly and asserts apartial failurestatus carrying the new message. It is tagged for stateful classic and serverless security complete.How to test
scout-no-read-000001with a@timestampdate field and ahost.namekeyword field, and index one document.view_index_metadataonly onscout-no-read-*,allon.alerts-security*,.internal.alerts-security*,.siem-signals-*,.lists*,.items*, and full Kibana access. Create a user with that role and log in as that user.host.nameoverscout-no-read-*. Without this PR the rule status isFailedwithexpected to find aggregations on search result. With this PR the status isWarningand the execution log explains that the events search returned no shards for the pattern and that the rule owner lacks thereadprivilege.host.name, and a New Terms rule onhost.name. All four end inWarningwith the same message.readonscout-no-read-*to the role and run the rule again. The status turns toSucceededand alerts are created.read, create the rule in the origin project, and confirm the warning names the linked project aliases.Identify risks
partial failurefor the plain custom query path, where it used to end insucceededwith zero events. This is intentional, since the rule cannot produce alerts in that state, but rule health dashboards will show these rules as warning instead of green.security_exceptionis now a user error for all rule types usingcheckErrorDetails. This also covers an invalidated rule API key, which is still an owner-side condition, but it removes those failures from the framework SLO.expand_wildcardsdifferences could produce the same shape and would now get this warning instead of the generic error, which is still more accurate than before.🤖 Generated with Claude Code