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

Fix config validation when elastic.apm is defined #96502

Merged

Conversation

pgayvallet
Copy link
Contributor

@pgayvallet pgayvallet commented Apr 7, 2021

Summary

Fix #96376
Fix a regression caused by the removal of the legacy server's schema in #95510

  • Ignore configuration properties with the elastic.apm prefix when checking for unused keys
  • Remove the dev config and use the same mechanism to ignore unused dev.* keys
  • Returns a 78 error code when the configuration validation fails

Checklist

@pgayvallet pgayvallet added release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.13.0 labels Apr 7, 2021
@pgayvallet pgayvallet mentioned this pull request Apr 7, 2021
33 tasks
@@ -9,22 +9,27 @@
import { ConfigService } from '@kbn/config';
import { CriticalError } from '../errors';

const ignoredPaths = ['dev.', 'elastic.apm.'];
Copy link
Contributor Author

@pgayvallet pgayvallet Apr 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dev schema was only added to avoid validation failures. Now that we have this ignore list instead, I removed the dev schema and added it here.

Comment on lines +14 to +15
const invalidConfigExitCode = 78;
const legacyInvalidConfigExitCode = 64;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping 64 for invalid config keys until 8.0 for BWC. Added a task to change that in the 8.0 meta issue.

@pgayvallet pgayvallet marked this pull request as ready for review April 8, 2021 06:12
@pgayvallet pgayvallet requested a review from a team as a code owner April 8, 2021 06:12
@elasticmachine
Copy link
Contributor

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

Copy link
Member

@Bamieh Bamieh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code LGTM

Copy link
Contributor

@mshustov mshustov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested locally:

for the legacy config:

logging.verbose: 42

received

 FATAL  Error: [config validation of [logging].verbose]: expected value of type [boolean] but got [number]
error Command failed with exit code 1.

with

server.basePath: 1

received:

FATAL CLI ERROR Error: [config validation of [server].basePath]: expected value of type [string] but got [number]

@pgayvallet
Copy link
Contributor Author

@mshustov you tested using the dev mode, right? If so, the main process is not 'forwarding' the return code from the kibana server.

For server.basePath, this is validated by the cli process before we starts the Kibana process as this part of the config is required for the basePath proxy.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@pgayvallet pgayvallet merged commit 1eea903 into elastic:master Apr 9, 2021
pgayvallet added a commit to pgayvallet/kibana that referenced this pull request Apr 9, 2021
pgayvallet added a commit that referenced this pull request Apr 9, 2021
phillipb added a commit to phillipb/kibana that referenced this pull request Apr 12, 2021
…to-metrics-tab

* 'master' of github.com:elastic/kibana: (44 commits)
  [Exploratory View]Additional metrics for kpi over time (elastic#96532)
  [Fleet] UI changes on hosted policy detail view (elastic#96337)
  Stacked line charts incorrectly shows one term as 100% (elastic#96203)
  [Fleet] Create enrollment API keys as current user (elastic#96464)
  [Lens] Make table and metric show on top Chart switcher (elastic#96601)
  skip flaky suite (elastic#96691)
  [Lens] Hide "Show more errors" once expanded (elastic#96605)
  [Discover] Unskip histogram hiding test (elastic#95759)
  skip flyout test, add linked issue elastic#96708
  skip copy_to_space_flyout_internal.test.tsx elastic#96708
  fix config validation (elastic#96502)
  Document telemetry fields for stack security features (elastic#96638)
  [Partial Results] Move inspector adapter integration into search source (elastic#96241)
  [RAC] Rule registry plugin (elastic#95903)
  [APM] Run precommit tasks sequentially (elastic#96551)
  [Maps] fix Kibana does not recognize a valid geo_shape index when attempting to create a Tracking Containment alert (elastic#96633)
  [Security Solution] [Cases] Small UI bugfixes (elastic#96511)
  [Actions UI] Changed PagerDuty action form UI to fill payload fields according to the API docs for Resolve and Acknowledge events. (elastic#96363)
  App Search: Result Component Updates (elastic#96184)
  [Alerting] Preconfigured alert history index connector (elastic#94909)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Kibana should use proper exit signal when stops due to a configuration error
5 participants