-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Advanced settings] Add settings allowlist #164471
[Advanced settings] Add settings allowlist #164471
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
packages/core/ui-settings/core-ui-settings-browser-internal/src/ui_settings_client_common.ts
Outdated
Show resolved
Hide resolved
packages/core/ui-settings/core-ui-settings-common/src/ui_settings.ts
Outdated
Show resolved
Hide resolved
src/plugins/advanced_settings/public/management_app/settings_allowlist.ts
Outdated
Show resolved
Hide resolved
…-ref HEAD~1..HEAD --fix'
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. |
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). :-) |
I'd like to suggest an alternative: 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
packages/core/ui-settings/core-ui-settings-common/src/ui_settings.ts
Outdated
Show resolved
Hide resolved
@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. |
Yes, here's an example of a UI Setting override: kibana/config/serverless.security.yml Line 18 in 5136714
At the moment, we show it in the UI but it cannot be edited: We could set the defaults in
I guess I am. I understand this may be undesirable because the list of settings to hide is larger than the ones to show. 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? |
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.
This was the motivation for not using
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 My ideal future state would be that |
Nice! Thanks for confirming! I hope we can clean up any of the deprecated/unused ones 😇
I just tested it and this is not possible today. Trying to override the {"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 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 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). |
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 Perhaps a |
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.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Security Solutions changes LGTM!
@ElenaStoeva While I was testing how the advanced settings page looks on the security solution serverless I noticed that the tabs don't make much sense.
Serverless doesn't support multiple spaces and apparently has no global setting. So we could get rid of the tabs.
This isn't a blocker for your PR, but we need to address it to enable the page on serverless.
…github.com/ElenaStoeva/kibana into advanced_settings/allowlist_for_serverless
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. |
…-ref HEAD~1..HEAD --fix'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you for iterating and pushing it through the finish line!
x-pack/test_serverless/functional/test_suites/observability/advanced_settings.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Appex-QA changes LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested and reviewed the code. lgtm. nice additions of serverless tests 👍
packages/core/ui-settings/core-ui-settings-server-internal/src/ui_settings_service.ts
Show resolved
Hide resolved
packages/core/ui-settings/core-ui-settings-server-internal/src/ui_settings_service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💔 Build FailedFailed CI Steps
Test Failures
Metrics [docs]Public APIs missing comments
Page load bundle
Unknown metric groupsAPI count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @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 thesetAllowlist()
method which is called by the serverless plugin to set an allowlist of setting keys.Testing in serverless:
advanced_settings.enabled: true
to enable the Advanced settings app in serverless:kibana/config/serverless.yml
Line 53 in 5b216c6
yarn es serverless --ssl
and Kibana withyarn serverless-{mode} --ssl
in any serverless mode.app/management/kibana/settings
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 packagepackages/serverless/settings/{mode}_project/index.ts
.Testing in self-managed:
yarn es snapshot
and Kibana withyarn start
.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