Licensing plugin and XPackInfo uses the same license data by mshustov · Pull Request #52507 · elastic/kibana

mshustov · 2019-12-09T14:02:27Z

Summary

Closes: #52502
With this PR XPack doesn't fetch license data anymore, but consume them from NP Licensing plugin. To facilitate migration I converted XPack code into TypeScript.
I tested locally, but I would appreciate a review help to verify changes locally.
My main concern is createXPackInfo, which hasn't got any tests.

TODO: deprecate xpack.xpack_main.xpack_api_polling_frequency_millis when #52251 merged

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

elasticmachine · 2019-12-09T14:02:29Z

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

otherwise new license won't be re-fetched when signature changed. was deleted by mistake

mshustov · 2019-12-10T13:13:37Z

x-pack/legacy/plugins/xpack_main/public/hacks/check_xpack_info_change.js

+    const currentSignature = response.headers('kbn-xpack-sig');
+    const cachedSignature = xpackInfoSignature.get();
+
+    if (currentSignature && cachedSignature !== currentSignature) {


That was removed in #51818 by mistake.
Xpack cannot detect license update without it. I also noticed that it doesn't work well with different fetching mechanism. Some plugins use NP fetch, some custom fetching mechanism, for example

axios
https://github.com/restrry/kibana/blob/354a152caa94d856b8ffe1fa4df0981421c1e134/x-pack/legacy/plugins/canvas/common/lib/fetch.ts#L10

graphql-apollo https://github.com/restrry/kibana/blob/354a152caa94d856b8ffe1fa4df0981421c1e134/x-pack/legacy/plugins/infra/public/pages/metrics/containers/with_metrics.tsx#L6-L8

We need to talk to solution teams about unifying the network stack.
@elastic/kibana-platform
Should we create a separate issue to track?

Do we want an issue, or should we 'just' add in our guidelines that the fetching mecanism in kibana should be core.http?

Apollo can be hard to migrate though, as it's not 'only' a fetch.

It seems we need to solve several problems:

to unify the network stack whenever possible

to investigate and document how we can integrate http interceptors in any network library. Apollo supports middlewares and afterwares https://www.apollographql.com/docs/react/networking/network-layer/#afterware
That could require adjusting interceptor API.

mshustov · 2019-12-10T13:17:01Z

x-pack/legacy/plugins/xpack_main/server/lib/setup_xpack_main.js


  server.expose('info', info);
-  server.expose('createXPackInfo', (options) => new XPackInfo(server, options));
+  server.expose('createXPackInfo', (options) => {


@chrisronline I need your help to test that it doesn't break https://github.com/elastic/kibana/blob/master/x-pack/legacy/plugins/monitoring/server/init_monitoring_xpack_info.js#L15
There are no tests to verify it

@restrry This LGTM. I pulled down the PR and the behavior seems consistent

Okay, so this actually wasn't the case. I missed something and this is causing a problem in 7.6

mshustov · 2019-12-10T13:30:02Z

x-pack/legacy/plugins/xpack_main/server/lib/__tests__/xpack_info.js


  describe('feature', () => {
    let xPackInfo;
+    let license$;


I removed all fetching tests. All remaining tests in the file are supposed to prove the xpack plugin contract.

mshustov · 2019-12-10T14:46:50Z

x-pack/test/licensing_plugin/apis/changes.ts

+      const {
+        body: legacyInitialLicense,
+        headers: legacyInitialLicenseHeaders,
+      } = await supertest.get('/api/xpack/v1/info').expect(200);


temporarily adds tests for Legacy Licensing to have at least smoke-tests

Should we move this to a separate legacy test so it can be more easily deleted?

I wish we could. I need to add a comment in tests #51818 (comment)

I'm going to add a client side tests in the followup PR. Probably I just create 2 separate FTR configs for NP & LP

pgayvallet · 2019-12-11T12:24:24Z

x-pack/legacy/plugins/xpack_main/public/hacks/check_xpack_info_change.js

+    const currentSignature = response.headers('kbn-xpack-sig');
+    const cachedSignature = xpackInfoSignature.get();
+
+    if (currentSignature && cachedSignature !== currentSignature) {


Do we want an issue, or should we 'just' add in our guidelines that the fetching mecanism in kibana should be core.http?

Apollo can be hard to migrate though, as it's not 'only' a fetch.

pgayvallet · 2019-12-11T12:47:55Z

x-pack/legacy/plugins/xpack_main/server/lib/__tests__/setup_xpack_main.js

-      newPlatform: { setup: { plugins: { features: {} } } },
+      newPlatform: { setup: { plugins: { features: {}, licensing: { license$: new BehaviorSubject() } } } },


Should we have a licensingPluginMock in the licensing plugin and use it instead of inline declaration ? something like what's done in src/legacy/ui/public/new_platform/__mocks__/helpers.ts.

I will add a mock for NP the plugin.

This file uses Sinon instead of Jest. I don't want to provide the full mock because we are about to delete this setup completely.

x-pack/plugins/licensing/common/types.ts

joshdover · 2019-12-12T18:20:25Z

x-pack/legacy/plugins/xpack_main/server/lib/xpack_info.ts

+      if (license.isActive) {
+        this._cache = {
+          license,
+          error: undefined,


Is this branch really needed? I would expect that license.error would be undefined when license.isActive is true anyways.

Yes, but this is this.cache._error, which is used by the legacy xpack plugin. Although it's not 1:1 mapping. The legacy plugin updated error whenever fetcher throws
https://github.com/elastic/kibana/pull/52507/files#diff-949ac00938b10918e9232965106cd7e1L186

x-pack/plugins/licensing/server/types.ts

joshdover · 2019-12-12T18:26:55Z

x-pack/test/licensing_plugin/apis/changes.ts

+      const {
+        body: legacyInitialLicense,
+        headers: legacyInitialLicenseHeaders,
+      } = await supertest.get('/api/xpack/v1/info').expect(200);


Should we move this to a separate legacy test so it can be more easily deleted?

elasticmachine · 2019-12-13T11:48:05Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 211844e

History

💚 Build #15093 succeeded d1f9936
💔 Build #15075 failed d3a899b
💚 Build #14506 succeeded 889e459
💔 Build #14204 failed 4d25ee4

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

) * convert xpackinfo to TS * use NP Licensing plugin in XPackInfo * update mocks * put license regresh hack back. otherwise new license won't be re-fetched when signature changed. was deleted by mistake * add functional test for legacy xpackmain * declare setup types on client & server explicitly * rename mock license --> licensing to match plugin name * add tests for createLicensePoller * fix type error * adopt tests for xpack_info * createXPackInfo uses new platform API * put back error mute * address comments * fix renamed import * address comment * update tests to reduce delays * deprecate xpack.xpack_main.xpack_api_polling_frequency_millis * use snake_case in config

…2507)" This reverts commit 2b88256.

) * convert xpackinfo to TS * use NP Licensing plugin in XPackInfo * update mocks * put license regresh hack back. otherwise new license won't be re-fetched when signature changed. was deleted by mistake * add functional test for legacy xpackmain * declare setup types on client & server explicitly * rename mock license --> licensing to match plugin name * add tests for createLicensePoller * fix type error * adopt tests for xpack_info * createXPackInfo uses new platform API * put back error mute * address comments * fix renamed import * address comment * update tests to reduce delays * deprecate xpack.xpack_main.xpack_api_polling_frequency_millis * use snake_case in config

* Licensing plugin and XPackInfo uses the same license data (#52507) * convert xpackinfo to TS * use NP Licensing plugin in XPackInfo * update mocks * put license regresh hack back. otherwise new license won't be re-fetched when signature changed. was deleted by mistake * add functional test for legacy xpackmain * declare setup types on client & server explicitly * rename mock license --> licensing to match plugin name * add tests for createLicensePoller * fix type error * adopt tests for xpack_info * createXPackInfo uses new platform API * put back error mute * address comments * fix renamed import * address comment * update tests to reduce delays * deprecate xpack.xpack_main.xpack_api_polling_frequency_millis * use snake_case in config * fix wrong import

* Licensing plugin and XPackInfo uses the same license data (elastic#52507) * convert xpackinfo to TS * use NP Licensing plugin in XPackInfo * update mocks * put license regresh hack back. otherwise new license won't be re-fetched when signature changed. was deleted by mistake * add functional test for legacy xpackmain * declare setup types on client & server explicitly * rename mock license --> licensing to match plugin name * add tests for createLicensePoller * fix type error * adopt tests for xpack_info * createXPackInfo uses new platform API * put back error mute * address comments * fix renamed import * address comment * update tests to reduce delays * deprecate xpack.xpack_main.xpack_api_polling_frequency_millis * use snake_case in config * fix wrong import

* Fix wrong impor (#52994) * Licensing plugin and XPackInfo uses the same license data (#52507) * convert xpackinfo to TS * use NP Licensing plugin in XPackInfo * update mocks * put license regresh hack back. otherwise new license won't be re-fetched when signature changed. was deleted by mistake * add functional test for legacy xpackmain * declare setup types on client & server explicitly * rename mock license --> licensing to match plugin name * add tests for createLicensePoller * fix type error * adopt tests for xpack_info * createXPackInfo uses new platform API * put back error mute * address comments * fix renamed import * address comment * update tests to reduce delays * deprecate xpack.xpack_main.xpack_api_polling_frequency_millis * use snake_case in config * fix wrong import * prevent eslint error with renaming mock file

mshustov added 3 commits December 9, 2019 14:00

convert xpackinfo to TS

3d4f707

use NP Licensing plugin in XPackInfo

b13aac8

update mocks

4d25ee4

mshustov added Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// Feature:New Platform labels Dec 9, 2019

mshustov added 8 commits December 10, 2019 13:00

put license regresh hack back.

bf36ad3

otherwise new license won't be re-fetched when signature changed. was deleted by mistake

add functional test for legacy xpackmain

2ec4399

declare setup types on client & server explicitly

9d14efb

rename mock license --> licensing to match plugin name

170e600

add tests for createLicensePoller

cc82db6

fix type error

c915f12

adopt tests for xpack_info

d2f5e03

createXPackInfo uses new platform API

354a152

mshustov commented Dec 10, 2019

View reviewed changes

put back error mute

889e459

mshustov commented Dec 10, 2019

View reviewed changes

mshustov added chore v7.6.0 release_note:skip Skip the PR/issue when compiling release notes labels Dec 10, 2019

mshustov marked this pull request as ready for review December 10, 2019 14:42

mshustov requested a review from a team as a code owner December 10, 2019 14:42

mshustov requested a review from a team December 10, 2019 14:42

mshustov commented Dec 10, 2019

View reviewed changes

pgayvallet reviewed Dec 11, 2019

View reviewed changes

mshustov added 2 commits December 12, 2019 12:14

Merge branch 'master' into issue-52502

2f88139

address comments

d3a899b

mshustov requested a review from pgayvallet December 12, 2019 11:39

fix renamed import

d1f9936

mshustov requested a review from a team December 12, 2019 16:34

joshdover approved these changes Dec 12, 2019

View reviewed changes

mshustov added 5 commits December 13, 2019 08:41

address comment

b8d4dbc

update tests to reduce delays

9b3e582

Merge branch 'master' into issue-52502

33c4b7c

deprecate xpack.xpack_main.xpack_api_polling_frequency_millis

ad130b4

use snake_case in config

211844e

mshustov merged commit 2b88256 into elastic:master Dec 13, 2019

mshustov deleted the issue-52502 branch December 13, 2019 12:46

mshustov mentioned this pull request Dec 13, 2019

[7.x] Licensing plugin and XPackInfo uses the same license data (#52507) #52988

Closed

spalger pushed a commit that referenced this pull request Dec 13, 2019

Revert "Licensing plugin and XPackInfo uses the same license data (#5…

f67a063

…2507)" This reverts commit 2b88256.

mshustov mentioned this pull request Dec 13, 2019

Xpack plugin uses NP Licensing plugin to get license data #52994

Merged

mshustov added the reverted label Dec 16, 2019

		newPlatform: { setup: { plugins: { features: {} } } },
		newPlatform: { setup: { plugins: { features: {}, licensing: { license$: new BehaviorSubject() } } } },

Conversation

mshustov commented Dec 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

elasticmachine commented Dec 9, 2019

Uh oh!

mshustov Dec 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mshustov Dec 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mshustov Dec 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Dec 13, 2019

💚 Build Succeeded

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mshustov commented Dec 9, 2019 •

edited

Loading

mshustov Dec 10, 2019 •

edited

Loading

mshustov Dec 11, 2019 •

edited

Loading

mshustov Dec 12, 2019 •

edited

Loading