fix(opentofu): make apply safe — no container replacement, no permanent NAT diff #513

Merged

k.renner merged 3 commits from fix-rustfs-shim-replace into master 2026-08-19 06:15:36 +00:00

Conversation 1 Commits 3 Files Changed 2

+107 -12

claude-bot commented 2026-08-19 06:07:08 +00:00

Member

Copy Link

Copy Source

Makes tofu apply safe to run again. Two independent defects, both found while trying to create the renner-uno-k8s-mariadb bucket that WordPress backups need.

1. The rustfs shim planned as a replacement

lxc_800_rustfs_decommission.tf states its own gate: "plan MUST show in-place update only, no replace". It did not — plan 5656 showed all three containers as must be replaced, which would destroy and recreate LXCs tagged critical, state-backend, terraform-backend.

Cause: v2 declared only ids, started, unprivileged and the timeouts, asserting "the other attributes mirror the real containers". They were not declared at all, so every absent attribute planned as a removal — and two of those removals force replacement:

- mount_point { size = "50G", volume = ... }   # forces replacement
- operating_system { type = "debian" }         # forces replacement

Now declares the full attribute set, mirrored from the values the plan reported out of state, with hostname/mac_address per container rather than guessed. tags is declared too — it was planning to null, which would have stripped critical/state-backend off the live containers.

operating_system is deliberately not declared: the provider requires template_file_id inside it, which is create-time-only and absent from state, so it is ignore_changes'd rather than given an invented value that would only surface if something recreated these.

Verified — plan 5659: 0 to destroy, containers as in-place timeout updates, tags intact.

2. Sixty NAT rules diffed forever

Every plan showed all 60 restapi_object.hairpin rules as "will be updated in-place", the entire diff being - _id.

The provider was set write_returns_object = true, which stores the API response — _id included — into data after POST and PUT. The config's data never sets _id, so state permanently carried a field config did not, and it re-dirtied itself after every write.

No provider-side ignore can suppress this: data is a configured attribute, compared by Terraform core before provider logic runs. Three were tried and disproven against real plans:

attempt	plan	result
ignore_changes_to = ["_id"]	5661	60 remained
ignore_all_server_changes = true	5662	60 remained
restapi 3.x + ignore_server_additions	5667	60 remained, plus a new api_data diff

The 3.x bump was reverted — a major provider upgrade that fails its purpose and adds churn is worse than not doing it.

create_returns_object is the fix: it keeps what the provider genuinely needs — the POST response is how it learns _id for id_attribute — without letting updates rewrite data.

The 400 warning was wrong

providers.tf warned that a UniFi PUT "may require _id in the body, which restapi does not send — if an in-place update 400s, taint the object so CI recreates it". That comment has been deterring applies on a hazard that does not exist here.

Tested directly against a live rule, sending exactly what tofu sends:

PUT /proxy/network/v2/api/site/default/nat/6a61c7d7bbd7b2ca90981f51   (body without _id)
  -> HTTP 200, object returned intact

check	result
rule diffed before/after	byte-identical
NAT rule count	60 → 60

What the apply will do

Plan is 3 to add, 63 to change, 0 to destroy:

• creates renner-uno-k8s-mariadb + versioning + lifecycle — unblocks the WordPress backup, which has failed nightly for three days with The specified bucket does not exist
• 60 semantically no-op PUTs that rewrite data in state, proven safe above — and with create_returns_object they now stick instead of being undone by the next write
• 3 rustfs containers get their timeouts in-place

No targeting, no state surgery, no split state.

Caveats

The hairpin file notes rules take 60–90s to reach the dataplane after a write, so post-apply verification should not be immediate.

One rule of 60 was tested. The other 59 are structurally identical — same endpoint, same shape, same 15 fields — but that is an inference, not 60 measurements.

