From 2a1ad532f454ed2ab25d19eaf168ba9f6d534f2f Mon Sep 17 00:00:00 2001 From: Ainar Garipov Date: Mon, 25 Apr 2022 18:41:39 +0300 Subject: [PATCH] Pull request: home: rm unnecessary locking in update; refactor Merge in DNS/adguard-home from 4499-rm-unnecessary-locking to master Squashed commit of the following: commit 6d70472506dd0fd69225454c73d9f7f6a208b76b Author: Ainar Garipov Date: Mon Apr 25 17:26:54 2022 +0300 home: rm unnecessary locking in update; refactor --- CHANGELOG.md | 2 + .../aghnet/{net_nolinux.go => net_bsd.go} | 4 +- internal/aghnet/net_windows.go | 8 +- internal/home/controlupdate.go | 122 ++++++++++-------- internal/updater/check.go | 2 +- scripts/make/go-lint.sh | 1 - 6 files changed, 79 insertions(+), 60 deletions(-) rename internal/aghnet/{net_nolinux.go => net_bsd.go} (69%) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9d82fb57..b43f0524 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -89,6 +89,7 @@ In this release, the schema version has changed from 12 to 13. ### Fixed +- Slow version update queries making other HTTP APIs unresponsible ([#4499]). - ARP tables refreshing process causing excessive PTR requests ([#3157]). [#1730]: https://github.com/AdguardTeam/AdGuardHome/issues/1730 @@ -106,6 +107,7 @@ In this release, the schema version has changed from 12 to 13. [#4221]: https://github.com/AdguardTeam/AdGuardHome/issues/4221 [#4238]: https://github.com/AdguardTeam/AdGuardHome/issues/4238 [#4276]: https://github.com/AdguardTeam/AdGuardHome/issues/4276 +[#4499]: https://github.com/AdguardTeam/AdGuardHome/issues/4499 [repr]: https://reproducible-builds.org/docs/source-date-epoch/ [doq-draft-10]: https://datatracker.ietf.org/doc/html/draft-ietf-dprive-dnsoquic-10#section-10.2 diff --git a/internal/aghnet/net_nolinux.go b/internal/aghnet/net_bsd.go similarity index 69% rename from internal/aghnet/net_nolinux.go rename to internal/aghnet/net_bsd.go index f429c6fa..bd705e92 100644 --- a/internal/aghnet/net_nolinux.go +++ b/internal/aghnet/net_bsd.go @@ -1,5 +1,5 @@ -//go:build !linux -// +build !linux +//go:build darwin || freebsd || openbsd +// +build darwin freebsd openbsd package aghnet diff --git a/internal/aghnet/net_windows.go b/internal/aghnet/net_windows.go index 0cea8fe7..17499cce 100644 --- a/internal/aghnet/net_windows.go +++ b/internal/aghnet/net_windows.go @@ -1,5 +1,5 @@ -//go:build !(linux || darwin || freebsd || openbsd) -// +build !linux,!darwin,!freebsd,!openbsd +//go:build windows +// +build windows package aghnet @@ -13,6 +13,10 @@ import ( "golang.org/x/sys/windows" ) +func canBindPrivilegedPorts() (can bool, err error) { + return true, nil +} + func ifaceHasStaticIP(string) (ok bool, err error) { return false, aghos.Unsupported("checking static ip") } diff --git a/internal/home/controlupdate.go b/internal/home/controlupdate.go index 79b9f37e..ae469598 100644 --- a/internal/home/controlupdate.go +++ b/internal/home/controlupdate.go @@ -3,6 +3,7 @@ package home import ( "context" "encoding/json" + "fmt" "net/http" "os" "os/exec" @@ -27,12 +28,16 @@ type temporaryError interface { // Get the latest available version from the Internet func handleGetVersionJSON(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + resp := &versionResponse{} if Context.disableUpdate { - // w.Header().Set("Content-Type", "application/json") resp.Disabled = true - _ = json.NewEncoder(w).Encode(resp) - // TODO(e.burkov): Add error handling and deal with headers. + err := json.NewEncoder(w).Encode(resp) + if err != nil { + aghhttp.Error(r, w, http.StatusInternalServerError, "writing body: %s", err) + } + return } @@ -44,30 +49,48 @@ func handleGetVersionJSON(w http.ResponseWriter, r *http.Request) { if r.ContentLength != 0 { err = json.NewDecoder(r.Body).Decode(req) if err != nil { - aghhttp.Error(r, w, http.StatusBadRequest, "JSON parse: %s", err) + aghhttp.Error(r, w, http.StatusBadRequest, "parsing request: %s", err) return } } + err = 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) + + return + } + + err = resp.setAllowedToAutoUpdate() + if err != nil { + // Don't wrap the error, because it's informative enough as is. + aghhttp.Error(r, w, http.StatusInternalServerError, "%s", err) + + return + } + + err = json.NewEncoder(w).Encode(resp) + if err != nil { + aghhttp.Error(r, w, http.StatusInternalServerError, "writing body: %s", err) + } +} + +// requestVersionInfo sets the VersionInfo field of resp if it can reach the +// update server. +func requestVersionInfo(resp *versionResponse, recheck bool) (err error) { for i := 0; i != 3; i++ { - func() { - Context.controlLock.Lock() - defer Context.controlLock.Unlock() - - resp.VersionInfo, err = Context.updater.VersionInfo(req.Recheck) - }() - + resp.VersionInfo, err = Context.updater.VersionInfo(recheck) if err != nil { var terr temporaryError if errors.As(err, &terr) && terr.Temporary() { - // Temporary network error. This case may happen while - // we're restarting our DNS server. Log and sleep for - // some time. + // Temporary network error. This case may happen while we're + // restarting our DNS server. Log and sleep for some time. // // See https://github.com/AdguardTeam/AdGuardHome/issues/934. d := time.Duration(i) * time.Second - log.Info("temp net error: %q; sleeping for %s and retrying", err, d) + log.Info("update: temp net error: %q; sleeping for %s and retrying", err, d) time.Sleep(d) continue @@ -76,29 +99,14 @@ func handleGetVersionJSON(w http.ResponseWriter, r *http.Request) { break } + if err != nil { vcu := Context.updater.VersionCheckURL() - // TODO(a.garipov): Figure out the purpose of %T verb. - aghhttp.Error( - r, - w, - http.StatusBadGateway, - "Couldn't get version check json from %s: %T %s\n", - vcu, - err, - err, - ) - return + return fmt.Errorf("getting version info from %s: %s", vcu, err) } - resp.confirmAutoUpdate() - - w.Header().Set("Content-Type", "application/json") - err = json.NewEncoder(w).Encode(resp) - if err != nil { - aghhttp.Error(r, w, http.StatusInternalServerError, "Couldn't write body: %s", err) - } + return nil } // handleUpdate performs an update to the latest available version procedure. @@ -132,31 +140,37 @@ func handleUpdate(w http.ResponseWriter, r *http.Request) { // versionResponse is the response for /control/version.json endpoint. type versionResponse struct { - Disabled bool `json:"disabled"` updater.VersionInfo + Disabled bool `json:"disabled"` } -// confirmAutoUpdate checks the real possibility of auto update. -func (vr *versionResponse) confirmAutoUpdate() { - if vr.CanAutoUpdate != nil && *vr.CanAutoUpdate { - canUpdate := true - - var tlsConf *tlsConfigSettings - if runtime.GOOS != "windows" { - tlsConf = &tlsConfigSettings{} - Context.tls.WriteDiskConfig(tlsConf) - } - - if tlsConf != nil && - ((tlsConf.Enabled && (tlsConf.PortHTTPS < 1024 || - tlsConf.PortDNSOverTLS < 1024 || - tlsConf.PortDNSOverQUIC < 1024)) || - config.BindPort < 1024 || - config.DNS.Port < 1024) { - canUpdate, _ = aghnet.CanBindPrivilegedPorts() - } - vr.CanAutoUpdate = &canUpdate +// setAllowedToAutoUpdate sets CanAutoUpdate to true if AdGuard Home is actually +// allowed to perform an automatic update by the OS. +func (vr *versionResponse) setAllowedToAutoUpdate() (err error) { + if vr.CanAutoUpdate == nil || !*vr.CanAutoUpdate { + return } + + tlsConf := &tlsConfigSettings{} + Context.tls.WriteDiskConfig(tlsConf) + + canUpdate := true + if tlsConfUsesPrivilegedPorts(tlsConf) || config.BindPort < 1024 || config.DNS.Port < 1024 { + canUpdate, err = aghnet.CanBindPrivilegedPorts() + if err != nil { + return fmt.Errorf("checking ability to bind privileged ports: %w", err) + } + } + + vr.CanAutoUpdate = &canUpdate + + return nil +} + +// tlsConfUsesPrivilegedPorts returns true if the provided TLS configuration +// indicates that privileged ports are used. +func tlsConfUsesPrivilegedPorts(c *tlsConfigSettings) (ok bool) { + return c.Enabled && (c.PortHTTPS < 1024 || c.PortDNSOverTLS < 1024 || c.PortDNSOverQUIC < 1024) } // finishUpdate completes an update procedure. diff --git a/internal/updater/check.go b/internal/updater/check.go index edf046af..ec7176b2 100644 --- a/internal/updater/check.go +++ b/internal/updater/check.go @@ -17,11 +17,11 @@ const versionCheckPeriod = 8 * time.Hour // VersionInfo contains information about a new version. type VersionInfo struct { + CanAutoUpdate *bool `json:"can_autoupdate,omitempty"` NewVersion string `json:"new_version,omitempty"` Announcement string `json:"announcement,omitempty"` AnnouncementURL string `json:"announcement_url,omitempty"` SelfUpdateMinVersion string `json:"-"` - CanAutoUpdate *bool `json:"can_autoupdate,omitempty"` } // MaxResponseSize is responses on server's requests maximum length in bytes. diff --git a/scripts/make/go-lint.sh b/scripts/make/go-lint.sh index 32d99480..c363e513 100644 --- a/scripts/make/go-lint.sh +++ b/scripts/make/go-lint.sh @@ -136,7 +136,6 @@ underscores() { -e '_freebsd.go'\ -e '_linux.go'\ -e '_little.go'\ - -e '_nolinux.go'\ -e '_openbsd.go'\ -e '_others.go'\ -e '_test.go'\