feat(HACBS-2422): add finalizer to Release PRs

This change adds a finalizer to Release PipelineRuns to avoid their deletion by external entities until the Release service has finished processing them. Notice that tests relying on finalizeRelease don't check if the PipelineRun was successfully deleted anymore. The removal of the finalizer is not instant so to test that, we would have to use Eventually blocks, which we know are flaky. Signed-off-by: David Moreno García <[email protected]>
konflux-ci · Aug 22, 2023 · 7ae603e · 7ae603e
1 parent 2bf3743
commit 7ae603e
Show file tree

Hide file tree

Showing 5 changed files with 48 additions and 12 deletions.
diff --git a/controllers/release/adapter.go b/controllers/release/adapter.go
@@ -54,9 +54,6 @@ type adapter struct {
 	syncer  *syncer.Syncer
 }
 
-// finalizerName is the finalizer name to be added to the Releases
-const finalizerName string = "appstudio.redhat.com/release-finalizer"
-
 // newAdapter creates and returns an adapter instance.
 func newAdapter(ctx context.Context, client client.Client, release *v1alpha1.Release, loader loader.ObjectLoader, logger *logr.Logger) *adapter {
 	return &adapter{
@@ -79,13 +76,13 @@ func (a *adapter) EnsureFinalizersAreCalled() (controller.OperationResult, error
 		return controller.ContinueProcessing()
 	}
 
-	if controllerutil.ContainsFinalizer(a.release, finalizerName) {
+	if controllerutil.ContainsFinalizer(a.release, metadata.ReleaseFinalizer) {
 		if err := a.finalizeRelease(); err != nil {
 			return controller.RequeueWithError(err)
 		}
 
 		patch := client.MergeFrom(a.release.DeepCopy())
-		controllerutil.RemoveFinalizer(a.release, finalizerName)
+		controllerutil.RemoveFinalizer(a.release, metadata.ReleaseFinalizer)
 		err := a.client.Patch(a.ctx, a.release, patch)
 		if err != nil {
 			return controller.RequeueWithError(err)
@@ -100,15 +97,15 @@ func (a *adapter) EnsureFinalizersAreCalled() (controller.OperationResult, error
 func (a *adapter) EnsureFinalizerIsAdded() (controller.OperationResult, error) {
 	var finalizerFound bool
 	for _, finalizer := range a.release.GetFinalizers() {
-		if finalizer == finalizerName {
+		if finalizer == metadata.ReleaseFinalizer {
 			finalizerFound = true
 		}
 	}
 
 	if !finalizerFound {
 		a.logger.Info("Adding Finalizer to the Release")
 		patch := client.MergeFrom(a.release.DeepCopy())
-		controllerutil.AddFinalizer(a.release, finalizerName)
+		controllerutil.AddFinalizer(a.release, metadata.ReleaseFinalizer)
 		err := a.client.Patch(a.ctx, a.release, patch)
 
 		return controller.RequeueOnErrorOrContinue(err)
@@ -374,6 +371,19 @@ func (a *adapter) finalizeRelease() error {
 	}
 
 	if pipelineRun != nil {
+		// The finalizer could still exist at this point. It should be removed if the Release is finalized
+		if controllerutil.ContainsFinalizer(pipelineRun, metadata.ReleaseFinalizer) {
+			patch := client.MergeFrom(pipelineRun)
+			removedFinalizer := controllerutil.RemoveFinalizer(pipelineRun, metadata.ReleaseFinalizer)
+			if !removedFinalizer {
+				return fmt.Errorf("finalizer not removed")
+			}
+			err := a.client.Patch(a.ctx, pipelineRun, patch)
+			if err != nil {
+				return err
+			}
+		}
+
 		err = a.client.Delete(a.ctx, pipelineRun)
 		if err != nil && !errors.IsNotFound(err) {
 			return err
@@ -470,6 +480,11 @@ func (a *adapter) registerProcessingStatus(pipelineRun *v1beta1.PipelineRun) err
 			a.release.MarkReleaseFailed("Release processing failed")
 		}
 
+		// At this point it's safe to remove the PipelineRun, so the finalizer can be removed
+		if controllerutil.ContainsFinalizer(pipelineRun, metadata.ReleaseFinalizer) {
+			controllerutil.RemoveFinalizer(pipelineRun, metadata.ReleaseFinalizer)
+		}
+
 		return a.client.Status().Patch(a.ctx, a.release, patch)
 	}
 

diff --git a/controllers/release/adapter_test.go b/controllers/release/adapter_test.go
@@ -127,7 +127,7 @@ var _ = Describe("Release adapter", Ordered, func() {
 			Expect(err).NotTo(HaveOccurred())
 
 			pipelineRun, err := adapter.loader.GetReleasePipelineRun(adapter.ctx, adapter.client, adapter.release)
-			Expect(pipelineRun).To(BeNil())
+			Expect(pipelineRun).To(Or(BeNil(), HaveField("DeletionTimestamp", Not(BeNil()))))
 			Expect(err).NotTo(HaveOccurred())
 
 			_, err = adapter.loader.GetRelease(adapter.ctx, adapter.client, adapter.release.Name, adapter.release.Namespace)
@@ -151,14 +151,14 @@ var _ = Describe("Release adapter", Ordered, func() {
 			result, err := adapter.EnsureFinalizerIsAdded()
 			Expect(!result.RequeueRequest && !result.CancelRequest).To(BeTrue())
 			Expect(err).NotTo(HaveOccurred())
-			Expect(adapter.release.Finalizers).To(ContainElement(finalizerName))
+			Expect(adapter.release.Finalizers).To(ContainElement(metadata.ReleaseFinalizer))
 		})
 
 		It("shouldn't fail if the Release already has the finalizer added", func() {
 			result, err := adapter.EnsureFinalizerIsAdded()
 			Expect(!result.RequeueRequest && !result.CancelRequest).To(BeTrue())
 			Expect(err).NotTo(HaveOccurred())
-			Expect(adapter.release.Finalizers).To(ContainElement(finalizerName))
+			Expect(adapter.release.Finalizers).To(ContainElement(metadata.ReleaseFinalizer))
 
 			result, err = adapter.EnsureFinalizerIsAdded()
 			Expect(!result.RequeueRequest && !result.CancelRequest).To(BeTrue())
@@ -993,7 +993,7 @@ var _ = Describe("Release adapter", Ordered, func() {
 
 			Expect(adapter.finalizeRelease()).To(Succeed())
 			pipelineRun, err = adapter.loader.GetReleasePipelineRun(adapter.ctx, adapter.client, adapter.release)
-			Expect(pipelineRun).To(BeNil())
+			Expect(pipelineRun).To(Or(BeNil(), HaveField("DeletionTimestamp", Not(BeNil()))))
 			Expect(err).NotTo(HaveOccurred())
 		})
 	})
@@ -1191,6 +1191,20 @@ var _ = Describe("Release adapter", Ordered, func() {
 			Expect(adapter.release.HasProcessingFinished()).To(BeTrue())
 			Expect(adapter.release.IsProcessed()).To(BeFalse())
 		})
+
+		It("removes the finalizer if present", func() {
+			pipelineRun := &v1beta1.PipelineRun{
+				ObjectMeta: ctrl.ObjectMeta{
+					Finalizers: []string{metadata.ReleaseFinalizer},
+				},
+			}
+			pipelineRun.Status.MarkFailed("", "")
+			adapter.release.MarkProcessing("")
+
+			Expect(adapter.registerProcessingStatus(pipelineRun)).To(Succeed())
+			Expect(pipelineRun.Finalizers).To(BeEmpty())
+		})
+
 	})
 
 	When("calling syncResources", func() {

diff --git a/metadata/finalizers.go b/metadata/finalizers.go
@@ -0,0 +1,4 @@
+package metadata
+
+// ReleaseFinalizer is the finalizer name to be added to the Releases
+const ReleaseFinalizer string = "appstudio.redhat.com/release-finalizer"
diff --git a/tekton/pipeline_run.go b/tekton/pipeline_run.go
@@ -20,6 +20,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"os"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"strings"
 	"unicode"
 
@@ -123,9 +124,10 @@ func (r *ReleasePipelineRun) WithObjectReferences(objects ...client.Object) *Rel
 	return r
 }
 
-// WithOwner set's owner annotations to the release PipelineRun.
+// WithOwner sets owner annotations to the release PipelineRun and a finalizer to prevent its deletion.
 func (r *ReleasePipelineRun) WithOwner(release *v1alpha1.Release) *ReleasePipelineRun {
 	_ = libhandler.SetOwnerAnnotations(release, r)
+	controllerutil.AddFinalizer(r, metadata.ReleaseFinalizer)
 
 	return r
 }

diff --git a/tekton/pipeline_run_test.go b/tekton/pipeline_run_test.go
@@ -172,6 +172,7 @@ var _ = Describe("PipelineRun", func() {
 		It("can append owner release information to the object as annotations", func() {
 			releasePipelineRun.WithOwner(release)
 			Expect(releasePipelineRun.Annotations).NotTo(BeNil())
+			Expect(releasePipelineRun.Finalizers).NotTo(BeEmpty())
 		})
 
 		It("can append the release Name, Namespace, and Application to a PipelineRun object and that these label key names match the correct label format", func() {