Filter out user devfile errors (#433)

Signed-off-by: Maysun J Faisal <[email protected]>

controllers/applicationsnapshotenvironmentbinding_controller_test.go

@@ -2371,7 +2371,7 @@ var _ = Describe("SnapshotEnvironmentBinding controller", func() {
 				return len(createdBinding.Status.GitOpsRepoConditions) > 0 && createdBinding.Status.GitOpsRepoConditions[0].Reason == "GenerateError"
 			}, timeout, interval).Should(BeTrue())
 
-			Expect(createdBinding.Status.GitOpsRepoConditions[0].Message).Should(ContainSubstring("failed to decode devfile json"))
+			Expect(createdBinding.Status.GitOpsRepoConditions[0].Message).Should(ContainSubstring("cannot unmarshal string into Go value of type map[string]interface"))
 
 			// Delete the specified HASComp resource
 			deleteHASCompCR(hasCompLookupKey)

controllers/component_controller.go

@@ -49,6 +49,7 @@ import (
 	"github.com/devfile/api/v2/pkg/attributes"
 	devfileParser "github.com/devfile/library/v2/pkg/devfile/parser"
 	data "github.com/devfile/library/v2/pkg/devfile/parser/data"
+	parserErrPkg "github.com/devfile/library/v2/pkg/devfile/parser/errors"
 	devfileParserUtil "github.com/devfile/library/v2/pkg/devfile/parser/util"
 	"github.com/go-logr/logr"
 
@@ -235,9 +236,20 @@ func (r *ComponentReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 			// 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 {
-				if isCreateReconcile {
-					// Gate it with a Create reconcile flag check since this code is executed by both Create and Update reconciliation
-					metrics.ComponentCreationFailed.Inc()
+				if err != nil {
+					if _, ok := err.(*parserErrPkg.NonCompliantDevfile); ok {
+						if isCreateReconcile {
+							// Gate it with a Create reconcile flag check since this code is executed by both Create and Update reconciliation
+							// user error in devfile, increment success metric
+							metrics.ComponentCreationSucceeded.Inc()
+						}
+					} else {
+						if isCreateReconcile {
+							// Gate it with a Create reconcile flag check since this code is executed by both Create and Update reconciliation
+							// not a user error, increment fail metric
+							metrics.ComponentCreationFailed.Inc()
+						}
+					}
 				}
 				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)
@@ -431,12 +443,14 @@ func (r *ComponentReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 			// 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 {
-				// Even though we are passing in a git token to devfile/library and there is a possibility of a bad token usage,
-				// we consider this a system error because the git token is being used by the git client to fetch the branch names,
-				// if there was an issue with the token, it would have been surfaced by the github client previously.
-				metrics.ComponentCreationFailed.Inc()
+				if _, ok := err.(*parserErrPkg.NonCompliantDevfile); ok {
+					// user error in devfile, increment success metric
+					metrics.ComponentCreationSucceeded.Inc()
+				} else {
+					// not a user error, increment fail metric
+					metrics.ComponentCreationFailed.Inc()
+				}
 				log.Error(err, fmt.Sprintf("Unable to parse the devfile from Component devfile location, exiting reconcile loop %v", req.NamespacedName))
 				_ = r.SetCreateConditionAndUpdateCR(ctx, req, &component, err)
 				return ctrl.Result{}, err
@@ -448,9 +462,14 @@ func (r *ComponentReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 			// 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 {
-				metrics.ComponentCreationFailed.Inc()
+				if _, ok := err.(*parserErrPkg.NonCompliantDevfile); ok {
+					// user error in devfile, increment success metric
+					metrics.ComponentCreationSucceeded.Inc()
+				} else {
+					// not a user error, increment fail metric
+					metrics.ComponentCreationFailed.Inc()
+				}
 				log.Error(err, fmt.Sprintf("Unable to parse the devfile from Component, exiting reconcile loop %v", req.NamespacedName))
 				_ = r.SetCreateConditionAndUpdateCR(ctx, req, &component, err)
 				return ctrl.Result{}, err
@@ -459,11 +478,13 @@ func (r *ComponentReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 
 		err = r.updateComponentDevfileModel(req, compDevfileData, component)
 		if err != nil {
-			// Increment the Component create failed metric on non-user errors
+			// Increment the Component create failed metric only on non-user errors
 			if _, ok := err.(*NotSupported); ok {
 				metrics.ComponentCreationSucceeded.Inc()
 			} else if _, ok := err.(*devfile.DevfileAttributeParse); ok {
 				metrics.ComponentCreationSucceeded.Inc()
+			} else if _, ok := err.(*parserErrPkg.NonCompliantDevfile); ok {
+				metrics.ComponentCreationSucceeded.Inc()
 			} else {
 				metrics.ComponentCreationFailed.Inc()
 			}
@@ -478,8 +499,8 @@ func (r *ComponentReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 			// 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 {
+				// not a user error, increment fail metric
 				metrics.ComponentCreationFailed.Inc()
 				log.Error(err, fmt.Sprintf("Unable to parse the devfile from Application, exiting reconcile loop %v", req.NamespacedName))
 				_ = r.SetCreateConditionAndUpdateCR(ctx, req, &component, err)
@@ -675,6 +696,9 @@ func (r *ComponentReconciler) generateGitops(ctx context.Context, ghClient *gith
 		} else if _, ok := err.(*devfile.MissingOuterloop); ok && isCreateReconcile {
 			// If Devfile has no Outerloop component, it is considered an user error
 			metrics.ComponentCreationSucceeded.Inc()
+		} else if _, ok := err.(*parserErrPkg.NonCompliantDevfile); ok && isCreateReconcile {
+			// If Devfile is incompatible such as an issue with unmarshaling, it is considered an user error
+			metrics.ComponentCreationSucceeded.Inc()
 		} else if isCreateReconcile {
 			metrics.ComponentCreationFailed.Inc()
 		}

controllers/component_controller_test.go

@@ -792,7 +792,7 @@ var _ = Describe("Component controller", func() {
 
 			errCondition := updatedHasComp.Status.Conditions[len(updatedHasComp.Status.Conditions)-1]
 			Expect(errCondition.Status).Should(Equal(metav1.ConditionFalse))
-			Expect(errCondition.Message).Should(ContainSubstring("failed to decode devfile json"))
+			Expect(errCondition.Message).Should(ContainSubstring("cannot unmarshal string into Go value of type map[string]interface"))
 
 			Expect(testutil.ToFloat64(metrics.ComponentCreationTotalReqs) > beforeCreateTotalReqs).To(BeTrue())
 			Expect(testutil.ToFloat64(metrics.ComponentCreationSucceeded) > beforeCreateSucceedReqs).To(BeTrue())
@@ -1249,8 +1249,8 @@ var _ = Describe("Component controller", func() {
 			hasAppLookupKey := types.NamespacedName{Name: applicationName, Namespace: HASAppNamespace}
 
 			Expect(testutil.ToFloat64(metrics.ComponentCreationTotalReqs) > beforeCreateTotalReqs).To(BeTrue())
-			Expect(testutil.ToFloat64(metrics.ComponentCreationSucceeded) == beforeCreateSucceedReqs).To(BeTrue())
-			Expect(testutil.ToFloat64(metrics.ComponentCreationFailed) > beforeCreateFailedReqs).To(BeTrue())
+			Expect(testutil.ToFloat64(metrics.ComponentCreationSucceeded) > beforeCreateSucceedReqs).To(BeTrue())
+			Expect(testutil.ToFloat64(metrics.ComponentCreationFailed) == beforeCreateFailedReqs).To(BeTrue())
 
 			// Delete the specified HASComp resource
 			deleteHASCompCR(hasCompLookupKey)
@@ -1386,7 +1386,7 @@ var _ = Describe("Component controller", func() {
 			Expect(createdHasComp.Status.Devfile).Should(Equal(""))
 			Expect(createdHasComp.Status.Conditions[len(createdHasComp.Status.Conditions)-1].Status).Should(Equal(metav1.ConditionFalse))
 			// This test case uses an invalid token with a public URL.  The Devfile library expects an unset token and will error out trying to retrieve the devfile since it assumes it's from a private repo
-			Expect(strings.ToLower(createdHasComp.Status.Conditions[len(createdHasComp.Status.Conditions)-1].Message)).Should(ContainSubstring("component create failed: failed to populateandparsedevfile: error getting devfile info from url: failed to retrieve"))
+			Expect(strings.ToLower(createdHasComp.Status.Conditions[len(createdHasComp.Status.Conditions)-1].Message)).Should(ContainSubstring("error getting devfile info from url: failed to retrieve"))
 			hasAppLookupKey := types.NamespacedName{Name: applicationName, Namespace: HASAppNamespace}
 
 			Expect(testutil.ToFloat64(metrics.ComponentCreationTotalReqs) > beforeCreateTotalReqs).To(BeTrue())
@@ -2892,6 +2892,71 @@ var _ = Describe("Component controller", func() {
 			deleteHASAppCR(hasAppLookupKey)
 		})
 	})
+
+	Context("Create Component with basic field set including devfileURL", func() {
+		It("Should error out on a devfile that has incompatible data and mark it as an user error on the metrics", func() {
+			beforeCreateTotalReqs := testutil.ToFloat64(metrics.ComponentCreationTotalReqs)
+			beforeCreateSucceedReqs := testutil.ToFloat64(metrics.ComponentCreationSucceeded)
+			beforeCreateFailedReqs := testutil.ToFloat64(metrics.ComponentCreationFailed)
+
+			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:        SampleRepoLink,
+								DevfileURL: "https://raw.githubusercontent.com/maysunfaisal/devfile-sample-go-basic-placeholder/main/devfile.yaml",
+							},
+						},
+					},
+				},
+			}
+			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 err was set
+			Expect(createdHasComp.Status.Devfile).Should(Equal(""))
+			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("error unmarshaling"))
+
+			hasAppLookupKey := types.NamespacedName{Name: applicationName, Namespace: HASAppNamespace}
+
+			Expect(testutil.ToFloat64(metrics.ComponentCreationTotalReqs) > beforeCreateTotalReqs).To(BeTrue())
+			Expect(testutil.ToFloat64(metrics.ComponentCreationSucceeded) > beforeCreateSucceedReqs).To(BeTrue())
+			Expect(testutil.ToFloat64(metrics.ComponentCreationFailed) == beforeCreateFailedReqs).To(BeTrue())
+
+			// Delete the specified HASComp resource
+			deleteHASCompCR(hasCompLookupKey)
+
+			// Delete the specified HASApp resource
+			deleteHASAppCR(hasAppLookupKey)
+		})
+	})
 })
 
 type updateChecklist struct {

controllers/componentdetectionquery_controller_test.go

@@ -923,7 +923,7 @@ var _ = Describe("Component Detection Query controller", func() {
 
 			// Make sure that the proper error condition is set
 			Expect(createdHasCompDetectionQuery.Status.Conditions[1].Status).Should(Equal(metav1.ConditionFalse))
-			Expect(createdHasCompDetectionQuery.Status.Conditions[1].Message).Should(ContainSubstring("failed to decode devfile json: json: cannot unmarshal string into Go value of type map[string]"))
+			Expect(createdHasCompDetectionQuery.Status.Conditions[1].Message).Should(ContainSubstring("cannot unmarshal string into Go value of type map[string]interface"))
 			// Delete the specified Detection Query resource
 			deleteCompDetQueryCR(hasCompDetQueryLookupKey)
 		})

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.20231206202302-705f00dd96f5
+	github.com/devfile/library/v2 v2.2.2-0.20240108233338-2a1d045e1e65
 	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

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.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/library/v2 v2.2.2-0.20240108233338-2a1d045e1e65 h1:OYMj9mHRs3Sx+uSdzs8S4TYAvNJKBoBBvX+C7m224ko=
+github.com/devfile/library/v2 v2.2.2-0.20240108233338-2a1d045e1e65/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=

pkg/devfile/errors.go

@@ -39,6 +39,15 @@ func (e *MissingOuterloop) Error() string {
 	return "the devfile has no kubernetes components defined, missing outerloop definition"
 }
 
+// IncompatibleDevfile returns an error if the Devfile being read is incompatible due to user error
+type IncompatibleDevfile struct {
+	Err error
+}
+
+func (e *IncompatibleDevfile) Error() string {
+	return fmt.Sprintf("devfile is incompatible: %v", e.Err)
+}
+
 // DevfileAttributeParse returns an error if was an issue parsing the attribute key
 type DevfileAttributeParse struct {
 	Key string