Repository: argoproj-labs/applicationset (go)

Issue:
Gracefully handle application validation errors
# Summary

Application set validates generated applications (which is good) but refuses to create any application even if only one of the generates apps is invalid. Instead, the controller should generate valid applications and skip only invalid ones.

Here is a sample use case where current behavior causes confusion. 
* App set generates an application that references cluster A
* Generator source has changes so now app set produces 2 applications and Cluster A got removed from Argo CD.
* Controller detects the error in the first A (since cluster no longer registered) 
* User assuming that something is wrong with new application and does not know how to fix the issue

# Proposal 

Two changes are proposed:

* Application Set controller should generate valid applications and skip only invalid once
* The applications generation error should be reported in `status.conditions` field of the ApplicationSet CR:

```yaml
status:
  conditions:
    type: ApplicationsGenerated
    status: False
    reason: ApplicationValidationFailed
    message: Cluster A is not registered
```



Pull request:
feat: Gracefully handle application validation errors
Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>

Closes #356 

PR changes how the ApplicationSet controller handles ApplicationSets that generate invalid apps. Instead of refusing to handle such app sets controller will process valid apps and log information about invalid apps. 

Patch:
diff --git a/pkg/controllers/applicationset_controller.go b/pkg/controllers/applicationset_controller.go
index 5992425..4772f88 100644
--- a/pkg/controllers/applicationset_controller.go
+++ b/pkg/controllers/applicationset_controller.go
@@ -96,7 +96,8 @@ func (r *ApplicationSetReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 		return ctrl.Result{}, err
 	}
 
