[Cases] Do not check for version conflicts when adding/updating comments to a case #173740

cnasikas · 2023-12-20T13:18:03Z

Summary

When we add or update a comment (attachment) to a case we update the updatedAt and updatedBy attributes of the case. When we update the case we pass the version of the current case to the SO client for concurrency control. If the version of the current case is different from the one we updating (someone else updated the case) the SO client will throw an error. Although we always fetch the case before adding a comment it may be possible in some weird race conditions for one Kibana node to get the case and before updating another node to update the case. This is extremely difficult to produce when users interact but it may be possible (still rare) when we introduce the case action. This PR does not do any concurrency control when updating the updatedAt and updatedBy attributes of the case when adding/updating a comment.

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

… case

cnasikas · 2023-12-20T13:28:34Z

x-pack/plugins/cases/server/common/models/case_with_comments.ts

@@ -160,7 +160,6 @@ export class CaseCommentModel {
          updated_at: date,
          updated_by: { ...this.params.user },
        },
-        version: this.caseInfo.version,


Removing the version we tell the SO client to not throw on conflicts. It will retry automatically.

cnasikas · 2023-12-20T13:30:24Z

x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/post_comment.ts

+          },
+        });
+
+        await createComment({


As we always get the case before adding the comment (and thus updating the case) the test does not simulate a race condition. It is a basic test to verify the correctness of a simple path and to avoid breaking something in the future.

elasticmachine · 2023-12-20T13:30:40Z

Pinging @elastic/response-ops (Team:ResponseOps)

elasticmachine · 2023-12-20T13:30:41Z

Pinging @elastic/response-ops-cases (Feature:Cases)

cnasikas · 2023-12-20T21:25:24Z

/ci

adcoelho

lgtm 👍

adcoelho · 2023-12-21T09:34:44Z

x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/patch_comment.ts

@@ -549,6 +551,48 @@ export default ({ getService }: FtrProviderContext): void => {
      }
    });

+    describe('partial updates', () => {
+      it('should not result to a version conflict (409) when adding a comment to an updated case', async () => {


this should be

Suggested change

it('should not result to a version conflict (409) when adding a comment to an updated case', async () => {

it('should not result to a version conflict (409) when updating a comment in an updated case', async () => {

right?

Correct! I will fix it.

adcoelho · 2023-12-21T09:48:58Z

What is the value of case updatedBy for case actions btw?

cnasikas · 2023-12-21T09:50:37Z

What is the value of case updatedBy for case actions btw?

This is a very good question. It would be the user that created the rule. I can explain offline why if you want.

kibana-ci · 2023-12-21T10:59:08Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 16d060b

Failed CI Steps

Detection Engine - Security Solution Cypress Tests #4

Test Failures

[job] [logs] Detection Engine - Security Solution Cypress Tests #4 / Timeline scope Alerts checkbox "before each" hook for "alerts checkbox behaves as expected" "before each" hook for "alerts checkbox behaves as expected"

Metrics [docs]

✅ unchanged

History

💚 Build #184681 succeeded eee950e

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

cc @cnasikas

## Summary Depends on: #166267, #170326, #169484, #173740, #173763, #178068, #178307, #178600, #180437 PRs: - #168370 - #169229 - #171754 - #172709 - #173012 - #175107 - #175452 - #175505 - #177033 - #178277 - #177139 - #179796 Fixes: #153837 ## Testing Run Kibana with `--run-examples` if you want to use the "Always firing" rule. Create a rule with a case action in observability and the stack. The security solution is not supported. You should not be able to assign a case action in a security solution rule. 1. Test the "Reopen closed cases" configuration. 2. Test the "Grouping by" configuration. Only one field is allowed. Not all fields are persisted in alerts. If you select a field not part of the alert the case action will create a case where the grouping value is set to `unknow`. 3. Test the "Time window" feature. You can comment out the validation to test for shorter times. 4. Verify that the case action is experimental. 5. Verify that based on the rule type the case is created in the correct solution. 6. Verify that you cannot create a rule with the case action on the basic license. 7. Verify that the execution of the case action fails if you do not have permission for cases. Pending work on the system actions framework level to not allow users to create rules with system actions where they do not have permission. 8. Stress test the case action by creating multiple rules. ### Checklist Delete any items that are not applicable to this PR. - [x] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios ### For maintainers - [x] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) ## Release notes Automatically create cases when an alert is triggered. --------- Co-authored-by: kibanamachine <[email protected]> Co-authored-by: adcoelho <[email protected]> Co-authored-by: Janki Salvi <[email protected]>

Do not check for version conflicts when adding/updating comments to a…

eee950e

… case

cnasikas self-assigned this Dec 20, 2023

cnasikas added Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Feature:Cases Cases feature v8.13.0 labels Dec 20, 2023

cnasikas mentioned this pull request Dec 20, 2023

[Cases] Case action #168369

Merged

3 tasks

cnasikas commented Dec 20, 2023

View reviewed changes

cnasikas marked this pull request as ready for review December 20, 2023 13:30

cnasikas requested a review from a team as a code owner December 20, 2023 13:30

cnasikas added the release_note:skip Skip the PR/issue when compiling release notes label Dec 20, 2023

adcoelho approved these changes Dec 21, 2023

View reviewed changes

PR feedback

16d060b

cnasikas enabled auto-merge (squash) December 21, 2023 09:54

cnasikas merged commit 9922453 into elastic:main Dec 21, 2023
38 checks passed

cnasikas deleted the pu_case_when_attaching branch December 21, 2023 10:59

kibanamachine added the backport:skip This commit does not require backporting label Dec 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Cases] Do not check for version conflicts when adding/updating comments to a case #173740

[Cases] Do not check for version conflicts when adding/updating comments to a case #173740

cnasikas commented Dec 20, 2023 •

edited

Loading

cnasikas Dec 20, 2023

cnasikas Dec 20, 2023

elasticmachine commented Dec 20, 2023

elasticmachine commented Dec 20, 2023

cnasikas commented Dec 20, 2023

adcoelho left a comment

adcoelho Dec 21, 2023

cnasikas Dec 21, 2023 •

edited

Loading

adcoelho commented Dec 21, 2023 •

edited

Loading

cnasikas commented Dec 21, 2023

kibana-ci commented Dec 21, 2023

	it('should not result to a version conflict (409) when adding a comment to an updated case', async () => {
	it('should not result to a version conflict (409) when updating a comment in an updated case', async () => {

[Cases] Do not check for version conflicts when adding/updating comments to a case #173740

[Cases] Do not check for version conflicts when adding/updating comments to a case #173740

Conversation

cnasikas commented Dec 20, 2023 • edited Loading

Summary

Checklist

For maintainers

cnasikas Dec 20, 2023

Choose a reason for hiding this comment

cnasikas Dec 20, 2023

Choose a reason for hiding this comment

elasticmachine commented Dec 20, 2023

elasticmachine commented Dec 20, 2023

cnasikas commented Dec 20, 2023

adcoelho left a comment

Choose a reason for hiding this comment

adcoelho Dec 21, 2023

Choose a reason for hiding this comment

cnasikas Dec 21, 2023 • edited Loading

Choose a reason for hiding this comment

adcoelho commented Dec 21, 2023 • edited Loading

cnasikas commented Dec 21, 2023

kibana-ci commented Dec 21, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

History

cnasikas commented Dec 20, 2023 •

edited

Loading

cnasikas Dec 21, 2023 •

edited

Loading

adcoelho commented Dec 21, 2023 •

edited

Loading