Skip to content

Commit 18a5a04

Browse files
fabriziopandinifabriziopandini
authored
🌱 Ensure CRS controller always add ownerReference to resources (kubernetes-sigs#10756)
* Ensure CRS controller always add ownerReference to resources * Address comments * Fix race conditions * Address comments
1 parent 72a3520 commit 18a5a04

File tree

2 files changed

+89
-43
lines changed

2 files changed

+89
-43
lines changed

‎exp/addons/internal/controllers/clusterresourceset_controller.go

Lines changed: 49 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -276,17 +276,38 @@ func (r *ClusterResourceSetReconciler) ApplyClusterResourceSet(ctx context.Conte
276276
log := ctrl.LoggerFrom(ctx, "Cluster", klog.KObj(cluster))
277277
ctx = ctrl.LoggerInto(ctx, log)
278278

279-
remoteClient, err := r.Tracker.GetClient(ctx, util.ObjectKey(cluster))
280-
if err != nil {
281-
conditions.MarkFalse(clusterResourceSet, addonsv1.ResourcesAppliedCondition, addonsv1.RemoteClusterClientFailedReason, clusterv1.ConditionSeverityError, err.Error())
282-
return err
283-
}
279+
// Iterate all resources and ensure an ownerReference to the clusterResourceSet is on the resource.
280+
// NOTE: we have to do this before getting a remote client, otherwise owner reference won't be created until it is
281+
// possible to connect to the remote cluster.
282+
errList := []error{}
283+
objList := make([]*unstructured.Unstructured, len(clusterResourceSet.Spec.Resources))
284+
for i, resource := range clusterResourceSet.Spec.Resources {
285+
unstructuredObj, err := r.getResource(ctx, resource, cluster.GetNamespace())
286+
if err != nil {
287+
if err == ErrSecretTypeNotSupported {
288+
conditions.MarkFalse(clusterResourceSet, addonsv1.ResourcesAppliedCondition, addonsv1.WrongSecretTypeReason, clusterv1.ConditionSeverityWarning, err.Error())
289+
} else {
290+
conditions.MarkFalse(clusterResourceSet, addonsv1.ResourcesAppliedCondition, addonsv1.RetrievingResourceFailedReason, clusterv1.ConditionSeverityWarning, err.Error())
284291

285-
// Ensure that the Kubernetes API Server service has been created in the remote cluster before applying the ClusterResourceSet to avoid service IP conflict.
286-
// This action is required when the remote cluster Kubernetes version is lower than v1.25.
287-
// TODO: Remove this action once CAPI no longer supports Kubernetes versions below v1.25. See: https://github.com/kubernetes-sigs/cluster-api/issues/7804
288-
if err = ensureKubernetesServiceCreated(ctx, remoteClient); err != nil {
289-
return errors.Wrapf(err, "failed to retrieve the Service for Kubernetes API Server of the cluster %s/%s", cluster.Namespace, cluster.Name)
292+
// Continue without adding the error to the aggregate if we can't find the resource.
293+
if apierrors.IsNotFound(err) {
294+
continue
295+
}
296+
}
297+
errList = append(errList, err)
298+
continue
299+
}
300+
301+
// Ensure an ownerReference to the clusterResourceSet is on the resource.
302+
if err := r.ensureResourceOwnerRef(ctx, clusterResourceSet, unstructuredObj); err != nil {
303+
log.Error(err, "Failed to add ClusterResourceSet as resource owner reference",
304+
"Resource type", unstructuredObj.GetKind(), "Resource name", unstructuredObj.GetName())
305+
errList = append(errList, err)
306+
}
307+
objList[i] = unstructuredObj
308+
}
309+
if len(errList) > 0 {
310+
return kerrors.NewAggregate(errList)
290311
}
291312

292313
// Get ClusterResourceSetBinding object for the cluster.
@@ -313,32 +334,28 @@ func (r *ClusterResourceSetReconciler) ApplyClusterResourceSet(ctx context.Conte
313334
Name: clusterResourceSet.Name,
314335
UID: clusterResourceSet.UID,
315336
}))
316-
var errList []error
337+
317338
resourceSetBinding := clusterResourceSetBinding.GetOrCreateBinding(clusterResourceSet)
318339

319-
// Iterate all resources and apply them to the cluster and update the resource status in the ClusterResourceSetBinding object.
320-
for _, resource := range clusterResourceSet.Spec.Resources {
321-
unstructuredObj, err := r.getResource(ctx, resource, cluster.GetNamespace())
322-
if err != nil {
323-
if err == ErrSecretTypeNotSupported {
324-
conditions.MarkFalse(clusterResourceSet, addonsv1.ResourcesAppliedCondition, addonsv1.WrongSecretTypeReason, clusterv1.ConditionSeverityWarning, err.Error())
325-
} else {
326-
conditions.MarkFalse(clusterResourceSet, addonsv1.ResourcesAppliedCondition, addonsv1.RetrievingResourceFailedReason, clusterv1.ConditionSeverityWarning, err.Error())
340+
remoteClient, err := r.Tracker.GetClient(ctx, util.ObjectKey(cluster))
341+
if err != nil {
342+
conditions.MarkFalse(clusterResourceSet, addonsv1.ResourcesAppliedCondition, addonsv1.RemoteClusterClientFailedReason, clusterv1.ConditionSeverityError, err.Error())
343+
return err
344+
}
327345

328-
// Continue without adding the error to the aggregate if we can't find the resource.
329-
if apierrors.IsNotFound(err) {
330-
continue
331-
}
332-
}
333-
errList = append(errList, err)
334-
continue
335-
}
346+
// Ensure that the Kubernetes API Server service has been created in the remote cluster before applying the ClusterResourceSet to avoid service IP conflict.
347+
// This action is required when the remote cluster Kubernetes version is lower than v1.25.
348+
// TODO: Remove this action once CAPI no longer supports Kubernetes versions below v1.25. See: https://github.com/kubernetes-sigs/cluster-api/issues/7804
349+
if err := ensureKubernetesServiceCreated(ctx, remoteClient); err != nil {
350+
return errors.Wrapf(err, "failed to retrieve the Service for Kubernetes API Server of the cluster %s/%s", cluster.Namespace, cluster.Name)
351+
}
336352

337-
// Ensure an ownerReference to the clusterResourceSet is on the resource.
338-
if err := r.ensureResourceOwnerRef(ctx, clusterResourceSet, unstructuredObj); err != nil {
339-
log.Error(err, "Failed to add ClusterResourceSet as resource owner reference",
340-
"Resource type", unstructuredObj.GetKind(), "Resource name", unstructuredObj.GetName())
341-
errList = append(errList, err)
353+
// Iterate all resources and apply them to the cluster and update the resource status in the ClusterResourceSetBinding object.
354+
for i, resource := range clusterResourceSet.Spec.Resources {
355+
unstructuredObj := objList[i]
356+
if unstructuredObj == nil {
357+
// Continue without adding the error to the aggregate if we can't find the resource.
358+
continue
342359
}
343360

344361
resourceScope, err := reconcileScopeForResource(clusterResourceSet, resource, resourceSetBinding, unstructuredObj)

‎exp/addons/internal/controllers/clusterresourceset_controller_test.go

Lines changed: 40 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -345,8 +345,13 @@ metadata:
345345
}
346346
g.Eventually(func() bool {
347347
m := &corev1.ConfigMap{}
348-
err := env.Get(ctx, cmKey, m)
349-
return err == nil
348+
if err := env.Get(ctx, cmKey, m); err != nil {
349+
return false
350+
}
351+
if len(m.OwnerReferences) != 1 || m.OwnerReferences[0].Name != crsInstance.Name {
352+
return false
353+
}
354+
return true
350355
}, timeout).Should(BeTrue())
351356

352357
// When the ConfigMap resource is created, CRS should get reconciled immediately.
@@ -445,8 +450,13 @@ metadata:
445450
}
446451
g.Eventually(func() bool {
447452
m := &corev1.Secret{}
448-
err := env.Get(ctx, cmKey, m)
449-
return err == nil
453+
if err := env.Get(ctx, cmKey, m); err != nil {
454+
return false
455+
}
456+
if len(m.OwnerReferences) != 1 || m.OwnerReferences[0].Name != crsInstance.Name {
457+
return false
458+
}
459+
return true
450460
}, timeout).Should(BeTrue())
451461

452462
// When the Secret resource is created, CRS should get reconciled immediately.
@@ -911,7 +921,7 @@ metadata:
911921
g.Expect(env.Delete(ctx, missingNs)).To(Succeed())
912922
})
913923

