-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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] Initial rule upgrade/install endpoints implementation #155517
Conversation
ddd7761
to
8fcf210
Compare
8fcf210
to
99f377b
Compare
7a2e0be
to
48307d3
Compare
48307d3
to
3b37309
Compare
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
Pinging @elastic/security-solution (Team: SecuritySolution) |
💚 Build Succeeded
Metrics [docs]Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @xcrzx |
stats: PrebuiltRulesStatusStats; | ||
}; | ||
/** Aggregated info about all prebuilt rules */ | ||
stats: PrebuiltRulesStatusStats; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the endpoint open to returning other kind of information other than stats
in the future; do we have any future use cases for that?
Just wandering why we would hold all data within the single stats
property instead of making the stats themselves first-class properties.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, I do not anticipate extending the endpoint to return information other than stats
. However, it's always good practice to keep future use cases in mind and design with flexibility for potential changes. If a new use case does arise in the future, it would be pretty straightforward to adapt this endpoint.
...Array.from(baseRulesMap.keys()), | ||
...Array.from(latestRulesMap.keys()), | ||
...Array.from(currentRulesMap.keys()), | ||
]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's not necessary to create a Set to de-duplicate the ruleId
s, since the Map will override any repeated value with the last key-pair created anyways.
I guess both ways achieve the same thing, not sure if this could make a difference time-complexity-wise, since the number of rules we'll be dealing is not sufficiently large.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea here is to gather all available rule IDs to iterate over, as these three structures only partially overlap. There could be missing base rules or rules that are no longer available in the package, meaning no latest version, and so on. Ultimately, we aim to create a structure similar to the following:
const map = {
rule_id_1: {
current: undefined,
base: undefined,
target: {},
},
rule_id_2: {
current: {},
base: {},
target: undefined,
},
};
But a similar result could probably be achieved by iterating over the three structures one by one.
const ruleAssetsClient = createPrebuiltRuleAssetsClient(soClient); | ||
const ruleObjectsClient = createPrebuiltRuleObjectsClient(rulesClient); | ||
|
||
const { mode, pick_version: globalPickVersion = PickVersionValues.TARGET } = request.body; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, if I understand correctly, for the current Milestone we won't need to pass the pick_version
param, since we will always want to update to the target version of the rule.
Is that correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's right. The version to pick will always be TARGET
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed, great changes, thanks for this! Some of the refactoring you did here on the diff logic made it even easier to understand.
I'm approving now but I haven't tested the endpoints; maybe we can sync tomorrow on how you've been testing? I guess that's where the generate_assets_route
comes in handy :)
I'm going to merge the PR so @jpdjere can start integrating it as soon as possible. If any issues arise, I can address them in follow-ups. However, since the functionality is under a feature flag, the risk of introducing any bugs is quite low. |
Resolves: #148186
Resolves (partially): #148184
Summary
This PR adds two new endpoints:
POST /internal/detection_engine/prebuilt_rules/installation/_perform
(see this issue for more detail on the endpoint interface)POST /internal/detection_engine/prebuilt_rules/upgrade/_perform
(see this issue for more detail on the endpoint interface)For both endpoints, I've implemented two modes:
ALL_RULES
andSPECIFIC_RULES
. So from the rules management page, all rules could be installed or upgraded in bulk or one by one if needed.Things not covered by this PR
MERGED
version for rule upgrades, but it is not needed so long as we do not allow rule modification