Allow deleting suspended objects

controllers/bucket_controller.go

@@ -258,12 +258,6 @@ func (r *BucketReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res
 	// Record suspended status metric
 	r.RecordSuspend(ctx, obj, obj.Spec.Suspend)
 
-	// Return early if the object is suspended
-	if obj.Spec.Suspend {
-		log.Info("reconciliation is suspended for this object")
-		return ctrl.Result{}, nil
-	}
-
 	// Initialize the patch helper with the current version of the object.
 	patchHelper, err := patch.NewHelper(obj, r.Client)
 	if err != nil {
@@ -309,6 +303,13 @@ func (r *BucketReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res
 		return
 	}
 
+	// Return if the object is suspended.
+	if obj.Spec.Suspend {
+		log.Info("reconciliation is suspended for this object")
+		recResult, retErr = sreconcile.ResultEmpty, nil
+		return
+	}
+
 	// Reconcile actual object
 	reconcilers := []bucketReconcileFunc{
 		r.reconcileStorage,

controllers/bucket_controller_test.go

@@ -30,7 +30,6 @@ import (
 
 	. "github.com/onsi/gomega"
 	corev1 "k8s.io/api/core/v1"
-	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/tools/record"
 	kstatus "sigs.k8s.io/cli-utils/pkg/kstatus/status"
@@ -85,7 +84,7 @@ func TestBucketReconciler_Reconcile(t *testing.T) {
 	g.Expect(testEnv.Create(ctx, secret)).To(Succeed())
 	defer testEnv.Delete(ctx, secret)
 
-	obj := &sourcev1.Bucket{
+	origObj := &sourcev1.Bucket{
 		ObjectMeta: metav1.ObjectMeta{
 			GenerateName: "bucket-reconcile-",
 			Namespace:    "default",
@@ -102,6 +101,7 @@ func TestBucketReconciler_Reconcile(t *testing.T) {
 			},
 		},
 	}
+	obj := origObj.DeepCopy()
 	g.Expect(testEnv.Create(ctx, obj)).To(Succeed())
 
 	key := client.ObjectKey{Name: obj.Name, Namespace: obj.Namespace}
@@ -115,17 +115,7 @@ func TestBucketReconciler_Reconcile(t *testing.T) {
 	}, timeout).Should(BeTrue())
 
 	// Wait for Bucket to be Ready
-	g.Eventually(func() bool {
-		if err := testEnv.Get(ctx, key, obj); err != nil {
-			return false
-		}
-		if !conditions.IsReady(obj) || obj.Status.Artifact == nil {
-			return false
-		}
-		readyCondition := conditions.Get(obj, meta.ReadyCondition)
-		return obj.Generation == readyCondition.ObservedGeneration &&
-			obj.Generation == obj.Status.ObservedGeneration
-	}, timeout).Should(BeTrue())
+	waitForSourceReadyWithArtifact(ctx, g, obj)
 
 	// Check if the object status is valid.
 	condns := &conditionscheck.Conditions{NegativePolarity: bucketReadyCondition.NegativePolarity}
@@ -157,12 +147,11 @@ func TestBucketReconciler_Reconcile(t *testing.T) {
 	g.Expect(testEnv.Delete(ctx, obj)).To(Succeed())
 
 	// Wait for Bucket to be deleted
-	g.Eventually(func() bool {
-		if err := testEnv.Get(ctx, key, obj); err != nil {
-			return apierrors.IsNotFound(err)
-		}
-		return false
-	}, timeout).Should(BeTrue())
+	waitForSourceDeletion(ctx, g, obj)
+
+	// Check if a suspended object gets deleted.
+	obj = origObj.DeepCopy()
+	testSuspendedObjectDeleteWithArtifact(ctx, g, obj)
 }
 
 func TestBucketReconciler_reconcileStorage(t *testing.T) {

controllers/common_test.go

@@ -0,0 +1,146 @@
+/*
+Copyright 2022 The Flux authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package controllers
+
+import (
+	"context"
+
+	. "github.com/onsi/gomega"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+
+	"github.com/fluxcd/pkg/apis/meta"
+	"github.com/fluxcd/pkg/runtime/conditions"
+	"github.com/fluxcd/pkg/runtime/patch"
+
+	"github.com/fluxcd/source-controller/internal/object"
+)
+
+// waitForSourceDeletion is a generic test helper to wait for object deletion of
+// any source kind.
+func waitForSourceDeletion(ctx context.Context, g *WithT, obj conditions.Setter) {
+	g.THelper()
+
+	key := client.ObjectKeyFromObject(obj)
+	g.Eventually(func() bool {
+		if err := testEnv.Get(ctx, key, obj); err != nil {
+			return apierrors.IsNotFound(err)
+		}
+		return false
+	}, timeout).Should(BeTrue())
+}
+
+// waitForSuspended is a generic test helper to wait for object to be suspended
+// of any source kind.
+func waitForSuspended(ctx context.Context, g *WithT, obj conditions.Setter) {
+	g.THelper()
+
+	key := client.ObjectKeyFromObject(obj)
+	g.Eventually(func() bool {
+		if err := testEnv.Get(ctx, key, obj); err != nil {
+			return false
+		}
+		suspended, err := object.GetSuspend(obj)
+		if err != nil {
+			return false
+		}
+		return suspended == true
+	}, timeout).Should(BeTrue())
+}
+
+// waitForSourceReadyWithArtifact is a generic test helper to wait for an object
+// to be ready of any source kind that have artifact in status when ready.
+func waitForSourceReadyWithArtifact(ctx context.Context, g *WithT, obj conditions.Setter) {
+	g.THelper()
+	waitForSourceReady(ctx, g, obj, true)
+}
+
+// waitForSourceReadyWithoutArtifact is a generic test helper to wait for an object
+// to be ready of any source kind that don't have artifact in status when ready.
+func waitForSourceReadyWithoutArtifact(ctx context.Context, g *WithT, obj conditions.Setter) {
+	g.THelper()
+	waitForSourceReady(ctx, g, obj, false)
+}
+
+// waitForSourceReady is a generic test helper to wait for an object to be
+// ready of any source kind.
+func waitForSourceReady(ctx context.Context, g *WithT, obj conditions.Setter, withArtifact bool) {
+	g.THelper()
+
+	key := client.ObjectKeyFromObject(obj)
+	g.Eventually(func() bool {
+		if err := testEnv.Get(ctx, key, obj); err != nil {
+			return false
+		}
+		if withArtifact {
+			artifact, err := object.GetArtifact(obj)
+			if err != nil {
+				return false
+			}
+			if artifact == nil {
+				return false
+			}
+		}
+		if !conditions.IsReady(obj) {
+			return false
+		}
+		readyCondition := conditions.Get(obj, meta.ReadyCondition)
+		statusObservedGen, err := object.GetStatusObservedGeneration(obj)
+		if err != nil {
+			return false
+		}
+		return obj.GetGeneration() == readyCondition.ObservedGeneration &&
+			obj.GetGeneration() == statusObservedGen
+	}, timeout).Should(BeTrue())
+}
+
+// testSuspendedObjectDeleteWithArtifact is a generic test helper to test if a
+// suspended object can be deleted for objects that have artifact in status when
+// ready.
+func testSuspendedObjectDeleteWithArtifact(ctx context.Context, g *WithT, obj conditions.Setter) {
+	g.THelper()
+	testSuspendedObjectDelete(ctx, g, obj, true)
+}
+
+// testSuspendedObjectDeleteWithoutArtifact is a generic test helper to test if
+// a suspended object can be deleted for objects that don't have artifact in
+// status when ready.
+func testSuspendedObjectDeleteWithoutArtifact(ctx context.Context, g *WithT, obj conditions.Setter) {
+	g.THelper()
+	testSuspendedObjectDelete(ctx, g, obj, false)
+}
+
+// testSuspendedObjectDelete is a generic test helper to test if a suspended
+// object can be deleted.
+func testSuspendedObjectDelete(ctx context.Context, g *WithT, obj conditions.Setter, withArtifact bool) {
+	g.THelper()
+
+	// Create the object and wait for it to be ready.
+	g.Expect(testEnv.Create(ctx, obj)).To(Succeed())
+	waitForSourceReady(ctx, g, obj, withArtifact)
+
+	// Suspend the object.
+	patchHelper, err := patch.NewHelper(obj, testEnv.Client)
+	g.Expect(err).ToNot(HaveOccurred())
+	g.Expect(object.SetSuspend(obj, true)).ToNot(HaveOccurred())
+	g.Expect(patchHelper.Patch(ctx, obj)).ToNot(HaveOccurred())
+	waitForSuspended(ctx, g, obj)
+
+	// Delete the object.
+	g.Expect(testEnv.Delete(ctx, obj)).To(Succeed())
+	waitForSourceDeletion(ctx, g, obj)
+}

controllers/gitrepository_controller.go

@@ -172,12 +172,6 @@ func (r *GitRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 	// Record suspended status metric
 	r.RecordSuspend(ctx, obj, obj.Spec.Suspend)
 
-	// Return early if the object is suspended
-	if obj.Spec.Suspend {
-		log.Info("reconciliation is suspended for this object")
-		return ctrl.Result{}, nil
-	}
-
 	// Initialize the patch helper with the current version of the object.
 	patchHelper, err := patch.NewHelper(obj, r.Client)
 	if err != nil {
@@ -225,6 +219,13 @@ func (r *GitRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 		return
 	}
 
+	// Return if the object is suspended.
+	if obj.Spec.Suspend {
+		log.Info("reconciliation is suspended for this object")
+		recResult, retErr = sreconcile.ResultEmpty, nil
+		return
+	}
+
 	// Reconcile actual object
 	reconcilers := []gitRepositoryReconcileFunc{
 		r.reconcileStorage,

controllers/gitrepository_controller_test.go

@@ -37,7 +37,6 @@ import (
 	. "github.com/onsi/gomega"
 	sshtestdata "golang.org/x/crypto/ssh/testdata"
 	corev1 "k8s.io/api/core/v1"
-	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/client-go/tools/record"
@@ -168,7 +167,7 @@ func TestGitRepositoryReconciler_Reconcile(t *testing.T) {
 	_, err = initGitRepo(server, "testdata/git/repository", git.DefaultBranch, repoPath)
 	g.Expect(err).NotTo(HaveOccurred())
 
-	obj := &sourcev1.GitRepository{
+	origObj := &sourcev1.GitRepository{
 		ObjectMeta: metav1.ObjectMeta{
 			GenerateName: "gitrepository-reconcile-",
 			Namespace:    "default",
@@ -178,6 +177,7 @@ func TestGitRepositoryReconciler_Reconcile(t *testing.T) {
 			URL:      server.HTTPAddress() + repoPath,
 		},
 	}
+	obj := origObj.DeepCopy()
 	g.Expect(testEnv.Create(ctx, obj)).To(Succeed())
 
 	key := client.ObjectKey{Name: obj.Name, Namespace: obj.Namespace}
@@ -191,17 +191,7 @@ func TestGitRepositoryReconciler_Reconcile(t *testing.T) {
 	}, timeout).Should(BeTrue())
 
 	// Wait for GitRepository to be Ready
-	g.Eventually(func() bool {
-		if err := testEnv.Get(ctx, key, obj); err != nil {
-			return false
-		}
-		if !conditions.IsReady(obj) || obj.Status.Artifact == nil {
-			return false
-		}
-		readyCondition := conditions.Get(obj, meta.ReadyCondition)
-		return obj.Generation == readyCondition.ObservedGeneration &&
-			obj.Generation == obj.Status.ObservedGeneration
-	}, timeout).Should(BeTrue())
+	waitForSourceReadyWithArtifact(ctx, g, obj)
 
 	// Check if the object status is valid.
 	condns := &conditionscheck.Conditions{NegativePolarity: gitRepositoryReadyCondition.NegativePolarity}
@@ -233,12 +223,11 @@ func TestGitRepositoryReconciler_Reconcile(t *testing.T) {
 	g.Expect(testEnv.Delete(ctx, obj)).To(Succeed())
 
 	// Wait for GitRepository to be deleted
-	g.Eventually(func() bool {
-		if err := testEnv.Get(ctx, key, obj); err != nil {
-			return apierrors.IsNotFound(err)
-		}
-		return false
-	}, timeout).Should(BeTrue())
+	waitForSourceDeletion(ctx, g, obj)
+
+	// Check if a suspended object gets deleted.
+	obj = origObj.DeepCopy()
+	testSuspendedObjectDeleteWithArtifact(ctx, g, obj)
 }
 
 func TestGitRepositoryReconciler_reconcileSource_authStrategy(t *testing.T) {

controllers/helmchart_controller.go

@@ -194,12 +194,6 @@ func (r *HelmChartReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 	// Record suspended status metric
 	r.RecordSuspend(ctx, obj, obj.Spec.Suspend)
 
-	// Return early if the object is suspended
-	if obj.Spec.Suspend {
-		log.Info("Reconciliation is suspended for this object")
-		return ctrl.Result{}, nil
-	}
-
 	// Initialize the patch helper with the current version of the object.
 	patchHelper, err := patch.NewHelper(obj, r.Client)
 	if err != nil {
@@ -246,6 +240,13 @@ func (r *HelmChartReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 		return
 	}
 
+	// Return if the object is suspended.
+	if obj.Spec.Suspend {
+		log.Info("Reconciliation is suspended for this object")
+		recResult, retErr = sreconcile.ResultEmpty, nil
+		return
+	}
+
 	// Reconcile actual object
 	reconcilers := []helmChartReconcileFunc{
 		r.reconcileStorage,