From 876563c4926c36159252bd75df96d89eaf6c6585 Mon Sep 17 00:00:00 2001 From: "Quentin McGaw (desktop)" Date: Sun, 30 May 2021 16:14:08 +0000 Subject: [PATCH] Maintenance: improve error wrapping --- .golangci.yml | 3 ++- cmd/gluetun/main.go | 8 ++++++-- internal/cli/update.go | 16 ++++++++++++---- internal/configuration/cyberghost.go | 10 +++++++--- internal/configuration/cyberghost_test.go | 9 ++++----- internal/dns/state.go | 7 +++++-- internal/firewall/ip6tables.go | 6 ++++-- internal/healthcheck/client.go | 7 ++++++- internal/httpproxy/state.go | 7 +++++-- internal/openvpn/command.go | 5 ++++- internal/openvpn/state.go | 7 +++++-- .../privateinternetaccess/portforward.go | 2 +- internal/publicip/publicip.go | 5 ++++- internal/publicip/state.go | 7 +++++-- internal/server/server.go | 3 ++- internal/server/wrappers.go | 8 +++++--- internal/shadowsocks/state.go | 7 +++++-- internal/updater/state.go | 7 +++++-- internal/version/version.go | 13 +++++++++---- 19 files changed, 96 insertions(+), 41 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 3bccedfb..5d89f435 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -10,6 +10,7 @@ issues: linters: - dupl - maligned + - goerr113 - path: internal/server/ linters: - dupl @@ -40,7 +41,7 @@ linters: - gomnd - goprintffuncname - gosec - # - goerr113 + - goerr113 - gosimple - govet - importas diff --git a/cmd/gluetun/main.go b/cmd/gluetun/main.go index 858b6845..0d357e49 100644 --- a/cmd/gluetun/main.go +++ b/cmd/gluetun/main.go @@ -106,6 +106,10 @@ func main() { nativeos.Exit(1) } +var ( + errCommandUnknown = errors.New("command is unknown") +) + //nolint:gocognit,gocyclo func _main(ctx context.Context, buildInfo models.BuildInformation, args []string, logger logging.ParentLogger, os os.OS, @@ -121,7 +125,7 @@ func _main(ctx context.Context, buildInfo models.BuildInformation, case "update": return cli.Update(ctx, args[2:], os, logger) default: - return fmt.Errorf("command %q is unknown", args[1]) + return fmt.Errorf("%w: %s", errCommandUnknown, args[1]) } } @@ -429,7 +433,7 @@ func routeReadyEvents(ctx context.Context, done chan<- struct{}, buildInfo model first = false message, err := versionpkg.GetMessage(ctx, buildInfo, httpClient) if err != nil { - logger.Error(err) + logger.Error("cannot get version information: " + err.Error()) } else { logger.Info(message) } diff --git a/internal/cli/update.go b/internal/cli/update.go index 13f5075d..ef9538a0 100644 --- a/internal/cli/update.go +++ b/internal/cli/update.go @@ -2,6 +2,7 @@ package cli import ( "context" + "errors" "flag" "fmt" "net/http" @@ -15,6 +16,13 @@ import ( "github.com/qdm12/golibs/os" ) +var ( + ErrNoFileOrStdoutFlag = errors.New("at least one of -file or -stdout must be specified") + ErrSyncServers = errors.New("cannot sync hardcoded and persisted servers") + ErrUpdateServerInformation = errors.New("cannot update server information") + ErrWriteToFile = errors.New("cannot write updated information to file") +) + func (c *cli) Update(ctx context.Context, args []string, os os.OS, logger logging.Logger) error { options := configuration.Updater{CLI: true} var flushToFile bool @@ -40,7 +48,7 @@ func (c *cli) Update(ctx context.Context, args []string, os os.OS, logger loggin return err } if !flushToFile && !options.Stdout { - return fmt.Errorf("at least one of -file or -stdout must be specified") + return ErrNoFileOrStdoutFlag } const clientTimeout = 10 * time.Second @@ -48,16 +56,16 @@ func (c *cli) Update(ctx context.Context, args []string, os os.OS, logger loggin storage := storage.New(logger, os, constants.ServersData) currentServers, err := storage.SyncServers(constants.GetAllServers()) if err != nil { - return fmt.Errorf("cannot update servers: %w", err) + return fmt.Errorf("%w: %s", ErrSyncServers, err) } updater := updater.New(options, httpClient, currentServers, logger) allServers, err := updater.UpdateServers(ctx) if err != nil { - return err + return fmt.Errorf("%w: %s", ErrUpdateServerInformation, err) } if flushToFile { if err := storage.FlushToFile(allServers); err != nil { - return fmt.Errorf("cannot update servers: %w", err) + return fmt.Errorf("%w: %s", ErrWriteToFile, err) } } diff --git a/internal/configuration/cyberghost.go b/internal/configuration/cyberghost.go index 567a53aa..8ae322ba 100644 --- a/internal/configuration/cyberghost.go +++ b/internal/configuration/cyberghost.go @@ -2,7 +2,7 @@ package configuration import ( "encoding/pem" - "fmt" + "errors" "strings" "github.com/qdm12/gluetun/internal/constants" @@ -81,10 +81,12 @@ func readCyberghostClientKey(r reader) (clientKey string, err error) { return extractClientKey(b) } +var errDecodePEMBlockClientKey = errors.New("cannot decode PEM block from client key") + func extractClientKey(b []byte) (key string, err error) { pemBlock, _ := pem.Decode(b) if pemBlock == nil { - return "", fmt.Errorf("cannot decode PEM block from client key") + return "", errDecodePEMBlockClientKey } parsedBytes := pem.EncodeToMemory(pemBlock) s := string(parsedBytes) @@ -102,10 +104,12 @@ func readCyberghostClientCertificate(r reader) (clientCertificate string, err er return extractClientCertificate(b) } +var errDecodePEMBlockClientCert = errors.New("cannot decode PEM block from client certificate") + func extractClientCertificate(b []byte) (certificate string, err error) { pemBlock, _ := pem.Decode(b) if pemBlock == nil { - return "", fmt.Errorf("cannot decode PEM block from client certificate") + return "", errDecodePEMBlockClientCert } parsedBytes := pem.EncodeToMemory(pemBlock) s := string(parsedBytes) diff --git a/internal/configuration/cyberghost_test.go b/internal/configuration/cyberghost_test.go index 54476c9f..e480f013 100644 --- a/internal/configuration/cyberghost_test.go +++ b/internal/configuration/cyberghost_test.go @@ -1,7 +1,6 @@ package configuration import ( - "fmt" "testing" "github.com/stretchr/testify/assert" @@ -71,11 +70,11 @@ zZjrL52saevO25cigVl+hxcnY8DTpbk= err error }{ "no input": { - err: fmt.Errorf("cannot decode PEM block from client key"), + err: errDecodePEMBlockClientKey, }, "bad input": { b: []byte{1, 2, 3}, - err: fmt.Errorf("cannot decode PEM block from client key"), + err: errDecodePEMBlockClientKey, }, "valid key": { b: []byte(validPEM), @@ -147,11 +146,11 @@ iOCYTbretAFZRhh6ycUN5hBeN8GMQxiMreMtDV4PEIQ= err error }{ "no input": { - err: fmt.Errorf("cannot decode PEM block from client certificate"), + err: errDecodePEMBlockClientCert, }, "bad input": { b: []byte{1, 2, 3}, - err: fmt.Errorf("cannot decode PEM block from client certificate"), + err: errDecodePEMBlockClientCert, }, "valid key": { b: []byte(validPEM), diff --git a/internal/dns/state.go b/internal/dns/state.go index a578b6c8..ec7d1c4e 100644 --- a/internal/dns/state.go +++ b/internal/dns/state.go @@ -1,6 +1,7 @@ package dns import ( + "errors" "fmt" "reflect" "sync" @@ -29,6 +30,8 @@ func (l *looper) GetStatus() (status models.LoopStatus) { return l.state.status } +var ErrInvalidStatus = errors.New("invalid status") + func (l *looper) SetStatus(status models.LoopStatus) (outcome string, err error) { l.state.statusMu.Lock() defer l.state.statusMu.Unlock() @@ -64,8 +67,8 @@ func (l *looper) SetStatus(status models.LoopStatus) (outcome string, err error) l.state.status = constants.Stopped return status.String(), nil default: - return "", fmt.Errorf("status %q can only be %q or %q", - status, constants.Running, constants.Stopped) + return "", fmt.Errorf("%w: %s: it can only be one of: %s, %s", + ErrInvalidStatus, status, constants.Running, constants.Stopped) } } diff --git a/internal/firewall/ip6tables.go b/internal/firewall/ip6tables.go index 01c0a42c..5ef8ed14 100644 --- a/internal/firewall/ip6tables.go +++ b/internal/firewall/ip6tables.go @@ -41,16 +41,18 @@ func (c *configurator) runIP6tablesInstruction(ctx context.Context, instruction } flags := strings.Fields(instruction) if output, err := c.commander.Run(ctx, "ip6tables", flags...); err != nil { - return fmt.Errorf("%w \"ip6tables %s\": %s: %s", ErrIP6Tables, instruction, output, err) + return fmt.Errorf("%w: \"ip6tables %s\": %s: %s", ErrIP6Tables, instruction, output, err) } return nil } +var errPolicyNotValid = errors.New("policy is not valid") + func (c *configurator) setIPv6AllPolicies(ctx context.Context, policy string) error { switch policy { case "ACCEPT", "DROP": default: - return fmt.Errorf("policy %q not recognized", policy) + return fmt.Errorf("%w: %s", errPolicyNotValid, policy) } return c.runIP6tablesInstructions(ctx, []string{ "--policy INPUT " + policy, diff --git a/internal/healthcheck/client.go b/internal/healthcheck/client.go index 1904a8eb..b2b0f84c 100644 --- a/internal/healthcheck/client.go +++ b/internal/healthcheck/client.go @@ -2,11 +2,16 @@ package healthcheck import ( "context" + "errors" "fmt" "io" "net/http" ) +var ( + ErrHTTPStatusNotOK = errors.New("HTTP response status is not OK") +) + type Checker interface { Check(ctx context.Context, url string) error } @@ -38,5 +43,5 @@ func (h *checker) Check(ctx context.Context, url string) error { if err != nil { return err } - return fmt.Errorf("%s: %s", response.Status, string(b)) + return fmt.Errorf("%w: %s: %s", ErrHTTPStatusNotOK, response.Status, string(b)) } diff --git a/internal/httpproxy/state.go b/internal/httpproxy/state.go index 6f3bc3d4..83d29677 100644 --- a/internal/httpproxy/state.go +++ b/internal/httpproxy/state.go @@ -1,6 +1,7 @@ package httpproxy import ( + "errors" "fmt" "reflect" "sync" @@ -29,6 +30,8 @@ func (l *looper) GetStatus() (status models.LoopStatus) { return l.state.status } +var ErrInvalidStatus = errors.New("invalid status") + func (l *looper) SetStatus(status models.LoopStatus) (outcome string, err error) { l.state.statusMu.Lock() defer l.state.statusMu.Unlock() @@ -64,8 +67,8 @@ func (l *looper) SetStatus(status models.LoopStatus) (outcome string, err error) l.state.status = status return status.String(), nil default: - return "", fmt.Errorf("status %q can only be %q or %q", - status, constants.Running, constants.Stopped) + return "", fmt.Errorf("%w: %s: it can only be one of: %s, %s", + ErrInvalidStatus, status, constants.Running, constants.Stopped) } } diff --git a/internal/openvpn/command.go b/internal/openvpn/command.go index bc639b63..49cd2ae2 100644 --- a/internal/openvpn/command.go +++ b/internal/openvpn/command.go @@ -2,6 +2,7 @@ package openvpn import ( "context" + "errors" "fmt" "strings" @@ -14,6 +15,8 @@ func (c *configurator) Start(ctx context.Context) ( return c.commander.Start(ctx, "openvpn", "--config", constants.OpenVPNConf) } +var ErrVersionTooShort = errors.New("version output is too short") + func (c *configurator) Version(ctx context.Context) (string, error) { output, err := c.commander.Run(ctx, "openvpn", "--version") if err != nil && err.Error() != "exit status 1" { @@ -23,7 +26,7 @@ func (c *configurator) Version(ctx context.Context) (string, error) { words := strings.Fields(firstLine) const minWords = 2 if len(words) < minWords { - return "", fmt.Errorf("openvpn --version: first line is too short: %q", firstLine) + return "", fmt.Errorf("%w: %s", ErrVersionTooShort, firstLine) } return words[1], nil } diff --git a/internal/openvpn/state.go b/internal/openvpn/state.go index ae85f1de..86379d54 100644 --- a/internal/openvpn/state.go +++ b/internal/openvpn/state.go @@ -1,6 +1,7 @@ package openvpn import ( + "errors" "fmt" "reflect" "sync" @@ -43,6 +44,8 @@ func (l *looper) GetStatus() (status models.LoopStatus) { return l.state.status } +var ErrInvalidStatus = errors.New("invalid status") + func (l *looper) SetStatus(status models.LoopStatus) (outcome string, err error) { l.state.statusMu.Lock() defer l.state.statusMu.Unlock() @@ -78,8 +81,8 @@ func (l *looper) SetStatus(status models.LoopStatus) (outcome string, err error) l.state.status = constants.Stopped return status.String(), nil default: - return "", fmt.Errorf("status %q can only be %q or %q", - status, constants.Running, constants.Stopped) + return "", fmt.Errorf("%w: %s: it can only be one of: %s, %s", + ErrInvalidStatus, status, constants.Running, constants.Stopped) } } diff --git a/internal/provider/privateinternetaccess/portforward.go b/internal/provider/privateinternetaccess/portforward.go index 180e6b80..52ba7dca 100644 --- a/internal/provider/privateinternetaccess/portforward.go +++ b/internal/provider/privateinternetaccess/portforward.go @@ -478,7 +478,7 @@ func writePortForwardedToFile(openFile os.OpenFileFunc, // replaceInErr is used to remove sensitive information from errors. func replaceInErr(err error, substitutions map[string]string) error { s := replaceInString(err.Error(), substitutions) - return errors.New(s) + return errors.New(s) //nolint:goerr113 } // replaceInString is used to remove sensitive information. diff --git a/internal/publicip/publicip.go b/internal/publicip/publicip.go index 8859dadb..f4b766a5 100644 --- a/internal/publicip/publicip.go +++ b/internal/publicip/publicip.go @@ -3,6 +3,7 @@ package publicip import ( "context" + "errors" "fmt" "io" "math/rand" @@ -27,6 +28,8 @@ func NewIPGetter(client *http.Client) IPGetter { } } +var ErrParseIP = errors.New("cannot parse IP address") + func (i *ipGetter) Get(ctx context.Context) (ip net.IP, err error) { urls := []string{ "https://ifconfig.me/ip", @@ -63,7 +66,7 @@ func (i *ipGetter) Get(ctx context.Context) (ip net.IP, err error) { s := strings.ReplaceAll(string(content), "\n", "") ip = net.ParseIP(s) if ip == nil { - return nil, fmt.Errorf("cannot parse IP address from %q", s) + return nil, fmt.Errorf("%w: %s", ErrParseIP, s) } return ip, nil } diff --git a/internal/publicip/state.go b/internal/publicip/state.go index cb781416..5f705e6e 100644 --- a/internal/publicip/state.go +++ b/internal/publicip/state.go @@ -1,6 +1,7 @@ package publicip import ( + "errors" "fmt" "net" "reflect" @@ -32,6 +33,8 @@ func (l *looper) GetStatus() (status models.LoopStatus) { return l.state.status } +var ErrInvalidStatus = errors.New("invalid status") + func (l *looper) SetStatus(status models.LoopStatus) (outcome string, err error) { l.state.statusMu.Lock() defer l.state.statusMu.Unlock() @@ -67,8 +70,8 @@ func (l *looper) SetStatus(status models.LoopStatus) (outcome string, err error) l.state.status = status return status.String(), nil default: - return "", fmt.Errorf("status %q can only be %q or %q", - status, constants.Running, constants.Stopped) + return "", fmt.Errorf("%w: %s: it can only be one of: %s, %s", + ErrInvalidStatus, status, constants.Running, constants.Stopped) } } diff --git a/internal/server/server.go b/internal/server/server.go index f3c8d383..ba6b393b 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -3,6 +3,7 @@ package server import ( "context" + "errors" "net/http" "time" @@ -51,7 +52,7 @@ func (s *server) Run(ctx context.Context, done chan<- struct{}) { }() s.logger.Info("listening on %s", s.address) err := server.ListenAndServe() - if err != nil && ctx.Err() != context.Canceled { + if err != nil && errors.Is(ctx.Err(), context.Canceled) { s.logger.Error(err) } } diff --git a/internal/server/wrappers.go b/internal/server/wrappers.go index 2b1f1eaa..26081c9a 100644 --- a/internal/server/wrappers.go +++ b/internal/server/wrappers.go @@ -1,6 +1,7 @@ package server import ( + "errors" "fmt" "github.com/qdm12/gluetun/internal/constants" @@ -11,15 +12,16 @@ type statusWrapper struct { Status string `json:"status"` } +var errInvalidStatus = errors.New("invalid status") + func (sw *statusWrapper) getStatus() (status models.LoopStatus, err error) { status = models.LoopStatus(sw.Status) switch status { case constants.Stopped, constants.Running: return status, nil default: - return "", fmt.Errorf( - "invalid status %q: possible values are: %s, %s", - sw.Status, constants.Stopped, constants.Running) + return "", fmt.Errorf("%w: %s: possible values are: %s, %s", + errInvalidStatus, sw.Status, constants.Stopped, constants.Running) } } diff --git a/internal/shadowsocks/state.go b/internal/shadowsocks/state.go index a91bc616..9871b717 100644 --- a/internal/shadowsocks/state.go +++ b/internal/shadowsocks/state.go @@ -1,6 +1,7 @@ package shadowsocks import ( + "errors" "fmt" "reflect" "sync" @@ -29,6 +30,8 @@ func (l *looper) GetStatus() (status models.LoopStatus) { return l.state.status } +var ErrInvalidStatus = errors.New("invalid status") + func (l *looper) SetStatus(status models.LoopStatus) (outcome string, err error) { l.state.statusMu.Lock() defer l.state.statusMu.Unlock() @@ -64,8 +67,8 @@ func (l *looper) SetStatus(status models.LoopStatus) (outcome string, err error) l.state.status = status return status.String(), nil default: - return "", fmt.Errorf("status %q can only be %q or %q", - status, constants.Running, constants.Stopped) + return "", fmt.Errorf("%w: %s: it can only be one of: %s, %s", + ErrInvalidStatus, status, constants.Running, constants.Stopped) } } diff --git a/internal/updater/state.go b/internal/updater/state.go index 79b5abc4..71cd99f7 100644 --- a/internal/updater/state.go +++ b/internal/updater/state.go @@ -1,6 +1,7 @@ package updater import ( + "errors" "fmt" "reflect" "sync" @@ -29,6 +30,8 @@ func (l *looper) GetStatus() (status models.LoopStatus) { return l.state.status } +var ErrInvalidStatus = errors.New("invalid status") + func (l *looper) SetStatus(status models.LoopStatus) (outcome string, err error) { l.state.statusMu.Lock() defer l.state.statusMu.Unlock() @@ -64,8 +67,8 @@ func (l *looper) SetStatus(status models.LoopStatus) (outcome string, err error) l.state.status = status return status.String(), nil default: - return "", fmt.Errorf("status %q can only be %q or %q", - status, constants.Running, constants.Stopped) + return "", fmt.Errorf("%w: %s: it can only be one of: %s, %s", + ErrInvalidStatus, status, constants.Running, constants.Stopped) } } diff --git a/internal/version/version.go b/internal/version/version.go index 8885c89e..bd1e931e 100644 --- a/internal/version/version.go +++ b/internal/version/version.go @@ -4,6 +4,7 @@ package version import ( "context" + "errors" "fmt" "net/http" "time" @@ -20,7 +21,7 @@ func GetMessage(ctx context.Context, buildInfo models.BuildInformation, // Find # of commits between current commit and latest commit commitsSince, err := getCommitsSince(ctx, client, buildInfo.Commit) if err != nil { - return "", fmt.Errorf("cannot get version information: %w", err) + return "", err } else if commitsSince == 0 { return fmt.Sprintf("You are running on the bleeding edge of %s!", buildInfo.Version), nil } @@ -32,7 +33,7 @@ func GetMessage(ctx context.Context, buildInfo models.BuildInformation, } tagName, name, releaseTime, err := getLatestRelease(ctx, client) if err != nil { - return "", fmt.Errorf("cannot get version information: %w", err) + return "", err } if tagName == buildInfo.Version { return fmt.Sprintf("You are running the latest release %s", buildInfo.Version), nil @@ -43,6 +44,8 @@ func GetMessage(ctx context.Context, buildInfo models.BuildInformation, nil } +var errReleaseNotFound = errors.New("release not found") + func getLatestRelease(ctx context.Context, client *http.Client) (tagName, name string, time time.Time, err error) { releases, err := getGithubReleases(ctx, client) if err != nil { @@ -54,9 +57,11 @@ func getLatestRelease(ctx context.Context, client *http.Client) (tagName, name s } return release.TagName, release.Name, release.PublishedAt, nil } - return "", "", time, fmt.Errorf("no releases found") + return "", "", time, errReleaseNotFound } +var errCommitNotFound = errors.New("commit not found") + func getCommitsSince(ctx context.Context, client *http.Client, commitShort string) (n int, err error) { commits, err := getGithubCommits(ctx, client) if err != nil { @@ -68,5 +73,5 @@ func getCommitsSince(ctx context.Context, client *http.Client, commitShort strin } n++ } - return 0, fmt.Errorf("no commit matching %q was found", commitShort) + return 0, fmt.Errorf("%w: %s", errCommitNotFound, commitShort) }