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

By default, a plugin should not be disable-able #89584

Closed
stacey-gammon opened this issue Jan 28, 2021 · 34 comments · Fixed by #113495
Closed

By default, a plugin should not be disable-able #89584

stacey-gammon opened this issue Jan 28, 2021 · 34 comments · Fixed by #113495
Assignees
Labels
Breaking Change Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.0.0

Comments

@stacey-gammon
Copy link
Contributor

stacey-gammon commented Jan 28, 2021

In an effort to create a more stable and well-tested architecture, starting in 8.0 we will require every plugin to explicitly opt-in to the ability to be disabled via the yml config file.

We have many small plugins that were not built with this capability in mind, and we don't test all of the possible permutations. Moving forward, we want developers to intentionally choose this feature for their plugin.

To make a plugin disable-able, you add an enabled setting in your plugin's config:

// myplugin/server/index.ts
export const config = {
  schema: schema.object({
    enabled: schema.boolean({ defaultValue: true }),
  }),
};

Plugins without enabled in their config will be turned on by default and cannot be disabled in the Kibana yml config or cli.

Plugins without any config schema implicitly have enabled added, however we will be removing this in 8.0.

Here is a list of plugins which currently specify an enabled config. We believe the vast majority of these do not have a strong need to support disabling. However, if you own a plugin listed below and you believe it should support disabling, please add a yes to the table.

