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

[Meta] Make spaces plugin optional #149687

Closed
thomheymann opened this issue Jan 27, 2023 · 24 comments
Closed

[Meta] Make spaces plugin optional #149687

thomheymann opened this issue Jan 27, 2023 · 24 comments
Assignees
Labels
Feature:Security/Authorization Platform Security - Authorization impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. :ml Team:EnterpriseSearch Team:Fleet Team label for Observability Data Collection Fleet team Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services Team:ML Team label for ML (also use :ml) Team:Observability Team label for Observability Team (for things that are handled across all of observability) Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability Team:Visualizations Visualization editors, elastic-charts and infrastructure

Comments

@thomheymann
Copy link
Contributor

thomheymann commented Jan 27, 2023

Summary

Spaces is currently a hard dependency on multiple of our plugins and solutions.

In order to better support future offerings we need to turn it into an optional dependency.

The purpose of this meta issue is to track our progress in converting all hard dependencies of spaces into optional dependencies.

Status

Link to PRs:

How to disable spaces

#149044 has reintroduced a config option to disable spaces:

xpack.spaces.enabled: false

This config option is only available in development mode while we are in the process of updating our plugins.

How to make spaces optional

  1. Move spaces from requiredPlugins to optionalPlugins in your kibana.json file:
{
  "requiredPlugins": [
-    "spaces"
  ],
  "optionalPlugins": [
+    "spaces"
  ]
}
  1. Update setup and start contracts making spaces optional (both server and public):
export interface AlertingPluginsStart {
-  spaces: SpacesPluginStart;
+  spaces?: SpacesPluginStart;
}
  1. If your plugin requires a space ID to construct saved object IDs or for other purposes use DEFAULT_SPACE_ID constant from @kbn/spaces-plugin/common package when spaces isn't available:
+ import { DEFAULT_SPACE_ID } from '@kbn/spaces-plugin/common';

function getSpaceId(request: KibanaRequest) {
-  return plugins.spaces.spacesService.getSpaceId(request);
+  return plugins.spaces?.spacesService.getSpaceId(request) ?? DEFAULT_SPACE_ID;
}

An example of these changes can be found in the alerting plugin here: https://github.com/elastic/kibana/pull/149044/files#diff-6e9c0cbc28a51ad0bc9251d157a956f612df837a853cff661edc204d9bde8e14

@thomheymann thomheymann added Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! Feature:Security/Authorization Platform Security - Authorization labels Jan 27, 2023
@thomheymann thomheymann self-assigned this Jan 27, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-security (Team:Security)

@thomheymann thomheymann added :ml Team:Visualizations Visualization editors, elastic-charts and infrastructure Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services Team:Observability Team label for Observability Team (for things that are handled across all of observability) Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Team:ML Team label for ML (also use :ml) Team:EnterpriseSearch labels Jan 31, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-visualizations @elastic/kibana-visualizations-external (Team:Visualizations)

@elasticmachine
Copy link
Contributor

Pinging @elastic/unified-observability (Team:Observability)

@thomheymann
Copy link
Contributor Author

@elastic/response-ops Could you please remove the hard dependency on spaces in x-pack/plugins/cases?

@elastic/enterprise-search-frontend Could you please remove the hard dependency on spaces in x-pack/plugins/enterprise_search?

@elastic/observability-design - Could you please remove the hard dependency on spaces in x-pack/plugins/fleet and x-pack/plugins/observability?

@elastic/infra-monitoring-ui - Could you please remove the hard dependency on spaces in x-pack/plugins/infra?

@elastic/kibana-visualizations - Could you please remove the hard dependency on spaces in x-pack/plugins/lens?

@elastic/ml-ui - Could you please remove the hard dependency on spaces in x-pack/plugins/ml?

@elastic/security-defend-workflows - Could you please remove the hard dependency on spaces in x-pack/plugins/osquery?

@elastic/uptime - Could you please remove the hard dependency on spaces in x-pack/plugins/synthetics?

Let me know if there are any questions.

@cnasikas
Copy link
Member

cnasikas commented Jan 31, 2023

