[8.x] [Http] Gracefully onboarding internal routes to `Versioned…

…Router` (elastic#194789) (elastic#194902) # Backport This will backport the following commits from `main` to `8.x`: - [[Http] Gracefully onboarding internal routes to `VersionedRouter` (elastic#194789)](elastic#194789)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Jean-Louis Leysens <[email protected]>
kibanamachine · Oct 4, 2024 · bc43469 · bc43469
1 parent 18b2b5c
commit bc43469
Show file tree

Hide file tree

Showing 5 changed files with 84 additions and 31 deletions.
diff --git a/...e/http/core-http-router-server-internal/src/versioned_router/core_versioned_route.test.ts b/...e/http/core-http-router-server-internal/src/versioned_router/core_versioned_route.test.ts
@@ -548,7 +548,7 @@ describe('Versioned route', () => {
     expect(
       // @ts-expect-error
       versionedRoute.getSecurity({
-        headers: {},
+        headers: { [ELASTIC_HTTP_VERSION_HEADER]: '99' },
       })
     ).toStrictEqual(securityConfigDefault);
 
@@ -569,7 +569,7 @@ describe('Versioned route', () => {
     expect(
       // @ts-expect-error
       versionedRoute.getSecurity({
-        headers: {},
+        headers: { [ELASTIC_HTTP_VERSION_HEADER]: '99' },
       })
     ).toStrictEqual(securityConfigDefault);
     expect(router.get).toHaveBeenCalledTimes(1);

diff --git a/...s/core/http/core-http-router-server-internal/src/versioned_router/core_versioned_route.ts b/...s/core/http/core-http-router-server-internal/src/versioned_router/core_versioned_route.ts
@@ -25,10 +25,10 @@ import type {
   RouteConfigOptions,
   RouteSecurityGetter,
   RouteSecurity,
+  RouteMethod,
 } from '@kbn/core-http-server';
 import type { Mutable } from 'utility-types';
-import type { Method, VersionedRouterRoute } from './types';
-import type { CoreVersionedRouter } from './core_versioned_router';
+import type { HandlerResolutionStrategy, Method, VersionedRouterRoute } from './types';
 
 import { validate } from './validate';
 import {
@@ -44,9 +44,16 @@ import { validRouteSecurity } from '../security_route_config_validator';
 import { resolvers } from './handler_resolvers';
 import { prepareVersionedRouteValidation, unwrapVersionedResponseBodyValidation } from './util';
 import type { RequestLike } from './route_version_utils';
+import { Router } from '../router';
 
 type Options = AddVersionOpts<unknown, unknown, unknown>;
 
+interface InternalVersionedRouteConfig<M extends RouteMethod> extends VersionedRouteConfig<M> {
+  isDev: boolean;
+  useVersionResolutionStrategyForInternalPaths: Map<string, boolean>;
+  defaultHandlerResolutionStrategy: HandlerResolutionStrategy;
+}
+
 // This validation is a pass-through so that we can apply our version-specific validation later
 export const passThroughValidation = {
   body: schema.nullable(schema.any()),
@@ -75,29 +82,43 @@ export class CoreVersionedRoute implements VersionedRoute {
     path,
     options,
   }: {
-    router: CoreVersionedRouter;
+    router: Router;
     method: Method;
     path: string;
-    options: VersionedRouteConfig<Method>;
+    options: InternalVersionedRouteConfig<Method>;
   }) {
     return new CoreVersionedRoute(router, method, path, options);
   }
 
+  public readonly options: VersionedRouteConfig<Method>;
+
   private useDefaultStrategyForPath: boolean;
   private isPublic: boolean;
+  private isDev: boolean;
   private enableQueryVersion: boolean;
   private defaultSecurityConfig: RouteSecurity | undefined;
+  private defaultHandlerResolutionStrategy: HandlerResolutionStrategy;
   private constructor(
-    private readonly router: CoreVersionedRouter,
+    private readonly router: Router,
     public readonly method: Method,
     public readonly path: string,
-    public readonly options: VersionedRouteConfig<Method>
+    internalOptions: InternalVersionedRouteConfig<Method>
   ) {
-    this.useDefaultStrategyForPath = router.useVersionResolutionStrategyForInternalPaths.has(path);
-    this.isPublic = this.options.access === 'public';
-    this.enableQueryVersion = this.options.enableQueryVersion === true;
-    this.defaultSecurityConfig = validRouteSecurity(this.options.security, this.options.options);
-    this.router.router[this.method](
+    const {
+      isDev,
+      useVersionResolutionStrategyForInternalPaths,
+      defaultHandlerResolutionStrategy,
+      ...options
+    } = internalOptions;
+    this.isPublic = options.access === 'public';
+    this.isDev = isDev;
+    this.defaultHandlerResolutionStrategy = defaultHandlerResolutionStrategy;
+    this.useDefaultStrategyForPath =
+      this.isPublic || useVersionResolutionStrategyForInternalPaths.has(path);
+    this.enableQueryVersion = options.enableQueryVersion === true;
+    this.defaultSecurityConfig = validRouteSecurity(options.security, options.options);
+    this.options = options;
+    this.router[this.method](
       {
         path: this.path,
         validate: passThroughValidation,
@@ -119,7 +140,7 @@ export class CoreVersionedRoute implements VersionedRoute {
 
   /** This method assumes that one or more versions handlers are registered  */
   private getDefaultVersion(): undefined | ApiVersion {
-    return resolvers[this.router.defaultHandlerResolutionStrategy](
+    return resolvers[this.defaultHandlerResolutionStrategy](
       [...this.handlers.keys()],
       this.options.access
     );
@@ -132,8 +153,14 @@ export class CoreVersionedRoute implements VersionedRoute {
   private getVersion(req: RequestLike): ApiVersion | undefined {
     let version;
     const maybeVersion = readVersion(req, this.enableQueryVersion);
-    if (!maybeVersion && (this.isPublic || this.useDefaultStrategyForPath)) {
-      version = this.getDefaultVersion();
+    if (!maybeVersion) {
+      if (this.useDefaultStrategyForPath) {
+        version = this.getDefaultVersion();
+      } else if (!this.isDev && !this.isPublic) {
+        // When in production, we default internal routes to v1 to allow
+        // gracefully onboarding of un-versioned to versioned routes
+        version = '1';
+      }
     } else {
       version = maybeVersion;
     }
@@ -207,7 +234,7 @@ export class CoreVersionedRoute implements VersionedRoute {
 
     const response = await handler.fn(ctx, req, res);
 
-    if (this.router.isDev && validation?.response?.[response.status]?.body) {
+    if (this.isDev && validation?.response?.[response.status]?.body) {
       const { [response.status]: responseValidation, unsafe } = validation.response;
       try {
         validate(
@@ -236,7 +263,7 @@ export class CoreVersionedRoute implements VersionedRoute {
   private validateVersion(version: string) {
     // We do an additional check here while we only have a single allowed public version
     // for all public Kibana HTTP APIs
-    if (this.router.isDev && this.isPublic) {
+    if (this.isDev && this.isPublic) {
       const message = isAllowedPublicVersion(version);
       if (message) {
         throw new Error(message);

diff --git a/.../core/http/core-http-router-server-internal/src/versioned_router/core_versioned_router.ts b/.../core/http/core-http-router-server-internal/src/versioned_router/core_versioned_router.ts
@@ -74,10 +74,16 @@ export class CoreVersionedRouter implements VersionedRouter {
     (routeMethod: Method) =>
     (options: VersionedRouteConfig<Method>): VersionedRoute<Method, any> => {
       const route = CoreVersionedRoute.from({
-        router: this,
+        router: this.router,
         method: routeMethod,
         path: options.path,
-        options,
+        options: {
+          ...options,
+          defaultHandlerResolutionStrategy: this.defaultHandlerResolutionStrategy,
+          useVersionResolutionStrategyForInternalPaths:
+            this.useVersionResolutionStrategyForInternalPaths,
+          isDev: this.isDev,
+        },
       });
       this.routes.add(route);
       return route;

diff --git a/src/core/server/integration_tests/http/versioned_router.test.ts b/src/core/server/integration_tests/http/versioned_router.test.ts
@@ -293,38 +293,46 @@ describe('Routing versioned requests', () => {
     ).resolves.toEqual('1');
   });
 
-  it('requires version headers to be set for internal HTTP APIs', async () => {
+  it('defaults to v1 for internal HTTP APIs to allow gracefully onboarding internal routes to versioned router', async () => {
     await setupServer({ dev: false });
 
     router.versioned
-      .get({ path: '/my-path', access: 'internal' })
+      .get({ path: '/my-internal-path', access: 'internal' })
       .addVersion(
         { version: '1', validate: { response: { 200: { body: () => schema.number() } } } },
-        async (ctx, req, res) => res.ok()
+        async (ctx, req, res) => res.ok({ body: 'v1' })
       )
       .addVersion(
         { version: '2', validate: { response: { 200: { body: () => schema.number() } } } },
-        async (ctx, req, res) => res.ok()
+        async (ctx, req, res) => res.ok({ body: 'v2' })
       );
     await server.start();
 
     await expect(
       supertest
-        .get('/my-path')
+        .get('/my-internal-path')
         .unset('Elastic-Api-Version')
-        .expect(400)
-        .then(({ body }) => body.message)
-    ).resolves.toMatch(/Please specify.+version/);
+        .expect(200)
+        .then(({ text }) => text)
+    ).resolves.toMatch('v1');
   });
 
-  it('requires version headers to be set for public endpoints when in dev', async () => {
+  it('requires version headers to be set for internal and public endpoints when in dev', async () => {
     await setupServer({ dev: true });
     router.versioned
       .get({
         path: '/my-path',
         access: 'public',
       })
       .addVersion({ version: '2023-10-31', validate: false }, async (ctx, req, res) => res.ok());
+
+    router.versioned
+      .get({
+        path: '/my-internal-path',
+        access: 'internal',
+      })
+      .addVersion({ version: '1', validate: false }, async (ctx, req, res) => res.ok());
+
     await server.start();
 
     await expect(
@@ -334,6 +342,18 @@ describe('Routing versioned requests', () => {
         .expect(400)
         .then(({ body }) => body.message)
     ).resolves.toMatch(/Please specify.+version/);
+
+    await supertest.get('/my-path').set('Elastic-Api-Version', '2023-10-31').expect(200);
+
+    await expect(
+      supertest
+        .get('/my-internal-path')
+        .unset('Elastic-Api-Version')
+        .expect(400)
+        .then(({ body }) => body.message)
+    ).resolves.toMatch(/Please specify.+version/);
+
+    await supertest.get('/my-internal-path').set('Elastic-Api-Version', '1').expect(200);
   });
 
   it('errors when no handler could be found', async () => {

diff --git a/x-pack/test/kubernetes_security/basic/tests/aggregate.ts b/x-pack/test/kubernetes_security/basic/tests/aggregate.ts
@@ -122,13 +122,13 @@ export default function aggregateTests({ getService }: FtrProviderContext) {
     });
 
     it(`${AGGREGATE_ROUTE} handles a bad request`, async () => {
-      const response = await supertest.get(AGGREGATE_ROUTE).set('kbn-xsrf', 'foo').query({
+      const response = await getRoute().query({
         query: 'asdf',
         groupBy: ORCHESTRATOR_NAMESPACE,
         page: 0,
         index: MOCK_INDEX,
       });
-      expect(response.status).to.be(400);
+      expect(response.status).to.be(500);
     });
   });
 }