From e01f1b60ccece2f0bc853fb28966e57113b729ad Mon Sep 17 00:00:00 2001 From: Maysun Faisal <31771087+maysunfaisal@users.noreply.github.com> Date: Thu, 7 Dec 2023 00:20:39 -0500 Subject: [PATCH] [DEVHAS-492] Test for Component Devfile with Private Parent references (#426) --- .gitignore | 3 +- cdq-analysis/go.mod | 2 +- cdq-analysis/go.sum | 4 +- cdq-analysis/pkg/devfile_test.go | 3 +- controllers/component_controller.go | 41 ++++-- controllers/component_controller_test.go | 152 +++++++++++++++++++++++ controllers/start_test_env.go | 3 +- go.mod | 2 +- go.sum | 4 +- pkg/devfile/devfile.go | 3 + 10 files changed, 197 insertions(+), 20 deletions(-) diff --git a/.gitignore b/.gitignore index 2a5b5ae01..f97bb76bc 100644 --- a/.gitignore +++ b/.gitignore @@ -12,4 +12,5 @@ tmp output.log syncer.yaml .DS_Store -pact.log \ No newline at end of file +pact.log +*resource.file* diff --git a/cdq-analysis/go.mod b/cdq-analysis/go.mod index 88cbb3051..a1bb22b91 100644 --- a/cdq-analysis/go.mod +++ b/cdq-analysis/go.mod @@ -5,7 +5,7 @@ go 1.19 require ( github.com/devfile/alizer v1.2.2-0.20231004141146-f36141673c7f github.com/devfile/api/v2 v2.2.1 - github.com/devfile/library/v2 v2.2.2-0.20231130202909-20a0c910b0e6 + github.com/devfile/library/v2 v2.2.2-0.20231206202302-705f00dd96f5 github.com/devfile/registry-support/index/generator v0.0.0-20221018203505-df96d34d4273 github.com/devfile/registry-support/registry-library v0.0.0-20221018213054-47b3ffaeadba github.com/go-logr/logr v1.2.4 diff --git a/cdq-analysis/go.sum b/cdq-analysis/go.sum index 4d60b321e..b18fa89a0 100644 --- a/cdq-analysis/go.sum +++ b/cdq-analysis/go.sum @@ -311,8 +311,8 @@ github.com/devfile/api/v2 v2.2.1/go.mod h1:qp8jcw12y1JdCsxjK/7LJ7uWaJOxcY1s2LUk5 github.com/devfile/library v1.2.1-0.20211104222135-49d635cb492f/go.mod h1:uFZZdTuRqA68FVe/JoJHP92CgINyQkyWnM2Qyiim+50= github.com/devfile/library v1.2.1-0.20220308191614-f0f7e11b17de/go.mod h1:GSPfJaBg0+bBjBHbwBE5aerJLH6tWGQu2q2rHYd9czM= github.com/devfile/library/v2 v2.0.1/go.mod h1:paJ0PARAVy0br13VpBEQ4fO3rZVDxWtooQ29+23PNBk= -github.com/devfile/library/v2 v2.2.2-0.20231130202909-20a0c910b0e6 h1:4WisxE/COs2ITGxZ1yA7qs+3HSFni4wQd4ZAiaOiBOY= -github.com/devfile/library/v2 v2.2.2-0.20231130202909-20a0c910b0e6/go.mod h1:zKKhnbSLXi8vu46c5RLr+y4kY9S9Ubi0SeCm3awndsw= +github.com/devfile/library/v2 v2.2.2-0.20231206202302-705f00dd96f5 h1:RcqH+Kg4mYWXRYyjNC+tQ3rZmsHEiAjXd1mCZfaoc8Y= +github.com/devfile/library/v2 v2.2.2-0.20231206202302-705f00dd96f5/go.mod h1:zKKhnbSLXi8vu46c5RLr+y4kY9S9Ubi0SeCm3awndsw= github.com/devfile/registry-support/index/generator v0.0.0-20220222194908-7a90a4214f3e/go.mod h1:iRPBxs+ZjfLEduVXpCCIOzdD2588Zv9OCs/CcXMcCCY= github.com/devfile/registry-support/index/generator v0.0.0-20220527155645-8328a8a883be/go.mod h1:1fyDJL+fPHtcrYA6yjSVWeLmXmjCNth0d5Rq1rvtryc= github.com/devfile/registry-support/index/generator v0.0.0-20221018203505-df96d34d4273 h1:DXENQSRTEDsk9com38njPg5511DD12HPIgzyFUErnpM= diff --git a/cdq-analysis/pkg/devfile_test.go b/cdq-analysis/pkg/devfile_test.go index b025bc2e5..679829643 100644 --- a/cdq-analysis/pkg/devfile_test.go +++ b/cdq-analysis/pkg/devfile_test.go @@ -345,7 +345,8 @@ func TestValidateDevfile(t *testing.T) { ConvertKubernetesContentInUri: &convert, } - parserArgs.DevfileUtilsClient = devfileParserUtil.NewMockDevfileUtilsClient() + mockDevfileUtilsClient := devfileParserUtil.NewMockDevfileUtilsClient() + parserArgs.DevfileUtilsClient = &mockDevfileUtilsClient springDevfileParser := parserArgs springDevfileParser.URL = "https://raw.githubusercontent.com/devfile-samples/devfile-sample-java-springboot-basic/main/devfile.yaml" diff --git a/controllers/component_controller.go b/controllers/component_controller.go index 8518114e0..52918ec64 100644 --- a/controllers/component_controller.go +++ b/controllers/component_controller.go @@ -224,8 +224,11 @@ func (r *ComponentReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( for _, condition := range component.Status.Conditions { if forceGenerateGitopsResource || (condition.Type == "GitOpsResourcesGenerated" && condition.Reason == "GenerateError" && condition.Status == metav1.ConditionFalse) { log.Info(fmt.Sprintf("Re-attempting GitOps generation for %s", component.Name)) - // Parse the Component Devfile - compDevfileData, err := cdqanalysis.ParseDevfileWithParserArgs(&devfileParser.ParserArgs{Data: []byte(component.Status.Devfile), Token: gitToken}) + // Parse the Component CR Devfile + // Not necessary to pass in a Token or DevfileUtils client to the parser here since the devfileBytes has: + // 1. Already been flattened on the create reconcile, so private parents are already expanded + // 2. Kubernetes Component Uri has already been converted to inlined content with a Token if required by default on the first parse + compDevfileData, err := cdqanalysis.ParseDevfileWithParserArgs(&devfileParser.ParserArgs{Data: []byte(component.Status.Devfile)}) if err != nil { errMsg := fmt.Sprintf("Unable to parse the devfile from Component status and re-attempt GitOps generation, exiting reconcile loop %v", req.NamespacedName) log.Error(err, errMsg) @@ -401,8 +404,12 @@ func (r *ComponentReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( } if devfileLocation != "" { - // Parse the Component Devfile log.Info(fmt.Sprintf("Parsing Devfile from the Devfile location %s... %v", devfileLocation, req.NamespacedName)) + // Parse the Component CR Devfile + // Pass in a Token and a DevfileUtils client because we need to + // 1. Flatten the Devfile and access a private parent if necessary + // 2. Convert the Kubernetes Uri to Inline by default + // 3. Provide a way to mock output for Component controller tests compDevfileData, err = cdqanalysis.ParseDevfileWithParserArgs(&devfileParser.ParserArgs{URL: devfileLocation, Token: gitToken, DevfileUtilsClient: r.DevfileUtilsClient}) if err != nil { @@ -411,9 +418,12 @@ func (r *ComponentReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( return ctrl.Result{}, err } } else { - // Parse the Component Devfile log.Info(fmt.Sprintf("Parsing Devfile from the Devfile bytes %v... %v", len(devfileBytes), req.NamespacedName)) - compDevfileData, err = cdqanalysis.ParseDevfileWithParserArgs(&devfileParser.ParserArgs{Data: devfileBytes, Token: gitToken}) + // Parse the Component CR Devfile + // Not necessary to pass in a Token or a DevfileUtils client to the parser here on devfileBytes, since: + // 1. devfileBytes are only used from a DockerfileURL or an Image, Component CR source (check if conditions above on Component CR sources) + // 2. We dont access any resources for either of these cases in devfile/library + compDevfileData, err = cdqanalysis.ParseDevfileWithParserArgs(&devfileParser.ParserArgs{Data: devfileBytes}) if err != nil { log.Error(err, fmt.Sprintf("Unable to parse the devfile from Component, exiting reconcile loop %v", req.NamespacedName)) @@ -430,8 +440,11 @@ func (r *ComponentReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( } if hasApplication.Status.Devfile != "" { - // Get the devfile of the hasApp CR - hasAppDevfileData, err := cdqanalysis.ParseDevfileWithParserArgs(&devfileParser.ParserArgs{Data: []byte(hasApplication.Status.Devfile), Token: gitToken}) + // Parse the Application CR Devfile + // No need to invoke devfile/library parser with a Token or a DevfileUtils client because the Application CR Devfile model: + // 1. Is constructed in the Application controller and there is no need for a Token + // 2. Only consists of Devfile metadata attributes and projects to store the Component CR information + hasAppDevfileData, err := cdqanalysis.ParseDevfileWithParserArgs(&devfileParser.ParserArgs{Data: []byte(hasApplication.Status.Devfile)}) if err != nil { log.Error(err, fmt.Sprintf("Unable to parse the devfile from Application, exiting reconcile loop %v", req.NamespacedName)) @@ -485,8 +498,11 @@ func (r *ComponentReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( // If the model already exists, see if fields have been updated log.Info(fmt.Sprintf("Checking if the Component has been updated %v", req.NamespacedName)) - // Parse the Component Devfile - hasCompDevfileData, err := cdqanalysis.ParseDevfileWithParserArgs(&devfileParser.ParserArgs{Data: []byte(component.Status.Devfile), Token: gitToken}) + // Parse the Component CR Devfile + // Not necessary to pass in a Token or DevfileUtils client to the parser here since the devfileBytes has: + // 1. Already been flattened on the create reconcile, so private parents are already expanded + // 2. Kubernetes Component Uri has already been converted to inlined content with a Token if required by default on the first parse + hasCompDevfileData, err := cdqanalysis.ParseDevfileWithParserArgs(&devfileParser.ParserArgs{Data: []byte(component.Status.Devfile)}) if err != nil { log.Error(err, fmt.Sprintf("Unable to parse the devfile from Component status, exiting reconcile loop %v", req.NamespacedName)) @@ -501,8 +517,11 @@ func (r *ComponentReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( return ctrl.Result{}, err } - // Read the devfile again to compare it with any updates - oldCompDevfileData, err := cdqanalysis.ParseDevfileWithParserArgs(&devfileParser.ParserArgs{Data: []byte(component.Status.Devfile), Token: gitToken}) + // Parse the Component CR Devfile again to compare it with any updates + // Not necessary to pass in a Token or DevfileUtils client to the parser here since the devfileBytes has: + // 1. Already been flattened on the create reconcile, so private parents are already expanded + // 2. Kubernetes Component Uri has already been converted to inlined content with a Token if required by default on the first parse + oldCompDevfileData, err := cdqanalysis.ParseDevfileWithParserArgs(&devfileParser.ParserArgs{Data: []byte(component.Status.Devfile)}) if err != nil { log.Error(err, fmt.Sprintf("Unable to parse the devfile from Component status, exiting reconcile loop %v", req.NamespacedName)) diff --git a/controllers/component_controller_test.go b/controllers/component_controller_test.go index 5c83d1077..ba53c4858 100644 --- a/controllers/component_controller_test.go +++ b/controllers/component_controller_test.go @@ -2411,6 +2411,158 @@ var _ = Describe("Component controller", func() { // Delete the specified HASApp resource deleteHASAppCR(hasAppLookupKey) }) + + Context("Create Private Component with basic field set and a private parent uri", func() { + It("Should create successfully and update the Application", func() { + ctx := context.Background() + + applicationName := HASAppName + "26" + componentName := HASCompName + "26" + + originalPort := 1111 + updatedPort := 2222 + + // Create a git secret + tokenSecret := &corev1.Secret{ + TypeMeta: metav1.TypeMeta{ + Kind: "Secret", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: componentName, + Namespace: HASAppNamespace, + }, + StringData: map[string]string{ + "password": "parent-devfile", // notsecret - see mock implementation in devfile/library https://github.com/devfile/library/blob/main/pkg/util/mock.go + }, + } + + Expect(k8sClient.Create(ctx, tokenSecret)).Should(Succeed()) + + createAndFetchSimpleApp(applicationName, HASAppNamespace, DisplayName, Description) + + hasComp := &appstudiov1alpha1.Component{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "appstudio.redhat.com/v1alpha1", + Kind: "Component", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: componentName, + Namespace: HASAppNamespace, + }, + Spec: appstudiov1alpha1.ComponentSpec{ + ComponentName: ComponentName, + Application: applicationName, + Secret: componentName, + Source: appstudiov1alpha1.ComponentSource{ + ComponentSourceUnion: appstudiov1alpha1.ComponentSourceUnion{ + GitSource: &appstudiov1alpha1.GitSource{ + URL: "https://github.com/maysunfaisal/devfile-sample-python-basic-private", // It doesn't matter if we are using pub/pvt repo here. We are mock testing the token, "parent-devfile" returns a mock devfile and mock parent. See https://github.com/devfile/library/blob/main/pkg/util/mock.go + }, + }, + }, + TargetPort: originalPort, + }, + } + Expect(k8sClient.Create(ctx, hasComp)).Should(Succeed()) + + // Look up the has app resource that was created. + // num(conditions) may still be < 2 (GeneratedGitOps, Created) on the first try, so retry until at least _some_ condition is set + hasCompLookupKey := types.NamespacedName{Name: componentName, Namespace: HASAppNamespace} + createdHasComp := &appstudiov1alpha1.Component{} + Eventually(func() bool { + k8sClient.Get(context.Background(), hasCompLookupKey, createdHasComp) + return len(createdHasComp.Status.Conditions) > 1 && createdHasComp.Status.GitOps.RepositoryURL != "" + }, timeout, interval).Should(BeTrue()) + + // Verify that the GitOpsGenerated status condition was also set + // ToDo: Add helper func for accessing the status conditions in a better way + gitopsCondition := createdHasComp.Status.Conditions[len(createdHasComp.Status.Conditions)-2] + Expect(gitopsCondition.Type).To(Equal("GitOpsResourcesGenerated")) + Expect(gitopsCondition.Status).To(Equal(metav1.ConditionTrue)) + + // Make sure the devfile model was properly set in Component + Expect(createdHasComp.Status.Devfile).Should(Not(Equal(""))) + + hasAppLookupKey := types.NamespacedName{Name: applicationName, Namespace: HASAppNamespace} + createdHasApp := &appstudiov1alpha1.Application{} + Eventually(func() bool { + k8sClient.Get(context.Background(), hasAppLookupKey, createdHasApp) + return len(createdHasApp.Status.Conditions) > 0 && strings.Contains(createdHasApp.Status.Devfile, ComponentName) + }, timeout, interval).Should(BeTrue()) + + // Make sure the devfile model was properly set in Application + Expect(createdHasApp.Status.Devfile).Should(Not(Equal(""))) + + // Check the Component devfile + _, err := cdqanalysis.ParseDevfileWithParserArgs(&parser.ParserArgs{Data: []byte(createdHasComp.Status.Devfile)}) + Expect(err).Should(Not(HaveOccurred())) + + // Check the HAS Application devfile + hasAppDevfile, err := cdqanalysis.ParseDevfileWithParserArgs(&parser.ParserArgs{Data: []byte(createdHasApp.Status.Devfile)}) + + Expect(err).Should(Not(HaveOccurred())) + + // gitOpsRepo and appModelRepo should both be set + Expect(string(hasAppDevfile.GetMetadata().Attributes["gitOpsRepository.url"].Raw)).Should(Not(Equal(""))) + Expect(string(hasAppDevfile.GetMetadata().Attributes["appModelRepository.url"].Raw)).Should(Not(Equal(""))) + + // gitOpsRepo set in the component equal the repository in the app cr's devfile + gitopsRepo := hasAppDevfile.GetMetadata().Attributes.GetString("gitOpsRepository.url", &err) + Expect(err).Should(Not(HaveOccurred())) + Expect(string(createdHasComp.Status.GitOps.RepositoryURL)).Should(Equal(gitopsRepo)) + + // Commit ID should be set in the gitops repository and not be empty + Expect(createdHasComp.Status.GitOps.CommitID).Should(Not(BeEmpty())) + + hasProjects, err := hasAppDevfile.GetProjects(common.DevfileOptions{}) + Expect(err).Should(Not(HaveOccurred())) + Expect(len(hasProjects)).ShouldNot(Equal(0)) + + nameMatched := false + repoLinkMatched := false + for _, project := range hasProjects { + if project.Name == ComponentName { + nameMatched = true + } + if project.Git != nil && project.Git.GitLikeProjectSource.Remotes["origin"] == "https://github.com/maysunfaisal/devfile-sample-python-basic-private" { + repoLinkMatched = true + } + } + Expect(nameMatched).Should(Equal(true)) + Expect(repoLinkMatched).Should(Equal(true)) + + // Update Component + createdHasComp.Spec.TargetPort = updatedPort + + Expect(k8sClient.Update(ctx, createdHasComp)).Should(Succeed()) + + updatedHasComp := &appstudiov1alpha1.Component{} + Eventually(func() bool { + k8sClient.Get(context.Background(), hasCompLookupKey, updatedHasComp) + return updatedHasComp.Status.Conditions[len(updatedHasComp.Status.Conditions)-1].Type == "Updated" + }, timeout, interval).Should(BeTrue()) + + // Make sure the devfile model was properly set in Component + Expect(updatedHasComp.Status.Devfile).Should(Not(Equal(""))) + + // Check the Component updated devfile + hasCompUpdatedDevfile, err := cdqanalysis.ParseDevfileWithParserArgs(&parser.ParserArgs{Data: []byte(updatedHasComp.Status.Devfile)}) + + Expect(err).Should(Not(HaveOccurred())) + + checklist := updateChecklist{ + port: updatedPort, + } + + verifyHASComponentUpdates(hasCompUpdatedDevfile, checklist, nil) + + // Delete the specified HASComp resource + deleteHASCompCR(hasCompLookupKey) + + // Delete the specified HASApp resource + deleteHASAppCR(hasAppLookupKey) + }) + }) }) }) diff --git a/controllers/start_test_env.go b/controllers/start_test_env.go index 1bb2242f6..2886a96fb 100644 --- a/controllers/start_test_env.go +++ b/controllers/start_test_env.go @@ -96,6 +96,7 @@ func SetupTestEnv() (client.Client, *envtest.Environment, context.Context, conte gomega.Expect(err).ToNot(gomega.HaveOccurred()) mockGhTokenClient := github.MockGitHubTokenClient{} + mockDevfileUtilsClient := devfileParserUtil.NewMockDevfileUtilsClient() // Retrieve the option to specify a cdq-analysis image cdqAnalysisImage := os.Getenv("CDQ_ANALYSIS_IMAGE") @@ -123,7 +124,7 @@ func SetupTestEnv() (client.Client, *envtest.Environment, context.Context, conte K8sClient: k8sClient, }, GitHubTokenClient: mockGhTokenClient, - DevfileUtilsClient: devfileParserUtil.NewMockDevfileUtilsClient(), + DevfileUtilsClient: &mockDevfileUtilsClient, }).SetupWithManager(ctx, k8sManager) gomega.Expect(err).ToNot(gomega.HaveOccurred()) diff --git a/go.mod b/go.mod index 5181ff436..55fdf652b 100644 --- a/go.mod +++ b/go.mod @@ -5,7 +5,7 @@ go 1.19 require ( github.com/brianvoe/gofakeit/v6 v6.9.0 github.com/devfile/api/v2 v2.2.1 - github.com/devfile/library/v2 v2.2.2-0.20231130202909-20a0c910b0e6 + github.com/devfile/library/v2 v2.2.2-0.20231206202302-705f00dd96f5 github.com/go-logr/logr v1.2.4 github.com/gofri/go-github-ratelimit v1.0.3-0.20230428184158-a500e14de53f github.com/golang/mock v1.6.0 diff --git a/go.sum b/go.sum index dd8c2ea9a..187ab47b1 100644 --- a/go.sum +++ b/go.sum @@ -484,8 +484,8 @@ github.com/devfile/api/v2 v2.2.1/go.mod h1:qp8jcw12y1JdCsxjK/7LJ7uWaJOxcY1s2LUk5 github.com/devfile/library v1.2.1-0.20211104222135-49d635cb492f/go.mod h1:uFZZdTuRqA68FVe/JoJHP92CgINyQkyWnM2Qyiim+50= github.com/devfile/library v1.2.1-0.20220308191614-f0f7e11b17de/go.mod h1:GSPfJaBg0+bBjBHbwBE5aerJLH6tWGQu2q2rHYd9czM= github.com/devfile/library/v2 v2.0.1/go.mod h1:paJ0PARAVy0br13VpBEQ4fO3rZVDxWtooQ29+23PNBk= -github.com/devfile/library/v2 v2.2.2-0.20231130202909-20a0c910b0e6 h1:4WisxE/COs2ITGxZ1yA7qs+3HSFni4wQd4ZAiaOiBOY= -github.com/devfile/library/v2 v2.2.2-0.20231130202909-20a0c910b0e6/go.mod h1:zKKhnbSLXi8vu46c5RLr+y4kY9S9Ubi0SeCm3awndsw= +github.com/devfile/library/v2 v2.2.2-0.20231206202302-705f00dd96f5 h1:RcqH+Kg4mYWXRYyjNC+tQ3rZmsHEiAjXd1mCZfaoc8Y= +github.com/devfile/library/v2 v2.2.2-0.20231206202302-705f00dd96f5/go.mod h1:zKKhnbSLXi8vu46c5RLr+y4kY9S9Ubi0SeCm3awndsw= github.com/devfile/registry-support/index/generator v0.0.0-20220222194908-7a90a4214f3e/go.mod h1:iRPBxs+ZjfLEduVXpCCIOzdD2588Zv9OCs/CcXMcCCY= github.com/devfile/registry-support/index/generator v0.0.0-20220527155645-8328a8a883be/go.mod h1:1fyDJL+fPHtcrYA6yjSVWeLmXmjCNth0d5Rq1rvtryc= github.com/devfile/registry-support/index/generator v0.0.0-20221018203505-df96d34d4273 h1:DXENQSRTEDsk9com38njPg5511DD12HPIgzyFUErnpM= diff --git a/pkg/devfile/devfile.go b/pkg/devfile/devfile.go index d3fbf48b2..40642599c 100644 --- a/pkg/devfile/devfile.go +++ b/pkg/devfile/devfile.go @@ -74,6 +74,9 @@ func GetResourceFromDevfile(log logr.Logger, devfileData data.DevfileData, deplo src := parser.YamlSrc{ Data: []byte(component.Kubernetes.Inlined), } + // No need to pass in the Token or the DevfileUtils client because + // it is inlined data and the file has already been parsed by the devfile/library + // on the initial parse and stored as inlined content. values, err := parser.ReadKubernetesYaml(src, nil, nil) if err != nil { return parser.KubernetesResources{}, err