Move base feature controls functionality from XPack Main plugin to a dedicated XPack Features plugin by azasypkin · Pull Request #44664 · elastic/kibana

azasypkin · 2019-09-03T14:43:48Z

We're moving Security Authorization system to the New Platform and it depends on the "features" functionality. To minimize a number of dependencies on the Legacy Platform plugins we're introducing "Features" plugin that will solely responsible for the base Feature Controls functionality.

NOTE: it's easier to review this PR commit by commit because of git mv. Not much of new code is written here, I just moved some code and split some large files into smaller ones.

~~Blocked by: #44855~~

elasticmachine · 2019-09-03T14:43:50Z

Pinging @elastic/kibana-platform

elasticmachine · 2019-09-03T14:43:51Z

Pinging @elastic/kibana-security

azasypkin · 2019-09-03T14:45:38Z

.github/CODEOWNERS

 /src/legacy/server/saved_objects/  @elastic/kibana-platform
 /src/legacy/ui/public/saved_objects  @elastic/kibana-platform
 /config/kibana.yml @elastic/kibana-platform
+/x-pack/plugins/features/  @elastic/kibana-platform


note: Platform team will own this plugin as soon as we merge it.

azasypkin · 2019-09-03T14:46:16Z

docs/api/features/get.asciidoc


 To retrieve all features, issue a GET request to the
-/api/features/v1 endpoint.
+/features/api endpoint.


note: it feels like not a big deal to change URL in a minor for this particular API.

I agree, this is a negligible change. Is the /${pluginId}/api format a new standard? I know that endpoints have to be prefixed with the plugin id as part of the NP migration, but the /api suffix feels strange to me. Reading this, I'd expect this to retrieve a feature with id api, instead of retrieving all registered features.

We had a similar problem when we wrote the spaces api, but I'm not thrilled with what we did there either...

/api/spaces/space retrieves all spaces
/api/spaces/space/foo retrieves a space with id foo.

We don't need the latter for this API call, since we don't support retrieving a single feature by id. If we follow the spaces pattern though (updating for the NP), we get something like:

/features/feature

...which isn't great either.

Maybe
/features/api/feature?

I'm more than open to other suggestions

Is the /${pluginId}/api format a new standard?

It's not clear yet, that's why I'd like to discuss it here and in #44620. I need to introduce NP routes in a couple PRs right now and was thinking whether we should separate API and view routes somehow, that's how this /api/ part appeared.

I'd expect this to retrieve a feature with id api, instead of retrieving all registered features.

Heh, that's a good point!

Maybe /features/api/feature?

Yeah, or something verbose like /features/api/all-features, /features/api/feature/{id}, still thinking, don't have good enough ideas yet....

I was also thinking about features/_api/, _features/api/, features.api/ to kind of stress on the difference between these two semantically different parts of the URL. We can also say that features/* are always API calls and features/{views|app|pages}/* are for {views|app|pages}.

@restrry it's probably a good time to pick some API naming style that we can follow :) Did you guys had a chance to discuss that already?

Once #44855 is merged I'll change URL from /api/features/v1 to /api/features.

Changed to /api/features

azasypkin · 2019-09-03T14:57:39Z

src/plugins/timelion/kibana.json

@@ -0,0 +1,8 @@
+{


note: we need to get access to the timelion.ui.enabled setting from the Features plugin so introducing NP timelion plugin felt natural to me, but I'm open to any better ideas!

azasypkin · 2019-09-03T15:32:25Z

x-pack/plugins/features/kibana.json

+  "id": "features",
+  "version": "8.0.0",
+  "kibanaVersion": "kibana",
+  "optionalPlugins": ["timelion"],


note: just because we want to know value of timelion.ui.enabled.

azasypkin · 2019-09-03T15:34:25Z

x-pack/plugins/features/server/plugin.ts

+export class Plugin {
+  private readonly logger: Logger;
+
+  private legacyAPI?: LegacyAPI;


note: same approach we used in NP Security plugin, but happy to hear any better suggestions.

azasypkin · 2019-09-03T15:37:56Z

x-pack/plugins/features/server/plugin.ts

+    return deepFreeze({
+      registerFeature: featureRegistry.register.bind(featureRegistry),
+      getFeatures: featureRegistry.getAll.bind(featureRegistry),
+      getFeaturesUICapabilities: () => uiCapabilitiesForFeatures(featureRegistry.getAll()),


question: I expose this just because it's used in uiCapabilities method of the XPack Main Plugin definition. Josh do we have anything like this in NP world so that I can use it here and don't expose to LP?

On the server-side, the NP doesn't have anything related to UI capabilities yet.

The ApplicationService on the client-side does currently expose the injected ui capabilities for the current user. We will change this to fetch UI capabilities (#36319) based on the registered applications once security's HTTP interceptors are moved over.

Whenever the fetch endpoint for this is moved over to the New Platform, I expect the New Platform will need to expose a way for this plugin to inject additional capabilities. We can make that change at a later time.

azasypkin · 2019-09-03T15:38:55Z

x-pack/plugins/translations/translations/ja-JP.json

    "common.ui.savedObjects.overwriteRejectedDescription": "上書き確認が拒否されました",
    "common.ui.savedObjects.saveAsNewLabel": "新規 {savedObjectName} として保存",
    "common.ui.savedObjects.saveDuplicateRejectedDescription": "重複ファイルの保存確認が拒否されました",
-    "kibana-react.savedObjects.saveModal.cancelButtonLabel": "キャンセル",


note: this has been automatically rearranged by i18n script.

Would it be worthwhile to find/replace xpack.main.featureRegistry.* with xpack.features.* in the translated files, since we know that the translated text doesn't need to change? Or is that not advised?

We never did this since translators are supposed to receive en.json for every release, and in this case these labels will be recognized as already translated even though IDs changed (so that we're not charged additionally). But let's double check with @Bamieh

Currently we send a json diff file in english and one for every language we have, so renaming these ID's will be synced with translators. We started doing this to sync translations introduced in PRs as we used to ignore them.

I'd vote for renaming these ids as it will also help keep kibana translated even before we sync with the translators which only happens a few days before every FF release.

Okay, will do that than. I usually advised against manual edits in files with translations and treated them almost as "auto-generated" as it's easy to make something wrong there :)

azasypkin · 2019-09-03T15:54:36Z

x-pack/legacy/plugins/security/server/lib/authorization/api_authorization.ts

-    if (!request.path.startsWith('/api/') || !mode.useRbacForRequest(request)) {
+    // if the api doesn't include or end with "/api/" or we aren't using RBAC for this request,
+    // just continue
+    if (


note: temp fix, we need to discuss what to do here with plugin-id prefixed NP API routes.

I'm trying to remember why we had the startsWith('/api/') check to begin with...I don't think it's necessary for us to check for that path part in addition to the existence of the access:foo tag. We should support securing any route with an appropriately configured access: tag, even if it doesn't have /api/ within its path.

This may have just been a holdover from an earlier implementation for securing API endpoints - @kobelb do you remember differently?

Reverted this change since we can keep /api/features now.

elasticmachine · 2019-09-03T16:16:59Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-09-03T17:32:40Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

azasypkin · 2019-09-03T18:09:17Z

@joshdover @legrego would really appreciate your feedback on this PR (will tag you for review)!

legrego · 2019-09-03T18:29:27Z

ACK will review today/tomorrow

legrego

Made a first pass at the code today (mostly questions) -- I'll test this out some more tomorrow, but I wanted to give you something to look at before my day starts

legrego · 2019-09-03T18:35:12Z

docs/api/features/get.asciidoc


 To retrieve all features, issue a GET request to the
-/api/features/v1 endpoint.
+/features/api endpoint.


I agree, this is a negligible change. Is the /${pluginId}/api format a new standard? I know that endpoints have to be prefixed with the plugin id as part of the NP migration, but the /api suffix feels strange to me. Reading this, I'd expect this to retrieve a feature with id api, instead of retrieving all registered features.

We had a similar problem when we wrote the spaces api, but I'm not thrilled with what we did there either...

/api/spaces/space retrieves all spaces
/api/spaces/space/foo retrieves a space with id foo.

We don't need the latter for this API call, since we don't support retrieving a single feature by id. If we follow the spaces pattern though (updating for the NP), we get something like:

/features/feature

...which isn't great either.

Maybe
/features/api/feature?

I'm more than open to other suggestions

legrego · 2019-09-03T18:37:59Z

src/plugins/timelion/server/plugin.ts

+import { first } from 'rxjs/operators';
+import { TypeOf } from '@kbn/config-schema';
+import { PluginInitializerContext, RecursiveReadonly } from '../../../../src/core/server';
+import { deepFreeze } from '../../../../src/core/utils';


I didn't know we could import from src/core/utils. Last time I tried within x-pack, it was disallowed/not working. Is this something we support for x-pack plugins too, or is it just for OSS plugins?

Is this something we support for x-pack plugins too, or is it just for OSS plugins?

We use it for Security plugin already, so it should be supported: https://github.com/elastic/kibana/blob/master/x-pack/plugins/security/server/plugin.ts#L16

legrego · 2019-09-03T18:45:40Z

x-pack/plugins/features/server/feature_schema.ts

+ * you may not use this file except in compliance with the Elastic License.
+ */
+
+import Joi from 'joi';


