Repository: argoproj-labs/applicationset
Pull request #38 (2020-09-04 13:55:23)

Fix rbac error listing Secrets at cluster scope.
The Secret Watch established in the ApplicationSet controller is global,
and rbac for global secrets was recently removed resulting in a broken
controller.

To work past this and avoid re-introducing global Secret watch RBAC, the
Manager's cache is now limited to only the namespace in which it is
running. This implies that any attempts to use the client outside that
namespace will not work. However this should be safe as we work with
Applications and Secrets, both of which should not exist beyond the
ArgoCD namespace where we're running.

mgoodness: Cluster-wide Secret access was restored in #21, but I agree that this is a far better approach.
dgoodwin: My mistake I forgot to update branch today I think. I will push an addition here to drop global Secret rbac, sound ok?
dgoodwin: Actually do we have any need for a cluster-install at all? I believe Argo does that so it can sync resource to the local cluster, however we don't need to do that. I think a namespace install may be all we need. Does that seem right?
mgoodness: > Actually do we have any need for a cluster-install at all? I believe Argo does that so it can sync resource to the local cluster, however we don't need to do that. I think a namespace install may be all we need. Does that seem right?

You may very well be correct. I'll look into this today.

Patch:
diff --git a/main.go b/main.go
index 8aabb5c..df68e33 100644
--- a/main.go
+++ b/main.go
@@ -6,6 +6,8 @@ import (
 	"github.com/argoproj-labs/applicationset/pkg/generators"
 	"github.com/argoproj-labs/applicationset/pkg/services"
 	argov1alpha1 "github.com/argoproj/argo-cd/pkg/apis/application/v1alpha1"
+	"sigs.k8s.io/controller-runtime/pkg/cache"
+
 	"os"
 
 	"k8s.io/apimachinery/pkg/runtime"
@@ -51,9 +53,23 @@ func main() {
 
 	ctrl.SetLogger(zap.New(zap.UseDevMode(true)))
 
+	// Determine the namespace we're running in. Normally injected into the pod as an env
+	// var via the Kube downward API configured in the Deployment.
+	// Developers running the binary locally will need to remember to set the NAMESPACE environment variable.
+	ns := os.Getenv("NAMESPACE")
+	if len(ns) == 0 {
+		setupLog.Info("Please set NAMESPACE environment variable to match where you are running the applicationset controller")
+		os.Exit(1)
+	}
+	setupLog.Info("using argocd namespace", "namespace", ns)
+
 	mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{
 		Scheme:                 scheme,
 		MetricsBindAddress:     metricsAddr,
+		// Our cache and thus watches and client queries are restricted to the namespace we're running in. This assumes
+		// the applicationset controller is in the same namespace as argocd, which should be the same namespace of
+		// all cluster Secrets and Applications we interact with.
+		NewCache: cache.MultiNamespacedCacheBuilder([]string{ns}),
 		HealthProbeBindAddress: probeBindAddr,
 		Port:                   9443,
 		LeaderElection:         enableLeaderElection,
diff --git a/manifests/base/deployment.yaml b/manifests/base/deployment.yaml
index b1cd8a7..9b4e3e7 100644
--- a/manifests/base/deployment.yaml
+++ b/manifests/base/deployment.yaml
@@ -21,4 +21,9 @@ spec:
           image: registry.cn-hangzhou.aliyuncs.com/appcenter/argocd-applicationset:v0.1.0
           imagePullPolicy: Always
           name: argocd-applicationset-controller
+          env:
+            - name: NAMESPACE
+              valueFrom:
+                fieldRef:
+                  fieldPath: metadata.namespace
       serviceAccountName: argocd-applicationset-controller
diff --git a/pkg/controllers/applicationset_controller.go b/pkg/controllers/applicationset_controller.go
index 1144bd9..0592750 100644
--- a/pkg/controllers/applicationset_controller.go
+++ b/pkg/controllers/applicationset_controller.go
@@ -57,15 +57,26 @@ func (r *ApplicationSetReconciler) Reconcile(req ctrl.Request) (ctrl.Result, err
 
 	var applicationSetInfo argoprojiov1alpha1.ApplicationSet
 	if err := r.Get(ctx, req.NamespacedName, &applicationSetInfo); err != nil {
-		log.Infof("Unable to fetch applicationSetInfo %e", err)
+		if client.IgnoreNotFound(err) != nil {
+			log.WithField("request", req).WithError(err).Infof("unable to get ApplicationSet")
+		}
 		return ctrl.Result{}, client.IgnoreNotFound(err)
 	}
 
 	// desiredApplications is the main list of all expected Applications from all generators in this appset.
-	desiredApplications := r.generateApplications(applicationSetInfo)
+	desiredApplications, err := r.generateApplications(applicationSetInfo)
+	if err != nil {
+		return ctrl.Result{}, err
+	}
 
-	r.createOrUpdateInCluster(ctx, applicationSetInfo, desiredApplications)
-	r.deleteInCluster(ctx, applicationSetInfo, desiredApplications)
+	err = r.createOrUpdateInCluster(ctx, applicationSetInfo, desiredApplications)
+	if err != nil {
+		return ctrl.Result{}, err
+	}
+	err = r.deleteInCluster(ctx, applicationSetInfo, desiredApplications)
+	if err != nil {
+		return ctrl.Result{}, err
+	}
 
 	return ctrl.Result{}, nil
 }
@@ -80,9 +91,10 @@ func getTempApplication(applicationSetTemplate argoprojiov1alpha1.ApplicationSet
 	return &tmplApplication
 }
 
-func (r *ApplicationSetReconciler) generateApplications(applicationSetInfo argoprojiov1alpha1.ApplicationSet) []argov1alpha1.Application {
+func (r *ApplicationSetReconciler) generateApplications(applicationSetInfo argoprojiov1alpha1.ApplicationSet) ([]argov1alpha1.Application, error) {
 	res := []argov1alpha1.Application{}
 
+	var firstError error
 	tmplApplication := getTempApplication(applicationSetInfo.Spec.Template)
 	for _, requestedGenerator := range applicationSetInfo.Spec.Generators {
 		for _, g := range r.Generators {
@@ -90,6 +102,9 @@ func (r *ApplicationSetReconciler) generateApplications(applicationSetInfo argop
 			if err != nil {
 				log.WithError(err).WithField("generator", g).
 					Error("error generating params")
+				if firstError == nil {
+					firstError = err
+				}
 				continue
 			}
 
@@ -98,6 +113,9 @@ func (r *ApplicationSetReconciler) generateApplications(applicationSetInfo argop
 				if err != nil {
 					log.WithError(err).WithField("params", params).WithField("generator", g).
 						Error("error generating application from params")
+					if firstError == nil {
+						firstError = err
+					}
 					continue
 				}
 				res = append(res, *app)
@@ -108,7 +126,7 @@ func (r *ApplicationSetReconciler) generateApplications(applicationSetInfo argop
 
 		}
 	}
-	return res
+	return res, firstError
 }
 
 func (r *ApplicationSetReconciler) SetupWithManager(mgr ctrl.Manager) error {
@@ -147,8 +165,9 @@ func (r *ApplicationSetReconciler) SetupWithManager(mgr ctrl.Manager) error {
 // For new application it will call create
 // For application that need to update it will call update
 // The function also adds owner reference to all applications, and uses it for delete them.
-func (r *ApplicationSetReconciler) createOrUpdateInCluster(ctx context.Context, applicationSet argoprojiov1alpha1.ApplicationSet, desiredApplications []argov1alpha1.Application) {
+func (r *ApplicationSetReconciler) createOrUpdateInCluster(ctx context.Context, applicationSet argoprojiov1alpha1.ApplicationSet, desiredApplications []argov1alpha1.Application) error {
 
+	var firstError error
 	//create or updates the application in appList
 	for _, app := range desiredApplications {
 		appLog := log.WithFields(log.Fields{"app": app.Name, "appSet": applicationSet.Name})
@@ -161,18 +180,22 @@ func (r *ApplicationSetReconciler) createOrUpdateInCluster(ctx context.Context,
 		})
 
 		if err != nil {
-			appLog.WithError(err).Errorf("failed to %s Application", action)
+			appLog.WithError(err).WithField("action", action).Error("failed to create/update Application")
+			if firstError == nil {
+				firstError = err
+			}
 			continue
 		}
 
 		r.Recorder.Eventf(&applicationSet, core.EventTypeNormal, fmt.Sprint(action), "%s Application %q", action, app.Name)
 		appLog.Logf(log.InfoLevel, "%s Application", action)
 	}
+	return firstError
 }
 
 // deleteInCluster will delete application that are current in the cluster but not in appList.
 // The function must be called after all generators had been called and generated applications
-func (r *ApplicationSetReconciler) deleteInCluster(ctx context.Context, applicationSet argoprojiov1alpha1.ApplicationSet, desiredApplications []argov1alpha1.Application) {
+func (r *ApplicationSetReconciler) deleteInCluster(ctx context.Context, applicationSet argoprojiov1alpha1.ApplicationSet, desiredApplications []argov1alpha1.Application) error {
 
 	// Save current applications to be able to delete the ones that are not in appList
 	var current argov1alpha1.ApplicationList
@@ -185,6 +208,7 @@ func (r *ApplicationSetReconciler) deleteInCluster(ctx context.Context, applicat
 	}
 
 	// Delete apps that are not in m[string]bool
+	var firstError error
 	for _, app := range current.Items {
 		appLog := log.WithFields(log.Fields{"app": app.Name, "appSet": applicationSet.Name})
 		_, exists := m[app.Name]
@@ -193,12 +217,16 @@ func (r *ApplicationSetReconciler) deleteInCluster(ctx context.Context, applicat
 			err := r.Client.Delete(ctx, &app)
 			if err != nil {
 				appLog.WithError(err).Error("failed to delete Application")
+				if firstError != nil {
+					firstError = err
+				}
 				continue
 			}
 			r.Recorder.Eventf(&applicationSet, core.EventTypeNormal, "Deleted", "Deleted Application %q", app.Name)
 			appLog.Log(log.InfoLevel, "Deleted application")
 		}
 	}
+	return firstError
 }
 
 var _ handler.EventHandler = &clusterSecretEventHandler{}


Test patch:
diff --git a/pkg/controllers/applicationset_controller_test.go b/pkg/controllers/applicationset_controller_test.go
index 3c969bb..81643bd 100644
--- a/pkg/controllers/applicationset_controller_test.go
+++ b/pkg/controllers/applicationset_controller_test.go
@@ -57,6 +57,7 @@ func TestExtractApplications(t *testing.T) {
 		template			argoprojiov1alpha1.ApplicationSetTemplate
 		generateParamsError	error
 		rendererError		error
+		expectErr bool
 	}{
 		{
 			name: 		"Generate two applications",
@@ -73,8 +74,9 @@ func TestExtractApplications(t *testing.T) {
 			},
 		},
 		{
-			name: 		"Handles error for the generator",
+			name: 		"Handles error from the generator",
 			generateParamsError: errors.New("error"),
+			expectErr: true,
 		},
 		{
 			name: 		"Handles error from the render",
@@ -90,6 +92,7 @@ func TestExtractApplications(t *testing.T) {
 				},
 			},
 			rendererError: errors.New("error"),
+			expectErr: true,
 		},
 	}{
 		cc := c
@@ -137,7 +140,7 @@ func TestExtractApplications(t *testing.T) {
 				Renderer: &rendererMock,
 			}
 
-			got := r.generateApplications(argoprojiov1alpha1.ApplicationSet{
+			got, err := r.generateApplications(argoprojiov1alpha1.ApplicationSet{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:      "name",
 					Namespace: "namespace",
@@ -148,6 +151,11 @@ func TestExtractApplications(t *testing.T) {
 				},
 			},)
 
+			if cc.expectErr {
+				assert.Error(t, err)
+			} else {
+				assert.NoError(t, err)
+			}
 			assert.Equal(t, expectedApps, got)
 			generatorMock.AssertNumberOfCalls(t, "GenerateParams", 1)