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]>

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)
@@ -290,7 +287,20 @@ func (a *adapter) EnsureReleaseProcessingIsTracked() (controller.OperationResult
 		return controller.RequeueWithError(err)
 	}
 	if pipelineRun != nil {
-		return controller.RequeueOnErrorOrContinue(a.registerProcessingStatus(pipelineRun))
+		err = a.registerProcessingStatus(pipelineRun)
+		if err != nil {
+			return controller.RequeueWithError(err)
+		}
+	}
+
+	// This condition can only be true if the call to registerProccessingStatus changed the state
+	if a.release.HasProcessingFinished() {
+		// At this point it's safe to remove the PipelineRun, so the finalizer can be removed
+		if controllerutil.ContainsFinalizer(pipelineRun, metadata.ReleaseFinalizer) {
+			patch := client.MergeFrom(pipelineRun.DeepCopy())
+			controllerutil.RemoveFinalizer(pipelineRun, metadata.ReleaseFinalizer)
+			return controller.RequeueOnErrorOrContinue(a.client.Patch(a.ctx, pipelineRun, patch))
+		}
 	}
 
 	return controller.ContinueProcessing()
@@ -374,6 +384,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.DeepCopy())
+			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

controllers/release/adapter_test.go

@@ -20,21 +20,18 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
-	"reflect"
-	"strings"
-
-	toolkit "github.com/redhat-appstudio/operator-toolkit/loader"
-
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
 	"github.com/operator-framework/operator-lib/handler"
+	toolkit "github.com/redhat-appstudio/operator-toolkit/loader"
 	"github.com/redhat-appstudio/release-service/api/v1alpha1"
 	"github.com/redhat-appstudio/release-service/loader"
 	"github.com/redhat-appstudio/release-service/metadata"
 	"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/types"
-
-	. "github.com/onsi/ginkgo/v2"
-	. "github.com/onsi/gomega"
+	"reflect"
+	"strings"
 
 	ecapiv1alpha1 "github.com/enterprise-contract/enterprise-contract-controller/api/v1alpha1"
 	applicationapiv1alpha1 "github.com/redhat-appstudio/application-api/api/v1alpha1"
@@ -127,7 +124,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 +148,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())
@@ -782,6 +779,37 @@ var _ = Describe("Release adapter", Ordered, func() {
 			Expect(adapter.release.HasProcessingFinished()).To(BeTrue())
 		})
 
+		It("removes the finalizer if present", func() {
+			adapter.release.MarkProcessing("")
+
+			pipelineRun := &v1beta1.PipelineRun{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:       "pipeline-run",
+					Namespace:  "default",
+					Finalizers: []string{metadata.ReleaseFinalizer},
+				},
+			}
+			// The resource needs to be created as it will get patched
+			Expect(adapter.client.Create(adapter.ctx, pipelineRun)).To(Succeed())
+
+			pipelineRun.Status.MarkSucceeded("", "")
+
+			adapter.ctx = toolkit.GetMockedContext(ctx, []toolkit.MockData{
+				{
+					ContextKey: loader.ReleasePipelineRunContextKey,
+					Resource:   pipelineRun,
+				},
+			})
+
+			result, err := adapter.EnsureReleaseProcessingIsTracked()
+			Expect(!result.RequeueRequest && !result.CancelRequest).To(BeTrue())
+			Expect(err).NotTo(HaveOccurred())
+			Expect(pipelineRun.Finalizers).To(BeEmpty())
+
+			// Clean up at the end
+			Expect(adapter.client.Delete(adapter.ctx, pipelineRun)).To(Succeed())
+		})
+
 		It("should continue if the PipelineRun doesn't exist", func() {
 			adapter.release.MarkProcessing("")
 
@@ -993,8 +1021,8 @@ 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(err).NotTo(HaveOccurred())
+			Expect(pipelineRun).To(BeNil())
 		})
 	})
 
@@ -1191,6 +1219,7 @@ var _ = Describe("Release adapter", Ordered, func() {
 			Expect(adapter.release.HasProcessingFinished()).To(BeTrue())
 			Expect(adapter.release.IsProcessed()).To(BeFalse())
 		})
+
 	})
 
 	When("calling syncResources", func() {

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"

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
 }

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() {