Updates basic auth secret owner reference with pipelinerun

this updates the created basic auth secret owner ref with pipelinerun
so that they get cleanedup with pipelinerun

also removes the secret deletion logic from reconciler and lets ownerref
do its magic.

Signed-off-by: Shivam Mukhade <[email protected]>

a

Author: Shivam Mukhade

config/201-controller-role.yaml

@@ -67,7 +67,7 @@ rules:
     verbs: ["create"]
   - apiGroups: [""]
     resources: ["secrets"]
-    verbs: ["get", "create"]
+    verbs: ["get", "create", "update"]
   - apiGroups: ["pipelinesascode.tekton.dev"]
     resources: ["repositories"]
     verbs: ["create", "list"]

pkg/kubeinteraction/kubeinteraction.go

@@ -15,7 +15,7 @@ import (
 type Interface interface {
 	CleanupPipelines(context.Context, *zap.SugaredLogger, *v1alpha1.Repository, *v1beta1.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, *v1beta1.PipelineRun) error
 	GetSecret(context.Context, ktypes.GetSecretOpt) (string, error)
 	GetPodLogs(context.Context, string, string, string, int64) (string, error)
 }

pkg/kubeinteraction/secrets.go

@@ -2,12 +2,15 @@ package kubeinteraction
 
 import (
 	"context"
+	"fmt"
 
 	ktypes "github.com/openshift-pipelines/pipelines-as-code/pkg/secrets/types"
+	"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
 	"go.uber.org/zap"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/client-go/util/retry"
 )
 
 func (k Interaction) GetSecret(ctx context.Context, secretopt ktypes.GetSecretOpt) (string, error) {
@@ -25,8 +28,38 @@ func (k Interaction) DeleteSecret(ctx context.Context, logger *zap.SugaredLogger
 	if err != nil && !errors.IsNotFound(err) {
 		return err
 	}
+	return nil
+}
+
+// UpdateSecretWithOwnerRef updates the secret with ownerReference
+func (k Interaction) UpdateSecretWithOwnerRef(ctx context.Context, logger *zap.SugaredLogger, targetNamespace, secretName string, pr *v1beta1.PipelineRun) error {
+	controllerOwned := false
+	ownerRef := &metav1.OwnerReference{
+		APIVersion:         pr.GetGroupVersionKind().GroupVersion().String(),
+		Kind:               pr.GetGroupVersionKind().Kind,
+		Name:               pr.GetName(),
+		UID:                pr.GetUID(),
+		BlockOwnerDeletion: &controllerOwned,
+		Controller:         &controllerOwned,
+	}
+	err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
+		secret, err := k.Run.Clients.Kube.CoreV1().Secrets(targetNamespace).Get(ctx, secretName, metav1.GetOptions{})
+		if err != nil {
+			return err
+		}
 
-	logger.Infof("Secret %s has been deleted in namespace %s", secretName, targetNamespace)
+		secret.OwnerReferences = []metav1.OwnerReference{*ownerRef}
+
+		_, err = k.Run.Clients.Kube.CoreV1().Secrets(targetNamespace).Update(ctx, secret, metav1.UpdateOptions{})
+		if err != nil {
+			logger.Infof("failed to update secret, retrying  %v/%v: %v", targetNamespace, secretName, err)
+			return err
+		}
+		return nil
+	})
+	if err != nil {
+		return fmt.Errorf("failed to update secret with ownerRef %v/%v: %w", targetNamespace, secretName, err)
+	}
 	return nil
 }
 

pkg/kubeinteraction/secrets_test.go

@@ -6,6 +6,7 @@ import (
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/params"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/params/clients"
 	testclient "github.com/openshift-pipelines/pipelines-as-code/pkg/test/clients"
+	"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
 	"go.uber.org/zap"
 	zapobserver "go.uber.org/zap/zaptest/observer"
 	"gotest.tools/v3/assert"
@@ -66,3 +67,44 @@ func TestDeleteSecret(t *testing.T) {
 		})
 	}
 }
+
+func TestUpdateSecretWithOwnerRef(t *testing.T) {
+	testNs := "there"
+	secrete := "dont-tell-anyone-its-a-secrete"
+	tdata := testclient.Data{
+		Secret: []*corev1.Secret{
+			{
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: testNs,
+					Name:      secrete,
+				},
+			},
+		},
+	}
+	ctx, _ := rtesting.SetupFakeContext(t)
+	stdata, _ := testclient.SeedTestData(t, ctx, tdata)
+	observer, _ := zapobserver.New(zap.InfoLevel)
+	fakelogger := zap.New(observer).Sugar()
+	kint := Interaction{
+		Run: &params.Run{
+			Clients: clients.Clients{
+				Kube: stdata.Kube,
+			},
+		},
+	}
+	pr := &v1beta1.PipelineRun{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "test",
+			UID:  "uid",
+		},
+	}
+
+	err := kint.UpdateSecretWithOwnerRef(ctx, fakelogger, testNs, secrete, pr)
+	assert.NilError(t, err)
+
+	updatedSecret, err := stdata.Kube.CoreV1().Secrets(testNs).Get(ctx, secrete, metav1.GetOptions{})
+	assert.NilError(t, err)
+	assert.Assert(t, len(updatedSecret.OwnerReferences) != 0)
+	assert.Equal(t, updatedSecret.OwnerReferences[0].Kind, "PipelineRun")
+	assert.Equal(t, updatedSecret.OwnerReferences[0].Name, pr.Name)
+}

