chore: additional testing, codegen

Signed-off-by: nromriell <nateromriell@gmail.com>

docs/operator-manual/server-commands/argocd-repo-server.md

@@ -20,6 +20,7 @@ argocd-repo-server [flags]
       --default-cache-expiration duration              Cache expiration default (default 24h0m0s)
       --disable-helm-manifest-max-extracted-size       Disable maximum size of helm manifest archives when extracted
       --disable-tls                                    Disable TLS on the gRPC endpoint
+      --enable-revision-cache-lock                     Enables a lock to prevent duplicate git requests during revision cache updates
       --helm-manifest-max-extracted-size string        Maximum size of helm manifest archives when extracted (default "1G")
   -h, --help                                           help for argocd-repo-server
       --logformat string                               Set the logging format. One of: text|json (default "text")
@@ -42,6 +43,8 @@ argocd-repo-server [flags]
       --redisdb int                                    Redis database.
       --repo-cache-expiration duration                 Cache expiration for repo state, incl. app lists, app details, manifest generation, revision meta-data (default 24h0m0s)
       --revision-cache-expiration duration             Cache expiration for cached revision (default 3m0s)
+      --revision-cache-lock-timeout duration           Specifies the max amount of time each routine making identical git requests will wait for the git reference cache to update before making a new git requests (default 10s)
+      --revision-cache-lock-wait-interval duration     Specifies the amount of time between each check to see if the git cache refs lock has been released (default 1s)
       --sentinel stringArray                           Redis sentinel hostname and port (e.g. argocd-redis-ha-announce-0:6379). 
       --sentinelmaster string                          Redis sentinel master group name. (default "master")
       --streamed-manifest-max-extracted-size string    Maximum size of streamed manifest archives when extracted (default "1G")

reposerver/cache/cache.go

@@ -224,6 +224,9 @@ func GitRefCacheItemToReferences(cacheItem [][2]string) []*plumbing.Reference {
 
 // GetGitReferences retrieves resolved Git repository references from cache
 func (c *Cache) GetGitReferences(repo string, references *[]*plumbing.Reference) error {
+	if references == nil {
+		return fmt.Errorf("references parameter cannot be nil")
+	}
 	var input [][2]string
 	if err := c.cache.GetItem(gitRefsKey(repo), &input); err != nil {
 		return err
@@ -233,7 +236,7 @@ func (c *Cache) GetGitReferences(repo string, references *[]*plumbing.Reference)
 }
 
 // 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) {
+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
@@ -243,11 +246,7 @@ func (c *Cache) TryLockGitRefCache(repo string, lockId string) {
 		Expiration:       c.revisionCacheLockTimeout,
 		DisableOverwrite: true,
 	}, false)
-	if err != nil {
-		// The key already existing does not produce an error for go-redis so throwing it here for in-memory
-		// would cause inconsistent behavior, use a get on the key to determine if the lock was successful
-		log.Debug("Error setting git references cache lock: ", err)
-	}
+	return err
 }
 
 // GetOrLockGitReferences retrieves the git references if they exist, otherwise creates a lock and returns so the caller can populate the cache
