diff --git a/Makefile b/Makefile index ca55f4c8fc..e566bb8c42 100644 --- a/Makefile +++ b/Makefile @@ -10,6 +10,8 @@ GIT_TAG=$(shell if [ -z "`git status --porcelain`" ]; then git describe --exact- GIT_TREE_STATE=$(shell if [ -z "`git status --porcelain`" ]; then echo "clean" ; else echo "dirty"; fi) PACKR_CMD=$(shell if [ "`which packr`" ]; then echo "packr"; else echo "go run vendor/github.com/gobuffalo/packr/packr/main.go"; fi) +PATH:=$(PATH):$(PWD)/hack + # docker image publishing options DOCKER_PUSH?=false IMAGE_TAG?=latest diff --git a/controller/state.go b/controller/state.go index 5f6e09dfe1..3a66e2de23 100644 --- a/controller/state.go +++ b/controller/state.go @@ -90,7 +90,10 @@ func (m *appStateManager) getRepoObjs(app *v1alpha1.Application, source v1alpha1 if err != nil { return nil, nil, nil, err } - repo := m.getRepo(source.RepoURL) + repo, err := m.db.GetRepository(context.Background(), source.RepoURL) + if err != nil { + return nil, nil, nil, err + } conn, repoClient, err := m.repoClientset.NewRepoServerClient() if err != nil { return nil, nil, nil, err @@ -374,15 +377,6 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, revision st return &compRes, nil } -func (m *appStateManager) getRepo(repoURL string) *v1alpha1.Repository { - repo, err := m.db.GetRepository(context.Background(), repoURL) - if err != nil { - // If we couldn't retrieve from the repo service, assume public repositories - repo = &v1alpha1.Repository{Repo: repoURL} - } - return repo -} - func (m *appStateManager) persistRevisionHistory(app *v1alpha1.Application, revision string, source v1alpha1.ApplicationSource) error { var nextID int64 if len(app.Status.History) > 0 { diff --git a/server/application/application.go b/server/application/application.go index f1cb9e3b56..f3394c1696 100644 --- a/server/application/application.go +++ b/server/application/application.go @@ -23,8 +23,8 @@ import ( "k8s.io/client-go/kubernetes" "k8s.io/client-go/rest" + "github.com/argoproj/argo-cd/pkg/apis/application/v1alpha1" appv1 "github.com/argoproj/argo-cd/pkg/apis/application/v1alpha1" - v1alpha1 "github.com/argoproj/argo-cd/pkg/apis/application/v1alpha1" appclientset "github.com/argoproj/argo-cd/pkg/client/clientset/versioned" "github.com/argoproj/argo-cd/reposerver" "github.com/argoproj/argo-cd/reposerver/repository" @@ -164,8 +164,10 @@ func (s *Server) GetManifests(ctx context.Context, q *ApplicationManifestQuery) if err := s.enf.EnforceErr(ctx.Value("claims"), rbacpolicy.ResourceApplications, rbacpolicy.ActionGet, appRBACName(*a)); err != nil { return nil, err } - repo := s.getRepo(ctx, a.Spec.Source.RepoURL) - + repo, err := s.db.GetRepository(ctx, a.Spec.Source.RepoURL) + if err != nil { + return nil, err + } conn, repoClient, err := s.repoClientset.NewRepoServerClient() if err != nil { return nil, err @@ -809,15 +811,6 @@ func (s *Server) getApplicationDestination(ctx context.Context, name string) (st return server, namespace, nil } -func (s *Server) getRepo(ctx context.Context, repoURL string) *appv1.Repository { - repo, err := s.db.GetRepository(ctx, repoURL) - if err != nil { - // If we couldn't retrieve from the repo service, assume public repositories - repo = &appv1.Repository{Repo: repoURL} - } - return repo -} - // Sync syncs an application to its target state func (s *Server) Sync(ctx context.Context, syncReq *ApplicationSyncRequest) (*appv1.Application, error) { appIf := s.appclientset.ArgoprojV1alpha1().Applications(s.ns) @@ -924,8 +917,7 @@ func (s *Server) resolveRevision(ctx context.Context, app *appv1.Application, sy } repo, err := s.db.GetRepository(ctx, app.Spec.Source.RepoURL) if err != nil { - // If we couldn't retrieve from the repo service, assume public repositories - repo = &appv1.Repository{Repo: app.Spec.Source.RepoURL} + return "", "", err } gitClient, err := s.gitFactory.NewClient(repo.Repo, "", repo.Username, repo.Password, repo.SSHPrivateKey, repo.InsecureIgnoreHostKey) if err != nil { diff --git a/server/application/application_test.go b/server/application/application_test.go index b6d760b400..2228e63d8a 100644 --- a/server/application/application_test.go +++ b/server/application/application_test.go @@ -5,7 +5,7 @@ import ( "testing" "time" - jwt "github.com/dgrijalva/jwt-go" + "github.com/dgrijalva/jwt-go" "github.com/ghodss/yaml" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" @@ -166,7 +166,7 @@ metadata: spec: source: path: some/path - repoURL: https://git.com/repo.git + repoURL: https://github.com/argoproj/argocd-example-apps.git targetRevision: HEAD ksonnet: environment: default diff --git a/server/repository/repository.go b/server/repository/repository.go index 35d73dffe1..fb347c8b5a 100644 --- a/server/repository/repository.go +++ b/server/repository/repository.go @@ -164,13 +164,7 @@ func (s *Server) ListApps(ctx context.Context, q *RepoAppsQuery) (*RepoAppsRespo } repo, err := s.db.GetRepository(ctx, q.Repo) if err != nil { - if errStatus, ok := status.FromError(err); ok && errStatus.Code() == codes.NotFound { - repo = &appsv1.Repository{ - Repo: q.Repo, - } - } else { - return nil, err - } + return nil, err } // Test the repo @@ -202,13 +196,7 @@ func (s *Server) GetAppDetails(ctx context.Context, q *RepoAppDetailsQuery) (*re } repo, err := s.db.GetRepository(ctx, q.Repo) if err != nil { - if errStatus, ok := status.FromError(err); ok && errStatus.Code() == codes.NotFound { - repo = &appsv1.Repository{ - Repo: q.Repo, - } - } else { - return nil, err - } + return nil, err } conn, repoClient, err := s.repoClientset.NewRepoServerClient() if err != nil { diff --git a/test/e2e/app_management_test.go b/test/e2e/app_management_test.go index fb6c7ff592..26a9565c73 100644 --- a/test/e2e/app_management_test.go +++ b/test/e2e/app_management_test.go @@ -45,7 +45,13 @@ func TestAppCreation(t *testing.T) { assert.Equal(t, fixture.DeploymentNamespace(), app.Spec.Destination.Namespace) assert.Equal(t, common.KubernetesInternalAPIServerAddr, app.Spec.Destination.Server) }). - Expect(Event(EventReasonResourceCreated, "create")) + Expect(Event(EventReasonResourceCreated, "create")). + And(func(_ *Application) { + // app should be listed + output, err := fixture.RunCli("app", "list") + assert.NoError(t, err) + assert.Contains(t, output, fixture.Name()) + }) } func TestAppDeletion(t *testing.T) { @@ -59,7 +65,13 @@ func TestAppDeletion(t *testing.T) { Delete(true). Then(). Expect(DoesNotExist()). - Expect(Event(EventReasonResourceDeleted, "delete")) + Expect(Event(EventReasonResourceDeleted, "delete")). + And(func(_ *Application) { + // app should NOT be listed + output, err := fixture.RunCli("app", "list") + assert.NoError(t, err) + assert.NotContains(t, output, fixture.Name()) + }) } func TestTrackAppStateAndSyncApp(t *testing.T) { diff --git a/test/e2e/fixture/app/actions.go b/test/e2e/fixture/app/actions.go index dbecefc087..b33d262917 100644 --- a/test/e2e/fixture/app/actions.go +++ b/test/e2e/fixture/app/actions.go @@ -10,7 +10,9 @@ import ( // none of the func implement error checks, and that is complete intended, you should check for errors // using the Then() type Actions struct { - context *Context + context *Context + lastOutput string + lastError error } func (a *Actions) Create() *Actions { @@ -31,18 +33,18 @@ func (a *Actions) Create() *Actions { args = append(args, "--parameter", parameter) } - _, _ = fixture.RunCli(args...) + a.runCli(args...) return a } func (a *Actions) Sync() *Actions { - _, _ = fixture.RunCli("app", "sync", a.context.name, "--timeout", "5", "--prune") + a.runCli("app", "sync", a.context.name, "--timeout", "5", "--prune") return a } func (a *Actions) TerminateOp() *Actions { - _, _ = fixture.RunCli("app", "terminate-op", a.context.name) + a.runCli("app", "terminate-op", a.context.name) return a } @@ -58,7 +60,7 @@ func (a *Actions) Refresh(refreshType RefreshType) *Actions { RefreshTypeHard: "--hard-refresh", }[refreshType] - _, _ = fixture.RunCli("app", "get", a.context.name, flag) + a.runCli("app", "get", a.context.name, flag) return a } @@ -68,10 +70,14 @@ func (a *Actions) Delete(cascade bool) *Actions { if cascade { args = append(args, "--cascade") } - _, _ = fixture.RunCli(args...) + a.runCli(args...) return a } func (a *Actions) Then() *Consequences { return &Consequences{a.context, a} } + +func (a *Actions) runCli(args ...string) { + a.lastOutput, a.lastError = fixture.RunCli(args...) +} diff --git a/test/e2e/fixture/app/context.go b/test/e2e/fixture/app/context.go index ab318a88b9..cda0c823dc 100644 --- a/test/e2e/fixture/app/context.go +++ b/test/e2e/fixture/app/context.go @@ -22,6 +22,11 @@ func Given(t *testing.T) *Context { return &Context{t: t, destServer: KubernetesInternalAPIServerAddr, name: fixture.Name()} } +func (c *Context) Repo(url string) *Context { + fixture.SetRepoURL(url) + return c +} + func (c *Context) Path(path string) *Context { c.path = path return c @@ -48,5 +53,5 @@ func (c *Context) And(block func()) *Context { } func (c *Context) When() *Actions { - return &Actions{c} + return &Actions{context: c} } diff --git a/test/e2e/fixture/app/expectation.go b/test/e2e/fixture/app/expectation.go index 7787cb6b11..7bbf0d953c 100644 --- a/test/e2e/fixture/app/expectation.go +++ b/test/e2e/fixture/app/expectation.go @@ -151,3 +151,23 @@ func Event(reason string, message string) Expectation { return failed, fmt.Sprintf("unable to find event with reason=%s; message=%s", reason, message) } } + +// asserts that the last command was successful +func Success(message string) Expectation { + return func(c *Consequences) (state, string) { + if c.actions.lastError == nil && strings.Contains(c.actions.lastOutput, message) { + return succeeded, fmt.Sprintf("found success with message '%s'", c.actions.lastOutput) + } + return failed, fmt.Sprintf("expected success with message '%s', got error '%v' message '%s'", message, c.actions.lastError, c.actions.lastOutput) + } +} + +// asserts that the last command was an error with substring match +func Error(message string) Expectation { + return func(c *Consequences) (state, string) { + if c.actions.lastError != nil && strings.Contains(c.actions.lastOutput, message) { + return succeeded, fmt.Sprintf("found error with message '%s'", c.actions.lastOutput) + } + return failed, fmt.Sprintf("expected error with message '%s', got error '%v' message '%s'", message, c.actions.lastError, c.actions.lastOutput) + } +} diff --git a/test/e2e/fixture/fixture.go b/test/e2e/fixture/fixture.go index c1674f8d2a..cfbfa03334 100644 --- a/test/e2e/fixture/fixture.go +++ b/test/e2e/fixture/fixture.go @@ -46,6 +46,7 @@ var ( apiServerAddress string token string plainText bool + repoUrl string ) // getKubeConfig creates new kubernetes client config using specified config path and config overrides variables @@ -109,21 +110,33 @@ func Name() string { func repoDirectory() string { return path.Join(tmpDir, id) } +func SetRepoURL(url string) { + repoUrl = url +} func RepoURL() string { - return fmt.Sprintf("file:///%s", repoDirectory()) + return repoUrl } func DeploymentNamespace() string { return fmt.Sprintf("argocd-e2e-ns-%s", id) } +// creates a secret for the current test, this currently can only create a single secret +func CreateSecret(username, password string) string { + secretName := fmt.Sprintf("argocd-e2e-secret-%s", id) + FailOnErr(Run("", "kubectl", "create", "secret", "generic", secretName, + "--from-literal=username="+username, + "--from-literal=password="+password)) + FailOnErr(Run("", "kubectl", "label", "secret", secretName, testingLabel+"=true")) + return secretName +} + func EnsureCleanState() { start := time.Now() // delete resources - text, err := Run("", "kubectl", "get", "app", "-o", "name") CheckError(err) for _, name := range strings.Split(text, "\n") { @@ -135,20 +148,31 @@ func EnsureCleanState() { FailOnErr(Run("", "kubectl", "-n", ArgoCDNamespace, "delete", "appprojects", "--field-selector", "metadata.name!=default")) // takes around 5s, so we don't wait FailOnErr(Run("", "kubectl", "delete", "ns", "-l", testingLabel+"=true", "--field-selector", "status.phase=Active", "--wait=false")) + FailOnErr(Run("", "kubectl", "delete", "secrets", "-l", testingLabel+"=true")) // reset settings - argoSettings, err := SettingsManager.GetSettings() + s, err := SettingsManager.GetSettings() CheckError(err) - if len(argoSettings.ResourceOverrides) > 0 { - argoSettings.ResourceOverrides = nil - CheckError(SettingsManager.SaveSettings(argoSettings)) - } + CheckError(SettingsManager.SaveSettings(&settings.ArgoCDSettings{ + // changing theses causes a restart + AdminPasswordHash: s.AdminPasswordHash, + AdminPasswordMtime: s.AdminPasswordMtime, + Certificate: s.Certificate, + DexConfig: s.DexConfig, + OIDCConfigRAW: s.OIDCConfigRAW, + URL: s.URL, + WebhookGitHubSecret: s.WebhookGitHubSecret, + WebhookGitLabSecret: s.WebhookGitLabSecret, + WebhookBitbucketUUID: s.WebhookBitbucketUUID, + Secrets: s.Secrets, + })) // remove tmp dir CheckError(os.RemoveAll(tmpDir)) // new random ID id = strings.ToLower(rand.RandString(5)) + repoUrl = fmt.Sprintf("file:///%s", repoDirectory()) // create tmp dir FailOnErr(Run("", "mkdir", "-p", tmpDir)) @@ -161,8 +185,8 @@ func EnsureCleanState() { FailOnErr(Run(repoDirectory(), "git", "commit", "-q", "-m", "initial commit")) // create namespace - FailOnErr(Run(repoDirectory(), "kubectl", "create", "ns", DeploymentNamespace())) - FailOnErr(Run(repoDirectory(), "kubectl", "label", "ns", DeploymentNamespace(), testingLabel+"=true")) + FailOnErr(Run("", "kubectl", "create", "ns", DeploymentNamespace())) + FailOnErr(Run("", "kubectl", "label", "ns", DeploymentNamespace(), testingLabel+"=true")) log.WithFields(log.Fields{"duration": time.Since(start), "id": id}).Info("clean state") } @@ -179,6 +203,10 @@ func RunCli(args ...string) (string, error) { func Patch(path string, jsonPatch string) { + if !strings.HasPrefix(repoUrl, "file://") { + log.WithFields(log.Fields{"repoUrl": repoUrl}).Fatal("cannot patch repo unless it is local") + } + log.WithFields(log.Fields{"path": path, "jsonPatch": jsonPatch}).Info("patching") filename := filepath.Join(repoDirectory(), path) diff --git a/test/e2e/repo_creds_test.go b/test/e2e/repo_creds_test.go new file mode 100644 index 0000000000..b35147bded --- /dev/null +++ b/test/e2e/repo_creds_test.go @@ -0,0 +1,56 @@ +package e2e + +import ( + "fmt" + "testing" + + . "github.com/argoproj/argo-cd/errors" + "github.com/argoproj/argo-cd/test/e2e/fixture" + . "github.com/argoproj/argo-cd/test/e2e/fixture/app" +) + +const repoUrl = "https://gitlab.com/argo-cd-test/test-apps.git" +const accessToken = "B5sBDeoqAVUouoHkrovy" +const appPath = "child-base" + +// make sure you cannot create an app from a private repo without set-up +func TestCannotAddAppFromPrivateRepoWithOutConfig(t *testing.T) { + Given(t). + Repo(repoUrl). + Path(appPath). + When(). + Create(). + Then(). + Expect(Error("repository not accessible: authentication required")) +} + +// make sure you can create an app from a private repo, if the repo is set-up in the CM +func TestCanAddAppFromPrivateRepoWithRepoConfig(t *testing.T) { + Given(t). + Repo(repoUrl). + Path(appPath). + And(func() { + // I use CLI, but you could also modify the settings, we get a free test of the CLI here + FailOnErr(fixture.RunCli("repo", "add", repoUrl, "--username", "blah", "--password", accessToken)) + }). + When(). + Create(). + Then(). + Expect(Success("")) +} + +// make sure you can create an app from a private repo, if the creds are set-up in the CM +func TestCanAddAppFromPrivateRepoWithCredConfig(t *testing.T) { + + Given(t). + Repo(repoUrl). + Path(appPath). + And(func() { + secretName := fixture.CreateSecret("blah", accessToken) + FailOnErr(fixture.Run("", "kubectl", "patch", "cm", "argocd-cm", "-p", fmt.Sprintf(`{"data": {"repository.credentials": "- passwordSecret:\n key: password\n name: %s\n url: %s\n usernameSecret:\n key: username\n name: %s\n"}}`, secretName, repoUrl, secretName))) + }). + When(). + Create(). + Then(). + Expect(Success("")) +} diff --git a/util/argo/argo.go b/util/argo/argo.go index e7fb14e98e..ac6e6128d3 100644 --- a/util/argo/argo.go +++ b/util/argo/argo.go @@ -143,24 +143,16 @@ func ValidateRepo(ctx context.Context, spec *argoappv1.ApplicationSpec, repoClie defer util.Close(conn) repoAccessable := false repoRes, err := db.GetRepository(ctx, spec.Source.RepoURL) - if err != nil { - if errStatus, ok := status.FromError(err); ok && errStatus.Code() == codes.NotFound { - // The repo has not been added to Argo CD so we do not have credentials to access it. - // We support the mode where apps can be created from public repositories. Test the - // repo to make sure it is publicly accessible - err = git.TestRepo(spec.Source.RepoURL, "", "", "", false) - if err != nil { - conditions = append(conditions, argoappv1.ApplicationCondition{ - Type: argoappv1.ApplicationConditionInvalidSpecError, - Message: fmt.Sprintf("No credentials available for source repository and repository is not publicly accessible: %v", err), - }) - } else { - repoAccessable = true - } - } else { - return nil, "", err - } + return nil, "", err + } + + err = git.TestRepo(repoRes.Repo, repoRes.Username, repoRes.Password, repoRes.SSHPrivateKey, repoRes.InsecureIgnoreHostKey) + if err != nil { + conditions = append(conditions, argoappv1.ApplicationCondition{ + Type: argoappv1.ApplicationConditionInvalidSpecError, + Message: fmt.Sprintf("repository not accessible: %v", err), + }) } else { repoAccessable = true } diff --git a/util/db/db_test.go b/util/db/db_test.go index a2ea78237e..deef694e34 100644 --- a/util/db/db_test.go +++ b/util/db/db_test.go @@ -2,7 +2,6 @@ package db import ( "context" - "reflect" "strings" "testing" @@ -103,12 +102,11 @@ func TestGetRepository(t *testing.T) { name string repoURL string want *v1alpha1.Repository - wantErr bool }{ { name: "TestUnknownRepo", - repoURL: "http://unknown/repo", - wantErr: true, + repoURL: "https://unknown/repo", + want: &v1alpha1.Repository{Repo: "https://unknown/repo"}, }, { name: "TestKnownRepo", @@ -124,13 +122,8 @@ func TestGetRepository(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { got, err := db.GetRepository(context.TODO(), tt.repoURL) - if (err != nil) != tt.wantErr { - t.Errorf("db.GetHydratedRepository() error = %v, wantErr %v", err, tt.wantErr) - return - } - if !reflect.DeepEqual(got, tt.want) { - t.Errorf("db.GetHydratedRepository() = %v, want %v", got, tt.want) - } + assert.NoError(t, err) + assert.Equal(t, tt.want, got) }) } } diff --git a/util/db/repository.go b/util/db/repository.go index 24bb0f2db9..ceb7023512 100644 --- a/util/db/repository.go +++ b/util/db/repository.go @@ -5,11 +5,6 @@ import ( "hash/fnv" "strings" - "github.com/argoproj/argo-cd/common" - appsv1 "github.com/argoproj/argo-cd/pkg/apis/application/v1alpha1" - "github.com/argoproj/argo-cd/util/git" - "github.com/argoproj/argo-cd/util/settings" - log "github.com/sirupsen/logrus" "golang.org/x/net/context" "google.golang.org/grpc/codes" @@ -17,6 +12,11 @@ import ( apiv1 "k8s.io/api/core/v1" apierr "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/argoproj/argo-cd/common" + appsv1 "github.com/argoproj/argo-cd/pkg/apis/application/v1alpha1" + "github.com/argoproj/argo-cd/util/git" + "github.com/argoproj/argo-cd/util/settings" ) const ( @@ -86,15 +86,13 @@ func (db *db) GetRepository(ctx context.Context, repoURL string) (*appsv1.Reposi return nil, err } + repo := &appsv1.Repository{Repo: repoURL} index := getRepositoryIndex(s, repoURL) - if index < 0 { - return nil, status.Errorf(codes.NotFound, "repo '%s' not found", repoURL) - } - - repo, err := db.credentialsToRepository(s.Repositories[index]) - - if err != nil { - return nil, err + if index >= 0 { + repo, err = db.credentialsToRepository(s.Repositories[index]) + if err != nil { + return nil, err + } } if !repo.HasCredentials() {