From 3b2ba1707b38d6f59c77b022e3a3ea81d3ce148c Mon Sep 17 00:00:00 2001 From: Nic Cope Date: Fri, 9 Aug 2024 17:29:06 -0700 Subject: [PATCH] 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 --- .../composite/composition_functions.go | 15 ++++++++++++--- .../composite/composition_functions_test.go | 14 ++++++++++++-- .../apiextensions/composite/composition_pt.go | 14 +++++++++++--- .../composite/composition_pt_test.go | 9 ++++++--- 4 files changed, 41 insertions(+), 11 deletions(-) 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{