From f85dc6f97c3c8cd7ee975153052757d5037911c5 Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Wed, 19 Apr 2023 21:54:19 -0400 Subject: [PATCH] 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 Change-Id: I8739bdb7bc4f75666a7385a7a26d56ec13741b7c --- .golangci.yml | 28 ++++++++++++++++++ cmd/k8s-operator/operator_test.go | 5 ++-- cmd/tailscale/cli/netcheck.go | 1 - hostinfo/hostinfo.go | 2 +- ipn/ipnlocal/expiry_test.go | 4 +-- ipn/ipnlocal/local.go | 2 +- ipn/ipnlocal/serve.go | 13 ++++----- net/dns/resolver/tsdns_test.go | 47 +++++++++++++++---------------- wgengine/magicsock/magicsock.go | 1 - 9 files changed, 63 insertions(+), 40 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index d752fd3ee..1c88a7110 100644 --- 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 diff --git a/cmd/k8s-operator/operator_test.go b/cmd/k8s-operator/operator_test.go index b3206e0dc..25167961c 100644 --- 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"}, diff --git a/cmd/tailscale/cli/netcheck.go b/cmd/tailscale/cli/netcheck.go index a1f79b1fe..0c2f7acd1 100644 --- 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": diff --git a/hostinfo/hostinfo.go b/hostinfo/hostinfo.go index 7994c90fd..9ca8418ef 100644 --- 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) diff --git a/ipn/ipnlocal/expiry_test.go b/ipn/ipnlocal/expiry_test.go index c39bc62bb..32ef44ab2 100644 --- 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)) } }) diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index ddb95d481..07decb04d 100644 --- 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) } diff --git a/ipn/ipnlocal/serve.go b/ipn/ipnlocal/serve.go index 7b49db5ee..a9058dec2 100644 --- 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) } } diff --git a/net/dns/resolver/tsdns_test.go b/net/dns/resolver/tsdns_test.go index 3980cfa33..9862da8fb 100644 --- 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 { diff --git a/wgengine/magicsock/magicsock.go b/wgengine/magicsock/magicsock.go index f315e88b6..d8595e2ba 100644 --- 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 }