Repository: argoproj-labs/applicationset (go)

Issue:
Prevent ApplicationSet controller from creating invalid Applications, causing 'unable to delete application resource' in Argo CD
While writing a new E2E test, I happened to create an `ApplicationSet` resource that produced invalid Argo CD applictions:
- I created the ApplicationSet, and the ApplicationSet controller correctly created the Application.
- However, the created Application was invalid.
- Argo CD correctly complained that `application destination can't have both name and server defined: in-cluster`.

It was correct for Argo CD to complain about this, because *I did*, in fact, have both a name and a server defined in my ApplicationSet. The ApplicationSet I had defined was producing an invalid Application via the templating process.

The ApplicationSet controller should *never* create invalid Argo CD `Application`s (thus leaving it up to Argo CD to deal with that), it should instead detect if an `ApplicationSet` will produce an invalid `Application`, and rather than creating the `Application` it should instead log an error to the  console (and, in the future, report a message as a condition within the status field).


This invalid `Application` also prevented Argo CD from deleting the Application, with this message printed on delete (eg via the finalizer):
`
Unable to delete application resources: application destination can't have both name and server defined: in-cluster
`

This was the invalid Application that was produced:
```yaml
apiVersion: argoproj.io/v1alpha1
kind: Application
metadata:
  finalizers:
  - resources-finalizer.argocd.argoproj.io
  name: in-cluster-guestbook
  namespace: argocd-e2e
spec:
  destination:
    name: in-cluster
    namespace: guestbook2
    server: https://kubernetes.default.svc
  project: default
  source:
    path: guestbook
    repoURL: https://github.com/argoproj/argocd-example-apps.git
    targetRevision: HEAD
```


Pull request:
Prevent ApplicationSet controller from creating invalid Applications, causing 'unable to delete application resource' in Argo CD
Fixes #112 

This PR:
- Use the Argo CD  "github.com/argoproj/argo-cd/util/argo" package's `ValidateDestination` and `ValidatePermissions` to verify generated application validity
- Adds unit tests to verify some of the desired validations

Patch:
diff --git a/api/v1alpha1/applicationset_types.go b/api/v1alpha1/applicationset_types.go
index 6e6c58f..ed4ed1b 100644
--- a/api/v1alpha1/applicationset_types.go
+++ b/api/v1alpha1/applicationset_types.go
@@ -98,7 +98,7 @@ type GitGenerator struct {
 	Directories         []GitDirectoryGeneratorItem `json:"directories,omitempty"`
 	Files               []GitFileGeneratorItem      `json:"files,omitempty"`
 	Revision            string                      `json:"revision"`
-	RequeueAfterSeconds int64                       `json:"requeueAfterSeconds,omitempty"`
+	RequeueAfterSeconds *int64                      `json:"requeueAfterSeconds,omitempty"`
 	Template            ApplicationSetTemplate      `json:"template,omitempty"`
 }
 
diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go
index 05ddb1e..4353e89 100644
--- a/api/v1alpha1/zz_generated.deepcopy.go
+++ b/api/v1alpha1/zz_generated.deepcopy.go
@@ -284,6 +284,11 @@ func (in *GitGenerator) DeepCopyInto(out *GitGenerator) {
 		*out = make([]GitFileGeneratorItem, len(*in))
 		copy(*out, *in)
 	}
+	if in.RequeueAfterSeconds != nil {
+		in, out := &in.RequeueAfterSeconds, &out.RequeueAfterSeconds
+		*out = new(int64)
+		**out = **in
+	}
 	in.Template.DeepCopyInto(&out.Template)
 }
 
