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.

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

feat: project-scoped repository credential improvements #18388

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
d2146ae
feat: project-scoped repo cred improvements
blakepettersson May 16, 2024
a93d1b9
fix: missed a test
blakepettersson May 23, 2024
b34000a
wip project key changes
blakepettersson May 24, 2024
490c94a
test: update mocks
blakepettersson May 26, 2024
d81ef2c
test: fix tests
blakepettersson May 27, 2024
c029139
fix: equivalence even if project is empty
blakepettersson May 27, 2024
e372068
fix: wip delete
blakepettersson May 27, 2024
ed1f250
refactor: remove repositorydb
blakepettersson May 28, 2024
2533f6b
chore: improve logging
blakepettersson May 28, 2024
24a613f
fix: pass project to getrepository
blakepettersson May 28, 2024
95c1b99
test: fix failing test
blakepettersson May 28, 2024
559fa87
fix: compare with project secret instead of app secret
blakepettersson May 29, 2024
5a69c07
fix: get repository needs same logic as delete
blakepettersson May 29, 2024
4e56003
feat: add project flag to repo rm command
blakepettersson May 29, 2024
d59fe08
docs: make codegen
blakepettersson May 29, 2024
3940e1c
test: fix failing test
blakepettersson May 29, 2024
46bd30d
test: more failing tests
blakepettersson May 29, 2024
90a2ff4
chore: minor cleanups
blakepettersson May 30, 2024
db1fc43
chore: propagate project from ui
blakepettersson May 31, 2024
ccd55a6
test: add new test cases
blakepettersson Jun 3, 2024
5eb9d0b
chore: code review, improve formulation
blakepettersson Jun 3, 2024
654ba2f
refactor: address cr feedback
blakepettersson Jun 4, 2024
bf2e6ee
Merge branch 'master' into feature/project-scoped-repository-credenti…
alexmt Jun 5, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 0 additions & 60 deletions applicationset/services/mocks/RepositoryDB.go

This file was deleted.

19 changes: 5 additions & 14 deletions applicationset/services/repo_service.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,21 +6,12 @@ import (

"github.com/argoproj/argo-cd/v2/pkg/apis/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/io"
)

//go:generate go run github.com/vektra/mockery/v2@v2.40.2 --name=RepositoryDB

// RepositoryDB Is a lean facade for ArgoDB,
// Using a lean interface makes it easier to test the functionality of the git generator
type RepositoryDB interface {
GetRepository(ctx context.Context, url string) (*v1alpha1.Repository, error)
}

