[CTI][PIT] adds PIT to indicator search

[ecezalp]

Summary

Adds PIT to indicator search.
Number of requested indicator documents per batch is updated to 1000 from 9000.

[elasticmachine]

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

[ecezalp]

@elasticmachine merge upstream

[FrankHassanabad]

The value (e.g. 1m, see [Time units](https://www.elastic.co/guide/en/elasticsearch/reference/current/api-conventions.html#time-units)) does not need to be long enough to process all data — it just needs to be long enough for the next request.

From docs here: https://www.elastic.co/guide/en/elasticsearch/reference/current/point-in-time-api.html

I don't think you want this to be 1 hour but rather a lower time. Just enough that you can re-call and process things again. So for example if the elastic search will be-recalled for sure within 5 minutes, 5 minutes is a better default.

[FrankHassanabad]

You should be able to remove the issue with atomic operation with a function closure like so:

    // This avoids an atomic race condition by passing in the pit into the function
    // and then atomically using it and writing it back into the closure'ed pitId.
    const searchFunction = async (pitIdForSearch: string) => {
      const response = await esClient.search<
        ThreatListDoc,
        Record<string, estypes.AggregationsAggregate>
      >({
        body: {
          ...threatListConfig,
          query: queryFilter,
          search_after: searchAfter,
          sort: ['_shard_doc', { '@timestamp': 'asc' }],
        },
        track_total_hits: false,
        size: INDICATOR_PER_PAGE,
        pit: { id: pitIdForSearch },
      });
      if (response.pit_id) {
        pitId = response.pit_id;
      }
      return response;
    };

I do think the linter is bringing up a valid point. Please test what I'm posting though as I did not test it myself.

[FrankHassanabad]

Left two comments, I think both should be fixed before merging but an LGTM is much deserved here.

[nkhristinin]

@elasticmachine merge upstream

[kibanamachine]

merge conflict between base and head

[marshallmain]

Using a static value here will break the concurrentSearches and itemsPerSearch functionality provided through rule params. Are we planning to remove those params?

[ecezalp]

this is a great question. I believe that concurrent search logic has been built into indicator match rules a while back with the intention of speeding up the rule execution. That being said, there seems to be no test coverage around the feature, and as far as I am aware the parameters itemsPerSearch and concurrentSearches are undocumented and can only be set through the API - which makes me think that anyone creating indicator match rules via the UI have never been able to use anything but the default values: 1 for concurrentSearches and 9000 for itemsPerSearch. I am under the impression that the feature of executing concurrent searches is not something we have ever intentionally released - some telemetry data could confirm this. Is it possible to see how many rule SOs actually have "itemsPerSearch" and "concurrentSearches" values set out there in the world?

In my opinion it makes sense to be intentional about the concurrent searching capability of Indicator Match rules by bringing it into 8.3 planning as a part of the performance epic and reevaluate the performance with our improved benchmarking tooling. Until then, we should consider the feature unsupported - I'll get some product input on this.

@peluja1012 @jethr0null @shimonmodi

[ecezalp]

With the latest commit I brought back concurrentSearches, itemsPerSearch, and perPage variables to ensure that the built-in concurrent searching logic remains intact. Created https://github.com/elastic/security-team/issues/3465 to benchmark concurrent threat searches for 8.3

[ecezalp]

@elasticmachine merge upstream

[ecezalp]

@elasticmachine merge upstream

[ecezalp]

@elasticmachine merge upstream

[ecezalp]

@elasticmachine merge upstream

[kibana-ci]

💛 Build succeeded, but was flaky

• Buildkite Build
• Commit: 2b9ebd4
• Storybooks Preview
• Cloud Deployment
• Webpack Bundle Analyzer

Test Failures

• [job] [logs] Security Solution Tests / value lists user with restricted access role "before each" hook for "Does not allow a t1 analyst user to upload a value list"

Metrics [docs]

✅ unchanged

History

• 💚 Build #34037 succeeded 628e7f7
• 💛 Build #33681 was flaky ad735bf
• 💔 Build #33609 failed 0127203
• 💔 Build #33578 failed 42d7cfd
• 💚 Build #33283 succeeded 1d3ef60

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

cc @ecezalp