diff --git a/reposerver/cache/cache.go b/reposerver/cache/cache.go index 4437bd3ac0dd7..36a7cd9679f9e 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" log "github.com/sirupsen/logrus" "github.com/spf13/cobra" @@ -24,11 +25,13 @@ import ( ) 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 @@ -40,7 +43,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 ...cacheutil.Options) func() (*Cache, error) { @@ -140,12 +143,17 @@ 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, nil) 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}) } func helmIndexRefsKey(repo string) string { @@ -154,12 +162,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 { @@ -172,21 +187,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}) +} + +// Converts raw cache items to plumbing.Reference objects +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}) + return err +} + +func (c *Cache) GetGitReferences(repo string, lockId string) (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{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); 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); 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}) + } + return err } // refSourceCommitSHAs is a list of resolved revisions for each ref source. This allows us to invalidate the cache @@ -231,7 +333,7 @@ func (c *Cache) SetNewRevisionManifests(newRevision string, revision string, app } 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, nil) if err != nil { return err @@ -274,11 +376,19 @@ 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}) } 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}) } func appDetailsCacheKey(revision string, appSrc *appv1.ApplicationSource, srcRefs appv1.RefTargetRevisionMapping, trackingMethod appv1.TrackingMethod, refSourceCommitSHAs ResolvedRevisions) string { @@ -289,11 +399,16 @@ 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, nil) } 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}) } func revisionMetadataKey(repoURL, revision string) string { @@ -302,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 { @@ -315,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 { @@ -327,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}) } 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, nil) } func gitDirectoriesKey(repoURL, revision string) string { @@ -340,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..e1e25c748f24b 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.RedisClient) + 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.RedisClient.AssertCalled(t, "Get", mock.Anything, 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: 4}) } 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,426 @@ 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") + 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, 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") + 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") + 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, ExternalGets: 1}) + }) + + t.Run("cache error", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + cache := fixtures.cache + fixtures.mockCache.RedisClient.On("Get", mock.Anything, mock.Anything).Unset() + fixtures.mockCache.RedisClient.On("Get", mock.Anything, mock.Anything).Return(errors.New("test cache error")) + lockOwner, references, err := cache.GetGitReferences("test-repo", "test-lock-id") + 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{ExternalGets: 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: 2}) + 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: 2, ExternalDeletes: 1}) + assert.NoError(t, err) + err = cache.TryLockGitRefCache("my-repo-url", "other-lock-id") + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalSets: 3, ExternalGets: 2, ExternalDeletes: 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: 3, ExternalDeletes: 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: 2}) + }) + + 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, ExternalGets: 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, ExternalGets: 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.RedisClient.On("Get", mock.Anything, mock.Anything).Unset() + fixtures.mockCache.RedisClient.On("Get", mock.Anything, 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, 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, ExternalGets: 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.RedisClient.On("Set", mock.Anything).Unset() + fixtures.mockCache.RedisClient.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.RedisClient.AssertNumberOfCalls(t, "Set", 2) + fixtures.mockCache.RedisClient.AssertNumberOfCalls(t, "Get", 4) + }) +} + +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}) + 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}) + 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("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{ExternalGets: 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}) + 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}) + 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("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{ExternalGets: 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}) + 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/repository/repository_test.go b/reposerver/repository/repository_test.go index 3f2f74c4e5ae0..db9c4d1a6d33a 100644 --- a/reposerver/repository/repository_test.go +++ b/reposerver/repository/repository_test.go @@ -15,6 +15,7 @@ import ( "regexp" "sort" "strings" + "sync" "testing" "time" @@ -30,6 +31,7 @@ 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" @@ -385,11 +387,11 @@ func TestGenerateManifest_RefOnlyShortCircuit(t *testing.T) { _, err := service.GenerateManifest(context.Background(), &q) assert.NoError(t, err) cacheMocks.mockCache.AssertCacheCalledTimes(t, &repositorymocks.CacheCallCounts{ - ExternalSets: 1, - ExternalGets: 1}) + ExternalSets: 2, + ExternalGets: 2}) assert.True(t, lsremoteCalled, "ls-remote should be called when the source is ref only") var revisions [][2]string - assert.NoError(t, cacheMocks.cacheutilCache.GetItem(fmt.Sprintf("git-refs|%s", repoRemote), &revisions)) + assert.NoError(t, cacheMocks.cacheutilCache.GetItem(fmt.Sprintf("git-refs|%s", repoRemote), &revisions, nil)) assert.ElementsMatch(t, [][2]string{{"refs/heads/main", revision}, {"HEAD", "ref: refs/heads/main"}}, revisions) } @@ -451,7 +453,7 @@ func TestGenerateManifestsHelmWithRefs_CachedNoLsRemote(t *testing.T) { ProjectSourceRepos: []string{"*"}, RefSources: map[string]*argoappv1.RefTarget{"$ref": {TargetRevision: "HEAD", Repo: *repo}}, } - err = cacheMocks.cacheutilCache.SetItem(fmt.Sprintf("git-refs|%s", repoRemote), [][2]string{{"HEAD", revision}}, 30*time.Second, false) + err = cacheMocks.cacheutilCache.SetItem(fmt.Sprintf("git-refs|%s", repoRemote), [][2]string{{"HEAD", revision}}, nil) assert.NoError(t, err) _, err = service.GenerateManifest(context.Background(), &q) assert.NoError(t, err) @@ -3365,6 +3367,113 @@ func Test_getRepoSanitizerRegex(t *testing.T) { 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() + initGitRepo(t, newGitRepoOptions{ + path: dir, + createPath: false, + remote: "", + addEmptyCommit: true, + }) + 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() + initGitRepo(t, newGitRepoOptions{ + path: dir, + createPath: false, + remote: "", + addEmptyCommit: true, + }) + 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() + initGitRepo(t, newGitRepoOptions{ + path: dir, + createPath: false, + remote: "", + addEmptyCommit: true, + }) + 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() + initGitRepo(t, newGitRepoOptions{ + path: dir, + createPath: false, + remote: "", + addEmptyCommit: true, + }) + 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() diff --git a/util/cache/appstate/cache.go b/util/cache/appstate/cache.go index bb161a429eff9..5d2d7dddb3a72 100644 --- a/util/cache/appstate/cache.go +++ b/util/cache/appstate/cache.go @@ -45,11 +45,11 @@ func AddCacheFlagsToCmd(cmd *cobra.Command, opts ...cacheutil.Options) func() (* } 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 d34fba5d38f7b..a073c90ee8df5 100644 --- a/util/cache/cache.go +++ b/util/cache/cache.go @@ -15,6 +15,7 @@ import ( certutil "github.com/argoproj/argo-cd/v2/util/cert" "github.com/argoproj/argo-cd/v2/util/env" "github.com/redis/go-redis/v9" + log "github.com/sirupsen/logrus" "github.com/spf13/cobra" ) @@ -244,30 +245,46 @@ func (c *Cache) RenameItem(oldKey string, newKey string, expiration time.Duratio return c.client.Rename(fmt.Sprintf("%s|%s", oldKey, common.CacheVersion), fmt.Sprintf("%s|%s", newKey, common.CacheVersion), expiration) } -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(key, item) } 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..a2a225bc8b9f2 100644 --- a/util/cache/cache_test.go +++ b/util/cache/cache_test.go @@ -1,9 +1,13 @@ 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" ) @@ -14,33 +18,73 @@ func TestAddCacheFlagsToCmd(t *testing.T) { assert.Equal(t, 24*time.Hour, cache.client.(*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 c8c7b4a6baa80..20cf7a910d76c 100644 --- a/util/cache/client.go +++ b/util/cache/client.go @@ -7,10 +7,20 @@ import ( ) var ErrCacheMiss = errors.New("cache: key is missing") +var ErrCacheKeyLocked = errors.New("cache: key is locked") +var CacheLockedValue = "locked" type Item struct { Key string Object interface{} + CacheActionOpts +} + +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 } diff --git a/util/cache/inmemory.go b/util/cache/inmemory.go index 6d970c1d4f567..4939a08425f4a 100644 --- a/util/cache/inmemory.go +++ b/util/cache/inmemory.go @@ -33,7 +33,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 } diff --git a/util/cache/redis.go b/util/cache/redis.go index 4648a553f08cc..2a8c6c5a39be7 100644 --- a/util/cache/redis.go +++ b/util/cache/redis.go @@ -115,6 +115,7 @@ func (r *redisCache) Set(item *Item) error { Key: r.getKey(item.Key), Value: val, TTL: expiration, + SetNX: item.DisableOverwrite, }) } diff --git a/util/git/client.go b/util/git/client.go index 73c85b54f3c1f..289a179832dce 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, "") diff --git a/util/oidc/oidc.go b/util/oidc/oidc.go index 2c376cc7e5b5b..b5c80d25e8384 100644 --- a/util/oidc/oidc.go +++ b/util/oidc/oidc.go @@ -398,9 +398,11 @@ func (a *ClientApp) HandleCallback(w http.ResponseWriter, r *http.Request) { } sub := jwtutil.StringField(claims, "sub") err = a.clientCache.Set(&cache.Item{ - Key: formatAccessTokenCacheKey(AccessTokenCachePrefix, sub), - Object: encToken, - Expiration: getTokenExpiration(claims), + Key: formatAccessTokenCacheKey(AccessTokenCachePrefix, sub), + Object: encToken, + CacheActionOpts: cache.CacheActionOpts{ + Expiration: getTokenExpiration(claims), + }, }) if err != nil { claimsJSON, _ := json.Marshal(claims) @@ -654,9 +656,11 @@ func (a *ClientApp) GetUserInfo(actualClaims jwt.MapClaims, issuerURL, userInfoP } err = a.clientCache.Set(&cache.Item{ - Key: clientCacheKey, - Object: encClaims, - Expiration: cacheExpiry, + Key: clientCacheKey, + Object: encClaims, + CacheActionOpts: cache.CacheActionOpts{ + Expiration: cacheExpiry, + }, }) if err != nil { return claims, false, fmt.Errorf("couldn't put item to cache: %w", err)