From 3e1d62ebc4530a1f9ebc05d9cbff858aa46ce438 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Fern=C3=A1ndez=20Haro?= Date: Mon, 2 Dec 2024 17:30:09 +0100 Subject: [PATCH] [Config Service] Use stripUnknownKeys when checking `enabled` flags (#201579) ## Summary Resolves #201442. The underlying issue is that `isEnabledAtPath` validates the entire config object when it only cares about `.enabled`. This PR performs that check using `stripUnknownKeys: true`, as we'll perform the actual validation later on. ### Checklist - [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 --- .../src/types/object_type.test.ts | 35 +++++++++++++++ .../kbn-config/src/config_service.test.ts | 31 +++++++++++++ packages/kbn-config/src/config_service.ts | 44 ++++++++++++++++--- 3 files changed, 103 insertions(+), 7 deletions(-) diff --git a/packages/kbn-config-schema/src/types/object_type.test.ts b/packages/kbn-config-schema/src/types/object_type.test.ts index aa3d362016f53..863d8e8a9ad14 100644 --- a/packages/kbn-config-schema/src/types/object_type.test.ts +++ b/packages/kbn-config-schema/src/types/object_type.test.ts @@ -517,6 +517,41 @@ describe('nested unknowns', () => { }, }); }); + + describe(`stripUnknownKeys: true in validate`, () => { + test('should strip unknown keys', () => { + const type = schema.object({ + myObj: schema.object({ + foo: schema.string({ defaultValue: 'test' }), + nested: schema.object({ + a: schema.number(), + }), + }), + }); + + expect( + type.validate( + { + myObj: { + bar: 'baz', + nested: { + a: 1, + b: 2, + }, + }, + }, + void 0, + void 0, + { stripUnknownKeys: true } + ) + ).toStrictEqual({ + myObj: { + foo: 'test', + nested: { a: 1 }, + }, + }); + }); + }); }); test('handles optional properties', () => { diff --git a/packages/kbn-config/src/config_service.test.ts b/packages/kbn-config/src/config_service.test.ts index 89ea4ee50b9ea..de78d96c5c4b0 100644 --- a/packages/kbn-config/src/config_service.test.ts +++ b/packages/kbn-config/src/config_service.test.ts @@ -313,6 +313,37 @@ test('handles disabled path and marks config as used', async () => { expect(unusedPaths).toEqual([]); }); +test('does not throw if extra options are provided', async () => { + const initialConfig = { + pid: { + enabled: false, + file: '/some/file.pid', + extraUnknownOption: 1, + extraNestedUnknownOptions: { + anOption: true, + anotherOption: 'something', + }, + }, + }; + + const rawConfigProvider = createRawConfigServiceMock({ rawConfig: initialConfig }); + const configService = new ConfigService(rawConfigProvider, defaultEnv, logger); + + configService.setSchema( + 'pid', + schema.object({ + enabled: schema.boolean({ defaultValue: false }), + file: schema.string(), + }) + ); + + const isEnabled = await configService.isEnabledAtPath('pid'); + expect(isEnabled).toBe(false); + + const unusedPaths = await configService.getUnusedPaths(); + expect(unusedPaths).toEqual([]); +}); + test('does not throw if schema does not define "enabled" schema', async () => { const initialConfig = { pid: { diff --git a/packages/kbn-config/src/config_service.ts b/packages/kbn-config/src/config_service.ts index 168de60460a1e..913f1ff032fd5 100644 --- a/packages/kbn-config/src/config_service.ts +++ b/packages/kbn-config/src/config_service.ts @@ -12,7 +12,7 @@ import { SchemaTypeError, Type, ValidationError } from '@kbn/config-schema'; import { cloneDeep, isEqual, merge, unset } from 'lodash'; import { set } from '@kbn/safer-lodash-set'; import { BehaviorSubject, combineLatest, firstValueFrom, Observable, identity } from 'rxjs'; -import { distinctUntilChanged, first, map, shareReplay, tap } from 'rxjs'; +import { distinctUntilChanged, map, shareReplay, tap } from 'rxjs'; import { Logger, LoggerFactory } from '@kbn/logging'; import { getDocLinks, DocLinks } from '@kbn/doc-links'; @@ -209,10 +209,31 @@ export class ConfigService { throw new Error(`No validation schema has been defined for [${namespace}]`); } - const validatedConfig = hasSchema - ? await this.atPath<{ enabled?: boolean }>(path).pipe(first()).toPromise() + let validatedConfig = hasSchema + ? await firstValueFrom( + this.getValidatedConfigAtPath$( + path, + // At this point we don't care about how valid the config is: we just want to read `enabled` + { stripUnknownKeys: true } + ) as Observable<{ enabled?: boolean }>, + { defaultValue: undefined } + ) : undefined; + // Special use case: when the provided config includes `enabled` and the validated config doesn't, + // it's quite likely that's not an allowed config and it should fail. + // Applying "normal" validation (not stripping unknowns) in that case. + if ( + hasSchema && + typeof config.get(path)?.enabled !== 'undefined' && + typeof validatedConfig?.enabled === 'undefined' + ) { + validatedConfig = await firstValueFrom( + this.getValidatedConfigAtPath$(path) as Observable<{ enabled?: boolean }>, + { defaultValue: undefined } + ); + } + const isDisabled = validatedConfig?.enabled === false; if (isDisabled) { // If the plugin is explicitly disabled, we mark the entire plugin @@ -325,7 +346,13 @@ export class ConfigService { }); } - private validateAtPath(path: ConfigPath, config: Record) { + private validateAtPath( + path: ConfigPath, + config: Record, + validateOptions?: { stripUnknownKeys?: boolean } + ) { + const stripUnknownKeys = validateOptions?.stripUnknownKeys || this.stripUnknownKeys; + const namespace = pathToString(path); const schema = this.schemas.get(namespace); if (!schema) { @@ -340,18 +367,21 @@ export class ConfigService { ...this.env.packageInfo, }, `config validation of [${namespace}]`, - this.stripUnknownKeys ? { stripUnknownKeys: this.stripUnknownKeys } : {} + stripUnknownKeys ? { stripUnknownKeys } : {} ); } private getValidatedConfigAtPath$( path: ConfigPath, - { ignoreUnchanged = true }: { ignoreUnchanged?: boolean } = {} + { + ignoreUnchanged = true, + stripUnknownKeys, + }: { ignoreUnchanged?: boolean; stripUnknownKeys?: boolean } = {} ) { return this.config$.pipe( map((config) => config.get(path)), ignoreUnchanged ? distinctUntilChanged(isEqual) : identity, - map((config) => this.validateAtPath(path, config)) + map((config) => this.validateAtPath(path, config, { stripUnknownKeys })) ); }