elastic / elastic/terraform-provider-elasticstack
security_exception_item: comments are append-only; provider needs to honor that on both Read (preserve ids) and Plan (require replacement for non-append edits)
- Dominant language
- Go
- Stars
- 209
- Forks
- 150
- Avg merge
- 23h 11m
- Merged PRs (30d)
- 169
Description
## Summary
The `comments` attribute on `elasticstack_kibana_security_exception_item` (and its sibling `_list`) does not faithfully model what the Kibana API actually allows, which produces two distinct bad outcomes:
1. **Silent duplication on every Update** when the user hasn't touched the comments — every `terraform apply` re-creates the same comments under fresh ids.
2. **`produced inconsistent result after apply`** errors (or worse, silent state drift) when the user tries to edit, remove, or reorder a comment via config.
Below is the empirical evidence from a Kibana 9.x cluster (live test, June 2026), and a proposed fix.
## Kibana API behaviour — verified empirically
Direct `PUT /api/exception_lists/items` against a test item with 4 existing comments:
| Operation in PUT body | HTTP | Effect on Kibana storage |
|---|---|---|
| No change (same ids + same texts in same order) | 200 | no-op ✓ |
| Append a new entry (no `id`) at the END | 200 | new comment added, server-assigned id ✓ |
| Send fewer entries than exist (omit some by id) | 200 | **silently ignored** — all existing kept |
| Same ids, but one entry has different `comment` text | 200 | **silently ignored** — original text preserved |
| Existing entries in non-original order, or new entry NOT at end | **400** | `"item \"comments\" are append only"` |
So the `comments` attribute on the public API is effectively:
- **append-only** for adding (only at the end, only without `id`)
- **immutable** for existing entries (any reordering of id-bearing entries returns 400; any text edit on a known id is silently dropped; any omission is silently dropped)
- **non-shrinkable** — there is no way to delete a single comment via the public API. To remove a comment, the entire exception item must be deleted and recreated.
Additionally, **`created_by` cannot be supplied by clients** — the request structs (`SecurityExceptionsAPICreateExceptionListItemComment`, `SecurityExceptionsAPIUpdateExceptionListItemComment`) only contain `comment` (and optionally `id`). Kibana stamps `created_by` unconditionally from the authenticated API-key owner. Verified by appending a comment with `created_by: \"terraform-alerting\"` and `created_at: \"2020-01-01\"`: Kibana ignored both and stamped the actual API-key owner (`mike.rohland@auto1.local`) and server time.
## Why the current provider behaviour breaks
In `internal/kibana/security_exception_item/`:
- [`models.go:817-861`](https://github.com/elastic/terraform-provider-elasticstack/blob/main/internal/kibana/security_exception_item/models.go#L817) — `commentsToUpdateAPI` builds entries of type `SecurityExceptionsAPIUpdateExceptionListItemComment` and only populates `Comment`, never `Id`. Each Update therefore sends every existing comment as if it were brand-new, and Kibana faithfully appends a fresh copy of each. We have observed a single real item grow 1 → 2 → 4 → 8 comments across three consecutive `terraform apply` runs with no user-side changes whatsoever.
- [`schema.go:232-247`](https://github.com/elastic/terraform-provider-elasticstack/blob/main/internal/kibana/security_exception_item/schema.go#L232) — `comment.id` is `Computed` only with no `PlanModifiers`. The framework therefore marks the id as Unknown during every plan that touches the parent `comments` attribute. Even if `commentsToUpdateAPI` were fixed to send `Id`, it would never have a known value to send.
- Operations the API silently ignores (edit-text, shrink) — the provider's read-after-apply sees the unchanged Kibana state, while the plan saw the user's intent, producing the framework's `produced inconsistent result after apply` diagnostic.
- The 400 case (reorder / prepend) — the provider returns the error verbatim, which is at least correct but not user-friendly when the user's actual intent is achievable via a different plan shape (resource replacement).
## Proposed fix
Three coordinated changes in `internal/kibana/security_exception_item/`:
**(1)** Update `commentsToUpdateAPI` to pass `Id` through when the planned value is Known. (Already proposed in [PR #3551](https://github.com/elastic/terraform-provider-elasticstack/pull/3551); this larger fix supersedes it.)
**(2)** Add `stringplanmodifier.UseStateForUnknown()` to the `comment.id` attribute schema. This makes the planned id carry through from the prior state's value, so (1) actually has something to send.
**(3)** Add a custom `RequiresReplace` PlanModifier on the `comments` list attribute, which triggers a resource replacement whenever the diff between state and plan is NOT pure append-at-end. Specifically: if any id from state is absent or out of position in the plan, or if any state comment's text differs from the corresponding plan comment, the modifier sets `resp.RequiresReplace = true` and emits a warning explaining that all `created_by` attribution on the recreated item will collapse to the API-key owner.
With this triplet of changes the resource accurately models the Kibana API semantics:
| User edits YAML to | Resulting plan |
|---|---|
| No change | no-op |
| Append at end | `~ update in-place` (only the new comment shown) |
| Edit existing text | **`-/+ destroy and replace`** with warning |
| Remove a comment | **`-/+ destroy and replace`** with warning |
| Reorder / prepend | **`-/+ destroy and replace`** with warning |
In every case the user's intent gets applied (text-wise) and the cost is made loud and visible in the plan output. No more silent duplication, no more silent drift, no more `produced inconsistent result` for this attribute.
### Trade-off explicit in the warning
A `-/+` recreate destroys the existing exception item and POSTs a fresh one. Since Kibana stamps `created_by` from the API key, every comment on the new item is attributed to the deploy account (in practice `terraform-alerting`), not the original analyst who wrote them. The warning makes this explicit so the user can decide whether the change is worth the audit-trail consequence.
This is irreducible at the public-API layer — Kibana does not expose any way to preserve client-supplied audit metadata.
## Reproducer (for either symptom)
```hcl
resource "elasticstack_kibana_security_exception_item" "this" {
list_id = "..."
item_id = "test-item"
namespace_type = "single"
type = "simple"
name = "test"
entries = [...]
comments = [
{ comment = "first" },
]
}
```
`terraform apply` once → 1 comment in Kibana. `terraform apply` again (no YAML change) → 2 comments. Continue → 4, 8, 16, … Duplications double on every Update. Verified live (June 2026).
To reproduce the edit-text drift: change the comment text on a 2nd apply. Kibana silently ignores; the read-after-apply state differs from the plan → `produced inconsistent result after apply`.
A PR with all three changes (plus acceptance tests covering each plan shape) follows.
Contributor guide
No contributing guide indexed for this repository
Research direction
Start by reading internal/kibana/security_exception_item/models.go around commentsToUpdateAPI and schema.go around the comment.id and comments attributes. Then inspect the existing security exception item tests and plan modifier patterns before covering unchanged, append, edit, removal, and reorder plans. Done means known ids are preserved, append-only updates avoid duplication, and non-append edits require replacement with the documented warning.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go, terraform
- Domain
- devops, tooling
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 48/100