feat: add resource identity support to TF Plugin Framework integration#623
feat: add resource identity support to TF Plugin Framework integration#623erikmiller-gusto wants to merge 2 commits intocrossplane:mainfrom
Conversation
erikmiller-gusto
requested review from
erhancagirici,
sergenyalcin and
ulucinar
as code owners
erikmiller-gusto
force-pushed
the
feat/resource-identity-support
branch
from
9b47a10 to
6c32257
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds persistent propagation of Terraform Plugin Framework resource identity through Read, Plan, and Apply: the external TF plugin framework client stores planned identity, RPC requests include Prior/Current/Planned identity fields, responses update the client and AsyncTracker, and AsyncTracker gains thread-safe identity getters/setters. Tests added. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ExternalClient
participant AsyncTracker
participant TFProvider as TF Provider (RPC)
Client->>ExternalClient: Observe / Plan / Apply (Create/Update/Delete)
ExternalClient->>AsyncTracker: GetFrameworkIdentity()
AsyncTracker-->>ExternalClient: current identity (may be nil)
ExternalClient->>TFProvider: ReadResourceRequest(CurrentIdentity) / PlanResourceChangeRequest(PriorIdentity) / ApplyResourceChangeRequest(PlannedIdentity)
TFProvider-->>ExternalClient: ReadResourceResponse(NewIdentity) / PlanResourceChangeResponse(PlannedIdentity) / ApplyResourceChangeResponse(NewIdentity)
ExternalClient->>AsyncTracker: SetFrameworkIdentity(NewIdentity or nil)
AsyncTracker-->>ExternalClient: ack
ExternalClient-->>Client: Return observed/plan/apply result
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Would you like me to point to the specific lines where identities are read/set for quicker review? Thanks for the work on this — it clarifies the identity lifecycle. 🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (6 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/controller/external_tfpluginfw_test.go (1)
148-153:⚠️ Potential issue | 🔴 CriticalWire the per-test config into the helper—use
testConfig.cfginstead of the undefined barecfg.At line 152, the helper references
cfg, but unlike the similar pattern inexternal_tfpluginsdk_test.go(which declarescfgas a package-scoped variable), this file doesn't have that declaration. The helper receives the config via thetestConfigparameter and already correctly usestestConfig.cfgelsewhere (line 131). This reference should be consistent.Suggested fix
- config: cfg, + config: testConfig.cfg,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/external_tfpluginfw_test.go` around lines 148 - 153, The helper constructing the terraformPluginFrameworkExternalClient mistakenly uses an undefined bare cfg; update the returned struct to use the per-test configuration passed in via testConfig (i.e., replace cfg with testConfig.cfg) so the config field is wired from the testConfig parameter when creating the terraformPluginFrameworkExternalClient (keep other symbols like ts: terraform.Setup and mockTPFProvider unchanged).
🤖 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/controller/external_tfpluginfw_test.go`:
- Around line 189-191: The test helper is pre-populating
tpfExternal.plannedIdentity which masks a regression in getDiffPlanResponse()
not copying PlanResourceChangeResponse.PlannedIdentity into the client; change
the tests so the mock PlanResourceChangeResponse remains populated but remove
the global pre-set of tpfExternal.plannedIdentity in the shared helper and
instead explicitly set tpfExternal.plannedIdentity only in the
create/update/delete tests that require a preplanned value (leave
TestTPFPlanIdentityPropagation to rely on Observe() to populate
plannedIdentity), locating the change around the shared helper that constructs
the mock response (getDiffPlanResponse /
PlanResourceChangeResponse.PlannedIdentity) and places
tpfExternal.plannedIdentity before Observe().
In `@pkg/controller/external_tfpluginfw.go`:
- Around line 529-536: In the branch that handles the “empty-state / not found”
result (the code path that calls TerraformPluginFrameworkIsStateEmptyFn and
currently clears fwState), also clear the stored framework identity by calling
n.opTracker.SetFrameworkIdentity(nil); specifically, add this call alongside the
existing n.opTracker.SetFrameworkTFState(nil) (or wherever readResponse.NewState
is treated as empty) so the tracker does not retain readResponse.NewIdentity for
resources classified as gone.
In `@pkg/controller/nofork_store.go`:
- Around line 58-59: AsyncTracker currently holds fwIdentity only in memory, so
after a restart or leader handoff fwIdentity becomes nil while fwState is
reconstructed; update the code that persists/restores fwState to also persist
and reconstruct fwIdentity (e.g., serialize ResourceIdentityData when saving
AsyncTracker state and rehydrate fwIdentity during tracker/load path), or add a
rebuild path in the controller that derives fwIdentity from the persisted
fwState before Read/Plan/Apply; locate and modify AsyncTracker, the fwState
persistence/load routines, and the places that call Read/Plan/Apply to ensure
fwIdentity is available after restart.
---
Outside diff comments:
In `@pkg/controller/external_tfpluginfw_test.go`:
- Around line 148-153: The helper constructing the
terraformPluginFrameworkExternalClient mistakenly uses an undefined bare cfg;
update the returned struct to use the per-test configuration passed in via
testConfig (i.e., replace cfg with testConfig.cfg) so the config field is wired
from the testConfig parameter when creating the
terraformPluginFrameworkExternalClient (keep other symbols like ts:
terraform.Setup and mockTPFProvider unchanged).
🪄 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: 0048bdd7-67a2-4d67-88cb-dc0fea832aa5
📒 Files selected for processing (4)
pkg/controller/external_tfpluginfw.gopkg/controller/external_tfpluginfw_test.gopkg/controller/nofork_store.gopkg/controller/nofork_store_test.go
erikmiller-gusto
force-pushed
the
feat/resource-identity-support
branch
3 times, most recently
from
988dd3d to
f50497d
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/controller/external_tfpluginfw.go (1)
853-870:⚠️ Potential issue | 🟡 MinorIdentity propagation in delete operation is correct and well-tested.
The code properly stores
applyResponse.NewIdentity(nil after successful delete) in the tracker, ensuring deleted resources don't retain identity. Unit tests confirm this behavior viaTestTPFDeleteIdentityPropagation.Note: While this file (
pkg/controller/external_tfpluginfw.go) is runtime code rather than a template, the identity propagation touches core resource lifecycle logic. Since the change affects how identity flows through all Terraform execution modes (CLI, SDK v2, Framework), it's worth confirming this has been validated beyond unit tests—especially given the fix targets a real provider issue (provider-upjet-aws#2003).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/external_tfpluginfw.go` around lines 853 - 870, The delete path already records applyResponse.NewIdentity via n.opTracker.SetFrameworkIdentity after calling n.server.ApplyResourceChange with an ApplyResourceChangeRequest, but you must ensure this identity-nil propagation is validated beyond unit tests: confirm the runtime retains the SetFrameworkIdentity call in pkg/controller/external_tfpluginfw.go (and that ApplyResourceChangeRequest/ApplyResourceChange usage is unchanged), and add/execute an integration or e2e test that exercises the delete flow across CLI, SDK v2 and Framework modes (in addition to TestTPFDeleteIdentityPropagation) to verify deleted resources' identities become nil in the opTracker at runtime.
🧹 Nitpick comments (1)
pkg/controller/external_tfpluginfw_test.go (1)
535-552: Helpful test helper!The
newTestIdentityDatafunction creates realistic identity data for tests. Quick question: is this helper also used inpkg/controller/nofork_store_test.goas mentioned in the summary? If so, could this be extracted to a shared test utilities file to avoid duplication?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/external_tfpluginfw_test.go` around lines 535 - 552, The helper newTestIdentityData is duplicated across tests; move it into a shared test utility (e.g., pkg/controller/testutil or pkg/testutils) and update callers (pkg/controller/external_tfpluginfw_test.go and pkg/controller/nofork_store_test.go) to import and call the single shared function; ensure the function signature and imports (tfprotov6, tftypes) are preserved and exported/unexported as needed (e.g., NewTestIdentityData) so both test packages can reference it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/controller/external_tfpluginfw.go`:
- Around line 853-870: The delete path already records applyResponse.NewIdentity
via n.opTracker.SetFrameworkIdentity after calling n.server.ApplyResourceChange
with an ApplyResourceChangeRequest, but you must ensure this identity-nil
propagation is validated beyond unit tests: confirm the runtime retains the
SetFrameworkIdentity call in pkg/controller/external_tfpluginfw.go (and that
ApplyResourceChangeRequest/ApplyResourceChange usage is unchanged), and
add/execute an integration or e2e test that exercises the delete flow across
CLI, SDK v2 and Framework modes (in addition to
TestTPFDeleteIdentityPropagation) to verify deleted resources' identities become
nil in the opTracker at runtime.
---
Nitpick comments:
In `@pkg/controller/external_tfpluginfw_test.go`:
- Around line 535-552: The helper newTestIdentityData is duplicated across
tests; move it into a shared test utility (e.g., pkg/controller/testutil or
pkg/testutils) and update callers (pkg/controller/external_tfpluginfw_test.go
and pkg/controller/nofork_store_test.go) to import and call the single shared
function; ensure the function signature and imports (tfprotov6, tftypes) are
preserved and exported/unexported as needed (e.g., NewTestIdentityData) so both
test packages can reference it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cdc689dc-544b-43f8-be54-9527d3c75033
📒 Files selected for processing (4)
pkg/controller/external_tfpluginfw.gopkg/controller/external_tfpluginfw_test.gopkg/controller/nofork_store.gopkg/controller/nofork_store_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/controller/nofork_store.go
- pkg/controller/nofork_store_test.go
erikmiller-gusto
force-pushed
the
feat/resource-identity-support
branch
from
f50497d to
83c4b55
Compare
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/controller/external_tfpluginfw_test.go`:
- Around line 816-864: Add a second subtest inside
TestTPFPlanIdentityPropagation that simulates a restart rehydration: configure
the testConfiguration so readNewIdentity is the non-nil identity returned by
ReadResourceResponse but the opTracker starts with nil (or ensure
ReadResourceRequest.CurrentIdentity is nil), then call tpfExternal.Observe(...)
and assert the capturedPlanReq (PlanResourceChangeRequest) has PriorIdentity
equal to the refreshed identity from ReadResourceResponse (the test should still
use capturedPlanReq, tpfExternal.plannedIdentity, readNewIdentity,
planPlannedIdentity, and opTracker to locate the behavior under
Observe/ReadResource/PlanResourceChange flows).
🪄 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: 7d4f562d-bb6d-4f72-b442-d30867656769
📒 Files selected for processing (4)
pkg/controller/external_tfpluginfw.gopkg/controller/external_tfpluginfw_test.gopkg/controller/nofork_store.gopkg/controller/nofork_store_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/controller/nofork_store_test.go
- pkg/controller/external_tfpluginfw.go
Pass resource identity data through all protov6 request/response cycles (ReadResource, PlanResourceChange, ApplyResourceChange) so that resources declaring an IdentitySchema in terraform-plugin-framework v1.17.0+ receive the required identity data and no longer fail with "Missing Resource Identity After Read". Fixes crossplane-contrib/provider-upjet-aws#2003 Signed-off-by: Erik Miller <erik.miller@gusto.com>
erikmiller-gusto
force-pushed
the
feat/resource-identity-support
branch
from
83c4b55 to
7d2845c
Compare
|
Have also confirmed that this works and fixed the same issue in the provider-upjet-cloudflare |
When a resource with an IdentitySchema is deleted externally, the terraform-plugin-framework's ReadResource returns a null state. The AWS provider's identity interceptor skips populating identity for null states, but the framework still validates that identity must be non-null after a successful Read - producing the "Missing Resource Identity After Read" error. This change treats that specific diagnostic as a resource-not-found condition, allowing Upjet to correctly detect that the external resource no longer exists and proceed with recreation. Complements the identity propagation from PR crossplane#623.
|
Thanks for the PR — the identity propagation fix is necessary but not sufficient on its own. We tested it in production and hit the same Root CauseThe PR's comment in
This is only true when the resource still exists. When the resource has been deleted externally (or hasn't been created yet):
Additional Fix NeededIn // hasMissingResourceIdentityDiagnostic checks whether the diagnostics contain
// the "Missing Resource Identity After Read" error that terraform-plugin-framework
// returns when a resource with an IdentitySchema has its state set to null
// (resource deleted externally) but the identity interceptor skips populating
// identity for null states. We treat this as a resource-not-found condition.
func hasMissingResourceIdentityDiagnostic(diags []*tfprotov6.Diagnostic) bool {
for _, d := range diags {
if d.Severity == tfprotov6.DiagnosticSeverityError && d.Summary == "Missing Resource Identity After Read" {
return true
}
}
return false
}And in isResourceNotFoundDiags := n.hasResourceNotFoundDiagnostic(readResponse.Diagnostics)
isMissingIdentityDiags := hasMissingResourceIdentityDiagnostic(readResponse.Diagnostics)
if fatalDiags := getFatalDiagnostics(readResponse.Diagnostics); fatalDiags != nil {
if !isResourceNotFoundDiags && !isMissingIdentityDiags {
n.opTracker.ResetReconstructedFrameworkTFState()
return managed.ExternalObservation{}, errors.Wrap(fatalDiags, "read resource request failed")
}
if isResourceNotFoundDiags {
n.logger.Debug("TF ReadResource returned error diagnostics, but XP resource was configured to treat them as `Resource not exists`. Skipping", "skippedDiags", fatalDiags)
}
if isMissingIdentityDiags {
n.logger.Debug("TF ReadResource returned Missing Resource Identity diagnostic, treating as resource not found", "skippedDiags", fatalDiags)
isResourceNotFoundDiags = true
}
}Without this, any Plugin Framework resource with an |
When a resource with an IdentitySchema is deleted externally, the terraform-plugin-framework's ReadResource returns a null state. The AWS provider's identity interceptor skips populating identity for null states, but the framework still validates that identity must be non-null after a successful Read - producing the "Missing Resource Identity After Read" error. This change treats that specific diagnostic as a resource-not-found condition, allowing Upjet to correctly detect that the external resource no longer exists and proceed with recreation. Complements the identity propagation from PR crossplane#623. Signed-off-by: Aditya Menon <amenon@canarytechnologies.com>
When a TF Plugin Framework resource with an IdentitySchema is deleted externally or doesn't yet exist, the provider's identity interceptor skips populating identity for null states, causing terraform-plugin-framework to return a "Missing Resource Identity After Read" error diagnostic. Treat this diagnostic as a resource-not-found condition so that Observe correctly reports ResourceExists: false instead of failing. Signed-off-by: Erik Miller <erik.miller@gusto.com>
erikmiller-gusto
force-pushed
the
feat/resource-identity-support
branch
from
416e927 to
b1d9689
Compare
|
@aditmeno I've updated the PR to address that. Will also run that through my own tests to confirm |
|
@aditmeno was able to confirm the external deletion flow on the aws Bucketlifecycleconfiguration |
2 participants
Description of your changes
Pass resource identity data through all protov6 request/response cycles (ReadResource, PlanResourceChange, ApplyResourceChange) so that resources declaring an IdentitySchema in terraform-plugin-framework v1.17.0+ receive the required identity data and no longer fail with "Missing Resource Identity After Read".
Fixes crossplane-contrib/provider-upjet-aws#2003
I have:
make reviewableto ensure this PR is ready for review.backport release-x.ylabels to auto-backport this PR if necessary.How has this code been tested