Lint: Resolve linter errors, improve code

.golangci.yml

@@ -0,0 +1,174 @@
+# Derived from https://github.com/golangci/golangci-lint/blob/master/.golangci.example.yml
+
+---
+
+run:
+  # timeout for analysis, e.g. 30s, 5m, default is 1m
+  timeout: 5m
+  # Force readonly modules usage for checking
+  modules-download-mode: readonly
+
+output:
+  formats:
+    - format: tab
+      path: stdout
+
+issues:
+  # This disables the included exclude-list in golangci-lint as that
+  # list for example fully hides G304 gosec rule, errcheck, exported
+  # rule of revive and other errors one really wants to see.
+  # Smme detail: https://github.com/golangci/golangci-lint/issues/456
+  exclude-use-default: false
+  # Don't limit the number of shown issues: Report ALL of them
+  max-issues-per-linter: 0
+  max-same-issues: 0
+
+linters:
+  disable-all: true
+  enable:
+    - asciicheck    # Simple linter to check that your code does not contain non-ASCII identifiers [fast: true, auto-fix: false]
+    - bidichk       # Checks for dangerous unicode character sequences [fast: true, auto-fix: false]
+    - bodyclose     # checks whether HTTP response body is closed successfully [fast: true, auto-fix: false]
+    - containedctx  # containedctx is a linter that detects struct contained context.Context field [fast: true, auto-fix: false]
+    - contextcheck  # check the function whether use a non-inherited context [fast: false, auto-fix: false]
+    - dogsled       # Checks assignments with too many blank identifiers (e.g. x, _, _, _, := f()) [fast: true, auto-fix: false]
+    - durationcheck # check for two durations multiplied together [fast: false, auto-fix: false]
+    - errcheck      # Errcheck is a program for checking for unchecked errors in go programs. These unchecked errors can be critical bugs in some cases [fast: false, auto-fix: false]
+    - errchkjson    # Checks types passed to the json encoding functions. Reports unsupported types and optionally reports occations, where the check for the returned error can be omitted. [fast: false, auto-fix: false]
+    - exportloopref # checks for pointers to enclosing loop variables [fast: true, auto-fix: false]
+    - forbidigo     # Forbids identifiers [fast: true, auto-fix: false]
+    - funlen        # Tool for detection of long functions [fast: true, auto-fix: false]
+    - gocognit      # Computes and checks the cognitive complexity of functions [fast: true, auto-fix: false]
+    - goconst       # Finds repeated strings that could be replaced by a constant [fast: true, auto-fix: false]
+    - gocritic      # The most opinionated Go source code linter [fast: true, auto-fix: false]
+    - gocyclo       # Computes and checks the cyclomatic complexity of functions [fast: true, auto-fix: false]
+    - godox         # Tool for detection of FIXME, TODO and other comment keywords [fast: true, auto-fix: false]
+    - gofmt         # Gofmt checks whether code was gofmt-ed. By default this tool runs with -s option to check for code simplification [fast: true, auto-fix: true]
+    - gofumpt       # Gofumpt checks whether code was gofumpt-ed. [fast: true, auto-fix: true]
+    - goimports     # Goimports does everything that gofmt does. Additionally it checks unused imports [fast: true, auto-fix: true]
+    - gosec         # Inspects source code for security problems [fast: true, auto-fix: false]
+    - gosimple      # Linter for Go source code that specializes in simplifying a code [fast: true, auto-fix: false]
+    - govet         # Vet examines Go source code and reports suspicious constructs, such as Printf calls whose arguments do not align with the format string [fast: true, auto-fix: false]
+    - ineffassign   # Detects when assignments to existing variables are not used [fast: true, auto-fix: false]
+    - misspell      # Finds commonly misspelled English words in comments [fast: true, auto-fix: true]
+    - mnd           # An analyzer to detect magic numbers. [fast: true, auto-fix: false]
+    - nakedret      # Finds naked returns in functions greater than a specified function length [fast: true, auto-fix: false]
+    - nilerr        # Finds the code that returns nil even if it checks that the error is not nil. [fast: false, auto-fix: false]
+    - nilnil        # Checks that there is no simultaneous return of `nil` error and an invalid value. [fast: false, auto-fix: false]
+    - noctx         # noctx finds sending http request without context.Context [fast: true, auto-fix: false]
+    - nolintlint    # Reports ill-formed or insufficient nolint directives [fast: true, auto-fix: false]
+    - revive        # Fast, configurable, extensible, flexible, and beautiful linter for Go. Drop-in replacement of golint. [fast: false, auto-fix: false]
+    - staticcheck   # Staticcheck is a go vet on steroids, applying a ton of static analysis checks [fast: true, auto-fix: false]
+    - stylecheck    # Stylecheck is a replacement for golint [fast: true, auto-fix: false]
+    - tenv          # tenv is analyzer that detects using os.Setenv instead of t.Setenv since Go1.17 [fast: false, auto-fix: false]
+    - typecheck     # Like the front-end of a Go compiler, parses and type-checks Go code [fast: true, auto-fix: false]
+    - unconvert     # Remove unnecessary type conversions [fast: true, auto-fix: false]
+    - unused        # Checks Go code for unused constants, variables, functions and types [fast: false, auto-fix: false]
+    - wastedassign  # wastedassign finds wasted assignment statements. [fast: false, auto-fix: false]
+    - wrapcheck     # Checks that errors returned from external packages are wrapped [fast: false, auto-fix: false]
+
+linters-settings:
+  funlen:
+    lines: 100
+    statements: 60
+
+  gocyclo:
+    # minimal code complexity to report, 30 by default (but we recommend 10-20)
+    min-complexity: 15
+
+  gomnd:
+    ignored-functions: 'strconv.(?:Format|Parse)\B+'
+
+  revive:
+    rules:
+      #- name: add-constant           # Suggests using constant for magic numbers and string literals
+                                      # Opinion: Makes sense for strings, not for numbers but checks numbers
+      #- name: argument-limit         # Specifies the maximum number of arguments a function can receive | Opinion: Don't need this
+      - name: atomic                  # Check for common mistaken usages of the `sync/atomic` package
+      - name: banned-characters       # Checks banned characters in identifiers
+        arguments:
+          - ';' # Greek question mark
+      - name: bare-return             # Warns on bare returns
+      - name: blank-imports           # Disallows blank imports
+      - name: bool-literal-in-expr    # Suggests removing Boolean literals from logic expressions
+      - name: call-to-gc              # Warns on explicit call to the garbage collector
+      #- name: cognitive-complexity   # Sets restriction for maximum Cognitive complexity.
+                                      # There is a dedicated linter for this
+      - name: confusing-naming        # Warns on methods with names that differ only by capitalization
+      - name: confusing-results       # Suggests to name potentially confusing function results
+      - name: constant-logical-expr   # Warns on constant logical expressions
+      - name: context-as-argument     # `context.Context` should be the first argument of a function.
+      - name: context-keys-type       # Disallows the usage of basic types in `context.WithValue`.
+      #- name: cyclomatic             # Sets restriction for maximum Cyclomatic complexity.
+                                      # There is a dedicated linter for this
+      #- name: datarace               # Spots potential dataraces
+                                      # Is not (yet) available?
+      - name: deep-exit               # Looks for program exits in funcs other than `main()` or `init()`
+      - name: defer                   # Warns on some [defer gotchas](https://blog.learngoprogramming.com/5-gotchas-of-defer-in-go-golang-part-iii-36a1ab3d6ef1)
+      - name: dot-imports             # Forbids `.` imports.
+      - name: duplicated-imports      # Looks for packages that are imported two or more times
+      - name: early-return            # Spots if-then-else statements that can be refactored to simplify code reading
+      - name: empty-block             # Warns on empty code blocks
+      - name: empty-lines             # Warns when there are heading or trailing newlines in a block
+      - name: errorf                  # Should replace `errors.New(fmt.Sprintf())` with `fmt.Errorf()`
+      - name: error-naming            # Naming of error variables.
+      - name: error-return            # The error return parameter should be last.
+      - name: error-strings           # Conventions around error strings.
+      - name: exported                # Naming and commenting conventions on exported symbols.
+        arguments: ['sayRepetitiveInsteadOfStutters']
+      #- name: file-header            # Header which each file should have.
+                                      # Useless without config, have no config for it
+      - name: flag-parameter          # Warns on boolean parameters that create a control coupling
+      #- name: function-length        # Warns on functions exceeding the statements or lines max
+                                      # There is a dedicated linter for this
+      #- name: function-result-limit  # Specifies the maximum number of results a function can return
+                                      # Opinion: Don't need this
+      - name: get-return              # Warns on getters that do not yield any result
+      - name: identical-branches      # Spots if-then-else statements with identical `then` and `else` branches
+      - name: if-return               # Redundant if when returning an error.
+      #- name: imports-blacklist      # Disallows importing the specified packages
+                                      # Useless without config, have no config for it
+      - name: import-shadowing        # Spots identifiers that shadow an import
+      - name: increment-decrement     # Use `i++` and `i--` instead of `i += 1` and `i -= 1`.
+      - name: indent-error-flow       # Prevents redundant else statements.
+      #- name: line-length-limit      # Specifies the maximum number of characters in a lined
+                                      # There is a dedicated linter for this
+      #- name: max-public-structs     # The maximum number of public structs in a file.
+                                      # Opinion: Don't need this
+      - name: modifies-parameter      # Warns on assignments to function parameters
+      - name: modifies-value-receiver # Warns on assignments to value-passed method receivers
+      #- name: nested-structs         # Warns on structs within structs
+                                      # Opinion: Don't need this
+      - name: optimize-operands-order # Checks inefficient conditional expressions
+      #- name: package-comments       # Package commenting conventions.
+                                      # Opinion: Don't need this
+      - name: range                   # Prevents redundant variables when iterating over a collection.
+      - name: range-val-address       # Warns if address of range value is used dangerously
+      - name: range-val-in-closure    # Warns if range value is used in a closure dispatched as goroutine
+      - name: receiver-naming         # Conventions around the naming of receivers.
+      - name: redefines-builtin-id    # Warns on redefinitions of builtin identifiers
+      #- name: string-format          # Warns on specific string literals that fail one or more user-configured regular expressions
+                                      # Useless without config, have no config for it
+      - name: string-of-int           # Warns on suspicious casts from int to string
+      - name: struct-tag              # Checks common struct tags like `json`,`xml`,`yaml`
+      - name: superfluous-else        # Prevents redundant else statements (extends indent-error-flow)
+      - name: time-equal              # Suggests to use `time.Time.Equal` instead of `==` and `!=` for equality check time.
+      - name: time-naming             # Conventions around the naming of time variables.
+      - name: unconditional-recursion # Warns on function calls that will lead to (direct) infinite recursion
+      - name: unexported-naming       # Warns on wrongly named un-exported symbols
+      - name: unexported-return       # Warns when a public return is from unexported type.
+      - name: unhandled-error         # Warns on unhandled errors returned by funcion calls
+        arguments:
+          - "fmt.(Fp|P)rint(f|ln|)"
+      - name: unnecessary-stmt        # Suggests removing or simplifying unnecessary statements
+      - name: unreachable-code        # Warns on unreachable code
+      - name: unused-parameter        # Suggests to rename or remove unused function parameters
+      - name: unused-receiver         # Suggests to rename or remove unused method receivers
+      #- name: use-any                # Proposes to replace `interface{}` with its alias `any`
+                                      # Is not (yet) available?
+      - name: useless-break           # Warns on useless `break` statements in case clauses
+      - name: var-declaration         # Reduces redundancies around variable declaration.
+      - name: var-naming              # Naming rules.
+      - name: waitgroup-by-value      # Warns on functions taking sync.WaitGroup as a by-value parameter
+
+...

