Repository: argoproj/argo-cd (go)

Issue:
Argo CD too frequently fetches Helm repo index that results in a lot of traffic
**Describe the bug**

Argo CD allows creating an application that points directly to the Helm repository. If chart version is not specified explicitly then Argo CD loads whole repository index multiple time per second that results in a lot of traffic and might be costly. 

**To Reproduce**

The issue can be reproduced by just creating an application that points directly to the Helm repository. Every reconciliation argocd loads repo index file.


**Expected behavior**

The index file should be cached or the number of queries should be reduced.

**Version**

```shell
v1.8.6
```


Pull request:
feat: support caching helm repo index
Closes #5659

PR introduces repo server env variable `ARGOCD_HELM_INDEX_CACHE_DURATION` that allows enabling Helm repo index caching.

This change is not the final solution: it only a workaround for #5659 . The root cause is: https://github.com/argoproj/argo-cd/issues/1399 . Argo CD should cache resolved revision per repo and resolve it only once every 3 minutes.


In the past, we had to remove caching to fix the opposite bug: CI publishes new chart version, immediately tries to sync it but don't see a new version due to caching. So for backward compatibility caching is disabled and can be enabled using env variable (e.g. `ARGOCD_HELM_INDEX_CACHE_DURATION=60s`)

Patch:
diff --git a/USERS.md b/USERS.md
index 9c6787780..e316cda6d 100644
--- a/USERS.md
+++ b/USERS.md
@@ -8,6 +8,7 @@ Currently, the following organizations are **officially** using Argo CD:
 1. [3Rein](https://www.3rein.com/)
 1. [7shifts](https://www.7shifts.com/)
 1. [Adevinta](https://www.adevinta.com/)
+1. [Ambassador Labs](https://www.getambassador.io/)
 1. [ANSTO - Australian Synchrotron](https://www.synchrotron.org.au/)
 1. [AppDirect](https://www.appdirect.com)
 1. [Arctiq Inc.](https://www.arctiq.ca)
diff --git a/common/common.go b/common/common.go
index 563cda98f..d411c013c 100644
--- a/common/common.go
+++ b/common/common.go
@@ -188,6 +188,8 @@ const (
 	EnvEnableGRPCTimeHistogramEnv = "ARGOCD_ENABLE_GRPC_TIME_HISTOGRAM"
 	// EnvGithubAppCredsExpirationDuration controls the caching of Github app credentials. This value is in minutes (default: 60)
 	EnvGithubAppCredsExpirationDuration = "ARGOCD_GITHUB_APP_CREDS_EXPIRATION_DURATION"
+	// EnvHelmIndexCacheDuration controls how the helm repository index file is cached for (default: 0)
+	EnvHelmIndexCacheDuration = "ARGOCD_HELM_INDEX_CACHE_DURATION"
 )
 
 const (
diff --git a/reposerver/repository/repository.go b/reposerver/repository/repository.go
index ad3312458..512d30a9d 100644
--- a/reposerver/repository/repository.go
+++ b/reposerver/repository/repository.go
@@ -202,7 +202,7 @@ func (s *Service) runRepoOperation(
 	var err error
 	revision = textutils.FirstNonEmpty(revision, source.TargetRevision)
 	if source.IsHelm() {
-		helmClient, revision, err = s.newHelmClientResolveRevision(repo, revision, source.Chart)
+		helmClient, revision, err = s.newHelmClientResolveRevision(repo, revision, source.Chart, settings.noCache)
 		if err != nil {
 			return nil, err
 		}
@@ -1299,7 +1299,7 @@ func (s *Service) newClientResolveRevision(repo *v1alpha1.Repository, revision s
 	return gitClient, commitSHA, nil
 }
 
-func (s *Service) newHelmClientResolveRevision(repo *v1alpha1.Repository, revision string, chart string) (helm.Client, string, error) {
+func (s *Service) newHelmClientResolveRevision(repo *v1alpha1.Repository, revision string, chart string, noCache bool) (helm.Client, string, error) {
 	helmClient := s.newHelmClient(repo.Repo, repo.GetHelmCreds(), repo.EnableOCI || helm.IsHelmOciChart(chart))
 	if helm.IsVersion(revision) {
 		return helmClient, revision, nil
@@ -1311,7 +1311,7 @@ func (s *Service) newHelmClientResolveRevision(repo *v1alpha1.Repository, revisi
 	if err != nil {
 		return nil, "", fmt.Errorf("invalid revision '%s': %v", revision, err)
 	}
-	index, err := helmClient.GetIndex()
+	index, err := helmClient.GetIndex(noCache)
 	if err != nil {
 		return nil, "", err
 	}
@@ -1360,7 +1360,7 @@ func checkoutRevision(gitClient git.Client, revision string) error {
 }
 
 func (s *Service) GetHelmCharts(ctx context.Context, q *apiclient.HelmChartsRequest) (*apiclient.HelmChartsResponse, error) {
-	index, err := s.newHelmClient(q.Repo.Repo, q.Repo.GetHelmCreds(), q.Repo.EnableOCI).GetIndex()
+	index, err := s.newHelmClient(q.Repo.Repo, q.Repo.GetHelmCreds(), q.Repo.EnableOCI).GetIndex(true)
 	if err != nil {
 		return nil, err
 	}
diff --git a/server/application/application.go b/server/application/application.go
index 83b5b33e8..68e1d606d 100644
--- a/server/application/application.go
+++ b/server/application/application.go
@@ -1404,7 +1404,7 @@ func (s *Server) resolveRevision(ctx context.Context, app *appv1.Application, sy
 			return ambiguousRevision, ambiguousRevision, nil
 		}
 		client := helm.NewClient(repo.Repo, repo.GetHelmCreds(), repo.EnableOCI || app.Spec.Source.IsHelmOci())
-		index, err := client.GetIndex()
+		index, err := client.GetIndex(false)
 		if err != nil {
 			return "", "", err
 		}
diff --git a/util/argo/argo.go b/util/argo/argo.go
index f2aac32ae..bcce4104a 100644
--- a/util/argo/argo.go
+++ b/util/argo/argo.go
@@ -161,7 +161,7 @@ func TestRepo(repo *argoappv1.Repository) error {
 				_, err := helm.NewClient(repo.Repo, repo.GetHelmCreds(), repo.EnableOCI).TestHelmOCI()
 				return err
 			} else {
-				_, err := helm.NewClient(repo.Repo, repo.GetHelmCreds(), repo.EnableOCI).GetIndex()
+				_, err := helm.NewClient(repo.Repo, repo.GetHelmCreds(), repo.EnableOCI).GetIndex(false)
 				return err
 			}
 		},
diff --git a/util/env/env.go b/util/env/env.go
index f26ecb087..a43002f7b 100644
--- a/util/env/env.go
+++ b/util/env/env.go
@@ -3,6 +3,9 @@ package env
 import (
 	"os"
 	"strconv"
+	"time"
+
+	timeutil "github.com/argoproj/pkg/time"
 
 	log "github.com/sirupsen/logrus"
 )
@@ -32,3 +35,29 @@ func ParseNumFromEnv(env string, defaultValue, min, max int) int {
 	}
 	return num
 }
+
+// Helper function to parse a time duration from an environment variable. Returns a
+// default if env is not set, is not parseable to a duration, exceeds max (if
+// max is greater than 0) or is less than min.
+func ParseDurationFromEnv(env string, defaultValue, min, max time.Duration) time.Duration {
+	str := os.Getenv(env)
+	if str == "" {
+		return defaultValue
+	}
+	durPtr, err := timeutil.ParseDuration(str)
+	if err != nil {
+		log.Warnf("Could not parse '%s' as a number from environment %s", str, env)
+		return defaultValue
+	}
+
+	dur := *durPtr
+	if dur < min {
+		log.Warnf("Value in %s is %s, which is less than minimum %s allowed", env, dur, min)
+		return defaultValue
+	}
+	if dur > max {
+		log.Warnf("Value in %s is %s, which is greater than maximum %s allowed", env, dur, max)
+		return defaultValue
+	}
+	return dur
+}
diff --git a/util/helm/client.go b/util/helm/client.go
index 3c7ed2cfc..c12163c0a 100644
--- a/util/helm/client.go
+++ b/util/helm/client.go
@@ -16,17 +16,25 @@ import (
 	"strings"
 	"time"
 
+	"github.com/argoproj/argo-cd/common"
+
+	"github.com/argoproj/argo-cd/util/env"
+
 	"github.com/Masterminds/semver"
 	"github.com/argoproj/pkg/sync"
 	log "github.com/sirupsen/logrus"
 	"gopkg.in/yaml.v2"
 
+	"github.com/argoproj/argo-cd/util/cache"
 	executil "github.com/argoproj/argo-cd/util/exec"
 	"github.com/argoproj/argo-cd/util/io"
 )
 
 var (
-	globalLock = sync.NewKeyLock()
+	globalLock    = sync.NewKeyLock()
+	indexDuration = env.ParseDurationFromEnv(common.EnvHelmIndexCacheDuration, 0, 0, time.Hour*24)
+	indexCache    = cache.NewInMemoryCache(indexDuration)
+	indexLock     = sync.NewKeyLock()
 )
 
 type Creds struct {
@@ -41,7 +49,7 @@ type Creds struct {
 type Client interface {
 	CleanChartCache(chart string, version *semver.Version) error
 	ExtractChart(chart string, version *semver.Version) (string, io.Closer, error)
-	GetIndex() (*Index, error)
+	GetIndex(noCache bool) (*Index, error)
 	TestHelmOCI() (bool, error)
 }
 
@@ -202,22 +210,38 @@ func (c *nativeHelmChart) ExtractChart(chart string, version *semver.Version) (s
 	}), nil
 }
 
-func (c *nativeHelmChart) GetIndex() (*Index, error) {
-	start := time.Now()
+func (c *nativeHelmChart) GetIndex(noCache bool) (*Index, error) {
+	indexLock.Lock(c.repoURL)
+	defer indexLock.Unlock(c.repoURL)
 
-	data, err := c.loadRepoIndex()
-	if err != nil {
-		return nil, err
+	var data []byte
+	useCache := !noCache && indexDuration > 0
+	if useCache {
+		if err := indexCache.Get(c.repoURL, &data); err != nil && err != cache.ErrCacheMiss {
+			log.Warnf("Failed to load index cache for repo: %s: %v", c.repoURL, err)
+		}
+	}
+
+	if len(data) == 0 {
+		start := time.Now()
+		var err error
+		data, err = c.loadRepoIndex()
+		if err != nil {
+			return nil, err
+		}
+		log.WithFields(log.Fields{"seconds": time.Since(start).Seconds()}).Info("took to get index")
+
+		if err := indexCache.Set(&cache.Item{Key: c.repoURL, Object: data}); err != nil {
+			log.Warnf("Failed to store index cache for repo: %s: %v", c.repoURL, err)
+		}
 	}
 
 	index := &Index{}
-	err = yaml.NewDecoder(bytes.NewBuffer(data)).Decode(index)
+	err := yaml.NewDecoder(bytes.NewBuffer(data)).Decode(index)
 	if err != nil {
 		return nil, err
 	}
 
-	log.WithFields(log.Fields{"seconds": time.Since(start).Seconds()}).Info("took to get index")
-
 	return index, nil
 }
 
diff --git a/util/helm/mocks/Client.go b/util/helm/mocks/Client.go
index 2683c400a..db7a6a5b5 100644
--- a/util/helm/mocks/Client.go
+++ b/util/helm/mocks/Client.go
@@ -4,7 +4,6 @@ package mocks
 
 import (
 	helm "github.com/argoproj/argo-cd/util/helm"
-
 	io "github.com/argoproj/argo-cd/util/io"
 
 	mock "github.com/stretchr/testify/mock"
@@ -61,13 +60,13 @@ func (_m *Client) ExtractChart(chart string, version *semver.Version) (string, i
 	return r0, r1, r2
 }
 
-// GetIndex provides a mock function with given fields:
-func (_m *Client) GetIndex() (*helm.Index, error) {
-	ret := _m.Called()
+// GetIndex provides a mock function with given fields: noCache
+func (_m *Client) GetIndex(noCache bool) (*helm.Index, error) {
+	ret := _m.Called(noCache)
 
 	var r0 *helm.Index
-	if rf, ok := ret.Get(0).(func() *helm.Index); ok {
-		r0 = rf()
+	if rf, ok := ret.Get(0).(func(bool) *helm.Index); ok {
+		r0 = rf(noCache)
 	} else {
 		if ret.Get(0) != nil {
 			r0 = ret.Get(0).(*helm.Index)
@@ -75,8 +74,8 @@ func (_m *Client) GetIndex() (*helm.Index, error) {
 	}
 
 	var r1 error
-	if rf, ok := ret.Get(1).(func() error); ok {
-		r1 = rf()
+	if rf, ok := ret.Get(1).(func(bool) error); ok {
+		r1 = rf(noCache)
 	} else {
 		r1 = ret.Error(1)
 	}
@@ -92,9 +91,7 @@ func (_m *Client) TestHelmOCI() (bool, error) {
 	if rf, ok := ret.Get(0).(func() bool); ok {
 		r0 = rf()
 	} else {
-		if ret.Get(0) != nil {
-			r0 = ret.Get(0).(bool)
-		}
+		r0 = ret.Get(0).(bool)
 	}
 
 	var r1 error


Test patch:
diff --git a/reposerver/repository/repository_test.go b/reposerver/repository/repository_test.go
index bd727644f..dcda8c189 100644
--- a/reposerver/repository/repository_test.go
+++ b/reposerver/repository/repository_test.go
@@ -78,7 +78,7 @@ func newServiceWithOpt(cf clientFunc) (*Service, *gitmocks.Client) {
 
 	chart := "my-chart"
 	version := semver.MustParse("1.1.0")
-	helmClient.On("GetIndex").Return(&helm.Index{Entries: map[string]helm.Entries{
+	helmClient.On("GetIndex", true).Return(&helm.Index{Entries: map[string]helm.Entries{
 		chart: {{Version: "1.0.0"}, {Version: version.String()}},
 	}}, nil)
 	helmClient.On("ExtractChart", chart, version).Return("./testdata/my-chart", io.NopCloser, nil)
@@ -1110,12 +1110,12 @@ func TestService_newHelmClientResolveRevision(t *testing.T) {
 	service := newService(".")
 
 	t.Run("EmptyRevision", func(t *testing.T) {
-		_, _, err := service.newHelmClientResolveRevision(&argoappv1.Repository{}, "", "")
+		_, _, err := service.newHelmClientResolveRevision(&argoappv1.Repository{}, "", "", true)
 		assert.EqualError(t, err, "invalid revision '': improper constraint: ")
 	})
 	t.Run("InvalidRevision", func(t *testing.T) {
-		_, _, err := service.newHelmClientResolveRevision(&argoappv1.Repository{}, "???", "")
-		assert.EqualError(t, err, "invalid revision '???': improper constraint: ???")
+		_, _, err := service.newHelmClientResolveRevision(&argoappv1.Repository{}, "???", "", true)
+		assert.EqualError(t, err, "invalid revision '???': improper constraint: ???", true)
 	})
 }
 
diff --git a/util/env/env_test.go b/util/env/env_test.go
index bbd51b59c..53843690b 100644
--- a/util/env/env_test.go
+++ b/util/env/env_test.go
@@ -4,6 +4,7 @@ import (
 	"io"
 	"os"
 	"testing"
+	"time"
 
 	util "github.com/argoproj/argo-cd/util/io"
 
@@ -60,3 +61,45 @@ func TestParseNumFromEnv_OutOfRangeValueSet(t *testing.T) {
 
 	assert.Equal(t, 10, num)
 }
+
+func TestParseDurationFromEnv(t *testing.T) {
+	testKey := "key"
+	defaultVal := 2 * time.Second
+	min := 1 * time.Second
+	max := 3 * time.Second
+
+	testCases := []struct {
+		name     string
+		env      string
+		expected time.Duration
+	}{{
+		name:     "EnvNotSet",
+		expected: defaultVal,
+	}, {
+		name:     "ValidValueSet",
+		env:      "2s",
+		expected: time.Second * 2,
+	}, {
+		name:     "MoreThanMaxSet",
+		env:      "5s",
+		expected: defaultVal,
+	}, {
+		name:     "LessThanMinSet",
+		env:      "-1s",
+		expected: defaultVal,
+	}, {
+		name:     "InvalidSet",
+		env:      "hello",
+		expected: defaultVal,
+	}}
+
+	for i, tc := range testCases {
+		t.Run(testCases[i].name, func(t *testing.T) {
+			tc = testCases[i]
+			setEnv(t, testKey, tc.env)
+
+			val := ParseDurationFromEnv(testKey, defaultVal, min, max)
+			assert.Equal(t, tc.expected, val)
+		})
+	}
+}
diff --git a/util/helm/client_test.go b/util/helm/client_test.go
index 44e87cb1f..9d869667a 100644
--- a/util/helm/client_test.go
+++ b/util/helm/client_test.go
@@ -3,6 +3,7 @@ package helm
 import (
 	"os"
 	"testing"
+	"time"
 
 	"github.com/Masterminds/semver"
 	"github.com/stretchr/testify/assert"
@@ -13,12 +14,12 @@ import (
 func TestIndex(t *testing.T) {
 	t.Run("Invalid", func(t *testing.T) {
 		client := NewClient("", Creds{}, false)
-		_, err := client.GetIndex()
+		_, err := client.GetIndex(false)
 		assert.Error(t, err)
 	})
 	t.Run("Stable", func(t *testing.T) {
 		client := NewClient("https://argoproj.github.io/argo-helm", Creds{}, false)
-		index, err := client.GetIndex()
+		index, err := client.GetIndex(false)
 		assert.NoError(t, err)
 		assert.NotNil(t, index)
 	})
@@ -27,10 +28,24 @@ func TestIndex(t *testing.T) {
 			Username: "my-password",
 			Password: "my-username",
 		}, false)
-		index, err := client.GetIndex()
+		index, err := client.GetIndex(false)
 		assert.NoError(t, err)
 		assert.NotNil(t, index)
 	})
+
+	t.Run("Cached", func(t *testing.T) {
+		var prev time.Duration
+		indexDuration, prev = time.Minute, indexDuration
+		defer func() {
+			indexDuration = prev
+		}()
+
+		client := NewClient("https://argoproj.github.io/argo-helm", Creds{}, false)
+		index, err := client.GetIndex(false)
+		assert.NoError(t, err)
+		assert.NotNil(t, index)
+	})
+
 }
 
 func Test_nativeHelmChart_ExtractChart(t *testing.T) {