https://github.com/orgs/elastic/teams/response-ops Could you please remove the hard dependency on spaces in x-pack/plugins/cases?

Ok @thomheymann! Is this for 8.8?

@stratoula
Copy link
Contributor

@thomheymann Lens configures spaces as optionalPlugins already https://github.com/elastic/kibana/blob/main/x-pack/plugins/lens/kibana.json#L40
Do you need something else to do?

@formgeist
Copy link
Contributor

@thomheymann I've re-assigned the x-pack/plugins/observability work to @elastic/observability-ui since @elastic/observability-design mostly just owns the UI/UX side of the Observability plugin (mainly the overview page). cc @jasonrhodes can you perhaps follow up with the UI team to find a suitable engineer to help out?

@shahzad31
Copy link
Contributor

instead of changing the code in each plugin like this

return plugins.spaces?.spacesService.getSpaceId(request) ?? DEFAULT_SPACE_ID

why not return DEFAULT_SPACE_ID from getSpaceId ??

@pgayvallet
Copy link
Contributor

why not return DEFAULT_SPACE_ID from getSpaceId ??

Because when the plugin is disabled, plugins.spaces will be undefined.

@thomheymann
Copy link
Contributor Author

thomheymann commented Feb 1, 2023

Is this for 8.8?

@cnasikas Yes, 8.8 is fine!

stratoula added a commit that referenced this issue Feb 2, 2023
## Summary
Part of #149687

Lens defines the space plugin as optional. I am fixing the types here in
order to be also optional on the contract.
@drewdaemon drewdaemon added the impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. label Feb 2, 2023
cnasikas added a commit that referenced this issue Feb 9, 2023
## Summary

This PR makes the spaces plugin an optional dependency for the cases
plugin.

Related: #149687

### Checklist

Delete any items that are not applicable to this PR.

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

### For maintainers

- [x] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
@serenachou
Copy link

Filed https://github.com/elastic/enterprise-search-team/issues/3941 to track the ent-search work.

@jasonrhodes
Copy link
Member

why not return DEFAULT_SPACE_ID from getSpaceId ??

Because when the plugin is disabled, plugins.spaces will be undefined.

@pgayvallet just out of curiosity, do you (or anyone else here) know if we considered not reintroducing the enabled/disabled plugin flag that we've mostly removed from the plugin ecosystem, and instead introduced an "on/off" "active/inactive" type flag that would've preserved the getSpaceId() API and just returned the default space ID as @shahzad31 suggested?

@jasonrhodes
Copy link
Member

With so many interlocking dependencies, this seems like it is going to be incredibly tricky to test. I've made the plugin optional for the observability plugin and then used the above config to disable spaces. Unfortunately, this makes the UI restricted to only the Analytics and Management sections in Kibana. I understand that other plugins that have not yet made spaces optional will not load while spaces is disabled, however, it seems that even my own plugin will not be shown in the UI if any of my required plugins still require spaces.

Is there a way to identify the plugins which most others rely on and get them converted first? Perhaps that's already happened? Observability relies on the following required plugins:

"alerting",
"cases",
"charts",
"data",
"dataViews",
"features",
"inspector",
"ruleRegistry",
"triggersActionsUi",
"inspector",
"unifiedSearch",
"security",
"guidedOnboarding",
"share"

I haven't checked each one's kibana.jsonc file, but I'm curious how other plugins are testing this change?

@shahzad31
Copy link
Contributor

i also agree with @jasonrhodes i think we should avoid again this enabled/disable stuff. If spaces isn't available. Spaces should return default from the methods it exposes but it should always be available for plugin contracts.

though i don't know what's the scope of this change. Are there any other plugins that are being made optional?

@thomheymann
Copy link
Contributor Author

thomheymann commented Feb 13, 2023

@pgayvallet just out of curiosity, do you (or anyone else here) know if we considered not reintroducing the enabled/disabled plugin flag that we've mostly removed from the plugin ecosystem, and instead introduced an "on/off" "active/inactive" type flag that would've preserved the getSpaceId() API and just returned the default space ID as @shahzad31 suggested?