main.go

@@ -2,27 +2,23 @@ package main
 
 import (
 	"bytes"
+	"context"
 	"fmt"
 	"io"
-	"io/ioutil"
 	"net/http"
 	"os"
 	"path/filepath"
 	"regexp"
-	"strings"
-	"sync"
-	"text/template"
 	"time"
 
-	"github.com/montanaflynn/stats"
-	log "github.com/sirupsen/logrus"
+	"github.com/sirupsen/logrus"
 
 	"github.com/Luzifer/rconfig/v2"
 )
 
 const (
-	dateFormat             = time.RFC1123
-	numHistoricalDurations = 300
+	cleanupInterval = 10 * time.Second
+	logFolderPerms  = 0o750
 )
 
 var (
@@ -30,6 +26,7 @@ var (
 		DisableLog     bool          `flag:"no-log" default:"false" description:"Disable response body logging"`
 		Interval       time.Duration `flag:"interval,i" default:"1s" description:"Check interval"`
 		LogDir         string        `flag:"log-dir,l" default:"/tmp/resp-log/" description:"Directory to log non-matched requests to"`
+		LogLevel       string        `flag:"log-level" default:"info" description:"Log level (debug, info, warn, error, fatal)"`
 		LogRetention   time.Duration `flag:"log-retention" default:"24h" description:"When to clean up file from log-dir"`
 		Match          string        `flag:"match,m" default:".*" description:"RegExp to match the response body against to validate it"`
 		Timeout        time.Duration `flag:"timeout,t" default:"30s" description:"Timeout for the request"`
@@ -40,120 +37,35 @@ var (
 	version = "dev"
 )
 
-type checkStatus uint
-
-func (c checkStatus) String() string {
-	return map[checkStatus]string{
-		statusUnknown: "UNKN",
-		statusFailed:  "FAIL",
-		statusOk:      "OKAY",
-	}[c]
-}
-
-const (
-	statusUnknown checkStatus = iota
-	statusOk
-	statusFailed
-)
-
-type checkResult struct {
-	DumpFile  string
-	Durations *ringDuration
-	Message   string
-	Start     time.Time
-	Status    checkStatus
-
-	lock        sync.RWMutex
-	lastLineLen int
-}
-
-func newCheckResult(status checkStatus, message string, duration time.Duration) *checkResult {
-	r := newRingDuration(numHistoricalDurations)
-	r.SetNext(duration)
-
-	return &checkResult{
-		Durations: r,
-		Message:   message,
-		Start:     time.Now(),
-		Status:    status,
-	}
-}
-
-func (c *checkResult) AddDuration(d time.Duration) {
-	c.lock.Lock()
-	defer c.lock.Unlock()
-
-	c.Durations.SetNext(d)
-}
-
-func (c *checkResult) DurationStats() string {
-	c.lock.RLock()
-	defer c.lock.RUnlock()
-
-	var (
-		s             = stats.LoadRawData(c.Durations.GetAll())
-		min, avg, max float64
-		err           error
-	)
-	if min, err = s.Min(); err != nil {
-		min = 0
-	}
-	if avg, err = s.Median(); err != nil {
-		avg = 0
-	}
-	if max, err = s.Max(); err != nil {
-		max = 0
-	}
-
-	return fmt.Sprintf("%s/%s/%s",
-		time.Duration(min).Round(time.Microsecond).String(),
-		time.Duration(avg).Round(time.Microsecond).String(),
-		time.Duration(max).Round(time.Microsecond).String(),
-	)
-}
-
-func (c *checkResult) Equals(r *checkResult) bool {
-	return c.Status == r.Status && c.Message == r.Message
-}
-
-func (c *checkResult) Print() {
-	tpl := strings.Join([]string{
-		`[{{ .Start.Format "` + dateFormat + `" }}]`,
-		`({{ .Status }})`,
-		`{{ .Message }}`,
-		`({{ .DurationStats }})`,
-		`{{ if ne .DumpFile "" }}(Resp: {{ .DumpFile }}){{ end }}`,
-	}, " ")
-	templ := template.Must(template.New("result").Parse(tpl))
-
-	buf := new(bytes.Buffer)
-	templ.Execute(buf, c)
-
-	if c.lastLineLen > 0 {
-		fmt.Fprintf(os.Stdout, "\r%s\r", strings.Repeat(" ", c.lastLineLen))
-	}
-
-	c.lastLineLen = buf.Len()
-	buf.WriteTo(os.Stdout)
-}
-
-func init() {
+func initApp() error {
 	rconfig.AutoEnv(true)
 	if err := rconfig.ParseAndValidate(&cfg); err != nil {
-		log.Fatalf("Unable to parse commandline options: %s", err)
+		return fmt.Errorf("parsing cli options: %w", err)
 	}
 
-	if cfg.VersionAndExit {
-		fmt.Printf("webcheck %s\n", version)
-		os.Exit(0)
+	l, err := logrus.ParseLevel(cfg.LogLevel)
+	if err != nil {
+		return fmt.Errorf("parsing log-level: %w", err)
 	}
+	logrus.SetLevel(l)
+
+	return nil
 }
 
 func main() {
-	http.DefaultClient.Timeout = cfg.Timeout
+	var err error
+	if err = initApp(); err != nil {
+		logrus.WithError(err).Fatal("initializing app")
+	}
+
+	if cfg.VersionAndExit {
+		logrus.WithField("version", version).Info("webcheck")
+		os.Exit(0)
+	}
+
 	matcher, err := regexp.Compile(cfg.Match)
 	if err != nil {
-		log.WithError(err).Fatal("Matcher regexp does not compile")
+		logrus.WithError(err).Fatal("compiling matcher RegExp")
 	}
 
 	lastResult := newCheckResult(statusUnknown, "Uninitialized", 0)
@@ -173,13 +85,13 @@ func main() {
 		result = doCheck(cfg.URL, matcher, body)
 
 		if !result.Equals(lastResult) {
-			fmt.Println()
+			fmt.Println() //nolint:forbidigo
 			lastResult = result
 
 			if result.Status == statusFailed {
 				fn, err := dumpRequest(body)
 				if err != nil {
-					log.WithError(err).Fatal("Could not dump request")
+					logrus.WithError(err).Fatal("logging request")
 				}
 				lastResult.DumpFile = fn
 			}
@@ -187,12 +99,14 @@ func main() {
 			lastResult.AddDuration(result.Durations.GetCurrent())
 		}
 
-		lastResult.Print()
+		if err = lastResult.Print(); err != nil {
+			logrus.WithError(err).Fatal("displaying status")
+		}
 	}
 }
 
 func cleanupLogFiles() {
-	for range time.Tick(10 * time.Second) {
+	for range time.Tick(cleanupInterval) {
 		if info, err := os.Stat(cfg.LogDir); err != nil || !info.IsDir() {
 			continue
 		}
@@ -208,14 +122,17 @@ func cleanupLogFiles() {
 
 			return nil
 		}); err != nil {
-			fmt.Println()
-			log.WithError(err).Error("Could not clean up logs")
+			fmt.Println() //nolint:forbidigo
+			logrus.WithError(err).Error("cleaning up logs")
 		}
 	}
 }
 
 func doCheck(url string, match *regexp.Regexp, responseBody io.Writer) *checkResult {
-	req, _ := http.NewRequest("GET", url, nil)
+	ctx, cancel := context.WithTimeout(context.Background(), cfg.Timeout)
+	defer cancel()
+
+	req, _ := http.NewRequestWithContext(ctx, http.MethodGet, url, nil)
 
 	respStart := time.Now()
 	resp, err := http.DefaultClient.Do(req)
@@ -227,7 +144,11 @@ func doCheck(url string, match *regexp.Regexp, responseBody io.Writer) *checkRes
 			respDuration,
 		)
 	}
-	defer resp.Body.Close()
+	defer func() {
+		if err := resp.Body.Close(); err != nil {
+			logrus.WithError(err).Error("closing response body (leaked fd)")
+		}
+	}()
 
 	body := new(bytes.Buffer)
 	if _, err = io.Copy(body, resp.Body); err != nil {
@@ -246,8 +167,8 @@ func doCheck(url string, match *regexp.Regexp, responseBody io.Writer) *checkRes
 				respDuration,
 			)
 		}
-		fmt.Fprintln(responseBody)
-		if _, err = responseBody.Write(body.Bytes()); err != nil {
+
+		if _, err = responseBody.Write(append([]byte{'\n'}, body.Bytes()...)); err != nil {
 			return newCheckResult(
 				statusFailed,
 				"Was not able to copy body",
@@ -284,17 +205,23 @@ func dumpRequest(body io.Reader) (string, error) {
 		return "", nil
 	}
 
-	if err := os.MkdirAll(cfg.LogDir, 0755); err != nil {
-		return "", err
+	if err := os.MkdirAll(cfg.LogDir, logFolderPerms); err != nil {
+		return "", fmt.Errorf("creating log folder: %w", err)
 	}
 
-	f, err := ioutil.TempFile(cfg.LogDir, "resp")
+	f, err := os.CreateTemp(cfg.LogDir, "resp")
 	if err != nil {
-		return "", err
+		return "", fmt.Errorf("creating log file: %w", err)
 	}
-	defer f.Close()
+	defer func() {
+		if err := f.Close(); err != nil {
+			logrus.WithError(err).Error("closing log file (leaked fd)")
+		}
+	}()
 
-	_, err = io.Copy(f, body)
+	if _, err = io.Copy(f, body); err != nil {
+		return "", fmt.Errorf("copying request body: %w", err)
+	}
 
-	return f.Name(), err
+	return f.Name(), nil
 }

types.go

@@ -0,0 +1,127 @@
+package main
+
+import (
+	"bytes"
+	"fmt"
+	"os"
+	"strings"
+	"sync"
+	"text/template"
+	"time"
+
+	"github.com/montanaflynn/stats"
+)
+
+const (
+	dateFormat             = time.RFC1123
+	numHistoricalDurations = 300
+
+	statusTemplateStr = `[{{ .Start.Format "Mon, 02 Jan 2006 15:04:05 MST" }}] ({{ .Status }}) {{ .Message }} ({{ .DurationStats }}){{ if ne .DumpFile "" }} (Resp: {{ .DumpFile }}){{ end }}`
+)
+
+const (
+	statusUnknown checkStatus = iota
+	statusOk
+	statusFailed
+)
+
+type (
+	checkResult struct {
+		DumpFile  string
+		Durations *ringDuration
+		Message   string
+		Start     time.Time
+		Status    checkStatus
+
+		lock        sync.RWMutex
+		lastLineLen int
+	}
+
+	checkStatus uint
+)
+
+var statusTemplate *template.Template
+
+func init() {
+	var err error
+	if statusTemplate, err = template.New("statusTemplate").Parse(statusTemplateStr); err != nil {
+		panic(fmt.Errorf("parsing status template: %w", err))
+	}
+}
+
+func newCheckResult(status checkStatus, message string, duration time.Duration) *checkResult {
+	r := newRingDuration(numHistoricalDurations)
+	r.SetNext(duration)
+
+	return &checkResult{
+		Durations: r,
+		Message:   message,
+		Start:     time.Now(),
+		Status:    status,
+	}
+}
+
+func (c *checkResult) AddDuration(d time.Duration) {
+	c.lock.Lock()
+	defer c.lock.Unlock()
+
+	c.Durations.SetNext(d)
+}
+
+func (c *checkResult) DurationStats() string {
+	c.lock.RLock()
+	defer c.lock.RUnlock()
+
+	var (
+		s             = stats.LoadRawData(c.Durations.GetAll())
+		min, avg, max float64
+		err           error
+	)
+	if min, err = s.Min(); err != nil {
+		min = 0
+	}
+	if avg, err = s.Median(); err != nil {
+		avg = 0
+	}
+	if max, err = s.Max(); err != nil {
+		max = 0
+	}
+
+	return fmt.Sprintf("%s/%s/%s",
+		time.Duration(min).Round(time.Microsecond).String(),
+		time.Duration(avg).Round(time.Microsecond).String(),
+		time.Duration(max).Round(time.Microsecond).String(),
+	)
+}
+
+func (c *checkResult) Equals(r *checkResult) bool {
+	return c.Status == r.Status && c.Message == r.Message
+}
+
+func (c *checkResult) Print() (err error) {
+	buf := new(bytes.Buffer)
+	if err = statusTemplate.Execute(buf, c); err != nil {
+		return fmt.Errorf("executing template: %w", err)
+	}
+
+	if c.lastLineLen > 0 {
+		if _, err = fmt.Fprintf(os.Stdout, "\r%s\r", strings.Repeat(" ", c.lastLineLen)); err != nil {
+			return fmt.Errorf("clearing status: %w", err)
+		}
+	}
+
+	c.lastLineLen = buf.Len()
+	if _, err = buf.WriteTo(os.Stdout); err != nil {
+		return fmt.Errorf("printing status: %w", err)
+	}
+
+	return nil
+}
+
+func (c checkStatus) String() string {
+	return map[checkStatus]string{
+		statusUnknown: "UNKN",
+		statusFailed:  "FAIL",
+		statusOk:      "OKAY",
+	}[c]
+}