Additionally, if you own a plugin which isn't on this list because it doesn't have a config schema (as in Josh's list below), please speak up if you are relying on enabled being added to your plugin implicitly. Otherwise, your plugin will no longer be disable-able in 8.0.

Toward the end of the 7.16 dev cycle, we will open a PR removing enabled from any plugins that don't have a yes indicated below. To prepare for this update, we'd ask that folks review the affected plugins by 31 August 2021.

Plugin Owner Must be disable-able in 8.0?
apm_oss @elastic/apm-ui
console @elastic/kibana-stack-management no
interactive_setup @elastic/kibana-security yes
newsfeed @elastic/kibana-core yes
telemetry @elastic/kibana-core yes
timelion @elastic/kibana-app removed in 7.16
vis_type_markdown @elastic/kibana-presentation yes
vis_type_metric @elastic/kibana-app yes
vis_type_table @elastic/kibana-app yes
vis_type_tagcloud @elastic/kibana-app yes
vis_type_timelion @elastic/kibana-app yes
vis_type_timeseries @elastic/kibana-app yes
vis_type_vega @elastic/kibana-app yes
vis_type_vislib @elastic/kibana-app yes
vis_type_pie @elastic/kibana-app yes
x-pack/actions @elastic/kibana-alerting-services no
x-pack/alerting @elastic/kibana-alerting-services no
x-pack/apm @elastic/apm-ui
x-pack/cases @elastic/security-threat-hunting
x-pack/cloud @elastic/kibana-core no
x-pack/cross_cluster_replication @elastic/kibana-stack-management no
x-pack/dashboard_mode @elastic/kibana-presentation no
x-pack/encrypted_saved_objects @elastic/kibana-security no
x-pack/enterprise_search @elastic/enterprise-search-frontend no
x-pack/event_log @elastic/kibana-alerting-services no
x-pack/fleet @elastic/fleet no
x-pack/graph @elastic/kibana-app no
x-pack/grokdebugger @elastic/kibana-stack-management no
x-pack/index_lifecycle_management @elastic/kibana-stack-management no
x-pack/index_management @elastic/kibana-stack-management no
x-pack/infra @elastic/logs-metrics-ui no
x-pack/lens @elastic/kibana-app no
x-pack/license_management @elastic/kibana-stack-management no
x-pack/lists @elastic/security-detections-response
x-pack/logstash ?
x-pack/maps @elastic/kibana-gis no
x-pack/metrics_entities @elastic/security-detections-response (?)
x-pack/monitoring @elastic/stack-monitoring-ui no
x-pack/observability @elastic/observability-ui no
x-pack/osquery @elastic/security-asset-management no
x-pack/remote_clusters @elastic/kibana-stack-management no
x-pack/reporting @elastic/kibana-app-services yes
x-pack/rollup @elastic/kibana-stack-management no
x-pack/rule_registry @elastic/security-detections-response (?) no
x-pack/saved_objects_tagging @elastic/kibana-core no
x-pack/security @elastic/kibana-security no
x-pack/security_solution @elastic/security-solution
x-pack/snapshot_restore @elastic/kibana-stack-management no
x-pack/spaces @elastic/kibana-security no
x-pack/stack_alerts @elastic/kibana-alerting-services no
x-pack/task_manager @elastic/kibana-alerting-services no
x-pack/timelines @elastic/security-threat-hunting
x-pack/upgrade_assistant @elastic/kibana-stack-management no
@stacey-gammon stacey-gammon added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.0.0 Breaking Change labels Jan 28, 2021
@elasticmachine
Copy link
Contributor

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

@lukeelmers
Copy link
Member

related #66621

@rudolf
Copy link
Contributor

rudolf commented Feb 1, 2021

Reducing the amount of permutations (and all the test cases to cover these permutations) is a good idea, we can get a lot of value by doing this for spaces, security, encrypted saved objects etc.

However, I don't believe this should be the default. Every plugin that can't be disabled poses an operational risk for users:

  1. Disabling the plugin is often the only way to mitigate a security vulnerability [discuss] Remove the ability to disable plugins in production #66621 (comment)
  2. Disabling a plugin is the fastest way to work around a migration bug. If e.g. SIEM's migrations fail because of a bug (or corrupt data) a sysadmin can decide to disable the plugin and proceed with the upgrade because SIEM isn't used or it's not a critical part of their infrastructure. When users upgrade from minors it's relatively easy to do a rollback if a migration fails, but when you upgrade from e.g. 6.x to 7.x you have to upgrade Elasticsearch first meaning you can't rollback without rolling back Elasticsearch and loosing all data that's been written since you started the upgrade.
  3. Disabling a plugin allows users to temporarily stop a plugin that's e.g. causing stability problems like excessive load on ES, slow response times from Kibana, a memory leak in Kibana (Constantly memory leaking on x-pack.reporting #90274).
  4. (Sync lifecycles probably reduces this risk sufficiently, but I've seen SDH's where we had to disable plugins to allow Kibana to start up because they were blocking on requests which always timedout)

Kibana is a gigantic monolith which comes with all the operational downsides of running a monolith. With Kibana being used increasingly for mission critical work loads these downsides have a bigger impact on users. Being able to enable/disable some plugins mitigates some of these downsides e.g. it's theoretically possible to scale up alerting-only Kibana instances, or only disable alerting if that's causing stability problems for other systems.

So I would prefer if plugins had to opt-out of being disable-able, and I think we should evaluate plugins on a case by case basis to weigh up the benefits/risk.

@joshdover
Copy link
Contributor

joshdover commented Feb 1, 2021

Reducing the amount of permutations (and all the test cases to cover these permutations) is a good idea, we can get a lot of value by doing this for spaces, security, encrypted saved objects etc.

However, I don't believe this should be the default. Every plugin that can't be disabled poses an operational risk for users:

If the issue with keeping the ability to disable these items is testing & support, maybe we need to make it more clear that running with disabled plugins is not supported. Yes, it's a workaround that may work for some situations detailed above, however it's not something we test for, and we shouldn't rely on this feature working without breaking other things if we aren't testing for it. IMO, at the very least there should be a warning logged when disabling a plugin that we don't test for.

I also think we can find other, well-tested solutions to most of the problems above that don't involve drastically changing which plugins are executing in production.

The exception being the security vulnerability case, and while I can see how that is a useful defense, is it an explicit part of our security model? Are there other ways we can achieve this based on the patterns we've seen? For instance, if disabling plugins is usually used to disable vulnerable REST endpoints, we could add a feature for disabling some route paths instead.

it's theoretically possible to scale up alerting-only Kibana instances, or only disable alerting if that's causing stability problems for other systems.

I'd prefer we explicitly add features that are well-tested to support dedicated node types over having this feature exist by accident. What good is a feature if it may be breaking between releases?

Overall, I do agree this functionality is needed in some cases, which is why I generally support this proposal over #66621. However, in the cases where we need this, we still need tests & verifications across versions to ensure we don't break configuration. If we agree that there is extra work that should be done for a plugin to be disable-able, then I think the default should reflect that.

@rudolf
Copy link
Contributor

rudolf commented Feb 1, 2021

The exception being the security vulnerability case, and while I can see how that is a useful defense, is it an explicit part of our security model?

I suspect we would always fix vulnerabilities in supported releases, so strictly speaking we don't need to provide the ability to disable a plugin to fix a vulnerability, the real fix is to upgrade Kibana. When Kibana ships with 100+ plugins it becomes increasingly likely that some plugin you don't use has a vulnerability and having to upgrade just for that is rather annoying. However, upgrading for security vulnerabilities is definitely the norm in the industry and as long as we make this process as seamless as possible maybe the impact/frustration/cost on users is negligible.

I'd prefer we explicitly add features that are well-tested to support dedicated node types over having this feature exist by accident. What good is a feature if it may be breaking between releases?

I agree, we should rather provide a more stream-lined and better documented way to achieve this.

If we agree that there is extra work that should be done for a plugin to be disable-able, then I think the default should reflect that.

My concern is that most plugins would just opt to stick with the default. I agree there's probably better ways to address the problems I mentioned. But short term this would reduce the amount of options users and support have to fix/stabilize a Kibana deployment.

I agree that in principle this default would reduce the amount of bugs we get from untested permutations. But is this currently a notable problem, I'm not aware of it impacting users. So my bias is towards the SDH's I've been part of where disabling a plugin was a quick win to get a deployment back up and running.

@legrego
Copy link
Member

legrego commented Feb 1, 2021

The exception being the security vulnerability case, and while I can see how that is a useful defense, is it an explicit part of our security model?

I don't think it's an explicit part of our security model, but it us a useful mitigation to protect our users. @joshbressers / @douglasday do you have an opinion on this? Do you/our users find it helpful that we can mitigate certain vulnerabilities by disabling plugins, or is this something that just "feels" like a good thing to offer?

Are there other ways we can achieve this based on the patterns we've seen?. For instance, if disabling plugins is usually used to disable vulnerable REST endpoints, we could add a feature for disabling some route paths instead.

I'm all for exploring other options, but I think it needs to include an answer for client-side as well. The reports we get are a mixed bag of front-end and back-end, so disabling REST endpoints (for example) might not help as much as we'd like.

@joshbressers
Copy link

I wanted to close this loop (Larry asked me about this in a security team meeting).

I'm comfortable not disabling plugins. We do provide mitigation advice to disable certain functionality for some security vulnerabilities, but I don't suspect this is widely used (I get very few comments on the feedback, I recall in the past there have been some instructions that broke things and we only heard from one or two people).

@joshdover
Copy link
Contributor

@kobelb @stacey-gammon Are we still comfortable moving forward with this?

@kobelb
Copy link
Contributor

kobelb commented Jul 1, 2021

@kobelb @stacey-gammon Are we still comfortable moving forward with this?

Yup!

@lukeelmers
Copy link
Member

lukeelmers commented Jul 6, 2021

Next steps:

  • Reach out to Kibana contributors to confirm which plugins need to be disable-able
  • Look at telemetry to check current usage
  • Check in with @thesmallestduck about any other known usages
  • Move forward with changing config defaults

@lukeelmers
Copy link
Member

lukeelmers commented Aug 10, 2021

Just updated the description here with a table of all of the plugins which currently are disable-able. To the teams I tagged, please update the table according to the instructions above if you believe your plugin must continue to be disable-able in 8.0. We'd ask that you do this by 31 August 2021.

@joshdover
Copy link
Contributor

Just updated the description here with a table of all of the plugins which currently are disable-able.

Did some digging and it appears we do add the enabled config by default in the cases where a plugin doesn't define any config schema. For instance, the src/plugins/field_formats plugin does not have any config schema defined, but you can start Kibana with --field_formats.enabled=false and every plugin that has a transitive dependency on this plugin will be disabled. This logic is performed here:

// if plugin hasn't got a config schema, we try to read "enabled" directly
const isEnabled = validatedConfig?.enabled ?? config.get(enabledPath);

I put a little logic to log this case and found that all of these plugin config paths currently do not define a config schema and thus have this enabled option by default:

Plugin does not have config schema at path: advanced_settings
Plugin does not have config schema at path: bfetch
Plugin does not have config schema at path: charts
Plugin does not have config schema at path: dev_tools
Plugin does not have config schema at path: discover
Plugin does not have config schema at path: embeddable
Plugin does not have config schema at path: es_ui_shared
Plugin does not have config schema at path: expression_error
Plugin does not have config schema at path: expression_image
Plugin does not have config schema at path: expression_metric
Plugin does not have config schema at path: expression_repeat_image
Plugin does not have config schema at path: expression_reveal_image
Plugin does not have config schema at path: expression_shape
Plugin does not have config schema at path: expressions
Plugin does not have config schema at path: field_formats
Plugin does not have config schema at path: index_pattern_editor
Plugin does not have config schema at path: index_pattern_field_editor
Plugin does not have config schema at path: index_pattern_management
Plugin does not have config schema at path: input_control_vis
Plugin does not have config schema at path: inspector
Plugin does not have config schema at path: kibana_overview
Plugin does not have config schema at path: kibana_react
Plugin does not have config schema at path: kibana_usage_collection
Plugin does not have config schema at path: kibana_utils
Plugin does not have config schema at path: legacy_export
Plugin does not have config schema at path: management
Plugin does not have config schema at path: navigation
Plugin does not have config schema at path: presentation_util
Plugin does not have config schema at path: region_map
Plugin does not have config schema at path: saved_objects
Plugin does not have config schema at path: saved_objects_management
Plugin does not have config schema at path: saved_objects_tagging_oss
Plugin does not have config schema at path: screenshot_mode
Plugin does not have config schema at path: share
Plugin does not have config schema at path: spaces_oss
Plugin does not have config schema at path: telemetry_collection_manager
Plugin does not have config schema at path: telemetry_management_section
Plugin does not have config schema at path: tile_map
Plugin does not have config schema at path: ui_actions
Plugin does not have config schema at path: url_forwarding
Plugin does not have config schema at path: vis_default_editor
Plugin does not have config schema at path: vis_type_pie
Plugin does not have config schema at path: vis_type_xy
Plugin does not have config schema at path: visualizations
Plugin does not have config schema at path: visualize
Plugin does not have config schema at path: xpack.canvas
Plugin does not have config schema at path: xpack.dashboardEnhanced
Plugin does not have config schema at path: data_visualizer
Plugin does not have config schema at path: url_drilldown
Plugin does not have config schema at path: embeddable_enhanced
Plugin does not have config schema at path: xpack.features
Plugin does not have config schema at path: file_upload
Plugin does not have config schema at path: xpack.global_search_bar
Plugin does not have config schema at path: xpack.global_search_providers
Plugin does not have config schema at path: xpack.grokdebugger
Plugin does not have config schema at path: xpack.ingest_pipelines
Plugin does not have config schema at path: xpack.licenseApiGuard
Plugin does not have config schema at path: xpack.ml
Plugin does not have config schema at path: xpack.painless_lab
Plugin does not have config schema at path: xpack.runtime_fields
Plugin does not have config schema at path: xpack.searchprofiler
Plugin does not have config schema at path: telemetry_collection_xpack
Plugin does not have config schema at path: xpack.transform
Plugin does not have config schema at path: x-pack.translations
Plugin does not have config schema at path: xpack.ui_actions_enhanced
Plugin does not have config schema at path: xpack.uptime
Plugin does not have config schema at path: xpack.watcher
Plugin does not have config schema at path: xpack_legacy

@lukeelmers
Copy link
Member

lukeelmers commented Aug 10, 2021

in the cases where a plugin doesn't define any config schema

Ah, that explains it then -- I had tested around plugins that already had a config schema. Thanks for clarifying!

@kobelb
Copy link
Contributor

kobelb commented Aug 23, 2021

Hey @stratoula, I noticed that you marked a bunch of the plugins prefixed with vis_type_ as continuing to be disableable in 8.0. Do you mind elaborating on why you think users should be able to explicitly disable them? Are these commonly disabled?

@stratoula
Copy link
Contributor

stratoula commented Aug 24, 2021

Hey, @kobelb we haven't checked the telemetry but we discussed it with @timroes and we think that the legacy visualizations should be disableable. For example, a user works only with Lens and doesn't want the other editors. (this is the reason that Lens is also not disableable). I can also think of users that don't want vega as an option. We think that as there are a lot of ways to create the same types of visualizations in kibana (for example you can have a legacy pie and a Lens pie), a user might want to disable some of them to avoid confusion.

@lukeelmers
Copy link
Member

lukeelmers commented Aug 31, 2021

Thanks to all who took the time to update the list above. After comparing this list with the list of implicitly disable-able plugins and following up with teams, we ended up with the following plugins which need to remain disable-able:

  • graph (@timroes @stratoula are we certain about this one too?)
  • interactive_setup
  • newsfeed
  • reporting
  • telemetry
  • vis_type_* plugins

Other than the above, no other plugins will be disable-able beginning in 8.0 (unless they explicitly opt-in by adding this to their config schema).

If you own a plugin which you still believe should be disable-able, and it is not in the list above, please speak up by the end of this week! (03 Sept 2021)

Otherwise, we will be moving forward with logging deprecation warnings for the rest of the plugins in 7.16. #110614

@stratoula
Copy link
Contributor

@lukeelmers we discussed it again with Tim and we are fine if this one is not disableable. You can remove it from the list ;)

@azasypkin
Copy link
Member

If you own a plugin which you still believe should be disable-able, and it is not in the list above, please speak up by the end of this week! (03 Sept 2021)

We've just discussed this during Kibana Platform Security Sync and agreed that we'd like to keep interactiveSetup plugin disableable in 8.0. This plugin, when enabled, might have certain security implications and we'd like users to be able to completely disable it should they need to. The cost of maintaining of two possible states for this plugin is reasonably low as well.

@mshustov
Copy link
Contributor

An implementation-wise question: as we want to enforce this rule for the internal plugins-only, can we add a dedicated flag to the manifest file to differentiate between 1st party and 3rd party plugins? It will help us in the future to add more strict / relax rules for the Elastic plugins (like this one #61087)

@lukeelmers
Copy link
Member

@azasypkin Yeah, agreed it makes sense to keep interactiveSetup disable-able. I updated the list above.

we want to enforce this rule for the internal plugins-only

@mshustov I'm not entirely sure this is the case -- once we remove the implicitly added enabled option that Josh mentions above, this would also affect 3rd party plugins too. But I'll move this thread over to #110614 and add some thoughts there for us to discuss implementation further.

@cjcenizal
Copy link
Contributor

cjcenizal commented Sep 27, 2021

@lukeelmers Do you recall why it was decided that the cloud plugin should not be disable-able? Why would an on-prem admin want this plugin enabled?

@lukeelmers
Copy link
Member

Do you recall why it was decided that the cloud plugin should not be disable-able? Why would an on-prem admin want this plugin enabled?

@cjcenizal We didn't feel it needed to be disable-able because the main purpose of the plugin is to (A) check if this Kibana deployment is running on Cloud, and (B) provide information about the deployment if it is in fact running on Cloud.

For on-prem users, the plugin effectively does nothing: it will start up, determine cloud isn't enabled (this doesn't require reaching out over the internet; it derives this from the yml config), and return empty/undefined values from its public contract.

@lukeelmers
Copy link
Member

This issue will be closed as soon as #113495 merges.

Just to close the loop here for clarity: with these changes, none of the plugins that ship with Kibana will be disable-able starting in 8.0, with the exception of the following:

  • interactive_setup
  • newsfeed
  • reporting
  • telemetry
  • vis_type_* plugins

Thanks all for your input as we worked to coordinate these changes ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.0.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.