[Http] Gracefully onboarding internal routes to VersionedRouter (#1…

…94789)

## Summary

If an internal route goes from unversioned -> versioned today this will
surface as a breaking change. This PR allows internal routes to
gracefully onboard to the versioned router.

The fix is to default to version `1` of an internal route if no version
is provided. In this way old clients can continue to interact with the
new servers as developers onboard to the versioned router and roll out
v2+ of an internal route.

## Notes

We could use `buildNr` to narrow down internal routes defaulting to v1
to older clients only. IMO this would be more accurate, but also of
marginal value. My rationale is: by requiring explicit versions during
dev time the risk of us shipping new builds that don't provide a version
is effectively nullified. Additionally we already run this risk with our
public route default behaviour (we even require that public routes have
explicit version in dev to mitigate this same risk for existing public
clients).

If reviewers feel otherwise I'm happy to revisit this!

### Checklist

Delete any items that are not applicable to this PR.

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios


### Risk Matrix

| Risk | Probability | Severity | Mitigation/Notes |

|---------------------------|-------------|----------|-------------------------|
| By defaulting internal routes to v1 it's possible that behaviour
becomes less predictable. | Low | Low | This is already true for public
routes and we are allowing a more limited/more predictable version of
that here. |

packages/core/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);

packages/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);

packages/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;

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 () => {

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);
     });
   });
 }