From 69ea4a086364aa1903f93d88838451f794bc3389 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Moreno=20Garc=C3=ADa?= Date: Wed, 16 Aug 2023 12:28:35 +0200 Subject: [PATCH] feat(HACBS-2422): add finalizer to Release PRs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- controllers/release/adapter.go | 40 +++++++++++++++++----- controllers/release/adapter_test.go | 52 ++++++++++++++++++++++------- metadata/finalizers.go | 4 +++ tekton/pipeline_run.go | 4 ++- tekton/pipeline_run_test.go | 1 + 5 files changed, 80 insertions(+), 21 deletions(-) create mode 100644 metadata/finalizers.go diff --git a/controllers/release/adapter.go b/controllers/release/adapter.go index e480dd31..9dd4c293 100644 --- 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,7 +97,7 @@ 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 } } @@ -108,7 +105,7 @@ func (a *adapter) EnsureFinalizerIsAdded() (controller.OperationResult, error) { 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 index 8edf0f57..f83e9a6e 100644 --- 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 new file mode 100644 index 00000000..bde23204 --- /dev/null +++ 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 index d19667ea..6ff8bcad 100644 --- 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 index ca11eeda..d242e3d7 100644 --- 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() {