Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove deprecated Core settings #103915

Closed
Tracked by #84380
joshdover opened this issue Jun 30, 2021 · 26 comments
Closed
Tracked by #84380

Remove deprecated Core settings #103915

joshdover opened this issue Jun 30, 2021 · 26 comments
Assignees
Labels
Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@joshdover
Copy link
Contributor

joshdover commented Jun 30, 2021

We need to remove the deprecations here for anything Core owns that should no longer be supported after 8.0+

This primarily includes https://github.com/elastic/kibana/blob/master/src/core/server/config/deprecation/core_deprecations.ts, but also any other Core-owned plugins.

Approach to removing these:

  • Doublecheck we are seeing all of the deprecations in UA on the 7.x branch (we should be)
  • Remove the deprecations themselves
  • Update schemas if needed (for example if a plugin has both an old and renamed key in the schema, the old one should be removed)
  • Document removal of the old config if needed

(🔴 : should remove, 🟢 should keep, ❔: TBD)

Removed configs

  • All the deprecations targeting the legacy logging system can definitely be removed once [8.0] Remove @kbn/legacy-logging #50660 lands:
    • 🔴 opsLoggingEventDeprecation
    • 🔴 requestLoggingEventDeprecation
    • 🔴 destLoggingDeprecation
    • 🔴 quietLoggingDeprecation
    • 🔴 silentLoggingDeprecation
    • 🔴 verboseLoggingDeprecation
    • 🔴 jsonLoggingDeprecation
    • 🔴 logRotateDeprecation
    • 🔴 logEventsLogDeprecation
    • 🔴 logEventsErrorDeprecation
    • 🔴 logFilterDeprecation
    • 🔴 timezoneLoggingDeprecation
  • 🔴 kibana.index should be removed as a part of Removing legacy multitenancy #82020

Unused configs

Handled in #113173 and docs

Renamed configs

