chore: move access checks from api server to repo server (#5940)

* chore: move api checks to reposerver

Signed-off-by: Ishita Sequeira <isequeir@redhat.com>

* resolving merge conflicts

Signed-off-by: Ishita Sequeira <isequeir@redhat.com>

* Address PR comments

Signed-off-by: Ishita Sequeira <isequeir@redhat.com>

reposerver/repository/repository.go

@@ -1372,3 +1372,35 @@ func (s *Service) GetHelmCharts(ctx context.Context, q *apiclient.HelmChartsRequ
 	}
 	return &res, nil
 }
+
+func (s *Service) TestRepository(ctx context.Context, q *apiclient.TestRepositoryRequest) (*apiclient.TestRepositoryResponse, error) {
+	repo := q.Repo
+	checks := map[string]func() error{
+		"git": func() error {
+			return git.TestRepo(repo.Repo, repo.GetGitCreds(), repo.IsInsecure(), repo.IsLFSEnabled())
+		},
+		"helm": func() error {
+			if repo.EnableOCI {
+				if !helm.IsHelmOciRepo(repo.Repo) {
+					return errors.New("OCI Helm repository URL should include hostname and port only")
+				}
+				_, err := helm.NewClient(repo.Repo, repo.GetHelmCreds(), repo.EnableOCI).TestHelmOCI()
+				return err
+			} else {
+				_, err := helm.NewClient(repo.Repo, repo.GetHelmCreds(), repo.EnableOCI).GetIndex(false)
+				return err
+			}
+		},
+	}
+	if check, ok := checks[repo.Type]; ok {
+		return &apiclient.TestRepositoryResponse{VerifiedRepository: false}, check()
+	}
+	var err error
+	for _, check := range checks {
+		err = check()
+		if err == nil {
+			return &apiclient.TestRepositoryResponse{VerifiedRepository: true}, nil
+		}
+	}
+	return &apiclient.TestRepositoryResponse{VerifiedRepository: false}, err
+}

reposerver/repository/repository.proto

@@ -28,6 +28,17 @@ message ManifestRequest {
     bool verifySignature = 16;
 }
 
