Proposal: Separate oss_features out of the Features plugin

[tsullivan]

Proposal: Create a new plugin to separate oss_features out of features. This will allow open-source applications to registered as "features" integrate Reporting export types as their own sub-feature(s).

Background

Issue: #19914.

The Reporting should switch from the roles-based access control model (ElasticsearchFeatureConfig) to using Kibana feature controls (a SubFeatureConfig declaration in KibanaFeatureConfig.subFeatures). The technical changes will be that that various applications will integrate with Reporting by registring reporting features as sub-feature privileges.

Example for the Canvas feature declaration, with the added subFeatures key:

plugins.features.registerKibanaFeature({
  id: 'canvas',
  // ...
  privileges: {
    all: {
      app: ['canvas', 'kibana'],
      catalogue: ['canvas'],
      savedObject: {
        // ...
      },
      ui: ['save', 'show'],
    },
    read: {
      app: ['canvas', 'kibana'],
      catalogue: ['canvas'],
      savedObject: {
        // ...
      },
      ui: ['show'],
    },
  },
  subFeatures: [
    {
      name: 'PDF Reports',
      privilegeGroups: [
        {
          groupType: 'independent',
          privileges: [
            {
              id: 'generate_screenshot_canvas',
              name: 'Generate PDF Reports',
              includeIn: 'all',
              savedObject: { all: [], read: [] },
              ui: ['generate_pdf'],
            },
          ],
        },
      ],
    // ...

With the Canvas feature registered in that way, an admin user could grant Reporting access to users in the Management > Security > Roles UI.

The difference for "OSS" applications

Discover, Dashboard and Visualize also have feature declarations that need to be updated with Reporting as a sub-feature. The current declarations are in x-pack/plugins/features/server/oss_features.ts

On the surface, it might seem like Reporting could be registered as sub-feature privileges of the 3 applications right in this file. If that were to happen, then the Features plugin would need a dependency for reporting in its kibana.json file. The detailed list of changes are:

1. Add a dependency for the Reporting plugin in the Features plugin
2. Check if the Reporting plugin is enabled.
3. If Reporting is enabled, call a method in the “ReportingPluginStart” API to ask if Reporting’s legacy model for access control is enabled (xpack.reporting.roles.enabled: true). If the answer is FALSE, then we know Reporting is not self-registering as an Elasticsearch feature.
4. Add the registrations of Reporting as a subfeature to Dashboard, Discover, and Visualize in oss_features.ts

Problem: the Features plugin can not depend on Reporting. There would be circular dependencies: Reporting also depends on features in order to register itself as an Elasticsearch feature. That can be solved by moving the Reporting declaration out - maybe to Features. However, that isn’t enough: Reporting also depends on security, which also depends on features. That makes an unavoidable set of circular dependencies:

Detailed Proposal

By taking oss_features logic out of x-pack/plugins/features and moving it to a new plugin, the Features plugin wouldn't need to depend on Reporting: the new plugin would centrally depend on Reporting and on Features. It would have the logic of the numbered steps above. Taking that code out of the Features plugin restores the dependency graph to flow in a single direction:

In the longer term, having a new plugin could help Reporting with other technical debt. Reporting has code that is specific to OSS applications could be moved to the new plugin. This refers to the code that registers Reporting features as a top-nav sharing menu action for Discover, the code that registers "Download CSV" as a saved search panel action, etc.

Unknowns

This proposal would solve an earlier barrier in one idea to implement #19914. I don't yet know enough Feature Controls capability when it comes to showing or hiding the Reporting widgets in the UI. The proposal is that the legacy model can be toggled off with a config setting, but is enough context going to be carried all the way down to the browser level?

[elasticmachine]

Pinging @elastic/kibana-core (Team:Core)

[elasticmachine]

Pinging @elastic/kibana-app-services (Team:AppServices)

[tsullivan]

cc @jportner @legrego

[mshustov]

By taking oss_features logic out of x-pack/plugins/features and moving it to a new plugin, the Features plugin wouldn't need to depend on Reporting: the new plugin would centrally depend on Reporting and on Features.

it doesn't scale very well: low-level plugins (such as Features) should not depend on high-level plugins (such as Reporting).

Discover, Dashboard and Visualize also have feature declarations that need to be updated with Reporting as a sub-feature.

It's a remnant of the division of code into OSS and x-pack. With the new licensing model, we can move most of the Features plugin logic (not coupled with any license level > Basic) to src/plugins to expand the usage across the plugins.
Thus, Discover, Dashboard and Visualize will be able to register their features themselves.
It doesn't solve the problem with registering Reporting as a sub-privilege of these plugins. To address this problem, we will have to reverse the dependency: for example, Reporting plugin might move to src/plugins keeping Gold+ features in x-pack or these plugins can provide extension points for Reporting plugin.

cc @kobelb

[legrego]

I agree with @mshustov that the correct solution is to move the bulk of the Features plugin outside of x-pack, and to have each non-x-pack feature register itself, instead of the features plugin registering these on their behalf.

That said, I'm not strictly opposed to @tsullivan's solution as a stepping stone towards our ideal end state. Moving the features plugin outside of x-pack makes complete sense, but I haven't dealt with the licensing implications of such a move yet. The interfaces for features and sub-feature privileges allow plugins to state which license level they require. The features plugin doesn't do much to enforce license levels, but is that alone enough to warrant concern? Do we need to move aspects of the licensing plugin outside of x-pack as well (mostly thinking type definitions/enums here)?

It doesn't solve the problem with registering Reporting as a sub-privilege of these plugins.

IMO this is only a problem for the remainder of 7.x. Once we move to 8.0, these features won't have to conditionally register reporting sub-feature privileges. Starting in 8.0, they can always specify the reporting sub-feature privileges, as the old privilege model will no longer be supported. We could consider a more flexible privilege model where plugins can register sub-features for features belonging to other plugins, but I'm not yet convinced that's something we need.

[kobelb]

I agree with @mshustov that the correct solution is to move the bulk of the Features plugin outside of x-pack, and to have each non-x-pack feature register itself, instead of the features plugin registering these on their behalf.

That said, I'm not strictly opposed to @tsullivan's solution as a stepping stone towards our ideal end state.

Agreed!

Moving the features plugin outside of x-pack makes complete sense, but I haven't dealt with the licensing implications of such a move yet. The interfaces for features and sub-feature privileges allow plugins to state which license level they require. The features plugin doesn't do much to enforce license levels, but is that alone enough to warrant concern? Do we need to move aspects of the licensing plugin outside of x-pack as well (mostly thinking type definitions/enums here)?

It's my understanding that license enforcement itself should not be in OSS; however, I think it'd be fine to have OSS aware of the license levels. @stacey-gammon, is this your understanding as well?

[tsullivan]

IMO this is only a problem for the remainder of 7.x. Once we move to 8.0, these features won't have to conditionally register reporting sub-feature privileges. Starting in 8.0, they can always specify the reporting sub-feature privileges, as the old privilege model will no longer be supported.

Yes! I am glad that came across clearly. I am looking for changes that can push toward Reporting using feature controls in 7.13.

[tsullivan]

There is a benefit to cleaning up code and moving features outside of x-pack. I don't want to let that discussion take over the original intent. Reporting needs the buildOSSFeatures function in x-pack/plugins/features/server/oss_features.ts to be separated to a new plugin. The new plugin can depend on Reporting, and add the subFeature declarations for Discover, Dashboard and Visualize. The additional subFeature declarations require a dependency on Reporting, and features can not depend on `reporting.

It sounds like there is general acceptance of my proposal to create a new plugin, x-pack/plugins/oss_features, to own the OSS feature extension points. The new plugin will depend on features and the buildOSSFeatures function will be moved to it.

The new plugin will also depend on reporting. OSS feature registration needs to be Reporting-aware to start work on #19914

This discuss issue doesn't need to stay open for long. If there is no objection by Wednesday March 24 2021, midnight GMT, I am going to consider this accepted

[mshustov]

It sounds like there is general acceptance of my proposal to create a new plugin, x-pack/plugins/oss_features, to own the OSS feature extension points.

ok for me.

The new plugin will depend on features and the buildOSSFeatures function will be moved to it.

IMO We should reverse the dependency: x-pack/features depends on oss_features, and it's going to provide /api/features HTTP endpoint.

The new plugin will also depend on reporting

It bothers me a little bit. As already being said, I don't think low-level plugins (building blocks for Kibana plugins) should depend on high-level plugins (providing user-facing functionality). Otherwise, we will face another circular dependency problem very soon.
I'd suggest considering reversing the dependency to allow features_oss plugin to provide an extension point for Reporting.

[tsullivan]

It's a remnant of the division of code into OSS and x-pack. With the new licensing model, we can move most of the Features plugin logic (not coupled with any license level > Basic) to src/plugins to expand the usage across the plugins.

It doesn't solve the problem with registering Reporting as a sub-privilege of these plugins. To address this problem, we will have to reverse the dependency: for example, Reporting plugin might move to src/plugins keeping Gold+ features in x-pack or these plugins can provide extension points for Reporting plugin.

Reporting isn't going to move in 7.x. I need a thing that can provide extension points for Reporting plugin, which is what this proposal is about.

I agree with @mshustov that the correct solution is to move the bulk of the Features plugin outside of x-pack, and to have each non-x-pack feature register itself, instead of the features plugin registering these on their behalf.

I really don't see a problem with having a plugin be responsible for registering non-x-pack features - the problem for me if the features plugin is the one that is responsible. I am asking that a new high-level plugin be responsible. The new plugin is called high level because nothing depends on it. That way, the new plugin can depend on Reporting. Reporting is a middle-level dependency because it has low level dependencies.

That said, I'm not strictly opposed to @tsullivan's solution as a stepping stone towards our ideal end state. Moving the features plugin outside of x-pack makes complete sense, but I haven't dealt with the licensing implications of such a move yet.

I don't suggest to move any code outside of x-pack :) The new plugin that provides extension points for oss features would be x-pack/plugins/oss_features.

I understand that there is a bit of maintenance cost with moving a file from one x-pack plugin to a new x-pack plugin: that is definitely not a plan that could fit every problem. But if the right thing means breaking up the features plugin and the reporting plugin, that means that Reporting is blocked from using Kibana access controls until 2 mountains get moved.

IMO We should reverse the dependency: x-pack/features depends on oss_features, and it's going to provide /api/features HTTP endpoint.

The need for Kibana to have an /api/features HTTP endpoint should be provided by something else. This would just be until the OSS features can register themselves with all the sub-features, including Reporting.

I truly want to find a way to allow Reporting to progress towards Kibana feature controls before 8.0. I think a lot can be achieved by adding a new x-pack plugin to register the the oss features, and include Reporting along with it, and keep it as a high-level plugin. If the new plugin becomes more than that, and it starts to mean more code needs to be refactored, we get a lot of complexity on what #19914 is waiting for.

• Maybe there was some refactoring work planned or in-progress that my proposal would conflict with?
• Can x-pack/plugins/reporting remain where it is throughout 8.x?

[tsullivan]

I looked more at the x-pack/plugins/features/server/plugin.ts file and found a reason that this proposal will not work as a simple change. The OSS features are getting registered in the start cycle of features: https://github.com/elastic/kibana/blob/3b3327d/x-pack/plugins/features/server/plugin.ts#L134 because it uses coreStart.savedObjects when building the feature definitions.

If a separate plugin were to register the features instead, it would have to do it in the setup cycle of the plugin since features locks the registry to the outside world. If the features are registered at setup, we can not use coreStart.savedObjects to do build the feature definitions.

[mshustov]

But if the right thing means breaking up the features plugin and the reporting plugin, that means that Reporting is blocked from using Kibana access controls until 2 mountains get moved.

It doesn't have to wait for such a long time, but we need to understand the desirable final state before introducing a temporary workaround. Otherwise, we can end up in a situation when this workaround doesn't allow us to refactor code later.

I need a thing that can provide extension points for Reporting plugin, which is what this proposal is about.

As a temporary measurement features plugin can provide an extension point for Reporting plugin to register itself during setup phase. Then Features plugin uses this flag when calling buildOSSFeatures. Does it unblock your work?

I've got a question about overall privilege registration logic: Why OSS plugins register reporting privilege without declaring a dependency on the reporting plugin? Do they render Reporting-related UI components (Generate Export button) if Reporting plugin is disabled?

[tsullivan]

As a temporary measurement features plugin can provide an extension point for Reporting plugin to register itself during setup phase. Then Features plugin uses this flag when calling buildOSSFeatures. Does it unblock your work?

One thing that may not have come clear: the area that provides an extension point for Reporting needs to have a dependency on Reporting. There will be an xpack.reporting config setting that controls using the legacy access control model, or the new one. Since a plugin can only read its own config settings, Reporting will have a public API to other plugins to share the value of the config. I have a draft PR showing what this looks like for Canvas: https://github.com/elastic/kibana/pull/94966/files#diff-9b73516da6dea3b037aab96c2c0b01c721043162dc84de409bd1f4d87a6597c0R26

But this should help explain why the features plugin should not provide the extension point for Reporting - it creates a circular dependency because features is a low-level plugin and it can not depend on a "medium" level plugin like Reporting. I illustrated the circle in the opening description: "Reporting also depends on security, which also depends on features. That makes an unavoidable set of circular dependencies..."

I've got a question about overall privilege registration logic: Why OSS plugins register reporting privilege without declaring a dependency on the reporting plugin? Do they render Reporting-related UI components (Generate Export button) if Reporting plugin is disabled?

Currently, there is much reorganization work to be done. Reporting-releated UI components are mostly in the Reporting domain:

• x-pack/plugins/reporting/public/share_context_menu: these are the share context menu components, used by the OSS apps for the "Generate CSV", "Generate PNG", "Generate PDF". The CSV sharing button is only used by Discover, and the PNG/PDF buttons are used only by Visualize Editor and Dashboard
• x-pack/plugins/reporting/public/panel_actions: This is the "Download CSV" button that shows for a Saved Search embeddable, only used in Dashboard

If Reporting is disabled, the Reporting-related UI components don't appear because they are registered in the Reporting domain. This is the kind of tech debt I brought up when I wrote:

In the longer term, having a new plugin could help Reporting with other technical debt. Reporting has code that is specific to OSS applications could be moved to the new plugin.

Side note: I wrote "mostly" because Canvas has a bug in that it has some dedicated Reporting-related UI components in its domain, but it does not have a dependency on reporting in its kibana.json. So those Reporting-specific components do get rendered if Reporting is disabled. The draft PR I have posted fixes that: https://github.com/elastic/kibana/pull/94966/files#diff-23ae3fa3782a89033ab95acf9bc5e1823a93d37df1fe346ba9c75f1f95ee5de5R83

[tsullivan]

Reporting plugin might move to src/plugins keeping Gold+ features in x-pack or these plugins can provide extension points for Reporting plugin.

I am starting to warm up to this idea, and I see how it makes sense as the "right" way to organize the code as preparation for #19914

We could have src/plugins/reporting and x-pack/plugins/reporting.

In src/plugins/reporting will be:

• ExportTypeRegistry, with a public PluginSetup API to allow other plugins to register their export types
• The CSV export type definition, which is under the Basic license.
• The config schema and the API routes
  • complication: (some of the config schema is specific to Platinum features [xpack.reporting.capture.browser])
  • complication: the API route handlers need to check for access privilege
• Registering the Reporting tasks in Task Manager that run the job queue

In xpack/plugins/reporting will be:

• The browser screenshot library
• The PDF and PNG export type definitions

[tsullivan]

Thanks @mshustov for reaching out to me to clarify your thoughts.

My suggestion was to add to features plugin a method, something like hasReportingEnabled, which can be called by Reporting plugin to let features plugin know that it’s enabled.

This suggestion is clear. It will enable me to continue work getting the features plugin to register the new sub-privileges for Reporting only when reporting plugin is enabled.

[tsullivan]

This PR attempts to implement the plan.

This diagram shows how the different plugins can be connected together: