From 3a53d1ebe27de6f76071a65c3f92cbdd83bde798 Mon Sep 17 00:00:00 2001 From: Dario Gieselaar Date: Tue, 10 Dec 2019 22:38:41 +0100 Subject: [PATCH] [APM] Address fallout from NP server migration (#51725) (#52116) * [APM] Address fallout from NP server migration * Fix type issues in integration tests * Await creation of agent configuration index --- .../apm/public/services/rest/callApi.ts | 13 ++---- .../plugins/apm/public/services/rest/ml.ts | 4 +- .../server/routes/create_api/index.test.ts | 6 +-- .../apm/server/routes/create_api/index.ts | 39 +++++++++++------ x-pack/plugins/apm/server/plugin.ts | 21 +++++---- .../apis/apm/agent_configuration.ts | 43 ++++++++++++++++++- .../apis/apm/feature_controls.ts | 5 ++- 7 files changed, 92 insertions(+), 39 deletions(-) diff --git a/x-pack/legacy/plugins/apm/public/services/rest/callApi.ts b/x-pack/legacy/plugins/apm/public/services/rest/callApi.ts index 887200bdfc22a7..853ba5023f6fd1 100644 --- a/x-pack/legacy/plugins/apm/public/services/rest/callApi.ts +++ b/x-pack/legacy/plugins/apm/public/services/rest/callApi.ts @@ -21,18 +21,11 @@ function fetchOptionsWithDebug(fetchOptions: FetchOptions) { sessionStorage.getItem('apm_debug') === 'true' && startsWith(fetchOptions.pathname, '/api/apm'); - const isGet = !fetchOptions.method || fetchOptions.method === 'GET'; - - // Need an empty body to pass route validation - const body = isGet - ? {} - : { - body: JSON.stringify(fetchOptions.body || {}) - }; + const { body, ...rest } = fetchOptions; return { - ...fetchOptions, - ...body, + ...rest, + ...(body !== undefined ? { body: JSON.stringify(body) } : {}), query: { ...fetchOptions.query, ...(debugEnabled ? { _debug: true } : {}) diff --git a/x-pack/legacy/plugins/apm/public/services/rest/ml.ts b/x-pack/legacy/plugins/apm/public/services/rest/ml.ts index 28b14fa722bbd4..e495a8968a7f37 100644 --- a/x-pack/legacy/plugins/apm/public/services/rest/ml.ts +++ b/x-pack/legacy/plugins/apm/public/services/rest/ml.ts @@ -61,7 +61,7 @@ export async function startMLJob({ return callApi(http, { method: 'POST', pathname: `/api/ml/modules/setup/apm_transaction`, - body: JSON.stringify({ + body: { prefix: getMlPrefix(serviceName, transactionType), groups, indexPatternName: transactionIndices, @@ -71,7 +71,7 @@ export async function startMLJob({ filter } } - }) + } }); } diff --git a/x-pack/legacy/plugins/apm/server/routes/create_api/index.test.ts b/x-pack/legacy/plugins/apm/server/routes/create_api/index.test.ts index 3a74c4377920aa..309e503c4a4975 100644 --- a/x-pack/legacy/plugins/apm/server/routes/create_api/index.test.ts +++ b/x-pack/legacy/plugins/apm/server/routes/create_api/index.test.ts @@ -131,7 +131,7 @@ describe('createApi', () => { // stub default values params: {}, query: {}, - body: {}, + body: null, ...requestMock }, responseMock @@ -144,7 +144,7 @@ describe('createApi', () => { it('adds a _debug query parameter by default', async () => { const { simulate, handlerMock, responseMock } = initApi({}); - await simulate({ query: { _debug: true } }); + await simulate({ query: { _debug: 'true' } }); expect(handlerMock).toHaveBeenCalledTimes(1); @@ -288,7 +288,7 @@ describe('createApi', () => { await simulate({ query: { bar: '', - _debug: true + _debug: 'true' } }); diff --git a/x-pack/legacy/plugins/apm/server/routes/create_api/index.ts b/x-pack/legacy/plugins/apm/server/routes/create_api/index.ts index 2bbd8b6ddfb621..3e97d851acd296 100644 --- a/x-pack/legacy/plugins/apm/server/routes/create_api/index.ts +++ b/x-pack/legacy/plugins/apm/server/routes/create_api/index.ts @@ -18,8 +18,9 @@ import { Route, Params } from '../typings'; +import { jsonRt } from '../../../common/runtime_types/json_rt'; -const debugRt = t.partial({ _debug: t.boolean }); +const debugRt = t.partial({ _debug: jsonRt.pipe(t.boolean) }); export function createApi() { const factoryFns: Array> = []; @@ -53,28 +54,37 @@ export function createApi() { | 'get' | 'delete'; - const bodyRt = params.body; - const fallbackBodyRt = bodyRt || t.strict({}); + // For all runtime types with props, we create an exact + // version that will strip all keys that are unvalidated. + + const bodyRt = + params.body && 'props' in params.body + ? t.exact(params.body) + : params.body; const rts = { - // add _debug query parameter to all routes + // Add _debug query parameter to all routes query: params.query ? t.exact(t.intersection([params.query, debugRt])) : t.exact(debugRt), path: params.path ? t.exact(params.path) : t.strict({}), - body: bodyRt && 'props' in bodyRt ? t.exact(bodyRt) : fallbackBodyRt + body: bodyRt || t.null }; + const anyObject = schema.object({}, { allowUnknowns: true }); + (router[routerMethod] as RouteRegistrar)( { path, options, validate: { - ...(routerMethod === 'get' - ? {} - : { body: schema.object({}, { allowUnknowns: true }) }), - params: schema.object({}, { allowUnknowns: true }), - query: schema.object({}, { allowUnknowns: true }) + // `body` can be null, but `validate` expects non-nullable types + // if any validation is defined. Not having validation currently + // means we don't get the payload. See + // https://github.com/elastic/kibana/issues/50179 + body: schema.nullable(anyObject) as typeof anyObject, + params: anyObject, + query: anyObject } }, async (context, request, response) => { @@ -83,7 +93,7 @@ export function createApi() { path: request.params, body: request.body, query: { - _debug: false, + _debug: 'false', ...request.query } }; @@ -100,6 +110,9 @@ export function createApi() { throw Boom.badRequest(PathReporter.report(result)[0]); } + // `io-ts` has stripped unvalidated keys, so we can compare + // the output with the input to see if all object keys are + // known and validated. const strippedKeys = difference( Object.keys(value || {}), Object.keys(result.right || {}) @@ -124,7 +137,9 @@ export function createApi() { context: { ...context, __LEGACY, - // only return values for parameters that have runtime types + // Only return values for parameters that have runtime types, + // but always include query as _debug is always set even if + // it's not defined in the route. params: pick(parsedParams, ...Object.keys(params), 'query'), config, logger diff --git a/x-pack/plugins/apm/server/plugin.ts b/x-pack/plugins/apm/server/plugin.ts index b28e00adcc6d18..00945e12db51d9 100644 --- a/x-pack/plugins/apm/server/plugin.ts +++ b/x-pack/plugins/apm/server/plugin.ts @@ -29,7 +29,7 @@ export class APMPlugin implements Plugin { this.legacySetup$ = new AsyncSubject(); } - public setup( + public async setup( core: CoreSetup, plugins: { apm_oss: APMOSSPlugin extends Plugin ? TSetup : never; @@ -46,14 +46,17 @@ export class APMPlugin implements Plugin { createApmApi().init(core, { config$: mergedConfig$, logger, __LEGACY }); }); - combineLatest(mergedConfig$, core.elasticsearch.dataClient$).subscribe( - ([config, dataClient]) => { - createApmAgentConfigurationIndex({ - esClient: dataClient, - config, - }); - } - ); + await new Promise(resolve => { + combineLatest(mergedConfig$, core.elasticsearch.dataClient$).subscribe( + async ([config, dataClient]) => { + await createApmAgentConfigurationIndex({ + esClient: dataClient, + config, + }); + resolve(); + } + ); + }); return { config$: mergedConfig$, diff --git a/x-pack/test/api_integration/apis/apm/agent_configuration.ts b/x-pack/test/api_integration/apis/apm/agent_configuration.ts index 43ba8616e6872f..12e08869fa5861 100644 --- a/x-pack/test/api_integration/apis/apm/agent_configuration.ts +++ b/x-pack/test/api_integration/apis/apm/agent_configuration.ts @@ -32,17 +32,56 @@ export default function agentConfigurationTests({ getService }: FtrProviderConte function deleteCreatedConfigurations() { const promises = Promise.all(createdConfigIds.map(deleteConfiguration)); - createdConfigIds = []; return promises; } function deleteConfiguration(configurationId: string) { return supertest .delete(`/api/apm/settings/agent-configuration/${configurationId}`) - .set('kbn-xsrf', 'foo'); + .set('kbn-xsrf', 'foo') + .then((response: any) => { + createdConfigIds = createdConfigIds.filter(id => id === configurationId); + return response; + }); } describe('agent configuration', () => { + describe('when creating one configuration', () => { + let createdConfigId: string; + + const parameters = { + service: { name: 'myservice', environment: 'development' }, + etag: '7312bdcc34999629a3d39df24ed9b2a7553c0c39', + }; + + before(async () => { + log.debug('creating agent configuration'); + + // all / all + const { body } = await createConfiguration({ + service: {}, + settings: { transaction_sample_rate: 0.1 }, + }); + + createdConfigId = body._id; + }); + + it('returns the created configuration', async () => { + const { statusCode, body } = await searchConfigurations(parameters); + + expect(statusCode).to.equal(200); + expect(body._id).to.equal(createdConfigId); + }); + + it('succesfully deletes the configuration', async () => { + await deleteConfiguration(createdConfigId); + + const { statusCode } = await searchConfigurations(parameters); + + expect(statusCode).to.equal(404); + }); + }); + describe('when creating four configurations', () => { before(async () => { log.debug('creating agent configuration'); diff --git a/x-pack/test/api_integration/apis/apm/feature_controls.ts b/x-pack/test/api_integration/apis/apm/feature_controls.ts index 7fe0fda27294d5..839d4ff420a76c 100644 --- a/x-pack/test/api_integration/apis/apm/feature_controls.ts +++ b/x-pack/test/api_integration/apis/apm/feature_controls.ts @@ -38,7 +38,10 @@ export default function featureControlsTests({ getService }: FtrProviderContext) } const endpoints: Endpoint[] = [ { - req: { url: `/api/apm/services/foo/errors?start=${start}&end=${end}&uiFilters=%7B%7D` }, + // this doubles as a smoke test for the _debug query parameter + req: { + url: `/api/apm/services/foo/errors?start=${start}&end=${end}&uiFilters=%7B%7D_debug=true`, + }, expectForbidden: expect404, expectResponse: expect200, },