Generate Better Index Keys

elastic / detection-rules

https://www.elastic.co/guide/en/security/current/detection-engine-overview.html

Other

1.91k stars 492 forks source link

Generate Better Index Keys #3826

Closed shashank-elastic closed 3 months ago

shashank-elastic commented 3 months ago

Issues

https://github.com/elastic/detection-rules/issues/3774

Summary

Allow greater index key mapping , if indexes exceeds the number of characters in letters (which is 52 characters, uppercase and lowercase) the current implementation will throw an IndexError
To go around these issues, generate greater index keys.

Code Changes

Remove the letters that is being used for index keys
Replace the iterator as the key for the index
This would mean, n number of indexes will always have n keys because of the iterator

Testing

Unit tests TestPackages/ test_package_summary covers generation of the package summary
No additional testing may be required.

terrancedejesus commented 3 months ago

@shashank-elastic - Any specific reason not to just rely on integers here? See my comment from the same bug I observed. https://github.com/elastic/detection-rules/pull/3819#discussion_r1655138865

Thoughts?

shashank-elastic commented 3 months ago

@shashank-elastic - Any specific reason not to just rely on integers here? See my comment from the same bug I observed. #3819 (comment)

Thoughts?

As such there was an inclination towards using a better combination of letters, But the fix suggested to make the index as an key would be the easiest solution!. we would not have any further updates to this method. Count of indexes will result in count of keys at any point in time.

Would u want to ship the index fix as part of the PR, let me know will close this here @terrancedejesus

terrancedejesus commented 3 months ago

@shashank-elastic feel free to use the code I added from my PR and I will remove it. Makes sense to deduplicate if you already had an issue and PR going! I'll pull in changes.

        index_map = {index: letters[i] for i, index in enumerate(sorted(indexes))}
        index_map = {index: str(i) for i, index in enumerate(sorted(indexes))}

shashank-elastic commented 3 months ago

@shashank-elastic feel free to use the code I added from my PR and I will remove it. Makes sense to deduplicate if you already had an issue and PR going! I'll pull in changes.
        index_map = {index: letters[i] for i, index in enumerate(sorted(indexes))}
        index_map = {index: str(i) for i, index in enumerate(sorted(indexes))}

Done, please review @terrancedejesus