From 685d9829242353196825b2d34a89324519c88057 Mon Sep 17 00:00:00 2001 From: Ainar Garipov Date: Wed, 19 Jul 2023 16:57:57 +0300 Subject: [PATCH] Pull request 1930: fewer-globals Squashed commit of the following: commit ce882cfff4c1f7afdf0cba13b39e6ee568eb812f Author: Ainar Garipov Date: Wed Jul 19 15:56:52 2023 +0300 all: imp code, lint commit 96fc5c589e7474f4bba291b0a20a0834148bb9c1 Merge: 3e91eea6b b0185201c Author: Ainar Garipov Date: Tue Jul 18 21:22:32 2023 +0300 Merge branch 'master' into fewer-globals commit 3e91eea6b68bac51251784e3069b1c9d241da439 Author: Ainar Garipov Date: Tue Jul 18 19:01:45 2023 +0300 home: rm globals --- internal/home/control.go | 10 +- internal/home/controlinstall.go | 2 +- internal/home/controlupdate.go | 26 +++--- internal/home/home.go | 160 +++++++++++++++----------------- internal/home/service.go | 24 +++-- internal/home/web.go | 12 ++- scripts/make/go-lint.sh | 10 ++ 7 files changed, 132 insertions(+), 112 deletions(-) diff --git a/internal/home/control.go b/internal/home/control.go index 48afcf71..a2414b35 100644 --- a/internal/home/control.go +++ b/internal/home/control.go @@ -176,12 +176,16 @@ func handleStatus(w http.ResponseWriter, r *http.Request) { // ------------------------ // registration of handlers // ------------------------ -func registerControlHandlers() { +func registerControlHandlers(web *webAPI) { + Context.mux.HandleFunc( + "/control/version.json", + postInstall(optionalAuth(web.handleVersionJSON)), + ) + httpRegister(http.MethodPost, "/control/update", web.handleUpdate) + httpRegister(http.MethodGet, "/control/status", handleStatus) httpRegister(http.MethodPost, "/control/i18n/change_language", handleI18nChangeLanguage) httpRegister(http.MethodGet, "/control/i18n/current_language", handleI18nCurrentLanguage) - Context.mux.HandleFunc("/control/version.json", postInstall(optionalAuth(handleVersionJSON))) - httpRegister(http.MethodPost, "/control/update", handleUpdate) httpRegister(http.MethodGet, "/control/profile", handleGetProfile) httpRegister(http.MethodPut, "/control/profile/update", handlePutProfile) diff --git a/internal/home/controlinstall.go b/internal/home/controlinstall.go index c9503fa0..a5be3354 100644 --- a/internal/home/controlinstall.go +++ b/internal/home/controlinstall.go @@ -448,7 +448,7 @@ func (web *webAPI) handleInstallConfigure(w http.ResponseWriter, r *http.Request web.conf.BindHost = req.Web.IP web.conf.BindPort = req.Web.Port - registerControlHandlers() + registerControlHandlers(web) aghhttp.OK(w) if f, ok := w.(http.Flusher); ok { diff --git a/internal/home/controlupdate.go b/internal/home/controlupdate.go index 434286ba..5238134c 100644 --- a/internal/home/controlupdate.go +++ b/internal/home/controlupdate.go @@ -29,9 +29,9 @@ type temporaryError interface { // handleVersionJSON is the handler for the POST /control/version.json HTTP API. // // TODO(a.garipov): Find out if this API used with a GET method by anyone. -func handleVersionJSON(w http.ResponseWriter, r *http.Request) { +func (web *webAPI) handleVersionJSON(w http.ResponseWriter, r *http.Request) { resp := &versionResponse{} - if Context.disableUpdate { + if web.conf.disableUpdate { resp.Disabled = true _ = aghhttp.WriteJSONResponse(w, r, resp) @@ -52,7 +52,7 @@ func handleVersionJSON(w http.ResponseWriter, r *http.Request) { } } - err = requestVersionInfo(resp, req.Recheck) + err = web.requestVersionInfo(resp, req.Recheck) if err != nil { // Don't wrap the error, because it's informative enough as is. aghhttp.Error(r, w, http.StatusBadGateway, "%s", err) @@ -73,9 +73,10 @@ func handleVersionJSON(w http.ResponseWriter, r *http.Request) { // requestVersionInfo sets the VersionInfo field of resp if it can reach the // update server. -func requestVersionInfo(resp *versionResponse, recheck bool) (err error) { +func (web *webAPI) requestVersionInfo(resp *versionResponse, recheck bool) (err error) { + updater := web.conf.updater for i := 0; i != 3; i++ { - resp.VersionInfo, err = Context.updater.VersionInfo(recheck) + resp.VersionInfo, err = updater.VersionInfo(recheck) if err != nil { var terr temporaryError if errors.As(err, &terr) && terr.Temporary() { @@ -95,7 +96,7 @@ func requestVersionInfo(resp *versionResponse, recheck bool) (err error) { } if err != nil { - vcu := Context.updater.VersionCheckURL() + vcu := updater.VersionCheckURL() return fmt.Errorf("getting version info from %s: %w", vcu, err) } @@ -104,8 +105,9 @@ func requestVersionInfo(resp *versionResponse, recheck bool) (err error) { } // handleUpdate performs an update to the latest available version procedure. -func handleUpdate(w http.ResponseWriter, r *http.Request) { - if Context.updater.NewVersion() == "" { +func (web *webAPI) handleUpdate(w http.ResponseWriter, r *http.Request) { + updater := web.conf.updater + if updater.NewVersion() == "" { aghhttp.Error(r, w, http.StatusBadRequest, "/update request isn't allowed now") return @@ -122,7 +124,7 @@ func handleUpdate(w http.ResponseWriter, r *http.Request) { return } - err = Context.updater.Update(false) + err = updater.Update(false) if err != nil { aghhttp.Error(r, w, http.StatusInternalServerError, "%s", err) @@ -137,7 +139,7 @@ func handleUpdate(w http.ResponseWriter, r *http.Request) { // The background context is used because the underlying functions wrap it // with timeout and shut down the server, which handles current request. It // also should be done in a separate goroutine for the same reason. - go finishUpdate(context.Background(), execPath) + go finishUpdate(context.Background(), execPath, web.conf.runningAsService) } // versionResponse is the response for /control/version.json endpoint. @@ -178,7 +180,7 @@ func tlsConfUsesPrivilegedPorts(c *tlsConfigSettings) (ok bool) { } // finishUpdate completes an update procedure. -func finishUpdate(ctx context.Context, execPath string) { +func finishUpdate(ctx context.Context, execPath string, runningAsService bool) { var err error log.Info("stopping all tasks") @@ -187,7 +189,7 @@ func finishUpdate(ctx context.Context, execPath string) { cleanupAlways() if runtime.GOOS == "windows" { - if Context.runningAsService { + if runningAsService { // NOTE: We can't restart the service via "kardianos/service" // package, because it kills the process first we can't start a new // instance, because Windows doesn't allow it. diff --git a/internal/home/home.go b/internal/home/home.go index 5355de2a..60eb63dc 100644 --- a/internal/home/home.go +++ b/internal/home/home.go @@ -64,33 +64,24 @@ type homeContext struct { // configuration files, for example /etc/hosts. etcHosts *aghnet.HostsContainer - updater *updater.Updater - // mux is our custom http.ServeMux. mux *http.ServeMux // Runtime properties // -- - configFilename string // Config filename (can be overridden via the command line arguments) - workDir string // Location of our directory, used to protect against CWD being somewhere else - pidFileName string // PID file name. Empty if no PID file was created. - controlLock sync.Mutex - tlsRoots *x509.CertPool // list of root CAs for TLSv1.2 - appSignalChannel chan os.Signal // Channel for receiving OS signals by the console app + configFilename string // Config filename (can be overridden via the command line arguments) + workDir string // Location of our directory, used to protect against CWD being somewhere else + pidFileName string // PID file name. Empty if no PID file was created. + controlLock sync.Mutex + tlsRoots *x509.CertPool // list of root CAs for TLSv1.2 // tlsCipherIDs are the ID of the cipher suites that AdGuard Home must use. tlsCipherIDs []uint16 - // disableUpdate, if true, tells AdGuard Home to not check for updates. - disableUpdate bool - // firstRun, if true, tells AdGuard Home to only start the web interface // service, and only serve the first-run APIs. firstRun bool - - // runningAsService flag is set to true when options are passed from the service runner - runningAsService bool } // getDataDir returns path to the directory where we store databases and filters @@ -113,11 +104,11 @@ func Main(clientBuildFS fs.FS) { // package flag. opts := loadCmdLineOpts() - Context.appSignalChannel = make(chan os.Signal) - signal.Notify(Context.appSignalChannel, syscall.SIGINT, syscall.SIGTERM, syscall.SIGHUP, syscall.SIGQUIT) + signals := make(chan os.Signal, 1) + signal.Notify(signals, syscall.SIGINT, syscall.SIGTERM, syscall.SIGHUP, syscall.SIGQUIT) go func() { for { - sig := <-Context.appSignalChannel + sig := <-signals log.Info("Received signal %q", sig) switch sig { case syscall.SIGHUP: @@ -132,7 +123,7 @@ func Main(clientBuildFS fs.FS) { }() if opts.serviceControlAction != "" { - handleServiceControlAction(opts, clientBuildFS) + handleServiceControlAction(opts, clientBuildFS, signals) return } @@ -144,61 +135,48 @@ func Main(clientBuildFS fs.FS) { // setupContext initializes [Context] fields. It also reads and upgrades // config file if necessary. func setupContext(opts options) (err error) { - setupContextFlags(opts) + Context.firstRun = detectFirstRun() Context.tlsRoots = aghtls.SystemRootCAs() Context.mux = http.NewServeMux() - if !Context.firstRun { - // Do the upgrade if necessary. - err = upgradeConfig() + if Context.firstRun { + log.Info("This is the first time AdGuard Home is launched") + checkPermissions() + + return nil + } + + // Do the upgrade if necessary. + err = upgradeConfig() + if err != nil { + // Don't wrap the error, because it's informative enough as is. + return err + } + + if err = parseConfig(); err != nil { + log.Error("parsing configuration file: %s", err) + + os.Exit(1) + } + + if opts.checkConfig { + log.Info("configuration file is ok") + + os.Exit(0) + } + + if !opts.noEtcHosts && config.Clients.Sources.HostsFile { + err = setupHostsContainer() if err != nil { // Don't wrap the error, because it's informative enough as is. return err } - - if err = parseConfig(); err != nil { - log.Error("parsing configuration file: %s", err) - - os.Exit(1) - } - - if opts.checkConfig { - log.Info("configuration file is ok") - - os.Exit(0) - } - - if !opts.noEtcHosts && config.Clients.Sources.HostsFile { - err = setupHostsContainer() - if err != nil { - // Don't wrap the error, because it's informative enough as is. - return err - } - } } return nil } -// setupContextFlags sets global flags and prints their status to the log. -func setupContextFlags(opts options) { - Context.firstRun = detectFirstRun() - if Context.firstRun { - log.Info("This is the first time AdGuard Home is launched") - checkPermissions() - } - - Context.runningAsService = opts.runningAsService - // Don't print the runningAsService flag, since that has already been done - // in [run]. - - Context.disableUpdate = opts.disableUpdate || version.Channel() == version.ChannelDevelopment - if Context.disableUpdate { - log.Info("AdGuard Home updates are disabled") - } -} - // logIfUnsupported logs a formatted warning if the error is one of the // unsupported errors and returns nil. If err is nil, logIfUnsupported returns // nil. Otherwise, it returns err. @@ -303,7 +281,7 @@ func initContextClients() (err error) { return err } - //lint:ignore SA1019 Migration is not over. + //lint:ignore SA1019 Migration is not over. config.DHCP.WorkDir = Context.workDir config.DHCP.DataDir = Context.getDataDir() config.DHCP.HTTPRegister = httpRegister @@ -318,18 +296,6 @@ func initContextClients() (err error) { return fmt.Errorf("initing dhcp: %w", err) } - Context.updater = updater.NewUpdater(&updater.Config{ - Client: config.DNS.DnsfilterConf.HTTPClient, - Version: version.Version(), - Channel: version.Channel(), - GOARCH: runtime.GOARCH, - GOOS: runtime.GOOS, - GOARM: version.GOARM(), - GOMIPS: version.GOMIPS(), - WorkDir: Context.workDir, - ConfName: config.getConfigFilename(), - }) - var arpdb aghnet.ARPDB if config.Clients.Sources.ARP { arpdb = aghnet.NewARPDB() @@ -493,7 +459,7 @@ func checkPorts() (err error) { return nil } -func initWeb(opts options, clientBuildFS fs.FS) (web *webAPI, err error) { +func initWeb(opts options, clientBuildFS fs.FS, upd *updater.Updater) (web *webAPI, err error) { var clientFS fs.FS if opts.localFrontend { log.Info("warning: using local frontend files") @@ -506,8 +472,16 @@ func initWeb(opts options, clientBuildFS fs.FS) (web *webAPI, err error) { } } - webConf := webConfig{ - firstRun: Context.firstRun, + disableUpdate := opts.disableUpdate || version.Channel() == version.ChannelDevelopment + if disableUpdate { + log.Info("AdGuard Home updates are disabled") + } + + webConf := &webConfig{ + updater: upd, + + clientFS: clientFS, + BindHost: config.HTTPConfig.Address.Addr(), BindPort: int(config.HTTPConfig.Address.Port()), @@ -515,12 +489,13 @@ func initWeb(opts options, clientBuildFS fs.FS) (web *webAPI, err error) { ReadHeaderTimeout: readHdrTimeout, WriteTimeout: writeTimeout, - clientFS: clientFS, - - serveHTTP3: config.DNS.ServeHTTP3, + firstRun: Context.firstRun, + disableUpdate: disableUpdate, + runningAsService: opts.runningAsService, + serveHTTP3: config.DNS.ServeHTTP3, } - web = newWebAPI(&webConf) + web = newWebAPI(webConf) if web == nil { return nil, fmt.Errorf("initializing web: %w", err) } @@ -571,9 +546,21 @@ func run(opts options, clientBuildFS fs.FS) { err = setupOpts(opts) fatalOnError(err) + upd := updater.NewUpdater(&updater.Config{ + Client: config.DNS.DnsfilterConf.HTTPClient, + Version: version.Version(), + Channel: version.Channel(), + GOARCH: runtime.GOARCH, + GOOS: runtime.GOOS, + GOARM: version.GOARM(), + GOMIPS: version.GOMIPS(), + WorkDir: Context.workDir, + ConfName: config.getConfigFilename(), + }) + // TODO(e.burkov): This could be made earlier, probably as the option's // effect. - cmdlineUpdate(opts) + cmdlineUpdate(opts, upd) if !Context.firstRun { // Save the updated config. @@ -602,7 +589,7 @@ func run(opts options, clientBuildFS fs.FS) { onConfigModified() } - Context.web, err = initWeb(opts, clientBuildFS) + Context.web, err = initWeb(opts, clientBuildFS, upd) fatalOnError(err) if !Context.firstRun { @@ -984,7 +971,7 @@ type jsonError struct { } // cmdlineUpdate updates current application and exits. -func cmdlineUpdate(opts options) { +func cmdlineUpdate(opts options, upd *updater.Updater) { if !opts.performUpdate { return } @@ -999,10 +986,9 @@ func cmdlineUpdate(opts options) { log.Info("cmdline update: performing update") - updater := Context.updater - info, err := updater.VersionInfo(true) + info, err := upd.VersionInfo(true) if err != nil { - vcu := updater.VersionCheckURL() + vcu := upd.VersionCheckURL() log.Error("getting version info from %s: %s", vcu, err) os.Exit(1) @@ -1014,7 +1000,7 @@ func cmdlineUpdate(opts options) { os.Exit(0) } - err = updater.Update(Context.firstRun) + err = upd.Update(Context.firstRun) fatalOnError(err) err = restartService() diff --git a/internal/home/service.go b/internal/home/service.go index 3ec44138..e98aa030 100644 --- a/internal/home/service.go +++ b/internal/home/service.go @@ -33,9 +33,13 @@ const ( // daemon. type program struct { clientBuildFS fs.FS + signals chan os.Signal opts options } +// type check +var _ service.Interface = (*program)(nil) + // Start implements service.Interface interface for *program. func (p *program) Start(_ service.Service) (err error) { // Start should not block. Do the actual work async. @@ -48,14 +52,14 @@ func (p *program) Start(_ service.Service) (err error) { } // Stop implements service.Interface interface for *program. -func (p *program) Stop(_ service.Service) error { - // Stop should not block. Return with a few seconds. - if Context.appSignalChannel == nil { - os.Exit(0) +func (p *program) Stop(_ service.Service) (err error) { + select { + case p.signals <- syscall.SIGINT: + // Go on. + default: + // Stop should not block. } - Context.appSignalChannel <- syscall.SIGINT - return nil } @@ -194,7 +198,7 @@ func restartService() (err error) { // - run: This is a special command that is not supposed to be used directly // it is specified when we register a service, and it indicates to the app // that it is being run as a service/daemon. -func handleServiceControlAction(opts options, clientBuildFS fs.FS) { +func handleServiceControlAction(opts options, clientBuildFS fs.FS, signals chan os.Signal) { // Call chooseSystem explicitly to introduce OpenBSD support for service // package. It's a noop for other GOOS values. chooseSystem() @@ -226,7 +230,11 @@ func handleServiceControlAction(opts options, clientBuildFS fs.FS) { } configureService(svcConfig) - s, err := service.New(&program{clientBuildFS: clientBuildFS, opts: runOpts}, svcConfig) + s, err := service.New(&program{ + clientBuildFS: clientBuildFS, + signals: signals, + opts: runOpts, + }, svcConfig) if err != nil { log.Fatalf("service: initializing service: %s", err) } diff --git a/internal/home/web.go b/internal/home/web.go index dd6386de..0578776a 100644 --- a/internal/home/web.go +++ b/internal/home/web.go @@ -11,6 +11,7 @@ import ( "github.com/AdguardTeam/AdGuardHome/internal/aghhttp" "github.com/AdguardTeam/AdGuardHome/internal/aghnet" + "github.com/AdguardTeam/AdGuardHome/internal/updater" "github.com/AdguardTeam/golibs/errors" "github.com/AdguardTeam/golibs/log" "github.com/AdguardTeam/golibs/netutil" @@ -32,6 +33,8 @@ const ( ) type webConfig struct { + updater *updater.Updater + clientFS fs.FS BindHost netip.Addr @@ -51,6 +54,13 @@ type webConfig struct { firstRun bool + // disableUpdate, if true, tells AdGuard Home to not check for updates. + disableUpdate bool + + // runningAsService flag is set to true when options are passed from the + // service runner. + runningAsService bool + serveHTTP3 bool } @@ -101,7 +111,7 @@ func newWebAPI(conf *webConfig) (w *webAPI) { Context.mux.Handle("/install.html", preInstallHandler(clientFS)) w.registerInstallHandlers() } else { - registerControlHandlers() + registerControlHandlers(w) } w.httpsServer.cond = sync.NewCond(&w.httpsServer.condLock) diff --git a/scripts/make/go-lint.sh b/scripts/make/go-lint.sh index b8a0c706..580ee155 100644 --- a/scripts/make/go-lint.sh +++ b/scripts/make/go-lint.sh @@ -189,6 +189,16 @@ run_linter gocognit --over 10\ ./scripts/\ ; +# TODO(a.garipov): move these to the group above. +run_linter gocognit --over 20 ./internal/aghnet/ ./internal/querylog/ +run_linter gocognit --over 19 ./internal/dnsforward/ ./internal/home/ +run_linter gocognit --over 18 ./internal/aghtls/ +run_linter gocognit --over 17 ./internal/filtering ./internal/filtering/rewrite/ +run_linter gocognit --over 15 ./internal/aghos/ ./internal/dhcpd/ +run_linter gocognit --over 14 ./internal/stats/ +run_linter gocognit --over 12 ./internal/updater/ +run_linter gocognit --over 11 ./internal/aghtest/ + run_linter ineffassign ./... run_linter unparam ./...