Fix: decomposed usage should be deleted

Signed-off-by: Hasan Turken <[email protected]>

Author: turkenh

internal/controller/apiextensions/composite/composition_functions.go

@@ -47,6 +47,7 @@ import (
 	fnv1 "github.com/crossplane/crossplane/apis/apiextensions/fn/proto/v1"
 	v1 "github.com/crossplane/crossplane/apis/apiextensions/v1"
 	"github.com/crossplane/crossplane/internal/names"
+	"github.com/crossplane/crossplane/internal/xcrd"
 )
 
 // Error strings.
@@ -73,6 +74,7 @@ const (
 	errFmtGetCredentialsFromSecret   = "cannot get Composition pipeline step %q credential %q from Secret"
 	errFmtRunPipelineStep            = "cannot run Composition pipeline step %q"
 	errFmtControllerMismatch         = "refusing to delete composed resource %q that is controlled by %s %q"
+	errFmtCleanupLabelsCD            = "cannot cleanup composed resource labels of resource %q (a %s named %s)"
 	errFmtDeleteCD                   = "cannot delete composed resource %q (a %s named %s)"
 	errFmtUnmarshalDesiredCD         = "cannot unmarshal desired composed resource %q from RunFunctionResponse"
 	errFmtCDAsStruct                 = "cannot encode composed resource %q to protocol buffer Struct well-known type"
@@ -751,6 +753,14 @@ func (d *DeletingComposedResourceGarbageCollector) GarbageCollectComposedResourc
 			return errors.Errorf(errFmtControllerMismatch, name, c.Kind, c.Name)
 		}
 
+		// Remove the labels that indicate this resource was owned by a
+		// Composition. This helps differentiate whether a resource was deleted
+		// due to garbage collection or because its owning composite was deleted.
+		meta.RemoveLabels(cd.Resource, xcrd.LabelKeyNamePrefixForComposed, xcrd.LabelKeyClaimName, xcrd.LabelKeyClaimNamespace)
+		if err := d.client.Update(ctx, cd.Resource); resource.IgnoreNotFound(err) != nil {
+			return errors.Wrapf(err, errFmtCleanupLabelsCD, name, cd.Resource.GetObjectKind().GroupVersionKind().Kind, cd.Resource.GetName())
+		}
+		// Delete the composed resource.
 		if err := d.client.Delete(ctx, cd.Resource); resource.IgnoreNotFound(err) != nil {
 			return errors.Wrapf(err, errFmtDeleteCD, name, cd.Resource.GetObjectKind().GroupVersionKind().Kind, cd.Resource.GetName())
 		}

internal/controller/apiextensions/composite/composition_functions_test.go

@@ -1253,10 +1253,42 @@ func TestGarbageCollectComposedResources(t *testing.T) {
 				err: errors.New(`refusing to delete composed resource "undesired-resource" that is controlled by XR "different"`),
 			},
 		},
+		"UpdateError": {
+			reason: "We should return any error encountered updating the resource with removed labels.",
+			params: params{
+				client: &test.MockClient{
+					MockUpdate: test.NewMockUpdateFn(errBoom),
+				},
+			},
+			args: args{
+				owner: &fake.Composite{
+					ObjectMeta: metav1.ObjectMeta{
+						UID: "cool-xr",
+					},
+				},
+				observed: ComposedResourceStates{
+					"undesired-resource": ComposedResourceState{
+						Resource: &fake.Composed{
+							ObjectMeta: metav1.ObjectMeta{
+								// This resource is controlled by the XR.
+								OwnerReferences: []metav1.OwnerReference{{
+									Controller: ptr.To(true),
+									UID:        "cool-xr",
+								}},
+							},
+						},
+					},
+				},
+			},
+			want: want{
+				err: errors.Wrapf(errBoom, errFmtCleanupLabelsCD, "undesired-resource", "", ""),
+			},
+		},
 		"DeleteError": {
 			reason: "We should return any error encountered deleting the resource.",
 			params: params{
 				client: &test.MockClient{
+					MockUpdate: test.NewMockUpdateFn(nil),
 					MockDelete: test.NewMockDeleteFn(errBoom),
 				},
 			},
@@ -1288,6 +1320,13 @@ func TestGarbageCollectComposedResources(t *testing.T) {
 			reason: "We should successfully delete an observed resource from the API server if it is not desired.",
 			params: params{
 				client: &test.MockClient{
+					MockUpdate: func(_ context.Context, obj client.Object, _ ...client.UpdateOption) error {
+						l := obj.GetLabels()
+						if l[xcrd.CategoryComposite] != "" || l[xcrd.LabelKeyClaimName] != "" || l[xcrd.LabelKeyClaimNamespace] != "" {
+							return errors.New("resource still has composed resource labels")
+						}
+						return nil
+					},
 					MockDelete: test.NewMockDeleteFn(nil),
 				},
 			},
@@ -1306,6 +1345,12 @@ func TestGarbageCollectComposedResources(t *testing.T) {
 									Controller: ptr.To(true),
 									UID:        "cool-xr",
 								}},
+								// With composed resource labels.
+								Labels: map[string]string{
+									xcrd.LabelKeyNamePrefixForComposed: "cool-xr",
+									xcrd.LabelKeyClaimName:             "cool-claim",
+									xcrd.LabelKeyClaimNamespace:        "cool-namespace",
+								},
 							},
 						},
 					},

internal/controller/apiextensions/composite/composition_pt.go

