[Advanced settings] Add settings allowlist

[ElenaStoeva]

Addresses #160411

Summary

This PR adds functionality for filtering out advanced settings that are not relevant for serverless.

For context, we need to build an Advanced settings page in serverless which only contains a set of the existing settings. We will reuse the section registry (#163502) from the original Advanced settings plugin as well as its UI components which will also be extracted into a separate package. The app will be registered from inside the serverless plugin.

In order to only display the settings that are relevant for serverless, we need to make some changes to the uiSettings service. The implementation in this PR leverages the existing readonly uiSettings param and adds the setAllowlist() method which is called by the serverless plugin to set an allowlist of setting keys.

Testing in serverless:

1. Set advanced_settings.enabled: true to enable the Advanced settings app in serverless:

   kibana/config/serverless.yml

   Line 53 in 5b216c6

   advanced_settings.enabled: false

2. Start Es with yarn es serverless --ssl and Kibana with yarn serverless-{mode} --ssl in any serverless mode.
3. Navigate to app/management/kibana/settings
4. Verify that the app only displays the settings from packages/serverless/settings/common/index.ts (these are the settings, relevant for all projects in serverless) as well as the settings from the corresponding project package packages/serverless/settings/{mode}_project/index.ts.
5. Verify that the app is functioning correctly.

Testing in self-managed:

1. Start Es with yarn es snapshot and Kibana with yarn start.
2. Go to Stack Management > Advanced settings
3. Verify that all settings are displayed as usual.
4. Verify that the app is functioning correctly.

If your team is a code owner of any of the serverless project plugins, please review the corresponding package packages/serverless/settings/{search/observanility/security}_project/index.ts where you've been added as an owner and test in the serverless solution accordingly.

For maintainers

• This was checked for breaking API changes and was labeled appropriately

[lukeelmers]

Thanks for pinging me on this! I added some initial thoughts on the overall approach. I think the core team will have more comments around the design & naming of the APIs, but if you started by moving everything to the server I think it would help make clear what you're trying to accomplish here, and ultimately make it easier for everyone else to review the proposal.

[ElenaStoeva]

Thank you very much for the feedback @lukeelmers! It is really helpful.

I addressed your comments regarding moving the allowlist APIs to the server and left a couple of follow-up comments. Let me know what you think.

[clintandrewhall]

This is great @ElenaStoeva , and I think it's nearly shippable. Once we answer a few more of these questions, we can send it to @elastic/kibana-core for feedback, (or send it now as a PRFC). :-)

[afharo]

I'd like to suggest an alternative:
We currently support uiSettings.overrides.

Could we leverage this and hide the YAML-overridden settings from the UI (AFAIK, the API won't allow overriding it)?

If something must have a value in Serverless, we can force-set it this way. WDYT?

[ElenaStoeva]

I'd like to suggest an alternative: We currently support uiSettings.overrides.

Could we leverage this and hide the YAML-overridden settings from the UI (AFAIK, the API won't allow overriding it)?

If something must have a value in Serverless, we can force-set it this way. WDYT?

Thanks for the suggestion, @afharo! Just a couple of questions to make sure I understand it correctly:

Are all uiSettings.overrides YAML-overridden settings? If so, where are they being overridden?
Is what you are suggesting to override all settings that are not relevant in serverless so that they are not displayed in the UI and are not allowed to be updated though API?

[clintandrewhall]

This is a great improvement. There's a few minor changes to the API that I think will make it more obvious how the allowList is constructed, and make it more secure.

[clintandrewhall]

Could we leverage this and hide the YAML-overridden settings from the UI

@afharo The list of settings available in Serverless is significantly shorter. We're allow-listing those on a project-by-project basis, and a YML-based approach wouldn't be practical.

In summary: we're not changing the settings, we're shortening the list available to API and UI changes.

[afharo]

Are all uiSettings.overrides YAML-overridden settings? If so, where are they being overridden?

Yes, here's an example of a UI Setting override:

kibana/config/serverless.security.yml

Line 18 in 5136714

uiSettings.overrides.defaultRoute: /app/security/get_started

At the moment, we show it in the UI but it cannot be edited:

We could set the defaults in serverless.yml, and per-project in serverless.{project}.yml.

Is what you are suggesting to override all settings that are not relevant in serverless so that they are not displayed in the UI and are not allowed to be updated through API?

I guess I am. I understand this may be undesirable because the list of settings to hide is larger than the ones to show.
However, I wonder if it's worth triggering the discussion about the defaults that make sense for each one. After all, Serverless is a different offering and things like timepicker:refreshIntervalDefaults might need to change.

I also understand that flipping it from allowlist to hide list (via overrides) might lead to the setting being exposed even when it shouldn't, if we fail to review that during PRs. While allowlist makes it more obvious (it doesn't show up... why? oh... right!).

If we are going the allowlist... should we maintain an on-premise allowlist as well? There may be settings that are intended only for serverless, won't they?

[lukeelmers]

I wonder if it's worth triggering the discussion about the defaults that make sense for each one.

There is some discussion happening in this area already around limits for some of these settings, especially with security solution. There's an internal document I can link you to.

flipping it from allowlist to hide list (via overrides) might lead to the setting being exposed even when it shouldn't, if we fail to review that during PRs. While allowlist makes it more obvious (it doesn't show up... why? oh... right!).

This was the motivation for not using uiSettings.overrides. The concern is a scenario where a user is able to circumvent a setting that's readonly in the UI by hitting the server endpoint directly.

If we are going the allowlist... should we maintain an on-premise allowlist as well? There may be settings that are intended only for serverless, won't they?

I think this is less of a problem for now, because settings intended only for serverless should only be registered in a serverless environment (e.g. in one of the serverless* plugins) and therefore wouldn't show up in self-managed.

My ideal future state would be that readonly on a setting behaves as you'd expect in both serverless & self-managed -- that is, it enforces readonly not just on the client as it does today, but also on the server. But because you can technically circumvent readonly settings by hitting the API directly today, there's a risk that this is a breaking change for self-managed users who may have some sort of tooling in place to change readonly settings by API. So this is something we'd need to save for a 9.0 stack release.

[afharo]

There is some discussion happening in this area already around limits for some of these settings, especially with security solution. There's an internal document I can link you to.

Nice! Thanks for confirming! I hope we can clean up any of the deprecated/unused ones 😇

This was the motivation for not using uiSettings.overrides. The concern is a scenario where a user is able to circumvent a setting that's readonly in the UI by hitting the server endpoint directly.

I just tested it and this is not possible today. Trying to override the defaultRoute via API POST /internal/kibana/settings fails with 400:

{"statusCode":400,"error":"Bad Request","message":"Unable to update \"defaultRoute\" because it is overridden"}

I even tried exporting the SO, tampering, and importing it. While stored in the SO, the YAML override takes precedence. If we follow the allowlist approach, we need to make sure that we dismiss any user SO values.

My concern is that we may forget to set an override to a setting that should not be visible in Advanced Settings because it's opt-out instead of being opt-in.

I think this is less of a problem for now, because settings intended only for serverless should only be registered in a serverless environment (e.g. in one of the serverless* plugins) and therefore wouldn't show up in self-managed.

I think this goes against the recommendation (#101907).

If we are looking for explicit opt-ins, I'd try to design an additional property in the UI Settings register API that would define in which offering it should be visible. This allowlist would require developers to change yet another unrelated plugin whenever they add a new setting.

Having said that, I won't oppose shipping this now and revisiting it in the future (given the times, that it's serverless, hence internal, and the low amount of affected settings).

[clintandrewhall]

If we are going the allowlist... should we maintain an on-premise allowlist as well? There may be settings that are intended only for serverless, won't they?

This is a very good point... but we don't have those today, AFAIK. But I don't think we would register those settings in Kibana, but rather in the serverless plugin, correct? In other words, we'd be adding settings in Serverless, rather than making common settings readonly in Serverless... so it's a different use case.

Perhaps a denyList would be a good addition if that case arises-- where we add a Serverless setting to "classic" Kibana. But I think it's out of scope for now.

[lukeelmers]

My concern is that we may forget to set an override to a setting that should not be visible in Advanced Settings because it's opt-out instead of being opt-in.

Same! uiSettings.overrides gives us all of the protections we want except for this. So the question this PR is trying to solve is how to provide this same type of enforcement in an opt-in manner.

I think this is less of a problem for now, because settings intended only for serverless should only be registered in a serverless environment (e.g. in one of the serverless* plugins) and therefore wouldn't show up in self-managed.

I think this goes against the recommendation (#101907).

Not necessarily, my point (and I think what Clint was saying above) is that such configs could be registered from the solution-specific serverless plugin, which is where they are ideally also being consumed (e.g. serverless_observability plugin would register serverless-only settings and also consume them in the serverless-only UI they have constructed).

[ElenaStoeva]

Thanks for the review @machadoum!

This PR is only for the changes on the uiSettings service. I enabled the existing Advanced settings plugin just so we can test these changes in the serverless UI, but I will disable it before I merge. We will make a separate PR for the serverless Advanced settings UI, which will not include tabs for Space/Global settings.

[afharo]

LGTM! Thank you for iterating and pushing it through the finish line!

[dmlemeshko]

Appex-QA changes LGTM

[Dosant]

I tested and reviewed the code. lgtm. nice additions of serverless tests 👍

[saarikabhasi]

Noticed no global settings similar this comment during testing and as it would be removed, should be fine I guess. Search project code changes LGTM !

[kibana-ci]

💔 Build Failed

• Buildkite Build
• Commit: 3dfbdb9
• Interpreting CI Failures

Failed CI Steps

• Serverless Observability Tests
• Explore - Security Solution Cypress Tests #4
• Investigations - Security Solution Cypress Tests #5
• Defend Workflows Cypress Tests #2

Test Failures

• [job] [logs] Investigations - Security Solution Cypress Tests #5 / Alert details expandable flyout right panel should mark as acknowledged should mark as acknowledged
• [job] [logs] Explore - Security Solution Cypress Tests #4 / Entity Analytics Dashboard With host risk data filters by risk classification filters by risk classification
• [job] [logs] Defend Workflows Cypress Tests #2 / Form User with access can edit and delete an endpoint response action delete response action inside of a rule delete response action inside of a rule
• [job] [logs] Defend Workflows Cypress Tests #2 / Form User with access can edit and delete an endpoint response action edit response action inside of a rule edit response action inside of a rule
• [job] [logs] Defend Workflows Cypress Tests #2 / Form User without access can not edit, add nor delete an endpoint response action All response action controls are disabled All response action controls are disabled

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
@kbn/core-ui-settings-server	16	17	+1
@kbn/management-settings-ids	-	127	+127
@kbn/serverless-common-settings	-	1	+1
@kbn/serverless-observability-settings	-	1	+1
@kbn/serverless-search-settings	-	1	+1
@kbn/serverless-security-settings	-	1	+1
serverless	16	18	+2
total			+134

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
core	381.2KB	381.4KB	+211.0B

Unknown metric groups

API count

id	before	after	diff
@kbn/core-ui-settings-common	25	27	+2
@kbn/core-ui-settings-server	32	34	+2
@kbn/management-settings-ids	-	127	+127
@kbn/serverless-common-settings	-	1	+1
@kbn/serverless-observability-settings	-	1	+1
@kbn/serverless-search-settings	-	1	+1
@kbn/serverless-security-settings	-	1	+1
serverless	17	19	+2
total			+137

ESLint disabled line counts

id	before	after	diff
serverless	3	2	-1

Total ESLint disabled count

id	before	after	diff
serverless	3	2	-1

History

• 💛 Build #158033 was flaky a272d86
• 💚 Build #157975 succeeded efdc13a
• 💔 Build #157647 failed 4e4ee30
• 💔 Build #157616 failed 8c13f80

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

cc @ElenaStoeva