-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Licensing plugin #49345
Licensing plugin #49345
Conversation
Pinging @elastic/kibana-platform (Team:Platform) |
|
||
return new License({ | ||
license: normalizedLicense, | ||
features: response.features, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we normalize features? we have the next logic
on the client:
const camelCasedXPackInfo = convertKeysToCamelCaseDeep(updatedXPackInfo); |
return new License({ | ||
license: response.license, | ||
features: response.features, | ||
signature: response.signature, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
http service doesn’t provide headers
https://github.com/elastic/kibana/blob/master/src/core/public/http/http_setup.ts#L207
💚 Build Succeeded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, the test coverage is amazing.
it('uid', () => { | ||
expect(basicLicense.uid).toBe('uid-000000001234'); | ||
expect(errorLicense.uid).toBeUndefined(); | ||
expect(unavailableLicense.uid).toBeUndefined(); | ||
}); | ||
|
||
it('status', () => { | ||
expect(basicLicense.status).toBe('active'); | ||
expect(errorLicense.status).toBeUndefined(); | ||
expect(unavailableLicense.status).toBeUndefined(); | ||
}); | ||
|
||
it('expiryDateInMillis', () => { | ||
expect(basicLicense.expiryDateInMillis).toBe(5000); | ||
expect(errorLicense.expiryDateInMillis).toBeUndefined(); | ||
expect(unavailableLicense.expiryDateInMillis).toBeUndefined(); | ||
}); | ||
|
||
it('type', () => { | ||
expect(basicLicense.type).toBe('basic'); | ||
expect(goldLicense.type).toBe('gold'); | ||
expect(errorLicense.type).toBeUndefined(); | ||
expect(unavailableLicense.type).toBeUndefined(); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with josh, there are tests in this file that make sense (such as isActive
or isOneOf
) the mock license accessors checks (uid/status/expiryDateInMillis/type
) don't really seems to provide any test value?
💚 Build Succeeded |
@joshdover I closed both. created #50931 |
💚 Build Succeeded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM after json serialization issue is resolved
💚 Build Succeeded |
💚 Build Succeeded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Security/Spaces changes LGTM
* Add x-pack plugin for new platform browser licensing information * Address next round of reviews * Remove poller functionality in favor of inline observables * More observable changes from review comments * Fix outstanding tests * More changes from review, adding additional testing * Add additional tests for license comparisons and sessions * Update test snapshot due to sessionstorage mock * Next round of review feedback from restrry * Fix more review requests from restrry, add additional tests * Pass correct sign mock to license info changed test * Improve doc comments, switch to I-interface pattern * Test error polling sanity, do not expose signature, do not poll on client * Fix type check issues from rebase * Fix build error from rebase * minimize config * move all types to server with consistency with other code * implement License * implement license update & refactor has License changed check * update tests for licensing extending route handler context * implement client side side license plugin * implement server side licensing plugin * remove old code * update testing harness * update types for license status * remove jest-localstorage-mock * fix tests * update license in security * address comments. first pass * error is a part of signature. pass error message to License * move common license types under common folder * rename feature props for BWC and unify name with ILicense * test should work in any timezone * make prettier happy * remove obsolete comment * address Pierre comments * use sha256 for security reasons * use stable stringify to avoid churn
* Add x-pack plugin for new platform browser licensing information * Address next round of reviews * Remove poller functionality in favor of inline observables * More observable changes from review comments * Fix outstanding tests * More changes from review, adding additional testing * Add additional tests for license comparisons and sessions * Update test snapshot due to sessionstorage mock * Next round of review feedback from restrry * Fix more review requests from restrry, add additional tests * Pass correct sign mock to license info changed test * Improve doc comments, switch to I-interface pattern * Test error polling sanity, do not expose signature, do not poll on client * Fix type check issues from rebase * Fix build error from rebase * minimize config * move all types to server with consistency with other code * implement License * implement license update & refactor has License changed check * update tests for licensing extending route handler context * implement client side side license plugin * implement server side licensing plugin * remove old code * update testing harness * update types for license status * remove jest-localstorage-mock * fix tests * update license in security * address comments. first pass * error is a part of signature. pass error message to License * move common license types under common folder * rename feature props for BWC and unify name with ILicense * test should work in any timezone * make prettier happy * remove obsolete comment * address Pierre comments * use sha256 for security reasons * use stable stringify to avoid churn
Summary
This work is based on #44922
I have several things to discuss, but we can implement them in follow-ups.
The main differences from the LP implementation:
state
That's the main question for me.
LP: The plugin allows consumers to calculate state on
license change
event and store this state within the license plugin.kibana/x-pack/legacy/plugins/xpack_main/server/lib/xpack_info.js
Line 280 in 60c596c
Later this state was passed to the client-side.
The signature calculation is based on this state + license content
NP: According to https://github.com/elastic/kibana/pull/44922/files#diff-d2c03a616ca4d2ea0b8c58f8a58e2c82 The licensing plugin does not store the state of the other plugins anymore. The licensing plugin emits a new license and other plugins have to react on the new state to enable/disable their API.
The license signature depends on the license object content only.
@epixa that design decision was discussed?
Network request failures
LP: The licensing plugin didn’t emit a license in case of network errors.
NP: Emits the license even if the request failed. Although I want to discuss how we going to handle this case.
Right now errors are ignored when creating a signature and not passed to the client.
I'd suggest to include error info in the signature. We'd have a consistent way to identify each license and can simplify this check to a string comparison https://github.com/elastic/kibana/pull/49345/files#diff-f5b81632d6d3326e85664175179e8664
clusterSource
LP: Allows specifying cluster source to perform polling. Monitoring uses this functionality plugin
kibana/x-pack/legacy/plugins/monitoring/server/init_monitoring_xpack_info.js
Line 16 in 60c596c
NP: The plugin always uses
data
client. If we want to support the case for monitoring we can expose already existing functionality https://github.com/elastic/kibana/pull/49345/files#diff-6cafae74a549dc892111f589f7faa6a7R74Initial value on the client
LP: Passed on the page via inlined
xpackInitialInfo
NP: Should be fetched
Config
LP:
xpack.xpack_main.xpack_api_polling_frequency_millis
NP:
xpack.licensing.pollingFrequency
License
NP:
mode
field not provided anymoresessionStorage
LP: License and signature were stored under different keys in session storage
NP: License and signature were stored under one key
xpack.licensing
Checklist
Use
strikethroughsto 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[ ] This was checked for keyboard-only and screenreader accessibilityFor maintainers
Dev Docs
Add x-pack plugin for new platform public licensing information. This will eventually replace the licensing information consumed via
xpack_main
. Upon setup, this plugin exposes an observable API for inspecting and making checks against the license information.