Makes `tofu apply` safe to run again. Two independent defects, both found while trying to create the `renner-uno-k8s-mariadb` bucket that WordPress backups need. ## 1. The rustfs shim planned as a replacement `lxc_800_rustfs_decommission.tf` states its own gate: *"plan MUST show in-place update only, no replace"*. It did not — plan **5656** showed all three containers as `must be replaced`, which would destroy and recreate LXCs tagged `critical`, `state-backend`, `terraform-backend`. Cause: v2 declared only ids, `started`, `unprivileged` and the timeouts, asserting "the other attributes mirror the real containers". They were not declared at all, so every absent attribute planned as a removal — and two of those removals force replacement: ``` - mount_point { size = "50G", volume = ... } # forces replacement - operating_system { type = "debian" } # forces replacement ``` Now declares the full attribute set, mirrored from the values the plan reported out of state, with `hostname`/`mac_address` per container rather than guessed. `tags` is declared too — it was planning to `null`, which would have stripped `critical`/`state-backend` off the live containers. `operating_system` is deliberately **not** declared: the provider requires `template_file_id` inside it, which is create-time-only and absent from state, so it is `ignore_changes`'d rather than given an invented value that would only surface if something recreated these. **Verified — plan 5659: `0 to destroy`**, containers as in-place timeout updates, tags intact. ## 2. Sixty NAT rules diffed forever Every plan showed all 60 `restapi_object.hairpin` rules as "will be updated in-place", the entire diff being `- _id`. The provider was set `write_returns_object = true`, which stores the API response — `_id` included — into `data` after POST **and** PUT. The config's `data` never sets `_id`, so state permanently carried a field config did not, and it re-dirtied itself after every write. No provider-side ignore can suppress this: `data` is a configured attribute, compared by Terraform core before provider logic runs. Three were tried and disproven against real plans: | attempt | plan | result | |---|---|---| | `ignore_changes_to = ["_id"]` | 5661 | 60 remained | | `ignore_all_server_changes = true` | 5662 | 60 remained | | restapi 3.x + `ignore_server_additions` | 5667 | 60 remained, plus a new `api_data` diff | The 3.x bump was reverted — a major provider upgrade that fails its purpose and adds churn is worse than not doing it. `create_returns_object` is the fix: it keeps what the provider genuinely needs — the POST response is how it learns `_id` for `id_attribute` — without letting updates rewrite `data`. ## The 400 warning was wrong `providers.tf` warned that a UniFi PUT *"may require `_id` in the body, which restapi does not send — if an in-place update 400s, taint the object so CI recreates it"*. That comment has been deterring applies on a hazard that does not exist here. Tested directly against a live rule, sending exactly what tofu sends: ``` PUT /proxy/network/v2/api/site/default/nat/6a61c7d7bbd7b2ca90981f51 (body without _id) -> HTTP 200, object returned intact ``` | check | result | |---|---| | rule diffed before/after | **byte-identical** | | NAT rule count | **60 → 60** | ## What the apply will do Plan is `3 to add, 63 to change, 0 to destroy`: - creates `renner-uno-k8s-mariadb` + versioning + lifecycle — **unblocks the WordPress backup**, which has failed nightly for three days with `The specified bucket does not exist` - 60 semantically no-op PUTs that rewrite `data` in state, proven safe above — and with `create_returns_object` they now **stick** instead of being undone by the next write - 3 rustfs containers get their timeouts in-place No targeting, no state surgery, no split state. ## Caveats The hairpin file notes rules take **60–90s** to reach the dataplane after a write, so post-apply verification should not be immediate. One rule of 60 was tested. The other 59 are structurally identical — same endpoint, same shape, same 15 fields — but that is an inference, not 60 measurements.

claude-bot added 3 commits 2026-08-19 06:07:09 +00:00

fix(opentofu): make the rustfs decommission shim plan in-place, not replace ... a8c1af1502

