fix: load config error (notaryproject#1145)

Fix:
- the `LoadConfigOnce` function forgets the error and return a nil
config and nil error next time. Updated to use `sync.OnceValues` to keep
the returned values.

Resolves notaryproject#1144

---------

Signed-off-by: Junjie Gao <junjiegao@microsoft.com>
Signed-off-by: Patrick Zheng <patrickzheng@microsoft.com>

internal/cmd/flags.go

@@ -44,14 +44,14 @@ var (
 		Usage: "signature envelope format, options: \"jws\", \"cose\"",
 	}
 	SetPflagSignatureFormat = func(fs *pflag.FlagSet, p *string) {
-		defaultSignatureFormat := envelope.JWS
-		// load config to get signatureFormat
 		config, err := configutil.LoadConfigOnce()
-		if err == nil && config.SignatureFormat != "" {
-			defaultSignatureFormat = config.SignatureFormat
+		if err != nil || config.SignatureFormat == "" {
+			fs.StringVar(p, PflagSignatureFormat.Name, envelope.JWS, PflagSignatureFormat.Usage)
+			return
 		}
 
-		fs.StringVar(p, PflagSignatureFormat.Name, defaultSignatureFormat, PflagSignatureFormat.Usage)
+		// set signatureFormat from config
+		fs.StringVar(p, PflagSignatureFormat.Name, config.SignatureFormat, PflagSignatureFormat.Usage)
 	}
 
 	PflagID = &pflag.Flag{

pkg/configutil/once.go

@@ -21,28 +21,28 @@ import (
 	"github.com/notaryproject/notation/internal/envelope"
 )
 
-var (
-	// configInfo is the config.json data
-	configInfo *config.Config
-	configOnce sync.Once
-)
+// loadConfigOnce is a function that invokes loadConfig only once.
+var loadConfigOnce = sync.OnceValues(loadConfig)
 
 // LoadConfigOnce returns the previously read config file.
 // If previous config file does not exist, it reads the config from file
 // or return a default config if not found.
 // The returned config is only suitable for read only scenarios for short-lived processes.
 func LoadConfigOnce() (*config.Config, error) {
-	var err error
-	configOnce.Do(func() {
-		configInfo, err = config.LoadConfig()
-		if err != nil {
-			return
-		}
-		// set default value
-		configInfo.SignatureFormat = strings.ToLower(configInfo.SignatureFormat)
-		if configInfo.SignatureFormat == "" {
-			configInfo.SignatureFormat = envelope.JWS
-		}
-	})
-	return configInfo, err
+	return loadConfigOnce()
+}
+
+// loadConfig reads the config from file or return a default config if not
+// found.
+func loadConfig() (*config.Config, error) {
+	configInfo, err := config.LoadConfig()
+	if err != nil {
+		return nil, err
+	}
+	// set default value
+	configInfo.SignatureFormat = strings.ToLower(configInfo.SignatureFormat)
+	if configInfo.SignatureFormat == "" {
+		configInfo.SignatureFormat = envelope.JWS
+	}
+	return configInfo, nil
 }

pkg/configutil/once_test.go

@@ -14,10 +14,19 @@
 package configutil
 
 import (
+	"os"
+	"path/filepath"
+	"strings"
+	"sync"
 	"testing"
+
+	"github.com/notaryproject/notation-go/dir"
 )
 
 func TestLoadConfigOnce(t *testing.T) {
+	defer func() {
+		loadConfigOnce = sync.OnceValues(loadConfig)
+	}()
 	config1, err := LoadConfigOnce()
 	if err != nil {
 		t.Fatal("LoadConfigOnce failed.")
@@ -27,6 +36,26 @@ func TestLoadConfigOnce(t *testing.T) {
 		t.Fatal("LoadConfigOnce failed.")
 	}
 	if config1 != config2 {
-		t.Fatal("LoadConfigOnce is invalid.")
+		t.Fatal("LoadConfigOnce should return the same config.")
+	}
+}
+
+func TestLoadConfigOnceError(t *testing.T) {
+	dir.UserConfigDir = t.TempDir()
+	defer func() {
+		dir.UserConfigDir = ""
+		loadConfigOnce = sync.OnceValues(loadConfig)
+	}()
+	if err := os.WriteFile(filepath.Join(dir.UserConfigDir, dir.PathConfigFile), []byte("invalid json"), 0600); err != nil {
+		t.Fatal("Failed to create file.")
+	}
+
+	_, err := LoadConfigOnce()
+	if err == nil || !strings.Contains(err.Error(), "invalid character") {
+		t.Fatal("LoadConfigOnce should fail.")
+	}
+	_, err2 := LoadConfigOnce()
+	if err != err2 {
+		t.Fatal("LoadConfigOnce should return the same error.")
 	}
 }

pkg/configutil/util_test.go

@@ -25,11 +25,10 @@ import (
 )
 
 func TestIsRegistryInsecure(t *testing.T) {
-	configOnce = sync.Once{}
 	// for restore dir
 	defer func(oldDir string) {
 		dir.UserConfigDir = oldDir
-		configOnce = sync.Once{}
+		loadConfigOnce = sync.OnceValues(loadConfig)
 	}(dir.UserConfigDir)
 	// update config dir
 	dir.UserConfigDir = "testdata"
@@ -56,11 +55,10 @@ func TestIsRegistryInsecure(t *testing.T) {
 }
 
 func TestIsRegistryInsecureMissingConfig(t *testing.T) {
-	configOnce = sync.Once{}
 	// for restore dir
 	defer func(oldDir string) {
 		dir.UserConfigDir = oldDir
-		configOnce = sync.Once{}
+		loadConfigOnce = sync.OnceValues(loadConfig)
 	}(dir.UserConfigDir)
 	// update config dir
 	dir.UserConfigDir = "./testdata2"
@@ -93,7 +91,7 @@ func TestIsRegistryInsecureConfigPermissionError(t *testing.T) {
 	defer func(oldDir string) error {
 		// restore permission
 		dir.UserConfigDir = oldDir
-		configOnce = sync.Once{}
+		loadConfigOnce = sync.OnceValues(loadConfig)
 		return os.Chmod(filepath.Join(configDir, "config.json"), 0644)
 	}(dir.UserConfigDir)
 
@@ -113,6 +111,7 @@ func TestIsRegistryInsecureConfigPermissionError(t *testing.T) {
 func TestResolveKey(t *testing.T) {
 	defer func(oldDir string) {
 		dir.UserConfigDir = oldDir
+		loadConfigOnce = sync.OnceValues(loadConfig)
 	}(dir.UserConfigDir)
 
 	t.Run("valid e2e key", func(t *testing.T) {