Handled in #113653

  • xpack.banners.placement: 'header' renamed to xpack.banners.placement: 'top'
  • xpack.xpack_main.xpack_api_polling_frequency_millis renamed to xpack.licensing.api_polling_frequency (related [Licensing] Add UA deprecation for xpack.xpack_main.xpack_api_polling_frequency_millis #113235)
  • kibana.disableWelcomeScreen renamed to home.disableWelcomeScreen
  • ui_metric.enabled to usageCollection.uiCounters.enabled
  • ui_metric.debug to usageCollection.uiCounters.debug
  • usageCollection.uiMetric.enabled to usageCollection.uiCounters.enabled
  • usageCollection.uiMetric.debug to usageCollection.uiCounters.debug

cc @stacey-gammon @kobelb

@joshdover joshdover changed the title Remove deprecated settings https://github.com/elastic/kibana/blob/master/src/core/server/config/deprecation/core_deprecations.ts Remove deprecated Core settings Jun 30, 2021
@botelastic botelastic bot added the needs-team Issues missing a team label label Jun 30, 2021
@joshdover joshdover added the Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc label Jun 30, 2021
@botelastic botelastic bot removed the needs-team Issues missing a team label label Jun 30, 2021
@pgayvallet
Copy link
Contributor

pgayvallet commented Sep 8, 2021

Edit: Moved these to the issue description


Looking at src/core/server/config/deprecation/core_deprecations.ts, what exactly should we handle here? (🔴 : should remove, 🟢 should keep, ❔: TBD)

  • All the deprecations targeting the legacy logging system can definitely be removed once [8.0] Remove @kbn/legacy-logging #50660 lands:

    • 🔴 opsLoggingEventDeprecation
    • 🔴 requestLoggingEventDeprecation
    • 🔴 destLoggingDeprecation
    • 🔴 quietLoggingDeprecation
    • 🔴 silentLoggingDeprecation
    • 🔴 verboseLoggingDeprecation
    • 🔴 jsonLoggingDeprecation
    • 🔴 logRotateDeprecation
    • 🔴 logEventsLogDeprecation
    • 🔴 logEventsErrorDeprecation
    • 🔴 logFilterDeprecation
    • 🔴 timezoneLoggingDeprecation
  • Not sure about the other 'custom' deprecation though?

    • 🔴 configPathDeprecation - safe to remove when [config] Remove deprecated environment variables CONFIG_PATH and DATA_PATH #111535 lands
      • warn about the usage of the CONFIG_PATH varenv (suggest using KBN_PATH_CONF instead). Do we know if the support for CONFIG_PATH will be dropped in 8.0? cc @elastic/kibana-operations
    • 🔴 dataPathDeprecation - safe to remove when [config] Remove deprecated environment variables CONFIG_PATH and DATA_PATH #111535 lands
      • warn about the usage of the DATA_PATH varenv (suggest using path.data in the config file instead). Do we know if the support for DATA_PATH will be dropped in 8.0? cc @elastic/kibana-operations
    • rewriteBasePathDeprecation
      • warn when server.basePath is specified in the config and server.rewriteBasePath is not.
    • rewriteCorsSettings
      • warn when using server.cors as a boolean instead of server.cors.enabled. I guess we should keep this one?
    • cspRulesDeprecation
      • warn when using nonce or not using self in csp.rules
    • 🔴 mapManifestServiceUrlDeprecation - handled in [Maps] Remove deprecated maps.manifestServiceUrl configuration #111656
      • warn about the usage of map.manifestServiceUrl. Not even sure why this one is still in core? cc @elastic/kibana-gis
  • ❔ Then there are a lot of unused config properties related to the optimizer:

unusedFromRoot('optimize.lazy'),
unusedFromRoot('optimize.lazyPort'),
unusedFromRoot('optimize.lazyHost'),
unusedFromRoot('optimize.lazyPrebuild'),
unusedFromRoot('optimize.lazyProxyTimeout'),
unusedFromRoot('optimize.enabled'),
unusedFromRoot('optimize.bundleFilter'),
unusedFromRoot('optimize.bundleDir'),
unusedFromRoot('optimize.viewCaching'),
unusedFromRoot('optimize.watch'),
unusedFromRoot('optimize.watchPort'),
unusedFromRoot('optimize.watchHost'),
unusedFromRoot('optimize.watchPrebuild'),
unusedFromRoot('optimize.watchProxyTimeout'),
unusedFromRoot('optimize.useBundleCache'),
unusedFromRoot('optimize.sourceMaps'),
unusedFromRoot('optimize.workers'),
unusedFromRoot('optimize.profile'),
unusedFromRoot('optimize.validateSyntaxOfNodeModules'),

@elastic/kibana-operations are these fine to remove in 8.0 or should we keep some or all of them?

  • And a bunch of other unused deprecations:
    • savedObjects.indexCheckTimeout
    • server.xsrf.token
    • elasticsearch.preserveHost
    • elasticsearch.startupTimeout

Should we keep or remove those?

@nreese
Copy link
Contributor

nreese commented Sep 8, 2021

warn about the usage of map.manifestServiceUrl. Not even sure why this one is still in core?

This setting was added in #54399. @nickpeihl are there plans to remove manifestServiceUrl for 8.0? There are still 2 files containing manifestServiceUrl in maps_ems plugin. Looks like the setting is not wired up to anything as well so maybe the deprecation notice is no longer needed and can just be removed?

@jbudz
Copy link
Member

jbudz commented Sep 8, 2021

I opened #111535 for CONFIG_PATH and DATA_PATH.

@nickpeihl
Copy link
Member

warn about the usage of map.manifestServiceUrl. Not even sure why this one is still in core?

This setting was added in #54399. @nickpeihl are there plans to remove manifestServiceUrl for 8.0? There are still 2 files containing manifestServiceUrl in maps_ems plugin. Looks like the setting is not wired up to anything as well so maybe the deprecation notice is no longer needed and can just be removed?

I've opened #111656 to remove the map.manifestServiceUrl setting in 8.0.

@lukeelmers
Copy link
Member

Based on Pierre's summary above, we'll break this down into the following chunks of work:

  • Removal of logging-related configs which will be done as part of [8.0] Remove @kbn/legacy-logging #50660
  • Environment / config related deprecations
  • Optimizer-related configs
  • Miscellaneous server / ES / SO configs

@TinaHeiligers
Copy link
Contributor

@pgayvallet did you plan to handle the other 3 "chunks" too or just the logging configs?

@pgayvallet
Copy link
Contributor

#112305 is only handling the logging config deprecations, the other chunks should be handled directly in this issue

@TinaHeiligers TinaHeiligers self-assigned this Sep 27, 2021
@spalger
Copy link
Contributor

spalger commented Sep 27, 2021

All of the unused optimizer settings are safe to remove

@TinaHeiligers
Copy link
Contributor

All of the unused optimizer settings are safe to remove

When we remove these, should we also be removing any mention of the legacy optimize.* settings from serve?
TBH, I'm a little unsure which optimizer we're configuring from the cliArgs here. @spalger I see that disableOptimizer was added in the same PR that added the optimize.* deprecations. Are we ok with removing support from cli too?

@spalger
Copy link
Contributor

spalger commented Sep 27, 2021

All of the unused optimizer settings are safe to remove

When we remove these, should we also be removing any mention of the legacy optimize.* settings from serve?
TBH, I'm a little unsure which optimizer we're configuring from the cliArgs here. @spalger I see that disableOptimizer was added in the same PR that added the optimize.* deprecations. Are we ok with removing support from cli too?

No, the --no-optimizer cli flag is still valid and prevents running the "new" @kbn/optimizer in development which is what powers:

optimize: !!opts.optimize,
disableOptimizer: !opts.optimizer,

The unused optimizer.* options described in the deprecations should literally be unused and there shouldn't be any code that needs to change except removing the deprecations.

@lukeelmers
Copy link
Member

lukeelmers commented Sep 28, 2021

Edit: Moved these to the issue description


Did a quick audit of all core-owned plugins, and found a few other deprecations (I guess we should track these here -- afaik there are not other issues for these, but if someone knows otherwise please lmk):

Unused:

  • newsfeed.defaultLanguage

Renamed:

  • xpack.banners.placement: 'header' renamed to xpack.banners.placement: 'top'
  • xpack.xpack_main.xpack_api_polling_frequency_millis renamed to xpack.licensing.api_polling_frequency (related [Licensing] Add UA deprecation for xpack.xpack_main.xpack_api_polling_frequency_millis #113235)
  • kibana.disableWelcomeScreen renamed to home.disableWelcomeScreen
  • ui_metric.enabled to usageCollection.uiCounters.enabled
  • ui_metric.debug to usageCollection.uiCounters.debug
  • usageCollection.uiMetric.enabled to usageCollection.uiCounters.enabled
  • usageCollection.uiMetric.debug to usageCollection.uiCounters.debug

@TinaHeiligers
Copy link
Contributor

Did a quick audit of all core-owned plugins, and found a few other deprecations

I confirmed that none of these settings are registered with the coreDeprecationProvider and am not to sure how we should proceed with them:

  • Do they already appear as deprecations and all we need to do is remove them?
  • or do we first need some sort of deprecation notice period?

If it's the latter, we should probably have done that a few minors ago. 🙍‍♀️

@lukeelmers
Copy link
Member

Do they already appear as deprecations and all we need to do is remove them?

Yes, because they are using the config deprecation provider, they should already be surfaced via the deprecations API & therefore in the UA.

So basically we'd need to:

  • Doublecheck we are seeing the deprecations in UA on the 7.x branch (we should be)
  • Remove the deprecations themselves
  • Update schemas if needed (for example if a plugin has both an old and renamed key in the schema, the old one should be removed)
  • Document removal of the old config if needed

@lukeelmers
Copy link
Member

This issue is getting hard to follow, I'm going to combine the lists Pierre and I have into the description

@lukeelmers
Copy link
Member

IMO we should go ahead and remove all of these other unused ones:

  • savedObjects.indexCheckTimeout
  • server.xsrf.token
  • elasticsearch.preserveHost
  • elasticsearch.startupTimeout

The one I have questions about is rewriteBasePathDeprecation. @elastic/kibana-core does anyone have more context than me on this one? The deprecation is confusing to me:

const rewriteBasePathDeprecation: ConfigDeprecation = (settings, fromPath, addDeprecation) => {
if (settings.server?.basePath && !settings.server?.rewriteBasePath) {
addDeprecation({
message:
'You should set server.basePath along with server.rewriteBasePath. Starting in 7.0, Kibana ' +
'will expect that all requests start with server.basePath rather than expecting you to rewrite ' +
'the requests in your reverse proxy. Set server.rewriteBasePath to false to preserve the ' +
'current behavior and silence this warning.',
correctiveActions: {
manualSteps: [
`Set 'server.rewriteBasePath' in the config file, CLI flag, or environment variable (in Docker only).`,
`Set to false to preserve the current behavior and silence this warning.`,
],
},
});
}
};

AFAICT the schema is already going to enforce that server.basePath is set when rewriteBasePath is provided. It looks like this warning will only happen if there is a basePath but rewriteBasePath is falsey, however in the warning itself we say you can suppress the message by setting rewriteBasePath to false. This doesn't make sense. What am I missing? TBH I'm not even sure what this warning is trying to do 🤔


@elastic/kibana-security do you have any opinions on proceeding with the removal of these items below? (they've been renamed or the structure has changed, so we'd just be removing the old key that is already deprecated):

  • rewriteCorsSettings (server.cors moved to server.cors.enabled)
    • const rewriteCorsSettings: ConfigDeprecation = (settings, fromPath, addDeprecation) => {
      const corsSettings = settings.server?.cors;
      if (typeof corsSettings === 'boolean') {
      addDeprecation({
      message: '"server.cors" is deprecated and has been replaced by "server.cors.enabled"',
      correctiveActions: {
      manualSteps: [
      `Replace "server.cors: ${corsSettings}" with "server.cors.enabled: ${corsSettings}"`,
      ],
      },
      });
      return {
      set: [{ path: 'server.cors', value: { enabled: corsSettings } }],
      };
      }
      };
  • cspRulesDeprecation
    • const cspRulesDeprecation: ConfigDeprecation = (settings, fromPath, addDeprecation) => {
      const NONCE_STRING = `{nonce}`;
      // Policies that should include the 'self' source
      const SELF_POLICIES = Object.freeze(['script-src', 'style-src']);
      const SELF_STRING = `'self'`;
      const rules: string[] = settings.csp?.rules;
      if (rules) {
      const parsed = new Map(
      rules.map((ruleStr) => {
      const parts = ruleStr.split(/\s+/);
      return [parts[0], parts.slice(1)];
      })
      );
      return {
      set: [
      {
      path: 'csp.rules',
      value: [...parsed].map(([policy, sourceList]) => {
      if (sourceList.find((source) => source.includes(NONCE_STRING))) {
      addDeprecation({
      message: `csp.rules no longer supports the {nonce} syntax. Replacing with 'self' in ${policy}`,
      correctiveActions: {
      manualSteps: [`Replace {nonce} syntax with 'self' in ${policy}`],
      },
      });
      sourceList = sourceList.filter((source) => !source.includes(NONCE_STRING));
      // Add 'self' if not present
      if (!sourceList.find((source) => source.includes(SELF_STRING))) {
      sourceList.push(SELF_STRING);
      }
      }
      if (
      SELF_POLICIES.includes(policy) &&
      !sourceList.find((source) => source.includes(SELF_STRING))
      ) {
      addDeprecation({
      message: `csp.rules must contain the 'self' source. Automatically adding to ${policy}.`,
      correctiveActions: {
      manualSteps: [`Add 'self' source to ${policy}.`],
      },
      });
      sourceList.push(SELF_STRING);
      }
      return `${policy} ${sourceList.join(' ')}`.trim();
      }),
      },
      ],
      };
      }
      };

@TinaHeiligers
Copy link
Contributor

TinaHeiligers commented Sep 29, 2021

@Bamieh, the upgrade assistant surfaces deprecations for ui_metric.enabled and ui_metric.debug.

usage_collection_deprecations_only

This doesn't work though when usageCollection.uiMetric.enabled and usageCollection.uiMetric.debug are configured in kibana.yml, Kibana throws.

server    log   [17:31:29.184] [fatal][root] ValidationError: [config validation of [usageCollection].uiMetric]: definition for this key is missing

Was this always the case or is there something else going on in the 7.x branch?

@TinaHeiligers
Copy link
Contributor

@jportner @legrego Could you please let us know if we should keep these deprecations or if they're safe to remove for 8.0?:

  • rewriteCorsSettings (server.cors moved to server.cors.enabled
  • cspRulesDeprecation

@jportner
Copy link
Contributor

@jportner @legrego Could you please let us know if we should keep these deprecations or if they're safe to remove for 8.0?:

  • rewriteCorsSettings (server.cors moved to server.cors.enabled
  • cspRulesDeprecation

Sorry I missed this!

  • rewriteCorsSettings - I don't feel strongly about this. FWIW I think the impact of keeping it around is low, so in the interest of Make It Minor we might want to just keep it.
  • cspRulesDeprecation - Interestingly enough, csp.rules is supposed to be superseded by the new additive CSP configuration (Only allow additive CSP configuration #94414). In the PR that closed that issue (Allow additive csp configuration #102059) we included a release note saying that csp.rules is deprecated, but it doesn't appear that we actually updated the code to log such a deprecation warning. At any rate, CSP misconfiguration is a regular cause of frustration for users and support tickets, and the new additive configuration alleviates that issue by getting rid of that foot-gun. So I'd say that it would be good to remove this in 8.0.

Perhaps it's worth checking usage data and seeing how many users are actually using these settings?

Of course we should make sure the upgrade assistant in 7.16 is functioning as expected if we do indeed plan to remove either of these in 8.0.

@TinaHeiligers
Copy link
Contributor

TinaHeiligers commented Sep 30, 2021

Of course we should make sure the upgrade assistant in 7.16 is functioning as expected if we do indeed plan to remove either of these in 8.0.

UA is showing the cspDeprecation:
csp_basepath_banners_welcomescreen_deprecations

The first two deprecations in that shot are for CONFIG_PATH and DATA_PATH. The warning message isn't very descriptive for those and we'll want to improve that for 7.16 (if they're going to stay). They do, however, show the info:
config_path_deprecation_visible

I won't remove the deprecation for rewriteCorsSetting in the PR I'm working on (hasn't been pushed yet).

@jportner
Copy link
Contributor

jportner commented Sep 30, 2021

UA is showing the cspDeprecation:

Sorry, I should have been clearer 😅
The current config deprecations for CSP rules are conditional on specific rules (can't include nonce, must include self). So if you have for example csp.rules: ['script-src self'], I believe nothing will currently show up in the Upgrade Assistant.

Whereas we really need to change that deprecation so that any usage of csp.rules shows in the Upgrade Assistant. And then we should remove csp.rules completely in 8.0, IMO.

@TinaHeiligers
Copy link
Contributor

Whereas we really need to change that deprecation so that any usage of csp.rules shows in the Upgrade Assistant. And then we should remove csp.rules completely in 8.0, IMO.

Changing the deprecation is going to need a different issue. The one I'm working on is purely to remove the ones we need to get rid of in 8.0.

Let's see where we're all at in a week from now. As it is, I'm sure we all have a lot to do in the new couple of weeks.

@jportner
Copy link
Contributor

Changing the deprecation is going to need a different issue. The one I'm working on is purely to remove the ones we need to get rid of in 8.0.

Let's see where we're all at in a week from now. As it is, I'm sure we all have a lot to do in the new couple of weeks.

Nevermind, there is already a different deprecation from the one that Luke linked above:

deprecations: () => [
(rawConfig, fromPath, addDeprecation) => {
const cspConfig = rawConfig[fromPath];
if (cspConfig?.rules) {
addDeprecation({
message:
'`csp.rules` is deprecated in favor of directive specific configuration. Please use `csp.connect_src`, ' +
'`csp.default_src`, `csp.font_src`, `csp.frame_ancestors`, `csp.frame_src`, `csp.img_src`, ' +
'`csp.report_uri`, `csp.report_to`, `csp.script_src`, `csp.style_src`, and `csp.worker_src` instead.',
correctiveActions: {
manualSteps: [
`Remove "csp.rules" from the Kibana config file."`,
`Add directive specific configurations to the config file using "csp.connect_src", "csp.default_src", "csp.font_src", ` +
`"csp.frame_ancestors", "csp.frame_src", "csp.img_src", "csp.report_uri", "csp.report_to", "csp.script_src", ` +
`"csp.style_src", and "csp.worker_src".`,
],
},
});
}
},

So I think that deprecation is fine and doesn't need any changes 👍

@TinaHeiligers
Copy link
Contributor

TinaHeiligers commented Oct 9, 2021

…does anyone have more context than me on this one? The deprecation is confusing to me:

I tracked the deprecation down to way back in 6.3 when rewriteBasePath was introduced
The original PR describes why this was needed quite well but the relevant parts are:
“ server.basePath expects the proxy sending the request to rewrite the request and strip the basePath” (this was tripping folks up).
We added server.rewriteBasePath to tell Kibana if it should remove the basePath from requests it receives. It looks like that was going to be enabled by default in 7.0 (which we didn't do) and the deprecation was supposed to be removed (which we also didn’t do).

.kibana.yml still has server.rewriteBasePath: false with the same info added in the original PR and
the deprecation message itself hasn’t changed so I guess we forgot about what we planned to change in 7.0.0, or it was intentionally left as-is. @spalger do you know why the plans weren't followed through?

Either way, how should we proceed now: flip the default to true and remove the deprecation or something else? I don't think we should carry this deprecation through for yet another major. WDYT?

@pgayvallet
Copy link
Contributor

Either way, how should we proceed now: flip the default to true and remove the deprecation or something else?

I'm mixed. As much as I would love to get rid of this legacy behavior (I mean, having basePath set requiring an additional config param to effectively serving from this basePath doesn't make a lot of sense), the impact on changing this default are quite serious (even if the fix is as trivial as manually setting the rewriteBasePath value to false in the config to get back to the old behavior).

In particular, we would need to check:

  • what changes are required in the dev server code (packages/kbn-cli-dev-mode/src/base_path_proxy_server.ts)
  • if we need to change the default config on cloud somehow (are they using server.basePath with the 'legacy' behavior?)

@TinaHeiligers
Copy link
Contributor

TinaHeiligers commented Oct 11, 2021

I could do some research and chat with a few folks. As you mention though, the repercussions might be quite serious given that users would probably have gotten used to the current behavior after 2+ years. I've created a new issue to set time aside for the research and continue discussions there (#114562).

I suggest we change the deprecation level from critical to warning for this issue and call it a day for 8.0, unless, of course, the @elastic/kibana-operations team feels differently.

@TinaHeiligers
Copy link
Contributor

I believe all the tasks that were going to be handled under this issue are done or dedicated issues for follow up tasks.
If I've missed something, please re-open this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

No branches or pull requests

9 participants