Repository: argoproj-labs/applicationset
Pull request #296 (2021-07-14 08:55:07)

ApplicationSets that initially fail validation might not be re-reconciled
This PR:
- Now queues a reconcile on validation error
- Unit test to verify that behaviour
- Tweak to unrelated Git unit test

Fixes https://github.com/argoproj-labs/applicationset/issues/219


Patch:
diff --git a/pkg/controllers/applicationset_controller.go b/pkg/controllers/applicationset_controller.go
index 044d94b..d0a3e72 100644
--- a/pkg/controllers/applicationset_controller.go
+++ b/pkg/controllers/applicationset_controller.go
@@ -49,7 +49,8 @@ const (
 	// Rather than importing the whole argocd-notifications controller, just copying the const here
 	//   https://github.com/argoproj-labs/argocd-notifications/blob/33d345fa838829bb50fca5c08523aba380d2c12b/pkg/controller/subscriptions.go#L12
 	//   https://github.com/argoproj-labs/argocd-notifications/blob/33d345fa838829bb50fca5c08523aba380d2c12b/pkg/controller/state.go#L17
-	NotifiedAnnotationKey = "notified.notifications.argoproj.io"
+	NotifiedAnnotationKey             = "notified.notifications.argoproj.io"
+	ReconcileRequeueOnValidationError = time.Minute * 3
 )
 
 // ApplicationSetReconciler reconciles a ApplicationSet object
@@ -96,19 +97,15 @@ func (r *ApplicationSetReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 	}
 
 	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
-		// infinitum until we are exponentially backed off).
+		// 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
+		// with a timeout to give this another shot at a later time.
 		//
-		// In this case, since we know that what the user provided is incorrect
-		// (we successfully generated and templated their ApplicationSet, but the
-		// result of that was bad), no matter how many times we try to do so it
-		// 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("%s", validateError.Error())
-		return ctrl.Result{}, nil
+		// 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())
+		return ctrl.Result{RequeueAfter: ReconcileRequeueOnValidationError}, nil
 	}
 
 	if r.Policy.Update() {


Test patch:
diff --git a/pkg/controllers/applicationset_controller_test.go b/pkg/controllers/applicationset_controller_test.go
index ace35bc..e5fb47a 100644
--- a/pkg/controllers/applicationset_controller_test.go
+++ b/pkg/controllers/applicationset_controller_test.go
@@ -19,7 +19,9 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/tools/record"
+	ctrl "sigs.k8s.io/controller-runtime"
 	crtclient "sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
@@ -27,6 +29,7 @@ import (
 	argoprojiov1alpha1 "github.com/argoproj-labs/applicationset/api/v1alpha1"
 	appclientset "github.com/argoproj/argo-cd/v2/pkg/client/clientset/versioned/fake"
 	dbmocks "github.com/argoproj/argo-cd/v2/util/db/mocks"
+	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	kubefake "k8s.io/client-go/kubernetes/fake"
 )
 
@@ -1790,3 +1793,61 @@ func TestValidateGeneratedApplications(t *testing.T) {
 		})
 	}
 }
+
+func TestReconcilerValidationErrorBehaviour(t *testing.T) {
+
+	scheme := runtime.NewScheme()
+	err := argoprojiov1alpha1.AddToScheme(scheme)
+	assert.Nil(t, err)
+	err = argov1alpha1.AddToScheme(scheme)
+	assert.Nil(t, err)
+
+	appSet := argoprojiov1alpha1.ApplicationSet{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "name",
+			Namespace: "argocd",
+		},
+		Spec: argoprojiov1alpha1.ApplicationSetSpec{
+			Generators: []argoprojiov1alpha1.ApplicationSetGenerator{
+				{List: &argoprojiov1alpha1.ListGenerator{
+					Elements: []apiextensionsv1.JSON{{
+						Raw: []byte(`{"cluster": "my-cluster","url": "https://kubernetes.default.svc"}`),
+					}},
+				}},
+			},
+			Template: argoprojiov1alpha1.ApplicationSetTemplate{},
+		},
+	}
+	kubeclientset := kubefake.NewSimpleClientset([]runtime.Object{}...)
+	argoDBMock := dbmocks.ArgoDB{}
+	argoObjs := []runtime.Object{}
+
+	client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(&appSet).Build()
+
+	r := ApplicationSetReconciler{
+		Log:      ctrl.Log.WithName("controllers").WithName("ApplicationSet"),
+		Client:   client,
+		Scheme:   scheme,
+		Renderer: &utils.Render{},
+		Recorder: record.NewFakeRecorder(1),
+		Generators: map[string]generators.Generator{
+			"List": generators.NewListGenerator(),
+		},
+		ArgoDB:           &argoDBMock,
+		ArgoAppClientset: appclientset.NewSimpleClientset(argoObjs...),
+		KubeClientset:    kubeclientset,
+	}
+
+	req := ctrl.Request{
+		NamespacedName: types.NamespacedName{
+			Namespace: "argocd",
+			Name:      "name",
+		},
+	}
+
+	// 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)
+
+}
diff --git a/pkg/services/scm_provider/github_test.go b/pkg/services/scm_provider/github_test.go
index c57b451..9501d62 100644
--- a/pkg/services/scm_provider/github_test.go
+++ b/pkg/services/scm_provider/github_test.go
@@ -11,7 +11,9 @@ import (
 
 func checkRateLimit(t *testing.T, err error) {
 	// Check if we've hit a rate limit, don't fail the test if so.
-	if err != nil && strings.Contains(err.Error(), "rate limit exceeded") {
+	if err != nil && (strings.Contains(err.Error(), "rate limit exceeded") ||
+		(strings.Contains(err.Error(), "API rate limit") && strings.Contains(err.Error(), "still exceeded"))) {
+
 		allowRateLimitErrors := os.Getenv("CI") == ""
 		t.Logf("Got a rate limit error, consider setting $GITHUB_TOKEN to increase your GitHub API rate limit: %v\n", err)
 		if allowRateLimitErrors {