[ResponseOps] use Data Streams for AAD indices in serverless #160572

pmuellr · 2023-06-26T21:23:41Z

Changes the way the alerts-as-data (AAD) indices are created and written to, to allow them to be built as they have been in the past (alias and backing indices created manually) OR as an ES Data Stream.

Serverless will use Data Streams, other environments will use the existing alias and backing indices. The determination is made by optionally including the serverless plugin, and determining if it's available.

The implementation is organized around a DataStreamAdapter object, which is instantiated with a "data stream" or "alias" flavor, and then it handles the specialized behavior. Currently, a lot of the smaller implementation bits, like setting property values in ES calls, is done via in-line boolean checks of that object, as to whether data streams or aliases are being used. This could probably be cleaned up some.

Existing non-serverless function tests are largely unchanged, as they can't test the new data stream path. Some tests have been added to the serverless function tests, to test basic reading / writing via updated alert documents.

DEFER

more serverless AaD tests
potentially re-org alerts_service tests; currently this module makes no direct ES calls, but it's jest tests are quite large and mock a LOT of ES calls. The calls are now indirect in helpers. Almost feels like the code was re-org'd to add the helpers, but the tests weren't. Would likely make the tests more understandable with a re-org - spent lots of time in this test module
- x-pack/plugins/alerting/server/alerts_service/alerts_service.test.ts
- x-pack/plugins/alerting/server/alerts_service/lib/create_or_update_ilm_policy.test.ts
- x-pack/plugins/alerting/server/alerts_service/lib/create_or_update_index_template.test.ts
- x-pack/plugins/alerting/server/alerts_service/lib/data_stream_adapter.ts
[Response Ops][Alerting] Discuss how to handle possible conflicts when updating alerts-as-data documents #158403 - this issue is more noticeable now that we HAVE to do OCC with data streams, so we get errors instead of simply overwriting documents (which is also bad)

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

Changes the way the alerts-as-data (AAD) indices are created and written to, to allow them to be built as they have been in the past (alias and backing indices created manually) OR as an ES Data Stream. Serverless will use Data Streams, other environments will use the existing alias and backing indices. The configuration `xpack.alerting.useDataStreamForAlerts` determines which to use, and is false by default. This configuration is not intended to be used by customers, as it when used in the wrong environment, will cause failures creating and writing to the alert indices. PR elastic#160572 - fix jest test - start integrating bits of the dataStreamAdapter - fix tests - fix function tests - fix FT - change the rule registry writer - fix FT - start using the new ds adapter to create alias indices - add create data stream logic - use alias by default createConcreteWriteIndex() as it's being called by other plugins now - start adapting jest tests to test datastream alerts as well - extend create_concrete_write_index jest tests for data stream - rebase, fix merge conflicts - fix jest test after merge conflict - convert the RR trial test to use data streams - fix jest test, remove ILM for datastream, run serverless o11y test with ds, add rr test with ds - fix jest and function tests - fix jest tests

x-pack/plugins/alerting/server/plugin.ts

pmuellr · 2023-08-29T16:46:00Z

@elasticmachine merge upstream

I think this should clear the failing FTR test via PR 165103

MadameSheema

security-engineering-productivity changes LGTM

PhilippeOberti

LGTM for the Threat Hunting Investigations team

elastic#164988) ## Summary Needed for elastic#160572 (comment) (cherry picked from commit f8ad13a) # Conflicts: # x-pack/test/security_solution_cypress/cypress/e2e/exceptions/rule_details_flow/add_edit_exception.cy.ts

e40pud

DE changes LGTM

vitaliidm · 2023-08-30T10:03:17Z

...ins/security_solution/server/lib/detection_engine/routes/signals/open_close_signals_route.ts

+    // https://www.elastic.co/guide/en/elasticsearch/reference/current/docs-update-by-query.html#_refreshing_shards_2
+    // Note: Before we tried to use "refresh: wait_for" but I do not think that was available and instead it defaulted to "refresh: true"
+    // but the tests do not pass with "refresh: false". If at some point a "refresh: wait_for" is implemented, we should use that instead.
+    refresh: true,


is allowing refresh:true something we want to do?
I remember we did some work to avoid explicitly refreshing indices: https://github.com/elastic/security-team/issues/6561

In similar work, done in #162508, I had the same issue, so had add retry calls to wait until item reindexed and only after this finish request.

It worth to note though, updates in lists/items are not available in UI, only through API calls, so we do not expect them to be called frequently, especially in Serverless. Only list delete available in UI, which is also not expected to be frequent operation

Thank you for the review @vitaliidm, I took it from here

kibana/x-pack/plugins/security_solution/server/lib/detection_engine/routes/signals/open_close_signals_route.ts

