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

License plugin #18356

Closed
12 tasks done
elasticmachine opened this issue Feb 1, 2018 · 10 comments · Fixed by #43623 or #53574
Closed
12 tasks done

License plugin #18356

elasticmachine opened this issue Feb 1, 2018 · 10 comments · Fixed by #43623 or #53574
Assignees
Labels
blocker Feature:License Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@elasticmachine
Copy link
Contributor

elasticmachine commented Feb 1, 2018

Actions

I see a lot of `│ERROR browser[SEVERE] http://localhost:6171/api/licensing/info - Failed to load resource: the server responded with a status of 401 (Unauthorized)` in the test logs. 
This is caused by the fact that plugins initialize even on login page, where user is non authenticated, and the /api/licensing/info route requires authentication
...We may be look to deprecate it </_xpack> in favour of a different endpoint path before 8.0 is released but AFAIK there are no current plans to do that.

...we’ll continue to keep the name xpack for these two endpoints since that’s the umbrella term that we continue to use to refer to the features in aggregate (and it’s the name we’ll keep for the top-level code folder)
  • [ ] normalize feature name keys. Licensing plugin #49345 (comment) server sends them in snake_case, client work with them in camelCase. can we simplify this ?
    the full context
// The decision to convert kebab-case/snake-case keys to camel-case keys stemmed from an old
// convention of using kebabe-case/snake-case in API response bodies but camel-case in JS
// objects. See pull #29304 for more info.

Won't be done. In #52284 we decided that we won't transform elastic data to avoid a mismatch between js runtime & REST endpoint data shape.
We have only 2 features in snake_case: frozen_indices & voting_only (as of 18.12.2019). I haven't found any occurrences in the codebase, shouldn't affect plugins. Also, Licensing REST API is not considered as a public and can be changed at any time. Plugins should use API provided by API explicitly.

The main changes

state

LP: The plugin allows consumers to calculate state on license change event and store this
The signature calculation is based on this state + license content
NP: We decided that license service doesn't keep plugins state #49345 (comment). Plugins have to react on license change and calculate license state on every license change. If another plugin needs that information, it should be exposed via a plugin contract.
This change makes NP & LP licensing service not compatible. We have to keep both until all plugins migrate to the new platform service. The legacy plugin consume license data from the LP plugin.

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.

clusterSource

LP: Allows specifying cluster source to perform polling. Monitoring uses this functionality plugin
NP: The plugin always uses data client. Provides API to create a license poller with custom ES cluster.

Initial 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.api_polling_frequency

License

NP: mode field is provided, but deprecated.

sessionStorage

LP: License and signature were stored under different keys in session storage
NP: License and signature were stored under one key xpack.licensing

isOneOf

isOneOf removed, use check or hasAtLeast instead

Endpoint

/api/xpack/v1/info is going to be removed. switch to /api/licensing/info

Original description *Original comment by @cjcenizal:*

While chatting with @kobelb about LINK REDACTED we decided it would be helpful to have a single source of truth regarding our license state.

The problem

Currently, each app implements its own license-checking logic in a variety of ways, typically (ab)using the showLinks property. The showLinks property couples our application state with our UI state. We need these to be uncoupled, so that we have the ability to change the UI and UX without how we represent application state.

For example, currently showLinks is used to disable the "Reporting" link in Management. What if we wanted to allow the user to navigate to Reporting and surface a message there stating, "Your license has expired so Reporting is disabled. Please renew or upgrade your License." In this scenario, using showLinks as the condition upon which to show this message makes little sense.

The solution

I think our solution needs to fulfill the following criteria:

  1. It's a single service which any app can consume to answer license-related questions.
  2. It answers the question, "Does the license offer so-and-so feature?"
  3. It answers the questions, "Is the license active? When does it expire?" (temporarily addressed by LINK REDACTED)

Just off the top of my head, I imagine the interface looking something like this:

const isReportingOffered = license.isFeatureOffered('reporting');

