[Http] Router refactor #205502

jloleysens · 2025-01-03T14:53:04Z

Summary

This refactor changes only internal implementation details. (Versioned) Router functionality should be unchanged.

The goals of this refactor:

(1) The versioned router uses the "external" facing IRouter APIs to register it's routes creating a situation where our internal router logic lives in the same conceptual space as consumer code. This is OK, but we had to have some weird dependency passing and pass-through validation to get this to work. This refactor basically removes that.

(2) Allow route registration to happen without specifying validation schemas. This required the route and versioned route to handle (hapi) Request objects, but ultimately gives more implementation freedom. The trade off is that there is potential for more code duplication, but I think with factored out functionality this can be well-tested and kept to an acceptable minimum. The current iteration actually improves our DRY metric slightly 😉. Achieved with new InternalRouterRoute that expects internal handlers to map (hapi)Request -> IKibanaResponse instead of CoreKibanaRequest -> IKibanaResponse allowing internal handlers more freedom for handling requests (like validation) while still giving the "top-level" internal handler ability to run some checks before mapping to HapiResponseObject.

(3) Reduce the size of the router.ts file. Quite a bit of implementation moved to route.ts or util.ts

(4) Add missing test coverage. We added Router.events a while ago to power detection of deprecated route usages and added a "post validation" request event that required updating the router. This refactor allows more functionality to be shared and better tested for both route and versioned route.

Risks

Moderate level of risk. This refactor moves around code and attempts to co-locate things that were separated. In the process subtle order-of-execution bugs (or omission) may creep in... Somewhat mitigated by tests.

…header

jloleysens · 2025-01-03T15:01:46Z

packages/core/http/core-http-router-server-internal/src/route.ts

99% of the added logic in this file comes from ./router.ts and aims to create simple functions to:

(1) create better reusability of these lower level functionalities between router and versioned router (specifically, creating KibanaRequests, validation and emitting of router events)
(2) slim down the size of the ./router.ts file

jloleysens · 2025-01-03T15:03:01Z

packages/core/http/core-http-router-server-internal/src/route.test.ts

