[Security Solution] Recalculate isCustomized when bulk editing rules

[xcrzx]

Resolves: #187706

Summary

Added the isCustomized field recalculation after a bulk edit operation on rules as part of the rules customization epic.

Background
The isCustomized field is a rule parameter indicating if a prebuilt Elastic rule has been modified by a user. This field is extensively used in the prebuilt rule upgrade workflow. It's essential to ensure any rule modification operation recalculates this field to keep its value in sync with the rule content. Most of the rule CRUD operations were already covered in a previous PR: Calculate and save ruleSource.isCustomized in API endpoint handlers. This PR addresses the remaining bulk rule modification operations performed using the rulesClient.bulkEdit method.

rulesClient.bulkEdit changes

The isCustomized calculation is based on the entire rule object (i.e., rule params and attributes) and should be performed after all bulk operations have been applied to the rule - after operations and paramsModifier. To support this, I changed the paramsModifier to accept entire rule object:

export type ParamsModifier<Params extends RuleParams> = (
-  params: Params
+  rule: Rule<Params>
) => Promise<ParamsModifierResult<Params>>;

Security Solution Bulk Endpoint changes

The /api/detection_engine/rules/_bulk_action endpoint now handles bulk edit actions a bit differently. Previously, most of the bulk action was delegated to the rules client. Now, we need to do some preparatory work:

1. Fetch the affected rules in memory first, regardless of whether we received a query or rule IDs as input (previously delegated to Alerting).
2. Identify all prebuilt rules among the fetched rules.
3. Fetch base versions of the prebuilt rules.
4. Provide base versions to ruleModifier for the isCustomized calculation.

These changes add a few extra roundtrips to Elasticsearch and make the bulk edit endpoint less efficient than before. However, this seems justified given the added functionality of the customization epic. In the future, we might consider optimizing to reduce the number of database requests. Ideally, for Security Solution use cases, we would need a more generic method than bulkEdit, such as bulkUpdate, allowing us to implement any required rule update logic fully on the solution side.

[pmuellr]

Just a note that this seems to be another case of an asset type (rule in this case) wanting some influence in API calls (verification, modification of data) that they are not involved in. We have a more general issue open for this, here: allow connector/rule types to provide hooks/feedback during create/update/delete calls #106724

[xcrzx]

I'm not sure how to implement that without type casting. There are several similar Rule types that aren't assignable to each other due to subtle differences.

[JiaweiWu]

It's fine to cast here since we're in the process of migrating and consolidating our rule types. But I would just put a TODO comment here

[elasticmachine]

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

[elasticmachine]

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

[jbudz]

ftr_configs.yml

[JiaweiWu]

Management experiences team LGTM!

[JiaweiWu]

It's fine to cast here since we're in the process of migrating and consolidating our rule types. But I would just put a TODO comment here

[jpdjere]

This worked for me:

export default async function ({ readConfigFile }: FtrConfigProviderContext) {
  const functionalConfig = await readConfigFile(
    require.resolve('../../../../../../../config/ess/config.base.trial')
  );

  const testConfig = {
    ...functionalConfig.getAll(),
    testFiles: [require.resolve('..')],
    junit: {
      reportName:
        'Rules Management - Prebuilt Rule Customization Integration Tests - ESS Env - Trial License',
    },
    kbnTestServer: {
      ...functionalConfig.get('kbnTestServer'),
      serverArgs: [
        ...functionalConfig.get('kbnTestServer.serverArgs'),
        `--xpack.securitySolution.enableExperimental=${JSON.stringify([
          'prebuiltRulesCustomizationEnabled',
        ])}`,
      ],
    },
  };

  return testConfig;
}

I would remove the additional check to simplify this config file, unless you still see it as necessary for some other reason.

[xcrzx]

With this implementation, a new --xpack.securitySolution.enableExperimental flag is added to the config. I'm not entirely sure how this will be interpreted by the Kibana server since the same flag from the base config will also be present. The server will be started with something like this:

node path/to/kibana --xpack.securitySolution.enableExperimental=valuesFromBase --xpack.securitySolution.enableExperimental=prebuiltRulesCustomizationEnabled

Therefore, I'd prefer to have the flag entirely replaced for more transparency.

[jpdjere]

Overall LGTM ✅ Thanks for finding a solution for this!

Just an observation + question. I stressed-tested bulk editing:

• all 1195 Elastic rules = ✅ good performance
• all 1195 Elastic rules + their 1195 duplicates = ✅ good performance

When I duplicated the Elastic rules again for a total of 3585 rules, that's when the endpoint started to fail.

First failed with an internal ES error:

 "failed_shards": [
        {
            "shard": 0,
            "index": ".kibana_alerting_cases_8.16.0_001",
            "node": "2n4sV2RASoa28WZrMu8vaw",
            "reason": {
                "type": "too_many_nested_clauses",
                "reason": "Query contains too many nested clauses; maxClauseCount is set to 6144"
            }
        }
    ],

So, I add a promise pool to the edit request:

                const bulkEditOutcome = await initPromisePool({
                  concurrency: 1000,
                  items: rules,
                  executor: async (rule) => {
                    const bulkEditResult = await bulkEditRules({
                      actionsClient,
                      rulesClient,
                      prebuiltRuleAssetClient,
                      rules,
                      actions: body.edit,
                      mlAuthz,
                      experimentalFeatures: config.experimentalFeatures,
                    });
                    return bulkEditResult;
                  },
                  abortSignal: abortController.signal,
                });

But that just caused the requests to eventually time out. Which I kind of expected. What do you think? I mean, this is the same scenario we're having for importing large amounts of rules, and it seems that the only "proper" way out of this is replacing our endpoints with async endpoints.

[xcrzx]

But that just caused the requests to eventually time out. Which I kind of expected. What do you think? I mean, this is the same scenario we're having for importing large amounts of rules, and it seems that the only "proper" way out of this is replacing our endpoints with async endpoints.

Thanks for catching that, @jpdjere! For now, we can add a more human-readable error message instead of the cryptic 500 Internal Server Error. I've added extra validation to the API handler to return a 400 error when there are more than 2,000 rules matching the query. This number seems to perform okay without hitting a timeout or triggering the "too many nested clauses" exception.

In the longer term, turning this API into an async process might be the best approach, but that will require more thorough exploration.

[kibana-ci]

💛 Build succeeded, but was flaky

• Buildkite Build
• Commit: f39b7c3

Failed CI Steps

Metrics [docs]

✅ unchanged

History

• 💔 Build #230484 failed efc32ab83a37f23f9ff3cc6b7aa6bc233e011f49
• 💔 Build #230469 failed 778a21522bfa26ee47b9beeabd6a4cfd59eb31be
• 💔 Build #230462 failed 778a21522bfa26ee47b9beeabd6a4cfd59eb31be
• 💔 Build #230455 failed 778a21522bfa26ee47b9beeabd6a4cfd59eb31be
• 💚 Build #230403 succeeded d9c98599c9a5dee2478676dff51a366a2a1faa0e
• 💛 Build #230089 was flaky ded507e8f63e4070ba9534218543231eddb5683b

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

cc @xcrzx