Implementation wise, we should define our rules in a way which reflects the X-Pack licensing matrix LINK REDACTED. This way our code acts as our single source of truth regarding our business rules.

const licenseToFeatureMap = {
  basic: {
    security: false,
    reporting: {
      pdf: false,
      csv: true,
    },
    graph: true,
    monitoring: true,
    /* ... */
  },
  /* ... */
};

export const isFeatureOffered = feature => {
  return licenseToFeatureMap[getLicenseType()][feature];
};
@elasticmachine
Copy link
Contributor Author

Original comment by @archanid:

Absolutely ++. I've been thinking we needed something like this, as well. Thanks for opening the discussion.

Thanks for sharing the License Checking UX spreadsheet, too. I didn't know it existed.

Maybe implementation needs to be something like

const isReportingOffered = licenseService.isFeatureOffered(license, 'reporting');

@elasticmachine
Copy link
Contributor Author

Original comment by @archanid:

I would be happy to take this on as soon as I have bandwidth. @kjbekkelund what do you think? We know we need a license service soon. (in the new platform)

@elasticmachine
Copy link
Contributor Author

Original comment by @kjbekkelund:

++ it's definitely something we need to explore. I created this super-enlightening issue for this a while ago (LINK REDACTED 🙈). I'll close that in favor of this issue.

Licenses can also change over time, so observables are super-relevant here.

@archanid ++ let's discuss this issue when you're done with all the Elasticon stuff 👍

@elasticmachine elasticmachine added Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:License release_note:enhancement labels Apr 25, 2018
@rhoboat rhoboat removed their assignment Apr 25, 2018
@epixa epixa changed the title Centralized license service License service May 5, 2018
@epixa epixa added this to Services in New platform miscellaneous May 6, 2018
@epixa epixa changed the title License service X-Pack core license service May 6, 2018
@epixa epixa changed the title X-Pack core license service X-Pack core plugin with license service May 6, 2018
@epixa epixa moved this from Services to Plugins in New platform miscellaneous May 6, 2018
@epixa epixa added enhancement New value added to drive a business result and removed release_note:enhancement labels May 6, 2018
@epixa epixa changed the title X-Pack core plugin with license service License plugin May 17, 2018
@joshdover joshdover moved this from To do to In progress in kibana-core [DEPRECATED] May 28, 2019
@jen-huang
Copy link
Contributor

Saw a link to this issue from this week's Kibana updates, happy to hear we are starting to revamp this!

FWIW @elastic/es-ui team has done some work on a re-usable license checker: https://github.com/elastic/kibana/blob/master/x-pack/legacy/server/lib/check_license/check_license.js

This is currently used by a number of our apps now and is instantiated on a per-plugin basis. For example in Snapshot and Restore. I'm not arguing for a specific implementation from the Platform team, but rather just raising awareness of what is currently in place 😄

This common license checker does not return properties like showLinks etc but rather a very simple object with the shape:

{
  status: (one of the LICENSE_STATUS strings)
  message: (if license isn't valid, a relevant message is returned)
}

The license checker uses a ranked array to compare the plugin's minimum license requirement with the current license.

The UI uses the returned status property to do whatever it needs. The possible status strings are in a /common constants folder so that it can be used on client and server: https://github.com/elastic/kibana/blob/master/x-pack/legacy/common/constants/license_status.ts

@jinmu03
Copy link
Contributor

jinmu03 commented Jul 1, 2019 via email

@mattapperson
Copy link
Contributor

For use-cases where quantity is involved, would the recommendation be to have (as an example) more_then_one_configuration: true?
Or would the spec need to be enhanced to support quantity?

kibana-core [DEPRECATED] automation moved this from In progress to Done (this week) Aug 29, 2019
@joshdover joshdover reopened this Sep 5, 2019
@mshustov mshustov moved this from Done (7.5) to In progress in kibana-core [DEPRECATED] Nov 12, 2019
@mshustov mshustov assigned mshustov and unassigned eliperelman Nov 12, 2019
@mshustov mshustov removed the enhancement New value added to drive a business result label Nov 12, 2019
@mshustov mshustov mentioned this issue Nov 29, 2019
12 tasks
@mshustov
Copy link
Contributor

mshustov commented Dec 13, 2019

In the legacy platform XPackInfoLicense provides next methods to check the license status:

NP plugin initially introduced isOneOf method, isActive flag, isAvailable flag + adopted a helper https://github.com/elastic/kibana/blob/master/x-pack/legacy/server/lib/check_license/check_license.js
This helper duplicates the whole set of legacy API, as it contains checks availability, active status + license level.
I'm thinking that we can reduce API surface and remove excessive methods. Although I'm still thinking if we should switch to checkLicense API in the current state. What can be done simpler:

  • we already provide isActive & isAvailable flags
  • providing an alternative to isOneOf that work with hierarchical license model would be enough. Something like
hasAtLeast(license: LicenseType) => boolean

In comparison to check API that simplify the core API a lot (as well as mocking)

If(!license.isAvailable) { //disable something}
If(!license.isActive) { //disable something}
if(!license.hasAtLeast('gold'){ //disable something

As a downside: all the plugins implement the similar logic and generate similar error messages.
Not friendly to extension - adding a new status would require all the plugins to implement an additional logic branch. However I'm not sure that mixing
vs
quite verbose

// checkLisense API
const {status, message} = license.check('pluginName', 'gold');
if(status === 'Invalid'){ //disable something
licenseMock.mockReturnValue({status: 'Valid'})

I'm leaning towards removing isOneOf method from NP License and keep checkLicense in the current status to reduce amount of work for plugins during migration, because some plugins already use this/similar logic + plugins have a lot of examples in the codebase. Want to hear your opinion.

@joshdover
Copy link
Contributor

So just to be clear, you're proposing that we replace isOneOf, isActive, and isAvailable with a single hasAtLeast? Or are you just proposing replacing isOneOf with hasAtLeast?

One question: do plugins really need to have different behaviors based on isActive and isAvailable or is it good enough to just know whether or not the license (if there is one) is at a specific level?

The only use-case I know of is where we may want to disable a NavLink when a user has a lower license level. This was used to hint to users to upgrade for more features. However, when I just tried loading up a cluster with Basic, I don't see this behavior for any applications (even ML).

@kobelb
Copy link
Contributor

kobelb commented Dec 13, 2019

We used to also do conditional rendering based on whether or not the license had expired. For example, when a Platinum license is expired all of the features granted with the Platinum license were greyed out. As opposed to when a license doesn't grant a feature, it's hidden.

@mshustov
Copy link
Contributor

mshustov commented Dec 16, 2019

So just to be clear, you're proposing that we replace isOneOf, isActive, and isAvailable with a single hasAtLeast?

NP Licensing already provides the check method that is a combined version of isOneOf, isActive, and isAvailable.

Or are you just proposing replacing isOneOf with hasAtLeast?

Yes. What confused me about the current check implementation that its results are not mutually exclusive
https://github.com/elastic/kibana/blob/master/x-pack/plugins/licensing/common/types.ts#L7-L12

From @kobelb examples:

We used to also do conditional rendering based on whether or not the license had expired. For example, when a Platinum license is expired all of the features granted with the Platinum license were greyed out.

A license can be in a tier enough to use a feature but expired. The current check implementation doesn't allow us to express it in the code, as it will return expired every time.

I see several options here:

  • To add hasAtLeast(licenseLevel), remove isOneOf and keep check.
    Pros: check already used by some plugins, centralized licensing messages.
    Cons: duplicated functionality. it can be not obvious when to use check or isAvailable + isActive + hasAtLeast
  • To remove check and and add hasAtLeast(licenseLevel) .
    Pros: smaller API surface, composability.
    Cons: have to combine add conditions manually isAvailable + isActive + hasAtLeast, duplicated functionality across the plugins.

From the practical point of view adding hasAtLeast without removing check requires less work.

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