From f7d89143acae679bdcbd0358d9050d9f13649ea9 Mon Sep 17 00:00:00 2001 From: nromriell Date: Mon, 13 Nov 2023 01:39:47 -0800 Subject: [PATCH] chore: additional testing, codegen Signed-off-by: nromriell --- .../server-commands/argocd-repo-server.md | 3 + reposerver/cache/cache.go | 17 ++- reposerver/cache/cache_test.go | 120 +++++++++++++++++- reposerver/repository/repository_test.go | 57 +++++++++ util/git/client_test.go | 100 +++++++++++++-- 5 files changed, 275 insertions(+), 22 deletions(-) diff --git a/docs/operator-manual/server-commands/argocd-repo-server.md b/docs/operator-manual/server-commands/argocd-repo-server.md index 33ecaf7c76dd4..610f213d6146a 100644 --- a/docs/operator-manual/server-commands/argocd-repo-server.md +++ b/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") diff --git a/reposerver/cache/cache.go b/reposerver/cache/cache.go index db1de48a61443..ed216ccc4bffe 100644 --- a/reposerver/cache/cache.go +++ b/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 { diff --git a/reposerver/cache/cache_test.go b/reposerver/cache/cache_test.go index d6b9cfb0f5d09..6f072516b7147 100644 --- a/reposerver/cache/cache_test.go +++ b/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,15 +389,17 @@ 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") @@ -363,9 +407,77 @@ func TestTryLockGitRefCache_OwnershipFlows(t *testing.T) { // 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) + }) +} diff --git a/reposerver/repository/repository_test.go b/reposerver/repository/repository_test.go index a5e1b155cf86d..8c50c7d9526fd 100644 --- a/reposerver/repository/repository_test.go +++ b/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) + } +} diff --git a/util/git/client_test.go b/util/git/client_test.go index 6e91868549f3e..b7214c1626f04 100644 --- a/util/git/client_test.go +++ b/util/git/client_test.go @@ -7,7 +7,10 @@ import ( "path" "path/filepath" "testing" + "time" + cacheutil "github.com/argoproj/argo-cd/v2/util/cache" + "github.com/go-git/go-git/v5/plumbing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -20,23 +23,19 @@ func runCmd(workingDir string, name string, args ...string) error { return cmd.Run() } -func _createEmptyGitRepo() (string, error) { - tempDir, err := os.MkdirTemp("", "") +func _createEmptyGitRepo(tempDir string) error { + err := runCmd(tempDir, "git", "init") if err != nil { - return tempDir, err - } - - err = runCmd(tempDir, "git", "init") - if err != nil { - return tempDir, err + return err } err = runCmd(tempDir, "git", "commit", "-m", "Initial commit", "--allow-empty") - return tempDir, err + return err } func Test_nativeGitClient_Fetch(t *testing.T) { - tempDir, err := _createEmptyGitRepo() + tempDir := t.TempDir() + err := _createEmptyGitRepo(tempDir) require.NoError(t, err) client, err := NewClient(fmt.Sprintf("file://%s", tempDir), NopCreds{}, true, false, "") @@ -50,7 +49,8 @@ func Test_nativeGitClient_Fetch(t *testing.T) { } func Test_nativeGitClient_Fetch_Prune(t *testing.T) { - tempDir, err := _createEmptyGitRepo() + tempDir := t.TempDir() + err := _createEmptyGitRepo(tempDir) require.NoError(t, err) client, err := NewClient(fmt.Sprintf("file://%s", tempDir), NopCreds{}, true, false, "") @@ -206,3 +206,81 @@ func TestNewClient_invalidSSHURL(t *testing.T) { assert.Nil(t, client) assert.ErrorIs(t, err, ErrInvalidRepoURL) } + +type MockCache struct { + cache *cacheutil.Cache + setResponse error + getResponse error + getOrLockResponse [3]interface{} + unlockResponse error +} + +func (m *MockCache) SetGitReferences(repo string, references []*plumbing.Reference) error { + return m.setResponse +} +func (m *MockCache) GetGitReferences(repo string, references *[]*plumbing.Reference) error { + return m.getResponse +} +func (m *MockCache) GetOrLockGitReferences(repo string, references *[]*plumbing.Reference) (updateCache bool, lockId string, err error) { + err = nil + if m.getOrLockResponse[2] != nil { + err = m.getOrLockResponse[2].(error) + } + return m.getOrLockResponse[0].(bool), m.getOrLockResponse[1].(string), err +} +func (m *MockCache) UnlockGitReferences(repo string, lockId string) error { + return m.unlockResponse +} + +func TestGetRefs(t *testing.T) { + tempDir := t.TempDir() + err := _createEmptyGitRepo(tempDir) + assert.NoError(t, err) + t.Run("cache disabled", func(t *testing.T) { + client, err := NewClient(fmt.Sprintf("file://%s", tempDir), NopCreds{}, true, false, "") + assert.NoError(t, err) + gitClient := client.(*nativeGitClient) + refs, err := gitClient.getRefs() + assert.NoError(t, err) + assert.NotEmpty(t, refs) + }) + testCache := cacheutil.NewCache(cacheutil.NewInMemoryCache(1 * time.Hour)) + t.Run("cache enabled, get lock", func(t *testing.T) { + mockCache := MockCache{ + cache: testCache, + getOrLockResponse: [3]interface{}{true, "lock", nil}, + } + client, err := NewClient(fmt.Sprintf("file://%s", tempDir), NopCreds{}, true, false, "", WithCache(&mockCache, true)) + assert.NoError(t, err) + gitClient := client.(*nativeGitClient) + refs, err := gitClient.getRefs() + assert.NoError(t, err) + assert.NotEmpty(t, refs) + }) + + t.Run("cache enabled, get cache hit", func(t *testing.T) { + mockCache := MockCache{ + cache: testCache, + getOrLockResponse: [3]interface{}{false, "lock", nil}, + } + client, err := NewClient(fmt.Sprintf("file://%s", tempDir), NopCreds{}, true, false, "", WithCache(&mockCache, true)) + assert.NoError(t, err) + gitClient := client.(*nativeGitClient) + refs, err := gitClient.getRefs() + assert.NoError(t, err) + assert.Empty(t, refs) + }) + + t.Run("cache enabled, get error", func(t *testing.T) { + mockCache := MockCache{ + cache: testCache, + getOrLockResponse: [3]interface{}{false, "lock", fmt.Errorf("test failure")}, + } + client, err := NewClient(fmt.Sprintf("file://%s", tempDir), NopCreds{}, true, false, "", WithCache(&mockCache, true)) + assert.NoError(t, err) + gitClient := client.(*nativeGitClient) + refs, err := gitClient.getRefs() + assert.Error(t, err) + assert.Empty(t, refs) + }) +}