Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: Argo CD doesn't detect the repo type when repository is scoped #11959

Merged
merged 1 commit into from
Jan 12, 2023
Merged

fix: Argo CD doesn't detect the repo type when repository is scoped #11959

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 10 additions & 6 deletions server/repository/repository.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"google.golang.org/grpc/status"
apierr "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/client-go/tools/cache"

"github.com/argoproj/argo-cd/v2/common"
repositorypkg "github.com/argoproj/argo-cd/v2/pkg/apiclient/repository"
Expand All @@ -37,8 +38,9 @@ type Server struct {
enf *rbac.Enforcer
cache *servercache.Cache
appLister applisters.ApplicationLister
projLister applisters.AppProjectNamespaceLister
projLister cache.SharedIndexInformer
settings *settings.SettingsManager
namespace string
}

// NewServer returns a new instance of the Repository service
Expand All @@ -48,7 +50,8 @@ func NewServer(
enf *rbac.Enforcer,
cache *servercache.Cache,
appLister applisters.ApplicationLister,
projLister applisters.AppProjectNamespaceLister,
projLister cache.SharedIndexInformer,
namespace string,
settings *settings.SettingsManager,
) *Server {
return &Server{
Expand All @@ -58,6 +61,7 @@ func NewServer(
cache: cache,
appLister: appLister,
projLister: projLister,
namespace: namespace,
settings: settings,
}
}
Expand Down Expand Up @@ -248,7 +252,7 @@ func (s *Server) ListApps(ctx context.Context, q *repositorypkg.RepoAppsQuery) (
return nil, errPermissionDenied
}
// Also ensure the repo is actually allowed in the project in question
if err := s.isRepoPermittedInProject(q.Repo, q.AppProject); err != nil {
if err := s.isRepoPermittedInProject(ctx, q.Repo, q.AppProject); err != nil {
return nil, err
}

Expand Down Expand Up @@ -312,7 +316,7 @@ func (s *Server) GetAppDetails(ctx context.Context, q *repositorypkg.RepoAppDeta
}
}
// Ensure the repo is actually allowed in the project in question
if err := s.isRepoPermittedInProject(q.Source.RepoURL, q.AppProject); err != nil {
if err := s.isRepoPermittedInProject(ctx, q.Source.RepoURL, q.AppProject); err != nil {
return nil, err
}

Expand Down Expand Up @@ -540,8 +544,8 @@ func (s *Server) testRepo(ctx context.Context, repo *appsv1.Repository) error {
return err
}

func (s *Server) isRepoPermittedInProject(repo string, projName string) error {
proj, err := s.projLister.Get(projName)
func (s *Server) isRepoPermittedInProject(ctx context.Context, repo string, projName string) error {
proj, err := argo.GetAppProjectByName(projName, applisters.NewAppProjectLister(s.projLister.GetIndexer()), s.namespace, s.settings, s.db, ctx)
if err != nil {
return err
}
Expand Down
58 changes: 35 additions & 23 deletions server/repository/repository_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/client-go/kubernetes/fake"
k8scache "k8s.io/client-go/tools/cache"

"github.com/argoproj/argo-cd/v2/common"
"github.com/argoproj/argo-cd/v2/pkg/apiclient/repository"
Expand Down Expand Up @@ -123,7 +124,7 @@ var (
}
)

func newAppAndProjLister(objects ...runtime.Object) (applisters.ApplicationLister, applisters.AppProjectNamespaceLister) {
func newAppAndProjLister(objects ...runtime.Object) (applisters.ApplicationLister, k8scache.SharedIndexInformer) {
fakeAppsClientset := fakeapps.NewSimpleClientset(objects...)
factory := appinformer.NewSharedInformerFactoryWithOptions(fakeAppsClientset, 0, appinformer.WithNamespace(""), appinformer.WithTweakListOptions(func(options *metav1.ListOptions) {}))
projInformer := factory.Argoproj().V1alpha1().AppProjects()
Expand All @@ -137,8 +138,7 @@ func newAppAndProjLister(objects ...runtime.Object) (applisters.ApplicationListe
}
}
appLister := appsInformer.Lister()
projLister := projInformer.Lister().AppProjects(testNamespace)
return appLister, projLister
return appLister, projInformer.Informer()
}

func Test_createRBACObject(t *testing.T) {
Expand All @@ -152,14 +152,14 @@ func TestRepositoryServer(t *testing.T) {
kubeclientset := fake.NewSimpleClientset(&argocdCM, &argocdSecret)
settingsMgr := settings.NewSettingsManager(context.Background(), kubeclientset, testNamespace)
enforcer := newEnforcer(kubeclientset)
appLister, projLister := newAppAndProjLister(defaultProj)
appLister, projInformer := newAppAndProjLister(defaultProj)
argoDB := db.NewDB("default", settingsMgr, kubeclientset)

t.Run("Test_getRepo", func(t *testing.T) {
repoServerClient := mocks.RepoServerServiceClient{}
repoServerClientset := mocks.Clientset{RepoServerServiceClient: &repoServerClient}

s := NewServer(&repoServerClientset, argoDB, enforcer, nil, appLister, projLister, settingsMgr)
s := NewServer(&repoServerClientset, argoDB, enforcer, nil, appLister, projInformer, testNamespace, settingsMgr)
url := "https://test"
repo, _ := s.getRepo(context.TODO(), url)
assert.Equal(t, repo.Repo, url)
Expand All @@ -170,7 +170,7 @@ func TestRepositoryServer(t *testing.T) {
repoServerClient.On("TestRepository", mock.Anything, mock.Anything).Return(&apiclient.TestRepositoryResponse{}, nil)
repoServerClientset := mocks.Clientset{RepoServerServiceClient: &repoServerClient}

s := NewServer(&repoServerClientset, argoDB, enforcer, nil, appLister, projLister, settingsMgr)
s := NewServer(&repoServerClientset, argoDB, enforcer, nil, appLister, projInformer, testNamespace, settingsMgr)
url := "https://test"
_, err := s.ValidateAccess(context.TODO(), &repository.RepoAccessQuery{
Repo: url,
Expand All @@ -188,7 +188,7 @@ func TestRepositoryServer(t *testing.T) {
db.On("GetRepository", context.TODO(), url).Return(&appsv1.Repository{Repo: url}, nil)
db.On("RepositoryExists", context.TODO(), url).Return(true, nil)

s := NewServer(&repoServerClientset, db, enforcer, newFixtures().Cache, appLister, projLister, settingsMgr)
s := NewServer(&repoServerClientset, db, enforcer, newFixtures().Cache, appLister, projInformer, testNamespace, settingsMgr)
repo, err := s.Get(context.TODO(), &repository.RepoQuery{
Repo: url,
})
Expand All @@ -205,7 +205,7 @@ func TestRepositoryServer(t *testing.T) {
db.On("GetRepository", context.TODO(), url).Return(nil, errors.New("some error"))
db.On("RepositoryExists", context.TODO(), url).Return(true, nil)

s := NewServer(&repoServerClientset, db, enforcer, newFixtures().Cache, appLister, projLister, settingsMgr)
s := NewServer(&repoServerClientset, db, enforcer, newFixtures().Cache, appLister, projInformer, testNamespace, settingsMgr)
repo, err := s.Get(context.TODO(), &repository.RepoQuery{
Repo: url,
})
Expand All @@ -222,7 +222,7 @@ func TestRepositoryServer(t *testing.T) {
db.On("GetRepository", context.TODO(), url).Return(&appsv1.Repository{Repo: url}, nil)
db.On("RepositoryExists", context.TODO(), url).Return(false, nil)

s := NewServer(&repoServerClientset, db, enforcer, newFixtures().Cache, appLister, projLister, settingsMgr)
s := NewServer(&repoServerClientset, db, enforcer, newFixtures().Cache, appLister, projInformer, testNamespace, settingsMgr)
repo, err := s.Get(context.TODO(), &repository.RepoQuery{
Repo: url,
})
Expand All @@ -242,7 +242,7 @@ func TestRepositoryServer(t *testing.T) {
Project: "proj",
}, nil)

s := NewServer(&repoServerClientset, db, enforcer, newFixtures().Cache, appLister, projLister, settingsMgr)
s := NewServer(&repoServerClientset, db, enforcer, newFixtures().Cache, appLister, projInformer, testNamespace, settingsMgr)
repo, err := s.CreateRepository(context.TODO(), &repository.RepoCreateRequest{
Repo: &appsv1.Repository{
Repo: "test",
Expand All @@ -266,7 +266,7 @@ func TestRepositoryServer(t *testing.T) {
db.On("CreateRepository", context.TODO(), mock.Anything).Return(nil, status.Errorf(codes.AlreadyExists, "repository already exists"))
db.On("UpdateRepository", context.TODO(), mock.Anything).Return(nil, nil)

s := NewServer(&repoServerClientset, db, enforcer, newFixtures().Cache, appLister, projLister, settingsMgr)
s := NewServer(&repoServerClientset, db, enforcer, newFixtures().Cache, appLister, projInformer, testNamespace, settingsMgr)
repo, err := s.CreateRepository(context.TODO(), &repository.RepoCreateRequest{
Repo: &appsv1.Repository{
Repo: "test",
Expand Down Expand Up @@ -296,7 +296,7 @@ func TestRepositoryServerListApps(t *testing.T) {
db.On("GetRepository", context.TODO(), url).Return(&appsv1.Repository{Repo: url}, nil)
appLister, projLister := newAppAndProjLister(defaultProj)

s := NewServer(&repoServerClientset, db, enforcer, newFixtures().Cache, appLister, projLister, settingsMgr)
s := NewServer(&repoServerClientset, db, enforcer, newFixtures().Cache, appLister, projLister, testNamespace, settingsMgr)
resp, err := s.ListApps(context.TODO(), &repository.RepoAppsQuery{
Repo: "https://test",
Revision: "HEAD",
Expand All @@ -317,13 +317,15 @@ func TestRepositoryServerListApps(t *testing.T) {
url := "https://test"
db := &dbmocks.ArgoDB{}
db.On("GetRepository", context.TODO(), url).Return(&appsv1.Repository{Repo: url}, nil)
db.On("GetProjectRepositories", context.TODO(), "default").Return(nil, nil)
db.On("GetProjectClusters", context.TODO(), "default").Return(nil, nil)
repoServerClient.On("ListApps", context.TODO(), mock.Anything).Return(&apiclient.AppList{
Apps: map[string]string{
"path/to/dir": "Kustomize",
},
}, nil)

s := NewServer(&repoServerClientset, db, enforcer, newFixtures().Cache, appLister, projLister, settingsMgr)
s := NewServer(&repoServerClientset, db, enforcer, newFixtures().Cache, appLister, projLister, testNamespace, settingsMgr)
resp, err := s.ListApps(context.TODO(), &repository.RepoAppsQuery{
Repo: "https://test",
Revision: "HEAD",
Expand All @@ -346,13 +348,15 @@ func TestRepositoryServerListApps(t *testing.T) {
url := "https://test"
db := &dbmocks.ArgoDB{}
db.On("GetRepository", context.TODO(), url).Return(&appsv1.Repository{Repo: url}, nil)
db.On("GetProjectRepositories", context.TODO(), "default").Return(nil, nil)
db.On("GetProjectClusters", context.TODO(), "default").Return(nil, nil)
repoServerClient.On("ListApps", context.TODO(), mock.Anything).Return(&apiclient.AppList{
Apps: map[string]string{
"path/to/dir": "Kustomize",
},
}, nil)

s := NewServer(&repoServerClientset, db, enforcer, newFixtures().Cache, appLister, projLister, settingsMgr)
s := NewServer(&repoServerClientset, db, enforcer, newFixtures().Cache, appLister, projLister, testNamespace, settingsMgr)
resp, err := s.ListApps(context.TODO(), &repository.RepoAppsQuery{
Repo: "https://test",
Revision: "HEAD",
Expand All @@ -379,7 +383,7 @@ func TestRepositoryServerGetAppDetails(t *testing.T) {
db.On("GetRepository", context.TODO(), url).Return(&appsv1.Repository{Repo: url}, nil)
appLister, projLister := newAppAndProjLister(defaultProj)

s := NewServer(&repoServerClientset, db, enforcer, newFixtures().Cache, appLister, projLister, settingsMgr)
s := NewServer(&repoServerClientset, db, enforcer, newFixtures().Cache, appLister, projLister, testNamespace, settingsMgr)
resp, err := s.GetAppDetails(context.TODO(), &repository.RepoAppDetailsQuery{
Source: &appsv1.ApplicationSource{
RepoURL: url,
Expand All @@ -402,7 +406,7 @@ func TestRepositoryServerGetAppDetails(t *testing.T) {
db.On("GetRepository", context.TODO(), url).Return(&appsv1.Repository{Repo: url}, nil)
appLister, projLister := newAppAndProjLister(defaultProj)

s := NewServer(&repoServerClientset, db, enforcer, newFixtures().Cache, appLister, projLister, settingsMgr)
s := NewServer(&repoServerClientset, db, enforcer, newFixtures().Cache, appLister, projLister, testNamespace, settingsMgr)
resp, err := s.GetAppDetails(context.TODO(), &repository.RepoAppDetailsQuery{
Source: &appsv1.ApplicationSource{
RepoURL: url,
Expand All @@ -424,7 +428,7 @@ func TestRepositoryServerGetAppDetails(t *testing.T) {
db.On("GetRepository", context.TODO(), url).Return(&appsv1.Repository{Repo: url}, nil)
appLister, projLister := newAppAndProjLister(defaultProj)

s := NewServer(&repoServerClientset, db, enforcer, newFixtures().Cache, appLister, projLister, settingsMgr)
s := NewServer(&repoServerClientset, db, enforcer, newFixtures().Cache, appLister, projLister, testNamespace, settingsMgr)
resp, err := s.GetAppDetails(context.TODO(), &repository.RepoAppDetailsQuery{
Source: &appsv1.ApplicationSource{
RepoURL: url,
Expand All @@ -444,11 +448,13 @@ func TestRepositoryServerGetAppDetails(t *testing.T) {
db := &dbmocks.ArgoDB{}
db.On("ListHelmRepositories", context.TODO(), mock.Anything).Return(nil, nil)
db.On("GetRepository", context.TODO(), url).Return(&appsv1.Repository{Repo: url}, nil)
db.On("GetProjectRepositories", context.TODO(), "default").Return(nil, nil)
db.On("GetProjectClusters", context.TODO(), "default").Return(nil, nil)
expectedResp := apiclient.RepoAppDetailsResponse{Type: "Directory"}
repoServerClient.On("GetAppDetails", context.TODO(), mock.Anything).Return(&expectedResp, nil)
appLister, projLister := newAppAndProjLister(defaultProj)

s := NewServer(&repoServerClientset, db, enforcer, newFixtures().Cache, appLister, projLister, settingsMgr)
s := NewServer(&repoServerClientset, db, enforcer, newFixtures().Cache, appLister, projLister, testNamespace, settingsMgr)
resp, err := s.GetAppDetails(context.TODO(), &repository.RepoAppDetailsQuery{
Source: &appsv1.ApplicationSource{
RepoURL: url,
Expand All @@ -467,11 +473,13 @@ func TestRepositoryServerGetAppDetails(t *testing.T) {
url := "https://test"
db := &dbmocks.ArgoDB{}
db.On("GetRepository", context.TODO(), url).Return(&appsv1.Repository{Repo: url}, nil)
db.On("GetProjectRepositories", context.TODO(), "default").Return(nil, nil)
db.On("GetProjectClusters", context.TODO(), "default").Return(nil, nil)
expectedResp := apiclient.RepoAppDetailsResponse{Type: "Directory"}
repoServerClient.On("GetAppDetails", context.TODO(), mock.Anything).Return(&expectedResp, nil)
appLister, projLister := newAppAndProjLister(defaultProjNoSources)

s := NewServer(&repoServerClientset, db, enforcer, newFixtures().Cache, appLister, projLister, settingsMgr)
s := NewServer(&repoServerClientset, db, enforcer, newFixtures().Cache, appLister, projLister, testNamespace, settingsMgr)
resp, err := s.GetAppDetails(context.TODO(), &repository.RepoAppDetailsQuery{
Source: &appsv1.ApplicationSource{
RepoURL: url,
Expand All @@ -491,11 +499,13 @@ func TestRepositoryServerGetAppDetails(t *testing.T) {
db := &dbmocks.ArgoDB{}
db.On("ListHelmRepositories", context.TODO(), mock.Anything).Return(nil, nil)
db.On("GetRepository", context.TODO(), url).Return(&appsv1.Repository{Repo: url}, nil)
db.On("GetProjectRepositories", context.TODO(), "default").Return(nil, nil)
db.On("GetProjectClusters", context.TODO(), "default").Return(nil, nil)
expectedResp := apiclient.RepoAppDetailsResponse{Type: "Directory"}
repoServerClient.On("GetAppDetails", context.TODO(), mock.Anything).Return(&expectedResp, nil)
appLister, projLister := newAppAndProjLister(defaultProj, guestbookApp)

s := NewServer(&repoServerClientset, db, enforcer, newFixtures().Cache, appLister, projLister, settingsMgr)
s := NewServer(&repoServerClientset, db, enforcer, newFixtures().Cache, appLister, projLister, testNamespace, settingsMgr)
resp, err := s.GetAppDetails(context.TODO(), &repository.RepoAppDetailsQuery{
Source: guestbookApp.Spec.GetSourcePtr(),
AppName: "guestbook",
Expand All @@ -514,7 +524,7 @@ func TestRepositoryServerGetAppDetails(t *testing.T) {
db.On("GetRepository", context.TODO(), url).Return(&appsv1.Repository{Repo: url}, nil)
appLister, projLister := newAppAndProjLister(defaultProj, guestbookApp)

s := NewServer(&repoServerClientset, db, enforcer, newFixtures().Cache, appLister, projLister, settingsMgr)
s := NewServer(&repoServerClientset, db, enforcer, newFixtures().Cache, appLister, projLister, testNamespace, settingsMgr)
resp, err := s.GetAppDetails(context.TODO(), &repository.RepoAppDetailsQuery{
Source: guestbookApp.Spec.GetSourcePtr(),
AppName: "guestbook",
Expand All @@ -535,7 +545,7 @@ func TestRepositoryServerGetAppDetails(t *testing.T) {
differentSource := guestbookApp.Spec.Source.DeepCopy()
differentSource.Helm.ValueFiles = []string{"/etc/passwd"}

s := NewServer(&repoServerClientset, db, enforcer, newFixtures().Cache, appLister, projLister, settingsMgr)
s := NewServer(&repoServerClientset, db, enforcer, newFixtures().Cache, appLister, projLister, testNamespace, settingsMgr)
resp, err := s.GetAppDetails(context.TODO(), &repository.RepoAppDetailsQuery{
Source: differentSource,
AppName: "guestbook",
Expand All @@ -553,13 +563,15 @@ func TestRepositoryServerGetAppDetails(t *testing.T) {
db := &dbmocks.ArgoDB{}
db.On("GetRepository", context.TODO(), url).Return(&appsv1.Repository{Repo: url}, nil)
db.On("ListHelmRepositories", context.TODO(), mock.Anything).Return(nil, nil)
db.On("GetProjectRepositories", context.TODO(), "default").Return(nil, nil)
db.On("GetProjectClusters", context.TODO(), "default").Return(nil, nil)
expectedResp := apiclient.RepoAppDetailsResponse{Type: "Directory"}
repoServerClient.On("GetAppDetails", context.TODO(), mock.Anything).Return(&expectedResp, nil)
appLister, projLister := newAppAndProjLister(defaultProj, guestbookApp)
previousSource := guestbookApp.Status.History[0].Source.DeepCopy()
previousSource.TargetRevision = guestbookApp.Status.History[0].Revision

s := NewServer(&repoServerClientset, db, enforcer, newFixtures().Cache, appLister, projLister, settingsMgr)
s := NewServer(&repoServerClientset, db, enforcer, newFixtures().Cache, appLister, projLister, testNamespace, settingsMgr)
resp, err := s.GetAppDetails(context.TODO(), &repository.RepoAppDetailsQuery{
Source: previousSource,
AppName: "guestbook",
Expand Down
2 changes: 1 addition & 1 deletion server/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -752,7 +752,7 @@ type ArgoCDServiceSet struct {
func newArgoCDServiceSet(a *ArgoCDServer) *ArgoCDServiceSet {
kubectl := kubeutil.NewKubectl()
clusterService := cluster.NewServer(a.db, a.enf, a.Cache, kubectl)
repoService := repository.NewServer(a.RepoClientset, a.db, a.enf, a.Cache, a.appLister, a.projLister, a.settingsMgr)
repoService := repository.NewServer(a.RepoClientset, a.db, a.enf, a.Cache, a.appLister, a.projInformer, a.Namespace, a.settingsMgr)
repoCredsService := repocreds.NewServer(a.RepoClientset, a.db, a.enf, a.settingsMgr)
var loginRateLimiter func() (io.Closer, error)
if maxConcurrentLoginRequestsCount > 0 {
Expand Down