fix: nested XRs should not show add/remove under claims

Signed-off-by: Jonathan Ogilvie <[email protected]>

Author: jcogilvie

cmd/diff/diffprocessor/diff_processor.go

@@ -303,7 +303,7 @@ func (p *DefaultDiffProcessor) DiffSingleResource(ctx context.Context, res *un.U
 	// Check for nested XRs in the composed resources and process them recursively
 	p.config.Logger.Debug("Checking for nested XRs", "resource", resourceID, "composedCount", len(desired.ComposedResources))
 
-	nestedDiffs, err := p.ProcessNestedXRs(ctx, desired.ComposedResources, compositionProvider, resourceID, 1)
+	nestedDiffs, err := p.ProcessNestedXRs(ctx, desired.ComposedResources, compositionProvider, resourceID, xr, 1)
 	if err != nil {
 		p.config.Logger.Debug("Error processing nested XRs", "resource", resourceID, "error", err)
 		return nil, errors.Wrap(err, "cannot process nested XRs")
@@ -329,6 +329,7 @@ func (p *DefaultDiffProcessor) ProcessNestedXRs(
 	composedResources []cpd.Unstructured,
 	compositionProvider types.CompositionProvider,
 	parentResourceID string,
+	parentXR *cmp.Unstructured,
 	depth int,
 ) (map[string]*dt.ResourceDiff, error) {
 	if depth > p.config.MaxNestedDepth {
@@ -345,27 +346,88 @@ func (p *DefaultDiffProcessor) ProcessNestedXRs(
 		"composedResourceCount", len(composedResources),
 		"depth", depth)
 
+	// Fetch observed resources from parent XR to find existing nested XRs
+	// This allows us to preserve the identity of nested XRs that already exist in the cluster
+	var observedResources []cpd.Unstructured
+	if parentXR != nil {
+		obs, err := p.diffCalculator.FetchObservedResources(ctx, parentXR)
+		if err != nil {
+			// Log but continue - nested XRs without existing cluster state will show as new (with "(generated)")
+			p.config.Logger.Debug("Could not fetch observed resources for parent XR (continuing)",
+				"parentResource", parentResourceID,
+				"error", err)
+		} else {
+			observedResources = obs
+		}
+	}
+
 	allDiffs := make(map[string]*dt.ResourceDiff)
 
 	for _, composed := range composedResources {
-		un := &un.Unstructured{Object: composed.UnstructuredContent()}
+		nestedXR := &un.Unstructured{Object: composed.UnstructuredContent()}
 
 		// Check if this composed resource is itself an XR
-		isXR, _ := p.getCompositeResourceXRD(ctx, un)
+		isXR, _ := p.getCompositeResourceXRD(ctx, nestedXR)
 
 		if !isXR {
 			// Skip non-XR resources
 			continue
 		}
 
-		nestedResourceID := fmt.Sprintf("%s/%s (nested depth %d)", un.GetKind(), un.GetName(), depth)
+		nestedResourceID := fmt.Sprintf("%s/%s (nested depth %d)", nestedXR.GetKind(), nestedXR.GetName(), depth)
 		p.config.Logger.Debug("Found nested XR, processing recursively",
 			"nestedXR", nestedResourceID,
 			"parentXR", parentResourceID,
 			"depth", depth)
 
+		// Find the matching existing nested XR in observed resources (if it exists)
+		// Match by composition-resource-name annotation to find the correct existing resource
+		var existingNestedXR *un.Unstructured
+		compositionResourceName := nestedXR.GetAnnotations()["crossplane.io/composition-resource-name"]
+		if compositionResourceName != "" {
+			for _, obs := range observedResources {
+				obsUnstructured := &un.Unstructured{Object: obs.UnstructuredContent()}
+				obsCompResName := obsUnstructured.GetAnnotations()["crossplane.io/composition-resource-name"]
+
+				// Match by composition-resource-name annotation and kind
+				if obsCompResName == compositionResourceName && obsUnstructured.GetKind() == nestedXR.GetKind() {
+					existingNestedXR = obsUnstructured
+					p.config.Logger.Debug("Found existing nested XR in cluster",
+						"nestedXR", nestedResourceID,
+						"existingName", existingNestedXR.GetName(),
+						"compositionResourceName", compositionResourceName)
+					break
+				}
+			}
+		}
+
+		// If we found an existing nested XR, preserve its identity (name, composite label)
+		// This ensures its managed resources can be matched correctly
+		if existingNestedXR != nil {
+			// Preserve the actual cluster name
+			nestedXR.SetName(existingNestedXR.GetName())
+			nestedXR.SetGenerateName(existingNestedXR.GetGenerateName())
+
+			// Preserve the composite label so child resources get matched correctly
+			if labels := existingNestedXR.GetLabels(); labels != nil {
+				if compositeLabel, exists := labels["crossplane.io/composite"]; exists {
+					nestedXRLabels := nestedXR.GetLabels()
+					if nestedXRLabels == nil {
+						nestedXRLabels = make(map[string]string)
+					}
+					nestedXRLabels["crossplane.io/composite"] = compositeLabel
+					nestedXR.SetLabels(nestedXRLabels)
+
+					p.config.Logger.Debug("Preserved nested XR identity",
+						"nestedXR", nestedResourceID,
+						"preservedName", nestedXR.GetName(),
+						"preservedCompositeLabel", compositeLabel)
+				}
+			}
+		}
+
 		// Recursively process this nested XR
-		nestedDiffs, err := p.DiffSingleResource(ctx, un, compositionProvider)
+		nestedDiffs, err := p.DiffSingleResource(ctx, nestedXR, compositionProvider)
 		if err != nil {
 			// Check if the error is due to missing composition
 			// Note: It's valid to have an XRD in Crossplane without a composition attached to it.
@@ -376,7 +438,7 @@ func (p *DefaultDiffProcessor) ProcessNestedXRs(
 				p.config.Logger.Info("Skipping nested XR processing due to missing composition",
 					"nestedXR", nestedResourceID,
 					"parentXR", parentResourceID,
-					"gvk", un.GroupVersionKind().String())
+					"gvk", nestedXR.GroupVersionKind().String())
 				// Continue processing other nested XRs
 				continue
 			}

cmd/diff/diffprocessor/diff_processor_test.go

@@ -1685,6 +1685,115 @@ func TestDefaultDiffProcessor_ProcessNestedXRs(t *testing.T) {
 			wantDiffCount:    1, // Only the child XR should be processed
 			wantErr:          false,
 		},
+		"NestedXRWithExistingResourcesPreservesIdentity": {
+			setupMocks: func() (xp.Clients, k8.Clients) {
+				// This test reproduces the bug where existing nested XR identity is not preserved
+				// resulting in all managed resources showing as removed/added instead of modified
+
+				// Create an EXISTING nested XR with actual cluster name (not generateName)
+				existingChildXR := tu.NewResource("nested.example.org/v1alpha1", "XChildResource", "parent-xr-child-abc123").
+					WithGenerateName("parent-xr-").
+					WithSpecField("childField", "existing-value").
+					WithCompositionResourceName("child-xr").
+					WithLabels(map[string]string{
+						"crossplane.io/composite": "parent-xr-abc",  // Existing composite label
+					}).
+					Build()
+
+				// Create an existing managed resource owned by the nested XR
+				existingManagedResource := tu.NewResource("nop.example.org/v1alpha1", "NopResource", "parent-xr-child-abc123-managed-xyz").
+					WithGenerateName("parent-xr-child-abc123-").
+					WithSpecField("forProvider", map[string]any{
+						"configData": "existing-data",
+					}).
+					WithCompositionResourceName("managed-resource").
+					WithLabels(map[string]string{
+						"crossplane.io/composite": "parent-xr-child-abc123",  // Points to existing nested XR
+					}).
+					Build()
+
+				// Create a parent XR that owns the nested XR
+				parentXR := tu.NewResource("parent.example.org/v1alpha1", "XParentResource", "parent-xr-abc").
+					WithGenerateName("parent-xr-").
+					Build()
+
+				// Create functions
+				functions := []pkgv1.Function{
+					{
+						ObjectMeta: metav1.ObjectMeta{
+							Name: "function-go-templating",
+						},
+						Spec: pkgv1.FunctionSpec{
+							PackageSpec: pkgv1.PackageSpec{
+								Package: "xpkg.crossplane.io/crossplane-contrib/function-go-templating:v0.11.0",
+							},
+						},
+					},
+				}
+
+				xpClients := xp.Clients{
+					Definition: tu.NewMockDefinitionClient().
+						WithXRD(childXRD).
+						Build(),
+					Composition: tu.NewMockCompositionClient().
+						WithComposition(childComposition).
+						Build(),
+					Function: tu.NewMockFunctionClient().
+						WithSuccessfulFunctionsFetch(functions).
+						Build(),
+					Environment: tu.NewMockEnvironmentClient().
+						WithNoEnvironmentConfigs().
+						Build(),
+					// Mock resource tree to return existing nested XR and its managed resources
+					ResourceTree: tu.NewMockResourceTreeClient().
+						WithResourceTreeFromXRAndComposed(
+							parentXR,
+							[]*un.Unstructured{existingChildXR, existingManagedResource},
+						).
+						Build(),
+				}
+
+				// Create CRDs
+				childCRD := tu.NewCRD("xchildresources.nested.example.org", "nested.example.org", "XChildResource").
+					WithListKind("XChildResourceList").
+					WithPlural("xchildresources").
+					WithSingular("xchildresource").
+					WithVersion("v1alpha1", true, true).
+					WithStandardSchema("childField").
+					Build()
+
+				nopCRD := tu.NewCRD("nopresources.nop.example.org", "nop.example.org", "NopResource").
+					WithListKind("NopResourceList").
+					WithPlural("nopresources").
+					WithSingular("nopresource").
+					WithVersion("v1alpha1", true, true).
+					WithStandardSchema("configData").
+					Build()
+
+				k8sClients := k8.Clients{
+					Apply:    tu.NewMockApplyClient().Build(),
+					Resource: tu.NewMockResourceClient().Build(),
+					Schema: tu.NewMockSchemaClient().
+						WithFoundCRD("nested.example.org", "XChildResource", childCRD).
+						WithFoundCRD("nop.example.org", "NopResource", nopCRD).
+						WithSuccessfulCRDByNameFetch("xchildresources.nested.example.org", childCRD).
+						Build(),
+					Type: tu.NewMockTypeConverter().Build(),
+				}
+
+				return xpClients, k8sClients
+			},
+			composedResources: []cpd.Unstructured{
+				// The RENDERED nested XR (from parent composition) with generateName but no name
+				{Unstructured: *childXR},
+			},
+			parentResourceID: "XParentResource/parent-xr-abc",
+			depth:            1,
+			// TODO: This will currently fail because the nested XR gets "(generated)" name
+			// After fix, should NOT show managed resources as removed/added
+			wantDiffCount: 1, // Just the nested XR diff, not its managed resources as separate remove/add
+			wantErr:       false,
+		},
 	}
 
 	for name, tt := range tests {
@@ -1730,8 +1839,11 @@ func TestDefaultDiffProcessor_ProcessNestedXRs(t *testing.T) {
 					return xpClients.Composition.FindMatchingComposition(ctx, res)
 				}
 
+				// Create a mock parent XR (nil is acceptable for tests that don't need observed resources)
+				var parentXR *cmp.Unstructured
+
 				// Call the method under test
-				diffs, err := processor.ProcessNestedXRs(ctx, tt.composedResources, compositionProvider, tt.parentResourceID, tt.depth)
+				diffs, err := processor.ProcessNestedXRs(ctx, tt.composedResources, compositionProvider, tt.parentResourceID, parentXR, tt.depth)
 
 				// Check error
 				if (err != nil) != tt.wantErr {