fix: Correctly load registry configuration and handle errors (#56)

5 files changed, 137 insertions, 23 deletions

diff --git a/cmd/main.go b/cmd/main.go
index 70064f0..c291f43 100644
--- a/cmd/main.go
+++ b/cmd/main.go
@@ -186,7 +186,7 @@ func newRunCommand() *cobra.Command {
 				if err != nil || st.IsDir() {
 					log.Warnf("Registry configuration at %s could not be read: %v -- using a default configuration", cfg.RegistriesConf, err)
 				} else {
-					err = registry.LoadRegistryConfiguration(cfg.RegistriesConf)
+					err = registry.LoadRegistryConfiguration(cfg.RegistriesConf, false)
 					if err != nil {
 						log.Errorf("Could not load registry configuration from %s: %v", cfg.RegistriesConf, err)
 						return nil
diff --git a/pkg/registry/config.go b/pkg/registry/config.go
index 5704bdd..509831b 100644
--- a/pkg/registry/config.go
+++ b/pkg/registry/config.go
@@ -27,7 +27,7 @@ type RegistryList struct {
 
 // LoadRegistryConfiguration loads a YAML-formatted registry configuration from
 // a given file at path.
-func LoadRegistryConfiguration(path string) error {
+func LoadRegistryConfiguration(path string, clear bool) error {
 	registryBytes, err := ioutil.ReadFile(path)
 	if err != nil {
 		return err
@@ -36,12 +36,20 @@ func LoadRegistryConfiguration(path string) error {
 	if err != nil {
 		return err
 	}
+
+	if clear {
+		registryLock.Lock()
+		registries = make(map[string]*RegistryEndpoint)
+		registryLock.Unlock()
+	}
+
 	for _, reg := range registryList.Items {
-		err = AddRegistryEndpoint(reg.Prefix, reg.Name, reg.ApiURL, "", "", reg.Credentials)
+		err = AddRegistryEndpoint(reg.Prefix, reg.Name, reg.ApiURL, reg.Credentials)
 		if err != nil {
 			return err
 		}
 	}
+
 	log.Infof("Loaded %d registry configurations from %s", len(registryList.Items), path)
 	return nil
 }
@@ -50,6 +58,7 @@ func LoadRegistryConfiguration(path string) error {
 // of registries.
 func ParseRegistryConfiguration(yamlSource string) (RegistryList, error) {
 	var regList RegistryList
+	var defaultPrefixFound = ""
 	err := yaml.UnmarshalStrict([]byte(yamlSource), &regList)
 	if err != nil {
 		return RegistryList{}, err
@@ -59,6 +68,22 @@ func ParseRegistryConfiguration(yamlSource string) (RegistryList, error) {
 	for _, registry := range regList.Items {
 		if registry.Name == "" {
 			err = fmt.Errorf("registry name is missing for entry %v", registry)
+		} else if registry.ApiURL == "" {
+			err = fmt.Errorf("API URL must be specified for registry %s", registry.Name)
+		} else if registry.Prefix == "" {
+			if defaultPrefixFound != "" {
+				err = fmt.Errorf("there must be only one default registry (already is %s), %s needs a prefix", defaultPrefixFound, registry.Name)
+			} else {
+				defaultPrefixFound = registry.Name
+			}
+		}
+
+		if err == nil {
+			switch registry.TagSortMode {
+			case "latest-first", "latest-last", "none", "":
+			default:
+				err = fmt.Errorf("unknown tag sort mode for registry %s: %s", registry.Name, registry.TagSortMode)
+			}
 		}
 	}
 
@@ -72,6 +97,8 @@ func ParseRegistryConfiguration(yamlSource string) (RegistryList, error) {
 // RestRestoreDefaultRegistryConfiguration restores the registry configuration
 // to the default values.
 func RestoreDefaultRegistryConfiguration() {
+	registryLock.Lock()
+	defer registryLock.Unlock()
 	registries = make(map[string]*RegistryEndpoint)
 	for k, v := range defaultRegistries {
 		registries[k] = v.DeepCopy()
diff --git a/pkg/registry/config_test.go b/pkg/registry/config_test.go
index dc9f503..29e6cfb 100644
--- a/pkg/registry/config_test.go
+++ b/pkg/registry/config_test.go
@@ -16,11 +16,66 @@ func Test_ParseRegistryConfFromYaml(t *testing.T) {
 		require.NoError(t, err)
 		assert.Len(t, regList.Items, 3)
 	})
+
+	t.Run("Parse from invalid YAML: no name found", func(t *testing.T) {
+		registries := `
+registries:
+- api_url: https://foo.io
+  ping: false
+`
+		regList, err := ParseRegistryConfiguration(registries)
+		require.Error(t, err)
+		assert.Contains(t, err.Error(), "name is missing")
+		assert.Len(t, regList.Items, 0)
+	})
+
+	t.Run("Parse from invalid YAML: no API URL found", func(t *testing.T) {
+		registries := `
+registries:
+- name: Foobar Registry
+  ping: false
+`
+		regList, err := ParseRegistryConfiguration(registries)
+		require.Error(t, err)
+		assert.Contains(t, err.Error(), "API URL must be")
+		assert.Len(t, regList.Items, 0)
+	})
+
+	t.Run("Parse from invalid YAML: multiple registries without prefix", func(t *testing.T) {
+		registries := `
+registries:
+- name: Foobar Registry
+  api_url: https://foobar.io
+  ping: false
+- name: Barbar Registry
+  api_url: https://barbar.io
+  ping: false
+`
+		regList, err := ParseRegistryConfiguration(registries)
+		require.Error(t, err)
+		assert.Contains(t, err.Error(), "already is Foobar Registry")
+		assert.Len(t, regList.Items, 0)
+	})
+
+	t.Run("Parse from invalid YAML: invalid tag sort mode", func(t *testing.T) {
+		registries := `
+registries:
+- name: Foobar Registry
+  api_url: https://foobar.io
+  ping: false
+  tagsortmode: invalid
+`
+		regList, err := ParseRegistryConfiguration(registries)
+		require.Error(t, err)
+		assert.Contains(t, err.Error(), "unknown tag sort mode")
+		assert.Len(t, regList.Items, 0)
+	})
+
 }
 
 func Test_LoadRegistryConfiguration(t *testing.T) {
 	t.Run("Load from valid location", func(t *testing.T) {
-		err := LoadRegistryConfiguration("../../config/example-config.yaml")
+		err := LoadRegistryConfiguration("../../config/example-config.yaml", false)
 		require.NoError(t, err)
 		assert.Len(t, registries, 4)
 		reg, err := GetRegistryEndpoint("gcr.io")
diff --git a/pkg/registry/endpoints.go b/pkg/registry/endpoints.go
index 0fb814a..18f43e7 100644
--- a/pkg/registry/endpoints.go
+++ b/pkg/registry/endpoints.go
@@ -2,9 +2,11 @@ package registry
 
 import (
 	"fmt"
+	"strings"
 	"sync"
 
 	"github.com/argoproj-labs/argocd-image-updater/pkg/cache"
+	"github.com/argoproj-labs/argocd-image-updater/pkg/log"
 )
 
 // TagListSort defines how the registry returns the list of tags
@@ -21,6 +23,21 @@ func (tls TagListSort) IsTimeSorted() bool {
 	return tls == SortLatestFirst || tls == SortLatestLast
 }
 
+// TagListSortFromString gets the TagListSort value from a given string
+func TagListSortFromString(tls string) TagListSort {
+	switch strings.ToLower(tls) {
+	case "latest-first":
+		return SortLatestFirst
+	case "latest-last":
+		return SortLatestLast
+	case "none", "":
+		return SortUnsorted
+	default:
+		log.Warnf("unknown tag list sort mode: %s", tls)
+		return SortUnsorted
+	}
+}
+
 // RegistryEndpoint holds information on how to access any specific registry API
 // endpoint.
 type RegistryEndpoint struct {
@@ -76,15 +93,13 @@ var registries map[string]*RegistryEndpoint = make(map[string]*RegistryEndpoint)
 var registryLock sync.RWMutex
 
 // AddRegistryEndpoint adds registry endpoint information with the given details
-func AddRegistryEndpoint(prefix, name, apiUrl, username, password, credentials string) error {
+func AddRegistryEndpoint(prefix, name, apiUrl, credentials string) error {
 	registryLock.Lock()
 	defer registryLock.Unlock()
 	registries[prefix] = &RegistryEndpoint{
 		RegistryName:   name,
 		RegistryPrefix: prefix,
 		RegistryAPI:    apiUrl,
-		Username:       username,
-		Password:       password,
 		Credentials:    credentials,
 		Cache:          cache.NewMemCache(),
 	}
@@ -104,14 +119,13 @@ func GetRegistryEndpoint(prefix string) (*RegistryEndpoint, error) {
 
 // SetRegistryEndpointCredentials allows to change the credentials used for
 // endpoint access for existing RegistryEndpoint configuration
-func SetRegistryEndpointCredentials(prefix, username, password string) error {
+func SetRegistryEndpointCredentials(prefix, credentials string) error {
 	registry, err := GetRegistryEndpoint(prefix)
 	if err != nil {
 		return err
 	}
 	registry.lock.Lock()
-	registry.Username = username
-	registry.Password = password
+	registry.Credentials = credentials
 	registry.lock.Unlock()
 	return nil
 }
@@ -122,8 +136,6 @@ func (ep *RegistryEndpoint) DeepCopy() *RegistryEndpoint {
 	newEp.RegistryAPI = ep.RegistryAPI
 	newEp.RegistryName = ep.RegistryName
 	newEp.RegistryPrefix = ep.RegistryPrefix
-	newEp.Username = ep.Username
-	newEp.Password = ep.Password
 	newEp.Credentials = ep.Credentials
 	newEp.Ping = ep.Ping
 	newEp.TagListSort = ep.TagListSort
diff --git a/pkg/registry/endpoints_test.go b/pkg/registry/endpoints_test.go
index 3ffc413..2535630 100644
--- a/pkg/registry/endpoints_test.go
+++ b/pkg/registry/endpoints_test.go
@@ -31,7 +31,7 @@ func Test_GetEndpoints(t *testing.T) {
 
 func Test_AddEndpoint(t *testing.T) {
 	t.Run("Add new endpoint", func(t *testing.T) {
-		err := AddRegistryEndpoint("example.com", "Example", "https://example.com", "", "", "")
+		err := AddRegistryEndpoint("example.com", "Example", "https://example.com", "")
 		require.NoError(t, err)
 	})
 	t.Run("Get example.com endpoint", func(t *testing.T) {
@@ -43,30 +43,28 @@ func Test_AddEndpoint(t *testing.T) {
 		assert.Equal(t, ep.RegistryAPI, "https://example.com")
 	})
 	t.Run("Change existing endpoint", func(t *testing.T) {
-		err := AddRegistryEndpoint("example.com", "Example", "https://example.com", "", "", "")
+		err := AddRegistryEndpoint("example.com", "Example", "https://example.com", "")
 		require.NoError(t, err)
 	})
 }
 
 func Test_SetEndpointCredentials(t *testing.T) {
 	t.Run("Set credentials on default registry", func(t *testing.T) {
-		err := SetRegistryEndpointCredentials("", "username", "password")
+		err := SetRegistryEndpointCredentials("", "env:FOOBAR")
 		require.NoError(t, err)
 		ep, err := GetRegistryEndpoint("")
 		require.NoError(t, err)
 		require.NotNil(t, ep)
-		assert.Equal(t, ep.Username, "username")
-		assert.Equal(t, ep.Password, "password")
+		assert.Equal(t, ep.Credentials, "env:FOOBAR")
 	})
 
 	t.Run("Unset credentials on default registry", func(t *testing.T) {
-		err := SetRegistryEndpointCredentials("", "", "")
+		err := SetRegistryEndpointCredentials("", "")
 		require.NoError(t, err)
 		ep, err := GetRegistryEndpoint("")
 		require.NoError(t, err)
 		require.NotNil(t, ep)
-		assert.Equal(t, ep.Username, "")
-		assert.Equal(t, ep.Password, "")
+		assert.Equal(t, ep.Credentials, "")
 	})
 }
 
@@ -85,9 +83,8 @@ func Test_EndpointConcurrentAccess(t *testing.T) {
 	t.Run("Concurrent write access", func(t *testing.T) {
 		for i := 0; i < 50; i++ {
 			go func(i int) {
-				username := fmt.Sprintf("Username-%d", i)
-				password := fmt.Sprintf("Password-%d", i)
-				err := SetRegistryEndpointCredentials("", username, password)
+				creds := fmt.Sprintf("secret:foo/secret-%d", i)
+				err := SetRegistryEndpointCredentials("", creds)
 				require.NoError(t, err)
 				ep, err := GetRegistryEndpoint("")
 				require.NoError(t, err)
@@ -112,3 +109,26 @@ func Test_DeepCopy(t *testing.T) {
 		assert.Equal(t, ep.Ping, newEp.Ping)
 	})
 }
+
+func Test_GetTagListSortFromString(t *testing.T) {
+	t.Run("Get latest-first sorting", func(t *testing.T) {
+		tls := TagListSortFromString("latest-first")
+		assert.Equal(t, SortLatestFirst, tls)
+	})
+	t.Run("Get latest-last sorting", func(t *testing.T) {
+		tls := TagListSortFromString("latest-last")
+		assert.Equal(t, SortLatestLast, tls)
+	})
+	t.Run("Get none sorting explicit", func(t *testing.T) {
+		tls := TagListSortFromString("none")
+		assert.Equal(t, SortUnsorted, tls)
+	})
+	t.Run("Get none sorting implicit", func(t *testing.T) {
+		tls := TagListSortFromString("")
+		assert.Equal(t, SortUnsorted, tls)
+	})
+	t.Run("Get none sorting from unknown", func(t *testing.T) {
+		tls := TagListSortFromString("unknown")
+		assert.Equal(t, SortUnsorted, tls)
+	})
+}