[Security Solution]Fix incorrect number of invalid connectors is shown on the toast message by WafaaNasr · Pull Request #152313 · elastic/kibana

WafaaNasr · 2023-02-28T08:38:05Z

Summary

Addresses [Security Solution] Incorrect number of invalid connectors is shown on the toast message #151988

Before the fix, the number of missing connectors was always 1, even if the message says the invalid connectors are more

After,

Checklist

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

…orrect-invalid-connectors

maximpn

@WafaaNasr thank you for fixing this bug 🙏 Let's just explore edge cases to make sure there are no bugs here.

maximpn · 2023-03-01T11:15:50Z

x-pack/plugins/security_solution/public/common/components/import_data_modal/utils.ts

-    const { error } = connectorError;
+  let concatenatedActionIds: string = '';
+  const mappedErrors = actionConnectorsErrors.map((connectorError) => {
+    const { id, error } = connectorError as ImportResponseError;


Why isn't it possible to just calculate the number of errors? Can we get more than one error per connector?

Generally speaking ImportRulesResponseError | ImportResponseError only differs by id and rule_id which come from BulkError (just the fact BulkError allows to omit both id and rule_id which is not supposed to happen I expect but it's another story). If we still support rule_id (most probably for backwards compatibility) can we have a situation when it's in use and this code is gonna fail? If it's not a case so then we should remove rule_id so as ImportResponseError type casting won't be necessary.

Why isn't it possible to just calculate the number of errors? Can we get more than one error per connector?

-Yes true.
Forex. if the user has rule to import in one file, and this rule has 3 connectors.

For instance, if 2 of them have missing connectors, what will happen as per the Old workflow=> One error message will be generated with the count of the failing connectors, and one error object will be pushed in the array.
2 connectors are missing. Connector ids missing are: X, Y

And just to clarify the id here is the action-connectors id not the rule_id

Hey @maximpn ! rule_id was initially created to act as a signature id so that a rule could be recognized, say, across platforms. This allows sigma, snort, suricata etc to identify a rule with a singular "signature id" that is never regenerated over time. So the concept of rule_id should hopefully stick around the solution unless platform has come up with a way for it to be supported more globally within a saved object. This comes in very handy on import when a user could be importing a rule they've ported over from elsewhere and want to maintain it's reference to the signature id. I don't think it would be in our interest to remove it's use. Like @WafaaNasr pointed out, the id she references is to the connector, the rule_id points back to the rule that had an action pointing to said connector so the user knows which rule was the source of the error.

If there's a worry that id would not exist in connectorError because it ends up being ImportRulesResponseError vs ImportResponseError maybe we can add a check for id != null.

If there's larger refactoring you think is necessary (this route is now a bit dated and could benefit from refactoring as it's complexity has grown a lot), feel free to create a ticket and we can keep discussing what the response should look like and how rule_id fits into the import use case.

Hey @yctercero, thank you for the explanation! My concern is misleading types as BulkError at backend and ImportRulesResponseError at frontend don't look related to the connectors. ImportResponseError looks too generic (by name). According to what I see in the code it's pretty easy to use some tailored for connectors error type which action_connectors_errors will use.

Absence of such a type leads to reusing id and rule_id fields to store serialized arrays in a case of missing connectors. I'm afraid it doesn't match with SO importer errors so that importer seems to returning an error per connector. Merging it now and creating a ticket looks like adding a technical debt while we already have it a lot.

There's definitely refactoring needed overall on import. It used to just be rules and now is exceptions, actions and connectors. With the added comments, we can certainly look to simplify and refactor this flow.

I understand the concern with creating technical debt. As we are far into the BCs, adding scope in this bug fix can also open us up to further bugs. Our team is working on documenting this route, the technical debt and suggested updates. I can be sure to make reference back to your concerns to ensure they're addressed in any upcoming refactors.

Here we go #152648

Thank you @maximpn !

maximpn · 2023-03-01T11:18:15Z

..._engine/rule_management/logic/import/action_connectors/import_rule_action_connectors.test.ts

              '1 connector is missing. Connector id missing is: cabc78e0-9031-11ed-b076-53cc4d57aaf1',
            status_code: 404,
          },
+          id: 'cabc78e0-9031-11ed-b076-53cc4d57aaf1',


Interesting, you provide both id and rule_id in tests while it should be one of them according to the operated types. See my question regarding them above.

They are different id is the actionConnectors ids and the rule_id is the rule to be imported.

And we need it, in case the user has multiple rules in one file and not all of them are failing

I see. For some reason actionsImporter returns a comma separated list of connector identifiers in id field and we don't have any breakdown on this. So this list corresponds to a rule.

maximpn · 2023-03-02T15:18:04Z

...on/server/lib/detection_engine/rule_management/logic/import/action_connectors/utils/index.ts

        : 'connector is missing. Connector id missing is:';
    errors.push(
      createBulkErrorObject({
+        id: actionsIds.join(),


As id field stores a serialized array it worth to have a comment in BulkError and ImportResponseError to highlight that fact it can contain more than one identifier in a case of missing connectors.

We'll go ahead and add a comment there. Looks like rule_id has been used this way for a bit. Will be sure to document.

Please review bc5547c

maximpn · 2023-03-02T15:18:22Z

...on/server/lib/detection_engine/rule_management/logic/import/action_connectors/utils/index.ts

+        id: actionsIds.join(),
        statusCode: 404,
        message: `${actionsIds.length} ${errorMessage} ${actionsIds.join(', ')}`,
        ruleId: ruleIds,


As rule_id field stores a serialized array it worth to have a comment in BulkError and ImportRuleResponseError to highlight that fact it can contain more than one identifier in a case of missing connectors.

We'll go ahead and add a comment there. Looks like rule_id has been used this way for a bit. Will be sure to document.

Please review here bc5547c

WafaaNasr · 2023-03-02T16:12:23Z

@elasticmachine merge upstream

yctercero

LGTM! Thanks for the fix and adding comments to clarify some of our existing types.

…/WafaaNasr/kibana into fix-incorrect-invalid-connectors

kibana-ci · 2023-03-02T17:27:53Z

💚 Build Succeeded

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`	15.7MB	15.7MB	+644.0B

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`securitySolution`	428	430	+2

Total ESLint disabled count

id	before	after	diff
`securitySolution`	506	508	+2

History

💚 Build #111763 succeeded 21308a4
💚 Build #111045 succeeded 4d2e9a1
💔 Build #110970 failed 9f710a2

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

cc @WafaaNasr

maximpn

@WafaaNasr Thank you for addressing my comments 👍 I've created a refactoring ticket so the rest of my concerns should be addressed there.

…n on the toast message (elastic#152313) ## Summary - Addresses elastic#151988 Before the fix, the number of missing connectors was always 1, even if the message says the invalid connectors are more ![image](https://user-images.githubusercontent.com/12671903/221798261-8fb7ba28-3a1a-4269-9e00-a540f7febb1b.png) After, ![image](https://user-images.githubusercontent.com/12671903/221798298-1f3728b2-d7a9-41c9-a0f4-d80892a7c0f1.png) ### Checklist - [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 --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit c0d2b0f)

kibanamachine · 2023-03-06T08:31:47Z

💚 All backports created successfully

Status	Branch	Result
✅	8.7

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…s shown on the toast message (#152313) (#152661) # Backport This will backport the following commits from `main` to `8.7`: - [[Security Solution]Fix incorrect number of invalid connectors is shown on the toast message (#152313)](#152313)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Wafaa Nasr <wafaa.nasr@elastic.co>

…n on the toast message (elastic#152313) ## Summary - Addresses elastic#151988 Before the fix, the number of missing connectors was always 1, even if the message says the invalid connectors are more ![image](https://user-images.githubusercontent.com/12671903/221798261-8fb7ba28-3a1a-4269-9e00-a540f7febb1b.png) After, ![image](https://user-images.githubusercontent.com/12671903/221798298-1f3728b2-d7a9-41c9-a0f4-d80892a7c0f1.png) ### Checklist - [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 --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

WafaaNasr added 3 commits February 23, 2023 19:13

initial

c75409b

Merge branch 'main' of https://github.com/elastic/kibana into fix-inc…

a9e7772

…orrect-invalid-connectors

fix incorrect number of connectors in the toast title

4434471

WafaaNasr added release_note:skip Skip the PR/issue when compiling release notes Team:Security Solution Platform v8.7.0 labels Feb 28, 2023

WafaaNasr self-assigned this Feb 28, 2023

WafaaNasr requested review from a team as code owners February 28, 2023 08:38

WafaaNasr requested a review from maximpn February 28, 2023 08:38

WafaaNasr added the ci:cloud-deploy Create or update a Cloud deployment label Feb 28, 2023

WafaaNasr added 3 commits February 28, 2023 08:39

change variable name

7ade2cc

add des

9f710a2

tests

4d2e9a1

maximpn reviewed Mar 1, 2023

View reviewed changes

maximpn reviewed Mar 2, 2023

View reviewed changes

explain the type casting in mapErrorMessage

a2b46ef

Merge branch 'main' into fix-incorrect-invalid-connectors

21308a4

yctercero approved these changes Mar 2, 2023

View reviewed changes

WafaaNasr added 2 commits March 2, 2023 16:55

add description for BulkError id and rule_id

bc5547c

Merge branch 'fix-incorrect-invalid-connectors' of https://github.com…

fa9a39e

…/WafaaNasr/kibana into fix-incorrect-invalid-connectors

maximpn approved these changes Mar 4, 2023

View reviewed changes

WafaaNasr merged commit c0d2b0f into elastic:main Mar 6, 2023

kibanamachine added the v8.8.0 label Mar 6, 2023

kibanamachine mentioned this pull request Mar 6, 2023

[8.7] [Security Solution]Fix incorrect number of invalid connectors is shown on the toast message (#152313) #152661

Merged

kibanamachine mentioned this pull request Mar 6, 2023

[Synthetics] Fix filters persistance #152543

Merged

WafaaNasr deleted the fix-incorrect-invalid-connectors branch February 6, 2024 10:27

elasticmachine unassigned WafaaNasr Oct 29, 2024

Conversation

WafaaNasr commented Feb 28, 2023 • edited by kibanamachine Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

maximpn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maximpn Mar 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WafaaNasr commented Mar 2, 2023

Uh oh!

yctercero left a comment

Choose a reason for hiding this comment

Uh oh!

kibana-ci commented Mar 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Metrics [docs]

Async chunks

ESLint disabled line counts

Total ESLint disabled count

History

Uh oh!

maximpn left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Mar 6, 2023

💚 All backports created successfully

Questions ?

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

WafaaNasr commented Feb 28, 2023 •

edited by kibanamachine

Loading

maximpn Mar 2, 2023 •

edited

Loading

kibana-ci commented Mar 2, 2023 •

edited

Loading

maximpn left a comment •

edited

Loading