914-
t.Run("Should only create ClusterResourceSetBinding after the remote cluster's Kubernetes API Server Service has been created", func(t *testing.T) {
924+
t.Run("Should only apply resources after the remote cluster's Kubernetes API Server Service has been created", func(t *testing.T) {
915925
g := NewWithT(t)
916926
ns := setup(t, g)
917927
defer teardown(t, g, ns)
@@ -962,6 +972,7 @@ metadata:
962972
ClusterSelector: metav1.LabelSelector{
963973
MatchLabels: labels,
964974
},
975+
Resources: []addonsv1.ResourceRef{{Name: secretName, Kind: "Secret"}},
965976
},
966977
}
967978
// Create the ClusterResourceSet.
@@ -970,13 +981,22 @@ metadata:
970981
testCluster.SetLabels(labels)
971982
g.Expect(env.Update(ctx, testCluster)).To(Succeed())
972983

973-
// ClusterResourceSetBinding for the Cluster is not created because the Kubernetes API Server Service doesn't exist.
984+
// Resources are not applied because the Kubernetes API Server Service doesn't exist.
974985
clusterResourceSetBindingKey := client.ObjectKey{Namespace: testCluster.Namespace, Name: testCluster.Name}
975986
g.Consistently(func() bool {
976987
binding := &addonsv1.ClusterResourceSetBinding{}
977988

978-
err := env.Get(ctx, clusterResourceSetBindingKey, binding)
979-
return apierrors.IsNotFound(err)
989+
if err := env.Get(ctx, clusterResourceSetBindingKey, binding); err != nil {
990+
// either the binding is not there
991+
return true
992+
}
993+
// or the binding is there but resources are not applied
994+
for _, b := range binding.Spec.Bindings {
995+
if len(b.Resources) > 0 {
996+
return false
997+
}
998+
}
999+
return true
9801000
}, timeout).Should(BeTrue())
9811001

9821002
t.Log("Create Kubernetes API Server Service")
@@ -990,10 +1010,19 @@ metadata:
9901010
g.Expect(env.Patch(ctx, clusterResourceSetInstance, client.MergeFrom(clusterResourceSetInstance.DeepCopy()))).To(Succeed())
9911011

9921012
// Wait until ClusterResourceSetBinding is created for the Cluster
993-
g.Eventually(func(g Gomega) {
1013+
g.Eventually(func() bool {
1014+
// the binding must exists and track resource being applied
9941015
binding := &addonsv1.ClusterResourceSetBinding{}
995-
g.Expect(env.Get(ctx, clusterResourceSetBindingKey, binding)).Should(Succeed())
996-
}, timeout).Should(Succeed())
1016+
if err := env.Get(ctx, clusterResourceSetBindingKey, binding); err != nil {
1017+
return false
1018+
}
1019+
for _, b := range binding.Spec.Bindings {
1020+
if len(b.Resources) == 0 {
1021+
return false
1022+
}
1023+
}
1024+
return len(binding.Spec.Bindings) != 0
1025+
}, timeout).Should(BeTrue())
9971026
})
9981027

9991028
t.Run("Should handle applying multiple ClusterResourceSets concurrently to the same cluster", func(t *testing.T) {

0 commit comments

Comments
 (0)