diff --git a/README.md b/README.md index 446ac6c..5cdf268 100644 --- a/README.md +++ b/README.md @@ -111,7 +111,28 @@ spec: regenerate-token: true ... ``` -After token rotation, the `spec.credentials` section will be deleted and `status.credentials.generationTimestamp` updated. +After token rotation, the `spec.credentials.regenerate-token` section will be deleted and `status.credentials.generationTimestamp` updated. + +--- + +### Verify and fix secrets links + +It will link secret to service account if link is missing. +It will remove duplicate links of secret in service account. +It will remove secret from imagePullSecrets in service account. +It will unlink secret from service account, if secret doesn't exist (you can recreate secret using 'regenerate-token'). +It's possible to request verification and fixing of secrets linking to service account by adding: +```yaml +... +spec: + ... + credentials: + verify-linking: true + ... +``` +After verification, the `spec.credentials.verify-linking` section will be deleted. + +--- ### Error handling diff --git a/api/v1alpha1/imagerepository_types.go b/api/v1alpha1/imagerepository_types.go index ae5d53c..adae0a7 100644 --- a/api/v1alpha1/imagerepository_types.go +++ b/api/v1alpha1/imagerepository_types.go @@ -64,6 +64,10 @@ type ImageCredentials struct { // Refreshes both, push and pull tokens. // The field gets cleared after the refresh. RegenerateToken *bool `json:"regenerate-token,omitempty"` + // VerifyLinking defines a request to verify and fix + // secret linking in pipeline service account. + // The field gets cleared after fixing. + VerifyLinking *bool `json:"verify-linking,omitempty"` } type Notifications struct { diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index c26f491..28e993c 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -52,6 +52,11 @@ func (in *ImageCredentials) DeepCopyInto(out *ImageCredentials) { *out = new(bool) **out = **in } + if in.VerifyLinking != nil { + in, out := &in.VerifyLinking, &out.VerifyLinking + *out = new(bool) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ImageCredentials. diff --git a/config/crd/bases/appstudio.redhat.com_imagerepositories.yaml b/config/crd/bases/appstudio.redhat.com_imagerepositories.yaml index 6d14fb1..19638fd 100644 --- a/config/crd/bases/appstudio.redhat.com_imagerepositories.yaml +++ b/config/crd/bases/appstudio.redhat.com_imagerepositories.yaml @@ -50,6 +50,11 @@ spec: accessing credentials. Refreshes both, push and pull tokens. The field gets cleared after the refresh. type: boolean + verify-linking: + description: VerifyLinking defines a request to verify and fix + secret linking in pipeline service account. The field gets cleared + after fixing. + type: boolean type: object image: description: Requested image repository configuration. diff --git a/controllers/imagerepository_controller.go b/controllers/imagerepository_controller.go index a7418fe..c7378ee 100644 --- a/controllers/imagerepository_controller.go +++ b/controllers/imagerepository_controller.go @@ -108,6 +108,11 @@ func (r *ImageRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Requ // Reread quay token r.QuayClient = r.BuildQuayClient(log) + if err := r.unlinkSecretFromServiceAccount(ctx, imageRepository.Status.Credentials.PushSecretName, imageRepository.Namespace); err != nil { + log.Error(err, "failed to unlink secret from service account", "SecretName", imageRepository.Status.Credentials.PushSecretName, l.Action, l.ActionUpdate) + return ctrl.Result{}, err + } + if controllerutil.ContainsFinalizer(imageRepository, ImageRepositoryFinalizer) { // Do not block deletion on failures r.CleanupImageRepository(ctx, imageRepository) @@ -161,21 +166,21 @@ func (r *ImageRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Requ return ctrl.Result{}, nil } - log.Error(err, "failed to get component", "ComponentName", componentName) + log.Error(err, "failed to get component", "ComponentName", componentName, l.Action, l.ActionView) return ctrl.Result{}, err } component.Spec.ContainerImage = imageRepository.Status.Image.URL if err := r.Client.Update(ctx, component); err != nil { - log.Error(err, "failed to update Component after provision", "ComponentName", componentName) + log.Error(err, "failed to update Component after provision", "ComponentName", componentName, l.Action, l.ActionUpdate) return ctrl.Result{}, err } log.Info("Updated component's ContainerImage", "ComponentName", componentName) delete(imageRepository.Annotations, updateComponentAnnotationName) if err := r.Client.Update(ctx, imageRepository); err != nil { - log.Error(err, "failed to update imageRepository annotation") + log.Error(err, "failed to update imageRepository annotation", l.Action, l.ActionUpdate) return ctrl.Result{}, err } log.Info("Updated image repository annotation") @@ -217,6 +222,15 @@ func (r *ImageRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Requ } return ctrl.Result{}, nil } + + // Check and fix linking is requested + verifyLinking := imageRepository.Spec.Credentials.VerifyLinking + if verifyLinking != nil && *verifyLinking { + if err := r.VerifyAndFixSecretsLinking(ctx, imageRepository); err != nil { + return ctrl.Result{}, err + } + return ctrl.Result{}, nil + } } if err = r.HandleNotifications(ctx, imageRepository); err != nil { @@ -224,7 +238,7 @@ func (r *ImageRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Requ } if err := r.Client.Status().Update(ctx, imageRepository); err != nil { - log.Error(err, "failed to update image repository status") + log.Error(err, "failed to update imageRepository status", l.Action, l.ActionUpdate) return ctrl.Result{}, err } @@ -242,7 +256,7 @@ func (r *ImageRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Requ // ProvisionImageRepository creates image repository, robot account(s) and secret(s) to access the image repository. // If labels with Application and Component name are present, robot account with pull only access -// will be created and pull token will be propagated to all environments via Remote Secret. +// will be created and pull token will be propagated Secret. func (r *ImageRepositoryReconciler) ProvisionImageRepository(ctx context.Context, imageRepository *imagerepositoryv1alpha1.ImageRepository) error { log := ctrllog.FromContext(ctx).WithName("ImageRepositoryProvision") ctx = ctrllog.IntoContext(ctx, log) @@ -257,13 +271,13 @@ func (r *ImageRepositoryReconciler) ProvisionImageRepository(ctx context.Context imageRepository.Status.State = imagerepositoryv1alpha1.ImageRepositoryStateFailed imageRepository.Status.Message = fmt.Sprintf("Component '%s' does not exist", componentName) if err := r.Client.Status().Update(ctx, imageRepository); err != nil { - log.Error(err, "failed to update image repository status") + log.Error(err, "failed to update imageRepository status", l.Action, l.ActionUpdate) return err } log.Info("attempt to create image repository related to non existing component", "Component", componentName) return nil } - log.Error(err, "failed to get component", "ComponentName", componentName) + log.Error(err, "failed to get component", "ComponentName", componentName, l.Action, l.ActionView) return err } } @@ -308,7 +322,7 @@ func (r *ImageRepositoryReconciler) ProvisionImageRepository(ctx context.Context imageRepository.Status.Message = err.Error() } if err := r.Client.Status().Update(ctx, imageRepository); err != nil { - log.Error(err, "failed to update image repository status") + log.Error(err, "failed to update imageRepository status", l.Action, l.ActionUpdate) } return nil } @@ -354,20 +368,19 @@ func (r *ImageRepositoryReconciler) ProvisionImageRepository(ctx context.Context if isComponentLinked(imageRepository) { if err := controllerutil.SetOwnerReference(component, imageRepository, r.Scheme); err != nil { log.Error(err, "failed to set component as owner", "ComponentName", component.Name) - // Do not brake provision because of failed owner reference + // Do not fail provision because of failed owner reference } } if err := r.Client.Update(ctx, imageRepository); err != nil { - log.Error(err, "failed to update CR after provision") + log.Error(err, "failed to update imageRepository after provision", l.Action, l.ActionUpdate) return err - } else { - log.Info("Finished provision of image repository and added finalizer") } + log.Info("Finished provision of image repository and added finalizer") imageRepository.Status = status if err := r.Client.Status().Update(ctx, imageRepository); err != nil { - log.Error(err, "failed to update CR status after provision") + log.Error(err, "failed to update imageRepository status after provision", l.Action, l.ActionUpdate) return err } @@ -380,7 +393,7 @@ type imageRepositoryAccessData struct { } // ProvisionImageRepositoryAccess makes existing quay image repository accessible -// by creating robot account and storing its token in a RemoteSecret. +// by creating robot account and storing its token in a Secret. func (r *ImageRepositoryReconciler) ProvisionImageRepositoryAccess(ctx context.Context, imageRepository *imagerepositoryv1alpha1.ImageRepository, isPullOnly bool) (*imageRepositoryAccessData, error) { log := ctrllog.FromContext(ctx).WithName("ProvisionImageRepositoryAccess").WithValues("IsPullOnly", isPullOnly) ctx = ctrllog.IntoContext(ctx, log) @@ -434,20 +447,20 @@ func (r *ImageRepositoryReconciler) RegenerateImageRepositoryCredentials(ctx con imageRepository.Spec.Credentials.RegenerateToken = nil if err := r.Client.Update(ctx, imageRepository); err != nil { - log.Error(err, "failed to update image repository", l.Action, l.ActionUpdate) + log.Error(err, "failed to update imageRepository", l.Action, l.ActionUpdate) return err } imageRepository.Status.Credentials.GenerationTimestamp = &metav1.Time{Time: time.Now()} if err := r.Client.Status().Update(ctx, imageRepository); err != nil { - log.Error(err, "failed to update image repository status", l.Action, l.ActionUpdate) + log.Error(err, "failed to update imageRepository status", l.Action, l.ActionUpdate) return err } return nil } -// RegenerateImageRepositoryAccessToken rotates robot account token and updates new one to the corresponding Remote Secret. +// RegenerateImageRepositoryAccessToken rotates robot account token and updates new one to the corresponding Secret. func (r *ImageRepositoryReconciler) RegenerateImageRepositoryAccessToken(ctx context.Context, imageRepository *imagerepositoryv1alpha1.ImageRepository, isPullOnly bool) error { log := ctrllog.FromContext(ctx).WithName("RegenerateImageRepositoryAccessToken").WithValues("IsPullOnly", isPullOnly) ctx = ctrllog.IntoContext(ctx, log) @@ -524,7 +537,7 @@ func (r *ImageRepositoryReconciler) ChangeImageRepositoryVisibility(ctx context. imageRepository.Status.Image.Visibility = imageRepository.Spec.Image.Visibility imageRepository.Status.Message = "" if err := r.Client.Status().Update(ctx, imageRepository); err != nil { - log.Error(err, "failed to update image repository name", l.Action, l.ActionUpdate) + log.Error(err, "failed to update imageRepository status", l.Action, l.ActionUpdate) return err } log.Info("changed image repository visibility", "visibility", imageRepository.Spec.Image.Visibility) @@ -536,13 +549,13 @@ func (r *ImageRepositoryReconciler) ChangeImageRepositoryVisibility(ctx context. imageRepository.Spec.Image.Visibility = imageRepository.Status.Image.Visibility if err := r.Client.Update(ctx, imageRepository); err != nil { - log.Error(err, "failed to update image repository", l.Action, l.ActionUpdate) + log.Error(err, "failed to update imageRepository", l.Action, l.ActionUpdate) return err } imageRepository.Status.Message = "Quay organization plan private repositories limit exceeded" if err := r.Client.Status().Update(ctx, imageRepository); err != nil { - log.Error(err, "failed to update image repository", l.Action, l.ActionUpdate) + log.Error(err, "failed to update imageRepository status", l.Action, l.ActionUpdate) return err } @@ -561,7 +574,7 @@ func (r *ImageRepositoryReconciler) EnsureSecret(ctx context.Context, imageRepos secretKey := types.NamespacedName{Namespace: imageRepository.Namespace, Name: secretName} if err := r.Client.Get(ctx, secretKey, secret); err != nil { if !errors.IsNotFound(err) { - log.Error(err, "failed to get remote secret", l.Action, l.ActionView) + log.Error(err, "failed to get secret", "SecretName", secretName, l.Action, l.ActionView) return err } @@ -590,20 +603,204 @@ func (r *ImageRepositoryReconciler) EnsureSecret(ctx context.Context, imageRepos } if !isPull { - serviceAccount := &corev1.ServiceAccount{} - serviceAccountKey := types.NamespacedName{Namespace: imageRepository.Namespace, Name: buildPipelineServiceAccountName} - if err := r.Client.Get(ctx, serviceAccountKey, serviceAccount); err != nil { - log.Error(err, "failed to get service account", l.Action, l.ActionView) + if err := r.linkSecretToServiceAccount(ctx, secretName, imageRepository.Namespace); err != nil { + log.Error(err, "failed to link secret to service account", "SecretName", secretName, l.Action, l.ActionUpdate) return err } - serviceAccount.Secrets = append(serviceAccount.Secrets, corev1.ObjectReference{Name: secretName}) - serviceAccount.ImagePullSecrets = append(serviceAccount.ImagePullSecrets, corev1.LocalObjectReference{Name: secretName}) - if err := r.Client.Update(ctx, serviceAccount); err != nil { - log.Error(err, "failed to update service account", l.Action, l.ActionUpdate) - return err + } + } + return nil +} + +// linkSecretToServiceAccount ensures that the given secret is linked with the provided service account. +func (r *ImageRepositoryReconciler) linkSecretToServiceAccount(ctx context.Context, secretNameToAdd, namespace string) error { + log := ctrllog.FromContext(ctx) + + serviceAccount := &corev1.ServiceAccount{} + err := r.Client.Get(ctx, types.NamespacedName{Name: buildPipelineServiceAccountName, Namespace: namespace}, serviceAccount) + if err != nil { + if errors.IsNotFound(err) { + return nil + } + log.Error(err, "failed to read pipeline service account", "ServiceAccountName", buildPipelineServiceAccountName, "NamespaceName", namespace, l.Action, l.ActionView) + return err + } + + // check if secret is already linked and add it only if it isn't to avoid duplication + secretLinked := false + for _, serviceAccountSecret := range serviceAccount.Secrets { + if serviceAccountSecret.Name == secretNameToAdd { + secretLinked = true + break + } + } + if !secretLinked { + // add it only to Secrets and not in imagePullSecrets which is used only for the image pod itself needs to run + serviceAccount.Secrets = append(serviceAccount.Secrets, corev1.ObjectReference{Name: secretNameToAdd}) + + if err := r.Client.Update(ctx, serviceAccount); err != nil { + log.Error(err, "failed to update pipeline service account", "ServiceAccountName", buildPipelineServiceAccountName, "NamespaceName", namespace, l.Action, l.ActionUpdate) + return err + } + log.Info("Added secret link to pipeline service account", "SecretName", secretNameToAdd, "ServiceAccountName", buildPipelineServiceAccountName, l.Action, l.ActionUpdate) + } + return nil +} + +// unlinkSecretFromServiceAccount ensures that the given secret is not linked with the provided service account. +func (r *ImageRepositoryReconciler) unlinkSecretFromServiceAccount(ctx context.Context, secretNameToRemove, namespace string) error { + log := ctrllog.FromContext(ctx) + + serviceAccount := &corev1.ServiceAccount{} + err := r.Client.Get(ctx, types.NamespacedName{Name: buildPipelineServiceAccountName, Namespace: namespace}, serviceAccount) + if err != nil { + if errors.IsNotFound(err) { + return nil + } + log.Error(err, "failed to read pipeline service account", "ServiceAccountName", buildPipelineServiceAccountName, "NamespaceName", namespace, l.Action, l.ActionView) + return err + } + + unlinkSecret := false + // Remove secret from secrets list + pushSecrets := []corev1.ObjectReference{} + for _, credentialSecret := range serviceAccount.Secrets { + // don't break and search for duplicities + if credentialSecret.Name == secretNameToRemove { + unlinkSecret = true + continue + } + pushSecrets = append(pushSecrets, credentialSecret) + } + serviceAccount.Secrets = pushSecrets + + // Remove secret from pull secrets list + // we aren't adding them anymore in imagePullSecrets but just to be sure check and remove them from there + imagePullSecrets := []corev1.LocalObjectReference{} + for _, pullSecret := range serviceAccount.ImagePullSecrets { + // don't break and search for duplicities + if pullSecret.Name == secretNameToRemove { + unlinkSecret = true + continue + } + imagePullSecrets = append(imagePullSecrets, pullSecret) + } + serviceAccount.ImagePullSecrets = imagePullSecrets + + if unlinkSecret { + if err := r.Client.Update(ctx, serviceAccount); err != nil { + log.Error(err, "failed to update pipeline service account", "ServiceAccountName", buildPipelineServiceAccountName, "NamespaceName", namespace, l.Action, l.ActionUpdate) + return err + } + log.Info("Removed secret link from pipeline service account", "SecretName", secretNameToRemove, "ServiceAccountName", buildPipelineServiceAccountName, l.Action, l.ActionUpdate) + } + return nil +} + +// cleanUpSecretInServiceAccount ensures that the given secret is linked with the provided service account just once +// and remove the secret from ImagePullSecrets +func (r *ImageRepositoryReconciler) cleanUpSecretInServiceAccount(ctx context.Context, secretName, namespace string) error { + log := ctrllog.FromContext(ctx) + + serviceAccount := &corev1.ServiceAccount{} + err := r.Client.Get(ctx, types.NamespacedName{Name: buildPipelineServiceAccountName, Namespace: namespace}, serviceAccount) + if err != nil { + if errors.IsNotFound(err) { + return nil + } + log.Error(err, "failed to read pipeline service account", "ServiceAccountName", buildPipelineServiceAccountName, "NamespaceName", namespace, l.Action, l.ActionView) + return err + } + + linksModified := false + + // Check for duplicates for the secret and remove them + pushSecrets := []corev1.ObjectReference{} + foundSecret := false + for _, credentialSecret := range serviceAccount.Secrets { + if credentialSecret.Name == secretName { + if !foundSecret { + pushSecrets = append(pushSecrets, credentialSecret) + foundSecret = true + } else { + linksModified = true } + } else { + pushSecrets = append(pushSecrets, credentialSecret) + } + } + serviceAccount.Secrets = pushSecrets + + // Remove secret from pull secrets list + imagePullSecrets := []corev1.LocalObjectReference{} + for _, pullSecret := range serviceAccount.ImagePullSecrets { + // don't break and search for duplicities + if pullSecret.Name == secretName { + linksModified = true + continue + } + imagePullSecrets = append(imagePullSecrets, pullSecret) + } + serviceAccount.ImagePullSecrets = imagePullSecrets + + if linksModified { + if err := r.Client.Update(ctx, serviceAccount); err != nil { + log.Error(err, "failed to update pipeline service account", "ServiceAccountName", buildPipelineServiceAccountName, "NamespaceName", namespace, l.Action, l.ActionUpdate) + return err + } + log.Info("Cleaned up secret links in pipeline service account", "SecretName", secretName, "ServiceAccountName", buildPipelineServiceAccountName, l.Action, l.ActionUpdate) + } + return nil +} + +// VerifyAndFixSecretsLinking ensures that the given secret is linked to the provided service account, and also removes duplicated link for the secret. +// when secret doesn't exist unlink it from service account +func (r *ImageRepositoryReconciler) VerifyAndFixSecretsLinking(ctx context.Context, imageRepository *imagerepositoryv1alpha1.ImageRepository) error { + log := ctrllog.FromContext(ctx) + + secretName := imageRepository.Status.Credentials.PushSecretName + + // check secret existence and if secret doesn't exist unlink it from service account + // users can recreate it by using regenerate-token + secret := &corev1.Secret{} + secretKey := types.NamespacedName{Namespace: imageRepository.Namespace, Name: secretName} + secretExists := true + if err := r.Client.Get(ctx, secretKey, secret); err != nil { + if !errors.IsNotFound(err) { + log.Error(err, "failed to get secret", "SecretName", secretName, l.Action, l.ActionView) + return err + } + + secretExists = false + log.Info("Secret doesn't exist, will unlink secret from service account", "SecretName", secretName) + log.Info("To recreate secret use regenerate-token") + + if err := r.unlinkSecretFromServiceAccount(ctx, secretName, imageRepository.Namespace); err != nil { + log.Error(err, "failed to unlink secret from service account", "SecretName", secretName, l.Action, l.ActionUpdate) + return err } } + + if secretExists { + // link secret to service account if isn't linked already + if err := r.linkSecretToServiceAccount(ctx, secretName, imageRepository.Namespace); err != nil { + log.Error(err, "failed to link secret to service account", "SecretName", secretName, l.Action, l.ActionUpdate) + return err + } + + // clean duplicate secret links and remove secret from ImagePullSecrets + if err := r.cleanUpSecretInServiceAccount(ctx, secretName, imageRepository.Namespace); err != nil { + log.Error(err, "failed to clean up secret in service account", "SecretName", secretName, l.Action, l.ActionUpdate) + return err + } + } + + imageRepository.Spec.Credentials.VerifyLinking = nil + if err := r.Client.Update(ctx, imageRepository); err != nil { + log.Error(err, "failed to update imageRepository", l.Action, l.ActionUpdate) + return err + } + return nil } @@ -648,7 +845,7 @@ func isComponentLinked(imageRepository *imagerepositoryv1alpha1.ImageRepository) func getRandomString(length int) string { bytes := make([]byte, length/2+1) if _, err := rand.Read(bytes); err != nil { - panic("Failed to read from random generator") + panic("failed to read from random generator") } return hex.EncodeToString(bytes)[0:length] } @@ -657,7 +854,7 @@ func (r *ImageRepositoryReconciler) UpdateImageRepositoryStatusMessage(ctx conte log := ctrllog.FromContext(ctx) imageRepository.Status.Message = statusMessage if err := r.Client.Status().Update(ctx, imageRepository); err != nil { - log.Error(err, "failed to update image repository status") + log.Error(err, "failed to update imageRepository status", l.Action, l.ActionUpdate) return err } diff --git a/controllers/imagerepository_controller_test.go b/controllers/imagerepository_controller_test.go index 9c95d0c..e1669ce 100644 --- a/controllers/imagerepository_controller_test.go +++ b/controllers/imagerepository_controller_test.go @@ -19,13 +19,14 @@ import ( "encoding/base64" "encoding/json" "fmt" - "github.com/konflux-ci/image-controller/pkg/quay" - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" "regexp" "strings" "time" + "github.com/konflux-ci/image-controller/pkg/quay" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" @@ -37,8 +38,6 @@ var _ = Describe("Image repository controller", func() { var ( authRegexp = regexp.MustCompile(`.*{"auth":"([A-Za-z0-9+/=]*)"}.*`) - resourceKey = types.NamespacedName{Name: defaultImageRepositoryName, Namespace: defaultNamespace} - pushToken string pullToken string expectedRobotAccountPrefix string @@ -50,7 +49,8 @@ var _ = Describe("Image repository controller", func() { createNamespace(defaultNamespace) }) - Context("Image repository provision", func() { + Context("Image repository provision without component", func() { + var resourceKey = types.NamespacedName{Name: defaultImageRepositoryName + "-isnotlinked", Namespace: defaultNamespace} BeforeEach(func() { quay.ResetTestQuayClientToFails() @@ -58,7 +58,7 @@ var _ = Describe("Image repository controller", func() { It("should prepare environment", func() { pushToken = "push-token1234" - expectedImageName = fmt.Sprintf("%s/%s", defaultNamespace, defaultImageRepositoryName) + expectedImageName = fmt.Sprintf("%s/%s", defaultNamespace, resourceKey.Name) expectedImage = fmt.Sprintf("quay.io/%s/%s", quay.TestQuayOrg, expectedImageName) expectedRobotAccountPrefix = strings.ReplaceAll(strings.ReplaceAll(expectedImageName, "-", "_"), "/", "_") createServiceAccount(defaultNamespace, buildPipelineServiceAccountName) @@ -101,7 +101,7 @@ var _ = Describe("Image repository controller", func() { return &quay.Notification{UUID: "uuid"}, nil } - createImageRepository(imageRepositoryConfig{}) + createImageRepository(imageRepositoryConfig{ResourceKey: &resourceKey}) Eventually(func() bool { return isCreateRepositoryInvoked }, timeout, interval).Should(BeTrue()) Eventually(func() bool { return isCreateRobotAccountInvoked }, timeout, interval).Should(BeTrue()) @@ -134,6 +134,8 @@ var _ = Describe("Image repository controller", func() { Expect(pushSecret.Type).To(Equal(corev1.SecretTypeDockerConfigJson)) sa := getServiceAccount(defaultNamespace, buildPipelineServiceAccountName) + Expect(sa.Secrets).To(HaveLen(1)) + Expect(sa.ImagePullSecrets).To(HaveLen(0)) Expect(sa.Secrets).To(ContainElement(corev1.ObjectReference{Name: pushSecret.Name})) pushSecretDockerconfigJson := string(pushSecret.Data[corev1.DockerConfigJsonKey]) @@ -177,7 +179,6 @@ var _ = Describe("Image repository controller", func() { pushSecretKey := types.NamespacedName{Name: imageRepository.Status.Credentials.PushSecretName, Namespace: imageRepository.Namespace} pushSecret := waitSecretExist(pushSecretKey) - defer deleteSecret(pushSecretKey) Expect(pushSecret.Type).To(Equal(corev1.SecretTypeDockerConfigJson)) pushSecretDockerconfigJson := string(pushSecret.Data[corev1.DockerConfigJsonKey]) @@ -225,6 +226,201 @@ var _ = Describe("Image repository controller", func() { }, timeout, interval).Should(BeTrue()) }) + It("verify and fix, secret is missing from SA", func() { + // will add it to SA, but not to ImagePullSecrets + sa := getServiceAccount(defaultNamespace, buildPipelineServiceAccountName) + sa.Secrets = []corev1.ObjectReference{} + Expect(k8sClient.Update(ctx, &sa)).To(Succeed()) + + imageRepository := getImageRepository(resourceKey) + verifyLinking := true + imageRepository.Spec.Credentials = &imagerepositoryv1alpha1.ImageCredentials{VerifyLinking: &verifyLinking} + Expect(k8sClient.Update(ctx, imageRepository)).To(Succeed()) + + waitImageRepositoryCredentialGone(resourceKey, "verify") + + secretName := fmt.Sprintf("%s-image-push", resourceKey.Name) + sa = getServiceAccount(defaultNamespace, buildPipelineServiceAccountName) + Expect(sa.Secrets).To(HaveLen(1)) + Expect(sa.ImagePullSecrets).To(HaveLen(0)) + Expect(sa.Secrets).To(ContainElement(corev1.ObjectReference{Name: secretName})) + }) + + It("verify and fix, secret is duplicated in SA, also is in ImagePullSecrets", func() { + // will remove duplicate, and remove it from ImagePullSecrets + sa := getServiceAccount(defaultNamespace, buildPipelineServiceAccountName) + secretName := fmt.Sprintf("%s-image-push", resourceKey.Name) + sa.Secrets = []corev1.ObjectReference{{Name: secretName}, {Name: secretName}} + sa.ImagePullSecrets = []corev1.LocalObjectReference{{Name: secretName}, {Name: secretName}} + Expect(k8sClient.Update(ctx, &sa)).To(Succeed()) + + imageRepository := getImageRepository(resourceKey) + verifyLinking := true + imageRepository.Spec.Credentials = &imagerepositoryv1alpha1.ImageCredentials{VerifyLinking: &verifyLinking} + Expect(k8sClient.Update(ctx, imageRepository)).To(Succeed()) + + waitImageRepositoryCredentialGone(resourceKey, "verify") + + sa = getServiceAccount(defaultNamespace, buildPipelineServiceAccountName) + Expect(sa.Secrets).To(HaveLen(1)) + Expect(sa.ImagePullSecrets).To(HaveLen(0)) + Expect(sa.Secrets).To(ContainElement(corev1.ObjectReference{Name: secretName})) + }) + + It("verify and fix, secret is missing", func() { + // will remove it from SA + secretName := types.NamespacedName{Name: fmt.Sprintf("%s-image-push", resourceKey.Name), Namespace: defaultNamespace} + deleteSecret(secretName) + + imageRepository := getImageRepository(resourceKey) + verifyLinking := true + imageRepository.Spec.Credentials = &imagerepositoryv1alpha1.ImageCredentials{VerifyLinking: &verifyLinking} + Expect(k8sClient.Update(ctx, imageRepository)).To(Succeed()) + + waitImageRepositoryCredentialGone(resourceKey, "verify") + + sa := getServiceAccount(defaultNamespace, buildPipelineServiceAccountName) + Expect(sa.Secrets).To(HaveLen(0)) + Expect(sa.ImagePullSecrets).To(HaveLen(0)) + }) + + It("should cleanup repository", func() { + isDeleteRobotAccountInvoked := false + quay.DeleteRobotAccountFunc = func(organization, robotAccountName string) (bool, error) { + defer GinkgoRecover() + isDeleteRobotAccountInvoked = true + Expect(organization).To(Equal(quay.TestQuayOrg)) + Expect(strings.HasPrefix(robotAccountName, expectedRobotAccountPrefix)).To(BeTrue()) + return true, nil + } + isDeleteRepositoryInvoked := false + quay.DeleteRepositoryFunc = func(organization, imageRepository string) (bool, error) { + defer GinkgoRecover() + isDeleteRepositoryInvoked = true + Expect(organization).To(Equal(quay.TestQuayOrg)) + Expect(imageRepository).To(Equal(expectedImageName)) + return true, nil + } + + deleteImageRepository(resourceKey) + + Eventually(func() bool { return isDeleteRobotAccountInvoked }, timeout, interval).Should(BeTrue()) + Eventually(func() bool { return isDeleteRepositoryInvoked }, timeout, interval).Should(BeTrue()) + + sa := getServiceAccount(defaultNamespace, buildPipelineServiceAccountName) + // verify that secret is unlinked + Expect(sa.Secrets).To(HaveLen(0)) + Expect(sa.ImagePullSecrets).To(HaveLen(0)) + + deleteServiceAccount(types.NamespacedName{Name: buildPipelineServiceAccountName, Namespace: defaultNamespace}) + }) + }) + + Context("Image repository provision secret already linked in SA", func() { + var resourceKey = types.NamespacedName{Name: defaultImageRepositoryName + "-islinked", Namespace: defaultNamespace} + + BeforeEach(func() { + quay.ResetTestQuayClientToFails() + }) + + It("should prepare environment", func() { + pushToken = "push-token1234" + expectedImageName = fmt.Sprintf("%s/%s", defaultNamespace, resourceKey.Name) + expectedImage = fmt.Sprintf("quay.io/%s/%s", quay.TestQuayOrg, expectedImageName) + expectedRobotAccountPrefix = strings.ReplaceAll(strings.ReplaceAll(expectedImageName, "-", "_"), "/", "_") + createServiceAccount(defaultNamespace, buildPipelineServiceAccountName) + + // add push secret to SA + sa := getServiceAccount(defaultNamespace, buildPipelineServiceAccountName) + secretName := fmt.Sprintf("%s-image-push", resourceKey.Name) + sa.Secrets = append(sa.Secrets, corev1.ObjectReference{Name: secretName}) + Expect(k8sClient.Update(ctx, &sa)).To(Succeed()) + }) + + It("should provision image repository", func() { + isCreateRepositoryInvoked := false + quay.CreateRepositoryFunc = func(repository quay.RepositoryRequest) (*quay.Repository, error) { + defer GinkgoRecover() + isCreateRepositoryInvoked = true + Expect(repository.Repository).To(Equal(expectedImageName)) + Expect(repository.Namespace).To(Equal(quay.TestQuayOrg)) + Expect(repository.Visibility).To(Equal("public")) + Expect(repository.Description).ToNot(BeEmpty()) + return &quay.Repository{Name: expectedImageName}, nil + } + isCreateRobotAccountInvoked := false + quay.CreateRobotAccountFunc = func(organization, robotName string) (*quay.RobotAccount, error) { + defer GinkgoRecover() + isCreateRobotAccountInvoked = true + Expect(organization).To(Equal(quay.TestQuayOrg)) + Expect(strings.HasPrefix(robotName, expectedRobotAccountPrefix)).To(BeTrue()) + return &quay.RobotAccount{Name: robotName, Token: pushToken}, nil + } + isAddPushPermissionsToRobotAccountInvoked := false + quay.AddPermissionsForRepositoryToRobotAccountFunc = func(organization, imageRepository, robotAccountName string, isWrite bool) error { + defer GinkgoRecover() + isAddPushPermissionsToRobotAccountInvoked = true + Expect(organization).To(Equal(quay.TestQuayOrg)) + Expect(imageRepository).To(Equal(expectedImageName)) + Expect(isWrite).To(BeTrue()) + Expect(strings.HasPrefix(robotAccountName, expectedRobotAccountPrefix)).To(BeTrue()) + return nil + } + + isCreateNotificationInvoked := false + quay.CreateNotificationFunc = func(organization, repository string, notification quay.Notification) (*quay.Notification, error) { + isCreateNotificationInvoked = true + Expect(organization).To(Equal(quay.TestQuayOrg)) + return &quay.Notification{UUID: "uuid"}, nil + } + createImageRepository(imageRepositoryConfig{ResourceKey: &resourceKey}) + + Eventually(func() bool { return isCreateRepositoryInvoked }, timeout, interval).Should(BeTrue()) + Eventually(func() bool { return isCreateRobotAccountInvoked }, timeout, interval).Should(BeTrue()) + Eventually(func() bool { return isAddPushPermissionsToRobotAccountInvoked }, timeout, interval).Should(BeTrue()) + Eventually(func() bool { return isCreateNotificationInvoked }, timeout, interval).Should(BeFalse()) + + waitImageRepositoryFinalizerOnImageRepository(resourceKey) + + imageRepository := getImageRepository(resourceKey) + Expect(imageRepository.Spec.Image.Name).To(Equal(expectedImageName)) + Expect(imageRepository.Spec.Image.Visibility).To(Equal(imagerepositoryv1alpha1.ImageVisibilityPublic)) + Expect(imageRepository.OwnerReferences).To(HaveLen(0)) + Expect(imageRepository.Status.State).To(Equal(imagerepositoryv1alpha1.ImageRepositoryStateReady)) + Expect(imageRepository.Status.Message).To(BeEmpty()) + Expect(imageRepository.Status.Image.URL).To(Equal(expectedImage)) + Expect(imageRepository.Status.Image.Visibility).To(Equal(imagerepositoryv1alpha1.ImageVisibilityPublic)) + Expect(imageRepository.Status.Credentials.PushRobotAccountName).To(HavePrefix(expectedRobotAccountPrefix)) + Expect(imageRepository.Status.Credentials.PushSecretName).To(Equal(imageRepository.Name + "-image-push")) + Expect(imageRepository.Status.Credentials.GenerationTimestamp).ToNot(BeNil()) + Expect(imageRepository.Status.Notifications).To(HaveLen(0)) + + pushSecretKey := types.NamespacedName{Name: imageRepository.Status.Credentials.PushSecretName, Namespace: imageRepository.Namespace} + pushSecret := waitSecretExist(pushSecretKey) + + defer deleteSecret(pushSecretKey) + Expect(pushSecret.OwnerReferences).To(HaveLen(1)) + Expect(pushSecret.OwnerReferences[0].Kind).To(Equal("ImageRepository")) + Expect(pushSecret.OwnerReferences[0].Name).To(Equal(imageRepository.GetName())) + Expect(pushSecret.Labels[InternalSecretLabelName]).To(Equal("true")) + Expect(pushSecret.Name).To(Equal(pushSecret.Name)) + Expect(pushSecret.Type).To(Equal(corev1.SecretTypeDockerConfigJson)) + + sa := getServiceAccount(defaultNamespace, buildPipelineServiceAccountName) + Expect(sa.Secrets).To(HaveLen(1)) + Expect(sa.ImagePullSecrets).To(HaveLen(0)) + Expect(sa.Secrets).To(ContainElement(corev1.ObjectReference{Name: pushSecret.Name})) + + pushSecretDockerconfigJson := string(pushSecret.Data[corev1.DockerConfigJsonKey]) + var authDataJson interface{} + Expect(json.Unmarshal([]byte(pushSecretDockerconfigJson), &authDataJson)).To(Succeed()) + Expect(pushSecretDockerconfigJson).To(ContainSubstring(expectedImage)) + pushSecretAuthString, err := base64.StdEncoding.DecodeString(authRegexp.FindStringSubmatch(pushSecretDockerconfigJson)[1]) + Expect(err).To(Succeed()) + pushRobotAccountName := imageRepository.Status.Credentials.PushRobotAccountName + Expect(string(pushSecretAuthString)).To(Equal(fmt.Sprintf("%s:%s", pushRobotAccountName, pushToken))) + }) + It("should cleanup repository", func() { isDeleteRobotAccountInvoked := false quay.DeleteRobotAccountFunc = func(organization, robotAccountName string) (bool, error) { @@ -247,10 +443,18 @@ var _ = Describe("Image repository controller", func() { Eventually(func() bool { return isDeleteRobotAccountInvoked }, timeout, interval).Should(BeTrue()) Eventually(func() bool { return isDeleteRepositoryInvoked }, timeout, interval).Should(BeTrue()) + + sa := getServiceAccount(defaultNamespace, buildPipelineServiceAccountName) + // verify that secret is unlinked + Expect(sa.Secrets).To(HaveLen(0)) + Expect(sa.ImagePullSecrets).To(HaveLen(0)) + + deleteServiceAccount(types.NamespacedName{Name: buildPipelineServiceAccountName, Namespace: defaultNamespace}) }) }) Context("Image repository for component provision", func() { + var resourceKey = types.NamespacedName{Name: defaultImageRepositoryName + "-componentprovision", Namespace: defaultNamespace} componentKey := types.NamespacedName{Name: defaultComponentName, Namespace: defaultNamespace} BeforeEach(func() { @@ -269,6 +473,7 @@ var _ = Describe("Image repository controller", func() { expectedImage = fmt.Sprintf("quay.io/%s/%s", quay.TestQuayOrg, expectedImageName) expectedRobotAccountPrefix = strings.ReplaceAll(strings.ReplaceAll(expectedImageName, "-", "_"), "/", "_") + createServiceAccount(defaultNamespace, buildPipelineServiceAccountName) }) assertProvisionRepository := func(updateComponentAnnotation bool) { @@ -334,6 +539,7 @@ var _ = Describe("Image repository controller", func() { } imageRepositoryConfigObject := imageRepositoryConfig{ + ResourceKey: &resourceKey, Labels: map[string]string{ ApplicationNameLabelName: defaultComponentApplication, ComponentNameLabelName: defaultComponentName, @@ -434,6 +640,11 @@ var _ = Describe("Image repository controller", func() { Expect(err).To(Succeed()) pullRobotAccountName := imageRepository.Status.Credentials.PullRobotAccountName Expect(string(pullSecretAuthString)).To(Equal(fmt.Sprintf("%s:%s", pullRobotAccountName, pullToken))) + + sa := getServiceAccount(defaultNamespace, buildPipelineServiceAccountName) + Expect(sa.Secrets).To(HaveLen(1)) + Expect(sa.ImagePullSecrets).To(HaveLen(0)) + Expect(sa.Secrets).To(ContainElement(corev1.ObjectReference{Name: pushSecret.Name})) } It("should provision image repository for component, without update component annotation", func() { @@ -562,10 +773,18 @@ var _ = Describe("Image repository controller", func() { Eventually(func() bool { return isDeleteRobotAccountForPushInvoked }, timeout, interval).Should(BeTrue()) Eventually(func() bool { return isDeleteRobotAccountForPullInvoked }, timeout, interval).Should(BeTrue()) Eventually(func() bool { return isDeleteRepositoryInvoked }, timeout, interval).Should(BeTrue()) + + sa := getServiceAccount(defaultNamespace, buildPipelineServiceAccountName) + // verify that secret is unlinked + Expect(sa.Secrets).To(HaveLen(0)) + Expect(sa.ImagePullSecrets).To(HaveLen(0)) + + deleteServiceAccount(types.NamespacedName{Name: buildPipelineServiceAccountName, Namespace: defaultNamespace}) }) }) Context("Notifications", func() { + var resourceKey = types.NamespacedName{Name: defaultImageRepositoryName + "-notification", Namespace: defaultNamespace} BeforeEach(func() { quay.ResetTestQuayClient() @@ -573,9 +792,10 @@ var _ = Describe("Image repository controller", func() { It("should prepare environment", func() { pushToken = "push-token1234" - expectedImageName = fmt.Sprintf("%s/%s", defaultNamespace, defaultImageRepositoryName) + expectedImageName = fmt.Sprintf("%s/%s", defaultNamespace, resourceKey.Name) expectedImage = fmt.Sprintf("quay.io/%s/%s", quay.TestQuayOrg, expectedImageName) expectedRobotAccountPrefix = strings.ReplaceAll(strings.ReplaceAll(expectedImageName, "-", "_"), "/", "_") + createServiceAccount(defaultNamespace, buildPipelineServiceAccountName) }) It("should provision image repository", func() { @@ -591,6 +811,7 @@ var _ = Describe("Image repository controller", func() { } createImageRepository(imageRepositoryConfig{ + ResourceKey: &resourceKey, Notifications: []imagerepositoryv1alpha1.Notifications{}, }) @@ -750,6 +971,7 @@ var _ = Describe("Image repository controller", func() { return &quay.Notification{UUID: "uuid"}, nil } createImageRepository(imageRepositoryConfig{ + ResourceKey: &resourceKey, Notifications: []imagerepositoryv1alpha1.Notifications{ { Title: "test-notification", @@ -778,15 +1000,24 @@ var _ = Describe("Image repository controller", func() { Expect(imageRepository.Status.Notifications[0].Title).To(Equal("test-notification")) Expect(imageRepository.Status.Notifications[1].Title).To(Equal("test-notification-2")) }) + + It("should clean environment", func() { + deleteServiceAccount(types.NamespacedName{Name: buildPipelineServiceAccountName, Namespace: defaultNamespace}) + }) }) Context("Other image repository scenarios", func() { + var resourceKey = types.NamespacedName{Name: defaultImageRepositoryName + "-other", Namespace: defaultNamespace} BeforeEach(func() { quay.ResetTestQuayClient() deleteImageRepository(resourceKey) }) + It("should prepare environment", func() { + createServiceAccount(defaultNamespace, buildPipelineServiceAccountName) + }) + It("should create image repository with requested name", func() { customImageName := "my-image" expectedImageName = defaultNamespace + "/" + customImageName @@ -802,7 +1033,7 @@ var _ = Describe("Image repository controller", func() { return &quay.Repository{Name: expectedImageName}, nil } - createImageRepository(imageRepositoryConfig{ImageName: customImageName}) + createImageRepository(imageRepositoryConfig{ImageName: customImageName, ResourceKey: &resourceKey}) defer deleteImageRepository(resourceKey) Eventually(func() bool { return isCreateRepositoryInvoked }, timeout, interval).Should(BeTrue()) @@ -828,7 +1059,7 @@ var _ = Describe("Image repository controller", func() { return &quay.Repository{Name: expectedImageName}, nil } - createImageRepository(imageRepositoryConfig{ImageName: customImageName}) + createImageRepository(imageRepositoryConfig{ImageName: customImageName, ResourceKey: &resourceKey}) defer deleteImageRepository(resourceKey) Eventually(func() bool { return isCreateRepositoryInvoked }, timeout, interval).Should(BeTrue()) @@ -839,9 +1070,13 @@ var _ = Describe("Image repository controller", func() { Expect(imageRepository.Spec.Image.Name).To(Equal(expectedImageName)) }) + It("should clean environment", func() { + deleteServiceAccount(types.NamespacedName{Name: buildPipelineServiceAccountName, Namespace: defaultNamespace}) + }) }) Context("Image repository error scenarios", func() { + var resourceKey = types.NamespacedName{Name: defaultImageRepositoryName + "-error", Namespace: defaultNamespace} BeforeEach(func() { quay.ResetTestQuayClient() @@ -850,9 +1085,11 @@ var _ = Describe("Image repository controller", func() { It("should prepare environment", func() { pushToken = "push-token1234" - expectedImageName = fmt.Sprintf("%s/%s", defaultNamespace, defaultImageRepositoryName) + expectedImageName = fmt.Sprintf("%s/%s", defaultNamespace, resourceKey.Name) expectedImage = fmt.Sprintf("quay.io/%s/%s", quay.TestQuayOrg, expectedImageName) expectedRobotAccountPrefix = strings.ReplaceAll(strings.ReplaceAll(expectedImageName, "-", "_"), "/", "_") + + createServiceAccount(defaultNamespace, buildPipelineServiceAccountName) }) It("should permanently fail if private image repository requested on creation but quota exceeded", func() { @@ -867,7 +1104,7 @@ var _ = Describe("Image repository controller", func() { return nil, fmt.Errorf("payment required") } - createImageRepository(imageRepositoryConfig{Visibility: "private"}) + createImageRepository(imageRepositoryConfig{Visibility: "private", ResourceKey: &resourceKey}) Eventually(func() bool { return isCreateRepositoryInvoked }, timeout, interval).Should(BeTrue()) @@ -892,7 +1129,7 @@ var _ = Describe("Image repository controller", func() { quay.CreateRobotAccountFunc = func(organization, robotName string) (*quay.RobotAccount, error) { return &quay.RobotAccount{Name: robotName, Token: pushToken}, nil } - createImageRepository(imageRepositoryConfig{}) + createImageRepository(imageRepositoryConfig{ResourceKey: &resourceKey}) defer deleteImageRepository(resourceKey) waitImageRepositoryFinalizerOnImageRepository(resourceKey) @@ -929,7 +1166,8 @@ var _ = Describe("Image repository controller", func() { quay.ResetTestQuayClientToFails() createImageRepository(imageRepositoryConfig{ - ImageName: fmt.Sprintf("%s/%s", defaultComponentApplication, defaultComponentName), + ResourceKey: &resourceKey, + ImageName: fmt.Sprintf("%s/%s", defaultComponentApplication, defaultComponentName), Labels: map[string]string{ ApplicationNameLabelName: defaultComponentApplication, ComponentNameLabelName: defaultComponentName, @@ -950,5 +1188,9 @@ var _ = Describe("Image repository controller", func() { }) Expect(k8sClient.Create(ctx, imageRepository)).ToNot(Succeed()) }) + + It("should clean environment", func() { + deleteServiceAccount(types.NamespacedName{Name: buildPipelineServiceAccountName, Namespace: defaultNamespace}) + }) }) }) diff --git a/controllers/suite_util_test.go b/controllers/suite_util_test.go index db271b9..b77bda5 100644 --- a/controllers/suite_util_test.go +++ b/controllers/suite_util_test.go @@ -246,6 +246,27 @@ func waitImageRepositoryFinalizerOnImageRepository(imageRepositoryKey types.Name }, timeout, interval).Should(BeTrue()) } +// waitImageRepositoryCredentialGone waits until Spec.Credentials section is gone +func waitImageRepositoryCredentialGone(imageRepositoryKey types.NamespacedName, operationName string) { + Eventually(func() bool { + imageRepository := getImageRepository(imageRepositoryKey) + switch operationName { + case "regenerate": + if imageRepository.Spec.Credentials.RegenerateToken == nil { + return true + } + return false + case "verify": + if imageRepository.Spec.Credentials.VerifyLinking == nil { + return true + } + return false + default: + return true + } + }, timeout, interval).Should(BeTrue()) +} + func createNamespace(name string) { namespace := corev1.Namespace{ TypeMeta: metav1.TypeMeta{ @@ -313,3 +334,17 @@ func getServiceAccount(namespace string, name string) corev1.ServiceAccount { }, timeout, interval).Should(BeTrue()) return sa } + +func deleteServiceAccount(serviceAccountKey types.NamespacedName) { + serviceAccount := &corev1.ServiceAccount{} + if err := k8sClient.Get(ctx, serviceAccountKey, serviceAccount); err != nil { + if k8sErrors.IsNotFound(err) { + return + } + Fail("Failed to get service account") + } + Expect(k8sClient.Delete(ctx, serviceAccount)).To(Succeed()) + Eventually(func() bool { + return k8sErrors.IsNotFound(k8sClient.Get(ctx, serviceAccountKey, serviceAccount)) + }, timeout, interval).Should(BeTrue()) +}