From 1cc3d4c01c712d6c806e80b3eba05b4b219f135b Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Tue, 14 Jan 2025 05:47:08 +0000 Subject: [PATCH 1/7] fix: load config error Signed-off-by: Junjie Gao --- pkg/configutil/once.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/configutil/once.go b/pkg/configutil/once.go index 00e2a2a46..0e162146e 100644 --- a/pkg/configutil/once.go +++ b/pkg/configutil/once.go @@ -25,6 +25,7 @@ var ( // configInfo is the config.json data configInfo *config.Config configOnce sync.Once + err error ) // LoadConfigOnce returns the previously read config file. @@ -32,7 +33,6 @@ var ( // 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 { From 65b8e71630784372eb0f8a285848f6af0fb24236 Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Tue, 14 Jan 2025 06:44:19 +0000 Subject: [PATCH 2/7] fix: update to use sync.OnceValues Signed-off-by: Junjie Gao --- pkg/configutil/once.go | 17 +++++------------ pkg/configutil/once_test.go | 29 +++++++++++++++++++++++++++-- pkg/configutil/util_test.go | 6 ------ 3 files changed, 32 insertions(+), 20 deletions(-) diff --git a/pkg/configutil/once.go b/pkg/configutil/once.go index 0e162146e..77daaee54 100644 --- a/pkg/configutil/once.go +++ b/pkg/configutil/once.go @@ -21,28 +21,21 @@ import ( "github.com/notaryproject/notation/internal/envelope" ) -var ( - // configInfo is the config.json data - configInfo *config.Config - configOnce sync.Once - err error -) - // 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) { - configOnce.Do(func() { - configInfo, err = config.LoadConfig() + return sync.OnceValues(func() (*config.Config, error) { + configInfo, err := config.LoadConfig() if err != nil { - return + return nil, err } // set default value configInfo.SignatureFormat = strings.ToLower(configInfo.SignatureFormat) if configInfo.SignatureFormat == "" { configInfo.SignatureFormat = envelope.JWS } - }) - return configInfo, err + return configInfo, nil + })() } diff --git a/pkg/configutil/once_test.go b/pkg/configutil/once_test.go index d218669dd..aba9c25a9 100644 --- a/pkg/configutil/once_test.go +++ b/pkg/configutil/once_test.go @@ -14,7 +14,13 @@ package configutil import ( + "os" + "path/filepath" + "reflect" + "strings" "testing" + + "github.com/notaryproject/notation-go/dir" ) func TestLoadConfigOnce(t *testing.T) { @@ -26,7 +32,26 @@ func TestLoadConfigOnce(t *testing.T) { if err != nil { t.Fatal("LoadConfigOnce failed.") } - if config1 != config2 { - t.Fatal("LoadConfigOnce is invalid.") + if !reflect.DeepEqual(config1, config2) { + t.Fatal("Configs differ in content.") + } +} + +func TestLoadConfigOnceError(t *testing.T) { + dir.UserConfigDir = t.TempDir() + defer func() { + dir.UserConfigDir = "" + }() + 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.") + } + _, err = LoadConfigOnce() + if err == nil || !strings.Contains(err.Error(), "invalid character") { + t.Fatal("LoadConfigOnce should fail.") } } diff --git a/pkg/configutil/util_test.go b/pkg/configutil/util_test.go index cac87584f..dda8b8edb 100644 --- a/pkg/configutil/util_test.go +++ b/pkg/configutil/util_test.go @@ -18,18 +18,15 @@ import ( "path/filepath" "runtime" "strings" - "sync" "testing" "github.com/notaryproject/notation-go/dir" ) func TestIsRegistryInsecure(t *testing.T) { - configOnce = sync.Once{} // for restore dir defer func(oldDir string) { dir.UserConfigDir = oldDir - configOnce = sync.Once{} }(dir.UserConfigDir) // update config dir dir.UserConfigDir = "testdata" @@ -56,11 +53,9 @@ 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{} }(dir.UserConfigDir) // update config dir dir.UserConfigDir = "./testdata2" @@ -93,7 +88,6 @@ func TestIsRegistryInsecureConfigPermissionError(t *testing.T) { defer func(oldDir string) error { // restore permission dir.UserConfigDir = oldDir - configOnce = sync.Once{} return os.Chmod(filepath.Join(configDir, "config.json"), 0644) }(dir.UserConfigDir) From 574f9afc1a61f195acf30b1f0bbf73823a02287c Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Tue, 14 Jan 2025 06:50:56 +0000 Subject: [PATCH 3/7] fix: update code Signed-off-by: Junjie Gao --- pkg/configutil/once.go | 26 ++++++++++++-------------- pkg/configutil/once_test.go | 11 +++++------ 2 files changed, 17 insertions(+), 20 deletions(-) diff --git a/pkg/configutil/once.go b/pkg/configutil/once.go index 77daaee54..3d6acd17d 100644 --- a/pkg/configutil/once.go +++ b/pkg/configutil/once.go @@ -25,17 +25,15 @@ import ( // 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) { - return sync.OnceValues(func() (*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 - })() -} +var LoadConfigOnce = sync.OnceValues(func() (*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 +}) diff --git a/pkg/configutil/once_test.go b/pkg/configutil/once_test.go index aba9c25a9..5f6ebdefc 100644 --- a/pkg/configutil/once_test.go +++ b/pkg/configutil/once_test.go @@ -16,7 +16,6 @@ package configutil import ( "os" "path/filepath" - "reflect" "strings" "testing" @@ -32,8 +31,8 @@ func TestLoadConfigOnce(t *testing.T) { if err != nil { t.Fatal("LoadConfigOnce failed.") } - if !reflect.DeepEqual(config1, config2) { - t.Fatal("Configs differ in content.") + if config1 != config2 { + t.Fatal("LoadConfigOnce should return the same config.") } } @@ -50,8 +49,8 @@ func TestLoadConfigOnceError(t *testing.T) { if err == nil || !strings.Contains(err.Error(), "invalid character") { t.Fatal("LoadConfigOnce should fail.") } - _, 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.") } } From cb284ac5b21693bb5534c6c1fc4ccf0570f8d73d Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Tue, 14 Jan 2025 07:04:59 +0000 Subject: [PATCH 4/7] test: update test Signed-off-by: Junjie Gao --- pkg/configutil/once_test.go | 22 ++++++++++++++++++++++ pkg/configutil/util_test.go | 4 ++++ 2 files changed, 26 insertions(+) diff --git a/pkg/configutil/once_test.go b/pkg/configutil/once_test.go index 5f6ebdefc..315dd12df 100644 --- a/pkg/configutil/once_test.go +++ b/pkg/configutil/once_test.go @@ -17,12 +17,18 @@ import ( "os" "path/filepath" "strings" + "sync" "testing" + "github.com/notaryproject/notation-go/config" "github.com/notaryproject/notation-go/dir" + "github.com/notaryproject/notation/internal/envelope" ) func TestLoadConfigOnce(t *testing.T) { + defer func() { + LoadConfigOnce = resetLoadConfigOnce() + }() config1, err := LoadConfigOnce() if err != nil { t.Fatal("LoadConfigOnce failed.") @@ -40,6 +46,7 @@ func TestLoadConfigOnceError(t *testing.T) { dir.UserConfigDir = t.TempDir() defer func() { dir.UserConfigDir = "" + LoadConfigOnce = resetLoadConfigOnce() }() if err := os.WriteFile(filepath.Join(dir.UserConfigDir, dir.PathConfigFile), []byte("invalid json"), 0600); err != nil { t.Fatal("Failed to create file.") @@ -54,3 +61,18 @@ func TestLoadConfigOnceError(t *testing.T) { t.Fatal("LoadConfigOnce should return the same error.") } } + +func resetLoadConfigOnce() func() (*config.Config, error) { + return sync.OnceValues(func() (*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 + }) +} diff --git a/pkg/configutil/util_test.go b/pkg/configutil/util_test.go index dda8b8edb..6f21266f9 100644 --- a/pkg/configutil/util_test.go +++ b/pkg/configutil/util_test.go @@ -27,6 +27,7 @@ func TestIsRegistryInsecure(t *testing.T) { // for restore dir defer func(oldDir string) { dir.UserConfigDir = oldDir + LoadConfigOnce = resetLoadConfigOnce() }(dir.UserConfigDir) // update config dir dir.UserConfigDir = "testdata" @@ -56,6 +57,7 @@ func TestIsRegistryInsecureMissingConfig(t *testing.T) { // for restore dir defer func(oldDir string) { dir.UserConfigDir = oldDir + LoadConfigOnce = resetLoadConfigOnce() }(dir.UserConfigDir) // update config dir dir.UserConfigDir = "./testdata2" @@ -88,6 +90,7 @@ func TestIsRegistryInsecureConfigPermissionError(t *testing.T) { defer func(oldDir string) error { // restore permission dir.UserConfigDir = oldDir + LoadConfigOnce = resetLoadConfigOnce() return os.Chmod(filepath.Join(configDir, "config.json"), 0644) }(dir.UserConfigDir) @@ -107,6 +110,7 @@ func TestIsRegistryInsecureConfigPermissionError(t *testing.T) { func TestResolveKey(t *testing.T) { defer func(oldDir string) { dir.UserConfigDir = oldDir + LoadConfigOnce = resetLoadConfigOnce() }(dir.UserConfigDir) t.Run("valid e2e key", func(t *testing.T) { From 68f71f38042bdefda430733fb1f8a94ffd3dc1d3 Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Tue, 14 Jan 2025 07:13:52 +0000 Subject: [PATCH 5/7] fix: update code Signed-off-by: Junjie Gao --- pkg/configutil/once.go | 30 ++++++++++++++++++------------ pkg/configutil/once_test.go | 22 ++-------------------- pkg/configutil/util_test.go | 8 ++++---- 3 files changed, 24 insertions(+), 36 deletions(-) diff --git a/pkg/configutil/once.go b/pkg/configutil/once.go index 3d6acd17d..7bcc53b2f 100644 --- a/pkg/configutil/once.go +++ b/pkg/configutil/once.go @@ -25,15 +25,21 @@ import ( // 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. -var LoadConfigOnce = sync.OnceValues(func() (*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 -}) +var LoadConfigOnce = loadConfigOnce() + +// loadConfigOnce returns a function that loads the config file only once. +// this function will be used in testing to reset the LoadConfigOnce variable. +func loadConfigOnce() func() (*config.Config, error) { + return sync.OnceValues(func() (*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 + }) +} diff --git a/pkg/configutil/once_test.go b/pkg/configutil/once_test.go index 315dd12df..9af33593a 100644 --- a/pkg/configutil/once_test.go +++ b/pkg/configutil/once_test.go @@ -17,17 +17,14 @@ import ( "os" "path/filepath" "strings" - "sync" "testing" - "github.com/notaryproject/notation-go/config" "github.com/notaryproject/notation-go/dir" - "github.com/notaryproject/notation/internal/envelope" ) func TestLoadConfigOnce(t *testing.T) { defer func() { - LoadConfigOnce = resetLoadConfigOnce() + LoadConfigOnce = loadConfigOnce() }() config1, err := LoadConfigOnce() if err != nil { @@ -46,7 +43,7 @@ func TestLoadConfigOnceError(t *testing.T) { dir.UserConfigDir = t.TempDir() defer func() { dir.UserConfigDir = "" - LoadConfigOnce = resetLoadConfigOnce() + LoadConfigOnce = loadConfigOnce() }() if err := os.WriteFile(filepath.Join(dir.UserConfigDir, dir.PathConfigFile), []byte("invalid json"), 0600); err != nil { t.Fatal("Failed to create file.") @@ -61,18 +58,3 @@ func TestLoadConfigOnceError(t *testing.T) { t.Fatal("LoadConfigOnce should return the same error.") } } - -func resetLoadConfigOnce() func() (*config.Config, error) { - return sync.OnceValues(func() (*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 - }) -} diff --git a/pkg/configutil/util_test.go b/pkg/configutil/util_test.go index 6f21266f9..b9561e855 100644 --- a/pkg/configutil/util_test.go +++ b/pkg/configutil/util_test.go @@ -27,7 +27,7 @@ func TestIsRegistryInsecure(t *testing.T) { // for restore dir defer func(oldDir string) { dir.UserConfigDir = oldDir - LoadConfigOnce = resetLoadConfigOnce() + LoadConfigOnce = loadConfigOnce() }(dir.UserConfigDir) // update config dir dir.UserConfigDir = "testdata" @@ -57,7 +57,7 @@ func TestIsRegistryInsecureMissingConfig(t *testing.T) { // for restore dir defer func(oldDir string) { dir.UserConfigDir = oldDir - LoadConfigOnce = resetLoadConfigOnce() + LoadConfigOnce = loadConfigOnce() }(dir.UserConfigDir) // update config dir dir.UserConfigDir = "./testdata2" @@ -90,7 +90,7 @@ func TestIsRegistryInsecureConfigPermissionError(t *testing.T) { defer func(oldDir string) error { // restore permission dir.UserConfigDir = oldDir - LoadConfigOnce = resetLoadConfigOnce() + LoadConfigOnce = loadConfigOnce() return os.Chmod(filepath.Join(configDir, "config.json"), 0644) }(dir.UserConfigDir) @@ -110,7 +110,7 @@ func TestIsRegistryInsecureConfigPermissionError(t *testing.T) { func TestResolveKey(t *testing.T) { defer func(oldDir string) { dir.UserConfigDir = oldDir - LoadConfigOnce = resetLoadConfigOnce() + LoadConfigOnce = loadConfigOnce() }(dir.UserConfigDir) t.Run("valid e2e key", func(t *testing.T) { From f44f52496718b2ad428f7c1a7b70cbdce56d6061 Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Tue, 14 Jan 2025 07:33:23 +0000 Subject: [PATCH 6/7] fix: update code Signed-off-by: Junjie Gao --- pkg/configutil/once.go | 35 +++++++++++++++++++---------------- pkg/configutil/once_test.go | 5 +++-- pkg/configutil/util_test.go | 9 +++++---- 3 files changed, 27 insertions(+), 22 deletions(-) diff --git a/pkg/configutil/once.go b/pkg/configutil/once.go index 7bcc53b2f..f5e1d96e5 100644 --- a/pkg/configutil/once.go +++ b/pkg/configutil/once.go @@ -21,25 +21,28 @@ import ( "github.com/notaryproject/notation/internal/envelope" ) +// 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. -var LoadConfigOnce = loadConfigOnce() +func LoadConfigOnce() (*config.Config, error) { + return loadConfigOnce() +} -// loadConfigOnce returns a function that loads the config file only once. -// this function will be used in testing to reset the LoadConfigOnce variable. -func loadConfigOnce() func() (*config.Config, error) { - return sync.OnceValues(func() (*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 - }) +// 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 } diff --git a/pkg/configutil/once_test.go b/pkg/configutil/once_test.go index 9af33593a..bd4069199 100644 --- a/pkg/configutil/once_test.go +++ b/pkg/configutil/once_test.go @@ -17,6 +17,7 @@ import ( "os" "path/filepath" "strings" + "sync" "testing" "github.com/notaryproject/notation-go/dir" @@ -24,7 +25,7 @@ import ( func TestLoadConfigOnce(t *testing.T) { defer func() { - LoadConfigOnce = loadConfigOnce() + loadConfigOnce = sync.OnceValues(loadConfig) }() config1, err := LoadConfigOnce() if err != nil { @@ -43,7 +44,7 @@ func TestLoadConfigOnceError(t *testing.T) { dir.UserConfigDir = t.TempDir() defer func() { dir.UserConfigDir = "" - LoadConfigOnce = loadConfigOnce() + 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.") diff --git a/pkg/configutil/util_test.go b/pkg/configutil/util_test.go index b9561e855..1570996de 100644 --- a/pkg/configutil/util_test.go +++ b/pkg/configutil/util_test.go @@ -18,6 +18,7 @@ import ( "path/filepath" "runtime" "strings" + "sync" "testing" "github.com/notaryproject/notation-go/dir" @@ -27,7 +28,7 @@ func TestIsRegistryInsecure(t *testing.T) { // for restore dir defer func(oldDir string) { dir.UserConfigDir = oldDir - LoadConfigOnce = loadConfigOnce() + loadConfigOnce = sync.OnceValues(loadConfig) }(dir.UserConfigDir) // update config dir dir.UserConfigDir = "testdata" @@ -57,7 +58,7 @@ func TestIsRegistryInsecureMissingConfig(t *testing.T) { // for restore dir defer func(oldDir string) { dir.UserConfigDir = oldDir - LoadConfigOnce = loadConfigOnce() + loadConfigOnce = sync.OnceValues(loadConfig) }(dir.UserConfigDir) // update config dir dir.UserConfigDir = "./testdata2" @@ -90,7 +91,7 @@ func TestIsRegistryInsecureConfigPermissionError(t *testing.T) { defer func(oldDir string) error { // restore permission dir.UserConfigDir = oldDir - LoadConfigOnce = loadConfigOnce() + loadConfigOnce = sync.OnceValues(loadConfig) return os.Chmod(filepath.Join(configDir, "config.json"), 0644) }(dir.UserConfigDir) @@ -110,7 +111,7 @@ func TestIsRegistryInsecureConfigPermissionError(t *testing.T) { func TestResolveKey(t *testing.T) { defer func(oldDir string) { dir.UserConfigDir = oldDir - LoadConfigOnce = loadConfigOnce() + loadConfigOnce = sync.OnceValues(loadConfig) }(dir.UserConfigDir) t.Run("valid e2e key", func(t *testing.T) { From 77278ff73f6c18d6437f68c989acae1f88287964 Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Tue, 14 Jan 2025 09:06:05 +0000 Subject: [PATCH 7/7] fix: resolve comments Signed-off-by: Junjie Gao --- internal/cmd/flags.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/cmd/flags.go b/internal/cmd/flags.go index 196768b2c..a2bed995b 100644 --- a/internal/cmd/flags.go +++ b/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{