Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Security Solution][Detection Engine] Fix chunking logic so importing many rules with exception lists works #190447

Merged
merged 5 commits into from
Aug 15, 2024

Conversation

marshallmain
Copy link
Contributor

@marshallmain marshallmain commented Aug 13, 2024

Summary

@Mikaayenson reported that rule import fails when importing many rules with exceptions. We are only fetching the first 100 exception lists for reference verification after importing exceptions from the rule import, so if there are more than 100 exception lists then they will be imported but the rules will appear to have a missing exception list reference.

Since we're already processing the rules in chunks, this PR changes the logic so we fetch exception lists for each chunk of 50 rules independently. There's still a possibility that 50 rules could have more than 1000 exception lists referenced and we might run into the same problem again. To fix the problem permanently we need to update the exception lists client to support paging through exception lists so we can reliably fetch all exception lists referenced by a chunk of rules. However that's a more involved fix that'll have to wait for a follow up PR.

Testing

The rule export file below contains 210 rules, each with an exception list. It triggers the failure when imported on main but works with this PR. Rename the file to .ndjson from .json - github doesn't allow uploading .ndjson files so I renamed it before uploading here.
rules_export (1).json

@marshallmain marshallmain marked this pull request as ready for review August 13, 2024 19:49
@marshallmain marshallmain requested review from a team as code owners August 13, 2024 19:49
@marshallmain marshallmain requested review from rylnd and maximpn August 13, 2024 19:49
@marshallmain marshallmain added release_note:skip Skip the PR/issue when compiling release notes v8.16.0 labels Aug 13, 2024
Comment on lines +233 to +242
const limiter = limit(10);
const promises = ids.map((id) =>
limiter(() =>
supertest
.delete(`${EXCEPTION_LIST_URL}?id=${id}&namespace_type=${type}`)
.set('kbn-xsrf', 'true')
.send()
)
);
await Promise.all(promises);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This massively speeds up deleting all the exceptions created by the new test in this PR.

Copy link
Contributor

@rylnd rylnd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a few minor quibbles about some tests/test comments, but this is a great improvement over the existing code.

Thanks for providing the ndjson file; I was able to do my own red/green smoke testing with it 👍

@@ -1376,6 +1378,65 @@ export default ({ getService }: FtrProviderContext): void => {
});
});

it('should resolve 100 or more exception references when importing into a clean slate', async () => {
// So importing a rule that references an exception list
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering why this comment was added, and then I saw that it was copied from an adjacent test 😭 .

I don't think the comments are needed; I would use either test structure or just more descriptive variable names to convey what these rules/exceptions represent:

// variable names:
const rulesWithExceptionLists = range(150)
...
const rulesWithExceptionListsPayload = combineArraysToNdJson();
// test structure:
describe('with a payload describing 150 rules, each with an exception list', () => {
  let saturatedRulePayload: string;
  beforeEach(() => {
    saturatedRulePayload = combineArraysToNdJson();
  });
  
  it('resolves the large payload when importing into a fresh environment');
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops yeah I glossed over the comments 😅 I removed those and fixed up the variable names a bit

}));
const ndjson = combineArraysToNdJson(rules, exceptionLists, exceptionItems);

// Importing the "simpleRule", along with the exception list
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment isn't correct, and IMO should just be deleted. Again, if this was really tricky I would maybe hide it behind a helper like importRulePayload(), but in this case it's pretty obvious.

Copy link
Contributor

@maximpn maximpn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marshallmain thank you for this improvement 👍

I tested it locally and indeed this PR fixes the problem with the attached rule export file while it's reproducible on main.

It seems I found a bug not related to this PR while testing it. The file name is rules_export (1).json but .ndjson is expected. UI shows an error but I can't upload .ndjson file after I tried .json. I'll double check on that and create a bug ticket if it doesn't exist yet. Import button is disabled.

Screenshot 2024-08-15 at 08 21 44

P.S. We still have an issue in rule import related to long processing time and connection drop. A large number of rule exceptions decreases a threshold when the problem becomes noticeable. It means we have a set of problems in rule import we'll need to solve eventually.

@@ -8,3 +8,7 @@
export function combineToNdJson(...parts: unknown[]): string {
return parts.map((p) => JSON.stringify(p)).join('\n');
}

export function combineArraysToNdJson(...arrays: unknown[][]): string {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct me if I'm wrong but combineToNdJson function is flexible enough so combineArraysToNdJson isn't required. Invocation could look like

const ndjson = combineToNdJson(...rules, ...exceptionLists, ...exceptionItems);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, that would work also. I try to avoid using the spread operator out of habit for arrays of more than a few items since it copies the items inside. I only went halfway initially though by using combineToNdJson internally so the arrays were still being spread. I added a new function to handle array inputs specifically without spreading.

@marshallmain
Copy link
Contributor Author

It seems I found a bug not related to this PR while testing it. The file name is rules_export (1).json but .ndjson is expected. UI shows an error but I can't upload .ndjson file after I tried .json. I'll double check on that and create a bug ticket if it doesn't exist yet. Import button is disabled.

That's a strange one, thanks for checking and following up! I forgot to put in the PR description before that the .json file needs to be renamed, github wouldn't let me upload it as .ndjson.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@marshallmain marshallmain merged commit 10f4c19 into elastic:main Aug 15, 2024
38 checks passed
@marshallmain marshallmain deleted the rule-import-bulk-exceptions branch August 15, 2024 20:15
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Aug 15, 2024
rylnd added a commit to rylnd/kibana that referenced this pull request Aug 20, 2024
This splits our old utility, `preparePrebuiltRulesForImport`, into a
class that has to responsibilities:

1. Ensures that the latest prebuilt #ssets are retrieved during `setup`
2. Retrieves the relevant prebuilt assets from a list of rules to import

Similar to elastic#190447, this was done so that we perform the per-rule work in
chunks during import, rather than all at once beforehand.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes v8.16.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants