diff --git a/cmd/argocd/commands/headless/headless.go b/cmd/argocd/commands/headless/headless.go index 070d9c9c83bcb..8c715b8d24508 100644 --- a/cmd/argocd/commands/headless/headless.go +++ b/cmd/argocd/commands/headless/headless.go @@ -78,9 +78,9 @@ func (c *forwardCacheClient) Set(item *cache.Item) error { }) } -func (c *forwardCacheClient) Get(key string, obj interface{}) error { +func (c *forwardCacheClient) Get(item *cache.Item) error { return c.doLazy(func(client cache.CacheClient) error { - return client.Get(key, obj) + return client.Get(item) }) } diff --git a/reposerver/cache/cache.go b/reposerver/cache/cache.go index 79d3a02b62750..241a5a2d5934c 100644 --- a/reposerver/cache/cache.go +++ b/reposerver/cache/cache.go @@ -12,6 +12,7 @@ import ( "github.com/argoproj/gitops-engine/pkg/utils/text" "github.com/go-git/go-git/v5/plumbing" + "github.com/google/uuid" "github.com/redis/go-redis/v9" log "github.com/sirupsen/logrus" "github.com/spf13/cobra" @@ -24,12 +25,16 @@ import ( "github.com/argoproj/argo-cd/v2/util/hash" ) +//go:generate go run github.com/vektra/mockery/v2@v2.37.0 --name=Cache + var ErrCacheMiss = cacheutil.ErrCacheMiss +var ErrCacheKeyLocked = cacheutil.ErrCacheKeyLocked type Cache struct { - cache *cacheutil.Cache - repoCacheExpiration time.Duration - revisionCacheExpiration time.Duration + cache *cacheutil.Cache + repoCacheExpiration time.Duration + revisionCacheExpiration time.Duration + revisionCacheLockTimeout time.Duration } // ClusterRuntimeInfo holds cluster runtime information @@ -41,7 +46,7 @@ type ClusterRuntimeInfo interface { } func NewCache(cache *cacheutil.Cache, repoCacheExpiration time.Duration, revisionCacheExpiration time.Duration) *Cache { - return &Cache{cache, repoCacheExpiration, revisionCacheExpiration} + return &Cache{cache, repoCacheExpiration, revisionCacheExpiration, 10 * time.Second} } func AddCacheFlagsToCmd(cmd *cobra.Command, opts ...func(client *redis.Client)) func() (*Cache, error) { @@ -54,7 +59,7 @@ func AddCacheFlagsToCmd(cmd *cobra.Command, opts ...func(client *redis.Client)) repoFactory := cacheutil.AddCacheFlagsToCmd(cmd, opts...) return func() (*Cache, error) { - cache, err := repoFactory() + cache, err := repoFactory(true) if err != nil { return nil, fmt.Errorf("error adding cache flags to cmd: %w", err) } @@ -141,12 +146,18 @@ func listApps(repoURL, revision string) string { func (c *Cache) ListApps(repoUrl, revision string) (map[string]string, error) { res := make(map[string]string) - err := c.cache.GetItem(listApps(repoUrl, revision), &res) + err := c.cache.GetItem(listApps(repoUrl, revision), &res, &cacheutil.CacheActionOpts{CacheType: cacheutil.CacheTypeExternal}) return res, err } func (c *Cache) SetApps(repoUrl, revision string, apps map[string]string) error { - return c.cache.SetItem(listApps(repoUrl, revision), apps, c.repoCacheExpiration, apps == nil) + return c.cache.SetItem( + listApps(repoUrl, revision), + apps, + &cacheutil.CacheActionOpts{ + Expiration: c.repoCacheExpiration, + Delete: apps == nil, + CacheType: cacheutil.CacheTypeExternal}) } func helmIndexRefsKey(repo string) string { @@ -155,12 +166,19 @@ func helmIndexRefsKey(repo string) string { // SetHelmIndex stores helm repository index.yaml content to cache func (c *Cache) SetHelmIndex(repo string, indexData []byte) error { - return c.cache.SetItem(helmIndexRefsKey(repo), indexData, c.revisionCacheExpiration, false) + if indexData == nil { + // Logged as warning upstream + return fmt.Errorf("helm index data is nil, skipping cache") + } + return c.cache.SetItem( + helmIndexRefsKey(repo), + indexData, + &cacheutil.CacheActionOpts{Expiration: c.revisionCacheExpiration}) } // GetHelmIndex retrieves helm repository index.yaml content from cache func (c *Cache) GetHelmIndex(repo string, indexData *[]byte) error { - return c.cache.GetItem(helmIndexRefsKey(repo), indexData) + return c.cache.GetItem(helmIndexRefsKey(repo), indexData, nil) } func gitRefsKey(repo string) string { @@ -173,21 +191,108 @@ func (c *Cache) SetGitReferences(repo string, references []*plumbing.Reference) for i := range references { input = append(input, references[i].Strings()) } - return c.cache.SetItem(gitRefsKey(repo), input, c.revisionCacheExpiration, false) + return c.cache.SetItem(gitRefsKey(repo), input, &cacheutil.CacheActionOpts{Expiration: c.revisionCacheExpiration}) +} + +func GitRefCacheItemToReferences(cacheItem [][2]string) *[]*plumbing.Reference { + var res []*plumbing.Reference + for i := range cacheItem { + // Skip empty data + if cacheItem[i][0] != "" || cacheItem[i][1] != "" { + res = append(res, plumbing.NewReferenceFromStrings(cacheItem[i][0], cacheItem[i][1])) + } + } + return &res } -// GetGitReferences retrieves resolved Git repository references from cache -func (c *Cache) GetGitReferences(repo string, references *[]*plumbing.Reference) error { +// TryLockGitRefCache attempts to lock the key for the Git repository references if the key doesn't exist +func (c *Cache) TryLockGitRefCache(repo string, lockId string) error { + // This try set with DisableOverwrite is important for making sure that only one process is able to claim ownership + // A normal get + set, or just set would cause ownership to go to whoever the last writer was, and during race conditions + // leads to duplicate requests + err := c.cache.SetItem(gitRefsKey(repo), [][2]string{{cacheutil.CacheLockedValue, lockId}}, &cacheutil.CacheActionOpts{ + Expiration: c.revisionCacheLockTimeout, + DisableOverwrite: true, + CacheType: cacheutil.CacheTypeExternal}) + return err +} + +func (c *Cache) GetGitReferences(repo string, lockId string, cacheType cacheutil.CacheType) (lockOwner string, references *[]*plumbing.Reference, err error) { var input [][2]string - if err := c.cache.GetItem(gitRefsKey(repo), &input); err != nil { - return err + err = c.cache.GetItem(gitRefsKey(repo), &input, &cacheutil.CacheActionOpts{CacheType: cacheType, Expiration: c.revisionCacheExpiration}) + if err == ErrCacheMiss { + // Expected + return "", nil, nil + } else if err == nil && len(input) > 0 && len(input[0]) > 0 { + if input[0][0] != cacheutil.CacheLockedValue { + // Valid value in cache, convert to plumbing.Reference and return + return "", GitRefCacheItemToReferences(input), nil + } else { + // The key lock is being held + return input[0][1], nil, nil + } } - var res []*plumbing.Reference - for i := range input { - res = append(res, plumbing.NewReferenceFromStrings(input[i][0], input[i][1])) + return "", nil, err +} + +// GetOrLockGitReferences retrieves the git references if they exist, otherwise creates a lock and returns so the caller can populate the cache +func (c *Cache) GetOrLockGitReferences(repo string, references *[]*plumbing.Reference) (updateCache bool, lockId string, err error) { + myLockUUID, err := uuid.NewRandom() + if err != nil { + log.Debug("Error generating git references cache lock id: ", err) + return false, "", err } - *references = res - return nil + // We need to be able to identify that our lock was the successful one, otherwise we'll still have duplicate requests + myLockId := myLockUUID.String() + // Value matches the ttl on the lock in TryLockGitRefCache + waitUntil := time.Now().Add(c.revisionCacheLockTimeout) + // Wait only the maximum amount of time configured for the lock + for time.Now().Before(waitUntil) { + // Attempt to retrieve the key from local cache only + if _, cacheReferences, err := c.GetGitReferences(repo, myLockId, cacheutil.CacheTypeInMemory); err != nil || cacheReferences != nil { + if cacheReferences != nil { + *references = *cacheReferences + } + return false, myLockId, err + } + // Could not get key locally attempt to get the lock + err = c.TryLockGitRefCache(repo, myLockId) + if err != nil { + // Log but ignore this error since we'll want to retry, failing to obtain the lock should not throw an error + log.Errorf("Error attempting to acquire git references cache lock: %v", err) + } + // Attempt to retrieve the key again to see if we have the lock, or the key was populated + if lockOwner, cacheReferences, err := c.GetGitReferences(repo, myLockId, cacheutil.CacheTypeTwoLevel); err != nil || cacheReferences != nil { + if cacheReferences != nil { + // Someone else populated the key + *references = *cacheReferences + } + return false, myLockId, err + } else if lockOwner == myLockId { + // We have the lock, populate the key + return true, myLockId, nil + } + // Wait for lock, valid value, or timeout + time.Sleep(1 * time.Second) + } + // Timeout waiting for lock + log.Debug("Repository cache was unable to acquire lock or valid data within timeout") + return true, myLockId, nil +} + +// UnlockGitReferences unlocks the key for the Git repository references if needed +func (c *Cache) UnlockGitReferences(repo string, lockId string) error { + var input [][2]string + var err error + if err = c.cache.GetItem(gitRefsKey(repo), &input, nil); err == nil && + len(input) > 0 && + len(input[0]) > 1 && + input[0][0] == cacheutil.CacheLockedValue && + input[0][1] == lockId { + // We have the lock, so remove it + return c.cache.SetItem(gitRefsKey(repo), input, &cacheutil.CacheActionOpts{Delete: true, CacheType: cacheutil.CacheTypeExternal}) + } + return err } // refSourceCommitSHAs is a list of resolved revisions for each ref source. This allows us to invalidate the cache @@ -226,7 +331,7 @@ func LogDebugManifestCacheKeyFields(message string, reason string, revision stri } func (c *Cache) GetManifests(revision string, appSrc *appv1.ApplicationSource, srcRefs appv1.RefTargetRevisionMapping, clusterInfo ClusterRuntimeInfo, namespace string, trackingMethod string, appLabelKey string, appName string, res *CachedManifestResponse, refSourceCommitSHAs ResolvedRevisions) error { - err := c.cache.GetItem(manifestCacheKey(revision, appSrc, srcRefs, namespace, trackingMethod, appLabelKey, appName, clusterInfo, refSourceCommitSHAs), res) + err := c.cache.GetItem(manifestCacheKey(revision, appSrc, srcRefs, namespace, trackingMethod, appLabelKey, appName, clusterInfo, refSourceCommitSHAs), res, &cacheutil.CacheActionOpts{CacheType: cacheutil.CacheTypeExternal}) if err != nil { return err @@ -269,11 +374,20 @@ func (c *Cache) SetManifests(revision string, appSrc *appv1.ApplicationSource, s res.CacheEntryHash = hash } - return c.cache.SetItem(manifestCacheKey(revision, appSrc, srcRefs, namespace, trackingMethod, appLabelKey, appName, clusterInfo, refSourceCommitSHAs), res, c.repoCacheExpiration, res == nil) + return c.cache.SetItem( + manifestCacheKey(revision, appSrc, srcRefs, namespace, trackingMethod, appLabelKey, appName, clusterInfo, refSourceCommitSHAs), + res, + &cacheutil.CacheActionOpts{ + Expiration: c.repoCacheExpiration, + Delete: res == nil, + CacheType: cacheutil.CacheTypeExternal}) } func (c *Cache) DeleteManifests(revision string, appSrc *appv1.ApplicationSource, srcRefs appv1.RefTargetRevisionMapping, clusterInfo ClusterRuntimeInfo, namespace, trackingMethod, appLabelKey, appName string, refSourceCommitSHAs ResolvedRevisions) error { - return c.cache.SetItem(manifestCacheKey(revision, appSrc, srcRefs, namespace, trackingMethod, appLabelKey, appName, clusterInfo, refSourceCommitSHAs), "", c.repoCacheExpiration, true) + return c.cache.SetItem( + manifestCacheKey(revision, appSrc, srcRefs, namespace, trackingMethod, appLabelKey, appName, clusterInfo, refSourceCommitSHAs), + "", + &cacheutil.CacheActionOpts{Delete: true, CacheType: cacheutil.CacheTypeExternal}) } func appDetailsCacheKey(revision string, appSrc *appv1.ApplicationSource, srcRefs appv1.RefTargetRevisionMapping, trackingMethod appv1.TrackingMethod, refSourceCommitSHAs ResolvedRevisions) string { @@ -284,11 +398,17 @@ func appDetailsCacheKey(revision string, appSrc *appv1.ApplicationSource, srcRef } func (c *Cache) GetAppDetails(revision string, appSrc *appv1.ApplicationSource, srcRefs appv1.RefTargetRevisionMapping, res *apiclient.RepoAppDetailsResponse, trackingMethod appv1.TrackingMethod, refSourceCommitSHAs ResolvedRevisions) error { - return c.cache.GetItem(appDetailsCacheKey(revision, appSrc, srcRefs, trackingMethod, refSourceCommitSHAs), res) + return c.cache.GetItem(appDetailsCacheKey(revision, appSrc, srcRefs, trackingMethod, refSourceCommitSHAs), res, &cacheutil.CacheActionOpts{CacheType: cacheutil.CacheTypeExternal}) } func (c *Cache) SetAppDetails(revision string, appSrc *appv1.ApplicationSource, srcRefs appv1.RefTargetRevisionMapping, res *apiclient.RepoAppDetailsResponse, trackingMethod appv1.TrackingMethod, refSourceCommitSHAs ResolvedRevisions) error { - return c.cache.SetItem(appDetailsCacheKey(revision, appSrc, srcRefs, trackingMethod, refSourceCommitSHAs), res, c.repoCacheExpiration, res == nil) + return c.cache.SetItem( + appDetailsCacheKey(revision, appSrc, srcRefs, trackingMethod, refSourceCommitSHAs), + res, + &cacheutil.CacheActionOpts{ + Expiration: c.repoCacheExpiration, + Delete: res == nil, + CacheType: cacheutil.CacheTypeExternal}) } func revisionMetadataKey(repoURL, revision string) string { @@ -297,11 +417,14 @@ func revisionMetadataKey(repoURL, revision string) string { func (c *Cache) GetRevisionMetadata(repoURL, revision string) (*appv1.RevisionMetadata, error) { item := &appv1.RevisionMetadata{} - return item, c.cache.GetItem(revisionMetadataKey(repoURL, revision), item) + return item, c.cache.GetItem(revisionMetadataKey(repoURL, revision), item, nil) } func (c *Cache) SetRevisionMetadata(repoURL, revision string, item *appv1.RevisionMetadata) error { - return c.cache.SetItem(revisionMetadataKey(repoURL, revision), item, c.repoCacheExpiration, false) + return c.cache.SetItem( + revisionMetadataKey(repoURL, revision), + item, + &cacheutil.CacheActionOpts{Expiration: c.repoCacheExpiration}) } func revisionChartDetailsKey(repoURL, chart, revision string) string { @@ -310,11 +433,14 @@ func revisionChartDetailsKey(repoURL, chart, revision string) string { func (c *Cache) GetRevisionChartDetails(repoURL, chart, revision string) (*appv1.ChartDetails, error) { item := &appv1.ChartDetails{} - return item, c.cache.GetItem(revisionChartDetailsKey(repoURL, chart, revision), item) + return item, c.cache.GetItem(revisionChartDetailsKey(repoURL, chart, revision), item, nil) } func (c *Cache) SetRevisionChartDetails(repoURL, chart, revision string, item *appv1.ChartDetails) error { - return c.cache.SetItem(revisionChartDetailsKey(repoURL, chart, revision), item, c.repoCacheExpiration, false) + return c.cache.SetItem( + revisionChartDetailsKey(repoURL, chart, revision), + item, + &cacheutil.CacheActionOpts{Expiration: c.repoCacheExpiration}) } func gitFilesKey(repoURL, revision, pattern string) string { @@ -322,12 +448,15 @@ func gitFilesKey(repoURL, revision, pattern string) string { } func (c *Cache) SetGitFiles(repoURL, revision, pattern string, files map[string][]byte) error { - return c.cache.SetItem(gitFilesKey(repoURL, revision, pattern), &files, c.repoCacheExpiration, false) + return c.cache.SetItem( + gitFilesKey(repoURL, revision, pattern), + &files, + &cacheutil.CacheActionOpts{Expiration: c.repoCacheExpiration, CacheType: cacheutil.CacheTypeExternal}) } func (c *Cache) GetGitFiles(repoURL, revision, pattern string) (map[string][]byte, error) { var item map[string][]byte - return item, c.cache.GetItem(gitFilesKey(repoURL, revision, pattern), &item) + return item, c.cache.GetItem(gitFilesKey(repoURL, revision, pattern), &item, &cacheutil.CacheActionOpts{CacheType: cacheutil.CacheTypeExternal}) } func gitDirectoriesKey(repoURL, revision string) string { @@ -335,12 +464,15 @@ func gitDirectoriesKey(repoURL, revision string) string { } func (c *Cache) SetGitDirectories(repoURL, revision string, directories []string) error { - return c.cache.SetItem(gitDirectoriesKey(repoURL, revision), &directories, c.repoCacheExpiration, false) + return c.cache.SetItem( + gitDirectoriesKey(repoURL, revision), + &directories, + &cacheutil.CacheActionOpts{Expiration: c.repoCacheExpiration}) } func (c *Cache) GetGitDirectories(repoURL, revision string) ([]string, error) { var item []string - return item, c.cache.GetItem(gitDirectoriesKey(repoURL, revision), &item) + return item, c.cache.GetItem(gitDirectoriesKey(repoURL, revision), &item, nil) } func (cmr *CachedManifestResponse) shallowCopy() *CachedManifestResponse { diff --git a/reposerver/cache/cache_test.go b/reposerver/cache/cache_test.go index 190ddfc78fe09..c4101e8f68445 100644 --- a/reposerver/cache/cache_test.go +++ b/reposerver/cache/cache_test.go @@ -3,35 +3,48 @@ package cache import ( "encoding/json" "errors" + "fmt" "strings" "testing" "time" - "github.com/spf13/cobra" - "github.com/stretchr/testify/assert" - . "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" + appv1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" "github.com/argoproj/argo-cd/v2/reposerver/apiclient" + "github.com/argoproj/argo-cd/v2/reposerver/cache/mocks" cacheutil "github.com/argoproj/argo-cd/v2/util/cache" + "github.com/go-git/go-git/v5/plumbing" + "github.com/spf13/cobra" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" ) -type fixtures struct { +type MockedCache struct { + mock.Mock *Cache } +type fixtures struct { + mockCache *mocks.MockRepoCache + cache *MockedCache +} + func newFixtures() *fixtures { - return &fixtures{NewCache( - cacheutil.NewCache(cacheutil.NewInMemoryCache(1*time.Hour)), - 1*time.Minute, - 1*time.Minute, - )} + mockCache := mocks.NewMockRepoCache(&mocks.MockCacheOptions{RevisionCacheExpiration: 1 * time.Minute, RepoCacheExpiration: 1 * time.Minute}) + newBaseCache := cacheutil.NewCache(mockCache.TwoLevelClient) + baseCache := NewCache(newBaseCache, 1*time.Minute, 1*time.Minute) + return &fixtures{mockCache: mockCache, cache: &MockedCache{Cache: baseCache}} } func TestCache_GetRevisionMetadata(t *testing.T) { - cache := newFixtures().Cache + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + cache := fixtures.cache + mockCache := fixtures.mockCache // cache miss _, err := cache.GetRevisionMetadata("my-repo-url", "my-revision") assert.Equal(t, ErrCacheMiss, err) + mockCache.TwoLevelClient.AssertCalled(t, "Get", mock.Anything) // populate cache err = cache.SetRevisionMetadata("my-repo-url", "my-revision", &RevisionMetadata{Message: "my-message"}) assert.NoError(t, err) @@ -45,10 +58,14 @@ func TestCache_GetRevisionMetadata(t *testing.T) { value, err := cache.GetRevisionMetadata("my-repo-url", "my-revision") assert.NoError(t, err) assert.Equal(t, &RevisionMetadata{Message: "my-message"}, value) + mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalSets: 1, ExternalGets: 3, InMemoryGets: 1}) } func TestCache_ListApps(t *testing.T) { - cache := newFixtures().Cache + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + cache := fixtures.cache + mockCache := fixtures.mockCache // cache miss _, err := cache.ListApps("my-repo-url", "my-revision") assert.Equal(t, ErrCacheMiss, err) @@ -65,10 +82,14 @@ func TestCache_ListApps(t *testing.T) { value, err := cache.ListApps("my-repo-url", "my-revision") assert.NoError(t, err) assert.Equal(t, map[string]string{"foo": "bar"}, value) + mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalSets: 1, ExternalGets: 4}) } func TestCache_GetManifests(t *testing.T) { - cache := newFixtures().Cache + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + cache := fixtures.cache + mockCache := fixtures.mockCache // cache miss q := &apiclient.ManifestRequest{} value := &CachedManifestResponse{} @@ -107,10 +128,14 @@ func TestCache_GetManifests(t *testing.T) { assert.NoError(t, err) assert.Equal(t, &CachedManifestResponse{ManifestResponse: &apiclient.ManifestResponse{SourceType: "my-source-type"}}, value) }) + mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalSets: 1, ExternalGets: 8}) } func TestCache_GetAppDetails(t *testing.T) { - cache := newFixtures().Cache + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + cache := fixtures.cache + mockCache := fixtures.mockCache // cache miss value := &apiclient.RepoAppDetailsResponse{} emptyRefSources := map[string]*RefTarget{} @@ -129,6 +154,7 @@ func TestCache_GetAppDetails(t *testing.T) { err = cache.GetAppDetails("my-revision", &ApplicationSource{}, emptyRefSources, value, "", nil) assert.NoError(t, err) assert.Equal(t, &apiclient.RepoAppDetailsResponse{Type: "my-type"}, value) + mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalSets: 1, ExternalGets: 4}) } func TestAddCacheFlagsToCmd(t *testing.T) { @@ -309,3 +335,428 @@ func TestCachedManifestResponse_ShallowCopyExpectedFields(t *testing.T) { } } + +func TestGetGitReferences(t *testing.T) { + t.Run("Valid args, nothing in cache, in-memory only", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + cache := fixtures.cache + lockOwner, references, err := cache.GetGitReferences("test-repo", "test-lock-id", cacheutil.CacheTypeInMemory) + assert.NoError(t, err, "Error is cache miss handled inside function") + assert.Equal(t, "", lockOwner, "Lock owner should be empty") + assert.Nil(t, references) + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{InMemoryGets: 1}) + }) + + t.Run("Valid args, nothing in cache, external only", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + cache := fixtures.cache + lockOwner, references, err := cache.GetGitReferences("test-repo", "test-lock-id", cacheutil.CacheTypeExternal) + assert.NoError(t, err, "Error is cache miss handled inside function") + assert.Equal(t, "", lockOwner, "Lock owner should be empty") + assert.Nil(t, references) + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalGets: 1}) + }) + + t.Run("Valid args, value in cache, in-memory only", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + cache := fixtures.cache + err := cache.SetGitReferences("test-repo", *GitRefCacheItemToReferences([][2]string{{"test-repo", "ref: test"}})) + assert.NoError(t, err) + lockOwner, references, err := cache.GetGitReferences("test-repo", "test-lock-id", cacheutil.CacheTypeInMemory) + assert.NoError(t, err) + assert.Equal(t, "", lockOwner, "Lock owner should be empty") + assert.Equal(t, 1, len(*references)) + assert.Equal(t, "test", (*references)[0].Target().String()) + assert.Equal(t, "test-repo", (*references)[0].Name().String()) + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalSets: 1, InMemoryGets: 1}) + }) + + t.Run("cache error", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + cache := fixtures.cache + fixtures.mockCache.TwoLevelClient.On("Get", mock.Anything).Unset() + fixtures.mockCache.TwoLevelClient.On("Get", mock.Anything).Return(errors.New("test cache error")) + lockOwner, references, err := cache.GetGitReferences("test-repo", "test-lock-id", cacheutil.CacheTypeExternal) + assert.ErrorContains(t, err, "test cache error", "Error should be propagated") + assert.Equal(t, "", lockOwner, "Lock owner should be empty") + assert.Nil(t, references) + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{InMemoryGets: 1}) + }) + +} + +func TestGitRefCacheItemToReferences_DataChecks(t *testing.T) { + references := *GitRefCacheItemToReferences(nil) + assert.Equal(t, 0, len(references), "No data should be handled gracefully by returning an empty slice") + references = *GitRefCacheItemToReferences([][2]string{{"", ""}}) + assert.Equal(t, 0, len(references), "Empty data should be discarded") + references = *GitRefCacheItemToReferences([][2]string{{"test", ""}}) + assert.Equal(t, 1, len(references), "Just the key being set should not be discarded") + assert.Equal(t, "test", references[0].Name().String(), "Name should be set and equal test") + references = *GitRefCacheItemToReferences([][2]string{{"", "ref: test1"}}) + assert.Equal(t, 1, len(references), "Just the value being set should not be discarded") + assert.Equal(t, "test1", references[0].Target().String(), "Target should be set and equal test1") + references = *GitRefCacheItemToReferences([][2]string{{"test2", "ref: test2"}}) + assert.Equal(t, 1, len(references), "Valid data is should be preserved") + assert.Equal(t, "test2", references[0].Name().String(), "Name should be set and equal test2") + assert.Equal(t, "test2", references[0].Target().String(), "Target should be set and equal test2") + references = *GitRefCacheItemToReferences([][2]string{{"test3", "ref: test3"}, {"test4", "ref: test4"}}) + assert.Equal(t, 2, len(references), "Valid data is should be preserved") + assert.Equal(t, "test3", references[0].Name().String(), "Name should be set and equal test3") + assert.Equal(t, "test3", references[0].Target().String(), "Target should be set and equal test3") + assert.Equal(t, "test4", references[1].Name().String(), "Name should be set and equal test4") + assert.Equal(t, "test4", references[1].Target().String(), "Target should be set and equal test4") +} + +func TestTryLockGitRefCache_OwnershipFlows(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + cache := fixtures.cache + utilCache := cache.cache + // Test setting the lock + err := cache.TryLockGitRefCache("my-repo-url", "my-lock-id") + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalSets: 1}) + assert.NoError(t, err) + var output [][2]string + key := fmt.Sprintf("git-refs|%s", "my-repo-url") + err = utilCache.GetItem(key, &output, nil) + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalSets: 1, ExternalGets: 1}) + assert.NoError(t, err) + assert.Equal(t, "locked", output[0][0], "The lock should be set") + assert.Equal(t, "my-lock-id", output[0][1], "The lock should be set to the provided lock id") + // Test not being able to overwrite the lock + err = cache.TryLockGitRefCache("my-repo-url", "other-lock-id") + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalSets: 2, ExternalGets: 1}) + assert.NoError(t, err) + err = utilCache.GetItem(key, &output, nil) + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalSets: 2, ExternalGets: 1, InMemoryGets: 1}) + assert.NoError(t, err) + assert.Equal(t, "locked", output[0][0], "The lock should not have changed") + assert.Equal(t, "my-lock-id", output[0][1], "The lock should not have changed") + // Test can overwrite once there is nothing set + err = utilCache.SetItem(key, [][2]string{}, &cacheutil.CacheActionOpts{Expiration: 0, Delete: true}) + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalSets: 2, ExternalGets: 1, ExternalDeletes: 1, InMemoryGets: 1}) + assert.NoError(t, err) + err = cache.TryLockGitRefCache("my-repo-url", "other-lock-id") + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalSets: 3, ExternalGets: 1, ExternalDeletes: 1, InMemoryGets: 1}) + assert.NoError(t, err) + err = utilCache.GetItem(key, &output, nil) + assert.NoError(t, err) + assert.Equal(t, "locked", output[0][0], "The lock should be set") + assert.Equal(t, "other-lock-id", output[0][1], "The lock id should have changed to other-lock-id") + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalSets: 3, ExternalGets: 2, ExternalDeletes: 1, InMemoryGets: 1}) +} + +func TestGetOrLockGitReferences(t *testing.T) { + t.Run("Test cache lock get lock", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + cache := fixtures.cache + var references []*plumbing.Reference + updateCache, lockId, err := cache.GetOrLockGitReferences("test-repo", &references) + assert.NoError(t, err) + assert.True(t, updateCache) + assert.NotEqual(t, "", lockId, "Lock id should be set") + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalSets: 1, ExternalGets: 1, InMemoryGets: 1}) + }) + + t.Run("Test cache lock, cache hit local", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + cache := fixtures.cache + err := cache.SetGitReferences("test-repo", *GitRefCacheItemToReferences([][2]string{{"test-repo", "ref: test"}})) + assert.NoError(t, err) + var references []*plumbing.Reference + updateCache, lockId, err := cache.GetOrLockGitReferences("test-repo", &references) + assert.NoError(t, err) + assert.False(t, updateCache) + assert.NotEqual(t, "", lockId, "Lock id should be set") + assert.Equal(t, "test-repo", references[0].Name().String()) + assert.Equal(t, "test", references[0].Target().String()) + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalSets: 1, InMemoryGets: 1}) + }) + + t.Run("Test cache lock, cache hit remote", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + cache := fixtures.cache + err := fixtures.cache.cache.SetItem( + "git-refs|test-repo", + [][2]string{{"test-repo", "ref: test"}}, + &cacheutil.CacheActionOpts{ + Expiration: 30 * time.Second}) + assert.NoError(t, err) + var references []*plumbing.Reference + updateCache, lockId, err := cache.GetOrLockGitReferences("test-repo", &references) + assert.NoError(t, err) + assert.False(t, updateCache) + assert.NotEqual(t, "", lockId, "Lock id should be set") + assert.Equal(t, "test-repo", references[0].Name().String()) + assert.Equal(t, "test", references[0].Target().String()) + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalSets: 1, InMemoryGets: 1}) + }) + + t.Run("Test miss, populated by external", func(t *testing.T) { + // Tests the case where another process populates the external cache when trying + // to obtain the lock + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + cache := fixtures.cache + fixtures.mockCache.TwoLevelClient.On("Get", mock.Anything).Unset() + fixtures.mockCache.TwoLevelClient.On("Get", mock.Anything).Return(cacheutil.ErrCacheMiss).Once().Run(func(args mock.Arguments) { + err := cache.SetGitReferences("test-repo", *GitRefCacheItemToReferences([][2]string{{"test-repo", "ref: test"}})) + assert.NoError(t, err) + }).On("Get", mock.Anything).Return(nil) + var references []*plumbing.Reference + updateCache, lockId, err := cache.GetOrLockGitReferences("test-repo", &references) + assert.NoError(t, err) + assert.False(t, updateCache) + assert.NotEqual(t, "", lockId, "Lock id should be set") + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalSets: 2, InMemoryGets: 2}) + }) + + t.Run("Test cache lock timeout", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + cache := fixtures.cache + // Create conditions for cache hit, which would result in false on updateCache if we weren't reaching the timeout + err := cache.SetGitReferences("test-repo", *GitRefCacheItemToReferences([][2]string{{"test-repo", "ref: test"}})) + assert.NoError(t, err) + cache.revisionCacheLockTimeout = -1 * time.Second + var references []*plumbing.Reference + updateCache, lockId, err := cache.GetOrLockGitReferences("test-repo", &references) + assert.NoError(t, err) + assert.True(t, updateCache) + assert.NotEqual(t, "", lockId, "Lock id should be set") + cache.revisionCacheLockTimeout = 10 * time.Second + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalSets: 1}) + }) + + t.Run("Test cache lock error", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + cache := fixtures.cache + fixtures.cache.revisionCacheLockTimeout = 10 * time.Second + fixtures.mockCache.TwoLevelClient.On("Set", mock.Anything).Unset() + fixtures.mockCache.TwoLevelClient.On("Set", mock.Anything).Return(errors.New("test cache error")).Once(). + On("Set", mock.Anything).Return(nil) + var references []*plumbing.Reference + updateCache, lockId, err := cache.GetOrLockGitReferences("test-repo", &references) + assert.NoError(t, err) + assert.True(t, updateCache) + assert.NotEqual(t, "", lockId, "Lock id should be set") + fixtures.mockCache.TwoLevelClient.AssertNumberOfCalls(t, "Set", 2) + fixtures.mockCache.RedisClient.AssertNumberOfCalls(t, "Set", 1) + fixtures.mockCache.TwoLevelClient.AssertNumberOfCalls(t, "Get", 4) + fixtures.mockCache.RedisClient.AssertNumberOfCalls(t, "Get", 2) + }) +} + +func TestUnlockGitReferences(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + cache := fixtures.cache + + t.Run("Test not locked", func(t *testing.T) { + err := cache.UnlockGitReferences("test-repo", "") + assert.Error(t, err) + assert.Contains(t, err.Error(), "key is missing") + }) + + t.Run("Test unlock", func(t *testing.T) { + // Get lock + var references []*plumbing.Reference + updateCache, lockId, err := cache.GetOrLockGitReferences("test-repo", &references) + assert.NoError(t, err) + assert.True(t, updateCache) + assert.NotEqual(t, "", lockId, "Lock id should be set") + // Release lock + err = cache.UnlockGitReferences("test-repo", lockId) + assert.NoError(t, err) + }) +} + +func TestSetHelmIndex(t *testing.T) { + t.Run("SetHelmIndex with valid data", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + err := fixtures.cache.SetHelmIndex("test-repo", []byte("test-data")) + assert.NoError(t, err) + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalSets: 1}) + }) + t.Run("SetHelmIndex with nil", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + err := fixtures.cache.SetHelmIndex("test-repo", nil) + assert.Error(t, err, "nil data should not be cached") + var indexData []byte + err = fixtures.cache.GetHelmIndex("test-repo", &indexData) + assert.Error(t, err) + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalGets: 1}) + }) +} + +func TestRevisionChartDetails(t *testing.T) { + t.Run("GetRevisionChartDetails cache miss", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + details, err := fixtures.cache.GetRevisionChartDetails("test-repo", "test-revision", "v1.0.0") + assert.ErrorAs(t, err, &ErrCacheMiss) + assert.Equal(t, &appv1.ChartDetails{}, details) + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalGets: 1}) + }) + t.Run("GetRevisionChartDetails cache miss local", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + cache := fixtures.cache + expectedItem := &appv1.ChartDetails{ + Description: "test-chart", + Home: "v1.0.0", + Maintainers: []string{"test-maintainer"}, + } + err := cache.cache.SetItem( + revisionChartDetailsKey("test-repo", "test-revision", "v1.0.0"), + expectedItem, + &cacheutil.CacheActionOpts{Expiration: 30 * time.Second, CacheType: cacheutil.CacheTypeExternal}) + assert.NoError(t, err) + details, err := fixtures.cache.GetRevisionChartDetails("test-repo", "test-revision", "v1.0.0") + assert.NoError(t, err) + assert.Equal(t, expectedItem, details) + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalGets: 1, ExternalSets: 1}) + }) + + t.Run("GetRevisionChartDetails cache hit local", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + cache := fixtures.cache + expectedItem := &appv1.ChartDetails{ + Description: "test-chart", + Home: "v1.0.0", + Maintainers: []string{"test-maintainer"}, + } + err := cache.cache.SetItem( + revisionChartDetailsKey("test-repo", "test-revision", "v1.0.0"), + expectedItem, + &cacheutil.CacheActionOpts{Expiration: 30 * time.Second, CacheType: cacheutil.CacheTypeInMemory}) + assert.NoError(t, err) + details, err := fixtures.cache.GetRevisionChartDetails("test-repo", "test-revision", "v1.0.0") + assert.NoError(t, err) + assert.Equal(t, expectedItem, details) + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{InMemoryGets: 1, InMemorySets: 1}) + }) + + t.Run("SetRevisionChartDetails", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + expectedItem := &appv1.ChartDetails{ + Description: "test-chart", + Home: "v1.0.0", + Maintainers: []string{"test-maintainer"}, + } + err := fixtures.cache.SetRevisionChartDetails("test-repo", "test-revision", "v1.0.0", expectedItem) + assert.NoError(t, err) + details, err := fixtures.cache.GetRevisionChartDetails("test-repo", "test-revision", "v1.0.0") + assert.NoError(t, err) + assert.Equal(t, expectedItem, details) + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{InMemoryGets: 1, ExternalSets: 1}) + }) + +} + +func TestGetGitDirectories(t *testing.T) { + t.Run("GetGitDirectories cache miss", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + directories, err := fixtures.cache.GetGitDirectories("test-repo", "test-revision") + assert.ErrorAs(t, err, &ErrCacheMiss) + assert.Equal(t, 0, len(directories)) + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalGets: 1}) + }) + t.Run("GetGitDirectories cache miss local", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + cache := fixtures.cache + expectedItem := []string{"test/dir", "test/dir2"} + err := cache.cache.SetItem( + gitDirectoriesKey("test-repo", "test-revision"), + expectedItem, + &cacheutil.CacheActionOpts{Expiration: 30 * time.Second, CacheType: cacheutil.CacheTypeExternal}) + assert.NoError(t, err) + directories, err := fixtures.cache.GetGitDirectories("test-repo", "test-revision") + assert.NoError(t, err) + assert.Equal(t, expectedItem, directories) + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalGets: 1, ExternalSets: 1}) + }) + + t.Run("GetGitDirectories cache hit local", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + cache := fixtures.cache + expectedItem := []string{"test/dir", "test/dir2"} + err := cache.cache.SetItem( + gitDirectoriesKey("test-repo", "test-revision"), + expectedItem, + &cacheutil.CacheActionOpts{Expiration: 30 * time.Second, CacheType: cacheutil.CacheTypeInMemory}) + assert.NoError(t, err) + directories, err := fixtures.cache.GetGitDirectories("test-repo", "test-revision") + assert.NoError(t, err) + assert.Equal(t, expectedItem, directories) + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{InMemoryGets: 1, InMemorySets: 1}) + }) + + t.Run("SetGitDirectories", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + expectedItem := []string{"test/dir", "test/dir2"} + err := fixtures.cache.SetGitDirectories("test-repo", "test-revision", expectedItem) + assert.NoError(t, err) + directories, err := fixtures.cache.GetGitDirectories("test-repo", "test-revision") + assert.NoError(t, err) + assert.Equal(t, expectedItem, directories) + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{InMemoryGets: 1, ExternalSets: 1}) + }) + +} + +func TestGetGitFiles(t *testing.T) { + t.Run("GetGitFiles cache miss", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + directories, err := fixtures.cache.GetGitFiles("test-repo", "test-revision", "*.json") + assert.ErrorAs(t, err, &ErrCacheMiss) + assert.Equal(t, 0, len(directories)) + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalGets: 1}) + }) + t.Run("GetGitFiles cache hit", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + cache := fixtures.cache + expectedItem := map[string][]byte{"test/file.json": []byte("\"test\":\"contents\""), "test/file1.json": []byte("\"test1\":\"contents1\"")} + err := cache.cache.SetItem( + gitFilesKey("test-repo", "test-revision", "*.json"), + expectedItem, + &cacheutil.CacheActionOpts{Expiration: 30 * time.Second, CacheType: cacheutil.CacheTypeExternal}) + assert.NoError(t, err) + files, err := fixtures.cache.GetGitFiles("test-repo", "test-revision", "*.json") + assert.NoError(t, err) + assert.Equal(t, expectedItem, files) + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalGets: 1, ExternalSets: 1}) + }) + + t.Run("SetGitFiles", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + expectedItem := map[string][]byte{"test/file.json": []byte("\"test\":\"contents\""), "test/file1.json": []byte("\"test1\":\"contents1\"")} + err := fixtures.cache.SetGitFiles("test-repo", "test-revision", "*.json", expectedItem) + assert.NoError(t, err) + files, err := fixtures.cache.GetGitFiles("test-repo", "test-revision", "*.json") + assert.NoError(t, err) + assert.Equal(t, expectedItem, files) + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalGets: 1, ExternalSets: 1}) + }) + +} diff --git a/reposerver/cache/mocks/reposervercache.go b/reposerver/cache/mocks/reposervercache.go new file mode 100644 index 0000000000000..4cd237dfa91e7 --- /dev/null +++ b/reposerver/cache/mocks/reposervercache.go @@ -0,0 +1,88 @@ +package mocks + +import ( + "testing" + "time" + + "github.com/alicebob/miniredis/v2" + cacheutil "github.com/argoproj/argo-cd/v2/util/cache" + cacheutilmocks "github.com/argoproj/argo-cd/v2/util/cache/mocks" + "github.com/redis/go-redis/v9" + "github.com/stretchr/testify/mock" +) + +type MockCacheType int + +const ( + MockCacheTypeRedis MockCacheType = iota + MockCacheTypeInMem +) + +type MockRepoCache struct { + mock.Mock + RedisClient *cacheutilmocks.MockCacheClient + TwoLevelClient *cacheutilmocks.MockCacheClient + StopRedisCallback func() +} + +type MockCacheOptions struct { + RepoCacheExpiration time.Duration + RevisionCacheExpiration time.Duration + ReadDelay time.Duration + WriteDelay time.Duration +} + +type CacheCallCounts struct { + ExternalSets int + ExternalGets int + ExternalDeletes int + InMemorySets int + InMemoryGets int + InMemoryDeletes int +} + +// Checks that the cache was called the expected number of times +func (mockCache *MockRepoCache) AssertCacheCalledTimes(t *testing.T, calls *CacheCallCounts) { + totalSets := calls.ExternalSets + calls.InMemorySets + totalGets := calls.ExternalGets + calls.InMemoryGets + totalDeletes := calls.ExternalDeletes + calls.InMemoryDeletes + mockCache.TwoLevelClient.AssertNumberOfCalls(t, "Get", totalGets) + mockCache.TwoLevelClient.AssertNumberOfCalls(t, "Set", totalSets) + mockCache.TwoLevelClient.AssertNumberOfCalls(t, "Delete", totalDeletes) + mockCache.RedisClient.AssertNumberOfCalls(t, "Get", calls.ExternalGets) + mockCache.RedisClient.AssertNumberOfCalls(t, "Set", calls.ExternalSets) + mockCache.RedisClient.AssertNumberOfCalls(t, "Delete", calls.ExternalDeletes) +} + +func (mockCache *MockRepoCache) ConfigureDefaultCallbacks() { + mockCache.TwoLevelClient.On("Get", mock.Anything).Return(nil) + mockCache.TwoLevelClient.On("Set", mock.Anything).Return(nil) + mockCache.TwoLevelClient.On("Delete", mock.Anything).Return(nil) + mockCache.RedisClient.On("Get", mock.Anything).Return(nil) + mockCache.RedisClient.On("Set", mock.Anything).Return(nil) + mockCache.RedisClient.On("Delete", mock.Anything).Return(nil) +} + +func NewInMemoryRedis() (*redis.Client, func()) { + cacheutil.NewInMemoryCache(5 * time.Second) + mr, err := miniredis.Run() + if err != nil { + panic(err) + } + return redis.NewClient(&redis.Options{Addr: mr.Addr()}), mr.Close +} + +func NewMockRepoCache(cacheOpts *MockCacheOptions) *MockRepoCache { + redisClient, stopRedis := NewInMemoryRedis() + redisCacheClient := &cacheutilmocks.MockCacheClient{ + ReadDelay: cacheOpts.ReadDelay, + WriteDelay: cacheOpts.WriteDelay, + BaseCache: cacheutil.NewRedisCache(redisClient, cacheOpts.RepoCacheExpiration, cacheutil.RedisCompressionNone)} + twoLevelClient := &cacheutilmocks.MockCacheClient{ + ReadDelay: cacheOpts.ReadDelay, + WriteDelay: cacheOpts.WriteDelay, + BaseCache: cacheutil.NewTwoLevelClient(redisCacheClient, cacheOpts.RepoCacheExpiration)} + newMockCache := &MockRepoCache{TwoLevelClient: twoLevelClient, RedisClient: redisCacheClient, StopRedisCallback: stopRedis} + newMockCache.ConfigureDefaultCallbacks() + return newMockCache +} diff --git a/reposerver/repository/repository.go b/reposerver/repository/repository.go index f66bc71ac4621..df9b1a36f4805 100644 --- a/reposerver/repository/repository.go +++ b/reposerver/repository/repository.go @@ -300,6 +300,7 @@ func (s *Service) runRepoOperation( var gitClient git.Client var helmClient helm.Client var err error + gitClientOpts := git.WithCache(s.cache, !settings.noRevisionCache && !settings.noCache) revision = textutils.FirstNonEmpty(revision, source.TargetRevision) unresolvedRevision := revision if source.IsHelm() { @@ -308,13 +309,13 @@ func (s *Service) runRepoOperation( return err } } else { - gitClient, revision, err = s.newClientResolveRevision(repo, revision, git.WithCache(s.cache, !settings.noRevisionCache && !settings.noCache)) + gitClient, revision, err = s.newClientResolveRevision(repo, revision, gitClientOpts) if err != nil { return err } } - repoRefs, err := resolveReferencedSources(hasMultipleSources, source.Helm, refSources, s.newClientResolveRevision) + repoRefs, err := resolveReferencedSources(hasMultipleSources, source.Helm, refSources, s.newClientResolveRevision, gitClientOpts) if err != nil { return err } @@ -463,7 +464,7 @@ type gitClientGetter func(repo *v1alpha1.Repository, revision string, opts ...gi // // Much of this logic is duplicated in runManifestGenAsync. If making changes here, check whether runManifestGenAsync // should be updated. -func resolveReferencedSources(hasMultipleSources bool, source *v1alpha1.ApplicationSourceHelm, refSources map[string]*v1alpha1.RefTarget, newClientResolveRevision gitClientGetter) (map[string]string, error) { +func resolveReferencedSources(hasMultipleSources bool, source *v1alpha1.ApplicationSourceHelm, refSources map[string]*v1alpha1.RefTarget, newClientResolveRevision gitClientGetter, gitClientOpts git.ClientOpts) (map[string]string, error) { repoRefs := make(map[string]string) if !hasMultipleSources || source == nil { return repoRefs, nil @@ -490,7 +491,7 @@ func resolveReferencedSources(hasMultipleSources bool, source *v1alpha1.Applicat normalizedRepoURL := git.NormalizeGitURL(refSourceMapping.Repo.Repo) _, ok = repoRefs[normalizedRepoURL] if !ok { - _, referencedCommitSHA, err := newClientResolveRevision(&refSourceMapping.Repo, refSourceMapping.TargetRevision) + _, referencedCommitSHA, err := newClientResolveRevision(&refSourceMapping.Repo, refSourceMapping.TargetRevision, gitClientOpts) if err != nil { log.Errorf("Failed to get git client for repo %s: %v", refSourceMapping.Repo.Repo, err) return nil, fmt.Errorf("failed to get git client for repo %s", refSourceMapping.Repo.Repo) @@ -506,6 +507,17 @@ func resolveReferencedSources(hasMultipleSources bool, source *v1alpha1.Applicat func (s *Service) GenerateManifest(ctx context.Context, q *apiclient.ManifestRequest) (*apiclient.ManifestResponse, error) { var res *apiclient.ManifestResponse var err error + + settings := operationSettings{sem: s.parallelismLimitSemaphore, noCache: q.NoCache, noRevisionCache: q.NoRevisionCache, allowConcurrent: q.ApplicationSource.AllowsConcurrentProcessing()} + // Skip this path for ref only sources + if q.HasMultipleSources && q.ApplicationSource.Path == "" && q.ApplicationSource.Chart == "" && q.ApplicationSource.Ref != "" { + _, revision, err := s.newClientResolveRevision(q.Repo, q.Revision, git.WithCache(s.cache, !settings.noRevisionCache && !settings.noCache)) + res = &apiclient.ManifestResponse{ + Revision: revision, + } + return res, err + } + cacheFn := func(cacheKey string, refSourceCommitSHAs cache.ResolvedRevisions, firstInvocation bool) (bool, error) { ok, resp, err := s.getManifestCacheEntry(cacheKey, q, refSourceCommitSHAs, firstInvocation) res = resp @@ -544,7 +556,6 @@ func (s *Service) GenerateManifest(ctx context.Context, q *apiclient.ManifestReq return nil } - settings := operationSettings{sem: s.parallelismLimitSemaphore, noCache: q.NoCache, noRevisionCache: q.NoRevisionCache, allowConcurrent: q.ApplicationSource.AllowsConcurrentProcessing()} err = s.runRepoOperation(ctx, q.Revision, q.Repo, q.ApplicationSource, q.VerifySignature, cacheFn, operation, settings, q.HasMultipleSources, q.RefSources) // if the tarDoneCh message is sent it means that the manifest @@ -686,6 +697,8 @@ func (s *Service) runManifestGenAsync(ctx context.Context, repoRoot, commitSHA, close(ch.responseCh) }() + settings := operationSettings{sem: s.parallelismLimitSemaphore, noCache: q.NoCache, noRevisionCache: q.NoRevisionCache, allowConcurrent: q.ApplicationSource.AllowsConcurrentProcessing()} + // GenerateManifests mutates the source (applies overrides). Those overrides shouldn't be reflected in the cache // key. Overrides will break the cache anyway, because changes to overrides will change the revision. appSourceCopy := q.ApplicationSource.DeepCopy() @@ -728,7 +741,7 @@ func (s *Service) runManifestGenAsync(ctx context.Context, repoRoot, commitSHA, return } } else { - gitClient, referencedCommitSHA, err := s.newClientResolveRevision(&refSourceMapping.Repo, refSourceMapping.TargetRevision) + gitClient, referencedCommitSHA, err := s.newClientResolveRevision(&refSourceMapping.Repo, refSourceMapping.TargetRevision, git.WithCache(s.cache, !settings.noRevisionCache && !settings.noCache)) if err != nil { log.Errorf("Failed to get git client for repo %s: %v", refSourceMapping.Repo.Repo, err) ch.errCh <- fmt.Errorf("failed to get git client for repo %s", refSourceMapping.Repo.Repo) diff --git a/reposerver/repository/repository_test.go b/reposerver/repository/repository_test.go index a24eb5008c47b..b2af471b30e6f 100644 --- a/reposerver/repository/repository_test.go +++ b/reposerver/repository/repository_test.go @@ -14,9 +14,11 @@ import ( "regexp" "sort" "strings" + "sync" "testing" "time" + cacheutil "github.com/argoproj/argo-cd/v2/util/cache" log "github.com/sirupsen/logrus" "k8s.io/apimachinery/pkg/api/resource" @@ -28,13 +30,15 @@ import ( "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/yaml" + "github.com/argoproj/argo-cd/v2/common" + "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" argoappv1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" "github.com/argoproj/argo-cd/v2/reposerver/apiclient" "github.com/argoproj/argo-cd/v2/reposerver/cache" + repositorymocks "github.com/argoproj/argo-cd/v2/reposerver/cache/mocks" "github.com/argoproj/argo-cd/v2/reposerver/metrics" fileutil "github.com/argoproj/argo-cd/v2/test/fixture/path" "github.com/argoproj/argo-cd/v2/util/argo" - cacheutil "github.com/argoproj/argo-cd/v2/util/cache" dbmocks "github.com/argoproj/argo-cd/v2/util/db/mocks" "github.com/argoproj/argo-cd/v2/util/git" gitmocks "github.com/argoproj/argo-cd/v2/util/git/mocks" @@ -51,7 +55,29 @@ gpg: Good signature from "GitHub (web-flow commit signing) " type clientFunc func(*gitmocks.Client, *helmmocks.Client, *iomocks.TempPaths) -func newServiceWithMocks(root string, signed bool) (*Service, *gitmocks.Client) { +type repoCacheMocks struct { + mock.Mock + cacheutilCache *cacheutil.Cache + cache *cache.Cache + mockCache *repositorymocks.MockRepoCache +} + +func newCacheMocks() *repoCacheMocks { + mockRepoCache := repositorymocks.NewMockRepoCache(&repositorymocks.MockCacheOptions{ + RepoCacheExpiration: 1 * time.Minute, + RevisionCacheExpiration: 1 * time.Minute, + ReadDelay: 0, + WriteDelay: 0, + }) + cacheutilCache := cacheutil.NewCache(mockRepoCache.TwoLevelClient) + return &repoCacheMocks{ + cacheutilCache: cacheutilCache, + cache: cache.NewCache(cacheutilCache, 1*time.Minute, 1*time.Minute), + mockCache: mockRepoCache, + } +} + +func newServiceWithMocks(root string, signed bool) (*Service, *gitmocks.Client, *repoCacheMocks) { root, err := filepath.Abs(root) if err != nil { panic(err) @@ -89,16 +115,13 @@ func newServiceWithMocks(root string, signed bool) (*Service, *gitmocks.Client) }, root) } -func newServiceWithOpt(cf clientFunc, root string) (*Service, *gitmocks.Client) { +func newServiceWithOpt(cf clientFunc, root string) (*Service, *gitmocks.Client, *repoCacheMocks) { helmClient := &helmmocks.Client{} gitClient := &gitmocks.Client{} paths := &iomocks.TempPaths{} cf(gitClient, helmClient, paths) - service := NewService(metrics.NewMetricsServer(), cache.NewCache( - cacheutil.NewCache(cacheutil.NewInMemoryCache(1*time.Minute)), - 1*time.Minute, - 1*time.Minute, - ), RepoServerInitConstants{ParallelismLimit: 1}, argo.NewResourceTracking(), &git.NoopCredsStore{}, root) + cacheMocks := newCacheMocks() + service := NewService(metrics.NewMetricsServer(), cacheMocks.cache, RepoServerInitConstants{ParallelismLimit: 1}, argo.NewResourceTracking(), &git.NoopCredsStore{}, root) service.newGitClient = func(rawRepoURL string, root string, creds git.Creds, insecure bool, enableLfs bool, prosy string, opts ...git.ClientOpts) (client git.Client, e error) { return gitClient, nil @@ -110,16 +133,16 @@ func newServiceWithOpt(cf clientFunc, root string) (*Service, *gitmocks.Client) return io.NopCloser } service.gitRepoPaths = paths - return service, gitClient + return service, gitClient, cacheMocks } func newService(root string) *Service { - service, _ := newServiceWithMocks(root, false) + service, _, _ := newServiceWithMocks(root, false) return service } func newServiceWithSignature(root string) *Service { - service, _ := newServiceWithMocks(root, true) + service, _, _ := newServiceWithMocks(root, true) return service } @@ -131,7 +154,7 @@ func newServiceWithCommitSHA(root, revision string) *Service { revisionErr = errors.New("not a commit SHA") } - service, gitClient := newServiceWithOpt(func(gitClient *gitmocks.Client, helmClient *helmmocks.Client, paths *iomocks.TempPaths) { + service, gitClient, _ := newServiceWithOpt(func(gitClient *gitmocks.Client, helmClient *helmmocks.Client, paths *iomocks.TempPaths) { gitClient.On("Init").Return(nil) gitClient.On("Fetch", mock.Anything).Return(nil) gitClient.On("Checkout", mock.Anything, mock.Anything).Return(nil) @@ -376,7 +399,7 @@ func TestHelmChartReferencingExternalValues_OutOfBounds_Symlink(t *testing.T) { } func TestGenerateManifestsUseExactRevision(t *testing.T) { - service, gitClient := newServiceWithMocks(".", false) + service, gitClient, _ := newServiceWithMocks(".", false) src := argoappv1.ApplicationSource{Path: "./testdata/recurse", Directory: &argoappv1.ApplicationSourceDirectory{Recurse: true}} @@ -1401,7 +1424,7 @@ func TestGetHelmCharts(t *testing.T) { } func TestGetRevisionMetadata(t *testing.T) { - service, gitClient := newServiceWithMocks("../..", false) + service, gitClient, _ := newServiceWithMocks("../..", false) now := time.Now() gitClient.On("RevisionMetadata", mock.Anything).Return(&git.RevisionMetadata{ @@ -1760,6 +1783,27 @@ func TestGenerateManifestsWithAppParameterFile(t *testing.T) { }) }) + t.Run("Multi-source with source as ref only does not generate manifests", func(t *testing.T) { + service := newService(".") + runWithTempTestdata(t, "single-app-only", func(t *testing.T, path string) { + manifests, err := service.GenerateManifest(context.Background(), &apiclient.ManifestRequest{ + Repo: &argoappv1.Repository{}, + ApplicationSource: &argoappv1.ApplicationSource{ + Path: "", + Chart: "", + Ref: "test", + }, + AppName: "testapp-multi-ref-only", + ProjectName: "something", + ProjectSourceRepos: []string{"*"}, + HasMultipleSources: true, + }) + assert.NoError(t, err) + assert.Empty(t, manifests.Manifests) + assert.NotEmpty(t, manifests.Revision) + }) + }) + t.Run("Application specific override for other app", func(t *testing.T) { service := newService(".") runWithTempTestdata(t, "single-app-only", func(t *testing.T, path string) { @@ -2579,9 +2623,11 @@ func TestDirectoryPermissionInitializer(t *testing.T) { require.Error(t, err) } -func initGitRepo(repoPath string, remote string) error { - if err := os.Mkdir(repoPath, 0755); err != nil { - return err +func initGitRepo(repoPath string, remote string, createPath bool, makeEmptyCommit bool) error { + if createPath { + if err := os.Mkdir(repoPath, 0755); err != nil { + return err + } } cmd := exec.Command("git", "init", repoPath) @@ -2589,9 +2635,21 @@ func initGitRepo(repoPath string, remote string) error { if err := cmd.Run(); err != nil { return err } - cmd = exec.Command("git", "remote", "add", "origin", remote) - cmd.Dir = repoPath - return cmd.Run() + if remote != "" { + cmd = exec.Command("git", "remote", "add", "origin", remote) + cmd.Dir = repoPath + if err := cmd.Run(); err != nil { + return err + } + } + if makeEmptyCommit { + cmd = exec.Command("git", "commit", "-m", "Initial commit", "--allow-empty") + cmd.Dir = repoPath + if err := cmd.Run(); err != nil { + return err + } + } + return nil } func TestInit(t *testing.T) { @@ -2604,7 +2662,7 @@ func TestInit(t *testing.T) { }) repoPath := path.Join(dir, "repo1") - require.NoError(t, initGitRepo(repoPath, "https://github.com/argo-cd/test-repo1")) + require.NoError(t, initGitRepo(repoPath, "https://github.com/argo-cd/test-repo1", true, false)) service := newService(".") service.rootDir = dir @@ -2613,7 +2671,7 @@ func TestInit(t *testing.T) { _, err := os.ReadDir(dir) require.Error(t, err) - require.NoError(t, initGitRepo(path.Join(dir, "repo2"), "https://github.com/argo-cd/test-repo2")) + require.NoError(t, initGitRepo(path.Join(dir, "repo2"), "https://github.com/argo-cd/test-repo2", true, false)) } // TestCheckoutRevisionCanGetNonstandardRefs shows that we can fetch a revision that points to a non-standard ref. In @@ -2935,7 +2993,7 @@ func TestErrorGetGitDirectories(t *testing.T) { }, }, want: nil, wantErr: assert.Error}, {name: "InvalidResolveRevision", fields: fields{service: func() *Service { - s, _ := newServiceWithOpt(func(gitClient *gitmocks.Client, helmClient *helmmocks.Client, paths *iomocks.TempPaths) { + s, _, _ := newServiceWithOpt(func(gitClient *gitmocks.Client, helmClient *helmmocks.Client, paths *iomocks.TempPaths) { gitClient.On("Checkout", mock.Anything, mock.Anything).Return(nil) gitClient.On("LsRemote", mock.Anything).Return("", fmt.Errorf("ah error")) paths.On("GetPath", mock.Anything).Return(".", nil) @@ -2966,7 +3024,7 @@ func TestErrorGetGitDirectories(t *testing.T) { func TestGetGitDirectories(t *testing.T) { // test not using the cache root := "./testdata/git-files-dirs" - s, _ := newServiceWithOpt(func(gitClient *gitmocks.Client, helmClient *helmmocks.Client, paths *iomocks.TempPaths) { + s, _, cacheMocks := newServiceWithOpt(func(gitClient *gitmocks.Client, helmClient *helmmocks.Client, paths *iomocks.TempPaths) { gitClient.On("Init").Return(nil) gitClient.On("Fetch", mock.Anything).Return(nil) gitClient.On("Checkout", mock.Anything, mock.Anything).Once().Return(nil) @@ -2989,6 +3047,11 @@ func TestGetGitDirectories(t *testing.T) { directories, err = s.GetGitDirectories(context.TODO(), dirRequest) assert.Nil(t, err) assert.ElementsMatch(t, []string{"app", "app/bar", "app/foo/bar", "somedir", "app/foo"}, directories.GetPaths()) + cacheMocks.mockCache.AssertCacheCalledTimes(t, &repositorymocks.CacheCallCounts{ + ExternalSets: 1, + ExternalGets: 1, + InMemoryGets: 1, + }) } func TestErrorGetGitFiles(t *testing.T) { @@ -3015,7 +3078,7 @@ func TestErrorGetGitFiles(t *testing.T) { }, }, want: nil, wantErr: assert.Error}, {name: "InvalidResolveRevision", fields: fields{service: func() *Service { - s, _ := newServiceWithOpt(func(gitClient *gitmocks.Client, helmClient *helmmocks.Client, paths *iomocks.TempPaths) { + s, _, _ := newServiceWithOpt(func(gitClient *gitmocks.Client, helmClient *helmmocks.Client, paths *iomocks.TempPaths) { gitClient.On("Checkout", mock.Anything, mock.Anything).Return(nil) gitClient.On("LsRemote", mock.Anything).Return("", fmt.Errorf("ah error")) paths.On("GetPath", mock.Anything).Return(".", nil) @@ -3048,7 +3111,7 @@ func TestGetGitFiles(t *testing.T) { files := []string{"./testdata/git-files-dirs/somedir/config.yaml", "./testdata/git-files-dirs/config.yaml", "./testdata/git-files-dirs/config.yaml", "./testdata/git-files-dirs/app/foo/bar/config.yaml"} root := "" - s, _ := newServiceWithOpt(func(gitClient *gitmocks.Client, helmClient *helmmocks.Client, paths *iomocks.TempPaths) { + s, _, cacheMocks := newServiceWithOpt(func(gitClient *gitmocks.Client, helmClient *helmmocks.Client, paths *iomocks.TempPaths) { gitClient.On("Init").Return(nil) gitClient.On("Fetch", mock.Anything).Return(nil) gitClient.On("Checkout", mock.Anything, mock.Anything).Once().Return(nil) @@ -3081,6 +3144,10 @@ func TestGetGitFiles(t *testing.T) { fileResponse, err = s.GetGitFiles(context.TODO(), filesRequest) assert.Nil(t, err) assert.Equal(t, expected, fileResponse.GetMap()) + cacheMocks.mockCache.AssertCacheCalledTimes(t, &repositorymocks.CacheCallCounts{ + ExternalSets: 1, + ExternalGets: 2, + }) } func Test_getRepoSanitizerRegex(t *testing.T) { @@ -3090,3 +3157,136 @@ func Test_getRepoSanitizerRegex(t *testing.T) { msg = r.ReplaceAllString("error message containing /tmp/_argocd-repo/SENSITIVE/with/trailing/path and other stuff", "") assert.Equal(t, "error message containing /with/trailing/path and other stuff", msg) } + +func TestGetRefs_CacheDisabled(t *testing.T) { + // Test that default get refs with cache disabled does not call GetOrLockGitReferences + dir := t.TempDir() + err := initGitRepo(dir, "", false, true) + assert.NoError(t, err, "Could not init git repo") + cacheMocks := newCacheMocks() + t.Cleanup(cacheMocks.mockCache.StopRedisCallback) + client, err := git.NewClient(fmt.Sprintf("file://%s", dir), git.NopCreds{}, true, false, "", git.WithCache(cacheMocks.cache, false)) + require.NoError(t, err) + refs, err := client.LsRefs() + assert.NoError(t, err) + assert.NotNil(t, refs) + assert.NotEqual(t, 0, len(refs.Branches), "Expected branches to be populated") + assert.NotEmpty(t, refs.Branches[0]) + // Unlock should not have been called + cacheMocks.mockCache.AssertNumberOfCalls(t, "UnlockGitReferences", 0) + cacheMocks.mockCache.AssertNumberOfCalls(t, "GetOrLockGitReferences", 0) +} + +func TestGetRefs_CacheWithLock(t *testing.T) { + // Test that there is only one call to SetGitReferences for the same repo which is done after the ls-remote + dir := t.TempDir() + err := initGitRepo(dir, "", false, true) + assert.NoError(t, err, "Could not init git repo") + cacheMocks := newCacheMocks() + t.Cleanup(cacheMocks.mockCache.StopRedisCallback) + var wg sync.WaitGroup + numberOfCallers := 10 + for i := 0; i < numberOfCallers; i++ { + wg.Add(1) + go func() { + defer wg.Done() + client, err := git.NewClient(fmt.Sprintf("file://%s", dir), git.NopCreds{}, true, false, "", git.WithCache(cacheMocks.cache, true)) + require.NoError(t, err) + refs, err := client.LsRefs() + assert.NoError(t, err) + assert.NotNil(t, refs) + assert.NotEqual(t, 0, len(refs.Branches), "Expected branches to be populated") + assert.NotEmpty(t, refs.Branches[0]) + }() + } + wg.Wait() + // Unlock should not have been called + cacheMocks.mockCache.AssertNumberOfCalls(t, "UnlockGitReferences", 0) + cacheMocks.mockCache.AssertNumberOfCalls(t, "GetOrLockGitReferences", 0) +} + +func TestGetRefs_CacheUnlockedOnUpdateFailed(t *testing.T) { + // Worst case the ttl on the lock expires and the lock is removed + // however if the holder of the lock fails to update the cache the caller should remove the lock + // to allow other callers to attempt to update the cache as quickly as possible + dir := t.TempDir() + err := initGitRepo(dir, "", false, true) + assert.NoError(t, err, "Could not init git repo") + cacheMocks := newCacheMocks() + t.Cleanup(cacheMocks.mockCache.StopRedisCallback) + repoUrl := fmt.Sprintf("file://%s", dir) + client, err := git.NewClient(repoUrl, git.NopCreds{}, true, false, "", git.WithCache(cacheMocks.cache, true)) + require.NoError(t, err) + refs, err := client.LsRefs() + assert.NoError(t, err) + assert.NotNil(t, refs) + assert.NotEqual(t, 0, len(refs.Branches), "Expected branches to be populated") + assert.NotEmpty(t, refs.Branches[0]) + var output [][2]string + err = cacheMocks.cacheutilCache.GetItem(fmt.Sprintf("git-refs|%s|%s", repoUrl, common.CacheVersion), &output, nil) + assert.Error(t, err, "Should be a cache miss") + assert.Equal(t, 0, len(output), "Expected cache to be empty for key") + cacheMocks.mockCache.AssertNumberOfCalls(t, "UnlockGitReferences", 0) + cacheMocks.mockCache.AssertNumberOfCalls(t, "GetOrLockGitReferences", 0) +} + +func TestGetRefs_CacheLockTryLockGitRefCacheError(t *testing.T) { + // Worst case the ttl on the lock expires and the lock is removed + // however if the holder of the lock fails to update the cache the caller should remove the lock + // to allow other callers to attempt to update the cache as quickly as possible + dir := t.TempDir() + err := initGitRepo(dir, "", false, true) + assert.NoError(t, err, "Could not init git repo") + cacheMocks := newCacheMocks() + t.Cleanup(cacheMocks.mockCache.StopRedisCallback) + repoUrl := fmt.Sprintf("file://%s", dir) + // buf := bytes.Buffer{} + // log.SetOutput(&buf) + client, err := git.NewClient(repoUrl, git.NopCreds{}, true, false, "", git.WithCache(cacheMocks.cache, true)) + require.NoError(t, err) + refs, err := client.LsRefs() + assert.NoError(t, err) + assert.NotNil(t, refs) +} + +func TestGetRevisionChartDetails(t *testing.T) { + t.Run("Test revision semvar", func(t *testing.T) { + root := t.TempDir() + service := newService(root) + _, err := service.GetRevisionChartDetails(context.Background(), &apiclient.RepoServerRevisionChartDetailsRequest{ + Repo: &v1alpha1.Repository{ + Repo: fmt.Sprintf("file://%s", root), + Name: "test-repo-name", + Type: "helm", + }, + Name: "test-name", + Revision: "test-revision", + }) + assert.ErrorContains(t, err, "invalid revision") + }) + + t.Run("Test GetRevisionChartDetails", func(t *testing.T) { + root := t.TempDir() + service := newService(root) + repoUrl := fmt.Sprintf("file://%s", root) + err := service.cache.SetRevisionChartDetails(repoUrl, "my-chart", "1.1.0", &argoappv1.ChartDetails{ + Description: "test-description", + Home: "test-home", + Maintainers: []string{"test-maintainer"}, + }) + assert.NoError(t, err) + chartDetails, err := service.GetRevisionChartDetails(context.Background(), &apiclient.RepoServerRevisionChartDetailsRequest{ + Repo: &v1alpha1.Repository{ + Repo: fmt.Sprintf("file://%s", root), + Name: "test-repo-name", + Type: "helm", + }, + Name: "my-chart", + Revision: "1.1.0", + }) + assert.NoError(t, err) + assert.Equal(t, "test-description", chartDetails.Description) + assert.Equal(t, "test-home", chartDetails.Home) + assert.Equal(t, []string{"test-maintainer"}, chartDetails.Maintainers) + }) +} diff --git a/util/cache/appstate/cache.go b/util/cache/appstate/cache.go index d59d31befb12e..3e9b81a0ff9fd 100644 --- a/util/cache/appstate/cache.go +++ b/util/cache/appstate/cache.go @@ -37,7 +37,7 @@ func AddCacheFlagsToCmd(cmd *cobra.Command, opts ...func(client *redis.Client)) cacheFactory := cacheutil.AddCacheFlagsToCmd(cmd, opts...) return func() (*Cache, error) { - cache, err := cacheFactory() + cache, err := cacheFactory(false) if err != nil { return nil, err } @@ -46,11 +46,11 @@ func AddCacheFlagsToCmd(cmd *cobra.Command, opts ...func(client *redis.Client)) } func (c *Cache) GetItem(key string, item interface{}) error { - return c.Cache.GetItem(key, item) + return c.Cache.GetItem(key, item, nil) } func (c *Cache) SetItem(key string, item interface{}, expiration time.Duration, delete bool) error { - return c.Cache.SetItem(key, item, expiration, delete) + return c.Cache.SetItem(key, item, &cacheutil.CacheActionOpts{Expiration: expiration, Delete: delete}) } func appManagedResourcesKey(appName string) string { diff --git a/util/cache/cache.go b/util/cache/cache.go index fdea46cdea0d2..049a1c998a5db 100644 --- a/util/cache/cache.go +++ b/util/cache/cache.go @@ -16,6 +16,7 @@ import ( "github.com/argoproj/argo-cd/v2/common" certutil "github.com/argoproj/argo-cd/v2/util/cert" "github.com/argoproj/argo-cd/v2/util/env" + log "github.com/sirupsen/logrus" ) const ( @@ -30,7 +31,7 @@ const ( ) func NewCache(client CacheClient) *Cache { - return &Cache{client} + return &Cache{client: client} } func buildRedisClient(redisAddress, password, username string, redisDB, maxRetries int, tlsConfig *tls.Config) *redis.Client { @@ -73,7 +74,7 @@ func buildFailoverRedisClient(sentinelMaster, password, username string, redisDB } // AddCacheFlagsToCmd adds flags which control caching to the specified command -func AddCacheFlagsToCmd(cmd *cobra.Command, opts ...func(client *redis.Client)) func() (*Cache, error) { +func AddCacheFlagsToCmd(cmd *cobra.Command, opts ...func(client *redis.Client)) func(useTwoLevelCache bool) (*Cache, error) { redisAddress := "" sentinelAddresses := make([]string, 0) sentinelMaster := "" @@ -97,7 +98,7 @@ func AddCacheFlagsToCmd(cmd *cobra.Command, opts ...func(client *redis.Client)) cmd.Flags().BoolVar(&insecureRedis, "redis-insecure-skip-tls-verify", false, "Skip Redis server certificate validation.") cmd.Flags().StringVar(&redisCACertificate, "redis-ca-certificate", "", "Path to Redis server CA certificate (e.g. /etc/certs/redis/ca.crt). If not specified, system trusted CAs will be used for server certificate validation.") cmd.Flags().StringVar(&compressionStr, "redis-compress", env.StringFromEnv("REDIS_COMPRESSION", string(RedisCompressionGZip)), "Enable compression for data sent to Redis with the required compression algorithm. (possible values: gzip, none)") - return func() (*Cache, error) { + return func(useTwoLevelCache bool) (*Cache, error) { var tlsConfig *tls.Config = nil if redisUseTLS { tlsConfig = &tls.Config{} @@ -131,22 +132,23 @@ func AddCacheFlagsToCmd(cmd *cobra.Command, opts ...func(client *redis.Client)) if err != nil { return nil, err } + var redisClient *redis.Client if len(sentinelAddresses) > 0 { - client := buildFailoverRedisClient(sentinelMaster, password, username, redisDB, maxRetries, tlsConfig, sentinelAddresses) - for i := range opts { - opts[i](client) + redisClient = buildFailoverRedisClient(sentinelMaster, password, username, redisDB, maxRetries, tlsConfig, sentinelAddresses) + } else { + if redisAddress == "" { + redisAddress = common.DefaultRedisAddr } - return NewCache(NewRedisCache(client, defaultCacheExpiration, compression)), nil + redisClient = buildRedisClient(redisAddress, password, username, redisDB, maxRetries, tlsConfig) } - if redisAddress == "" { - redisAddress = common.DefaultRedisAddr - } - - client := buildRedisClient(redisAddress, password, username, redisDB, maxRetries, tlsConfig) + redisCache := NewRedisCache(redisClient, defaultCacheExpiration, compression) for i := range opts { - opts[i](client) + opts[i](redisClient) + } + if useTwoLevelCache { + return NewCache(NewTwoLevelClient(redisCache, defaultCacheExpiration)), nil } - return NewCache(NewRedisCache(client, defaultCacheExpiration, compression)), nil + return NewCache(redisCache), nil } } @@ -155,6 +157,7 @@ type Cache struct { client CacheClient } +// Returns the cache client with the given type func (c *Cache) GetClient() CacheClient { return c.client } @@ -163,30 +166,46 @@ func (c *Cache) SetClient(client CacheClient) { c.client = client } -func (c *Cache) SetItem(key string, item interface{}, expiration time.Duration, delete bool) error { - key = fmt.Sprintf("%s|%s", key, common.CacheVersion) - if delete { - return c.client.Delete(key) +func (c *Cache) generateFullKey(key string) string { + if key == "" { + log.Debug("Cache key is empty, this will result in key collisions if there is more than one empty key") + } + return fmt.Sprintf("%s|%s", key, common.CacheVersion) +} + +// Sets or deletes an item in cache +func (c *Cache) SetItem(key string, item interface{}, opts *CacheActionOpts) error { + if opts == nil { + opts = &CacheActionOpts{} + } + if item == nil { + return fmt.Errorf("cannot set nil item in cache") + } + fullKey := c.generateFullKey(key) + client := c.GetClient() + if opts.Delete { + return client.Delete(fullKey) } else { - if item == nil { - return fmt.Errorf("cannot set item to nil for key %s", key) - } - return c.client.Set(&Item{Object: item, Key: key, Expiration: expiration}) + return client.Set(&Item{Key: fullKey, Object: item, CacheActionOpts: *opts}) } } -func (c *Cache) GetItem(key string, item interface{}) error { +func (c *Cache) GetItem(key string, item interface{}, opts *CacheActionOpts) error { + if opts == nil { + opts = &CacheActionOpts{} + } + key = c.generateFullKey(key) if item == nil { return fmt.Errorf("cannot get item into a nil for key %s", key) } - key = fmt.Sprintf("%s|%s", key, common.CacheVersion) - return c.client.Get(key, item) + client := c.GetClient() + return client.Get(&Item{Key: key, Object: item, CacheActionOpts: *opts}) } func (c *Cache) OnUpdated(ctx context.Context, key string, callback func() error) error { - return c.client.OnUpdated(ctx, fmt.Sprintf("%s|%s", key, common.CacheVersion), callback) + return c.client.OnUpdated(ctx, c.generateFullKey(key), callback) } func (c *Cache) NotifyUpdated(key string) error { - return c.client.NotifyUpdated(fmt.Sprintf("%s|%s", key, common.CacheVersion)) + return c.client.NotifyUpdated(c.generateFullKey(key)) } diff --git a/util/cache/cache_test.go b/util/cache/cache_test.go index 6a1b519e7eb57..aef831d83ad6a 100644 --- a/util/cache/cache_test.go +++ b/util/cache/cache_test.go @@ -1,46 +1,96 @@ package cache import ( + "fmt" "testing" "time" + "github.com/alicebob/miniredis/v2" + "github.com/argoproj/argo-cd/v2/common" + "github.com/redis/go-redis/v9" "github.com/spf13/cobra" "github.com/stretchr/testify/assert" ) func TestAddCacheFlagsToCmd(t *testing.T) { - cache, err := AddCacheFlagsToCmd(&cobra.Command{})() + cache, err := AddCacheFlagsToCmd(&cobra.Command{})(false) assert.NoError(t, err) assert.Equal(t, 24*time.Hour, cache.client.(*redisCache).expiration) } +func TestAddCacheFlagsToCmdTwoLevelCache(t *testing.T) { + cache, err := AddCacheFlagsToCmd(&cobra.Command{})(true) + assert.NoError(t, err) + assert.Equal(t, 24*time.Hour, cache.client.(*twoLevelClient).externalCache.(*redisCache).expiration) +} + +func NewInMemoryRedis() (*redis.Client, func()) { + mr, err := miniredis.Run() + if err != nil { + panic(err) + } + return redis.NewClient(&redis.Options{Addr: mr.Addr()}), mr.Close +} + func TestCacheClient(t *testing.T) { + clientRedis, stopRedis := NewInMemoryRedis() + defer stopRedis() + redisCache := NewRedisCache(clientRedis, 5*time.Second, RedisCompressionNone) + clientMemCache := NewInMemoryCache(60 * time.Second) + twoLevelClient := NewTwoLevelClient(redisCache, 5*time.Second) + // Run tests for both Redis and InMemoryCache + for _, client := range []CacheClient{clientMemCache, redisCache, twoLevelClient} { + cache := NewCache(client) + t.Run("SetItem", func(t *testing.T) { + err := cache.SetItem("foo", "bar", &CacheActionOpts{Expiration: 60 * time.Second, DisableOverwrite: true, Delete: false}) + assert.NoError(t, err) + var output string + err = cache.GetItem("foo", &output, nil) + assert.NoError(t, err) + assert.Equal(t, "bar", output) + }) + t.Run("SetCacheItem W/Disable Overwrite", func(t *testing.T) { + err := cache.SetItem("foo", "bar", &CacheActionOpts{Expiration: 60 * time.Second, DisableOverwrite: true, Delete: false}) + assert.NoError(t, err) + var output string + err = cache.GetItem("foo", &output, nil) + assert.NoError(t, err) + assert.Equal(t, "bar", output) + err = cache.SetItem("foo", "bar", &CacheActionOpts{Expiration: 60 * time.Second, DisableOverwrite: true, Delete: false}) + assert.NoError(t, err) + err = cache.GetItem("foo", &output, nil) + assert.NoError(t, err) + assert.Equal(t, "bar", output, "output should not have changed with DisableOverwrite set to true") + }) + t.Run("GetItem", func(t *testing.T) { + var val string + err := cache.GetItem("foo", &val, nil) + assert.NoError(t, err) + assert.Equal(t, "bar", val) + }) + t.Run("DeleteItem", func(t *testing.T) { + err := cache.SetItem("foo", "bar", &CacheActionOpts{Expiration: 0, Delete: true}) + assert.NoError(t, err) + var val string + err = cache.GetItem("foo", &val, nil) + assert.Error(t, err) + assert.Empty(t, val) + }) + t.Run("Check for nil items", func(t *testing.T) { + err := cache.SetItem("foo", nil, &CacheActionOpts{Expiration: 0, Delete: true}) + assert.Error(t, err) + assert.Contains(t, err.Error(), "cannot set nil item") + err = cache.GetItem("foo", nil, nil) + assert.Error(t, err) + assert.Contains(t, err.Error(), "cannot get item") + }) + } +} + +// Smoke test to ensure key changes aren't done accidentally +func TestGenerateCacheKey(t *testing.T) { client := NewInMemoryCache(60 * time.Second) cache := NewCache(client) - t.Run("SetItem", func(t *testing.T) { - err := cache.SetItem("foo", "bar", 60*time.Second, false) - assert.NoError(t, err) - }) - t.Run("GetItem", func(t *testing.T) { - var val string - err := cache.GetItem("foo", &val) - assert.NoError(t, err) - assert.Equal(t, "bar", val) - }) - t.Run("DeleteItem", func(t *testing.T) { - err := cache.SetItem("foo", "bar", 0, true) - assert.NoError(t, err) - var val string - err = cache.GetItem("foo", &val) - assert.Error(t, err) - assert.Empty(t, val) - }) - t.Run("Check for nil items", func(t *testing.T) { - err := cache.SetItem("foo", nil, 0, false) - assert.Error(t, err) - assert.Contains(t, err.Error(), "cannot set item") - err = cache.GetItem("foo", nil) - assert.Error(t, err) - assert.Contains(t, err.Error(), "cannot get item") - }) + testKey := cache.generateFullKey("testkey") + assert.Equal(t, fmt.Sprintf("testkey|%s", common.CacheVersion), testKey) } diff --git a/util/cache/client.go b/util/cache/client.go index 434c2a8da187a..33e188057c1c1 100644 --- a/util/cache/client.go +++ b/util/cache/client.go @@ -7,17 +7,38 @@ import ( ) var ErrCacheMiss = errors.New("cache: key is missing") +var ErrCacheKeyLocked = errors.New("cache: key is locked") +var CacheLockedValue = "locked" + +type CacheType int + +const ( + CacheTypeDefault CacheType = iota + CacheTypeInMemory + CacheTypeTwoLevel + CacheTypeExternal +) + +type CacheActionOpts struct { + // Delete item from cache + Delete bool + // Disable writing if key already exists (NX) + DisableOverwrite bool + // Expiration is the cache expiration time. + Expiration time.Duration + // Determines the cache type to use in case of two level cache, Default == Two Level(in-memory + external) + CacheType CacheType +} type Item struct { Key string Object interface{} - // Expiration is the cache expiration time. - Expiration time.Duration + CacheActionOpts } type CacheClient interface { Set(item *Item) error - Get(key string, obj interface{}) error + Get(item *Item) error Delete(key string) error OnUpdated(ctx context.Context, key string, callback func() error) error NotifyUpdated(key string) error diff --git a/util/cache/client_test.go b/util/cache/client_test.go index 058b30e21ccc0..bba7876089eb2 100644 --- a/util/cache/client_test.go +++ b/util/cache/client_test.go @@ -15,7 +15,7 @@ type testStruct struct { func TestCache(t *testing.T) { c := NewInMemoryCache(time.Hour) var obj testStruct - err := c.Get("key", &obj) + err := c.Get(&Item{Key: "key", Object: &obj}) assert.Equal(t, err, ErrCacheMiss) cacheObj := testStruct{ Foo: "foo", @@ -26,13 +26,13 @@ func TestCache(t *testing.T) { Object: &cacheObj, }) cacheObj.Foo = "baz" - err = c.Get("key", &obj) + err = c.Get(&Item{Key: "key", Object: &obj}) assert.Nil(t, err) assert.EqualValues(t, obj.Foo, "foo") assert.EqualValues(t, string(obj.Bar), "bar") err = c.Delete("key") assert.Nil(t, err) - err = c.Get("key", &obj) + err = c.Get(&Item{Key: "key", Object: &obj}) assert.Equal(t, err, ErrCacheMiss) } diff --git a/util/cache/inmemory.go b/util/cache/inmemory.go index 53e690925d940..77ae9355e5190 100644 --- a/util/cache/inmemory.go +++ b/util/cache/inmemory.go @@ -29,7 +29,12 @@ func (i *InMemoryCache) Set(item *Item) error { if err != nil { return err } - i.memCache.Set(item.Key, buf, item.Expiration) + if item.DisableOverwrite { + // go-redis doesn't throw an error on Set with NX, so absorbing here to keep the interface consistent + _ = i.memCache.Add(item.Key, buf, item.Expiration) + } else { + i.memCache.Set(item.Key, buf, item.Expiration) + } return nil } @@ -52,13 +57,13 @@ func (i *InMemoryCache) HasSame(key string, obj interface{}) (bool, error) { return bytes.Equal(buf.Bytes(), existingBuf.Bytes()), nil } -func (i *InMemoryCache) Get(key string, obj interface{}) error { - bufIf, found := i.memCache.Get(key) +func (i *InMemoryCache) Get(item *Item) error { + bufIf, found := i.memCache.Get(item.Key) if !found { return ErrCacheMiss } buf := bufIf.(bytes.Buffer) - return gob.NewDecoder(&buf).Decode(obj) + return gob.NewDecoder(&buf).Decode(item.Object) } func (i *InMemoryCache) Delete(key string) error { diff --git a/util/cache/inmemory_test.go b/util/cache/inmemory_test.go index 4e0d1e48a5a89..3c4ae3d25193d 100644 --- a/util/cache/inmemory_test.go +++ b/util/cache/inmemory_test.go @@ -16,12 +16,12 @@ func TestInMemoryCache(t *testing.T) { // https://stackoverflow.com/questions/46671636/gob-decode-giving-decodevalue-of-unassignable-value-error obj := &foo{} // cache miss - err := cache.Get("my-key", obj) + err := cache.Get(&Item{Key: "my-key", Object: obj}) assert.Equal(t, ErrCacheMiss, err) // cache hit err = cache.Set(&Item{Key: "my-key", Object: &foo{Bar: "bar"}}) assert.NoError(t, err) - err = cache.Get("my-key", obj) + err = cache.Get(&Item{Key: "my-key", Object: obj}) assert.NoError(t, err) assert.Equal(t, &foo{Bar: "bar"}, obj) } diff --git a/util/cache/mocks/cachclient.go b/util/cache/mocks/cachclient.go new file mode 100644 index 0000000000000..0637735572541 --- /dev/null +++ b/util/cache/mocks/cachclient.go @@ -0,0 +1,65 @@ +package mocks + +import ( + "context" + "time" + + cache "github.com/argoproj/argo-cd/v2/util/cache" + "github.com/stretchr/testify/mock" +) + +type MockCacheClient struct { + mock.Mock + BaseCache cache.CacheClient + ReadDelay time.Duration + WriteDelay time.Duration +} + +func (c *MockCacheClient) Set(item *cache.Item) error { + args := c.Called(item) + if len(args) > 0 && args.Get(0) != nil { + return args.Get(0).(error) + } + if c.WriteDelay > 0 { + time.Sleep(c.WriteDelay) + } + return c.BaseCache.Set(item) +} + +func (c *MockCacheClient) Get(item *cache.Item) error { + args := c.Called(item) + if len(args) > 0 && args.Get(0) != nil { + return args.Get(0).(error) + } + if c.ReadDelay > 0 { + time.Sleep(c.ReadDelay) + } + return c.BaseCache.Get(item) +} + +func (c *MockCacheClient) Delete(key string) error { + args := c.Called(key) + if len(args) > 0 && args.Get(0) != nil { + return args.Get(0).(error) + } + if c.WriteDelay > 0 { + time.Sleep(c.WriteDelay) + } + return c.BaseCache.Delete(key) +} + +func (c *MockCacheClient) OnUpdated(ctx context.Context, key string, callback func() error) error { + args := c.Called(ctx, key, callback) + if len(args) > 0 && args.Get(0) != nil { + return args.Get(0).(error) + } + return c.BaseCache.OnUpdated(ctx, key, callback) +} + +func (c *MockCacheClient) NotifyUpdated(key string) error { + args := c.Called(key) + if len(args) > 0 && args.Get(0) != nil { + return args.Get(0).(error) + } + return c.BaseCache.NotifyUpdated(key) +} diff --git a/util/cache/redis.go b/util/cache/redis.go index c5365c4984e21..832094203d348 100644 --- a/util/cache/redis.go +++ b/util/cache/redis.go @@ -110,19 +110,20 @@ func (r *redisCache) Set(item *Item) error { Key: r.getKey(item.Key), Value: val, TTL: expiration, + SetNX: item.DisableOverwrite, }) } -func (r *redisCache) Get(key string, obj interface{}) error { +func (r *redisCache) Get(item *Item) error { var data []byte - err := r.cache.Get(context.TODO(), r.getKey(key), &data) + err := r.cache.Get(context.TODO(), r.getKey(item.Key), &data) if err == rediscache.ErrCacheMiss { err = ErrCacheMiss } if err != nil { return err } - return r.unmarshal(data, obj) + return r.unmarshal(data, item.Object) } func (r *redisCache) Delete(key string) error { diff --git a/util/cache/redis_test.go b/util/cache/redis_test.go index 3800753cee3ec..b0fc602498c0a 100644 --- a/util/cache/redis_test.go +++ b/util/cache/redis_test.go @@ -27,7 +27,7 @@ func TestRedisSetCache(t *testing.T) { t.Run("Successful get", func(t *testing.T) { var res string client := NewRedisCache(redis.NewClient(&redis.Options{Addr: mr.Addr()}), 10*time.Second, RedisCompressionNone) - err = client.Get("foo", &res) + err = client.Get(&Item{Key: "foo", Object: &res}) assert.NoError(t, err) assert.Equal(t, res, "bar") }) @@ -41,7 +41,7 @@ func TestRedisSetCache(t *testing.T) { t.Run("Cache miss", func(t *testing.T) { var res string client := NewRedisCache(redis.NewClient(&redis.Options{Addr: mr.Addr()}), 10*time.Second, RedisCompressionNone) - err = client.Get("foo", &res) + err = client.Get(&Item{Key: "foo", Object: &res}) assert.Error(t, err) assert.Contains(t, err.Error(), "cache: key is missing") }) @@ -66,7 +66,7 @@ func TestRedisSetCacheCompressed(t *testing.T) { assert.True(t, len(compressedData) > len([]byte(testValue)), "compressed data is bigger than uncompressed") var result string - assert.NoError(t, client.Get("my-key", &result)) + assert.NoError(t, client.Get(&Item{Key: "my-key", Object: &result})) assert.Equal(t, testValue, result) } diff --git a/util/cache/twolevelclient.go b/util/cache/twolevelclient.go index 14a4279e87c89..fc972634b09ca 100644 --- a/util/cache/twolevelclient.go +++ b/util/cache/twolevelclient.go @@ -13,6 +13,9 @@ func NewTwoLevelClient(client CacheClient, inMemoryExpiration time.Duration) *tw return &twoLevelClient{inMemoryCache: NewInMemoryCache(inMemoryExpiration), externalCache: client} } +// compile-time validation of adherance of the CacheClient contract +var _ CacheClient = &twoLevelClient{} + type twoLevelClient struct { inMemoryCache *InMemoryCache externalCache CacheClient @@ -21,6 +24,17 @@ type twoLevelClient struct { // Set stores the given value in both in-memory and external cache. // Skip storing the value in external cache if the same value already exists in memory to avoid requesting external cache. func (c *twoLevelClient) Set(item *Item) error { + switch item.CacheType { + case CacheTypeInMemory: + return c.inMemoryCache.Set(item) + case CacheTypeExternal: + return c.externalCache.Set(item) + default: + return c.SetTwoLevel(item) + } +} + +func (c *twoLevelClient) SetTwoLevel(item *Item) error { has, err := c.inMemoryCache.HasSame(item.Key, item.Object) if has { return nil @@ -35,17 +49,28 @@ func (c *twoLevelClient) Set(item *Item) error { return c.externalCache.Set(item) } +func (c *twoLevelClient) Get(item *Item) error { + switch item.CacheType { + case CacheTypeInMemory: + return c.inMemoryCache.Get(item) + case CacheTypeExternal: + return c.externalCache.Get(item) + default: + return c.GetTwoLevel(item) + } +} + // Get returns cache value from in-memory cache if it present. Otherwise loads it from external cache and persists // in memory to avoid future requests to external cache. -func (c *twoLevelClient) Get(key string, obj interface{}) error { - err := c.inMemoryCache.Get(key, obj) +func (c *twoLevelClient) GetTwoLevel(item *Item) error { + err := c.inMemoryCache.Get(item) if err == nil { return nil } - err = c.externalCache.Get(key, obj) + err = c.externalCache.Get(item) if err == nil { - _ = c.inMemoryCache.Set(&Item{Key: key, Object: obj}) + _ = c.inMemoryCache.Set(item) } return err } diff --git a/util/git/client.go b/util/git/client.go index 6a8828d13f432..bba124d79f4f1 100644 --- a/util/git/client.go +++ b/util/git/client.go @@ -55,7 +55,8 @@ type Refs struct { type gitRefCache interface { SetGitReferences(repo string, references []*plumbing.Reference) error - GetGitReferences(repo string, references *[]*plumbing.Reference) error + GetOrLockGitReferences(repo string, references *[]*plumbing.Reference) (updateCache bool, lockId string, err error) + UnlockGitReferences(repo string, lockId string) error } // Client is a generic git client interface @@ -477,11 +478,26 @@ func (m *nativeGitClient) Checkout(revision string, submoduleEnabled bool) error } func (m *nativeGitClient) getRefs() ([]*plumbing.Reference, error) { + // Prevent an additional get call to cache if we know our state isn't stale + needsUnlock := true if m.gitRefCache != nil && m.loadRefFromCache { var res []*plumbing.Reference - if m.gitRefCache.GetGitReferences(m.repoURL, &res) == nil { + updateCache, lockId, err := m.gitRefCache.GetOrLockGitReferences(m.repoURL, &res) + if !updateCache && err == nil { + // Valid value already in cache return res, nil + } else if !updateCache && err != nil { + // Error getting value from cache + log.Debugf("Error getting git references from cache: %v", err) + return nil, err } + // Defer a soft reset of the cache lock, if the value is set this call will be ignored + defer func() { + if needsUnlock { + err := m.gitRefCache.UnlockGitReferences(m.repoURL, lockId) + log.Debugf("Error unlocking git references from cache: %v", err) + } + }() } if m.OnLsRemote != nil { @@ -508,6 +524,9 @@ func (m *nativeGitClient) getRefs() ([]*plumbing.Reference, error) { if err == nil && m.gitRefCache != nil { if err := m.gitRefCache.SetGitReferences(m.repoURL, res); err != nil { log.Warnf("Failed to store git references to cache: %v", err) + } else { + // Since we successfully overwrote the lock with valid data, we don't need to unlock + needsUnlock = false } return res, nil } diff --git a/util/git/client_test.go b/util/git/client_test.go index d5509edc2b55c..6e91868549f3e 100644 --- a/util/git/client_test.go +++ b/util/git/client_test.go @@ -20,14 +20,23 @@ func runCmd(workingDir string, name string, args ...string) error { return cmd.Run() } -func Test_nativeGitClient_Fetch(t *testing.T) { +func _createEmptyGitRepo() (string, error) { tempDir, err := os.MkdirTemp("", "") - require.NoError(t, err) + if err != nil { + return tempDir, err + } err = runCmd(tempDir, "git", "init") - require.NoError(t, err) + if err != nil { + return tempDir, err + } err = runCmd(tempDir, "git", "commit", "-m", "Initial commit", "--allow-empty") + return tempDir, err +} + +func Test_nativeGitClient_Fetch(t *testing.T) { + tempDir, err := _createEmptyGitRepo() require.NoError(t, err) client, err := NewClient(fmt.Sprintf("file://%s", tempDir), NopCreds{}, true, false, "") @@ -41,13 +50,7 @@ func Test_nativeGitClient_Fetch(t *testing.T) { } func Test_nativeGitClient_Fetch_Prune(t *testing.T) { - tempDir, err := os.MkdirTemp("", "") - require.NoError(t, err) - - err = runCmd(tempDir, "git", "init") - require.NoError(t, err) - - err = runCmd(tempDir, "git", "commit", "-m", "Initial commit", "--allow-empty") + tempDir, err := _createEmptyGitRepo() require.NoError(t, err) client, err := NewClient(fmt.Sprintf("file://%s", tempDir), NopCreds{}, true, false, "")