From 76dc5e6f5a629212e421c38c3694061c050195c2 Mon Sep 17 00:00:00 2001 From: Laurentiu Niculae Date: Tue, 22 Aug 2023 17:08:34 +0300 Subject: [PATCH] refactor(log): replace panics with log fatal or log panic functions Signed-off-by: Laurentiu Niculae --- errors/errors.go | 2 + pkg/api/authn.go | 37 ++-- pkg/api/controller.go | 8 +- pkg/api/controller_test.go | 154 +++++-------- pkg/cli/client/config_cmd.go | 6 +- pkg/cli/server/config_reloader.go | 6 +- pkg/cli/server/extensions_test.go | 73 +++--- pkg/cli/server/root.go | 43 ++-- pkg/cli/server/root_test.go | 345 +++++++++++++++++++++++++---- pkg/compliance/v1_0_0/check.go | 2 +- pkg/exporter/cli/cli.go | 9 +- pkg/meta/meta.go | 17 +- pkg/meta/meta_test.go | 9 +- pkg/storage/cache.go | 26 +-- pkg/storage/cache/boltdb.go | 52 +++-- pkg/storage/cache/boltdb_test.go | 3 +- pkg/storage/cache/dynamodb.go | 13 +- pkg/storage/cache/dynamodb_test.go | 24 +- pkg/storage/cache_test.go | 3 +- pkg/storage/storage.go | 53 +++-- 20 files changed, 584 insertions(+), 301 deletions(-) diff --git a/errors/errors.go b/errors/errors.go index 1506530317..cb8f30fe06 100644 --- a/errors/errors.go +++ b/errors/errors.go @@ -161,6 +161,8 @@ var ( ErrFileAlreadyClosed = errors.New("storageDriver: file already closed") ErrFileAlreadyCommitted = errors.New("storageDriver: file already committed") ErrInvalidOutputFormat = errors.New("cli: invalid output format") + ErrServerIsRunning = errors.New("server is running") + ErrDatabaseFileAlreadyInUse = errors.New("boltdb file is already in use") ErrFlagValueUnsupported = errors.New("supported values ") ErrUnknownSubcommand = errors.New("cli: unknown subcommand") ErrMultipleReposSameName = errors.New("test: can't have multiple repos with the same name") diff --git a/pkg/api/authn.go b/pkg/api/authn.go index 9777b8bee9..1978a27b29 100644 --- a/pkg/api/authn.go +++ b/pkg/api/authn.go @@ -48,10 +48,11 @@ const ( type AuthnMiddleware struct { credMap map[string]string ldapClient *LDAPClient + log log.Logger } func AuthHandler(ctlr *Controller) mux.MiddlewareFunc { - authnMiddleware := &AuthnMiddleware{} + authnMiddleware := &AuthnMiddleware{log: ctlr.Log} if ctlr.Config.IsBearerAuthEnabled() { return bearerAuthHandler(ctlr) @@ -279,13 +280,15 @@ func (amw *AuthnMiddleware) tryAuthnHandlers(ctlr *Controller) mux.MiddlewareFun if ctlr.Config.HTTP.Auth.LDAP.CACert != "" { caCert, err := os.ReadFile(ctlr.Config.HTTP.Auth.LDAP.CACert) if err != nil { - panic(err) + amw.log.Panic().Err(err).Str("caCert", ctlr.Config.HTTP.Auth.LDAP.CACert). + Msg("failed to read caCert") } caCertPool := x509.NewCertPool() if !caCertPool.AppendCertsFromPEM(caCert) { - panic(zerr.ErrBadCACert) + amw.log.Panic().Err(zerr.ErrBadCACert).Str("caCert", ctlr.Config.HTTP.Auth.LDAP.CACert). + Msg("failed to read caCert") } amw.ldapClient.ClientCAs = caCertPool @@ -293,7 +296,8 @@ func (amw *AuthnMiddleware) tryAuthnHandlers(ctlr *Controller) mux.MiddlewareFun // default to system cert pool caCertPool, err := x509.SystemCertPool() if err != nil { - panic(zerr.ErrBadCACert) + amw.log.Panic().Err(zerr.ErrBadCACert).Str("caCert", ctlr.Config.HTTP.Auth.LDAP.CACert). + Msg("failed to get system cert pool") } amw.ldapClient.ClientCAs = caCertPool @@ -303,7 +307,8 @@ func (amw *AuthnMiddleware) tryAuthnHandlers(ctlr *Controller) mux.MiddlewareFun if ctlr.Config.IsHtpasswdAuthEnabled() { credsFile, err := os.Open(ctlr.Config.HTTP.Auth.HTPasswd.Path) if err != nil { - panic(err) + amw.log.Panic().Err(err).Str("credsFile", ctlr.Config.HTTP.Auth.HTPasswd.Path). + Msg("failed to open creds-file") } defer credsFile.Close() @@ -324,10 +329,10 @@ func (amw *AuthnMiddleware) tryAuthnHandlers(ctlr *Controller) mux.MiddlewareFun for provider := range ctlr.Config.HTTP.Auth.OpenID.Providers { if config.IsOpenIDSupported(provider) { - rp := NewRelyingPartyOIDC(ctlr.Config, provider) + rp := NewRelyingPartyOIDC(ctlr.Config, provider, ctlr.Log) ctlr.RelyingParties[provider] = rp } else if config.IsOauth2Supported(provider) { - rp := NewRelyingPartyGithub(ctlr.Config, provider) + rp := NewRelyingPartyGithub(ctlr.Config, provider, ctlr.Log) ctlr.RelyingParties[provider] = rp } } @@ -557,19 +562,20 @@ func (rh *RouteHandler) AuthURLHandler() http.HandlerFunc { } } -func NewRelyingPartyOIDC(config *config.Config, provider string) rp.RelyingParty { - issuer, clientID, clientSecret, redirectURI, scopes, options := getRelyingPartyArgs(config, provider) +func NewRelyingPartyOIDC(config *config.Config, provider string, log log.Logger) rp.RelyingParty { + issuer, clientID, clientSecret, redirectURI, scopes, options := getRelyingPartyArgs(config, provider, log) relyingParty, err := rp.NewRelyingPartyOIDC(issuer, clientID, clientSecret, redirectURI, scopes, options...) if err != nil { - panic(err) + log.Panic().Err(err).Str("issuer", issuer).Str("redirectURI", redirectURI).Strs("scopes", scopes). + Msg("failed to get new relying party oicd") } return relyingParty } -func NewRelyingPartyGithub(config *config.Config, provider string) rp.RelyingParty { - _, clientID, clientSecret, redirectURI, scopes, options := getRelyingPartyArgs(config, provider) +func NewRelyingPartyGithub(config *config.Config, provider string, log log.Logger) rp.RelyingParty { + _, clientID, clientSecret, redirectURI, scopes, options := getRelyingPartyArgs(config, provider, log) rpConfig := &oauth2.Config{ ClientID: clientID, @@ -581,17 +587,18 @@ func NewRelyingPartyGithub(config *config.Config, provider string) rp.RelyingPar relyingParty, err := rp.NewRelyingPartyOAuth(rpConfig, options...) if err != nil { - panic(err) + log.Panic().Err(err).Str("redirectURI", redirectURI).Strs("scopes", scopes). + Msg("failed to get new relying party oauth") } return relyingParty } -func getRelyingPartyArgs(cfg *config.Config, provider string) ( +func getRelyingPartyArgs(cfg *config.Config, provider string, log log.Logger) ( string, string, string, string, []string, []rp.Option, ) { if _, ok := cfg.HTTP.Auth.OpenID.Providers[provider]; !ok { - panic(zerr.ErrOpenIDProviderDoesNotExist) + log.Panic().Err(zerr.ErrOpenIDProviderDoesNotExist).Str("provider", provider).Msg("") } clientID := cfg.HTTP.Auth.OpenID.Providers[provider].ClientID diff --git a/pkg/api/controller.go b/pkg/api/controller.go index e958048d77..c1025185b5 100644 --- a/pkg/api/controller.go +++ b/pkg/api/controller.go @@ -192,13 +192,17 @@ func (c *Controller) Run(reloadCtx context.Context) error { caCert, err := os.ReadFile(c.Config.HTTP.TLS.CACert) if err != nil { - panic(err) + c.Log.Error().Err(err).Str("caCert", c.Config.HTTP.TLS.CACert).Msg("failed to read file") + + return err } caCertPool := x509.NewCertPool() if !caCertPool.AppendCertsFromPEM(caCert) { - panic(errors.ErrBadCACert) + c.Log.Error().Err(errors.ErrBadCACert).Msg("failed to append certs from pem") + + return errors.ErrBadCACert } server.TLSConfig.ClientAuth = clientAuth diff --git a/pkg/api/controller_test.go b/pkg/api/controller_test.go index e3396a046e..de262871f5 100644 --- a/pkg/api/controller_test.go +++ b/pkg/api/controller_test.go @@ -113,13 +113,15 @@ func TestCreateCacheDatabaseDriver(t *testing.T) { panic(err) } - driver := storage.CreateCacheDatabaseDriver(conf.Storage.StorageConfig, log) + driver, err := storage.CreateCacheDatabaseDriver(conf.Storage.StorageConfig, log) + So(err, ShouldNotBeNil) So(driver, ShouldBeNil) conf.Storage.RemoteCache = true conf.Storage.RootDirectory = t.TempDir() - driver = storage.CreateCacheDatabaseDriver(conf.Storage.StorageConfig, log) + driver, err = storage.CreateCacheDatabaseDriver(conf.Storage.StorageConfig, log) + So(err, ShouldBeNil) So(driver, ShouldBeNil) }) tskip.SkipDynamo(t) @@ -146,15 +148,16 @@ func TestCreateCacheDatabaseDriver(t *testing.T) { "name": "dynamodb", "endpoint": endpoint, "region": "us-east-2", - "cacheTablename": "BlobTable", - "repoMetaTablename": "RepoMetadataTable", - "imageMetaTablename": "ZotImageMetaTable", - "repoBlobsInfoTablename": "ZotRepoBlobsInfoTable", - "userDataTablename": "ZotUserDataTable", - "versionTablename": "Version", + "cachetablename": "BlobTable", + "repometatablename": "RepoMetadataTable", + "imagemetatablename": "ZotImageMetaTable", + "repoblobsinfotablename": "ZotRepoBlobsInfoTable", + "userdatatablename": "ZotUserDataTable", + "versiontablename": "Version", } - driver := storage.CreateCacheDatabaseDriver(conf.Storage.StorageConfig, log) + driver, err := storage.CreateCacheDatabaseDriver(conf.Storage.StorageConfig, log) + So(err, ShouldBeNil) So(driver, ShouldNotBeNil) // negative test cases @@ -162,30 +165,32 @@ func TestCreateCacheDatabaseDriver(t *testing.T) { conf.Storage.CacheDriver = map[string]interface{}{ "endpoint": endpoint, "region": "us-east-2", - "cacheTablename": "BlobTable", - "repoMetaTablename": "RepoMetadataTable", - "imageMetaTablename": "ZotImageMetaTable", - "repoBlobsInfoTablename": "ZotRepoBlobsInfoTable", - "userDataTablename": "ZotUserDataTable", - "versionTablename": "Version", + "cachetablename": "BlobTable", + "repometatablename": "RepoMetadataTable", + "imagemetatablename": "ZotImageMetaTable", + "repoblobsinfotablename": "ZotRepoBlobsInfoTable", + "userdatatablename": "ZotUserDataTable", + "versiontablename": "Version", } - driver = storage.CreateCacheDatabaseDriver(conf.Storage.StorageConfig, log) + driver, err = storage.CreateCacheDatabaseDriver(conf.Storage.StorageConfig, log) + So(err, ShouldBeNil) So(driver, ShouldBeNil) conf.Storage.CacheDriver = map[string]interface{}{ "name": "dummy", "endpoint": endpoint, "region": "us-east-2", - "cacheTablename": "BlobTable", - "repoMetaTablename": "RepoMetadataTable", - "imageMetaTablename": "ZotImageMetaTable", - "repoBlobsInfoTablename": "ZotRepoBlobsInfoTable", - "userDataTablename": "ZotUserDataTable", - "versionTablename": "Version", + "cachetablename": "BlobTable", + "repometatablename": "RepoMetadataTable", + "imagemetatablename": "ZotImageMetaTable", + "repoblobsinfotablename": "ZotRepoBlobsInfoTable", + "userdatatablename": "ZotUserDataTable", + "versiontablename": "Version", } - driver = storage.CreateCacheDatabaseDriver(conf.Storage.StorageConfig, log) + driver, err = storage.CreateCacheDatabaseDriver(conf.Storage.StorageConfig, log) + So(err, ShouldBeNil) So(driver, ShouldBeNil) }) } @@ -295,7 +300,7 @@ func TestRunAlreadyRunningServer(t *testing.T) { defer cm.StopServer() err := ctlr.Init(context.Background()) - So(err, ShouldBeNil) + So(err, ShouldNotBeNil) err = ctlr.Run(context.Background()) So(err, ShouldNotBeNil) @@ -361,12 +366,14 @@ func TestObjectStorageController(t *testing.T) { port := test.GetFreePort() conf := config.New() conf.HTTP.Port = port + tmp := t.TempDir() + storageDriverParams := map[string]interface{}{ - "rootdirectory": "zot", + "rootdirectory": tmp, "name": storageConstants.S3StorageDriverName, } conf.Storage.StorageDriver = storageDriverParams - ctlr := makeController(conf, "zot") + ctlr := makeController(conf, tmp) So(ctlr, ShouldNotBeNil) err := ctlr.Init(context.Background()) @@ -379,9 +386,10 @@ func TestObjectStorageController(t *testing.T) { conf.HTTP.Port = port endpoint := os.Getenv("S3MOCK_ENDPOINT") + tmp := t.TempDir() storageDriverParams := map[string]interface{}{ - "rootdirectory": "zot", + "rootdirectory": tmp, "name": storageConstants.S3StorageDriverName, "region": "us-east-2", "bucket": bucket, @@ -391,7 +399,7 @@ func TestObjectStorageController(t *testing.T) { } conf.Storage.StorageDriver = storageDriverParams - ctlr := makeController(conf, "/") + ctlr := makeController(conf, tmp) So(ctlr, ShouldNotBeNil) cm := test.NewControllerManager(ctlr) @@ -485,9 +493,10 @@ func TestObjectStorageControllerSubPaths(t *testing.T) { conf.HTTP.Port = port endpoint := os.Getenv("S3MOCK_ENDPOINT") + tmp := t.TempDir() storageDriverParams := map[string]interface{}{ - "rootdirectory": "zot", + "rootdirectory": tmp, "name": storageConstants.S3StorageDriverName, "region": "us-east-2", "bucket": bucket, @@ -496,12 +505,12 @@ func TestObjectStorageControllerSubPaths(t *testing.T) { "skipverify": false, } conf.Storage.StorageDriver = storageDriverParams - ctlr := makeController(conf, "zot") + ctlr := makeController(conf, tmp) So(ctlr, ShouldNotBeNil) subPathMap := make(map[string]config.StorageConfig) subPathMap["/a"] = config.StorageConfig{ - RootDirectory: "/a", + RootDirectory: t.TempDir(), StorageDriver: storageDriverParams, } ctlr.Config.Storage.SubPaths = subPathMap @@ -1278,7 +1287,7 @@ func TestMultipleInstance(t *testing.T) { Convey("Test zot multiple subpath with same root directory", t, func() { port := test.GetFreePort() - baseURL := test.GetBaseURL(port) + // baseURL := test.GetBaseURL(port) conf := config.New() conf.HTTP.Port = port username, seedUser := test.GenerateRandomString() @@ -1319,27 +1328,8 @@ func TestMultipleInstance(t *testing.T) { ctlr.Config.Storage.SubPaths = subPathMap - cm := test.NewControllerManager(ctlr) - cm.StartAndWait(port) - defer cm.StopServer() - - // without creds, should get access error - resp, err := resty.R().Get(baseURL + "/v2/") - So(err, ShouldBeNil) - So(resp, ShouldNotBeNil) - So(resp.StatusCode(), ShouldEqual, http.StatusUnauthorized) - var e apiErr.Error - err = json.Unmarshal(resp.Body(), &e) - So(err, ShouldBeNil) - - // with creds, should get expected status code - resp, _ = resty.R().SetBasicAuth(username, password).Get(baseURL) - So(resp, ShouldNotBeNil) - So(resp.StatusCode(), ShouldEqual, http.StatusNotFound) - - resp, _ = resty.R().SetBasicAuth(username, password).Get(baseURL + "/v2/") - So(resp, ShouldNotBeNil) - So(resp.StatusCode(), ShouldEqual, http.StatusOK) + err = ctlr.Init(context.Background()) + So(err, ShouldNotBeNil) }) } @@ -2581,7 +2571,7 @@ func TestNewRelyingPartyOIDC(t *testing.T) { } Convey("provider not found in config", func() { - So(func() { _ = api.NewRelyingPartyOIDC(conf, "notDex") }, ShouldPanic) + So(func() { _ = api.NewRelyingPartyOIDC(conf, "notDex", log.NewLogger("debug", "")) }, ShouldPanic) }) Convey("key path not found on disk", func() { @@ -2589,7 +2579,7 @@ func TestNewRelyingPartyOIDC(t *testing.T) { oidcProviderCfg.KeyPath = "path/to/file" conf.HTTP.Auth.OpenID.Providers["oidc"] = oidcProviderCfg - So(func() { _ = api.NewRelyingPartyOIDC(conf, "oidc") }, ShouldPanic) + So(func() { _ = api.NewRelyingPartyOIDC(conf, "oidc", log.NewLogger("debug", "")) }, ShouldPanic) }) Convey("https callback", func() { @@ -2598,7 +2588,7 @@ func TestNewRelyingPartyOIDC(t *testing.T) { Key: ServerKey, } - rp := api.NewRelyingPartyOIDC(conf, "oidc") + rp := api.NewRelyingPartyOIDC(conf, "oidc", log.NewLogger("debug", "")) So(rp, ShouldNotBeNil) }) @@ -2607,7 +2597,7 @@ func TestNewRelyingPartyOIDC(t *testing.T) { oidcProvider.ClientSecret = "" conf.HTTP.Auth.OpenID.Providers["oidc"] = oidcProvider - rp := api.NewRelyingPartyOIDC(conf, "oidc") + rp := api.NewRelyingPartyOIDC(conf, "oidc", log.NewLogger("debug", "")) So(rp, ShouldNotBeNil) }) @@ -2616,7 +2606,7 @@ func TestNewRelyingPartyOIDC(t *testing.T) { oidcProvider.Issuer = "" conf.HTTP.Auth.OpenID.Providers["oidc"] = oidcProvider - So(func() { _ = api.NewRelyingPartyOIDC(conf, "oidc") }, ShouldPanic) + So(func() { _ = api.NewRelyingPartyOIDC(conf, "oidc", log.NewLogger("debug", "")) }, ShouldPanic) }) }) } @@ -8264,46 +8254,10 @@ func TestPeriodicGC(t *testing.T) { func TestSearchRoutes(t *testing.T) { Convey("Upload image for test", t, func(c C) { - port := test.GetFreePort() - baseURL := test.GetBaseURL(port) - conf := config.New() - conf.HTTP.Port = port tempDir := t.TempDir() - - ctlr := makeController(conf, tempDir) - cm := test.NewControllerManager(ctlr) - - cm.StartAndWait(port) - defer cm.StopServer() - repoName := "testrepo" //nolint:goconst inaccessibleRepo := "inaccessible" - cfg, layers, manifest, err := deprecated.GetImageComponents(10000) //nolint:staticcheck - So(err, ShouldBeNil) - - err = UploadImage( - Image{ - Config: cfg, - Layers: layers, - Manifest: manifest, - }, baseURL, repoName, "latest") - - So(err, ShouldBeNil) - - // data for the inaccessible repo - cfg, layers, manifest, err = deprecated.GetImageComponents(10000) //nolint:staticcheck - So(err, ShouldBeNil) - - err = UploadImage( - Image{ - Config: cfg, - Layers: layers, - Manifest: manifest, - }, baseURL, inaccessibleRepo, "latest") - - So(err, ShouldBeNil) - Convey("GlobalSearch with authz enabled", func(c C) { conf := config.New() port := test.GetFreePort() @@ -8512,7 +8466,7 @@ func TestSearchRoutes(t *testing.T) { img := CreateRandomImage() - err = UploadImageWithBasicAuth(img, baseURL, repoName, img.DigestStr(), user1, password1) + err := UploadImageWithBasicAuth(img, baseURL, repoName, img.DigestStr(), user1, password1) So(err, ShouldBeNil) query := ` @@ -8599,7 +8553,7 @@ func TestSearchRoutes(t *testing.T) { img := CreateRandomImage() - err = UploadImageWithBasicAuth(img, baseURL, repoName, img.DigestStr(), user1, password1) + err := UploadImageWithBasicAuth(img, baseURL, repoName, img.DigestStr(), user1, password1) So(err, ShouldNotBeNil) }) @@ -8667,7 +8621,7 @@ func TestSearchRoutes(t *testing.T) { img := CreateRandomImage() - err = UploadImageWithBasicAuth(img, baseURL, repoName, img.DigestStr(), user1, password1) + err := UploadImageWithBasicAuth(img, baseURL, repoName, img.DigestStr(), user1, password1) So(err, ShouldBeNil) }) @@ -8735,7 +8689,7 @@ func TestSearchRoutes(t *testing.T) { img := CreateRandomImage() - err = UploadImageWithBasicAuth(img, baseURL, repoName, img.DigestStr(), user1, password1) + err := UploadImageWithBasicAuth(img, baseURL, repoName, img.DigestStr(), user1, password1) So(err, ShouldBeNil) }) @@ -8789,7 +8743,7 @@ func TestSearchRoutes(t *testing.T) { img := CreateRandomImage() - err = UploadImageWithBasicAuth(img, baseURL, repoName, img.DigestStr(), user1, password1) + err := UploadImageWithBasicAuth(img, baseURL, repoName, img.DigestStr(), user1, password1) So(err, ShouldBeNil) }) @@ -8854,7 +8808,7 @@ func TestSearchRoutes(t *testing.T) { img := CreateRandomImage() - err = UploadImageWithBasicAuth(img, baseURL, repoName, img.DigestStr(), "", "") + err := UploadImageWithBasicAuth(img, baseURL, repoName, img.DigestStr(), "", "") So(err, ShouldBeNil) }) }) diff --git a/pkg/cli/client/config_cmd.go b/pkg/cli/client/config_cmd.go index cf32d0c35d..f17ec14cb9 100644 --- a/pkg/cli/client/config_cmd.go +++ b/pkg/cli/client/config_cmd.go @@ -37,7 +37,7 @@ func NewConfigCommand() *cobra.Command { RunE: func(cmd *cobra.Command, args []string) error { home, err := os.UserHomeDir() if err != nil { - panic(err) + return err } configPath := path.Join(home, "/.zot") @@ -112,7 +112,7 @@ func NewConfigAddCommand() *cobra.Command { RunE: func(cmd *cobra.Command, args []string) error { home, err := os.UserHomeDir() if err != nil { - panic(err) + return err } configPath := path.Join(home, "/.zot") @@ -142,7 +142,7 @@ func NewConfigRemoveCommand() *cobra.Command { RunE: func(cmd *cobra.Command, args []string) error { home, err := os.UserHomeDir() if err != nil { - panic(err) + return err } configPath := path.Join(home, "/.zot") diff --git a/pkg/cli/server/config_reloader.go b/pkg/cli/server/config_reloader.go index 91e22105dc..2c90499ea1 100644 --- a/pkg/cli/server/config_reloader.go +++ b/pkg/cli/server/config_reloader.go @@ -105,15 +105,13 @@ func (hr *HotReloader) Start() context.Context { } // watch for errors case err := <-hr.watcher.Errors: - log.Error().Err(err).Str("config", hr.filePath).Msg("fsnotfy error while watching config") - panic(err) + log.Panic().Err(err).Str("config", hr.filePath).Msg("fsnotfy error while watching config") } } }() if err := hr.watcher.Add(hr.filePath); err != nil { - log.Error().Err(err).Str("config", hr.filePath).Msg("error adding config file to FsNotify watcher") - panic(err) + log.Panic().Err(err).Str("config", hr.filePath).Msg("error adding config file to FsNotify watcher") } <-done diff --git a/pkg/cli/server/extensions_test.go b/pkg/cli/server/extensions_test.go index c27c9948dc..7cb1338726 100644 --- a/pkg/cli/server/extensions_test.go +++ b/pkg/cli/server/extensions_test.go @@ -61,7 +61,7 @@ func TestVerifyExtensionsConfig(t *testing.T) { So(err, ShouldBeNil) os.Args = []string{"cli_test", "verify", tmpfile.Name()} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldPanic) + So(cli.NewServerRootCmd().Execute(), ShouldNotBeNil) content = []byte(`{ "storage":{ @@ -100,7 +100,7 @@ func TestVerifyExtensionsConfig(t *testing.T) { So(err, ShouldBeNil) os.Args = []string{"cli_test", "verify", tmpfile.Name()} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldPanic) + So(cli.NewServerRootCmd().Execute(), ShouldNotBeNil) }) Convey("Test verify w/ sync and w/o filesystem storage", t, func(c C) { @@ -117,7 +117,7 @@ func TestVerifyExtensionsConfig(t *testing.T) { err = tmpfile.Close() So(err, ShouldBeNil) os.Args = []string{"cli_test", "verify", tmpfile.Name()} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldPanic) + So(cli.NewServerRootCmd().Execute(), ShouldNotBeNil) }) Convey("Test verify w/ sync and w/ filesystem storage", t, func(c C) { @@ -134,7 +134,7 @@ func TestVerifyExtensionsConfig(t *testing.T) { err = tmpfile.Close() So(err, ShouldBeNil) os.Args = []string{"cli_test", "verify", tmpfile.Name()} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldNotPanic) + So(cli.NewServerRootCmd().Execute(), ShouldBeNil) }) Convey("Test verify with bad sync prefixes", t, func(c C) { @@ -152,7 +152,7 @@ func TestVerifyExtensionsConfig(t *testing.T) { err = tmpfile.Close() So(err, ShouldBeNil) os.Args = []string{"cli_test", "verify", tmpfile.Name()} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldPanic) + So(cli.NewServerRootCmd().Execute(), ShouldNotBeNil) }) Convey("Test verify with bad sync content config", t, func(c C) { @@ -170,7 +170,7 @@ func TestVerifyExtensionsConfig(t *testing.T) { err = tmpfile.Close() So(err, ShouldBeNil) os.Args = []string{"cli_test", "verify", tmpfile.Name()} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldPanic) + So(cli.NewServerRootCmd().Execute(), ShouldNotBeNil) }) Convey("Test verify with good sync content config", t, func(c C) { @@ -225,7 +225,7 @@ func TestVerifyExtensionsConfig(t *testing.T) { err = tmpfile.Close() So(err, ShouldBeNil) os.Args = []string{"cli_test", "verify", tmpfile.Name()} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldPanic) + So(cli.NewServerRootCmd().Execute(), ShouldNotBeNil) }) } @@ -374,10 +374,11 @@ func TestServeExtensions(t *testing.T) { logFile, err := os.CreateTemp("", "zot-log*.txt") So(err, ShouldBeNil) defer os.Remove(logFile.Name()) // clean up + tmpFile := t.TempDir() content := fmt.Sprintf(`{ "storage": { - "rootDirectory": "/tmp/zot" + "rootDirectory": "%s" }, "http": { "address": "127.0.0.1", @@ -387,7 +388,7 @@ func TestServeExtensions(t *testing.T) { "level": "debug", "output": "%s" } - }`, port, logFile.Name()) + }`, tmpFile, port, logFile.Name()) cfgfile, err := os.CreateTemp("", "zot-test*.json") So(err, ShouldBeNil) @@ -399,8 +400,10 @@ func TestServeExtensions(t *testing.T) { os.Args = []string{"cli_test", "serve", cfgfile.Name()} go func() { - err = cli.NewServerRootCmd().Execute() - So(err, ShouldBeNil) + Convey("run", t, func() { + err = cli.NewServerRootCmd().Execute() + So(err, ShouldBeNil) + }) }() WaitTillServerReady(baseURL) @@ -415,10 +418,11 @@ func TestServeExtensions(t *testing.T) { logFile, err := os.CreateTemp("", "zot-log*.txt") So(err, ShouldBeNil) defer os.Remove(logFile.Name()) // clean up + tmpFile := t.TempDir() content := fmt.Sprintf(`{ "storage": { - "rootDirectory": "/tmp/zot" + "rootDirectory": "%s" }, "http": { "address": "127.0.0.1", @@ -430,7 +434,7 @@ func TestServeExtensions(t *testing.T) { }, "extensions": { } - }`, port, logFile.Name()) + }`, tmpFile, port, logFile.Name()) cfgfile, err := os.CreateTemp("", "zot-test*.json") So(err, ShouldBeNil) @@ -441,9 +445,12 @@ func TestServeExtensions(t *testing.T) { So(err, ShouldBeNil) os.Args = []string{"cli_test", "serve", cfgfile.Name()} + go func() { - err = cli.NewServerRootCmd().Execute() - So(err, ShouldBeNil) + Convey("run", t, func() { + err = cli.NewServerRootCmd().Execute() + So(err, ShouldBeNil) + }) }() WaitTillServerReady(baseURL) @@ -454,7 +461,8 @@ func TestServeExtensions(t *testing.T) { }) } -func testWithMetricsEnabled(cfgContentFormat string) { +func testWithMetricsEnabled(t *testing.T, cfgContentFormat string) { + t.Helper() port := GetFreePort() baseURL := GetBaseURL(port) logFile, err := os.CreateTemp("", "zot-log*.txt") @@ -475,8 +483,10 @@ func testWithMetricsEnabled(cfgContentFormat string) { os.Args = []string{"cli_test", "serve", cfgfile.Name()} go func() { - err = cli.NewServerRootCmd().Execute() - So(err, ShouldBeNil) + Convey("run", t, func() { + err = cli.NewServerRootCmd().Execute() + So(err, ShouldBeNil) + }) }() WaitTillServerReady(baseURL) @@ -500,9 +510,11 @@ func TestServeMetricsExtension(t *testing.T) { defer func() { os.Args = oldArgs }() Convey("no explicit enable", t, func(c C) { + tmpFile := t.TempDir() + content := `{ "storage": { - "rootDirectory": "/tmp/zot" + "rootDirectory": "` + tmpFile + `" }, "http": { "address": "127.0.0.1", @@ -517,13 +529,15 @@ func TestServeMetricsExtension(t *testing.T) { } } }` - testWithMetricsEnabled(content) + testWithMetricsEnabled(t, content) }) Convey("no explicit enable but with prometheus parameter", t, func(c C) { + tmpFile := t.TempDir() + content := `{ "storage": { - "rootDirectory": "/tmp/zot" + "rootDirectory": "` + tmpFile + `" }, "http": { "address": "127.0.0.1", @@ -541,13 +555,15 @@ func TestServeMetricsExtension(t *testing.T) { } } }` - testWithMetricsEnabled(content) + testWithMetricsEnabled(t, content) }) Convey("with explicit enable, but without prometheus parameter", t, func(c C) { + tmpFile := t.TempDir() + content := `{ "storage": { - "rootDirectory": "/tmp/zot" + "rootDirectory": "` + tmpFile + `" }, "http": { "address": "127.0.0.1", @@ -563,7 +579,7 @@ func TestServeMetricsExtension(t *testing.T) { } } }` - testWithMetricsEnabled(content) + testWithMetricsEnabled(t, content) }) Convey("with explicit disable", t, func(c C) { @@ -571,11 +587,12 @@ func TestServeMetricsExtension(t *testing.T) { baseURL := GetBaseURL(port) logFile, err := os.CreateTemp("", "zot-log*.txt") So(err, ShouldBeNil) + tmpFile := t.TempDir() defer os.Remove(logFile.Name()) // clean up content := fmt.Sprintf(`{ "storage": { - "rootDirectory": "/tmp/zot" + "rootDirectory": "`+tmpFile+`" }, "http": { "address": "127.0.0.1", @@ -602,8 +619,10 @@ func TestServeMetricsExtension(t *testing.T) { os.Args = []string{"cli_test", "serve", cfgfile.Name()} go func() { - err = cli.NewServerRootCmd().Execute() - So(err, ShouldBeNil) + Convey("run", t, func() { + err = cli.NewServerRootCmd().Execute() + So(err, ShouldBeNil) + }) }() WaitTillServerReady(baseURL) diff --git a/pkg/cli/server/root.go b/pkg/cli/server/root.go index c4088fa806..03e8855230 100644 --- a/pkg/cli/server/root.go +++ b/pkg/cli/server/root.go @@ -43,10 +43,10 @@ func newServeCmd(conf *config.Config) *cobra.Command { Aliases: []string{"serve"}, Short: "`serve` stores and distributes OCI images", Long: "`serve` stores and distributes OCI images", - Run: func(cmd *cobra.Command, args []string) { + RunE: func(cmd *cobra.Command, args []string) error { if len(args) > 0 { if err := LoadConfiguration(conf, args[0]); err != nil { - panic(err) + return err } } @@ -57,7 +57,7 @@ func newServeCmd(conf *config.Config) *cobra.Command { if err != nil { ctlr.Log.Error().Err(err).Msg("failed to create a new hot reloader") - panic(err) + return err } /* context used to cancel go routines so that @@ -67,12 +67,14 @@ func newServeCmd(conf *config.Config) *cobra.Command { if err := ctlr.Init(reloaderCtx); err != nil { ctlr.Log.Error().Err(err).Msg("failed to init controller") - panic(err) + return err } if err := ctlr.Run(reloaderCtx); err != nil { - ctlr.Log.Fatal().Err(err).Msg("unable to start controller, exiting") + log.Error().Err(err).Msg("unable to start controller, exiting") } + + return nil }, } @@ -86,17 +88,17 @@ func newScrubCmd(conf *config.Config) *cobra.Command { Aliases: []string{"scrub"}, Short: "`scrub` checks manifest/blob integrity", Long: "`scrub` checks manifest/blob integrity", - Run: func(cmd *cobra.Command, args []string) { + RunE: func(cmd *cobra.Command, args []string) error { if len(args) > 0 { if err := LoadConfiguration(conf, args[0]); err != nil { - panic(err) + return err } } else { if err := cmd.Usage(); err != nil { - panic(err) + return err } - return + return nil } // checking if the server is already running @@ -106,30 +108,34 @@ func newScrubCmd(conf *config.Config) *cobra.Command { nil) if err != nil { log.Error().Err(err).Msg("unable to create a new http request") - panic(err) + + return err } response, err := http.DefaultClient.Do(req) if err == nil { response.Body.Close() log.Warn().Msg("The server is running, in order to perform the scrub command the server should be shut down") - panic("Error: server is running") + + return zerr.ErrServerIsRunning } else { // server is down ctlr := api.NewController(conf) ctlr.Metrics = monitoring.NewMetricsServer(false, ctlr.Log) if err := ctlr.InitImageStore(); err != nil { - panic(err) + return err } result, err := ctlr.StoreController.CheckAllBlobsIntegrity(cmd.Context()) if err != nil { - panic(err) + return err } result.PrintScrubResults(cmd.OutOrStdout()) } + + return nil }, } @@ -143,15 +149,18 @@ func newVerifyCmd(conf *config.Config) *cobra.Command { Aliases: []string{"verify"}, Short: "`verify` validates a zot config file", Long: "`verify` validates a zot config file", - Run: func(cmd *cobra.Command, args []string) { + RunE: func(cmd *cobra.Command, args []string) error { if len(args) > 0 { if err := LoadConfiguration(conf, args[0]); err != nil { log.Error().Str("config", args[0]).Msg("Config file is invalid") - panic(err) + + return err } log.Info().Str("config", args[0]).Msg("Config file is valid") } + + return nil }, } @@ -167,7 +176,7 @@ func NewServerRootCmd() *cobra.Command { Use: "zot", Short: "`zot`", Long: "`zot`", - Run: func(cmd *cobra.Command, args []string) { + RunE: func(cmd *cobra.Command, args []string) error { if showVersion { log.Info().Str("distribution-spec", distspec.Version).Str("commit", config.Commit). Str("binary-type", config.BinaryType).Str("go version", config.GoVersion).Msg("version") @@ -175,6 +184,8 @@ func NewServerRootCmd() *cobra.Command { _ = cmd.Usage() cmd.SilenceErrors = false } + + return nil }, } diff --git a/pkg/cli/server/root_test.go b/pkg/cli/server/root_test.go index c97c0277d4..fcdcafe996 100644 --- a/pkg/cli/server/root_test.go +++ b/pkg/cli/server/root_test.go @@ -49,12 +49,14 @@ func TestServe(t *testing.T) { Convey("Test serve config", t, func(c C) { Convey("unknown config", func(c C) { os.Args = []string{"cli_test", "serve", path.Join(os.TempDir(), "/x")} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldPanic) + err := cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) }) Convey("non-existent config", func(c C) { os.Args = []string{"cli_test", "serve", path.Join(os.TempDir(), "/x.yaml")} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldPanic) + err := cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) }) Convey("bad config", func(c C) { @@ -66,7 +68,8 @@ func TestServe(t *testing.T) { os.Args = []string{"cli_test", "serve", tmpFile} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldPanic) + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) }) Convey("config with missing rootDir", func(c C) { @@ -87,7 +90,8 @@ func TestServe(t *testing.T) { os.Args = []string{"cli_test", "serve", tmpFile} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldPanic) + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) // wait for the config reloader goroutine to start watching the config file // if we end the test too fast it will delete the config file @@ -112,7 +116,88 @@ func TestVerify(t *testing.T) { err = tmpfile.Close() So(err, ShouldBeNil) os.Args = []string{"cli_test", "verify", tmpfile.Name()} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldPanic) + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) + }) + + Convey("Test verify CVE warn for remote storage", t, func(c C) { + tmpfile, err := os.CreateTemp("", "zot-test*.json") + So(err, ShouldBeNil) + defer os.Remove(tmpfile.Name()) // clean up + + content := []byte(`{ + "storage":{ + "rootDirectory":"/tmp/zot", + "dedupe":true, + "remoteCache":false, + "storageDriver":{ + "name":"s3", + "rootdirectory":"/zot", + "region":"us-east-2", + "bucket":"zot-storage", + "secure":true, + "skipverify":false + } + }, + "http":{ + "address":"127.0.0.1", + "port":"8080" + }, + "extensions":{ + "search": { + "enable": true, + "cve": { + "updateInterval": "24h" + } + } + } + }`) + err = os.WriteFile(tmpfile.Name(), content, 0o0600) + So(err, ShouldBeNil) + + os.Args = []string{"cli_test", "verify", tmpfile.Name()} + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) + + content = []byte(`{ + "storage":{ + "rootDirectory":"/tmp/zot", + "dedupe":true, + "remoteCache":false, + "subPaths":{ + "/a": { + "rootDirectory": "/tmp/zot1", + "dedupe": false, + "storageDriver":{ + "name":"s3", + "rootdirectory":"/zot-a", + "region":"us-east-2", + "bucket":"zot-storage", + "secure":true, + "skipverify":false + } + } + } + }, + "http":{ + "address":"127.0.0.1", + "port":"8080" + }, + "extensions":{ + "search": { + "enable": true, + "cve": { + "updateInterval": "24h" + } + } + } + }`) + err = os.WriteFile(tmpfile.Name(), content, 0o0600) + So(err, ShouldBeNil) + + os.Args = []string{"cli_test", "verify", tmpfile.Name()} + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) }) Convey("Test cached db config", t, func(c C) { @@ -151,7 +236,8 @@ func TestVerify(t *testing.T) { So(err, ShouldBeNil) os.Args = []string{"cli_test", "verify", tmpfile.Name()} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldPanic) + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) // local storage with remote caching content = []byte(`{ @@ -182,7 +268,8 @@ func TestVerify(t *testing.T) { So(err, ShouldBeNil) os.Args = []string{"cli_test", "verify", tmpfile.Name()} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldPanic) + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) // unsupported cache driver content = []byte(`{ @@ -218,7 +305,8 @@ func TestVerify(t *testing.T) { So(err, ShouldBeNil) os.Args = []string{"cli_test", "verify", tmpfile.Name()} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldPanic) + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) // remoteCache false but provided cacheDriver config, ignored content = []byte(`{ @@ -258,7 +346,8 @@ func TestVerify(t *testing.T) { So(err, ShouldBeNil) os.Args = []string{"cli_test", "verify", tmpfile.Name()} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldNotPanic) + err = cli.NewServerRootCmd().Execute() + So(err, ShouldBeNil) // SubPaths // dedupe true, remote storage, remoteCache true, but no cacheDriver (remote) @@ -298,7 +387,8 @@ func TestVerify(t *testing.T) { So(err, ShouldBeNil) os.Args = []string{"cli_test", "verify", tmpfile.Name()} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldPanic) + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) // local storage with remote caching content = []byte(`{ @@ -335,7 +425,8 @@ func TestVerify(t *testing.T) { So(err, ShouldBeNil) os.Args = []string{"cli_test", "verify", tmpfile.Name()} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldPanic) + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) // unsupported cache driver content = []byte(`{ @@ -377,7 +468,8 @@ func TestVerify(t *testing.T) { So(err, ShouldBeNil) os.Args = []string{"cli_test", "verify", tmpfile.Name()} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldPanic) + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) // remoteCache false but provided cacheDriver config, ignored content = []byte(`{ @@ -414,7 +506,8 @@ func TestVerify(t *testing.T) { So(err, ShouldBeNil) os.Args = []string{"cli_test", "verify", tmpfile.Name()} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldNotPanic) + err = cli.NewServerRootCmd().Execute() + So(err, ShouldBeNil) }) Convey("Test apply defaults cache db", t, func(c C) { @@ -436,7 +529,8 @@ func TestVerify(t *testing.T) { So(err, ShouldBeNil) os.Args = []string{"cli_test", "verify", tmpfile.Name()} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldPanic) + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) // subpath s3 dedup=false, check for previous dedup usage and set to true if cachedb found cacheDir = t.TempDir() @@ -453,7 +547,8 @@ func TestVerify(t *testing.T) { So(err, ShouldBeNil) os.Args = []string{"cli_test", "verify", tmpfile.Name()} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldPanic) + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) // subpath s3 dedup=false, check for previous dedup usage and set to true if cachedb found cacheDir = t.TempDir() @@ -467,7 +562,8 @@ func TestVerify(t *testing.T) { So(err, ShouldBeNil) os.Args = []string{"cli_test", "verify", tmpfile.Name()} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldPanic) + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) }) Convey("Test verify storage driver different than s3", t, func(c C) { @@ -482,7 +578,8 @@ func TestVerify(t *testing.T) { err = tmpfile.Close() So(err, ShouldBeNil) os.Args = []string{"cli_test", "verify", tmpfile.Name()} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldPanic) + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) }) Convey("Test verify subpath storage driver different than s3", t, func(c C) { @@ -498,7 +595,8 @@ func TestVerify(t *testing.T) { err = tmpfile.Close() So(err, ShouldBeNil) os.Args = []string{"cli_test", "verify", tmpfile.Name()} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldPanic) + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) }) Convey("Test verify subpath storage config", t, func(c C) { @@ -524,7 +622,8 @@ func TestVerify(t *testing.T) { err = os.WriteFile(tmpfile.Name(), content, 0o0600) So(err, ShouldBeNil) os.Args = []string{"cli_test", "verify", tmpfile.Name()} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldPanic) + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) // sub paths that point to default root directory should not be allowed. content = []byte(`{"storage":{"rootDirectory":"/tmp/zot", @@ -534,7 +633,8 @@ func TestVerify(t *testing.T) { err = os.WriteFile(tmpfile.Name(), content, 0o0600) So(err, ShouldBeNil) os.Args = []string{"cli_test", "verify", tmpfile.Name()} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldPanic) + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) content = []byte(`{"storage":{"rootDirectory":"/tmp/zot", "subPaths": {"/a": {"rootDirectory": "/zot-a","dedupe":"true","gc":"true"}, @@ -544,7 +644,8 @@ func TestVerify(t *testing.T) { err = os.WriteFile(tmpfile.Name(), content, 0o0600) So(err, ShouldBeNil) os.Args = []string{"cli_test", "verify", tmpfile.Name()} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldPanic) + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) content = []byte(`{"storage":{"rootDirectory":"/tmp/zot", "subPaths": {"/a": {"rootDirectory": "/zot-a","dedupe":"true","gc":"true"}, @@ -554,7 +655,8 @@ func TestVerify(t *testing.T) { err = os.WriteFile(tmpfile.Name(), content, 0o0600) So(err, ShouldBeNil) os.Args = []string{"cli_test", "verify", tmpfile.Name()} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldPanic) + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) content = []byte(`{"storage":{"rootDirectory":"/tmp/zot", "subPaths": {"/a": {"rootDirectory": "/zot-a","dedupe":"true","gc":"true","gcDelay":"1s","gcInterval":"1s"}, @@ -564,7 +666,8 @@ func TestVerify(t *testing.T) { err = os.WriteFile(tmpfile.Name(), content, 0o0600) So(err, ShouldBeNil) os.Args = []string{"cli_test", "verify", tmpfile.Name()} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldPanic) + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) content = []byte(`{"storage":{"rootDirectory":"/tmp/zot", "subPaths": {"/a": {"rootDirectory": "/tmp/zot","dedupe":"true","gc":"true","gcDelay":"1s","gcInterval":"1s"}, @@ -574,7 +677,8 @@ func TestVerify(t *testing.T) { err = os.WriteFile(tmpfile.Name(), content, 0o0600) So(err, ShouldBeNil) os.Args = []string{"cli_test", "verify", tmpfile.Name()} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldPanic) + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) }) Convey("Test verify w/ authorization and w/o authentication", t, func(c C) { @@ -590,7 +694,8 @@ func TestVerify(t *testing.T) { err = tmpfile.Close() So(err, ShouldBeNil) os.Args = []string{"cli_test", "verify", tmpfile.Name()} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldPanic) + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) }) Convey("Test verify w/ authorization and w/ authentication", t, func(c C) { @@ -607,7 +712,8 @@ func TestVerify(t *testing.T) { err = tmpfile.Close() So(err, ShouldBeNil) os.Args = []string{"cli_test", "verify", tmpfile.Name()} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldNotPanic) + err = cli.NewServerRootCmd().Execute() + So(err, ShouldBeNil) }) Convey("Test verify anonymous authorization", t, func(c C) { @@ -624,7 +730,8 @@ func TestVerify(t *testing.T) { err = tmpfile.Close() So(err, ShouldBeNil) os.Args = []string{"cli_test", "verify", tmpfile.Name()} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldNotPanic) + err = cli.NewServerRootCmd().Execute() + So(err, ShouldBeNil) }) Convey("Test verify admin policy authz is not allowed if no authn is configured", t, func(c C) { @@ -649,7 +756,8 @@ func TestVerify(t *testing.T) { err = tmpfile.Close() So(err, ShouldBeNil) os.Args = []string{"cli_test", "verify", tmpfile.Name()} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldPanic) + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) }) Convey("Test verify default policy authz is not allowed if no authn is configured", t, func(c C) { @@ -670,7 +778,8 @@ func TestVerify(t *testing.T) { err = tmpfile.Close() So(err, ShouldBeNil) os.Args = []string{"cli_test", "verify", tmpfile.Name()} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldPanic) + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) }) Convey("Test verify authz per user policies fail if no authn is configured", t, func(c C) { @@ -696,7 +805,101 @@ func TestVerify(t *testing.T) { err = tmpfile.Close() So(err, ShouldBeNil) os.Args = []string{"cli_test", "verify", tmpfile.Name()} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldPanic) + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) + }) + + Convey("Test verify w/ sync and w/o filesystem storage", t, func(c C) { + tmpfile, err := os.CreateTemp("", "zot-test*.json") + So(err, ShouldBeNil) + defer os.Remove(tmpfile.Name()) // clean up + content := []byte(`{"storage":{"rootDirectory":"/tmp/zot", "storageDriver": {"name": "s3"}}, + "http":{"address":"127.0.0.1","port":"8080","realm":"zot", + "auth":{"htpasswd":{"path":"test/data/htpasswd"},"failDelay":1}}, + "extensions":{"sync": {"registries": [{"urls":["localhost:9999"], + "maxRetries": 1, "retryDelay": "10s"}]}}}`) + _, err = tmpfile.Write(content) + So(err, ShouldBeNil) + err = tmpfile.Close() + So(err, ShouldBeNil) + os.Args = []string{"cli_test", "verify", tmpfile.Name()} + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) + }) + + Convey("Test verify w/ sync and w/ filesystem storage", t, func(c C) { + tmpfile, err := os.CreateTemp("", "zot-test*.json") + So(err, ShouldBeNil) + defer os.Remove(tmpfile.Name()) // clean up + content := []byte(`{"storage":{"rootDirectory":"/tmp/zot"}, + "http":{"address":"127.0.0.1","port":"8080","realm":"zot", + "auth":{"htpasswd":{"path":"test/data/htpasswd"},"failDelay":1}}, + "extensions":{"sync": {"registries": [{"urls":["localhost:9999"], + "maxRetries": 1, "retryDelay": "10s"}]}}}`) + _, err = tmpfile.Write(content) + So(err, ShouldBeNil) + err = tmpfile.Close() + So(err, ShouldBeNil) + os.Args = []string{"cli_test", "verify", tmpfile.Name()} + err = cli.NewServerRootCmd().Execute() + So(err, ShouldBeNil) + }) + + Convey("Test verify with bad sync prefixes", t, func(c C) { + tmpfile, err := os.CreateTemp("", "zot-test*.json") + So(err, ShouldBeNil) + defer os.Remove(tmpfile.Name()) // clean up + content := []byte(`{"storage":{"rootDirectory":"/tmp/zot"}, + "http":{"address":"127.0.0.1","port":"8080","realm":"zot", + "auth":{"htpasswd":{"path":"test/data/htpasswd"},"failDelay":1}}, + "extensions":{"sync": {"registries": [{"urls":["localhost:9999"], + "maxRetries": 1, "retryDelay": "10s", + "content": [{"prefix":"[repo%^&"}]}]}}}`) + _, err = tmpfile.Write(content) + So(err, ShouldBeNil) + err = tmpfile.Close() + So(err, ShouldBeNil) + os.Args = []string{"cli_test", "verify", tmpfile.Name()} + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) + }) + + Convey("Test verify with bad sync content config", t, func(c C) { + tmpfile, err := os.CreateTemp("", "zot-test*.json") + So(err, ShouldBeNil) + defer os.Remove(tmpfile.Name()) // clean up + content := []byte(`{"storage":{"rootDirectory":"/tmp/zot"}, + "http":{"address":"127.0.0.1","port":"8080","realm":"zot", + "auth":{"htpasswd":{"path":"test/data/htpasswd"},"failDelay":1}}, + "extensions":{"sync": {"registries": [{"urls":["localhost:9999"], + "maxRetries": 1, "retryDelay": "10s", + "content": [{"prefix":"zot-repo","stripPrefix":true,"destination":"/"}]}]}}}`) + _, err = tmpfile.Write(content) + So(err, ShouldBeNil) + err = tmpfile.Close() + So(err, ShouldBeNil) + os.Args = []string{"cli_test", "verify", tmpfile.Name()} + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) + }) + + Convey("Test verify with good sync content config", t, func(c C) { + tmpfile, err := os.CreateTemp("", "zot-test*.json") + So(err, ShouldBeNil) + defer os.Remove(tmpfile.Name()) // clean up + content := []byte(`{"storage":{"rootDirectory":"/tmp/zot"}, + "http":{"address":"127.0.0.1","port":"8080","realm":"zot", + "auth":{"htpasswd":{"path":"test/data/htpasswd"},"failDelay":1}}, + "extensions":{"sync": {"registries": [{"urls":["localhost:9999"], + "maxRetries": 1, "retryDelay": "10s", + "content": [{"prefix":"zot-repo/*","stripPrefix":true,"destination":"/"}]}]}}}`) + _, err = tmpfile.Write(content) + So(err, ShouldBeNil) + err = tmpfile.Close() + So(err, ShouldBeNil) + os.Args = []string{"cli_test", "verify", tmpfile.Name()} + err = cli.NewServerRootCmd().Execute() + So(err, ShouldBeNil) }) Convey("Test verify with bad authorization repo patterns", t, func(c C) { @@ -712,7 +915,45 @@ func TestVerify(t *testing.T) { err = tmpfile.Close() So(err, ShouldBeNil) os.Args = []string{"cli_test", "verify", tmpfile.Name()} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldPanic) + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) + }) + + Convey("Test verify sync config default tls value", t, func(c C) { + tmpfile, err := os.CreateTemp("", "zot-test*.json") + So(err, ShouldBeNil) + defer os.Remove(tmpfile.Name()) // clean up + content := []byte(`{"storage":{"rootDirectory":"/tmp/zot"}, + "http":{"address":"127.0.0.1","port":"8080","realm":"zot", + "auth":{"htpasswd":{"path":"test/data/htpasswd"},"failDelay":1}}, + "extensions":{"sync": {"registries": [{"urls":["localhost:9999"], + "maxRetries": 1, "retryDelay": "10s", + "content": [{"prefix":"repo**"}]}]}}}`) + _, err = tmpfile.Write(content) + So(err, ShouldBeNil) + err = tmpfile.Close() + So(err, ShouldBeNil) + os.Args = []string{"cli_test", "verify", tmpfile.Name()} + err = cli.NewServerRootCmd().Execute() + So(err, ShouldBeNil) + }) + + Convey("Test verify sync without retry options", t, func(c C) { + tmpfile, err := os.CreateTemp("", "zot-test*.json") + So(err, ShouldBeNil) + defer os.Remove(tmpfile.Name()) // clean up + content := []byte(`{"storage":{"rootDirectory":"/tmp/zot"}, + "http":{"address":"127.0.0.1","port":"8080","realm":"zot", + "auth":{"htpasswd":{"path":"test/data/htpasswd"},"failDelay":1}}, + "extensions":{"sync": {"registries": [{"urls":["localhost:9999"], + "maxRetries": 10, "content": [{"prefix":"repo**"}]}]}}}`) + _, err = tmpfile.Write(content) + So(err, ShouldBeNil) + err = tmpfile.Close() + So(err, ShouldBeNil) + os.Args = []string{"cli_test", "verify", tmpfile.Name()} + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) }) Convey("Test verify config with unknown keys", t, func(c C) { @@ -727,7 +968,8 @@ func TestVerify(t *testing.T) { err = tmpfile.Close() So(err, ShouldBeNil) os.Args = []string{"cli_test", "verify", tmpfile.Name()} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldPanic) + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) }) Convey("Test verify openid config with missing parameter", t, func(c C) { @@ -743,7 +985,8 @@ func TestVerify(t *testing.T) { err = tmpfile.Close() So(err, ShouldBeNil) os.Args = []string{"cli_test", "verify", tmpfile.Name()} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldPanic) + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) }) Convey("Test verify oauth2 config with missing parameter", t, func(c C) { @@ -759,7 +1002,8 @@ func TestVerify(t *testing.T) { err = tmpfile.Close() So(err, ShouldBeNil) os.Args = []string{"cli_test", "verify", tmpfile.Name()} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldPanic) + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) }) Convey("Test verify openid config with unsupported provider", t, func(c C) { @@ -775,7 +1019,8 @@ func TestVerify(t *testing.T) { err = tmpfile.Close() So(err, ShouldBeNil) os.Args = []string{"cli_test", "verify", tmpfile.Name()} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldPanic) + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) }) Convey("Test verify openid config without apikey extension enabled", t, func(c C) { @@ -792,7 +1037,8 @@ func TestVerify(t *testing.T) { err = tmpfile.Close() So(err, ShouldBeNil) os.Args = []string{"cli_test", "verify", tmpfile.Name()} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldNotPanic) + err = cli.NewServerRootCmd().Execute() + So(err, ShouldBeNil) }) Convey("Test verify config with missing basedn key", t, func(c C) { @@ -808,7 +1054,8 @@ func TestVerify(t *testing.T) { err = tmpfile.Close() So(err, ShouldBeNil) os.Args = []string{"cli_test", "verify", tmpfile.Name()} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldPanic) + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) }) Convey("Test verify config with missing address key", t, func(c C) { @@ -824,7 +1071,8 @@ func TestVerify(t *testing.T) { err = tmpfile.Close() So(err, ShouldBeNil) os.Args = []string{"cli_test", "verify", tmpfile.Name()} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldPanic) + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) }) Convey("Test verify config with missing userattribute key", t, func(c C) { @@ -840,7 +1088,8 @@ func TestVerify(t *testing.T) { err = tmpfile.Close() So(err, ShouldBeNil) os.Args = []string{"cli_test", "verify", tmpfile.Name()} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldPanic) + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) }) Convey("Test verify good config", t, func(c C) { @@ -1218,12 +1467,14 @@ func TestScrub(t *testing.T) { Convey("Test scrub config", t, func(c C) { Convey("non-existent config", func(c C) { os.Args = []string{"cli_test", "scrub", path.Join(os.TempDir(), "/x.yaml")} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldPanic) + err := cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) }) Convey("unknown config", func(c C) { os.Args = []string{"cli_test", "scrub", path.Join(os.TempDir(), "/x")} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldPanic) + err := cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) }) Convey("bad config", func(c C) { @@ -1236,7 +1487,8 @@ func TestScrub(t *testing.T) { err = tmpfile.Close() So(err, ShouldBeNil) os.Args = []string{"cli_test", "scrub", tmpfile.Name()} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldPanic) + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) }) Convey("server is running", func(c C) { @@ -1272,7 +1524,8 @@ func TestScrub(t *testing.T) { So(err, ShouldBeNil) os.Args = []string{"cli_test", "scrub", tmpfile.Name()} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldPanic) + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) defer ctrlManager.StopServer() }) @@ -1300,7 +1553,8 @@ func TestScrub(t *testing.T) { err = tmpfile.Close() So(err, ShouldBeNil) os.Args = []string{"cli_test", "scrub", tmpfile.Name()} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldPanic) + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) }) Convey("bad index.json", func(c C) { @@ -1354,7 +1608,8 @@ func TestScrub(t *testing.T) { So(err, ShouldBeNil) os.Args = []string{"cli_test", "scrub", tmpfile.Name()} - So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldPanic) + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) }) }) } diff --git a/pkg/compliance/v1_0_0/check.go b/pkg/compliance/v1_0_0/check.go index 54e856aeef..31d5eefcbd 100644 --- a/pkg/compliance/v1_0_0/check.go +++ b/pkg/compliance/v1_0_0/check.go @@ -30,7 +30,7 @@ func CheckWorkflows(t *testing.T, config *compliance.Config) { t.Helper() if config == nil || config.Address == "" || config.Port == "" { - panic("insufficient config") + t.Fatal("insufficient config") } if config.OutputJSON { diff --git a/pkg/exporter/cli/cli.go b/pkg/exporter/cli/cli.go index 27a5471974..8bf9e59364 100644 --- a/pkg/exporter/cli/cli.go +++ b/pkg/exporter/cli/cli.go @@ -60,18 +60,15 @@ func loadConfiguration(config *api.Config, configPath string) { viper.SetConfigFile(configPath) if err := viper.ReadInConfig(); err != nil { - log.Error().Err(err).Msg("Error while reading configuration") - panic(err) + log.Panic().Err(err).Msg("Error while reading configuration") } metaData := &mapstructure.Metadata{} if err := viper.Unmarshal(&config, metadataConfig(metaData)); err != nil { - log.Error().Err(err).Msg("Error while unmarshaling new config") - panic(err) + log.Panic().Err(err).Msg("Error while unmarshaling new config") } if len(metaData.Keys) == 0 || len(metaData.Unused) > 0 { - log.Error().Err(zerr.ErrBadConfig).Msg("Bad configuration, retry writing it") - panic(zerr.ErrBadConfig) + log.Panic().Err(zerr.ErrBadConfig).Msg("Bad configuration, retry writing it") } } diff --git a/pkg/meta/meta.go b/pkg/meta/meta.go index ef685676e8..db31e5f663 100644 --- a/pkg/meta/meta.go +++ b/pkg/meta/meta.go @@ -43,7 +43,10 @@ func Create(dbtype string, dbDriver, parameters interface{}, log log.Logger, //n { properDriver, ok := dbDriver.(*bbolt.DB) if !ok { - panic("failed type assertion") + log.Error().Err(errors.ErrTypeAssertionFailed).Msgf("expected type '%T' but got '%T'", + &bbolt.DB{}, dbDriver) + + return nil, errors.ErrTypeAssertionFailed } return boltdb.New(properDriver, log) @@ -52,12 +55,18 @@ func Create(dbtype string, dbDriver, parameters interface{}, log log.Logger, //n { properDriver, ok := dbDriver.(*dynamodb.Client) if !ok { - panic("failed type assertion") + log.Error().Err(errors.ErrTypeAssertionFailed).Msgf("expected type '%T' but got '%T'", + &dynamodb.Client{}, dbDriver) + + return nil, errors.ErrTypeAssertionFailed } properParameters, ok := parameters.(mdynamodb.DBDriverParameters) if !ok { - panic("failed type assertion") + log.Error().Err(errors.ErrTypeAssertionFailed).Msgf("expected type '%T' but got '%T'", + mdynamodb.DBDriverParameters{}, parameters) + + return nil, errors.ErrTypeAssertionFailed } return mdynamodb.New(properDriver, properParameters, log) @@ -97,7 +106,7 @@ func getDynamoParams(cacheDriverConfig map[string]interface{}, log log.Logger) m allParametersOk = allParametersOk && ok if !allParametersOk { - panic("dynamo parameters are not specified correctly, can't proceede") + log.Panic().Msg("dynamo parameters are not specified correctly, can't proceed") } return mdynamodb.DBDriverParameters{ diff --git a/pkg/meta/meta_test.go b/pkg/meta/meta_test.go index 08997c9b26..99342773f6 100644 --- a/pkg/meta/meta_test.go +++ b/pkg/meta/meta_test.go @@ -2360,9 +2360,11 @@ func TestCreateDynamo(t *testing.T) { Convey("Fails", t, func() { log := log.NewLogger("debug", "") - So(func() { _, _ = meta.Create("dynamodb", nil, boltdb.DBParameters{RootDir: "root"}, log) }, ShouldPanic) + _, err := meta.Create("dynamodb", nil, boltdb.DBParameters{RootDir: "root"}, log) + So(err, ShouldNotBeNil) - So(func() { _, _ = meta.Create("dynamodb", &dynamodb.Client{}, "bad", log) }, ShouldPanic) + _, err = meta.Create("dynamodb", &dynamodb.Client{}, "bad", log) + So(err, ShouldNotBeNil) metaDB, err := meta.Create("random", nil, boltdb.DBParameters{RootDir: "root"}, log) So(metaDB, ShouldBeNil) @@ -2389,6 +2391,7 @@ func TestCreateBoltDB(t *testing.T) { Convey("fails", t, func() { log := log.NewLogger("debug", "") - So(func() { _, _ = meta.Create("boltdb", nil, mdynamodb.DBDriverParameters{}, log) }, ShouldPanic) + _, err := meta.Create("boltdb", nil, mdynamodb.DBDriverParameters{}, log) + So(err, ShouldNotBeNil) }) } diff --git a/pkg/storage/cache.go b/pkg/storage/cache.go index 9f4ba3576e..78d13f0700 100644 --- a/pkg/storage/cache.go +++ b/pkg/storage/cache.go @@ -1,16 +1,16 @@ package storage import ( - "zotregistry.io/zot/errors" + zerr "zotregistry.io/zot/errors" "zotregistry.io/zot/pkg/api/config" zlog "zotregistry.io/zot/pkg/log" "zotregistry.io/zot/pkg/storage/cache" "zotregistry.io/zot/pkg/storage/constants" ) -func CreateCacheDatabaseDriver(storageConfig config.StorageConfig, log zlog.Logger) cache.Cache { +func CreateCacheDatabaseDriver(storageConfig config.StorageConfig, log zlog.Logger) (cache.Cache, error) { if !storageConfig.Dedupe && storageConfig.StorageDriver == nil { - return nil + return nil, nil } // local cache @@ -20,9 +20,7 @@ func CreateCacheDatabaseDriver(storageConfig config.StorageConfig, log zlog.Logg params.Name = constants.BoltdbName params.UseRelPaths = getUseRelPaths(&storageConfig) - driver, _ := Create("boltdb", params, log) - - return driver + return Create("boltdb", params, log) } // remote cache @@ -31,13 +29,13 @@ func CreateCacheDatabaseDriver(storageConfig config.StorageConfig, log zlog.Logg if !ok { log.Warn().Msg("remote cache driver name missing!") - return nil + return nil, nil } if name != constants.DynamoDBDriverName { log.Warn().Str("driver", name).Msg("remote cache driver unsupported!") - return nil + return nil, nil } // dynamodb @@ -46,27 +44,25 @@ func CreateCacheDatabaseDriver(storageConfig config.StorageConfig, log zlog.Logg dynamoParams.Region, _ = storageConfig.CacheDriver["region"].(string) dynamoParams.TableName, _ = storageConfig.CacheDriver["cachetablename"].(string) - driver, _ := Create("dynamodb", dynamoParams, log) - - return driver + return Create("dynamodb", dynamoParams, log) } - return nil + return nil, nil } func Create(dbtype string, parameters interface{}, log zlog.Logger) (cache.Cache, error) { switch dbtype { case "boltdb": { - return cache.NewBoltDBCache(parameters, log), nil + return cache.NewBoltDBCache(parameters, log) } case "dynamodb": { - return cache.NewDynamoDBCache(parameters, log), nil + return cache.NewDynamoDBCache(parameters, log) } default: { - return nil, errors.ErrBadConfig + return nil, zerr.ErrBadConfig } } } diff --git a/pkg/storage/cache/boltdb.go b/pkg/storage/cache/boltdb.go index 7a8163616c..0b8ab7a6dc 100644 --- a/pkg/storage/cache/boltdb.go +++ b/pkg/storage/cache/boltdb.go @@ -1,6 +1,7 @@ package cache import ( + "fmt" "os" "path" "path/filepath" @@ -9,7 +10,7 @@ import ( godigest "github.com/opencontainers/go-digest" "go.etcd.io/bbolt" - "zotregistry.io/zot/errors" + zerr "zotregistry.io/zot/errors" zlog "zotregistry.io/zot/pkg/log" "zotregistry.io/zot/pkg/storage/constants" ) @@ -27,17 +28,20 @@ type BoltDBDriverParameters struct { UseRelPaths bool } -func NewBoltDBCache(parameters interface{}, log zlog.Logger) Cache { +func NewBoltDBCache(parameters interface{}, log zlog.Logger) (Cache, error) { properParameters, ok := parameters.(BoltDBDriverParameters) if !ok { - panic("Failed type assertion") + log.Error().Err(zerr.ErrTypeAssertionFailed).Msgf("expected type '%T' but got '%T'", + BoltDBDriverParameters{}, parameters) + + return nil, zerr.ErrTypeAssertionFailed } err := os.MkdirAll(properParameters.RootDir, constants.DefaultDirPerms) if err != nil { log.Error().Err(err).Str("directory", properParameters.RootDir).Msg("unable to create directory for cache db") - return nil + return nil, err } dbPath := path.Join(properParameters.RootDir, properParameters.Name+constants.DBExtensionName) @@ -48,9 +52,17 @@ func NewBoltDBCache(parameters interface{}, log zlog.Logger) Cache { cacheDB, err := bbolt.Open(dbPath, 0o600, dbOpts) //nolint:gomnd if err != nil { + if strings.Contains(err.Error(), "timeout") { + err := fmt.Errorf("%w: %w, path '%s'", zerr.ErrTimeout, zerr.ErrDatabaseFileAlreadyInUse, dbPath) + + log.Error().Err(err).Str("dbPath", dbPath).Msg("unable to create cache db") + + return nil, err + } + log.Error().Err(err).Str("dbPath", dbPath).Msg("unable to create cache db") - return nil + return nil, err } if err := cacheDB.Update(func(tx *bbolt.Tx) error { @@ -66,7 +78,7 @@ func NewBoltDBCache(parameters interface{}, log zlog.Logger) Cache { // something went wrong log.Error().Err(err).Msg("unable to create a cache") - return nil + return nil, err } return &BoltDBDriver{ @@ -74,7 +86,7 @@ func NewBoltDBCache(parameters interface{}, log zlog.Logger) Cache { db: cacheDB, useRelPaths: properParameters.UseRelPaths, log: log, - } + }, nil } func (d *BoltDBDriver) UsesRelativePaths() bool { @@ -87,9 +99,9 @@ func (d *BoltDBDriver) Name() string { func (d *BoltDBDriver) PutBlob(digest godigest.Digest, path string) error { if path == "" { - d.log.Error().Err(errors.ErrEmptyValue).Str("digest", digest.String()).Msg("empty path provided") + d.log.Error().Err(zerr.ErrEmptyValue).Str("digest", digest.String()).Msg("empty path provided") - return errors.ErrEmptyValue + return zerr.ErrEmptyValue } // use only relative (to rootDir) paths on blobs @@ -105,7 +117,7 @@ func (d *BoltDBDriver) PutBlob(digest godigest.Digest, path string) error { root := tx.Bucket([]byte(constants.BlobsCache)) if root == nil { // this is a serious failure - err := errors.ErrCacheRootBucket + err := zerr.ErrCacheRootBucket d.log.Error().Err(err).Msg("unable to access root bucket") return err @@ -168,7 +180,7 @@ func (d *BoltDBDriver) GetBlob(digest godigest.Digest) (string, error) { root := tx.Bucket([]byte(constants.BlobsCache)) if root == nil { // this is a serious failure - err := errors.ErrCacheRootBucket + err := zerr.ErrCacheRootBucket d.log.Error().Err(err).Msg("unable to access root bucket") return err @@ -182,7 +194,7 @@ func (d *BoltDBDriver) GetBlob(digest godigest.Digest) (string, error) { return nil } - return errors.ErrCacheMiss + return zerr.ErrCacheMiss }); err != nil { return "", err } @@ -195,7 +207,7 @@ func (d *BoltDBDriver) HasBlob(digest godigest.Digest, blob string) bool { root := tx.Bucket([]byte(constants.BlobsCache)) if root == nil { // this is a serious failure - err := errors.ErrCacheRootBucket + err := zerr.ErrCacheRootBucket d.log.Error().Err(err).Msg("unable to access root bucket") return err @@ -203,22 +215,22 @@ func (d *BoltDBDriver) HasBlob(digest godigest.Digest, blob string) bool { bucket := root.Bucket([]byte(digest.String())) if bucket == nil { - return errors.ErrCacheMiss + return zerr.ErrCacheMiss } origin := bucket.Bucket([]byte(constants.OriginalBucket)) if origin == nil { - return errors.ErrCacheMiss + return zerr.ErrCacheMiss } deduped := bucket.Bucket([]byte(constants.DuplicatesBucket)) if deduped == nil { - return errors.ErrCacheMiss + return zerr.ErrCacheMiss } if origin.Get([]byte(blob)) == nil { if deduped.Get([]byte(blob)) == nil { - return errors.ErrCacheMiss + return zerr.ErrCacheMiss } } @@ -255,7 +267,7 @@ func (d *BoltDBDriver) DeleteBlob(digest godigest.Digest, path string) error { root := tx.Bucket([]byte(constants.BlobsCache)) if root == nil { // this is a serious failure - err := errors.ErrCacheRootBucket + err := zerr.ErrCacheRootBucket d.log.Error().Err(err).Msg("unable to access root bucket") return err @@ -263,12 +275,12 @@ func (d *BoltDBDriver) DeleteBlob(digest godigest.Digest, path string) error { bucket := root.Bucket([]byte(digest.String())) if bucket == nil { - return errors.ErrCacheMiss + return zerr.ErrCacheMiss } deduped := bucket.Bucket([]byte(constants.DuplicatesBucket)) if deduped == nil { - return errors.ErrCacheMiss + return zerr.ErrCacheMiss } if err := deduped.Delete([]byte(path)); err != nil { diff --git a/pkg/storage/cache/boltdb_test.go b/pkg/storage/cache/boltdb_test.go index 0e75ccbf31..d6f4e309be 100644 --- a/pkg/storage/cache/boltdb_test.go +++ b/pkg/storage/cache/boltdb_test.go @@ -19,7 +19,8 @@ func TestBoltDBCache(t *testing.T) { log := log.NewLogger("debug", "") So(log, ShouldNotBeNil) - So(func() { _, _ = storage.Create("boltdb", "failTypeAssertion", log) }, ShouldPanic) + _, err := storage.Create("boltdb", "failTypeAssertion", log) + So(err, ShouldNotBeNil) cacheDriver, _ := storage.Create("boltdb", cache.BoltDBDriverParameters{"/deadBEEF", "cache_test", true}, log) So(cacheDriver, ShouldBeNil) diff --git a/pkg/storage/cache/dynamodb.go b/pkg/storage/cache/dynamodb.go index 9d63e018e5..495b42b7ec 100644 --- a/pkg/storage/cache/dynamodb.go +++ b/pkg/storage/cache/dynamodb.go @@ -61,10 +61,13 @@ func (d *DynamoDBDriver) NewTable(tableName string) error { return nil } -func NewDynamoDBCache(parameters interface{}, log zlog.Logger) Cache { +func NewDynamoDBCache(parameters interface{}, log zlog.Logger) (Cache, error) { properParameters, ok := parameters.(DynamoDBDriverParameters) if !ok { - panic("Failed type assertion!") + log.Error().Err(zerr.ErrTypeAssertionFailed).Msgf("expected type '%T' but got '%T'", + BoltDBDriverParameters{}, parameters) + + return nil, zerr.ErrTypeAssertionFailed } // custom endpoint resolver to point to localhost @@ -85,7 +88,7 @@ func NewDynamoDBCache(parameters interface{}, log zlog.Logger) Cache { if err != nil { log.Error().Err(err).Msg("unable to load AWS SDK config for dynamodb") - return nil + return nil, err } driver := &DynamoDBDriver{client: dynamodb.NewFromConfig(cfg), tableName: properParameters.TableName, log: log} @@ -93,10 +96,12 @@ func NewDynamoDBCache(parameters interface{}, log zlog.Logger) Cache { err = driver.NewTable(driver.tableName) if err != nil { log.Error().Err(err).Str("tableName", driver.tableName).Msg("unable to create table for cache") + + return nil, err } // Using the Config value, create the DynamoDB client - return driver + return driver, nil } func (d *DynamoDBDriver) UsesRelativePaths() bool { diff --git a/pkg/storage/cache/dynamodb_test.go b/pkg/storage/cache/dynamodb_test.go index 543b508d67..1ff3ed3214 100644 --- a/pkg/storage/cache/dynamodb_test.go +++ b/pkg/storage/cache/dynamodb_test.go @@ -22,9 +22,8 @@ func TestDynamoDB(t *testing.T) { // bad params - So(func() { - _ = cache.NewDynamoDBCache("bad params", log) - }, ShouldPanic) + _, err := cache.NewDynamoDBCache("bad params", log) + So(err, ShouldNotBeNil) keyDigest := godigest.FromString("key") @@ -33,20 +32,7 @@ func TestDynamoDB(t *testing.T) { TableName: "BlobTable", Region: "us-east-2", }, log) - So(cacheDriver, ShouldNotBeNil) - So(err, ShouldBeNil) - - val, err := cacheDriver.GetBlob(keyDigest) - So(err, ShouldNotBeNil) - So(val, ShouldBeEmpty) - - err = cacheDriver.PutBlob(keyDigest, path.Join(dir, "value")) - So(err, ShouldNotBeNil) - - exists := cacheDriver.HasBlob(keyDigest, path.Join(dir, "value")) - So(exists, ShouldBeFalse) - - err = cacheDriver.DeleteBlob(keyDigest, path.Join(dir, "value")) + So(cacheDriver, ShouldBeNil) So(err, ShouldNotBeNil) cacheDriver, err = storage.Create("dynamodb", cache.DynamoDBDriverParameters{ @@ -60,7 +46,7 @@ func TestDynamoDB(t *testing.T) { returnedName := cacheDriver.Name() So(returnedName, ShouldEqual, "dynamodb") - val, err = cacheDriver.GetBlob(keyDigest) + val, err := cacheDriver.GetBlob(keyDigest) So(err, ShouldNotBeNil) So(val, ShouldBeEmpty) @@ -74,7 +60,7 @@ func TestDynamoDB(t *testing.T) { So(err, ShouldBeNil) So(val, ShouldNotBeEmpty) - exists = cacheDriver.HasBlob(keyDigest, path.Join(dir, "value")) + exists := cacheDriver.HasBlob(keyDigest, path.Join(dir, "value")) So(exists, ShouldBeTrue) err = cacheDriver.DeleteBlob(keyDigest, path.Join(dir, "value")) diff --git a/pkg/storage/cache_test.go b/pkg/storage/cache_test.go index 6158f25eae..53ce9c1802 100644 --- a/pkg/storage/cache_test.go +++ b/pkg/storage/cache_test.go @@ -19,7 +19,8 @@ func TestCache(t *testing.T) { log := log.NewLogger("debug", "") So(log, ShouldNotBeNil) - So(func() { _, _ = storage.Create("boltdb", "failTypeAssertion", log) }, ShouldPanic) + _, err := storage.Create("boltdb", "failTypeAssertion", log) + So(err, ShouldNotBeNil) cacheDriver, _ := storage.Create("boltdb", cache.BoltDBDriverParameters{ RootDir: "/deadBEEF", diff --git a/pkg/storage/storage.go b/pkg/storage/storage.go index 7d3cf4cd75..504c00d648 100644 --- a/pkg/storage/storage.go +++ b/pkg/storage/storage.go @@ -9,7 +9,7 @@ import ( godigest "github.com/opencontainers/go-digest" ispec "github.com/opencontainers/image-spec/specs-go/v1" - "zotregistry.io/zot/errors" + zerr "zotregistry.io/zot/errors" "zotregistry.io/zot/pkg/api/config" zcommon "zotregistry.io/zot/pkg/common" "zotregistry.io/zot/pkg/extensions/monitoring" @@ -28,9 +28,9 @@ func New(config *config.Config, linter common.Lint, metrics monitoring.MetricSer if config.Storage.RootDirectory == "" { // we can't proceed without global storage - log.Error().Err(errors.ErrImgStoreNotFound).Msg("controller: no storage config provided") + log.Error().Err(zerr.ErrImgStoreNotFound).Msg("controller: no storage config provided") - return storeController, errors.ErrImgStoreNotFound + return storeController, zerr.ErrImgStoreNotFound } // no need to validate hard links work on s3 @@ -47,18 +47,24 @@ func New(config *config.Config, linter common.Lint, metrics monitoring.MetricSer var defaultStore storageTypes.ImageStore if config.Storage.StorageDriver == nil { + cacheDriver, err := CreateCacheDatabaseDriver(config.Storage.StorageConfig, log) + if err != nil { + return storeController, err + } + // false positive lint - linter does not implement Lint method //nolint:typecheck,contextcheck rootDir := config.Storage.RootDirectory defaultStore = local.NewImageStore(rootDir, - config.Storage.Dedupe, config.Storage.Commit, log, metrics, linter, - CreateCacheDatabaseDriver(config.Storage.StorageConfig, log), + config.Storage.Dedupe, config.Storage.Commit, log, metrics, linter, cacheDriver, ) } else { storeName := fmt.Sprintf("%v", config.Storage.StorageDriver["name"]) if storeName != constants.S3StorageDriverName { - log.Fatal().Err(errors.ErrBadConfig).Str("storageDriver", storeName). + log.Error().Err(zerr.ErrBadConfig).Str("storageDriver", storeName). Msg("unsupported storage driver") + + return storeController, fmt.Errorf("storageDriver '%s' unsupported storage driver: %w", storeName, zerr.ErrBadConfig) } // Init a Storager from connection string. store, err := factory.Create(storeName, config.Storage.StorageDriver) @@ -75,11 +81,15 @@ func New(config *config.Config, linter common.Lint, metrics monitoring.MetricSer rootDir = fmt.Sprintf("%v", config.Storage.StorageDriver["rootdirectory"]) } + cacheDriver, err := CreateCacheDatabaseDriver(config.Storage.StorageConfig, log) + if err != nil { + return storeController, err + } + // false positive lint - linter does not implement Lint method //nolint: typecheck,contextcheck defaultStore = s3.NewImageStore(rootDir, config.Storage.RootDirectory, - config.Storage.Dedupe, config.Storage.Commit, log, metrics, linter, store, - CreateCacheDatabaseDriver(config.Storage.StorageConfig, log)) + config.Storage.Dedupe, config.Storage.Commit, log, metrics, linter, store, cacheDriver) } storeController.DefaultStore = defaultStore @@ -128,9 +138,9 @@ func getSubStore(cfg *config.Config, subPaths map[string]config.StorageConfig, isSame, _ := config.SameFile(cfg.Storage.RootDirectory, storageConfig.RootDirectory) if isSame { - log.Error().Err(errors.ErrBadConfig).Msg("sub path storage directory is same as root directory") + log.Error().Err(zerr.ErrBadConfig).Msg("sub path storage directory is same as root directory") - return nil, errors.ErrBadConfig + return nil, zerr.ErrBadConfig } isUnique := true @@ -149,10 +159,14 @@ func getSubStore(cfg *config.Config, subPaths map[string]config.StorageConfig, // add it to uniqueSubFiles // Create a new image store and assign it to imgStoreMap if isUnique { + cacheDriver, err := CreateCacheDatabaseDriver(storageConfig, log) + if err != nil { + return nil, err + } + rootDir := storageConfig.RootDirectory imgStoreMap[storageConfig.RootDirectory] = local.NewImageStore(rootDir, - storageConfig.Dedupe, storageConfig.Commit, log, metrics, linter, - CreateCacheDatabaseDriver(storageConfig, log), + storageConfig.Dedupe, storageConfig.Commit, log, metrics, linter, cacheDriver, ) subImageStore[route] = imgStoreMap[storageConfig.RootDirectory] @@ -160,8 +174,10 @@ func getSubStore(cfg *config.Config, subPaths map[string]config.StorageConfig, } else { storeName := fmt.Sprintf("%v", storageConfig.StorageDriver["name"]) if storeName != constants.S3StorageDriverName { - log.Fatal().Err(errors.ErrBadConfig).Str("storageDriver", storeName). + log.Error().Err(zerr.ErrBadConfig).Str("storageDriver", storeName). Msg("unsupported storage driver") + + return nil, fmt.Errorf("storageDriver '%s' unsupported storage driver: %w", storeName, zerr.ErrBadConfig) } // Init a Storager from connection string. @@ -179,11 +195,18 @@ func getSubStore(cfg *config.Config, subPaths map[string]config.StorageConfig, rootDir = fmt.Sprintf("%v", cfg.Storage.StorageDriver["rootdirectory"]) } + cacheDriver, err := CreateCacheDatabaseDriver(storageConfig, log) + if err != nil { + log.Error().Err(err).Any("config", storageConfig). + Msg("failed creating storage driver") + + return nil, err + } + // false positive lint - linter does not implement Lint method //nolint: typecheck subImageStore[route] = s3.NewImageStore(rootDir, storageConfig.RootDirectory, - storageConfig.Dedupe, storageConfig.Commit, log, metrics, linter, store, - CreateCacheDatabaseDriver(storageConfig, log), + storageConfig.Dedupe, storageConfig.Commit, log, metrics, linter, store, cacheDriver, ) } }