@@ -275,7 +274,11 @@ func (c *Cache) GetOrLockGitReferences(repo string, references *[]*plumbing.Refe
 	// Wait only the maximum amount of time configured for the lock
 	for time.Now().Before(waitUntil) {
 		// Attempt to get the lock
-		c.TryLockGitRefCache(repo, myLockId)
+		err = c.TryLockGitRefCache(repo, myLockId)
+		if err != nil {
+			// Log but ignore this error since we'll want to retry
+			log.Errorf("Error attempting to acquire git references cache lock: %v", err)
+		}
 		err = c.cache.GetItem(gitRefsKey(repo), &input)
 		if err == nil && len(input) > 0 && len(input[0]) > 0 {
 			if input[0][0] != cacheutil.CacheLockedValue {

reposerver/cache/cache_test.go

@@ -8,6 +8,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/go-git/go-git/v5/plumbing"
 	"github.com/spf13/cobra"
 	"github.com/stretchr/testify/assert"
 
@@ -321,6 +322,47 @@ func TestCachedManifestResponse_ShallowCopyExpectedFields(t *testing.T) {
 
 }
 
+func TestGetGitReferences(t *testing.T) {
+	inMemCache := cacheutil.NewInMemoryCache(1 * time.Hour)
+
+	repoCache := NewCache(
+		cacheutil.NewCache(inMemCache),
+		&CacheOpts{
+			RepoCacheExpiration:           1 * time.Minute,
+			RevisionCacheExpiration:       1 * time.Minute,
+			RevisionCacheLockWaitEnabled:  true,
+			RevisionCacheLockTimeout:      10 * time.Second,
+			RevisionCacheLockWaitInterval: 1 * time.Second,
+		},
+	)
+
+	t.Run("Valid args, nothing in cache", func(t *testing.T) {
+		var references []*plumbing.Reference
+		err := repoCache.GetGitReferences("test-repo", &references)
+		assert.Error(t, err)
+		assert.Equal(t, ErrCacheMiss, err)
+		assert.Equal(t, 0, len(references))
+	})
+
+	t.Run("Valid args, value in cache", func(t *testing.T) {
+		var references []*plumbing.Reference
+		err := repoCache.SetGitReferences("test-repo", GitRefCacheItemToReferences([][2]string{{"test-repo", "ref: test"}}))
+		assert.NoError(t, err)
+		err = repoCache.GetGitReferences("test-repo", &references)
+		assert.NoError(t, err)
+		assert.Equal(t, 1, len(references))
+		assert.Equal(t, "test", references[0].Target().String())
+		assert.Equal(t, "test-repo", references[0].Name().String())
+	})
+
+	t.Run("nil reference", func(t *testing.T) {
+		err := repoCache.GetGitReferences("test-repo", nil)
+		assert.Error(t, err)
+		assert.Contains(t, err.Error(), "cannot be nil")
+	})
+
+}
+
 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")
@@ -347,25 +389,95 @@ func TestGitRefCacheItemToReferences_DataChecks(t *testing.T) {
 func TestTryLockGitRefCache_OwnershipFlows(t *testing.T) {
 	cache := newFixtures().Cache
 	// Test setting the lock
-	cache.TryLockGitRefCache("my-repo-url", "my-lock-id")
+	err := cache.TryLockGitRefCache("my-repo-url", "my-lock-id")
+	assert.NoError(t, err)
 	var output [][2]string
 	key := fmt.Sprintf("git-refs|%s", "my-repo-url")
-	err := cache.cache.GetItem(key, &output)
+	err = cache.cache.GetItem(key, &output)
 	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
-	cache.TryLockGitRefCache("my-repo-url", "other-lock-id")
+	err = cache.TryLockGitRefCache("my-repo-url", "other-lock-id")
+	assert.NoError(t, err)
 	err = cache.cache.GetItem(key, &output)
 	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 = cache.cache.SetItem(key, [][2]string{}, 0, true)
 	assert.NoError(t, err)
-	cache.TryLockGitRefCache("my-repo-url", "other-lock-id")
+	err = cache.TryLockGitRefCache("my-repo-url", "other-lock-id")
+	assert.NoError(t, err)
 	err = cache.cache.GetItem(key, &output)
 	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")
 }
+
+func TestGetOrLockGitReferences(t *testing.T) {
+	cache := newFixtures().Cache
+
+	t.Run("Test lock disabled and cache miss", func(t *testing.T) {
+		cache.revisionCacheLockWaitEnabled = false
+		var references []*plumbing.Reference
+		updateCache, lockId, err := cache.GetOrLockGitReferences("test-repo", &references)
+		assert.NoError(t, err)
+		assert.True(t, updateCache)
+		assert.Equal(t, "", lockId)
+		cache.revisionCacheLockWaitEnabled = true
+	})
+
+	t.Run("Test lock enabled and get lock", func(t *testing.T) {
+		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")
+	})
+
+	t.Run("Test lock enabled and cache hit", func(t *testing.T) {
+		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")
+	})
+
+	t.Run("Test cache lock timeout", func(t *testing.T) {
+		// 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
+	})
+}
+
+func TestUnlockGitReferences(t *testing.T) {
+	cache := newFixtures().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)
+	})
+}

reposerver/repository/repository_test.go

@@ -1768,6 +1768,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) {
@@ -3336,3 +3357,39 @@ func TestGetRefs_CacheUnlockedOnUpdateFailed(t *testing.T) {
 		assert.Equal(t, 0, len(output), "Expected cache to be empty for key")
 	}
 }
+
+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")
+	// Test in-memory and redis
+	cacheTypes := []repositorymocks.MockCacheType{repositorymocks.MockCacheTypeInMem, repositorymocks.MockCacheTypeRedis}
+	for _, cacheType := range cacheTypes {
+		repoCache := repositorymocks.NewMockRepoCache(cacheType, &repositorymocks.MockCacheOptions{
+			CacheOpts: cache.CacheOpts{
+				RepoCacheExpiration:           cache.CacheOptsDefaults.RepoCacheExpiration,
+				RevisionCacheExpiration:       cache.CacheOptsDefaults.RevisionCacheExpiration,
+				RevisionCacheLockWaitEnabled:  true,
+				RevisionCacheLockTimeout:      30 * time.Second,
+				RevisionCacheLockWaitInterval: 100 * time.Millisecond,
+			},
+			ReadDelay:  0,
+			WriteDelay: 0,
+			ErrorResponses: map[string]error{
+				"TryLockGitRefCache": fmt.Errorf("test error getting lock"),
+			},
+		})
+		t.Cleanup(repoCache.StopRedisCallback)
+		repoUrl := fmt.Sprintf("file://%s", dir)
+		// buf := bytes.Buffer{}
+		// log.SetOutput(&buf)
+		client, err := git.NewClient(repoUrl, git.NopCreds{}, true, false, "", git.WithCache(repoCache, true))
+		require.NoError(t, err)
+		refs, err := client.LsRefs()
+		assert.NoError(t, err)
+		assert.NotNil(t, refs)
+	}
+}