@@ -38,14 +38,16 @@ import (
 	v1 "github.com/crossplane/crossplane/apis/apiextensions/v1"
 	"github.com/crossplane/crossplane/internal/controller/apiextensions/usage"
 	"github.com/crossplane/crossplane/internal/names"
+	"github.com/crossplane/crossplane/internal/xcrd"
 )
 
 // Error strings.
 const (
-	errGetComposed  = "cannot get composed resource"
-	errGCComposed   = "cannot garbage collect composed resource"
-	errFetchDetails = "cannot fetch connection details"
-	errInline       = "cannot inline Composition patch sets"
+	errGetComposed     = "cannot get composed resource"
+	errGCCleanupLabels = "cannot cleanup composed resource labels before garbage collection"
+	errGCComposed      = "cannot garbage collect composed resource"
+	errFetchDetails    = "cannot fetch connection details"
+	errInline          = "cannot inline Composition patch sets"
 
 	errFmtApplyComposed              = "cannot apply composed resource %q"
 	errFmtParseBase                  = "cannot parse base template of composed resource %q"
@@ -520,6 +522,14 @@ func (a *GarbageCollectingAssociator) AssociateTemplates(ctx context.Context, cr
 
 		// This existing resource does not correspond to an extant template. It
 		// should be garbage collected.
+		// Remove the labels that indicate this resource was owned by a
+		// Composition. This helps differentiate whether a resource was deleted
+		// due to garbage collection or because its owning composite was deleted.
+		meta.RemoveLabels(cd, xcrd.LabelKeyNamePrefixForComposed, xcrd.LabelKeyClaimName, xcrd.LabelKeyClaimNamespace)
+		if err := a.client.Update(ctx, cd); resource.IgnoreNotFound(err) != nil {
+			return nil, errors.Wrap(err, errGCCleanupLabels)
+		}
+		// Delete the composed resource.
 		if err := a.client.Delete(ctx, cd); resource.IgnoreNotFound(err) != nil {
 			return nil, errors.Wrap(err, errGCComposed)
 		}

internal/controller/apiextensions/composite/composition_pt_test.go

@@ -41,6 +41,7 @@ import (
 
 	v1 "github.com/crossplane/crossplane/apis/apiextensions/v1"
 	"github.com/crossplane/crossplane/internal/names"
+	"github.com/crossplane/crossplane/internal/xcrd"
 )
 
 func TestPTCompose(t *testing.T) {
@@ -688,6 +689,7 @@ func TestGarbageCollectingAssociator(t *testing.T) {
 					// This resource is not controlled by anyone.
 					return nil
 				}),
+				MockUpdate: test.NewMockUpdateFn(nil),
 				MockDelete: test.NewMockDeleteFn(nil),
 			},
 			args: args{
@@ -717,6 +719,7 @@ func TestGarbageCollectingAssociator(t *testing.T) {
 					}})
 					return nil
 				}),
+				MockUpdate: test.NewMockUpdateFn(nil),
 				MockDelete: test.NewMockDeleteFn(errBoom),
 			},
 			args: args{
@@ -744,9 +747,21 @@ func TestGarbageCollectingAssociator(t *testing.T) {
 						BlockOwnerDeletion: &ctrl,
 						UID:                types.UID("it-me"),
 					}})
+					obj.SetLabels(map[string]string{
+						xcrd.LabelKeyNamePrefixForComposed: "cool-xr",
+						xcrd.LabelKeyClaimName:             "cool-claim",
+						xcrd.LabelKeyClaimNamespace:        "cool-namespace",
+					})
 
 					return nil
 				}),
+				MockUpdate: func(_ context.Context, obj client.Object, _ ...client.UpdateOption) error {
+					l := obj.GetLabels()
+					if l[xcrd.CategoryComposite] != "" || l[xcrd.LabelKeyClaimName] != "" || l[xcrd.LabelKeyClaimNamespace] != "" {
+						return errors.New("resource still has composed resource labels")
+					}
+					return nil
+				},
 				MockDelete: test.NewMockDeleteFn(nil),
 			},
 			args: args{

internal/controller/apiextensions/usage/reconciler.go

@@ -47,6 +47,7 @@ import (
 	"github.com/crossplane/crossplane/apis/apiextensions/v1alpha1"
 	apiextensionscontroller "github.com/crossplane/crossplane/internal/controller/apiextensions/controller"
 	"github.com/crossplane/crossplane/internal/usage"
+	"github.com/crossplane/crossplane/internal/xcrd"
 )
 
 const (
@@ -253,7 +254,17 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco
 	}))
 
 	if meta.WasDeleted(u) {
-		if by != nil {
+		// Note (turkenh): A Usage can be composed as part of a Composition
+		// together with the using resource. When the composite is deleted, the
+		// usage resource will also be deleted. In this case, we need to wait
+		// for the using resource to be deleted; otherwise, we won’t be properly
+		// waiting for its deletion since Usage will be gone immediately. We
+		// intentionally avoid checking whether they are part of the same
+		// Composition, as they may not be, but could still be composed together
+		// as part of a higher-level Composition. Therefore, as an approximation
+		// we wait for the using resource to be deleted before deleting the
+		// usage resource, if the usage resource is part of a Composition.
+		if by != nil && u.Labels[xcrd.LabelKeyNamePrefixForComposed] != "" {
 			// Identify using resource as an unstructured object.
 			using := composed.New(composed.FromReference(v1.ObjectReference{
 				Kind:       by.Kind,