Skip to content

Commit cdeffcc

Browse files
turkenhturkenh
authored
Merge pull request crossplane#6155 from turkenh/no-longer-uses
Usages: decomposed usages should be deleted properly
2 parents 5ba0282 + 4a1ccf7 commit cdeffcc

File tree

13 files changed

+482
-5
lines changed

13 files changed

+482
-5
lines changed

‎internal/controller/apiextensions/composite/composition_functions.go‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ import (
4747
fnv1 "github.com/crossplane/crossplane/apis/apiextensions/fn/proto/v1"
4848
v1 "github.com/crossplane/crossplane/apis/apiextensions/v1"
4949
"github.com/crossplane/crossplane/internal/names"
50+
"github.com/crossplane/crossplane/internal/xcrd"
5051
)
5152

5253
// Error strings.
@@ -73,6 +74,7 @@ const (
7374
errFmtGetCredentialsFromSecret = "cannot get Composition pipeline step %q credential %q from Secret"
7475
errFmtRunPipelineStep = "cannot run Composition pipeline step %q"
7576
errFmtControllerMismatch = "refusing to delete composed resource %q that is controlled by %s %q"
77+
errFmtCleanupLabelsCD = "cannot cleanup composed resource labels of resource %q (a %s named %s)"
7678
errFmtDeleteCD = "cannot delete composed resource %q (a %s named %s)"
7779
errFmtUnmarshalDesiredCD = "cannot unmarshal desired composed resource %q from RunFunctionResponse"
7880
errFmtCDAsStruct = "cannot encode composed resource %q to protocol buffer Struct well-known type"
@@ -751,6 +753,14 @@ func (d *DeletingComposedResourceGarbageCollector) GarbageCollectComposedResourc
751753
return errors.Errorf(errFmtControllerMismatch, name, c.Kind, c.Name)
752754
}
753755

756+
// Remove the labels that indicate this resource was owned by a
757+
// Composition. This helps differentiate whether a resource was deleted
758+
// due to garbage collection or because its owning composite was deleted.
759+
meta.RemoveLabels(cd.Resource, xcrd.LabelKeyNamePrefixForComposed, xcrd.LabelKeyClaimName, xcrd.LabelKeyClaimNamespace)
760+
if err := d.client.Update(ctx, cd.Resource); resource.IgnoreNotFound(err) != nil {
761+
return errors.Wrapf(err, errFmtCleanupLabelsCD, name, cd.Resource.GetObjectKind().GroupVersionKind().Kind, cd.Resource.GetName())
762+
}
763+
// Delete the composed resource.
754764
if err := d.client.Delete(ctx, cd.Resource); resource.IgnoreNotFound(err) != nil {
755765
return errors.Wrapf(err, errFmtDeleteCD, name, cd.Resource.GetObjectKind().GroupVersionKind().Kind, cd.Resource.GetName())
756766
}

‎internal/controller/apiextensions/composite/composition_functions_test.go‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1253,10 +1253,42 @@ func TestGarbageCollectComposedResources(t *testing.T) {
12531253
err: errors.New(`refusing to delete composed resource "undesired-resource" that is controlled by XR "different"`),
12541254
},
12551255
},
1256+
"UpdateError": {
1257+
reason: "We should return any error encountered updating the resource with removed labels.",
1258+
params: params{
1259+
client: &test.MockClient{
1260+
MockUpdate: test.NewMockUpdateFn(errBoom),
1261+
},
1262+
},
1263+
args: args{
1264+
owner: &fake.Composite{
1265+
ObjectMeta: metav1.ObjectMeta{
1266+
UID: "cool-xr",
1267+
},
1268+
},
1269+
observed: ComposedResourceStates{
1270+
"undesired-resource": ComposedResourceState{
1271+
Resource: &fake.Composed{
1272+
ObjectMeta: metav1.ObjectMeta{
1273+
// This resource is controlled by the XR.
1274+
OwnerReferences: []metav1.OwnerReference{{
1275+
Controller: ptr.To(true),
1276+
UID: "cool-xr",
1277+
}},
1278+
},
1279+
},
1280+
},
1281+
},
1282+
},
1283+
want: want{
1284+
err: errors.Wrapf(errBoom, errFmtCleanupLabelsCD, "undesired-resource", "", ""),
1285+
},
1286+
},
12561287
"DeleteError": {
12571288
reason: "We should return any error encountered deleting the resource.",
12581289
params: params{
12591290
client: &test.MockClient{
1291+
MockUpdate: test.NewMockUpdateFn(nil),
12601292
MockDelete: test.NewMockDeleteFn(errBoom),
12611293
},
12621294
},
@@ -1288,6 +1320,13 @@ func TestGarbageCollectComposedResources(t *testing.T) {
12881320
reason: "We should successfully delete an observed resource from the API server if it is not desired.",
12891321
params: params{
12901322
client: &test.MockClient{
1323+
MockUpdate: func(_ context.Context, obj client.Object, _ ...client.UpdateOption) error {
1324+
l := obj.GetLabels()
1325+
if l[xcrd.CategoryComposite] != "" || l[xcrd.LabelKeyClaimName] != "" || l[xcrd.LabelKeyClaimNamespace] != "" {
1326+
return errors.New("resource still has composed resource labels")
1327+
}
1328+
return nil
1329+
},
12911330
MockDelete: test.NewMockDeleteFn(nil),
12921331
},
12931332
},
@@ -1306,6 +1345,12 @@ func TestGarbageCollectComposedResources(t *testing.T) {
13061345
Controller: ptr.To(true),
13071346
UID: "cool-xr",
13081347
}},
1348+
// With composed resource labels.
1349+
Labels: map[string]string{
1350+
xcrd.LabelKeyNamePrefixForComposed: "cool-xr",
1351+
xcrd.LabelKeyClaimName: "cool-claim",
1352+
xcrd.LabelKeyClaimNamespace: "cool-namespace",
1353+
},
13091354
},
13101355
},
13111356
},

‎internal/controller/apiextensions/composite/composition_pt.go‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -38,14 +38,16 @@ import (
3838
v1 "github.com/crossplane/crossplane/apis/apiextensions/v1"
3939
"github.com/crossplane/crossplane/internal/controller/apiextensions/usage"
4040
"github.com/crossplane/crossplane/internal/names"
41+
"github.com/crossplane/crossplane/internal/xcrd"
4142
)
4243

4344
// Error strings.
4445
const (
45-
errGetComposed = "cannot get composed resource"
46-
errGCComposed = "cannot garbage collect composed resource"
47-
errFetchDetails = "cannot fetch connection details"
48-
errInline = "cannot inline Composition patch sets"
46+
errGetComposed = "cannot get composed resource"
47+
errGCCleanupLabels = "cannot cleanup composed resource labels before garbage collection"
48+
errGCComposed = "cannot garbage collect composed resource"
49+
errFetchDetails = "cannot fetch connection details"
50+
errInline = "cannot inline Composition patch sets"
4951

5052
errFmtApplyComposed = "cannot apply composed resource %q"
5153
errFmtParseBase = "cannot parse base template of composed resource %q"
@@ -520,6 +522,14 @@ func (a *GarbageCollectingAssociator) AssociateTemplates(ctx context.Context, cr
520522

521523
// This existing resource does not correspond to an extant template. It
522524
// should be garbage collected.
525+
// Remove the labels that indicate this resource was owned by a
526+
// Composition. This helps differentiate whether a resource was deleted
527+
// due to garbage collection or because its owning composite was deleted.
528+
meta.RemoveLabels(cd, xcrd.LabelKeyNamePrefixForComposed, xcrd.LabelKeyClaimName, xcrd.LabelKeyClaimNamespace)
529+
if err := a.client.Update(ctx, cd); resource.IgnoreNotFound(err) != nil {
530+
return nil, errors.Wrap(err, errGCCleanupLabels)
531+
}
532+
// Delete the composed resource.
523533
if err := a.client.Delete(ctx, cd); resource.IgnoreNotFound(err) != nil {
524534
return nil, errors.Wrap(err, errGCComposed)
525535
}

‎internal/controller/apiextensions/composite/composition_pt_test.go‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ import (
4141

4242
v1 "github.com/crossplane/crossplane/apis/apiextensions/v1"
4343
"github.com/crossplane/crossplane/internal/names"
44+
"github.com/crossplane/crossplane/internal/xcrd"
4445
)
4546

4647
func TestPTCompose(t *testing.T) {
@@ -688,6 +689,7 @@ func TestGarbageCollectingAssociator(t *testing.T) {
688689
// This resource is not controlled by anyone.
689690
return nil
690691
}),
692+
MockUpdate: test.NewMockUpdateFn(nil),
691693
MockDelete: test.NewMockDeleteFn(nil),
692694
},
693695
args: args{
@@ -717,6 +719,7 @@ func TestGarbageCollectingAssociator(t *testing.T) {
717719
}})
718720
return nil
719721
}),
722+
MockUpdate: test.NewMockUpdateFn(nil),
720723
MockDelete: test.NewMockDeleteFn(errBoom),
721724
},
722725
args: args{
@@ -744,9 +747,21 @@ func TestGarbageCollectingAssociator(t *testing.T) {
744747
BlockOwnerDeletion: &ctrl,
745748
UID: types.UID("it-me"),
746749
}})
750+
obj.SetLabels(map[string]string{
751+
xcrd.LabelKeyNamePrefixForComposed: "cool-xr",
752+
xcrd.LabelKeyClaimName: "cool-claim",
753+
xcrd.LabelKeyClaimNamespace: "cool-namespace",
754+
})
747755

748756
return nil
749757
}),
758+
MockUpdate: func(_ context.Context, obj client.Object, _ ...client.UpdateOption) error {
759+
l := obj.GetLabels()
760+
if l[xcrd.CategoryComposite] != "" || l[xcrd.LabelKeyClaimName] != "" || l[xcrd.LabelKeyClaimNamespace] != "" {
761+
return errors.New("resource still has composed resource labels")
762+
}
763+
return nil
764+
},
750765
MockDelete: test.NewMockDeleteFn(nil),
751766
},
752767
args: args{

‎internal/controller/apiextensions/usage/reconciler.go‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ import (
4747
"github.com/crossplane/crossplane/apis/apiextensions/v1alpha1"
4848
apiextensionscontroller "github.com/crossplane/crossplane/internal/controller/apiextensions/controller"
4949
"github.com/crossplane/crossplane/internal/usage"
50+
"github.com/crossplane/crossplane/internal/xcrd"
5051
)
5152

5253
const (
@@ -253,7 +254,17 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco
253254
}))
254255

255256
if meta.WasDeleted(u) {
256-
if by != nil {
257+
// Note (turkenh): A Usage can be composed as part of a Composition
258+
// together with the using resource. When the composite is deleted, the
259+
// usage resource will also be deleted. In this case, we need to wait
260+
// for the using resource to be deleted; otherwise, we won’t be properly
261+
// waiting for its deletion since Usage will be gone immediately. We
262+
// intentionally avoid checking whether they are part of the same
263+
// Composition, as they may not be, but could still be composed together
264+
// as part of a higher-level Composition. Therefore, as an approximation
265+
// we wait for the using resource to be deleted before deleting the
266+
// usage resource, if the usage resource is part of a Composition.
267+
if by != nil && u.Labels[xcrd.LabelKeyNamePrefixForComposed] != "" {
257268
// Identify using resource as an unstructured object.
258269
using := composed.New(composed.FromReference(v1.ObjectReference{
259270
Kind: by.Kind,

‎internal/controller/apiextensions/usage/reconciler_test.go‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -533,6 +533,7 @@ func TestReconcile(t *testing.T) {
533533
MockGet: test.NewMockGetFn(nil, func(obj client.Object) error {
534534
if o, ok := obj.(*v1alpha1.Usage); ok {
535535
o.SetDeletionTimestamp(&now)
536+
o.SetLabels(map[string]string{xcrd.LabelKeyNamePrefixForComposed: "some-composite"})
536537
o.Spec.Of.ResourceRef = &v1alpha1.ResourceRef{Name: "used"}
537538
o.Spec.By = &v1alpha1.Resource{
538539
APIVersion: "v1",
@@ -565,6 +566,47 @@ func TestReconcile(t *testing.T) {
565566
err: errors.Wrap(errBoom, errGetUsing),
566567
},
567568
},
569+
"ShouldNotGetUsingOnDeleteIfNotComposed": {
570+
reason: "We should not get using resource on delete if the usage is not composed.",
571+
args: args{
572+
mgr: &fake.Manager{},
573+
opts: []ReconcilerOption{
574+
WithClientApplicator(xpresource.ClientApplicator{
575+
Client: &test.MockClient{
576+
MockGet: test.NewMockGetFn(nil, func(obj client.Object) error {
577+
if o, ok := obj.(*v1alpha1.Usage); ok {
578+
o.SetDeletionTimestamp(&now)
579+
o.Spec.Of.ResourceRef = &v1alpha1.ResourceRef{Name: "used"}
580+
o.Spec.By = &v1alpha1.Resource{
581+
APIVersion: "v1",
582+
Kind: "AnotherKind",
583+
ResourceRef: &v1alpha1.ResourceRef{Name: "using"},
584+
}
585+
return nil
586+
}
587+
if o, ok := obj.(*composed.Unstructured); ok {
588+
if o.GetName() == "using" {
589+
return errors.New("unexpected call, should not get using resource")
590+
}
591+
return nil
592+
}
593+
return errors.New("unexpected object type")
594+
}),
595+
MockList: test.NewMockListFn(nil),
596+
MockUpdate: test.NewMockUpdateFn(nil),
597+
},
598+
}),
599+
WithSelectorResolver(fakeSelectorResolver{
600+
resourceSelectorFn: func(_ context.Context, _ *v1alpha1.Usage) error {
601+
return nil
602+
},
603+
}),
604+
WithFinalizer(xpresource.FinalizerFns{RemoveFinalizerFn: func(_ context.Context, _ xpresource.Object) error {
605+
return nil
606+
}}),
607+
},
608+
},
609+
},
568610
"CannotListUsagesOnDelete": {
569611
reason: "We should return an error if we cannot list usages on delete.",
570612
args: args{

‎test/e2e/manifests/apiextensions/usage/composition-pipeline/claim.yaml‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
apiVersion: nop.example.org/v1alpha1
2+
kind: NopResource
3+
metadata:
4+
namespace: default
5+
name: test
6+
spec:
7+
coolField: "I'm cool!"

‎test/e2e/manifests/apiextensions/usage/composition-pipeline/composition-updated.yaml‎

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
apiVersion: apiextensions.crossplane.io/v1
2+
kind: Composition
3+
metadata:
4+
name: xnopresources.nop.example.org
5+
spec:
6+
compositeTypeRef:
7+
apiVersion: nop.example.org/v1alpha1
8+
kind: XNopResource
9+
mode: Pipeline
10+
pipeline:
11+
- step: be-a-dummy
12+
functionRef:
13+
name: function-dummy
14+
input:
15+
apiVersion: dummy.fn.crossplane.io/v1beta1
16+
kind: Response
17+
# This is a YAML-serialized RunFunctionResponse. function-dummy will
18+
# overlay the desired state on any that was passed into it.
19+
response:
20+
desired:
21+
composite:
22+
resource:
23+
status:
24+
coolerField: "I'M COOLER!"
25+
resources:
26+
used-resource:
27+
resource:
28+
apiVersion: nop.crossplane.io/v1alpha1
29+
kind: NopResource
30+
metadata:
31+
labels:
32+
usage: used
33+
spec:
34+
forProvider:
35+
conditionAfter:
36+
- conditionType: "Synced"
37+
conditionStatus: "True"
38+
time: "5s"
39+
- conditionType: "Ready"
40+
conditionStatus: "True"
41+
time: "10s"
42+
using-resource:
43+
resource:
44+
apiVersion: nop.crossplane.io/v1alpha1
45+
kind: NopResource
46+
metadata:
47+
# We are delaying deletion of using resource with this finalizer. This is to ensure that the used resource is
48+
# not deleted before the using resource. Imagine a scenario where a Release resource using a Cluster resource
49+
# where we expect the Cluster resource not to be deleted until the Release resource is deleted. We are mimicking
50+
# this behavior with the help of a finalizer on a NopResource.
51+
finalizers:
52+
- delay-deletion-of-using-resource
53+
labels:
54+
usage: using
55+
spec:
56+
forProvider:
57+
conditionAfter:
58+
- conditionType: "Synced"
59+
conditionStatus: "True"
60+
time: "5s"
61+
- conditionType: "Ready"
62+
conditionStatus: "True"
63+
time: "10s"
64+
usage-resource-updated:
65+
resource:
66+
apiVersion: apiextensions.crossplane.io/v1alpha1
67+
kind: Usage
68+
metadata:
69+
labels:
70+
version: updated
71+
spec:
72+
replayDeletion: true
73+
of:
74+
apiVersion: nop.crossplane.io/v1alpha1
75+
kind: NopResource
76+
resourceSelector:
77+
matchControllerRef: true
78+
matchLabels:
79+
usage: used
80+
by:
81+
apiVersion: nop.crossplane.io/v1alpha1
82+
kind: NopResource
83+
resourceSelector:
84+
matchControllerRef: true
85+
matchLabels:
86+
usage: using
87+
results:
88+
- severity: SEVERITY_NORMAL
89+
message: "I am doing a compose!"
90+
- step: detect-readiness
91+
functionRef:
92+
name: function-auto-ready

0 commit comments

Comments
 (0)