[CPS] Per-client CPS routing via OnRequestHandlerFactory#254186
Conversation
|
Pinging @elastic/kibana-core (Team:Core) |
1 similar comment
|
Pinging @elastic/kibana-core (Team:Core) |
| /** | ||
| * The behavior for automatically injecting 'project_routing' into requests. | ||
| */ | ||
| searchRouting: 'origin-only' | 'space-default' | 'all'; |
There was a problem hiding this comment.
What is the value of separating space-default and all? I guess the benefit is you can create a client that will always search with _alias:* by default?
Reason I'm asking is bc we were originally thinking of origin-only or auto where auto would take care of using space default when appropriate but implies possible linked project routing. It feels like space-default is the new auto.
There was a problem hiding this comment.
Exactly, it gives you more flexibility.
If you think it can be confusing for devs I can get rid of it and keep space-default for now.
Wrt the naming, auto seems too much of a black box IMO, whereas space-default hints users that the routing strategy is configurable per-space.
There was a problem hiding this comment.
Got it, how about origin-only, space or all, weakly held opinion.
Any way we go I think the public interface should document the tradeoffs very clearly. Assuming developers have no idea what CPS even is 😃
We should also document the behaviour of internal users in our public APIs.
jloleysens
left a comment
There was a problem hiding this comment.
Nice work @gsoldevila overall looks good to me! Left a couple of qs I'd like to get your thoughts on before approving.
| this.onRequest = onRequest; | ||
| this.onRequestHandlerFactory = onRequestHandlerFactory; | ||
|
|
||
| const internalUserOnRequest = onRequestHandlerFactory({ searchRouting: 'origin-only' }); |
There was a problem hiding this comment.
Fmi, this means .asScoped().asInternalUser will default to _alias:_origin? Sounds good 👍🏻
There was a problem hiding this comment.
Yes, I basically made the onRequest handler configurable instead of opinionated.
ffd1b2a to
358dd6a
Compare
358dd6a to
ffdcb04
Compare
⏳ Build in-progress, with failures
Failed CI StepsHistory
|
rudolf
left a comment
There was a problem hiding this comment.
Left some nits but I think we can discuss if we want to do these as follow-up, don't think it's worth blocking over it.
| }); | ||
|
|
||
| it('passes `onRequest` handler to `createTransport`', () => { | ||
| it('calls onRequestHandlerFactory with null request for asInternalUser', () => { |
There was a problem hiding this comment.
nit: these tests assert a lot of the wiring of the requestHandlerFactory and createTransport which tie the tests to the architecture. I think it might be simpler and less fragile to test the behavior "a space scoped client injects space default npre into project_routing param" or "a child client created from a space scoped client injects space default npre into project_routing param"
There was a problem hiding this comment.
Addressed in a follow-up PR:
https://github.com/elastic/kibana/pull/254531/changes
Could you PTAL?
Introduce `OnRequestHandlerFactory` to `ClusterClient` so each client (internal user, scoped) builds its own `OnRequestHandler` on demand. `CpsRequestHandler` becomes a factory (`createHandler(projectRouting)`) rather than exposing a pre-built handler. `ElasticsearchService` owns all CPS routing logic and wires the factory. Constants are centralised in `@kbn/cps-server-utils`. Co-authored-by: Cursor <cursoragent@cursor.com>
- Convert CpsRequestHandler class to a simple getCpsRequestHandler function accepting cpsEnabled and projectRouting directly - Introduce cps_request_handler_factory.ts with getRequestHandlerFactory, which encapsulates all routing-mode logic and returns an OnRequestHandlerFactory - Remove this.cpsRequestHandler instance from ElasticsearchService; setup() now delegates to getRequestHandlerFactory(cpsEnabled) in a single line - Update tests accordingly, adding a dedicated test file for the factory Co-authored-by: Cursor <cursoragent@cursor.com>
- `getSpaceNPRE` now accepts `string | KibanaRequest` only (FakeRequest is no longer a valid input). A defensive runtime throw is raised when a KibanaRequest without a `url` is passed, guarding against JavaScript callers bypassing the type system. - `OnRequestHandlerFactory` opts use a discriminated union: passing a KibanaRequest as `searchRouting` signals space-default routing, while the string literals 'origin-only' and 'all' cover the remaining modes. This removes the need for a separate `request` field and makes FakeRequest structurally invalid for space-default routing. - `kbn-cps-server-utils` tsconfig updated: `@kbn/core-elasticsearch-server` replaced by `@kbn/core-http-server`. - Tests and README updated accordingly. Co-authored-by: Cursor <cursoragent@cursor.com>
… create_transport tests When onRequest was made mandatory in configure_client.ts and create_transport.ts, the corresponding test files were not updated. Added the required onRequest mock to all affected call sites and updated the stale assertions that expected `onRequest: undefined`. Co-authored-by: Cursor <cursoragent@cursor.com>
Pass the required onRequest handler (strip-only, cpsEnabled=false) to the three callers that were missed when onRequest became mandatory in configureClient: the model-version test kit, the migrator test kit, and the user-agent integration test. Co-authored-by: Cursor <cursoragent@cursor.com>
- Rename searchRouting option 'space-default' to 'space'
- Rename scopeable_request.ts to types.ts and add new request types:
- UrlRequest: a minimal FakeRequest with url: URL, for space-level CPS
routing in non-HTTP contexts (background tasks, scheduled jobs)
- ScopeableUrlRequest: KibanaRequest | UrlRequest, the accepted type
for asScoped when searchRouting: 'space'
- Add typed AsScopedOptions specializations with self-documenting names:
- OriginOnlyRouting (searchRouting: 'origin-only')
- SpaceNPRE (searchRouting: 'space')
- AllProjectsRouting (searchRouting: 'all')
- Overload IClusterClient.asScoped and ClusterClient.asScoped to
enforce that a ScopeableUrlRequest is passed when using SpaceNPRE
- Broaden getSpaceNPRE to accept string | { url: URL } instead of
string | KibanaRequest, removing the KibanaRequest coupling
- Export all new public types from the package root
- Improve TSDoc across all affected types and interfaces
Co-authored-by: Cursor <cursoragent@cursor.com>
Renames the `searchRouting` field in `AsScopedOptions` and all related types, factory parameters, and destructuring sites to `projectRouting`, which more accurately reflects that this controls Elasticsearch `project_routing` injection rather than anything search-specific. Affected: AsScopedOptions, OriginOnlyRouting, SpaceNPRERouting, AllProjectsRouting, OnRequestHandlerFactory, ClusterClient.asScoped, getRequestHandlerFactory, and associated tests. Co-authored-by: Cursor <cursoragent@cursor.com>
- Initialize onRequestHandlerFactory in the ElasticsearchService constructor (non-CPS mode) so that clients created during preboot (before setup() runs) are correctly configured - Update getSpaceNPRE test to match the updated error message Co-authored-by: Cursor <cursoragent@cursor.com>
7b97226 to
af1a2c0
Compare
…nt.ts Co-authored-by: Rudolf Meijering <skaapgif@gmail.com>
jloleysens
left a comment
There was a problem hiding this comment.
Thank you for addressing my feedback! Let's see how projectRouting goes! Nice work @gsoldevila
💛 Build succeeded, but was flaky
Failed CI Steps
Test FailuresMetrics [docs]Public APIs missing comments
Public APIs missing exports
Unknown metric groupsAPI count
History
|
) Part of elastic/kibana-team#2829 ## Summary Builds on elastic#254085 (`@kbn/cps-server-utils`) to allow callers of `ClusterClient.asScoped()` to control how the CPS `project_routing` header is injected on a per-client basis. ### `asScoped` options `ClusterClient.asScoped(request, opts?)` now accepts a typed `opts` argument. Three named specializations of `AsScopedOptions` are provided, each narrowing the `projectRouting` field: - `OriginOnlyRouting` (`projectRouting: 'origin-only'`, default) — preserves the existing behavior, injects `_alias:_origin`. - `AllProjectsRouting` (`projectRouting: 'all'`) — injects `_alias:*`, routing across all CPS-connected projects. - `SpaceNPRERouting` (`projectRouting: 'space'`) — derives the NPRE from the request URL and injects `kibana_space_${spaceId}_default`. Using `SpaceNPRERouting` requires passing a `ScopeableUrlRequest` (`KibanaRequest | UrlRequest`) as the first argument. This is enforced by an overload on `asScoped`, so passing an incompatible `FakeRequest` (without a `url`) is a compile-time error. ### New request types - `UrlRequest` — a minimal `FakeRequest` with a `url: URL` property, for use in non-HTTP contexts (e.g. background tasks) where no real `KibanaRequest` is available but space-level routing is needed. - `ScopeableUrlRequest` — `KibanaRequest | UrlRequest`, the type accepted by the `SpaceNPRERouting` overload of `asScoped`. Route handlers can pass their incoming `KibanaRequest` directly. ### `OnRequestHandlerFactory` A mandatory factory (`OnRequestHandlerFactory`) is passed to the `ClusterClient` constructor instead of a bare `OnRequestHandler`. The factory is called lazily per scope, producing the right handler based on the routing mode. The `projectRouting` field is a discriminated union: a `ScopeableUrlRequest` signals space routing, while `'origin-only'` and `'all'` cover the remaining modes. ### `getSpaceNPRE` in `@kbn/cps-server-utils` Broadened to accept `string | { url: URL }` instead of `string | KibanaRequest`, removing the coupling to `KibanaRequest`. Both `KibanaRequest` and `UrlRequest` satisfy this structural type. A defensive runtime guard is retained (with a documented `@throws`) to protect against JavaScript callers bypassing the type system. ### Known limitation `getSpaceNPRE` assumes the server base path is `/`. If Kibana were deployed with a custom `server.basePath` the space segment would not be stripped before matching, and the function would always resolve to the default space. CPS is a Serverless-only feature and Serverless deployments always run at the root path, so this is not a concern in practice today. This assumption is documented in the JSDoc. ### CLI setup client `cli_setup`'s `ClusterClient` is on-prem only; it uses `getRequestHandlerFactory(false)` (CPS disabled), which strips any `project_routing` header rather than no-oping. Made with [Cursor](https://cursor.com) --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Rudolf Meijering <skaapgif@gmail.com>
#254531) Follows up on #254186. ## Summary Replaces the four wiring-focused tests in `ClusterClient` that asserted on internal mock call arguments (factory invocation, `createTransport` arguments) with behavioral tests that directly verify the `project_routing` value injected or stripped on synthetic ES request params. **New test structure:** - `#asInternalUser > CPS routing` — verifies that `configureClient` receives an origin-only `onRequest` handler and that invoking it sets `project_routing: '_alias:_origin'`. - `#asScoped().asCurrentUser > CPS routing` — three routing-mode tests (`'space'`, `'origin-only'`, `'all'`) using `captureTransportOnRequest()` to exercise the real factory end-to-end via `createTransport`. - `#asScoped().asSecondaryAuthUser > CPS routing` — verifies that `asSecondaryAuthUser` always delegates to `asInternalUser.child()` without a `Transport` override, inheriting origin-only routing regardless of the `projectRouting` option. - `without CPS (project_routing stripping)` — four tests asserting that `nonCpsRequestHandlerFactory` strips pre-existing `project_routing` from both `asInternalUser` and `asScoped` requests and never injects it. Also fixes the mock setup: replaces `mockReturnValue(wrappedFactory)` with `mockImplementation(getRequestHandlerFactory(...))` so the spy delegates to the real factory and produces correctly-typed `OnRequestHandler` instances per routing mode. Adds a `TODO` in `ClusterClient.asScoped` documenting the known limitation that callers overriding `Transport` via `child()` bypass the `onRequest` routing handler. Made with [Cursor](https://cursor.com) --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
elastic#254531) Follows up on elastic#254186. ## Summary Replaces the four wiring-focused tests in `ClusterClient` that asserted on internal mock call arguments (factory invocation, `createTransport` arguments) with behavioral tests that directly verify the `project_routing` value injected or stripped on synthetic ES request params. **New test structure:** - `#asInternalUser > CPS routing` — verifies that `configureClient` receives an origin-only `onRequest` handler and that invoking it sets `project_routing: '_alias:_origin'`. - `#asScoped().asCurrentUser > CPS routing` — three routing-mode tests (`'space'`, `'origin-only'`, `'all'`) using `captureTransportOnRequest()` to exercise the real factory end-to-end via `createTransport`. - `#asScoped().asSecondaryAuthUser > CPS routing` — verifies that `asSecondaryAuthUser` always delegates to `asInternalUser.child()` without a `Transport` override, inheriting origin-only routing regardless of the `projectRouting` option. - `without CPS (project_routing stripping)` — four tests asserting that `nonCpsRequestHandlerFactory` strips pre-existing `project_routing` from both `asInternalUser` and `asScoped` requests and never injects it. Also fixes the mock setup: replaces `mockReturnValue(wrappedFactory)` with `mockImplementation(getRequestHandlerFactory(...))` so the spy delegates to the real factory and produces correctly-typed `OnRequestHandler` instances per routing mode. Adds a `TODO` in `ClusterClient.asScoped` documenting the known limitation that callers overriding `Transport` via `child()` bypass the `onRequest` routing handler. Made with [Cursor](https://cursor.com) --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Part of https://github.com/elastic/kibana-team/issues/2829
Summary
Builds on #254085 (
@kbn/cps-server-utils) to allow callers ofClusterClient.asScoped()to control how the CPSproject_routingheader is injected on a per-client basis.asScopedoptionsClusterClient.asScoped(request, opts?)now accepts a typedoptsargument. Three named specializations ofAsScopedOptionsare provided, each narrowing theprojectRoutingfield:OriginOnlyRouting(projectRouting: 'origin-only', default) — preserves the existing behavior, injects_alias:_origin.AllProjectsRouting(projectRouting: 'all') — injects_alias:*, routing across all CPS-connected projects.SpaceNPRERouting(projectRouting: 'space') — derives the NPRE from the request URL and injectskibana_space_${spaceId}_default.Using
SpaceNPRERoutingrequires passing aScopeableUrlRequest(KibanaRequest | UrlRequest) as the first argument. This is enforced by an overload onasScoped, so passing an incompatibleFakeRequest(without aurl) is a compile-time error.New request types
UrlRequest— a minimalFakeRequestwith aurl: URLproperty, for use in non-HTTP contexts (e.g. background tasks) where no realKibanaRequestis available but space-level routing is needed.ScopeableUrlRequest—KibanaRequest | UrlRequest, the type accepted by theSpaceNPRERoutingoverload ofasScoped. Route handlers can pass their incomingKibanaRequestdirectly.OnRequestHandlerFactoryA mandatory factory (
OnRequestHandlerFactory) is passed to theClusterClientconstructor instead of a bareOnRequestHandler. The factory is called lazily per scope, producing the right handler based on the routing mode. TheprojectRoutingfield is a discriminated union: aScopeableUrlRequestsignals space routing, while'origin-only'and'all'cover the remaining modes.getSpaceNPREin@kbn/cps-server-utilsBroadened to accept
string | { url: URL }instead ofstring | KibanaRequest, removing the coupling toKibanaRequest. BothKibanaRequestandUrlRequestsatisfy this structural type. A defensive runtime guard is retained (with a documented@throws) to protect against JavaScript callers bypassing the type system.Known limitation
getSpaceNPREassumes the server base path is/. If Kibana were deployed with a customserver.basePaththe space segment would not be stripped before matching, and the function would always resolve to the default space. CPS is a Serverless-only feature and Serverless deployments always run at the root path, so this is not a concern in practice today. This assumption is documented in the JSDoc.CLI setup client
cli_setup'sClusterClientis on-prem only; it usesgetRequestHandlerFactory(false)(CPS disabled), which strips anyproject_routingheader rather than no-oping.Made with Cursor