[Core] Strip unsupported project_routing when CPS is enabled - #288212
Draft
wildemat wants to merge 1 commit into
Draft
[Core] Strip unsupported project_routing when CPS is enabled#288212wildemat wants to merge 1 commit into
wildemat wants to merge 1 commit into
Conversation
When CPS is enabled and the target ES API does not declare project_routing in its acceptedParams, a caller-supplied value was forwarded to Elasticsearch, producing a parsing_exception. Strip it, log a warning naming the API, and record the strip on the CPS request metric as kibana.cps.routing.unsupported_param_stripped. The strip is scoped to the api_does_not_support_routing bypass reason so that raw transport.request() callers, which have no acceptedParams metadata, keep their explicitly supplied routing. routingType and bypassReason are unchanged. Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Jobs for this PR can be triggered through checkboxes. 🚧
ℹ️ To trigger the CI, please tick the checkbox below 👇
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
getCpsRequestHandlerinjectsproject_routingfor the Elasticsearch APIs that declare it in the client'smeta.acceptedParams, and strips it everywhere when CPS is disabled. There was a gap in between: when CPS is enabled but the target API does not acceptproject_routing, the handler recordedroutingType: 'none'withbypassReason: 'api_does_not_support_routing'and then forwarded the caller's value to Elasticsearch unchanged. The ES JS client puts unrecognised parameters into the request body, so such a request is guaranteed to fail withparsing_exception: Unknown key for a VALUE_STRING in [project_routing]— or, for document APIs where the body is the document, to silently index a stray field.That is not hypothetical. In https://github.com/elastic/sdh-kibana/issues/6491, ML's "clear job notifications" passed
project_routingthrough toupdate_by_queryandindex, and users on serverless projects with CPS enabled got an HTTP 400. The plugin-side fix for that specific caller is #288210; this PR is the defence-in-depth change in Core so the same mistake from any other plugin cannot reach Elasticsearch.What changed. When CPS is enabled and the API does not accept
project_routing, a caller-supplied value is now stripped fromparams.body,params.querystringand the NDJSONparams.bulkBody; a warning naming the offending API is logged (only when something was actually removed, so the well-behaved majority of bypassed requests stay silent); and the strip is recorded on the CPS request metric. The package README gains a behaviour table covering every CPS-enabled case and a table mapping routing-context fields to metric attributes.Why the strip is scoped to
api_does_not_support_routingThis is the detail most worth a reviewer's attention. The strip is gated on the specific bypass reason rather than on the whole bypass branch, because that branch catches more than one situation.
Requests with no
meta.acceptedParamsat all also land there, and that is exactly what every rawtransport.request()caller looks like. Those callers may legitimately supplyproject_routing:src/core/server/integration_tests/elasticsearch/project_routing_serverless_cps.test.tshas a wholedescribe('project_routing using transport.request()')block plus an NDJSON_msearchcase that pass it explicitly against live CPS-enabled Elasticsearch, and ES accepts it. Stripping there would silently drop caller routing intent — a scoping change, not a bug fix. Legacy flat-arrayacceptedParamsis excluded for the same reason: it is not authoritative about what the API accepts.Gating on the structured
acceptedParamsform, which the client generates from the ES spec, confines the strip to the cases where the metadata actually is authoritative. Unit tests pin both exclusions (leaves project_routing untouched for raw transport callers without acceptedParams,leaves project_routing untouched for legacy flat-array acceptedParams).Metric
kibana.elasticsearch.cps.request.countgains one attribute,kibana.cps.routing.unsupported_param_stripped, attached unconditionally as a boolean. That matches the existing unconditional booleanskibana.cps.enabledandkibana.cps.routing.accepted, and it makes a stripped-over-total ratio computable — an attribute that is absent rather thanfalseon the negative case would make that ambiguous. The value is narrowed with=== trueat the emit site so a customTransportsupplying a routing context that predates the field still produces a boolean rather thanundefined. The same flag is added to the existing debug log line. No new time series are created; the existingapi_does_not_support_routingbucket simply splits in two.routingTypeandbypassReasonare deliberately unchanged, and it is worth saying why rather than leaving reviewers to wonder.bypass_reasonis only attached whenroutingType === 'none', so relabelling this case asroutingType: 'stripped'would drop the bypass reason for precisely the requests where the reason matters most. It would also conflate the case with the two existing meanings of'stripped'(CPS disabled, and PIT-scoped searches), and anything countingrouting.type=nonefor CPS coverage would start undercounting. An additive attribute splits the bucket without disturbing any of that.Naming
"unsupported" rather than "invalid": the parameter itself is perfectly valid, it is the API that does not support it, and the wording mirrors the existing
api_does_not_support_routingvocabulary. Easy to change if reviewers prefer otherwise — no data exists under either name yet.Testing
instrumentCpsMetricshad no test coverage at all before this. The PR addsconfigure_client_cps_metrics.test.ts(7 tests) covering the new attribute across routing types, theundefined-context fallback, the untouched existing attributes, and the debug log line.cps_request_handler.test.tsgains 13 tests for the strip itself, the warning, the unchanged bypass reason, and the two exclusions above.The new assertions were confirmed to fail against the unmodified implementation and pass with it. Full package suite is green: 14 suites, 272 tests, 61 snapshots.
node scripts/eslintandnode scripts/type_check --project src/core/packages/elasticsearch/client-server-internal/tsconfig.jsonboth clean.Observation, not addressed here
Whoever owns the CPS dashboards may want to know: the
event.kind: 'alert'condition on the debug log is driven byroutingType === 'none' && bypassReason, which means routine_bulkand_cat/indicescalls are already labelled'alert'. That predates this PR, it is at debug level, and narrowing it would change existing behaviour, so it is deliberately out of scope.Backport
None needed. CPS is serverless-only and this code exists only on
main.Checklist
Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support— n/a, the only added text is a server-side log message for developers, which is not translated.cps_request_handlerREADME documents the new behaviour and the routing-context-to-metric-attribute mapping.If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list— n/a, no configuration keys changed.This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The— n/a, no HTTP APIs are touched; the change is internal to the Elasticsearch client wrapper.release_note:breakinglabel should be applied in these situations.Flaky Test Runner was used on any tests changed— n/a, the changed tests are deterministic Jest unit tests with no browser or stack dependency.The PR description includes the appropriate Release Notes section, and the correct— n/a as far as I can tell: this is internal hardening with no user-facing behaviour change on its own, and the user-visible fix for the reported issue ships in [ML] Remove unsupported project_routing from clear job audit messages #288210. Happy to add a label if a reviewer disagrees.release_note:*label is applied per the guidelinesbackport:*labels — no backport needed, see above.Identify risks
bypassReason === 'api_does_not_support_routing'rather than on the bypass branch as a whole. If the gate were widened, rawtransport.request()callers that deliberately passproject_routingwould have it silently removed and would quietly query the wrong scope — a correctness bug with no error to surface it. Severity would be high; mitigation is that the gate depends on the structuredacceptedParamsthe client derives from the ES spec, and two unit tests fail if the exclusions for missing and legacy flat-arrayacceptedParamsare removed.parsing_exceptionnow succeed with the parameter removed, so a caller relying on the failure would see a different outcome. Severity is low — those requests were failing for users — but it does mean the fix can mask a plugin bug rather than surfacing it, which is why the strip logs awarnnaming the API and sets an attribute that can be alerted on rather than passing silently.api_does_not_support_routingbucket splits) androutingType/bypassReasonkeep their current meanings, so existing queries and dashboards continue to return the same numbers. Low severity, but worth a glance from anyone who has built on this counter.