From 064d471629eab02a6dbc77264a33f23dda5da0f1 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 | 1 + pkg/api/authn.go | 41 ++++++++++++++---------- pkg/api/controller.go | 8 +++-- pkg/api/controller_test.go | 27 +++++++++------- pkg/cli/config_cmd.go | 6 ++-- pkg/cli/config_reloader.go | 6 ++-- pkg/cli/cve_cmd.go | 2 +- pkg/cli/image_cmd.go | 2 +- pkg/cli/repo_cmd.go | 2 +- pkg/cli/root.go | 47 ++++++++++++++++----------- pkg/cli/search_cmd.go | 2 +- 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 | 22 ++++++------- pkg/storage/cache/boltdb.go | 47 +++++++++++++++------------ pkg/storage/cache/boltdb_test.go | 3 +- pkg/storage/cache/dynamodb.go | 13 +++++--- pkg/storage/cache/dynamodb_test.go | 5 ++- pkg/storage/cache_test.go | 3 +- pkg/storage/storage.go | 51 ++++++++++++++++++++++-------- 22 files changed, 196 insertions(+), 129 deletions(-) diff --git a/errors/errors.go b/errors/errors.go index b3cf4bfdb4..22c7243760 100644 --- a/errors/errors.go +++ b/errors/errors.go @@ -151,4 +151,5 @@ var ( ErrInvalidPublicKeyContent = errors.New("signatures: invalid public key content") ErrInvalidStateCookie = errors.New("auth: state cookie not present or differs from original state") ErrSyncNoURLsLeft = errors.New("sync: no valid registry urls left after filtering local ones") + ErrServerIsRunning = errors.New("server is running") ) diff --git a/pkg/api/authn.go b/pkg/api/authn.go index 8e7bb6b999..d623b014f5 100644 --- a/pkg/api/authn.go +++ b/pkg/api/authn.go @@ -51,10 +51,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) @@ -245,14 +246,14 @@ func (amw *AuthnMiddleware) TryAuthnHandlers(ctlr *Controller) mux.MiddlewareFun cookieStoreHashKey := securecookie.GenerateRandomKey(64) if cookieStoreHashKey == nil { - panic(zerr.ErrHashKeyNotCreated) + amw.log.Panic().Err(zerr.ErrHashKeyNotCreated).Msg("failed to generate random key") } // if storage is filesystem then use zot's rootDir to store sessions if ctlr.Config.Storage.StorageDriver == nil { sessionsDir := path.Join(ctlr.Config.Storage.RootDirectory, "_sessions") if err := os.MkdirAll(sessionsDir, storageConstants.DefaultDirPerms); err != nil { - panic(err) + amw.log.Panic().Err(err).Str("session-dir", sessionsDir).Msg("failed to create session dir") } cookieStore := sessions.NewFilesystemStore(sessionsDir, cookieStoreHashKey) @@ -291,13 +292,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 @@ -305,7 +308,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 @@ -315,7 +319,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() @@ -336,10 +341,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 } } @@ -548,19 +553,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, @@ -572,17 +578,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 ebc9c20279..279b03e72e 100644 --- a/pkg/api/controller.go +++ b/pkg/api/controller.go @@ -188,13 +188,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 341c13e710..445470dbcf 100644 --- a/pkg/api/controller_test.go +++ b/pkg/api/controller_test.go @@ -131,13 +131,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) }) skipDynamo(t) @@ -170,8 +172,9 @@ func TestCreateCacheDatabaseDriver(t *testing.T) { "versionTablename": "Version", } - driver := storage.CreateCacheDatabaseDriver(conf.Storage.StorageConfig, log) - So(driver, ShouldNotBeNil) + driver, err := storage.CreateCacheDatabaseDriver(conf.Storage.StorageConfig, log) + So(err, ShouldNotBeNil) + So(driver, ShouldBeNil) // negative test cases @@ -185,7 +188,8 @@ func TestCreateCacheDatabaseDriver(t *testing.T) { "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{}{ @@ -199,7 +203,8 @@ func TestCreateCacheDatabaseDriver(t *testing.T) { "versionTablename": "Version", } - driver = storage.CreateCacheDatabaseDriver(conf.Storage.StorageConfig, log) + driver, err = storage.CreateCacheDatabaseDriver(conf.Storage.StorageConfig, log) + So(err, ShouldBeNil) So(driver, ShouldBeNil) }) } @@ -2547,7 +2552,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() { @@ -2555,7 +2560,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() { @@ -2564,7 +2569,7 @@ func TestNewRelyingPartyOIDC(t *testing.T) { Key: ServerKey, } - rp := api.NewRelyingPartyOIDC(conf, "oidc") + rp := api.NewRelyingPartyOIDC(conf, "oidc", log.NewLogger("debug", "")) So(rp, ShouldNotBeNil) }) @@ -2573,7 +2578,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) }) @@ -2582,7 +2587,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) }) }) } diff --git a/pkg/cli/config_cmd.go b/pkg/cli/config_cmd.go index 0460cffd33..dc022f4faa 100644 --- a/pkg/cli/config_cmd.go +++ b/pkg/cli/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/config_reloader.go b/pkg/cli/config_reloader.go index e5559c298e..ed8f948142 100644 --- a/pkg/cli/config_reloader.go +++ b/pkg/cli/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/cve_cmd.go b/pkg/cli/cve_cmd.go index 875eebc17a..d7352c7a46 100644 --- a/pkg/cli/cve_cmd.go +++ b/pkg/cli/cve_cmd.go @@ -34,7 +34,7 @@ func NewCveCommand(searchService SearchService) *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/image_cmd.go b/pkg/cli/image_cmd.go index 8661733cb1..2947632c2e 100644 --- a/pkg/cli/image_cmd.go +++ b/pkg/cli/image_cmd.go @@ -30,7 +30,7 @@ func NewImageCommand(searchService SearchService) *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/repo_cmd.go b/pkg/cli/repo_cmd.go index 784de164bf..0baa041a98 100644 --- a/pkg/cli/repo_cmd.go +++ b/pkg/cli/repo_cmd.go @@ -27,7 +27,7 @@ func NewRepoCommand(searchService SearchService) *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/root.go b/pkg/cli/root.go index cd4f1be769..a7690ee978 100644 --- a/pkg/cli/root.go +++ b/pkg/cli/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 } } @@ -55,7 +55,7 @@ func newServeCmd(conf *config.Config) *cobra.Command { // config reloader hotReloader, err := NewHotReloader(ctlr, args[0]) if err != nil { - panic(err) + return err } /* context used to cancel go routines so that @@ -63,12 +63,10 @@ func newServeCmd(conf *config.Config) *cobra.Command { reloaderCtx := hotReloader.Start() if err := ctlr.Init(reloaderCtx); err != nil { - panic(err) + return err } - if err := ctlr.Run(reloaderCtx); err != nil { - log.Fatal().Err(err).Msg("unable to start controller, exiting") - } + return ctlr.Run(reloaderCtx) }, } @@ -82,17 +80,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 @@ -102,30 +100,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 errors.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() if err != nil { - panic(err) + return err } result.PrintScrubResults(cmd.OutOrStdout()) } + + return nil }, } @@ -139,15 +141,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 }, } @@ -163,7 +168,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") @@ -171,6 +176,8 @@ func NewServerRootCmd() *cobra.Command { _ = cmd.Usage() cmd.SilenceErrors = false } + + return nil }, } @@ -194,7 +201,7 @@ func NewCliRootCmd() *cobra.Command { Use: "zli", Short: "`zli`", Long: "`zli`", - 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") @@ -202,6 +209,8 @@ func NewCliRootCmd() *cobra.Command { _ = cmd.Usage() cmd.SilenceErrors = false } + + return nil }, } diff --git a/pkg/cli/search_cmd.go b/pkg/cli/search_cmd.go index 3433418e6d..39b5d110b0 100644 --- a/pkg/cli/search_cmd.go +++ b/pkg/cli/search_cmd.go @@ -39,7 +39,7 @@ Example: 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/compliance/v1_0_0/check.go b/pkg/compliance/v1_0_0/check.go index bac6079c66..d188d497fc 100644 --- a/pkg/compliance/v1_0_0/check.go +++ b/pkg/compliance/v1_0_0/check.go @@ -29,7 +29,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 24285556e6..272af45622 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 unmarshalling new config") - panic(err) + log.Panic().Err(err).Msg("Error while unmarshalling new config") } if len(metaData.Keys) == 0 || len(metaData.Unused) > 0 { - log.Error().Err(errors.ErrBadConfig).Msg("Bad configuration, retry writing it") - panic(errors.ErrBadConfig) + log.Panic().Err(errors.ErrBadConfig).Msg("Bad configuration, retry writing it") } } diff --git a/pkg/meta/meta.go b/pkg/meta/meta.go index 30ca29ec2e..6ec1275358 100644 --- a/pkg/meta/meta.go +++ b/pkg/meta/meta.go @@ -49,7 +49,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) @@ -58,12 +61,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) @@ -103,7 +112,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 proceede") } return mdynamodb.DBDriverParameters{ diff --git a/pkg/meta/meta_test.go b/pkg/meta/meta_test.go index 0112061953..e534ad4a62 100644 --- a/pkg/meta/meta_test.go +++ b/pkg/meta/meta_test.go @@ -2505,9 +2505,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) @@ -2534,7 +2536,8 @@ 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..10b3a04e5b 100644 --- a/pkg/storage/cache.go +++ b/pkg/storage/cache.go @@ -8,9 +8,9 @@ import ( "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,23 +44,21 @@ 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: { diff --git a/pkg/storage/cache/boltdb.go b/pkg/storage/cache/boltdb.go index 89d068911c..eaf8449cbf 100644 --- a/pkg/storage/cache/boltdb.go +++ b/pkg/storage/cache/boltdb.go @@ -9,7 +9,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 +27,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 +51,13 @@ 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") { + return nil, nil + } + 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 +73,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 +81,7 @@ func NewBoltDBCache(parameters interface{}, log zlog.Logger) Cache { db: cacheDB, useRelPaths: properParameters.UseRelPaths, log: log, - } + }, nil } func (d *BoltDBDriver) Name() string { @@ -83,9 +90,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 @@ -101,7 +108,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 @@ -164,7 +171,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 @@ -178,7 +185,7 @@ func (d *BoltDBDriver) GetBlob(digest godigest.Digest) (string, error) { return nil } - return errors.ErrCacheMiss + return zerr.ErrCacheMiss }); err != nil { return "", err } @@ -191,7 +198,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 @@ -199,22 +206,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 } } @@ -251,7 +258,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 @@ -259,12 +266,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 8759b781db..c3127535b5 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 890d5177f3..1b6b24aa10 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) Name() string { diff --git a/pkg/storage/cache/dynamodb_test.go b/pkg/storage/cache/dynamodb_test.go index 64a67f4dd9..f851282c3f 100644 --- a/pkg/storage/cache/dynamodb_test.go +++ b/pkg/storage/cache/dynamodb_test.go @@ -29,9 +29,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") diff --git a/pkg/storage/cache_test.go b/pkg/storage/cache_test.go index 01b31b7712..396b80be4d 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 7aab199d7b..5153416232 100644 --- a/pkg/storage/storage.go +++ b/pkg/storage/storage.go @@ -11,7 +11,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" @@ -30,9 +30,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 @@ -49,18 +49,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 defaultStore = local.NewImageStore(config.Storage.RootDirectory, config.Storage.GC, config.Storage.GCDelay, - 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) @@ -77,12 +83,16 @@ 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.GC, config.Storage.GCDelay, config.Storage.Dedupe, - config.Storage.Commit, log, metrics, linter, store, - CreateCacheDatabaseDriver(config.Storage.StorageConfig, log)) + config.Storage.Commit, log, metrics, linter, store, cacheDriver) } storeController.DefaultStore = defaultStore @@ -131,9 +141,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 @@ -152,17 +162,24 @@ 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 + } + imgStoreMap[storageConfig.RootDirectory] = local.NewImageStore(storageConfig.RootDirectory, storageConfig.GC, storageConfig.GCDelay, storageConfig.Dedupe, - storageConfig.Commit, log, metrics, linter, CreateCacheDatabaseDriver(storageConfig, log)) + storageConfig.Commit, log, metrics, linter, cacheDriver) subImageStore[route] = imgStoreMap[storageConfig.RootDirectory] } } 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. @@ -180,12 +197,20 @@ 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.GC, storageConfig.GCDelay, storageConfig.Dedupe, storageConfig.Commit, log, metrics, linter, store, - CreateCacheDatabaseDriver(storageConfig, log), + cacheDriver, ) } }