Skip to content

Commit

Permalink
[http] Fix running FTR tests locally (#161336)
Browse files Browse the repository at this point in the history
## Summary

Running FTR tests locally (not on CI) that make API requests to public
versioned Kibana endpoints is currently broken. This is because:

* we require version headers to be set for both internal and **public**
endpoints in **dev** by way of a runtime check (ensures our code is
locked to a version)
* the vast majority of FTR tests do not set these headers for talking to
public endpoints
* on CI, this is different as we run these tests against a distributable
build (i.e., non-dev)

This manifests locally as a 400 response. E.g. the current
api_integration tests for data views by running:

```
node scripts/functional_test_runner.js --config ./test/api_integration/config.js --grep 'index_pattern_crud'
```

---

There are a few ways to resolve this, this PR proposes that we:

* Keep FTR tests as they are (i.e., don't update all of them to set
headers when making requests to public versioned endpoints), this is a
useful way to exercise our intended end-user behaviour
* Make the version resolution behaviour not depend on `--dev`: rather
set the default resolution to `none` (new setting) when we run in
`--dev` so that it can be configured to behave as we want for tests.

In this way we keep the runtime check for its intended purpose and can
run our FTR tests "as end users" of our endpoints.

---

Close #161435
  • Loading branch information
jloleysens authored Jul 11, 2023
1 parent 15a86c3 commit bda0195
Show file tree
Hide file tree
Showing 10 changed files with 93 additions and 37 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

export { filterHeaders } from './src/headers';
export { Router, type RouterOptions } from './src/router';
export type { HandlerResolutionStrategy } from './src/versioned_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 @@ -124,10 +124,10 @@ export interface RouterOptions {
/** Whether we are running in development */
isDev?: boolean;
/**
* Which route resolution algo to use
* @default 'oldest'
* Which route resolution algo to use.
* @note default to "oldest", but when running in dev default to "none"
*/
versionedRouteResolution?: 'newest' | 'oldest';
versionedRouteResolution?: 'newest' | 'oldest' | 'none';
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,6 @@ export class CoreVersionedRoute implements VersionedRoute {
}

private isPublic: boolean;
private isInternal: boolean;
private enableQueryVersion: boolean;
private constructor(
private readonly router: CoreVersionedRouter,
Expand All @@ -81,7 +80,6 @@ export class CoreVersionedRoute implements VersionedRoute {
) {
this.isPublic = this.options.access === 'public';
this.enableQueryVersion = this.options.enableQueryVersion === true;
this.isInternal = !this.isPublic;
this.router.router[this.method](
{
path: this.path,
Expand All @@ -100,7 +98,7 @@ export class CoreVersionedRoute implements VersionedRoute {
}

/** This method assumes that one or more versions handlers are registered */
private getDefaultVersion(): ApiVersion {
private getDefaultVersion(): undefined | ApiVersion {
return resolvers[this.router.defaultHandlerResolutionStrategy]([...this.handlers.keys()]);
}

Expand All @@ -120,8 +118,15 @@ export class CoreVersionedRoute implements VersionedRoute {
});
}
const req = originalReq as Mutable<KibanaRequest>;
const requestVersion = readVersion(req, this.enableQueryVersion);
if (!requestVersion && !this.canUseDefaultVersion()) {
let version: undefined | ApiVersion;

const maybeVersion = readVersion(req, this.enableQueryVersion);
if (!maybeVersion && this.isPublic) {
version = this.getDefaultVersion();
} else {
version = maybeVersion;
}
if (!version) {
return res.badRequest({
body: `Please specify a version via ${ELASTIC_HTTP_VERSION_HEADER} header. Available versions: ${this.versionsToString()}`,
});
Expand All @@ -135,7 +140,6 @@ export class CoreVersionedRoute implements VersionedRoute {
body: `Use of query parameter "${ELASTIC_HTTP_VERSION_QUERY_PARAM}" is not allowed. Please specify the API version using the "${ELASTIC_HTTP_VERSION_HEADER}" header.`,
});
}
const version: ApiVersion = requestVersion ?? this.getDefaultVersion();

const invalidVersionMessage = isValidRouteVersion(this.isPublic, version);
if (invalidVersionMessage) {
Expand Down Expand Up @@ -198,10 +202,6 @@ export class CoreVersionedRoute implements VersionedRoute {
);
};

private canUseDefaultVersion(): boolean {
return !this.isInternal && !this.router.isDev;
}

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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,16 @@
* Assumes that there is at least one version in the array.
* @internal
*/
type Resolver = (versions: string[]) => string;
type Resolver = (versions: string[]) => undefined | string;

const oldest: Resolver = (versions) => [...versions].sort((a, b) => a.localeCompare(b))[0];

const newest: Resolver = (versions) => [...versions].sort((a, b) => b.localeCompare(a))[0];

const none: Resolver = () => undefined;

export const resolvers = {
oldest,
newest,
none,
};
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,4 @@
*/

export { CoreVersionedRouter } from './core_versioned_router';
export type { HandlerResolutionStrategy } from './types';
Original file line number Diff line number Diff line change
Expand Up @@ -42,4 +42,6 @@ export type HandlerResolutionStrategy =
/** Use the oldest available version by default */
| 'oldest'
/** Use the newest available version by default */
| 'newest';
| 'newest'
/** Dev-only: remove resolution and fail if no version is provided */
| 'none';
Original file line number Diff line number Diff line change
Expand Up @@ -486,6 +486,29 @@ describe('cors', () => {
});
});

describe('versioned', () => {
it('defaults version resolution "oldest" not in dev', () => {
expect(config.schema.validate({}, { dev: undefined })).toMatchObject({
versioned: { versionResolution: 'oldest' },
});
expect(config.schema.validate({}, { dev: false })).toMatchObject({
versioned: { versionResolution: 'oldest' },
});
});

it('does not allow "none" when not in dev', () => {
expect(() =>
config.schema.validate({ versioned: { versionResolution: 'none' } }, { dev: false })
).toThrow(/failed validation/);
});

it('defaults version resolution "none" when in dev', () => {
expect(config.schema.validate({}, { dev: true })).toMatchObject({
versioned: { versionResolution: 'none' },
});
});
});

describe('HttpConfig', () => {
it('converts customResponseHeaders to strings or arrays of strings', () => {
const httpSchema = config.schema;
Expand Down
35 changes: 29 additions & 6 deletions packages/core/http/core-http-server-internal/src/http_config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import url from 'url';

import type { Duration } from 'moment';
import { IHttpEluMonitorConfig } from '@kbn/core-http-server/src/elu_monitor';
import type { HandlerResolutionStrategy } from '@kbn/core-http-router-server-internal';
import { CspConfigType, CspConfig } from './csp';
import { ExternalUrlConfig } from './external_url';
import {
Expand Down Expand Up @@ -168,11 +169,30 @@ const configSchema = schema.object(
),
restrictInternalApis: schema.boolean({ defaultValue: false }), // allow access to internal routes by default to prevent breaking changes in current offerings
versioned: schema.object({
// Which handler resolution algo to use: "newest" or "oldest"
versionResolution: schema.oneOf([schema.literal('newest'), schema.literal('oldest')], {
defaultValue: 'oldest',
}),
// Whether we enforce version checks on client requests
/**
* Which handler resolution algo to use: "newest" or "oldest".
*
* @note in development we have an additional option "none" which is also the default.
* This prevents any fallbacks and requires that a version specified.
* Useful for ensuring that a given client always specifies a version.
*/
versionResolution: schema.conditional(
schema.contextRef('dev'),
true,
schema.oneOf([schema.literal('newest'), schema.literal('oldest'), schema.literal('none')], {
defaultValue: 'none',
}),
schema.oneOf([schema.literal('newest'), schema.literal('oldest')], {
defaultValue: 'oldest',
})
),

/**
* Whether we require the Kibana browser build version to match the Kibana server build version.
*
* This number is determined when a distributable version of Kibana is built and ensures that only
* same-build browsers can access the Kibana server.
*/
strictClientVersionCheck: schema.boolean({ defaultValue: true }),
}),
},
Expand Down Expand Up @@ -247,7 +267,10 @@ export class HttpConfig implements IHttpConfig {
public externalUrl: IExternalUrlConfig;
public xsrf: { disableProtection: boolean; allowlist: string[] };
public requestId: { allowFromAnyIp: boolean; ipAllowlist: string[] };
public versioned: { versionResolution: 'newest' | 'oldest'; strictClientVersionCheck: boolean };
public versioned: {
versionResolution: HandlerResolutionStrategy;
strictClientVersionCheck: boolean;
};
public shutdownTimeout: Duration;
public restrictInternalApis: boolean;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,16 @@ export async function startServers(log: ToolingLog, options: StartServerOptions)
procs,
config,
installDir: options.installDir,
extraKbnOpts: options.installDir ? [] : ['--dev', '--no-dev-config', '--no-dev-credentials'],
extraKbnOpts: options.installDir
? []
: [
'--dev',
'--no-dev-config',
'--no-dev-credentials',
config.get('serverless')
? '--server.versioned.versionResolution=newest'
: '--server.versioned.versionResolution=oldest',
],
});

reportTime(runStartTime, 'ready', {
Expand Down
24 changes: 9 additions & 15 deletions src/core/server/integration_tests/http/versioned_router.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import { loggingSystemMock } from '@kbn/core-logging-server-mocks';
import { executionContextServiceMock } from '@kbn/core-execution-context-server-mocks';
import { contextServiceMock } from '@kbn/core-http-context-server-mocks';
import { createHttpServer, createConfigService } from '@kbn/core-http-server-mocks';
import type { HttpService } from '@kbn/core-http-server-internal';
import type { HttpConfigType, HttpService } from '@kbn/core-http-server-internal';
import type { IRouter } from '@kbn/core-http-server';
import type { CliArgs } from '@kbn/config';
import { ELASTIC_HTTP_VERSION_QUERY_PARAM } from '@kbn/core-http-common';
Expand All @@ -30,20 +30,17 @@ describe('Routing versioned requests', () => {
async function setupServer(cliArgs: Partial<CliArgs> = {}) {
logger = loggingSystemMock.create();
await server?.stop(); // stop the already started server
const serverConfig: Partial<HttpConfigType> = {
versioned: {
versionResolution: cliArgs.dev ? 'none' : cliArgs.serverless ? 'newest' : 'oldest',
strictClientVersionCheck: !cliArgs.serverless,
},
};
server = createHttpServer({
logger,
env: createTestEnv({ envOptions: getEnvOptions({ cliArgs }) }),
configService: createConfigService({
// We manually sync the config in our mock at this point
server:
cliArgs.serverless === true
? {
versioned: {
versionResolution: 'newest',
strictClientVersionCheck: false,
},
}
: undefined,
server: serverConfig,
}),
});
await server.preboot({ context: contextServiceMock.createPrebootContract() });
Expand Down Expand Up @@ -316,10 +313,7 @@ describe('Routing versioned requests', () => {
path: '/my-path',
access: 'public',
})
.addVersion(
{ version: '2023-10-31', validate: { response: { 200: { body: schema.number() } } } },
async (ctx, req, res) => res.ok()
);
.addVersion({ version: '2023-10-31', validate: false }, async (ctx, req, res) => res.ok());
await server.start();

await expect(
Expand Down

0 comments on commit bda0195

Please sign in to comment.