From 3723cde09f10309f21be6f03d8bc5bd3dbef4d35 Mon Sep 17 00:00:00 2001
From: Emil Natan <ena@redhat.com>
Date: Mon, 23 Sep 2024 19:44:53 +0300
Subject: [PATCH] Fix gitauth secrets cleanup

The gitauth secrets are created before the pipelineRun and deleted
through ownerRef when the pipelineRun is deleted. This fixes the issue
where the secrets are left in the namespace if the pipelineRun creation
fails, hitting the secrets quota and blocking subsequent pipelineRuns.
---
 pkg/kubeinteraction/kubeinteraction.go |  1 +
 pkg/pipelineascode/pipelineascode.go   | 28 +++++++++++++++++---------
 2 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/pkg/kubeinteraction/kubeinteraction.go b/pkg/kubeinteraction/kubeinteraction.go
index 2cac142b7..2d19176e2 100644
--- a/pkg/kubeinteraction/kubeinteraction.go
+++ b/pkg/kubeinteraction/kubeinteraction.go
@@ -14,6 +14,7 @@ import (
 type Interface interface {
 	CleanupPipelines(context.Context, *zap.SugaredLogger, *v1alpha1.Repository, *pipelinev1.PipelineRun, int) error
 	CreateSecret(ctx context.Context, ns string, secret *corev1.Secret) error
+	DeleteSecret(context.Context, *zap.SugaredLogger, string, string) error
 	UpdateSecretWithOwnerRef(context.Context, *zap.SugaredLogger, string, string, *pipelinev1.PipelineRun) error
 	GetSecret(context.Context, ktypes.GetSecretOpt) (string, error)
 	GetPodLogs(context.Context, string, string, string, int64) (string, error)
diff --git a/pkg/pipelineascode/pipelineascode.go b/pkg/pipelineascode/pipelineascode.go
index 13bdee2fc..0c7fd0544 100644
--- a/pkg/pipelineascode/pipelineascode.go
+++ b/pkg/pipelineascode/pipelineascode.go
@@ -176,15 +176,32 @@ func (p *PacRun) startPR(ctx context.Context, match matcher.Match) (*tektonv1.Pi
 		match.PipelineRun.Annotations[keys.State] = kubeinteraction.StateQueued
 	}
 
-	// Create the actual pipeline
+	// Create the actual pipelineRun
 	pr, err := p.run.Clients.Tekton.TektonV1().PipelineRuns(match.Repo.GetNamespace()).Create(ctx,
 		match.PipelineRun, metav1.CreateOptions{})
 	if err != nil {
+		// cleanup the gitauth secret because ownerRef isn't set when the pipelineRun creation failed
+		if p.pacInfo.SecretAutoCreation {
+			if errDelSec := p.k8int.DeleteSecret(ctx, p.logger, match.Repo.GetNamespace(), gitAuthSecretName); errDelSec != nil {
+				// don't overshadow the pipelineRun creation error, just log
+				p.logger.Errorf("removing auto created secret: %s in namespace %s has failed: %w ", gitAuthSecretName, match.Repo.GetNamespace(), errDelSec)
+			}
+		}
 		// we need to make difference between markdown error and normal error that goes to namespace/controller stream
 		return nil, fmt.Errorf("creating pipelinerun %s in namespace %s has failed.\n\nTekton Controller has reported this error: ```%w``` ", match.PipelineRun.GetGenerateName(),
 			match.Repo.GetNamespace(), err)
 	}
 
+	// update ownerRef of secret with pipelineRun, so that it gets cleanedUp with pipelineRun
+	if p.pacInfo.SecretAutoCreation {
+		err := p.k8int.UpdateSecretWithOwnerRef(ctx, p.logger, pr.Namespace, gitAuthSecretName, pr)
+		if err != nil {
+			// we still return the created PR with error, and allow caller to decide what to do with the PR, and avoid
+			// unneeded SIGSEGV's
+			return pr, fmt.Errorf("cannot update pipelinerun %s with ownerRef: %w", pr.GetGenerateName(), err)
+		}
+	}
+
 	// Create status with the log url
 	p.logger.Infof("pipelinerun %s has been created in namespace %s for SHA: %s Target Branch: %s",
 		pr.GetName(), match.Repo.GetNamespace(), p.event.SHA, p.event.BaseBranch)
@@ -236,15 +253,6 @@ func (p *PacRun) startPR(ctx context.Context, match matcher.Match) (*tektonv1.Pi
 		}
 	}
 
-	// update ownerRef of secret with pipelineRun, so that it gets cleanedUp with pipelineRun
-	if p.pacInfo.SecretAutoCreation {
-		err := p.k8int.UpdateSecretWithOwnerRef(ctx, p.logger, pr.Namespace, gitAuthSecretName, pr)
-		if err != nil {
-			// we still return the created PR with error, and allow caller to decide what to do with the PR, and avoid
-			// unneeded SIGSEGV's
-			return pr, fmt.Errorf("cannot update pipelinerun %s with ownerRef: %w", pr.GetGenerateName(), err)
-		}
-	}
 	return pr, nil
 }