Add `deprecation.skip_deprecated_settings` config setting #114751

pgayvallet · 2021-10-13T06:04:36Z

Summary

Part of #113734

Add a new mandatory configPath property to config deprecations
Add the deprecation.skip_deprecated_settings config property
Use this new property in the deprecation service to ignore/silence the matching deprecations
Adapt the codebase to add configPath to all custom config deprecations

Notes:

as a lot of deprecations are no longer present on master, this will be manually backported to 7.x to also adapt the 7.x-only deprecations.
once merged and backported, I'll open PRs again cloud and cloud-assets to mute the legacy logging deprecations

Checklist

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

…onfig-deprecations

pgayvallet · 2021-10-14T10:48:37Z

src/core/server/deprecations/deprecation_config.ts

+const configSchema = schema.object({
+  skip_deprecated_settings: schema.arrayOf(schema.string(), { defaultValue: [] }),
+});


The deprecation prefix and skip_deprecated_settings property are consistent with the equivalent ES feature which use the same naming (#113734 (comment)).

maybe we should add this comment to the code?

pgayvallet · 2021-10-14T10:53:20Z

src/core/server/deprecations/deprecations_factory.ts

-    return deprecationInfoBody
-      .flat()
-      .filter(Boolean)
-      .map((pluginDeprecation) => ({
-        ...pluginDeprecation,
-        domainId,
-      }));


The whole flattening and filtering is unnecessary (a unit test was asserting that this was supported, but has to violate the typings to do so)

pgayvallet · 2021-10-14T11:11:50Z

src/core/server/deprecations/types.ts

+export interface FeatureDeprecationDetails extends BaseDeprecationDetails {
+  deprecationType?: 'feature' | undefined;
+}


We should make deprecationType non-optional and probably introduce a other type (as we previously discussed), but given the timing and the fact that this will require to adapt all registered feature deprecations, I did not include it in this PR.

elasticmachine · 2021-10-14T11:12:23Z

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

azasypkin

Code changes in Security\Spaces LGTM.

azasypkin · 2021-10-14T11:33:08Z

x-pack/plugins/security/server/config_deprecations.ts

@@ -121,6 +124,7 @@ export const securityConfigDeprecationProvider: ConfigDeprecationProvider = ({
    >;
    if (Object.values(samlProviders).find((provider) => !!provider.maxRedirectURLSize)) {
      addDeprecation({
+        configPath: 'xpack.security.authc.providers.saml.provider.maxRedirectURLSize',


question: just curious, does random provider do the job for your purposes? The <provider-name> is basically a dynamic string.

Hum, you're right, I should use the value from the found provider instead. Will fix that

azasypkin · 2021-10-14T11:38:06Z

x-pack/plugins/security/server/config_deprecations.ts

@@ -92,6 +94,7 @@ export const securityConfigDeprecationProvider: ConfigDeprecationProvider = ({

    if (hasProviderType('basic') && hasProviderType('token')) {
      addDeprecation({
+        configPath: 'xpack.security.authc.providers',


question: technically this deprecation is about both xpack.security.authc.providers.basic and xpack.security.authc.providers.token or it's okay here?

Yea, I used the 'parent' path because there's no way to specify multiple paths for the deprecation. I'd say this is okay because it's unlikely we'll want to disable those specific deprecations anyhow

elasticmachine · 2021-10-14T11:38:38Z

Pinging @elastic/fleet (Team:Fleet)

mshustov · 2021-10-14T11:47:47Z

src/core/server/deprecations/types.ts

+/**
+ * @internal
+ */
+export type DomainDeprecationDetails = DeprecationsDetails & {


for the sake of consistency and TS performance, prefer interface over type.

Yea, I tried, but I discover new typescript errors everyday. Can't have an interface extend a type that is a composition of types, apparently.

argh. makes sense

mshustov · 2021-10-14T11:50:48Z

packages/kbn-config/src/deprecation/types.ts

@@ -20,6 +20,8 @@ export type AddConfigDeprecation = (details: DeprecatedConfigDetails) => void;
 * @public
 */
 export interface DeprecatedConfigDetails {
+  /** The path of the deprecated config setting */
+  configPath: string;


~~Do we require it to be unique?~~ not really. a value might be deprecated for different reasons.

No, we don't. I reproduced ES deprecation behavior. These are disabled by configPath, e.g if you have multiple deprecations targeting the same config property, they'll have the same configPath and the user will not be able to only disable a given deprecation for a given configPath.

Not ideal imho, but we're consistent (and I doubt we'll really need more granularity than that anyway)

mshustov · 2021-10-14T11:52:59Z

src/core/server/deprecations/deprecations_factory.test.ts

@@ -181,6 +187,85 @@ describe('DeprecationsFactory', () => {
        },
      ]);
    });
+
+    it('excludes config deprecations explicitly ignored via `ignoredConfigDeprecations`', async () => {


Could you add a test that it doesn't throw when configured with an unknown path? I don't think we should throw in this case since it might happen after Kibana started and this feature is not critical to stopping Kibana.

mshustov · 2021-10-14T11:55:49Z

src/core/server/deprecations/deprecations_service.ts

    this.logger.debug('Setting up Deprecations service');
-    const deprecationsFactory = this.deprecationsFactory;
+
+    const config = await this.configService


Why not use atPathSync instead of changing the signature?

For consistency with how other core services behave. We're not using atPathSync in any service atm. Even if that's mostly for historical reasons, as this API was not available at this time, I felt it was better to stay consistent. Can change if we want to, don't really mind.

nreese

maps changes LGTM
code review

ymao1 · 2021-10-14T12:15:38Z

x-pack/plugins/actions/server/index.ts

@@ -69,6 +69,7 @@ export const config: PluginConfigDescriptor<ActionsConfig> = {
        )
      ) {
        addDeprecation({
+          configPath: 'xpack.actions.customHostSettings.ssl.rejectUnauthorized',


xpack.actions.customHostSettings is an array...does that need to be reflected in this config path?

No, as the deprecation will be identified the same way regardless of the index in the array

ymao1

Alerting changes LGTM. Code review only.

jloleysens

Reporting config changes LGTM, did not test locally.

cjcenizal

Code changes to the UA test LGTM.

I haven't reviewed the rest of the code, so forgive a dumb question. Let's say a developer registers a deprecation using coreSetup.deprecations.registerDeprecations (per this example) that checks for the presence of a setting in kibana.yml and registers a custom deprecation message for it. Will deprecation.skip_deprecated_settings be able to filter out this deprecation?

pgayvallet · 2021-10-15T06:02:50Z

Let's say a developer registers a deprecation using coreSetup.deprecations.registerDeprecations (per this example) that checks for the presence of a setting in kibana.yml and registers a custom deprecation message for it. Will deprecation.skip_deprecated_settings be able to filter out this deprecation?

as long as the deprecation registeted via coreSetup.deprecations.registerDeprecations is deprecationType: 'config', it will yes.

…onfig-deprecations

kibanamachine · 2021-10-15T10:09:29Z

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`core`	1018	1023	+5

Unknown metric groups

API count

id	before	after	diff
`core`	2298	2304	+6

History

💚 Build #161283 succeeded e9b1df8
💚 Build #161269 succeeded e2c98ee
💛 Build #160791 was flaky 69e2a7b
💔 Build #160784 failed 7d3a66b
💔 Build #160782 failed bc209da

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

…4751) * Add `deprecation.skip_deprecated_settings` config setting * fix deprecation from service * fix existing config deprecations * fix kbn-config unit tests * adapt deprecation types * fix more deprecations * add filtering and adapt unit tests * add service unit tests * update generated doc * fix telemetry unit tests * address review comments * add missing deprecation titles # Conflicts: # src/core/server/elasticsearch/elasticsearch_config.ts # x-pack/plugins/maps/server/index.ts # x-pack/plugins/spaces/server/config.ts

) (#115164) * Add `deprecation.skip_deprecated_settings` config setting (#114751) * Add `deprecation.skip_deprecated_settings` config setting * fix deprecation from service * fix existing config deprecations * fix kbn-config unit tests * adapt deprecation types * fix more deprecations * add filtering and adapt unit tests * add service unit tests * update generated doc * fix telemetry unit tests * address review comments * add missing deprecation titles # Conflicts: # src/core/server/elasticsearch/elasticsearch_config.ts # x-pack/plugins/maps/server/index.ts # x-pack/plugins/spaces/server/config.ts * add configPath for core deprecations * add configPath for csp deprecation * add configPath for kibana.defaultAppId deprecation * add configPath for plugins deprecations * fix test deprecation

cjcenizal · 2021-10-15T19:21:38Z

@pgayvallet

as long as the deprecation registeted via coreSetup.deprecations.registerDeprecations is deprecationType: 'config', it will yes.

Here's the example from the docs I referred to. I don't see deprecationType used anywhere, so it looks like this might be a possible gap? If it is, it probably isn't a blocker since this setting is intended for internal use.

async function getDeprecations({ esClient, savedObjectsClient }: GetDeprecationsContext): Promise<DeprecationsDetails[]> {
  const deprecations: DeprecationsDetails[] = [];
  const testDashboardUser = await getTestDashboardUser(savedObjectsClient);

  // Imagine checking for a setting here instead
  if (testDashboardUser) {
    deprecations.push({
      title: i18n.translate('security.deprecations.kibanaUserRoleTitle', {
        defaultMessage: 'Deprecated roles are assigned to some users',
      }),
      message: i18n.translate('security.deprecations.kibanaUserRoleMessage', {
        defaultMessage: 'User "test_dashboard_user" is using a deprecated role: "kibana_user".',
      }),
      documentationUrl: 'https://www.elastic.co/guide/en/elasticsearch/reference/current/security-api-put-user.html',
      level: 'critical',
      correctiveActions: {
        api: {
            path: '/internal/security/users/test_dashboard_user',
            method: 'POST',
            body: {
              username: 'test_dashboard_user',
              roles: [
                'machine_learning_user',
                'enrich_user',
                'kibana_admin'
              ],
              full_name: 'Alison Goryachev',
              email: '[email protected]',
              metadata: {},
              enabled: true
            }
        },
        manualSteps: [
          i18n.translate('security.deprecations.kibanaUserRole.manualStepOneMessage', {
            defaultMessage: 'Switch all users with the "kibana_user" role to the kibana_admin role in Management > Security > Users.',
          }),
          i18n.translate('security.deprecations.kibanaUserRole.manualStepTwoMessage', {
            defaultMessage: 'Update all mappings in Management > Security > Role Mappings to assign the "kibana_admin" role instead of the "kibana_user" role.'
          }),
        ],
      },
    });
  }

  return deprecations;
}

pgayvallet · 2021-10-18T14:24:40Z

Here's the example from the docs I referred to. I don't see deprecationType used anywhere, so it looks like this might be a possible gap?

No, it's just that deprecationType is another property that is optional when it shouldn't probably be.

Adding deprecationType: 'config' to your example will have the TS type validation fail unless you also specify configPath.

See

kibana/src/core/server/deprecations/types.ts

Lines 82 to 85 in 81ba068

    
           export interface ConfigDeprecationDetails extends BaseDeprecationDetails { 
        
             configPath: string; 
        
             deprecationType: 'config'; 
        
           }

Add deprecation.skip_deprecated_settings config setting

6a8218a

pgayvallet added Feature:Upgrade Assistant Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.16.0 v8.0.0 labels Oct 13, 2021

pgayvallet added 11 commits October 13, 2021 08:08

fix deprecation from service

bdde4c1

fix existing config deprecations

637c87d

fix kbn-config unit tests

57fff55

adapt deprecation types

5eec8e2

fix more deprecations

69c8660

add filtering and adapt unit tests

41ac30d

Merge remote-tracking branch 'upstream/master' into kbn-113734-skip-c…

42e0c72

…onfig-deprecations

add service unit tests

06df184

Merge remote-tracking branch 'upstream/master' into kbn-113734-skip-c…

bc209da

…onfig-deprecations

update generated doc

7d3a66b

fix telemetry unit tests

69e2a7b

pgayvallet commented Oct 14, 2021

View reviewed changes

pgayvallet marked this pull request as ready for review October 14, 2021 11:12

pgayvallet requested review from a team as code owners October 14, 2021 11:12

pgayvallet requested review from a team October 14, 2021 11:12

pgayvallet added the release_note:skip Skip the PR/issue when compiling release notes label Oct 14, 2021

azasypkin approved these changes Oct 14, 2021

View reviewed changes

botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label Oct 14, 2021

mshustov reviewed Oct 14, 2021

View reviewed changes

mshustov approved these changes Oct 14, 2021

View reviewed changes

nreese approved these changes Oct 14, 2021

View reviewed changes

ymao1 reviewed Oct 14, 2021

View reviewed changes

ymao1 approved these changes Oct 14, 2021

View reviewed changes

jen-huang approved these changes Oct 14, 2021

View reviewed changes

neptunian approved these changes Oct 14, 2021

View reviewed changes

jloleysens approved these changes Oct 14, 2021

View reviewed changes

cjcenizal approved these changes Oct 14, 2021

View reviewed changes

pgayvallet added 2 commits October 15, 2021 08:07

Merge remote-tracking branch 'upstream/master' into kbn-113734-skip-c…

e2c98ee

…onfig-deprecations

address review comments

e9b1df8

pgayvallet mentioned this pull request Oct 15, 2021

Kibana deprecation API shouldn't default level to critical #114197

Closed

pgayvallet added 2 commits October 15, 2021 10:07

add missing deprecation titles

d39788d

Merge remote-tracking branch 'upstream/master' into kbn-113734-skip-c…

645930d

…onfig-deprecations

pgayvallet merged commit 81ba068 into elastic:master Oct 15, 2021

pgayvallet mentioned this pull request Oct 15, 2021

[7.x] Add deprecation.skip_deprecated_settings config setting (#114751) #115164

Merged

pgayvallet mentioned this pull request Oct 25, 2021

[discuss] Allow to ignore specific deprecations #113734

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `deprecation.skip_deprecated_settings` config setting #114751

Add `deprecation.skip_deprecated_settings` config setting #114751

pgayvallet commented Oct 13, 2021 •

edited

Loading

pgayvallet Oct 14, 2021

mshustov Oct 14, 2021

pgayvallet Oct 14, 2021

pgayvallet Oct 14, 2021

elasticmachine commented Oct 14, 2021

azasypkin left a comment

azasypkin Oct 14, 2021

pgayvallet Oct 14, 2021 •

edited

Loading

azasypkin Oct 14, 2021

pgayvallet Oct 14, 2021

elasticmachine commented Oct 14, 2021

mshustov Oct 14, 2021

pgayvallet Oct 14, 2021 •

edited

Loading

mshustov Oct 14, 2021

mshustov Oct 14, 2021 •

edited

Loading

pgayvallet Oct 14, 2021 •

edited

Loading

mshustov Oct 14, 2021

mshustov Oct 14, 2021

pgayvallet Oct 14, 2021

nreese left a comment

ymao1 Oct 14, 2021

pgayvallet Oct 14, 2021

ymao1 left a comment

jloleysens left a comment

cjcenizal left a comment •

edited

Loading

pgayvallet commented Oct 15, 2021

kibanamachine commented Oct 15, 2021

API count

cjcenizal commented Oct 15, 2021 •

edited

Loading

pgayvallet commented Oct 18, 2021

Add deprecation.skip_deprecated_settings config setting #114751

Add deprecation.skip_deprecated_settings config setting #114751

Conversation

pgayvallet commented Oct 13, 2021 • edited Loading

Summary

Checklist

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 Oct 14, 2021

azasypkin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgayvallet Oct 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Oct 14, 2021

Choose a reason for hiding this comment

pgayvallet Oct 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshustov Oct 14, 2021 • edited Loading

Choose a reason for hiding this comment

pgayvallet Oct 14, 2021 • edited Loading

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

nreese left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ymao1 left a comment

Choose a reason for hiding this comment

jloleysens left a comment

Choose a reason for hiding this comment

cjcenizal left a comment • edited Loading

Choose a reason for hiding this comment

pgayvallet commented Oct 15, 2021

kibanamachine commented Oct 15, 2021

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

API count

History

cjcenizal commented Oct 15, 2021 • edited Loading

pgayvallet commented Oct 18, 2021

Add `deprecation.skip_deprecated_settings` config setting #114751

Add `deprecation.skip_deprecated_settings` config setting #114751

pgayvallet commented Oct 13, 2021 •

edited

Loading

pgayvallet Oct 14, 2021 •

edited

Loading

pgayvallet Oct 14, 2021 •

edited

Loading

mshustov Oct 14, 2021 •

edited

Loading

pgayvallet Oct 14, 2021 •

edited

Loading

cjcenizal left a comment •

edited

Loading

cjcenizal commented Oct 15, 2021 •

edited

Loading