[Security Solution] Enable prebuilt rules customization feature flag #212761

maximpn · 2025-02-28T12:18:42Z

Addresses: #180267

Summary

This PR enables prebuiltRulesCustomizationEnabled feature flag.

Details

Besides simply enabling prebuiltRulesCustomizationEnabled feature flag the following required changes were done

failed tests due enabling the FF were fixed
FF setting was removed from test configurations (integrations and Cypress tests)
FF logic was removed from the codebase. Disabling the FF would require roll back test changes as well. So just in case we have to disable the FF it's simpler to roll back the PR's commit.

elasticmachine · 2025-03-05T11:38:25Z

Pinging @elastic/security-detections-response (Team:Detections and Resp)

elasticmachine · 2025-03-05T11:38:26Z

Pinging @elastic/security-solution (Team: SecuritySolution)

elasticmachine · 2025-03-05T11:38:27Z

Pinging @elastic/security-detection-rule-management (Team:Detection Rule Management)

jbudz

ftr_configs.yml LGTM

banderror

@maximpn Thank you, I guess that was quite an effort.

Overall LGTM, but I'm not sure I understand the approach with removing the FF checks from the code but keeping the FF itself. I'd suggest we remove the FF from x-pack/solutions/security/plugins/security_solution/common/experimental_features.ts as well to make sure there's no leftover logic that depends on it.

I left some comments but I don't think any of them are critical. Will be happy to approve the PR when you respond to them.

banderror · 2025-03-05T15:02:51Z

x-pack/solutions/security/plugins/security_solution/common/experimental_features.ts

+   * Turned: 12 March 2025 in https://github.com/elastic/kibana/pull/212761
+   *
+   * Enabling the FF required fixing some tests.
+   * To disable revert merge commit by https://github.com/elastic/kibana/pull/212761.
   */
-  prebuiltRulesCustomizationEnabled: false,
+  prebuiltRulesCustomizationEnabled: true,


If we say that in order to disable the feature back we have to revert the commit instead of just flipping the feature flag to false, why are we keeping the feature flag in the codebase? Can we remove it right away in this PR?

IMHO keeping it in the code overcomplicates the setup. There's a FF, but it's not respected anymore, according to the PR description: "FF logic was removed from the codebase". Or is it? One has to check the code to be sure. This could be confusing for future us or someone else who'd find this flag.

banderror · 2025-03-05T15:11:07Z

...ne/rule_management/logic/prebuilt_rules/use_prebuilt_rule_customization_upselling_message.ts

-  // We show the upselling message only if the feature flag is enabled
-  const isFeatureFlagEnabled = useIsExperimentalFeatureEnabled('prebuiltRulesCustomizationEnabled');
-  return isFeatureFlagEnabled ? upsellingMessage : undefined;
+  return useUpsellingMessage(messageId);


cc @xcrzx @nikitaindik

banderror · 2025-03-05T15:16:15Z

...ction_engine/rule_management/logic/prebuilt_rules/use_prebuilt_rules_customization_status.ts

+  const isRulesCustomizationEnabled = !upsellingMessage;
+  const customizationDisabledReason = isRulesCustomizationEnabled
+    ? undefined
+    : PrebuiltRulesCustomizationDisabledReason.License;


cc @xcrzx @nikitaindik

...ution/public/detections/components/rules/related_integrations/integrations_popover/index.tsx

banderror · 2025-03-05T16:24:07Z

...on/server/lib/detection_engine/rule_management/logic/export/get_export_by_object_ids.test.ts

+    const prebuiltRule = getRuleMock(
+      getQueryRuleParams({
+        ruleId: 'rule-1',
+        immutable: true,
+        ruleSource: {
+          type: 'external',
+          isCustomized: false,
+        },
+      })
+    );


Nit: Maybe we could specify a mixture of non-customized, customized, and even custom rules here. Or have separate unit tests for these cases.

Nit because the main tests for this should be at the integration level.

banderror · 2025-03-05T18:04:23Z

...le_management/logic/detection_rules_client/detection_rules_client.create_custom_rule.test.ts

