fix(secure): persist API-returned version on Falco element updates - #747
Open
ombellare wants to merge 1 commit into
Open
fix(secure): persist API-returned version on Falco element updates#747ombellare wants to merge 1 commit into
ombellare wants to merge 1 commit into
Conversation
The Update path of the Falco element resources sends the version held in state to the API but discards the version returned in the response. Create already writes it back, so after any successful update state kept the pre-update version while the backend had moved on. This is currently latent: the v2 macro service performs no version comparison, and Read repopulates the field on the next refresh. It becomes user-visible the moment the backend enforces the version as an optimistic concurrency token, and it already sends a knowingly stale value on `-refresh=false` runs. Fixed in sysdig_secure_macro, sysdig_secure_list and the container, filesystem, network, process, stateful and syscall rule resources, all following the pattern sysdig_secure_rule_falco already used. Also: * Replace the sysdig_secure_macro doc example. It appended a macro the customer had just created, which backends that have not migrated to the v2 macro storage reject with "The field 'name' must not be the same as another Secure UI macro" — so the primary documented example failed for those users. The example now appends a Sysdig-provided macro, which works on every backend, and the append note records the backend difference and the operator-prefix requirement for the appended condition. * Add TestAccMacroVersionIsPersistedOnUpdate, which asserts version is 1 after create and 2 after an in-place update. It fails on the unfixed code with `Attribute 'version' expected "2", got "1"`. * Add TestAccMacroAppendToCustomMacro, covering appends to a customer-owned macro. Existing coverage only appended a Sysdig-provided macro, which is why the documentation problem went unnoticed. The test is opt-in via SYSDIG_SECURE_MACROS_V2 because it cannot pass on unmigrated backends. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates Sysdig Secure Terraform resources to persist the backend-returned version after updates, and expands documentation/tests around macros—especially append behavior and version handling.
Changes:
- Persist updated
versionin state for multiple Secure rule resources, macros, and lists after Update API calls. - Add acceptance tests covering macro version persistence on update and appending to customer-owned macros (opt-in).
- Improve Secure macro documentation with clearer append semantics and backend-compatibility notes.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| website/docs/r/secure_macro.md | Clarifies macro append usage, includes examples, and documents backend limitations. |
| sysdig/resource_sysdig_secure_rule_syscall.go | Writes API-returned rule version back to Terraform state on update. |
| sysdig/resource_sysdig_secure_rule_stateful.go | Writes API-returned rule version back to Terraform state on update. |
| sysdig/resource_sysdig_secure_rule_process.go | Writes API-returned rule version back to Terraform state on update. |
| sysdig/resource_sysdig_secure_rule_network.go | Writes API-returned rule version back to Terraform state on update. |
| sysdig/resource_sysdig_secure_rule_filesystem.go | Writes API-returned rule version back to Terraform state on update. |
| sysdig/resource_sysdig_secure_rule_container.go | Writes API-returned rule version back to Terraform state on update. |
| sysdig/resource_sysdig_secure_macro_test.go | Adds acceptance tests for version persistence and append-to-custom-macro behavior. |
| sysdig/resource_sysdig_secure_macro.go | Writes API-returned macro version back to Terraform state on update. |
| sysdig/resource_sysdig_secure_list.go | Writes API-returned list version back to Terraform state on update. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+97
to
105
| updatedMacro, err := client.UpdateMacro(ctx, macro) | ||
| if err != nil { | ||
| return diag.FromErr(err) | ||
| } | ||
| sysdigClients.AddCleanupHook(sendPoliciesToAgents) | ||
|
|
||
| _ = d.Set("version", updatedMacro.Version) | ||
|
|
||
| return nil |
Comment on lines
+97
to
105
| updatedList, err := client.UpdateList(ctx, list) | ||
| if err != nil { | ||
| return diag.FromErr(err) | ||
| } | ||
| sysdigClients.AddCleanupHook(sendPoliciesToAgents) | ||
|
|
||
| _ = d.Set("version", updatedList.Version) | ||
|
|
||
| return nil |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
The
Updatepath of the Falco element resources sends theversionheld in state to the API, then discards theversionreturned in the response.Createalready writes it back, so after any successful update state kept the pre-update version while the backend had moved on.Fixed in
sysdig_secure_macro,sysdig_secure_list, and thecontainer,filesystem,network,process,statefulandsyscallrule resources — all following the patternsysdig_secure_rule_falcoalready used.Impact
Latent today, worth fixing now. The v2 macro service performs no version comparison, and
Readrepopulates the field on the next refresh, so ordinary applies self-heal. It becomes user-visible the moment the backend treatsversionas an optimistic concurrency token — every second consecutive update would fail with a spurious conflict. It also already sends a knowingly stale value on-refresh=falseruns and saved plans, andterraform state showreports a version the backend does not have.Observed against an onprem backend before the fix: macro at server version 2 while state reported 1; a further
-refresh=falseupdate sent version 1 and the server advanced to 3.Documentation
The
sysdig_secure_macroexample appended a macro the customer had just created. Backends that have not migrated to the v2 macro storage reject that withThe field 'name' must not be the same as another Secure UI macro, so the resource's primary documented example failed outright for those users. The example now appends a Sysdig-provided macro, which works on every backend.The
appendnote now also records that appending a customer-owned macro requires a migrated backend, that the appended condition must begin with a logical operator, and that the "extend only once" limit applies to earlier backends but is not enforced by newer ones.Tests
TestAccMacroVersionIsPersistedOnUpdate— assertsversionis1after create and2after an in-place update. Fails on the unfixed code withAttribute 'version' expected "2", got "1".TestAccMacroAppendToCustomMacro— covers appending a customer-owned macro. Existing coverage only appended a Sysdig-provided macro (macroAppendToDefaultusescontainer), which is why the documentation problem went unnoticed. Opt-in viaSYSDIG_SECURE_MACROS_V2since it cannot pass on unmigrated backends.go build ./...,go vet(incl. acceptance tags) andgofmtare clean. Acceptance run against an onprem backend:TestAccMacroVersionIsPersistedOnUpdateTestAccMacroAppendToCustomMacroTestAccListTestAccRuleFalcoTerminalShell,...WithMinimumEngineVersionTestAccRuleFalcoDataSource,TestAccRuleFalcoCountDataSourceThe rule resource changes are compile-verified only:
TestAccRuleContainer/Filesystem/Network/Process/Syscallare unconditionally skipped in the repo (List matching rules are deprecated - skipping tests).Two pre-existing failures on that environment, unrelated to this change and reproduced identically on unmodified
master:TestAccMacrostep 6/7 —Undefined macro '<name>' used in filterwhen a macro references another custom macro created in the same apply. Reproduces onmaster; step number and resource vary between runs, so it looks like a race in the v2 validation path rather than anything in this diff.TestAccRuleStatefulDataSource/...CountDataSource—cannot append to a non-existent rule 'API Gateway Enumeration Detected'; that default rule is absent on this backend.🤖 Generated with Claude Code