diff --git a/pkg/api/controller.go b/pkg/api/controller.go index fdf09c22..c0429c81 100644 --- a/pkg/api/controller.go +++ b/pkg/api/controller.go @@ -31,16 +31,14 @@ const ( ) type Controller struct { - Config *config.Config - Router *mux.Router - StoreController storage.StoreController - Log log.Logger - Audit *log.Logger - Server *http.Server - Metrics monitoring.MetricServer - wgShutDown *goSync.WaitGroup // use it to gracefully shutdown goroutines - reloadCtx context.Context // use it to gracefully reload goroutines with new configuration - cancelOnReloadFunc context.CancelFunc // use it to stop goroutines + Config *config.Config + Router *mux.Router + StoreController storage.StoreController + Log log.Logger + Audit *log.Logger + Server *http.Server + Metrics monitoring.MetricServer + wgShutDown *goSync.WaitGroup // use it to gracefully shutdown goroutines } func NewController(config *config.Config) *Controller { @@ -50,9 +48,6 @@ func NewController(config *config.Config) *Controller { controller.Config = config controller.Log = logger controller.wgShutDown = new(goSync.WaitGroup) - /* context used to cancel go routines so that - we can change their config on the fly (restart routines with different config) */ - controller.reloadCtx, controller.cancelOnReloadFunc = context.WithCancel(context.Background()) if config.Log.Audit != "" { audit := log.NewAuditLogger(config.Log.Level, config.Log.Audit) @@ -106,7 +101,7 @@ func DumpRuntimeParams(log log.Logger) { evt.Msg("runtime params") } -func (c *Controller) Run() error { +func (c *Controller) Run(reloadCtx context.Context) error { // validate configuration if err := c.Config.Validate(c.Log); err != nil { c.Log.Error().Err(err).Msg("configuration validation failed") @@ -157,13 +152,14 @@ func (c *Controller) Run() error { c.Metrics = monitoring.NewMetricsServer(enabled, c.Log) - if err := c.InitImageStore(); err != nil { + if err := c.InitImageStore(reloadCtx); err != nil { return err } monitoring.SetServerInfo(c.Metrics, c.Config.Commit, c.Config.BinaryType, c.Config.GoVersion, c.Config.DistSpecVersion) + // nolint: contextcheck _ = NewRouteHandler(c) addr := fmt.Sprintf("%s:%s", c.Config.HTTP.Address, c.Config.HTTP.Port) @@ -225,7 +221,7 @@ func (c *Controller) Run() error { return server.Serve(listener) } -func (c *Controller) InitImageStore() error { +func (c *Controller) InitImageStore(reloadCtx context.Context) error { c.StoreController = storage.StoreController{} if c.Config.Storage.RootDirectory != "" { @@ -326,28 +322,22 @@ func (c *Controller) InitImageStore() error { // Enable extensions if extension config is provided if c.Config.Extensions != nil && c.Config.Extensions.Sync != nil && *c.Config.Extensions.Sync.Enable { - ext.EnableSyncExtension(c.reloadCtx, c.Config, c.wgShutDown, c.StoreController, c.Log) + ext.EnableSyncExtension(reloadCtx, c.Config, c.wgShutDown, c.StoreController, c.Log) } return nil } -func (c *Controller) LoadNewConfig(config *config.Config) { - // cancel go routines context so we can reload configuration - c.cancelOnReloadFunc() - +func (c *Controller) LoadNewConfig(reloadCtx context.Context, config *config.Config) { // reload access control config c.Config.AccessControl = config.AccessControl c.Config.HTTP.RawAccessControl = config.HTTP.RawAccessControl - // create new context for the next config reload - c.reloadCtx, c.cancelOnReloadFunc = context.WithCancel(context.Background()) - // Enable extensions if extension config is provided if config.Extensions != nil && config.Extensions.Sync != nil { // reload sync config c.Config.Extensions.Sync = config.Extensions.Sync - ext.EnableSyncExtension(c.reloadCtx, c.Config, c.wgShutDown, c.StoreController, c.Log) + ext.EnableSyncExtension(reloadCtx, c.Config, c.wgShutDown, c.StoreController, c.Log) } else if c.Config.Extensions != nil { c.Config.Extensions.Sync = nil } diff --git a/pkg/api/controller_test.go b/pkg/api/controller_test.go index 74fd6365..3eaaabee 100644 --- a/pkg/api/controller_test.go +++ b/pkg/api/controller_test.go @@ -115,7 +115,7 @@ func TestRunAlreadyRunningServer(t *testing.T) { ctlr.Config.Storage.RootDirectory = globalDir go func() { - if err := ctlr.Run(); err != nil { + if err := ctlr.Run(context.Background()); err != nil { return } }() @@ -135,7 +135,7 @@ func TestRunAlreadyRunningServer(t *testing.T) { _ = ctlr.Server.Shutdown(ctx) }() - err := ctlr.Run() + err := ctlr.Run(context.Background()) So(err, ShouldNotBeNil) }) } @@ -156,7 +156,7 @@ func TestObjectStorageController(t *testing.T) { ctlr.Config.Storage.RootDirectory = "zot" - err := ctlr.Run() + err := ctlr.Run(context.Background()) So(err, ShouldNotBeNil) }) @@ -770,7 +770,7 @@ func TestMultipleInstance(t *testing.T) { }, } ctlr := api.NewController(conf) - err := ctlr.Run() + err := ctlr.Run(context.Background()) So(err, ShouldEqual, errors.ErrImgStoreNotFound) globalDir := t.TempDir() @@ -3006,7 +3006,7 @@ func TestImageSignatures(t *testing.T) { ctlr.Config.Storage.RootDirectory = dir go func(controller *api.Controller) { // this blocks - if err := controller.Run(); err != nil { + if err := controller.Run(context.Background()); err != nil { return } }(ctlr) @@ -4198,7 +4198,8 @@ func getAllManifests(imagePath string) []string { func startServer(c *api.Controller) { // this blocks - if err := c.Run(); err != nil { + ctx := context.Background() + if err := c.Run(ctx); err != nil { return } } diff --git a/pkg/api/routes.go b/pkg/api/routes.go index 3944dd20..06796507 100644 --- a/pkg/api/routes.go +++ b/pkg/api/routes.go @@ -42,6 +42,7 @@ type RouteHandler struct { c *Controller } +// nolint: contextcheck func NewRouteHandler(c *Controller) *RouteHandler { rh := &RouteHandler{c: c} rh.SetupRoutes() @@ -53,6 +54,7 @@ func allowedMethods(method string) []string { return []string{http.MethodOptions, method} } +// nolint: contextcheck func (rh *RouteHandler) SetupRoutes() { rh.c.Router.Use(AuthHandler(rh.c)) // authz is being enabled because authn is found diff --git a/pkg/cli/client_test.go b/pkg/cli/client_test.go index ac00ba45..159ec552 100644 --- a/pkg/cli/client_test.go +++ b/pkg/cli/client_test.go @@ -72,7 +72,7 @@ func TestTLSWithAuth(t *testing.T) { ctlr.Config.Storage.RootDirectory = t.TempDir() go func() { // this blocks - if err := ctlr.Run(); err != nil { + if err := ctlr.Run(context.Background()); err != nil { return } }() @@ -164,7 +164,7 @@ func TestTLSWithoutAuth(t *testing.T) { ctlr.Config.Storage.RootDirectory = t.TempDir() go func() { // this blocks - if err := ctlr.Run(); err != nil { + if err := ctlr.Run(context.Background()); err != nil { return } }() @@ -227,7 +227,7 @@ func TestTLSWithoutAuth(t *testing.T) { ctlr.Config.Storage.RootDirectory = t.TempDir() go func() { // this blocks - if err := ctlr.Run(); err != nil { + if err := ctlr.Run(context.Background()); err != nil { return } }() @@ -285,7 +285,7 @@ func TestTLSBadCerts(t *testing.T) { ctlr.Config.Storage.RootDirectory = t.TempDir() go func() { // this blocks - if err := ctlr.Run(); err != nil { + if err := ctlr.Run(context.Background()); err != nil { return } }() diff --git a/pkg/cli/config_reloader.go b/pkg/cli/config_reloader.go index 50ca4222..62fb907b 100644 --- a/pkg/cli/config_reloader.go +++ b/pkg/cli/config_reloader.go @@ -1,6 +1,8 @@ package cli import ( + "context" + "github.com/fsnotify/fsnotify" "github.com/rs/zerolog/log" "zotregistry.io/zot/pkg/api" @@ -29,8 +31,10 @@ func NewHotReloader(ctlr *api.Controller, filePath string) (*HotReloader, error) return hotReloader, nil } -func (hr *HotReloader) Start() { +func (hr *HotReloader) Start() context.Context { done := make(chan bool) + + reloadCtx, cancelOnReloadFunc := context.WithCancel(context.Background()) // run watcher go func() { defer hr.watcher.Close() @@ -51,8 +55,12 @@ func (hr *HotReloader) Start() { continue } + // if valid config then reload + cancelOnReloadFunc() - hr.ctlr.LoadNewConfig(newConfig) + // create new context + reloadCtx, cancelOnReloadFunc = context.WithCancel(context.Background()) + hr.ctlr.LoadNewConfig(reloadCtx, newConfig) } // watch for errors case err := <-hr.watcher.Errors: @@ -69,4 +77,6 @@ func (hr *HotReloader) Start() { <-done }() + + return reloadCtx } diff --git a/pkg/cli/cve_cmd_test.go b/pkg/cli/cve_cmd_test.go index 5cbf2ef2..9d758049 100644 --- a/pkg/cli/cve_cmd_test.go +++ b/pkg/cli/cve_cmd_test.go @@ -314,7 +314,7 @@ func TestServerCVEResponse(t *testing.T) { go func(controller *api.Controller) { // this blocks - if err := controller.Run(); err != nil { + if err := controller.Run(context.Background()); err != nil { return } }(ctlr) diff --git a/pkg/cli/image_cmd_test.go b/pkg/cli/image_cmd_test.go index 555e1e14..2c03e6d7 100644 --- a/pkg/cli/image_cmd_test.go +++ b/pkg/cli/image_cmd_test.go @@ -294,7 +294,7 @@ func TestServerResponse(t *testing.T) { ctlr.Config.Storage.RootDirectory = t.TempDir() go func(controller *api.Controller) { // this blocks - if err := controller.Run(); err != nil { + if err := controller.Run(context.Background()); err != nil { return } }(ctlr) diff --git a/pkg/cli/root.go b/pkg/cli/root.go index 6022dcc1..8b7a779e 100644 --- a/pkg/cli/root.go +++ b/pkg/cli/root.go @@ -44,14 +44,17 @@ func newServeCmd(conf *config.Config) *cobra.Command { ctlr := api.NewController(conf) + // config reloader hotReloader, err := NewHotReloader(ctlr, args[0]) if err != nil { panic(err) } - hotReloader.Start() + /* context used to cancel go routines so that + we can change their config on the fly (restart routines with different config) */ + reloaderCtx := hotReloader.Start() - if err := ctlr.Run(); err != nil { + if err := ctlr.Run(reloaderCtx); err != nil { panic(err) } }, @@ -100,7 +103,7 @@ func newScrubCmd(conf *config.Config) *cobra.Command { ctlr := api.NewController(conf) ctlr.Metrics = monitoring.NewMetricsServer(false, ctlr.Log) - if err := ctlr.InitImageStore(); err != nil { + if err := ctlr.InitImageStore(context.Background()); err != nil { panic(err) } diff --git a/pkg/cli/root_test.go b/pkg/cli/root_test.go index 0ddcc8f3..b2461415 100644 --- a/pkg/cli/root_test.go +++ b/pkg/cli/root_test.go @@ -409,7 +409,7 @@ func TestScrub(t *testing.T) { controller.Config.Storage.RootDirectory = dir go func(controller *api.Controller) { // this blocks - if err := controller.Run(); err != nil { + if err := controller.Run(context.Background()); err != nil { return } }(controller) diff --git a/pkg/compliance/v1_0_0/check_test.go b/pkg/compliance/v1_0_0/check_test.go index 6130e53d..67025e3d 100644 --- a/pkg/compliance/v1_0_0/check_test.go +++ b/pkg/compliance/v1_0_0/check_test.go @@ -81,7 +81,7 @@ func startServer(t *testing.T) (*api.Controller, string) { go func() { // this blocks - if err := ctrl.Run(); err != nil { + if err := ctrl.Run(context.Background()); err != nil { return } }() diff --git a/pkg/exporter/api/controller_test.go b/pkg/exporter/api/controller_test.go index d1e4e469..eea74d62 100644 --- a/pkg/exporter/api/controller_test.go +++ b/pkg/exporter/api/controller_test.go @@ -127,7 +127,7 @@ func TestNewExporter(t *testing.T) { serverController.Config.Storage.RootDirectory = dir go func(c *zotapi.Controller) { // this blocks - if err := c.Run(); !errors.Is(err, http.ErrServerClosed) { + if err := c.Run(context.Background()); !errors.Is(err, http.ErrServerClosed) { panic(err) } }(serverController) diff --git a/pkg/extensions/monitoring/monitoring_test.go b/pkg/extensions/monitoring/monitoring_test.go index 06aea0e8..6a82ce3d 100644 --- a/pkg/extensions/monitoring/monitoring_test.go +++ b/pkg/extensions/monitoring/monitoring_test.go @@ -105,7 +105,7 @@ func TestExtensionMetrics(t *testing.T) { func startServer(c *api.Controller) { // this blocks - if err := c.Run(); err != nil { + if err := c.Run(context.Background()); err != nil { return } } diff --git a/pkg/extensions/search/common/common_test.go b/pkg/extensions/search/common/common_test.go index 316d54e2..cb834312 100644 --- a/pkg/extensions/search/common/common_test.go +++ b/pkg/extensions/search/common/common_test.go @@ -200,7 +200,7 @@ func TestLatestTagSearchHTTP(t *testing.T) { go func() { // this blocks - if err := ctlr.Run(); err != nil { + if err := ctlr.Run(context.Background()); err != nil { return } }() @@ -338,7 +338,7 @@ func TestExpandedRepoInfo(t *testing.T) { go func() { // this blocks - if err := ctlr.Run(); err != nil { + if err := ctlr.Run(context.Background()); err != nil { return } }() diff --git a/pkg/extensions/search/cve/cve_test.go b/pkg/extensions/search/cve/cve_test.go index 8430cae4..4d29051b 100644 --- a/pkg/extensions/search/cve/cve_test.go +++ b/pkg/extensions/search/cve/cve_test.go @@ -399,7 +399,7 @@ func TestCVESearch(t *testing.T) { go func() { // this blocks - if err := ctlr.Run(); err != nil { + if err := ctlr.Run(context.Background()); err != nil { return } }() @@ -634,7 +634,7 @@ func TestCVEConfig(t *testing.T) { go func() { // this blocks - if err := ctlr.Run(); err != nil { + if err := ctlr.Run(context.Background()); err != nil { return } }() @@ -707,7 +707,7 @@ func TestHTTPOptionsResponse(t *testing.T) { go func() { // this blocks - if err := ctlr.Run(); err != nil { + if err := ctlr.Run(context.Background()); err != nil { return } }() diff --git a/pkg/extensions/search/digest/digest_test.go b/pkg/extensions/search/digest/digest_test.go index 93c38407..efebfc24 100644 --- a/pkg/extensions/search/digest/digest_test.go +++ b/pkg/extensions/search/digest/digest_test.go @@ -154,7 +154,7 @@ func TestDigestSearchHTTP(t *testing.T) { go func() { // this blocks - if err := ctlr.Run(); err != nil { + if err := ctlr.Run(context.Background()); err != nil { return } }() @@ -296,7 +296,7 @@ func TestDigestSearchHTTPSubPaths(t *testing.T) { go func() { // this blocks - if err := ctlr.Run(); err != nil { + if err := ctlr.Run(context.Background()); err != nil { return } }() @@ -355,7 +355,7 @@ func TestDigestSearchDisabled(t *testing.T) { go func() { // this blocks - if err := ctlr.Run(); err != nil { + if err := ctlr.Run(context.Background()); err != nil { return } }() diff --git a/pkg/extensions/sync/sync_test.go b/pkg/extensions/sync/sync_test.go index 7d859d88..30d744df 100644 --- a/pkg/extensions/sync/sync_test.go +++ b/pkg/extensions/sync/sync_test.go @@ -160,7 +160,7 @@ func startUpstreamServer( go func() { // this blocks - if err := sctlr.Run(); err != nil { + if err := sctlr.Run(context.Background()); err != nil { return } }() @@ -233,7 +233,7 @@ func startDownstreamServer( go func() { // this blocks - if err := dctlr.Run(); err != nil { + if err := dctlr.Run(context.Background()); err != nil { return } }() @@ -611,7 +611,7 @@ func TestOnDemandPermsDenied(t *testing.T) { go func() { // this blocks - if err := dctlr.Run(); err != nil { + if err := dctlr.Run(context.Background()); err != nil { return } }() @@ -701,9 +701,26 @@ func TestConfigReloader(t *testing.T) { dctlr.Shutdown() }() + content := fmt.Sprintf(`{"distSpecVersion": "0.1.0-dev", "storage": {"rootDirectory": "%s"}, + "http": {"address": "127.0.0.1", "port": "%s", "ReadOnly": false}, + "log": {"level": "debug", "output": "%s"}}`, destDir, destPort, logFile.Name()) + + cfgfile, err := ioutil.TempFile("", "zot-test*.json") + So(err, ShouldBeNil) + + defer os.Remove(cfgfile.Name()) // clean up + + _, err = cfgfile.Write([]byte(content)) + So(err, ShouldBeNil) + + hotReloader, err := cli.NewHotReloader(dctlr, cfgfile.Name()) + So(err, ShouldBeNil) + + reloadCtx := hotReloader.Start() + go func() { // this blocks - if err := dctlr.Run(); err != nil { + if err := dctlr.Run(reloadCtx); err != nil { return } }() @@ -718,23 +735,6 @@ func TestConfigReloader(t *testing.T) { time.Sleep(100 * time.Millisecond) } - content := fmt.Sprintf(`{"distSpecVersion": "0.1.0-dev", "storage": {"rootDirectory": "%s"}, - "http": {"address": "127.0.0.1", "port": "%s", "ReadOnly": false}, - "log": {"level": "debug", "output": "%s"}}`, destDir, destPort, logFile.Name()) - - cfgfile, err := ioutil.TempFile("", "zot-test*.json") - So(err, ShouldBeNil) - - defer os.Remove(cfgfile.Name()) // clean up - - _, err = cfgfile.Write([]byte(content)) - So(err, ShouldBeNil) - - hotReloader, err := cli.NewHotReloader(dctlr, cfgfile.Name()) - So(err, ShouldBeNil) - - hotReloader.Start() - // let it sync time.Sleep(3 * time.Second) @@ -1055,7 +1055,7 @@ func TestBasicAuth(t *testing.T) { go func() { // this blocks - if err := dctlr.Run(); err != nil { + if err := dctlr.Run(context.Background()); err != nil { return } }() @@ -1662,7 +1662,7 @@ func TestSubPaths(t *testing.T) { go func() { // this blocks - if err := sctlr.Run(); err != nil { + if err := sctlr.Run(context.Background()); err != nil { return } }() @@ -1729,7 +1729,7 @@ func TestSubPaths(t *testing.T) { go func() { // this blocks - if err := dctlr.Run(); err != nil { + if err := dctlr.Run(context.Background()); err != nil { return } }() @@ -2099,7 +2099,8 @@ func TestPeriodicallySignaturesErr(t *testing.T) { defer func() { _ = os.Chdir(cwd) }() tdir := t.TempDir() - _ = os.Chdir(tdir) + err = os.Chdir(tdir) + So(err, ShouldBeNil) generateKeyPairs(tdir) So(func() { signImage(tdir, srcPort, repoName, digest) }, ShouldNotPanic) @@ -2632,7 +2633,7 @@ func TestOnDemandRetryGoroutine(t *testing.T) { // start upstream server go func() { // this blocks - if err := sctlr.Run(); err != nil { + if err := sctlr.Run(context.Background()); err != nil { return } }() @@ -2790,7 +2791,7 @@ func TestOnDemandMultipleRetries(t *testing.T) { // start upstream server go func() { // this blocks - if err := sctlr.Run(); err != nil { + if err := sctlr.Run(context.Background()); err != nil { return } }() @@ -3273,7 +3274,7 @@ func TestSyncOnlyDiff(t *testing.T) { go func() { // this blocks - if err := dctlr.Run(); err != nil { + if err := dctlr.Run(context.Background()); err != nil { return } }() @@ -3422,7 +3423,7 @@ func TestSyncWithDiffDigest(t *testing.T) { go func() { // this blocks - if err := dctlr.Run(); err != nil { + if err := dctlr.Run(context.Background()); err != nil { return } }() diff --git a/pkg/log/log_test.go b/pkg/log/log_test.go index d42dc9a2..dd734ee0 100644 --- a/pkg/log/log_test.go +++ b/pkg/log/log_test.go @@ -72,7 +72,7 @@ func TestAuditLogMessages(t *testing.T) { ctlr.Config.Storage.RootDirectory = dir go func() { // this blocks - if err := ctlr.Run(); err != nil { + if err := ctlr.Run(context.Background()); err != nil { return } }()