From f68f6c9b37cea1f33cb6fa791e6678b9df3a237b Mon Sep 17 00:00:00 2001 From: Ainar Garipov Date: Mon, 15 Feb 2021 19:07:08 +0300 Subject: [PATCH] Pull request: all: fix statip ip ck Merge in DNS/adguard-home from fix-static-ip-check to master Squashed commit of the following: commit af365c106f3d620afc77492a06f5368611328f5f Author: Ainar Garipov Date: Mon Feb 15 18:55:35 2021 +0300 all: doc changes commit 922afb262458fc488e03cad232430d90c504f2f3 Merge: 43fec5fb dbcc55f5 Author: Ainar Garipov Date: Mon Feb 15 18:53:31 2021 +0300 Merge branch 'master' into fix-static-ip-check commit 43fec5fb79f5c67b375da00aa73d11b3ed9ba3a4 Author: Ainar Garipov Date: Mon Feb 15 18:37:16 2021 +0300 all: fix statip ip ck --- CHANGELOG.md | 2 ++ internal/dhcpd/http.go | 32 +++++++++++++++++++++++--------- internal/sysutil/net.go | 7 +++++++ internal/sysutil/net_linux.go | 17 +++++++++++++---- 4 files changed, 45 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 127971a1..39217416 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,8 @@ and this project adheres to ### Fixed +- Error when enabling the DHCP server when AdGuard Home couldn't determine if + the machine has a static IP. - Optical issue on custom rules ([#2641]). - Occasional crashes during startup. - The field `"range_start"` in the `GET /control/dhcp/status` HTTP API response diff --git a/internal/dhcpd/http.go b/internal/dhcpd/http.go index de155c5c..2dab1132 100644 --- a/internal/dhcpd/http.go +++ b/internal/dhcpd/http.go @@ -95,26 +95,40 @@ func (s *Server) enableDHCP(ifaceName string) (code int, err error) { var hasStaticIP bool hasStaticIP, err = sysutil.IfaceHasStaticIP(ifaceName) if err != nil { - // ErrPermission may happen here on Linux systems where AdGuard - // Home is installed using Snap. That doesn't necessarily mean - // that the machine doesn't have a static IP, so we can assume - // that it has and go on. If the machine doesn't, we'll get an - // error later. - // - // See https://github.com/AdguardTeam/AdGuardHome/issues/2667. if errors.Is(err, os.ErrPermission) { + // ErrPermission may happen here on Linux systems where + // AdGuard Home is installed using Snap. That doesn't + // necessarily mean that the machine doesn't have + // a static IP, so we can assume that it has and go on. + // If the machine doesn't, we'll get an error later. + // + // See https://github.com/AdguardTeam/AdGuardHome/issues/2667. + // + // TODO(a.garipov): I was thinking about moving this + // into IfaceHasStaticIP, but then we wouldn't be able + // to log it. Think about it more. log.Info("error while checking static ip: %s; "+ "assuming machine has static ip and going on", err) hasStaticIP = true + } else if errors.Is(err, sysutil.ErrNoStaticIPInfo) { + // Couldn't obtain a definitive answer. Assume static + // IP an go on. + log.Info("can't check for static ip; " + + "assuming machine has static ip and going on") + hasStaticIP = true } else { - return http.StatusInternalServerError, fmt.Errorf("checking static ip: %w", err) + err = fmt.Errorf("checking static ip: %w", err) + + return http.StatusInternalServerError, err } } if !hasStaticIP { err = sysutil.IfaceSetStaticIP(ifaceName) if err != nil { - return http.StatusInternalServerError, fmt.Errorf("setting static ip: %w", err) + err = fmt.Errorf("setting static ip: %w", err) + + return http.StatusInternalServerError, err } } diff --git a/internal/sysutil/net.go b/internal/sysutil/net.go index 0e3b448e..9ba41704 100644 --- a/internal/sysutil/net.go +++ b/internal/sysutil/net.go @@ -5,10 +5,17 @@ import ( "os/exec" "strings" + "github.com/AdguardTeam/AdGuardHome/internal/agherr" "github.com/AdguardTeam/golibs/log" ) +// ErrNoStaticIPInfo is returned by IfaceHasStaticIP when no information about +// the IP being static is available. +const ErrNoStaticIPInfo agherr.Error = "no information about static ip" + // IfaceHasStaticIP checks if interface is configured to have static IP address. +// If it can't give a definitive answer, it returns false and an error for which +// errors.Is(err, ErrNoStaticIPInfo) is true. func IfaceHasStaticIP(ifaceName string) (has bool, err error) { return ifaceHasStaticIP(ifaceName) } diff --git a/internal/sysutil/net_linux.go b/internal/sysutil/net_linux.go index 9205fd5c..1964f9dd 100644 --- a/internal/sysutil/net_linux.go +++ b/internal/sysutil/net_linux.go @@ -21,7 +21,11 @@ import ( const maxConfigFileSize = 1024 * 1024 func ifaceHasStaticIP(ifaceName string) (has bool, err error) { - var f *os.File + // TODO(a.garipov): Currently, this function returns the first + // definitive result. So if /etc/dhcpcd.conf has a static IP while + // /etc/network/interfaces doesn't, it will return true. Perhaps this + // is not the most desirable behavior. + for _, check := range []struct { checker func(io.Reader, string) (bool, error) filePath string @@ -32,8 +36,11 @@ func ifaceHasStaticIP(ifaceName string) (has bool, err error) { checker: ifacesStaticConfig, filePath: "/etc/network/interfaces", }} { + var f *os.File f, err = os.Open(check.filePath) if err != nil { + // ErrNotExist can happen here if there is no such file. + // This is normal, as not every system uses those files. if errors.Is(err, os.ErrNotExist) { err = nil @@ -52,12 +59,14 @@ func ifaceHasStaticIP(ifaceName string) (has bool, err error) { defer fileReadCloser.Close() has, err = check.checker(fileReadCloser, ifaceName) - if has || err != nil { - break + if err != nil { + return false, err } + + return has, nil } - return has, err + return false, ErrNoStaticIPInfo } // dhcpcdStaticConfig checks if interface is configured by /etc/dhcpcd.conf to