-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Add toggle to enable/disable rule install from SOs #106189
Add toggle to enable/disable rule install from SOs #106189
Conversation
...ns/security_solution/server/lib/detection_engine/routes/rules/add_prepackaged_rules_route.ts
Outdated
Show resolved
Hide resolved
...ns/security_solution/server/lib/detection_engine/routes/rules/add_prepackaged_rules_route.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.
kibana-docker 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.
LGTM,
What I did:
- Review the code
What I couldn't do:
- I couldn't run Cypress, it crashes right now on my system. But it also on master, so it's not this PR issue.
💚 Build SucceededMetrics [docs]
History
To update your PR or re-run it, just comment with: |
Could add some unit tests as well for prebuiltRulesFromSavedObjects=true? I only see them for the false scenario. |
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 🚢
Had only one question, but it's more for my own understanding.
@@ -37,6 +39,8 @@ export const installPrepackagedRules = async ({ | |||
securityStart, | |||
alerts, | |||
maxTimelineImportExportSize, | |||
prebuiltRulesFromFileSystem, |
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.
Just for my own understanding - Is this why we needed to "thread" these two new arguments all the way through to the endpoint context + Fleet integration callback? to feed it into alerts.getAlertsClientWithRequest()
?
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.
yeah exactly
* Add toggle to enable/disable rule install from SOs * Fix lint and type tests * Fix types for tests * Enable prebuiltRulesFromSavedObjects with extra rules in fleet * Restore rules and disable prebuiltRulesFromSavedObjects * Pass explicit arguments instead of full config * Set values in 'endpoint' setup * Propagate prebuiltRulesFrom{FileSystem,SavedObjects} from configs
💔 Backport failed
Successful backport PRs will be merged automatically after passing CI. To backport manually run: |
* Add toggle to enable/disable rule install from SOs * Fix lint and type tests * Fix types for tests * Enable prebuiltRulesFromSavedObjects with extra rules in fleet * Restore rules and disable prebuiltRulesFromSavedObjects * Pass explicit arguments instead of full config * Set values in 'endpoint' setup * Propagate prebuiltRulesFrom{FileSystem,SavedObjects} from configs Co-authored-by: Ross Wolf <[email protected]>
…y-show-migrate-to-authzd-users * 'master' of github.com:elastic/kibana: (48 commits) [Canvas] Expression shape (elastic#103219) [FTR] Skips Vega tests [Sample data] Use Lens in ecommerce data (elastic#106039) [APM] Backends inventory & overview page routes (elastic#106223) [TSVB] Add more functional tests for Gauge and TopN (elastic#105361) Add toggle to enable/disable rule install from SOs (elastic#106189) Improve unit test coverage of FS API calls (elastic#106242) Remove recursive plugin status in meta field (elastic#106286) [Ingest pipelines] add community id processor (elastic#103863) [XY axis] Fixes the values inside bar charts (elastic#106198) [data.search] Set default expiration to 1m if search sessions are disabled (elastic#105329) set the doc title when navigating to reporting and unset when navigating away (elastic#106253) [Lens] Display legend inside chart (elastic#105571) [RAC] [TGrid] Migrate the TGrid's rendering to `EuiDataGrid` (elastic#106199) [Security Solutions] Removes the elastic legacy client from lists and security_solution plugins (elastic#106130) [Enterprise Search] Require security plugin in 8.0 (elastic#106307) [DOCS] Updates screenshots in Dev Tools docs (elastic#105859) [DOCS] Updates text and screenshots in tags doc (elastic#105853) [Alerting] Allow rule types to extract/inject saved object references on rule CRU (elastic#101896) Jest and Storybook fixes (elastic#104991) ... # Conflicts: # x-pack/plugins/reporting/public/plugin.ts
…106418) * Add toggle to enable/disable rule install from SOs * Fix lint and type tests * Fix types for tests * Enable prebuiltRulesFromSavedObjects with extra rules in fleet * Restore rules and disable prebuiltRulesFromSavedObjects * Pass explicit arguments instead of full config * Set values in 'endpoint' setup * Propagate prebuiltRulesFrom{FileSystem,SavedObjects} from configs
Summary
Closes #105980
Added config flags
xpack.securitySolution.prebuiltRulesFromSavedObjects
andxpack.securitySolution.prebuiltRulesFromFileSystem
so cloud rule updates can be disabled for CI.Here's how I tested the flag locally. Left a persistent ES running and removed three rules, then ran:
$ yarn start --ssl --xpack.securitySolution.prebuiltRulesFromSavedObjects=false
Then restarted Kibana (kept ES running) and enabled rules from Fleet SOs. The removed rules are now available as an update:
$ yarn start --ssl --xpack.securitySolution.prebuiltRulesFromSavedObjects=true
Checklist
Existing tests should work as-is.
Testing this under a few different scenarios
prebuiltRulesFromSavedObjects=false
. Should pass CI (035e116). Result: ✅ as expectedprebuiltRulesFromSavedObjects=true
. Should fail CI (327abcb). Result ❌ as expected for the relevant testsprebuiltRulesFromSavedObjects=false
. Must pass CI and be ready to merge (c0e4e03). Result: ✅ as expected