fw-external-client: fix TF unexpected identity change errors at Observe#698
Open
erhancagirici wants to merge 3 commits into
Open
fw-external-client: fix TF unexpected identity change errors at Observe#698erhancagirici wants to merge 3 commits into
erhancagirici wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughFramework identity handling now rejects placeholder identifiers, clears identity on not-found observations and failed creates, and resets reconstructed identity alongside reconstructed state. Tests cover each updated path. ChangesFramework identity handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Observe
participant ApplyCreate
participant TerraformPluginFramework
participant AsyncTracker
Observe->>TerraformPluginFramework: observe resource
TerraformPluginFramework-->>Observe: not found with identity
Observe->>AsyncTracker: clear framework identity
ApplyCreate->>TerraformPluginFramework: apply resource change
TerraformPluginFramework-->>ApplyCreate: diagnostics and partial state
ApplyCreate->>AsyncTracker: store partial state without identity
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
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.
🧹 Nitpick comments (2)
pkg/controller/external_tfpluginfw.go (2)
759-767: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider polishing the comment text.
Similarly, this is an excellent explanation of the diag-error identity behavior. Just catching a tiny typo ("an superfluous") for you so we can keep the docs as clean as the code.
💬 Proposed fix for the comment text
// note: do not store returned TF Framework Identity here. // https://github.com/crossplane-contrib/provider-upjet-aws/issues/2135 // The returned TF identity might be partial/garbage (missing some fields). - // When subsequent observe recovers the resource via state, it detects an + // When subsequent observe recovers the resource via state, it detects a // superfluous identity change garbage->valid and errors out. // // Instead, ignore the returned TF identity and let subsequent // reconciliations rehydrate TF identity via state🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/controller/external_tfpluginfw.go` around lines 759 - 767, Polish the explanatory comment near the external Terraform identity handling by correcting the grammar from “an superfluous identity change” to “a superfluous identity change,” while preserving the existing explanation and behavior.
621-634: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider polishing the comment text.
Thank you for adding such detailed comments explaining the 'why' behind dropping the identity here! These explanations will be super helpful for future maintainers trying to understand the component's edge cases.
While reading through, I noticed a couple of minor grammatical typos. I've proposed a small tweak below to polish them up, if you'd like to include it!
💬 Proposed fixes for the comment text
- // For FW resources that uses stub/placeholder identifiers + // For FW resources that use stub/placeholder identifiers // in their external name, the initial Observe returns a // "not-found" with nil TF state as expected, however they might // return a non-empty TF identity with the placeholder identifier. // Discard the TF identity for non-existent resource observations // otherwise subsequent reconciles detect a superfluous TF identity // change, failing the Observe. // // This also clears the TF identity for other not-found cases // like resource was fully established but was deleted out-of-band. - // This is fine as Upjet in fact does not rely on the TF resource - // identities and rehydrate it in subsequent reconcile + // This is fine as Upjet in fact does not rely on TF resource + // identities and rehydrates them in subsequent reconciles n.opTracker.SetFrameworkIdentity(nil)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/controller/external_tfpluginfw.go` around lines 621 - 634, Polish the explanatory comments immediately above opTracker.SetFrameworkIdentity(nil) by correcting the grammatical typos in the existing wording, including subject–verb agreement and phrasing around placeholder identifiers. Preserve the detailed explanation and behavior of the supportsIdentity branch unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/controller/external_tfpluginfw.go`:
- Around line 759-767: Polish the explanatory comment near the external
Terraform identity handling by correcting the grammar from “an superfluous
identity change” to “a superfluous identity change,” while preserving the
existing explanation and behavior.
- Around line 621-634: Polish the explanatory comments immediately above
opTracker.SetFrameworkIdentity(nil) by correcting the grammatical typos in the
existing wording, including subject–verb agreement and phrasing around
placeholder identifiers. Preserve the detailed explanation and behavior of the
supportsIdentity branch unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a34b2a8e-3e1d-4801-b0ab-97c0098dc92a
📒 Files selected for processing (6)
pkg/config/externalname.gopkg/config/externalname_test.gopkg/controller/external_tfpluginfw.gopkg/controller/external_tfpluginfw_test.gopkg/controller/nofork_store.gopkg/controller/nofork_store_test.go
erhancagirici
force-pushed
the
fix-unexpected-identity-change
branch
2 times, most recently
from
July 17, 2026 09:37
a092922 to
fe66a71
Compare
…bserve Signed-off-by: Erhan Cagirici <erhan@upbound.io>
… returns the placeholder as external name Signed-off-by: Erhan Cagirici <erhan@upbound.io>
Signed-off-by: Erhan Cagirici <erhan@upbound.io>
erhancagirici
force-pushed
the
fix-unexpected-identity-change
branch
from
July 17, 2026 09:39
fe66a71 to
2c9957e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/config/externalname_test.go (1)
346-347: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider using
cmpopts.EquateErrors()for error comparison.Thanks for putting together these comprehensive test cases! They beautifully cover the different identifier states. I noticed we are using
test.EquateErrors()here. Our path instructions specify usingcmpopts.EquateErrors()alongsidecmp.Difffor error testing in**/*_test.gofiles.Would it be possible to switch to
cmpopts.EquateErrors()? I'd love to understand if there's a specific reasontest.EquateErrors()was chosen over it here.♻️ Proposed refactor
- if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want.err, err, cmpopts.EquateErrors()); diff != "" {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/config/externalname_test.go` around lines 346 - 347, Update the error comparison in FrameworkResourceWithComputedIdentifier.GetExternalNameFn to use cmpopts.EquateErrors() with cmp.Diff instead of test.EquateErrors(), preserving the existing want-versus-got comparison and failure reporting.Source: Path instructions
pkg/controller/external_tfpluginfw_test.go (1)
638-675: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider adopting a table-driven structure for these test cases.
Thank you for adding these critical regression tests! I noticed they are written in a sequential, imperative style using individual
t.Runblocks. Our path instructions for**/*_test.gofiles strongly emphasize enforcing a table-driven test structure with anargs/wantpattern and areasonfield.Could we refactor these tests to follow the table-driven pattern? I'd love to hear your thoughts on whether this makes sense for this PR, given it might require restructuring the parent tests as well!
pkg/controller/external_tfpluginfw_test.go#L638-L675: refactor theObserveClearsIdentityWhenResourceNotFoundtest to use a table-driven structure.pkg/controller/external_tfpluginfw_test.go#L723-L769: refactor theCreateDoesNotStoreIdentityOnDiagErrortest to use a table-driven structure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/controller/external_tfpluginfw_test.go` around lines 638 - 675, Refactor the test cases at pkg/controller/external_tfpluginfw_test.go lines 638-675 and 723-769 into table-driven tests using args, want, and reason fields. Preserve the existing assertions and regression scenarios for ObserveClearsIdentityWhenResourceNotFound and CreateDoesNotStoreIdentityOnDiagError, executing each case through subtests driven by the table.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/config/externalname_test.go`:
- Around line 346-347: Update the error comparison in
FrameworkResourceWithComputedIdentifier.GetExternalNameFn to use
cmpopts.EquateErrors() with cmp.Diff instead of test.EquateErrors(), preserving
the existing want-versus-got comparison and failure reporting.
In `@pkg/controller/external_tfpluginfw_test.go`:
- Around line 638-675: Refactor the test cases at
pkg/controller/external_tfpluginfw_test.go lines 638-675 and 723-769 into
table-driven tests using args, want, and reason fields. Preserve the existing
assertions and regression scenarios for
ObserveClearsIdentityWhenResourceNotFound and
CreateDoesNotStoreIdentityOnDiagError, executing each case through subtests
driven by the table.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: de623b99-535b-4f9d-9afb-234a8aa21cdc
📒 Files selected for processing (6)
pkg/config/externalname.gopkg/config/externalname_test.gopkg/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 (4)
- pkg/controller/nofork_store.go
- pkg/controller/nofork_store_test.go
- pkg/config/externalname.go
- pkg/controller/external_tfpluginfw.go
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
1 participant
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.
Description of your changes
Fixes spurious "unexpected identity change" errors raised by
TF provider during
Observefor Plugin Framework resources that supportresource identity
Root cause
During
Observecalls, TF Plugin Framework resource identity is fed to TFReadResourceRPC (asCurrentIdentity) and TFReadResourceResponsereturns theNewIdentityWhen the current identity does not match the returned NewIdentity, TF
ReadResourceRPC fails the read with an "unexpected identity change" diagnostics.Observeoperations to fail due to a spurious "identity change", even if the newly returned identity is valid (i.e invalid prior identity -> valid identity transition)ObservestepA provider restart fixes it by rehydrating the cache, but still inconvenient.
An example reported issue is crossplane-contrib/provider-upjet-aws#2135
This only affects the following resources in following particular conditions:
Fix
Reset or do not store potentially invalid TF resource identities.
CreateafterApplyResourceRPC calls with fatal diagnostics: Treat the returned identity as invalid/partial. Still preserves the returned (partial) state, so that is still available for recovery, rehydrationObserveresource not found case: this is to cover the case forFW resources with
FrameworkResourceWithComputedIdentifier, that uses a stub/placeholder identifier for the very-firstObserveto trigger an initial not-found. This expectedly returns a nil state, but can return a non-empty FW Identity with placeholder value. We don't want to store this, otherwise when the resource is actually created, this will cause an identity change fromplaceholder -> realidentity.ResetReconstructedState: This now also clear the associated identity of a reconstructed state.FrameworkResourceWithComputedIdentifier: This is a complementary change. Do not return the placeholder value as a valid external-name atGetExternalNameFn. Placeholder identifiers can only exist at "input states" of TF Observe, should never exist in resulting TF states (after Read or Apply ), whichGetExternalNameFnis only called for in FW resources. Instead error out if found.Addresses crossplane-contrib/provider-upjet-aws#2135
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