Skip to content

Commit

Permalink
Merge pull request #322 from rcerven/remove_linking
Browse files Browse the repository at this point in the history 
remove secrets linking and unlinking code based on finalizer image-re…
  • Loading branch information
@rcerven
rcerven authored Jul 10, 2024
2 parents ace4167 + 71aeefb commit 2055ba3
Show file tree
Hide file tree
Showing 4 changed files with 7 additions and 323 deletions.
59 changes: 7 additions & 52 deletions controllers/component_build_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import (
"fmt"
"time"

corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
Expand Down Expand Up @@ -184,6 +183,12 @@ 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")
Expand All @@ -206,21 +211,8 @@ func (r *ComponentBuildReconciler) Reconcile(ctx context.Context, req ctrl.Reque
// remove component from metrics map
delete(bometrics.ComponentTimesForMetrics, componentIdForMetrics)

// can be removed in the future, just keeping it for backwards compatibility
if controllerutil.ContainsFinalizer(&component, ImageRegistrySecretLinkFinalizer) {
pipelineSA := &corev1.ServiceAccount{}
err := r.Client.Get(ctx, types.NamespacedName{Name: buildPipelineServiceAccountName, Namespace: req.Namespace}, pipelineSA)
if err != nil && !errors.IsNotFound(err) {
log.Error(err, fmt.Sprintf("Failed to read service account %s in namespace %s", buildPipelineServiceAccountName, req.Namespace), l.Action, l.ActionView)
return ctrl.Result{}, err
}
if err == nil { // If pipeline service account found, unlink the secret from it
if _, generatedImageRepoSecretName, err := getComponentImageRepoAndSecretNameFromImageAnnotation(&component); err == nil {
if _, err := r.unlinkSecretFromServiceAccount(ctx, generatedImageRepoSecretName, pipelineSA.Name, pipelineSA.Namespace); err != nil {
return ctrl.Result{}, err
}
}
}

if err := r.Client.Get(ctx, req.NamespacedName, &component); err != nil {
log.Error(err, "failed to get Component", l.Action, l.ActionView)
return ctrl.Result{}, err
Expand Down Expand Up @@ -296,43 +288,6 @@ func (r *ComponentBuildReconciler) Reconcile(ctx context.Context, req ctrl.Reque
return ctrl.Result{}, nil
}

// Ensure pipeline service account exists
pipelineSA, err := r.ensurePipelineServiceAccount(ctx, component.Namespace)
if err != nil {
return ctrl.Result{}, err
}

// Link auto generated image registry secret in case of auto generated image repository is used.
if !controllerutil.ContainsFinalizer(&component, ImageRegistrySecretLinkFinalizer) {
imageRepoGenerated, imageRepoSecretName, err := getComponentImageRepoAndSecretNameFromImageAnnotation(&component)
if err != nil {
return ctrl.Result{}, err
}

// Check if the generated image is used
if imageRepoGenerated != "" && (component.Spec.ContainerImage == "" || imageRepoGenerated == getContainerImageRepository(component.Spec.ContainerImage)) {
_, err = r.linkSecretToServiceAccount(ctx, imageRepoSecretName, pipelineSA.Name, pipelineSA.Namespace, true)
if err != nil {
return ctrl.Result{}, err
}

// Ensure finalizer exists to clean up image registry secret link on component deletion
if component.ObjectMeta.DeletionTimestamp.IsZero() {
if !controllerutil.ContainsFinalizer(&component, ImageRegistrySecretLinkFinalizer) {
if err := r.Client.Get(ctx, req.NamespacedName, &component); err != nil {
log.Error(err, "failed to get Component", l.Action, l.ActionView)
return ctrl.Result{}, err
}
controllerutil.AddFinalizer(&component, ImageRegistrySecretLinkFinalizer)
if err := r.Client.Update(ctx, &component); err != nil {
return ctrl.Result{}, err
}
log.Info("Image registry secret service account link finalizer added", l.Action, l.ActionUpdate)
}
}
}
}

requestedAction, requestedActionExists := component.Annotations[BuildRequestAnnotationName]
if !requestedActionExists {
if _, statusExists := component.Annotations[BuildStatusAnnotationName]; statusExists {
Expand Down
114 changes: 0 additions & 114 deletions controllers/component_build_controller_common.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -211,120 +211,6 @@ func (r *ComponentBuildReconciler) ensurePipelineServiceAccount(ctx context.Cont
return pipelinesServiceAccount, nil
}

func (r *ComponentBuildReconciler) linkSecretToServiceAccount(ctx context.Context, secretName, serviceAccountName, namespace string, isPullSecret bool) (bool, error) {
log := ctrllog.FromContext(ctx)

if secretName == "" {
// The secret is empty, no updates needed
return false, nil
}

serviceAccount := &corev1.ServiceAccount{}
err := r.Client.Get(ctx, types.NamespacedName{Name: serviceAccountName, Namespace: namespace}, serviceAccount)
if err != nil {
if errors.IsNotFound(err) {
return false, nil
}
log.Error(err, fmt.Sprintf("Failed to read service account %s in namespace %s", serviceAccountName, namespace), l.Action, l.ActionView)
return false, err
}

isNewSecretLinked := false

shouldAddLink := true
for _, secret := range serviceAccount.Secrets {
if secret.Name == secretName {
// The secret is present in the service account, no updates needed
shouldAddLink = false
break
}
}
if shouldAddLink {
serviceAccount.Secrets = append(serviceAccount.Secrets, corev1.ObjectReference{Name: secretName, Namespace: namespace})
}
isNewSecretLinked = shouldAddLink

if isPullSecret {
shouldAddLink = true
for _, secret := range serviceAccount.ImagePullSecrets {
if secret.Name == secretName {
// The secret is present in the service account, no updates needed
shouldAddLink = false
break
}
}
if shouldAddLink {
serviceAccount.ImagePullSecrets = append(serviceAccount.ImagePullSecrets, corev1.LocalObjectReference{Name: secretName})
}
isNewSecretLinked = isNewSecretLinked || shouldAddLink
}

// Update service account if needed
if isNewSecretLinked {
err := r.Client.Update(ctx, serviceAccount)
if err != nil {
log.Error(err, fmt.Sprintf("Unable to update service account %s", serviceAccount.Name), l.Action, l.ActionUpdate)
return false, err
}
log.Info(fmt.Sprintf("Service Account %s updated with secret %s", serviceAccount.Name, secretName), l.Action, l.ActionUpdate)
return true, nil
}
return false, nil
}

// unlinkSecretFromServiceAccount ensures that the given secret is not linked with the provided service account.
// Returns true if the secret was unlinked, false if the link didn't exist.
func (r *ComponentBuildReconciler) unlinkSecretFromServiceAccount(ctx context.Context, secretNameToRemove, serviceAccountName, namespace string) (bool, error) {
log := ctrllog.FromContext(ctx)

serviceAccount := &corev1.ServiceAccount{}
err := r.Client.Get(ctx, types.NamespacedName{Name: serviceAccountName, Namespace: namespace}, serviceAccount)
if err != nil {
if errors.IsNotFound(err) {
return false, nil
}
log.Error(err, fmt.Sprintf("Failed to read service account %s in namespace %s", serviceAccountName, namespace), l.Action, l.ActionView)
return false, err
}

isSecretUnlinked := false
// Remove secret from secrets list
for index, credentialSecret := range serviceAccount.Secrets {
if credentialSecret.Name == secretNameToRemove {
secrets := make([]corev1.ObjectReference, 0, len(serviceAccount.Secrets)-1)
if len(serviceAccount.Secrets) != 1 {
secrets = append(secrets, serviceAccount.Secrets[:index]...)
secrets = append(secrets, serviceAccount.Secrets[index+1:]...)
}
serviceAccount.Secrets = secrets
isSecretUnlinked = true
break
}
}
// Remove secret from pull secrets list
for index, pullSecret := range serviceAccount.ImagePullSecrets {
if pullSecret.Name == secretNameToRemove {
secrets := make([]corev1.LocalObjectReference, 0, len(serviceAccount.ImagePullSecrets)-1)
if len(serviceAccount.ImagePullSecrets) != 1 {
secrets = append(secrets, serviceAccount.ImagePullSecrets[:index]...)
secrets = append(secrets, serviceAccount.ImagePullSecrets[index+1:]...)
}
serviceAccount.ImagePullSecrets = secrets
isSecretUnlinked = true
break
}
}

if isSecretUnlinked {
if err := r.Client.Update(ctx, serviceAccount); err != nil {
log.Error(err, fmt.Sprintf("Unable to update pipeline service account %v", serviceAccount), l.Action, l.ActionUpdate)
return false, err
}
log.Info(fmt.Sprintf("Removed %s secret link from %s service account", secretNameToRemove, serviceAccount.Name), l.Action, l.ActionUpdate)
}
return isSecretUnlinked, nil
}

func getContainerImageRepositoryForComponent(component *appstudiov1alpha1.Component) string {
if component.Spec.ContainerImage != "" {
return getContainerImageRepository(component.Spec.ContainerImage)
Expand Down
153 changes: 0 additions & 153 deletions controllers/component_build_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -777,159 +777,6 @@ var _ = Describe("Component initial build controller", func() {
return isCreatePaCPullRequestInvoked
}, timeout, interval).Should(BeTrue())
})

assertLinkSecretToSa := func(secretNotEmpty bool) {
userImageRepo := "docker.io/user/image"
generatedImageRepo := "quay.io/appstudio/generated-image"
generatedImageRepoSecretName := "generated-image-repo-secret"
generatedImageRepoSecretKey := types.NamespacedName{Namespace: resourcePacPrepKey.Namespace, Name: generatedImageRepoSecretName}
pipelineSAKey := types.NamespacedName{Namespace: resourcePacPrepKey.Namespace, Name: buildPipelineServiceAccountName}

checkPROutputImage := func(fileContent []byte, expectedImageRepo string) {
var prYaml tektonapi.PipelineRun
Expect(yaml.Unmarshal(fileContent, &prYaml)).To(Succeed())
outoutImage := ""
for _, param := range prYaml.Spec.Params {
if param.Name == "output-image" {
outoutImage = param.Value.StringVal
break
}
}
Expect(outoutImage).ToNot(BeEmpty())
Expect(outoutImage).To(ContainSubstring(expectedImageRepo))
}

isCreatePaCPullRequestInvoked := false
EnsurePaCMergeRequestFunc = func(repoUrl string, d *gp.MergeRequestData) (string, error) {
defer GinkgoRecover()
checkPROutputImage(d.Files[0].Content, userImageRepo)
isCreatePaCPullRequestInvoked = true
return "url", nil
}

// Create a component with user's ContainerImage
createCustomComponentWithBuildRequest(componentConfig{
componentKey: resourcePacPrepKey,
containerImage: userImageRepo,
annotations: defaultPipelineAnnotations,
}, BuildRequestConfigurePaCAnnotationValue)

Eventually(func() bool {
return isCreatePaCPullRequestInvoked
}, timeout, interval).Should(BeTrue())

// Switch to generated image repository
createSecret(generatedImageRepoSecretKey, nil)
defer deleteSecret(generatedImageRepoSecretKey)

component := getComponent(resourcePacPrepKey)
component.Annotations[ImageRepoGenerateAnnotationName] = "false"

if secretNotEmpty {
component.Annotations[ImageRepoAnnotationName] =
fmt.Sprintf("{\"image\":\"%s\",\"secret\":\"%s\"}", generatedImageRepo, generatedImageRepoSecretName)

} else {
component.Annotations[ImageRepoAnnotationName] =
fmt.Sprintf("{\"image\":\"%s\",\"secret\":\"\"}", generatedImageRepo)
}
component.Spec.ContainerImage = generatedImageRepo
Expect(k8sClient.Update(ctx, component)).To(Succeed())

// wait also for image finalizer on component
// image-registry-secret-sa-link.component.appstudio.openshift.io/finalizer
// otherwise when finalizer isn't there secret won't be unlinked from SA (when component is removed)
waitImageRegistrySecretLinkFinalizerOnComponent(resourcePacPrepKey)

Eventually(func() bool {
component = getComponent(resourcePacPrepKey)
return component.Spec.ContainerImage == generatedImageRepo
}, timeout, interval).Should(BeTrue())

pipelineSA := &corev1.ServiceAccount{}
Expect(k8sClient.Get(ctx, pipelineSAKey, pipelineSA)).To(Succeed())
isImageRegistryGeneratedSecretLinked := false
if pipelineSA.Secrets == nil {
time.Sleep(1 * time.Second)
Expect(k8sClient.Get(ctx, pipelineSAKey, pipelineSA)).To(Succeed())
}
for _, secret := range pipelineSA.Secrets {
if secret.Name == generatedImageRepoSecretName {
isImageRegistryGeneratedSecretLinked = true
break
}
}
isImageRegistryGeneratedPullSecretLinked := false
for _, secret := range pipelineSA.ImagePullSecrets {
if secret.Name == generatedImageRepoSecretName {
isImageRegistryGeneratedPullSecretLinked = true
break
}
}

if secretNotEmpty {
Expect(isImageRegistryGeneratedSecretLinked).To(BeTrue())
Expect(isImageRegistryGeneratedPullSecretLinked).To(BeTrue())
} else {
Expect(isImageRegistryGeneratedSecretLinked).To(BeFalse())
Expect(isImageRegistryGeneratedPullSecretLinked).To(BeFalse())
}
}

It("should link auto generated image repository secret to pipeline service account", func() {
assertLinkSecretToSa(true)
})

It("should not link auto generated image repository secret to pipeline service account, when it is linked already, also check that it keeps other secrets", func() {
generatedImageRepoSecretName := "generated-image-repo-secret"
serviceAccountName := types.NamespacedName{Name: buildPipelineServiceAccountName, Namespace: "default"}
serviceAccount := &corev1.ServiceAccount{}
Expect(k8sClient.Get(ctx, serviceAccountName, serviceAccount)).To(Succeed())

anotherSecretName := "anothersecret"
// add secret to the SA
serviceAccount.Secrets = []corev1.ObjectReference{{Name: generatedImageRepoSecretName, Namespace: "default"}, {Name: anotherSecretName, Namespace: "default"}}
serviceAccount.ImagePullSecrets = []corev1.LocalObjectReference{{Name: generatedImageRepoSecretName}, {Name: anotherSecretName}}
Expect(k8sClient.Update(ctx, serviceAccount)).Should(Succeed())

generatedImageRepoSecretKey := types.NamespacedName{Namespace: resourcePacPrepKey.Namespace, Name: generatedImageRepoSecretName}
// wait for secret to be removed because component was removed in afterEach
waitSecretGone(generatedImageRepoSecretKey)
assertLinkSecretToSa(true)

// will remove just generate image repo secret from SA and keep anothersecret
deleteComponent(resourcePacPrepKey)
waitSecretGone(generatedImageRepoSecretKey)

Eventually(func() bool {
err := k8sClient.Get(ctx, serviceAccountName, serviceAccount)
if err != nil {
return false
}
return len(serviceAccount.Secrets) == 1
}, timeout, interval).Should(BeTrue())

Expect(k8sClient.Get(ctx, serviceAccountName, serviceAccount)).To(Succeed())
Expect(len(serviceAccount.Secrets)).To(Equal(1))
Expect(len(serviceAccount.ImagePullSecrets)).To(Equal(1))
Expect(serviceAccount.Secrets[0].Name).To(Equal(anotherSecretName))
Expect(serviceAccount.ImagePullSecrets[0].Name).To(Equal(anotherSecretName))

// cleanup, remove anothersecret from the SA
Expect(k8sClient.Get(ctx, serviceAccountName, serviceAccount)).To(Succeed())
serviceAccount.Secrets = []corev1.ObjectReference{}
serviceAccount.ImagePullSecrets = []corev1.LocalObjectReference{}
Expect(k8sClient.Update(ctx, serviceAccount)).Should(Succeed())
})

It("should not link auto generated image repository secret to pipeline service account, when secret is missing", func() {
// wait for secret to be removed because component was removed in afterEach
generatedImageRepoSecretName := "generated-image-repo-secret"
generatedImageRepoSecretKey := types.NamespacedName{Namespace: resourcePacPrepKey.Namespace, Name: generatedImageRepoSecretName}
waitSecretGone(generatedImageRepoSecretKey)

assertLinkSecretToSa(false)
})
})

Context("Test Pipelines as Code build clean up", func() {
Expand Down
4 changes: 0 additions & 4 deletions controllers/suite_util_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -260,10 +260,6 @@ func waitPaCFinalizerOnComponent(componentKey types.NamespacedName) {
waitFinalizerOnComponent(componentKey, PaCProvisionFinalizer, true)
}

func waitImageRegistrySecretLinkFinalizerOnComponent(componentKey types.NamespacedName) {
waitFinalizerOnComponent(componentKey, ImageRegistrySecretLinkFinalizer, true)
}

func waitPaCFinalizerOnComponentGone(componentKey types.NamespacedName) {
waitFinalizerOnComponent(componentKey, PaCProvisionFinalizer, false)
}
Expand Down

0 comments on commit 2055ba3

Please sign in to comment.