Do not recreate pipeline service account on component deletetion (#344)

Do not recreate pipeline service account on component deletetion

Author: mmorhun

controllers/component_build_controller.go

@@ -182,12 +182,6 @@ func (r *ComponentBuildReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 		return ctrl.Result{}, err
 	}
 
-	// Ensure pipeline service account exists
-	_, err = r.ensurePipelineServiceAccount(ctx, component.Namespace)
-	if err != nil {
-		return ctrl.Result{}, err
-	}
-
 	if getContainerImageRepositoryForComponent(&component) == "" {
 		// Container image must be set. It's not possible to proceed without it.
 		log.Info("Waiting for ContainerImage to be set")
@@ -244,6 +238,12 @@ func (r *ComponentBuildReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 		return ctrl.Result{}, nil
 	}
 
+	// Ensure pipeline service account exists
+	_, err = r.ensurePipelineServiceAccount(ctx, component.Namespace)
+	if err != nil {
+		return ctrl.Result{}, err
+	}
+
 	_, err = r.GetBuildPipelineFromComponentAnnotation(ctx, &component)
 	if err != nil {
 		buildStatus := readBuildStatus(&component)

controllers/component_build_controller_pac_repository.go

@@ -22,7 +22,6 @@ import (
 	"net/url"
 	"strings"
 
-	"github.com/go-logr/logr"
 	appstudiov1alpha1 "github.com/konflux-ci/application-api/api/v1alpha1"
 	pacv1alpha1 "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1"
 	corev1 "k8s.io/api/core/v1"
@@ -103,7 +102,7 @@ func (r *ComponentBuildReconciler) ensurePaCRepository(ctx context.Context, comp
 		return err
 	}
 	if val, ok := ns.Labels[appstudioWorkspaceNameLabel]; ok {
-		pacRepoAddParamWorkspaceName(log, repository, val)
+		pacRepoAddParamWorkspaceName(repository, val)
 	}
 
 	existingRepository := &pacv1alpha1.Repository{}
@@ -153,7 +152,7 @@ func (r *ComponentBuildReconciler) getNamespace(ctx context.Context, name string
 
 // pacRepoAddParamWorkspaceName adds custom parameter workspace name to a PaC repository.
 // Existing parameter will be overridden.
-func pacRepoAddParamWorkspaceName(log logr.Logger, repository *pacv1alpha1.Repository, workspaceName string) {
+func pacRepoAddParamWorkspaceName(repository *pacv1alpha1.Repository, workspaceName string) {
 	var params []pacv1alpha1.Params
 	// Before pipelines-as-code gets upgraded for application-service to the
 	// version supporting custom parameters, this check must be taken.

controllers/component_build_controller_pipeline.go

@@ -467,10 +467,6 @@ func getContainerImageRepositoryForComponent(component *appstudiov1alpha1.Compon
 	if component.Spec.ContainerImage != "" {
 		return getContainerImageRepository(component.Spec.ContainerImage)
 	}
-	imageRepo, _, err := getComponentImageRepoAndSecretNameFromImageAnnotation(component)
-	if err == nil && imageRepo != "" {
-		return imageRepo
-	}
 	return ""
 }
 
@@ -484,25 +480,6 @@ func getContainerImageRepository(image string) string {
 	return strings.Split(image, ":")[0]
 }
 
-// getComponentImageRepoAndSecretNameFromImageAnnotation parses image.redhat.com/image annotation
-// for image repository and secret name to access it.
-// If image.redhat.com/image is not set, the procedure returns empty values.
-func getComponentImageRepoAndSecretNameFromImageAnnotation(component *appstudiov1alpha1.Component) (string, string, error) {
-	type RepositoryInfo struct {
-		Image  string `json:"image"`
-		Secret string `json:"secret"`
-	}
-
-	var repoInfo RepositoryInfo
-	if imageRepoDataJson, exists := component.Annotations[ImageRepoAnnotationName]; exists {
-		if err := json.Unmarshal([]byte(imageRepoDataJson), &repoInfo); err != nil {
-			return "", "", boerrors.NewBuildOpError(boerrors.EFailedToParseImageAnnotation, err)
-		}
-		return repoInfo.Image, repoInfo.Secret, nil
-	}
-	return "", "", nil
-}
-
 func getPipelineNameAndBundle(pipelineRef *tektonapi.PipelineRef) (string, string, error) {
 	if pipelineRef.Resolver != "" && pipelineRef.Resolver != "bundles" {
 		return "", "", boerrors.NewBuildOpError(

controllers/component_build_controller_test.go

@@ -522,18 +522,6 @@ var _ = Describe("Component initial build controller", func() {
 			expectPacBuildStatus(resourcePacPrepKey, "enabled", 0, "", mergeUrl)
 		})
 
-		It("should not copy PaC secret into local namespace if GitHub application is used", func() {
-			deleteSecret(namespacePaCSecretKey)
-
-			createComponentAndProcessBuildRequest(componentConfig{
-				componentKey: resourcePacPrepKey,
-				annotations:  defaultPipelineAnnotations,
-			}, BuildRequestConfigurePaCAnnotationValue)
-			waitPaCRepositoryCreated(resourcePacPrepKey)
-
-			ensureSecretNotCreated(namespacePaCSecretKey)
-		})
-
 		It("should successfully submit PR with PaC definitions using token", func() {
 			isCreatePaCPullRequestInvoked := false
 			EnsurePaCMergeRequestFunc = func(repoUrl string, d *gp.MergeRequestData) (string, error) {
@@ -1167,6 +1155,40 @@ var _ = Describe("Component initial build controller", func() {
 			expectError := boerrors.NewBuildOpError(boerrors.EUnknownGitProvider, nil)
 			expectPacBuildStatus(resourceCleanupKey, "error", expectError.GetErrorId(), expectError.ShortError(), "")
 		})
+
+		It("should not attempt to create service account on component deletion", func() {
+			pacSecretData := map[string]string{
+				"github-application-id": "12345",
+				"github-private-key":    githubAppPrivateKey,
+			}
+			createSecret(pacSecretKey, pacSecretData)
+			createComponentAndProcessBuildRequest(componentConfig{
+				componentKey: resourceCleanupKey,
+				annotations:  defaultPipelineAnnotations,
+			}, BuildRequestConfigurePaCAnnotationValue)
+			waitPaCFinalizerOnComponent(resourceCleanupKey)
+
+			waitPipelineServiceAccount(resourceCleanupKey.Namespace)
+
+			// Make sure that proper cleanup was invoked
+			isRemovePaCPullRequestInvoked := false
+			UndoPaCMergeRequestFunc = func(repoUrl string, d *gp.MergeRequestData) (webUrl string, err error) {
+				isRemovePaCPullRequestInvoked = true
+				return "merge-url", nil
+			}
+
+			deletePipelineServiceAccount(resourceCleanupKey.Namespace)
+
+			Expect(isRemovePaCPullRequestInvoked).To(BeFalse())
+			// Clean up for the component should not recreate pipeline service account
+			deleteComponent(resourceCleanupKey)
+			Expect(isRemovePaCPullRequestInvoked).To(BeTrue())
+
+			pipelineServiceAccountKey := types.NamespacedName{Name: buildPipelineServiceAccountName, Namespace: resourceCleanupKey.Namespace}
+			pipelineServiceAccount := &corev1.ServiceAccount{}
+			err := k8sClient.Get(ctx, pipelineServiceAccountKey, pipelineServiceAccount)
+			Expect(k8sErrors.IsNotFound(err)).To(BeTrue())
+		})
 	})
 
 	Context("Test Pipelines as Code multi component git repository", func() {

controllers/component_build_controller_unit_test.go

@@ -1190,7 +1190,7 @@ func TestPaCRepoAddParamWorkspace(t *testing.T) {
 
 	t.Run("add to Spec.Params", func(t *testing.T) {
 		repository, _ := generatePACRepository(*component, secret)
-		pacRepoAddParamWorkspaceName(log, repository, workspaceName)
+		pacRepoAddParamWorkspaceName(repository, workspaceName)
 
 		params := convertCustomParamsToMap(repository)
 		param, ok := params[pacCustomParamAppstudioWorkspace]
@@ -1210,7 +1210,7 @@ func TestPaCRepoAddParamWorkspace(t *testing.T) {
 		}
 		repository.Spec.Params = &params
 
-		pacRepoAddParamWorkspaceName(log, repository, workspaceName)
+		pacRepoAddParamWorkspaceName(repository, workspaceName)
 
 		existingParams := convertCustomParamsToMap(repository)
 		param, ok := existingParams[pacCustomParamAppstudioWorkspace]

controllers/suite_util_test.go

@@ -438,14 +438,6 @@ func waitSecretCreated(resourceKey types.NamespacedName) {
 	}, timeout, interval).Should(BeTrue())
 }
 
-func ensureSecretNotCreated(resourceKey types.NamespacedName) {
-	secret := &corev1.Secret{}
-	Consistently(func() bool {
-		err := k8sClient.Get(ctx, resourceKey, secret)
-		return k8sErrors.IsNotFound(err)
-	}, timeout, interval).WithTimeout(ensureTimeout).Should(BeTrue())
-}
-
 func waitSecretGone(resourceKey types.NamespacedName) {
 	secret := &corev1.Secret{}
 	Eventually(func() bool {
@@ -599,3 +591,45 @@ func deleteBuildPipelineConfigMap(configMapKey types.NamespacedName) {
 		Fail(err.Error())
 	}
 }
+
+func waitServiceAccount(serviceAccountKey types.NamespacedName) {
+	serviceAccount := &corev1.ServiceAccount{}
+	Eventually(func() bool {
+		if err := k8sClient.Get(ctx, serviceAccountKey, serviceAccount); err != nil {
+			return false
+		}
+		return serviceAccount.ResourceVersion != ""
+	}, timeout, interval).Should(BeTrue())
+}
+
+func deleteServiceAccount(serviceAccountKey types.NamespacedName) {
+	serviceAccount := &corev1.ServiceAccount{}
+
+	// Check if the SA exists
+	if err := k8sClient.Get(ctx, serviceAccountKey, serviceAccount); k8sErrors.IsNotFound(err) {
+		return
+	}
+
+	// Delete
+	Eventually(func() error {
+		if err := k8sClient.Get(ctx, serviceAccountKey, serviceAccount); err != nil {
+			return err
+		}
+		return k8sClient.Delete(ctx, serviceAccount)
+	}, timeout, interval).Should(Succeed())
+
+	// Wait for the deletion to finish
+	Eventually(func() bool {
+		return k8sErrors.IsNotFound(k8sClient.Get(ctx, serviceAccountKey, serviceAccount))
+	}, timeout, interval).Should(BeTrue())
+}
+
+func waitPipelineServiceAccount(namespace string) {
+	pipelineServiceAccountKey := types.NamespacedName{Name: buildPipelineServiceAccountName, Namespace: namespace}
+	waitServiceAccount(pipelineServiceAccountKey)
+}
+
+func deletePipelineServiceAccount(namespace string) {
+	pipelineServiceAccountkey := types.NamespacedName{Name: buildPipelineServiceAccountName, Namespace: namespace}
+	deleteServiceAccount((pipelineServiceAccountkey))
+}