Fix secret cleanup issue

When max-keep-runs is exceeded, The GitHub basic auth secret
for the latest pipelineRun is deleted.

For example, if max-keep-runs is set to 1, kick off a pipelineRun.
when it's done, kick off another instance of the same pipelineRun.
When the second pipelineRun is done and pac performs a cleanup
to delete the first pipelineRun, the deletion is proper for first
pipelinerun, but it also deletes the secret associated
with the second pipeline run

Signed-off-by: Savita Ashture <sashture@redhat.com>

pkg/kubeinteraction/cleanups.go

@@ -44,9 +44,9 @@ func (k Interaction) CleanupPipelines(ctx context.Context, logger *zap.SugaredLo
 				return err
 			}
 
-			// Try to Delete the secret created for git-clone basic-auth, it should have been created with a owneref on the pipelinerun and due being deleted when the pipelinerun is deleted
-			// but in some cases of conflicts and the ownerRef not being set, the secret is not deleted and we need to delete it manually.
-			if secretName, ok := pr.GetAnnotations()[keys.GitAuthSecret]; ok {
+			// Try to Delete the secret created for git-clone basic-auth, it should have been created with a ownerRef on the pipelinerun and due being deleted when the pipelinerun is deleted
+			// but in some cases of conflicts and the ownerRef not being set, the secret is not deleted, and we need to delete it manually.
+			if secretName, ok := prun.GetAnnotations()[keys.GitAuthSecret]; ok {
 				err = k.Run.Clients.Kube.CoreV1().Secrets(repo.GetNamespace()).Delete(ctx, secretName, metav1.DeleteOptions{})
 				if err == nil {
 					logger.Infof("secret %s attached to pipelinerun %s has been deleted", secretName, prun.GetName())

pkg/kubeinteraction/cleanups_test.go

@@ -43,6 +43,7 @@ func TestCleanupPipelines(t *testing.T) {
 		kept             int
 		prunLatestInList string
 		secrets          []*corev1.Secret
+		sList            int
 	}
 
 	tests := []struct {
@@ -121,6 +122,61 @@ func TestCleanupPipelines(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "cleanup the secrets related to pipelinerun but not the other secret",
+			args: args{
+				namespace:      ns,
+				repositoryName: cleanupRepoName,
+				logSnippet:     "secret pac-gitauth-secret attached to pipelinerun pipeline-toclean has been deleted",
+				prunCurrent: &tektonv1.PipelineRun{
+					ObjectMeta: metav1.ObjectMeta{
+						Labels: cleanupLabels,
+						Annotations: map[string]string{
+							keys.OriginalPRName: cleanupPRName,
+							keys.Repository:     cleanupRepoName,
+							keys.GitAuthSecret:  "pac-gitauth-secret",
+						},
+					},
+				},
+				maxKeep: 1,
+				kept:    1,
+				pruns: []*tektonv1.PipelineRun{
+					tektontest.MakePRCompletion(clock, "pipeline-notoclean", ns, tektonv1.PipelineRunReasonSuccessful.String(), map[string]string{
+						keys.OriginalPRName: cleanupPRName,
+						keys.Repository:     cleanupRepoName,
+						keys.GitAuthSecret:  "no-cleanuppac-gitauth-secret",
+					}, cleanupLabels, 30),
+					tektontest.MakePRCompletion(clock, "pipeline-toclean", ns, tektonv1.PipelineRunReasonSuccessful.String(), map[string]string{
+						keys.OriginalPRName: cleanupPRName,
+						keys.Repository:     cleanupRepoName,
+						keys.GitAuthSecret:  "pac-gitauth-secret",
+					}, cleanupLabels, 30),
+				},
+				secrets: []*corev1.Secret{
+					{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "pac-gitauth-secret",
+							Namespace: ns,
+						},
+						Data: map[string][]byte{
+							"username": []byte("test"),
+							"password": []byte("test"),
+						},
+					},
+					{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "no-cleanuppac-gitauth-secret",
+							Namespace: ns,
+						},
+						Data: map[string][]byte{
+							"username": []byte("test"),
+							"password": []byte("test"),
+						},
+					},
+				},
+				sList: 1,
+			},
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -170,6 +226,10 @@ func TestCleanupPipelines(t *testing.T) {
 			if tt.args.logSnippet != "" {
 				assert.Assert(t, logCatcher.FilterMessageSnippet(tt.args.logSnippet).Len() > 0, logCatcher.All())
 			}
+
+			secretList, err := kint.Run.Clients.Kube.CoreV1().Secrets(tt.args.namespace).List(ctx, metav1.ListOptions{})
+			assert.NilError(t, err)
+			assert.Equal(t, len(secretList.Items), tt.args.sList)
 		})
 	}
 }

test/github_config_maxkeepruns_test.go

@@ -5,15 +5,17 @@ package test
 
 import (
 	"context"
+	"fmt"
 	"testing"
 	"time"
 
 	ghlib "github.com/google/go-github/v55/github"
-	"gotest.tools/v3/assert"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys"
 	tgithub "github.com/openshift-pipelines/pipelines-as-code/test/pkg/github"
 	twait "github.com/openshift-pipelines/pipelines-as-code/test/pkg/wait"
+	"gotest.tools/v3/assert"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"knative.dev/pkg/apis"
 )
 
 func TestGithubMaxKeepRuns(t *testing.T) {
@@ -34,7 +36,7 @@ func TestGithubMaxKeepRuns(t *testing.T) {
 	waitOpts := twait.Opts{
 		RepoName:        targetNS,
 		Namespace:       targetNS,
-		MinNumberStatus: 1,
+		MinNumberStatus: 2,
 		PollTimeout:     twait.DefaultTimeout,
 		TargetSHA:       sha,
 	}
@@ -45,6 +47,16 @@ func TestGithubMaxKeepRuns(t *testing.T) {
 	for {
 		prs, err := runcnx.Clients.Tekton.TektonV1().PipelineRuns(targetNS).List(ctx, metav1.ListOptions{})
 		if err == nil && len(prs.Items) == 1 {
+			if prs.Items[0].GetStatusCondition().GetCondition(apis.ConditionSucceeded).GetReason() == "Running" {
+				t.Log(fmt.Sprintf("skipping %s since currently running", prs.Items[0].GetName()))
+				continue
+			}
+			// making sure secret is not deleted for existing pipelinerun
+			if secretName, ok := prs.Items[0].GetAnnotations()[keys.GitAuthSecret]; ok {
+				sData, err := runcnx.Clients.Kube.CoreV1().Secrets(targetNS).Get(ctx, secretName, metav1.GetOptions{})
+				assert.NilError(t, err)
+				assert.Assert(t, sData.Name != "")
+			}
 			break
 		}
 		time.Sleep(10 * time.Second)
@@ -56,5 +68,5 @@ func TestGithubMaxKeepRuns(t *testing.T) {
 }
 
 // Local Variables:
-// compile-command: "go test -tags=e2e -v -run TestMaxKeepRuns$ ."
+// compile-command: "go test -tags=e2e -v -run ^TestGithubMaxKeepRuns$"
 // End: