-
Notifications
You must be signed in to change notification settings - Fork 120
Install prebuilt rules #1296
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
base: main
Are you sure you want to change the base?
Install prebuilt rules #1296
Conversation
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.
tags = ["OS: Linux", "OS: Windows"]
I'm not sure how I feel about this resource as it's written.
We're kind of implementing two things (install, and enable) within a single resource. That's not necessarily bad, but I wonder if it would be simpler to have two independent resources in this case.
There's also a reasonable amount of complexity in managing the tag set, and I'm not sure there's a tangible benefit for users in supporting multiple tags within a single resource. If we reduced this to a single tag then Terraform would be able to fully manage the state transitions for us.
WDYT about something like:
resource "elasticstack_kibana_install_prebuilt_rules" "rules" {}
resource "elasticstack_kibana_enable_prebuilt_rules" "linux" {
key = "OS"
value = "Linux"
}
resource "elasticstack_kibana_enable_prebuilt_rules" "windows" {
key = "OS"
value = "Windows"
}
}) | ||
} | ||
|
||
func testAccPrebuiltRuleDestroy(s *terraform.State) error { |
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.
If this is a no-op we can just omit it entirely from the test definition.
TimelinesNotUpdated int `json:"timelines_not_updated"` | ||
} | ||
|
||
func (model *prebuiltRuleModel) populateFromStatus(ctx context.Context, status *prebuiltRulesStatus) diag.Diagnostics { |
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.
func (model *prebuiltRuleModel) populateFromStatus(ctx context.Context, status *prebuiltRulesStatus) diag.Diagnostics { | |
func (model *prebuiltRuleModel) populateFromStatus(ctx context.Context, status *prebuiltRulesStatus) { |
This function never returns a non-nil diagnostic
var resp *kbapi.ReadPrebuiltRulesAndTimelinesStatusResponse | ||
var err error | ||
|
||
if spaceID != "default" { | ||
resp, err = client.API.ReadPrebuiltRulesAndTimelinesStatusWithResponse(ctx, func(ctx context.Context, req *http.Request) error { | ||
req.Header.Set("kbn-space-id", spaceID) | ||
return nil | ||
}) | ||
} else { | ||
resp, err = client.API.ReadPrebuiltRulesAndTimelinesStatusWithResponse(ctx) | ||
} |
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.
var resp *kbapi.ReadPrebuiltRulesAndTimelinesStatusResponse | |
var err error | |
if spaceID != "default" { | |
resp, err = client.API.ReadPrebuiltRulesAndTimelinesStatusWithResponse(ctx, func(ctx context.Context, req *http.Request) error { | |
req.Header.Set("kbn-space-id", spaceID) | |
return nil | |
}) | |
} else { | |
resp, err = client.API.ReadPrebuiltRulesAndTimelinesStatusWithResponse(ctx) | |
} | |
resp, err = client.API.ReadPrebuiltRulesAndTimelinesStatusWithResponse(ctx, func(ctx context.Context, req *http.Request) error { | |
if spaceID != "default" { | |
req.Header.Set("kbn-space-id", spaceID) | |
return nil | |
} | |
}) |
}, | ||
}, | ||
"tags": schema.ListAttribute{ | ||
Description: "A list of tag names to filter prebuilt rules for enabling/disabling. Use ['all'] to enable all prebuilt rules, or an empty list to install but not enable any rules.", |
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.
Description: "A list of tag names to filter prebuilt rules for enabling/disabling. Use ['all'] to enable all prebuilt rules, or an empty list to install but not enable any rules.", | |
Description: "A list of tag names to filter prebuilt rules for enabling/disabling.", |
We're explicitly returning an error for all
, it doesn't seem helpful to suggest it in the docs.
Is there a reason we expect this to be set to an empty list to install but not enable, the attribute is marked as optional so I'd expect that just omitting it entirely should be fine?
ElementType: types.StringType, | ||
Optional: true, | ||
Validators: []validator.List{ | ||
listvalidator.SizeAtLeast(0), |
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.
listvalidator.SizeAtLeast(0), | |
listvalidator.SizeAtLeast(1), |
Linked to the above comment, I'd expect that if a user just wants to install the rules then they can leave tags
undefined, and if they want to enable rules then they should include some tags. I don't really feel strongly about enforcing a non-empty list, but if we're going to maintain support for an empty list of tags then we can just remove this validator altogether, it's impossible to have a list with a negative size.
diags := performBulkActionByTags(ctx, client, spaceID, "enable", tags) | ||
if diags.HasError() { | ||
return diags | ||
} | ||
|
||
return nil |
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.
diags := performBulkActionByTags(ctx, client, spaceID, "enable", tags) | |
if diags.HasError() { | |
return diags | |
} | |
return nil | |
return performBulkActionByTags(ctx, client, spaceID, "enable", tags) |
In theory there could be warning diags that are getting dropped here.
} | ||
|
||
// Enable rules for added tags | ||
if len(addedTags) > 0 { |
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.
We only enable newly added tags here, is it possible this misses potential new rules:
- User create this resource with
os:Windows
- New rules are added matching
os:Windows
(Stack release? do we push rulesets outside of Stack releases IDK?) - User re-runs this resource with the same tag set. The new rules are presumably installed, but they'll never be updated because no tags were added.
|
||
var resp *kbapi.PerformRulesBulkActionResponse | ||
|
||
if spaceID != "default" { |
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.
Same, can we move the if inside the request editor
@@ -0,0 +1 @@ | |||
terraform import elasticstack_kibana_prebuilt_rule.enable "default" |
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 resource doesn't implement an import handler, so this command will fail.
IMO I'm not sure import actually makes sense for this resource though, it's quite difficult for us to know that all rules matching a given tag set have been enabled. I don't think there's a real impact to just running create again if the rules have been installed and the expected rules are already enabled?
return nil | ||
} | ||
|
||
func performBulkActionByTags(ctx context.Context, client *kibana_oapi.Client, spaceID, action string, tags []string) diag.Diagnostics { |
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'm certainly unfamiliar with this API, so apologies if I'm missing something obvious. What prevents this resource from operating on user defined rules that also match the given tag set? That is, what restricts the resource to only enabling 'pre-built' rules, and not user defined rules which also match a given tag?
I think this is the best way to go. One resource to install and ensure the latest prebuilt rules exist on the cluster. And another resource that enables rules based on one tag. This also makes sense since a user may not want the prebuilt rules but wants to have terraform enable rules for them. Having everything under one resource forces the user to install the prebuilt rules. Do we want to split this into two separate PRs for the resources? Thanks for the review! @tobio |
Yea, that would be simplest IMO. It's not a hill I'm going to die on though :)
Absolutely no problem, great to see some new additions here! |
This PR adds a resource to install and enable prebuilt rules. It will also update the prebuilt rules if it detects there is an update from Elastic.
This will install and enabled rules with the following tags
This will only install rules and not enable any of them
The initial issue wanted to support enabling all rules at once but this is not feasible and highly unlikely that a user will want to enable 1500 rules. Thus, one can only enable 1000 rules at a time.
Set the minimum version to 8.0.0 as I don't think this was supported in version 7.X and the EOL for 7.17.x is coming soon.
This PR will close #750