Question: Is joi still the validation framework of choice, or should we be using @kbn/config-schema?

I just kept what we had before, but I believe we should eventually migrate to @kbn/config-schema assuming everything we have in these schemas is supported by @kbn/config-schema

Yeah I think it's fine if we change this later.

legrego · 2019-09-03T18:46:23Z

x-pack/plugins/features/server/feature_schema.ts

+
+import Joi from 'joi';
+
+// Each feature gets its own property on the UICapabilities object,


nit: I think this comment belongs with line 15 below.

Ha, right, thanks!

legrego · 2019-09-03T18:51:08Z

x-pack/plugins/features/server/plugin.ts

+}
+
+/**
+ * Describes a set of APIs that is available in the legacy platform only and required by this plugin


nit

Suggested change

* Describes a set of APIs that is available in the legacy platform only and required by this plugin

* Describes a set of APIs that are available in the legacy platform only and required by this plugin

Fixed! Out of curiosity and to improve my grammar skills: does *set* .... that *is* .... sound too unnatural comparing to set of *things* that *are*? :)

legrego · 2019-09-03T18:52:57Z

x-pack/plugins/features/server/plugin.ts

+  Logger,
+  PluginInitializerContext,
+  RecursiveReadonly,
+} from '../../../../src/core/server';


Should we be using the from 'src/core/server' alias instead of the relative import?

I believe I heard somewhere that we should use relative paths instead of this alias when I was working Security NP plugin since it's exactly same approach we'd use to import from other plugins, but maybe something has changed since then. @elastic/kibana-platform what would be the correct approach to import from the core?

Ah ok, I'm interested to see what Platform says here. I'm ok with relative imports if that's the direction we want to go, but third-party plugins wouldn't be able to take advantage of that, right?

I see the relevant discussion is still happening here: #40446 (comment)

legrego · 2019-09-03T18:59:02Z

x-pack/plugins/features/server/plugin.ts

+      registerLegacyAPI: (legacyAPI: LegacyAPI) => {
+        this.legacyAPI = legacyAPI;
+
+        // Register OSS features.


Are we registering OSS features here because we need the saved object types from the legacy API? It was a little confusing when I first read this, as it feels out of place right now. I'm OK leaving it as-is since it's temporary.

Are we registering OSS features here because we need the saved object types from the legacy API?

Yep. And it's confusing, definitely agree. Initially I had additional setup contract registerOSSFeatures method, but it was also confusing as "something" that is outside of Features control is supposed to initialize Features plugin at "some" point 🤷‍♂️

Hopefully we'll get SO on server side in NP soon and get rid of this hack.

legrego · 2019-09-03T19:03:22Z

x-pack/plugins/translations/translations/ja-JP.json

    "common.ui.savedObjects.overwriteRejectedDescription": "上書き確認が拒否されました",
    "common.ui.savedObjects.saveAsNewLabel": "新規 {savedObjectName} として保存",
    "common.ui.savedObjects.saveDuplicateRejectedDescription": "重複ファイルの保存確認が拒否されました",
-    "kibana-react.savedObjects.saveModal.cancelButtonLabel": "キャンセル",


Would it be worthwhile to find/replace xpack.main.featureRegistry.* with xpack.features.* in the translated files, since we know that the translated text doesn't need to change? Or is that not advised?

legrego · 2019-09-03T19:34:12Z

x-pack/plugins/features/server/index.ts

+// functions or removal of exports should be considered as a breaking change. Ideally we should
+// reduce number of such exports to zero and provide everything we want to expose via Setup/Start
+// run-time contracts.
+export { uiCapabilitiesRegex } from './feature_schema';


For what it's worth, we should probably move uiCapabilitiesRegex into src/core, as UI Capabilities are a part of OSS now. You can leave it here for the time being, but if we're concerned about API surface, that's one way to reduce it 😄.

question: your comment about reducing these exports to zero doesn't include the types we export below, right? We can still export types from our plugins outside of the lifecycle methods?

For what it's worth, we should probably move uiCapabilitiesRegex into src/core, as UI Capabilities are a part of OSS now. You can leave it here for the time being, but if we're concerned about API surface, that's one way to reduce it

Yeah, I tried to do that several times while working on this, but failed to do so every time 😆 I just can't find a proper place for it, it was kind of confusing that it sits in the core, but is not used there at all and it's not clear how exactly it's used while reading core code. If you can help me to find a justification I'll happily move that to the core 🙂

question: your comment about reducing these exports to zero doesn't include the types we export below, right? We can still export types from our plugins outside of the lifecycle methods?

Correct!

Yeah, I tried to do that several times while working on this, but failed to do so every time 😆 I just can't find a proper place for it, it was kind of confusing that it sits in the core, but is not used there at all and it's not clear how exactly it's used while reading core code. If you can help me to find a justification I'll happily move that to the core 🙂

It doesn't exist today, but I could see this being useful when we build the list of capabilities. Currently, plugins can provide any capabilities they'd like, even if they violate the regex. We're just trusting that they don't right now.

It's potentially also useful when we resolve capabilities. Core might want to prevent registered Capabilities Modifiers from returning a capability which violates the regex. In theory, the modifiers shouldn't be adding or removing any capabilities; rather, they should just be flipping the boolean flags...

Ah, good to know, thanks. Yeah, it makes a lot of sense to have it in core then.

Moved to the core.

Nah, had to revert and move back to the Features plugin. There is something weird happening when we require regex from ui/capabilities in server code. It's probably expected, but I'll let platform team to handle that when they want to move that to core :)

Thanks for trying!

azasypkin · 2019-09-06T07:35:18Z

@joshdover @legrego thanks for the feedback! I think I've handled everything you mentioned so far, so PR is ready for another pass.

elasticmachine · 2019-09-06T08:43:38Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-09-06T12:12:48Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

legrego

Everything seems to be working great! Just a doc update needed from my perspective.

legrego · 2019-09-06T12:09:20Z

docs/development/plugin/development-plugin-feature-registration.asciidoc

  xpackMainPlugin.registerFeature({
    id: 'dev_tools',
-    name: i18n.translate('xpack.main.featureRegistry.devToolsFeatureName', {
+    name: i18n.translate('xpack.features.devToolsFeatureName', {


This file has a few references to the legacy filepath that we'll want to update:

kibana/docs/development/plugin/development-plugin-feature-registration.asciidoc

Line 25 in af66d11

Registering a feature consists of the following fields. For more information, consult the {repo}blob/{branch}/x-pack/legacy/plugins/xpack_main/server/lib/feature_registry/feature_registry.ts[feature registry interface].

kibana/docs/development/plugin/development-plugin-feature-registration.asciidoc

Line 48 in af66d11

|{repo}blob/{branch}/x-pack/legacy/plugins/xpack_main/server/lib/feature_registry/feature_registry.ts[`FeatureWithAllOrReadPrivileges`].

kibana/docs/development/plugin/development-plugin-feature-registration.asciidoc

Line 66 in af66d11

For a full explanation of fields and options, consult the {repo}blob/{branch}/x-pack/legacy/plugins/xpack_main/server/lib/feature_registry/feature_registry.ts[feature registry interface].

Good catch!

legrego

LGTM on green CI. Thanks!

elasticmachine · 2019-09-06T13:47:05Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

azasypkin · 2019-09-06T13:47:43Z

14:55:32 │ warn Failure loading service "webdriver"
14:55:32 │ERROR Error: Server terminated early with status 1

Looks like unrelated issue.

azasypkin · 2019-09-06T13:47:48Z

retest

elasticmachine · 2019-09-06T15:01:24Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

joshdover

LGTM, thanks @azasypkin!

elasticmachine · 2019-09-09T17:13:30Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

azasypkin · 2019-09-09T18:58:14Z

7.x/7.5.0: 6341714

azasypkin added 2 commits September 3, 2019 16:01

Move files.

3eed113

Changes....

af66d11

azasypkin requested review from a team as code owners September 3, 2019 14:43

azasypkin added the release_note:skip Skip the PR/issue when compiling release notes label Sep 3, 2019

azasypkin commented Sep 3, 2019

View reviewed changes

Temp fix for tag-based API authz.

f3a1ef1

azasypkin commented Sep 3, 2019

View reviewed changes

azasypkin requested review from joshdover and legrego September 3, 2019 18:09

legrego reviewed Sep 3, 2019

View reviewed changes

mshustov mentioned this pull request Sep 4, 2019

[New platform] Security plugin integration #33775

Closed

34 tasks

joshdover mentioned this pull request Sep 4, 2019

Introduce HttpApi and HttpResource services #44620

Closed

azasypkin added 2 commits September 6, 2019 08:53

Review#1: handle review comment and switch back to /api/* API URLs.

b646d6b

Review#1: revert uiCapabilities regex move due to weird bundling issues.

fdbe9f3

azasypkin requested review from joshdover and legrego September 6, 2019 07:35

This comment has been minimized.

Sign in to view

Fix typo in test.

1bbeae4

legrego reviewed Sep 6, 2019

View reviewed changes

Review#2: fix docs links.

eb03238

legrego approved these changes Sep 6, 2019

View reviewed changes

Merge branch 'master' into issue-xxx-np-features

2d35f6d

joshdover approved these changes Sep 9, 2019

View reviewed changes

Merge branch 'master' into issue-xxx-np-features

e2abff6

azasypkin merged commit 9d69b72 into elastic:master Sep 9, 2019

azasypkin deleted the issue-xxx-np-features branch September 9, 2019 17:16

azasypkin added the backport pending label Sep 9, 2019

azasypkin mentioned this pull request Sep 9, 2019

[7.x] Move base feature controls functionality from XPack Main plugin to a dedicated XPack Features plugin (#44664) #45172

Merged

azasypkin added backported and removed backport pending labels Sep 9, 2019

gospodarsky mentioned this pull request Sep 11, 2019

[NP] Timelion #44039

Merged

7 tasks

jfsiii mentioned this pull request Oct 23, 2019

[EPM] Use NP registerFeature #49105

Closed

ogupte mentioned this pull request Nov 23, 2019

[APM] NP Migration feature_catalogue -> home.featureCatalogue.register #51542

Closed


		import Joi from 'joi';

		// Each feature gets its own property on the UICapabilities object,

	* Describes a set of APIs that is available in the legacy platform only and required by this plugin
	* Describes a set of APIs that are available in the legacy platform only and required by this plugin

Conversation

azasypkin commented Sep 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Sep 3, 2019

Uh oh!

elasticmachine commented Sep 3, 2019

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!

azasypkin Sep 3, 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!

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!

elasticmachine commented Sep 3, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Sep 3, 2019

💚 Build Succeeded

Uh oh!

azasypkin commented Sep 3, 2019

Uh oh!

legrego commented Sep 3, 2019

Uh oh!

legrego 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!

azasypkin Sep 4, 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

azasypkin commented Sep 3, 2019 •

edited

Loading

azasypkin Sep 3, 2019 •

edited

Loading

azasypkin Sep 4, 2019 •

edited

Loading

azasypkin Sep 4, 2019 •

edited

Loading