Yes, we have considered this option but it wouldn't reduce complexity or the amount of effort involved since we would still have to update the UI to show/hide spaces across all of Kibana/solutions depending on whether this newly introduced flag is enabled or not. xpack.spaces.enabled gives this to us for free so we'd essentially just be replicating what it's already doing. Using the existing plugin based architecture makes more sense than introducing a new config option in this case.

Is there a way to identify the plugins which most others rely on and get them converted first? Perhaps that's already happened?

Which plugins are you currently blocked by? You can see why your plugin was disabled when Kibana starts. (Might require verbose logging)

I think you will find the list of plugins that have a hard dependency on spaces a lot smaller than you think.

Have a look at this POC which makes spaces optional across all of Kibana as a reference: 6f26e8f#diff-7a779413cc407a29027dc5c4ece33f5959046c4cbaad5f6a3e309b657b9929d1

@pgayvallet
Copy link
Contributor

pgayvallet commented Feb 14, 2023

Is there a way to identify the plugins which most others rely on and get them converted first? Perhaps that's already happened?

FWIW, the relevant console output from current main when disabling the space plugin:

[2023-02-14T09:58:50.794+01:00][INFO ][plugins-service] Plugin "apm" has been disabled since the following direct or transitive dependencies are missing, disabled, or have incompatible types: [infra, observability]
[2023-02-14T09:58:50.800+01:00][INFO ][plugins-service] Plugin "cloudDefend" has been disabled since the following direct or transitive dependencies are missing, disabled, or have incompatible types: [fleet]
[2023-02-14T09:58:50.801+01:00][INFO ][plugins-service] Plugin "cloudChat" is disabled.
[2023-02-14T09:58:50.801+01:00][INFO ][plugins-service] Plugin "cloudExperiments" is disabled.
[2023-02-14T09:58:50.801+01:00][INFO ][plugins-service] Plugin "cloudFullStory" is disabled.
[2023-02-14T09:58:50.801+01:00][INFO ][plugins-service] Plugin "cloudGainsight" is disabled.
[2023-02-14T09:58:50.802+01:00][INFO ][plugins-service] Plugin "cloudSecurityPosture" has been disabled since the following direct or transitive dependencies are missing, disabled, or have incompatible types: [fleet]
[2023-02-14T09:58:50.809+01:00][INFO ][plugins-service] Plugin "enterpriseSearch" has been disabled since the following direct or transitive dependencies are missing, disabled, or have incompatible types: [spaces, infra]
[2023-02-14T09:58:50.809+01:00][INFO ][plugins-service] Plugin "fleet" has been disabled since the following direct or transitive dependencies are missing, disabled, or have incompatible types: [spaces]
[2023-02-14T09:58:50.813+01:00][INFO ][plugins-service] Plugin "infra" has been disabled since the following direct or transitive dependencies are missing, disabled, or have incompatible types: [spaces, observability]
[2023-02-14T09:58:50.825+01:00][INFO ][plugins-service] Plugin "monitoring" has been disabled since the following direct or transitive dependencies are missing, disabled, or have incompatible types: [observability]
[2023-02-14T09:58:50.826+01:00][INFO ][plugins-service] Plugin "observability" has been disabled since the following direct or transitive dependencies are missing, disabled, or have incompatible types: [spaces]
[2023-02-14T09:58:50.826+01:00][INFO ][plugins-service] Plugin "profiling" is disabled.
[2023-02-14T09:58:50.832+01:00][INFO ][plugins-service] Plugin "securitySolution" has been disabled since the following direct or transitive dependencies are missing, disabled, or have incompatible types: [cloudSecurityPosture]
[2023-02-14T09:58:50.833+01:00][INFO ][plugins-service] Plugin "spaces" is disabled.
[2023-02-14T09:58:50.834+01:00][INFO ][plugins-service] Plugin "synthetics" has been disabled since the following direct or transitive dependencies are missing, disabled, or have incompatible types: [observability, spaces]
[2023-02-14T09:58:50.840+01:00][INFO ][plugins-service] Plugin "ux" has been disabled since the following direct or transitive dependencies are missing, disabled, or have incompatible types: [infra, apm]

