Test get ref sources (#17)

* test GetRefSources Signed-off-by: Michael Crenshaw <[email protected]> * fix lint Signed-off-by: Michael Crenshaw <[email protected]> Signed-off-by: ishitasequeira <[email protected]> Signed-off-by: Michael Crenshaw <[email protected]> Co-authored-by: ishitasequeira <[email protected]>
crenshaw-dev · Dec 14, 2022 · 546c07d · 546c07d
1 parent d9ba5fa
commit 546c07d
Show file tree

Hide file tree

Showing 3 changed files with 85 additions and 36 deletions.
diff --git a/server/extension/extension_test.go b/server/extension/extension_test.go
@@ -3,21 +3,22 @@ package extension_test
 import (
 	"context"
 	"fmt"
-	"io/ioutil"
+	"io"
 	"net/http"
 	"net/http/httptest"
 	"strings"
 	"testing"
 
-	v1alpha1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
-	"github.com/argoproj/argo-cd/v2/server/extension"
-	"github.com/argoproj/argo-cd/v2/server/extension/mocks"
-	"github.com/argoproj/argo-cd/v2/util/settings"
 	"github.com/gorilla/mux"
 	"github.com/sirupsen/logrus/hooks/test"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
+
+	"github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
+	"github.com/argoproj/argo-cd/v2/server/extension"
+	"github.com/argoproj/argo-cd/v2/server/extension/mocks"
+	"github.com/argoproj/argo-cd/v2/util/settings"
 )
 
 func TestRegisterHandlers(t *testing.T) {
@@ -196,7 +197,7 @@ func TestExtensionsHandlers(t *testing.T) {
 		require.NoError(t, err)
 		require.NotNil(t, resp)
 		require.Equal(t, http.StatusOK, resp.StatusCode)
-		body, err := ioutil.ReadAll(resp.Body)
+		body, err := io.ReadAll(resp.Body)
 		require.NoError(t, err)
 		actual := strings.TrimSuffix(string(body), "\n")
 		assert.Equal(t, backendResponse, actual)
@@ -259,14 +260,14 @@ func TestExtensionsHandlers(t *testing.T) {
 		// then
 		require.NotNil(t, resp1)
 		require.Equal(t, http.StatusOK, resp1.StatusCode)
-		body, err := ioutil.ReadAll(resp1.Body)
+		body, err := io.ReadAll(resp1.Body)
 		require.NoError(t, err)
 		actual := strings.TrimSuffix(string(body), "\n")
 		assert.Equal(t, response1, actual)
 
 		require.NotNil(t, resp2)
 		require.Equal(t, http.StatusOK, resp2.StatusCode)
-		body, err = ioutil.ReadAll(resp2.Body)
+		body, err = io.ReadAll(resp2.Body)
 		require.NoError(t, err)
 		actual = strings.TrimSuffix(string(body), "\n")
 		assert.Equal(t, response2, actual)

diff --git a/util/argo/argo.go b/util/argo/argo.go
@@ -338,20 +338,34 @@ func validateRepo(ctx context.Context,
 	return conditions, nil
 }
 
+// GetRefSources creates a map of ref keys (from the sources' 'ref' fields) to information about the referenced source.
+// This function also validates the references use allowed characters and does not define the same ref key more than
+// once (which would lead to ambiguous references).
 func GetRefSources(ctx context.Context, spec argoappv1.ApplicationSpec, db db.ArgoDB) (argoappv1.RefTargetRevisionMapping, error) {
 	refSources := make(argoappv1.RefTargetRevisionMapping)
 	if spec.HasMultipleSources() {
+		// Validate first to avoid unnecessary DB calls.
+		refKeys := make(map[string]bool)
+		for _, source := range spec.Sources {
+			if source.Ref != "" {
+				isValidRefKey := regexp.MustCompile(`^[a-zA-Z0-9]+$`).MatchString
+				if !isValidRefKey(source.Ref) {
+					return nil, fmt.Errorf("source.ref %s cannot contain any special characters except '_' and '-'", source.Ref)
+				}
+				refKey := "$" + source.Ref
+				if _, ok := refKeys[refKey]; ok {
+					return nil, fmt.Errorf("invalid sources: multiple sources had the same `ref` key")
+				}
+				refKeys[refKey] = true
+			}
+		}
 		// Get Repositories for all sources before generating Manifests
 		for _, source := range spec.Sources {
 			if source.Ref != "" {
 				repo, err := db.GetRepository(ctx, source.RepoURL)
 				if err != nil {
 					return nil, fmt.Errorf("failed to get repository %s: %v", source.RepoURL, err)
 				}
-				isValidRefKey := regexp.MustCompile(`^[a-zA-Z0-9]+$`).MatchString
-				if !isValidRefKey(source.Ref) {
-					return nil, fmt.Errorf("source.ref %s cannot contain any special characters except '_' and '-'", source.Ref)
-				}
 				refKey := "$" + source.Ref
 				refSources[refKey] = &argoappv1.RefTarget{
 					Repo:           *repo,

diff --git a/util/argo/argo_test.go b/util/argo/argo_test.go
@@ -2,6 +2,7 @@ package argo
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"path/filepath"
 	"testing"
@@ -1023,44 +1024,77 @@ func Test_AppInstanceNameFromQualified(t *testing.T) {
 			assert.Equal(t, tt.result, result)
 		})
 	}
-
 }
 
-func getMultiSourceAppSpec() *argoappv1.ApplicationSpec {
+func Test_GetRefSources(t *testing.T) {
+	repoPath, err := filepath.Abs("./../..")
+	assert.NoError(t, err)
+
+	getMultiSourceAppSpec := func(sources argoappv1.ApplicationSources) *argoappv1.ApplicationSpec {
+		return &argoappv1.ApplicationSpec{
+			Sources: sources,
+		}
+	}
 
-	repoPath, _ := filepath.Abs("./../..")
+	repo := &argoappv1.Repository{Repo: fmt.Sprintf("file://%s", repoPath)}
 
-	return &argoappv1.ApplicationSpec{
-		Sources: argoappv1.ApplicationSources{
+	t.Run("target ref exists", func(t *testing.T) {
+		repoDB := &dbmocks.ArgoDB{}
+		repoDB.On("GetRepository", context.Background(), repo.Repo).Return(repo, nil)
+
+		argoSpec := getMultiSourceAppSpec(argoappv1.ApplicationSources{
 			{RepoURL: fmt.Sprintf("file://%s", repoPath), Ref: "source1"},
 			{RepoURL: fmt.Sprintf("file://%s", repoPath)},
-		},
-	}
-}
+		})
 
-func Test_GetRefSources(t *testing.T) {
+		refSources, err := GetRefSources(context.TODO(), *argoSpec, repoDB)
 
-	repoPath, err := filepath.Abs("./../..")
-	assert.NoError(t, err)
+		expectedRefSource := argoappv1.RefTargetRevisionMapping{
+			"$source1": &argoappv1.RefTarget{
+				Repo: *repo,
+			},
+		}
+		assert.NoError(t, err)
+		assert.Len(t, refSources, 1)
+		assert.Equal(t, expectedRefSource, refSources)
+	})
 
-	repo := &argoappv1.Repository{Repo: fmt.Sprintf("file://%s", repoPath)}
+	t.Run("target ref does not exist", func(t *testing.T) {
+		repoDB := &dbmocks.ArgoDB{}
+		repoDB.On("GetRepository", context.Background(), "file://does-not-exist").Return(nil, errors.New("repo does not exist"))
 
-	argoSpec := getMultiSourceAppSpec()
+		argoSpec := getMultiSourceAppSpec(argoappv1.ApplicationSources{
+			{RepoURL: "file://does-not-exist", Ref: "source1"},
+		})
 
-	db := &dbmocks.ArgoDB{}
+		refSources, err := GetRefSources(context.TODO(), *argoSpec, repoDB)
 
-	db.On("GetRepository", context.Background(), repo.Repo).Return(repo, nil)
+		assert.Error(t, err)
+		assert.Empty(t, refSources)
+	})
 
-	refSources, err := GetRefSources(context.TODO(), *argoSpec, db)
+	t.Run("invalid ref", func(t *testing.T) {
+		argoSpec := getMultiSourceAppSpec(argoappv1.ApplicationSources{
+			{RepoURL: "file://does-not-exist", Ref: "%invalid-name%"},
+		})
 
-	expectedRefSource := argoappv1.RefTargetRevisionMapping{
-		"$source1": &argoappv1.RefTarget{
-			Repo: *repo,
-		},
-	}
-	assert.NoError(t, err)
-	assert.Equal(t, 1, len(refSources))
-	assert.Equal(t, expectedRefSource, refSources)
+		refSources, err := GetRefSources(context.TODO(), *argoSpec, &dbmocks.ArgoDB{})
+
+		assert.Error(t, err)
+		assert.Empty(t, refSources)
+	})
+
+	t.Run("duplicate ref keys", func(t *testing.T) {
+		argoSpec := getMultiSourceAppSpec(argoappv1.ApplicationSources{
+			{RepoURL: "file://does-not-exist", Ref: "source1"},
+			{RepoURL: "file://does-not-exist", Ref: "source1"},
+		})
+
+		refSources, err := GetRefSources(context.TODO(), *argoSpec, &dbmocks.ArgoDB{})
+
+		assert.Error(t, err)
+		assert.Empty(t, refSources)
+	})
 }
 
 func TestValidatePermissionsMultipleSources(t *testing.T) {