Line 130 in 4d6f732

refresh: true,

I have tested it locally and it seems false works fine, let's see if CI agrees 🤞

vitaliidm · 2023-08-30T10:03:40Z

...plugins/security_solution/server/lib/detection_engine/routes/signals/set_alert_tags_route.ts

+          // https://www.elastic.co/guide/en/elasticsearch/reference/current/docs-update-by-query.html#_refreshing_shards_2
+          // Note: Before we tried to use "refresh: wait_for" but I do not think that was available and instead it defaulted to "refresh: true"
+          // but the tests do not pass with "refresh: false". If at some point a "refresh: wait_for" is implemented, we should use that instead.
+          refresh: true,


same as above

is allowing refresh:true something we want to do?
I remember we did some work to avoid explicitly refreshing indices: https://github.com/elastic/security-team/issues/6561

kibana-ci · 2023-08-30T14:10:26Z

💔 Build Failed

Buildkite Build
Commit: d1506cf
Interpreting CI Failures
Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-160572-d1506cf0df7f

Failed CI Steps

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`alerting`	749	759	+10
`ruleRegistry`	235	238	+3
total			+13

Async chunks

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

id	before	after	diff
`securitySolution`	12.6MB	12.6MB	+4.0B

Canvas Sharable Runtime

The Canvas "shareable runtime" is an bundle produced to enable running Canvas workpads outside of Kibana. This bundle is included in third-party webpages that embed canvas and therefor should be as slim as possible.

id	before	after	diff
`module count`	-	5580	+5580
`total size`	-	6.0MB	+6.0MB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`alerting`	46	49	+3

Unknown metric groups

API count

id	before	after	diff
`alerting`	780	790	+10
`ruleRegistry`	264	267	+3
total			+13

ESLint disabled line counts

id	before	after	diff
`alerting`	90	89	-1

Total ESLint disabled count

id	before	after	diff
`alerting`	92	91	-1

History

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