+// TestRepositoryRequest is a query to test repository is valid or not and has valid access.
+message TestRepositoryRequest {
+    github.com.argoproj.argo_cd.v2.pkg.apis.application.v1alpha1.Repository repo = 1;
+}
+
+// TestRepositoryRequest is a query to test repository is valid or not and has valid access.
+message TestRepositoryResponse {
+    // Request to verify the signature when generating the manifests (only for Git repositories)
+    bool verifiedRepository = 1;
+}
+
 message ManifestResponse {
     repeated string manifests = 1;
     string namespace = 2;
@@ -154,6 +165,10 @@ service RepoServerService {
     rpc GenerateManifest(ManifestRequest) returns (ManifestResponse) {
     }
 
+    // Returns a bool val if the repository is valid and has proper access
+    rpc TestRepository(TestRepositoryRequest) returns (TestRepositoryResponse) {
+    }
+
     // Returns a list of refs (eg. branches and tags) in the repo
     rpc ListRefs(ListRefsRequest) returns (Refs) {
     }

reposerver/repository/repository_test.go

@@ -1467,3 +1467,16 @@ func TestFindManifests_Exclude_NothingMatches(t *testing.T) {
 	assert.ElementsMatch(t,
 		[]string{"nginx-deployment", "nginx-deployment-sub"}, []string{objs[0].GetName(), objs[1].GetName()})
 }
+
+func TestTestRepoOCI(t *testing.T) {
+	service := newService(".")
+	_, err := service.TestRepository(context.Background(), &apiclient.TestRepositoryRequest{
+		Repo: &argoappv1.Repository{
+			Repo:      "https://demo.goharbor.io",
+			Type:      "helm",
+			EnableOCI: true,
+		},
+	})
+	assert.Error(t, err)
+	assert.Equal(t, "OCI Helm repository URL should include hostname and port only", err.Error())
+}

server/application/application_test.go

@@ -100,6 +100,7 @@ func newTestAppServer(objects ...runtime.Object) *Server {
 	mockRepoServiceClient.On("ListApps", mock.Anything, mock.Anything).Return(fakeAppList(), nil)
 	mockRepoServiceClient.On("GenerateManifest", mock.Anything, mock.Anything).Return(&apiclient.ManifestResponse{}, nil)
 	mockRepoServiceClient.On("GetAppDetails", mock.Anything, mock.Anything).Return(&apiclient.RepoAppDetailsResponse{}, nil)
+	mockRepoServiceClient.On("TestRepository", mock.Anything, mock.Anything).Return(&apiclient.TestRepositoryResponse{}, nil)
 
 	mockRepoClient := &mocks.Clientset{RepoServerServiceClient: &mockRepoServiceClient}
 

server/repository/repository.go

@@ -18,7 +18,6 @@ import (
 	"github.com/argoproj/argo-cd/v2/reposerver/apiclient"
 	servercache "github.com/argoproj/argo-cd/v2/server/cache"
 	"github.com/argoproj/argo-cd/v2/server/rbacpolicy"
-	"github.com/argoproj/argo-cd/v2/util/argo"
 	"github.com/argoproj/argo-cd/v2/util/db"
 	"github.com/argoproj/argo-cd/v2/util/errors"
 	"github.com/argoproj/argo-cd/v2/util/io"
@@ -69,7 +68,7 @@ func (s *Server) getConnectionState(ctx context.Context, url string, forceRefres
 	var err error
 	repo, err := s.db.GetRepository(ctx, url)
 	if err == nil {
-		err = argo.TestRepo(repo)
+		err = s.testRepo(ctx, repo)
 	}
 	if err != nil {
 		connectionState.Status = appsv1.ConnectionStatusFailed
@@ -295,7 +294,8 @@ func (s *Server) CreateRepository(ctx context.Context, q *repositorypkg.RepoCrea
 			}
 			repo.CopyCredentialsFrom(creds)
 		}
-		err = argo.TestRepo(repo)
+
+		err = s.testRepo(ctx, repo)
 		if err != nil {
 			return nil, err
 		}
@@ -405,9 +405,22 @@ func (s *Server) ValidateAccess(ctx context.Context, q *repositorypkg.RepoAccess
 			repo.CopyCredentialsFrom(repoCreds)
 		}
 	}
-	err = argo.TestRepo(repo)
+	err = s.testRepo(ctx, repo)
 	if err != nil {
 		return nil, err
 	}
 	return &repositorypkg.RepoResponse{}, nil
 }
+
+func (s *Server) testRepo(ctx context.Context, repo *appsv1.Repository) error {
+	conn, repoClient, err := s.repoClientset.NewRepoServerClient()
+	if err != nil {
+		return err
+	}
+	defer io.Close(conn)
+
+	_, err = repoClient.TestRepository(ctx, &apiclient.TestRepositoryRequest{
+		Repo: repo,
+	})
+	return err
+}

util/argo/argo.go

@@ -3,7 +3,6 @@ package argo
 import (
 	"context"
 	"encoding/json"
-	"errors"
 	"fmt"
 	"strings"
 	"time"
@@ -25,8 +24,6 @@ import (
 	applicationsv1 "github.com/argoproj/argo-cd/v2/pkg/client/listers/application/v1alpha1"
 	"github.com/argoproj/argo-cd/v2/reposerver/apiclient"
 	"github.com/argoproj/argo-cd/v2/util/db"
-	"github.com/argoproj/argo-cd/v2/util/git"
-	"github.com/argoproj/argo-cd/v2/util/helm"
 	"github.com/argoproj/argo-cd/v2/util/io"
 	"github.com/argoproj/argo-cd/v2/util/settings"
 )
@@ -154,7 +151,7 @@ func WaitForRefresh(ctx context.Context, appIf v1alpha1.ApplicationInterface, na
 	return nil, fmt.Errorf("application refresh deadline exceeded")
 }
 
-func TestRepoWithKnownType(repo *argoappv1.Repository, isHelm bool, isHelmOci bool) error {
+func TestRepoWithKnownType(ctx context.Context, repoClient apiclient.RepoServerServiceClient, repo *argoappv1.Repository, isHelm bool, isHelmOci bool) error {
 	repo = repo.DeepCopy()
 	if isHelm {
 		repo.Type = "helm"
@@ -163,37 +160,10 @@ func TestRepoWithKnownType(repo *argoappv1.Repository, isHelm bool, isHelmOci bo
 	}
 	repo.EnableOCI = repo.EnableOCI || isHelmOci
 
-	return TestRepo(repo)
-}
+	_, err := repoClient.TestRepository(ctx, &apiclient.TestRepositoryRequest{
+		Repo: repo,
+	})
 
-func TestRepo(repo *argoappv1.Repository) error {
-	checks := map[string]func() error{
-		"git": func() error {
-			return git.TestRepo(repo.Repo, repo.GetGitCreds(), repo.IsInsecure(), repo.IsLFSEnabled())
-		},
-		"helm": func() error {
-			if repo.EnableOCI {
-				if !helm.IsHelmOciRepo(repo.Repo) {
-					return errors.New("OCI Helm repository URL should include hostname and port only")
-				}
-				_, err := helm.NewClient(repo.Repo, repo.GetHelmCreds(), repo.EnableOCI).TestHelmOCI()
-				return err
-			} else {
-				_, err := helm.NewClient(repo.Repo, repo.GetHelmCreds(), repo.EnableOCI).GetIndex(false)
-				return err
-			}
-		},
-	}
-	if check, ok := checks[repo.Type]; ok {
-		return check()
-	}
-	var err error
-	for _, check := range checks {
-		err = check()
-		if err == nil {
-			return nil
-		}
-	}
 	return err
 }
 
@@ -226,7 +196,8 @@ func ValidateRepo(
 	}
 
 	repoAccessible := false
-	err = TestRepoWithKnownType(repo, app.Spec.Source.IsHelm(), app.Spec.Source.IsHelmOci())
+
+	err = TestRepoWithKnownType(ctx, repoClient, repo, app.Spec.Source.IsHelm(), app.Spec.Source.IsHelmOci())
 	if err != nil {
 		conditions = append(conditions, argoappv1.ApplicationCondition{
 			Type:    argoappv1.ApplicationConditionInvalidSpecError,

util/argo/argo_test.go

@@ -245,6 +245,13 @@ func TestValidateRepo(t *testing.T) {
 		KustomizeOptions: kustomizeOptions,
 	}).Return(&apiclient.RepoAppDetailsResponse{}, nil)
 
+	repo.Type = "git"
+	repoClient.On("TestRepository", context.Background(), &apiclient.TestRepositoryRequest{
+		Repo: repo,
+	}).Return(&apiclient.TestRepositoryResponse{
+		VerifiedRepository: true,
+	}, nil)
+
 	repoClientSet := &mocks.Clientset{RepoServerServiceClient: repoClient}
 
 	db := &dbmocks.ArgoDB{}
@@ -743,9 +750,3 @@ func TestFilterByName(t *testing.T) {
 		assert.Len(t, res, 0)
 	})
 }
-
-func TestTestRepoOCI(t *testing.T) {
-	err := TestRepo(&argoappv1.Repository{Repo: "https://demo.goharbor.io", Type: "helm", EnableOCI: true})
-	assert.Error(t, err)
-	assert.Equal(t, "OCI Helm repository URL should include hostname and port only", err.Error())
-}