Extract License service from CCR and Watcher into license_api_guard plugin in x-pack by cjcenizal · Pull Request #95973 · elastic/kibana

cjcenizal · 2021-03-31T19:26:18Z

Release note

Cross-cluster Replication and Watcher used to log misleading warnings about the basic license on Kibana startup. These warnings have been removed.

Summary

Fixes #95032

When you're on the Basic license, Watcher and CCR log disconcerting license failure messages to the command line on startup. These messages are phrased in a way that makes users think something is wrong, when in reality nothing is wrong. This PR makes two fundamental changes:

In terms of behavior, license failure messages are only logged when a consumer attempts to access a disallowed endpoint. Because the UI doesn't display apps that aren't covered by the user's license, such messages will no longer be logged on startup.
In terms of architecture, the error messages we log are now localized within our internal license service instead of consumed from the core license service. In the long-term, they'll be removed from the core license service and plugins will be expected to follow this pattern and be responsible for their own license-failure UX.

I also refactored our internal license service to leverage the types exposed by the core license service.

Here's how license failures are now surfaced to the user in the UI and CLI for CCR:

Watcher is not quite as helpful but we can address that separately:

Steps to test

Edit the license.ts service to fake the license status.
Load Watcher and CCR to send a request to the API and see how the response is surfaced in the UI and the Kibana server logs.

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support

elasticmachine · 2021-03-31T19:26:20Z

Pinging @elastic/es-ui (Team:Elasticsearch UI)

alisonelizabeth

Thanks for working on this @cjcenizal! I’m happy with this approach and also verified the changes locally.

Next steps: If reviewers approve of this direction, I'll extract this approach into a reusable service and then apply it to similar code in Watcher.

👍 I can re-review once this is complete. Can this service be used more broadly across our other plugins as well (perhaps addressed in a separate PR)?

alisonelizabeth · 2021-04-02T00:24:37Z

x-pack/plugins/cross_cluster_replication/server/services/license.ts

not directly related to this PR - but I noticed while looking at the code in my IDE that LicensingPluginSetup.license$ is deprecated, and we should be using license$ provided by the start contract.

Good point! Done.

…ages.

cjcenizal · 2021-04-02T23:17:47Z

src/plugins/es_ui_shared/server/license/license.ts

+  RequestHandlerContext,
+} from 'src/core/server';
+
+/* eslint-disable @kbn/eslint/no-restricted-paths */


@elastic/kibana-core Do we still need this linting rule to disallow importing x-pack modules into non-x-pack?

For concrete imports, we definitely do, as it would plainly breaks on OSS distrib.

For types, it's not a 'technical' requirement, but until the basic plugins are moved out of xpack, we should stick with that rule.

AFAIK we don't release OSS distributable starting from v7.11 https://www.elastic.co/downloads/past-releases#kibana-oss

But I'd rather plugins didn't import from x-pack. Note that the code in src/ and x-pack have different licenses, so it's easy to leak commercial code outside by importing runtime code accidentally.
Regarding the current use case: why licensing code lives in src/ at all? Licensing compatible types can be declared in the current file, but maybe an x-pack plugin is a better place?

cjcenizal · 2021-04-05T19:34:55Z

src/plugins/es_ui_shared/server/license/license.ts

