Implements config deprecation in New Platform #52251

pgayvallet · 2019-12-05T08:28:59Z

Summary

Implement configuration deprecation management in new platform. core and plugins can now register deprecations to be used to mutates the raw configuration (by renaming or removing deprecated properties)

Actual implementation

The legacy config deprecation system was chaotic in the sense that deprecations were actually applied in different points (and time) in the code. However, the underneath implementation and API was pretty clever and NP implementation is strongly inspired by it.

core can now register deprecation providers using configService.addDeprecationProvider. Plugins can register the same providers when declaring their configuration using their PluginConfigDescriptor

 const configSchema = schema.object({
   someNewKey: schema.string({ defaultValue: 'a string' }),
 });
 
 type ConfigType = TypeOf<typeof configSchema>;
 
 export const config: PluginConfigDescriptor<ConfigType> = {
   schema: configSchema,
   deprecations: ({ rename, unused }) => [
     rename('someOldKey', 'someNewKey'),
     unused('deprecatedProperty'),
   ],
 };

By default, deprecations can only access a plugin's configuration schema, meaning that rename('someOldKey', 'someNewKey'), will actually rename myplugin.someOldKey to myplugin.someNewKey. However due to legacy and edge case requirements, we also expose APIs to access the whole config (renameFromRoot and unusedFromRoot)

Deprecations are not limited to rename and unused (even if they cover 99% of the needs) and any custom implementation of ConfigDeprecation can be returned by a provider.

`RawConfigService` and `ConfigService` changes

To properly manage and apply deprecations to the configuration values, I had to change these two services responsibilities a little. Now the RawConfigService returns the configuration values in a raw, Record<string, any> format instead of returning an actual Config object. This allows to move all configuration data manipulation logic in a single place (the config service). This is probably more coherent regarding what we could expect of a Raw[..]-type service anyway.

The most important change regarding ConfigService is in the schema validation timing: because of some edge cases of deprecations, it's no longer possible to validates a plugin's schema when registering it. Practical exemple is a property that would have been migrated from one plugin to another (and we got some of these in the legacy deprecations, such as rename('xpack.telemetry.enabled', 'telemetry.enabled'),). For this reason, global config validation is now done at a single point of time, just after plugins discovery.

Legacy deprecations in new platform

As legacy and new deprecation system are pretty close, I was able to create an adapter from legacy deprecations to new ones. the LegacyService now collect every legacy plugin's deprecation providers, adapt them, and register them in the new platform. That way, the NP configuration is properly aware of legacy deprecations and expose up to date values.

What remains to be done

~~Totally removes the legacy deprecation system~~

UPDATE after discussion, this will be done after NP plugin migration, and is out of the scope of this PR

We are so close and yet so far. This impacts LegacyService and legacy code the most. Currently, the legacy service creates a LegacyConfig from the raw config's value, and calls findPluginSpecs with it. The config then enter in the legacy world/code, where discovered legacy plugins mutates the legacy configuration with both their schema, their value and their deprecations. This mutated legacy config is then considered the source of truth when we instanciate the legacy kibana server.

To be able to totally suppress the legacy deprecation system, we will need to remove the part where the deprecation are applied from legacy findPluginSpecs, get the non-migrated config back in legacyService, and then apply the new platform deprecation to it. However, all this code is strongly coupled atm, not typed (js, no ts), and lack test coverage, so I will need to be extremely careful with that part, and think that we might want to do it in a separate PR.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~- [ ] This was checked for cross-browser compatibility, including a check against IE11~~
~~- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
~~- [ ] This was checked for keyboard-only and screenreader accessibility~~

For maintainers

~~- [ ] This was checked for breaking API changes and was labeled appropriately~~

This includes a feature addition or change that requires a release note and was labeled appropriately

Dev Docs

New platform plugin's configuration now supports deprecation. Use the deprecations key of a plugin's config descriptor to register them.

 // my_plugin/server/index.ts
 import { schema, TypeOf } from '@kbn/config-schema';
 import { PluginConfigDescriptor } from 'kibana/server';
 
 const configSchema = schema.object({
   someNewKey: schema.string({ defaultValue: 'a string' }),
 });
 
 type ConfigType = TypeOf<typeof configSchema>;
 
 export const config: PluginConfigDescriptor<ConfigType> = {
   schema: configSchema,
   deprecations: ({ rename, unused }) => [
     rename('someOldKey', 'someNewKey'),
     unused('deprecatedProperty'),
   ],
 };

elasticmachine · 2019-12-06T13:15:04Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 995e02e

elasticmachine · 2019-12-06T13:20:49Z

Pinging @elastic/kibana-platform (Team:Platform)

src/core/server/config/config_service.ts

src/core/server/config/deprecation/deprecation_factory.test.ts

src/core/server/config/config_service.ts

src/core/server/config/deprecation/utils.ts

pgayvallet · 2019-12-06T13:38:17Z

src/plugins/testbed/server/index.ts

+  deprecations: ({ rename, unused, renameFromRoot }) => [
+    rename('securityKey', 'secret'),
+    renameFromRoot('oldtestbed.uiProp', 'testbed.uiProp'),
+    unused('deprecatedProperty'),
+  ],


Added an example in testbed. Not sure if I should keep it.

we could add something similar to the integration tests in LP https://github.com/restrry/kibana/blob/55604f711f2b06ca58235c1aabc45f5281e13642/src/legacy/server/config/__tests__/deprecation_warnings.js#L28
although it tests only logging part

Do we want a similar approach to what LP does, by actually spawning a real kibana process? Could not find a NP equivalent to legacy const RUN_KBN_SERVER_STARTUP = require.resolve('./fixtures/run_kbn_server_startup'); ? Seems like more than what we call integration_test in NP?

run_kbn_server_startup uses test_utils under the hood. So do we in the integrations tests.
https://github.com/restrry/kibana/blob/55604f711f2b06ca58235c1aabc45f5281e13642/src/core/server/http/integration_tests/core_services.test.ts#L23
so yes, I think we can adopt the same approach

Thanks, missed that.

joshdover · 2019-12-06T19:02:33Z

Overall this is looking great. Haven't dug deep into the implementation yet, but the interface and design looks pretty good to me.

Totally removes the legacy deprecation system

We are so close and yet so far. This impacts LegacyService and legacy code the most. Currently, the legacy service creates a LegacyConfig from the raw config's value, and calls findPluginSpecs with it. The config then enter in the legacy world/code, where discovered legacy plugins mutates the legacy configuration with both their schema, their value and their deprecations. This mutated legacy config is then considered the source of truth when we instanciate the legacy kibana server.

To be able to totally suppress the legacy deprecation system, we will need to remove the part where the deprecation are applied from legacy findPluginSpecs, get the non-migrated config back in legacyService, and then apply the new platform deprecation to it. However, all this code is strongly coupled atm, not typed (js, no ts), and lack test coverage, so I will need to be extremely careful with that part, and think that we might want to do it in a separate PR.

Just so I understand correctly:

As-is in this PR right now, New Platform plugins and Legacy plugins will be reading different configs because the NP deprecations are not applied to the config that legacy reads. However, the legacy deprecations are applied to the config that NP reads. Correct?

If so, are we sure there's a need to apply NP deprecations to the config that LP reads? It seems that if an LP plugin wants to read a renamed config key, it'd be fine if we told them they still need to implement the deprecation in the LP too.

pgayvallet · 2019-12-09T07:24:19Z

As-is in this PR right now, New Platform plugins and Legacy plugins will be reading different configs because the NP deprecations are not applied to the config that legacy reads. However, the legacy deprecations are applied to the config that NP reads. Correct?

That is correct.

If so, are we sure there's a need to apply NP deprecations to the config that LP reads? It seems that if an LP plugin wants to read a renamed config key, it'd be fine if we told them they still need to implement the deprecation in the LP too.

I don't know if there is a functional need. To be honest I'm not even sure we really should do it.

However what I meant what more about technical cleanup: ATM, legacy deprecations are applied to the legacy config (a good thing) but in the legacy world (which is the issue), therefor blocking me from actually remove all the legacy config deprecation system. Now that legacy deprecation can be adapted to NP one, we could apply the adapted legacy deprecations (and maybe the NP ones) to LP config in NP (probably in the legacy service), which would allow us to get rid of the legacy code now without waiting for post 8.0.

But we can also decide that this is not necessary and just keep this legacy code until we actually unplug legacy.

WDYT?

joshdover · 2019-12-09T19:59:34Z

