Skip to content

Commit ca0f0ff

Browse files
darkowlzzdarkowlzz
committed
Handle delete before adding finalizer
In Reconcile() methods, move the object deletion above add finalizer. Finalizers can't be set when an object is being deleted. Introduce a cacheless client in suite_test to use for testing this change. It ensures that the Reconcile() call always operates on the latest version of the object which has the deletion timestamp and existing finalizer. Signed-off-by: Sunny <[email protected]>
1 parent 66b93aa commit ca0f0ff

13 files changed

+281
-41
lines changed

‎internal/controller/bucket_controller.go‎

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -215,16 +215,19 @@ func (r *BucketReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res
215215
r.Metrics.RecordDuration(ctx, obj, start)
216216
}()
217217

218-
// Add finalizer first if not exist to avoid the race condition between init and delete
219-
if !controllerutil.ContainsFinalizer(obj, sourcev1.SourceFinalizer) {
220-
controllerutil.AddFinalizer(obj, sourcev1.SourceFinalizer)
221-
recResult = sreconcile.ResultRequeue
218+
// Examine if the object is under deletion.
219+
if !obj.ObjectMeta.DeletionTimestamp.IsZero() {
220+
recResult, retErr = r.reconcileDelete(ctx, obj)
222221
return
223222
}
224223

225-
// Examine if the object is under deletion
226-
if !obj.ObjectMeta.DeletionTimestamp.IsZero() {
227-
recResult, retErr = r.reconcileDelete(ctx, obj)
224+
// Add finalizer first if not exist to avoid the race condition between init
225+
// and delete.
226+
// Note: Finalizers in general can only be added when the deletionTimestamp
227+
// is not set.
228+
if !controllerutil.ContainsFinalizer(obj, sourcev1.SourceFinalizer) {
229+
controllerutil.AddFinalizer(obj, sourcev1.SourceFinalizer)
230+
recResult = sreconcile.ResultRequeue
228231
return
229232
}
230233

‎internal/controller/bucket_controller_test.go‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ import (
3333
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
3434
"k8s.io/client-go/tools/record"
3535
kstatus "sigs.k8s.io/cli-utils/pkg/kstatus/status"
36+
ctrl "sigs.k8s.io/controller-runtime"
3637
"sigs.k8s.io/controller-runtime/pkg/client"
3738
fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake"
3839

@@ -54,6 +55,42 @@ import (
5455
// Environment variable to set the GCP Storage host for the GCP client.
5556
const EnvGcpStorageHost = "STORAGE_EMULATOR_HOST"
5657

58+
func TestBucketReconciler_deleteBeforeFinalizer(t *testing.T) {
59+
g := NewWithT(t)
60+
61+
namespaceName := "bucket-" + randStringRunes(5)
62+
namespace := &corev1.Namespace{
63+
ObjectMeta: metav1.ObjectMeta{Name: namespaceName},
64+
}
65+
g.Expect(k8sClient.Create(ctx, namespace)).ToNot(HaveOccurred())
66+
t.Cleanup(func() {
67+
g.Expect(k8sClient.Delete(ctx, namespace)).NotTo(HaveOccurred())
68+
})
69+
70+
bucket := &bucketv1.Bucket{}
71+
bucket.Name = "test-bucket"
72+
bucket.Namespace = namespaceName
73+
bucket.Spec = bucketv1.BucketSpec{
74+
Interval: metav1.Duration{Duration: interval},
75+
BucketName: "foo",
76+
Endpoint: "bar",
77+
}
78+
// Add a test finalizer to prevent the object from getting deleted.
79+
bucket.SetFinalizers([]string{"test-finalizer"})
80+
g.Expect(k8sClient.Create(ctx, bucket)).NotTo(HaveOccurred())
81+
// Add deletion timestamp by deleting the object.
82+
g.Expect(k8sClient.Delete(ctx, bucket)).NotTo(HaveOccurred())
83+
84+
r := &BucketReconciler{
85+
Client: k8sClient,
86+
EventRecorder: record.NewFakeRecorder(32),
87+
Storage: testStorage,
88+
}
89+
// NOTE: Only a real API server responds with an error in this scenario.
90+
_, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(bucket)})
91+
g.Expect(err).NotTo(HaveOccurred())
92+
}
93+
5794
func TestBucketReconciler_Reconcile(t *testing.T) {
5895
g := NewWithT(t)
5996

‎internal/controller/gitrepository_controller.go‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -209,20 +209,22 @@ func (r *GitRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reques
209209
r.Metrics.RecordDuration(ctx, obj, start)
210210
}()
211211

212+
// Examine if the object is under deletion.
213+
if !obj.ObjectMeta.DeletionTimestamp.IsZero() {
214+
recResult, retErr = r.reconcileDelete(ctx, obj)
215+
return
216+
}
217+
212218
// Add finalizer first if not exist to avoid the race condition
213-
// between init and delete
219+
// between init and delete.
220+
// Note: Finalizers in general can only be added when the deletionTimestamp
221+
// is not set.
214222
if !controllerutil.ContainsFinalizer(obj, sourcev1.SourceFinalizer) {
215223
controllerutil.AddFinalizer(obj, sourcev1.SourceFinalizer)
216224
recResult = sreconcile.ResultRequeue
217225
return
218226
}
219227

220-
// Examine if the object is under deletion
221-
if !obj.ObjectMeta.DeletionTimestamp.IsZero() {
222-
recResult, retErr = r.reconcileDelete(ctx, obj)
223-
return
224-
}
225-
226228
// Return if the object is suspended.
227229
if obj.Spec.Suspend {
228230
log.Info("reconciliation is suspended for this object")

‎internal/controller/gitrepository_controller_test.go‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,41 @@ Oomb3gD/TRf/nAdVED+k81GdLzciYdUGtI71/qI47G0nMBluLRE=
143143
`
144144
)
145145

146+
func TestGitRepositoryReconciler_deleteBeforeFinalizer(t *testing.T) {
147+
g := NewWithT(t)
148+
149+
namespaceName := "gitrepo-" + randStringRunes(5)
150+
namespace := &corev1.Namespace{
151+
ObjectMeta: metav1.ObjectMeta{Name: namespaceName},
152+
}
153+
g.Expect(k8sClient.Create(ctx, namespace)).ToNot(HaveOccurred())
154+
t.Cleanup(func() {
155+
g.Expect(k8sClient.Delete(ctx, namespace)).NotTo(HaveOccurred())
156+
})
157+
158+
gitRepo := &sourcev1.GitRepository{}
159+
gitRepo.Name = "test-gitrepo"
160+
gitRepo.Namespace = namespaceName
161+
gitRepo.Spec = sourcev1.GitRepositorySpec{
162+
Interval: metav1.Duration{Duration: interval},
163+
URL: "https://example.com",
164+
}
165+
// Add a test finalizer to prevent the object from getting deleted.
166+
gitRepo.SetFinalizers([]string{"test-finalizer"})
167+
g.Expect(k8sClient.Create(ctx, gitRepo)).NotTo(HaveOccurred())
168+
// Add deletion timestamp by deleting the object.
169+
g.Expect(k8sClient.Delete(ctx, gitRepo)).NotTo(HaveOccurred())
170+
171+
r := &GitRepositoryReconciler{
172+
Client: k8sClient,
173+
EventRecorder: record.NewFakeRecorder(32),
174+
Storage: testStorage,
175+
}
176+
// NOTE: Only a real API server responds with an error in this scenario.
177+
_, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(gitRepo)})
178+
g.Expect(err).NotTo(HaveOccurred())
179+
}
180+
146181
func TestGitRepositoryReconciler_Reconcile(t *testing.T) {
147182
g := NewWithT(t)
148183

‎internal/controller/helmchart_controller.go‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -230,20 +230,22 @@ func (r *HelmChartReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
230230
r.Metrics.RecordDuration(ctx, obj, start)
231231
}()
232232

233+
// Examine if the object is under deletion.
234+
if !obj.ObjectMeta.DeletionTimestamp.IsZero() {
235+
recResult, retErr = r.reconcileDelete(ctx, obj)
236+
return
237+
}
238+
233239
// Add finalizer first if not exist to avoid the race condition
234-
// between init and delete
240+
// between init and delete.
241+
// Note: Finalizers in general can only be added when the deletionTimestamp
242+
// is not set.
235243
if !controllerutil.ContainsFinalizer(obj, sourcev1.SourceFinalizer) {
236244
controllerutil.AddFinalizer(obj, sourcev1.SourceFinalizer)
237245
recResult = sreconcile.ResultRequeue
238246
return
239247
}
240248

241-
// Examine if the object is under deletion
242-
if !obj.ObjectMeta.DeletionTimestamp.IsZero() {
243-
recResult, retErr = r.reconcileDelete(ctx, obj)
244-
return
245-
}
246-
247249
// Return if the object is suspended.
248250
if obj.Spec.Suspend {
249251
log.Info("Reconciliation is suspended for this object")

‎internal/controller/helmchart_controller_test.go‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ import (
4444
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
4545
"k8s.io/client-go/tools/record"
4646
kstatus "sigs.k8s.io/cli-utils/pkg/kstatus/status"
47+
ctrl "sigs.k8s.io/controller-runtime"
4748
"sigs.k8s.io/controller-runtime/pkg/client"
4849
fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake"
4950
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
@@ -66,6 +67,45 @@ import (
6667
"github.com/fluxcd/source-controller/internal/reconcile/summarize"
6768
)
6869

70+
func TestHelmChartReconciler_deleteBeforeFinalizer(t *testing.T) {
71+
g := NewWithT(t)
72+
73+
namespaceName := "helmchart-" + randStringRunes(5)
74+
namespace := &corev1.Namespace{
75+
ObjectMeta: metav1.ObjectMeta{Name: namespaceName},
76+
}
77+
g.Expect(k8sClient.Create(ctx, namespace)).ToNot(HaveOccurred())
78+
t.Cleanup(func() {
79+
g.Expect(k8sClient.Delete(ctx, namespace)).NotTo(HaveOccurred())
80+
})
81+
82+
helmchart := &helmv1.HelmChart{}
83+
helmchart.Name = "test-helmchart"
84+
helmchart.Namespace = namespaceName
85+
helmchart.Spec = helmv1.HelmChartSpec{
86+
Interval: metav1.Duration{Duration: interval},
87+
Chart: "foo",
88+
SourceRef: helmv1.LocalHelmChartSourceReference{
89+
Kind: "HelmRepository",
90+
Name: "bar",
91+
},
92+
}
93+
// Add a test finalizer to prevent the object from getting deleted.
94+
helmchart.SetFinalizers([]string{"test-finalizer"})
95+
g.Expect(k8sClient.Create(ctx, helmchart)).NotTo(HaveOccurred())
96+
// Add deletion timestamp by deleting the object.
97+
g.Expect(k8sClient.Delete(ctx, helmchart)).NotTo(HaveOccurred())
98+
99+
r := &HelmChartReconciler{
100+
Client: k8sClient,
101+
EventRecorder: record.NewFakeRecorder(32),
102+
Storage: testStorage,
103+
}
104+
// NOTE: Only a real API server responds with an error in this scenario.
105+
_, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(helmchart)})
106+
g.Expect(err).NotTo(HaveOccurred())
107+
}
108+
69109
func TestHelmChartReconciler_Reconcile(t *testing.T) {
70110
g := NewWithT(t)
71111

‎internal/controller/helmrepository_controller.go‎

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -191,21 +191,22 @@ func (r *HelmRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reque
191191
r.Metrics.RecordDuration(ctx, obj, start)
192192
}()
193193

194+
// Examine if the object is under deletion or if a type change has happened.
195+
if !obj.ObjectMeta.DeletionTimestamp.IsZero() || (obj.Spec.Type != "" && obj.Spec.Type != helmv1.HelmRepositoryTypeDefault) {
196+
recResult, retErr = r.reconcileDelete(ctx, obj)
197+
return
198+
}
199+
194200
// Add finalizer first if not exist to avoid the race condition
195-
// between init and delete
201+
// between init and delete.
202+
// Note: Finalizers in general can only be added when the deletionTimestamp
203+
// is not set.
196204
if !controllerutil.ContainsFinalizer(obj, sourcev1.SourceFinalizer) {
197205
controllerutil.AddFinalizer(obj, sourcev1.SourceFinalizer)
198206
recResult = sreconcile.ResultRequeue
199207
return
200208
}
201209

202-
// Examine if the object is under deletion
203-
// or if a type change has happened
204-
if !obj.ObjectMeta.DeletionTimestamp.IsZero() || (obj.Spec.Type != "" && obj.Spec.Type != helmv1.HelmRepositoryTypeDefault) {
205-
recResult, retErr = r.reconcileDelete(ctx, obj)
206-
return
207-
}
208-
209210
// Return if the object is suspended.
210211
if obj.Spec.Suspend {
211212
log.Info("reconciliation is suspended for this object")

‎internal/controller/helmrepository_controller_oci.go‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -175,18 +175,20 @@ func (r *HelmRepositoryOCIReconciler) Reconcile(ctx context.Context, req ctrl.Re
175175
r.Metrics.RecordDuration(ctx, obj, start)
176176
}()
177177

178+
// Examine if the object is under deletion.
179+
if !obj.ObjectMeta.DeletionTimestamp.IsZero() {
180+
return r.reconcileDelete(ctx, obj)
181+
}
182+
178183
// Add finalizer first if it doesn't exist to avoid the race condition
179184
// between init and delete.
185+
// Note: Finalizers in general can only be added when the deletionTimestamp
186+
// is not set.
180187
if !controllerutil.ContainsFinalizer(obj, sourcev1.SourceFinalizer) {
181188
controllerutil.AddFinalizer(obj, sourcev1.SourceFinalizer)
182189
return ctrl.Result{Requeue: true}, nil
183190
}
184191

185-
// Examine if the object is under deletion.
186-
if !obj.ObjectMeta.DeletionTimestamp.IsZero() {
187-
return r.reconcileDelete(ctx, obj)
188-
}
189-
190192
// Return if the object is suspended.
191193
if obj.Spec.Suspend {
192194
log.Info("reconciliation is suspended for this object")

‎internal/controller/helmrepository_controller_oci_test.go‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,41 @@ import (
4141
"github.com/fluxcd/source-controller/internal/helm/registry"
4242
)
4343

44+
func TestHelmRepositoryOCIReconciler_deleteBeforeFinalizer(t *testing.T) {
45+
g := NewWithT(t)
46+
47+
namespaceName := "helmrepo-" + randStringRunes(5)
48+
namespace := &corev1.Namespace{
49+
ObjectMeta: metav1.ObjectMeta{Name: namespaceName},
50+
}
51+
g.Expect(k8sClient.Create(ctx, namespace)).ToNot(HaveOccurred())
52+
t.Cleanup(func() {
53+
g.Expect(k8sClient.Delete(ctx, namespace)).NotTo(HaveOccurred())
54+
})
55+
56+
helmrepo := &helmv1.HelmRepository{}
57+
helmrepo.Name = "test-helmrepo"
58+
helmrepo.Namespace = namespaceName
59+
helmrepo.Spec = helmv1.HelmRepositorySpec{
60+
Interval: metav1.Duration{Duration: interval},
61+
URL: "https://example.com",
62+
Type: "oci",
63+
}
64+
// Add a test finalizer to prevent the object from getting deleted.
65+
helmrepo.SetFinalizers([]string{"test-finalizer"})
66+
g.Expect(k8sClient.Create(ctx, helmrepo)).NotTo(HaveOccurred())
67+
// Add deletion timestamp by deleting the object.
68+
g.Expect(k8sClient.Delete(ctx, helmrepo)).NotTo(HaveOccurred())
69+
70+
r := &HelmRepositoryOCIReconciler{
71+
Client: k8sClient,
72+
EventRecorder: record.NewFakeRecorder(32),
73+
}
74+
// NOTE: Only a real API server responds with an error in this scenario.
75+
_, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(helmrepo)})
76+
g.Expect(err).NotTo(HaveOccurred())
77+
}
78+
4479
func TestHelmRepositoryOCIReconciler_Reconcile(t *testing.T) {
4580
tests := []struct {
4681
name string

‎internal/controller/helmrepository_controller_test.go‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ import (
3737
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
3838
"k8s.io/client-go/tools/record"
3939
kstatus "sigs.k8s.io/cli-utils/pkg/kstatus/status"
40+
ctrl "sigs.k8s.io/controller-runtime"
4041
"sigs.k8s.io/controller-runtime/pkg/client"
4142
fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake"
4243

@@ -56,6 +57,41 @@ import (
5657
"github.com/fluxcd/source-controller/internal/reconcile/summarize"
5758
)
5859

60+
func TestHelmRepositoryReconciler_deleteBeforeFinalizer(t *testing.T) {
61+
g := NewWithT(t)
62+
63+
namespaceName := "helmrepo-" + randStringRunes(5)
64+
namespace := &corev1.Namespace{
65+
ObjectMeta: metav1.ObjectMeta{Name: namespaceName},
66+
}
67+
g.Expect(k8sClient.Create(ctx, namespace)).ToNot(HaveOccurred())
68+
t.Cleanup(func() {
69+
g.Expect(k8sClient.Delete(ctx, namespace)).NotTo(HaveOccurred())
70+
})
71+
72+
helmrepo := &helmv1.HelmRepository{}
73+
helmrepo.Name = "test-helmrepo"
74+
helmrepo.Namespace = namespaceName
75+
helmrepo.Spec = helmv1.HelmRepositorySpec{
76+
Interval: metav1.Duration{Duration: interval},
77+
URL: "https://example.com",
78+
}
79+
// Add a test finalizer to prevent the object from getting deleted.
80+
helmrepo.SetFinalizers([]string{"test-finalizer"})
81+
g.Expect(k8sClient.Create(ctx, helmrepo)).NotTo(HaveOccurred())
82+
// Add deletion timestamp by deleting the object.
83+
g.Expect(k8sClient.Delete(ctx, helmrepo)).NotTo(HaveOccurred())
84+
85+
r := &HelmRepositoryReconciler{
86+
Client: k8sClient,
87+
EventRecorder: record.NewFakeRecorder(32),
88+
Storage: testStorage,
89+
}
90+
// NOTE: Only a real API server responds with an error in this scenario.
91+
_, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(helmrepo)})
92+
g.Expect(err).NotTo(HaveOccurred())
93+
}
94+
5995
func TestHelmRepositoryReconciler_Reconcile(t *testing.T) {
6096
g := NewWithT(t)
6197

0 commit comments

Comments
 (0)