+  start({ pluginId, minimumLicenseType, licensing }: StartSettings) {
+    licensing.license$.subscribe((license) => {
+      this.licenseType = license.type;
+      this.licenseCheckState = license.check(pluginId, minimumLicenseType!).state;


To testers: you can hardcode a value here, e.g. 'invalid', to test the behavior under various conditions.

cjcenizal · 2021-04-05T19:36:40Z

@alisonelizabeth @jloleysens This is ready for your 👀 !

pgayvallet

I know this may change soon, but I'm not at ease with the fact a xpack-dependent utility class importing types from licensing is exposed from an OSS plugin.

@mshustov wdyt?

pgayvallet · 2021-04-06T08:30:05Z

src/plugins/es_ui_shared/server/license/license.ts

+  RequestHandlerContext,
+} from 'src/core/server';
+
+/* eslint-disable @kbn/eslint/no-restricted-paths */


For concrete imports, we definitely do, as it would plainly breaks on OSS distrib.

For types, it's not a 'technical' requirement, but until the basic plugins are moved out of xpack, we should stick with that rule.

…mocks.

cjcenizal · 2021-04-14T04:02:39Z

@alisonelizabeth @jloleysens @pgayvallet @mshustov Thanks for your reviews! I've addressed your feedback. I agree that we should sequester x-pack related code together until we make a formal commitment to move this code into src, so I've moved the License module into a new x-pack plugin called license_api_guard. At some point in the future I think we could discuss either moving it into the licensing plugin if we think non-ESUI plugins would benefit from it, or into an x-pack-specific version of es_ui_shared if not.

@alisonelizabeth To your question about using this in our other plugins -- I just checked and I believe all of our other plugins are licensed under Basic, so there's no need to guard their API routes this way.

Reviewers, would you mind taking another look at sharing your thoughts? Thanks!

cjcenizal · 2021-04-14T04:04:06Z

x-pack/plugins/license_api_guard/server/license.test.ts

+import { of } from 'rxjs';
+import type { KibanaRequest, RequestHandlerContext } from 'src/core/server';
+// eslint-disable-next-line @kbn/eslint/no-restricted-paths
+import { httpServerMock } from 'src/core/server/http/http_server.mocks';


@mshustov Do you think we could update src/core/server to export these helpers publicly?

They are available from src/core/server/mocks

Thanks, fixed in #97334

mshustov

LGTM!

At some point in the future I think we could discuss either moving it into the licensing plugin if we think non-ESUI plugins would benefit from it, or into an x-pack-specific version of es_ui_shared if not.

We are planning to add something similar #56926

mshustov · 2021-04-15T12:37:47Z

x-pack/plugins/license_api_guard/server/license.ts

+  }
+
+  start({ pluginId, minimumLicenseType, licensing }: StartSettings) {
+    licensing.license$.subscribe((license: ILicense) => {


shouldn't we unsubscribe on the stop lifecycle?

@mshustov I was under the impression that stop is never called in server-side plugins -- plugins just "live" until the Kibana server process is killed. Is that not the case?

plugins just "live" until the Kibana server process is killed.

@cjcenizal Nope. It's always called during the normal teardown process (unless Kibana crashes).

When does teardown occur? How can I trigger it?

mshustov · 2021-04-15T12:41:34Z

x-pack/plugins/license_api_guard/server/license.ts

+        const licenseErrorMessage = this.getLicenseErrorMessage(this.licenseCheckState);
+        this.logger?.warn(licenseErrorMessage);
+
+        return response.customError({


why not response.forbidden?

return response.forbidden({ body: { message: licenseErrorMessage, }, });

Good suggestion!

alisonelizabeth

Changes LGTM. Thanks @cjcenizal!

@alisonelizabeth To your question about using this in our other plugins -- I just checked and I believe all of our other plugins are licensed under Basic, so there's no need to guard their API routes this way.

I think most - if not all - of our plugins have a similar license check (for example, Ingest Node Pipelines, Index Management and ILM). Can we remove this guard now?

alisonelizabeth · 2021-04-15T12:54:40Z

x-pack/plugins/license_api_guard/server/license.test.ts

+    },
+  ].forEach(({ licenseState, expectedMessage }) => {
+    describe(`${licenseState} license`, () => {
+      it('replies with 403 and message and logs the message', () => {


Suggested change

it('replies with 403 and message and logs the message', () => {

it('replies with 403 status and logs the message', () => {

x-pack/plugins/license_api_guard/server/license.ts

jloleysens

Thanks for addressing that feedback @cjcenizal. Happy with the current changes!

Co-authored-by: Alison Goryachev <alisonmllr20@gmail.com>

cjcenizal · 2021-04-15T19:46:27Z

@alisonelizabeth Great call, I created #97314 to track this work. @mshustov I'm going to go ahead and merge once CI is green. We can follow up on your comment about unsubscribing from the license service in another PR if this needs to be addressed.

kibanamachine · 2021-04-15T23:59:18Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💚 Build #120148 succeeded d54f85c
💚 Build #119849 succeeded 0706a25
💔 Build #119800 failed a0fe425
💔 Build #119768 failed 48293ff
💔 Build #119493 failed 066a69f

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

…lugin in x-pack (#95973) (#97332) * Localize error messages. # Conflicts: # .github/CODEOWNERS

cjcenizal requested review from alisonelizabeth and jloleysens March 31, 2021 19:26

cjcenizal requested a review from a team as a code owner March 31, 2021 19:26

alisonelizabeth reviewed Apr 2, 2021

View reviewed changes

cjcenizal force-pushed the bug/ccr-watcher-license-messages branch from 361d501 to 4ba7a29 Compare April 2, 2021 22:09

cjcenizal added 4 commits April 2, 2021 16:14

Restrict CCR license-checking to API requests and localize error mess…

a66cff2

…ages.

Extract License service into es_ui_shared and consume it in CCR.

394ff8a

Consume License service in Watcher.

ff254aa

Add unit tests for License service.

87c0a21

cjcenizal force-pushed the bug/ccr-watcher-license-messages branch from 5e573a1 to 87c0a21 Compare April 2, 2021 23:14

cjcenizal commented Apr 2, 2021

View reviewed changes

Merge branch 'master' into bug/ccr-watcher-license-messages

5d1e3cf

cjcenizal force-pushed the bug/ccr-watcher-license-messages branch from 937ed8a to 3c6acd7 Compare April 5, 2021 18:35

Fix i18n and TS errors. Refactor license test.

7c34572

cjcenizal force-pushed the bug/ccr-watcher-license-messages branch from 3c6acd7 to 7c34572 Compare April 5, 2021 19:31

elastic deleted a comment from kibanamachine Apr 5, 2021

cjcenizal commented Apr 5, 2021

View reviewed changes

cjcenizal added 2 commits April 5, 2021 15:44

Fix TS errors.

25bc327

Fix more TS errors.

f6002cc

pgayvallet reviewed Apr 6, 2021

View reviewed changes

cjcenizal added 2 commits April 13, 2021 20:55

Use httpServerMock and licensingMock helpers instead of hand-rolling …

efd968e

…mocks.

Merge branch 'master' into bug/ccr-watcher-license-messages

066a69f

cjcenizal commented Apr 14, 2021

View reviewed changes

cjcenizal added 5 commits April 14, 2021 06:21

Fix i18n error.

7448fb1

Fix tsconfig errors.

48293ff

Add plugin to i18nrc.

a0fe425

Update plugin list docs.

5701904

Merge branch 'master' into bug/ccr-watcher-license-messages

0706a25

mshustov reviewed Apr 15, 2021

View reviewed changes

alisonelizabeth approved these changes Apr 15, 2021

View reviewed changes

jloleysens approved these changes Apr 15, 2021

View reviewed changes

cjcenizal and others added 2 commits April 15, 2021 12:35

Make isEsSecurityEnabled a public method.

3add225

Co-authored-by: Alison Goryachev <alisonmllr20@gmail.com>

Use forbidden instead of customError.

d54f85c

cjcenizal mentioned this pull request Apr 15, 2021

Remove license checks from ES UI Basic-licensed plugins #97314

Open

7 tasks

cjcenizal mentioned this pull request Apr 15, 2021

Build license-checking into the New Platform #56926

Closed

cjcenizal merged commit 3521441 into elastic:master Apr 15, 2021

cjcenizal deleted the bug/ccr-watcher-license-messages branch April 15, 2021 21:54

cjcenizal changed the title ~~Restrict CCR license-checking to API requests and localize error messages~~ Extract License service from CCR and Watcher into license_api_guard plugin in x-pack Apr 15, 2021

cjcenizal added v7.12.2 and removed v7.12.1 labels Apr 15, 2021

cjcenizal mentioned this pull request Apr 15, 2021

[7.x] Extract License service from CCR and Watcher into license_api_guard plugin in x-pack (#95973) #97332

Merged

cjcenizal added release_note:fix and removed v7.12.2 release_note:skip Skip the PR/issue when compiling release notes labels Apr 15, 2021

cjcenizal mentioned this pull request Apr 15, 2021

Fix typo in license_api_guard README name and import http server mocks from public interface #97334

Merged

cjcenizal added a commit that referenced this pull request Apr 16, 2021

Extract License service from CCR and Watcher into license_api_guard p…

84d8a7d

…lugin in x-pack (#95973) (#97332) * Localize error messages. # Conflicts: # .github/CODEOWNERS

	it('replies with 403 and message and logs the message', () => {
	it('replies with 403 status and logs the message', () => {

Conversation

cjcenizal commented Mar 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Release note

Summary

Steps to test

Checklist

Uh oh!

elasticmachine commented Mar 31, 2021

Uh oh!

alisonelizabeth left a comment

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!

cjcenizal Apr 2, 2021 • 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!

cjcenizal commented Apr 5, 2021

Uh oh!

pgayvallet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjcenizal commented Apr 14, 2021

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 left a comment • 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!

mshustov Apr 16, 2021 • 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!

alisonelizabeth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jloleysens left a comment

Choose a reason for hiding this comment

Uh oh!

cjcenizal commented Apr 15, 2021

Uh oh!

kibanamachine commented Apr 15, 2021

💚 Build Succeeded

Metrics [docs]

History

Uh oh!

Reviewers

Assignees

Labels

Projects

cjcenizal commented Mar 31, 2021 •

edited

Loading

cjcenizal Apr 2, 2021 •

edited

Loading

mshustov left a comment •

edited

Loading

mshustov Apr 16, 2021 •

edited

Loading