Skip synthetic TF state creation when external identifier is empty#625
Skip synthetic TF state creation when external identifier is empty#625bitgandtter wants to merge 1 commit intocrossplane:mainfrom
Conversation
bitgandtter
requested review from
erhancagirici,
sergenyalcin and
ulucinar
as code owners
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested reviewers
Thanks — any preference on additional tests or edge cases you want covered for empty 🚥 Pre-merge checks | ✅ 7✅ Passed checks (7 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Review rate limit: 0/1 reviews remaining, refill in 55 minutes and 54 seconds.Comment |
|
|
bitgandtter
force-pushed
the
fix-empty-tfid-state
branch
from
e051c1f to
b02fb78
Compare
When a managed resource has no external-name (tfID is empty), upjet was creating a synthetic TF state with all parameters but no ID. This caused TF Plugin Framework providers to fail on Refresh (Read requires ID) before Create could be attempted. Skip synthetic state creation entirely when tfID is empty. This leaves the state file empty, causing Refresh to return Exists=false and properly triggering the Create flow for new resources. Signed-off-by: Yasmany Cubela Medina <yasmanycm@gmail.com>
bitgandtter
force-pushed
the
fix-empty-tfid-state
branch
from
b02fb78 to
0f0bbaa
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/terraform/files.go`:
- Around line 245-248: Add a table-driven unit test for the logic in
pkg/terraform/files.go that contains the guard "if !empty ||
meta.WasDeleted(fp.Resource) || tfID == """: create cases that cover (1) empty
state + not deleted + tfID == "" and assert that no synthetic terraform.tfstate
file is written, and (2) complementary cases where state should be written. In
the test, instantiate a fake FileProcessor/FP (or the same test helper used
elsewhere), set fp.Resource so meta.WasDeleted(fp.Resource) returns false, set
empty=true and tfID="" for the critical case, run the function that evaluates
this guard (the function in files.go that writes synthetic state), and assert on
filesystem or in-memory writer that no file was created; repeat for cases where
a file should be created to lock behavior and prevent regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3f2bdd7c-08f9-4d67-bdd9-5ac1a662192b
📒 Files selected for processing (1)
pkg/terraform/files.go
| // Skip synthetic state creation when: state already exists, resource is | ||
| // being deleted, or there is no external identifier yet (new resource). | ||
| // Without an ID, Refresh would fail on providers that require it for Read. | ||
| if !empty || meta.WasDeleted(fp.Resource) || tfID == "" { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Please add (or point to) a regression test for the empty tfID guard
Nice fix—this guard is key for IdentifierFromProvider flows. Could you add or reference a table-driven unit test that covers: empty state + not deleted + tfID == "" should not write synthetic terraform.tfstate? That would lock the behavior and prevent future observe/create regressions.
As per coding guidelines: **/*.go: “All Upjet code must be covered by tests… Upjet encourages the use of table driven unit tests.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/terraform/files.go` around lines 245 - 248, Add a table-driven unit test
for the logic in pkg/terraform/files.go that contains the guard "if !empty ||
meta.WasDeleted(fp.Resource) || tfID == """: create cases that cover (1) empty
state + not deleted + tfID == "" and assert that no synthetic terraform.tfstate
file is written, and (2) complementary cases where state should be written. In
the test, instantiate a fake FileProcessor/FP (or the same test helper used
elsewhere), set fp.Resource so meta.WasDeleted(fp.Resource) returns false, set
empty=true and tfID="" for the critical case, run the function that evaluates
this guard (the function in files.go that writes synthetic state), and assert on
filesystem or in-memory writer that no file was created; repeat for cases where
a file should be created to lock behavior and prevent regressions.
2 participants
When a managed resource uses IdentifierFromProvider and has no external-name yet (a new resource that needs to be created), EnsureTFState creates a synthetic Terraform state populated with all forProvider parameters but no
valid resource ID. This synthetic state causes Refresh (tofu apply -refresh-only) to attempt a Read against the cloud API using those parameters, which fails on Terraform Plugin Framework providers because the Read function
strictly requires the resource ID to construct the API call.
The result is that Observe returns an error before it can determine that the resource does not exist, so the managed reconciler never reaches the Create path. The resource gets stuck in a permanent observe failed: cannot run
refresh loop.
Root cause: EnsureTFState unconditionally creates a synthetic state when the state file is empty, regardless of whether the resource has an external identifier. For Terraform Plugin SDK providers this was silently tolerated
because their Read functions handle missing IDs gracefully (returning "not found"). Terraform Plugin Framework providers are stricter — they require the ID to build the API request and fail with errors like missing required
_id parameter.
Fix: Add tfID == "" to the early-return guard in EnsureTFState. When there is no external identifier, the state file is left empty. This allows Refresh to return Exists: false, which correctly triggers the Create flow in the
managed reconciler.
Flow before fix (new resource, no external-name):
Flow after fix:
This affects all upjet-based providers that wrap Terraform Plugin Framework providers using IdentifierFromProvider external-name configuration. We encountered this with provider-upjet-cloudflare (wrapping Cloudflare TF
provider v5 which uses Plugin Framework), specifically when creating cloudflare_ruleset and cloudflare_dns_record resources from scratch.
I have:
How has this code been tested
Tested end-to-end with https://github.com/wildbitca/provider-upjet-cloudflare (wrapping cloudflare/cloudflare TF provider v5.18.0, Plugin Framework-based) on a Crossplane v2.2.0 cluster:
The fix was validated using a go.mod replace directive pointing to the fork branch, with 6 Crossplane claims managing 20 rulesets and 61 DNS records across 13 Cloudflare zones — all reaching SYNCED=True, READY=True.