-	if validateError := r.validateGeneratedApplications(ctx, desiredApplications, applicationSetInfo, req.Namespace); validateError != nil {
+	validateErrors, err := r.validateGeneratedApplications(ctx, desiredApplications, applicationSetInfo, req.Namespace)
+	if err != nil {
 		// While some generators may return an error that requires user intervention,
 		// other generators reference external resources that may change to cause
 		// the error to no longer occur. We thus log the error and requeue
@@ -104,17 +105,36 @@ func (r *ApplicationSetReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 		//
 		// Changes to watched resources will cause this to be reconciled sooner than
 		// the RequeueAfter time.
-		log.Errorf("error occurred during application generation: %s", validateError.Error())
+		log.Errorf("error occurred during application validation: %s", err.Error())
 		return ctrl.Result{RequeueAfter: ReconcileRequeueOnValidationError}, nil
 	}
 
+	var validApps []argov1alpha1.Application
+	for i := range desiredApplications {
+		if validateErrors[i] == nil {
+			validApps = append(validApps, desiredApplications[i])
+		}
+	}
+
+	if len(validateErrors) > 0 {
+		var message string
+		for _, v := range validateErrors {
+			message = v.Error()
+			break
+		}
+		if len(validateErrors) > 1 {
+			message = fmt.Sprintf("%s (and %d more)", message, len(validateErrors)-1)
+		}
+		log.Errorf("error occurred during application validation: %s", message)
+	}
+
 	if r.Policy.Update() {
-		err = r.createOrUpdateInCluster(ctx, applicationSetInfo, desiredApplications)
+		err = r.createOrUpdateInCluster(ctx, applicationSetInfo, validApps)
 		if err != nil {
 			return ctrl.Result{}, err
 		}
 	} else {
-		err = r.createInCluster(ctx, applicationSetInfo, desiredApplications)
+		err = r.createInCluster(ctx, applicationSetInfo, validApps)
 		if err != nil {
 			return ctrl.Result{}, err
 		}
@@ -146,48 +166,44 @@ func (r *ApplicationSetReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 
 // validateGeneratedApplications uses the Argo CD validation functions to verify the correctness of the
 // generated applications.
-func (r *ApplicationSetReconciler) validateGeneratedApplications(ctx context.Context, desiredApplications []argov1alpha1.Application, applicationSetInfo argoprojiov1alpha1.ApplicationSet, namespace string) (err error) {
-
-	if hasDuplicates, name := hasDuplicateNames(desiredApplications); hasDuplicates {
-		return fmt.Errorf("ApplicationSet %s contains applications with duplicate name: %s", applicationSetInfo.Name, name)
-	}
-
-	for _, app := range desiredApplications {
+func (r *ApplicationSetReconciler) validateGeneratedApplications(ctx context.Context, desiredApplications []argov1alpha1.Application, applicationSetInfo argoprojiov1alpha1.ApplicationSet, namespace string) (map[int]error, error) {
+	errorsByIndex := map[int]error{}
+	namesSet := map[string]bool{}
+	for i, app := range desiredApplications {
+
+		if !namesSet[app.Name] {
+			namesSet[app.Name] = true
+		} else {
+			errorsByIndex[i] = fmt.Errorf("ApplicationSet %s contains applications with duplicate name: %s", applicationSetInfo.Name, app.Name)
+			continue
+		}
 
 		proj, err := r.ArgoAppClientset.ArgoprojV1alpha1().AppProjects(namespace).Get(ctx, app.Spec.GetProject(), metav1.GetOptions{})
 		if err != nil {
 			if apierr.IsNotFound(err) {
-				return fmt.Errorf("application references project %s which does not exist", app.Spec.Project)
+				errorsByIndex[i] = fmt.Errorf("application references project %s which does not exist", app.Spec.Project)
+				continue
 			}
-			return err
+			return nil, err
 		}
 
 		if err := utils.ValidateDestination(ctx, &app.Spec.Destination, r.KubeClientset, namespace); err != nil {
-			return fmt.Errorf("application destination spec is invalid: %s", err.Error())
+			errorsByIndex[i] = fmt.Errorf("application destination spec is invalid: %s", err.Error())
+			continue
 		}
 
 		conditions, err := argoutil.ValidatePermissions(ctx, &app.Spec, proj, r.ArgoDB)
 		if err != nil {
-			return err
+			return nil, err
 		}
 		if len(conditions) > 0 {
-			return fmt.Errorf("application spec is invalid: %s", argoutil.FormatAppConditions(conditions))
+			errorsByIndex[i] = fmt.Errorf("application spec is invalid: %s", argoutil.FormatAppConditions(conditions))
+			continue
 		}
 
 	}
 
-	return nil
-}
-
-func hasDuplicateNames(applications []argov1alpha1.Application) (bool, string) {
-	nameSet := map[string]struct{}{}
-	for _, app := range applications {
-		if _, present := nameSet[app.Name]; present {
-			return true, app.Name
-		}
-		nameSet[app.Name] = struct{}{}
-	}
-	return false, ""
+	return errorsByIndex, nil
 }
 
 func (r *ApplicationSetReconciler) getMinRequeueAfter(applicationSetInfo *argoprojiov1alpha1.ApplicationSet) time.Duration {


Test patch:
diff --git a/pkg/controllers/applicationset_controller_test.go b/pkg/controllers/applicationset_controller_test.go
index 1b9a47b..9a1e2a2 100644
--- a/pkg/controllers/applicationset_controller_test.go
+++ b/pkg/controllers/applicationset_controller_test.go
@@ -1516,65 +1516,6 @@ func TestGetMinRequeueAfter(t *testing.T) {
 
 	assert.Equal(t, time.Duration(1)*time.Second, got)
 }
-func TestHasDuplicateNames(t *testing.T) {
-
-	scheme := runtime.NewScheme()
-	err := argoprojiov1alpha1.AddToScheme(scheme)
-	assert.Nil(t, err)
-	err = argov1alpha1.AddToScheme(scheme)
-	assert.Nil(t, err)
-
-	for _, c := range []struct {
-		testName      string
-		desiredApps   []argov1alpha1.Application
-		hasDuplicates bool
-		duplicateName string
-	}{
-		{
-			testName: "has no duplicates",
-			desiredApps: []argov1alpha1.Application{
-				{
-					ObjectMeta: metav1.ObjectMeta{
-						Name: "app1",
-					},
-				},
-				{
-					ObjectMeta: metav1.ObjectMeta{
-						Name: "app2",
-					},
-				},
-			},
-			hasDuplicates: false,
-			duplicateName: "",
-		},
-		{
-			testName: "has duplicates",
-			desiredApps: []argov1alpha1.Application{
-				{
-					ObjectMeta: metav1.ObjectMeta{
-						Name: "app1",
-					},
-				},
-				{
-					ObjectMeta: metav1.ObjectMeta{
-						Name: "app2",
-					},
-				},
-				{
-					ObjectMeta: metav1.ObjectMeta{
-						Name: "app1",
-					},
-				},
-			},
-			hasDuplicates: true,
-			duplicateName: "app1",
-		},
-	} {
-		hasDuplicates, name := hasDuplicateNames(c.desiredApps)
-		assert.Equal(t, c.hasDuplicates, hasDuplicates)
-		assert.Equal(t, c.duplicateName, name)
-	}
-}
 
 func TestValidateGeneratedApplications(t *testing.T) {
 
@@ -1775,15 +1716,19 @@ func TestValidateGeneratedApplications(t *testing.T) {
 
 			appSetInfo := argoprojiov1alpha1.ApplicationSet{}
 
-			err := r.validateGeneratedApplications(context.TODO(), cc.apps, appSetInfo, "namespace")
+			validationErrors, err := r.validateGeneratedApplications(context.TODO(), cc.apps, appSetInfo, "namespace")
+			var errorMessages []string
+			for _, v := range validationErrors {
+				errorMessages = append(errorMessages, v.Error())
+			}
 
-			if err == nil {
+			if len(errorMessages) == 0 {
 				assert.Equal(t, len(cc.expectedErrors), 0, "Expected errors but none were seen")
 			} else {
 				// An error was returned: it should be expected
 				matched := false
 				for _, expectedErr := range cc.expectedErrors {
-					foundMatch := strings.Contains(err.Error(), expectedErr)
+					foundMatch := strings.Contains(strings.Join(errorMessages, ";"), expectedErr)
 					assert.True(t, foundMatch, "Unble to locate expected error: %s", cc.expectedErrors)
 					matched = matched || foundMatch
 				}
@@ -1801,6 +1746,10 @@ func TestReconcilerValidationErrorBehaviour(t *testing.T) {
 	err = argov1alpha1.AddToScheme(scheme)
 	assert.Nil(t, err)
 
+	defaultProject := argov1alpha1.AppProject{
+		ObjectMeta: metav1.ObjectMeta{Name: "default", Namespace: "argocd"},
+		Spec:       argov1alpha1.AppProjectSpec{SourceRepos: []string{"*"}, Destinations: []argov1alpha1.ApplicationDestination{{Namespace: "*", Server: "https://good-cluster"}}},
+	}
 	appSet := argoprojiov1alpha1.ApplicationSet{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      "name",
@@ -1810,18 +1759,38 @@ func TestReconcilerValidationErrorBehaviour(t *testing.T) {
 			Generators: []argoprojiov1alpha1.ApplicationSetGenerator{
 				{List: &argoprojiov1alpha1.ListGenerator{
 					Elements: []apiextensionsv1.JSON{{
-						Raw: []byte(`{"cluster": "my-cluster","url": "https://kubernetes.default.svc"}`),
+						Raw: []byte(`{"cluster": "good-cluster","url": "https://good-cluster"}`),
+					}, {
+						Raw: []byte(`{"cluster": "bad-cluster","url": "https://bad-cluster"}`),
 					}},
 				}},
 			},
-			Template: argoprojiov1alpha1.ApplicationSetTemplate{},
+			Template: argoprojiov1alpha1.ApplicationSetTemplate{
+				ApplicationSetTemplateMeta: argoprojiov1alpha1.ApplicationSetTemplateMeta{
+					Name:      "{{cluster}}",
+					Namespace: "argocd",
+				},
+				Spec: argov1alpha1.ApplicationSpec{
+					Source:      argov1alpha1.ApplicationSource{RepoURL: "https://github.com/argoproj/argocd-example-apps", Path: "guestbook"},
+					Project:     "default",
+					Destination: argov1alpha1.ApplicationDestination{Server: "{{url}}"},
+				},
+			},
 		},
 	}
-	kubeclientset := kubefake.NewSimpleClientset([]runtime.Object{}...)
+
+	kubeclientset := kubefake.NewSimpleClientset()
 	argoDBMock := dbmocks.ArgoDB{}
-	argoObjs := []runtime.Object{}
+	argoObjs := []runtime.Object{&defaultProject}
 
 	client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(&appSet).Build()
+	goodCluster := argov1alpha1.Cluster{Server: "https://good-cluster", Name: "good-cluster"}
+	badCluster := argov1alpha1.Cluster{Server: "https://bad-cluster", Name: "bad-cluster"}
+	argoDBMock.On("GetCluster", mock.Anything, "https://good-cluster").Return(&goodCluster, nil)
+	argoDBMock.On("GetCluster", mock.Anything, "https://bad-cluster").Return(&badCluster, nil)
+	argoDBMock.On("ListClusters", mock.Anything).Return(&argov1alpha1.ClusterList{Items: []argov1alpha1.Cluster{
+		goodCluster,
+	}}, nil)
 
 	r := ApplicationSetReconciler{
 		Log:      ctrl.Log.WithName("controllers").WithName("ApplicationSet"),
@@ -1835,6 +1804,7 @@ func TestReconcilerValidationErrorBehaviour(t *testing.T) {
 		ArgoDB:           &argoDBMock,
 		ArgoAppClientset: appclientset.NewSimpleClientset(argoObjs...),
 		KubeClientset:    kubeclientset,
+		Policy:           &utils.SyncPolicy{},
 	}
 
 	req := ctrl.Request{
@@ -1847,6 +1817,16 @@ func TestReconcilerValidationErrorBehaviour(t *testing.T) {
 	// Verify that on validation error, no error is returned, but the object is requeued
 	res, err := r.Reconcile(context.Background(), req)
 	assert.Nil(t, err)
-	assert.True(t, res.RequeueAfter > 0)
+	assert.True(t, res.RequeueAfter == 0)
+
+	var app argov1alpha1.Application
+
+	// make sure good app got created
+	err = r.Client.Get(context.TODO(), crtclient.ObjectKey{Namespace: "argocd", Name: "good-cluster"}, &app)
+	assert.NoError(t, err)
+	assert.Equal(t, app.Name, "good-cluster")
 
+	// make sure bad app was not created
+	err = r.Client.Get(context.TODO(), crtclient.ObjectKey{Namespace: "argocd", Name: "bad-cluster"}, &app)
+	assert.Error(t, err)
 }