Skip to content

Commit 96f6041

Browse files
darkowlzzdarkowlzz
authored
Merge pull request #1177 from fluxcd/delete-before-finalizer
Handle delete before adding finalizer
2 parents 66b93aa + ca0f0ff commit 96f6041

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)