ci: add more lints (#7909)

This is a follow-up to #7905 that adds two more linters and fixes the corresponding findings. As per the previous PR, this only flags things that are "obviously" wrong, and fixes the issues found. Signed-off-by: Andrew Dunham <andrew@du.nham.ca> Change-Id: I8739bdb7bc4f75666a7385a7a26d56ec13741b7c
1 year ago · f85dc6f97c
parent 5acc7c4b1e
commit f85dc6f97c
9 changed files with 63 additions and 40 deletions
--- a/.golangci.yml
+++ b/.golangci.yml
@ -3,9 +3,11 @@ linters:
  # enable in the list below.
  disable-all: true
  enable:
+    - bidichk
    - gofmt
    - goimports
    - misspell
+    - revive

 # Configuration for how we run golangci-lint
 run:
@ -23,6 +25,9 @@ issues:

 # Per-linter settings are contained in this top-level key
 linters-settings:
+  # Enable all rules by default; we don't use invisible unicode runes.
+  bidichk:
+
  gofmt:
    rewrite-rules:
      - pattern: 'interface{}'
@ -31,3 +36,26 @@ linters-settings:
  goimports:

  misspell:
+
+  revive:
+    enable-all-rules: false
+    ignore-generated-header: true
+    rules:
+      - name: atomic
+      - name: context-keys-type
+      - name: defer
+        arguments: [[
+          # Calling 'recover' at the time a defer is registered (i.e. "defer recover()") has no effect.
+          "immediate-recover",
+          # Calling 'recover' outside of a deferred function has no effect
+          "recover",
+          # Returning values from a deferred function has no effect
+          "return",
+        ]]
+      - name: duplicated-imports
+      - name: errorf
+      - name: string-of-int
+      - name: time-equal
+      - name: unconditional-recursion
+      - name: useless-break
+      - name: waitgroup-by-value
--- a/cmd/k8s-operator/operator_test.go
+++ b/cmd/k8s-operator/operator_test.go
@ -14,7 +14,6 @@ import (
 	"go.uber.org/zap"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
-	v1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
@ -670,11 +669,11 @@ func expectedSTS(stsName, secretName, hostname string) *appsv1.StatefulSet {
 							},
 						},
 					},