+    });
+  });
+
+  it('emits post validation events on the router', () => {


This is new test coverage

jloleysens · 2025-01-03T15:05:17Z

packages/core/http/core-http-router-server-internal/src/util.ts

@@ -92,3 +95,76 @@ export function getVersionHeader(version: string): ResponseHeaders {
 export function injectVersionHeader(version: string, response: IKibanaResponse): IKibanaResponse {
  return injectResponseHeaders(getVersionHeader(version), response);
 }
+
+export function formatErrorMeta(


These functions were airlifted out of ./router.ts

jloleysens · 2025-01-03T15:06:38Z

...ages/core/http/core-http-router-server-internal/src/versioned_router/core_versioned_route.ts

-export const passThroughValidation = {
-  body: schema.nullable(schema.any()),
-  params: schema.nullable(schema.any()),
-  query: schema.nullable(schema.any()),
-};


This is an important change of this refactor, bc we have an InternalRouteHandler that allows for more distinct route vs versioned route implementations we can avoid the "handler in a handler" flow that existed before causing us to run validation twice (once pass through, once for real).

jloleysens · 2025-01-03T15:08:34Z

...ages/core/http/core-http-router-server-internal/src/versioned_router/core_versioned_route.ts

-        security: this.getSecurity,
-      },
-      this.requestHandler,
-      { isVersioned: true, events: false }


Another change: removal of opaque option events that created conditional flows behind indirection

jloleysens · 2025-01-03T15:09:19Z

...ages/core/http/core-http-router-server-internal/src/versioned_router/core_versioned_route.ts

-
-        this.router.emitPostValidate(req, postValidateMetadata);
-        return res.badRequest({ body: e.message, headers: getVersionHeader(version) });
+      const { error, ok: validatedRequest } = validateHapiRequest(hapiRequest, {


Use the same validation flow as route.ts

jloleysens · 2025-01-03T15:34:48Z

packages/core/http/core-http-router-server-internal/src/router.ts

-
-  return { ...options, body };
-}
+export type InternalRouterRoute = Omit<RouterRoute, 'handler'> & {


Sort of the main change of this refactor, see PR description (1) & (2)

jloleysens · 2025-01-03T15:53:10Z

/ci

jloleysens · 2025-01-06T13:11:08Z

/ci

TinaHeiligers

router refactor:

Nice work handling debt debt while allowing more freedom for internal route owners!

I agree that we'll have some duplication, and we'll keep an eye on it so that things don't stray too far or get out of hand.

I left a nit and comment, completely optional.

Overall, looks great!

src/core/packages/http/router-server-internal/src/util.ts

TinaHeiligers · 2025-01-09T23:02:27Z

src/core/packages/http/server/src/router/request.ts

@@ -214,3 +214,8 @@ export interface KibanaRequest<
   */
  readonly body: Body;
 }
+
+/**
+ * @remark Convenience type, use when the concrete values of P, Q, B and route method do not matter.


WDYT about adding a typical example of when concrete values don't matter? It might help prevent unintended use of AnyKibanaRequest

pgayvallet

Great to see this happening. I agree that it moves complexity around, and sometimes brings a bit of duplication in the logic between our 2 router impls, but the freedom it brings is worth it ihmo, as it makes sure that we won't be limited by our own public-facing router hapi when implementing internals ever again.

elena-shostak

Checked security config - LGTM 👍

Bamieh

LGTM! I paid special attention to the emitPostValidate and isVersioned changes since this is where some things diverged in the previous layered implementaiton.

Bamieh · 2025-01-13T14:38:49Z

@elasticmachine merge upstream

elasticmachine · 2025-01-15T14:09:47Z

💚 Build Succeeded

Buildkite Build
Commit: 718d77b

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`@kbn/core-http-server`	242	243	+1

Unknown metric groups

API count

id	before	after	diff
`@kbn/core-http-server`	567	568	+1

History

💔 Build #266553 failed f71ab61
💛 Build #265921 was flaky 6010378
💚 Build #265539 succeeded a4618b8
💔 Build #264933 failed 0829811
💛 Build #264810 was flaky bb2bf16

cc @jloleysens

kibanamachine · 2025-01-15T15:11:04Z

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/12791260143

kibanamachine · 2025-01-15T15:16:24Z

💔 All backports failed

Status	Branch	Result
❌	8.x	Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 205502

Questions ?

Please refer to the Backport tool documentation

kibanamachine · 2025-01-17T15:49:22Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 205502 locally

kibanamachine · 2025-01-20T16:48:57Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 205502 locally

kibanamachine · 2025-01-21T16:49:21Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 205502 locally

(cherry picked from commit ca77772) # Conflicts: # src/core/packages/http/router-server-internal/src/router.ts

jloleysens · 2025-01-22T14:01:01Z

💚 All backports created successfully

Status	Branch	Result
✅	8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

# Backport This will backport the following commits from `main` to `8.x`: - [[Http] Router refactor (#205502)](#205502)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport)

jloleysens added 10 commits January 2, 2025 14:12

ee -> events

34bb576

added link to router events interface

f50d4cc

export http protocol detection logic

495b9e4

wip 1

f5670a1

remove unused import

829f8c8

wip 2

14bdbcb

some test fixes, mainly for http resources and returning the version …

9b8e30a

…header

delete unused code

ebd8add

wiiip 3

4da954e

unrefactor minor test change

eec58bd

jloleysens added Feature:http Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc release_note:skip Skip the PR/issue when compiling release notes v9.0.0 backport:version Backport to applied version labels v8.18.0 labels Jan 3, 2025

jloleysens self-assigned this Jan 3, 2025

jloleysens added 7 commits January 3, 2025 15:57

added missing test coverage

b382675

remove unnecessary code

cdb39be

minor tweaks

6c55327

remove unused import

43be1ad

remove unused type

97d3375

added comment

d6f5e4e

remove unused generics

427b619

jloleysens commented Jan 3, 2025

View reviewed changes

fix types

0c4643c

jloleysens added 2 commits January 6, 2025 11:16

Merge branch 'main' into http/refactor-versioned-router

8a446af

fix validation behaviour on versioned route and added more test coverage

c96fd9b

jloleysens added 3 commits January 7, 2025 19:43

Merge branch 'main' into http/refactor-versioned-router

0829811

Merge branch 'main' into http/refactor-versioned-router

ac9c4bb

Merge branch 'main' into http/refactor-versioned-router

a4618b8

TinaHeiligers approved these changes Jan 9, 2025

View reviewed changes

Merge branch 'main' into http/refactor-versioned-router

2a23a05

jloleysens requested review from Bamieh, elena-shostak and pgayvallet January 10, 2025 10:17

Address nit

6010378

pgayvallet approved these changes Jan 13, 2025

View reviewed changes

elena-shostak approved these changes Jan 13, 2025

View reviewed changes

Bamieh approved these changes Jan 13, 2025

View reviewed changes

elasticmachine and others added 2 commits January 13, 2025 14:38

Merge branch 'main' into http/refactor-versioned-router

f71ab61

Merge branch 'main' into http/refactor-versioned-router

718d77b

jloleysens merged commit ca77772 into elastic:main Jan 15, 2025
8 checks passed

jloleysens deleted the http/refactor-versioned-router branch January 15, 2025 15:10

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jan 17, 2025

jloleysens added a commit to jloleysens/kibana that referenced this pull request Jan 22, 2025

[Http] Router refactor (elastic#205502)

24abc02

(cherry picked from commit ca77772) # Conflicts: # src/core/packages/http/router-server-internal/src/router.ts

jloleysens mentioned this pull request Jan 22, 2025

[8.x] [Http] Router refactor (#205502) #207828

Merged

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jan 22, 2025

viduni94 pushed a commit to viduni94/kibana that referenced this pull request Jan 23, 2025

[Http] Router refactor (elastic#205502)

41c3704

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Http] Router refactor #205502

[Http] Router refactor #205502

jloleysens commented Jan 3, 2025 •

edited by kibanamachine

Loading

jloleysens Jan 3, 2025

jloleysens Jan 3, 2025

jloleysens Jan 3, 2025

jloleysens Jan 3, 2025

jloleysens Jan 3, 2025

jloleysens Jan 3, 2025

jloleysens Jan 3, 2025

jloleysens commented Jan 3, 2025

jloleysens commented Jan 6, 2025

TinaHeiligers left a comment

TinaHeiligers Jan 9, 2025

pgayvallet left a comment

elena-shostak left a comment

Bamieh left a comment

Bamieh commented Jan 13, 2025

elasticmachine commented Jan 15, 2025

API count

kibanamachine commented Jan 15, 2025

kibanamachine commented Jan 15, 2025

kibanamachine commented Jan 17, 2025

kibanamachine commented Jan 20, 2025

kibanamachine commented Jan 21, 2025

jloleysens commented Jan 22, 2025

[Http] Router refactor #205502

[Http] Router refactor #205502

Conversation

jloleysens commented Jan 3, 2025 • edited by kibanamachine Loading

Summary

Risks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jloleysens commented Jan 3, 2025

jloleysens commented Jan 6, 2025

TinaHeiligers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgayvallet left a comment

Choose a reason for hiding this comment

elena-shostak left a comment

Choose a reason for hiding this comment

Bamieh left a comment

Choose a reason for hiding this comment

Bamieh commented Jan 13, 2025

elasticmachine commented Jan 15, 2025

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

API count

History

kibanamachine commented Jan 15, 2025

kibanamachine commented Jan 15, 2025

💔 All backports failed

Manual backport

Questions ?

kibanamachine commented Jan 17, 2025

kibanamachine commented Jan 20, 2025

kibanamachine commented Jan 21, 2025

jloleysens commented Jan 22, 2025

💚 All backports created successfully

Questions ?

jloleysens commented Jan 3, 2025 •

edited by kibanamachine

Loading