…ver.load (#164988) (#165203) # Backport This will backport the following commits from `main` to `8.10`: - [[security_solution_cypress] Add support for options in EsArchiver.load (#164988)](#164988)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)

…#160572) resolves elastic#154266 Changes the way the alerts-as-data (AAD) indices are created and written to, to allow them to be built as they have been in the past (alias and backing indices created manually) OR as an ES Data Stream. Serverless will use Data Streams, other environments will use the existing alias and backing indices. The determination is made by optionally including the `serverless` plugin, and determining if it's available. The implementation is organized around a `DataStreamAdapter` object, which is instantiated with a "data stream" or "alias" flavor, and then it handles the specialized behavior. Currently, a lot of the smaller implementation bits, like setting property values in ES calls, is done via in-line boolean checks of that object, as to whether data streams or aliases are being used. This could probably be cleaned up some. Existing non-serverless function tests are largely unchanged, as they can't test the new data stream path. Some tests have been added to the serverless function tests, to test basic reading / writing via updated alert documents. ## DEFER - more serverless AaD tests - elastic#158403 - this issue is more noticeable now that we HAVE to do OCC with data streams, so we get errors instead of simply overwriting documents (which is also bad) Co-authored-by: Patryk Kopycinski <[email protected]>

resolves: #190376 In PR #160572, we changed from using just the bulk op `index` to using `create` when new alerts are being created. Unfortunately, the code to handle the bulk responses didn't take into account that the bulk responses for `create`s need different handling than `index`s. Specifically, conflicts for `create` were being treated as errors. This PR changes the processing to consider additional ops besides just `index`.

resolves: elastic#190376 In PR elastic#160572, we changed from using just the bulk op `index` to using `create` when new alerts are being created. Unfortunately, the code to handle the bulk responses didn't take into account that the bulk responses for `create`s need different handling than `index`s. Specifically, conflicts for `create` were being treated as errors. This PR changes the processing to consider additional ops besides just `index`.

pmuellr mentioned this pull request Jun 27, 2023

[on hold] [ResponseOps] change AAD indices to data streams #156123

Closed

9 tasks

pmuellr force-pushed the alerting/serverless-aad-to-datastream branch from 1722158 to 1591879 Compare July 10, 2023 23:11

pmuellr force-pushed the alerting/serverless-aad-to-datastream branch from 64625ec to 986774c Compare July 12, 2023 14:22

pmuellr mentioned this pull request Jul 14, 2023

[Response Ops][Alerting] Discuss how to handle possible conflicts when updating alerts-as-data documents #158403

Closed

pmuellr force-pushed the alerting/serverless-aad-to-datastream branch from 695a5a2 to 0060185 Compare July 25, 2023 19:03

pmuellr force-pushed the alerting/serverless-aad-to-datastream branch from 0060185 to 259f517 Compare July 28, 2023 17:43

pmuellr force-pushed the alerting/serverless-aad-to-datastream branch 2 times, most recently from 4df6539 to 11d2667 Compare August 3, 2023 20:36

pmuellr force-pushed the alerting/serverless-aad-to-datastream branch 2 times, most recently from 74421c2 to c7c0886 Compare August 4, 2023 18:37

pmuellr added ci:serverless-test-all ci:build-serverless-image labels Aug 8, 2023

pmuellr force-pushed the alerting/serverless-aad-to-datastream branch 2 times, most recently from 8ad645e to fa31bf0 Compare August 9, 2023 18:44

pmuellr force-pushed the alerting/serverless-aad-to-datastream branch 2 times, most recently from 4007f39 to b7e0206 Compare August 10, 2023 18:18

mikecote reviewed Aug 15, 2023

View reviewed changes

x-pack/plugins/alerting/server/plugin.ts Show resolved Hide resolved

pmuellr force-pushed the alerting/serverless-aad-to-datastream branch 2 times, most recently from d361034 to bf59bb3 Compare August 15, 2023 18:27

patrykkopycinski added 2 commits August 28, 2023 23:22

fix update alert status

86f1300

unskip

433c204

patrykkopycinski approved these changes Aug 29, 2023

View reviewed changes

patrykkopycinski and others added 2 commits August 29, 2023 08:52

Merge branch 'main' into alerting/serverless-aad-to-datastream

2463c45

merge main and fix conflicts

da45e12

kibanamachine and others added 2 commits August 29, 2023 12:46

Merge branch 'main' into alerting/serverless-aad-to-datastream

9772510

merge main, fix conflicts

34840ab

MadameSheema approved these changes Aug 30, 2023

View reviewed changes

PhilippeOberti approved these changes Aug 30, 2023

View reviewed changes

Merge branch 'main' into alerting/serverless-aad-to-datastream

4d6f732

e40pud approved these changes Aug 30, 2023

View reviewed changes

vitaliidm reviewed Aug 30, 2023

View reviewed changes

patrykkopycinski added 3 commits August 30, 2023 13:34

cr

97f17f9

skip tests

f2d2d2a

skip

d1506cf

pmuellr merged commit 95fa356 into elastic:main Aug 30, 2023

kibanamachine added the backport:skip This commit does not require backporting label Aug 30, 2023

pmuellr mentioned this pull request Sep 10, 2024

[ResponseOps][Alerting] alert updates not recognizing document conflicts #190376

Open

pmuellr mentioned this pull request Sep 11, 2024

[ResponseOps][Alerting] fix alert conflict resolution to support create #192534

Draft

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ResponseOps] use Data Streams for AAD indices in serverless #160572

[ResponseOps] use Data Streams for AAD indices in serverless #160572

pmuellr commented Jun 26, 2023 •

edited

Loading

pmuellr commented Aug 29, 2023

MadameSheema left a comment

PhilippeOberti left a comment

e40pud left a comment

vitaliidm Aug 30, 2023

vitaliidm Aug 30, 2023

patrykkopycinski Aug 30, 2023

patrykkopycinski Aug 30, 2023

vitaliidm Aug 30, 2023

kibana-ci commented Aug 30, 2023 •

edited

Loading

API count

ESLint disabled line counts

Total ESLint disabled count

[ResponseOps] use Data Streams for AAD indices in serverless #160572

[ResponseOps] use Data Streams for AAD indices in serverless #160572

Conversation

pmuellr commented Jun 26, 2023 • edited Loading

DEFER

Checklist

pmuellr commented Aug 29, 2023

MadameSheema left a comment

Choose a reason for hiding this comment

PhilippeOberti left a comment

Choose a reason for hiding this comment

e40pud left a comment

Choose a reason for hiding this comment

vitaliidm Aug 30, 2023

Choose a reason for hiding this comment

vitaliidm Aug 30, 2023

Choose a reason for hiding this comment

patrykkopycinski Aug 30, 2023

Choose a reason for hiding this comment

patrykkopycinski Aug 30, 2023

Choose a reason for hiding this comment

vitaliidm Aug 30, 2023

Choose a reason for hiding this comment

kibana-ci commented Aug 30, 2023 • edited Loading

💔 Build Failed

Failed CI Steps

Metrics [docs]

Public APIs missing comments

Async chunks

Canvas Sharable Runtime

Public APIs missing exports

API count

ESLint disabled line counts

Total ESLint disabled count

History

pmuellr commented Jun 26, 2023 •

edited

Loading

kibana-ci commented Aug 30, 2023 •

edited

Loading