From 2dac049bf4c7f33ecd418751c5247ef2b2a7c54d Mon Sep 17 00:00:00 2001 From: Robert Cerven Date: Fri, 29 Nov 2024 22:38:25 +0100 Subject: [PATCH] when 2 components are create with the same URL, only one PaC repository will be created with ownership of 2 components when 1st component is removed, repository with the name of 1st component remains, with ownership only for 2nd component when 1st component is re-created it checks if repository exists with the same name, but it should ONLY use it when URL is actually the same, if it isn't it will create new PaC repository with name of the component and random suffix STONEBLD-2899 Signed-off-by: Robert Cerven --- ...mponent_build_controller_pac_repository.go | 56 ++++++---- .../component_build_controller_test.go | 100 ++++++++++++++++++ 2 files changed, 133 insertions(+), 23 deletions(-) diff --git a/controllers/component_build_controller_pac_repository.go b/controllers/component_build_controller_pac_repository.go index 33ab3c5b..f2ff6348 100644 --- a/controllers/component_build_controller_pac_repository.go +++ b/controllers/component_build_controller_pac_repository.go @@ -106,37 +106,47 @@ func (r *ComponentBuildReconciler) ensurePaCRepository(ctx context.Context, comp } existingRepository := &pacv1alpha1.Repository{} - if err := r.Client.Get(ctx, types.NamespacedName{Name: repository.Name, Namespace: repository.Namespace}, existingRepository); err != nil { - if errors.IsNotFound(err) { - if err := controllerutil.SetOwnerReference(component, repository, r.Scheme); err != nil { - return err - } - if err := r.Client.Create(ctx, repository); err != nil { - if strings.Contains(err.Error(), "repository already exist") { - // PaC admission webhook denied creation of the PaC repository, - // because PaC repository object that references the same git repository already exists. - log.Info("An attempt to create second PaC Repository for the same git repository", "GitRepository", repository.Spec.URL, l.Action, l.ActionAdd, l.Audit, "true") - return boerrors.NewBuildOpError(boerrors.EPaCDuplicateRepository, err) - } + err = r.Client.Get(ctx, types.NamespacedName{Name: repository.Name, Namespace: repository.Namespace}, existingRepository) + if err == nil { + gitUrl := strings.TrimSuffix(strings.TrimSuffix(component.Spec.Source.GitSource.URL, ".git"), "/") - if strings.Contains(err.Error(), "denied the request: failed to validate") { - // PaC admission webhook denied creation of the PaC repository, - // because PaC repository object references not allowed git repository url. + if existingRepository.Spec.URL == gitUrl { + return nil + } + // repository with the same name exists but with different git URL, add random string to repository name + repository.ObjectMeta.Name = fmt.Sprintf("%s-%s", repository.ObjectMeta.Name, RandomString(5)) + } - log.Info("An attempt to create PaC Repository for not allowed repository url", "GitRepository", repository.Spec.URL, l.Action, l.ActionAdd, l.Audit, "true") - return boerrors.NewBuildOpError(boerrors.EPaCNotAllowedRepositoryUrl, err) - } + // create repository if not found or when found but with different URL + if (err != nil && errors.IsNotFound(err)) || err == nil { + if err := controllerutil.SetOwnerReference(component, repository, r.Scheme); err != nil { + return err + } + if err := r.Client.Create(ctx, repository); err != nil { + if strings.Contains(err.Error(), "repository already exist") { + // PaC admission webhook denied creation of the PaC repository, + // because PaC repository object that references the same git repository already exists. + log.Info("An attempt to create second PaC Repository for the same git repository", "GitRepository", repository.Spec.URL, l.Action, l.ActionAdd, l.Audit, "true") + return boerrors.NewBuildOpError(boerrors.EPaCDuplicateRepository, err) + } - log.Error(err, "failed to create Component PaC repository object", l.Action, l.ActionAdd) - return err + if strings.Contains(err.Error(), "denied the request: failed to validate") { + // PaC admission webhook denied creation of the PaC repository, + // because PaC repository object references not allowed git repository url. + log.Info("An attempt to create PaC Repository for not allowed repository url", "GitRepository", repository.Spec.URL, l.Action, l.ActionAdd, l.Audit, "true") + return boerrors.NewBuildOpError(boerrors.EPaCNotAllowedRepositoryUrl, err) } - log.Info("Created PaC Repository object for the component") - } else { - log.Error(err, "failed to get Component PaC repository object", l.Action, l.ActionView) + log.Error(err, "failed to create Component PaC repository object", l.Action, l.ActionAdd) return err } + log.Info("Created PaC Repository object for the component", "RepositoryName", repository.ObjectMeta.Name) + + } else { + log.Error(err, "failed to get Component PaC repository object", l.Action, l.ActionView) + return err } + return nil } diff --git a/controllers/component_build_controller_test.go b/controllers/component_build_controller_test.go index 80fe0511..154904cf 100644 --- a/controllers/component_build_controller_test.go +++ b/controllers/component_build_controller_test.go @@ -1323,6 +1323,106 @@ var _ = Describe("Component initial build controller", func() { Expect(k8sClient.List(ctx, pacRepositoriesList, &client.ListOptions{Namespace: component1Key.Namespace})).To(Succeed()) Expect(pacRepositoriesList.Items).To(HaveLen(2)) // 2-nd repository for the anotherComponentKey component }) + + It("when 2 components are created with the same url, Pac Repository will be reused, when 1st component is removed and created with new URL, new repository is created", func() { + deleteComponent(anotherComponentKey) + deletePaCRepository(anotherComponentKey) + component1Key := types.NamespacedName{Name: "test-multi-component1", Namespace: HASAppNamespace} + component2Key := types.NamespacedName{Name: "test-multi-component2", Namespace: HASAppNamespace} + pacRepositoriesList := &pacv1alpha1.RepositoryList{} + pacRepository := &pacv1alpha1.Repository{} + + Expect(k8sClient.List(ctx, pacRepositoriesList, &client.ListOptions{Namespace: component1Key.Namespace})).To(Succeed()) + Expect(pacRepositoriesList.Items).To(HaveLen(0)) + + component1PaCMergeRequestCreated := false + component2PaCMergeRequestCreated := false + EnsurePaCMergeRequestFunc = func(repoUrl string, d *gp.MergeRequestData) (string, error) { + defer GinkgoRecover() + if strings.Contains(d.Files[0].FullPath, component1Key.Name) { + component1PaCMergeRequestCreated = true + return "url1", nil + } else if strings.Contains(d.Files[0].FullPath, component2Key.Name) { + component2PaCMergeRequestCreated = true + return "url2", nil + } else { + Fail("Unknown component in EnsurePaCMergeRequest") + } + return "", nil + } + + // create 1st component with shared URL + createCustomComponentWithBuildRequest(componentConfig{ + componentKey: component1Key, + annotations: defaultPipelineAnnotations, + gitURL: multiComponentGitRepositoryUrl, + }, BuildRequestConfigurePaCAnnotationValue) + waitComponentAnnotationGone(component1Key, BuildRequestAnnotationName) + Eventually(func() bool { return component1PaCMergeRequestCreated }, timeout, interval).Should(BeTrue()) + waitPaCRepositoryCreated(component1Key) + + Expect(k8sClient.Get(ctx, component1Key, pacRepository)).To(Succeed()) + Expect(pacRepository.OwnerReferences).To(HaveLen(1)) + Expect(pacRepository.Spec.URL).To(Equal(multiComponentGitRepositoryUrl)) + Expect(k8sClient.List(ctx, pacRepositoriesList, &client.ListOptions{Namespace: component1Key.Namespace})).To(Succeed()) + Expect(pacRepositoriesList.Items).To(HaveLen(1)) + + // create 2nd component with shared URL + createCustomComponentWithBuildRequest(componentConfig{ + componentKey: component2Key, + annotations: defaultPipelineAnnotations, + gitURL: multiComponentGitRepositoryUrl, + }, BuildRequestConfigurePaCAnnotationValue) + waitComponentAnnotationGone(component2Key, BuildRequestAnnotationName) + Eventually(func() bool { return component2PaCMergeRequestCreated }, timeout, interval).Should(BeTrue()) + + Expect(k8sClient.Get(ctx, component1Key, pacRepository)).To(Succeed()) + Expect(pacRepository.OwnerReferences).To(HaveLen(2)) + Expect(pacRepository.OwnerReferences[0].Name).To(Equal(component1Key.Name)) + Expect(pacRepository.OwnerReferences[0].Kind).To(Equal("Component")) + Expect(pacRepository.OwnerReferences[1].Name).To(Equal(component2Key.Name)) + Expect(pacRepository.OwnerReferences[1].Kind).To(Equal("Component")) + Expect(k8sClient.List(ctx, pacRepositoriesList, &client.ListOptions{Namespace: component1Key.Namespace})).To(Succeed()) + Expect(pacRepositoriesList.Items).To(HaveLen(1)) + + // remove 1st component + deleteComponent(component1Key) + Expect(k8sClient.Get(ctx, component1Key, pacRepository)).To(Succeed()) + + // create 1st component again, but with different URL, which will create new Pac repository and not use the one which has same name as component + differentGitRepositoryUrl := "https://github.com/samples/multi-different-component-repository" + createCustomComponentWithBuildRequest(componentConfig{ + componentKey: component1Key, + annotations: defaultPipelineAnnotations, + gitURL: differentGitRepositoryUrl, + }, BuildRequestConfigurePaCAnnotationValue) + waitComponentAnnotationGone(component1Key, BuildRequestAnnotationName) + Eventually(func() bool { return component1PaCMergeRequestCreated }, timeout, interval).Should(BeTrue()) + + Expect(k8sClient.List(ctx, pacRepositoriesList, &client.ListOptions{Namespace: component1Key.Namespace})).To(Succeed()) + Expect(pacRepositoriesList.Items).To(HaveLen(2)) // new repository will be created with added randomStr suffix + originalRepositoryFound := false + newRepositoryFound := false + for _, repo := range pacRepositoriesList.Items { + // original repository will be still named after component1 even though component1 was removed, because component2 was using it + if repo.Name == component1Key.Name { + Expect(repo.Spec.URL).To(Equal(multiComponentGitRepositoryUrl)) + // not testing ownerReferences because it will still have 2 references even after component1 removal, because + // in tests we aren't running controller which removes references from Repository upon component removal + originalRepositoryFound = true + // new repository for component1 will be named with prefix of component1 and random string, because component1 name repository has different url + } else if strings.HasPrefix(repo.Name, fmt.Sprintf("%s-", component1Key.Name)) { + Expect(repo.Spec.URL).To(Equal(differentGitRepositoryUrl)) + Expect(repo.OwnerReferences).To(HaveLen(1)) + Expect(repo.OwnerReferences[0].Name).To(Equal(component1Key.Name)) + Expect(repo.OwnerReferences[0].Kind).To(Equal("Component")) + newRepositoryFound = true + } + } + Expect(originalRepositoryFound).To(BeTrue()) + Expect(newRepositoryFound).To(BeTrue()) + }) + }) Context("Test Pipelines as Code trigger build", func() {