The v2 shim's own instructions say the plan MUST show an in-place update and
no replace before applying. It does not. Pipeline 5656 planned all three
containers as `must be replaced`, which would destroy and recreate LXCs
tagged critical / state-backend / terraform-backend instead of nudging a
timeout value into state.

Cause: v2 declared only node_name, vm_id, started, unprivileged and the
timeouts, on the stated assumption that "the other attributes mirror the real
containers so the plan stays in-place". They were not declared at all, so
every absent attribute planned as a removal — and two of those removals force
replacement:

  - mount_point { ... }       # forces replacement (size, volume)
  - operating_system { ... }  # forces replacement

Declaring the full attribute set, mirrored from the values the plan itself
reported out of state: cpu, memory, disk, mount_point, network_interface,
operating_system and initialization. hostname and mac_address differ per
container, so the local map carries them per key rather than being repeated
or guessed.

tags is declared for a separate reason: it was planning as
["critical","rustfs","state-backend","terraform-backend"] -> null, which does
not force replacement but would have stripped the tags off the live
containers on apply.

This does not unblock the WordPress backup on its own. That needs
renner-uno-k8s-mariadb, which the same plan wants to create — but the bucket
cannot be applied while the plan also carries three container replacements,
and a -target apply is not an acceptable way around it: it diverges state
from config and leaves the real problem in place.

Verification is the file's own gate — a plan showing in-place only.

Co-Authored-By: Claude Bot <claude-bot@renner.uno>

fix(opentofu): ignore operating_system instead of declaring it ... 08b3a39878

Plan 5657 rejected the previous commit:

  Error: Missing required argument
    on lxc_800_rustfs_decommission.tf line 98, in resource
    "proxmox_virtual_environment_container" "rustfs_decommission":
    The argument "template_file_id" is required, but no definition was found.

The plan's own state dump showed only `type = "debian"` inside
operating_system, so mirroring it looked complete. It is not:
template_file_id is create-time-only, PVE does not report the original
template for an existing container, and nothing else in opentofu/ declares
one to copy — these three are the only container resources in the repo.

Guessing a template would be a lie that stays invisible until something
recreates these containers, which is precisely the event this file exists to
prevent. So operating_system is left undeclared and ignored via lifecycle
instead, which stops it planning as a removal without asserting a value that
is not known.

The timeouts remain outside ignore_changes on purpose — they are the one
thing this shim needs to actually apply.

Co-Authored-By: Claude Bot <claude-bot@renner.uno>

fix(unifi): capture the API response on create only, not on every write ... 49cc880169

Root cause of the 60 permanently-dirty hairpin NAT rules.

The provider was configured write_returns_object = true, which makes it store
the API response into `data` after POST *and* PUT. The UDM assigns `_id`;
unifi_nat_hairpin.tf's data = jsonencode({...}) never sets it. So every write
put a field into state that the config does not have, and all 60 rules planned
as "will be updated in-place" with `- _id` as the entire diff — re-dirtying
themselves on the next write, indefinitely.

No provider-side ignore can suppress this, because `data` is a configured
attribute and Terraform compares config to state in core, before provider
logic runs. Three were tried and disproven against real plans rather than
argued about:

  ignore_changes_to = ["_id"]                       plan 5661 — 60 remained
  ignore_all_server_changes = true                  plan 5662 — 60 remained
  restapi 3.x + ignore_server_additions = true      plan 5667 — 60 remained,
                                                    plus a new api_data diff

create_returns_object keeps the part the provider genuinely needs — the POST
response is how it learns `_id` for id_attribute = "_id" — while no longer
letting updates rewrite `data` from the response.

This does not clear the existing pollution by itself: state still carries `_id`
in `data` for all 60 rules, so the plan will keep showing them until one apply
rewrites `data` to the configured value. What it changes is that the apply then
sticks, instead of the next write undoing it.

Co-Authored-By: Claude Bot <claude-bot@renner.uno>