Or, "processed" (best I can do, we don't have a proper cycle output, but this is fairly sufficient):

// spaces
"spaces" is disabled.

// depending directly or transitively on spaces
"apm"  => [infra, observability]
"cloudDefend" =>  [fleet]
"cloudSecurityPosture" =>  [fleet]
"enterpriseSearch" =>   [spaces, infra]
"fleet" =>  [spaces]
"infra" =>  [spaces, observability]
"monitoring" =>  [observability]
"observability" =>  [spaces]
"securitySolution" =>  [cloudSecurityPosture]
"synthetics" =>  [observability, spaces]
"ux" =>  [infra, apm]

// disabled by default
"profiling" is disabled.
"cloudChat" is disabled.
"cloudExperiments" is disabled.
"cloudFullStory" is disabled.
"cloudGainsight" is disabled.

@jasonrhodes
Copy link
Member

Thanks, all. I think there is some other issue with how the Observability section appears in the nav because the plugin has not been disabled due to some transitive spaces dependency, yet it still doesn't appear. I'm going to dig into it in the PR later this week, but the draft PR is now submitted for the observability plugin.

jgowdyelastic added a commit that referenced this issue Feb 16, 2023
Changes to ensure ML behaves correctly when the spaces plugin is
disabled.
Related to #149687
Closes #149953

ML was in pretty good shape for this change as we already assumed spaces
could be optional.
The only change needed is to ensure the saved object sync button is
enabled on the management page even when spaces is missing.
Previously we incorrectly hid this button.
@thomheymann thomheymann added the Team:Fleet Team label for Observability Data Collection Fleet team label Feb 21, 2023
@clintandrewhall
Copy link
Contributor

clintandrewhall commented Feb 22, 2023

why not return DEFAULT_SPACE_ID from getSpaceId ??

Because when the plugin is disabled, plugins.spaces will be undefined.

@pgayvallet just out of curiosity, do you (or anyone else here) know if we considered not reintroducing the enabled/disabled plugin flag that we've mostly removed from the plugin ecosystem, and instead introduced an "on/off" "active/inactive" type flag that would've preserved the getSpaceId() API and just returned the default space ID as @shahzad31 suggested?

@thomheymann @pgayvallet I have to echo this. Spaces was introduced as a constant, and making it optional is incredibly unstable and disruptive, particularly with some of the ideas we have on the horizon. Why not just reduce the space count to (1) and use the default space? Then the changes can be isolated to the Spaces UX (if space count <= 1, don't render Spaces UX)...??

Why is disabling the plugin a hard requirement?

@thomheymann
Copy link
Contributor Author

Spaces was introduced as a constant

@clintandrewhall Spaces was introduced as an optional dependency and has remained so throughout the entire v7.x release line. It was turned into an always-on plugin in v8.x. Most plugins never updated the dependency and kept spaces as optional even in 8.x.

Making it optional is incredibly unstable and disruptive

As you can see by the PRs linked to this meta issue it takes only a few line changes to turn spaces into an optional dependency - That can hardly be described as "disruptive":

Screenshot 2023-02-22 at 15 48 26Screenshot 2023-02-22 at 15 48 08Screenshot 2023-02-22 at 15 48 57

It is not unreasonable to update the 10 plugins that have a hard dependency on spaces.

particularly with some of the ideas we have on the horizon.

What ideas are you referring to?

Why not just reduce the space count to (1) and use the default space? Then the changes can be isolated to the Spaces UX (if space count <= 1, don't render Spaces UX)...??

I have already answered this question here but to reiterate, some of our plugins render spaces specific components directly in their UI. Even if we update the spaces plugin and introduce a new flag to virtually "disable" it, changes wouldn't be isolated to spaces and all these other plugins would still need to be updated to conditionally render those components. I don't think hacking around the problem here and trying to hide the fact that spaces will not exist on serverless is the right solution here. Kibana uses a plugin based architecture for exactly this purpose so I think it makes sense to utilise that.

@jasonrhodes
Copy link
Member

FWIW, I think the nervousness you are hearing from myself (and possibly from Clint and Shahzad and others) is a function of the fact that we deliberately moved away from making plugins optional in the 8.x release in part because of how difficult it is to test the infinite permutations of enabled/disabled, and seeing this creep back into the product here feels like a backward step that could provide precedent for more of these situations. Even with just this one, I think we will almost certainly run into unforeseen knock-on effects that we don't anticipate that can't be caught by types.

In other words, I don't think that the disruption here is limited to "a few line changes to turn spaces into an optional dependency", and the complexity that is introduced by allowing for this possibility is difficult to fully predict.

changes wouldn't be isolated to spaces and all these other plugins would still need to be updated to conditionally render those components

I assume this means that there are other parts of the Spaces plugin's API that are in use, that would also need to be "mocked" or otherwise handled, if the plugin wasn't fully disabled, is that right? That definitely sounds like a reasonable concern and I'm not familiar with the full surface area of the spaces plugin API. I do think that both of these options have an element of "hacking around the problem", unfortunately. The bigger question for me, at least, is how much complexity do we have to adopt for the future with (a) always accounting for the if/else of whether spaces is enabled, and (b) potentially encouraging other plugins to reintroduce the "optional dependency problem" that we had usefully moved away from.

@sophiec20
Copy link
Contributor

Setting aside implementation detail just for this comment - to avoid test permutation explosion, we do not intend to support Spaces as an optional feature for our on-prem and ESS deployments. As far as on-prem/ESS is concerned, the supported configuration includes Spaces.

@azasypkin
Copy link
Member

@thomheymann summarized our stance quite well. I just want to add my two cents from a security point of view: Spaces is also a security feature that we know should not be available in certain Kibana configurations.

The safe, future-proof, and industry-recognized way to ensure that consumers/plugins/solutions do not misuse this security feature is to not provide it at all when it is not supposed to be used. Having space.size <= 1 across the Kibana codebase is akin to having isUserAdmin or isNewOffering flags all over the place, which is neither scalable nor secure.

Making a plugin/dependency optional is not a hack and never was. It is a fundamental part of Kibana's Core plugin API that was intentionally designed from the beginning for cases like this one. I admit that making a dependency optional may be difficult for some plugins, and if you have a specific, non-hypothetical concern that certain changes might make your plugin "incredibly unstable", please come talk to @elastic/kibana-security and @elastic/kibana-core. This way, we can have more substantial, fact-based discussions and work out clear plans for specific teams on a case-by-case basis.

P.S. I was among the supporters of making the Spaces plugin required, but the reality has drastically changed since then.

jasonrhodes added a commit that referenced this issue Mar 1, 2023
## Summary

This PR makes spaces optional in the "observability" plugin as requested
in #149687

To test this, use the following setting in your kibana.yml config file:

```yaml
xpack.spaces.enabled: false
```

When you log in, there will be no spaces choice. 

<img width="2543" alt="Screenshot 2023-02-21 at 11 53 18 AM"
src="https://user-images.githubusercontent.com/159370/220409537-43a216d5-81c9-4b29-97d8-47705bdacd06.png">
bmorelli25 pushed a commit to bmorelli25/kibana that referenced this issue Mar 10, 2023
## Summary

This PR makes spaces optional in the "observability" plugin as requested
in elastic#149687

To test this, use the following setting in your kibana.yml config file:

```yaml
xpack.spaces.enabled: false
```

When you log in, there will be no spaces choice. 

<img width="2543" alt="Screenshot 2023-02-21 at 11 53 18 AM"
src="https://user-images.githubusercontent.com/159370/220409537-43a216d5-81c9-4b29-97d8-47705bdacd06.png">
@thomheymann
Copy link
Contributor Author

All plugins have been updated - Thanks everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Security/Authorization Platform Security - Authorization impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. :ml Team:EnterpriseSearch Team:Fleet Team label for Observability Data Collection Fleet team Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services Team:ML Team label for ML (also use :ml) Team:Observability Team label for Observability Team (for things that are handled across all of observability) Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability Team:Visualizations Visualization editors, elastic-charts and infrastructure
Projects
None yet
Development

No branches or pull requests