From 92837c2bcb416febe90e856cea7f4961d25103da Mon Sep 17 00:00:00 2001 From: LaurentiuNiculae Date: Fri, 24 Nov 2023 10:38:36 +0200 Subject: [PATCH] refactor(log): replace panics with log fatal or log panic functions (#1723) Signed-off-by: Laurentiu Niculae --- errors/errors.go | 2 + pkg/api/authn.go | 37 ++- pkg/api/controller.go | 8 +- pkg/api/controller_test.go | 363 ++++++++++++++++------ pkg/cli/client/config_cmd.go | 6 +- pkg/cli/server/config_reloader.go | 6 +- pkg/cli/server/extensions_test.go | 63 ++-- pkg/cli/server/root.go | 43 ++- pkg/cli/server/root_test.go | 353 ++++++++++++++++++--- 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_internal_test.go | 63 ++++ pkg/storage/cache/boltdb_test.go | 3 +- pkg/storage/cache/dynamodb.go | 21 +- pkg/storage/cache/dynamodb_test.go | 53 ++-- pkg/storage/cache_test.go | 3 +- pkg/storage/storage.go | 53 +++- pkg/storage/storage_test.go | 79 +++++ 22 files changed, 969 insertions(+), 302 deletions(-) create mode 100644 pkg/storage/cache/boltdb_internal_test.go diff --git a/errors/errors.go b/errors/errors.go index 0022a53b..c60c4af6 100644 --- a/errors/errors.go +++ b/errors/errors.go @@ -164,6 +164,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 741777dd..d3252ff3 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 09fc6988..436b45e2 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 1e419e87..a051c75e 100644 --- a/pkg/api/controller_test.go +++ b/pkg/api/controller_test.go @@ -114,13 +114,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) @@ -147,15 +149,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 @@ -163,30 +166,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) }) } @@ -296,7 +301,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) @@ -362,12 +367,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()) @@ -380,9 +387,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, @@ -392,7 +400,7 @@ func TestObjectStorageController(t *testing.T) { } conf.Storage.StorageDriver = storageDriverParams - ctlr := makeController(conf, "/") + ctlr := makeController(conf, tmp) So(ctlr, ShouldNotBeNil) cm := test.NewControllerManager(ctlr) @@ -486,9 +494,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, @@ -497,12 +506,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 @@ -1279,7 +1288,6 @@ func TestMultipleInstance(t *testing.T) { Convey("Test zot multiple subpath with same root directory", t, func() { port := test.GetFreePort() - baseURL := test.GetBaseURL(port) conf := config.New() conf.HTTP.Port = port username, seedUser := test.GenerateRandomString() @@ -1320,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) }) } @@ -1568,6 +1557,131 @@ func TestMutualTLSAuthWithUserPermissions(t *testing.T) { }) } +func TestAuthnErrors(t *testing.T) { + Convey("ldap CA certs fail", t, func() { + port := test.GetFreePort() + conf := config.New() + conf.HTTP.Port = port + tmpDir := t.TempDir() + tmpFile := path.Join(tmpDir, "test-file.txt") + + err := os.WriteFile(tmpFile, []byte("test"), 0o000) + So(err, ShouldBeNil) + + conf.HTTP.Auth.LDAP = (&config.LDAPConfig{ + Insecure: true, + Address: LDAPAddress, + Port: 9000, + BaseDN: LDAPBaseDN, + UserAttribute: "uid", + CACert: tmpFile, + }).SetBindDN(LDAPBindDN).SetBindPassword(LDAPBindPassword) + + ctlr := makeController(conf, t.TempDir()) + + So(func() { + api.AuthHandler(ctlr) + }, ShouldPanic) + + err = os.Chmod(tmpFile, 0o644) + So(err, ShouldBeNil) + }) + + Convey("ldap CA certs is empty", t, func() { + port := test.GetFreePort() + conf := config.New() + conf.HTTP.Port = port + tmpDir := t.TempDir() + tmpFile := path.Join(tmpDir, "test-file.txt") + err := os.WriteFile(tmpFile, []byte(""), 0o600) + So(err, ShouldBeNil) + + conf.HTTP.Auth.LDAP = (&config.LDAPConfig{ + Insecure: true, + Address: LDAPAddress, + Port: 9000, + BaseDN: LDAPBaseDN, + UserAttribute: "uid", + CACert: tmpFile, + }).SetBindDN(LDAPBindDN).SetBindPassword(LDAPBindPassword) + + ctlr := makeController(conf, t.TempDir()) + + So(func() { + api.AuthHandler(ctlr) + }, ShouldPanic) + + So(err, ShouldBeNil) + }) + + Convey("ldap CA certs is empty", t, func() { + port := test.GetFreePort() + conf := config.New() + conf.HTTP.Port = port + + conf.HTTP.Auth.LDAP = (&config.LDAPConfig{ + Insecure: true, + Address: LDAPAddress, + Port: 9000, + BaseDN: LDAPBaseDN, + UserAttribute: "uid", + CACert: CACert, + }).SetBindDN(LDAPBindDN).SetBindPassword(LDAPBindPassword) + + ctlr := makeController(conf, t.TempDir()) + + So(func() { + api.AuthHandler(ctlr) + }, ShouldNotPanic) + }) + + Convey("Htpasswd file fail", t, func() { + port := test.GetFreePort() + conf := config.New() + conf.HTTP.Port = port + tmpDir := t.TempDir() + tmpFile := path.Join(tmpDir, "test-file.txt") + + err := os.WriteFile(tmpFile, []byte("test"), 0o000) + So(err, ShouldBeNil) + + conf.HTTP.Auth.HTPasswd = config.AuthHTPasswd{ + Path: tmpFile, + } + + ctlr := makeController(conf, t.TempDir()) + + So(func() { + api.AuthHandler(ctlr) + }, ShouldPanic) + + err = os.Chmod(tmpFile, 0o644) + So(err, ShouldBeNil) + }) + + Convey("NewRelyingPartyGithub fail", t, func() { + port := test.GetFreePort() + conf := config.New() + conf.HTTP.Port = port + tmpDir := t.TempDir() + tmpFile := path.Join(tmpDir, "test-file.txt") + + err := os.WriteFile(tmpFile, []byte("test"), 0o000) + So(err, ShouldBeNil) + + conf.HTTP.Auth.HTPasswd = config.AuthHTPasswd{ + Path: tmpFile, + } + + So(func() { + api.NewRelyingPartyGithub(conf, "prov", log.NewLogger("debug", "")) + }, ShouldPanic) + + err = os.Chmod(tmpFile, 0o644) + So(err, ShouldBeNil) + }) +} + func TestMutualTLSAuthWithoutCN(t *testing.T) { Convey("Make a new controller", t, func() { caCert, err := os.ReadFile("../../test/data/noidentity/ca.crt") @@ -1690,6 +1804,91 @@ func TestTLSMutualAuth(t *testing.T) { }) } +func TestTSLFailedReadingOfCACert(t *testing.T) { + Convey("no permissions", t, func() { + port := test.GetFreePort() + conf := config.New() + conf.HTTP.Port = port + conf.HTTP.TLS = &config.TLSConfig{ + Cert: ServerCert, + Key: ServerKey, + CACert: CACert, + } + + err := os.Chmod(CACert, 0o000) + defer func() { + err := os.Chmod(CACert, 0o644) + So(err, ShouldBeNil) + }() + So(err, ShouldBeNil) + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + ctlr := makeController(conf, t.TempDir()) + + err = ctlr.Init(ctx) + So(err, ShouldBeNil) + + errChan := make(chan error, 1) + go func() { + err = ctlr.Run(ctx) + errChan <- err + }() + + testTimeout := false + + select { + case err := <-errChan: + So(err, ShouldNotBeNil) + case <-ctx.Done(): + testTimeout = true + cancel() + } + + So(testTimeout, ShouldBeFalse) + }) + + Convey("empty CACert", t, func() { + badCACert := filepath.Join(t.TempDir(), "badCACert") + err := os.WriteFile(badCACert, []byte(""), 0o600) + So(err, ShouldBeNil) + + port := test.GetFreePort() + conf := config.New() + conf.HTTP.Port = port + conf.HTTP.TLS = &config.TLSConfig{ + Cert: ServerCert, + Key: ServerKey, + CACert: badCACert, + } + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + ctlr := makeController(conf, t.TempDir()) + + err = ctlr.Init(ctx) + So(err, ShouldBeNil) + + errChan := make(chan error, 1) + go func() { + err = ctlr.Run(ctx) + errChan <- err + }() + + testTimeout := false + + select { + case err := <-errChan: + So(err, ShouldNotBeNil) + case <-ctx.Done(): + testTimeout = true + cancel() + } + + So(testTimeout, ShouldBeFalse) + }) +} + func TestTLSMutualAuthAllowReadAccess(t *testing.T) { Convey("Make a new controller", t, func() { caCert, err := os.ReadFile(CACert) @@ -3046,7 +3245,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() { @@ -3054,7 +3253,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() { @@ -3063,7 +3262,7 @@ func TestNewRelyingPartyOIDC(t *testing.T) { Key: ServerKey, } - rp := api.NewRelyingPartyOIDC(conf, "oidc") + rp := api.NewRelyingPartyOIDC(conf, "oidc", log.NewLogger("debug", "")) So(rp, ShouldNotBeNil) }) @@ -3072,7 +3271,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) }) @@ -3081,7 +3280,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) }) }) } @@ -8757,46 +8956,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() @@ -9005,7 +9168,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 := ` @@ -9092,7 +9255,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) }) @@ -9160,7 +9323,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) }) @@ -9228,7 +9391,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) }) @@ -9282,7 +9445,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) }) @@ -9347,7 +9510,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 cf32d0c3..f17ec14c 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 91e22105..2c90499e 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 57cd32fe..727e96a7 100644 --- a/pkg/cli/server/extensions_test.go +++ b/pkg/cli/server/extensions_test.go @@ -62,7 +62,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 = fmt.Sprintf(`{ "storage":{ @@ -101,7 +101,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) { @@ -118,7 +118,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) { @@ -135,7 +135,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) { @@ -153,7 +153,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) { @@ -171,7 +171,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) { @@ -226,7 +226,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) }) } @@ -375,6 +375,7 @@ 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": { @@ -388,7 +389,7 @@ func TestServeExtensions(t *testing.T) { "level": "debug", "output": "%s" } - }`, t.TempDir(), port, logFile.Name()) + }`, tmpFile, port, logFile.Name()) cfgfile, err := os.CreateTemp("", "zot-test*.json") So(err, ShouldBeNil) @@ -400,8 +401,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) @@ -416,6 +419,7 @@ 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": { @@ -431,7 +435,7 @@ func TestServeExtensions(t *testing.T) { }, "extensions": { } - }`, t.TempDir(), port, logFile.Name()) + }`, tmpFile, port, logFile.Name()) cfgfile, err := os.CreateTemp("", "zot-test*.json") So(err, ShouldBeNil) @@ -442,9 +446,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) @@ -455,7 +462,8 @@ func TestServeExtensions(t *testing.T) { }) } -func testWithMetricsEnabled(rootDir string, cfgContentFormat string) { +func testWithMetricsEnabled(t *testing.T, rootDir string, cfgContentFormat string) { + t.Helper() port := GetFreePort() baseURL := GetBaseURL(port) logFile, err := os.CreateTemp("", "zot-log*.txt") @@ -476,8 +484,10 @@ func testWithMetricsEnabled(rootDir string, 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) @@ -501,6 +511,8 @@ func TestServeMetricsExtension(t *testing.T) { defer func() { os.Args = oldArgs }() Convey("no explicit enable", t, func(c C) { + tmpFile := t.TempDir() + content := `{ "storage": { "rootDirectory": "%s" @@ -518,10 +530,12 @@ func TestServeMetricsExtension(t *testing.T) { } } }` - testWithMetricsEnabled(t.TempDir(), content) + testWithMetricsEnabled(t, tmpFile, content) }) Convey("no explicit enable but with prometheus parameter", t, func(c C) { + tmpFile := t.TempDir() + content := `{ "storage": { "rootDirectory": "%s" @@ -542,10 +556,12 @@ func TestServeMetricsExtension(t *testing.T) { } } }` - testWithMetricsEnabled(t.TempDir(), content) + testWithMetricsEnabled(t, tmpFile, content) }) Convey("with explicit enable, but without prometheus parameter", t, func(c C) { + tmpFile := t.TempDir() + content := `{ "storage": { "rootDirectory": "%s" @@ -564,7 +580,7 @@ func TestServeMetricsExtension(t *testing.T) { } } }` - testWithMetricsEnabled(t.TempDir(), content) + testWithMetricsEnabled(t, tmpFile, content) }) Convey("with explicit disable", t, func(c C) { @@ -572,6 +588,7 @@ 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(`{ @@ -591,7 +608,7 @@ func TestServeMetricsExtension(t *testing.T) { "enable": false } } - }`, t.TempDir(), port, logFile.Name()) + }`, tmpFile, port, logFile.Name()) cfgfile, err := os.CreateTemp("", "zot-test*.json") So(err, ShouldBeNil) @@ -603,8 +620,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 f83d2fd5..5809ddad 100644 --- a/pkg/cli/server/root.go +++ b/pkg/cli/server/root.go @@ -44,10 +44,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 } } @@ -58,7 +58,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 @@ -68,12 +68,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 }, } @@ -87,17 +89,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 @@ -107,30 +109,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 }, } @@ -144,15 +150,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 }, } @@ -168,7 +177,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") @@ -176,6 +185,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 a7b5e130..98072c17 100644 --- a/pkg/cli/server/root_test.go +++ b/pkg/cli/server/root_test.go @@ -50,12 +50,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) { @@ -67,7 +69,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) { @@ -88,7 +91,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 @@ -113,7 +117,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) { @@ -152,7 +237,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(`{ @@ -183,7 +269,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(`{ @@ -219,7 +306,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(`{ @@ -259,7 +347,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) @@ -299,7 +388,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(`{ @@ -336,7 +426,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(`{ @@ -378,7 +469,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(`{ @@ -415,7 +507,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 verify with bad gc retention repo patterns", t, func(c C) { @@ -463,7 +556,7 @@ 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) + So(cli.NewServerRootCmd().Execute(), ShouldNotBeNil) }) Convey("Test verify with bad gc image retention tag regex", t, func(c C) { @@ -503,7 +596,7 @@ 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) + So(cli.NewServerRootCmd().Execute(), ShouldNotBeNil) }) Convey("Test apply defaults cache db", t, func(c C) { @@ -525,7 +618,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() @@ -542,7 +636,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() @@ -556,7 +651,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) { @@ -571,7 +667,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) { @@ -587,7 +684,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) { @@ -613,7 +711,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", @@ -623,7 +722,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"}, @@ -633,7 +733,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"}, @@ -643,7 +744,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"}, @@ -653,7 +755,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"}, @@ -663,7 +766,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) { @@ -679,7 +783,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) { @@ -696,7 +801,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) { @@ -713,7 +819,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) { @@ -738,7 +845,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) { @@ -759,7 +867,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) { @@ -785,7 +894,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) { @@ -801,7 +1004,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) { @@ -816,7 +1057,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) { @@ -832,7 +1074,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) { @@ -848,7 +1091,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) { @@ -864,7 +1108,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) { @@ -881,7 +1126,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) { @@ -897,7 +1143,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) { @@ -913,7 +1160,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) { @@ -929,7 +1177,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) { @@ -1307,12 +1556,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) { @@ -1325,7 +1576,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) { @@ -1361,7 +1613,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() }) @@ -1389,7 +1642,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) { @@ -1443,7 +1697,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) }) }) } @@ -1486,14 +1741,14 @@ func TestUpdateLDAPConfig(t *testing.T) { server := cli.NewServerRootCmd() server.SetArgs([]string{"serve", configPath}) - So(func() { err = server.Execute() }, ShouldPanic) + So(server.Execute(), ShouldNotBeNil) err = os.Chmod(ldapConfigPath, 0o600) So(err, ShouldBeNil) server = cli.NewServerRootCmd() server.SetArgs([]string{"serve", configPath}) - So(func() { err = server.Execute() }, ShouldPanic) + So(server.Execute(), ShouldNotBeNil) }) Convey("unauthenticated LDAP config", t, func() { diff --git a/pkg/compliance/v1_0_0/check.go b/pkg/compliance/v1_0_0/check.go index 54e856ae..31d5eefc 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 27a54719..8bf9e593 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 ef685676..db31e5f6 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 de7b7ef0..214b8478 100644 --- a/pkg/meta/meta_test.go +++ b/pkg/meta/meta_test.go @@ -2488,9 +2488,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) @@ -2517,6 +2519,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 9f4ba357..78d13f07 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 7a816361..94f1bae2 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) (*BoltDBDriver, 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_internal_test.go b/pkg/storage/cache/boltdb_internal_test.go new file mode 100644 index 00000000..31d5860a --- /dev/null +++ b/pkg/storage/cache/boltdb_internal_test.go @@ -0,0 +1,63 @@ +package cache + +import ( + "path/filepath" + "testing" + + "github.com/opencontainers/go-digest" + . "github.com/smartystreets/goconvey/convey" + "go.etcd.io/bbolt" + + "zotregistry.io/zot/pkg/storage/constants" +) + +func TestBoltDriverErrors(t *testing.T) { + Convey("Make a new cache", t, func() { + tmpDir := t.TempDir() + + boltDB, err := bbolt.Open(filepath.Join(tmpDir, "bolt.db"), 0o644, bbolt.DefaultOptions) + So(err, ShouldBeNil) + + driver := BoltDBDriver{ + db: boltDB, + } + + Convey("Empty boltdb", func() { + // bucket not found + err = driver.PutBlob(digest.FromString("s"), "path") + So(err, ShouldNotBeNil) + + _, err = driver.GetBlob(digest.FromString("s")) + So(err, ShouldNotBeNil) + + has := driver.HasBlob(digest.FromString("s"), "blob") + So(has, ShouldBeFalse) + + err = driver.DeleteBlob(digest.FromString("s"), "blob") + So(err, ShouldNotBeNil) + }) + + Convey("cache miss", func() { + goodDigest := digest.FromString("s") + + err := driver.db.Update(func(tx *bbolt.Tx) error { + buck, err := tx.CreateBucketIfNotExists([]byte(constants.BlobsCache)) + So(err, ShouldBeNil) + + _, err = buck.CreateBucket([]byte(goodDigest)) + So(err, ShouldBeNil) + + return nil + }) + So(err, ShouldBeNil) + + // digest bucket not found + err = driver.DeleteBlob(digest.FromString("bad-digest"), "path") + So(err, ShouldNotBeNil) + + // duplicate bucket not exist + err = driver.DeleteBlob(goodDigest, "path") + So(err, ShouldNotBeNil) + }) + }) +} diff --git a/pkg/storage/cache/boltdb_test.go b/pkg/storage/cache/boltdb_test.go index 0e75ccbf..d6f4e309 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 9d63e018..e97ffc40 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) (*DynamoDBDriver, 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,16 @@ 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) SetTableName(table string) { + d.tableName = table } func (d *DynamoDBDriver) UsesRelativePaths() bool { @@ -212,7 +221,7 @@ func (d *DynamoDBDriver) DeleteBlob(digest godigest.Digest, path string) error { // if original blob is the one deleted if originBlob == path { // move duplicate blob to original, storage will move content here - originBlob, _ = d.getDuplicateBlob(digest) + originBlob, _ = d.GetDuplicateBlob(digest) if originBlob != "" { if err := d.putOriginBlob(digest, originBlob); err != nil { return err @@ -232,7 +241,7 @@ func (d *DynamoDBDriver) DeleteBlob(digest godigest.Digest, path string) error { return nil } -func (d *DynamoDBDriver) getDuplicateBlob(digest godigest.Digest) (string, error) { +func (d *DynamoDBDriver) GetDuplicateBlob(digest godigest.Digest) (string, error) { resp, err := d.client.GetItem(context.TODO(), &dynamodb.GetItemInput{ TableName: aws.String(d.tableName), Key: map[string]types.AttributeValue{ diff --git a/pkg/storage/cache/dynamodb_test.go b/pkg/storage/cache/dynamodb_test.go index 543b508d..e0cd85e8 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")) @@ -159,3 +145,32 @@ func TestDynamoDB(t *testing.T) { So(val, ShouldBeEmpty) }) } + +func TestDynamoDBError(t *testing.T) { + tskip.SkipDynamo(t) + Convey("Test dynamoDB", t, func(c C) { + log := log.NewLogger("debug", "") + + cacheDriver, err := cache.NewDynamoDBCache(cache.DynamoDBDriverParameters{ + Endpoint: os.Getenv("DYNAMODBMOCK_ENDPOINT"), + TableName: "BlobTable", + Region: "us-east-2", + }, log) + So(cacheDriver, ShouldNotBeNil) + So(err, ShouldBeNil) + + returnedName := cacheDriver.Name() + So(returnedName, ShouldEqual, "dynamodb") + + cacheDriver.SetTableName("bad-table") + + _, err = cacheDriver.GetBlob(godigest.FromString("str")) + So(err, ShouldNotBeNil) + found := cacheDriver.HasBlob(godigest.FromString("str"), "path") + So(found, ShouldBeFalse) + _, err = cacheDriver.GetDuplicateBlob(godigest.FromString("str")) + So(err, ShouldNotBeNil) + err = cacheDriver.DeleteBlob(godigest.FromString("str"), "path") + So(err, ShouldNotBeNil) + }) +} diff --git a/pkg/storage/cache_test.go b/pkg/storage/cache_test.go index 6158f25e..53ce9c18 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 e9f5e37e..daaf7ed9 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, ) } } diff --git a/pkg/storage/storage_test.go b/pkg/storage/storage_test.go index f9dde13d..f46dd63d 100644 --- a/pkg/storage/storage_test.go +++ b/pkg/storage/storage_test.go @@ -121,6 +121,18 @@ var testCases = []struct { }, } +func TestStorageNew(t *testing.T) { + Convey("New fail", t, func() { + // store name is wrong + conf := config.New() + conf.Storage.RootDirectory = "dir" + conf.Storage.StorageDriver = map[string]interface{}{} + + _, err := storage.New(conf, nil, nil, zlog.NewLogger("debug", "")) + So(err, ShouldNotBeNil) + }) +} + func TestStorageAPIs(t *testing.T) { for _, testcase := range testCases { testcase := testcase @@ -871,6 +883,73 @@ func TestMandatoryAnnotations(t *testing.T) { } } +func TestStorageSubpaths(t *testing.T) { + Convey("Reach for coverage", t, func() { + tmpDirSubpath := t.TempDir() + config := &config.Config{ + Storage: config.GlobalStorageConfig{ + StorageConfig: config.StorageConfig{RootDirectory: t.TempDir()}, + SubPaths: map[string]config.StorageConfig{ + "a/": {RootDirectory: tmpDirSubpath}, + tmpDirSubpath: {RootDirectory: tmpDirSubpath}, + }, + }, + } + + _, err := storage.New(config, nil, nil, zlog.NewLogger("debug", "")) + So(err, ShouldBeNil) + }) + + Convey("Create unique subpath, error for cache driver", t, func() { + tmpDirSubpath := t.TempDir() + config := &config.Config{ + Storage: config.GlobalStorageConfig{ + StorageConfig: config.StorageConfig{RootDirectory: t.TempDir()}, + SubPaths: map[string]config.StorageConfig{ + "a/": { + RootDirectory: tmpDirSubpath, + RemoteCache: true, + StorageDriver: map[string]interface{}{}, + Dedupe: true, + }, + }, + }, + } + + // create boltdb file and make it un-openable + dbPath := path.Join(tmpDirSubpath, storageConstants.BoltdbName+storageConstants.DBExtensionName) + err := os.WriteFile(dbPath, []byte(""), 0o000) + So(err, ShouldBeNil) + + _, err = storage.New(config, nil, nil, zlog.NewLogger("debug", "")) + So(err, ShouldNotBeNil) + + err = os.Chmod(dbPath, 0o600) + So(err, ShouldBeNil) + }) + + Convey("storeName != constants.S3StorageDriverName", t, func() { + tmpDirSubpath := t.TempDir() + config := &config.Config{ + Storage: config.GlobalStorageConfig{ + StorageConfig: config.StorageConfig{RootDirectory: t.TempDir()}, + SubPaths: map[string]config.StorageConfig{ + "a/": { + RootDirectory: tmpDirSubpath, + RemoteCache: true, + StorageDriver: map[string]interface{}{ + "name": "bad-name", + }, + }, + }, + }, + } + + _, err := storage.New(config, nil, nil, zlog.NewLogger("debug", "")) + So(err, ShouldNotBeNil) + }) +} + func TestDeleteBlobsInUse(t *testing.T) { for _, testcase := range testCases { testcase := testcase