From eb26cefd10eb76051fd09a6d0c32d770b04526fe Mon Sep 17 00:00:00 2001 From: thepetk Date: Tue, 19 Mar 2024 13:35:45 +0000 Subject: [PATCH 01/21] Return error for non github urls Signed-off-by: thepetk --- cdq-analysis/pkg/util.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/cdq-analysis/pkg/util.go b/cdq-analysis/pkg/util.go index e17ce8acd..c247ff355 100644 --- a/cdq-analysis/pkg/util.go +++ b/cdq-analysis/pkg/util.go @@ -170,8 +170,11 @@ func ConvertGitHubURL(URL string, revision string, context string) (string, erro if err != nil { return "", &InvalidURL{URL: URL, Err: err} } + if !strings.Contains(url.Host, "github") { + return "", &InvalidURL{URL: URL, Err: fmt.Errorf("url %v not from github.com")} + } - if strings.Contains(url.Host, "github") && !strings.Contains(url.Host, "raw") { + if !strings.Contains(url.Host, "raw") { // Convert path part of the URL URLSlice := strings.Split(URL, "/") if len(URLSlice) > 2 && URLSlice[len(URLSlice)-2] == "tree" { From 69b04cfdeb491d0355345864e6ae145ca32b2fdc Mon Sep 17 00:00:00 2001 From: thepetk Date: Tue, 19 Mar 2024 13:40:09 +0000 Subject: [PATCH 02/21] Add test cases for gitlab urls Signed-off-by: thepetk --- cdq-analysis/pkg/util_test.go | 5 ++ controllers/component_controller_test.go | 79 +++++++++++++++++++++--- 2 files changed, 76 insertions(+), 8 deletions(-) diff --git a/cdq-analysis/pkg/util_test.go b/cdq-analysis/pkg/util_test.go index 42a967fb7..11d9c53f8 100644 --- a/cdq-analysis/pkg/util_test.go +++ b/cdq-analysis/pkg/util_test.go @@ -317,6 +317,11 @@ func TestConvertGitHubURL(t *testing.T) { url: "https://github.com/devfile/api/tree/2.1.x", wantUrl: "https://raw.githubusercontent.com/devfile/api/2.1.x", }, + { + name: "A non github url", + url: "https://gitlab.com/", + wantErr: true, + }, { name: "A non url", url: "\000x", diff --git a/controllers/component_controller_test.go b/controllers/component_controller_test.go index ad6ffc591..69875b426 100644 --- a/controllers/component_controller_test.go +++ b/controllers/component_controller_test.go @@ -52,14 +52,15 @@ var _ = Describe("Component controller", func() { // Define utility constants for object names and testing timeouts/durations and intervals. const ( - HASAppName = "test-application" - HASCompName = "test-component" - HASAppNamespace = "default" - DisplayName = "petclinic" - Description = "Simple petclinic app" - ComponentName = "backend" - SampleRepoLink = "https://github.com/devfile-samples/devfile-sample-java-springboot-basic" - gitToken = "" //empty for public repo test + HASAppName = "test-application" + HASCompName = "test-component" + HASAppNamespace = "default" + DisplayName = "petclinic" + Description = "Simple petclinic app" + ComponentName = "backend" + SampleRepoLink = "https://github.com/devfile-samples/devfile-sample-java-springboot-basic" + SampleGitlabRepoLink = "https://gitlab.com/devfile-samples/devfile-sample-java-springboot-basic" + gitToken = "" //empty for public repo test ) prometheus.MustRegister(metrics.GetComponentCreationTotalReqs(), metrics.GetComponentCreationFailed(), metrics.GetComponentCreationSucceeded()) @@ -2953,6 +2954,68 @@ var _ = Describe("Component controller", func() { // Delete the specified HASComp resource deleteHASCompCR(hasCompLookupKey) + // Delete the specified HASApp resource + deleteHASAppCR(hasAppLookupKey) + }) + }) + Context("Create component having git source from gitlab", func() { + It("Should return a user error for invalid url", func() { + beforeCreateTotalReqs := testutil.ToFloat64(metrics.GetComponentCreationTotalReqs()) + beforeCreateSucceedReqs := testutil.ToFloat64(metrics.GetComponentCreationSucceeded()) + beforeCreateFailedReqs := testutil.ToFloat64(metrics.GetComponentCreationFailed()) + + ctx := context.Background() + + applicationName := HASAppName + "28" + componentName := HASCompName + "28" + + 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, + Source: appstudiov1alpha1.ComponentSource{ + ComponentSourceUnion: appstudiov1alpha1.ComponentSourceUnion{ + GitSource: &appstudiov1alpha1.GitSource{ + URL: SampleGitlabRepoLink, + }, + }, + }, + }, + } + Expect(k8sClient.Create(ctx, hasComp)).Should(Succeed()) + + // Look up the has app resource that was created. + // num(conditions) may still be < 1 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(ctx, hasCompLookupKey, createdHasComp) + return len(createdHasComp.Status.Conditions) > 0 + }, timeout, interval).Should(BeTrue()) + + // Make sure the err was set + Expect(createdHasComp.Status.Conditions[len(createdHasComp.Status.Conditions)-1].Reason).Should(Equal("Error")) + Expect(strings.ToLower(createdHasComp.Status.Conditions[len(createdHasComp.Status.Conditions)-1].Message)).Should(ContainSubstring("invalid URL")) + + hasAppLookupKey := types.NamespacedName{Name: applicationName, Namespace: HASAppNamespace} + + Expect(testutil.ToFloat64(metrics.GetComponentCreationTotalReqs()) > beforeCreateTotalReqs).To(BeTrue()) + Expect(testutil.ToFloat64(metrics.GetComponentCreationSucceeded()) > beforeCreateSucceedReqs).To(BeTrue()) + Expect(testutil.ToFloat64(metrics.GetComponentCreationFailed()) == beforeCreateFailedReqs).To(BeTrue()) + + // Delete the specified HASComp resource + deleteHASCompCR(hasCompLookupKey) + // Delete the specified HASApp resource deleteHASAppCR(hasAppLookupKey) }) From abaa558eba97d6e30f5589d7f40c0ba5ba99bf22 Mon Sep 17 00:00:00 2001 From: thepetk Date: Tue, 19 Mar 2024 13:55:02 +0000 Subject: [PATCH 03/21] Fix test cases Signed-off-by: thepetk --- cdq-analysis/pkg/util.go | 2 +- controllers/component_controller_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cdq-analysis/pkg/util.go b/cdq-analysis/pkg/util.go index c247ff355..d0bc32fcc 100644 --- a/cdq-analysis/pkg/util.go +++ b/cdq-analysis/pkg/util.go @@ -171,7 +171,7 @@ func ConvertGitHubURL(URL string, revision string, context string) (string, erro return "", &InvalidURL{URL: URL, Err: err} } if !strings.Contains(url.Host, "github") { - return "", &InvalidURL{URL: URL, Err: fmt.Errorf("url %v not from github.com")} + return "", &InvalidURL{URL: URL, Err: fmt.Errorf("url %v is not from github.com", URL)} } if !strings.Contains(url.Host, "raw") { diff --git a/controllers/component_controller_test.go b/controllers/component_controller_test.go index 69875b426..39ea2cbaf 100644 --- a/controllers/component_controller_test.go +++ b/controllers/component_controller_test.go @@ -3005,7 +3005,7 @@ var _ = Describe("Component controller", func() { // Make sure the err was set Expect(createdHasComp.Status.Conditions[len(createdHasComp.Status.Conditions)-1].Reason).Should(Equal("Error")) - Expect(strings.ToLower(createdHasComp.Status.Conditions[len(createdHasComp.Status.Conditions)-1].Message)).Should(ContainSubstring("invalid URL")) + Expect(strings.ToLower(createdHasComp.Status.Conditions[len(createdHasComp.Status.Conditions)-1].Message)).Should(ContainSubstring("invalid url")) hasAppLookupKey := types.NamespacedName{Name: applicationName, Namespace: HASAppNamespace} From 74872b5a61a512f025a50955ea8b117b81ed0584 Mon Sep 17 00:00:00 2001 From: thepetk Date: Tue, 19 Mar 2024 15:57:59 +0000 Subject: [PATCH 04/21] Move the github url check to component controller only Signed-off-by: thepetk --- cdq-analysis/pkg/util.go | 5 +---- cdq-analysis/pkg/util_test.go | 2 +- controllers/component_controller.go | 19 +++++++++++++++++++ controllers/component_controller_test.go | 6 +----- 4 files changed, 22 insertions(+), 10 deletions(-) diff --git a/cdq-analysis/pkg/util.go b/cdq-analysis/pkg/util.go index d0bc32fcc..e17ce8acd 100644 --- a/cdq-analysis/pkg/util.go +++ b/cdq-analysis/pkg/util.go @@ -170,11 +170,8 @@ func ConvertGitHubURL(URL string, revision string, context string) (string, erro if err != nil { return "", &InvalidURL{URL: URL, Err: err} } - if !strings.Contains(url.Host, "github") { - return "", &InvalidURL{URL: URL, Err: fmt.Errorf("url %v is not from github.com", URL)} - } - if !strings.Contains(url.Host, "raw") { + if strings.Contains(url.Host, "github") && !strings.Contains(url.Host, "raw") { // Convert path part of the URL URLSlice := strings.Split(URL, "/") if len(URLSlice) > 2 && URLSlice[len(URLSlice)-2] == "tree" { diff --git a/cdq-analysis/pkg/util_test.go b/cdq-analysis/pkg/util_test.go index 11d9c53f8..068f6bf6d 100644 --- a/cdq-analysis/pkg/util_test.go +++ b/cdq-analysis/pkg/util_test.go @@ -320,7 +320,7 @@ func TestConvertGitHubURL(t *testing.T) { { name: "A non github url", url: "https://gitlab.com/", - wantErr: true, + wantUrl: "https://gitlab.com/", }, { name: "A non url", diff --git a/controllers/component_controller.go b/controllers/component_controller.go index 682e6c83d..36b2213ae 100644 --- a/controllers/component_controller.go +++ b/controllers/component_controller.go @@ -298,6 +298,12 @@ func (r *ComponentReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( var devfileBytes []byte if source.GitSource != nil && source.GitSource.URL != "" { + if !isGithubURL(source.GitSource.URL) { + // User error - the git url provided is not from github + log.Error("source git url %v is not from github", source.GitSource.URL) + metrics.IncrementComponentCreationSucceeded(prevErrCondition, err.Error()) + return ctrl.Result{}, nil + } context := source.GitSource.Context // If a Git secret was passed in, retrieve it for use in our Git operations // The secret needs to be in the same namespace as the Component @@ -884,3 +890,16 @@ func checkForCreateReconcile(component appstudiov1alpha1.Component) (bool, strin // If there are no Conditions or no Updated Condition, then this is a Create reconcile return true, errCondition } + + +func isGithubURL(URL string) (bool, error) { + url, err := url.Parse(URL) + if err != nil { + return false, &InvalidURL{URL: URL, Err: err} + } + + if strings.Contains(url.Host, "github") { + return true, nil + } + return false, nil +} \ No newline at end of file diff --git a/controllers/component_controller_test.go b/controllers/component_controller_test.go index 39ea2cbaf..b60938452 100644 --- a/controllers/component_controller_test.go +++ b/controllers/component_controller_test.go @@ -2959,7 +2959,7 @@ var _ = Describe("Component controller", func() { }) }) Context("Create component having git source from gitlab", func() { - It("Should return a user error for invalid url", func() { + It("Should not increase the component failure metrics", func() { beforeCreateTotalReqs := testutil.ToFloat64(metrics.GetComponentCreationTotalReqs()) beforeCreateSucceedReqs := testutil.ToFloat64(metrics.GetComponentCreationSucceeded()) beforeCreateFailedReqs := testutil.ToFloat64(metrics.GetComponentCreationFailed()) @@ -3003,10 +3003,6 @@ var _ = Describe("Component controller", func() { return len(createdHasComp.Status.Conditions) > 0 }, timeout, interval).Should(BeTrue()) - // Make sure the err was set - Expect(createdHasComp.Status.Conditions[len(createdHasComp.Status.Conditions)-1].Reason).Should(Equal("Error")) - Expect(strings.ToLower(createdHasComp.Status.Conditions[len(createdHasComp.Status.Conditions)-1].Message)).Should(ContainSubstring("invalid url")) - hasAppLookupKey := types.NamespacedName{Name: applicationName, Namespace: HASAppNamespace} Expect(testutil.ToFloat64(metrics.GetComponentCreationTotalReqs()) > beforeCreateTotalReqs).To(BeTrue()) From 4f9c975ccd608b09b545eaadcb6584a09cbf2f7a Mon Sep 17 00:00:00 2001 From: thepetk Date: Tue, 19 Mar 2024 16:05:25 +0000 Subject: [PATCH 05/21] Fix isGithubUrl func Signed-off-by: thepetk --- controllers/component_controller.go | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/controllers/component_controller.go b/controllers/component_controller.go index 36b2213ae..1ce101366 100644 --- a/controllers/component_controller.go +++ b/controllers/component_controller.go @@ -19,14 +19,14 @@ package controllers import ( "context" "fmt" + corev1 "k8s.io/api/core/v1" + "net/url" "os" "path/filepath" "reflect" "strings" "time" - corev1 "k8s.io/api/core/v1" - "github.com/prometheus/client_golang/prometheus" cdqanalysis "github.com/redhat-appstudio/application-service/cdq-analysis/pkg" "github.com/redhat-appstudio/application-service/pkg/metrics" @@ -300,7 +300,7 @@ func (r *ComponentReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( if source.GitSource != nil && source.GitSource.URL != "" { if !isGithubURL(source.GitSource.URL) { // User error - the git url provided is not from github - log.Error("source git url %v is not from github", source.GitSource.URL) + log.Info("source git url %v is not from github", source.GitSource.URL) metrics.IncrementComponentCreationSucceeded(prevErrCondition, err.Error()) return ctrl.Result{}, nil } @@ -891,15 +891,16 @@ func checkForCreateReconcile(component appstudiov1alpha1.Component) (bool, strin return true, errCondition } - -func isGithubURL(URL string) (bool, error) { - url, err := url.Parse(URL) +// isGithubURL checks if the given url includes github in hostname +// In case of invalid url (not able to parse) returns false. +func isGithubURL(URL string) bool { + parsedURL, err := url.Parse(URL) if err != nil { - return false, &InvalidURL{URL: URL, Err: err} + return false } - if strings.Contains(url.Host, "github") { - return true, nil + if strings.Contains(parsedURL.Host, "github") { + return true } - return false, nil -} \ No newline at end of file + return false +} From 30365844e465d6554eba5068694988138c64956a Mon Sep 17 00:00:00 2001 From: thepetk Date: Tue, 19 Mar 2024 16:21:38 +0000 Subject: [PATCH 06/21] Fix format Signed-off-by: thepetk --- controllers/component_controller.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/controllers/component_controller.go b/controllers/component_controller.go index 1ce101366..82b70e52a 100644 --- a/controllers/component_controller.go +++ b/controllers/component_controller.go @@ -19,7 +19,6 @@ package controllers import ( "context" "fmt" - corev1 "k8s.io/api/core/v1" "net/url" "os" "path/filepath" @@ -27,6 +26,8 @@ import ( "strings" "time" + corev1 "k8s.io/api/core/v1" + "github.com/prometheus/client_golang/prometheus" cdqanalysis "github.com/redhat-appstudio/application-service/cdq-analysis/pkg" "github.com/redhat-appstudio/application-service/pkg/metrics" From dde4d040772772b3b40add24c61f7119419d96a9 Mon Sep 17 00:00:00 2001 From: thepetk Date: Tue, 19 Mar 2024 20:55:45 +0000 Subject: [PATCH 07/21] Fix component controller logging Signed-off-by: thepetk --- controllers/component_controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/component_controller.go b/controllers/component_controller.go index 82b70e52a..ed8c52da2 100644 --- a/controllers/component_controller.go +++ b/controllers/component_controller.go @@ -301,7 +301,7 @@ func (r *ComponentReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( if source.GitSource != nil && source.GitSource.URL != "" { if !isGithubURL(source.GitSource.URL) { // User error - the git url provided is not from github - log.Info("source git url %v is not from github", source.GitSource.URL) + log.Info(fmt.Sprintf("source git url %v is not from github", source.GitSource.URL)) metrics.IncrementComponentCreationSucceeded(prevErrCondition, err.Error()) return ctrl.Result{}, nil } From b7acbd9748c5f63424909c44cc90badbdff9e8a3 Mon Sep 17 00:00:00 2001 From: thepetk Date: Tue, 19 Mar 2024 21:13:13 +0000 Subject: [PATCH 08/21] Fix nil point reference in metrics Signed-off-by: thepetk --- controllers/component_controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/component_controller.go b/controllers/component_controller.go index ed8c52da2..00aa77230 100644 --- a/controllers/component_controller.go +++ b/controllers/component_controller.go @@ -302,7 +302,7 @@ func (r *ComponentReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( if !isGithubURL(source.GitSource.URL) { // User error - the git url provided is not from github log.Info(fmt.Sprintf("source git url %v is not from github", source.GitSource.URL)) - metrics.IncrementComponentCreationSucceeded(prevErrCondition, err.Error()) + metrics.IncrementComponentCreationSucceeded("", "") return ctrl.Result{}, nil } context := source.GitSource.Context From 61732e83ab701997a1a113b659188b836821ebfb Mon Sep 17 00:00:00 2001 From: thepetk Date: Tue, 19 Mar 2024 22:29:02 +0000 Subject: [PATCH 09/21] Update test cases Signed-off-by: thepetk --- controllers/component_controller_test.go | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/controllers/component_controller_test.go b/controllers/component_controller_test.go index b60938452..914ff8676 100644 --- a/controllers/component_controller_test.go +++ b/controllers/component_controller_test.go @@ -1105,11 +1105,7 @@ var _ = Describe("Component controller", func() { return len(createdHasComp.Status.Conditions) > 0 }, timeout, interval).Should(BeTrue()) - // Make sure the devfile model was properly set in Component - errCondition := createdHasComp.Status.Conditions[len(createdHasComp.Status.Conditions)-1] - Expect(errCondition.Status).Should(Equal(metav1.ConditionFalse)) - Expect(errCondition.Message).Should(ContainSubstring("Component create failed: unable to get default branch of Github Repo")) - + // Confirm user error hasn't increase the failure metrics hasAppLookupKey := types.NamespacedName{Name: applicationName, Namespace: HASAppNamespace} Expect(testutil.ToFloat64(metrics.GetComponentCreationTotalReqs()) > beforeCreateTotalReqs).To(BeTrue()) @@ -2966,8 +2962,8 @@ var _ = Describe("Component controller", func() { ctx := context.Background() - applicationName := HASAppName + "28" - componentName := HASCompName + "28" + applicationName := HASAppName + "30" + componentName := HASCompName + "30" createAndFetchSimpleApp(applicationName, HASAppNamespace, DisplayName, Description) From 442d56b70e2eb9a6eccde5c49f9cfcdb4e0ac56c Mon Sep 17 00:00:00 2001 From: thepetk Date: Wed, 20 Mar 2024 12:25:16 +0000 Subject: [PATCH 10/21] Return user error for non github url Signed-off-by: thepetk --- controllers/component_controller.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/controllers/component_controller.go b/controllers/component_controller.go index 00aa77230..0ee8996c7 100644 --- a/controllers/component_controller.go +++ b/controllers/component_controller.go @@ -299,11 +299,11 @@ func (r *ComponentReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( var devfileBytes []byte if source.GitSource != nil && source.GitSource.URL != "" { - if !isGithubURL(source.GitSource.URL) { + if err := validateGithubURL(source.GitSource.URL); err != nil { // User error - the git url provided is not from github - log.Info(fmt.Sprintf("source git url %v is not from github", source.GitSource.URL)) + log.Error(err, "unable to validate github url") metrics.IncrementComponentCreationSucceeded("", "") - return ctrl.Result{}, nil + return ctrl.Result{}, err } context := source.GitSource.Context // If a Git secret was passed in, retrieve it for use in our Git operations @@ -894,14 +894,14 @@ func checkForCreateReconcile(component appstudiov1alpha1.Component) (bool, strin // isGithubURL checks if the given url includes github in hostname // In case of invalid url (not able to parse) returns false. -func isGithubURL(URL string) bool { +func validateGithubURL(URL string) (bool, error) { parsedURL, err := url.Parse(URL) if err != nil { - return false + return false, err } if strings.Contains(parsedURL.Host, "github") { - return true + return true, nil } - return false + return false, fmt.Errorf("source git url %v is not from github", URL) } From 496b20ad0c4666278274bf2f36e8f77132220a1e Mon Sep 17 00:00:00 2001 From: thepetk Date: Wed, 20 Mar 2024 12:48:03 +0000 Subject: [PATCH 11/21] Finalize validateGithubURL Signed-off-by: thepetk --- controllers/component_controller.go | 8 ++--- controllers/component_controller_unit_test.go | 33 +++++++++++++++++++ 2 files changed, 37 insertions(+), 4 deletions(-) diff --git a/controllers/component_controller.go b/controllers/component_controller.go index 0ee8996c7..6ffab705a 100644 --- a/controllers/component_controller.go +++ b/controllers/component_controller.go @@ -894,14 +894,14 @@ func checkForCreateReconcile(component appstudiov1alpha1.Component) (bool, strin // isGithubURL checks if the given url includes github in hostname // In case of invalid url (not able to parse) returns false. -func validateGithubURL(URL string) (bool, error) { +func validateGithubURL(URL string) error { parsedURL, err := url.Parse(URL) if err != nil { - return false, err + return err } if strings.Contains(parsedURL.Host, "github") { - return true, nil + return nil } - return false, fmt.Errorf("source git url %v is not from github", URL) + return fmt.Errorf("source git url %v is not from github", URL) } diff --git a/controllers/component_controller_unit_test.go b/controllers/component_controller_unit_test.go index fb4191cfb..c15e0a962 100644 --- a/controllers/component_controller_unit_test.go +++ b/controllers/component_controller_unit_test.go @@ -677,3 +677,36 @@ func TestCheckForCreateReconcile(t *testing.T) { } } + +func TestValidateGithubURL(t *testing.T) { + tests := []struct { + name string + sourceGitURL string + wantErr bool + }{ + { + name: "Valid github url", + sourceGitURL: "https://github.com/devfile-samples", + wantErr: false, + }, + { + name: "Invalid url", + sourceGitURL: "afgae devfile", + wantErr: true, + }, + { + name: "Not github url", + sourceGitURL: "https://gitlab.com/devfile-samples", + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateGithubURL(tt.sourceGitURL) + if (err != nil) != tt.wantErr { + t.Errorf("TestValidateGithubURL() unexpected error: %v", err) + } + }) + } +} From e911aef05209d58f60d1b6d0a778353bf003ca0f Mon Sep 17 00:00:00 2001 From: thepetk Date: Wed, 20 Mar 2024 13:18:38 +0000 Subject: [PATCH 12/21] Update test cases Signed-off-by: thepetk --- controllers/component_controller_test.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/controllers/component_controller_test.go b/controllers/component_controller_test.go index 914ff8676..2e378e108 100644 --- a/controllers/component_controller_test.go +++ b/controllers/component_controller_test.go @@ -1105,6 +1105,10 @@ var _ = Describe("Component controller", func() { return len(createdHasComp.Status.Conditions) > 0 }, timeout, interval).Should(BeTrue()) + // Make sure the err was set + Expect(createdHasComp.Status.Conditions[len(createdHasComp.Status.Conditions)-1].Reason).Should(Equal("Error")) + Expect(strings.ToLower(createdHasComp.Status.Conditions[len(createdHasComp.Status.Conditions)-1].Message)).Should(ContainSubstring("is not from github")) + // Confirm user error hasn't increase the failure metrics hasAppLookupKey := types.NamespacedName{Name: applicationName, Namespace: HASAppNamespace} @@ -2999,6 +3003,9 @@ var _ = Describe("Component controller", func() { return len(createdHasComp.Status.Conditions) > 0 }, timeout, interval).Should(BeTrue()) + // Make sure the err was set + Expect(createdHasComp.Status.Conditions[len(createdHasComp.Status.Conditions)-1].Reason).Should(Equal("Error")) + Expect(strings.ToLower(createdHasComp.Status.Conditions[len(createdHasComp.Status.Conditions)-1].Message)).Should(ContainSubstring("is not from github")) hasAppLookupKey := types.NamespacedName{Name: applicationName, Namespace: HASAppNamespace} Expect(testutil.ToFloat64(metrics.GetComponentCreationTotalReqs()) > beforeCreateTotalReqs).To(BeTrue()) From 729e738c484af7c9b8b6d545d5752ca4129493bc Mon Sep 17 00:00:00 2001 From: thepetk Date: Wed, 20 Mar 2024 13:46:01 +0000 Subject: [PATCH 13/21] Set create condition for non github url Signed-off-by: thepetk --- controllers/component_controller.go | 1 + 1 file changed, 1 insertion(+) diff --git a/controllers/component_controller.go b/controllers/component_controller.go index 6ffab705a..6171a3b89 100644 --- a/controllers/component_controller.go +++ b/controllers/component_controller.go @@ -303,6 +303,7 @@ func (r *ComponentReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( // User error - the git url provided is not from github log.Error(err, "unable to validate github url") metrics.IncrementComponentCreationSucceeded("", "") + _ = r.SetCreateConditionAndUpdateCR(ctx, req, &component, err) return ctrl.Result{}, err } context := source.GitSource.Context From 9df08023e49ca63cb6fc11b92ec2df6cb0024676 Mon Sep 17 00:00:00 2001 From: thepetk Date: Wed, 20 Mar 2024 13:56:59 +0000 Subject: [PATCH 14/21] Fix test cases Signed-off-by: thepetk --- controllers/component_controller_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/component_controller_test.go b/controllers/component_controller_test.go index 2e378e108..f638cb466 100644 --- a/controllers/component_controller_test.go +++ b/controllers/component_controller_test.go @@ -1107,7 +1107,7 @@ var _ = Describe("Component controller", func() { // Make sure the err was set Expect(createdHasComp.Status.Conditions[len(createdHasComp.Status.Conditions)-1].Reason).Should(Equal("Error")) - Expect(strings.ToLower(createdHasComp.Status.Conditions[len(createdHasComp.Status.Conditions)-1].Message)).Should(ContainSubstring("is not from github")) + Expect(strings.ToLower(createdHasComp.Status.Conditions[len(createdHasComp.Status.Conditions)-1].Message)).Should(ContainSubstring("component create failed:")) // Confirm user error hasn't increase the failure metrics hasAppLookupKey := types.NamespacedName{Name: applicationName, Namespace: HASAppNamespace} From 5630782f1a43960ef065d9b9e259395ec929475e Mon Sep 17 00:00:00 2001 From: thepetk Date: Wed, 20 Mar 2024 14:28:20 +0000 Subject: [PATCH 15/21] Fix test case for convert Signed-off-by: thepetk --- cdq-analysis/pkg/util_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cdq-analysis/pkg/util_test.go b/cdq-analysis/pkg/util_test.go index 068f6bf6d..31cde4af0 100644 --- a/cdq-analysis/pkg/util_test.go +++ b/cdq-analysis/pkg/util_test.go @@ -320,7 +320,7 @@ func TestConvertGitHubURL(t *testing.T) { { name: "A non github url", url: "https://gitlab.com/", - wantUrl: "https://gitlab.com/", + wantUrl: "https://gitlab.com", }, { name: "A non url", From e424a7796d8714a365e46c9a908e10438aeae684 Mon Sep 17 00:00:00 2001 From: thepetk Date: Wed, 20 Mar 2024 16:24:40 +0000 Subject: [PATCH 16/21] Final fixes on the github validation Signed-off-by: thepetk --- cdq-analysis/pkg/util.go | 14 ++++++++ cdq-analysis/pkg/util_test.go | 33 +++++++++++++++++++ controllers/component_controller.go | 20 ++--------- controllers/component_controller_unit_test.go | 33 ------------------- 4 files changed, 50 insertions(+), 50 deletions(-) diff --git a/cdq-analysis/pkg/util.go b/cdq-analysis/pkg/util.go index e17ce8acd..1a7c695e6 100644 --- a/cdq-analysis/pkg/util.go +++ b/cdq-analysis/pkg/util.go @@ -279,3 +279,17 @@ func UpdateGitLink(repo, revision, context string) (string, error) { } return rawGitURL, nil } + +// ValidateGithubURL checks if the given url includes github in hostname +// In case of invalid url (not able to parse / not github) returns an error. +func ValidateGithubURL(URL string) error { + parsedURL, err := url.Parse(URL) + if err != nil { + return err + } + + if strings.Contains(parsedURL.Host, "github") { + return nil + } + return fmt.Errorf("source git url %v is not from github", URL) +} diff --git a/cdq-analysis/pkg/util_test.go b/cdq-analysis/pkg/util_test.go index 31cde4af0..e9f0d3b6b 100644 --- a/cdq-analysis/pkg/util_test.go +++ b/cdq-analysis/pkg/util_test.go @@ -640,3 +640,36 @@ func TestUpdateGitLink(t *testing.T) { }) } } + +func TestValidateGithubURL(t *testing.T) { + tests := []struct { + name string + sourceGitURL string + wantErr bool + }{ + { + name: "Valid github url", + sourceGitURL: "https://github.com/devfile-samples", + wantErr: false, + }, + { + name: "Invalid url", + sourceGitURL: "afgae devfile", + wantErr: true, + }, + { + name: "Not github url", + sourceGitURL: "https://gitlab.com/devfile-samples", + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := ValidateGithubURL(tt.sourceGitURL) + if (err != nil) != tt.wantErr { + t.Errorf("TestValidateGithubURL() unexpected error: %v", err) + } + }) + } +} diff --git a/controllers/component_controller.go b/controllers/component_controller.go index 6171a3b89..51305f8c5 100644 --- a/controllers/component_controller.go +++ b/controllers/component_controller.go @@ -19,7 +19,6 @@ package controllers import ( "context" "fmt" - "net/url" "os" "path/filepath" "reflect" @@ -29,6 +28,7 @@ import ( corev1 "k8s.io/api/core/v1" "github.com/prometheus/client_golang/prometheus" + "github.com/redhat-appstudio/application-service/cdq-analysis/pkg" cdqanalysis "github.com/redhat-appstudio/application-service/cdq-analysis/pkg" "github.com/redhat-appstudio/application-service/pkg/metrics" "k8s.io/apimachinery/pkg/api/errors" @@ -299,12 +299,12 @@ func (r *ComponentReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( var devfileBytes []byte if source.GitSource != nil && source.GitSource.URL != "" { - if err := validateGithubURL(source.GitSource.URL); err != nil { + if err := pkg.ValidateGithubURL(source.GitSource.URL); err != nil { // User error - the git url provided is not from github log.Error(err, "unable to validate github url") metrics.IncrementComponentCreationSucceeded("", "") _ = r.SetCreateConditionAndUpdateCR(ctx, req, &component, err) - return ctrl.Result{}, err + return ctrl.Result{}, nil } context := source.GitSource.Context // If a Git secret was passed in, retrieve it for use in our Git operations @@ -892,17 +892,3 @@ func checkForCreateReconcile(component appstudiov1alpha1.Component) (bool, strin // If there are no Conditions or no Updated Condition, then this is a Create reconcile return true, errCondition } - -// isGithubURL checks if the given url includes github in hostname -// In case of invalid url (not able to parse) returns false. -func validateGithubURL(URL string) error { - parsedURL, err := url.Parse(URL) - if err != nil { - return err - } - - if strings.Contains(parsedURL.Host, "github") { - return nil - } - return fmt.Errorf("source git url %v is not from github", URL) -} diff --git a/controllers/component_controller_unit_test.go b/controllers/component_controller_unit_test.go index c15e0a962..fb4191cfb 100644 --- a/controllers/component_controller_unit_test.go +++ b/controllers/component_controller_unit_test.go @@ -677,36 +677,3 @@ func TestCheckForCreateReconcile(t *testing.T) { } } - -func TestValidateGithubURL(t *testing.T) { - tests := []struct { - name string - sourceGitURL string - wantErr bool - }{ - { - name: "Valid github url", - sourceGitURL: "https://github.com/devfile-samples", - wantErr: false, - }, - { - name: "Invalid url", - sourceGitURL: "afgae devfile", - wantErr: true, - }, - { - name: "Not github url", - sourceGitURL: "https://gitlab.com/devfile-samples", - wantErr: true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - err := validateGithubURL(tt.sourceGitURL) - if (err != nil) != tt.wantErr { - t.Errorf("TestValidateGithubURL() unexpected error: %v", err) - } - }) - } -} From e995edbf1b9797777492126ba8eb84610a1367b9 Mon Sep 17 00:00:00 2001 From: thepetk Date: Thu, 21 Mar 2024 16:50:17 +0000 Subject: [PATCH 17/21] Raise error for cdq and gitlab urls Signed-off-by: thepetk --- .../componentdetectionquery_controller.go | 9 ++++ ...componentdetectionquery_controller_test.go | 44 +++++++++++++++++++ 2 files changed, 53 insertions(+) diff --git a/controllers/componentdetectionquery_controller.go b/controllers/componentdetectionquery_controller.go index b5348f014..2363056e8 100644 --- a/controllers/componentdetectionquery_controller.go +++ b/controllers/componentdetectionquery_controller.go @@ -29,6 +29,7 @@ import ( "github.com/hashicorp/go-multierror" "github.com/prometheus/client_golang/prometheus" appstudiov1alpha1 "github.com/redhat-appstudio/application-api/api/v1alpha1" + "github.com/redhat-appstudio/application-service/cdq-analysis/pkg" cdqanalysis "github.com/redhat-appstudio/application-service/cdq-analysis/pkg" "github.com/redhat-appstudio/application-service/pkg/github" logutil "github.com/redhat-appstudio/application-service/pkg/log" @@ -170,6 +171,14 @@ func (r *ComponentDetectionQueryReconciler) Reconcile(ctx context.Context, req c return ctrl.Result{}, nil } + // check if the given url is from github + if err := pkg.ValidateGithubURL(sourceURL); err != nil { + // User error - the git url provided is not from github + log.Error(err, "unable to validate github url") + r.SetCompleteConditionAndUpdateCR(ctx, req, &componentDetectionQuery, copiedCDQ, err) + return ctrl.Result{}, nil + } + cdqInfo := &cdqanalysis.CDQInfo{ DevfileRegistryURL: r.DevfileRegistryURL, GitURL: cdqanalysis.GitURL{RepoURL: source.URL, Revision: source.Revision, Token: gitToken}, diff --git a/controllers/componentdetectionquery_controller_test.go b/controllers/componentdetectionquery_controller_test.go index 3f17d10cd..204e1e455 100644 --- a/controllers/componentdetectionquery_controller_test.go +++ b/controllers/componentdetectionquery_controller_test.go @@ -3337,6 +3337,50 @@ metadata: }) }) + Context("Create Component Detection Query with non Github URL", func() { + It("Should err out", func() { + ctx := context.Background() + + queryName := HASCompDetQuery + "21" + + hasCompDetectionQuery := &appstudiov1alpha1.ComponentDetectionQuery{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "appstudio.redhat.com/v1alpha1", + Kind: "ComponentDetectionQuery", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: queryName, + Namespace: HASNamespace, + Annotations: map[string]string{ + "runCDQAnalysisLocal": "true", + }, + }, + Spec: appstudiov1alpha1.ComponentDetectionQuerySpec{ + GitSource: appstudiov1alpha1.GitSource{ + URL: "https://gitlab.com/redhat-appstudio-appdata/sample", + Revision: "main", + }, + }, + } + + Expect(k8sClient.Create(ctx, hasCompDetectionQuery)).Should(Succeed()) + + // Look up the has app resource that was created. + // num(conditions) may still be < 1 on the first try, so retry until at least _some_ condition is set + hasCompDetQueryLookupKey := types.NamespacedName{Name: queryName, Namespace: HASNamespace} + createdHasCompDetectionQuery := &appstudiov1alpha1.ComponentDetectionQuery{} + Eventually(func() bool { + k8sClient.Get(context.Background(), hasCompDetQueryLookupKey, createdHasCompDetectionQuery) + return len(createdHasCompDetectionQuery.Status.Conditions) > 1 + }, timeout, interval).Should(BeTrue()) + + // Make sure the right err is set + Expect(createdHasCompDetectionQuery.Status.Conditions[1].Message).Should(ContainSubstring("parse \"https://github.com/redhat-appstudio-appdata/!@#$%U%I$F DFDN##\": invalid URL escape \"%U%\"")) + + // Delete the specified Detection Query resource + deleteCompDetQueryCR(hasCompDetQueryLookupKey) + }) + }) }) From 9d50ce2953bc6dd77c28ac7823e1f49c96f9e6e5 Mon Sep 17 00:00:00 2001 From: thepetk Date: Thu, 21 Mar 2024 17:03:38 +0000 Subject: [PATCH 18/21] Fix imports Signed-off-by: thepetk --- controllers/component_controller.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/controllers/component_controller.go b/controllers/component_controller.go index 51305f8c5..c32302c59 100644 --- a/controllers/component_controller.go +++ b/controllers/component_controller.go @@ -28,7 +28,6 @@ import ( corev1 "k8s.io/api/core/v1" "github.com/prometheus/client_golang/prometheus" - "github.com/redhat-appstudio/application-service/cdq-analysis/pkg" cdqanalysis "github.com/redhat-appstudio/application-service/cdq-analysis/pkg" "github.com/redhat-appstudio/application-service/pkg/metrics" "k8s.io/apimachinery/pkg/api/errors" @@ -299,7 +298,7 @@ func (r *ComponentReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( var devfileBytes []byte if source.GitSource != nil && source.GitSource.URL != "" { - if err := pkg.ValidateGithubURL(source.GitSource.URL); err != nil { + if err := cdqanalysis.ValidateGithubURL(source.GitSource.URL); err != nil { // User error - the git url provided is not from github log.Error(err, "unable to validate github url") metrics.IncrementComponentCreationSucceeded("", "") From 73f1250a1078b169921d740a4a8309870d58e764 Mon Sep 17 00:00:00 2001 From: thepetk Date: Thu, 21 Mar 2024 17:42:46 +0000 Subject: [PATCH 19/21] Update test case Signed-off-by: thepetk --- controllers/componentdetectionquery_controller_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/componentdetectionquery_controller_test.go b/controllers/componentdetectionquery_controller_test.go index 204e1e455..41d5f674f 100644 --- a/controllers/componentdetectionquery_controller_test.go +++ b/controllers/componentdetectionquery_controller_test.go @@ -3375,7 +3375,7 @@ metadata: }, timeout, interval).Should(BeTrue()) // Make sure the right err is set - Expect(createdHasCompDetectionQuery.Status.Conditions[1].Message).Should(ContainSubstring("parse \"https://github.com/redhat-appstudio-appdata/!@#$%U%I$F DFDN##\": invalid URL escape \"%U%\"")) + Expect(createdHasCompDetectionQuery.Status.Conditions[1].Message).Should(ContainSubstring("is not from github")) // Delete the specified Detection Query resource deleteCompDetQueryCR(hasCompDetQueryLookupKey) From 60258b704827c853650299ae5a131023184ef1b7 Mon Sep 17 00:00:00 2001 From: thepetk Date: Thu, 21 Mar 2024 19:07:38 +0000 Subject: [PATCH 20/21] Fix double import Signed-off-by: thepetk --- controllers/componentdetectionquery_controller.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/controllers/componentdetectionquery_controller.go b/controllers/componentdetectionquery_controller.go index 2363056e8..8f8cb0d71 100644 --- a/controllers/componentdetectionquery_controller.go +++ b/controllers/componentdetectionquery_controller.go @@ -29,7 +29,6 @@ import ( "github.com/hashicorp/go-multierror" "github.com/prometheus/client_golang/prometheus" appstudiov1alpha1 "github.com/redhat-appstudio/application-api/api/v1alpha1" - "github.com/redhat-appstudio/application-service/cdq-analysis/pkg" cdqanalysis "github.com/redhat-appstudio/application-service/cdq-analysis/pkg" "github.com/redhat-appstudio/application-service/pkg/github" logutil "github.com/redhat-appstudio/application-service/pkg/log" @@ -172,7 +171,7 @@ func (r *ComponentDetectionQueryReconciler) Reconcile(ctx context.Context, req c } // check if the given url is from github - if err := pkg.ValidateGithubURL(sourceURL); err != nil { + if err := cdqanalysis.ValidateGithubURL(sourceURL); err != nil { // User error - the git url provided is not from github log.Error(err, "unable to validate github url") r.SetCompleteConditionAndUpdateCR(ctx, req, &componentDetectionQuery, copiedCDQ, err) From bd6794db811760ea395630d3408436b52b5152a4 Mon Sep 17 00:00:00 2001 From: thepetk Date: Fri, 22 Mar 2024 13:12:54 +0000 Subject: [PATCH 21/21] Fix envtest version Signed-off-by: thepetk --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index fee41d23d..fdaa0e7b3 100644 --- a/Makefile +++ b/Makefile @@ -206,7 +206,7 @@ kustomize: ## Download kustomize locally if necessary. ENVTEST = $(shell pwd)/bin/setup-envtest envtest: ## Download envtest-setup locally if necessary. - $(call go-get-tool,$(ENVTEST),sigs.k8s.io/controller-runtime/tools/setup-envtest@latest) + $(call go-get-tool,$(ENVTEST),sigs.k8s.io/controller-runtime/tools/setup-envtest@395cfc7486e652d19fe1b544a436f9852ba26e4f) DLV = $(shell pwd)/bin/dlv dlv: