Add security exception item #1496
Conversation
nick-benoit
force-pushed
the
add_security_exception_item
branch
from
0eb08b1 to
4fc1308
Compare
…tack into add_security_exception_item
…tack into add_security_exception_item
There was a problem hiding this comment.
Pull request overview
This PR adds a new elasticstack_kibana_security_exception_item resource for managing Kibana security exception items. Exception items define specific query conditions to prevent detection rules from generating alerts.
Key Changes
- New resource implementation with comprehensive CRUD operations
- Support for multiple entry types (match, match_any, list, exists, nested, wildcard)
- Extensive validation logic for entry configurations
- Full test coverage including unit tests for different entry types and validation scenarios
Reviewed changes
Copilot reviewed 38 out of 38 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| provider/plugin_framework.go | Registers the new exception item resource in the provider |
| internal/kibana/security_exception_item/*.go | Core implementation files (schema, CRUD operations, models, validation) |
| internal/kibana/security_exception_item/testdata/* | Terraform test configurations for acceptance tests |
| internal/kibana/security_exception_item/acc_test.go | Comprehensive acceptance test suite |
| examples/resources/elasticstack_kibana_security_exception_item/resource.tf | Example usage documentation |
| CODING_STANDARDS.md | Updated with new coding guidelines |
| }, | ||
| }, | ||
| }, | ||
| "expire_time": schema.StringAttribute{ |
There was a problem hiding this comment.
It seems there is an issue where if a round second is provided then the go client doesn't send the milliseconds and generates a 400 from kibana
So eg "2027-12-31T23:59:59.001Z" works and "2027-12-31T23:59:59.000Z" does not. Still trying to find an elegant solution to this.
There was a problem hiding this comment.
Golang will format times as RFC3339 in JSON which isn't strictly compatible with the API spec. I wonder if we should remove the format: date-time from Security_Exceptions_API_ExceptionListItemExpireTime so we can take control of the string formatting ourselves?
There was a problem hiding this comment.
Yeah I think that is probably the best bet
There was a problem hiding this comment.
I added that here: 4e4b02d
I added custom serialization that always includes the millis. It seems that since this is a subset of RFC3339 we can still use that for deserialization.
This includes kibana bug fix: elastic/kibana#159223
| switch entryType { | ||
| case "match", "wildcard": | ||
| // 'value' is required (only validate if not unknown) | ||
| if entry.Value.IsNull() { |
There was a problem hiding this comment.
Validation checks only for null values but doesn't check for unknown values. When values are unknown (e.g., references to resources not yet created), they should be skipped from validation. Use utils.IsKnown(entry.Value) instead to properly handle both null and unknown cases, consistent with the pattern used elsewhere in the file (lines 37, 54).
| if entry.Value.IsNull() { | |
| if !utils.IsKnown(entry.Value) { | |
| // skip validation if value is unknown | |
| } else if entry.Value.IsNull() { |
| ) | ||
| } | ||
| // 'operator' is required (only validate if not unknown) | ||
| if entry.Operator.IsNull() { |
There was a problem hiding this comment.
Multiple validation checks use IsNull() without checking for unknown values. These should all use utils.IsKnown() instead to properly skip validation when values are unknown (e.g., from resource references). This is critical for Terraform's two-phase execution model where unknown values are common during planning.
| if entry.Values.IsNull() { | ||
| diags.AddError( | ||
| "Missing Required Field", | ||
| fmt.Sprintf("Entry type 'match_any' requires 'values' to be set at %s.", entryPath), | ||
| ) | ||
| } | ||
| // 'operator' is required (only validate if not unknown) | ||
| if entry.Operator.IsNull() { |
There was a problem hiding this comment.
Multiple validation checks use IsNull() without checking for unknown values. These should all use utils.IsKnown() instead to properly skip validation when values are unknown (e.g., from resource references). This is critical for Terraform's two-phase execution model where unknown values are common during planning.
| if entry.Values.IsNull() { | |
| diags.AddError( | |
| "Missing Required Field", | |
| fmt.Sprintf("Entry type 'match_any' requires 'values' to be set at %s.", entryPath), | |
| ) | |
| } | |
| // 'operator' is required (only validate if not unknown) | |
| if entry.Operator.IsNull() { | |
| if utils.IsKnown(entry.Values) && entry.Values.IsNull() { | |
| diags.AddError( | |
| "Missing Required Field", | |
| fmt.Sprintf("Entry type 'match_any' requires 'values' to be set at %s.", entryPath), | |
| ) | |
| } | |
| // 'operator' is required (only validate if not unknown) | |
| if utils.IsKnown(entry.Operator) && entry.Operator.IsNull() { |
| ) | ||
| } | ||
| // 'operator' is required (only validate if not unknown) | ||
| if entry.Operator.IsNull() { |
There was a problem hiding this comment.
Multiple validation checks use IsNull() without checking for unknown values. These should all use utils.IsKnown() instead to properly skip validation when values are unknown (e.g., from resource references). This is critical for Terraform's two-phase execution model where unknown values are common during planning.
|
|
||
| case "list": | ||
| // 'list' object is required (only validate if not unknown) | ||
| if entry.List.IsNull() { |
There was a problem hiding this comment.
Multiple validation checks use IsNull() without checking for unknown values. These should all use utils.IsKnown() instead to properly skip validation when values are unknown (e.g., from resource references). This is critical for Terraform's two-phase execution model where unknown values are common during planning.
| } | ||
|
|
||
| // 'operator' is always required for nested entries (only validate if not unknown) | ||
| if entry.Operator.IsNull() { |
There was a problem hiding this comment.
Multiple validation checks use IsNull() without checking for unknown values. These should all use utils.IsKnown() instead to properly skip validation when values are unknown (e.g., from resource references). This is critical for Terraform's two-phase execution model where unknown values are common during planning.
| if entry.Operator.IsNull() { | |
| if utils.IsKnown(entry.Operator) && entry.Operator.IsNull() { |
| } | ||
|
|
||
| // toUpdateRequest converts the Terraform model to API update request | ||
| func (m *ExceptionItemModel) toUpdateRequest(ctx context.Context, resourceId string, client clients.MinVersionEnforceable) (*kbapi.UpdateExceptionListItemJSONRequestBody, diag.Diagnostics) { |
There was a problem hiding this comment.
There's a lot of duplication between toCreate and toUpdate. I wonder if we can just build the update request from the create request, or reuse some of the common code.
There was a problem hiding this comment.
What do you think of this approach? a4da71b
This is kind of similar to what we did in security detection rules
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
It looks like the acceptance tests have become pretty flaky with these exception list resources? |
Adds security exception item resource (docs)
Rel: #1332