diff --git a/internal/controller/bucket_controller.go b/internal/controller/bucket_controller.go index 0fd4082b..ccac13ef 100644 --- a/internal/controller/bucket_controller.go +++ b/internal/controller/bucket_controller.go @@ -215,16 +215,19 @@ func (r *BucketReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res r.Metrics.RecordDuration(ctx, obj, start) }() - // Add finalizer first if not exist to avoid the race condition between init and delete - if !controllerutil.ContainsFinalizer(obj, sourcev1.SourceFinalizer) { - controllerutil.AddFinalizer(obj, sourcev1.SourceFinalizer) - recResult = sreconcile.ResultRequeue + // Examine if the object is under deletion. + if !obj.ObjectMeta.DeletionTimestamp.IsZero() { + recResult, retErr = r.reconcileDelete(ctx, obj) return } - // Examine if the object is under deletion - if !obj.ObjectMeta.DeletionTimestamp.IsZero() { - recResult, retErr = r.reconcileDelete(ctx, obj) + // Add finalizer first if not exist to avoid the race condition between init + // and delete. + // Note: Finalizers in general can only be added when the deletionTimestamp + // is not set. + if !controllerutil.ContainsFinalizer(obj, sourcev1.SourceFinalizer) { + controllerutil.AddFinalizer(obj, sourcev1.SourceFinalizer) + recResult = sreconcile.ResultRequeue return } diff --git a/internal/controller/bucket_controller_test.go b/internal/controller/bucket_controller_test.go index 57da1a31..93a551d6 100644 --- a/internal/controller/bucket_controller_test.go +++ b/internal/controller/bucket_controller_test.go @@ -33,6 +33,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/tools/record" kstatus "sigs.k8s.io/cli-utils/pkg/kstatus/status" + ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -54,6 +55,42 @@ import ( // Environment variable to set the GCP Storage host for the GCP client. const EnvGcpStorageHost = "STORAGE_EMULATOR_HOST" +func TestBucketReconciler_deleteBeforeFinalizer(t *testing.T) { + g := NewWithT(t) + + namespaceName := "bucket-" + randStringRunes(5) + namespace := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: namespaceName}, + } + g.Expect(k8sClient.Create(ctx, namespace)).ToNot(HaveOccurred()) + t.Cleanup(func() { + g.Expect(k8sClient.Delete(ctx, namespace)).NotTo(HaveOccurred()) + }) + + bucket := &bucketv1.Bucket{} + bucket.Name = "test-bucket" + bucket.Namespace = namespaceName + bucket.Spec = bucketv1.BucketSpec{ + Interval: metav1.Duration{Duration: interval}, + BucketName: "foo", + Endpoint: "bar", + } + // Add a test finalizer to prevent the object from getting deleted. + bucket.SetFinalizers([]string{"test-finalizer"}) + g.Expect(k8sClient.Create(ctx, bucket)).NotTo(HaveOccurred()) + // Add deletion timestamp by deleting the object. + g.Expect(k8sClient.Delete(ctx, bucket)).NotTo(HaveOccurred()) + + r := &BucketReconciler{ + Client: k8sClient, + EventRecorder: record.NewFakeRecorder(32), + Storage: testStorage, + } + // NOTE: Only a real API server responds with an error in this scenario. + _, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(bucket)}) + g.Expect(err).NotTo(HaveOccurred()) +} + func TestBucketReconciler_Reconcile(t *testing.T) { g := NewWithT(t) diff --git a/internal/controller/gitrepository_controller.go b/internal/controller/gitrepository_controller.go index c3fb3888..dc7d7a0f 100644 --- a/internal/controller/gitrepository_controller.go +++ b/internal/controller/gitrepository_controller.go @@ -209,17 +209,19 @@ func (r *GitRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reques r.Metrics.RecordDuration(ctx, obj, start) }() - // Add finalizer first if not exist to avoid the race condition - // between init and delete - if !controllerutil.ContainsFinalizer(obj, sourcev1.SourceFinalizer) { - controllerutil.AddFinalizer(obj, sourcev1.SourceFinalizer) - recResult = sreconcile.ResultRequeue + // Examine if the object is under deletion. + if !obj.ObjectMeta.DeletionTimestamp.IsZero() { + recResult, retErr = r.reconcileDelete(ctx, obj) return } - // Examine if the object is under deletion - if !obj.ObjectMeta.DeletionTimestamp.IsZero() { - recResult, retErr = r.reconcileDelete(ctx, obj) + // Add finalizer first if not exist to avoid the race condition + // between init and delete. + // Note: Finalizers in general can only be added when the deletionTimestamp + // is not set. + if !controllerutil.ContainsFinalizer(obj, sourcev1.SourceFinalizer) { + controllerutil.AddFinalizer(obj, sourcev1.SourceFinalizer) + recResult = sreconcile.ResultRequeue return } diff --git a/internal/controller/gitrepository_controller_test.go b/internal/controller/gitrepository_controller_test.go index 8b452daa..cd3c085e 100644 --- a/internal/controller/gitrepository_controller_test.go +++ b/internal/controller/gitrepository_controller_test.go @@ -143,6 +143,41 @@ Oomb3gD/TRf/nAdVED+k81GdLzciYdUGtI71/qI47G0nMBluLRE= ` ) +func TestGitRepositoryReconciler_deleteBeforeFinalizer(t *testing.T) { + g := NewWithT(t) + + namespaceName := "gitrepo-" + randStringRunes(5) + namespace := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: namespaceName}, + } + g.Expect(k8sClient.Create(ctx, namespace)).ToNot(HaveOccurred()) + t.Cleanup(func() { + g.Expect(k8sClient.Delete(ctx, namespace)).NotTo(HaveOccurred()) + }) + + gitRepo := &sourcev1.GitRepository{} + gitRepo.Name = "test-gitrepo" + gitRepo.Namespace = namespaceName + gitRepo.Spec = sourcev1.GitRepositorySpec{ + Interval: metav1.Duration{Duration: interval}, + URL: "https://example.com", + } + // Add a test finalizer to prevent the object from getting deleted. + gitRepo.SetFinalizers([]string{"test-finalizer"}) + g.Expect(k8sClient.Create(ctx, gitRepo)).NotTo(HaveOccurred()) + // Add deletion timestamp by deleting the object. + g.Expect(k8sClient.Delete(ctx, gitRepo)).NotTo(HaveOccurred()) + + r := &GitRepositoryReconciler{ + Client: k8sClient, + EventRecorder: record.NewFakeRecorder(32), + Storage: testStorage, + } + // NOTE: Only a real API server responds with an error in this scenario. + _, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(gitRepo)}) + g.Expect(err).NotTo(HaveOccurred()) +} + func TestGitRepositoryReconciler_Reconcile(t *testing.T) { g := NewWithT(t) diff --git a/internal/controller/helmchart_controller.go b/internal/controller/helmchart_controller.go index 548b4bc5..d393fcb3 100644 --- a/internal/controller/helmchart_controller.go +++ b/internal/controller/helmchart_controller.go @@ -230,17 +230,19 @@ func (r *HelmChartReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( r.Metrics.RecordDuration(ctx, obj, start) }() - // Add finalizer first if not exist to avoid the race condition - // between init and delete - if !controllerutil.ContainsFinalizer(obj, sourcev1.SourceFinalizer) { - controllerutil.AddFinalizer(obj, sourcev1.SourceFinalizer) - recResult = sreconcile.ResultRequeue + // Examine if the object is under deletion. + if !obj.ObjectMeta.DeletionTimestamp.IsZero() { + recResult, retErr = r.reconcileDelete(ctx, obj) return } - // Examine if the object is under deletion - if !obj.ObjectMeta.DeletionTimestamp.IsZero() { - recResult, retErr = r.reconcileDelete(ctx, obj) + // Add finalizer first if not exist to avoid the race condition + // between init and delete. + // Note: Finalizers in general can only be added when the deletionTimestamp + // is not set. + if !controllerutil.ContainsFinalizer(obj, sourcev1.SourceFinalizer) { + controllerutil.AddFinalizer(obj, sourcev1.SourceFinalizer) + recResult = sreconcile.ResultRequeue return } diff --git a/internal/controller/helmchart_controller_test.go b/internal/controller/helmchart_controller_test.go index 35d73695..c6f03017 100644 --- a/internal/controller/helmchart_controller_test.go +++ b/internal/controller/helmchart_controller_test.go @@ -44,6 +44,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/tools/record" kstatus "sigs.k8s.io/cli-utils/pkg/kstatus/status" + ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -66,6 +67,45 @@ import ( "github.com/fluxcd/source-controller/internal/reconcile/summarize" ) +func TestHelmChartReconciler_deleteBeforeFinalizer(t *testing.T) { + g := NewWithT(t) + + namespaceName := "helmchart-" + randStringRunes(5) + namespace := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: namespaceName}, + } + g.Expect(k8sClient.Create(ctx, namespace)).ToNot(HaveOccurred()) + t.Cleanup(func() { + g.Expect(k8sClient.Delete(ctx, namespace)).NotTo(HaveOccurred()) + }) + + helmchart := &helmv1.HelmChart{} + helmchart.Name = "test-helmchart" + helmchart.Namespace = namespaceName + helmchart.Spec = helmv1.HelmChartSpec{ + Interval: metav1.Duration{Duration: interval}, + Chart: "foo", + SourceRef: helmv1.LocalHelmChartSourceReference{ + Kind: "HelmRepository", + Name: "bar", + }, + } + // Add a test finalizer to prevent the object from getting deleted. + helmchart.SetFinalizers([]string{"test-finalizer"}) + g.Expect(k8sClient.Create(ctx, helmchart)).NotTo(HaveOccurred()) + // Add deletion timestamp by deleting the object. + g.Expect(k8sClient.Delete(ctx, helmchart)).NotTo(HaveOccurred()) + + r := &HelmChartReconciler{ + Client: k8sClient, + EventRecorder: record.NewFakeRecorder(32), + Storage: testStorage, + } + // NOTE: Only a real API server responds with an error in this scenario. + _, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(helmchart)}) + g.Expect(err).NotTo(HaveOccurred()) +} + func TestHelmChartReconciler_Reconcile(t *testing.T) { g := NewWithT(t) diff --git a/internal/controller/helmrepository_controller.go b/internal/controller/helmrepository_controller.go index 1b6161ee..99ace6ec 100644 --- a/internal/controller/helmrepository_controller.go +++ b/internal/controller/helmrepository_controller.go @@ -191,18 +191,19 @@ func (r *HelmRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reque r.Metrics.RecordDuration(ctx, obj, start) }() - // Add finalizer first if not exist to avoid the race condition - // between init and delete - if !controllerutil.ContainsFinalizer(obj, sourcev1.SourceFinalizer) { - controllerutil.AddFinalizer(obj, sourcev1.SourceFinalizer) - recResult = sreconcile.ResultRequeue + // Examine if the object is under deletion or if a type change has happened. + if !obj.ObjectMeta.DeletionTimestamp.IsZero() || (obj.Spec.Type != "" && obj.Spec.Type != helmv1.HelmRepositoryTypeDefault) { + recResult, retErr = r.reconcileDelete(ctx, obj) return } - // Examine if the object is under deletion - // or if a type change has happened - if !obj.ObjectMeta.DeletionTimestamp.IsZero() || (obj.Spec.Type != "" && obj.Spec.Type != helmv1.HelmRepositoryTypeDefault) { - recResult, retErr = r.reconcileDelete(ctx, obj) + // Add finalizer first if not exist to avoid the race condition + // between init and delete. + // Note: Finalizers in general can only be added when the deletionTimestamp + // is not set. + if !controllerutil.ContainsFinalizer(obj, sourcev1.SourceFinalizer) { + controllerutil.AddFinalizer(obj, sourcev1.SourceFinalizer) + recResult = sreconcile.ResultRequeue return } diff --git a/internal/controller/helmrepository_controller_oci.go b/internal/controller/helmrepository_controller_oci.go index 04822797..2752a612 100644 --- a/internal/controller/helmrepository_controller_oci.go +++ b/internal/controller/helmrepository_controller_oci.go @@ -175,18 +175,20 @@ func (r *HelmRepositoryOCIReconciler) Reconcile(ctx context.Context, req ctrl.Re r.Metrics.RecordDuration(ctx, obj, start) }() - // Add finalizer first if it doesn't exist to avoid the race condition - // between init and delete. - if !controllerutil.ContainsFinalizer(obj, sourcev1.SourceFinalizer) { - controllerutil.AddFinalizer(obj, sourcev1.SourceFinalizer) - return ctrl.Result{Requeue: true}, nil - } - // Examine if the object is under deletion. if !obj.ObjectMeta.DeletionTimestamp.IsZero() { return r.reconcileDelete(ctx, obj) } + // Add finalizer first if it doesn't exist to avoid the race condition + // between init and delete. + // Note: Finalizers in general can only be added when the deletionTimestamp + // is not set. + if !controllerutil.ContainsFinalizer(obj, sourcev1.SourceFinalizer) { + controllerutil.AddFinalizer(obj, sourcev1.SourceFinalizer) + return ctrl.Result{Requeue: true}, nil + } + // Return if the object is suspended. if obj.Spec.Suspend { log.Info("reconciliation is suspended for this object") diff --git a/internal/controller/helmrepository_controller_oci_test.go b/internal/controller/helmrepository_controller_oci_test.go index 2a576802..88f1c0aa 100644 --- a/internal/controller/helmrepository_controller_oci_test.go +++ b/internal/controller/helmrepository_controller_oci_test.go @@ -41,6 +41,41 @@ import ( "github.com/fluxcd/source-controller/internal/helm/registry" ) +func TestHelmRepositoryOCIReconciler_deleteBeforeFinalizer(t *testing.T) { + g := NewWithT(t) + + namespaceName := "helmrepo-" + randStringRunes(5) + namespace := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: namespaceName}, + } + g.Expect(k8sClient.Create(ctx, namespace)).ToNot(HaveOccurred()) + t.Cleanup(func() { + g.Expect(k8sClient.Delete(ctx, namespace)).NotTo(HaveOccurred()) + }) + + helmrepo := &helmv1.HelmRepository{} + helmrepo.Name = "test-helmrepo" + helmrepo.Namespace = namespaceName + helmrepo.Spec = helmv1.HelmRepositorySpec{ + Interval: metav1.Duration{Duration: interval}, + URL: "https://example.com", + Type: "oci", + } + // Add a test finalizer to prevent the object from getting deleted. + helmrepo.SetFinalizers([]string{"test-finalizer"}) + g.Expect(k8sClient.Create(ctx, helmrepo)).NotTo(HaveOccurred()) + // Add deletion timestamp by deleting the object. + g.Expect(k8sClient.Delete(ctx, helmrepo)).NotTo(HaveOccurred()) + + r := &HelmRepositoryOCIReconciler{ + Client: k8sClient, + EventRecorder: record.NewFakeRecorder(32), + } + // NOTE: Only a real API server responds with an error in this scenario. + _, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(helmrepo)}) + g.Expect(err).NotTo(HaveOccurred()) +} + func TestHelmRepositoryOCIReconciler_Reconcile(t *testing.T) { tests := []struct { name string diff --git a/internal/controller/helmrepository_controller_test.go b/internal/controller/helmrepository_controller_test.go index bd3e45f6..9e8fc5d4 100644 --- a/internal/controller/helmrepository_controller_test.go +++ b/internal/controller/helmrepository_controller_test.go @@ -37,6 +37,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/tools/record" kstatus "sigs.k8s.io/cli-utils/pkg/kstatus/status" + ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -56,6 +57,41 @@ import ( "github.com/fluxcd/source-controller/internal/reconcile/summarize" ) +func TestHelmRepositoryReconciler_deleteBeforeFinalizer(t *testing.T) { + g := NewWithT(t) + + namespaceName := "helmrepo-" + randStringRunes(5) + namespace := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: namespaceName}, + } + g.Expect(k8sClient.Create(ctx, namespace)).ToNot(HaveOccurred()) + t.Cleanup(func() { + g.Expect(k8sClient.Delete(ctx, namespace)).NotTo(HaveOccurred()) + }) + + helmrepo := &helmv1.HelmRepository{} + helmrepo.Name = "test-helmrepo" + helmrepo.Namespace = namespaceName + helmrepo.Spec = helmv1.HelmRepositorySpec{ + Interval: metav1.Duration{Duration: interval}, + URL: "https://example.com", + } + // Add a test finalizer to prevent the object from getting deleted. + helmrepo.SetFinalizers([]string{"test-finalizer"}) + g.Expect(k8sClient.Create(ctx, helmrepo)).NotTo(HaveOccurred()) + // Add deletion timestamp by deleting the object. + g.Expect(k8sClient.Delete(ctx, helmrepo)).NotTo(HaveOccurred()) + + r := &HelmRepositoryReconciler{ + Client: k8sClient, + EventRecorder: record.NewFakeRecorder(32), + Storage: testStorage, + } + // NOTE: Only a real API server responds with an error in this scenario. + _, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(helmrepo)}) + g.Expect(err).NotTo(HaveOccurred()) +} + func TestHelmRepositoryReconciler_Reconcile(t *testing.T) { g := NewWithT(t) diff --git a/internal/controller/ocirepository_controller.go b/internal/controller/ocirepository_controller.go index 9c7c0fed..9986dc9b 100644 --- a/internal/controller/ocirepository_controller.go +++ b/internal/controller/ocirepository_controller.go @@ -210,16 +210,19 @@ func (r *OCIRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reques r.Metrics.RecordDuration(ctx, obj, start) }() - // Add finalizer first if not exist to avoid the race condition between init and delete - if !controllerutil.ContainsFinalizer(obj, sourcev1.SourceFinalizer) { - controllerutil.AddFinalizer(obj, sourcev1.SourceFinalizer) - recResult = sreconcile.ResultRequeue + // Examine if the object is under deletion. + if !obj.ObjectMeta.DeletionTimestamp.IsZero() { + recResult, retErr = r.reconcileDelete(ctx, obj) return } - // Examine if the object is under deletion - if !obj.ObjectMeta.DeletionTimestamp.IsZero() { - recResult, retErr = r.reconcileDelete(ctx, obj) + // Add finalizer first if not exist to avoid the race condition between init + // and delete. + // Note: Finalizers in general can only be added when the deletionTimestamp + // is not set. + if !controllerutil.ContainsFinalizer(obj, sourcev1.SourceFinalizer) { + controllerutil.AddFinalizer(obj, sourcev1.SourceFinalizer) + recResult = sreconcile.ResultRequeue return } diff --git a/internal/controller/ocirepository_controller_test.go b/internal/controller/ocirepository_controller_test.go index 12350c37..ee8f3af8 100644 --- a/internal/controller/ocirepository_controller_test.go +++ b/internal/controller/ocirepository_controller_test.go @@ -48,6 +48,7 @@ import ( "k8s.io/client-go/tools/record" "k8s.io/utils/pointer" kstatus "sigs.k8s.io/cli-utils/pkg/kstatus/status" + ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -67,6 +68,41 @@ import ( sreconcile "github.com/fluxcd/source-controller/internal/reconcile" ) +func TestOCIRepositoryReconciler_deleteBeforeFinalizer(t *testing.T) { + g := NewWithT(t) + + namespaceName := "ocirepo-" + randStringRunes(5) + namespace := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: namespaceName}, + } + g.Expect(k8sClient.Create(ctx, namespace)).ToNot(HaveOccurred()) + t.Cleanup(func() { + g.Expect(k8sClient.Delete(ctx, namespace)).NotTo(HaveOccurred()) + }) + + ocirepo := &ociv1.OCIRepository{} + ocirepo.Name = "test-ocirepo" + ocirepo.Namespace = namespaceName + ocirepo.Spec = ociv1.OCIRepositorySpec{ + Interval: metav1.Duration{Duration: interval}, + URL: "oci://example.com", + } + // Add a test finalizer to prevent the object from getting deleted. + ocirepo.SetFinalizers([]string{"test-finalizer"}) + g.Expect(k8sClient.Create(ctx, ocirepo)).NotTo(HaveOccurred()) + // Add deletion timestamp by deleting the object. + g.Expect(k8sClient.Delete(ctx, ocirepo)).NotTo(HaveOccurred()) + + r := &OCIRepositoryReconciler{ + Client: k8sClient, + EventRecorder: record.NewFakeRecorder(32), + Storage: testStorage, + } + // NOTE: Only a real API server responds with an error in this scenario. + _, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(ocirepo)}) + g.Expect(err).NotTo(HaveOccurred()) +} + func TestOCIRepository_Reconcile(t *testing.T) { g := NewWithT(t) diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go index e9434f20..2200fe12 100644 --- a/internal/controller/suite_test.go +++ b/internal/controller/suite_test.go @@ -40,6 +40,7 @@ import ( "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/tools/record" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" "github.com/distribution/distribution/v3/configuration" dcontext "github.com/distribution/distribution/v3/context" @@ -78,6 +79,7 @@ const ( ) var ( + k8sClient client.Client testEnv *testenv.Environment testStorage *Storage testServer *testserver.ArtifactServer @@ -255,6 +257,12 @@ func TestMain(m *testing.M) { ) var err error + // Initialize a cacheless client for tests that need the latest objects. + k8sClient, err = client.New(testEnv.Config, client.Options{Scheme: scheme.Scheme}) + if err != nil { + panic(fmt.Sprintf("failed to create k8s client: %v", err)) + } + testServer, err = testserver.NewTempArtifactServer() if err != nil { panic(fmt.Sprintf("Failed to create a temporary storage server: %v", err))