[DEVHAS-482] add metrics git import (#395)

* add metrics to cdq import Signed-off-by: Stephanie <[email protected]> * add component metrics tests Signed-off-by: Stephanie <[email protected]> * fix some tests Signed-off-by: Stephanie <[email protected]> * update test Signed-off-by: Stephanie <[email protected]> * test Signed-off-by: Stephanie <[email protected]> * remove log Signed-off-by: Stephanie <[email protected]> * add Authentication Failure to go module Signed-off-by: Stephanie <[email protected]> * fix the test failure Signed-off-by: Stephanie <[email protected]> * update text failures Signed-off-by: Stephanie <[email protected]> * fix security alert Signed-off-by: Stephanie <[email protected]> --------- Signed-off-by: Stephanie <[email protected]>
redhat-appstudio · Oct 4, 2023 · 99b9e45 · 99b9e45
1 parent 1c619fb
commit 99b9e45
Show file tree

Hide file tree

Showing 13 changed files with 312 additions and 409 deletions.
diff --git a/cdq-analysis/pkg/componentdetectionquery.go b/cdq-analysis/pkg/componentdetectionquery.go
@@ -172,8 +172,9 @@ func CloneAndAnalyze(k K8sInfoClient, namespace, name, context string, cdqInfo *
 
 		shouldIgnoreDevfile, devfileBytes, err := ValidateDevfile(log, updatedLink, gitToken)
 		if err != nil {
-			k.SendBackDetectionResult(devfilesMap, devfilesURLMap, dockerfileContextMap, componentPortsMap, revision, name, namespace, err)
-			return nil, nil, nil, nil, "", err
+			retErr := &InvalidDevfile{Err: err}
+			k.SendBackDetectionResult(devfilesMap, devfilesURLMap, dockerfileContextMap, componentPortsMap, revision, name, namespace, retErr)
+			return nil, nil, nil, nil, "", retErr
 		}
 		if shouldIgnoreDevfile {
 			isDevfilePresent = false
@@ -279,6 +280,14 @@ func (k K8sInfoClient) SendBackDetectionResult(devfilesMap map[string][]byte, de
 			errorMap["NoDevfileFound"] = fmt.Sprintf("%v", completeError)
 		case *NoDockerfileFound:
 			errorMap["NoDockerfileFound"] = fmt.Sprintf("%v", completeError)
+		case *RepoNotFound:
+			errorMap["RepoNotFound"] = fmt.Sprintf("%v", completeError)
+		case *InvalidDevfile:
+			errorMap["InvalidDevfile"] = fmt.Sprintf("%v", completeError)
+		case *InvalidURL:
+			errorMap["InvalidURL"] = fmt.Sprintf("%v", completeError)
+		case *AuthenticationFailed:
+			errorMap["AuthenticationFailed"] = fmt.Sprintf("%v", completeError)
 		default:
 			errorMap["InternalError"] = fmt.Sprintf("%v", completeError)
 		}

diff --git a/cdq-analysis/pkg/componentdetectionquery_test.go b/cdq-analysis/pkg/componentdetectionquery_test.go
@@ -50,8 +50,8 @@ func TestCloneAndAnalyze(t *testing.T) {
 	multiComponentRepoURL := "https://github.com/maysunfaisal/multi-components-dockerfile"
 	springNoDevfileNoDockerfileURL := "https://github.com/kim-tsao/devfile-sample-java-springboot-basic-no-devfile-no-dockerfile"
 	springNoDevfileURL := "https://github.com/yangcao77/devfile-sample-java-springboot-basic-no-devfile"
-	multiComponentWithNoDevfileAndDockerfileURL := "https://github.com/redhat-appstudio/quality-dashboard"
-	failedToCloneRepoErr := "failed to clone the repo.*"
+	multiComponentWithNoDevfileAndDockerfileURL := "https://github.com/yangcao77/quality-dashboard"
+	authenticationFailedErr := "authentication failed .*"
 
 	springDevfileContext := `
 	schemaVersion: 2.2.0
@@ -150,7 +150,7 @@ func TestCloneAndAnalyze(t *testing.T) {
 			wantDevfilesURLMap:       map[string]string{},
 			wantDockerfileContextMap: map[string]string{},
 			wantComponentsPortMap:    map[string][]int{},
-			wantErr:                  failedToCloneRepoErr,
+			wantErr:                  authenticationFailedErr,
 		},
 		{
 			testCase:                 "private repo - should error out with invalid token provided",
@@ -161,7 +161,7 @@ func TestCloneAndAnalyze(t *testing.T) {
 			wantDevfilesURLMap:       map[string]string{},
 			wantDockerfileContextMap: map[string]string{},
 			wantComponentsPortMap:    map[string][]int{},
-			wantErr:                  failedToCloneRepoErr,
+			wantErr:                  authenticationFailedErr,
 		},
 		{
 			testCase:           "should successfully detect multi-component with dockerfile present",

diff --git a/cdq-analysis/pkg/detect.go b/cdq-analysis/pkg/detect.go
@@ -86,7 +86,8 @@ func search(log logr.Logger, a Alizer, localpath string, srcContext string, cdqI
 					}
 					shouldIgnoreDevfile, devfileBytes, err := ValidateDevfile(log, devfilePath, token)
 					if err != nil {
-						return nil, nil, nil, nil, err
+						retErr := &InvalidDevfile{Err: err}
+						return nil, nil, nil, nil, retErr
 					}
 					if shouldIgnoreDevfile {
 						isDevfilePresent = false
@@ -117,7 +118,8 @@ func search(log logr.Logger, a Alizer, localpath string, srcContext string, cdqI
 							}
 							shouldIgnoreDevfile, devfileBytes, err := ValidateDevfile(log, devfilePath, token)
 							if err != nil {
-								return nil, nil, nil, nil, err
+								retErr := &InvalidDevfile{Err: err}
+								return nil, nil, nil, nil, retErr
 							}
 
 							if shouldIgnoreDevfile {
@@ -283,7 +285,8 @@ func SearchForDockerfile(devfileBytes []byte, token string) (*v1alpha2.Dockerfil
 	devfileData, err := ParseDevfileWithParserArgs(&parser.ParserArgs{Data: devfileBytes, Token: token})
 
 	if err != nil {
-		return nil, err
+		retErr := &InvalidDevfile{Err: err}
+		return nil, retErr
 	}
 	devfileImageComponents, err := devfileData.GetComponents(common.DevfileOptions{
 		ComponentOptions: common.ComponentOptions{

diff --git a/cdq-analysis/pkg/errors.go b/cdq-analysis/pkg/errors.go
@@ -45,6 +45,63 @@ func (e *NoDockerfileFound) Error() string {
 	return errMsg
 }
 
+// RepoNotFound returns an error if no git repo was found
+type RepoNotFound struct {
+	URL      string
+	Revision string
+	Err      error
+}
+
+func (e *RepoNotFound) Error() string {
+	errMsg := fmt.Sprintf("unable to find git repository %s %s", e.URL, e.Revision)
+	if e.Err != nil {
+		errMsg = fmt.Sprintf("%s due to %v", errMsg, e.Err)
+	}
+	return errMsg
+}
+
+// InvalidDevfile returns an error if no devfile is invalid
+type InvalidDevfile struct {
+	Err error
+}
+
+func (e *InvalidDevfile) Error() string {
+	var errMsg string
+	if e.Err != nil {
+		errMsg = fmt.Sprintf("invalid devfile due to %v", e.Err)
+	}
+	return errMsg
+}
+
+// InvalidURL returns an error if URL is invalid to be parsed
+type InvalidURL struct {
+	URL string
+	Err error
+}
+
+func (e *InvalidURL) Error() string {
+	var errMsg string
+	if e.Err != nil {
+		errMsg = fmt.Sprintf("invalid URL %v due to %v", e.URL, e.Err)
+	}
+	return errMsg
+}
+
+// AuthenticationFailed returns an error if authenticated failed when cloning the repository
+// indicates the token is not valid
+type AuthenticationFailed struct {
+	URL string
+	Err error
+}
+
+func (e *AuthenticationFailed) Error() string {
+	var errMsg string
+	if e.Err != nil {
+		errMsg = fmt.Sprintf("authentication failed to URL %v: %v", e.URL, e.Err)
+	}
+	return errMsg
+}
+
 // InternalError returns cdq errors other than user error
 type InternalError struct {
 	Err error

diff --git a/cdq-analysis/pkg/util.go b/cdq-analysis/pkg/util.go
@@ -38,6 +38,12 @@ type GitURL struct {
 	Token    string
 }
 
+const (
+	RepoNotFoundMsg         = "repository .* not found"
+	RevisionNotFoundMsg     = "pathspec .* did not match any file(s) known to git"
+	AuthenticationFailedMsg = "Authentication failed .*"
+)
+
 // CloneRepo clones the repoURL to specfied clonePath
 func CloneRepo(clonePath string, gitURL GitURL) error {
 	exist, err := IsExist(clonePath)
@@ -59,8 +65,15 @@ func CloneRepo(clonePath string, gitURL GitURL) error {
 	c.Env = os.Environ()
 	c.Env = append(c.Env, "GIT_TERMINAL_PROMPT=0", "GIT_ASKPASS=/bin/echo")
 
-	_, err = c.CombinedOutput()
+	output, err := c.CombinedOutput()
 	if err != nil {
+
+		if matched, _ := regexp.MatchString(RepoNotFoundMsg, string(output)); matched {
+			return &RepoNotFound{URL: cloneURL, Err: err}
+		} else if matched, _ := regexp.MatchString(AuthenticationFailedMsg, string(output)); matched {
+			return &AuthenticationFailed{URL: cloneURL, Err: err}
+		}
+
 		return fmt.Errorf("failed to clone the repo: %v", err)
 	}
 
@@ -70,6 +83,10 @@ func CloneRepo(clonePath string, gitURL GitURL) error {
 
 		_, err = c.CombinedOutput()
 		if err != nil {
+			if matched, _ := regexp.MatchString(RevisionNotFoundMsg, string(output)); matched {
+				return &RepoNotFound{URL: cloneURL, Revision: gitURL.Revision, Err: err}
+			}
+
 			return fmt.Errorf("failed to checkout the revision %q: %v", gitURL.Revision, err)
 		}
 	}
@@ -129,7 +146,7 @@ func ConvertGitHubURL(URL string, revision string, context string) (string, erro
 
 	url, err := url.Parse(URL)
 	if err != nil {
-		return "", err
+		return "", &InvalidURL{URL: URL, Err: err}
 	}
 
 	if strings.Contains(url.Host, "github") && !strings.Contains(url.Host, "raw") {

diff --git a/controllers/component_controller.go b/controllers/component_controller.go
@@ -302,15 +302,21 @@ func (r *ComponentReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 
 			var gitURL string
 			if source.GitSource.DevfileURL == "" && source.GitSource.DockerfileURL == "" {
+				metrics.ImportGitRepoTotalReqs.Inc()
+
 				if gitToken == "" {
-					gitURL, err = util.ConvertGitHubURL(source.GitSource.URL, source.GitSource.Revision, context)
+					gitURL, err = cdqanalysis.ConvertGitHubURL(source.GitSource.URL, source.GitSource.Revision, context)
 					if err != nil {
+						// ConvertGitHubURL only returns user error
+						metrics.ImportGitRepoSucceeded.Inc()
 						log.Error(err, fmt.Sprintf("Unable to convert Github URL to raw format, exiting reconcile loop %v", req.NamespacedName))
 						_ = r.SetCreateConditionAndUpdateCR(ctx, req, &component, err)
 						return ctrl.Result{}, err
 					}
 
 					devfileBytes, devfileLocation, err = devfile.FindAndDownloadDevfile(gitURL)
+					// FindAndDownloadDevfile only returns user error
+					metrics.ImportGitRepoSucceeded.Inc()
 					if err != nil {
 						log.Error(err, fmt.Sprintf("Unable to read the devfile from dir %s %v", gitURL, req.NamespacedName))
 						_ = r.SetCreateConditionAndUpdateCR(ctx, req, &component, err)
@@ -324,12 +330,19 @@ func (r *ComponentReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 					// Use SPI to retrieve the devfile from the private repository
 					devfileBytes, devfileLocation, err = spi.DownloadDevfileUsingSPI(r.SPIClient, ctx, component, gitURL, source.GitSource.Revision, context)
 					if err != nil {
+						// Increment the import git repo failed metric on non-user errors
+						if _, ok := err.(*devfile.NoFileFound); !ok {
+							metrics.ImportGitRepoFailed.Inc()
+						} else {
+							metrics.ImportGitRepoSucceeded.Inc()
+						}
 						log.Error(err, fmt.Sprintf("Unable to download from any known devfile locations from %s %v", gitURL, req.NamespacedName))
 						_ = r.SetCreateConditionAndUpdateCR(ctx, req, &component, err)
 						return ctrl.Result{}, err
 					}
+					metrics.ImportGitRepoSucceeded.Inc()
 
-					convertedGitURL, err := util.ConvertGitHubURL(source.GitSource.URL, source.GitSource.Revision, context)
+					convertedGitURL, err := cdqanalysis.ConvertGitHubURL(source.GitSource.URL, source.GitSource.Revision, context)
 					if err != nil {
 						log.Error(err, fmt.Sprintf("Unable to convert Github URL to raw format, exiting reconcile loop %v", req.NamespacedName))
 						_ = r.SetCreateConditionAndUpdateCR(ctx, req, &component, err)
@@ -340,7 +353,7 @@ func (r *ComponentReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 
 			} else if source.GitSource.DevfileURL != "" {
 				devfileLocation = source.GitSource.DevfileURL
-				devfileBytes, err = util.CurlEndpoint(source.GitSource.DevfileURL)
+				devfileBytes, err = cdqanalysis.CurlEndpoint(source.GitSource.DevfileURL)
 				if err != nil {
 					log.Error(err, fmt.Sprintf("Unable to GET %s, exiting reconcile loop %v", source.GitSource.DevfileURL, req.NamespacedName))
 					err := fmt.Errorf("unable to GET from %s", source.GitSource.DevfileURL)

diff --git a/controllers/component_controller_test.go b/controllers/component_controller_test.go
@@ -22,6 +22,9 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/prometheus/client_golang/prometheus/testutil"
+	"github.com/redhat-appstudio/application-service/pkg/metrics"
+
 	"github.com/devfile/library/v2/pkg/devfile/parser"
 
 	"github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2"
@@ -601,6 +604,10 @@ var _ = Describe("Component controller", func() {
 		It("Should create successfully", func() {
 			ctx := context.Background()
 
+			beforeImportGitRepoTotalReqs := testutil.ToFloat64(metrics.ImportGitRepoTotalReqs)
+			beforeImportGitRepoSucceeded := testutil.ToFloat64(metrics.ImportGitRepoSucceeded)
+			beforeImportGitRepoFailed := testutil.ToFloat64(metrics.ImportGitRepoFailed)
+
 			applicationName := HASAppName + "6"
 			componentName := HASCompName + "6"
 
@@ -660,6 +667,10 @@ var _ = Describe("Component controller", func() {
 			// Make sure the component's built image is included in the status
 			Expect(createdHasComp.Status.ContainerImage).Should(Equal("quay.io/test/testimage:latest"))
 
+			Expect(testutil.ToFloat64(metrics.ImportGitRepoTotalReqs) > beforeImportGitRepoTotalReqs).To(BeTrue())
+			Expect(testutil.ToFloat64(metrics.ImportGitRepoSucceeded) > beforeImportGitRepoSucceeded).To(BeTrue())
+			Expect(testutil.ToFloat64(metrics.ImportGitRepoFailed) == beforeImportGitRepoFailed).To(BeTrue())
+
 			// Delete the specified HASComp resource
 			deleteHASCompCR(hasCompLookupKey)
 
@@ -1028,6 +1039,70 @@ var _ = Describe("Component controller", func() {
 		})
 	})
 
+	Context("Create Component with non-exist git url", func() {
+		It("Should fail with error", func() {
+			ctx := context.Background()
+
+			beforeImportGitRepoTotalReqs := testutil.ToFloat64(metrics.ImportGitRepoTotalReqs)
+			beforeImportGitRepoSucceeded := testutil.ToFloat64(metrics.ImportGitRepoSucceeded)
+			beforeImportGitRepoFailed := testutil.ToFloat64(metrics.ImportGitRepoFailed)
+
+			applicationName := HASAppName + "-test-import-user-error"
+			componentName := HASCompName + "-test-import-user-error"
+
+			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:      "http://github.com/non-exist-git-repo",
+								Revision: "main",
+							},
+						},
+					},
+				},
+			}
+			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(context.Background(), hasCompLookupKey, createdHasComp)
+				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))
+
+			hasAppLookupKey := types.NamespacedName{Name: applicationName, Namespace: HASAppNamespace}
+
+			Expect(testutil.ToFloat64(metrics.ImportGitRepoTotalReqs) > beforeImportGitRepoTotalReqs).To(BeTrue())
+			Expect(testutil.ToFloat64(metrics.ImportGitRepoSucceeded) > beforeImportGitRepoSucceeded).To(BeTrue())
+			Expect(testutil.ToFloat64(metrics.ImportGitRepoFailed) == beforeImportGitRepoFailed).To(BeTrue())
+
+			// Delete the specified HASComp resource
+			deleteHASCompCR(hasCompLookupKey)
+
+			// Delete the specified HASApp resource
+			deleteHASAppCR(hasAppLookupKey)
+		})
+	})
+
 	Context("Create Component with invalid devfile url", func() {
 		It("Should fail with error that devfile couldn't be unmarshalled", func() {
 			ctx := context.Background()