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. Signed-off-by: David Moreno García <[email protected]>
konflux-ci · Aug 31, 2023 · 69ea4a0 · 69ea4a0
1 parent 3ba7874
commit 69ea4a0
Show file tree

Hide file tree

Showing 5 changed files with 80 additions and 21 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)
@@ -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 registerProcessingStatus 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,20 @@ func (a *adapter) finalizeRelease() error {
 	}
 
 	if pipelineRun != nil {
+		// The finalizer could still exist at this point in the case of the PipelineRun not having succeeded at the time
+		// of finalizing the Release.
+		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

diff --git a/controllers/release/adapter_test.go b/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())
 		})
 	})
 

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