-					Containers: []v1.Container{
+					Containers: []corev1.Container{
 						{
 							Name:  "tailscale",
 							Image: "tailscale/tailscale",
-							Env: []v1.EnvVar{
+							Env: []corev1.EnvVar{
 								{Name: "TS_USERSPACE", Value: "false"},
 								{Name: "TS_AUTH_ONCE", Value: "true"},
 								{Name: "TS_DEST_IP", Value: "10.20.30.40"},
--- a/cmd/tailscale/cli/netcheck.go
+++ b/cmd/tailscale/cli/netcheck.go
@ -97,7 +97,6 @@ func printReport(dm *tailcfg.DERPMap, report *netcheck.Report) error {
 	var err error
 	switch netcheckArgs.format {
 	case "":
-		break
 	case "json":
 		j, err = json.MarshalIndent(report, "", "\t")
 	case "json-line":
--- a/hostinfo/hostinfo.go
+++ b/hostinfo/hostinfo.go
@ -405,7 +405,7 @@ func DisabledEtcAptSource() bool {
 		return false
 	}
 	mod := fi.ModTime()
-	if c, ok := etcAptSrcCache.Load().(etcAptSrcResult); ok && c.mod == mod {
+	if c, ok := etcAptSrcCache.Load().(etcAptSrcResult); ok && c.mod.Equal(mod) {
 		return c.disabled
 	}
 	f, err := os.Open(path)
--- a/ipn/ipnlocal/expiry_test.go
+++ b/ipn/ipnlocal/expiry_test.go
@ -243,7 +243,7 @@ func TestNextPeerExpiry(t *testing.T) {
 			em := newExpiryManager(t.Logf)
 			em.timeNow = func() time.Time { return now }
 			got := em.nextPeerExpiry(tt.netmap, now)
-			if got != tt.want {
+			if !got.Equal(tt.want) {
 				t.Errorf("got %q, want %q", got.Format(time.RFC3339), tt.want.Format(time.RFC3339))
 			} else if !got.IsZero() && got.Before(now) {
 				t.Errorf("unexpectedly got expiry %q before now %q", got.Format(time.RFC3339), now.Format(time.RFC3339))
@ -269,7 +269,7 @@ func TestNextPeerExpiry(t *testing.T) {
 		}
 		got := em.nextPeerExpiry(nm, now)
 		want := now.Add(30 * time.Second)
-		if got != want {
+		if !got.Equal(want) {
 			t.Errorf("got %q, want %q", got.Format(time.RFC3339), want.Format(time.RFC3339))
 		}
 	})
--- a/ipn/ipnlocal/local.go
+++ b/ipn/ipnlocal/local.go
@ -438,7 +438,7 @@ func (b *LocalBackend) SetComponentDebugLogging(component string, until time.Tim
 			// unchanged when the timer actually fires.
 			b.mu.Lock()
 			defer b.mu.Unlock()
-			if ls := b.componentLogUntil[component]; ls.until == until {
+			if ls := b.componentLogUntil[component]; ls.until.Equal(until) {
 				setEnabled(false)
 				b.logf("debugging logging for component %q disabled (by timer)", component)
 			}
--- a/ipn/ipnlocal/serve.go
+++ b/ipn/ipnlocal/serve.go
@ -17,7 +17,6 @@ import (
 	"net/url"
 	"os"
 	"path"
-	pathpkg "path"
 	"strconv"
 	"strings"
 	"sync"
@ -420,19 +419,19 @@ func (b *LocalBackend) getServeHandler(r *http.Request) (_ ipn.HTTPHandlerView,
 	if h, ok := wsc.Handlers().GetOk(r.URL.Path); ok {
 		return h, r.URL.Path, true
 	}
-	path := path.Clean(r.URL.Path)
+	pth := path.Clean(r.URL.Path)
 	for {
-		withSlash := path + "/"
+		withSlash := pth + "/"
 		if h, ok := wsc.Handlers().GetOk(withSlash); ok {
 			return h, withSlash, true
 		}
-		if h, ok := wsc.Handlers().GetOk(path); ok {
-			return h, path, true
+		if h, ok := wsc.Handlers().GetOk(pth); ok {
+			return h, pth, true
 		}
-		if path == "/" {
+		if pth == "/" {
 			return z, "", false
 		}
-		path = pathpkg.Dir(path)
+		pth = path.Dir(pth)
 	}
 }

--- a/net/dns/resolver/tsdns_test.go
+++ b/net/dns/resolver/tsdns_test.go
@ -22,7 +22,6 @@ import (
 	"time"

 	miekdns "github.com/miekg/dns"
-	"golang.org/x/net/dns/dnsmessage"
 	dns "golang.org/x/net/dns/dnsmessage"
 	"tailscale.com/net/netaddr"
 	"tailscale.com/net/tsdial"
@ -1121,15 +1120,15 @@ func TestHandleExitNodeDNSQueryWithNetPkg(t *testing.T) {

 	t.Run("no_such_host", func(t *testing.T) {
 		res, err := handleExitNodeDNSQueryWithNetPkg(context.Background(), t.Logf, backResolver, &response{
-			Header: dnsmessage.Header{
+			Header: dns.Header{
 				ID:       123,
 				Response: true,
 				OpCode:   0, // query
 			},
-			Question: dnsmessage.Question{
-				Name:  dnsmessage.MustNewName("nx-domain.test."),
-				Type:  dnsmessage.TypeA,
-				Class: dnsmessage.ClassINET,
+			Question: dns.Question{
+				Name:  dns.MustNewName("nx-domain.test."),
+				Type:  dns.TypeA,
+				Class: dns.ClassINET,
 			},
 		})
 		if err != nil {
@ -1156,74 +1155,74 @@ func TestHandleExitNodeDNSQueryWithNetPkg(t *testing.T) {
 	}

 	tests := []struct {
-		Type  dnsmessage.Type
+		Type  dns.Type
 		Name  string
 		Check func(t testing.TB, got []byte)
 	}{
 		{
-			Type:  dnsmessage.TypeA,
+			Type:  dns.TypeA,
 			Name:  "one-a.test.",
 			Check: matchPacked("\x00{\x84\x00\x00\x01\x00\x01\x00\x00\x00\x00\x05one-a\x04test\x00\x00\x01\x00\x01\x05one-a\x04test\x00\x00\x01\x00\x01\x00\x00\x02X\x00\x04\x01\x02\x03\x04"),
 		},
 		{
-			Type:  dnsmessage.TypeA,
+			Type:  dns.TypeA,
 			Name:  "two-a.test.",
 			Check: matchPacked("\x00{\x84\x00\x00\x01\x00\x02\x00\x00\x00\x00\x05two-a\x04test\x00\x00\x01\x00\x01\xc0\f\x00\x01\x00\x01\x00\x00\x02X\x00\x04\x01\x02\x03\x04\xc0\f\x00\x01\x00\x01\x00\x00\x02X\x00\x04\x05\x06\a\b"),
 		},
 		{
-			Type:  dnsmessage.TypeAAAA,
+			Type:  dns.TypeAAAA,
 			Name:  "one-aaaa.test.",
 			Check: matchPacked("\x00{\x84\x00\x00\x01\x00\x01\x00\x00\x00\x00\bone-aaaa\x04test\x00\x00\x1c\x00\x01\bone-aaaa\x04test\x00\x00\x1c\x00\x01\x00\x00\x02X\x00\x10\x00\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02"),
 		},
 		{
-			Type:  dnsmessage.TypeAAAA,
+			Type:  dns.TypeAAAA,
 			Name:  "two-aaaa.test.",
 			Check: matchPacked("\x00{\x84\x00\x00\x01\x00\x02\x00\x00\x00\x00\btwo-aaaa\x04test\x00\x00\x1c\x00\x01\xc0\f\x00\x1c\x00\x01\x00\x00\x02X\x00\x10\x00\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\xc0\f\x00\x1c\x00\x01\x00\x00\x02X\x00\x10\x00\x03\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x04"),
 		},
 		{
-			Type:  dnsmessage.TypePTR,
+			Type:  dns.TypePTR,
 			Name:  "4.3.2.1.in-addr.arpa.",
 			Check: matchPacked("\x00{\x84\x00\x00\x01\x00\x01\x00\x00\x00\x00\x014\x013\x012\x011\ain-addr\x04arpa\x00\x00\f\x00\x01\x014\x013\x012\x011\ain-addr\x04arpa\x00\x00\f\x00\x01\x00\x00\x02X\x00\t\x03foo\x03com\x00"),
 		},
 		{
-			Type:  dnsmessage.TypeCNAME,
+			Type:  dns.TypeCNAME,
 			Name:  "cname.test.",
 			Check: matchPacked("\x00{\x84\x00\x00\x01\x00\x01\x00\x00\x00\x00\x05cname\x04test\x00\x00\x05\x00\x01\x05cname\x04test\x00\x00\x05\x00\x01\x00\x00\x02X\x00\x10\nthe-target\x03foo\x00"),
 		},

 		// No records of various types
 		{
-			Type:  dnsmessage.TypeA,
+			Type:  dns.TypeA,
 			Name:  "no-records.test.",
 			Check: matchPacked("\x00{\x84\x03\x00\x01\x00\x00\x00\x00\x00\x00\nno-records\x04test\x00\x00\x01\x00\x01"),
 		},
 		{
-			Type:  dnsmessage.TypeAAAA,
+			Type:  dns.TypeAAAA,
 			Name:  "no-records.test.",
 			Check: matchPacked("\x00{\x84\x03\x00\x01\x00\x00\x00\x00\x00\x00\nno-records\x04test\x00\x00\x1c\x00\x01"),
 		},
 		{
-			Type:  dnsmessage.TypeCNAME,
+			Type:  dns.TypeCNAME,
 			Name:  "no-records.test.",
 			Check: matchPacked("\x00{\x84\x03\x00\x01\x00\x00\x00\x00\x00\x00\nno-records\x04test\x00\x00\x05\x00\x01"),
 		},
 		{
-			Type:  dnsmessage.TypeSRV,
+			Type:  dns.TypeSRV,
 			Name:  "no-records.test.",
 			Check: matchPacked("\x00{\x84\x03\x00\x01\x00\x00\x00\x00\x00\x00\nno-records\x04test\x00\x00!\x00\x01"),
 		},
 		{
-			Type:  dnsmessage.TypeTXT,
+			Type:  dns.TypeTXT,
 			Name:  "txt.test.",
 			Check: matchPacked("\x00{\x84\x00\x00\x01\x00\x03\x00\x00\x00\x00\x03txt\x04test\x00\x00\x10\x00\x01\x03txt\x04test\x00\x00\x10\x00\x01\x00\x00\x02X\x00\t\btxt1=one\x03txt\x04test\x00\x00\x10\x00\x01\x00\x00\x02X\x00\t\btxt2=two\x03txt\x04test\x00\x00\x10\x00\x01\x00\x00\x02X\x00\v\ntxt3=three"),
 		},
 		{
-			Type:  dnsmessage.TypeSRV,
+			Type:  dns.TypeSRV,
 			Name:  "srv.test.",
 			Check: matchPacked("\x00{\x84\x00\x00\x01\x00\x02\x00\x00\x00\x00\x03srv\x04test\x00\x00!\x00\x01\x03srv\x04test\x00\x00!\x00\x01\x00\x00\x02X\x00\x0f\x00\x01\x00\x02\x00\x03\x03foo\x03com\x00\x03srv\x04test\x00\x00!\x00\x01\x00\x00\x02X\x00\x0f\x00\x04\x00\x05\x00\x06\x03bar\x03com\x00"),
 		},
 		{
-			Type:  dnsmessage.TypeNS,
+			Type:  dns.TypeNS,
 			Name:  "ns.test.",
 			Check: matchPacked("\x00{\x84\x00\x00\x01\x00\x02\x00\x00\x00\x00\x02ns\x04test\x00\x00\x02\x00\x01\x02ns\x04test\x00\x00\x02\x00\x01\x00\x00\x02X\x00\t\x03ns1\x03foo\x00\x02ns\x04test\x00\x00\x02\x00\x01\x00\x00\x02X\x00\t\x03ns2\x03bar\x00"),
 		},
@ -1232,15 +1231,15 @@ func TestHandleExitNodeDNSQueryWithNetPkg(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(fmt.Sprintf("%v_%v", tt.Type, strings.Trim(tt.Name, ".")), func(t *testing.T) {
 			got, err := handleExitNodeDNSQueryWithNetPkg(context.Background(), t.Logf, backResolver, &response{
-				Header: dnsmessage.Header{
+				Header: dns.Header{
 					ID:       123,
 					Response: true,
 					OpCode:   0, // query
 				},
-				Question: dnsmessage.Question{
-					Name:  dnsmessage.MustNewName(tt.Name),
+				Question: dns.Question{
+					Name:  dns.MustNewName(tt.Name),
 					Type:  tt.Type,
-					Class: dnsmessage.ClassINET,
+					Class: dns.ClassINET,
 				},
 			})
 			if err != nil {
--- a/wgengine/magicsock/magicsock.go
+++ b/wgengine/magicsock/magicsock.go
@ -3160,7 +3160,6 @@ func (c *Conn) goroutinesRunningLocked() bool {
 	if c.activeDerp != nil {
 		select {
 		case <-c.derpStarted:
-			break
 		default:
 			return true
 		}