diff --git a/main.go b/main.go
index f055fb8..33fa32e 100644
--- a/main.go
+++ b/main.go
@@ -31,11 +31,14 @@ import (
 	"github.com/argoproj-labs/applicationset/pkg/utils"
 
 	argov1alpha1 "github.com/argoproj/argo-cd/pkg/apis/application/v1alpha1"
+	"github.com/argoproj/argo-cd/util/db"
+	argosettings "github.com/argoproj/argo-cd/util/settings"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/client-go/kubernetes"
 	clientgoscheme "k8s.io/client-go/kubernetes/scheme"
 	_ "k8s.io/client-go/plugin/pkg/client/auth/gcp"
 
+	appclientset "github.com/argoproj/argo-cd/pkg/client/clientset/versioned"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/cache"
 	"sigs.k8s.io/controller-runtime/pkg/log/zap"
@@ -124,19 +127,25 @@ func main() {
 	}
 
 	k8s := kubernetes.NewForConfigOrDie(mgr.GetConfig())
+	argoSettingsMgr := argosettings.NewSettingsManager(context.Background(), k8s, namespace)
+	appclientset := appclientset.NewForConfigOrDie(mgr.GetConfig())
+
+	argoCDDB := db.NewDB(namespace, argoSettingsMgr, k8s)
 
 	if err = (&controllers.ApplicationSetReconciler{
 		Generators: map[string]generators.Generator{
 			"List":     generators.NewListGenerator(),
 			"Clusters": generators.NewClusterGenerator(mgr.GetClient(), context.Background(), k8s, namespace),
-			"Git":      generators.NewGitGenerator(services.NewArgoCDService(context.Background(), k8s, namespace, argocdRepoServer)),
+			"Git":      generators.NewGitGenerator(services.NewArgoCDService(argoCDDB, argocdRepoServer)),
 		},
-		Client:   mgr.GetClient(),
-		Log:      ctrl.Log.WithName("controllers").WithName("ApplicationSet"),
-		Scheme:   mgr.GetScheme(),
-		Recorder: mgr.GetEventRecorderFor("applicationset-controller"),
-		Renderer: &utils.Render{},
-		Policy:   policyObj,
+		Client:           mgr.GetClient(),
+		Log:              ctrl.Log.WithName("controllers").WithName("ApplicationSet"),
+		Scheme:           mgr.GetScheme(),
+		Recorder:         mgr.GetEventRecorderFor("applicationset-controller"),
+		Renderer:         &utils.Render{},
+		Policy:           policyObj,
+		ArgoAppClientset: appclientset,
+		ArgoDB:           argoCDDB,
 	}).SetupWithManager(mgr); err != nil {
 		setupLog.Error(err, "unable to create controller", "controller", "ApplicationSet")
 		os.Exit(1)
diff --git a/pkg/controllers/applicationset_controller.go b/pkg/controllers/applicationset_controller.go
index 0675980..01fdcc5 100644
--- a/pkg/controllers/applicationset_controller.go
+++ b/pkg/controllers/applicationset_controller.go
@@ -28,6 +28,8 @@ import (
 	"github.com/argoproj-labs/applicationset/pkg/generators"
 	"github.com/argoproj-labs/applicationset/pkg/utils"
 	argov1alpha1 "github.com/argoproj/argo-cd/pkg/apis/application/v1alpha1"
+	"github.com/argoproj/argo-cd/util/argo"
+	"github.com/argoproj/argo-cd/util/db"
 	"github.com/go-logr/logr"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -43,6 +45,10 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	argoprojiov1alpha1 "github.com/argoproj-labs/applicationset/api/v1alpha1"
+	appclientset "github.com/argoproj/argo-cd/pkg/client/clientset/versioned"
+	argoutil "github.com/argoproj/argo-cd/util/argo"
+
+	apierr "k8s.io/apimachinery/pkg/api/errors"
 
 	"github.com/imdario/mergo"
 )
@@ -50,10 +56,12 @@ import (
 // ApplicationSetReconciler reconciles a ApplicationSet object
 type ApplicationSetReconciler struct {
 	client.Client
-	Log        logr.Logger
-	Scheme     *runtime.Scheme
-	Recorder   record.EventRecorder
-	Generators map[string]generators.Generator
+	Log              logr.Logger
+	Scheme           *runtime.Scheme
+	Recorder         record.EventRecorder
+	Generators       map[string]generators.Generator
+	ArgoDB           db.ArgoDB
+	ArgoAppClientset appclientset.Interface
 	utils.Policy
 	utils.Renderer
 }
@@ -81,7 +89,8 @@ func (r *ApplicationSetReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 	if err != nil {
 		return ctrl.Result{}, err
 	}
-	if hasDuplicates, name := hasDuplicateNames(desiredApplications); hasDuplicates {
+
+	if validateError := r.validateGeneratedApplications(ctx, desiredApplications, applicationSetInfo, req.Namespace); validateError != nil {
 		// The reconciler presumes that any errors that are returned are a signal
 		// that the resource should attempt to be reconciled again (causing
 		// Reconcile to be called again, which will return the same error, ad
@@ -93,7 +102,7 @@ func (r *ApplicationSetReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 		// will fail. So just log it and return that the resource was
 		// successfully reconciled (which is true... it was reconciled to an
 		// error condition).
-		log.Errorf("ApplicationSet %s contains applications with duplicate name: %s", applicationSetInfo.Name, name)
+		log.Errorf("%s", validateError.Error())
 		return ctrl.Result{}, nil
 	}
 
@@ -124,6 +133,41 @@ func (r *ApplicationSetReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 	}, nil
 }
 
+// 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 {
+
+		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)
+			}
+			return err
+		}
+
+		if err := argoutil.ValidateDestination(ctx, &app.Spec.Destination, r.ArgoDB); err != nil {
+			return fmt.Errorf("application destination spec is invalid: %s", err.Error())
+		}
+
+		conditions, err := argoutil.ValidatePermissions(ctx, &app.Spec, proj, r.ArgoDB)
+		if err != nil {
+			return err
+		}
+		if len(conditions) > 0 {
+			return fmt.Errorf("application spec is invalid: %s", argo.FormatAppConditions(conditions))
+		}
+
+	}
+
+	return nil
+}
+
 // Log a warning if there are unrecognized generators
 func checkInvalidGenerators(applicationSetInfo *argoprojiov1alpha1.ApplicationSet) {
 	hasInvalidGenerators, invalidGenerators := invalidGenerators(applicationSetInfo)
diff --git a/pkg/generators/git.go b/pkg/generators/git.go
index 9c58fb0..93e22ce 100644
--- a/pkg/generators/git.go
+++ b/pkg/generators/git.go
@@ -15,6 +15,10 @@ import (
 
 var _ Generator = (*GitGenerator)(nil)
 
+const (
+	DefaultRequeueAfterSeconds = 3 * time.Minute
+)
+
 type GitGenerator struct {
 	repos services.Repos
 }
@@ -31,7 +35,14 @@ func (g *GitGenerator) GetTemplate(appSetGenerator *argoprojiov1alpha1.Applicati
 }
 
 func (g *GitGenerator) GetRequeueAfter(appSetGenerator *argoprojiov1alpha1.ApplicationSetGenerator) time.Duration {
-	return time.Duration(appSetGenerator.Git.RequeueAfterSeconds) * time.Second
+
+	// Return a requeue default of 3 minutes, if no default is specified.
+
+	if appSetGenerator.Git.RequeueAfterSeconds != nil {
+		return time.Duration(*appSetGenerator.Git.RequeueAfterSeconds) * time.Second
+	}
+
+	return DefaultRequeueAfterSeconds
 }
 
 func (g *GitGenerator) GenerateParams(appSetGenerator *argoprojiov1alpha1.ApplicationSetGenerator) ([]map[string]string, error) {
diff --git a/pkg/services/repo_service.go b/pkg/services/repo_service.go
index 3496324..3c22303 100644
--- a/pkg/services/repo_service.go
+++ b/pkg/services/repo_service.go
@@ -10,10 +10,8 @@ import (
 	"github.com/argoproj/argo-cd/util/db"
 	"github.com/argoproj/argo-cd/util/git"
 	"github.com/argoproj/argo-cd/util/io"
-	"github.com/argoproj/argo-cd/util/settings"
 	"github.com/pkg/errors"
 	log "github.com/sirupsen/logrus"
-	"k8s.io/client-go/kubernetes"
 )
 
 // RepositoryDB Is a lean facade for ArgoDB,
@@ -33,11 +31,10 @@ type Repos interface {
 	GetFileContent(ctx context.Context, repoURL string, revision string, path string) ([]byte, error)
 }
 
-func NewArgoCDService(ctx context.Context, clientset kubernetes.Interface, namespace string, repoServerAddress string) Repos {
-	settingsMgr := settings.NewSettingsManager(ctx, clientset, namespace)
+func NewArgoCDService(db db.ArgoDB, repoServerAddress string) Repos {
 
 	return &argoCDService{
-		repositoriesDB: db.NewDB(namespace, settingsMgr, clientset).(RepositoryDB),
+		repositoriesDB: db.(RepositoryDB),
 		repoClientset:  apiclient.NewRepoServerClientset(repoServerAddress, 5),
 	}
 }


Test patch:
diff --git a/pkg/controllers/applicationset_controller_test.go b/pkg/controllers/applicationset_controller_test.go
index c086d53..8ad98d1 100644
--- a/pkg/controllers/applicationset_controller_test.go
+++ b/pkg/controllers/applicationset_controller_test.go
@@ -4,10 +4,12 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"strings"
 	"testing"
 	"time"
 
 	"github.com/argoproj-labs/applicationset/pkg/generators"
+	"github.com/argoproj/argo-cd/pkg/apis/application/v1alpha1"
 	argov1alpha1 "github.com/argoproj/argo-cd/pkg/apis/application/v1alpha1"
 	"github.com/sirupsen/logrus"
 	logtest "github.com/sirupsen/logrus/hooks/test"
@@ -22,6 +24,8 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
 	argoprojiov1alpha1 "github.com/argoproj-labs/applicationset/api/v1alpha1"
+	appclientset "github.com/argoproj/argo-cd/pkg/client/clientset/versioned/fake"
+	dbmocks "github.com/argoproj/argo-cd/util/db/mocks"
 )
 
 type generatorMock struct {
@@ -1302,3 +1306,201 @@ func TestHasDuplicateNames(t *testing.T) {
 		assert.Equal(t, c.duplicateName, name)
 	}
 }
+
+func TestValidateGeneratedApplications(t *testing.T) {
+
+	scheme := runtime.NewScheme()
+	err := argoprojiov1alpha1.AddToScheme(scheme)
+	assert.Nil(t, err)
+
+	err = argov1alpha1.AddToScheme(scheme)
+	assert.Nil(t, err)
+
+	client := fake.NewClientBuilder().WithScheme(scheme).Build()
+
+	// Valid cluster
+	myCluster := argov1alpha1.Cluster{
+		Server: "https://kubernetes.default.svc",
+		Name:   "my-cluster",
+	}
+
+	// Valid project
+	myProject := &argov1alpha1.AppProject{
+		ObjectMeta: metav1.ObjectMeta{Name: "default", Namespace: "namespace"},
+		Spec: argov1alpha1.AppProjectSpec{
+			SourceRepos: []string{"*"},
+			Destinations: []argov1alpha1.ApplicationDestination{
+				{
+					Namespace: "*",
+					Server:    "*",
+				},
+			},
+			ClusterResourceWhitelist: []metav1.GroupKind{
+				{
+					Group: "*",
+					Kind:  "*",
+				},
+			},
+		},
+	}
+
+	// Test a subset of the validations that 'validateGeneratedApplications' performs
+	for _, cc := range []struct {
+		name           string
+		apps           []argov1alpha1.Application
+		expectedErrors []string
+	}{
+		{
+			name: "valid app should return true",
+			apps: []argov1alpha1.Application{
+				{
+					TypeMeta:   metav1.TypeMeta{},
+					ObjectMeta: metav1.ObjectMeta{},
+					Spec: argov1alpha1.ApplicationSpec{
+						Project: "default",
+						Source: argov1alpha1.ApplicationSource{
+							RepoURL:        "https://url",
+							Path:           "/",
+							TargetRevision: "HEAD",
+						},
+						Destination: argov1alpha1.ApplicationDestination{
+							Namespace: "namespace",
+							Name:      "my-cluster",
+						},
+					},
+				},
+			},
+			expectedErrors: []string{},
+		},
+		{
+			name: "can't have both name and server defined",
+			apps: []argov1alpha1.Application{
+				{
+					TypeMeta:   metav1.TypeMeta{},
+					ObjectMeta: metav1.ObjectMeta{},
+					Spec: argov1alpha1.ApplicationSpec{
+						Project: "default",
+						Source: argov1alpha1.ApplicationSource{
+							RepoURL:        "https://url",
+							Path:           "/",
+							TargetRevision: "HEAD",
+						},
+						Destination: argov1alpha1.ApplicationDestination{
+							Namespace: "namespace",
+							Server:    "my-server",
+							Name:      "my-cluster",
+						},
+					},
+				},
+			},
+			expectedErrors: []string{"application destination can't have both name and server defined"},
+		},
+		{
+			name: "project mismatch should return error",
+			apps: []argov1alpha1.Application{
+				{
+					TypeMeta:   metav1.TypeMeta{},
+					ObjectMeta: metav1.ObjectMeta{},
+					Spec: argov1alpha1.ApplicationSpec{
+						Project: "DOES-NOT-EXIST",
+						Source: argov1alpha1.ApplicationSource{
+							RepoURL:        "https://url",
+							Path:           "/",
+							TargetRevision: "HEAD",
+						},
+						Destination: argov1alpha1.ApplicationDestination{
+							Namespace: "namespace",
+							Name:      "my-cluster",
+						},
+					},
+				},
+			},
+			expectedErrors: []string{"application references project DOES-NOT-EXIST which does not exist"},
+		},
+		{
+			name: "valid app should return true",
+			apps: []argov1alpha1.Application{
+				{
+					TypeMeta:   metav1.TypeMeta{},
+					ObjectMeta: metav1.ObjectMeta{},
+					Spec: argov1alpha1.ApplicationSpec{
+						Project: "default",
+						Source: argov1alpha1.ApplicationSource{
+							RepoURL:        "https://url",
+							Path:           "/",
+							TargetRevision: "HEAD",
+						},
+						Destination: argov1alpha1.ApplicationDestination{
+							Namespace: "namespace",
+							Name:      "my-cluster",
+						},
+					},
+				},
+			},
+			expectedErrors: []string{},
+		},
+		{
+			name: "cluster should match",
+			apps: []argov1alpha1.Application{
+				{
+					TypeMeta:   metav1.TypeMeta{},
+					ObjectMeta: metav1.ObjectMeta{},
+					Spec: argov1alpha1.ApplicationSpec{
+						Project: "default",
+						Source: argov1alpha1.ApplicationSource{
+							RepoURL:        "https://url",
+							Path:           "/",
+							TargetRevision: "HEAD",
+						},
+						Destination: argov1alpha1.ApplicationDestination{
+							Namespace: "namespace",
+							Name:      "nonexistent-cluster",
+						},
+					},
+				},
+			},
+			expectedErrors: []string{"there are no clusters with this name: nonexistent-cluster"},
+		},
+	} {
+
+		t.Run(cc.name, func(t *testing.T) {
+
+			argoDBMock := dbmocks.ArgoDB{}
+			argoDBMock.On("GetCluster", mock.Anything, "https://kubernetes.default.svc").Return(&myCluster, nil)
+			argoDBMock.On("ListClusters", mock.Anything).Return(&v1alpha1.ClusterList{Items: []argov1alpha1.Cluster{
+				myCluster,
+			}}, nil)
+
+			argoObjs := []runtime.Object{myProject}
+			for _, app := range cc.apps {
+				argoObjs = append(argoObjs, &app)
+			}
+
+			r := ApplicationSetReconciler{
+				Client:           client,
+				Scheme:           scheme,
+				Recorder:         record.NewFakeRecorder(1),
+				Generators:       map[string]generators.Generator{},
+				ArgoDB:           &argoDBMock,
+				ArgoAppClientset: appclientset.NewSimpleClientset(argoObjs...),
+			}
+
+			appSetInfo := argoprojiov1alpha1.ApplicationSet{}
+
+			err := r.validateGeneratedApplications(context.TODO(), cc.apps, appSetInfo, "namespace")
+
+			if err == nil {
+				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)
+					assert.True(t, foundMatch, "Unble to locate expected error: %s", cc.expectedErrors)
+					matched = matched || foundMatch
+				}
+				assert.True(t, matched, "An unexpected error occurrred: %v", err)
+			}
+		})
+	}
+}