Skip to content

Commit

Permalink
[Config Service] Use stripUnknownKeys when checking enabled flags (#…
Browse files Browse the repository at this point in the history
…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
  • Loading branch information
afharo authored Dec 2, 2024
1 parent 5731fb3 commit 3e1d62e
Show file tree
Hide file tree
Showing 3 changed files with 103 additions and 7 deletions.
35 changes: 35 additions & 0 deletions packages/kbn-config-schema/src/types/object_type.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down
31 changes: 31 additions & 0 deletions packages/kbn-config/src/config_service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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: {
Expand Down
44 changes: 37 additions & 7 deletions packages/kbn-config/src/config_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -325,7 +346,13 @@ export class ConfigService {
});
}

private validateAtPath(path: ConfigPath, config: Record<string, unknown>) {
private validateAtPath(
path: ConfigPath,
config: Record<string, unknown>,
validateOptions?: { stripUnknownKeys?: boolean }
) {
const stripUnknownKeys = validateOptions?.stripUnknownKeys || this.stripUnknownKeys;

const namespace = pathToString(path);
const schema = this.schemas.get(namespace);
if (!schema) {
Expand All @@ -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 }))
);
}

Expand Down

0 comments on commit 3e1d62e

Please sign in to comment.