[Config Service] Use stripUnknownKeys when checking `enabled` flags #201579

afharo · 2024-11-25T12:54:40Z

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

Unit or functional tests were updated or added to match the most common scenarios

afharo · 2024-11-25T12:56:08Z

packages/kbn-config-schema/src/types/object_type.test.ts

proving that stripUnknownKeys works as intended

afharo · 2024-11-25T13:00:13Z

packages/kbn-config/src/config_service.ts


    const validatedConfig = hasSchema
-      ? await this.atPath<{ enabled?: boolean }>(path).pipe(first()).toPromise()
+      ? await firstValueFrom(


I took the opportunity to change the deprecated usage of .toPromise()

afharo · 2024-11-25T14:49:38Z

packages/kbn-config/src/config_service.ts

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


I had to add this special use case since stripUnknownKeys effectively breaks it 😿

This makes sense, although it's possible that the validation error may contain more than just "enabled"... perhaps we can catch and throw a more specific message here about the special "enabled" setting?

Great point! I created #202490 for us to discuss the new potential message.

elasticmachine · 2024-11-25T17:59:01Z

Pinging @elastic/kibana-core (Team:Core)

jloleysens

Overall lgtm, left a comment about a test and an error message. Let me know what you think! Non blockers.

jloleysens · 2024-11-27T07:31:59Z

packages/kbn-config/src/config_service.ts

+        )
      : undefined;

+    // Special use case: when the provided config includes `enabled` and the validated config doesn't,


This is only at the top level of the object?

Also could we add a test case for this?

This is the existing test that reminded me that I would introduce a regression with simply using stripUnknownKeys: true above:

kibana/packages/kbn-config/src/config_service.test.ts

Lines 352 to 374 in 3daaaa5

test('throws if reading "enabled" when it is not present in the schema', async () => {

const initialConfig = {

foo: {

enabled: false,

},

};

const rawConfigProvider = createRawConfigServiceMock({ rawConfig: initialConfig });

const configService = new ConfigService(rawConfigProvider, defaultEnv, logger);

configService.setSchema(

'foo',

schema.object({

bar: schema.maybe(schema.string()),

})

);

await expect(

async () => await configService.isEnabledAtPath('foo')

).rejects.toThrowErrorMatchingInlineSnapshot(

`"[config validation of [foo].enabled]: definition for this key is missing"`

);

});

jloleysens · 2024-11-27T07:40:11Z

packages/kbn-config/src/config_service.ts

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


This makes sense, although it's possible that the validation error may contain more than just "enabled"... perhaps we can catch and throw a more specific message here about the special "enabled" setting?

afharo · 2024-12-02T14:49:05Z

This feature is Serverless-only. However, I chose to backport to prev-minor to avoid harder conflicts in the future.

elasticmachine · 2024-12-02T16:30:27Z

💚 Build Succeeded

Buildkite Build
Commit: c5b2da7

Metrics [docs]

✅ unchanged

History

💚 Build #254047 succeeded 156ed57
💔 Build #253996 failed e7c0bd7

cc @afharo

kibanamachine · 2024-12-02T16:30:28Z

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/12123693009

…lastic#201579) ## Summary Resolves elastic#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 (cherry picked from commit 3e1d62e)

kibanamachine · 2024-12-02T16:35:18Z

💚 All backports created successfully

