Merge pull request #5950 from negz/release-1.16-backport-pr-5876

[release-1.16] Delete resources that don't have a controller but appear in resourceRefs

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{