From 30a1623e38623c3f7cdcbbd2d7fc744991a33382 Mon Sep 17 00:00:00 2001 From: Arthur Outhenin-Chalandre Date: Tue, 16 Jul 2024 23:56:36 +0200 Subject: [PATCH] fix(applicationset): use requeue after if generate app errors out (#18761) The `GenerateApplications` can call to external resources like Github API for instance which might be rate limited or fail. If those requests somehow fail we should requeue them after some time like (same reason as https://github.com/argoproj/argo-cd/blob/e98d3b2a877fb7140880dc1450703a99db4398b5/applicationset/controllers/applicationset_controller.go#L154). For instance, in our environments we were rate limited by Github and the ArgoCD applicationset controller was logging the following error about every second or less for every application set using the pull request generator that we have: ``` time="2024-06-21T14:17:15Z" level=error msg="error generating params" error="error listing repos: error listing pull requests for LedgerHQ/xxx: GET https://api.github.com/repos/LedgerHQ/xxx/pulls?per_page=100: 403 API rate limit exceeded for installation ID xxx. If you reach out to GitHub Support for help, please include the request ID xxx and timestamp 2024-06-xx xxx UTC. [rate reset in 8m18s]" generator="&{0xc000d652c0 0x289a100 {0xc00087bdd0} [] true}" time="2024-06-21T14:17:15Z" level=error msg="error generating application from params" applicationset=argocd/xxx error="error listing repos: error listing pull requests for LedgerHQ/xxxx: GET https://api.github.com/repos/LedgerHQ/xxx/pulls?per_page=100: 403 API rate limit exceeded for installation ID xxx. If you reach out to GitHub Support for help, please include the request ID xxx and timestamp 2024-06-xx xxx UTC. [rate reset in 8m18s]" generator="{nil nil nil nil nil &PullRequestGenerator{Github:&PullRequestGeneratorGithub{Owner:LedgerHQ,Repo:xxx,API:,TokenRef:nil,AppSecretName:xxxx,Labels:[argocd/preview],},GitLab:nil,Gitea:nil,BitbucketServer:nil,Filters:[]PullRequestGeneratorFilter{},RequeueAfterSeconds:*1800,Template:ApplicationSetTemplate{ApplicationSetTemplateMeta:ApplicationSetTemplateMeta{Name:,Namespace:,Labels:map[string]string{},Annotations:map[string]string{},Finalizers:[],},Spec:ApplicationSpec{Source:nil,Destination:ApplicationDestination{Server:,Namespace:,Name:,},Project:,SyncPolicy:nil,IgnoreDifferences:[]ResourceIgnoreDifferences{},Info:[]Info{},RevisionHistoryLimit:nil,Sources:[]ApplicationSource{},},},Bitbucket:nil,AzureDevOps:nil,} nil nil nil nil}" ``` Signed-off-by: Arthur Outhenin-Chalandre --- .../controllers/applicationset_controller.go | 2 +- .../applicationset_controller_test.go | 52 +++++++++++++++++++ 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/applicationset/controllers/applicationset_controller.go b/applicationset/controllers/applicationset_controller.go index f13e9272bde8f..63f6ea906a1ce 100644 --- a/applicationset/controllers/applicationset_controller.go +++ b/applicationset/controllers/applicationset_controller.go @@ -142,7 +142,7 @@ func (r *ApplicationSetReconciler) Reconcile(ctx context.Context, req ctrl.Reque Status: argov1alpha1.ApplicationSetConditionStatusTrue, }, parametersGenerated, ) - return ctrl.Result{}, err + return ctrl.Result{RequeueAfter: ReconcileRequeueOnValidationError}, err } parametersGenerated = true diff --git a/applicationset/controllers/applicationset_controller_test.go b/applicationset/controllers/applicationset_controller_test.go index 51699a5976a34..7ee2fe52650a8 100644 --- a/applicationset/controllers/applicationset_controller_test.go +++ b/applicationset/controllers/applicationset_controller_test.go @@ -1875,6 +1875,58 @@ func TestGetMinRequeueAfter(t *testing.T) { assert.Equal(t, time.Duration(1)*time.Second, got) } +func TestRequeueGeneratorFails(t *testing.T) { + scheme := runtime.NewScheme() + err := v1alpha1.AddToScheme(scheme) + require.NoError(t, err) + err = v1alpha1.AddToScheme(scheme) + require.NoError(t, err) + + appSet := v1alpha1.ApplicationSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "name", + Namespace: "argocd", + }, + Spec: v1alpha1.ApplicationSetSpec{ + Generators: []v1alpha1.ApplicationSetGenerator{{ + PullRequest: &v1alpha1.PullRequestGenerator{}, + }}, + }, + } + client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(&appSet).Build() + + generator := v1alpha1.ApplicationSetGenerator{ + PullRequest: &v1alpha1.PullRequestGenerator{}, + } + + generatorMock := mocks.Generator{} + generatorMock.On("GetTemplate", &generator). + Return(&v1alpha1.ApplicationSetTemplate{}) + generatorMock.On("GenerateParams", &generator, mock.AnythingOfType("*v1alpha1.ApplicationSet"), mock.Anything). + Return([]map[string]interface{}{}, fmt.Errorf("Simulated error generating params that could be related to an external service/API call")) + + r := ApplicationSetReconciler{ + Client: client, + Scheme: scheme, + Recorder: record.NewFakeRecorder(0), + Cache: &fakeCache{}, + Generators: map[string]generators.Generator{ + "PullRequest": &generatorMock, + }, + } + + req := ctrl.Request{ + NamespacedName: types.NamespacedName{ + Namespace: "argocd", + Name: "name", + }, + } + + res, err := r.Reconcile(context.Background(), req) + require.Error(t, err) + assert.Equal(t, ReconcileRequeueOnValidationError, res.RequeueAfter) +} + func TestValidateGeneratedApplications(t *testing.T) { scheme := runtime.NewScheme() err := v1alpha1.AddToScheme(scheme)