type argoCDService struct {
repositoriesDB RepositoryDB
blakepettersson marked this conversation as resolved.
Show resolved Hide resolved
getRepository func(ctx context.Context, url, project string) (*v1alpha1.Repository, error)
storecreds git.CredsStore
submoduleEnabled bool
repoServerClientSet apiclient.Clientset
Expand All @@ -38,17 +29,17 @@ type Repos interface {
GetDirectories(ctx context.Context, repoURL string, revision string, noRevisionCache bool) ([]string, error)
}

func NewArgoCDService(db db.ArgoDB, submoduleEnabled bool, repoClientset apiclient.Clientset, newFileGlobbingEnabled bool) (Repos, error) {
func NewArgoCDService(getRepository func(ctx context.Context, url, project string) (*v1alpha1.Repository, error), submoduleEnabled bool, repoClientset apiclient.Clientset, newFileGlobbingEnabled bool) (Repos, error) {
return &argoCDService{
repositoriesDB: db.(RepositoryDB),
getRepository: getRepository,
submoduleEnabled: submoduleEnabled,
repoServerClientSet: repoClientset,
newFileGlobbingEnabled: newFileGlobbingEnabled,
}, nil
}

func (a *argoCDService) GetFiles(ctx context.Context, repoURL string, revision string, pattern string, noRevisionCache bool) (map[string][]byte, error) {
repo, err := a.repositoriesDB.GetRepository(ctx, repoURL)
repo, err := a.getRepository(ctx, repoURL, "")
if err != nil {
return nil, fmt.Errorf("error in GetRepository: %w", err)
}
Expand All @@ -75,7 +66,7 @@ func (a *argoCDService) GetFiles(ctx context.Context, repoURL string, revision s
}

func (a *argoCDService) GetDirectories(ctx context.Context, repoURL string, revision string, noRevisionCache bool) ([]string, error) {
repo, err := a.repositoriesDB.GetRepository(ctx, repoURL)
repo, err := a.getRepository(ctx, repoURL, "")
if err != nil {
return nil, fmt.Errorf("error in GetRepository: %w", err)
}
Expand Down
58 changes: 19 additions & 39 deletions applicationset/services/repo_service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,8 @@ import (
"fmt"
"testing"

"github.com/argoproj/argo-cd/v2/applicationset/services/mocks"
"github.com/argoproj/argo-cd/v2/reposerver/apiclient"
repo_mocks "github.com/argoproj/argo-cd/v2/reposerver/apiclient/mocks"
db_mocks "github.com/argoproj/argo-cd/v2/util/db/mocks"
"github.com/argoproj/argo-cd/v2/util/git"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
Expand All @@ -19,9 +17,9 @@ import (
func TestGetDirectories(t *testing.T) {

type fields struct {
repositoriesDBFuncs []func(*mocks.RepositoryDB)
storecreds git.CredsStore
submoduleEnabled bool
getRepository func(ctx context.Context, url, project string) (*v1alpha1.Repository, error)
repoServerClientFuncs []func(*repo_mocks.RepoServerServiceClient)
}
type args struct {
Expand All @@ -38,17 +36,13 @@ func TestGetDirectories(t *testing.T) {
wantErr assert.ErrorAssertionFunc
}{
{name: "ErrorGettingRepos", fields: fields{
repositoriesDBFuncs: []func(*mocks.RepositoryDB){
func(db *mocks.RepositoryDB) {
db.On("GetRepository", mock.Anything, mock.Anything).Return(nil, fmt.Errorf("unable to get repos"))
},
getRepository: func(ctx context.Context, url, project string) (*v1alpha1.Repository, error) {
return nil, fmt.Errorf("unable to get repos")
},
}, args: args{}, want: nil, wantErr: assert.Error},
{name: "ErrorGettingDirs", fields: fields{
repositoriesDBFuncs: []func(*mocks.RepositoryDB){
func(db *mocks.RepositoryDB) {
db.On("GetRepository", mock.Anything, mock.Anything).Return(&v1alpha1.Repository{}, nil)
},
getRepository: func(ctx context.Context, url, project string) (*v1alpha1.Repository, error) {
return &v1alpha1.Repository{}, nil
},
repoServerClientFuncs: []func(*repo_mocks.RepoServerServiceClient){
func(client *repo_mocks.RepoServerServiceClient) {
Expand All @@ -57,10 +51,8 @@ func TestGetDirectories(t *testing.T) {
},
}, args: args{}, want: nil, wantErr: assert.Error},
{name: "HappyCase", fields: fields{
repositoriesDBFuncs: []func(*mocks.RepositoryDB){
func(db *mocks.RepositoryDB) {
db.On("GetRepository", mock.Anything, mock.Anything).Return(&v1alpha1.Repository{}, nil)
},
getRepository: func(ctx context.Context, url, project string) (*v1alpha1.Repository, error) {
return &v1alpha1.Repository{}, nil
},
repoServerClientFuncs: []func(*repo_mocks.RepoServerServiceClient){
func(client *repo_mocks.RepoServerServiceClient) {
Expand All @@ -73,18 +65,14 @@ func TestGetDirectories(t *testing.T) {
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
mockDb := &mocks.RepositoryDB{}
mockRepoClient := &repo_mocks.RepoServerServiceClient{}
// decorate the mocks
for i := range tt.fields.repositoriesDBFuncs {
tt.fields.repositoriesDBFuncs[i](mockDb)
}
for i := range tt.fields.repoServerClientFuncs {
tt.fields.repoServerClientFuncs[i](mockRepoClient)
}

a := &argoCDService{
repositoriesDB: mockDb,
getRepository: tt.fields.getRepository,
storecreds: tt.fields.storecreds,
submoduleEnabled: tt.fields.submoduleEnabled,
repoServerClientSet: &repo_mocks.Clientset{RepoServerServiceClient: mockRepoClient},
Expand All @@ -100,10 +88,10 @@ func TestGetDirectories(t *testing.T) {

func TestGetFiles(t *testing.T) {
type fields struct {
repositoriesDBFuncs []func(*mocks.RepositoryDB)
storecreds git.CredsStore
submoduleEnabled bool
repoServerClientFuncs []func(*repo_mocks.RepoServerServiceClient)
getRepository func(ctx context.Context, url, project string) (*v1alpha1.Repository, error)
}
type args struct {
ctx context.Context
Expand All @@ -120,17 +108,13 @@ func TestGetFiles(t *testing.T) {
wantErr assert.ErrorAssertionFunc
}{
{name: "ErrorGettingRepos", fields: fields{
repositoriesDBFuncs: []func(*mocks.RepositoryDB){
func(db *mocks.RepositoryDB) {
db.On("GetRepository", mock.Anything, mock.Anything).Return(nil, fmt.Errorf("unable to get repos"))
},
getRepository: func(ctx context.Context, url, project string) (*v1alpha1.Repository, error) {
return nil, fmt.Errorf("unable to get repos")
},
}, args: args{}, want: nil, wantErr: assert.Error},
{name: "ErrorGettingFiles", fields: fields{
repositoriesDBFuncs: []func(*mocks.RepositoryDB){
func(db *mocks.RepositoryDB) {
db.On("GetRepository", mock.Anything, mock.Anything).Return(&v1alpha1.Repository{}, nil)
},
getRepository: func(ctx context.Context, url, project string) (*v1alpha1.Repository, error) {
return &v1alpha1.Repository{}, nil
},
repoServerClientFuncs: []func(*repo_mocks.RepoServerServiceClient){
func(client *repo_mocks.RepoServerServiceClient) {
Expand All @@ -139,10 +123,8 @@ func TestGetFiles(t *testing.T) {
},
}, args: args{}, want: nil, wantErr: assert.Error},
{name: "HappyCase", fields: fields{
repositoriesDBFuncs: []func(*mocks.RepositoryDB){
func(db *mocks.RepositoryDB) {
db.On("GetRepository", mock.Anything, mock.Anything).Return(&v1alpha1.Repository{}, nil)
},
getRepository: func(ctx context.Context, url, project string) (*v1alpha1.Repository, error) {
return &v1alpha1.Repository{}, nil
},
repoServerClientFuncs: []func(*repo_mocks.RepoServerServiceClient){
func(client *repo_mocks.RepoServerServiceClient) {
Expand All @@ -161,18 +143,14 @@ func TestGetFiles(t *testing.T) {
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
mockDb := &mocks.RepositoryDB{}
mockRepoClient := &repo_mocks.RepoServerServiceClient{}
// decorate the mocks
for i := range tt.fields.repositoriesDBFuncs {
tt.fields.repositoriesDBFuncs[i](mockDb)
}
for i := range tt.fields.repoServerClientFuncs {
tt.fields.repoServerClientFuncs[i](mockRepoClient)
}

a := &argoCDService{
repositoriesDB: mockDb,
getRepository: tt.fields.getRepository,
storecreds: tt.fields.storecreds,
submoduleEnabled: tt.fields.submoduleEnabled,
repoServerClientSet: &repo_mocks.Clientset{RepoServerServiceClient: mockRepoClient},
Expand All @@ -187,7 +165,9 @@ func TestGetFiles(t *testing.T) {
}

func TestNewArgoCDService(t *testing.T) {
service, err := NewArgoCDService(&db_mocks.ArgoDB{}, false, &repo_mocks.Clientset{}, false)
service, err := NewArgoCDService(func(ctx context.Context, url, project string) (*v1alpha1.Repository, error) {
return &v1alpha1.Repository{}, nil
}, false, &repo_mocks.Clientset{}, false)
assert.NoError(t, err, err)
assert.NotNil(t, service)
}
36 changes: 33 additions & 3 deletions assets/swagger.json
Original file line number Diff line number Diff line change
Expand Up @@ -3171,7 +3171,7 @@
"parameters": [
{
"type": "string",
"description": "URL is the URL that this credentials matches to",
"description": "URL is the URL to which these credentials match",
"name": "creds.url",
"in": "path",
"required": true
Expand Down Expand Up @@ -3251,6 +3251,12 @@
"description": "Whether to force a cache refresh on repo's connection state.",
"name": "forceRefresh",
"in": "query"
},
{
"type": "string",
"description": "App project for query.",
"name": "appProject",
"in": "query"
}
],
"responses": {
Expand Down Expand Up @@ -3373,6 +3379,12 @@
"description": "Whether to force a cache refresh on repo's connection state.",
"name": "forceRefresh",
"in": "query"
},
{
"type": "string",
"description": "App project for query.",
"name": "appProject",
"in": "query"
}
],
"responses": {
Expand Down Expand Up @@ -3409,6 +3421,12 @@
"description": "Whether to force a cache refresh on repo's connection state.",
"name": "forceRefresh",
"in": "query"
},
{
"type": "string",
"description": "App project for query.",
"name": "appProject",
"in": "query"
}
],
"responses": {
Expand Down Expand Up @@ -3493,6 +3511,12 @@
"description": "Whether to force a cache refresh on repo's connection state.",
"name": "forceRefresh",
"in": "query"
},
{
"type": "string",
"description": "App project for query.",
"name": "appProject",
"in": "query"
}
],
"responses": {
Expand Down Expand Up @@ -3530,6 +3554,12 @@
"description": "Whether to force a cache refresh on repo's connection state.",
"name": "forceRefresh",
"in": "query"
},
{
"type": "string",
"description": "App project for query.",
"name": "appProject",
"in": "query"
}
],
"responses": {
Expand Down Expand Up @@ -8058,7 +8088,7 @@
},
"url": {
"type": "string",
"title": "URL is the URL that this credentials matches to"
"title": "URL is the URL to which these credentials match"
},
"username": {
"type": "string",
Expand Down Expand Up @@ -8144,7 +8174,7 @@
},
"project": {
"type": "string",
"title": "Reference between project and repository that allow you automatically to be added as item inside SourceRepos project entity"
"title": "Reference between project and repository that allows it to be automatically added as an item inside SourceRepos project entity"
},
"proxy": {
"type": "string",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ func NewCommand() *cobra.Command {
os.Exit(1)
}

// By default watch all namespace
// By default, watch all namespaces
var watchedNamespace string = ""

// If the applicationset-namespaces contains only one namespace it corresponds to the current namespace
Expand Down Expand Up @@ -172,7 +172,7 @@ func NewCommand() *cobra.Command {
}

repoClientset := apiclient.NewRepoServerClientset(argocdRepoServer, repoServerTimeoutSeconds, tlsConfig)
argoCDService, err := services.NewArgoCDService(argoCDDB, gitSubmoduleEnabled, repoClientset, enableNewGitFileGlobbing)
argoCDService, err := services.NewArgoCDService(argoCDDB.GetRepository, gitSubmoduleEnabled, repoClientset, enableNewGitFileGlobbing)
errors.CheckError(err)

terminalGenerators := map[string]generators.Generator{
Expand Down
2 changes: 1 addition & 1 deletion cmd/argocd/commands/admin/repo.go
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ func NewGenRepoSpecCommand() *cobra.Command {
_, err := argoDB.CreateRepository(ctx, &repoOpts.Repo)
errors.CheckError(err)

secret, err := kubeClientset.CoreV1().Secrets(ArgoCDNamespace).Get(ctx, db.RepoURLToSecretName(repoSecretPrefix, repoOpts.Repo.Repo), v1.GetOptions{})
secret, err := kubeClientset.CoreV1().Secrets(ArgoCDNamespace).Get(ctx, db.RepoURLToSecretName(repoSecretPrefix, repoOpts.Repo.Repo, repoOpts.Repo.Project), v1.GetOptions{})
errors.CheckError(err)

errors.CheckError(PrintResources(outputFormat, os.Stdout, secret))
Expand Down
Loading