Status	Branch	Result
✅	8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…ed` flags (#201579) (#202531) # Backport This will backport the following commits from `main` to `8.x`: - [[Config Service] Use stripUnknownKeys when checking `enabled` flags (#201579)](#201579)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Alejandro Fernández Haro <afharo@gmail.com>

…lastic#201579) ## Summary Resolves elastic#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

afharo added Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// release_note:skip Skip the PR/issue when compiling release notes Feature:Configuration Settings in kibana.yml backport:skip This PR does not require backporting labels Nov 25, 2024

afharo self-assigned this Nov 25, 2024

afharo force-pushed the config/strip-unknown-on-serverless branch from 3e45864 to b852da7 Compare November 25, 2024 12:55

[Config Service] Use stripUnknownKeys when checking enabled flags

e7c0bd7

afharo force-pushed the config/strip-unknown-on-serverless branch from b852da7 to e7c0bd7 Compare November 25, 2024 12:59

afharo commented Nov 25, 2024

View reviewed changes

Cover "unsupported enabled config" use case

156ed57

afharo commented Nov 25, 2024

View reviewed changes

afharo marked this pull request as ready for review November 25, 2024 17:58

afharo requested a review from a team as a code owner November 25, 2024 17:58

jloleysens approved these changes Nov 27, 2024

View reviewed changes

afharo mentioned this pull request Dec 2, 2024

[Config Service] Human-friendly error message when config has enabled flag but it's not in the schema #202490

Open

Merge branch 'main' into config/strip-unknown-on-serverless

c5b2da7

afharo enabled auto-merge (squash) December 2, 2024 14:48

afharo added backport:prev-minor and removed backport:skip This PR does not require backporting labels Dec 2, 2024

afharo merged commit 3e1d62e into elastic:main Dec 2, 2024
12 checks passed

afharo deleted the config/strip-unknown-on-serverless branch December 2, 2024 16:30

kibanamachine added the v9.0.0 label Dec 2, 2024

kibanamachine mentioned this pull request Dec 2, 2024

[8.x] [Config Service] Use stripUnknownKeys when checking `enabled` flags (#201579) #202531

Merged

kibanamachine added the v8.18.0 label Dec 2, 2024

hammad-nasir-elastic mentioned this pull request Oct 31, 2025

updating error message when config has enabled flag not in schema #241365

Open

	test('throws if reading "enabled" when it is not present in the schema', async () => {
	const initialConfig = {
	foo: {
	enabled: false,
	},
	};

	const rawConfigProvider = createRawConfigServiceMock({ rawConfig: initialConfig });
	const configService = new ConfigService(rawConfigProvider, defaultEnv, logger);

	configService.setSchema(
	'foo',
	schema.object({
	bar: schema.maybe(schema.string()),
	})
	);

	await expect(
	async () => await configService.isEnabledAtPath('foo')
	).rejects.toThrowErrorMatchingInlineSnapshot(
	`"[config validation of [foo].enabled]: definition for this key is missing"`
	);
	});

[Config Service] Use stripUnknownKeys when checking enabled flags #201579

[Config Service] Use stripUnknownKeys when checking enabled flags #201579

Uh oh!

Conversation

afharo commented Nov 25, 2024 • edited by kibanamachine Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

afharo Nov 25, 2024

Choose a reason for hiding this comment

Uh oh!

afharo Nov 25, 2024

Choose a reason for hiding this comment

Uh oh!

afharo Nov 25, 2024

Choose a reason for hiding this comment

Uh oh!

jloleysens Nov 27, 2024

Choose a reason for hiding this comment

Uh oh!

afharo Dec 2, 2024

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Nov 25, 2024

Uh oh!

jloleysens left a comment

Choose a reason for hiding this comment

Uh oh!

jloleysens Nov 27, 2024

Choose a reason for hiding this comment

Uh oh!

afharo Dec 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jloleysens Nov 27, 2024

Choose a reason for hiding this comment

Uh oh!

afharo commented Dec 2, 2024

Uh oh!

Uh oh!

elasticmachine commented Dec 2, 2024

💚 Build Succeeded

Metrics [docs]

History

Uh oh!

kibanamachine commented Dec 2, 2024

Uh oh!

kibanamachine commented Dec 2, 2024

💚 All backports created successfully

Questions ?

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[Config Service] Use stripUnknownKeys when checking `enabled` flags #201579

[Config Service] Use stripUnknownKeys when checking `enabled` flags #201579

afharo commented Nov 25, 2024 •

edited by kibanamachine

Loading

afharo Dec 2, 2024 •

edited

Loading