Fix claim label tracking, name and generateName preservation (#55)

* Fix claim label tracking, name and generateName preservation

Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>

* Remove unnecessary test comment

Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>

* Fix multiple ownerrefs problem with claims

Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>

* Add claim e2e

Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>

* Add note for potential inconsistency in resource matching logic

Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>

---------

Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>

Author: jcogilvie

CLAUDE.md

@@ -1,4 +1,5 @@
 - We should never silently continue a diff in the face of a failure, and we should strive not to make best-guesses. 
 - The diff command should strive for accuracy above all else, so in cases where errors or guesses may compromise accuracy, we should fail the diff completely.
 - We should not emit partial results for a given XR; if given multiple XRs, it's okay to emit results only for those that pass so long as we call attention to the others that failed.
-- We should always emit useful logging with appropriate contextual objects attached.
+- We should always emit useful logging with appropriate contextual objects attached.
+- the earthly `reviewable` command should always be run with a long timeout.  it runs all of our ITs and they can take several minutes.

cmd/diff/diff_integration_test.go

@@ -1222,14 +1222,12 @@ Summary: 2 added`,
   metadata:
     annotations:
       crossplane.io/composition-resource-name: nop-resource
--   generateName: test-claim-82crv-
+    generateName: test-claim-82crv-
     labels:
       crossplane.io/claim-name: test-claim
       crossplane.io/claim-namespace: existing-namespace
--     crossplane.io/composite: test-claim-82crv
--   name: test-claim-82crv
-+     crossplane.io/composite: test-claim
-+   name: test-claim
+      crossplane.io/composite: test-claim-82crv
+    name: test-claim-82crv
   spec:
     forProvider:
 -     configData: existing-value

cmd/diff/diffprocessor/diff_calculator.go

@@ -93,23 +93,14 @@ func (c *DefaultDiffCalculator) CalculateDiff(ctx context.Context, composite *un
 			"resource", current)
 	}
 
-	// Update owner references if needed
-	c.resourceManager.UpdateOwnerRefs(composite, desired)
-
-	// If this is an existing resource but our desired object uses generateName,
-	// set the name from the current resource for the dry-run apply
-	if current != nil && name == "" && generateName != "" {
-		currentName := current.GetName()
-		c.logger.Debug("Using existing resource name for desired object with generateName",
-			"resource", resourceID,
-			"currentName", currentName)
+	// Preserve existing resource identity for resources with generateName
+	desired = c.preserveExistingResourceIdentity(current, desired, resourceID, name)
 
-		// Create a copy of the desired object and set the name from the current resource
-		// This is needed for server-side apply which requires a name
-		desiredCopy := desired.DeepCopy()
-		desiredCopy.SetName(currentName)
-		desired = desiredCopy
-	}
+	// Update owner references if needed (done after preserving existing labels)
+	// IMPORTANT: For composed resources, the owner should be the XR, not a Claim.
+	// When composite is the current XR from the cluster, we use it as the owner.
+	// This ensures composed resources only have the XR as their controller owner.
+	c.resourceManager.UpdateOwnerRefs(ctx, composite, desired)
 
 	// Determine what the resource would look like after application
 	wouldBeResult := desired
@@ -306,3 +297,50 @@ func (c *DefaultDiffCalculator) CalculateRemovedResourceDiffs(ctx context.Contex
 
 	return removedDiffs, nil
 }
+
+// preserveExistingResourceIdentity preserves the identity (name, generateName, labels) from an existing
+// resource with generateName to ensure dry-run apply works on the correct resource identity.
+// This is critical for claim scenarios where the rendered name differs from the generated name.
+func (c *DefaultDiffCalculator) preserveExistingResourceIdentity(current, desired *un.Unstructured, resourceID, renderedName string) *un.Unstructured {
+	// Only preserve identity for existing resources with both generateName and name
+	if current == nil || current.GetGenerateName() == "" || current.GetName() == "" {
+		return desired
+	}
+
+	currentName := current.GetName()
+	currentGenerateName := current.GetGenerateName()
+
+	c.logger.Debug("Using existing resource identity for dry-run apply",
+		"resource", resourceID,
+		"renderedName", renderedName,
+		"currentName", currentName,
+		"currentGenerateName", currentGenerateName)
+
+	// Create a copy of the desired object and set the identity from the current resource
+	// This ensures dry-run apply works on the correct resource identity
+	desiredCopy := desired.DeepCopy()
+	desiredCopy.SetName(currentName)
+	desiredCopy.SetGenerateName(currentGenerateName)
+
+	// Preserve important labels from the existing resource, particularly the composite label
+	// This ensures the dry-run apply gets the right labels and preserves them correctly
+	currentLabels := current.GetLabels()
+	if currentLabels != nil {
+		desiredLabels := desiredCopy.GetLabels()
+		if desiredLabels == nil {
+			desiredLabels = make(map[string]string)
+		}
+
+		// Preserve the composite label if it exists (critical for claims)
+		if compositeLabel, exists := currentLabels["crossplane.io/composite"]; exists {
+			desiredLabels["crossplane.io/composite"] = compositeLabel
+			c.logger.Debug("Preserved composite label for dry-run apply",
+				"resource", resourceID,
+				"compositeLabel", compositeLabel)
+		}
+
+		desiredCopy.SetLabels(desiredLabels)
+	}
+
+	return desiredCopy
+}

cmd/diff/diffprocessor/resource_manager.go

@@ -12,6 +12,7 @@ import (
 	un "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/util/uuid"
+	"k8s.io/utils/ptr"
 
 	"github.com/crossplane/crossplane-runtime/v2/pkg/errors"
 	"github.com/crossplane/crossplane-runtime/v2/pkg/logging"
@@ -24,7 +25,7 @@ type ResourceManager interface {
 	FetchCurrentObject(ctx context.Context, composite *un.Unstructured, desired *un.Unstructured) (*un.Unstructured, bool, error)
 
 	// UpdateOwnerRefs ensures all OwnerReferences have valid UIDs
-	UpdateOwnerRefs(parent *un.Unstructured, child *un.Unstructured)
+	UpdateOwnerRefs(ctx context.Context, parent *un.Unstructured, child *un.Unstructured)
 }
 
 // DefaultResourceManager implements ResourceManager interface.
@@ -289,6 +290,9 @@ func (m *DefaultResourceManager) findMatchingResource(
 			continue
 		}
 
+		// TODO:  is this logic correct?  we should definitely match composition-resource-name, even if the generateName
+		// doesn't match.  maybe as a fallback if we don't have the annotation?
+
 		// If we have a generateName, verify the match has the right prefix
 		if generateName != "" {
 			resName := res.GetName()
@@ -333,17 +337,79 @@ func (m *DefaultResourceManager) hasMatchingResourceName(annotations map[string]
 }
 
 // UpdateOwnerRefs ensures all OwnerReferences have valid UIDs.
-func (m *DefaultResourceManager) UpdateOwnerRefs(parent *un.Unstructured, child *un.Unstructured) {
+// It handles Claims and XRs differently according to Crossplane's ownership model:
+// - Claims should never be controller owners of composed resources.
+// - XRs should be controller owners and use their UID for matching references.
+func (m *DefaultResourceManager) UpdateOwnerRefs(ctx context.Context, parent *un.Unstructured, child *un.Unstructured) {
 	// if there's no parent, we are the parent.
 	if parent == nil {
 		m.logger.Debug("No parent provided for owner references update")
 		return
 	}
 
-	uid := parent.GetUID()
+	// Check if the parent is a claim and dispatch to appropriate handler
+	isParentAClaim := m.defClient.IsClaimResource(ctx, parent)
+
+	switch {
+	case isParentAClaim:
+		m.updateOwnerRefsForClaim(parent, child)
+	default:
+		m.updateOwnerRefsForXR(parent, child)
+	}
+
+	// Update composite owner label (common for both Claims and XRs)
+	m.updateCompositeOwnerLabel(ctx, parent, child)
+}
+
+// updateOwnerRefsForClaim handles owner reference updates when the parent is a Claim.
+// Claims should not be controller owners of composed resources per Crossplane's model:
+// Claim -> owns -> XR -> owns -> Composed Resources.
+func (m *DefaultResourceManager) updateOwnerRefsForClaim(claim *un.Unstructured, child *un.Unstructured) {
+	m.logger.Debug("Processing owner references with claim parent",
+		"parentKind", claim.GetKind(),
+		"parentName", claim.GetName(),
+		"childKind", child.GetKind(),
+		"childName", child.GetName())
+
+	// Get the current owner references
+	refs := child.GetOwnerReferences()
+	updatedRefs := make([]metav1.OwnerReference, 0, len(refs))
+
+	for _, ref := range refs {
+		// Generate UIDs for any owner references that don't have them
+		// For Claims, we never use the Claim's UID even for matching refs
+		if ref.UID == "" {
+			ref.UID = uuid.NewUUID()
+			m.logger.Debug("Generated UID for owner reference",
+				"refKind", ref.Kind,
+				"refName", ref.Name,
+				"newUID", ref.UID)
+		}
+
+		// Ensure Claims are never controller owners
+		if ref.Kind == claim.GetKind() && ref.Name == claim.GetName() {
+			if ref.Controller != nil && *ref.Controller {
+				ref.Controller = ptr.To(false)
+				m.logger.Debug("Set Controller to false for claim owner reference",
+					"refKind", ref.Kind,
+					"refName", ref.Name)
+			}
+		}
+
+		updatedRefs = append(updatedRefs, ref)
+	}
+
+	child.SetOwnerReferences(updatedRefs)
+}
+
+// updateOwnerRefsForXR handles owner reference updates when the parent is an XR.
+// XRs should be controller owners of their composed resources and use their UID
+// for matching owner references.
+func (m *DefaultResourceManager) updateOwnerRefsForXR(xr *un.Unstructured, child *un.Unstructured) {
+	uid := xr.GetUID()
 	m.logger.Debug("Updating owner references",
-		"parentKind", parent.GetKind(),
-		"parentName", parent.GetName(),
+		"parentKind", xr.GetKind(),
+		"parentName", xr.GetName(),
 		"parentUID", uid,
 		"childKind", child.GetKind(),
 		"childName", child.GetName())
@@ -359,11 +425,11 @@ func (m *DefaultResourceManager) UpdateOwnerRefs(parent *un.Unstructured, child
 	for _, ref := range refs {
 		originalUID := ref.UID
 
-		// if there is an owner ref on the dependent that we are pretty sure comes from us,
-		// point the UID to the parent.
-		if ref.Name == parent.GetName() &&
-			ref.APIVersion == parent.GetAPIVersion() &&
-			ref.Kind == parent.GetKind() &&
+		// If there is an owner ref on the dependent that matches the parent XR,
+		// use the parent's UID
+		if ref.Name == xr.GetName() &&
+			ref.APIVersion == xr.GetAPIVersion() &&
+			ref.Kind == xr.GetKind() &&
 			ref.UID == "" {
 			ref.UID = uid
 			m.logger.Debug("Updated matching owner reference with parent UID",
@@ -372,7 +438,7 @@ func (m *DefaultResourceManager) UpdateOwnerRefs(parent *un.Unstructured, child
 				"newUID", ref.UID)
 		}
 
-		// if we have a non-matching owner ref don't use the parent UID.
+		// For non-matching owner refs, generate a random UID
 		if ref.UID == "" {
 			ref.UID = uuid.NewUUID()
 			m.logger.Debug("Generated new random UID for owner reference",
@@ -387,17 +453,14 @@ func (m *DefaultResourceManager) UpdateOwnerRefs(parent *un.Unstructured, child
 	// Update the object with the modified owner references
 	child.SetOwnerReferences(updatedRefs)
 
-	// Update composite owner label
-	m.updateCompositeOwnerLabel(parent, child)
-
 	m.logger.Debug("Updated owner references and labels",
 		"newCount", len(updatedRefs))
 }
 
 // updateCompositeOwnerLabel updates the ownership labels on the child resource.
 // For Claims, it sets claim-name and claim-namespace labels.
 // For XRs, it sets the composite label.
-func (m *DefaultResourceManager) updateCompositeOwnerLabel(parent, child *un.Unstructured) {
+func (m *DefaultResourceManager) updateCompositeOwnerLabel(ctx context.Context, parent, child *un.Unstructured) {
 	if parent == nil {
 		return
 	}
@@ -419,18 +482,32 @@ func (m *DefaultResourceManager) updateCompositeOwnerLabel(parent, child *un.Uns
 	}
 
 	// Check if the parent is a claim
-	ctx := context.Background()
 	isParentAClaim := m.defClient.IsClaimResource(ctx, parent)
 
 	switch {
 	case isParentAClaim:
 		// For claims, set claim-specific labels (all claims are namespaced)
 		labels["crossplane.io/claim-name"] = parentName
 		labels["crossplane.io/claim-namespace"] = parent.GetNamespace()
-		m.logger.Debug("Updated claim owner labels",
-			"claimName", parentName,
-			"claimNamespace", parent.GetNamespace(),
-			"child", child.GetName())
+
+		// For claims, only set the composite label if it doesn't already exist
+		// If it exists, it likely points to a generated XR name which we should preserve
+		existingComposite, hasComposite := labels["crossplane.io/composite"]
+		if !hasComposite {
+			// No existing composite label, set it to the claim name
+			labels["crossplane.io/composite"] = parentName
+			m.logger.Debug("Set composite label for new claim resource",
+				"claimName", parentName,
+				"claimNamespace", parent.GetNamespace(),
+				"child", child.GetName())
+		} else {
+			// Preserve existing composite label (likely a generated XR name)
+			m.logger.Debug("Preserved existing composite label for claim resource",
+				"claimName", parentName,
+				"claimNamespace", parent.GetNamespace(),
+				"existingComposite", existingComposite,
+				"child", child.GetName())
+		}
 	default:
 		// For XRs, set the composite label
 		labels["crossplane.io/composite"] = parentName