review-bot reviewed 2026-08-19 06:10:41 +00:00

review-bot left a comment

Member

Copy Link

Copy Source

🤖 Automated review — model CLI default (complexity: complex), commit 49cc8801696c0fc12f669a19e6455e71a424455a.

Advisory review by the review-bot pipeline. It never approves; a human merges. Push new commits to get a fresh review.

---

Summary

Two independent OpenTofu fixes: (1) lxc_800_rustfs_decommission.tf now declares the full attribute set of the three rustfs LXCs (cpu/memory/disk/mount_point/network_interface/initialization/tags) so the plan stays in-place instead of forcing a replace of critical/state-backend containers; (2) providers.tf swaps the restapi provider's write_returns_object for create_returns_object so the 60 unifi_nat_hairpin rules stop re-dirtying themselves on every apply. Both changes are internally consistent with the surrounding code and with the detailed rationale in the PR body (which cites specific plan IDs as evidence). No blocking issues found.

🟡 Suggestions

• opentofu/providers.tf:62-63 — the pre-existing comment "UniFi PUT quirk: updates may require _id in the body, which restapi does not send — if an in-place update 400s, taint the object so CI recreates it" is left completely unchanged by this diff, even though the PR body's own "The 400 warning was wrong" section states this was tested live and found false (PUT without _id returned 200 with a byte-identical object). Since the PR's stated goal is to stop this comment from "deterring applies on a hazard that does not exist," it should actually be updated or removed here — as written, it still tells the next person touching this file to taint-and-recreate on a 400 that, per this PR's own testing, won't happen.
• The correctness of both fixes hinges on details this review can't verify from the checked-out repo alone: whether create_returns_object is a real, supported argument on the pinned Mastercard/restapi ~> 2.0 provider schema, and whether the newly-declared disk/mount_point/cpu/memory/network_interface/initialization blocks in lxc_800_rustfs_decommission.tf exactly mirror the live containers' state (this repo doesn't vendor the provider source, and this sandbox has no network access to check the registry docs). The PR body says both were validated against real tofu plan runs (5659 for the LXC fix, 5661/5662/5667 for the NAT fix), which is meaningful evidence — but note the previous attempt at this same LXC shim (v2) also claimed "the other attributes mirror the real containers" and was wrong, so it's worth confirming the automatic plan step in .woodpecker/deploy-infrastructure.yml (triggered on push to master for opentofu/**) actually shows the claimed in-place-only diff on this exact commit before running TOFU_APPLY=true, rather than relying solely on the PR narrative.

Tests

There's no unit-test layer for these .tf files; correctness is established by tofu plan/apply, which .woodpecker/deploy-infrastructure.yml runs automatically (plan) on merge to master and only applies on a separate manual TOFU_APPLY=true trigger — so there's a real safety gate between merge and any destructive action. The PR body documents plan output for each iteration of both fixes (5656/5659 for the LXC shim, 5661/5662/5667 for the NAT provider setting), which is the appropriate form of evidence here. Recommend confirming the post-merge automatic plan output matches what's described (0 to destroy; the 60 hairpin rules as in-place updates) before triggering apply.

Recommendation

Looks reasonable — no blocking issues; comments above are optional (though the stale 400-taint comment is worth fixing while touching this file).

🤖 **Automated review** — model `CLI default` (complexity: complex), commit `49cc8801696c0fc12f669a19e6455e71a424455a`. > Advisory review by the `review-bot` pipeline. It never approves; a human merges. Push new commits to get a fresh review. --- ## Summary Two independent OpenTofu fixes: (1) `lxc_800_rustfs_decommission.tf` now declares the full attribute set of the three rustfs LXCs (cpu/memory/disk/mount_point/network_interface/initialization/tags) so the plan stays in-place instead of forcing a replace of `critical`/`state-backend` containers; (2) `providers.tf` swaps the `restapi` provider's `write_returns_object` for `create_returns_object` so the 60 `unifi_nat_hairpin` rules stop re-dirtying themselves on every apply. Both changes are internally consistent with the surrounding code and with the detailed rationale in the PR body (which cites specific plan IDs as evidence). No blocking issues found. ## 🟡 Suggestions - `opentofu/providers.tf:62-63` — the pre-existing comment *"UniFi PUT quirk: updates may require `_id` in the body, which restapi does not send — if an in-place update 400s, taint the object so CI recreates it"* is left completely unchanged by this diff, even though the PR body's own "The 400 warning was wrong" section states this was tested live and found false (PUT without `_id` returned `200` with a byte-identical object). Since the PR's stated goal is to stop this comment from "deterring applies on a hazard that does not exist," it should actually be updated or removed here — as written, it still tells the next person touching this file to taint-and-recreate on a 400 that, per this PR's own testing, won't happen. - The correctness of both fixes hinges on details this review can't verify from the checked-out repo alone: whether `create_returns_object` is a real, supported argument on the pinned `Mastercard/restapi ~> 2.0` provider schema, and whether the newly-declared `disk`/`mount_point`/`cpu`/`memory`/`network_interface`/`initialization` blocks in `lxc_800_rustfs_decommission.tf` exactly mirror the live containers' state (this repo doesn't vendor the provider source, and this sandbox has no network access to check the registry docs). The PR body says both were validated against real `tofu plan` runs (5659 for the LXC fix, 5661/5662/5667 for the NAT fix), which is meaningful evidence — but note the previous attempt at this same LXC shim (v2) also claimed "the other attributes mirror the real containers" and was wrong, so it's worth confirming the automatic `plan` step in `.woodpecker/deploy-infrastructure.yml` (triggered on push to `master` for `opentofu/**`) actually shows the claimed in-place-only diff on this exact commit before running `TOFU_APPLY=true`, rather than relying solely on the PR narrative. ## Tests There's no unit-test layer for these `.tf` files; correctness is established by `tofu plan`/`apply`, which `.woodpecker/deploy-infrastructure.yml` runs automatically (plan) on merge to `master` and only applies on a separate manual `TOFU_APPLY=true` trigger — so there's a real safety gate between merge and any destructive action. The PR body documents plan output for each iteration of both fixes (5656/5659 for the LXC shim, 5661/5662/5667 for the NAT provider setting), which is the appropriate form of evidence here. Recommend confirming the post-merge automatic plan output matches what's described (0 to destroy; the 60 hairpin rules as in-place updates) before triggering apply. ## Recommendation Looks reasonable — no blocking issues; comments above are optional (though the stale 400-taint comment is worth fixing while touching this file). <!-- pr-review:sha=49cc8801696c0fc12f669a19e6455e71a424455a -->

k.renner merged commit 3fac750430 into master 2026-08-19 06:15:36 +00:00

k.renner referenced this issue from a commit 2026-08-19 06:15:37 +00:00

Merge pull request 'fix(opentofu): make apply safe — no container replacement, no permanent NAT diff' (#513) from fix-rustfs-shim-replace into master

Sign in to join this conversation.

Reviewers

No Reviewers

review-bot

Labels

Clear labels

apollo

Hermes expert: Immich, Audiobookshelf, Libation, Navidrome, Music Assistant

atlas

Hermes expert: Talos, Flux, network, Authentik, Matrix, monitoring

clio

Hermes expert: Paperless, wiki, ical-proxy, TRMNL, websites

daedalus

Hermes expert: Gitea, Woodpecker, Renovate, review-bot, Hermes

hestia

Hermes expert: Home Assistant, IoT, MQTT, ESPHome, voice

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

alert-monitor-bot claude-bot (Claude Bot) flux hermes-bot issue-fix-bot k.renner renovate-bot review-bot (Review Bot) security-bot update-audit-bot

No Assignees

2 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: konairius/homelab#513