Delete resources that don't have a controller but appear in resourceRefs

Previously if a composed resource appeared in an XR's spec.resourceRefs
but didn't have a controller reference the XR would refuse to garbage
collect it. The XR would then remove the composed resource from its
resource refs, effectively orphaning it.

Now if the composed resource has _no_ controller, the XR will delete it.
Most likely it was owned by the XR, then had its controller ref stripped
(e.g. due to being backed up and restored using a tool like Velero).

If the composed resource is controlled by another resource, we'll now
return an error rather than silently orphaning it.

Signed-off-by: Nic Cope <[email protected]>

internal/controller/apiextensions/composite/composition_functions.go

@@ -72,6 +72,7 @@ const (
 	errFmtUnmarshalPipelineStepInput = "cannot unmarshal input for Composition pipeline step %q"
 	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"
 	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"
@@ -828,9 +829,17 @@ func (d *DeletingComposedResourceGarbageCollector) GarbageCollectComposedResourc
 	}
 
 	for name, cd := range del {
-		// We want to garbage collect this resource, but we don't control it.
-		if c := metav1.GetControllerOf(cd.Resource); c == nil || c.UID != owner.GetUID() {
-			continue
+		// Don't garbage collect composed resources that someone else controls.
+		//
+		// We do garbage collect composed resources that no-one controls. If a
+		// composed resource appears in observed (i.e. appears in the XR's
+		// spec.resourceRefs) but doesn't have a controller ref, most likely we
+		// created it but its controller ref was stripped. In this situation it
+		// would be permissible for us to adopt the composed resource by setting
+		// our XR as the controller ref, then delete it. So we may as well just
+		// go straight to deleting it.
+		if c := metav1.GetControllerOf(cd.Resource); c != nil && c.UID != owner.GetUID() {
+			return errors.Errorf(errFmtControllerMismatch, name, c.Kind, c.Name)
 		}
 
 		if err := d.client.Delete(ctx, cd.Resource); resource.IgnoreNotFound(err) != nil {

internal/controller/apiextensions/composite/composition_functions_test.go

@@ -1283,11 +1283,21 @@ func TestGarbageCollectComposedResources(t *testing.T) {
 					},
 				},
 				observed: ComposedResourceStates{
-					"undesired-resource": ComposedResourceState{Resource: &fake.Composed{}},
+					"undesired-resource": ComposedResourceState{Resource: &fake.Composed{
+						ObjectMeta: metav1.ObjectMeta{
+							// This resource isn't controlled by the XR.
+							OwnerReferences: []metav1.OwnerReference{{
+								Controller: ptr.To(true),
+								UID:        "a-different-xr",
+								Kind:       "XR",
+								Name:       "different",
+							}},
+						},
+					}},
 				},
 			},
 			want: want{
-				err: nil,
+				err: errors.New(`refusing to delete composed resource "undesired-resource" that is controlled by XR "different"`),
 			},
 		},
 		"DeleteError": {

internal/controller/apiextensions/composite/composition_pt.go

@@ -509,9 +509,17 @@ func (a *GarbageCollectingAssociator) AssociateTemplates(ctx context.Context, cr
 			continue
 		}
 
-		// We want to garbage collect this resource, but we don't control it.
-		if c := metav1.GetControllerOf(cd); c == nil || c.UID != cr.GetUID() {
-			continue
+		// Don't garbage collect composed resources that someone else controls.
+		//
+		// We do garbage collect composed resources that no-one controls. If a
+		// composed resource appears in observed (i.e. appears in the XR's
+		// spec.resourceRefs) but doesn't have a controller ref, most likely we
+		// created it but its controller ref was stripped. In this situation it
+		// would be permissible for us to adopt the composed resource by setting
+		// our XR as the controller ref, then delete it. So we may as well just
+		// go straight to deleting it.
+		if c := metav1.GetControllerOf(cd); c != nil && c.UID != cr.GetUID() {
+			return nil, errors.Errorf(errFmtControllerMismatch, name, c.Kind, c.Name)
 		}
 
 		// This existing resource does not correspond to an extant template. It

internal/controller/apiextensions/composite/composition_pt_test.go

@@ -646,7 +646,7 @@ func TestGarbageCollectingAssociator(t *testing.T) {
 			},
 		},
 		"ResourceControlledBySomeoneElse": {
-			reason: "We should not garbage colle_ a resource that is controlled by another resource.",
+			reason: "We should not garbage collect a resource that is controlled by another resource.",
 			c: &test.MockClient{
 				MockGet: test.NewMockGetFn(nil, func(obj client.Object) error {
 					// The template used to create this resource is no longer known to us.
@@ -658,6 +658,8 @@ func TestGarbageCollectingAssociator(t *testing.T) {
 						Controller:         &ctrl,
 						BlockOwnerDeletion: &ctrl,
 						UID:                types.UID("who-dat"),
+						Kind:               "XR",
+						Name:               "different",
 					}})
 					return nil
 				}),
@@ -670,11 +672,11 @@ func TestGarbageCollectingAssociator(t *testing.T) {
 				ct: []v1.ComposedTemplate{t0},
 			},
 			want: want{
-				tas: []TemplateAssociation{{Template: t0}},
+				err: errors.New(`refusing to delete composed resource "unknown" that is controlled by XR "different"`),
 			},
 		},
 		"ResourceNotControlled": {
-			reason: "We should not garbage colle_ a resource that has no controller reference.",
+			reason: "We should garbage collect a resource that has no controller reference.",
 			c: &test.MockClient{
 				MockGet: test.NewMockGetFn(nil, func(obj client.Object) error {
 					// The template used to create this resource is no longer known to us.
@@ -683,6 +685,7 @@ func TestGarbageCollectingAssociator(t *testing.T) {
 					// This resource is not controlled by anyone.
 					return nil
 				}),
+				MockDelete: test.NewMockDeleteFn(nil),
 			},
 			args: args{
 				cr: &fake.Composite{