Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Http] Router refactor #205502

Merged
merged 29 commits into from
Jan 15, 2025
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
34bb576
ee -> events
jloleysens Jan 2, 2025
f50d4cc
added link to router events interface
jloleysens Jan 2, 2025
495b9e4
export http protocol detection logic
jloleysens Jan 2, 2025
f5670a1
wip 1
jloleysens Jan 3, 2025
829f8c8
remove unused import
jloleysens Jan 3, 2025
14bdbcb
wip 2
jloleysens Jan 3, 2025
9b8e30a
some test fixes, mainly for http resources and returning the version …
jloleysens Jan 3, 2025
ebd8add
delete unused code
jloleysens Jan 3, 2025
4da954e
wiiip 3
jloleysens Jan 3, 2025
eec58bd
unrefactor minor test change
jloleysens Jan 3, 2025
b382675
added missing test coverage
jloleysens Jan 3, 2025
cdb39be
remove unnecessary code
jloleysens Jan 3, 2025
6c55327
minor tweaks
jloleysens Jan 3, 2025
43be1ad
remove unused import
jloleysens Jan 3, 2025
97d3375
remove unused type
jloleysens Jan 3, 2025
d6f5e4e
added comment
jloleysens Jan 3, 2025
427b619
remove unused generics
jloleysens Jan 3, 2025
0c4643c
fix types
jloleysens Jan 3, 2025
8a446af
Merge branch 'main' into http/refactor-versioned-router
jloleysens Jan 6, 2025
c96fd9b
fix validation behaviour on versioned route and added more test coverage
jloleysens Jan 6, 2025
532280b
fix passing through versioned deprecation meta info
jloleysens Jan 6, 2025
bb2bf16
Merge branch 'main' into http/refactor-versioned-router
jloleysens Jan 7, 2025
0829811
Merge branch 'main' into http/refactor-versioned-router
jloleysens Jan 7, 2025
ac9c4bb
Merge branch 'main' into http/refactor-versioned-router
jloleysens Jan 8, 2025
a4618b8
Merge branch 'main' into http/refactor-versioned-router
jloleysens Jan 9, 2025
2a23a05
Merge branch 'main' into http/refactor-versioned-router
jloleysens Jan 10, 2025
6010378
Address nit
jloleysens Jan 10, 2025
f71ab61
Merge branch 'main' into http/refactor-versioned-router
elasticmachine Jan 13, 2025
718d77b
Merge branch 'main' into http/refactor-versioned-router
jloleysens Jan 15, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ export {
type HandlerResolutionStrategy,
} from './src/versioned_router';
export { Router } from './src/router';
export type { RouterOptions, InternalRegistrar, InternalRegistrarOptions } from './src/router';
export type { RouterOptions } from './src/router';
export { isKibanaRequest, isRealRequest, ensureRawRequest, CoreKibanaRequest } from './src/request';
export { isSafeMethod } from './src/route';
export { HapiResponseAdapter } from './src/response_adapter';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ export class CoreKibanaRequest<
enumerable: false,
});

this.httpVersion = isRealReq ? request.raw.req.httpVersion : '1.0';
this.httpVersion = isRealReq ? getHttpVersionFromRequest(request) : '1.0';
this.apiVersion = undefined;
this.protocol = getProtocolFromHttpVersion(this.httpVersion);

Expand Down Expand Up @@ -424,3 +424,11 @@ function sanitizeRequest(req: Request): { query: unknown; params: unknown; body:
function getProtocolFromHttpVersion(httpVersion: string): HttpProtocol {
return httpVersion.split('.')[0] === '2' ? 'http2' : 'http1';
}

function getHttpVersionFromRequest(request: Request) {
return request.raw.req.httpVersion;
}

export function getProtocolFromRequest(request: Request) {
return getProtocolFromHttpVersion(getHttpVersionFromRequest(request));
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the "Elastic License
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
* Public License v 1"; you may not use this file except in compliance with, at
* your election, the "Elastic License 2.0", the "GNU Affero General Public
* License v3.0 only", or the "Server Side Public License, v 1".
*/

import { hapiMocks } from '@kbn/hapi-mocks';
import { validateHapiRequest } from './route';
import { createRouter } from './versioned_router/mocks';
import { loggingSystemMock } from '@kbn/core-logging-server-mocks';
import { Logger } from '@kbn/logging';
import { RouteValidator } from './validator';
import { schema } from '@kbn/config-schema';
import { Router } from './router';
import { RouteAccess } from '@kbn/core-http-server';

describe('validateHapiRequest', () => {
let router: Router;
let log: Logger;
beforeEach(() => {
router = createRouter();
log = loggingSystemMock.createLogger();
});
it('validates hapi requests and returns kibana requests: ok case', () => {
const { ok, error } = validateHapiRequest(hapiMocks.createRequest({ payload: { ok: true } }), {
log,
routeInfo: { access: 'public', httpResource: false },
router,
routeSchemas: RouteValidator.from({ body: schema.object({ ok: schema.literal(true) }) }),
});
expect(ok?.body).toEqual({ ok: true });
expect(error).toBeUndefined();
expect(log.error).not.toHaveBeenCalled();
});
it('validates hapi requests and returns kibana requests: error case', () => {
const { ok, error } = validateHapiRequest(hapiMocks.createRequest({ payload: { ok: false } }), {
log,
routeInfo: { access: 'public', httpResource: false },
router,
routeSchemas: RouteValidator.from({ body: schema.object({ ok: schema.literal(true) }) }),
});
expect(ok).toBeUndefined();
expect(error?.status).toEqual(400);
expect(error?.payload).toMatch(/expected value to equal/);
expect(log.error).toHaveBeenCalledTimes(1);
expect(log.error).toHaveBeenCalledWith('400 Bad Request', {
error: { message: '[request body.ok]: expected value to equal [true]' },
http: { request: { method: undefined, path: undefined }, response: { status_code: 400 } },
});
});

it('emits post validation events on the router', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is new test coverage

const deps = {
log,
routeInfo: { access: 'public' as RouteAccess, httpResource: false },
router,
routeSchemas: RouteValidator.from({ body: schema.object({ ok: schema.literal(true) }) }),
};
{
const { ok, error } = validateHapiRequest(
hapiMocks.createRequest({ payload: { ok: false } }),
deps
);
expect(ok).toBeUndefined();
expect(error).toBeDefined();
expect(router.emitPostValidate).toHaveBeenCalledTimes(1);
expect(router.emitPostValidate).toHaveBeenCalledWith(expect.any(Object), {
deprecated: undefined,
isInternalApiRequest: false,
isPublicAccess: true,
});
}
{
const { ok, error } = validateHapiRequest(
hapiMocks.createRequest({ payload: { ok: true } }),
deps
);
expect(ok).toBeDefined();
expect(error).toBeUndefined();
expect(router.emitPostValidate).toHaveBeenCalledTimes(2);
expect(router.emitPostValidate).toHaveBeenNthCalledWith(2, expect.any(Object), {
deprecated: undefined,
isInternalApiRequest: false,
isPublicAccess: true,
});
}
});
});
184 changes: 182 additions & 2 deletions packages/core/http/core-http-router-server-internal/src/route.ts
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,40 @@
* License v3.0 only", or the "Server Side Public License, v 1".
*/

import type { RouteMethod, SafeRouteMethod, RouteConfig } from '@kbn/core-http-server';
import type { RouteSecurityGetter, RouteSecurity } from '@kbn/core-http-server';
import {
type RouteMethod,
type SafeRouteMethod,
type RouteConfig,
getRequestValidation,
} from '@kbn/core-http-server';
import type {
RouteSecurityGetter,
RouteSecurity,
AnyKibanaRequest,
IKibanaResponse,
RouteAccess,
RouteConfigOptions,
} from '@kbn/core-http-server';
import { isConfigSchema } from '@kbn/config-schema';
import { isZod } from '@kbn/zod';
import type { Logger } from '@kbn/logging';
import type { DeepPartial } from '@kbn/utility-types';
import { Request } from '@hapi/hapi';
import { Mutable } from 'utility-types';
import type { InternalRouterRoute, RequestHandlerEnhanced, Router } from './router';
import { CoreKibanaRequest } from './request';
import { RouteValidator } from './validator';
import { BASE_PUBLIC_VERSION } from './versioned_router';
import { kibanaResponseFactory } from './response';
import {
getVersionHeader,
injectVersionHeader,
formatErrorMeta,
getRouteFullPath,
validOptions,
prepareRouteConfigValidation,
} from './util';
import { validRouteSecurity } from './security_route_config_validator';

export function isSafeMethod(method: RouteMethod): method is SafeRouteMethod {
return method === 'get' || method === 'options';
Expand All @@ -21,3 +53,151 @@ export type InternalRouteConfig<P, Q, B, M extends RouteMethod> = Omit<
> & {
security?: RouteSecurityGetter | RouteSecurity;
};

/** @internal */
interface Dependencies {
router: Router;
route: InternalRouteConfig<unknown, unknown, unknown, RouteMethod>;
handler: RequestHandlerEnhanced<unknown, unknown, unknown, RouteMethod>;
log: Logger;
method: RouteMethod;
}

export function buildRoute({
handler,
log,
route,
router,
method,
}: Dependencies): InternalRouterRoute {
route = prepareRouteConfigValidation(route);
const routeSchemas = routeSchemasFromRouteConfig(route, method);
return {
handler: async (req) => {
return await handle(req, {
handler,
log,
method,
route,
router,
routeSchemas,
});
},
method,
path: getRouteFullPath(router.routerPath, route.path),
options: validOptions(method, route),
security: validRouteSecurity(route.security as DeepPartial<RouteSecurity>, route.options),
validationSchemas: route.validate,
isVersioned: false,
};
}

/** @internal */
interface HandlerDependencies extends Dependencies {
routeSchemas?: RouteValidator<unknown, unknown, unknown>;
}

interface ValidationContext {
routeInfo: Pick<RouteConfigOptions<RouteMethod>, 'access' | 'httpResource'>;
router: Router;
log: Logger;
routeSchemas?: RouteValidator<unknown, unknown, unknown>;
version?: string;
}

/** @internal */
export function validateHapiRequest(
request: Request,
{ routeInfo, router, log, routeSchemas, version }: ValidationContext
): { ok: AnyKibanaRequest; error?: never } | { ok?: never; error: IKibanaResponse } {
let kibanaRequest: Mutable<AnyKibanaRequest>;
try {
kibanaRequest = CoreKibanaRequest.from(request, routeSchemas);
kibanaRequest.apiVersion = version;
} catch (error) {
log.error('400 Bad Request', formatErrorMeta(400, { request, error }));
const isPublicApi = isPublicAccessApiRoute(routeInfo);
// Emit onPostValidation even if validation fails.
const failedRequest: Mutable<AnyKibanaRequest> = CoreKibanaRequest.from(request);
router.emitPostValidate(failedRequest, {
deprecated: failedRequest.route.options.deprecated,
isInternalApiRequest: failedRequest.isInternalApiRequest,
isPublicAccess: isPublicApi,
});

const response = kibanaResponseFactory.badRequest({
body: error.message,
headers: isPublicApi ? getVersionHeader(BASE_PUBLIC_VERSION) : undefined,
});
return { error: response };
}

router.emitPostValidate(kibanaRequest, {
deprecated: kibanaRequest.route.options.deprecated,
isInternalApiRequest: kibanaRequest.isInternalApiRequest,
isPublicAccess: isPublicAccessApiRoute(routeInfo),
});
return { ok: kibanaRequest };
}

/** @internal */
export const handle = async (
request: Request,
{ router, route, handler, routeSchemas, log }: HandlerDependencies
) => {
const { error, ok: kibanaRequest } = validateHapiRequest(request, {
routeInfo: { access: route.options?.access, httpResource: route.options?.httpResource },
router,
log,
routeSchemas,
});
if (error) {
return error;
}
const kibanaResponse = await handler(kibanaRequest, kibanaResponseFactory);
if (isPublicAccessApiRoute(route.options)) {
injectVersionHeader(BASE_PUBLIC_VERSION, kibanaResponse);
}
return kibanaResponse;
};

function isPublicAccessApiRoute({
access,
httpResource,
}: {
access?: RouteAccess;
httpResource?: boolean;
} = {}): boolean {
return !httpResource && access === 'public';
}

/**
* Create the validation schemas for a route
*
* @returns Route schemas if `validate` is specified on the route, otherwise
* undefined.
*/
function routeSchemasFromRouteConfig<P, Q, B>(
route: InternalRouteConfig<P, Q, B, typeof routeMethod>,
routeMethod: RouteMethod
) {
// The type doesn't allow `validate` to be undefined, but it can still
// happen when it's used from JavaScript.
if (route.validate === undefined) {
throw new Error(
`The [${routeMethod}] at [${route.path}] does not have a 'validate' specified. Use 'false' as the value if you want to bypass validation.`
);
}

if (route.validate !== false) {
const validation = getRequestValidation(route.validate);
Object.entries(validation).forEach(([key, schema]) => {
if (!(isConfigSchema(schema) || isZod(schema) || typeof schema === 'function')) {
throw new Error(
`Expected a valid validation logic declared with '@kbn/config-schema' package, '@kbn/zod' package or a RouteValidationFunction at key: [${key}].`
);
}
});
return RouteValidator.from(validation);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -95,14 +95,12 @@ describe('Router', () => {
it('can exclude versioned routes', () => {
const router = new Router('', logger, enhanceWithContext, routerOptions);
const validation = schema.object({ foo: schema.string() });
router.post(
{
router.versioned
.post({
path: '/versioned',
validate: { body: validation, query: validation, params: validation },
},
(context, req, res) => res.ok(),
{ isVersioned: true, events: false }
);
access: 'internal',
})
.addVersion({ version: '999', validate: false }, async (ctx, req, res) => res.ok());
router.get(
{
path: '/unversioned',
Expand Down
Loading