[Reporting/New Platform Migration] Use a new config service on server-side by tsullivan · Pull Request #55882 · elastic/kibana

tsullivan · 2020-01-24T18:52:46Z

This PR integrates the ReportingConfig service. based on a New-Platform schema, throughout server-side Reporting. This removes the biggest legacy dependency in Reporting.

Getting access to the config requires access to the ReportingCore object on the server.

async function getConfig(reportingCore: ReportingCore) {
   return await reportingCore.getConfig();
}

The getConfig function returns an object with a get method, and a kbnConfig object property that also has a get method. I'm using config.get() to read the Reporting config, and config.kbnConfig.get() to read the global Kibana config. The kbnConfig part is to keep the call signatures about the same as the legacy code, when reading the global config. For example, to get the server host name, the legacy call is server.config().get('server.host') and now it's: config.kbnConfig.get('server', 'host'). And it is also type-safe unlike the legacy calls.

Because the access is asynchronous, the code tries to strip the config out of the reportingCore object when passing down to other modules, such as routes registration. This is to avoid the need to convert those modules to asynchronous.

As part of the new accessor on the ReportingCore object, this PR also adds an accessor for the Elasticsearch service:

async function getElasticsearch(reportingCore: ReportingCore) {
  return await reportingCore.getElasticsearchService();
}

Summary of the changes:

Remove legacy config code
Add new config schema to NP Reporting plugin
Change function signatures to accept the ReportingConfig object
Change createJobFactory / executeJobFactory functions to be async
For async functions that accept the ReportingCore object, don't pass the ElasticsearchServiceSetup API: use await reporting.getElasticsearchService() instead.
Add createConfig$ to update defaults as needed, because config.set is not available anymore.

Todo

~~[ ] Access path.data from New Platform API~~ this is a TODO for after moving the code to NP
Unit test for the new schema declaration
Unit test for function createConfig$
Unit test for validating the server host
Unit test for random encryption key generation
I18N for the modified files 🎉
Make config.get() and config.kbnConfig.get() type-safe 🎉
Resolution on comments about the disableSandbox code changes: https://github.com/elastic/kibana/pull/55882/files#r395927154

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~~

x-pack/legacy/plugins/reporting/server/usage/get_reporting_usage.ts

tsullivan · 2020-01-28T20:39:37Z

x-pack/legacy/plugins/reporting/index.ts

The UI needs to be updated in-step with this change cc @joelgriffith

x-pack/legacy/plugins/reporting/index.ts

elasticmachine · 2020-02-07T10:08:21Z

Pinging @elastic/kibana-reporting-services (Team:Reporting Services)

joelgriffith · 2020-03-20T22:22:55Z

x-pack/legacy/plugins/reporting/server/legacy.ts

+
+  const kbnConfig = {
+    path: {
+      data: config.get('path.data'), // FIXME: get from the real PluginInitializerContext


Does this FIXME apply once we're out of legacy?

dq, yup, it does

Yes: since we're in legacy, we have to mock out the initializerContext variable. When we're out of legacy, we will get the data path from the a real initializerContext variable instead of passing server

joelgriffith · 2020-03-20T22:23:44Z

x-pack/legacy/plugins/reporting/server/legacy.ts

+    },
+  };
+
+  // spreading arguments as an array allows the return type to be known by the compiler


Very clever. Do you think (since we're using get) there's an opportunity for injection of any kind?

The reportingConfig object would have to be polluted somehow. It doesn't look like that could happen in this part of the code though.

joelgriffith · 2020-03-20T22:34:48Z

x-pack/legacy/plugins/reporting/server/lib/validate/validate_max_content_length.test.js

-        get: sinon.stub().returns(FIVE_HUNDRED_MEGABYTES),
-      }),
-    };
+    const config = { get: sinon.stub().returns(FIVE_HUNDRED_MEGABYTES) };


WHY ARE YOU YELLING AT ME

joelgriffith · 2020-03-20T22:39:34Z

x-pack/legacy/plugins/reporting/server/routes/generate_from_savedobject_immediate.ts

       */
-      const createJobFn = createJobFactory(reporting, server, elasticsearch, logger);
-      const executeJobFn = await executeJobFactory(reporting, server, elasticsearch, logger);
+      const createJobFn = await createJobFactory(reporting, logger);


Possible to Promise.all these?

joelgriffith · 2020-03-20T22:41:41Z

x-pack/legacy/plugins/reporting/server/routes/jobs.ts

  const exportTypesRegistry = reporting.getExportTypesRegistry();
-  const getRouteConfigDownload = getRouteConfigFactoryDownloadPre(server, plugins, logger);
-  const downloadResponseHandler = downloadJobResponseHandlerFactory(server, elasticsearch, exportTypesRegistry); // prettier-ignore
+  const getRouteConfigDownload = getRouteConfigFactoryDownloadPre(config, plugins, logger);


Nice work with the API pattern (config, plugins, logger etc)

joelgriffith

Just a few thoughts on passing ReportingConfig around vs properties the modules themselves care about. Might make testing easier and less terse to setup?

joelgriffith

LGTM! Going to test this today and see how it all plays out.

joelgriffith · 2020-03-23T19:25:50Z

@elasticmachine merge upstream

joelgriffith · 2020-03-23T19:52:33Z

Smoke tests looking good. Will merge once CI is green

kibanamachine · 2020-03-23T21:17:10Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 6151df8

History

💚 Build #35142 succeeded ad8269c
💔 Build #35066 failed b3ec447
💔 Build #34987 failed 6bcd58d
💚 Build #34977 succeeded dde6188615c29a38729a6aa637243c727b81b58b
💚 Build #34759 succeeded 44a94ce750d9438790a307e4f9d0601e9a10ba01

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

* master: (34 commits) [APM] add service map config options to legacy plugin (elastic#61002) [App Arch] migrate legacy CSS to new platform (core_plugins/kibana_react) (elastic#59882) Migrated styles for "share" plugin to new platform (elastic#59981) [ML] Module setup with dynamic model memory estimation (elastic#60656) Drilldowns (elastic#59632) Upgrade mocha dev-dependency from 6.2.2 to 7.1.1 (elastic#60779) [SIEM] Overview: Recent cases widget (elastic#60993) [ML] Functional tests - stabilize df analytics clone tests (elastic#60497) [SIEM] Updates process and TLS tables to use ECS 1.5 fields (elastic#60854) Migrate doc view part of discover (elastic#58094) Revert "[APM] Collect telemetry about data/API performance (elastic#51612)" fix(NA): log rotation watchers usage (elastic#60956) [SIEM] [CASES] Build lego blocks case details view (elastic#60864) Create Painless Lab app (elastic#57538) [SIEM] Move Timeline Template field to first step of rule creation (elastic#60840) [Reporting/New Platform Migration] Use a new config service on server-side (elastic#55882) [Alerting] allow email action to not require auth (elastic#60839) [Maps] Default ES document layer scaling type to clusters and show scaling UI in the create wizard (elastic#60668) [APM] Collect telemetry about data/API performance (elastic#51612) Implement Kibana Login Selector (elastic#53010) ...

…n server-side (#55882)" (#61075) This reverts commit 5755b2a.

kibanamachine · 2020-03-25T23:08:44Z