diff --git a/internal/controller/apiextensions/composite/composition_functions.go b/internal/controller/apiextensions/composite/composition_functions.go index a126dac18..4554ca262 100644 --- a/internal/controller/apiextensions/composite/composition_functions.go +++ b/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 { diff --git a/internal/controller/apiextensions/composite/composition_functions_test.go b/internal/controller/apiextensions/composite/composition_functions_test.go index 0c5d524d0..519e5712b 100644 --- a/internal/controller/apiextensions/composite/composition_functions_test.go +++ b/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": { diff --git a/internal/controller/apiextensions/composite/composition_pt.go b/internal/controller/apiextensions/composite/composition_pt.go index 023afa2fa..29b9fc512 100644 --- a/internal/controller/apiextensions/composite/composition_pt.go +++ b/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 diff --git a/internal/controller/apiextensions/composite/composition_pt_test.go b/internal/controller/apiextensions/composite/composition_pt_test.go index 52558ab59..6eb3b5f52 100644 --- a/internal/controller/apiextensions/composite/composition_pt_test.go +++ b/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{