lint: Move out config reloader context from controller struct

Signed-off-by: Petu Eusebiu <peusebiu@cisco.com>

pkg/api/controller.go

@@ -31,16 +31,14 @@ const (
 )
 
 type Controller struct {
-	Config             *config.Config
+	Config          *config.Config
-	Router             *mux.Router
+	Router          *mux.Router
-	StoreController    storage.StoreController
+	StoreController storage.StoreController
-	Log                log.Logger
+	Log             log.Logger
-	Audit              *log.Logger
+	Audit           *log.Logger
-	Server             *http.Server
+	Server          *http.Server
-	Metrics            monitoring.MetricServer
+	Metrics         monitoring.MetricServer
-	wgShutDown         *goSync.WaitGroup  // use it to gracefully shutdown goroutines
+	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
 }
 
 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) {
+func (c *Controller) LoadNewConfig(reloadCtx context.Context, config *config.Config) {
-	// cancel go routines context so we can reload configuration
-	c.cancelOnReloadFunc()
-
 	// 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
 	}

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
 	}
 }

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

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
 			}
 		}()

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
 }

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)

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)

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)
 				}
 

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)

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
 		}
 	}()

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)

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
 	}
 }

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
 			}
 		}()

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
 			}
 		}()

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
 			}
 		}()

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
 			}
 		}()

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
 			}
 		}()