pkg/pipelineascode/pipelineascode.go

@@ -146,7 +146,14 @@ func (p *PacRun) startPR(ctx context.Context, match matcher.Match) error {
 
 	// Patch pipelineRun with logURL annotation, skips for GitHub App as we patch logURL while patching checkrunID
 	if _, ok := pr.Annotations[keys.InstallationID]; !ok {
-		return patchPipelineRunWithLogURL(ctx, p.logger, p.run.Clients, pr)
+		if err := patchPipelineRunWithLogURL(ctx, p.logger, p.run.Clients, pr); err != nil {
+			return err
+		}
+	}
+
+	// update ownerRef of secret with pipelineRun, so that it gets cleanedUp with pipelineRun
+	if p.run.Info.Pac.SecretAutoCreation {
+		return p.k8int.UpdateSecretWithOwnerRef(ctx, p.logger, pr.Namespace, gitAuthSecretName, pr)
 	}
 	return nil
 }

pkg/pipelineascode/pipelinesascode_github_test.go

@@ -14,6 +14,7 @@ import (
 
 	"github.com/google/go-github/v48/github"
 	apipac "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode"
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/consoleui"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/params"
@@ -496,6 +497,13 @@ func TestRun(t *testing.T) {
 						logger.Fatalf("failed to find log-url label on pipelinerun: %v/%v", pr.GetNamespace(), pr.GetName())
 					}
 					assert.Equal(t, logURL, cs.Clients.ConsoleUI.DetailURL(pr.Namespace, pr.Name))
+
+					if cs.Info.Pac.SecretAutoCreation {
+						secretName := pr.GetAnnotations()[keys.GitAuthSecret]
+						secret, err := cs.Clients.Kube.CoreV1().Secrets(pr.Namespace).Get(ctx, secretName, metav1.GetOptions{})
+						assert.NilError(t, err)
+						assert.Assert(t, len(secret.OwnerReferences) != 0)
+					}
 				}
 			}
 		})

pkg/reconciler/cleanup.go

@@ -2,7 +2,6 @@ package reconciler
 
 import (
 	"context"
-	"fmt"
 	"strconv"
 
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys"
@@ -11,21 +10,6 @@ import (
 	"go.uber.org/zap"
 )
 
-func (r *Reconciler) cleanupSecrets(ctx context.Context, logger *zap.SugaredLogger, repo *v1alpha1.Repository, pr *v1beta1.PipelineRun) error {
-	var gitAuthSecretName string
-	if annotation, ok := pr.Annotations[keys.GitAuthSecret]; ok {
-		gitAuthSecretName = annotation
-	} else {
-		return fmt.Errorf("cannot get annotation %s as set on PR", keys.GitAuthSecret)
-	}
-
-	err := r.kinteract.DeleteSecret(ctx, logger, repo.GetNamespace(), gitAuthSecretName)
-	if err != nil {
-		return fmt.Errorf("deleting basic auth secret has failed: %w ", err)
-	}
-	return nil
-}
-
 func (r *Reconciler) cleanupPipelineRuns(ctx context.Context, logger *zap.SugaredLogger, repo *v1alpha1.Repository, pr *v1beta1.PipelineRun) error {
 	keepMaxPipeline, ok := pr.Annotations[keys.MaxKeepRuns]
 	if ok {

pkg/reconciler/reconciler.go

@@ -151,12 +151,6 @@ func (r *Reconciler) reportFinalStatus(ctx context.Context, logger *zap.SugaredL
 		return repo, fmt.Errorf("cannot clean prs: %w", err)
 	}
 
-	if r.run.Info.Pac.SecretAutoCreation {
-		if err := r.cleanupSecrets(ctx, logger, repo, pr); err != nil {
-			return repo, fmt.Errorf("cannot clean secret: %w", err)
-		}
-	}
-
 	newPr, err := r.postFinalStatus(ctx, logger, provider, event, pr)
 	if err != nil {
 		return repo, fmt.Errorf("cannot post final status: %w", err)

pkg/reconciler/reconciler_test.go

@@ -191,10 +191,6 @@ func TestReconciler_ReconcileKind(t *testing.T) {
 			got, err := stdata.Pipeline.TektonV1beta1().PipelineRuns(pr.Namespace).Get(ctx, pr.Name, metav1.GetOptions{})
 			assert.NilError(t, err)
 
-			// make sure secret is deleted
-			_, err = stdata.Kube.CoreV1().Secrets(testRepo.Namespace).Get(ctx, secretName, metav1.GetOptions{})
-			assert.Error(t, err, fmt.Sprintf("secrets \"%s\" not found", secretName))
-
 			// state must be updated to completed
 			assert.Equal(t, got.Labels[keys.State], kubeinteraction.StateCompleted)
 		})

pkg/test/kubernetestint/kubernetesint.go

@@ -36,6 +36,10 @@ func (k *KinterfaceTest) GetPodLogs(_ context.Context, ns, pod, cont string, _ i
 	return "", nil
 }
 
+func (k *KinterfaceTest) UpdateSecretWithOwnerRef(_ context.Context, _ *zap.SugaredLogger, _, _ string, _ *v1beta1.PipelineRun) error {
+	return nil
+}
+
 func (k *KinterfaceTest) GetSecret(ctx context.Context, secret ktypes.GetSecretOpt) (string, error) {
 	// check if secret exist in k.GetSecretResult
 	if k.GetSecretResult[secret.Name] == "" {