@@ -53,7 +53,7 @@ describe('DetectionRulesClient.createCustomRule', () => {
      mlAuthz,
      savedObjectsClient,
      license: licenseMock.createLicenseMock(),
-      experimentalFeatures: { prebuiltRulesCustomizationEnabled: true } as ExperimentalFeatures,
+      experimentalFeatures: {} as ExperimentalFeatures,


Detection rules client doesn't depend on experimentalFeatures anymore, so this could be removed completely if we remove the experimentalFeatures parameter from the createDetectionRulesClient function.

banderror · 2025-03-05T18:19:27Z

...sponse/rules_management/rule_bulk_actions/trial_license_complete_tier/perform_bulk_action.ts

@@ -1525,7 +1525,7 @@ export default ({ getService }: FtrProviderContext): void => {
          },
        ];
        cases.forEach(({ type, value }) => {
-          it(`should return error when trying to apply "${type}" edit action to prebuilt rule`, async () => {
+          it(`should NOT return error when trying to apply "${type}" edit action to prebuilt rule`, async () => {


I think this test should be moved outside of this file and be split into two tests:

The first one would be created in x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/prebuilt_rule_customization/customization_disabled/customization_via_bulk_editing.ts and would test that it should return error when trying to apply "${type}" edit action to prebuilt rule.

The second one would be created in x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/prebuilt_rule_customization/customization_enabled/customization_via_bulk_editing.ts and would test that it should NOT return error when trying to apply "${type}" edit action to prebuilt rule.

banderror · 2025-03-05T18:22:34Z

...sponse/rules_management/rule_bulk_actions/trial_license_complete_tier/perform_bulk_action.ts

-
-          // if rule action is applied together with another edit action, that can't be applied to prebuilt rule (for example: tags action)
-          // bulk edit request should return error
-          it(`should return error if one of edit action is not eligible for prebuilt rule`, async () => {


Similarly, this should go to x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/prebuilt_rule_customization/customization_disabled/customization_via_bulk_editing.ts.

banderror · 2025-03-05T18:25:09Z

...ules_management/rule_bulk_actions/trial_license_complete_tier/perform_bulk_action_dry_run.ts

@@ -201,47 +201,29 @@ export default ({ getService }: FtrProviderContext): void => {
        expect(ruleBody.tags).toEqual(tags);
      });

-      it('should validate immutable rule edit', async () => {
+      it('should allow prebuilt rules edit', async () => {


Similarly, this should be moved to x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/prebuilt_rule_customization/customization_enabled/customization_via_bulk_editing.ts.

banderror · 2025-03-05T18:27:32Z

...ons_response/rules_management/rule_import_export/trial_license_complete_tier/import_rules.ts

@@ -1625,7 +1625,7 @@ export default ({ getService }: FtrProviderContext): void => {

    describe('supporting prebuilt rule customization', () => {
      describe('compatibility with prebuilt rule fields', () => {
-        it('rejects rules with "immutable: true" when the feature flag is disabled', async () => {
+        it('accepts rules with "immutable: true"', async () => {


The whole describe('supporting prebuilt rule customization' should be moved under x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/prebuilt_rule_customization/customization_enabled.

maximpn · 2025-03-08T07:13:50Z

/ci

elasticmachine · 2025-03-08T08:33:49Z

💔 Build Failed

Failed CI Steps

Test Failures

[job] [logs] FTR Configs #59 / Rules Management - Prebuilt rule export @ess @serverless @skipInServerlessMKI Prebuilt rule export - feature disabled Export API - doesn't export prebuilt rules when the feature is disabled
[job] [logs] FTR Configs #69 / Rules Management - Prebuilt rule export @ess @serverless @skipInServerlessMKI Prebuilt rule export - feature disabled Export API - doesn't export prebuilt rules when the feature is disabled
[job] [logs] FTR Configs #59 / Rules Management - Prebuilt rule export @ess @serverless @skipInServerlessMKI Prebuilt rule export - feature disabled Export API - doesn't export prebuilt rules when the feature is disabled
[job] [logs] FTR Configs #69 / Rules Management - Prebuilt rule export @ess @serverless @skipInServerlessMKI Prebuilt rule export - feature disabled Export API - doesn't export prebuilt rules when the feature is disabled
[job] [logs] FTR Configs #99 / Rules Management - Rule Import API - Customized prebuilt rules @ess @serverless @skipInServerlessMKI Prebuilt rule import does NOT import customized prebuilt rules when rule customization is disabled
[job] [logs] FTR Configs #99 / Rules Management - Rule Import API - Customized prebuilt rules @ess @serverless @skipInServerlessMKI Prebuilt rule import does NOT import customized prebuilt rules when rule customization is disabled
[job] [logs] FTR Configs #89 / Rules Management - Rule Import API - Non-customized prebuilt rules @ess @serverless @skipInServerlessMKI Prebuilt rule import does NOT allow importing non-customized prebuilt rules
[job] [logs] FTR Configs #101 / Rules Management - Rule Import API - Non-customized prebuilt rules @ess @serverless @skipInServerlessMKI Prebuilt rule import does NOT allow importing non-customized prebuilt rules
[job] [logs] FTR Configs #89 / Rules Management - Rule Import API - Non-customized prebuilt rules @ess @serverless @skipInServerlessMKI Prebuilt rule import does NOT allow importing non-customized prebuilt rules
[job] [logs] FTR Configs #101 / Rules Management - Rule Import API - Non-customized prebuilt rules @ess @serverless @skipInServerlessMKI Prebuilt rule import does NOT allow importing non-customized prebuilt rules

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`securitySolution`	8.9MB	8.9MB	-677.0B

Page load bundle

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

id	before	after	diff
`securitySolution`	87.5KB	87.5KB	-37.0B

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`securitySolution`	578	576	-2

Total ESLint disabled count

id	before	after	diff
`securitySolution`	661	659	-2

History

💔 Build #281844 failed be648a4
💛 Build #280999 was flaky 86db5ac
💔 Build #280873 failed 8c882e6
💔 Build #280694 failed 771a33d

cc @maximpn

maximpn self-assigned this Feb 28, 2025

maximpn force-pushed the enable-prebuilt-rules-customization-ff branch 7 times, most recently from 100afa4 to 86db5ac Compare March 5, 2025 09:33

maximpn marked this pull request as ready for review March 5, 2025 11:38

maximpn requested review from a team as code owners March 5, 2025 11:38

maximpn requested a review from jkelas March 5, 2025 11:38

maximpn requested review from xcrzx and banderror and removed request for jkelas March 5, 2025 11:38

jbudz approved these changes Mar 5, 2025

View reviewed changes

banderror reviewed Mar 5, 2025

View reviewed changes

enable prebuilt rules customization FF

2c70a09

maximpn force-pushed the enable-prebuilt-rules-customization-ff branch from 86db5ac to be648a4 Compare March 7, 2025 15:17

test fixes

afd314e

maximpn force-pushed the enable-prebuilt-rules-customization-ff branch from be648a4 to afd314e Compare March 8, 2025 07:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] Enable prebuilt rules customization feature flag #212761

[Security Solution] Enable prebuilt rules customization feature flag #212761

maximpn commented Feb 28, 2025 •

edited

Loading

elasticmachine commented Mar 5, 2025

elasticmachine commented Mar 5, 2025

elasticmachine commented Mar 5, 2025

jbudz left a comment

banderror left a comment

banderror Mar 5, 2025

banderror Mar 5, 2025

banderror Mar 5, 2025

banderror Mar 5, 2025

banderror Mar 5, 2025

banderror Mar 5, 2025

banderror Mar 5, 2025

banderror Mar 5, 2025

banderror Mar 5, 2025

maximpn commented Mar 8, 2025

elasticmachine commented Mar 8, 2025 •

edited

Loading

ESLint disabled line counts

Total ESLint disabled count

[Security Solution] Enable prebuilt rules customization feature flag #212761

Are you sure you want to change the base?

[Security Solution] Enable prebuilt rules customization feature flag #212761

Conversation

maximpn commented Feb 28, 2025 • edited Loading

Summary

Details

elasticmachine commented Mar 5, 2025

elasticmachine commented Mar 5, 2025

elasticmachine commented Mar 5, 2025

jbudz left a comment

Choose a reason for hiding this comment

banderror left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maximpn commented Mar 8, 2025

elasticmachine commented Mar 8, 2025 • edited Loading

💔 Build Failed

Failed CI Steps

Test Failures

Metrics [docs]

Async chunks

Page load bundle

ESLint disabled line counts

Total ESLint disabled count

History

maximpn commented Feb 28, 2025 •

edited

Loading

elasticmachine commented Mar 8, 2025 •

edited

Loading