It's not worth to do it now. Removing it after plugins migration is safer and way less complicated.

👍

…ig-deprecation

mshustov · 2019-12-12T08:59:28Z

src/core/server/config/deprecation/core_deprecations.ts

+ * under the License.
+ */
+
+import _, { has, get } from 'lodash';


Suggested change

import _, { has, get } from 'lodash';

import { has, get } from 'lodash';

…ig-deprecation

mshustov · 2019-12-12T09:50:53Z

src/cli/cluster/cluster_manager.js


 process.env.kbnWorkerType = 'managr';

 export default class ClusterManager {
  static create(opts, settings = {}, basePathProxy) {
    return new ClusterManager(
      opts,
-      Config.withDefaultSchema(transformDeprecations(settings)),
+      Config.withDefaultSchema(settings),


Not sure if it works as expected. As I understand settings is expected to be POJO here. But we pass LegacyConfig https://github.com/elastic/kibana/pull/52251/files#diff-748376502c51ed8bfc271f4a6c4c26d3R251
Shouldn't we wrap in getLegacyRawConfig or pass this.settings in legacy_service ?

I think you are right. However the PR did not change the arguments we were invoking ClusterManager.create with. So I guess this has always been broken?

The fix is (well, should be) easy

require('../../../cli/cluster/cluster_manager').create( this.coreContext.env.cliArgs, -- config, ++ config.get(), await basePathProxy$.toPromise() );

However the legacy service tests are a mess, and are not properly mocking with correct object types (the config in test is actually a Record and not a LegacyConfig ). Already had to fix that in #52060, so would be easier to fix in a separate PR once it lands.

Will open a ticket for that if it's alright with you

#52860 created

yeah, not a blocker for the current work.

mshustov · 2019-12-12T10:05:47Z

src/core/server/server.ts

@@ -220,6 +228,8 @@ export class Server {
      [uiSettingsConfig.path, uiSettingsConfig.schema],
    ];

+    this.configService.addDeprecationProvider(rootConfigPath, coreDeprecationProvider);


shouldn't we rename setupConfigSchemas ? for example setupCoreConfig.

…ig-deprecation

elasticmachine · 2019-12-13T08:37:38Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: bb9ee0e

History

💚 Build #15057 succeeded d402ac7
💔 Build #15043 failed 46a4f73
💚 Build #14739 succeeded 03549fa
💚 Build #14446 succeeded b9e0c6c
💚 Build #14357 succeeded 46b7532

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

* implements 'rename' and 'unset' deprecations * introduce usage of ConfigDeprecationProvider * adapt RawConfigService to only returns unmodified raw config * apply deprecations when accessing config * register legacy plugin deprecation in new platform * implements ConfigService#validate * add exemple config deprecation usage in testbed * documentation * export public config deprecation types * fix new test due to rebase * name ConfigDeprecationFactory * update generated doc * add tests for unset and move it to src/core/utils * add tests for renameFromRoot and unusedFromRoot * cast paths as any as get expects a fixed-length string array * use specific logger for deprecations * add additional test on renameFromRoot * update migration guide * migrate core deprecations to NP * add integration test * use same log context as legacy * remove old deprecation warnings integration tests, now covered in NP * migrates csp deprecation to NP * removes deprecationWarningMixin from legacy * remove legacy core deprecations * remove unused import * rename setupConfigSchemas to setupCoreConfig * update generated doc

This comment has been minimized.

Sign in to view

pgayvallet added 10 commits December 6, 2019 12:02

implements 'rename' and 'unset' deprecations

1c5dc41

introduce usage of ConfigDeprecationProvider

52e34f9

adapt RawConfigService to only returns unmodified raw config

033193b

apply deprecations when accessing config

2fe6f97

register legacy plugin deprecation in new platform

3dbd4b1

implements ConfigService#validate

a0f0dea

add exemple config deprecation usage in testbed

d2dba27

documentation

181cd74

export public config deprecation types

c078836

fix new test due to rebase

31a9c38

pgayvallet force-pushed the kbn-40255-NP-config-deprecation branch from 98bcb38 to 31a9c38 Compare December 6, 2019 11:15

pgayvallet added 2 commits December 6, 2019 12:53

name ConfigDeprecationFactory

c0725d3

update generated doc

995e02e

This comment has been minimized.

Sign in to view

pgayvallet added Feature:New Platform release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.6.0 v8.0.0 labels Dec 6, 2019

pgayvallet commented Dec 6, 2019

View reviewed changes

pgayvallet marked this pull request as ready for review December 6, 2019 13:38

pgayvallet requested a review from a team as a code owner December 6, 2019 13:38

pgayvallet added 4 commits December 9, 2019 21:13

remove old deprecation warnings integration tests, now covered in NP

ba45ea8

migrates csp deprecation to NP

efe2ec8

removes deprecationWarningMixin from legacy

46b7532

remove legacy core deprecations

b9e0c6c

mshustov mentioned this pull request Dec 10, 2019

Licensing plugin and XPackInfo uses the same license data #52507

Merged

3 tasks

pgayvallet mentioned this pull request Dec 10, 2019

[Telemetry] Migrate ui_metric plugin to NP under usageCollection #51972

Merged

5 tasks

Merge remote-tracking branch 'upstream/master' into kbn-40255-NP-conf…

03549fa

…ig-deprecation

joshdover mentioned this pull request Dec 11, 2019

Move CSP options to new platform #52698

Merged

7 tasks

mshustov mentioned this pull request Dec 11, 2019

Migrate CSP configuration #50644

Closed

3 tasks

mshustov reviewed Dec 12, 2019

View reviewed changes

pgayvallet added 2 commits December 12, 2019 10:17

Merge remote-tracking branch 'upstream/master' into kbn-40255-NP-conf…

b44d9e1

…ig-deprecation

remove unused import

46a4f73

mshustov reviewed Dec 12, 2019

View reviewed changes

pgayvallet mentioned this pull request Dec 12, 2019

ClusterManager is created with incorrect parameters #52860

Closed

mshustov approved these changes Dec 12, 2019

View reviewed changes

pgayvallet added 3 commits December 12, 2019 11:36

rename setupConfigSchemas to setupCoreConfig

d432d8a

update generated doc

d402ac7

Merge remote-tracking branch 'upstream/master' into kbn-40255-NP-conf…

bb9ee0e

…ig-deprecation

pgayvallet merged commit eb3d9b1 into elastic:master Dec 13, 2019

pgayvallet mentioned this pull request Dec 13, 2019

[7.x] Implements config deprecation in New Platform (#52251) #52976

Merged

azasypkin mentioned this pull request Dec 13, 2019

Change session timeout values to use duration instead of number #52520

Merged

TinaHeiligers mentioned this pull request Sep 27, 2021

Removes misc unused so/server/es config deprecations #113173

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implements config deprecation in New Platform #52251

Implements config deprecation in New Platform #52251

pgayvallet commented Dec 5, 2019 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

elasticmachine commented Dec 6, 2019

elasticmachine commented Dec 6, 2019

pgayvallet Dec 6, 2019

mshustov Dec 9, 2019

pgayvallet Dec 9, 2019 •

edited

Loading

mshustov Dec 9, 2019

pgayvallet Dec 9, 2019

joshdover commented Dec 6, 2019

pgayvallet commented Dec 9, 2019

joshdover commented Dec 9, 2019

mshustov Dec 12, 2019

mshustov Dec 12, 2019

pgayvallet Dec 12, 2019

pgayvallet Dec 12, 2019

mshustov Dec 12, 2019

mshustov Dec 12, 2019

elasticmachine commented Dec 13, 2019

	import _, { has, get } from 'lodash';
	import { has, get } from 'lodash';

Implements config deprecation in New Platform #52251

Implements config deprecation in New Platform #52251

Conversation

pgayvallet commented Dec 5, 2019 • edited Loading

Summary

Actual implementation

RawConfigService and ConfigService changes

Legacy deprecations in new platform

What remains to be done

Checklist

For maintainers

Dev Docs

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

elasticmachine commented Dec 6, 2019

💚 Build Succeeded

elasticmachine commented Dec 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgayvallet Dec 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshdover commented Dec 6, 2019

pgayvallet commented Dec 9, 2019

joshdover commented Dec 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Dec 13, 2019

💚 Build Succeeded

History

pgayvallet commented Dec 5, 2019 •

edited

Loading

`RawConfigService` and `ConfigService` changes

pgayvallet Dec 9, 2019 •

edited

Loading