From 38596a2a6455794f5a22c4611508f3eeb930ce6b Mon Sep 17 00:00:00 2001 From: Michael Crenshaw Date: Thu, 24 Mar 2022 22:25:30 -0400 Subject: [PATCH 1/7] fix: prevent excessive repo-server disk usage for large repos Signed-off-by: Michael Crenshaw --- reposerver/repository/repository.go | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/reposerver/repository/repository.go b/reposerver/repository/repository.go index 9c897af05569..3c41fbbbdf53 100644 --- a/reposerver/repository/repository.go +++ b/reposerver/repository/repository.go @@ -1683,25 +1683,28 @@ func (s *Service) checkoutRevision(gitClient git.Client, revision string, submod return closer, status.Errorf(codes.Internal, "Failed to initialize git repo: %v", err) } - err = gitClient.Fetch(revision) + // Fetching with no revision first. Fetching with an explicit version can cause repo bloat. https://github.com/argoproj/argo-cd/issues/8845 + err = gitClient.Fetch("") + if err != nil { + return closer, status.Errorf(codes.Internal, "Failed to fetch default: %v", err) + } + err = gitClient.Checkout(revision, submoduleEnabled) if err != nil { - log.Infof("Failed to fetch revision %s: %v", revision, err) - log.Infof("Fallback to fetch default") - err = gitClient.Fetch("") - if err != nil { - return closer, status.Errorf(codes.Internal, "Failed to fetch default: %v", err) - } - err = gitClient.Checkout(revision, submoduleEnabled) + // When fetching with no revision, only refs/heads/* and refs/remotes/origin/* are fetched. If checkout fails + // for the given revision, try explicitly fetching it. + log.Infof("Failed to checkout revision %s: %v", revision, err) + log.Infof("Fallback to fetching specific revision %s. ref might not have been in the default refspec fetched.", revision) + + err = gitClient.Fetch(revision) if err != nil { return closer, status.Errorf(codes.Internal, "Failed to checkout revision %s: %v", revision, err) } - return closer, err - } - err = gitClient.Checkout("FETCH_HEAD", submoduleEnabled) - if err != nil { - return closer, status.Errorf(codes.Internal, "Failed to checkout FETCH_HEAD: %v", err) + err = gitClient.Checkout("FETCH_HEAD", submoduleEnabled) + if err != nil { + return closer, status.Errorf(codes.Internal, "Failed to checkout FETCH_HEAD: %v", err) + } } return closer, err From 6618e7a41d733a5748a44af2ef6857eb145f3104 Mon Sep 17 00:00:00 2001 From: Michael Crenshaw Date: Fri, 25 Mar 2022 10:02:09 -0400 Subject: [PATCH 2/7] fix: add test Signed-off-by: Michael Crenshaw --- reposerver/repository/repository.go | 14 +++++--- reposerver/repository/repository_test.go | 45 ++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 5 deletions(-) diff --git a/reposerver/repository/repository.go b/reposerver/repository/repository.go index 3c41fbbbdf53..f6fc8aa859c6 100644 --- a/reposerver/repository/repository.go +++ b/reposerver/repository/repository.go @@ -1678,15 +1678,19 @@ func directoryPermissionInitializer(rootPath string) goio.Closer { // nolint:unparam func (s *Service) checkoutRevision(gitClient git.Client, revision string, submoduleEnabled bool) (goio.Closer, error) { closer := s.gitRepoInitializer(gitClient.Root()) + return closer, checkoutRevision(gitClient, revision, submoduleEnabled) +} + +func checkoutRevision(gitClient git.Client, revision string, submoduleEnabled bool) error { err := gitClient.Init() if err != nil { - return closer, status.Errorf(codes.Internal, "Failed to initialize git repo: %v", err) + return status.Errorf(codes.Internal, "Failed to initialize git repo: %v", err) } // Fetching with no revision first. Fetching with an explicit version can cause repo bloat. https://github.com/argoproj/argo-cd/issues/8845 err = gitClient.Fetch("") if err != nil { - return closer, status.Errorf(codes.Internal, "Failed to fetch default: %v", err) + return status.Errorf(codes.Internal, "Failed to fetch default: %v", err) } err = gitClient.Checkout(revision, submoduleEnabled) @@ -1698,16 +1702,16 @@ func (s *Service) checkoutRevision(gitClient git.Client, revision string, submod err = gitClient.Fetch(revision) if err != nil { - return closer, status.Errorf(codes.Internal, "Failed to checkout revision %s: %v", revision, err) + return status.Errorf(codes.Internal, "Failed to checkout revision %s: %v", revision, err) } err = gitClient.Checkout("FETCH_HEAD", submoduleEnabled) if err != nil { - return closer, status.Errorf(codes.Internal, "Failed to checkout FETCH_HEAD: %v", err) + return status.Errorf(codes.Internal, "Failed to checkout FETCH_HEAD: %v", err) } } - return closer, err + return err } func (s *Service) GetHelmCharts(ctx context.Context, q *apiclient.HelmChartsRequest) (*apiclient.HelmChartsResponse, error) { diff --git a/reposerver/repository/repository_test.go b/reposerver/repository/repository_test.go index a3949564db0a..49c5ec9a30aa 100644 --- a/reposerver/repository/repository_test.go +++ b/reposerver/repository/repository_test.go @@ -1779,3 +1779,48 @@ func TestInit(t *testing.T) { require.Error(t, err) require.NoError(t, initGitRepo(path.Join(dir, "repo2"), "https://github.com/argo-cd/test-repo2")) } + +// TestCheckoutRevisionCanGetNonstandardRefs shows that we can fetch a revision that points to a non-standard ref. In +// other words, we haven't regressed and caused this issue again: https://github.com/argoproj/argo-cd/issues/4935 +func TestCheckoutRevision(t *testing.T) { + rootPath, err := ioutil.TempDir("", "") + require.NoError(t, err) + + sourceRepoPath, err := ioutil.TempDir(rootPath, "") + require.NoError(t, err) + + // Create a repo such that one commit is on a non-standard ref _and nowhere else_. This is meant to simulate, for + // example, a GitHub ref for a pull into one repo from a fork of that repo. + run(t, sourceRepoPath, "git", "init") + run(t, sourceRepoPath, "git", "checkout", "-b", "main") // make sure there's a main branch to switch back to + run(t, sourceRepoPath, "git", "commit", "-m", "empty", "--allow-empty") + run(t, sourceRepoPath, "git", "checkout", "-b", "branch") + run(t, sourceRepoPath, "git", "commit", "-m", "empty", "--allow-empty") + sha := run(t, sourceRepoPath, "git", "rev-parse", "HEAD") + run(t, sourceRepoPath, "git", "update-ref", "refs/pull/123/head", strings.TrimSuffix(sha, "\n")) + run(t, sourceRepoPath, "git", "checkout", "main") + run(t, sourceRepoPath, "git", "branch", "-D", "branch") + + destRepoPath, err := ioutil.TempDir(rootPath, "") + require.NoError(t, err) + + gitClient, err := git.NewClientExt("file://"+sourceRepoPath, destRepoPath, &git.NopCreds{}, true, false, "") + require.NoError(t, err) + + pullSha, err := gitClient.LsRemote("refs/pull/123/head") + require.NoError(t, err) + + err = checkoutRevision(gitClient, pullSha, false) + require.NoError(t, err) +} + +// run runs a command in the given working directory. If the command succeeds, it returns the combined standard and +// error output. If it fails, it stops the test with a failure message. +func run(t *testing.T, workDir string, command string, args ...string) string { + cmd := exec.Command(command, args...) + cmd.Dir = workDir + out, err := cmd.CombinedOutput() + stringOut := string(out) + require.NoError(t, err, stringOut) + return stringOut +} From 35b9e273835e806324384b25357a1bfc76905546 Mon Sep 17 00:00:00 2001 From: Michael Crenshaw Date: Fri, 25 Mar 2022 10:05:13 -0400 Subject: [PATCH 3/7] chore: lint Signed-off-by: Michael Crenshaw --- reposerver/repository/repository_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/reposerver/repository/repository_test.go b/reposerver/repository/repository_test.go index 49c5ec9a30aa..daa45f4dc3d9 100644 --- a/reposerver/repository/repository_test.go +++ b/reposerver/repository/repository_test.go @@ -1792,7 +1792,7 @@ func TestCheckoutRevision(t *testing.T) { // Create a repo such that one commit is on a non-standard ref _and nowhere else_. This is meant to simulate, for // example, a GitHub ref for a pull into one repo from a fork of that repo. run(t, sourceRepoPath, "git", "init") - run(t, sourceRepoPath, "git", "checkout", "-b", "main") // make sure there's a main branch to switch back to + run(t, sourceRepoPath, "git", "checkout", "-b", "main") // make sure there's a main branch to switch back to run(t, sourceRepoPath, "git", "commit", "-m", "empty", "--allow-empty") run(t, sourceRepoPath, "git", "checkout", "-b", "branch") run(t, sourceRepoPath, "git", "commit", "-m", "empty", "--allow-empty") From 03d93c0dcb1cb118df38619a97b4707dd6ca8553 Mon Sep 17 00:00:00 2001 From: Michael Crenshaw Date: Fri, 25 Mar 2022 10:55:30 -0400 Subject: [PATCH 4/7] fix: test that checking out a fake commit fails Signed-off-by: Michael Crenshaw --- reposerver/repository/repository_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/reposerver/repository/repository_test.go b/reposerver/repository/repository_test.go index daa45f4dc3d9..840b82a5d848 100644 --- a/reposerver/repository/repository_test.go +++ b/reposerver/repository/repository_test.go @@ -1810,8 +1810,11 @@ func TestCheckoutRevision(t *testing.T) { pullSha, err := gitClient.LsRemote("refs/pull/123/head") require.NoError(t, err) + err = checkoutRevision(gitClient, "does-not-exist", false) + assert.Error(t, err) + err = checkoutRevision(gitClient, pullSha, false) - require.NoError(t, err) + assert.NoError(t, err) } // run runs a command in the given working directory. If the command succeeds, it returns the combined standard and From 8ea0cba5ac73af1cc756ea6cecbb4e17ef3f0047 Mon Sep 17 00:00:00 2001 From: Michael Crenshaw Date: Fri, 25 Mar 2022 11:01:44 -0400 Subject: [PATCH 5/7] chore: fix test name Signed-off-by: Michael Crenshaw --- reposerver/repository/repository_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/reposerver/repository/repository_test.go b/reposerver/repository/repository_test.go index 840b82a5d848..e5e54d045a70 100644 --- a/reposerver/repository/repository_test.go +++ b/reposerver/repository/repository_test.go @@ -1782,7 +1782,7 @@ func TestInit(t *testing.T) { // TestCheckoutRevisionCanGetNonstandardRefs shows that we can fetch a revision that points to a non-standard ref. In // other words, we haven't regressed and caused this issue again: https://github.com/argoproj/argo-cd/issues/4935 -func TestCheckoutRevision(t *testing.T) { +func TestCheckoutRevisionCanGetNonstandardRefs(t *testing.T) { rootPath, err := ioutil.TempDir("", "") require.NoError(t, err) From 70dad90641aac855c47bfff90ece195a59dfa5e8 Mon Sep 17 00:00:00 2001 From: Michael Crenshaw Date: Fri, 25 Mar 2022 11:56:33 -0400 Subject: [PATCH 6/7] chore: fix lint Signed-off-by: Michael Crenshaw --- reposerver/repository/repository_test.go | 26 ++++++++++++------------ 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/reposerver/repository/repository_test.go b/reposerver/repository/repository_test.go index e5e54d045a70..a17d1ce4b883 100644 --- a/reposerver/repository/repository_test.go +++ b/reposerver/repository/repository_test.go @@ -1791,15 +1791,15 @@ func TestCheckoutRevisionCanGetNonstandardRefs(t *testing.T) { // Create a repo such that one commit is on a non-standard ref _and nowhere else_. This is meant to simulate, for // example, a GitHub ref for a pull into one repo from a fork of that repo. - run(t, sourceRepoPath, "git", "init") - run(t, sourceRepoPath, "git", "checkout", "-b", "main") // make sure there's a main branch to switch back to - run(t, sourceRepoPath, "git", "commit", "-m", "empty", "--allow-empty") - run(t, sourceRepoPath, "git", "checkout", "-b", "branch") - run(t, sourceRepoPath, "git", "commit", "-m", "empty", "--allow-empty") - sha := run(t, sourceRepoPath, "git", "rev-parse", "HEAD") - run(t, sourceRepoPath, "git", "update-ref", "refs/pull/123/head", strings.TrimSuffix(sha, "\n")) - run(t, sourceRepoPath, "git", "checkout", "main") - run(t, sourceRepoPath, "git", "branch", "-D", "branch") + runGit(t, sourceRepoPath, "init") + runGit(t, sourceRepoPath, "checkout", "-b", "main") // make sure there's a main branch to switch back to + runGit(t, sourceRepoPath, "commit", "-m", "empty", "--allow-empty") + runGit(t, sourceRepoPath, "checkout", "-b", "branch") + runGit(t, sourceRepoPath, "commit", "-m", "empty", "--allow-empty") + sha := runGit(t, sourceRepoPath, "rev-parse", "HEAD") + runGit(t, sourceRepoPath, "update-ref", "refs/pull/123/head", strings.TrimSuffix(sha, "\n")) + runGit(t, sourceRepoPath, "checkout", "main") + runGit(t, sourceRepoPath, "branch", "-D", "branch") destRepoPath, err := ioutil.TempDir(rootPath, "") require.NoError(t, err) @@ -1817,10 +1817,10 @@ func TestCheckoutRevisionCanGetNonstandardRefs(t *testing.T) { assert.NoError(t, err) } -// run runs a command in the given working directory. If the command succeeds, it returns the combined standard and -// error output. If it fails, it stops the test with a failure message. -func run(t *testing.T, workDir string, command string, args ...string) string { - cmd := exec.Command(command, args...) +// runGit runs a git command in the given working directory. If the command succeeds, it returns the combined standard +// and error output. If it fails, it stops the test with a failure message. +func runGit(t *testing.T, workDir string, args ...string) string { + cmd := exec.Command("git", args...) cmd.Dir = workDir out, err := cmd.CombinedOutput() stringOut := string(out) From a595331998f21794ebe7252f86f709013f357e29 Mon Sep 17 00:00:00 2001 From: Michael Crenshaw Date: Fri, 25 Mar 2022 13:04:24 -0400 Subject: [PATCH 7/7] chore: start CI Signed-off-by: Michael Crenshaw