Prevent ApplicationSet controller from creating invalid Applications, causing 'unable to delete application resource' in Argo CD

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)

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,17 +45,23 @@ 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"
 )
 
 // 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)

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)
+			}
+		})
+	}
+}

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),
 	}
 }