[cwahn]

Description of your changes

Use a fresh destroy-only Terraform Plugin SDK diff when deleting an external resource.

Reusing the observation diff and setting Destroy retains pending RequiresNew changes. The SDK interprets that as replacement and calls Create after Delete. A destroy-only diff preserves existing timeout metadata while deleting from prior state, without mutating the observation/update diff.

Fixes #420

I have:

• Read and followed Upjet's contribution process.
• Run make reviewable to ensure this PR is ready for review.
• Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

Added table-driven regression tests using real schema.Resource.Apply and Create/Delete callbacks. They cover nil and unchanged diffs, timeout overrides and fallback, pending ordinary updates, and pending ForceNew changes. Each asserts Delete once, Create never, prior-state attributes and identity, cleared tracked state, no panic, and an unchanged observation diff. The ForceNew case also verifies that Update still rejects replacement before any SDK callback.

Before the fix, the ForceNew regression failed by invoking Delete followed by Create and mutating the observation diff. After the fix:

• go test -mod=readonly ./pkg/controller -run '^TestTerraformPluginSDKDelete' -count=1 -v passed.
• go test -mod=readonly ./pkg/controller -count=1 passed.
• go vet -mod=readonly ./pkg/controller passed.
• make reviewable passed after initializing the pinned build submodule.
• gofmt and git diff --check were clean.

No provider-specific logic, dependency updates, or changes to Update's ForceNew rejection are included.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

🧰 Additional context used

📚 Code guidelines (2)

CONTRIBUTING.md — configured
CLAUDE.md — configured

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository: crossplane/upjet/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: 703edb0d-c265-4cd5-a948-6c63c1135fd8

📥 Commits

Reviewing files that changed from the base of the PR and between 8ce2886 and 06e3cea.

📒 Files selected for processing (2)

• pkg/controller/external_tfpluginsdk.go
• pkg/controller/external_tfpluginsdk_test.go

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.

---

📝 Walkthrough

Walkthrough

Delete now passes a fresh destroy-only diff to the Terraform SDK instead of modifying the observed diff. It copies timeout metadata when available. Regression tests cover pending changes, deletion callbacks, tracked state, and preservation of the original diff.

Changes

Terraform SDK deletion

Layer / File(s)	Summary
Destroy-only diff and regression coverage pkg/controller/external_tfpluginsdk.go, pkg/controller/external_tfpluginsdk_test.go	Delete uses a fresh diff with Destroy set and copies timeout metadata from the existing diff when available. Tests cover nil and empty diffs, pending updates, ForceNew, timeout selection, SDK callbacks, tracked state, and preservation of the original diff.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 06e3c

No actionable merge-blocking risk was identified. The change appears ready to merge after normal checks.

Security Architecture Review

Security architecture risk: ⚪ Minimal · up to 06e3c

The change narrows deletion to the existing resource instead of allowing pending changes to trigger replacement. Existing resource identity, provider authority, and update protections are preserved. No introduced or worsened security risk was identified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The behavior change applies to resources using the shared Terraform Plugin SDK deletion path. Its effective targets remain the tracked resource identities and operations permitted by the existing provider credentials; the change does not introduce another identity or authority source.

Security Findings and Attack Paths

• inferred — Pending desired attributes previously carried into deletion are now excluded. Together with the regression assertions, this supports removal of the unintended replacement path rather than expansion of attacker-controlled reachability. No new attack path was established within the reviewed transition.

Trust Boundaries and Controls

• observed — Delete continues to ignore its managed-resource argument and delegates through Resource.Apply using tracked state and existing provider metadata. The diff construction does not add a caller-supplied deletion target, direct provider callback, or alternate credential path.

Resilience and Maintainability Implications

• inferred — Partial external deletion followed by an error still depends on provider retry semantics. Tracker access is individually locked rather than serializing the entire Apply transition, so this change does not establish exactly-once behavior for concurrent direct calls. These are existing lifecycle limitations, not demonstrated regressions from the fresh diff.

🚥 Pre-merge checks | ✅ 6 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Template Breaking Changes	⚠️ Warning	This changes shared SDK controller deletion behavior used by generated providers. The generated controller template wires SDK-backed resources to NewTerraformPluginSDKConnector, and the changed `Del…	Before merging, validate the deletion behavior with multiple generated SDK providers, including a resource with a pending ForceNew change. Add or document those provider-level test results alongside the existing controller regression test…

✅ Passed checks (6 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is under 72 characters and clearly describes the destroy-only diff change for SDK deletion.
Description check	✅ Passed	The description explains the deletion issue, the destroy-only diff fix, and the regression tests. It is directly related to the changeset.
Linked Issues check	✅ Passed	Issue #420 requires deletion to avoid recreation or panic when a pending change requires replacement. Delete now uses a fresh diff with Destroy set and does not include pending attribute changes. …
Out of Scope Changes check	✅ Passed	The whole-PR diff changes SDK-backed deletion and adds regression tests for the issue #420 failure mode. Preserving timeout metadata and verifying that Delete leaves the observation diff unchanged sup…
Configuration Api Breaking Changes	✅ Passed	The reviewed diff changes only pkg/controller/external_tfpluginsdk.go and its test file. It makes no changes under pkg/config, so it does not meet any failure condition in this check.
Generated Code Manual Edits	✅ Passed	The authoritative PR diff changes only pkg/controller/external_tfpluginsdk.go and pkg/controller/external_tfpluginsdk_test.go. Neither filename matches zz_*.go, so the check's failure condition …

Full details: Template Breaking Changes

Explanation

This changes shared SDK controller deletion behavior used by generated providers. The generated controller template wires SDK-backed resources to NewTerraformPluginSDKConnector, and the changed Delete now prevents pending ForceNew changes from triggering replacement. That affects the resource lifecycle across providers. The added regression test exercises one synthetic schema.Resource in pkg/controller; the PR contains no testing with multiple providers, as this check requires for significant shared-controller behavior changes.

Resolution

Before merging, validate the deletion behavior with multiple generated SDK providers, including a resource with a pending ForceNew change. Add or document those provider-level test results alongside the existing controller regression test.

• Fix all pre-merge checks with AI

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}